diff mbox

[edk2] EmbeddedPkg/MmcDxe: Add non-DDR timing mode support

Message ID 1496909547-16085-1-git-send-email-jun.nie@linaro.org
State New
Headers show

Commit Message

Jun Nie June 8, 2017, 8:12 a.m. UTC 
Only DDR mode is support for 8bit mode currently. Add
non-DDR case when configuring ECSD.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jun Nie <jun.nie@linaro.org>

---
 EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
1.9.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Haojian Zhuang June 8, 2017, 8:39 a.m. UTC | #1 
On 2017/6/8 16:12, Jun Nie wrote:
> Only DDR mode is support for 8bit mode currently. Add

> non-DDR case when configuring ECSD.

> 

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Jun Nie <jun.nie@linaro.org>

> ---

>   EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 5 ++++-

>   1 file changed, 4 insertions(+), 1 deletion(-)

> 

> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c

> index 574a77e..5c0d7e7 100644

> --- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c

> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c

> @@ -286,7 +286,10 @@ InitializeEmmcDevice (

>       }

>       Status = Host->SetIos (Host, BusClockFreq, 8, TimingMode[Idx]);

>       if (!EFI_ERROR (Status)) {

> -      Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);

> +      if (Idx < 2)


It's better to avoid hardcoded value at here. Maybe you can use switch & 
case on TimingMode array at here.

> +        Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);

> +      else

> +        Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_8BIT);

>         if (EFI_ERROR (Status)) {

>           DEBUG ((DEBUG_ERROR, "InitializeEmmcDevice(): Failed to set EXTCSD bus width, Status:%r\n", Status));

>         }

> 


Best Regards
Haojian
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm June 8, 2017, 4:51 p.m. UTC | #2 
Hi Jun,

Don't forget to cc the package maintainers on your patch submission,
as found in top-level Maintainers.txt.

On Thu, Jun 08, 2017 at 04:12:27PM +0800, Jun Nie wrote:
> Only DDR mode is support for 8bit mode currently. Add

> non-DDR case when configuring ECSD.

> 

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Jun Nie <jun.nie@linaro.org>

> ---

>  EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 5 ++++-

>  1 file changed, 4 insertions(+), 1 deletion(-)

> 

> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c

> index 574a77e..5c0d7e7 100644

> --- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c

> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c

> @@ -286,7 +286,10 @@ InitializeEmmcDevice (

>      }

>      Status = Host->SetIos (Host, BusClockFreq, 8, TimingMode[Idx]);

>      if (!EFI_ERROR (Status)) {

> -      Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);

> +      if (Idx < 2)


While currently programatically correct, this would break if anyone
changed the order of entries in TimingMode.

A less fragile test would be
  if (TimingMode[Idx] & (EMMCHS52DDR1V2 | EMMCHS52DDR1V8))

(If we ever need to support HS400 devices in this function, I would
suggest a rewrite without fixed indexes and with a helper macro to
determine DDR-ness.)

Regards,

Leif

> +        Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);

> +      else

> +        Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_8BIT);

>        if (EFI_ERROR (Status)) {

>          DEBUG ((DEBUG_ERROR, "InitializeEmmcDevice(): Failed to set EXTCSD bus width, Status:%r\n", Status));

>        }

> -- 

> 1.9.1

> 

> _______________________________________________

> edk2-devel mailing list

> edk2-devel@lists.01.org

> https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm June 8, 2017, 4:55 p.m. UTC | #3 
On Thu, Jun 08, 2017 at 04:39:44PM +0800, Haojian Zhuang wrote:
> On 2017/6/8 16:12, Jun Nie wrote:

> >Only DDR mode is support for 8bit mode currently. Add

> >non-DDR case when configuring ECSD.

> >

> >Contributed-under: TianoCore Contribution Agreement 1.0

> >Signed-off-by: Jun Nie <jun.nie@linaro.org>

> >---

> >  EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 5 ++++-

> >  1 file changed, 4 insertions(+), 1 deletion(-)

> >

> >diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c

> >index 574a77e..5c0d7e7 100644

