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

["Pete Batard" <pete@akeo.ie>]

On 2020.07.14 14:25, Leif Lindholm wrote:
> 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:

I agree with the proposed changes. Thanks for volunteering to fix these.

I just tested your diff with a Pi 4, for good measure, and everything 
looks good.

Regards,

/Pete

> 
>> ---
>>   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
>>