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

["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%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]
> -=-=-=-=-=-=
> 
> 
>