> >--- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c

> >+++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c

> >@@ -286,7 +286,10 @@ InitializeEmmcDevice (

> >      }

> >      Status = Host->SetIos (Host, BusClockFreq, 8, TimingMode[Idx]);

> >      if (!EFI_ERROR (Status)) {

> >-      Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);

> >+      if (Idx < 2)

> 

> It's better to avoid hardcoded value at here. Maybe you can use switch &

> case on TimingMode array at here.


Yes, that would also work.

Indeed, if any other possible values than EMMC_BUS_WIDTH_DDR_8BIT or
EMMC_BUS_WIDTH_8BIT could be likely in the future, that would be the
preferable solution.

Regards,

Leif

> >+        Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);

> >+      else

> >+        Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_8BIT);

> >        if (EFI_ERROR (Status)) {

> >          DEBUG ((DEBUG_ERROR, "InitializeEmmcDevice(): Failed to set EXTCSD bus width, Status:%r\n", Status));

> >        }

> >

> 

> Best Regards

> Haojian

> _______________________________________________

> edk2-devel mailing list

> edk2-devel@lists.01.org

> https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Jun Nie June 9, 2017, 3:54 a.m. UTC | #4 
2017-06-09 0:55 GMT+08:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Thu, Jun 08, 2017 at 04:39:44PM +0800, Haojian Zhuang wrote:

>> On 2017/6/8 16:12, Jun Nie wrote:

>> >Only DDR mode is support for 8bit mode currently. Add

>> >non-DDR case when configuring ECSD.

>> >

>> >Contributed-under: TianoCore Contribution Agreement 1.0

>> >Signed-off-by: Jun Nie <jun.nie@linaro.org>

>> >---

>> >  EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c | 5 ++++-

>> >  1 file changed, 4 insertions(+), 1 deletion(-)

>> >

>> >diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c

>> >index 574a77e..5c0d7e7 100644

>> >--- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c

>> >+++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c

>> >@@ -286,7 +286,10 @@ InitializeEmmcDevice (

>> >      }

>> >      Status = Host->SetIos (Host, BusClockFreq, 8, TimingMode[Idx]);

>> >      if (!EFI_ERROR (Status)) {

>> >-      Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);

>> >+      if (Idx < 2)

>>

>> It's better to avoid hardcoded value at here. Maybe you can use switch &

>> case on TimingMode array at here.

>

> Yes, that would also work.

>

> Indeed, if any other possible values than EMMC_BUS_WIDTH_DDR_8BIT or

> EMMC_BUS_WIDTH_8BIT could be likely in the future, that would be the

> preferable solution.

>

> Regards,

>

> Leif



Yes, follow switch/case way is more extendable. Will add it and add
maintainer in next version.

Jun

>

>> >+        Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);

>> >+      else

>> >+        Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_8BIT);

>> >        if (EFI_ERROR (Status)) {

>> >          DEBUG ((DEBUG_ERROR, "InitializeEmmcDevice(): Failed to set EXTCSD bus width, Status:%r\n", Status));

>> >        }

>> >

>>

>> Best Regards

>> Haojian

>> _______________________________________________

>> edk2-devel mailing list

>> edk2-devel@lists.01.org

>> https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox

Patch

diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
index 574a77e..5c0d7e7 100644
--- a/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
+++ b/EmbeddedPkg/Universal/MmcDxe/MmcIdentification.c
@@ -286,7 +286,10 @@  InitializeEmmcDevice (
     }
     Status = Host->SetIos (Host, BusClockFreq, 8, TimingMode[Idx]);
     if (!EFI_ERROR (Status)) {
-      Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);
+      if (Idx < 2)
+        Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_DDR_8BIT);
+      else
+        Status = EmmcSetEXTCSD (MmcHostInstance, EXTCSD_BUS_WIDTH, EMMC_BUS_WIDTH_8BIT);
       if (EFI_ERROR (Status)) {
         DEBUG ((DEBUG_ERROR, "InitializeEmmcDevice(): Failed to set EXTCSD bus width, Status:%r\n", Status));
       }