<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> <div dir="ltr">Yep, all understood and acknowledged. No actionable items - lgtm. <div> </div> <div> <blockquote type="cite"> <div dir="auto"> <div> <div> <div>Reviewed-by: Andrei Warkentin <awarkentin@vmware.com></div> </div> </div> </div> </blockquote> </div> <div> </div> </div> --- Отправлено из <a href="https://whatisworkspaceone.com/boxer">Workspace ONE Boxer</a> <div> <div class="null" dir="auto">15 декабря 2020 г. в 12:46:24 PM GMT-6 Jeremy Linton <jeremy.linton@arm.com> пишет: </div> <blockquote type="cite" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;" class="null"> <div class="null" dir="auto"> <div class="null"> <meta name="Generator" content="Microsoft Exchange Server" class="null">  <div class="null"> <div class="null">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): <a href="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" target="_BLANK" class="null"> 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</a> > Mute This Topic: <a href="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" target="_BLANK" class="null"> 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</a> > Group Owner: devel+owner@edk2.groups.io > Unsubscribe: <a href="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" target="_BLANK" class="null"> 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</a> [awarkentin@vmware.com] > -=-=-=-=-=-= > > > </div> </div> </div> </div> </blockquote> </div> </body> </html> <div width="1" style="color:white;clear:both">_._,_._,_</div> <hr> Groups.io Links: You receive all messages sent to this group. <a target="_blank" href="https://edk2.groups.io/g/devel/message/68881">View/Reply Online (#68881)</a> | | <a target="_blank" href="https://groups.io/mt/78964893/1813853">Mute This Topic</a> | <a href="https://edk2.groups.io/g/devel/post">New Topic</a> <a href="https://edk2.groups.io/g/devel/editsub/1813853">Your Subscription</a> | <a href="mailto:devel+owner@edk2.groups.io">Contact Group Owner</a> | <a href="https://edk2.groups.io/g/devel/unsub">Unsubscribe</a> [edk2-devel-archive@redhat.com] <div width="1" style="color:white;clear:both">_._,_._,_</div>