public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v3 0/4] SdMmcOverride extension
@ 2018-11-08  1:57 Marcin Wojtas
  2018-11-08  1:57 ` [PATCH v3 1/4] MdeModulePkg/SdMmcPciHcDxe: Add an optional parameter in NotifyPhase Marcin Wojtas
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Marcin Wojtas @ 2018-11-08  1:57 UTC (permalink / raw)
  To: edk2-devel
  Cc: leif.lindholm, hao.a.wu, michael.d.kinney, liming.gao,
	ard.biesheuvel, nadavh, mw, jsd, tm, jaz

Hi,

This is the third version of the patchset, which addresses
all remarks that came up during the v2 review, such as
updating the bus mode related enum/macro, headers cleanup,
etc. Details can be found in the changelog below.

Patches are available in the github:
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/sdmmc-override-upstream-r20181108

Please note that extending SdMmcOverride protocol was impacting
so far the only user of it (Synquacer controller). In paralel
edk2-platforms patchset, a patch can be found:
("Silicon/SynQuacer/PlatformDxe: adjust to updated SdMmcOverride")
which immunizes for above and future extensions of the protocol:
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/xenon-upstream-r20181108

I'm looking forward to the comments and remarks.

Best regards,
Marcin

Changelog:
* 1/4:
  - rename new parameter to PhaseData
  - add Ard's RB

* 2/4:
  - s/Controler/Controller/
  - remove all references to MMC_SDR_50 mode
  - rename and reorder MMC bus modes
  - rename enum: s/SD_MMC_UHS_TIMING/SD_MMC_BUS_MODE/
    and move it to protocol header in order to drop including private one
  - fix if condition in EmmcSwitchToHighSpeed
  - call SdMmcHcUhsSignaling unconditionally before SdMmcOverride
    callback, so that protocol producer can optionally modify only quirky
    timing mode values.

*4/4
  - bump protocol version to 2
  - remove redundant assert from SdMmcPciHcDriverBindingStart
    (BaseClkFreq is already checked in SdMmcHcInitClockFreq)
  - update comment in SdMmcHcInitClockFreq
  - restore original DumpCapabilityReg and append 

v1 -> v2
* Rebase onto newest master
* 1/4 [new patch] - preparation for extending NotifyPhase
* 2/4 - UhsSignaling as a part of NotifyPhase
* 3/4 - SwitchClockFreqPost as a part of NotifyPhase
* 4/4 - Allow updating BaseClkFreq via Capability instead of the
        independent callback.

Marcin Wojtas (2):
  MdeModulePkg/SdMmcPciHcDxe: Add an optional parameter in NotifyPhase
  MdeModulePkg/SdMmcPciHcDxe: Allow overriding base clock frequency

Tomasz Michalec (2):
  MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
  MdeModulePkg/SdMmcPciHcDxe: Add SwitchClockFreqPost to SdMmcOverride

 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |   6 +
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   |  40 +++-
 MdeModulePkg/Include/Protocol/SdMmcOverride.h      |  29 ++-
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c    | 200 +++++++++++++++-----
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c      |  53 +++++-
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c |  13 +-
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 101 ++++++++--
 7 files changed, 358 insertions(+), 84 deletions(-)

-- 
2.7.4



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3 1/4] MdeModulePkg/SdMmcPciHcDxe: Add an optional parameter in NotifyPhase
  2018-11-08  1:57 [PATCH v3 0/4] SdMmcOverride extension Marcin Wojtas
@ 2018-11-08  1:57 ` Marcin Wojtas
  2018-11-08  1:57 ` [PATCH v3 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol Marcin Wojtas
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Marcin Wojtas @ 2018-11-08  1:57 UTC (permalink / raw)
  To: edk2-devel
  Cc: leif.lindholm, hao.a.wu, michael.d.kinney, liming.gao,
	ard.biesheuvel, nadavh, mw, jsd, tm, jaz

In order to ensure bigger flexibility in the NotifyPhase
routine of the SdMmcOverride protocol, enable using an
optional phase-specific data. This will allow to exchange
more information between the protocol producer driver
and SdMmcPciHcDxe in the newly added callbacks.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Include/Protocol/SdMmcOverride.h    |  4 +++-
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 12 ++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
index 0766252..8a7669e 100644
--- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
+++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
@@ -63,6 +63,7 @@ EFI_STATUS
   @param[in]      PhaseType             The type of operation and whether the
                                         hook is invoked right before (pre) or
                                         right after (post)
+  @param[in,out]  PhaseData             The pointer to a phase-specific data.
 
   @retval EFI_SUCCESS           The override function completed successfully.
   @retval EFI_NOT_FOUND         The specified controller or slot does not exist.
@@ -74,7 +75,8 @@ EFI_STATUS
 (EFIAPI * EDKII_SD_MMC_NOTIFY_PHASE) (
   IN      EFI_HANDLE                      ControllerHandle,
   IN      UINT8                           Slot,
-  IN      EDKII_SD_MMC_PHASE_TYPE         PhaseType
+  IN      EDKII_SD_MMC_PHASE_TYPE         PhaseType,
+  IN OUT  VOID                           *PhaseData
   );
 
 struct _EDKII_SD_MMC_OVERRIDE {
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
index bedc968..923c55b 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -444,7 +444,8 @@ SdMmcHcReset (
     Status = mOverride->NotifyPhase (
                           Private->ControllerHandle,
                           Slot,
-                          EdkiiSdMmcResetPre);
+                          EdkiiSdMmcResetPre,
+                          NULL);
     if (EFI_ERROR (Status)) {
       DEBUG ((DEBUG_WARN,
         "%a: SD/MMC pre reset notifier callback failed - %r\n",
@@ -494,7 +495,8 @@ SdMmcHcReset (
     Status = mOverride->NotifyPhase (
                           Private->ControllerHandle,
                           Slot,
-                          EdkiiSdMmcResetPost);
+                          EdkiiSdMmcResetPost,
+                          NULL);
     if (EFI_ERROR (Status)) {
       DEBUG ((DEBUG_WARN,
         "%a: SD/MMC post reset notifier callback failed - %r\n",
@@ -1088,7 +1090,8 @@ SdMmcHcInitHost (
     Status = mOverride->NotifyPhase (
                           Private->ControllerHandle,
                           Slot,
-                          EdkiiSdMmcInitHostPre);
+                          EdkiiSdMmcInitHostPre,
+                          NULL);
     if (EFI_ERROR (Status)) {
       DEBUG ((DEBUG_WARN,
         "%a: SD/MMC pre init notifier callback failed - %r\n",
@@ -1123,7 +1126,8 @@ SdMmcHcInitHost (
     Status = mOverride->NotifyPhase (
                           Private->ControllerHandle,
                           Slot,
-                          EdkiiSdMmcInitHostPost);
+                          EdkiiSdMmcInitHostPost,
+                          NULL);
     if (EFI_ERROR (Status)) {
       DEBUG ((DEBUG_WARN,
         "%a: SD/MMC post init notifier callback failed - %r\n",
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
  2018-11-08  1:57 [PATCH v3 0/4] SdMmcOverride extension Marcin Wojtas
  2018-11-08  1:57 ` [PATCH v3 1/4] MdeModulePkg/SdMmcPciHcDxe: Add an optional parameter in NotifyPhase Marcin Wojtas
@ 2018-11-08  1:57 ` Marcin Wojtas
  2018-11-08 11:06   ` Ard Biesheuvel
  2018-11-08  1:57 ` [PATCH v3 3/4] MdeModulePkg/SdMmcPciHcDxe: Add SwitchClockFreqPost to SdMmcOverride Marcin Wojtas
  2018-11-08  1:57 ` [PATCH v3 4/4] MdeModulePkg/SdMmcPciHcDxe: Allow overriding base clock frequency Marcin Wojtas
  3 siblings, 1 reply; 10+ messages in thread
From: Marcin Wojtas @ 2018-11-08  1:57 UTC (permalink / raw)
  To: edk2-devel
  Cc: leif.lindholm, hao.a.wu, michael.d.kinney, liming.gao,
	ard.biesheuvel, nadavh, mw, jsd, tm, jaz

From: Tomasz Michalec <tm@semihalf.com>

Some SD Host Controllers use different values in Host Control 2 Register
to select UHS Mode. This patch adds a new UhsSignaling type routine to
the NotifyPhase of the SdMmcOverride protocol.

UHS signaling configuration is moved to a common, default routine
(SdMmcHcUhsSignaling). After it is executed, the protocol producer
can override the values if needed..

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h |  32 +++++
 MdeModulePkg/Include/Protocol/SdMmcOverride.h    |  17 +++
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c  | 136 +++++++++++++-------
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c    |  31 ++++-
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c |  66 ++++++++++
 5 files changed, 225 insertions(+), 57 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
index 7e3f588..1a11d51 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
@@ -63,6 +63,21 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #define SD_MMC_HC_CTRL_VER            0xFE
 
 //
+// SD Host Controller bits to HOST_CTRL2 register
+//
+#define SD_MMC_HC_CTRL_UHS_MASK       0x0007
+#define SD_MMC_HC_CTRL_UHS_SDR12      0x0000
+#define SD_MMC_HC_CTRL_UHS_SDR25      0x0001
+#define SD_MMC_HC_CTRL_UHS_SDR50      0x0002
+#define SD_MMC_HC_CTRL_UHS_SDR104     0x0003
+#define SD_MMC_HC_CTRL_UHS_DDR50      0x0004
+#define SD_MMC_HC_CTRL_MMC_LEGACY     0x0000
+#define SD_MMC_HC_CTRL_MMC_HS_SDR     0x0001
+#define SD_MMC_HC_CTRL_MMC_HS_DDR     0x0004
+#define SD_MMC_HC_CTRL_MMC_HS200      0x0003
+#define SD_MMC_HC_CTRL_MMC_HS400      0x0005
+
+//
 // The transfer modes supported by SD Host Controller
 // Simplified Spec 3.0 Table 1-2
 //
@@ -518,4 +533,21 @@ SdMmcHcInitTimeoutCtrl (
   IN UINT8                  Slot
   );
 
+/**
+  Set SD Host Controller control 2 registry according to selected speed.
+
+  @param[in] PciIo          The PCI IO protocol instance.
+  @param[in] Slot           The slot number of the SD card to send the command to.
+  @param[in] Timing         The timing to select.
+
+  @retval EFI_SUCCESS       The timing is set successfully.
+  @retval Others            The timing isn't set successfully.
+**/
+EFI_STATUS
+SdMmcHcUhsSignaling (
+  IN EFI_PCI_IO_PROTOCOL    *PciIo,
+  IN UINT8                  Slot,
+  IN SD_MMC_BUS_MODE        Timing
+  );
+
 #endif
diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
index 8a7669e..f948bef 100644
--- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
+++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
@@ -26,11 +26,28 @@
 
 typedef struct _EDKII_SD_MMC_OVERRIDE EDKII_SD_MMC_OVERRIDE;
 
