On Tue, 24 Dec 2019 at 03:52, Wu, Hao A <hao.a...@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...@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...@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 (#52547): https://edk2.groups.io/g/devel/message/52547
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]
-=-=-=-=-=-=-=-=-=-=-=-
