Re: [PATCH 1/2] SdMmcPciHcDxe: Send EdkiiSdMmcSwitchClockFreq after SD clock start

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----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.wu@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.wu@intel.com>
> Cc: Marcin Wojtas <mw@semihalf.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> 
> Signed-off-by: Mateusz Albecki <mateusz.albecki@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