Hi,
wt., 24 gru 2019 o 10:54 Ard Biesheuvel <ard.biesheu...@linaro.org> napisał(a):
>
> 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.
>
I'll check the branch today or tomorrow.
Best regards,
Marcin
> >
> > >
> > > 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 (#52608): https://edk2.groups.io/g/devel/message/52608
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]
-=-=-=-=-=-=-=-=-=-=-=-
