From: "Leif Lindholm" <leif@nuviainc.com>
To: Pete Batard <pete@akeo.ie>
Cc: devel@edk2.groups.io, ard.biesheuvel@arm.com,
awarkentin@vmware.com,
Andrei Warkentin <andrey.warkentin@gmail.com>
Subject: Re: [edk2-platforms][PATCH v3 1/1] Platform/RaspberryPi/Drivers: Add SD/(e)MMC card detection
Date: Tue, 14 Jul 2020 15:44:00 +0100 [thread overview]
Message-ID: <20200714144400.GY12303@vanye> (raw)
In-Reply-To: <fbc75f21-6d37-d7f8-8868-724a23d664e4@akeo.ie>
On Tue, Jul 14, 2020 at 14:49:21 +0100, Pete Batard wrote:
> 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.
Thanks! With that
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
Pushed as 225426271bb0.
> 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
> > >
>
prev parent reply other threads:[~2020-07-14 14:44 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-13 11:16 [edk2-platforms][PATCH v3 0/1] Platform/RaspberryPi/Drivers: Add SD/(e)MMC card detection Pete Batard
2020-07-13 11:16 ` [edk2-platforms][PATCH v3 1/1] " Pete Batard
2020-07-14 13:25 ` Leif Lindholm
2020-07-14 13:49 ` Pete Batard
2020-07-14 14:44 ` Leif Lindholm [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200714144400.GY12303@vanye \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox