[PATCH v3 0/3] SdMmc fixes

[PATCH v3 0/3] SdMmc fixes

[Marcin Wojtas]

Hi,

The third version of the patchset removes
first commit, as the changes in EmmcSwitchToHS200
function ocurred to be unnecessary. In additon to
that improve macro definition in 2/3 patch.

Patches are available in the github:
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/sdmmc-fixes-r20180915

I'm looking forward to the comments and remarks.

Best regards,
Marcin

Changelog:
v2 -> v3
 * rebase on top of the newest master
 * remove changes in EmmcSwitchToHS200 ()
 * 2/3 - use BIT0 in macro

v1 -> v2
 * rebase on top of the newest master
 * resolve conflicts after taking fixes out from new features
 * 3/4 - use macro instead of raw value in SdMmcHcReset

Marcin Wojtas (2):
  MdeModulePkg/SdMmcPciHcDxe: Adjust eMMC clock and bus width sequence
  MdeModulePkg/SdMmcPciHcDxe: Execute card detect only for RemovableSlot

Tomasz Michalec (1):
  MdeModulePkg/SdMmcPciHcDxe: Fix SdMmcHcReset to set only necesery bits

 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   |  5 +++++
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c    |  9 +++++----
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 18 ++++++++++++------
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   |  6 +++---
 4 files changed, 25 insertions(+), 13 deletions(-)

-- 
2.7.4

[PATCH v3 1/3] MdeModulePkg/SdMmcPciHcDxe: Adjust eMMC clock and bus width sequence

[Marcin Wojtas]

According to JESD84-B50-1 chapter A.6 (documentation about eMMC4.5
standard) step "Changing the data bus width" (A.6.3) should be
executed after step "Switching to high-speed mode" (A.6.2).

This patch fixes the bus-width/clock-setting sequence
in EmmcSwitchToHighSpeed ().

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
index c5fd214..12935ef 100755
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
@@ -745,10 +745,6 @@ EmmcSwitchToHighSpeed (
   UINT8               HostCtrl1;
   UINT8               HostCtrl2;
 
-  Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, BusWidth);
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
   //
   // Set to Hight Speed timing
   //
@@ -783,6 +779,11 @@ EmmcSwitchToHighSpeed (
 
   HsTiming = 1;
   Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, ClockFreq);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, BusWidth);
 
   return Status;
 }
-- 
2.7.4

Re: [PATCH v3 1/3] MdeModulePkg/SdMmcPciHcDxe: Adjust eMMC clock and bus width sequence

[Wu, Hao A]

> -----Original Message-----
> From: Marcin Wojtas [mailto:mw@semihalf.com]
> Sent: Sunday, September 16, 2018 6:26 AM
> To: edk2-devel@lists.01.org
> Cc: Tian, Feng; Kinney, Michael D; Gao, Liming; leif.lindholm@linaro.org; Wu,
> Hao A; ard.biesheuvel@linaro.org; nadavh@marvell.com; mw@semihalf.com;
> jsd@semihalf.com; tm@semihalf.com
> Subject: [PATCH v3 1/3] MdeModulePkg/SdMmcPciHcDxe: Adjust eMMC clock
> and bus width sequence
> 
> According to JESD84-B50-1 chapter A.6 (documentation about eMMC4.5
> standard) step "Changing the data bus width" (A.6.3) should be
> executed after step "Switching to high-speed mode" (A.6.2).

Hi,

My understanding to the spec is that the spec seems do not impose a
sequence for 'Switching to high-speed mode' and 'Changing the data bus
width'.

My find is that both operation (A.6.2 & A.6.3 of JESD84-B50-1 ) requires:

* Do these steps after the bus is initialized according to A.6.1, Bus
* initialization.

Also, for HS200 mode selection, the flow chart in Section 6.6.4 shows the
bus width set before bus mode switch. So I think the current code is
taking this as a reference when switching the 'High Speed' mode.

I tried the eMMC device on my side, the current code implementation and
codes after applying your patch both work. So do you met with issues with
certain device with this sequence? If not, I prefer to keep the current
logic.

Best Regards,
Hao Wu

