> -----Original Message-----
> From: Albecki, Mateusz
> Sent: Saturday, December 21, 2019 1:13 AM
> To: devel@edk2.groups.io
> Cc: Albecki, Mateusz; Wu, Hao A; Marcin Wojtas; Gao, Zhichao; Gao, Liming
> Subject: [PATCH 1/2] SdMmcPciHcDxe: Send EdkiiSdMmcSwitchClockFreq after
> SD clock start
Hello Mateusz,
Just a minor format comment, how about changing the subject to:
MdeModulePkg/SdMmcPciHcDxe: Hook SwitchClockFreq after SD clock start
If there is no other major concern from other reviewers for the patch, I will
handle this when pushing the patch.
Reviewed-by: Hao A Wu <hao.a...@intel.com>
Best Regards,
Hao Wu
>
> For eMMC modules we used to notify the platform about frequency
> change only after sending CMD13 which meant that platform
> might not get a chance to apply required post frequency
> change fixes to get the clock stable. To fix this
> notification has been moved to SdMmcHcClockSupply function
> just after we start the SD clock. During first time setup
> the notification won't be sent to avoid changing old behavior.
>
> Cc: Hao A Wu <hao.a...@intel.com>
> Cc: Marcin Wojtas <m...@semihalf.com>
> Cc: Zhichao Gao <zhichao....@intel.com>
> Cc: Liming Gao <liming....@intel.com>
>
> Signed-off-by: Mateusz Albecki <mateusz.albe...@intel.com>
> ---
> MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 20 +---
> MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c | 28 ++----
> MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h | 24 +++++
> MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 107 +++++++++--
> ----------
> MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h | 44 ---------
> 5 files changed, 81 insertions(+), 142 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> index 082904ccc5..776c0e796c 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> @@ -727,7 +727,7 @@ EmmcSwitchBusTiming (
> //
> // Convert the clock freq unit from MHz to KHz.
> //
> - Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private-
> >BaseClkFreq[Slot], Private->ControllerVersion[Slot]);
> + Status = SdMmcHcClockSupply (Private, Slot, BusTiming, FALSE, ClockFreq *
> 1000);
> if (EFI_ERROR (Status)) {
> return Status;
> }
> @@ -745,24 +745,6 @@ EmmcSwitchBusTiming (
> return EFI_DEVICE_ERROR;
> }
>
> - if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> - Status = mOverride->NotifyPhase (
> - Private->ControllerHandle,
> - Slot,
> - EdkiiSdMmcSwitchClockFreqPost,
> - &BusTiming
> - );
> - if (EFI_ERROR (Status)) {
> - DEBUG ((
> - DEBUG_ERROR,
> - "%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
> - __FUNCTION__,
> - Status
> - ));
> - return Status;
> - }
> - }
> -
> return Status;
> }
>
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> index 336baade9e..d63dc54e8c 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> @@ -1145,29 +1145,11 @@ SdCardSetBusMode (
> return Status;
> }
>
> - Status = SdMmcHcClockSupply (PciIo, Slot, BusMode.ClockFreq * 1000,
> Private->BaseClkFreq[Slot], Private->ControllerVersion[Slot]);
> + Status = SdMmcHcClockSupply (Private, Slot, BusMode.BusTiming, FALSE,
> BusMode.ClockFreq * 1000);
> if (EFI_ERROR (Status)) {
> return Status;
> }
>
> - if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> - Status = mOverride->NotifyPhase (
> - Private->ControllerHandle,
> - Slot,
> - EdkiiSdMmcSwitchClockFreqPost,
> - &BusMode.BusTiming
> - );
> - if (EFI_ERROR (Status)) {
> - DEBUG ((
> - DEBUG_ERROR,
> - "%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
> - __FUNCTION__,
> - Status
> - ));
> - return Status;
> - }
> - }
> -
> if ((BusMode.BusTiming == SdMmcUhsSdr104) || ((BusMode.BusTiming ==
> SdMmcUhsSdr50) && (Capability->TuningSDR50 != 0))) {
> Status = SdCardTuningClock (PciIo, PassThru, Slot);
> if (EFI_ERROR (Status)) {
> @@ -1345,7 +1327,13 @@ SdCardIdentification (
> goto Error;
> }
>
> - SdMmcHcInitClockFreq (PciIo, Slot, Private->BaseClkFreq[Slot], Private-
> >ControllerVersion[Slot]);
> + //
> + // Restart the clock with first time parameters.
> + // NOTE: it is not required to actually restart the clock
> + // and go through internal clock setup again. Some time
> + // could be saved if we simply started the SD clock.
> + //
> + SdMmcHcClockSupply (Private, Slot, 0, TRUE, 400);
>
> gBS->Stall (1000);
>
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> index c29e48767e..0304960132 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> @@ -796,6 +796,30 @@ SdCardIdentification (
> IN UINT8 Slot
> );
>
> +/**
> + SD/MMC card clock supply.
> +
> + Refer to SD Host Controller Simplified spec 3.0 Section 3.2.1 for details.
> +
> + @param[in] Private A pointer to the SD_MMC_HC_PRIVATE_DATA
> instance.
> + @param[in] Slot The slot number of the SD card to send the
> command
> to.
> + @param[in] BusTiming BusTiming at which the frequency change is done.
> + @param[in] FirstTimeSetup Flag to indicate whether the clock is being
> setup
> for the first time.
> + @param[in] ClockFreq The max clock frequency to be set. The unit is
> KHz.
> +
> + @retval EFI_SUCCESS The clock is supplied successfully.
> + @retval Others The clock isn't supplied successfully.
> +
> +**/
> +EFI_STATUS
> +SdMmcHcClockSupply (
> + IN SD_MMC_HC_PRIVATE_DATA *Private,
> + IN UINT8 Slot,
> + IN SD_MMC_BUS_MODE BusTiming,
> + IN BOOLEAN FirstTimeSetup,
> + IN UINT64 ClockFreq
> + );
> +
> /**
> Software reset the specified SD/MMC host controller.
>
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> index b9d04e0f17..f667264c5e 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> @@ -763,11 +763,11 @@ SdMmcHcStopClock (
>
> Refer to SD Host Controller Simplified spec 3.0 Section 3.2.1 for details.
>
> - @param[in] PciIo The PCI IO protocol instance.
> - @param[in] Slot The slot number of the SD card to send the
> command
> to.
> - @param[in] ClockFreq The max clock frequency to be set. The unit is
> KHz.
> - @param[in] BaseClkFreq The base clock frequency of host controller in
> MHz.
> - @param[in] ControllerVer The version of host controller.
> + @param[in] Private A pointer to the SD_MMC_HC_PRIVATE_DATA
> instance.
> + @param[in] Slot The slot number of the SD card to send the
> command
> to.
> + @param[in] BusTiming BusTiming at which the frequency change is done.
> + @param[in] FirstTimeSetup Flag to indicate whether the clock is being
> setup
> for the first time.
> + @param[in] ClockFreq The max clock frequency to be set. The unit is
> KHz.
>
> @retval EFI_SUCCESS The clock is supplied successfully.
> @retval Others The clock isn't supplied successfully.
> @@ -775,11 +775,11 @@ SdMmcHcStopClock (
> **/
> EFI_STATUS
> SdMmcHcClockSupply (
> - IN EFI_PCI_IO_PROTOCOL *PciIo,
> - IN UINT8 Slot,
> - IN UINT64 ClockFreq,
> - IN UINT32 BaseClkFreq,
> - IN UINT16 ControllerVer
> + IN SD_MMC_HC_PRIVATE_DATA *Private,
> + IN UINT8 Slot,
> + IN SD_MMC_BUS_MODE BusTiming,
> + IN BOOLEAN FirstTimeSetup,
> + IN UINT64 ClockFreq
> )
> {
> EFI_STATUS Status;
> @@ -787,13 +787,15 @@ SdMmcHcClockSupply (
> UINT32 Divisor;
> UINT32 Remainder;
> UINT16 ClockCtrl;
> + UINT32 BaseClkFreq;
> + UINT16 ControllerVer;
> + EFI_PCI_IO_PROTOCOL *PciIo;
>
> - //
> - // Calculate a divisor for SD clock frequency
> - //
> - ASSERT (BaseClkFreq != 0);
> + PciIo = Private->PciIo;
> + BaseClkFreq = Private->BaseClkFreq[Slot];
> + ControllerVer = Private->ControllerVersion[Slot];
>
> - if (ClockFreq == 0) {
> + if (BaseClkFreq == 0 || ClockFreq == 0) {
> return EFI_INVALID_PARAMETER;
> }
>
> @@ -883,6 +885,29 @@ SdMmcHcClockSupply (
> ClockCtrl = BIT2;
> Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_CLOCK_CTRL, sizeof
> (ClockCtrl), &ClockCtrl);
>
> + //
> + // We don't notify the platform on first time setup to avoid changing
> + // legacy behavior. During first time setup we also don't know what type
> + // of the card slot it is and which enum value of BusTiming applies.
> + //
> + if (!FirstTimeSetup && mOverride != NULL && mOverride->NotifyPhase !=
> NULL) {
> + Status = mOverride->NotifyPhase (
> + Private->ControllerHandle,
> + Slot,
> + EdkiiSdMmcSwitchClockFreqPost,
> + &BusTiming
> + );
> + if (EFI_ERROR (Status)) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
> + __FUNCTION__,
> + Status
> + ));
> + return Status;
> + }
> + }
> +
> return Status;
> }
>
> @@ -1038,49 +1063,6 @@ SdMmcHcInitV4Enhancements (
> return EFI_SUCCESS;
> }
>
> -/**
> - Supply SD/MMC card with lowest clock frequency at initialization.
> -
> - @param[in] PciIo The PCI IO protocol instance.
> - @param[in] Slot The slot number of the SD card to send the
> command
> to.
> - @param[in] BaseClkFreq The base clock frequency of host controller in
> MHz.
> - @param[in] ControllerVer The version of host controller.
> -
> - @retval EFI_SUCCESS The clock is supplied successfully.
> - @retval Others The clock isn't supplied successfully.
> -
> -**/
> -EFI_STATUS
> -SdMmcHcInitClockFreq (
> - IN EFI_PCI_IO_PROTOCOL *PciIo,
> - IN UINT8 Slot,
> - IN UINT32 BaseClkFreq,
> - IN UINT16 ControllerVer
> - )
> -{
> - EFI_STATUS Status;
> - UINT32 InitFreq;
> -
> - //
> - // According to SDHCI specification ver. 4.2, BaseClkFreq field value of
> - // the Capability Register 1 can be zero, which means a need for obtaining
> - // the clock frequency via another method. Fail in case it is not updated
> - // by SW at this point.
> - //
> - if (BaseClkFreq == 0) {
> - //
> - // Don't support get Base Clock Frequency information via another method
> - //
> - return EFI_UNSUPPORTED;
> - }
> - //
> - // Supply 400KHz clock frequency at initialization phase.
> - //
> - InitFreq = 400;
> - Status = SdMmcHcClockSupply (PciIo, Slot, InitFreq, BaseClkFreq,
> ControllerVer);
> - return Status;
> -}
> -
> /**
> Supply SD/MMC card with maximum voltage at initialization.
>
> @@ -1216,7 +1198,14 @@ SdMmcHcInitHost (
> return Status;
> }
>
> - Status = SdMmcHcInitClockFreq (PciIo, Slot, Private->BaseClkFreq[Slot],
> Private->ControllerVersion[Slot]);
> + //
> + // Perform first time clock setup with 400 KHz frequency.
> + // We send the 0 as the BusTiming value because at this time
> + // we still do not know the slot type and which enum value will apply.
> + // Since it is a first time setup SdMmcHcClockSupply won't notify
> + // the platofrm driver anyway so it doesn't matter.
> + //
> + Status = SdMmcHcClockSupply (Private, Slot, 0, TRUE, 400);
> if (EFI_ERROR (Status)) {
> return Status;
> }
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> index 088c70451c..826e851b04 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> @@ -478,30 +478,6 @@ SdMmcHcStopClock (
> IN UINT8 Slot
> );
>
> -/**
> - SD/MMC card clock supply.
> -
> - Refer to SD Host Controller Simplified spec 3.0 Section 3.2.1 for details.
> -
> - @param[in] PciIo The PCI IO protocol instance.
> - @param[in] Slot The slot number of the SD card to send the
> command
> to.
> - @param[in] ClockFreq The max clock frequency to be set. The unit is
> KHz.
> - @param[in] BaseClkFreq The base clock frequency of host controller in
> MHz.
> - @param[in] ControllerVer The version of host controller.
> -
> - @retval EFI_SUCCESS The clock is supplied successfully.
> - @retval Others The clock isn't supplied successfully.
> -
> -**/
> -EFI_STATUS
> -SdMmcHcClockSupply (
> - IN EFI_PCI_IO_PROTOCOL *PciIo,
> - IN UINT8 Slot,
> - IN UINT64 ClockFreq,
> - IN UINT32 BaseClkFreq,
> - IN UINT16 ControllerVer
> - );
> -
> /**
> SD/MMC bus power control.
>
> @@ -542,26 +518,6 @@ SdMmcHcSetBusWidth (
> IN UINT16 BusWidth
> );
>
> -/**
> - Supply SD/MMC card with lowest clock frequency at initialization.
> -
> - @param[in] PciIo The PCI IO protocol instance.
> - @param[in] Slot The slot number of the SD card to send the
> command
> to.
> - @param[in] BaseClkFreq The base clock frequency of host controller in
> MHz.
> - @param[in] ControllerVer The version of host controller.
> -
> - @retval EFI_SUCCESS The clock is supplied successfully.
> - @retval Others The clock isn't supplied successfully.
> -
> -**/
> -EFI_STATUS
> -SdMmcHcInitClockFreq (
> - IN EFI_PCI_IO_PROTOCOL *PciIo,
> - IN UINT8 Slot,
> - IN UINT32 BaseClkFreq,
> - IN UINT16 ControllerVer
> - );
> -
> /**
> Supply SD/MMC card with maximum voltage at initialization.
>
> --
> 2.14.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#52521): https://edk2.groups.io/g/devel/message/52521
Mute This Topic: https://groups.io/mt/68852796/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
