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

[Marcin Wojtas <mw@semihalf.com>]

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.

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

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