> 
> This patch fixes the bus-width/clock-setting sequence
> in EmmcSwitchToHighSpeed ().
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> index c5fd214..12935ef 100755
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> @@ -745,10 +745,6 @@ EmmcSwitchToHighSpeed (
>    UINT8               HostCtrl1;
>    UINT8               HostCtrl2;
> 
> -  Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, BusWidth);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
>    //
>    // Set to Hight Speed timing
>    //
> @@ -783,6 +779,11 @@ EmmcSwitchToHighSpeed (
> 
>    HsTiming = 1;
>    Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming,
> ClockFreq);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, BusWidth);
> 
>    return Status;
>  }
> --
> 2.7.4

Re: [PATCH v3 1/3] MdeModulePkg/SdMmcPciHcDxe: Adjust eMMC clock and bus width sequence

[Marcin Wojtas]

Hi Hao,

pon., 17 wrz 2018 o 09:18 Wu, Hao A <hao.a.wu@intel.com> napisał(a):
>
> > -----Original Message-----
> > From: Marcin Wojtas [mailto:mw@semihalf.com]
> > Sent: Sunday, September 16, 2018 6:26 AM
> > To: edk2-devel@lists.01.org
> > Cc: Tian, Feng; Kinney, Michael D; Gao, Liming; leif.lindholm@linaro.org; Wu,
> > Hao A; ard.biesheuvel@linaro.org; nadavh@marvell.com; mw@semihalf.com;
> > jsd@semihalf.com; tm@semihalf.com
> > Subject: [PATCH v3 1/3] MdeModulePkg/SdMmcPciHcDxe: Adjust eMMC clock
> > and bus width sequence
> >
> > According to JESD84-B50-1 chapter A.6 (documentation about eMMC4.5
> > standard) step "Changing the data bus width" (A.6.3) should be
> > executed after step "Switching to high-speed mode" (A.6.2).
>
> Hi,
>
> My understanding to the spec is that the spec seems do not impose a
> sequence for 'Switching to high-speed mode' and 'Changing the data bus
> width'.
>
> My find is that both operation (A.6.2 & A.6.3 of JESD84-B50-1 ) requires:
>
> * Do these steps after the bus is initialized according to A.6.1, Bus
> * initialization.
>
> Also, for HS200 mode selection, the flow chart in Section 6.6.4 shows the
> bus width set before bus mode switch. So I think the current code is
> taking this as a reference when switching the 'High Speed' mode.
>
> I tried the eMMC device on my side, the current code implementation and
> codes after applying your patch both work. So do you met with issues with
> certain device with this sequence? If not, I prefer to keep the current
> logic.
>

I re-checked on my boards with unchanged logic - all 3 can work in
High Speed. Patch is pretty old, I can't recall exact circumstances.
I'll drop it in v4.

Best regards,
Marcin

> Best Regards,
> Hao Wu
>
> >
> > This patch fixes the bus-width/clock-setting sequence
> > in EmmcSwitchToHighSpeed ().
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > ---
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > index c5fd214..12935ef 100755
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > @@ -745,10 +745,6 @@ EmmcSwitchToHighSpeed (
> >    UINT8               HostCtrl1;
> >    UINT8               HostCtrl2;
> >
> > -  Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, BusWidth);
> > -  if (EFI_ERROR (Status)) {
> > -    return Status;
> > -  }
> >    //
> >    // Set to Hight Speed timing
> >    //
> > @@ -783,6 +779,11 @@ EmmcSwitchToHighSpeed (
> >
> >    HsTiming = 1;
> >    Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming,
> > ClockFreq);
> > +  if (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> > +
> > +  Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, BusWidth);
> >
> >    return Status;
> >  }
> > --
> > 2.7.4
>

[PATCH v3 2/3] MdeModulePkg/SdMmcPciHcDxe: Fix SdMmcHcReset to set only necesery bits

[Marcin Wojtas]

From: Tomasz Michalec <tm@semihalf.com>

SdMmcHcReset used to set all bits of Software Reset Register to 1
including reserved ones.

Now only first bit is set which means "Software Reset for All".

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h | 5 +++++
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 6 +++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
index e389d52..bcc31bd 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
@@ -63,6 +63,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #define SD_MMC_HC_CTRL_VER            0xFE
 
 //
