[edk2-devel] [PATCH 4/7] Platform/RaspberryPi/Arasan: Add write delay and voltage/clock config

Jeremy Linton posted 7 patches 5 years, 1 month ago
There is a newer version of this series
[edk2-devel] [PATCH 4/7] Platform/RaspberryPi/Arasan: Add write delay and voltage/clock config
Posted by Jeremy Linton 5 years, 1 month ago
The uboot and linux drivers have notes that there is a clock domain crossing
problem that happens with back to back writes to the sd controllers on the
rpi. Its not clear if this is still applicable to the rpi4/emmc2 but
it seems wise to add it.

Futher, we need to assure that the card voltage is set to 3.3V, and
we should try and follow some of the SDHCI docs when it comes to
changing the clock.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 .../Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c    | 112 +++++++++++++++++----
 .../Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.h    |   1 +
 2 files changed, 93 insertions(+), 20 deletions(-)

diff --git a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c
index 0cb7e85b38..a7b538a91a 100644
--- a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c
+++ b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c
@@ -18,6 +18,56 @@ UINT32 LastExecutedCommand = (UINT32) -1;
 STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol;
 STATIC UINTN MMCHS_BASE;
 
+STATIC
+UINT32
+EFIAPI
+SdMmioWrite32 (
+  IN      UINTN                     Address,
+  IN      UINT32                    Value
+  )
+{
+  UINT32 ret;
+  ret = (UINT32)MmioWrite32 (Address, Value);
+  // There is a bug about clock domain crossing on writes, delay to avoid it
+  gBS->Stall (STALL_AFTER_REG_WRITE_US);
+  return ret;
+}
+
+STATIC
+UINT32
+EFIAPI
+SdMmioOr32 (
+  IN      UINTN                     Address,
+  IN      UINT32                    OrData
+  )
+{
+  return SdMmioWrite32 (Address, MmioRead32 (Address) | OrData);
+}
+
+STATIC
+UINT32
+EFIAPI
+SdMmioAnd32 (
+  IN      UINTN                     Address,
+  IN      UINT32                    AndData
+  )
+{
+  return SdMmioWrite32 (Address, MmioRead32 (Address) & AndData);
+}
+
+STATIC
+UINT32
+EFIAPI
+SdMmioAndThenOr32 (
+  IN      UINTN                     Address,
+  IN      UINT32                    AndData,
+  IN      UINT32                    OrData
+  )
+{
+  return SdMmioWrite32 (Address, (MmioRead32 (Address) & AndData) | OrData);
+}
+
+
 /**
    These SD commands are optional, according to the SD Spec
 **/
