Re: [PATCH v2 3/4] MdeModulePkg/SdMmcPciHcDxe: Add SwitchClockFreqPost to SdMmcOverride

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

> -----Original Message-----
> From: Marcin Wojtas [mailto:mw@semihalf.com]
> Sent: Friday, November 02, 2018 5:39 PM
> To: Wu, Hao A
> Cc: edk2-devel-01; Tomasz Michalec; nadavh@marvell.com; Gao, Liming;
> Kinney, Michael D
> Subject: Re: [edk2] [PATCH v2 3/4] MdeModulePkg/SdMmcPciHcDxe: Add
> SwitchClockFreqPost to SdMmcOverride
> 
> Hi Hao,
> 
> czw., 1 lis 2018 o 08:06 Wu, Hao A <hao.a.wu@intel.com> napisał(a):
> >
> > > -----Original Message-----
> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > > Marcin Wojtas
> > > Sent: Friday, October 05, 2018 9:25 PM
> > > To: edk2-devel@lists.01.org
> > > Cc: Tian, Feng; tm@semihalf.com; Wu, Hao A; nadavh@marvell.com; Gao,
> > > Liming; Kinney, Michael D
> > > Subject: [edk2] [PATCH v2 3/4] MdeModulePkg/SdMmcPciHcDxe: Add
> > > SwitchClockFreqPost to SdMmcOverride
> > >
> > > From: Tomasz Michalec <tm@semihalf.com>
> > >
> > > Some SD Host Controlers need to do additional opperations after clock
> > > frequency switch.
> > >
> > > This patch add new callback type to NotifyPhase of the SdMmcOverride
> > > protocol. It is called after EmmcSwitchClockFreq and SdMmcHcClockSupply.
> >
> > Hi Marcin,
> >
> > Just curious, I had a quick glance at the implementation of the
> > XenonSwitchClockFreqPost() in your platform part changes. Are those
> operations
> > within the function mandatory during the HC initialization? Are they mainly for
> > performance or stability consideration?
> 
> As for Marvellt he Xenon controller is pretty complicated IP, which
> consists of standard Sd/Mmc part and the dedicated PHY, that's
> responsible for signal integrity for all bus modes. It requires
> additional configuration, depending on the mode.

Got it.

> 
> >
> > I am wondering if this kind of customization is common among the SD &
> eMMC devices.
> 
> Well, in Linux this clock tuning after switching to certain bus mode.
> This driver simply does it in 'set_ios' callback, whose custom
> implementation (platform-specific code surrounding generic
> sdhci_set_ios call) is _very_ common among all drivers/mmc/host
> drivers.
> 
> Do you have any objections to the patch itself, given above explanation?

No, thanks for the detailed explanation.

Best Regards,
Hao Wu

> 
> Best regards,
> Marcin
> 
> >
> > Best Regards,
> > Hao Wu
> >
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > > ---
> > >  MdeModulePkg/Include/Protocol/SdMmcOverride.h   |  1 +
> > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 60
> > > ++++++++++++++++++++
> > >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c   | 18 ++++++
> > >  3 files changed, 79 insertions(+)
> > >
> > > diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > > b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > > index 25db98a..d9daada 100644
> > > --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > > +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > > @@ -33,6 +33,7 @@ typedef enum {
> > >    EdkiiSdMmcInitHostPre,
> > >    EdkiiSdMmcInitHostPost,
> > >    EdkiiSdMmcUhsSignaling,
> > > +  EdkiiSdMmcSwitchClockFreqPost,
> > >  } EDKII_SD_MMC_PHASE_TYPE;
> > >
> > >  /**
> > > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > > index 05bd4a0..7e75283 100755
> > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > > @@ -796,6 +796,27 @@ EmmcSwitchToHighSpeed (
> > >
> > >    HsTiming = 1;
> > >    Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming,
> > > ClockFreq);
> > > +  if (EFI_ERROR (Status)) {
> > > +    return Status;
> > > +  }
> > > +
> > > +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> > > +    Status = mOverride->NotifyPhase (
> > > +                          Private->ControllerHandle,
> > > +                          Slot,
> > > +                          EdkiiSdMmcSwitchClockFreqPost,
> > > +                          &Timing
> > > +                          );
> > > +    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;
> > >  }
> > > @@ -905,6 +926,24 @@ EmmcSwitchToHS200 (
> > >      return Status;
> > >    }
> > >
> > > +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> > > +    Status = mOverride->NotifyPhase (
> > > +                          Private->ControllerHandle,
> > > +                          Slot,
> > > +                          EdkiiSdMmcSwitchClockFreqPost,
> > > +                          &Timing
> > > +                          );
> > > +    if (EFI_ERROR (Status)) {
> > > +      DEBUG ((
> > > +        DEBUG_ERROR,
> > > +        "%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
> > > +        __FUNCTION__,
> > > +        Status
> > > +        ));
> > > +      return Status;
> > > +    }
> > > +  }
> > > +
> > >    Status = EmmcTuningClkForHs200 (PciIo, PassThru, Slot, BusWidth);
> > >
> > >    return Status;
> > > @@ -989,6 +1028,27 @@ EmmcSwitchToHS400 (
> > >
> > >    HsTiming = 3;
> > >    Status = EmmcSwitchClockFreq (PciIo, PassThru, Slot, Rca, HsTiming,
> > > ClockFreq);
> > > +  if (EFI_ERROR (Status)) {
> > > +    return Status;
> > > +  }
> > > +
> > > +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> > > +    Status = mOverride->NotifyPhase (
> > > +                          Private->ControllerHandle,
> > > +                          Slot,
> > > +                          EdkiiSdMmcSwitchClockFreqPost,
> > > +                          &Timing
> > > +                          );
> > > +    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 5645a71..057a4e2 100644
> > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> > > @@ -887,6 +887,24 @@ SdCardSetBusMode (
> > >      return Status;
> > >    }
> > >
> > > +  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
> > > +    Status = mOverride->NotifyPhase (
> > > +                          Private->ControllerHandle,
> > > +                          Slot,
> > > +                          EdkiiSdMmcSwitchClockFreqPost,
> > > +                          &Timing
> > > +                          );
> > > +    if (EFI_ERROR (Status)) {
> > > +      DEBUG ((
> > > +        DEBUG_ERROR,
> > > +        "%a: SD/MMC switch clock freq post notifier callback failed - %r\n",
> > > +        __FUNCTION__,
> > > +        Status
> > > +        ));
> > > +      return Status;
> > > +    }
> > > +  }
> > > +
> > >    if ((AccessMode == 3) || ((AccessMode == 2) && (Capability-
> > > >TuningSDR50 != 0))) {
> > >      Status = SdCardTuningClock (PciIo, PassThru, Slot);
> > >      if (EFI_ERROR (Status)) {
> > > --
> > > 2.7.4
> > >
> > > _______________________________________________
> > > edk2-devel mailing list
> > > edk2-devel@lists.01.org
> > > https://lists.01.org/mailman/listinfo/edk2-devel