+// SD Software Reset Register bits description
+//
+#define SD_MMC_HC_SW_RST_ALL          BIT0
+
+//
 // The transfer modes supported by SD Host Controller
 // Simplified Spec 3.0 Table 1-2
 //
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
index 9672b5b..9d9bca8 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -454,8 +454,8 @@ SdMmcHcReset (
   }
 
   PciIo   = Private->PciIo;
-  SwReset = 0xFF;
-  Status  = SdMmcHcRwMmio (PciIo, Slot, SD_MMC_HC_SW_RST, FALSE, sizeof (SwReset), &SwReset);
+  SwReset = SD_MMC_HC_SW_RST_ALL;
+  Status  = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_SW_RST, sizeof (SwReset), &SwReset);
 
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR, "SdMmcHcReset: write full 1 fails: %r\n", Status));
@@ -467,7 +467,7 @@ SdMmcHcReset (
              Slot,
              SD_MMC_HC_SW_RST,
              sizeof (SwReset),
-             0xFF,
+             SD_MMC_HC_SW_RST_ALL,
              0x00,
              SD_MMC_HC_GENERIC_TIMEOUT
              );
-- 
2.7.4

Re: [PATCH v3 2/3] MdeModulePkg/SdMmcPciHcDxe: Fix SdMmcHcReset to set only necesery bits

[Wu, Hao A]

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Marcin Wojtas
> Sent: Sunday, September 16, 2018 6:26 AM
> To: edk2-devel@lists.01.org
> Cc: Tian, Feng; tm@semihalf.com; Wu, Hao A; nadavh@marvell.com; Gao,
> Liming; Kinney, Michael D
> Subject: [edk2] [PATCH v3 2/3] MdeModulePkg/SdMmcPciHcDxe: Fix
> SdMmcHcReset to set only necesery bits
> 
> From: Tomasz Michalec <tm@semihalf.com>
> 
> SdMmcHcReset used to set all bits of Software Reset Register to 1
> including reserved ones.

Hi,

I did a quick search of the SD Host Controller Simplified Specification, I
do not find the spec prohibit setting all the bits when performing a reset
for the host controller.

Do you met with issues during the controller reset with the current logic?

Best Regards,
Hao Wu

> 
> Now only first bit is set which means "Software Reset for All".
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h | 5 +++++
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 6 +++---
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> index e389d52..bcc31bd 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> @@ -63,6 +63,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
>  #define SD_MMC_HC_CTRL_VER            0xFE
> 
>  //
> +// SD Software Reset Register bits description
> +//
> +#define SD_MMC_HC_SW_RST_ALL          BIT0
> +
> +//
>  // The transfer modes supported by SD Host Controller
>  // Simplified Spec 3.0 Table 1-2
>  //
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> index 9672b5b..9d9bca8 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> @@ -454,8 +454,8 @@ SdMmcHcReset (
>    }
> 
>    PciIo   = Private->PciIo;
> -  SwReset = 0xFF;
> -  Status  = SdMmcHcRwMmio (PciIo, Slot, SD_MMC_HC_SW_RST, FALSE, sizeof
> (SwReset), &SwReset);
> +  SwReset = SD_MMC_HC_SW_RST_ALL;
> +  Status  = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_SW_RST, sizeof
> (SwReset), &SwReset);
> 
>    if (EFI_ERROR (Status)) {
>      DEBUG ((DEBUG_ERROR, "SdMmcHcReset: write full 1 fails: %r\n", Status));
> @@ -467,7 +467,7 @@ SdMmcHcReset (
>               Slot,
>               SD_MMC_HC_SW_RST,
>               sizeof (SwReset),
> -             0xFF,
> +             SD_MMC_HC_SW_RST_ALL,
>               0x00,
>               SD_MMC_HC_GENERIC_TIMEOUT
>               );
> --
> 2.7.4
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v3 2/3] MdeModulePkg/SdMmcPciHcDxe: Fix SdMmcHcReset to set only necesery bits

[Marcin Wojtas]

Hi Hao,