@@ -175,7 +225,9 @@ SoftReset (
   IN UINT32 Mask
   )
 {
-  MmioOr32 (MMCHS_SYSCTL, Mask);
+  DEBUG ((DEBUG_MMCHOST_SD, "SoftReset with mask 0x%x\n", Mask));
+
+  SdMmioOr32 (MMCHS_SYSCTL, Mask);
   if (PollRegisterWithMask (MMCHS_SYSCTL, Mask, 0) == EFI_TIMEOUT) {
     DEBUG ((DEBUG_ERROR, "Failed to SoftReset with mask 0x%x\n", Mask));
     return EFI_TIMEOUT;
@@ -326,29 +378,29 @@ MMCSendCommand (
   }
 
   if (IsAppCmd && MmcCmd == ACMD22) {
-    MmioWrite32 (MMCHS_BLK, 4);
+    SdMmioWrite32 (MMCHS_BLK, 4);
   } else if (IsAppCmd && MmcCmd == ACMD51) {
-    MmioWrite32 (MMCHS_BLK, 8);
+    SdMmioWrite32 (MMCHS_BLK, 8);
   } else if (!IsAppCmd && MmcCmd == CMD6) {
-    MmioWrite32 (MMCHS_BLK, 64);
+    SdMmioWrite32 (MMCHS_BLK, 64);
   } else if (IsADTCCmd) {
-    MmioWrite32 (MMCHS_BLK, BLEN_512BYTES);
+    SdMmioWrite32 (MMCHS_BLK, BLEN_512BYTES);
   }
 
   // Set Data timeout counter value to max value.
-  MmioAndThenOr32 (MMCHS_SYSCTL, (UINT32) ~DTO_MASK, DTO_VAL);
+  SdMmioAndThenOr32 (MMCHS_SYSCTL, (UINT32) ~DTO_MASK, DTO_VAL);
 
   //
   // Clear Interrupt Status Register, but not the Card Inserted bit
   // to avoid messing with card detection logic.
   //
-  MmioWrite32 (MMCHS_INT_STAT, ALL_EN & ~(CARD_INS));
+  SdMmioWrite32 (MMCHS_INT_STAT, ALL_EN & ~(CARD_INS));
 
   // Set command argument register
-  MmioWrite32 (MMCHS_ARG, Argument);
+  SdMmioWrite32 (MMCHS_ARG, Argument);
 
   // Send the command
-  MmioWrite32 (MMCHS_CMD, MmcCmd);
+  SdMmioWrite32 (MMCHS_CMD, MmcCmd);
 
   // Check for the command status.
   while (RetryCount < MAX_RETRY_COUNT) {
@@ -373,7 +425,7 @@ MMCSendCommand (
 
     // Check if command is completed.
     if ((MmcStatus & CC) == CC) {
-      MmioWrite32 (MMCHS_INT_STAT, CC);
+      SdMmioWrite32 (MMCHS_INT_STAT, CC);
       break;
     }
 
@@ -428,6 +480,21 @@ MMCNotifyState (
         return Status;
       }
 
+      DEBUG ((DEBUG_MMCHOST_SD, "ArasanMMCHost: CAP %X CAPH %X\n", MmioRead32(MMCHS_CAPA),MmioRead32(MMCHS_CUR_CAPA)));
+
+      // Lets switch to card detect test mode.
+      SdMmioOr32 (MMCHS_HCTL, BIT7|BIT6);
+
+      // set card voltage
+      SdMmioAnd32 (MMCHS_HCTL, ~SDBP_ON);
+      SdMmioAndThenOr32 (MMCHS_HCTL, (UINT32) ~SDBP_MASK, SDVS_3_3_V);
+      SdMmioOr32 (MMCHS_HCTL, SDBP_ON);
+
+      DEBUG ((DEBUG_MMCHOST_SD, "ArasanMMCHost: AC12 %X HCTL %X\n", MmioRead32(MMCHS_AC12),MmioRead32(MMCHS_HCTL)));
+
+      // First turn off the clock
+      SdMmioAnd32 (MMCHS_SYSCTL, ~CEN);
+
       // Attempt to set the clock to 400Khz which is the expected initialization speed
       Status = CalculateClockFrequencyDivisor (400000, &Divisor, NULL);
       if (EFI_ERROR (Status)) {
@@ -436,10 +503,15 @@ MMCNotifyState (
       }
 
       // Set Data Timeout Counter value, set clock frequency, enable internal clock
-      MmioOr32 (MMCHS_SYSCTL, DTO_VAL | Divisor | CEN | ICS | ICE);
+      SdMmioOr32 (MMCHS_SYSCTL, DTO_VAL | Divisor | CEN | ICS | ICE);
+      SdMmioOr32 (MMCHS_HCTL, SDBP_ON);
+      // wait for ICS
+      while ((MmioRead32 (MMCHS_SYSCTL) & ICS_MASK) != ICS);
+
+      DEBUG ((DEBUG_MMCHOST_SD, "ArasanMMCHost: AC12 %X HCTL %X\n", MmioRead32(MMCHS_AC12),MmioRead32(MMCHS_HCTL)));
 
       // Enable interrupts
-      MmioWrite32 (MMCHS_IE, ALL_EN);
+      SdMmioWrite32 (MMCHS_IE, ALL_EN);
     }
     break;
   case MmcIdleState:
@@ -452,7 +524,7 @@ MMCNotifyState (
     ClockFrequency = 25000000;
 
     // First turn off the clock
-    MmioAnd32 (MMCHS_SYSCTL, ~CEN);
+    SdMmioAnd32 (MMCHS_SYSCTL, ~CEN);
 
     Status = CalculateClockFrequencyDivisor (ClockFrequency, &Divisor, NULL);
     if (EFI_ERROR (Status)) {
@@ -462,13 +534,13 @@ MMCNotifyState (
     }
 
     // Setup new divisor
-    MmioAndThenOr32 (MMCHS_SYSCTL, (UINT32) ~CLKD_MASK, Divisor);
+    SdMmioAndThenOr32 (MMCHS_SYSCTL, (UINT32) ~CLKD_MASK, Divisor);
 
     // Wait for the clock to stabilise
     while ((MmioRead32 (MMCHS_SYSCTL) & ICS_MASK) != ICS);
 
     // Set Data Timeout Counter value, set clock frequency, enable internal clock
-    MmioOr32 (MMCHS_SYSCTL, CEN);
+    SdMmioOr32 (MMCHS_SYSCTL, CEN);
     break;
   case MmcTransferState:
     break;
@@ -635,7 +707,7 @@ MMCReadBlockData (
     while (RetryCount < MAX_RETRY_COUNT) {
       MmcStatus = MmioRead32 (MMCHS_INT_STAT);
       if ((MmcStatus & BRR) != 0) {
-        MmioWrite32 (MMCHS_INT_STAT, BRR);
+        SdMmioWrite32 (MMCHS_INT_STAT, BRR);
         /*
          * Data is ready.
          */
@@ -662,7 +734,7 @@ MMCReadBlockData (
     gBS->Stall (STALL_AFTER_READ_US);
   }
 
-  MmioWrite32 (MMCHS_INT_STAT, BRR);
+  SdMmioWrite32 (MMCHS_INT_STAT, BRR);
   return EFI_SUCCESS;
 }
 
@@ -699,13 +771,13 @@ MMCWriteBlockData (
     while (RetryCount < MAX_RETRY_COUNT) {
       MmcStatus = MmioRead32 (MMCHS_INT_STAT);
       if ((MmcStatus & BWR) != 0) {
-        MmioWrite32 (MMCHS_INT_STAT, BWR);
+        SdMmioWrite32 (MMCHS_INT_STAT, BWR);
         /*
          * Can write data.
          */
         mFwProtocol->SetLed (TRUE);
         for (Count = 0; Count < BlockLen; Count += 4, Buffer++) {
-          MmioWrite32 (MMCHS_DATA, *Buffer);
+          SdMmioWrite32 (MMCHS_DATA, *Buffer);
         }
 
         mFwProtocol->SetLed (FALSE);
@@ -726,7 +798,7 @@ MMCWriteBlockData (
     gBS->Stall (STALL_AFTER_WRITE_US);
   }
 
-  MmioWrite32 (MMCHS_INT_STAT, BWR);
+  SdMmioWrite32 (MMCHS_INT_STAT, BWR);
   return EFI_SUCCESS;
 }
 
diff --git a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.h b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.h
index 6cd600f738..e94606cc5b 100644
--- a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.h
+++ b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.h
@@ -37,6 +37,7 @@
 #define STALL_AFTER_REC_RESP_US (50)
 #define STALL_AFTER_WRITE_US (200)
 #define STALL_AFTER_READ_US (20)
+#define STALL_AFTER_REG_WRITE_US (10)
 #define STALL_AFTER_RETRY_US (20)
 
 #define MAX_DIVISOR_VALUE 1023
-- 
2.13.7



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#68816): https://edk2.groups.io/g/devel/message/68816
Mute This Topic: https://groups.io/mt/78964893/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 4/7] Platform/RaspberryPi/Arasan: Add write delay and voltage/clock config
Posted by Andrei Warkentin 5 years, 1 month ago
I believe that applies only to the Arasan integration, not MMC2.

I'm trying to recollect why I thought this didn't matter  (or how it was getting mitigated), but i'm drawing a blank. I'm okay doing it.

Reviewed-by: Andrei Warkentin <awarkentin@vmware.com>
________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Jeremy Linton via groups.io <jeremy.linton=arm.com@groups.io>
Sent: Monday, December 14, 2020 5:23 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: ard.biesheuvel@arm.com <ard.biesheuvel@arm.com>; leif@nuviainc.com <leif@nuviainc.com>; pete@akeo.ie <pete@akeo.ie>; andrey.warkentin@gmail.com <andrey.warkentin@gmail.com>; samer.el-haj-mahmoud@arm.com <samer.el-haj-mahmoud@arm.com>; Jeremy Linton <jeremy.linton@arm.com>
Subject: [edk2-devel] [PATCH 4/7] Platform/RaspberryPi/Arasan: Add write delay and voltage/clock config

The uboot and linux drivers have notes that there is a clock domain crossing
problem that happens with back to back writes to the sd controllers on the
rpi. Its not clear if this is still applicable to the rpi4/emmc2 but
it seems wise to add it.

Futher, we need to assure that the card voltage is set to 3.3V, and
we should try and follow some of the SDHCI docs when it comes to
changing the clock.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 .../Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c    | 112 +++++++++++++++++----
 .../Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.h    |   1 +
 2 files changed, 93 insertions(+), 20 deletions(-)

diff --git a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c
index 0cb7e85b38..a7b538a91a 100644
--- a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c
+++ b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c
@@ -18,6 +18,56 @@ UINT32 LastExecutedCommand = (UINT32) -1;
 STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol;

 STATIC UINTN MMCHS_BASE;



+STATIC

+UINT32

+EFIAPI

+SdMmioWrite32 (

+  IN      UINTN                     Address,

+  IN      UINT32                    Value

+  )

+{

+  UINT32 ret;

+  ret = (UINT32)MmioWrite32 (Address, Value);

+  // There is a bug about clock domain crossing on writes, delay to avoid it

+  gBS->Stall (STALL_AFTER_REG_WRITE_US);

+  return ret;

+}

+

+STATIC

+UINT32

+EFIAPI

+SdMmioOr32 (

+  IN      UINTN                     Address,

+  IN      UINT32                    OrData

+  )

+{

+  return SdMmioWrite32 (Address, MmioRead32 (Address) | OrData);

+}

+

+STATIC

+UINT32

+EFIAPI

+SdMmioAnd32 (

+  IN      UINTN                     Address,

+  IN      UINT32                    AndData

+  )

+{

+  return SdMmioWrite32 (Address, MmioRead32 (Address) & AndData);

+}

+

+STATIC

+UINT32

+EFIAPI

+SdMmioAndThenOr32 (

+  IN      UINTN                     Address,

+  IN      UINT32                    AndData,

+  IN      UINT32                    OrData

+  )

+{

+  return SdMmioWrite32 (Address, (MmioRead32 (Address) & AndData) | OrData);

+}

+

+

 /**

    These SD commands are optional, according to the SD Spec

 **/

@@ -175,7 +225,9 @@ SoftReset (
   IN UINT32 Mask

   )

 {

-  MmioOr32 (MMCHS_SYSCTL, Mask);

+  DEBUG ((DEBUG_MMCHOST_SD, "SoftReset with mask 0x%x\n", Mask));

+

+  SdMmioOr32 (MMCHS_SYSCTL, Mask);

   if (PollRegisterWithMask (MMCHS_SYSCTL, Mask, 0) == EFI_TIMEOUT) {

     DEBUG ((DEBUG_ERROR, "Failed to SoftReset with mask 0x%x\n", Mask));

     return EFI_TIMEOUT;

@@ -326,29 +378,29 @@ MMCSendCommand (
   }



   if (IsAppCmd && MmcCmd == ACMD22) {

-    MmioWrite32 (MMCHS_BLK, 4);

+    SdMmioWrite32 (MMCHS_BLK, 4);

   } else if (IsAppCmd && MmcCmd == ACMD51) {

-    MmioWrite32 (MMCHS_BLK, 8);

+    SdMmioWrite32 (MMCHS_BLK, 8);

   } else if (!IsAppCmd && MmcCmd == CMD6) {

-    MmioWrite32 (MMCHS_BLK, 64);

+    SdMmioWrite32 (MMCHS_BLK, 64);

   } else if (IsADTCCmd) {

-    MmioWrite32 (MMCHS_BLK, BLEN_512BYTES);

+    SdMmioWrite32 (MMCHS_BLK, BLEN_512BYTES);

   }



   // Set Data timeout counter value to max value.

-  MmioAndThenOr32 (MMCHS_SYSCTL, (UINT32) ~DTO_MASK, DTO_VAL);

+  SdMmioAndThenOr32 (MMCHS_SYSCTL, (UINT32) ~DTO_MASK, DTO_VAL);



   //

   // Clear Interrupt Status Register, but not the Card Inserted bit

   // to avoid messing with card detection logic.

   //

-  MmioWrite32 (MMCHS_INT_STAT, ALL_EN & ~(CARD_INS));

+  SdMmioWrite32 (MMCHS_INT_STAT, ALL_EN & ~(CARD_INS));



   // Set command argument register

-  MmioWrite32 (MMCHS_ARG, Argument);

+  SdMmioWrite32 (MMCHS_ARG, Argument);



   // Send the command

-  MmioWrite32 (MMCHS_CMD, MmcCmd);

+  SdMmioWrite32 (MMCHS_CMD, MmcCmd);



   // Check for the command status.

   while (RetryCount < MAX_RETRY_COUNT) {

@@ -373,7 +425,7 @@ MMCSendCommand (


     // Check if command is completed.

     if ((MmcStatus & CC) == CC) {

-      MmioWrite32 (MMCHS_INT_STAT, CC);

+      SdMmioWrite32 (MMCHS_INT_STAT, CC);

       break;

     }



@@ -428,6 +480,21 @@ MMCNotifyState (
         return Status;

       }



+      DEBUG ((DEBUG_MMCHOST_SD, "ArasanMMCHost: CAP %X CAPH %X\n", MmioRead32(MMCHS_CAPA),MmioRead32(MMCHS_CUR_CAPA)));

+

+      // Lets switch to card detect test mode.

+      SdMmioOr32 (MMCHS_HCTL, BIT7|BIT6);

+

+      // set card voltage

+      SdMmioAnd32 (MMCHS_HCTL, ~SDBP_ON);

+      SdMmioAndThenOr32 (MMCHS_HCTL, (UINT32) ~SDBP_MASK, SDVS_3_3_V);

+      SdMmioOr32 (MMCHS_HCTL, SDBP_ON);

+

+      DEBUG ((DEBUG_MMCHOST_SD, "ArasanMMCHost: AC12 %X HCTL %X\n", MmioRead32(MMCHS_AC12),MmioRead32(MMCHS_HCTL)));

+

+      // First turn off the clock

+      SdMmioAnd32 (MMCHS_SYSCTL, ~CEN);

+

       // Attempt to set the clock to 400Khz which is the expected initialization speed

       Status = CalculateClockFrequencyDivisor (400000, &Divisor, NULL);

       if (EFI_ERROR (Status)) {

@@ -436,10 +503,15 @@ MMCNotifyState (
       }



       // Set Data Timeout Counter value, set clock frequency, enable internal clock

-      MmioOr32 (MMCHS_SYSCTL, DTO_VAL | Divisor | CEN | ICS | ICE);

+      SdMmioOr32 (MMCHS_SYSCTL, DTO_VAL | Divisor | CEN | ICS | ICE);

+      SdMmioOr32 (MMCHS_HCTL, SDBP_ON);

+      // wait for ICS

+      while ((MmioRead32 (MMCHS_SYSCTL) & ICS_MASK) != ICS);

+

+      DEBUG ((DEBUG_MMCHOST_SD, "ArasanMMCHost: AC12 %X HCTL %X\n", MmioRead32(MMCHS_AC12),MmioRead32(MMCHS_HCTL)));



       // Enable interrupts

-      MmioWrite32 (MMCHS_IE, ALL_EN);

+      SdMmioWrite32 (MMCHS_IE, ALL_EN);

     }

     break;

   case MmcIdleState:

@@ -452,7 +524,7 @@ MMCNotifyState (
     ClockFrequency = 25000000;



     // First turn off the clock

-    MmioAnd32 (MMCHS_SYSCTL, ~CEN);

+    SdMmioAnd32 (MMCHS_SYSCTL, ~CEN);



     Status = CalculateClockFrequencyDivisor (ClockFrequency, &Divisor, NULL);

     if (EFI_ERROR (Status)) {

@@ -462,13 +534,13 @@ MMCNotifyState (
     }



     // Setup new divisor

-    MmioAndThenOr32 (MMCHS_SYSCTL, (UINT32) ~CLKD_MASK, Divisor);

+    SdMmioAndThenOr32 (MMCHS_SYSCTL, (UINT32) ~CLKD_MASK, Divisor);



     // Wait for the clock to stabilise

     while ((MmioRead32 (MMCHS_SYSCTL) & ICS_MASK) != ICS);



     // Set Data Timeout Counter value, set clock frequency, enable internal clock

-    MmioOr32 (MMCHS_SYSCTL, CEN);

+    SdMmioOr32 (MMCHS_SYSCTL, CEN);

     break;

   case MmcTransferState:

     break;

@@ -635,7 +707,7 @@ MMCReadBlockData (
     while (RetryCount < MAX_RETRY_COUNT) {

       MmcStatus = MmioRead32 (MMCHS_INT_STAT);

       if ((MmcStatus & BRR) != 0) {

-        MmioWrite32 (MMCHS_INT_STAT, BRR);

+        SdMmioWrite32 (MMCHS_INT_STAT, BRR);

         /*

          * Data is ready.

          */

@@ -662,7 +734,7 @@ MMCReadBlockData (
     gBS->Stall (STALL_AFTER_READ_US);

   }



-  MmioWrite32 (MMCHS_INT_STAT, BRR);

+  SdMmioWrite32 (MMCHS_INT_STAT, BRR);

   return EFI_SUCCESS;

 }



@@ -699,13 +771,13 @@ MMCWriteBlockData (
     while (RetryCount < MAX_RETRY_COUNT) {

       MmcStatus = MmioRead32 (MMCHS_INT_STAT);

       if ((MmcStatus & BWR) != 0) {

-        MmioWrite32 (MMCHS_INT_STAT, BWR);

+        SdMmioWrite32 (MMCHS_INT_STAT, BWR);

         /*

          * Can write data.

          */

         mFwProtocol->SetLed (TRUE);

         for (Count = 0; Count < BlockLen; Count += 4, Buffer++) {

-          MmioWrite32 (MMCHS_DATA, *Buffer);

+          SdMmioWrite32 (MMCHS_DATA, *Buffer);

         }



         mFwProtocol->SetLed (FALSE);

@@ -726,7 +798,7 @@ MMCWriteBlockData (
     gBS->Stall (STALL_AFTER_WRITE_US);

   }



-  MmioWrite32 (MMCHS_INT_STAT, BWR);

+  SdMmioWrite32 (MMCHS_INT_STAT, BWR);

   return EFI_SUCCESS;

 }



diff --git a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.h b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.h
index 6cd600f738..e94606cc5b 100644
--- a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.h
+++ b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.h
@@ -37,6 +37,7 @@
 #define STALL_AFTER_REC_RESP_US (50)

 #define STALL_AFTER_WRITE_US (200)

 #define STALL_AFTER_READ_US (20)

+#define STALL_AFTER_REG_WRITE_US (10)

 #define STALL_AFTER_RETRY_US (20)



 #define MAX_DIVISOR_VALUE 1023

--
2.13.7



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#68816): https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F68816&amp;data=04%7C01%7Cawarkentin%40vmware.com%7C121bf8f78f3b437be80708d8a0875a08%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637435850504293478%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=6UBUDOHEs8MHLhK7%2B9DkL4NAdxzwttiJRvE88aqD%2BHw%3D&amp;reserved=0
Mute This Topic: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.io%2Fmt%2F78964893%2F4387333&amp;data=04%7C01%7Cawarkentin%40vmware.com%7C121bf8f78f3b437be80708d8a0875a08%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637435850504293478%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=OVqu67c28Qqnk25HasY%2BxkEC0KgOBQfxZWJxGZ0nJlk%3D&amp;reserved=0
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Funsub&amp;data=04%7C01%7Cawarkentin%40vmware.com%7C121bf8f78f3b437be80708d8a0875a08%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637435850504293478%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=aXFzE1B1xX6JQeXymAEmWa2Z99welypR6HTHXrv1%2B7E%3D&amp;reserved=0 [awarkentin@vmware.com]
-=-=-=-=-=-=




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#68877): https://edk2.groups.io/g/devel/message/68877
Mute This Topic: https://groups.io/mt/78964893/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 4/7] Platform/RaspberryPi/Arasan: Add write delay and voltage/clock config
Posted by Jeremy Linton 5 years, 1 month ago
Hi,

On 12/15/20 12:26 PM, Andrei Warkentin wrote:
> I believe that applies only to the Arasan integration, not MMC2.
> 
> I'm trying to recollect why I thought this didn't matter  (or how it was getting mitigated), but i'm drawing a blank. I'm okay doing it.

Well, I think it "works" without it, although it appears both uboot and 
linux have similar workarounds for both controllers. So "works" might be 
a case of works for me but not for you. That is a large part of the 
problem around using PNP0D40 as the _CID too. It works for both 
controllers, despite the lack of careful alignment controls, or this 
workaround in linux with the straight sdhci_acpi driver. If I can figure 
out how to suppress/quirk the cmd12 warnings the emmc2 it would almost 
be worth doing.

A good part of this set is just based on me banging my head and 
inserting trace/prints in the linux and uboot gpio/mailbox and mmc 
paths, and then supporting the most obvious differences. So while I 
backed a few things out, some of these things remain (like this) because 
there is a bit of documentation in those drivers claiming there is a 
clock domain crossing bug. What the actual details, or how to reproduce 
aren't included.

> 
> Reviewed-by: Andrei Warkentin <awarkentin@vmware.com>
> ________________________________
> From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Jeremy Linton via groups.io <jeremy.linton=arm.com@groups.io>
> Sent: Monday, December 14, 2020 5:23 PM
> To: devel@edk2.groups.io <devel@edk2.groups.io>
> Cc: ard.biesheuvel@arm.com <ard.biesheuvel@arm.com>; leif@nuviainc.com <leif@nuviainc.com>; pete@akeo.ie <pete@akeo.ie>; andrey.warkentin@gmail.com <andrey.warkentin@gmail.com>; samer.el-haj-mahmoud@arm.com <samer.el-haj-mahmoud@arm.com>; Jeremy Linton <jeremy.linton@arm.com>
> Subject: [edk2-devel] [PATCH 4/7] Platform/RaspberryPi/Arasan: Add write delay and voltage/clock config
> 
> The uboot and linux drivers have notes that there is a clock domain crossing
> problem that happens with back to back writes to the sd controllers on the
> rpi. Its not clear if this is still applicable to the rpi4/emmc2 but
> it seems wise to add it.
> 
> Futher, we need to assure that the card voltage is set to 3.3V, and
> we should try and follow some of the SDHCI docs when it comes to
> changing the clock.
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>   .../Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c    | 112 +++++++++++++++++----
>   .../Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.h    |   1 +
>   2 files changed, 93 insertions(+), 20 deletions(-)
> 
> diff --git a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c
> index 0cb7e85b38..a7b538a91a 100644
> --- a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c
> +++ b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c
> @@ -18,6 +18,56 @@ UINT32 LastExecutedCommand = (UINT32) -1;
>   STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol;
> 
>   STATIC UINTN MMCHS_BASE;
> 
> 
> 
> +STATIC
> 
> +UINT32
> 
> +EFIAPI
> 
> +SdMmioWrite32 (
> 
> +  IN      UINTN                     Address,
> 
> +  IN      UINT32                    Value
> 
> +  )
> 
> +{
> 
> +  UINT32 ret;
> 
> +  ret = (UINT32)MmioWrite32 (Address, Value);
> 
> +  // There is a bug about clock domain crossing on writes, delay to avoid it
> 
> +  gBS->Stall (STALL_AFTER_REG_WRITE_US);
> 
> +  return ret;
> 
> +}
> 
> +
> 
> +STATIC
> 
> +UINT32
> 
> +EFIAPI
> 
> +SdMmioOr32 (
> 
> +  IN      UINTN                     Address,
> 
> +  IN      UINT32                    OrData
> 
> +  )
> 
> +{
> 
> +  return SdMmioWrite32 (Address, MmioRead32 (Address) | OrData);
> 
> +}
> 
> +
> 
> +STATIC
> 
> +UINT32
> 
> +EFIAPI
> 
> +SdMmioAnd32 (
> 
> +  IN      UINTN                     Address,
> 
> +  IN      UINT32                    AndData
> 
> +  )
> 
> +{
> 
> +  return SdMmioWrite32 (Address, MmioRead32 (Address) & AndData);
> 
> +}
> 
> +
> 
> +STATIC
> 
> +UINT32
> 
> +EFIAPI
> 
> +SdMmioAndThenOr32 (
> 
> +  IN      UINTN                     Address,
> 
> +  IN      UINT32                    AndData,
> 
> +  IN      UINT32                    OrData
> 
> +  )
> 
> +{
> 
> +  return SdMmioWrite32 (Address, (MmioRead32 (Address) & AndData) | OrData);
> 
> +}
> 
> +
> 
> +
> 
>   /**
> 
>      These SD commands are optional, according to the SD Spec
> 
>   **/
> 
> @@ -175,7 +225,9 @@ SoftReset (
>     IN UINT32 Mask
> 
>     )
> 
>   {
> 
> -  MmioOr32 (MMCHS_SYSCTL, Mask);
> 
> +  DEBUG ((DEBUG_MMCHOST_SD, "SoftReset with mask 0x%x\n", Mask));
> 
> +
> 
> +  SdMmioOr32 (MMCHS_SYSCTL, Mask);
> 
>     if (PollRegisterWithMask (MMCHS_SYSCTL, Mask, 0) == EFI_TIMEOUT) {
> 
>       DEBUG ((DEBUG_ERROR, "Failed to SoftReset with mask 0x%x\n", Mask));
> 
>       return EFI_TIMEOUT;
> 
> @@ -326,29 +378,29 @@ MMCSendCommand (
>     }
> 
> 
> 
>     if (IsAppCmd && MmcCmd == ACMD22) {
> 
> -    MmioWrite32 (MMCHS_BLK, 4);
> 
> +    SdMmioWrite32 (MMCHS_BLK, 4);
> 
>     } else if (IsAppCmd && MmcCmd == ACMD51) {
> 
> -    MmioWrite32 (MMCHS_BLK, 8);
> 
> +    SdMmioWrite32 (MMCHS_BLK, 8);
> 
>     } else if (!IsAppCmd && MmcCmd == CMD6) {
> 
> -    MmioWrite32 (MMCHS_BLK, 64);
> 
> +    SdMmioWrite32 (MMCHS_BLK, 64);
> 
>     } else if (IsADTCCmd) {
> 
> -    MmioWrite32 (MMCHS_BLK, BLEN_512BYTES);
> 
> +    SdMmioWrite32 (MMCHS_BLK, BLEN_512BYTES);
> 
>     }
> 
> 
> 
>     // Set Data timeout counter value to max value.
> 
> -  MmioAndThenOr32 (MMCHS_SYSCTL, (UINT32) ~DTO_MASK, DTO_VAL);
> 
> +  SdMmioAndThenOr32 (MMCHS_SYSCTL, (UINT32) ~DTO_MASK, DTO_VAL);
> 
> 
> 
>     //
> 
>     // Clear Interrupt Status Register, but not the Card Inserted bit
> 
>     // to avoid messing with card detection logic.
> 
>     //
> 
> -  MmioWrite32 (MMCHS_INT_STAT, ALL_EN & ~(CARD_INS));
> 
> +  SdMmioWrite32 (MMCHS_INT_STAT, ALL_EN & ~(CARD_INS));
> 
> 
> 
>     // Set command argument register
> 
> -  MmioWrite32 (MMCHS_ARG, Argument);
> 
> +  SdMmioWrite32 (MMCHS_ARG, Argument);
> 
> 
> 
>     // Send the command
> 
> -  MmioWrite32 (MMCHS_CMD, MmcCmd);
> 
> +  SdMmioWrite32 (MMCHS_CMD, MmcCmd);
> 
> 
> 
>     // Check for the command status.
> 
>     while (RetryCount < MAX_RETRY_COUNT) {
> 
> @@ -373,7 +425,7 @@ MMCSendCommand (
> 
> 
>       // Check if command is completed.
> 
>       if ((MmcStatus & CC) == CC) {
> 
> -      MmioWrite32 (MMCHS_INT_STAT, CC);
> 
> +      SdMmioWrite32 (MMCHS_INT_STAT, CC);
> 
>         break;
> 
>       }
> 
> 
> 
> @@ -428,6 +480,21 @@ MMCNotifyState (
>           return Status;
> 
>         }
> 
> 
> 
> +      DEBUG ((DEBUG_MMCHOST_SD, "ArasanMMCHost: CAP %X CAPH %X\n", MmioRead32(MMCHS_CAPA),MmioRead32(MMCHS_CUR_CAPA)));
> 
> +
> 
> +      // Lets switch to card detect test mode.
> 
> +      SdMmioOr32 (MMCHS_HCTL, BIT7|BIT6);
> 
> +
> 
> +      // set card voltage
> 
> +      SdMmioAnd32 (MMCHS_HCTL, ~SDBP_ON);
> 
> +      SdMmioAndThenOr32 (MMCHS_HCTL, (UINT32) ~SDBP_MASK, SDVS_3_3_V);
> 
> +      SdMmioOr32 (MMCHS_HCTL, SDBP_ON);
> 
> +
> 
> +      DEBUG ((DEBUG_MMCHOST_SD, "ArasanMMCHost: AC12 %X HCTL %X\n", MmioRead32(MMCHS_AC12),MmioRead32(MMCHS_HCTL)));
> 
> +
> 
> +      // First turn off the clock
> 
> +      SdMmioAnd32 (MMCHS_SYSCTL, ~CEN);
> 
> +
> 
>         // Attempt to set the clock to 400Khz which is the expected initialization speed
> 
>         Status = CalculateClockFrequencyDivisor (400000, &Divisor, NULL);
> 
>         if (EFI_ERROR (Status)) {
> 
> @@ -436,10 +503,15 @@ MMCNotifyState (
>         }
> 
> 
> 
>         // Set Data Timeout Counter value, set clock frequency, enable internal clock
> 
> -      MmioOr32 (MMCHS_SYSCTL, DTO_VAL | Divisor | CEN | ICS | ICE);
> 
> +      SdMmioOr32 (MMCHS_SYSCTL, DTO_VAL | Divisor | CEN | ICS | ICE);
> 
> +      SdMmioOr32 (MMCHS_HCTL, SDBP_ON);
> 
> +      // wait for ICS
> 
> +      while ((MmioRead32 (MMCHS_SYSCTL) & ICS_MASK) != ICS);
> 
> +
> 
> +      DEBUG ((DEBUG_MMCHOST_SD, "ArasanMMCHost: AC12 %X HCTL %X\n", MmioRead32(MMCHS_AC12),MmioRead32(MMCHS_HCTL)));
> 
> 
> 
>         // Enable interrupts
> 
> -      MmioWrite32 (MMCHS_IE, ALL_EN);
> 
> +      SdMmioWrite32 (MMCHS_IE, ALL_EN);
> 
>       }
> 
>       break;
> 
>     case MmcIdleState:
> 
> @@ -452,7 +524,7 @@ MMCNotifyState (
>       ClockFrequency = 25000000;
> 
> 
> 
>       // First turn off the clock
> 
> -    MmioAnd32 (MMCHS_SYSCTL, ~CEN);
> 
> +    SdMmioAnd32 (MMCHS_SYSCTL, ~CEN);
> 
> 
> 
>       Status = CalculateClockFrequencyDivisor (ClockFrequency, &Divisor, NULL);
> 
>       if (EFI_ERROR (Status)) {
> 
> @@ -462,13 +534,13 @@ MMCNotifyState (
>       }
> 
> 
> 
>       // Setup new divisor
> 
> -    MmioAndThenOr32 (MMCHS_SYSCTL, (UINT32) ~CLKD_MASK, Divisor);
> 
> +    SdMmioAndThenOr32 (MMCHS_SYSCTL, (UINT32) ~CLKD_MASK, Divisor);
> 
> 
> 
>       // Wait for the clock to stabilise
> 
>       while ((MmioRead32 (MMCHS_SYSCTL) & ICS_MASK) != ICS);
> 
> 
> 
>       // Set Data Timeout Counter value, set clock frequency, enable internal clock
> 
> -    MmioOr32 (MMCHS_SYSCTL, CEN);
> 
> +    SdMmioOr32 (MMCHS_SYSCTL, CEN);
> 
>       break;
> 
>     case MmcTransferState:
> 
>       break;
> 
> @@ -635,7 +707,7 @@ MMCReadBlockData (
>       while (RetryCount < MAX_RETRY_COUNT) {
> 
>         MmcStatus = MmioRead32 (MMCHS_INT_STAT);
> 
>         if ((MmcStatus & BRR) != 0) {
> 
> -        MmioWrite32 (MMCHS_INT_STAT, BRR);
> 
> +        SdMmioWrite32 (MMCHS_INT_STAT, BRR);
> 
>           /*
> 
>            * Data is ready.
> 
>            */
> 
> @@ -662,7 +734,7 @@ MMCReadBlockData (
>       gBS->Stall (STALL_AFTER_READ_US);
> 
>     }
> 
> 
> 
> -  MmioWrite32 (MMCHS_INT_STAT, BRR);
> 
> +  SdMmioWrite32 (MMCHS_INT_STAT, BRR);
> 
>     return EFI_SUCCESS;
> 
>   }
> 
> 
> 
> @@ -699,13 +771,13 @@ MMCWriteBlockData (
>       while (RetryCount < MAX_RETRY_COUNT) {
> 
>         MmcStatus = MmioRead32 (MMCHS_INT_STAT);
> 
>         if ((MmcStatus & BWR) != 0) {
> 
> -        MmioWrite32 (MMCHS_INT_STAT, BWR);
> 
> +        SdMmioWrite32 (MMCHS_INT_STAT, BWR);
> 
>           /*
> 
>            * Can write data.
> 
>            */
> 
>           mFwProtocol->SetLed (TRUE);
> 
>           for (Count = 0; Count < BlockLen; Count += 4, Buffer++) {
> 
> -          MmioWrite32 (MMCHS_DATA, *Buffer);
> 
> +          SdMmioWrite32 (MMCHS_DATA, *Buffer);
> 
>           }
> 
> 
> 
>           mFwProtocol->SetLed (FALSE);
> 
> @@ -726,7 +798,7 @@ MMCWriteBlockData (
>       gBS->Stall (STALL_AFTER_WRITE_US);
> 
>     }
> 
> 
> 
> -  MmioWrite32 (MMCHS_INT_STAT, BWR);
> 
> +  SdMmioWrite32 (MMCHS_INT_STAT, BWR);
> 
>     return EFI_SUCCESS;
> 
>   }
> 
> 
> 
> diff --git a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.h b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.h
> index 6cd600f738..e94606cc5b 100644
> --- a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.h
> +++ b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.h
> @@ -37,6 +37,7 @@
>   #define STALL_AFTER_REC_RESP_US (50)
> 
>   #define STALL_AFTER_WRITE_US (200)
> 
>   #define STALL_AFTER_READ_US (20)
> 
> +#define STALL_AFTER_REG_WRITE_US (10)
> 
>   #define STALL_AFTER_RETRY_US (20)
> 
> 
> 
>   #define MAX_DIVISOR_VALUE 1023
> 
> --
> 2.13.7
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#68816): https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F68816&amp;data=04%7C01%7Cawarkentin%40vmware.com%7C121bf8f78f3b437be80708d8a0875a08%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637435850504293478%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=6UBUDOHEs8MHLhK7%2B9DkL4NAdxzwttiJRvE88aqD%2BHw%3D&amp;reserved=0
> Mute This Topic: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.io%2Fmt%2F78964893%2F4387333&amp;data=04%7C01%7Cawarkentin%40vmware.com%7C121bf8f78f3b437be80708d8a0875a08%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637435850504293478%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=OVqu67c28Qqnk25HasY%2BxkEC0KgOBQfxZWJxGZ0nJlk%3D&amp;reserved=0
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Funsub&amp;data=04%7C01%7Cawarkentin%40vmware.com%7C121bf8f78f3b437be80708d8a0875a08%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637435850504293478%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=aXFzE1B1xX6JQeXymAEmWa2Z99welypR6HTHXrv1%2B7E%3D&amp;reserved=0 [awarkentin@vmware.com]
> -=-=-=-=-=-=
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#68880): https://edk2.groups.io/g/devel/message/68880
Mute This Topic: https://groups.io/mt/78964893/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH 4/7] Platform/RaspberryPi/Arasan: Add write delay and voltage/clock config
Posted by Andrei Warkentin 5 years, 1 month ago
Yep, all understood and acknowledged. No actionable items - lgtm.

Reviewed-by: Andrei Warkentin <awarkentin@vmware.com>



---
Отправлено из Workspace ONE Boxer<https://whatisworkspaceone.com/boxer>

15 декабря 2020 г. в 12:46:24 PM GMT-6 Jeremy Linton <jeremy.linton@arm.com> пишет:
Hi,

On 12/15/20 12:26 PM, Andrei Warkentin wrote:
> I believe that applies only to the Arasan integration, not MMC2.
>
> I'm trying to recollect why I thought this didn't matter  (or how it was getting mitigated), but i'm drawing a blank. I'm okay doing it.

Well, I think it "works" without it, although it appears both uboot and
linux have similar workarounds for both controllers. So "works" might be
a case of works for me but not for you. That is a large part of the
problem around using PNP0D40 as the _CID too. It works for both
controllers, despite the lack of careful alignment controls, or this
workaround in linux with the straight sdhci_acpi driver. If I can figure
out how to suppress/quirk the cmd12 warnings the emmc2 it would almost
be worth doing.

A good part of this set is just based on me banging my head and
inserting trace/prints in the linux and uboot gpio/mailbox and mmc
paths, and then supporting the most obvious differences. So while I
backed a few things out, some of these things remain (like this) because
there is a bit of documentation in those drivers claiming there is a
clock domain crossing bug. What the actual details, or how to reproduce
aren't included.

>
> Reviewed-by: Andrei Warkentin <awarkentin@vmware.com>
> ________________________________
> From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Jeremy Linton via groups.io <jeremy.linton=arm.com@groups.io>
> Sent: Monday, December 14, 2020 5:23 PM
> To: devel@edk2.groups.io <devel@edk2.groups.io>
> Cc: ard.biesheuvel@arm.com <ard.biesheuvel@arm.com>; leif@nuviainc.com <leif@nuviainc.com>; pete@akeo.ie <pete@akeo.ie>; andrey.warkentin@gmail.com <andrey.warkentin@gmail.com>; samer.el-haj-mahmoud@arm.com <samer.el-haj-mahmoud@arm.com>; Jeremy Linton <jeremy.linton@arm.com>
> Subject: [edk2-devel] [PATCH 4/7] Platform/RaspberryPi/Arasan: Add write delay and voltage/clock config
>
> The uboot and linux drivers have notes that there is a clock domain crossing
> problem that happens with back to back writes to the sd controllers on the
> rpi. Its not clear if this is still applicable to the rpi4/emmc2 but
> it seems wise to add it.
>
> Futher, we need to assure that the card voltage is set to 3.3V, and
> we should try and follow some of the SDHCI docs when it comes to
> changing the clock.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>   .../Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c    | 112 +++++++++++++++++----
>   .../Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.h    |   1 +
>   2 files changed, 93 insertions(+), 20 deletions(-)
>
> diff --git a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c
> index 0cb7e85b38..a7b538a91a 100644
> --- a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c
> +++ b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c
> @@ -18,6 +18,56 @@ UINT32 LastExecutedCommand = (UINT32) -1;
>   STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol;
>
>   STATIC UINTN MMCHS_BASE;
>
>
>
> +STATIC
>
> +UINT32
>
> +EFIAPI
>
> +SdMmioWrite32 (
>
> +  IN      UINTN                     Address,
>
> +  IN      UINT32                    Value
>
> +  )
>
> +{
>
> +  UINT32 ret;
>
> +  ret = (UINT32)MmioWrite32 (Address, Value);
>
> +  // There is a bug about clock domain crossing on writes, delay to avoid it
>
> +  gBS->Stall (STALL_AFTER_REG_WRITE_US);
>
> +  return ret;
>
> +}
>
> +
>
> +STATIC
>
> +UINT32
>
> +EFIAPI
>
> +SdMmioOr32 (
>
> +  IN      UINTN                     Address,
>
> +  IN      UINT32                    OrData
>
> +  )
>
> +{
>
> +  return SdMmioWrite32 (Address, MmioRead32 (Address) | OrData);
>
> +}
>
> +
>
> +STATIC
>
> +UINT32
>
> +EFIAPI
>
> +SdMmioAnd32 (
>
> +  IN      UINTN                     Address,
>
> +  IN      UINT32                    AndData
>
> +  )
>
> +{
>
> +  return SdMmioWrite32 (Address, MmioRead32 (Address) & AndData);
>
> +}
>
> +
>
> +STATIC
>
> +UINT32
>
> +EFIAPI
>
> +SdMmioAndThenOr32 (
>
> +  IN      UINTN                     Address,
>
> +  IN      UINT32                    AndData,
>
> +  IN      UINT32                    OrData
>
> +  )
>
> +{
>
> +  return SdMmioWrite32 (Address, (MmioRead32 (Address) & AndData) | OrData);
>
> +}
>
> +
>
> +
>
>   /**
>
>      These SD commands are optional, according to the SD Spec
>
>   **/
>
> @@ -175,7 +225,9 @@ SoftReset (
>     IN UINT32 Mask
>
>     )
>
>   {
>
> -  MmioOr32 (MMCHS_SYSCTL, Mask);
>
> +  DEBUG ((DEBUG_MMCHOST_SD, "SoftReset with mask 0x%x\n", Mask));
>
> +
>
> +  SdMmioOr32 (MMCHS_SYSCTL, Mask);
>
>     if (PollRegisterWithMask (MMCHS_SYSCTL, Mask, 0) == EFI_TIMEOUT) {
>
>       DEBUG ((DEBUG_ERROR, "Failed to SoftReset with mask 0x%x\n", Mask));
>
>       return EFI_TIMEOUT;
>
> @@ -326,29 +378,29 @@ MMCSendCommand (
>     }
>
>
>
>     if (IsAppCmd && MmcCmd == ACMD22) {
>
> -    MmioWrite32 (MMCHS_BLK, 4);
>
> +    SdMmioWrite32 (MMCHS_BLK, 4);
>
>     } else if (IsAppCmd && MmcCmd == ACMD51) {
>
> -    MmioWrite32 (MMCHS_BLK, 8);
>
> +    SdMmioWrite32 (MMCHS_BLK, 8);
>
>     } else if (!IsAppCmd && MmcCmd == CMD6) {
>
> -    MmioWrite32 (MMCHS_BLK, 64);
>
> +    SdMmioWrite32 (MMCHS_BLK, 64);
>
>     } else if (IsADTCCmd) {
>
> -    MmioWrite32 (MMCHS_BLK, BLEN_512BYTES);
>
> +    SdMmioWrite32 (MMCHS_BLK, BLEN_512BYTES);
>
>     }
>
>
>
>     // Set Data timeout counter value to max value.
>
> -  MmioAndThenOr32 (MMCHS_SYSCTL, (UINT32) ~DTO_MASK, DTO_VAL);
>
> +  SdMmioAndThenOr32 (MMCHS_SYSCTL, (UINT32) ~DTO_MASK, DTO_VAL);
>
>
>
>     //
>
>     // Clear Interrupt Status Register, but not the Card Inserted bit
>
>     // to avoid messing with card detection logic.
>
>     //
>
> -  MmioWrite32 (MMCHS_INT_STAT, ALL_EN & ~(CARD_INS));
>
> +  SdMmioWrite32 (MMCHS_INT_STAT, ALL_EN & ~(CARD_INS));
>
>
>
>     // Set command argument register
>
> -  MmioWrite32 (MMCHS_ARG, Argument);
>
> +  SdMmioWrite32 (MMCHS_ARG, Argument);
>
>
>
>     // Send the command
>
> -  MmioWrite32 (MMCHS_CMD, MmcCmd);
>
> +  SdMmioWrite32 (MMCHS_CMD, MmcCmd);
>
>
>
>     // Check for the command status.
>
>     while (RetryCount < MAX_RETRY_COUNT) {
>
> @@ -373,7 +425,7 @@ MMCSendCommand (
>
>
>       // Check if command is completed.
>
>       if ((MmcStatus & CC) == CC) {
>
> -      MmioWrite32 (MMCHS_INT_STAT, CC);
>
> +      SdMmioWrite32 (MMCHS_INT_STAT, CC);
>
>         break;
>
>       }
>
>
>
> @@ -428,6 +480,21 @@ MMCNotifyState (
>           return Status;
>
>         }
>
>
>
> +      DEBUG ((DEBUG_MMCHOST_SD, "ArasanMMCHost: CAP %X CAPH %X\n", MmioRead32(MMCHS_CAPA),MmioRead32(MMCHS_CUR_CAPA)));
>
> +
>
> +      // Lets switch to card detect test mode.
>
> +      SdMmioOr32 (MMCHS_HCTL, BIT7|BIT6);
>
> +
>
> +      // set card voltage
>
> +      SdMmioAnd32 (MMCHS_HCTL, ~SDBP_ON);
>
> +      SdMmioAndThenOr32 (MMCHS_HCTL, (UINT32) ~SDBP_MASK, SDVS_3_3_V);
>
> +      SdMmioOr32 (MMCHS_HCTL, SDBP_ON);
>
> +
>
> +      DEBUG ((DEBUG_MMCHOST_SD, "ArasanMMCHost: AC12 %X HCTL %X\n", MmioRead32(MMCHS_AC12),MmioRead32(MMCHS_HCTL)));
>
> +
>
> +      // First turn off the clock
>
> +      SdMmioAnd32 (MMCHS_SYSCTL, ~CEN);
>
> +
>
>         // Attempt to set the clock to 400Khz which is the expected initialization speed
>
>         Status = CalculateClockFrequencyDivisor (400000, &Divisor, NULL);
>
>         if (EFI_ERROR (Status)) {
>
> @@ -436,10 +503,15 @@ MMCNotifyState (
>         }
>
>
>
>         // Set Data Timeout Counter value, set clock frequency, enable internal clock
>
> -      MmioOr32 (MMCHS_SYSCTL, DTO_VAL | Divisor | CEN | ICS | ICE);
>
> +      SdMmioOr32 (MMCHS_SYSCTL, DTO_VAL | Divisor | CEN | ICS | ICE);
>
> +      SdMmioOr32 (MMCHS_HCTL, SDBP_ON);
>
> +      // wait for ICS
>
> +      while ((MmioRead32 (MMCHS_SYSCTL) & ICS_MASK) != ICS);
>
> +
>
> +      DEBUG ((DEBUG_MMCHOST_SD, "ArasanMMCHost: AC12 %X HCTL %X\n", MmioRead32(MMCHS_AC12),MmioRead32(MMCHS_HCTL)));
>
>
>
>         // Enable interrupts
>
> -      MmioWrite32 (MMCHS_IE, ALL_EN);
>
> +      SdMmioWrite32 (MMCHS_IE, ALL_EN);
>
>       }
>
>       break;
>
>     case MmcIdleState:
>
> @@ -452,7 +524,7 @@ MMCNotifyState (
>       ClockFrequency = 25000000;
>
>
>
>       // First turn off the clock
>
> -    MmioAnd32 (MMCHS_SYSCTL, ~CEN);
>
> +    SdMmioAnd32 (MMCHS_SYSCTL, ~CEN);
>
>
>
>       Status = CalculateClockFrequencyDivisor (ClockFrequency, &Divisor, NULL);
>
>       if (EFI_ERROR (Status)) {
>
> @@ -462,13 +534,13 @@ MMCNotifyState (
>       }
>
>
>
>       // Setup new divisor
>
> -    MmioAndThenOr32 (MMCHS_SYSCTL, (UINT32) ~CLKD_MASK, Divisor);
>
> +    SdMmioAndThenOr32 (MMCHS_SYSCTL, (UINT32) ~CLKD_MASK, Divisor);
>
>
>
>       // Wait for the clock to stabilise
>
>       while ((MmioRead32 (MMCHS_SYSCTL) & ICS_MASK) != ICS);
>
>
>
>       // Set Data Timeout Counter value, set clock frequency, enable internal clock
>
> -    MmioOr32 (MMCHS_SYSCTL, CEN);
>
> +    SdMmioOr32 (MMCHS_SYSCTL, CEN);
>
>       break;
>
>     case MmcTransferState:
>
>       break;
>
> @@ -635,7 +707,7 @@ MMCReadBlockData (
>       while (RetryCount < MAX_RETRY_COUNT) {
>
>         MmcStatus = MmioRead32 (MMCHS_INT_STAT);
>
>         if ((MmcStatus & BRR) != 0) {
>
> -        MmioWrite32 (MMCHS_INT_STAT, BRR);
>
> +        SdMmioWrite32 (MMCHS_INT_STAT, BRR);
>
>           /*
>
>            * Data is ready.
>
>            */
>
> @@ -662,7 +734,7 @@ MMCReadBlockData (
>       gBS->Stall (STALL_AFTER_READ_US);
>
>     }
>
>
>
> -  MmioWrite32 (MMCHS_INT_STAT, BRR);
>
> +  SdMmioWrite32 (MMCHS_INT_STAT, BRR);
>
>     return EFI_SUCCESS;
>
>   }
>
>
>
> @@ -699,13 +771,13 @@ MMCWriteBlockData (
>       while (RetryCount < MAX_RETRY_COUNT) {
>
>         MmcStatus = MmioRead32 (MMCHS_INT_STAT);
>
>         if ((MmcStatus & BWR) != 0) {
>
> -        MmioWrite32 (MMCHS_INT_STAT, BWR);
>
> +        SdMmioWrite32 (MMCHS_INT_STAT, BWR);
>
>           /*
>
>            * Can write data.
>
>            */
>
>           mFwProtocol->SetLed (TRUE);
>
>           for (Count = 0; Count < BlockLen; Count += 4, Buffer++) {
>
> -          MmioWrite32 (MMCHS_DATA, *Buffer);
>
> +          SdMmioWrite32 (MMCHS_DATA, *Buffer);
>
>           }
>
>
>
>           mFwProtocol->SetLed (FALSE);
>
> @@ -726,7 +798,7 @@ MMCWriteBlockData (
>       gBS->Stall (STALL_AFTER_WRITE_US);
>
>     }
>
>
>
> -  MmioWrite32 (MMCHS_INT_STAT, BWR);
>
> +  SdMmioWrite32 (MMCHS_INT_STAT, BWR);
>
>     return EFI_SUCCESS;
>
>   }
>
>
>
> diff --git a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.h b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.h
> index 6cd600f738..e94606cc5b 100644
> --- a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.h
> +++ b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.h
> @@ -37,6 +37,7 @@
>   #define STALL_AFTER_REC_RESP_US (50)
>
>   #define STALL_AFTER_WRITE_US (200)
>
>   #define STALL_AFTER_READ_US (20)
>
> +#define STALL_AFTER_REG_WRITE_US (10)
>
>   #define STALL_AFTER_RETRY_US (20)
>
>
>
>   #define MAX_DIVISOR_VALUE 1023
>
> --
> 2.13.7
>
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#68816): https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F68816&amp;data=04%7C01%7Cawarkentin%40vmware.com%7Cc2bc5f44461c484196e008d8a129b6e2%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637436547836525703%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=tUEGHmsaEG%2B0dy6qn0GgCAK97aZTtnKXntkcSft%2BH14%3D&amp;reserved=0
> Mute This Topic: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.io%2Fmt%2F78964893%2F4387333&amp;data=04%7C01%7Cawarkentin%40vmware.com%7Cc2bc5f44461c484196e008d8a129b6e2%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637436547836525703%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=8vOe%2BmdxokTuXKlAqSNuZWXFWDgGLA%2FFD24Ao1J6Qws%3D&amp;reserved=0
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Funsub&amp;data=04%7C01%7Cawarkentin%40vmware.com%7Cc2bc5f44461c484196e008d8a129b6e2%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637436547836525703%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=zBnexfVSW9erLRPOWZCJIT%2FqLnF%2Bw95A4snliXN8HHg%3D&amp;reserved=0 [awarkentin@vmware.com]
> -=-=-=-=-=-=
>
>
>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#68881): https://edk2.groups.io/g/devel/message/68881
Mute This Topic: https://groups.io/mt/78964893/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-