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

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Tue, 24 Dec 2019 at 03:52, Wu, Hao A <hao.a.wu@intel.com> wrote:
>
> > -----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
>

I don't have any objections to this patch, but I'd like Marcin to
confirm, given that this override callback is used on the Marvell
platforms.

>
> >
> > 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
>