pon., 17 wrz 2018 o 09:17 Wu, Hao A <hao.a.wu@intel.com> napisał(a):
>
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Marcin Wojtas
> > Sent: Sunday, September 16, 2018 6:26 AM
> > To: edk2-devel@lists.01.org
> > Cc: Tian, Feng; tm@semihalf.com; Wu, Hao A; nadavh@marvell.com; Gao,
> > Liming; Kinney, Michael D
> > Subject: [edk2] [PATCH v3 2/3] MdeModulePkg/SdMmcPciHcDxe: Fix
> > SdMmcHcReset to set only necesery bits
> >
> > From: Tomasz Michalec <tm@semihalf.com>
> >
> > SdMmcHcReset used to set all bits of Software Reset Register to 1
> > including reserved ones.
>
> Hi,
>
> I did a quick search of the SD Host Controller Simplified Specification, I
> do not find the spec prohibit setting all the bits when performing a reset
> for the host controller.
>
> Do you met with issues during the controller reset with the current logic?
>

Yes, with SwResetMask = 0xFF, on all my boards (regardless speed),
SdMmcHcWaitMmioSet times out on each interface. With
SwReset = SD_MMC_HC_SW_RST_ALL it's all ok.

Best regards,
Marcin

> Best Regards,
> Hao Wu
>
> >
> > Now only first bit is set which means "Software Reset for All".
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > ---
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h | 5 +++++
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 6 +++---
> >  2 files changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > index e389d52..bcc31bd 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > @@ -63,6 +63,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> > KIND, EITHER EXPRESS OR IMPLIED.
> >  #define SD_MMC_HC_CTRL_VER            0xFE
> >
> >  //
> > +// SD Software Reset Register bits description
> > +//
> > +#define SD_MMC_HC_SW_RST_ALL          BIT0
> > +
> > +//
> >  // The transfer modes supported by SD Host Controller
> >  // Simplified Spec 3.0 Table 1-2
> >  //
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > index 9672b5b..9d9bca8 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > @@ -454,8 +454,8 @@ SdMmcHcReset (
> >    }
> >
> >    PciIo   = Private->PciIo;
> > -  SwReset = 0xFF;
> > -  Status  = SdMmcHcRwMmio (PciIo, Slot, SD_MMC_HC_SW_RST, FALSE, sizeof
> > (SwReset), &SwReset);
> > +  SwReset = SD_MMC_HC_SW_RST_ALL;
> > +  Status  = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_SW_RST, sizeof
> > (SwReset), &SwReset);
> >
> >    if (EFI_ERROR (Status)) {
> >      DEBUG ((DEBUG_ERROR, "SdMmcHcReset: write full 1 fails: %r\n", Status));
> > @@ -467,7 +467,7 @@ SdMmcHcReset (
> >               Slot,
> >               SD_MMC_HC_SW_RST,
> >               sizeof (SwReset),
> > -             0xFF,
> > +             SD_MMC_HC_SW_RST_ALL,
> >               0x00,
> >               SD_MMC_HC_GENERIC_TIMEOUT
> >               );
> > --
> > 2.7.4
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v3 2/3] MdeModulePkg/SdMmcPciHcDxe: Fix SdMmcHcReset to set only necesery bits

[Wu, Hao A]

