Re: [edk2-platforms][PATCH v3 1/1] Platform/RaspberryPi/Drivers: Add SD/(e)MMC card detection

["Leif Lindholm" <leif@nuviainc.com>]

On Mon, Jul 13, 2020 at 12:16:20 +0100, Pete Batard wrote:
> The Raspberry Pi 3 and Pi 4 platforms (with latest EEPROM) can boot
> straight from USB, without the need for an SD card being present.
> However, the IsCardPresent () calls from the ArasanMmcHost and SdHost
> drivers are currently hardwired to return TRUE, which results in
> straight to USB boot failing due to the SD drivers looping on
> errors while trying to poke at a non-existent card...
> 
> Ideally, we would use the Card Detect signal from the uSD slot, to
> report on the presence or absence of a card, but the Raspberry Pi
> Foundation did not wire those signals in the Pi 2 and subsequent
> models, leaving us with only potentially interfering SD commands
> as means to perform card detection.
> 
> As a result of this, we are left with no other choice but limit
> detection to occurring only once, prior to formal SD card init,
> and then return the detected value for subsequent calls. This,
> however, is more than good enough for the intended purpose, which
> is to allow straight to USB boot. The sequence is a simplified
> variant of the identification code in MmcDxe.
> 
> Tested on Raspberry Pi 2B, 3B and CM3 (for both SD controllers)
> and Pi 4 (for Arasan, as that's the only controller available today)
> 
> Addresses pftf/RPi3#13, pftf/RPi3#14, pftf/RPi4#37.
> 
> Co-authored-by: Andrei Warkentin <andrey.warkentin@gmail.com>
> Signed-off-by: Pete Batard <pete@akeo.ie>

Some minor style comments below, I'm happy to fix them before pushing
if you're OK with these:

> ---
>  Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c | 86 ++++++++++++++++++--
>  Platform/RaspberryPi/Drivers/SdHostDxe/SdHostDxe.c               | 75 +++++++++++++++--
>  Platform/RaspberryPi/Include/Protocol/RpiMmcHost.h               |  6 ++
>  3 files changed, 150 insertions(+), 17 deletions(-)
> 
> diff --git a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c
> index 6d706af6f276..d2a8ffddbb66 100644
> --- a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c
> +++ b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c
> @@ -11,7 +11,8 @@
>  
>  #define DEBUG_MMCHOST_SD DEBUG_VERBOSE
>  
> -BOOLEAN PreviousIsCardPresent = FALSE;
> +BOOLEAN CardIsPresent = FALSE;
> +CARD_DETECT_STATE CardDetectState = CardDetectRequired;

Global variables, so add 'm' prefix?
Also, add STATIC (which also matches SdHostDxe version)?

>  UINT32 LastExecutedCommand = (UINT32) -1;
>  
>  STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol;
> @@ -239,14 +240,6 @@ CalculateClockFrequencyDivisor (
>    return EFI_SUCCESS;
>  }
>  
> -BOOLEAN
> -MMCIsCardPresent (
> -  IN EFI_MMC_HOST_PROTOCOL *This
> -)
> -{
> -  return TRUE;
> -}
> -
>  BOOLEAN
>  MMCIsReadOnly (
>    IN EFI_MMC_HOST_PROTOCOL *This
> @@ -418,6 +411,10 @@ MMCNotifyState (
>  
>    DEBUG ((DEBUG_MMCHOST_SD, "ArasanMMCHost: MMCNotifyState(State: %d)\n", State));
>  
> +  // Stall all operations except init until card detection has occurred.
> +  if (State != MmcHwInitializationState && CardDetectState != CardDetectCompleted)
> +    return EFI_NOT_READY;
> +

Add {}?

>    switch (State) {
>    case MmcHwInitializationState:
>      {
> @@ -489,6 +486,77 @@ MMCNotifyState (
>    return EFI_SUCCESS;
>  }
>  
> +BOOLEAN
> +MMCIsCardPresent (
> +  IN EFI_MMC_HOST_PROTOCOL *This
> +)
> +{
> +  EFI_STATUS Status;
> +
> +  //
> +  // If we are already in progress (we may get concurrent calls)
> +  // or completed the detection, just return the current value.
> +  //
> +  if (CardDetectState != CardDetectRequired)
> +    return CardIsPresent;

Add {}?

> +
> +  CardDetectState = CardDetectInProgress;
> +  CardIsPresent = FALSE;
> +
> +  //
> +  // The two following commands should succeed even if no card is present.
> +  //
> +  Status = MMCNotifyState (This, MmcHwInitializationState);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "MMCIsCardPresent: Error MmcHwInitializationState, Status=%r.\n", Status));
> +    // If we failed init, go back to requiring card detection
> +    CardDetectState = CardDetectRequired;
> +    return FALSE;
> +  }
> +
> +  Status = MMCSendCommand (This, MMC_CMD0, 0);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "MMCIsCardPresent: CMD0 Error, Status=%r.\n", Status));
> +    goto out;
> +  }
> +
> +  //
> +  // CMD8 should tell us if an SD card is present.
> +  //
> +  Status = MMCSendCommand (This, MMC_CMD8, CMD8_SD_ARG);
> +  if (!EFI_ERROR (Status)) {
> +     DEBUG ((DEBUG_INFO, "MMCIsCardPresent: Maybe SD card detected.\n"));
> +     CardIsPresent = TRUE;
> +     goto out;
> +  }
> +
> +  //
> +  // MMC/eMMC won't accept CMD8, but we can try CMD1.
> +  //
> +  Status = MMCSendCommand (This, MMC_CMD1, EMMC_CMD1_CAPACITY_GREATER_THAN_2GB);
> +  if (!EFI_ERROR (Status)) {
> +     DEBUG ((DEBUG_INFO, "MMCIsCardPresent: Maybe MMC card detected.\n"));
> +     CardIsPresent = TRUE;
> +     goto out;
> +  }
> +
> +  //
> +  // SDIO?
> +  //
> +  Status = MMCSendCommand (This, MMC_CMD5, 0);
> +  if (!EFI_ERROR (Status)) {
> +     DEBUG ((DEBUG_INFO, "MMCIsCardPresent: Maybe SDIO card detected.\n"));
> +     CardIsPresent = TRUE;
> +     goto out;
> +  }
> +
> +  DEBUG ((DEBUG_INFO, "MMCIsCardPresent: Not detected, Status=%r.\n", Status));
> +
> +out:
> +  CardDetectState = CardDetectCompleted;
> +  return CardIsPresent;
> +}
> +
>  EFI_STATUS
>  MMCReceiveResponse (
>    IN EFI_MMC_HOST_PROTOCOL    *This,
> diff --git a/Platform/RaspberryPi/Drivers/SdHostDxe/SdHostDxe.c b/Platform/RaspberryPi/Drivers/SdHostDxe/SdHostDxe.c
> index 2f31c5eb8c46..aac8b34c4bf4 100644
> --- a/Platform/RaspberryPi/Drivers/SdHostDxe/SdHostDxe.c
> +++ b/Platform/RaspberryPi/Drivers/SdHostDxe/SdHostDxe.c
> @@ -64,6 +64,8 @@ STATIC CONST CHAR8 *mFsmState[] = { "identmode", "datamode", "readdata",
>                                      "genpulses", "writewait2", "?",
>                                      "startpowdown" };
>  #endif /* NDEBUG */
> +STATIC BOOLEAN CardIsPresent = FALSE;
> +STATIC CARD_DETECT_STATE CardDetectState = CardDetectRequired;

Add 'm' prefix?

>  STATIC UINT32 mLastGoodCmd = MMC_GET_INDX (MMC_CMD0);
>  
>  STATIC inline BOOLEAN
> @@ -264,14 +266,6 @@ SdHostSetClockFrequency (
>    return Status;
>  }
>  
> -STATIC BOOLEAN
> -SdIsCardPresent (
> -  IN EFI_MMC_HOST_PROTOCOL *This
> -  )
> -{
> -  return TRUE;
> -}
> -
>  STATIC BOOLEAN
>  SdIsReadOnly (
>    IN EFI_MMC_HOST_PROTOCOL *This
> @@ -639,6 +633,10 @@ SdNotifyState (
>  {
>    DEBUG ((DEBUG_MMCHOST_SD, "SdHost: SdNotifyState(State: %d) ", State));
>  
> +  // Stall all operations except init until card detection has occurred.
> +  if (State != MmcHwInitializationState && CardDetectState != CardDetectCompleted)
> +    return EFI_NOT_READY;
> +

Add {}?

>    switch (State) {
>    case MmcHwInitializationState:
>      DEBUG ((DEBUG_MMCHOST_SD, "MmcHwInitializationState\n", State));
> @@ -718,6 +716,67 @@ SdNotifyState (
>    return EFI_SUCCESS;
>  }
>  
> +STATIC BOOLEAN
> +SdIsCardPresent (
> +  IN EFI_MMC_HOST_PROTOCOL *This
> +  )
> +{
> +  EFI_STATUS Status;
> +
> +  //
> +  // If we are already in progress (we may get concurrent calls)
> +  // or completed the detection, just return the current value.
> +  //
> +  if (CardDetectState != CardDetectRequired)
> +    return CardIsPresent;

Add {}?

I.e. in total, fold in:
diff --git a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c
index d2a8ffddbb66..88e9126e3549 100644
--- a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c
+++ b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.c
@@ -11,8 +11,8 @@
 
 #define DEBUG_MMCHOST_SD DEBUG_VERBOSE
 
-BOOLEAN CardIsPresent = FALSE;
-CARD_DETECT_STATE CardDetectState = CardDetectRequired;
+STATIC BOOLEAN mCardIsPresent = FALSE;
+STATIC CARD_DETECT_STATE mCardDetectState = CardDetectRequired;
 UINT32 LastExecutedCommand = (UINT32) -1;
 
 STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL *mFwProtocol;
@@ -412,8 +412,9 @@ MMCNotifyState (
   DEBUG ((DEBUG_MMCHOST_SD, "ArasanMMCHost: MMCNotifyState(State: %d)\n", State));
 
   // Stall all operations except init until card detection has occurred.
-  if (State != MmcHwInitializationState && CardDetectState != CardDetectCompleted)
+  if (State != MmcHwInitializationState && mCardDetectState != CardDetectCompleted) {
     return EFI_NOT_READY;
+  }
 
   switch (State) {
   case MmcHwInitializationState:
@@ -497,11 +498,12 @@ MMCIsCardPresent (
   // If we are already in progress (we may get concurrent calls)
   // or completed the detection, just return the current value.
   //
-  if (CardDetectState != CardDetectRequired)
-    return CardIsPresent;
+  if (mCardDetectState != CardDetectRequired) {
+    return mCardIsPresent;
+  }
 
-  CardDetectState = CardDetectInProgress;
-  CardIsPresent = FALSE;
+  mCardDetectState = CardDetectInProgress;
+  mCardIsPresent = FALSE;
 
   //
   // The two following commands should succeed even if no card is present.
@@ -510,7 +512,7 @@ MMCIsCardPresent (
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR, "MMCIsCardPresent: Error MmcHwInitializationState, Status=%r.\n", Status));
     // If we failed init, go back to requiring card detection
-    CardDetectState = CardDetectRequired;
+    mCardDetectState = CardDetectRequired;
     return FALSE;
   }
 
@@ -526,7 +528,7 @@ MMCIsCardPresent (
   Status = MMCSendCommand (This, MMC_CMD8, CMD8_SD_ARG);
   if (!EFI_ERROR (Status)) {
      DEBUG ((DEBUG_INFO, "MMCIsCardPresent: Maybe SD card detected.\n"));
-     CardIsPresent = TRUE;
+     mCardIsPresent = TRUE;
      goto out;
   }
 
@@ -536,7 +538,7 @@ MMCIsCardPresent (
   Status = MMCSendCommand (This, MMC_CMD1, EMMC_CMD1_CAPACITY_GREATER_THAN_2GB);
   if (!EFI_ERROR (Status)) {
      DEBUG ((DEBUG_INFO, "MMCIsCardPresent: Maybe MMC card detected.\n"));
-     CardIsPresent = TRUE;
+     mCardIsPresent = TRUE;
      goto out;
   }
 
@@ -546,15 +548,15 @@ MMCIsCardPresent (
   Status = MMCSendCommand (This, MMC_CMD5, 0);
   if (!EFI_ERROR (Status)) {
      DEBUG ((DEBUG_INFO, "MMCIsCardPresent: Maybe SDIO card detected.\n"));
-     CardIsPresent = TRUE;
+     mCardIsPresent = TRUE;
      goto out;
   }
 
   DEBUG ((DEBUG_INFO, "MMCIsCardPresent: Not detected, Status=%r.\n", Status));
 
 out:
-  CardDetectState = CardDetectCompleted;
-  return CardIsPresent;
+  mCardDetectState = CardDetectCompleted;
+  return mCardIsPresent;
 }
 
 EFI_STATUS
diff --git a/Platform/RaspberryPi/Drivers/SdHostDxe/SdHostDxe.c b/Platform/RaspberryPi/Drivers/SdHostDxe/SdHostDxe.c
index aac8b34c4bf4..0fd1ac6e8985 100644
--- a/Platform/RaspberryPi/Drivers/SdHostDxe/SdHostDxe.c
+++ b/Platform/RaspberryPi/Drivers/SdHostDxe/SdHostDxe.c
@@ -64,8 +64,8 @@ STATIC CONST CHAR8 *mFsmState[] = { "identmode", "datamode", "readdata",
                                     "genpulses", "writewait2", "?",
                                     "startpowdown" };
 #endif /* NDEBUG */
-STATIC BOOLEAN CardIsPresent = FALSE;
-STATIC CARD_DETECT_STATE CardDetectState = CardDetectRequired;
+STATIC BOOLEAN mCardIsPresent = FALSE;
+STATIC CARD_DETECT_STATE mCardDetectState = CardDetectRequired;
 STATIC UINT32 mLastGoodCmd = MMC_GET_INDX (MMC_CMD0);
 
 STATIC inline BOOLEAN
@@ -634,8 +634,9 @@ SdNotifyState (
   DEBUG ((DEBUG_MMCHOST_SD, "SdHost: SdNotifyState(State: %d) ", State));
 
   // Stall all operations except init until card detection has occurred.
-  if (State != MmcHwInitializationState && CardDetectState != CardDetectCompleted)
+  if (State != MmcHwInitializationState && mCardDetectState != CardDetectCompleted) {
     return EFI_NOT_READY;
+  }
 
   switch (State) {
   case MmcHwInitializationState:
@@ -727,11 +728,12 @@ SdIsCardPresent (
   // If we are already in progress (we may get concurrent calls)
   // or completed the detection, just return the current value.
   //
-  if (CardDetectState != CardDetectRequired)
-    return CardIsPresent;
+  if (mCardDetectState != CardDetectRequired) {
+    return mCardIsPresent;
+  }
 
-  CardDetectState = CardDetectInProgress;
-  CardIsPresent = FALSE;
+  mCardDetectState = CardDetectInProgress;
+  mCardIsPresent = FALSE;
 
   //
   // The two following commands should succeed even if no card is present.
@@ -740,7 +742,7 @@ SdIsCardPresent (
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR, "SdIsCardPresent: Error MmcHwInitializationState, Status=%r.\n", Status));
     // If we failed init, go back to requiring card detection
-    CardDetectState = CardDetectRequired;
+    mCardDetectState = CardDetectRequired;
     return FALSE;
   }
 
@@ -756,7 +758,7 @@ SdIsCardPresent (
   Status = SdSendCommand (This, MMC_CMD8, CMD8_SD_ARG);
   if (!EFI_ERROR (Status)) {
      DEBUG ((DEBUG_INFO, "SdIsCardPresent: Maybe SD card detected.\n"));
-     CardIsPresent = TRUE;
+     mCardIsPresent = TRUE;
      goto out;
   }
 
@@ -766,15 +768,15 @@ SdIsCardPresent (
   Status = SdSendCommand (This, MMC_CMD1, EMMC_CMD1_CAPACITY_GREATER_THAN_2GB);
   if (!EFI_ERROR (Status)) {
      DEBUG ((DEBUG_INFO, "SdIsCardPresent: Maybe MMC card detected.\n"));
-     CardIsPresent = TRUE;
+     mCardIsPresent = TRUE;
      goto out;
   }
 
   DEBUG ((DEBUG_INFO, "SdIsCardPresent: Not detected, Status=%r.\n", Status));
 
 out:
-  CardDetectState = CardDetectCompleted;
-  return CardIsPresent;
+  mCardDetectState = CardDetectCompleted;
+  return mCardIsPresent;
 }
 
 BOOLEAN

(Compile tested to ensure I didn't screw up the renamings.)

/
    Leif

> +
> +  CardDetectState = CardDetectInProgress;
> +  CardIsPresent = FALSE;
> +
> +  //
> +  // The two following commands should succeed even if no card is present.
> +  //
> +  Status = SdNotifyState (This, MmcHwInitializationState);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "SdIsCardPresent: Error MmcHwInitializationState, Status=%r.\n", Status));
> +    // If we failed init, go back to requiring card detection
> +    CardDetectState = CardDetectRequired;
> +    return FALSE;
> +  }
> +
> +  Status = SdSendCommand (This, MMC_CMD0, 0);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "SdIsCardPresent: CMD0 Error, Status=%r.\n", Status));
> +    goto out;
> +  }
> +
> +  //
> +  // CMD8 should tell us if an SD card is present.
> +  //
> +  Status = SdSendCommand (This, MMC_CMD8, CMD8_SD_ARG);
> +  if (!EFI_ERROR (Status)) {
> +     DEBUG ((DEBUG_INFO, "SdIsCardPresent: Maybe SD card detected.\n"));
> +     CardIsPresent = TRUE;
> +     goto out;
> +  }
> +
> +  //
> +  // MMC/eMMC won't accept CMD8, but we can try CMD1.
> +  //
> +  Status = SdSendCommand (This, MMC_CMD1, EMMC_CMD1_CAPACITY_GREATER_THAN_2GB);
> +  if (!EFI_ERROR (Status)) {
> +     DEBUG ((DEBUG_INFO, "SdIsCardPresent: Maybe MMC card detected.\n"));
> +     CardIsPresent = TRUE;
> +     goto out;
> +  }
> +
> +  DEBUG ((DEBUG_INFO, "SdIsCardPresent: Not detected, Status=%r.\n", Status));
> +
> +out:
> +  CardDetectState = CardDetectCompleted;
> +  return CardIsPresent;
> +}
> +
>  BOOLEAN
>  SdIsMultiBlock (
>    IN EFI_MMC_HOST_PROTOCOL *This
> diff --git a/Platform/RaspberryPi/Include/Protocol/RpiMmcHost.h b/Platform/RaspberryPi/Include/Protocol/RpiMmcHost.h
> index c558e00bf500..78514a31bc4e 100644
> --- a/Platform/RaspberryPi/Include/Protocol/RpiMmcHost.h
> +++ b/Platform/RaspberryPi/Include/Protocol/RpiMmcHost.h
> @@ -82,6 +82,12 @@ typedef enum _MMC_STATE {
>      MmcDisconnectState,
>  } MMC_STATE;
>  
> +typedef enum _CARD_DETECT_STATE {
> +    CardDetectRequired = 0,
> +    CardDetectInProgress,
> +    CardDetectCompleted
> +} CARD_DETECT_STATE;
> +
>  #define EMMCBACKWARD         (0)
>  #define EMMCHS26             (1 << 0)      // High-Speed @26MHz at rated device voltages
>  #define EMMCHS52             (1 << 1)      // High-Speed @52MHz at rated device voltages
> -- 
> 2.21.0.windows.1
>