+//
+// Bus timing modes
+//
+typedef enum {
+  SdMmcUhsSdr12,
+  SdMmcUhsSdr25,
+  SdMmcUhsSdr50,
+  SdMmcUhsSdr104,
+  SdMmcUhsDdr50,
+  SdMmcMmcLegacy,
+  SdMmcMmcHsSdr,
+  SdMmcMmcHsDdr,
+  SdMmcMmcHs200,
+  SdMmcMmcHs400,
+} SD_MMC_BUS_MODE;
+
 typedef enum {
   EdkiiSdMmcResetPre,
   EdkiiSdMmcResetPost,
   EdkiiSdMmcInitHostPre,
   EdkiiSdMmcInitHostPost,
+  EdkiiSdMmcUhsSignaling,
 } EDKII_SD_MMC_PHASE_TYPE;
 
 /**
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
index c5fd214..473df8d 100755
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
@@ -740,10 +740,13 @@ EmmcSwitchToHighSpeed (
   IN UINT8                              BusWidth
   )
 {
-  EFI_STATUS          Status;
-  UINT8               HsTiming;
-  UINT8               HostCtrl1;
-  UINT8               HostCtrl2;
+  EFI_STATUS              Status;
+  UINT8                   HsTiming;
+  UINT8                   HostCtrl1;
+  SD_MMC_BUS_MODE         Timing;
+  SD_MMC_HC_PRIVATE_DATA  *Private;
+
+  Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
 
   Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, BusWidth);
   if (EFI_ERROR (Status)) {
@@ -758,29 +761,37 @@ EmmcSwitchToHighSpeed (
     return Status;
   }
 
-  //
-  // Clean UHS Mode Select field of Host Control 2 reigster before update
-  //
-  HostCtrl2 = (UINT8)~0x7;
-  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
-  //
-  // Set UHS Mode Select field of Host Control 2 reigster to SDR12/25/50
-  //
   if (IsDdr) {
-    HostCtrl2 = BIT2;
+    Timing = SdMmcMmcHsDdr;
   } else if (ClockFreq == 52) {
-    HostCtrl2 = BIT0;
+    Timing = SdMmcMmcHsSdr;
   } else {
-    HostCtrl2 = 0;
+    Timing = SdMmcMmcLegacy;
   }
-  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
+
+  Status = SdMmcHcUhsSignaling (PciIo, Slot, Timing);
   if (EFI_ERROR (Status)) {
     return Status;
   }
 
+  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
+    Status = mOverride->NotifyPhase (
+                          Private->ControllerHandle,
+                          Slot,
+                          EdkiiSdMmcUhsSignaling,
+                          &Timing
+                          );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a: SD/MMC uhs signaling notifier callback failed - %r\n",
+        __FUNCTION__,
+        Status
+        ));
+      return Status;
+    }
+  }
+
   HsTiming = 1;
   Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, ClockFreq);
 
@@ -814,10 +825,13 @@ EmmcSwitchToHS200 (
   IN UINT8                              BusWidth
   )
 {
-  EFI_STATUS          Status;
-  UINT8               HsTiming;
-  UINT8               HostCtrl2;
-  UINT16              ClockCtrl;
+  EFI_STATUS               Status;
+  UINT8                    HsTiming;
+  UINT16                   ClockCtrl;
+  SD_MMC_BUS_MODE          Timing;
+  SD_MMC_HC_PRIVATE_DATA  *Private;
+
+  Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
 
   if ((BusWidth != 4) && (BusWidth != 8)) {
     return EFI_INVALID_PARAMETER;
@@ -837,22 +851,32 @@ EmmcSwitchToHS200 (
   if (EFI_ERROR (Status)) {
     return Status;
   }
-  //
-  // Clean UHS Mode Select field of Host Control 2 reigster before update
-  //
-  HostCtrl2 = (UINT8)~0x7;
-  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
+
+  Timing = SdMmcMmcHs200;
+
+  Status = SdMmcHcUhsSignaling (PciIo, Slot, Timing);
   if (EFI_ERROR (Status)) {
     return Status;
   }
-  //
-  // Set UHS Mode Select field of Host Control 2 reigster to SDR104
-  //
-  HostCtrl2 = BIT0 | BIT1;
-  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
-  if (EFI_ERROR (Status)) {
-    return Status;
+
+  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
+    Status = mOverride->NotifyPhase (
+                          Private->ControllerHandle,
+                          Slot,
+                          EdkiiSdMmcUhsSignaling,
+                          &Timing
+                          );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a: SD/MMC uhs signaling notifier callback failed - %r\n",
+        __FUNCTION__,
+        Status
+        ));
+      return Status;
+    }
   }
+
   //
   // Wait Internal Clock Stable in the Clock Control register to be 1 before set SD Clock Enable bit
   //
@@ -910,9 +934,12 @@ EmmcSwitchToHS400 (
   IN UINT32                             ClockFreq
   )
 {
-  EFI_STATUS          Status;
-  UINT8               HsTiming;
-  UINT8               HostCtrl2;
+  EFI_STATUS                 Status;
+  UINT8                      HsTiming;
+  SD_MMC_BUS_MODE            Timing;
+  SD_MMC_HC_PRIVATE_DATA     *Private;
+
+  Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
 
   Status = EmmcSwitchToHS200 (PciIo, PassThru, Slot, Rca, ClockFreq, 8);
   if (EFI_ERROR (Status)) {
@@ -933,21 +960,30 @@ EmmcSwitchToHS400 (
   if (EFI_ERROR (Status)) {
     return Status;
   }
-  //
-  // Clean UHS Mode Select field of Host Control 2 reigster before update
-  //
-  HostCtrl2 = (UINT8)~0x7;
-  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
+
+  Timing = SdMmcMmcHs400;
+
+  Status = SdMmcHcUhsSignaling (PciIo, Slot, Timing);
   if (EFI_ERROR (Status)) {
     return Status;
   }
-  //
-  // Set UHS Mode Select field of Host Control 2 reigster to HS400
-  //
-  HostCtrl2 = BIT0 | BIT2;
-  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
-  if (EFI_ERROR (Status)) {
-    return Status;
+
+  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
+    Status = mOverride->NotifyPhase (
+                          Private->ControllerHandle,
+                          Slot,
+                          EdkiiSdMmcUhsSignaling,
+                          &Timing
+                          );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a: SD/MMC uhs signaling notifier callback failed - %r\n",
+        __FUNCTION__,
+        Status
+        ));
+      return Status;
+    }
   }
 
   HsTiming = 3;
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
index 6ee9ed7..850ad26 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
@@ -784,8 +784,8 @@ SdCardSetBusMode (
   UINT8                        BusWidth;
   UINT8                        AccessMode;
   UINT8                        HostCtrl1;
-  UINT8                        HostCtrl2;
   UINT8                        SwitchResp[64];
+  SD_MMC_BUS_MODE              Timing;
   SD_MMC_HC_PRIVATE_DATA       *Private;
 
   Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
@@ -817,18 +817,23 @@ SdCardSetBusMode (
   if (S18A && (Capability->Sdr104 != 0) && ((SwitchResp[13] & BIT3) != 0)) {
     ClockFreq = 208;
     AccessMode = 3;
+    Timing = SdMmcUhsSdr104;
   } else if (S18A && (Capability->Sdr50 != 0) && ((SwitchResp[13] & BIT2) != 0)) {
     ClockFreq = 100;
     AccessMode = 2;
+    Timing = SdMmcUhsSdr50;
   } else if (S18A && (Capability->Ddr50 != 0) && ((SwitchResp[13] & BIT4) != 0)) {
     ClockFreq = 50;
     AccessMode = 4;
+    Timing = SdMmcUhsDdr50;
   } else if ((SwitchResp[13] & BIT1) != 0) {
     ClockFreq = 50;
     AccessMode = 1;
+    Timing = SdMmcUhsSdr25;
   } else {
     ClockFreq = 25;
     AccessMode = 0;
+    Timing = SdMmcUhsSdr12;
   }
 
   Status = SdCardSwitch (PassThru, Slot, AccessMode, 0xF, 0xF, 0xF, TRUE, SwitchResp);
@@ -854,15 +859,27 @@ SdCardSetBusMode (
     }
   }
 
-  HostCtrl2 = (UINT8)~0x7;
-  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
+  Status = SdMmcHcUhsSignaling (PciIo, Slot, Timing);
   if (EFI_ERROR (Status)) {
     return Status;
   }
-  HostCtrl2 = AccessMode;
-  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
-  if (EFI_ERROR (Status)) {
-    return Status;
+
+  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
+    Status = mOverride->NotifyPhase (
+                          Private->ControllerHandle,
+                          Slot,
+                          EdkiiSdMmcUhsSignaling,
+                          &Timing
+                          );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a: SD/MMC uhs signaling notifier callback failed - %r\n",
+        __FUNCTION__,
+        Status
+        ));
+      return Status;
+    }
   }
 
   Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, *Capability);
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
index 923c55b..85aa625 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -1138,6 +1138,72 @@ SdMmcHcInitHost (
 }
 
 /**
+  Set SD Host Controler control 2 registry according to selected speed.
+
+  @param[in] PciIo          The PCI IO protocol instance.
+  @param[in] Slot           The slot number of the SD card to send the command to.
+  @param[in] Timing         The timing to select.
+
+  @retval EFI_SUCCESS       The timing is set successfully.
+  @retval Others            The timing isn't set successfully.
+**/
+EFI_STATUS
+SdMmcHcUhsSignaling (
+  IN EFI_PCI_IO_PROTOCOL    *PciIo,
+  IN UINT8                  Slot,
+  IN SD_MMC_BUS_MODE        Timing
+  )
+{
+  EFI_STATUS                 Status;
+  UINT8                      HostCtrl2;
+
+  HostCtrl2 = (UINT8)~SD_MMC_HC_CTRL_UHS_MASK;
+  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  switch (Timing) {
+    case SdMmcUhsSdr12:
+      HostCtrl2 = SD_MMC_HC_CTRL_UHS_SDR12;
+      break;
+    case SdMmcUhsSdr25:
+      HostCtrl2 = SD_MMC_HC_CTRL_UHS_SDR25;
+      break;
+    case SdMmcUhsSdr50:
+      HostCtrl2 = SD_MMC_HC_CTRL_UHS_SDR50;
+      break;
+    case SdMmcUhsSdr104:
+      HostCtrl2 = SD_MMC_HC_CTRL_UHS_SDR104;
+      break;
+    case SdMmcUhsDdr50:
+      HostCtrl2 = SD_MMC_HC_CTRL_UHS_DDR50;
+      break;
+    case SdMmcMmcLegacy:
+      HostCtrl2 = SD_MMC_HC_CTRL_MMC_LEGACY;
+      break;
+    case SdMmcMmcHsSdr:
+      HostCtrl2 = SD_MMC_HC_CTRL_MMC_HS_SDR;
+      break;
+    case SdMmcMmcHsDdr:
+      HostCtrl2 = SD_MMC_HC_CTRL_MMC_HS_DDR;
+      break;
+    case SdMmcMmcHs200:
+      HostCtrl2 = SD_MMC_HC_CTRL_MMC_HS200;
+      break;
+    case SdMmcMmcHs400:
+      HostCtrl2 = SD_MMC_HC_CTRL_MMC_HS400;
+      break;
+    default:
+     HostCtrl2 = 0;
+     break;
+  }
+  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
+
+  return Status;
+}
+
+/**
   Turn on/off LED.
 
   @param[in] PciIo          The PCI IO protocol instance.
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 3/4] MdeModulePkg/SdMmcPciHcDxe: Add SwitchClockFreqPost to SdMmcOverride
  2018-11-08  1:57 [PATCH v3 0/4] SdMmcOverride extension Marcin Wojtas
  2018-11-08  1:57 ` [PATCH v3 1/4] MdeModulePkg/SdMmcPciHcDxe: Add an optional parameter in NotifyPhase Marcin Wojtas
  2018-11-08  1:57 ` [PATCH v3 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol Marcin Wojtas
@ 2018-11-08  1:57 ` Marcin Wojtas
  2018-11-08 11:09   ` Ard Biesheuvel
  2018-11-08  1:57 ` [PATCH v3 4/4] MdeModulePkg/SdMmcPciHcDxe: Allow overriding base clock frequency Marcin Wojtas
  3 siblings, 1 reply; 10+ messages in thread
From: Marcin Wojtas @ 2018-11-08  1:57 UTC (permalink / raw)
  To: edk2-devel
  Cc: leif.lindholm, hao.a.wu, michael.d.kinney, liming.gao,
	ard.biesheuvel, nadavh, mw, jsd, tm, jaz

From: Tomasz Michalec <tm@semihalf.com>

Some SD Host Controlers need to do additional opperations after clock
frequency switch.

This patch add new callback type to NotifyPhase of the SdMmcOverride
protocol. It is called after EmmcSwitchClockFreq and SdMmcHcClockSupply.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 MdeModulePkg/Include/Protocol/SdMmcOverride.h   |  1 +
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 60 ++++++++++++++++++++
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c   | 18 ++++++
 3 files changed, 79 insertions(+)

diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
index f948bef..6160b5b 100644
--- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
+++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
@@ -48,6 +48,7 @@ typedef enum {
   EdkiiSdMmcInitHostPre,
   EdkiiSdMmcInitHostPost,
   EdkiiSdMmcUhsSignaling,
+  EdkiiSdMmcSwitchClockFreqPost,
 } EDKII_SD_MMC_PHASE_TYPE;
 
 /**
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
index 473df8d..6fc6871 100755
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
@@ -794,6 +794,27 @@ EmmcSwitchToHighSpeed (
 
   HsTiming = 1;
   Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, ClockFreq);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
+    Status = mOverride->NotifyPhase (
+                          Private->ControllerHandle,
+                          Slot,
+                          EdkiiSdMmcSwitchClockFreqPost,
+                          &Timing
+                          );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
+        __FUNCTION__,
+        Status
+        ));
+      return Status;
+    }
+  }
 
   return Status;
 }
@@ -904,6 +925,24 @@ EmmcSwitchToHS200 (
     return Status;
   }
 
+  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
+    Status = mOverride->NotifyPhase (
+                          Private->ControllerHandle,
+                          Slot,
+                          EdkiiSdMmcSwitchClockFreqPost,
+                          &Timing
+                          );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
+        __FUNCTION__,
+        Status
+        ));
+      return Status;
+    }
+  }
+
   Status = EmmcTuningClkForHs200 (PciIo, PassThru, Slot, BusWidth);
 
   return Status;
@@ -988,6 +1027,27 @@ EmmcSwitchToHS400 (
 
   HsTiming = 3;
   Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, ClockFreq);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
+    Status = mOverride->NotifyPhase (
+                          Private->ControllerHandle,
+                          Slot,
+                          EdkiiSdMmcSwitchClockFreqPost,
+                          &Timing
+                          );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
+        __FUNCTION__,
+        Status
+        ));
+      return Status;
+    }
+  }
 
   return Status;
 }
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
index 850ad26..5408bbc 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
@@ -887,6 +887,24 @@ SdCardSetBusMode (
     return Status;
   }
 
+  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
+    Status = mOverride->NotifyPhase (
+                          Private->ControllerHandle,
+                          Slot,
+                          EdkiiSdMmcSwitchClockFreqPost,
+                          &Timing
+                          );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((
+        DEBUG_ERROR,
+        "%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
+        __FUNCTION__,
+        Status
+        ));
+      return Status;
+    }
+  }
+
   if ((AccessMode == 3) || ((AccessMode == 2) && (Capability->TuningSDR50 != 0))) {
     Status = SdCardTuningClock (PciIo, PassThru, Slot);
     if (EFI_ERROR (Status)) {
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 4/4] MdeModulePkg/SdMmcPciHcDxe: Allow overriding base clock frequency
  2018-11-08  1:57 [PATCH v3 0/4] SdMmcOverride extension Marcin Wojtas
                   ` (2 preceding siblings ...)
  2018-11-08  1:57 ` [PATCH v3 3/4] MdeModulePkg/SdMmcPciHcDxe: Add SwitchClockFreqPost to SdMmcOverride Marcin Wojtas
@ 2018-11-08  1:57 ` Marcin Wojtas
  2018-11-08 11:10   ` Ard Biesheuvel
  3 siblings, 1 reply; 10+ messages in thread
From: Marcin Wojtas @ 2018-11-08  1:57 UTC (permalink / raw)
  To: edk2-devel
  Cc: leif.lindholm, hao.a.wu, michael.d.kinney, liming.gao,
	ard.biesheuvel, nadavh, mw, jsd, tm, jaz

Some SdMmc host controllers are run by clocks with different
frequency than it is reflected in Capabilities Register 1.
It is allowed by SDHCI specification ver. 4.2 - if BaseClkFreq
field value of the Capability Register 1 is zero, the clock
frequency must be obtained via another method.

Because the bitfield is only 8 bits wide, a maximum value
that could be obtained from hardware is 255MHz.
In case the actual frequency exceeds 255MHz, the 8-bit BaseClkFreq
member of SD_MMC_HC_SLOT_CAP structure occurs to be not sufficient
to be used for setting the clock speed in SdMmcHcClockSupply
function.

This patch adds new UINT32 array ('BaseClkFreq[]') to
SD_MMC_HC_PRIVATE_DATA structure for specifying
the input clock speed for each slot of the host controller.
All routines that are used for clock configuration are
updated accordingly.

This patch also adds new IN OUT BaseClockFreq field
in the Capability callback of the SdMmcOverride,
protocol which allows to update BaseClkFreq value.

The patch reuses original commit from edk2-platforms:
20f6f144d3a8 ("Marvell/Drivers: XenonDxe: Allow overriding base clock
frequency")

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |  6 +++++
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   |  8 +++----
 MdeModulePkg/Include/Protocol/SdMmcOverride.h      |  7 ++++--
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c    |  4 ++--
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c      |  4 ++--
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 13 ++++++++++-
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 23 ++++++++++----------
 7 files changed, 43 insertions(+), 22 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
index c683600..8c1a589 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
@@ -118,6 +118,12 @@ typedef struct {
   UINT64                              MaxCurrent[SD_MMC_HC_MAX_SLOT];
 
   UINT32                              ControllerVersion;
+
+  //
+  // Some controllers may require to override base clock frequency
+  // value stored in Capabilities Register 1.
+  //
+  UINT32                              BaseClkFreq[SD_MMC_HC_MAX_SLOT];
 } SD_MMC_HC_PRIVATE_DATA;
 
 #define SD_MMC_HC_TRB_SIG             SIGNATURE_32 ('T', 'R', 'B', 'T')
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
index 1a11d51..8eefc31 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
@@ -423,7 +423,7 @@ SdMmcHcStopClock (
   @param[in] PciIo          The PCI IO protocol instance.
   @param[in] Slot           The slot number of the SD card to send the command to.
   @param[in] ClockFreq      The max clock frequency to be set. The unit is KHz.
-  @param[in] Capability     The capability of the slot.
+  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
 
   @retval EFI_SUCCESS       The clock is supplied successfully.
   @retval Others            The clock isn't supplied successfully.
@@ -434,7 +434,7 @@ SdMmcHcClockSupply (
   IN EFI_PCI_IO_PROTOCOL    *PciIo,
   IN UINT8                  Slot,
   IN UINT64                 ClockFreq,
-  IN SD_MMC_HC_SLOT_CAP     Capability
+  IN UINT32                 BaseClkFreq
   );
 
 /**
@@ -482,7 +482,7 @@ SdMmcHcSetBusWidth (
 
   @param[in] PciIo          The PCI IO protocol instance.
   @param[in] Slot           The slot number of the SD card to send the command to.
-  @param[in] Capability     The capability of the slot.
+  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
 
   @retval EFI_SUCCESS       The clock is supplied successfully.
   @retval Others            The clock isn't supplied successfully.
@@ -492,7 +492,7 @@ EFI_STATUS
 SdMmcHcInitClockFreq (
   IN EFI_PCI_IO_PROTOCOL    *PciIo,
   IN UINT8                  Slot,
-  IN SD_MMC_HC_SLOT_CAP     Capability
+  IN UINT32                 BaseClkFreq
   );
 
 /**
diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
index 6160b5b..0aaf258 100644
--- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
+++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
@@ -22,7 +22,7 @@
 #define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
   { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83, 0x23, 0x23 } }
 
-#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION    0x1
+#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION    0x2
 
 typedef struct _EDKII_SD_MMC_OVERRIDE EDKII_SD_MMC_OVERRIDE;
 
@@ -58,6 +58,8 @@ typedef enum {
   @param[in]      ControllerHandle      The EFI_HANDLE of the controller.
   @param[in]      Slot                  The 0 based slot index.
   @param[in,out]  SdMmcHcSlotCapability The SDHCI capability structure.
+  @param[in,out]  BaseClkFreq           The base clock frequency value that
+                                        optionally can be updated.
 
   @retval EFI_SUCCESS           The override function completed successfully.
   @retval EFI_NOT_FOUND         The specified controller or slot does not exist.
@@ -69,7 +71,8 @@ EFI_STATUS
 (EFIAPI * EDKII_SD_MMC_CAPABILITY) (
   IN      EFI_HANDLE                      ControllerHandle,
   IN      UINT8                           Slot,
-  IN  OUT VOID                            *SdMmcHcSlotCapability
+  IN OUT  VOID                            *SdMmcHcSlotCapability,
+  IN OUT  UINT32                          *BaseClkFreq
   );
 
 /**
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
index 6fc6871..0393fd4 100755
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
@@ -705,7 +705,7 @@ EmmcSwitchClockFreq (
   //
   // Convert the clock freq unit from MHz to KHz.
   //
-  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->Capability[Slot]);
+  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->BaseClkFreq[Slot]);
 
   return Status;
 }
@@ -1098,7 +1098,7 @@ EmmcSetBusMode (
     return Status;
   }
 
-  ASSERT (Private->Capability[Slot].BaseClkFreq != 0);
+  ASSERT (Private->BaseClkFreq[Slot] != 0);
   //
   // Check if the Host Controller support 8bits bus width.
   //
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
index 5408bbc..a9d0d3a 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
@@ -882,7 +882,7 @@ SdCardSetBusMode (
     }
   }
 
-  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, *Capability);
+  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->BaseClkFreq[Slot]);
   if (EFI_ERROR (Status)) {
     return Status;
   }
@@ -1082,7 +1082,7 @@ SdCardIdentification (
         goto Error;
       }
 
-      SdMmcHcInitClockFreq (PciIo, Slot, Private->Capability[Slot]);
+      SdMmcHcInitClockFreq (PciIo, Slot, Private->BaseClkFreq[Slot]);
 
       gBS->Stall (1000);
 
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
index bf9869d..a87f8de 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
@@ -625,11 +625,16 @@ SdMmcPciHcDriverBindingStart (
     if (EFI_ERROR (Status)) {
       continue;
     }
+
+    Private->BaseClkFreq[Slot] = Private->Capability[Slot].BaseClkFreq;
+
     if (mOverride != NULL && mOverride->Capability != NULL) {
       Status = mOverride->Capability (
                             Controller,
                             Slot,
-                            &Private->Capability[Slot]);
+                            &Private->Capability[Slot],
+                            &Private->BaseClkFreq[Slot]
+                            );
       if (EFI_ERROR (Status)) {
         DEBUG ((DEBUG_WARN, "%a: Failed to override capability - %r\n",
           __FUNCTION__, Status));
@@ -637,6 +642,12 @@ SdMmcPciHcDriverBindingStart (
       }
     }
     DumpCapabilityReg (Slot, &Private->Capability[Slot]);
+    DEBUG ((
+      DEBUG_INFO,
+      "Slot[%d] Base Clock Frequency: %dMHz\n",
+      Slot,
+      Private->BaseClkFreq[Slot]
+      ));
 
     Support64BitDma &= Private->Capability[Slot].SysBus64;
 
diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
index 85aa625..040a959 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
@@ -721,7 +721,7 @@ SdMmcHcStopClock (
   @param[in] PciIo          The PCI IO protocol instance.
   @param[in] Slot           The slot number of the SD card to send the command to.
   @param[in] ClockFreq      The max clock frequency to be set. The unit is KHz.
-  @param[in] Capability     The capability of the slot.
+  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
 
   @retval EFI_SUCCESS       The clock is supplied successfully.
   @retval Others            The clock isn't supplied successfully.
@@ -732,11 +732,10 @@ SdMmcHcClockSupply (
   IN EFI_PCI_IO_PROTOCOL    *PciIo,
   IN UINT8                  Slot,
   IN UINT64                 ClockFreq,
-  IN SD_MMC_HC_SLOT_CAP     Capability
+  IN UINT32                 BaseClkFreq
   )
 {
   EFI_STATUS                Status;
-  UINT32                    BaseClkFreq;
   UINT32                    SettingFreq;
   UINT32                    Divisor;
   UINT32                    Remainder;
@@ -746,9 +745,8 @@ SdMmcHcClockSupply (
   //
   // Calculate a divisor for SD clock frequency
   //
-  ASSERT (Capability.BaseClkFreq != 0);
+  ASSERT (BaseClkFreq != 0);
 
-  BaseClkFreq = Capability.BaseClkFreq;
   if (ClockFreq == 0) {
     return EFI_INVALID_PARAMETER;
   }
@@ -940,7 +938,7 @@ SdMmcHcSetBusWidth (
 
   @param[in] PciIo          The PCI IO protocol instance.
   @param[in] Slot           The slot number of the SD card to send the command to.
-  @param[in] Capability     The capability of the slot.
+  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
 
   @retval EFI_SUCCESS       The clock is supplied successfully.
   @retval Others            The clock isn't supplied successfully.
@@ -950,16 +948,19 @@ EFI_STATUS
 SdMmcHcInitClockFreq (
   IN EFI_PCI_IO_PROTOCOL    *PciIo,
   IN UINT8                  Slot,
-  IN SD_MMC_HC_SLOT_CAP     Capability
+  IN UINT32                 BaseClkFreq
   )
 {
   EFI_STATUS                Status;
   UINT32                    InitFreq;
 
   //
-  // Calculate a divisor for SD clock frequency
+  // According to SDHCI specification ver. 4.2, BaseClkFreq field value of
+  // the Capability Register 1 can be zero, which means a need for obtaining
+  // the clock frequency via another method. Fail in case it is not updated
+  // by SW at this point.
   //
-  if (Capability.BaseClkFreq == 0) {
+  if (BaseClkFreq == 0) {
     //
     // Don't support get Base Clock Frequency information via another method
     //
@@ -969,7 +970,7 @@ SdMmcHcInitClockFreq (
   // Supply 400KHz clock frequency at initialization phase.
   //
   InitFreq = 400;
-  Status = SdMmcHcClockSupply (PciIo, Slot, InitFreq, Capability);
+  Status = SdMmcHcClockSupply (PciIo, Slot, InitFreq, BaseClkFreq);
   return Status;
 }
 
@@ -1103,7 +1104,7 @@ SdMmcHcInitHost (
   PciIo = Private->PciIo;
   Capability = Private->Capability[Slot];
 
-  Status = SdMmcHcInitClockFreq (PciIo, Slot, Capability);
+  Status = SdMmcHcInitClockFreq (PciIo, Slot, Private->BaseClkFreq[Slot]);
   if (EFI_ERROR (Status)) {
     return Status;
   }
-- 
2.7.4



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
  2018-11-08  1:57 ` [PATCH v3 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol Marcin Wojtas
@ 2018-11-08 11:06   ` Ard Biesheuvel
  2018-11-08 11:15     ` Marcin Wojtas
  0 siblings, 1 reply; 10+ messages in thread
From: Ard Biesheuvel @ 2018-11-08 11:06 UTC (permalink / raw)
  To: Marcin Wojtas
  Cc: edk2-devel@lists.01.org, Leif Lindholm, Wu, Hao A,
	Kinney, Michael D, Gao, Liming, Nadav Haklai,
	Jan Dąbroś, Tomasz Michalec, Grzegorz Jaszczyk

On 8 November 2018 at 02:57, Marcin Wojtas <mw@semihalf.com> wrote:
> From: Tomasz Michalec <tm@semihalf.com>
>
> Some SD Host Controllers use different values in Host Control 2 Register
> to select UHS Mode. This patch adds a new UhsSignaling type routine to
> the NotifyPhase of the SdMmcOverride protocol.
>
> UHS signaling configuration is moved to a common, default routine
> (SdMmcHcUhsSignaling). After it is executed, the protocol producer
> can override the values if needed..
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h |  32 +++++
>  MdeModulePkg/Include/Protocol/SdMmcOverride.h    |  17 +++
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c  | 136 +++++++++++++-------
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c    |  31 ++++-
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c |  66 ++++++++++
>  5 files changed, 225 insertions(+), 57 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> index 7e3f588..1a11d51 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> @@ -63,6 +63,21 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #define SD_MMC_HC_CTRL_VER            0xFE
>
>  //
> +// SD Host Controller bits to HOST_CTRL2 register
> +//
> +#define SD_MMC_HC_CTRL_UHS_MASK       0x0007
> +#define SD_MMC_HC_CTRL_UHS_SDR12      0x0000
> +#define SD_MMC_HC_CTRL_UHS_SDR25      0x0001
> +#define SD_MMC_HC_CTRL_UHS_SDR50      0x0002
> +#define SD_MMC_HC_CTRL_UHS_SDR104     0x0003
> +#define SD_MMC_HC_CTRL_UHS_DDR50      0x0004
> +#define SD_MMC_HC_CTRL_MMC_LEGACY     0x0000
> +#define SD_MMC_HC_CTRL_MMC_HS_SDR     0x0001
> +#define SD_MMC_HC_CTRL_MMC_HS_DDR     0x0004
> +#define SD_MMC_HC_CTRL_MMC_HS200      0x0003
> +#define SD_MMC_HC_CTRL_MMC_HS400      0x0005
> +
> +//
>  // The transfer modes supported by SD Host Controller
>  // Simplified Spec 3.0 Table 1-2
>  //
> @@ -518,4 +533,21 @@ SdMmcHcInitTimeoutCtrl (
>    IN UINT8                  Slot
>    );
>
> +/**
> +  Set SD Host Controller control 2 registry according to selected speed.
> +
> +  @param[in] PciIo          The PCI IO protocol instance.
> +  @param[in] Slot           The slot number of the SD card to send the command to.
> +  @param[in] Timing         The timing to select.
> +
> +  @retval EFI_SUCCESS       The timing is set successfully.
> +  @retval Others            The timing isn't set successfully.
> +**/
> +EFI_STATUS
> +SdMmcHcUhsSignaling (
> +  IN EFI_PCI_IO_PROTOCOL    *PciIo,
> +  IN UINT8                  Slot,
> +  IN SD_MMC_BUS_MODE        Timing
> +  );
> +
>  #endif
> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> index 8a7669e..f948bef 100644
> --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> @@ -26,11 +26,28 @@
>
>  typedef struct _EDKII_SD_MMC_OVERRIDE EDKII_SD_MMC_OVERRIDE;
>
> +//
> +// Bus timing modes
> +//
> +typedef enum {
> +  SdMmcUhsSdr12,
> +  SdMmcUhsSdr25,
> +  SdMmcUhsSdr50,
> +  SdMmcUhsSdr104,
> +  SdMmcUhsDdr50,
> +  SdMmcMmcLegacy,
> +  SdMmcMmcHsSdr,
> +  SdMmcMmcHsDdr,
> +  SdMmcMmcHs200,
> +  SdMmcMmcHs400,
> +} SD_MMC_BUS_MODE;
> +
>  typedef enum {
>    EdkiiSdMmcResetPre,
>    EdkiiSdMmcResetPost,
>    EdkiiSdMmcInitHostPre,
>    EdkiiSdMmcInitHostPost,
> +  EdkiiSdMmcUhsSignaling,
>  } EDKII_SD_MMC_PHASE_TYPE;
>
>  /**
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> index c5fd214..473df8d 100755
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> @@ -740,10 +740,13 @@ EmmcSwitchToHighSpeed (
>    IN UINT8                              BusWidth
>    )
>  {
> -  EFI_STATUS          Status;
> -  UINT8               HsTiming;
> -  UINT8               HostCtrl1;
> -  UINT8               HostCtrl2;
> +  EFI_STATUS              Status;
> +  UINT8                   HsTiming;
> +  UINT8                   HostCtrl1;
> +  SD_MMC_BUS_MODE         Timing;
> +  SD_MMC_HC_PRIVATE_DATA  *Private;
> +
> +  Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
>
>    Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, BusWidth);
>    if (EFI_ERROR (Status)) {
> @@ -758,29 +761,37 @@ EmmcSwitchToHighSpeed (
>      return Status;
>    }
>
> -  //
> -  // Clean UHS Mode Select field of Host Control 2 reigster before update
> -  //
> -  HostCtrl2 = (UINT8)~0x7;
> -  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
> -  //
> -  // Set UHS Mode Select field of Host Control 2 reigster to SDR12/25/50
> -  //
>    if (IsDdr) {
> -    HostCtrl2 = BIT2;
> +    Timing = SdMmcMmcHsDdr;
>    } else if (ClockFreq == 52) {
> -    HostCtrl2 = BIT0;
> +    Timing = SdMmcMmcHsSdr;
>    } else {
> -    HostCtrl2 = 0;
> +    Timing = SdMmcMmcLegacy;
>    }
> -  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> +
> +  Status = SdMmcHcUhsSignaling (PciIo, Slot, Timing);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
>
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +    Status = mOverride->NotifyPhase (
> +                          Private->ControllerHandle,
> +                          Slot,
> +                          EdkiiSdMmcUhsSignaling,
> +                          &Timing
> +                          );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "%a: SD/MMC uhs signaling notifier callback failed - %r\n",
> +        __FUNCTION__,
> +        Status
> +        ));
> +      return Status;
> +    }
> +  }
> +

Can we move the override handling into SdMmcHcUhsSignaling()? That
way, we no longer have to duplicate it 4 times.

>    HsTiming = 1;
>    Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, ClockFreq);
>
> @@ -814,10 +825,13 @@ EmmcSwitchToHS200 (
>    IN UINT8                              BusWidth
>    )
>  {
> -  EFI_STATUS          Status;
> -  UINT8               HsTiming;
> -  UINT8               HostCtrl2;
> -  UINT16              ClockCtrl;
> +  EFI_STATUS               Status;
> +  UINT8                    HsTiming;
> +  UINT16                   ClockCtrl;
> +  SD_MMC_BUS_MODE          Timing;
> +  SD_MMC_HC_PRIVATE_DATA  *Private;
> +
> +  Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
>
>    if ((BusWidth != 4) && (BusWidth != 8)) {
>      return EFI_INVALID_PARAMETER;
> @@ -837,22 +851,32 @@ EmmcSwitchToHS200 (
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> -  //
> -  // Clean UHS Mode Select field of Host Control 2 reigster before update
> -  //
> -  HostCtrl2 = (UINT8)~0x7;
> -  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> +
> +  Timing = SdMmcMmcHs200;
> +
> +  Status = SdMmcHcUhsSignaling (PciIo, Slot, Timing);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> -  //
> -  // Set UHS Mode Select field of Host Control 2 reigster to SDR104
> -  //
> -  HostCtrl2 = BIT0 | BIT1;
> -  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> +
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +    Status = mOverride->NotifyPhase (
> +                          Private->ControllerHandle,
> +                          Slot,
> +                          EdkiiSdMmcUhsSignaling,
> +                          &Timing
> +                          );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "%a: SD/MMC uhs signaling notifier callback failed - %r\n",
> +        __FUNCTION__,
> +        Status
> +        ));
> +      return Status;
> +    }
>    }
> +
>    //
>    // Wait Internal Clock Stable in the Clock Control register to be 1 before set SD Clock Enable bit
>    //
> @@ -910,9 +934,12 @@ EmmcSwitchToHS400 (
>    IN UINT32                             ClockFreq
>    )
>  {
> -  EFI_STATUS          Status;
> -  UINT8               HsTiming;
> -  UINT8               HostCtrl2;
> +  EFI_STATUS                 Status;
> +  UINT8                      HsTiming;
> +  SD_MMC_BUS_MODE            Timing;
> +  SD_MMC_HC_PRIVATE_DATA     *Private;
> +
> +  Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
>
>    Status = EmmcSwitchToHS200 (PciIo, PassThru, Slot, Rca, ClockFreq, 8);
>    if (EFI_ERROR (Status)) {
> @@ -933,21 +960,30 @@ EmmcSwitchToHS400 (
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> -  //
> -  // Clean UHS Mode Select field of Host Control 2 reigster before update
> -  //
> -  HostCtrl2 = (UINT8)~0x7;
> -  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> +
> +  Timing = SdMmcMmcHs400;
> +
> +  Status = SdMmcHcUhsSignaling (PciIo, Slot, Timing);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> -  //
> -  // Set UHS Mode Select field of Host Control 2 reigster to HS400
> -  //
> -  HostCtrl2 = BIT0 | BIT2;
> -  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> +
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +    Status = mOverride->NotifyPhase (
> +                          Private->ControllerHandle,
> +                          Slot,
> +                          EdkiiSdMmcUhsSignaling,
> +                          &Timing
> +                          );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "%a: SD/MMC uhs signaling notifier callback failed - %r\n",
> +        __FUNCTION__,
> +        Status
> +        ));
> +      return Status;
> +    }
>    }
>
>    HsTiming = 3;
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> index 6ee9ed7..850ad26 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> @@ -784,8 +784,8 @@ SdCardSetBusMode (
>    UINT8                        BusWidth;
>    UINT8                        AccessMode;
>    UINT8                        HostCtrl1;
> -  UINT8                        HostCtrl2;
>    UINT8                        SwitchResp[64];
> +  SD_MMC_BUS_MODE              Timing;
>    SD_MMC_HC_PRIVATE_DATA       *Private;
>
>    Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
> @@ -817,18 +817,23 @@ SdCardSetBusMode (
>    if (S18A && (Capability->Sdr104 != 0) && ((SwitchResp[13] & BIT3) != 0)) {
>      ClockFreq = 208;
>      AccessMode = 3;
> +    Timing = SdMmcUhsSdr104;
>    } else if (S18A && (Capability->Sdr50 != 0) && ((SwitchResp[13] & BIT2) != 0)) {
>      ClockFreq = 100;
>      AccessMode = 2;
> +    Timing = SdMmcUhsSdr50;
>    } else if (S18A && (Capability->Ddr50 != 0) && ((SwitchResp[13] & BIT4) != 0)) {
>      ClockFreq = 50;
>      AccessMode = 4;
> +    Timing = SdMmcUhsDdr50;
>    } else if ((SwitchResp[13] & BIT1) != 0) {
>      ClockFreq = 50;
>      AccessMode = 1;
> +    Timing = SdMmcUhsSdr25;
>    } else {
>      ClockFreq = 25;
>      AccessMode = 0;
> +    Timing = SdMmcUhsSdr12;
>    }
>
>    Status = SdCardSwitch (PassThru, Slot, AccessMode, 0xF, 0xF, 0xF, TRUE, SwitchResp);
> @@ -854,15 +859,27 @@ SdCardSetBusMode (
>      }
>    }
>
> -  HostCtrl2 = (UINT8)~0x7;
> -  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> +  Status = SdMmcHcUhsSignaling (PciIo, Slot, Timing);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> -  HostCtrl2 = AccessMode;
> -  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> +
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +    Status = mOverride->NotifyPhase (
> +                          Private->ControllerHandle,
> +                          Slot,
> +                          EdkiiSdMmcUhsSignaling,
> +                          &Timing
> +                          );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "%a: SD/MMC uhs signaling notifier callback failed - %r\n",
> +        __FUNCTION__,
> +        Status
> +        ));
> +      return Status;
> +    }
>    }
>
>    Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, *Capability);
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> index 923c55b..85aa625 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> @@ -1138,6 +1138,72 @@ SdMmcHcInitHost (
>  }
>
>  /**
> +  Set SD Host Controler control 2 registry according to selected speed.
> +
> +  @param[in] PciIo          The PCI IO protocol instance.
> +  @param[in] Slot           The slot number of the SD card to send the command to.
> +  @param[in] Timing         The timing to select.
> +
> +  @retval EFI_SUCCESS       The timing is set successfully.
> +  @retval Others            The timing isn't set successfully.
> +**/
> +EFI_STATUS
> +SdMmcHcUhsSignaling (
> +  IN EFI_PCI_IO_PROTOCOL    *PciIo,
> +  IN UINT8                  Slot,
> +  IN SD_MMC_BUS_MODE        Timing
> +  )
> +{
> +  EFI_STATUS                 Status;
> +  UINT8                      HostCtrl2;
> +
> +  HostCtrl2 = (UINT8)~SD_MMC_HC_CTRL_UHS_MASK;
> +  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  switch (Timing) {
> +    case SdMmcUhsSdr12:
> +      HostCtrl2 = SD_MMC_HC_CTRL_UHS_SDR12;
> +      break;
> +    case SdMmcUhsSdr25:
> +      HostCtrl2 = SD_MMC_HC_CTRL_UHS_SDR25;
> +      break;
> +    case SdMmcUhsSdr50:
> +      HostCtrl2 = SD_MMC_HC_CTRL_UHS_SDR50;
> +      break;
> +    case SdMmcUhsSdr104:
> +      HostCtrl2 = SD_MMC_HC_CTRL_UHS_SDR104;
> +      break;
> +    case SdMmcUhsDdr50:
> +      HostCtrl2 = SD_MMC_HC_CTRL_UHS_DDR50;
> +      break;
> +    case SdMmcMmcLegacy:
> +      HostCtrl2 = SD_MMC_HC_CTRL_MMC_LEGACY;
> +      break;
> +    case SdMmcMmcHsSdr:
> +      HostCtrl2 = SD_MMC_HC_CTRL_MMC_HS_SDR;
> +      break;
> +    case SdMmcMmcHsDdr:
> +      HostCtrl2 = SD_MMC_HC_CTRL_MMC_HS_DDR;
> +      break;
> +    case SdMmcMmcHs200:
> +      HostCtrl2 = SD_MMC_HC_CTRL_MMC_HS200;
> +      break;
> +    case SdMmcMmcHs400:
> +      HostCtrl2 = SD_MMC_HC_CTRL_MMC_HS400;
> +      break;
> +    default:
> +     HostCtrl2 = 0;
> +     break;
> +  }
> +  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> +
> +  return Status;
> +}
> +
> +/**
>    Turn on/off LED.
>
>    @param[in] PciIo          The PCI IO protocol instance.
> --
> 2.7.4
>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 3/4] MdeModulePkg/SdMmcPciHcDxe: Add SwitchClockFreqPost to SdMmcOverride
  2018-11-08  1:57 ` [PATCH v3 3/4] MdeModulePkg/SdMmcPciHcDxe: Add SwitchClockFreqPost to SdMmcOverride Marcin Wojtas
@ 2018-11-08 11:09   ` Ard Biesheuvel
  2018-11-08 11:18     ` Marcin Wojtas
  0 siblings, 1 reply; 10+ messages in thread
From: Ard Biesheuvel @ 2018-11-08 11:09 UTC (permalink / raw)
  To: Marcin Wojtas
  Cc: edk2-devel@lists.01.org, Leif Lindholm, Wu, Hao A,
	Kinney, Michael D, Gao, Liming, Nadav Haklai,
	Jan Dąbroś, Tomasz Michalec, Grzegorz Jaszczyk

On 8 November 2018 at 02:57, Marcin Wojtas <mw@semihalf.com> wrote:
> From: Tomasz Michalec <tm@semihalf.com>
>
> Some SD Host Controlers need to do additional opperations after clock
> frequency switch.
>
> This patch add new callback type to NotifyPhase of the SdMmcOverride
> protocol. It is called after EmmcSwitchClockFreq and SdMmcHcClockSupply.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  MdeModulePkg/Include/Protocol/SdMmcOverride.h   |  1 +
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 60 ++++++++++++++++++++
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c   | 18 ++++++
>  3 files changed, 79 insertions(+)
>
> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> index f948bef..6160b5b 100644
> --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> @@ -48,6 +48,7 @@ typedef enum {
>    EdkiiSdMmcInitHostPre,
>    EdkiiSdMmcInitHostPost,
>    EdkiiSdMmcUhsSignaling,
> +  EdkiiSdMmcSwitchClockFreqPost,
>  } EDKII_SD_MMC_PHASE_TYPE;
>
>  /**
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> index 473df8d..6fc6871 100755
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> @@ -794,6 +794,27 @@ EmmcSwitchToHighSpeed (
>
>    HsTiming = 1;
>    Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, ClockFreq);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +    Status = mOverride->NotifyPhase (
> +                          Private->ControllerHandle,
> +                          Slot,
> +                          EdkiiSdMmcSwitchClockFreqPost,
> +                          &Timing
> +                          );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
> +        __FUNCTION__,
> +        Status
> +        ));
> +      return Status;
> +    }
> +  }

Is there a way we could move this into EmmcSwitchClockFreq() rather
than duplicate it?

>
>    return Status;
>  }
> @@ -904,6 +925,24 @@ EmmcSwitchToHS200 (
>      return Status;
>    }
>
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +    Status = mOverride->NotifyPhase (
> +                          Private->ControllerHandle,
> +                          Slot,
> +                          EdkiiSdMmcSwitchClockFreqPost,
> +                          &Timing
> +                          );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
> +        __FUNCTION__,
> +        Status
> +        ));
> +      return Status;
> +    }
> +  }
> +
>    Status = EmmcTuningClkForHs200 (PciIo, PassThru, Slot, BusWidth);
>
>    return Status;
> @@ -988,6 +1027,27 @@ EmmcSwitchToHS400 (
>
>    HsTiming = 3;
>    Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, ClockFreq);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +    Status = mOverride->NotifyPhase (
> +                          Private->ControllerHandle,
> +                          Slot,
> +                          EdkiiSdMmcSwitchClockFreqPost,
> +                          &Timing
> +                          );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
> +        __FUNCTION__,
> +        Status
> +        ));
> +      return Status;
> +    }
> +  }
>
>    return Status;
>  }
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> index 850ad26..5408bbc 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> @@ -887,6 +887,24 @@ SdCardSetBusMode (
>      return Status;
>    }
>
> +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> +    Status = mOverride->NotifyPhase (
> +                          Private->ControllerHandle,
> +                          Slot,
> +                          EdkiiSdMmcSwitchClockFreqPost,
> +                          &Timing
> +                          );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
> +        __FUNCTION__,
> +        Status
> +        ));
> +      return Status;
> +    }
> +  }
> +
>    if ((AccessMode == 3) || ((AccessMode == 2) && (Capability->TuningSDR50 != 0))) {
>      Status = SdCardTuningClock (PciIo, PassThru, Slot);
>      if (EFI_ERROR (Status)) {
> --
> 2.7.4
>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 4/4] MdeModulePkg/SdMmcPciHcDxe: Allow overriding base clock frequency
  2018-11-08  1:57 ` [PATCH v3 4/4] MdeModulePkg/SdMmcPciHcDxe: Allow overriding base clock frequency Marcin Wojtas
@ 2018-11-08 11:10   ` Ard Biesheuvel
  0 siblings, 0 replies; 10+ messages in thread
From: Ard Biesheuvel @ 2018-11-08 11:10 UTC (permalink / raw)
  To: Marcin Wojtas
  Cc: edk2-devel@lists.01.org, Leif Lindholm, Wu, Hao A,
	Kinney, Michael D, Gao, Liming, Nadav Haklai,
	Jan Dąbroś, Tomasz Michalec, Grzegorz Jaszczyk

On 8 November 2018 at 02:57, Marcin Wojtas <mw@semihalf.com> wrote:
> Some SdMmc host controllers are run by clocks with different
> frequency than it is reflected in Capabilities Register 1.
> It is allowed by SDHCI specification ver. 4.2 - if BaseClkFreq
> field value of the Capability Register 1 is zero, the clock
> frequency must be obtained via another method.
>
> Because the bitfield is only 8 bits wide, a maximum value
> that could be obtained from hardware is 255MHz.
> In case the actual frequency exceeds 255MHz, the 8-bit BaseClkFreq
> member of SD_MMC_HC_SLOT_CAP structure occurs to be not sufficient
> to be used for setting the clock speed in SdMmcHcClockSupply
> function.
>
> This patch adds new UINT32 array ('BaseClkFreq[]') to
> SD_MMC_HC_PRIVATE_DATA structure for specifying
> the input clock speed for each slot of the host controller.
> All routines that are used for clock configuration are
> updated accordingly.
>
> This patch also adds new IN OUT BaseClockFreq field
> in the Capability callback of the SdMmcOverride,
> protocol which allows to update BaseClkFreq value.
>
> The patch reuses original commit from edk2-platforms:
> 20f6f144d3a8 ("Marvell/Drivers: XenonDxe: Allow overriding base clock
> frequency")
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |  6 +++++
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   |  8 +++----
>  MdeModulePkg/Include/Protocol/SdMmcOverride.h      |  7 ++++--
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c    |  4 ++--
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c      |  4 ++--
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 13 ++++++++++-
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 23 ++++++++++----------
>  7 files changed, 43 insertions(+), 22 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> index c683600..8c1a589 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> @@ -118,6 +118,12 @@ typedef struct {
>    UINT64                              MaxCurrent[SD_MMC_HC_MAX_SLOT];
>
>    UINT32                              ControllerVersion;
> +
> +  //
> +  // Some controllers may require to override base clock frequency
> +  // value stored in Capabilities Register 1.
> +  //
> +  UINT32                              BaseClkFreq[SD_MMC_HC_MAX_SLOT];
>  } SD_MMC_HC_PRIVATE_DATA;
>
>  #define SD_MMC_HC_TRB_SIG             SIGNATURE_32 ('T', 'R', 'B', 'T')
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> index 1a11d51..8eefc31 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> @@ -423,7 +423,7 @@ SdMmcHcStopClock (
>    @param[in] PciIo          The PCI IO protocol instance.
>    @param[in] Slot           The slot number of the SD card to send the command to.
>    @param[in] ClockFreq      The max clock frequency to be set. The unit is KHz.
> -  @param[in] Capability     The capability of the slot.
> +  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
>
>    @retval EFI_SUCCESS       The clock is supplied successfully.
>    @retval Others            The clock isn't supplied successfully.
> @@ -434,7 +434,7 @@ SdMmcHcClockSupply (
>    IN EFI_PCI_IO_PROTOCOL    *PciIo,
>    IN UINT8                  Slot,
>    IN UINT64                 ClockFreq,
> -  IN SD_MMC_HC_SLOT_CAP     Capability
> +  IN UINT32                 BaseClkFreq
>    );
>
>  /**
> @@ -482,7 +482,7 @@ SdMmcHcSetBusWidth (
>
>    @param[in] PciIo          The PCI IO protocol instance.
>    @param[in] Slot           The slot number of the SD card to send the command to.
> -  @param[in] Capability     The capability of the slot.
> +  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
>
>    @retval EFI_SUCCESS       The clock is supplied successfully.
>    @retval Others            The clock isn't supplied successfully.
> @@ -492,7 +492,7 @@ EFI_STATUS
>  SdMmcHcInitClockFreq (
>    IN EFI_PCI_IO_PROTOCOL    *PciIo,
>    IN UINT8                  Slot,
> -  IN SD_MMC_HC_SLOT_CAP     Capability
> +  IN UINT32                 BaseClkFreq
>    );
>
>  /**
> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> index 6160b5b..0aaf258 100644
> --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> @@ -22,7 +22,7 @@
>  #define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
>    { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83, 0x23, 0x23 } }
>
> -#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION    0x1
> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION    0x2
>
>  typedef struct _EDKII_SD_MMC_OVERRIDE EDKII_SD_MMC_OVERRIDE;
>
> @@ -58,6 +58,8 @@ typedef enum {
>    @param[in]      ControllerHandle      The EFI_HANDLE of the controller.
>    @param[in]      Slot                  The 0 based slot index.
>    @param[in,out]  SdMmcHcSlotCapability The SDHCI capability structure.
> +  @param[in,out]  BaseClkFreq           The base clock frequency value that
> +                                        optionally can be updated.
>
>    @retval EFI_SUCCESS           The override function completed successfully.
>    @retval EFI_NOT_FOUND         The specified controller or slot does not exist.
> @@ -69,7 +71,8 @@ EFI_STATUS
>  (EFIAPI * EDKII_SD_MMC_CAPABILITY) (
>    IN      EFI_HANDLE                      ControllerHandle,
>    IN      UINT8                           Slot,
> -  IN  OUT VOID                            *SdMmcHcSlotCapability
> +  IN OUT  VOID                            *SdMmcHcSlotCapability,
> +  IN OUT  UINT32                          *BaseClkFreq
>    );
>
>  /**
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> index 6fc6871..0393fd4 100755
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> @@ -705,7 +705,7 @@ EmmcSwitchClockFreq (
>    //
>    // Convert the clock freq unit from MHz to KHz.
>    //
> -  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->Capability[Slot]);
> +  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->BaseClkFreq[Slot]);
>
>    return Status;
>  }
> @@ -1098,7 +1098,7 @@ EmmcSetBusMode (
>      return Status;
>    }
>
> -  ASSERT (Private->Capability[Slot].BaseClkFreq != 0);
> +  ASSERT (Private->BaseClkFreq[Slot] != 0);
>    //
>    // Check if the Host Controller support 8bits bus width.
>    //
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> index 5408bbc..a9d0d3a 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> @@ -882,7 +882,7 @@ SdCardSetBusMode (
>      }
>    }
>
> -  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, *Capability);
> +  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private->BaseClkFreq[Slot]);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> @@ -1082,7 +1082,7 @@ SdCardIdentification (
>          goto Error;
>        }
>
> -      SdMmcHcInitClockFreq (PciIo, Slot, Private->Capability[Slot]);
> +      SdMmcHcInitClockFreq (PciIo, Slot, Private->BaseClkFreq[Slot]);
>
>        gBS->Stall (1000);
>
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> index bf9869d..a87f8de 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> @@ -625,11 +625,16 @@ SdMmcPciHcDriverBindingStart (
>      if (EFI_ERROR (Status)) {
>        continue;
>      }
> +
> +    Private->BaseClkFreq[Slot] = Private->Capability[Slot].BaseClkFreq;
> +
>      if (mOverride != NULL && mOverride->Capability != NULL) {
>        Status = mOverride->Capability (
>                              Controller,
>                              Slot,
> -                            &Private->Capability[Slot]);
> +                            &Private->Capability[Slot],
> +                            &Private->BaseClkFreq[Slot]
> +                            );
>        if (EFI_ERROR (Status)) {
>          DEBUG ((DEBUG_WARN, "%a: Failed to override capability - %r\n",
>            __FUNCTION__, Status));
> @@ -637,6 +642,12 @@ SdMmcPciHcDriverBindingStart (
>        }
>      }
>      DumpCapabilityReg (Slot, &Private->Capability[Slot]);
> +    DEBUG ((
> +      DEBUG_INFO,
> +      "Slot[%d] Base Clock Frequency: %dMHz\n",
> +      Slot,
> +      Private->BaseClkFreq[Slot]
> +      ));
>
>      Support64BitDma &= Private->Capability[Slot].SysBus64;
>
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> index 85aa625..040a959 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> @@ -721,7 +721,7 @@ SdMmcHcStopClock (
>    @param[in] PciIo          The PCI IO protocol instance.
>    @param[in] Slot           The slot number of the SD card to send the command to.
>    @param[in] ClockFreq      The max clock frequency to be set. The unit is KHz.
> -  @param[in] Capability     The capability of the slot.
> +  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
>
>    @retval EFI_SUCCESS       The clock is supplied successfully.
>    @retval Others            The clock isn't supplied successfully.
> @@ -732,11 +732,10 @@ SdMmcHcClockSupply (
>    IN EFI_PCI_IO_PROTOCOL    *PciIo,
>    IN UINT8                  Slot,
>    IN UINT64                 ClockFreq,
> -  IN SD_MMC_HC_SLOT_CAP     Capability
> +  IN UINT32                 BaseClkFreq
>    )
>  {
>    EFI_STATUS                Status;
> -  UINT32                    BaseClkFreq;
>    UINT32                    SettingFreq;
>    UINT32                    Divisor;
>    UINT32                    Remainder;
> @@ -746,9 +745,8 @@ SdMmcHcClockSupply (
>    //
>    // Calculate a divisor for SD clock frequency
>    //
> -  ASSERT (Capability.BaseClkFreq != 0);
> +  ASSERT (BaseClkFreq != 0);
>
> -  BaseClkFreq = Capability.BaseClkFreq;
>    if (ClockFreq == 0) {
>      return EFI_INVALID_PARAMETER;
>    }
> @@ -940,7 +938,7 @@ SdMmcHcSetBusWidth (
>
>    @param[in] PciIo          The PCI IO protocol instance.
>    @param[in] Slot           The slot number of the SD card to send the command to.
> -  @param[in] Capability     The capability of the slot.
> +  @param[in] BaseClkFreq    The base clock frequency of host controller in MHz.
>
>    @retval EFI_SUCCESS       The clock is supplied successfully.
>    @retval Others            The clock isn't supplied successfully.
> @@ -950,16 +948,19 @@ EFI_STATUS
>  SdMmcHcInitClockFreq (
>    IN EFI_PCI_IO_PROTOCOL    *PciIo,
>    IN UINT8                  Slot,
> -  IN SD_MMC_HC_SLOT_CAP     Capability
> +  IN UINT32                 BaseClkFreq
>    )
>  {
>    EFI_STATUS                Status;
>    UINT32                    InitFreq;
>
>    //
> -  // Calculate a divisor for SD clock frequency
> +  // According to SDHCI specification ver. 4.2, BaseClkFreq field value of
> +  // the Capability Register 1 can be zero, which means a need for obtaining
> +  // the clock frequency via another method. Fail in case it is not updated
> +  // by SW at this point.
>    //
> -  if (Capability.BaseClkFreq == 0) {
> +  if (BaseClkFreq == 0) {
>      //
>      // Don't support get Base Clock Frequency information via another method
>      //
> @@ -969,7 +970,7 @@ SdMmcHcInitClockFreq (
>    // Supply 400KHz clock frequency at initialization phase.
>    //
>    InitFreq = 400;
> -  Status = SdMmcHcClockSupply (PciIo, Slot, InitFreq, Capability);
> +  Status = SdMmcHcClockSupply (PciIo, Slot, InitFreq, BaseClkFreq);
>    return Status;
>  }
>
> @@ -1103,7 +1104,7 @@ SdMmcHcInitHost (
>    PciIo = Private->PciIo;
>    Capability = Private->Capability[Slot];
>
> -  Status = SdMmcHcInitClockFreq (PciIo, Slot, Capability);
> +  Status = SdMmcHcInitClockFreq (PciIo, Slot, Private->BaseClkFreq[Slot]);
>    if (EFI_ERROR (Status)) {
>      return Status;
>    }
> --
> 2.7.4
>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol
  2018-11-08 11:06   ` Ard Biesheuvel
@ 2018-11-08 11:15     ` Marcin Wojtas
  0 siblings, 0 replies; 10+ messages in thread
From: Marcin Wojtas @ 2018-11-08 11:15 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: edk2-devel-01, Leif Lindholm, hao.a.wu, Kinney, Michael D,
	Gao, Liming, nadavh, jsd@semihalf.com, Tomasz Michalec,
	Grzegorz Jaszczyk

Hi Ard,

I'm glad you're back :)

czw., 8 lis 2018 o 12:06 Ard Biesheuvel <ard.biesheuvel@linaro.org> napisał(a):
>
> On 8 November 2018 at 02:57, Marcin Wojtas <mw@semihalf.com> wrote:
> > From: Tomasz Michalec <tm@semihalf.com>
> >
> > Some SD Host Controllers use different values in Host Control 2 Register
> > to select UHS Mode. This patch adds a new UhsSignaling type routine to
> > the NotifyPhase of the SdMmcOverride protocol.
> >
> > UHS signaling configuration is moved to a common, default routine
> > (SdMmcHcUhsSignaling). After it is executed, the protocol producer
> > can override the values if needed..
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > ---
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h |  32 +++++
> >  MdeModulePkg/Include/Protocol/SdMmcOverride.h    |  17 +++
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c  | 136 +++++++++++++-------
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c    |  31 ++++-
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c |  66 ++++++++++
> >  5 files changed, 225 insertions(+), 57 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > index 7e3f588..1a11d51 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > @@ -63,6 +63,21 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> >  #define SD_MMC_HC_CTRL_VER            0xFE
> >
> >  //
> > +// SD Host Controller bits to HOST_CTRL2 register
> > +//
> > +#define SD_MMC_HC_CTRL_UHS_MASK       0x0007
> > +#define SD_MMC_HC_CTRL_UHS_SDR12      0x0000
> > +#define SD_MMC_HC_CTRL_UHS_SDR25      0x0001
> > +#define SD_MMC_HC_CTRL_UHS_SDR50      0x0002
> > +#define SD_MMC_HC_CTRL_UHS_SDR104     0x0003
> > +#define SD_MMC_HC_CTRL_UHS_DDR50      0x0004
> > +#define SD_MMC_HC_CTRL_MMC_LEGACY     0x0000
> > +#define SD_MMC_HC_CTRL_MMC_HS_SDR     0x0001
> > +#define SD_MMC_HC_CTRL_MMC_HS_DDR     0x0004
> > +#define SD_MMC_HC_CTRL_MMC_HS200      0x0003
> > +#define SD_MMC_HC_CTRL_MMC_HS400      0x0005
> > +
> > +//
> >  // The transfer modes supported by SD Host Controller
> >  // Simplified Spec 3.0 Table 1-2
> >  //
> > @@ -518,4 +533,21 @@ SdMmcHcInitTimeoutCtrl (
> >    IN UINT8                  Slot
> >    );
> >
> > +/**
> > +  Set SD Host Controller control 2 registry according to selected speed.
> > +
> > +  @param[in] PciIo          The PCI IO protocol instance.
> > +  @param[in] Slot           The slot number of the SD card to send the command to.
> > +  @param[in] Timing         The timing to select.
> > +
> > +  @retval EFI_SUCCESS       The timing is set successfully.
> > +  @retval Others            The timing isn't set successfully.
> > +**/
> > +EFI_STATUS
> > +SdMmcHcUhsSignaling (
> > +  IN EFI_PCI_IO_PROTOCOL    *PciIo,
> > +  IN UINT8                  Slot,
> > +  IN SD_MMC_BUS_MODE        Timing
> > +  );
> > +
> >  #endif
> > diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > index 8a7669e..f948bef 100644
> > --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > @@ -26,11 +26,28 @@
> >
> >  typedef struct _EDKII_SD_MMC_OVERRIDE EDKII_SD_MMC_OVERRIDE;
> >
> > +//
> > +// Bus timing modes
> > +//
> > +typedef enum {
> > +  SdMmcUhsSdr12,
> > +  SdMmcUhsSdr25,
> > +  SdMmcUhsSdr50,
> > +  SdMmcUhsSdr104,
> > +  SdMmcUhsDdr50,
> > +  SdMmcMmcLegacy,
> > +  SdMmcMmcHsSdr,
> > +  SdMmcMmcHsDdr,
> > +  SdMmcMmcHs200,
> > +  SdMmcMmcHs400,
> > +} SD_MMC_BUS_MODE;
> > +
> >  typedef enum {
> >    EdkiiSdMmcResetPre,
> >    EdkiiSdMmcResetPost,
> >    EdkiiSdMmcInitHostPre,
> >    EdkiiSdMmcInitHostPost,
> > +  EdkiiSdMmcUhsSignaling,
> >  } EDKII_SD_MMC_PHASE_TYPE;
> >
> >  /**
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > index c5fd214..473df8d 100755
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > @@ -740,10 +740,13 @@ EmmcSwitchToHighSpeed (
> >    IN UINT8                              BusWidth
> >    )
> >  {
> > -  EFI_STATUS          Status;
> > -  UINT8               HsTiming;
> > -  UINT8               HostCtrl1;
> > -  UINT8               HostCtrl2;
> > +  EFI_STATUS              Status;
> > +  UINT8                   HsTiming;
> > +  UINT8                   HostCtrl1;
> > +  SD_MMC_BUS_MODE         Timing;
> > +  SD_MMC_HC_PRIVATE_DATA  *Private;
> > +
> > +  Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
> >
> >    Status = EmmcSwitchBusWidth (PciIo, PassThru, Slot, Rca, IsDdr, BusWidth);
> >    if (EFI_ERROR (Status)) {
> > @@ -758,29 +761,37 @@ EmmcSwitchToHighSpeed (
> >      return Status;
> >    }
> >
> > -  //
> > -  // Clean UHS Mode Select field of Host Control 2 reigster before update
> > -  //
> > -  HostCtrl2 = (UINT8)~0x7;
> > -  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> > -  if (EFI_ERROR (Status)) {
> > -    return Status;
> > -  }
> > -  //
> > -  // Set UHS Mode Select field of Host Control 2 reigster to SDR12/25/50
> > -  //
> >    if (IsDdr) {
> > -    HostCtrl2 = BIT2;
> > +    Timing = SdMmcMmcHsDdr;
> >    } else if (ClockFreq == 52) {
> > -    HostCtrl2 = BIT0;
> > +    Timing = SdMmcMmcHsSdr;
> >    } else {
> > -    HostCtrl2 = 0;
> > +    Timing = SdMmcMmcLegacy;
> >    }
> > -  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> > +
> > +  Status = SdMmcHcUhsSignaling (PciIo, Slot, Timing);
> >    if (EFI_ERROR (Status)) {
> >      return Status;
> >    }
> >
> > +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> > +    Status = mOverride->NotifyPhase (
> > +                          Private->ControllerHandle,
> > +                          Slot,
> > +                          EdkiiSdMmcUhsSignaling,
> > +                          &Timing
> > +                          );
> > +    if (EFI_ERROR (Status)) {
> > +      DEBUG ((
> > +        DEBUG_ERROR,
> > +        "%a: SD/MMC uhs signaling notifier callback failed - %r\n",
> > +        __FUNCTION__,
> > +        Status
> > +        ));
> > +      return Status;
> > +    }
> > +  }
> > +
>
> Can we move the override handling into SdMmcHcUhsSignaling()? That
> way, we no longer have to duplicate it 4 times.

Sure! I should've come up on this on my own...

Thanks,
Marcin

>
> >    HsTiming = 1;
> >    Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, ClockFreq);
> >
> > @@ -814,10 +825,13 @@ EmmcSwitchToHS200 (
> >    IN UINT8                              BusWidth
> >    )
> >  {
> > -  EFI_STATUS          Status;
> > -  UINT8               HsTiming;
> > -  UINT8               HostCtrl2;
> > -  UINT16              ClockCtrl;
> > +  EFI_STATUS               Status;
> > +  UINT8                    HsTiming;
> > +  UINT16                   ClockCtrl;
> > +  SD_MMC_BUS_MODE          Timing;
> > +  SD_MMC_HC_PRIVATE_DATA  *Private;
> > +
> > +  Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
> >
> >    if ((BusWidth != 4) && (BusWidth != 8)) {
> >      return EFI_INVALID_PARAMETER;
> > @@ -837,22 +851,32 @@ EmmcSwitchToHS200 (
> >    if (EFI_ERROR (Status)) {
> >      return Status;
> >    }
> > -  //
> > -  // Clean UHS Mode Select field of Host Control 2 reigster before update
> > -  //
> > -  HostCtrl2 = (UINT8)~0x7;
> > -  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> > +
> > +  Timing = SdMmcMmcHs200;
> > +
> > +  Status = SdMmcHcUhsSignaling (PciIo, Slot, Timing);
> >    if (EFI_ERROR (Status)) {
> >      return Status;
> >    }
> > -  //
> > -  // Set UHS Mode Select field of Host Control 2 reigster to SDR104
> > -  //
> > -  HostCtrl2 = BIT0 | BIT1;
> > -  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> > -  if (EFI_ERROR (Status)) {
> > -    return Status;
> > +
> > +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> > +    Status = mOverride->NotifyPhase (
> > +                          Private->ControllerHandle,
> > +                          Slot,
> > +                          EdkiiSdMmcUhsSignaling,
> > +                          &Timing
> > +                          );
> > +    if (EFI_ERROR (Status)) {
> > +      DEBUG ((
> > +        DEBUG_ERROR,
> > +        "%a: SD/MMC uhs signaling notifier callback failed - %r\n",
> > +        __FUNCTION__,
> > +        Status
> > +        ));
> > +      return Status;
> > +    }
> >    }
> > +
> >    //
> >    // Wait Internal Clock Stable in the Clock Control register to be 1 before set SD Clock Enable bit
> >    //
> > @@ -910,9 +934,12 @@ EmmcSwitchToHS400 (
> >    IN UINT32                             ClockFreq
> >    )
> >  {
> > -  EFI_STATUS          Status;
> > -  UINT8               HsTiming;
> > -  UINT8               HostCtrl2;
> > +  EFI_STATUS                 Status;
> > +  UINT8                      HsTiming;
> > +  SD_MMC_BUS_MODE            Timing;
> > +  SD_MMC_HC_PRIVATE_DATA     *Private;
> > +
> > +  Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
> >
> >    Status = EmmcSwitchToHS200 (PciIo, PassThru, Slot, Rca, ClockFreq, 8);
> >    if (EFI_ERROR (Status)) {
> > @@ -933,21 +960,30 @@ EmmcSwitchToHS400 (
> >    if (EFI_ERROR (Status)) {
> >      return Status;
> >    }
> > -  //
> > -  // Clean UHS Mode Select field of Host Control 2 reigster before update
> > -  //
> > -  HostCtrl2 = (UINT8)~0x7;
> > -  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> > +
> > +  Timing = SdMmcMmcHs400;
> > +
> > +  Status = SdMmcHcUhsSignaling (PciIo, Slot, Timing);
> >    if (EFI_ERROR (Status)) {
> >      return Status;
> >    }
> > -  //
> > -  // Set UHS Mode Select field of Host Control 2 reigster to HS400
> > -  //
> > -  HostCtrl2 = BIT0 | BIT2;
> > -  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> > -  if (EFI_ERROR (Status)) {
> > -    return Status;
> > +
> > +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> > +    Status = mOverride->NotifyPhase (
> > +                          Private->ControllerHandle,
> > +                          Slot,
> > +                          EdkiiSdMmcUhsSignaling,
> > +                          &Timing
> > +                          );
> > +    if (EFI_ERROR (Status)) {
> > +      DEBUG ((
> > +        DEBUG_ERROR,
> > +        "%a: SD/MMC uhs signaling notifier callback failed - %r\n",
> > +        __FUNCTION__,
> > +        Status
> > +        ));
> > +      return Status;
> > +    }
> >    }
> >
> >    HsTiming = 3;
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> > index 6ee9ed7..850ad26 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> > @@ -784,8 +784,8 @@ SdCardSetBusMode (
> >    UINT8                        BusWidth;
> >    UINT8                        AccessMode;
> >    UINT8                        HostCtrl1;
> > -  UINT8                        HostCtrl2;
> >    UINT8                        SwitchResp[64];
> > +  SD_MMC_BUS_MODE              Timing;
> >    SD_MMC_HC_PRIVATE_DATA       *Private;
> >
> >    Private = SD_MMC_HC_PRIVATE_FROM_THIS (PassThru);
> > @@ -817,18 +817,23 @@ SdCardSetBusMode (
> >    if (S18A && (Capability->Sdr104 != 0) && ((SwitchResp[13] & BIT3) != 0)) {
> >      ClockFreq = 208;
> >      AccessMode = 3;
> > +    Timing = SdMmcUhsSdr104;
> >    } else if (S18A && (Capability->Sdr50 != 0) && ((SwitchResp[13] & BIT2) != 0)) {
> >      ClockFreq = 100;
> >      AccessMode = 2;
> > +    Timing = SdMmcUhsSdr50;
> >    } else if (S18A && (Capability->Ddr50 != 0) && ((SwitchResp[13] & BIT4) != 0)) {
> >      ClockFreq = 50;
> >      AccessMode = 4;
> > +    Timing = SdMmcUhsDdr50;
> >    } else if ((SwitchResp[13] & BIT1) != 0) {
> >      ClockFreq = 50;
> >      AccessMode = 1;
> > +    Timing = SdMmcUhsSdr25;
> >    } else {
> >      ClockFreq = 25;
> >      AccessMode = 0;
> > +    Timing = SdMmcUhsSdr12;
> >    }
> >
> >    Status = SdCardSwitch (PassThru, Slot, AccessMode, 0xF, 0xF, 0xF, TRUE, SwitchResp);
> > @@ -854,15 +859,27 @@ SdCardSetBusMode (
> >      }
> >    }
> >
> > -  HostCtrl2 = (UINT8)~0x7;
> > -  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> > +  Status = SdMmcHcUhsSignaling (PciIo, Slot, Timing);
> >    if (EFI_ERROR (Status)) {
> >      return Status;
> >    }
> > -  HostCtrl2 = AccessMode;
> > -  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> > -  if (EFI_ERROR (Status)) {
> > -    return Status;
> > +
> > +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> > +    Status = mOverride->NotifyPhase (
> > +                          Private->ControllerHandle,
> > +                          Slot,
> > +                          EdkiiSdMmcUhsSignaling,
> > +                          &Timing
> > +                          );
> > +    if (EFI_ERROR (Status)) {
> > +      DEBUG ((
> > +        DEBUG_ERROR,
> > +        "%a: SD/MMC uhs signaling notifier callback failed - %r\n",
> > +        __FUNCTION__,
> > +        Status
> > +        ));
> > +      return Status;
> > +    }
> >    }
> >
> >    Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, *Capability);
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > index 923c55b..85aa625 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > @@ -1138,6 +1138,72 @@ SdMmcHcInitHost (
> >  }
> >
> >  /**
> > +  Set SD Host Controler control 2 registry according to selected speed.
> > +
> > +  @param[in] PciIo          The PCI IO protocol instance.
> > +  @param[in] Slot           The slot number of the SD card to send the command to.
> > +  @param[in] Timing         The timing to select.
> > +
> > +  @retval EFI_SUCCESS       The timing is set successfully.
> > +  @retval Others            The timing isn't set successfully.
> > +**/
> > +EFI_STATUS
> > +SdMmcHcUhsSignaling (
> > +  IN EFI_PCI_IO_PROTOCOL    *PciIo,
> > +  IN UINT8                  Slot,
> > +  IN SD_MMC_BUS_MODE        Timing
> > +  )
> > +{
> > +  EFI_STATUS                 Status;
> > +  UINT8                      HostCtrl2;
> > +
> > +  HostCtrl2 = (UINT8)~SD_MMC_HC_CTRL_UHS_MASK;
> > +  Status = SdMmcHcAndMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> > +  if (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> > +
> > +  switch (Timing) {
> > +    case SdMmcUhsSdr12:
> > +      HostCtrl2 = SD_MMC_HC_CTRL_UHS_SDR12;
> > +      break;
> > +    case SdMmcUhsSdr25:
> > +      HostCtrl2 = SD_MMC_HC_CTRL_UHS_SDR25;
> > +      break;
> > +    case SdMmcUhsSdr50:
> > +      HostCtrl2 = SD_MMC_HC_CTRL_UHS_SDR50;
> > +      break;
> > +    case SdMmcUhsSdr104:
> > +      HostCtrl2 = SD_MMC_HC_CTRL_UHS_SDR104;
> > +      break;
> > +    case SdMmcUhsDdr50:
> > +      HostCtrl2 = SD_MMC_HC_CTRL_UHS_DDR50;
> > +      break;
> > +    case SdMmcMmcLegacy:
> > +      HostCtrl2 = SD_MMC_HC_CTRL_MMC_LEGACY;
> > +      break;
> > +    case SdMmcMmcHsSdr:
> > +      HostCtrl2 = SD_MMC_HC_CTRL_MMC_HS_SDR;
> > +      break;
> > +    case SdMmcMmcHsDdr:
> > +      HostCtrl2 = SD_MMC_HC_CTRL_MMC_HS_DDR;
> > +      break;
> > +    case SdMmcMmcHs200:
> > +      HostCtrl2 = SD_MMC_HC_CTRL_MMC_HS200;
> > +      break;
> > +    case SdMmcMmcHs400:
> > +      HostCtrl2 = SD_MMC_HC_CTRL_MMC_HS400;
> > +      break;
> > +    default:
> > +     HostCtrl2 = 0;
> > +     break;
> > +  }
> > +  Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL2, sizeof (HostCtrl2), &HostCtrl2);
> > +
> > +  return Status;
> > +}
> > +
> > +/**
> >    Turn on/off LED.
> >
> >    @param[in] PciIo          The PCI IO protocol instance.
> > --
> > 2.7.4
> >


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 3/4] MdeModulePkg/SdMmcPciHcDxe: Add SwitchClockFreqPost to SdMmcOverride
  2018-11-08 11:09   ` Ard Biesheuvel
@ 2018-11-08 11:18     ` Marcin Wojtas
  0 siblings, 0 replies; 10+ messages in thread
From: Marcin Wojtas @ 2018-11-08 11:18 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: edk2-devel-01, Leif Lindholm, hao.a.wu, Kinney, Michael D,
	Gao, Liming, nadavh, jsd@semihalf.com, Tomasz Michalec,
	Grzegorz Jaszczyk

czw., 8 lis 2018 o 12:09 Ard Biesheuvel <ard.biesheuvel@linaro.org> napisał(a):
>
> On 8 November 2018 at 02:57, Marcin Wojtas <mw@semihalf.com> wrote:
> > From: Tomasz Michalec <tm@semihalf.com>
> >
> > Some SD Host Controlers need to do additional opperations after clock
> > frequency switch.
> >
> > This patch add new callback type to NotifyPhase of the SdMmcOverride
> > protocol. It is called after EmmcSwitchClockFreq and SdMmcHcClockSupply.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > ---
> >  MdeModulePkg/Include/Protocol/SdMmcOverride.h   |  1 +
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 60 ++++++++++++++++++++
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c   | 18 ++++++
> >  3 files changed, 79 insertions(+)
> >
> > diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > index f948bef..6160b5b 100644
> > --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > @@ -48,6 +48,7 @@ typedef enum {
> >    EdkiiSdMmcInitHostPre,
> >    EdkiiSdMmcInitHostPost,
> >    EdkiiSdMmcUhsSignaling,
> > +  EdkiiSdMmcSwitchClockFreqPost,
> >  } EDKII_SD_MMC_PHASE_TYPE;
> >
> >  /**
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > index 473df8d..6fc6871 100755
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > @@ -794,6 +794,27 @@ EmmcSwitchToHighSpeed (
> >
> >    HsTiming = 1;
> >    Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, ClockFreq);
> > +  if (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> > +
> > +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> > +    Status = mOverride->NotifyPhase (
> > +                          Private->ControllerHandle,
> > +                          Slot,
> > +                          EdkiiSdMmcSwitchClockFreqPost,
> > +                          &Timing
> > +                          );
> > +    if (EFI_ERROR (Status)) {
> > +      DEBUG ((
> > +        DEBUG_ERROR,
> > +        "%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
> > +        __FUNCTION__,
> > +        Status
> > +        ));
> > +      return Status;
> > +    }
> > +  }
>
> Is there a way we could move this into EmmcSwitchClockFreq() rather
> than duplicate it?
>

Yes, it will only require modifying EmmcSwitchClockFreq argument list,
but this is no cost.

> >
> >    return Status;
> >  }
> > @@ -904,6 +925,24 @@ EmmcSwitchToHS200 (
> >      return Status;
> >    }
> >
> > +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> > +    Status = mOverride->NotifyPhase (
> > +                          Private->ControllerHandle,
> > +                          Slot,
> > +                          EdkiiSdMmcSwitchClockFreqPost,
> > +                          &Timing
> > +                          );
> > +    if (EFI_ERROR (Status)) {
> > +      DEBUG ((
> > +        DEBUG_ERROR,
> > +        "%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
> > +        __FUNCTION__,
> > +        Status
> > +        ));
> > +      return Status;
> > +    }
> > +  }
> > +
> >    Status = EmmcTuningClkForHs200 (PciIo, PassThru, Slot, BusWidth);
> >
> >    return Status;
> > @@ -988,6 +1027,27 @@ EmmcSwitchToHS400 (
> >
> >    HsTiming = 3;
> >    Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming, ClockFreq);
> > +  if (EFI_ERROR (Status)) {
> > +    return Status;
> > +  }
> > +
> > +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> > +    Status = mOverride->NotifyPhase (
> > +                          Private->ControllerHandle,
> > +                          Slot,
> > +                          EdkiiSdMmcSwitchClockFreqPost,
> > +                          &Timing
> > +                          );
> > +    if (EFI_ERROR (Status)) {
> > +      DEBUG ((
> > +        DEBUG_ERROR,
> > +        "%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
> > +        __FUNCTION__,
> > +        Status
> > +        ));
> > +      return Status;
> > +    }
> > +  }
> >
> >    return Status;
> >  }
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> > index 850ad26..5408bbc 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> > @@ -887,6 +887,24 @@ SdCardSetBusMode (
> >      return Status;
> >    }
> >
> > +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> > +    Status = mOverride->NotifyPhase (
> > +                          Private->ControllerHandle,
> > +                          Slot,
> > +                          EdkiiSdMmcSwitchClockFreqPost,
> > +                          &Timing
> > +                          );
> > +    if (EFI_ERROR (Status)) {
> > +      DEBUG ((
> > +        DEBUG_ERROR,
> > +        "%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
> > +        __FUNCTION__,
> > +        Status
> > +        ));
> > +      return Status;
> > +    }
> > +  }
> > +
> >    if ((AccessMode == 3) || ((AccessMode == 2) && (Capability->TuningSDR50 != 0))) {
> >      Status = SdCardTuningClock (PciIo, PassThru, Slot);
> >      if (EFI_ERROR (Status)) {
> > --
> > 2.7.4
> >


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-11-08 11:18 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-08  1:57 [PATCH v3 0/4] SdMmcOverride extension Marcin Wojtas
2018-11-08  1:57 ` [PATCH v3 1/4] MdeModulePkg/SdMmcPciHcDxe: Add an optional parameter in NotifyPhase Marcin Wojtas
2018-11-08  1:57 ` [PATCH v3 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol Marcin Wojtas
2018-11-08 11:06   ` Ard Biesheuvel
2018-11-08 11:15     ` Marcin Wojtas
2018-11-08  1:57 ` [PATCH v3 3/4] MdeModulePkg/SdMmcPciHcDxe: Add SwitchClockFreqPost to SdMmcOverride Marcin Wojtas
2018-11-08 11:09   ` Ard Biesheuvel
2018-11-08 11:18     ` Marcin Wojtas
2018-11-08  1:57 ` [PATCH v3 4/4] MdeModulePkg/SdMmcPciHcDxe: Allow overriding base clock frequency Marcin Wojtas
2018-11-08 11:10   ` Ard Biesheuvel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox