[edk2-devel] [PATCH 4/7] Platform/RaspberryPi/Arasan: Add write delay and voltage/clock config
Andrei Warkentin
awarkentin at vmware.com
Tue Dec 15 18:52:47 UTC 2020
Yep, all understood and acknowledged. No actionable items - lgtm.
Reviewed-by: Andrei Warkentin <awarkentin at vmware.com>
---
Отправлено из Workspace ONE Boxer<https://whatisworkspaceone.com/boxer>
15 декабря 2020 г. в 12:46:24 PM GMT-6 Jeremy Linton <jeremy.linton at 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 at vmware.com>
> ________________________________
> From: devel at edk2.groups.io <devel at edk2.groups.io> on behalf of Jeremy Linton via groups.io <jeremy.linton=arm.com at groups.io>
> Sent: Monday, December 14, 2020 5:23 PM
> To: devel at edk2.groups.io <devel at edk2.groups.io>
> Cc: ard.biesheuvel at arm.com <ard.biesheuvel at arm.com>; leif at nuviainc.com <leif at nuviainc.com>; pete at akeo.ie <pete at akeo.ie>; andrey.warkentin at gmail.com <andrey.warkentin at gmail.com>; samer.el-haj-mahmoud at arm.com <samer.el-haj-mahmoud at arm.com>; Jeremy Linton <jeremy.linton at 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 at 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&data=04%7C01%7Cawarkentin%40vmware.com%7Cc2bc5f44461c484196e008d8a129b6e2%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637436547836525703%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=tUEGHmsaEG%2B0dy6qn0GgCAK97aZTtnKXntkcSft%2BH14%3D&reserved=0
> Mute This Topic: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.io%2Fmt%2F78964893%2F4387333&data=04%7C01%7Cawarkentin%40vmware.com%7Cc2bc5f44461c484196e008d8a129b6e2%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637436547836525703%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=8vOe%2BmdxokTuXKlAqSNuZWXFWDgGLA%2FFD24Ao1J6Qws%3D&reserved=0
> Group Owner: devel+owner at edk2.groups.io
> Unsubscribe: https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Funsub&data=04%7C01%7Cawarkentin%40vmware.com%7Cc2bc5f44461c484196e008d8a129b6e2%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637436547836525703%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=zBnexfVSW9erLRPOWZCJIT%2FqLnF%2Bw95A4snliXN8HHg%3D&reserved=0 [awarkentin at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20201215/d7aafa57/attachment.htm>
More information about the edk2-devel-archive
mailing list