> -----Original Message-----
> From: Marcin Wojtas [mailto:mw@semihalf.com]
> Sent: Monday, September 17, 2018 11:00 PM
> To: Wu, Hao A
> Cc: edk2-devel-01; Tomasz Michalec; nadavh@marvell.com; Gao, Liming;
> Kinney, Michael D
> Subject: Re: [edk2] [PATCH v3 2/3] MdeModulePkg/SdMmcPciHcDxe: Fix
> SdMmcHcReset to set only necesery bits
> 
> Hi Hao,
> 
> pon., 17 wrz 2018 o 09:17 Wu, Hao A <hao.a.wu@intel.com> napisał(a):
> >
> > > -----Original Message-----
> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > > Marcin Wojtas
> > > Sent: Sunday, September 16, 2018 6:26 AM
> > > To: edk2-devel@lists.01.org
> > > Cc: Tian, Feng; tm@semihalf.com; Wu, Hao A; nadavh@marvell.com; Gao,
> > > Liming; Kinney, Michael D
> > > Subject: [edk2] [PATCH v3 2/3] MdeModulePkg/SdMmcPciHcDxe: Fix
> > > SdMmcHcReset to set only necesery bits
> > >
> > > From: Tomasz Michalec <tm@semihalf.com>
> > >
> > > SdMmcHcReset used to set all bits of Software Reset Register to 1
> > > including reserved ones.
> >
> > Hi,
> >
> > I did a quick search of the SD Host Controller Simplified Specification, I
> > do not find the spec prohibit setting all the bits when performing a reset
> > for the host controller.
> >
> > Do you met with issues during the controller reset with the current logic?
> >
> 
> Yes, with SwResetMask = 0xFF, on all my boards (regardless speed),
> SdMmcHcWaitMmioSet times out on each interface. With
> SwReset = SD_MMC_HC_SW_RST_ALL it's all ok.

Thanks Marcin,

Got it. Could you help to file a EDK2 Bugzilla tracker as well? Thanks in
advance.

Also some minor comments below:

> 
> Best regards,
> Marcin
> 
> > Best Regards,
> > Hao Wu
> >
> > >
> > > Now only first bit is set which means "Software Reset for All".
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > > ---
> > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h | 5 +++++
> > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 6 +++---
> > >  2 files changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > > index e389d52..bcc31bd 100644
> > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > > @@ -63,6 +63,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY
> > > KIND, EITHER EXPRESS OR IMPLIED.
> > >  #define SD_MMC_HC_CTRL_VER            0xFE
> > >
> > >  //
> > > +// SD Software Reset Register bits description
> > > +//
> > > +#define SD_MMC_HC_SW_RST_ALL          BIT0

I think you can directly use BIT0 in .c file rather than introducing a new
macro here when performing "Software Reset for All".

> > > +
> > > +//
> > >  // The transfer modes supported by SD Host Controller
> > >  // Simplified Spec 3.0 Table 1-2
> > >  //
> > > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > > index 9672b5b..9d9bca8 100644
> > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > > @@ -454,8 +454,8 @@ SdMmcHcReset (
> > >    }
> > >
> > >    PciIo   = Private->PciIo;
> > > -  SwReset = 0xFF;
> > > -  Status  = SdMmcHcRwMmio (PciIo, Slot, SD_MMC_HC_SW_RST, FALSE,
> sizeof
> > > (SwReset), &SwReset);
> > > +  SwReset = SD_MMC_HC_SW_RST_ALL;
> > > +  Status  = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_SW_RST, sizeof
> > > (SwReset), &SwReset);
> > >
> > >    if (EFI_ERROR (Status)) {
> > >      DEBUG ((DEBUG_ERROR, "SdMmcHcReset: write full 1 fails: %r\n",
> Status));

Could you help to refine the debug message above to reflect the change?

Best Regards,
Hao Wu

> > > @@ -467,7 +467,7 @@ SdMmcHcReset (
> > >               Slot,
> > >               SD_MMC_HC_SW_RST,
> > >               sizeof (SwReset),
> > > -             0xFF,
> > > +             SD_MMC_HC_SW_RST_ALL,
> > >               0x00,
> > >               SD_MMC_HC_GENERIC_TIMEOUT
> > >               );
> > > --
> > > 2.7.4
> > >
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel

[PATCH v3 3/3] MdeModulePkg/SdMmcPciHcDxe: Execute card detect only for RemovableSlot

[Marcin Wojtas]

Some devices can be non removable (such as eMMC) and checking
Present State Register on host controller may falsely return
an information that device is not present. Execute this
check conditionally on the SloType field value.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
index f923930..bf9869d 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
@@ -661,12 +661,18 @@ SdMmcPciHcDriverBindingStart (
     //
     // Check whether there is a SD/MMC card attached
     //
-    Status = SdMmcHcCardDetect (PciIo, Slot, &MediaPresent);
-    if (EFI_ERROR (Status) && (Status != EFI_MEDIA_CHANGED)) {
-      continue;
-    } else if (!MediaPresent) {
-      DEBUG ((DEBUG_INFO, "SdMmcHcCardDetect: No device attached in Slot[%d]!!!\n", Slot));
-      continue;
+    if (Private->Slot[Slot].SlotType == RemovableSlot) {
+      Status = SdMmcHcCardDetect (PciIo, Slot, &MediaPresent);
+      if (EFI_ERROR (Status) && (Status != EFI_MEDIA_CHANGED)) {
+        continue;
+      } else if (!MediaPresent) {
+        DEBUG ((
+          DEBUG_INFO,
+          "SdMmcHcCardDetect: No device attached in Slot[%d]!!!\n",
+          Slot
+          ));
+        continue;
+      }
     }
 
     Status = SdMmcHcInitHost (Private, Slot);
-- 
2.7.4

Re: [PATCH v3 3/3] MdeModulePkg/SdMmcPciHcDxe: Execute card detect only for RemovableSlot

[Wu, Hao A]

Hi,

Could you help to file a EDK2 Bugzilla tracker at:
https://bugzilla.tianocore.org/enter_bug.cgi?product=EDK2

to have a general description of the issue you met? And reference the
tracker within the commit message?
Really appreciate the help, thanks in advance.

With that,
Reviewed-by: Hao Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> -----Original Message-----
> From: Marcin Wojtas [mailto:mw@semihalf.com]
> Sent: Sunday, September 16, 2018 6:26 AM
> To: edk2-devel@lists.01.org
> Cc: Tian, Feng; Kinney, Michael D; Gao, Liming; leif.lindholm@linaro.org; Wu,
> Hao A; ard.biesheuvel@linaro.org; nadavh@marvell.com; mw@semihalf.com;
> jsd@semihalf.com; tm@semihalf.com
> Subject: [PATCH v3 3/3] MdeModulePkg/SdMmcPciHcDxe: Execute card detect
> only for RemovableSlot
> 
> Some devices can be non removable (such as eMMC) and checking
> Present State Register on host controller may falsely return
> an information that device is not present. Execute this
> check conditionally on the SloType field value.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 18
> ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> index f923930..bf9869d 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> @@ -661,12 +661,18 @@ SdMmcPciHcDriverBindingStart (
>      //
>      // Check whether there is a SD/MMC card attached
>      //
> -    Status = SdMmcHcCardDetect (PciIo, Slot, &MediaPresent);
> -    if (EFI_ERROR (Status) && (Status != EFI_MEDIA_CHANGED)) {
> -      continue;
> -    } else if (!MediaPresent) {
> -      DEBUG ((DEBUG_INFO, "SdMmcHcCardDetect: No device attached in
> Slot[%d]!!!\n", Slot));
> -      continue;
> +    if (Private->Slot[Slot].SlotType == RemovableSlot) {
> +      Status = SdMmcHcCardDetect (PciIo, Slot, &MediaPresent);
> +      if (EFI_ERROR (Status) && (Status != EFI_MEDIA_CHANGED)) {
> +        continue;
> +      } else if (!MediaPresent) {
> +        DEBUG ((
> +          DEBUG_INFO,
> +          "SdMmcHcCardDetect: No device attached in Slot[%d]!!!\n",
> +          Slot
> +          ));
> +        continue;
> +      }
>      }
> 
>      Status = SdMmcHcInitHost (Private, Slot);
> --
> 2.7.4

Re: [PATCH v3 3/3] MdeModulePkg/SdMmcPciHcDxe: Execute card detect only for RemovableSlot

[Marcin Wojtas]

Hi Hao,

pon., 17 wrz 2018 o 09:18 Wu, Hao A <hao.a.wu@intel.com> napisał(a):
>
> Hi,
>
> Could you help to file a EDK2 Bugzilla tracker at:
> https://bugzilla.tianocore.org/enter_bug.cgi?product=EDK2
>
> to have a general description of the issue you met? And reference the
> tracker within the commit message?
> Really appreciate the help, thanks in advance.
>
> With that,
> Reviewed-by: Hao Wu <hao.a.wu@intel.com>
>

I created bug and will refer to it in v4:
https://bugzilla.tianocore.org/show_bug.cgi?id=1182

Best regards,
Marcin