Re: [PATCH v2 4/4] MdeModulePkg/SdMmcPciHcDxe: Allow overriding base clock frequency

[Marcin Wojtas <mw@semihalf.com>]

Hi Hao,


czw., 1 lis 2018 o 08:11 Wu, Hao A <hao.a.wu@intel.com> napisał(a):
>
> Hi Marcin,
>
> > -----Original Message-----
> > From: Marcin Wojtas [mailto:mw@semihalf.com]
> > Sent: Friday, October 05, 2018 9:25 PM
> > To: edk2-devel@lists.01.org
> > Cc: Tian, Feng; Kinney, Michael D; Gao, Liming; leif.lindholm@linaro.org; Wu,
> > Hao A; ard.biesheuvel@linaro.org; nadavh@marvell.com;
> > mw@semihalf.com; jsd@semihalf.com; tm@semihalf.com
> > Subject: [PATCH v2 4/4] MdeModulePkg/SdMmcPciHcDxe: Allow overriding
> > base clock frequency
> >
> > Some SdMmc host controllers are run by clocks with different
> > frequency than it is reflected in Capabilities Register 1.
> > It is allowed by SDHCI specification ver. 4.2 - if BaseClkFreq
> > field value of the Capability Register 1 is zero, the clock
> > frequency must be obtained via another method.
> >
> > Because the bitfield is only 8 bits wide, a maximum value
> > that could be obtained from hardware is 255MHz.
> > In case the actual frequency exceeds 255MHz, the 8-bit BaseClkFreq
> > member of SD_MMC_HC_SLOT_CAP structure occurs to be not sufficient
> > to be used for setting the clock speed in SdMmcHcClockSupply
> > function.
> >
> > This patch adds new UINT32 array ('BaseClkFreq[]') to
> > SD_MMC_HC_PRIVATE_DATA structure for specifying
> > the input clock speed for each slot of the host controller.
> > All routines that are used for clock configuration are
> > updated accordingly.
> >
> > This patch also adds new IN OUT BaseClockFreq field
> > in the Capability callback of the SdMmcOverride,
> > protocol which allows to update BaseClkFreq value.
> >
> > The patch reuses original commit from edk2-platforms:
> > 20f6f144d3a8 ("Marvell/Drivers: XenonDxe: Allow overriding base clock
> > frequency")
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> > ---
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h |  6 +++++
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h   | 12 +++++----
> >  MdeModulePkg/Include/Protocol/SdMmcOverride.h      |  5 +++-
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c    |  4 +--
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c      |  4 +--
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 18
> > +++++++++++--
> >  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c   | 27
> > +++++++++++---------
> >  7 files changed, 52 insertions(+), 24 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> > index c683600..8c1a589 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h
> > @@ -118,6 +118,12 @@ typedef struct {
> >    UINT64                              MaxCurrent[SD_MMC_HC_MAX_SLOT];
> >
> >    UINT32                              ControllerVersion;
> > +
> > +  //
> > +  // Some controllers may require to override base clock frequency
> > +  // value stored in Capabilities Register 1.
> > +  //
> > +  UINT32                              BaseClkFreq[SD_MMC_HC_MAX_SLOT];
> >  } SD_MMC_HC_PRIVATE_DATA;
> >
> >  #define SD_MMC_HC_TRB_SIG             SIGNATURE_32 ('T', 'R', 'B', 'T')
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > index a03160d..f01ba21 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> > @@ -173,12 +173,14 @@ typedef struct {
> >
> >    @param[in]  Slot            The slot number of the SD card to send the
> > command to.
> >    @param[in]  Capability      The buffer to store the capability data.
> > +  @param[in]  BaseClkFreq     The base clock frequency of host controller in
> > MHz.
> >
> >  **/
> >  VOID
> >  DumpCapabilityReg (
> >    IN UINT8                Slot,
> > -  IN SD_MMC_HC_SLOT_CAP   *Capability
> > +  IN SD_MMC_HC_SLOT_CAP   *Capability,
> > +  IN UINT32               BaseClkFreq
> >    );
> >
> >  /**
> > @@ -431,7 +433,7 @@ SdMmcHcStopClock (
> >    @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] Capability     The capability of the slot.
> > +  @param[in] BaseClkFreq    The base clock frequency of host controller in
> > MHz.
> >
> >    @retval EFI_SUCCESS       The clock is supplied successfully.
> >    @retval Others            The clock isn't supplied successfully.
> > @@ -442,7 +444,7 @@ SdMmcHcClockSupply (
> >    IN EFI_PCI_IO_PROTOCOL    *PciIo,
> >    IN UINT8                  Slot,
> >    IN UINT64                 ClockFreq,
> > -  IN SD_MMC_HC_SLOT_CAP     Capability
> > +  IN UINT32                 BaseClkFreq
> >    );
> >
> >  /**
> > @@ -490,7 +492,7 @@ SdMmcHcSetBusWidth (
> >
> >    @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] Capability     The capability of the slot.
> > +  @param[in] BaseClkFreq    The base clock frequency of host controller in
> > MHz.
> >
> >    @retval EFI_SUCCESS       The clock is supplied successfully.
> >    @retval Others            The clock isn't supplied successfully.
> > @@ -500,7 +502,7 @@ EFI_STATUS
> >  SdMmcHcInitClockFreq (
> >    IN EFI_PCI_IO_PROTOCOL    *PciIo,
> >    IN UINT8                  Slot,
> > -  IN SD_MMC_HC_SLOT_CAP     Capability
> > +  IN UINT32                 BaseClkFreq
> >    );
> >
> >  /**
> > diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > index d9daada..27023d3 100644
> > --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> > @@ -43,6 +43,8 @@ typedef enum {
> >    @param[in]      ControllerHandle      The EFI_HANDLE of the controller.
> >    @param[in]      Slot                  The 0 based slot index.
> >    @param[in,out]  SdMmcHcSlotCapability The SDHCI capability structure.
> > +  @param[in,out]  BaseClkFreq           The base clock frequency value that
> > +                                        optionally can be updated.
> >
> >    @retval EFI_SUCCESS           The override function completed successfully.
> >    @retval EFI_NOT_FOUND         The specified controller or slot does not
> > exist.
> > @@ -54,7 +56,8 @@ EFI_STATUS
> >  (EFIAPI * EDKII_SD_MMC_CAPABILITY) (
> >    IN      EFI_HANDLE                      ControllerHandle,
> >    IN      UINT8                           Slot,
> > -  IN  OUT VOID                            *SdMmcHcSlotCapability
> > +  IN OUT  VOID                            *SdMmcHcSlotCapability,
> > +  IN OUT  UINT32                          *BaseClkFreq
> >    );
> >
> >  /**
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > index 7e75283..27ccd63 100755
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c
> > @@ -705,7 +705,7 @@ EmmcSwitchClockFreq (
> >    //
> >    // Convert the clock freq unit from MHz to KHz.
> >    //
> > -  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private-
> > >Capability[Slot]);
> > +  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private-
> > >BaseClkFreq[Slot]);
> >
> >    return Status;
> >  }
> > @@ -1099,7 +1099,7 @@ EmmcSetBusMode (
> >      return Status;
> >    }
> >
> > -  ASSERT (Private->Capability[Slot].BaseClkFreq != 0);
> > +  ASSERT (Private->BaseClkFreq[Slot] != 0);
> >    //
> >    // Check if the Host Controller support 8bits bus width.
> >    //
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> > index 057a4e2..9ea13be 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c
> > @@ -882,7 +882,7 @@ SdCardSetBusMode (
> >      }
> >    }
> >
> > -  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, *Capability);
> > +  Status = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private-
> > >BaseClkFreq[Slot]);
> >    if (EFI_ERROR (Status)) {
> >      return Status;
> >    }
> > @@ -1081,7 +1081,7 @@ SdCardIdentification (
> >          goto Error;
> >        }
> >
> > -      SdMmcHcInitClockFreq (PciIo, Slot, Private->Capability[Slot]);
> > +      SdMmcHcInitClockFreq (PciIo, Slot, Private->BaseClkFreq[Slot]);
> >
> >        gBS->Stall (1000);
> >
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> > index bf9869d..d7cc0ce 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> > @@ -625,18 +625,32 @@ SdMmcPciHcDriverBindingStart (
> >      if (EFI_ERROR (Status)) {
> >        continue;
> >      }
> > +
> > +    Private->BaseClkFreq[Slot] = Private->Capability[Slot].BaseClkFreq;
> > +
> >      if (mOverride != NULL && mOverride->Capability != NULL) {
> >        Status = mOverride->Capability (
> >                              Controller,
> >                              Slot,
> > -                            &Private->Capability[Slot]);
> > +                            &Private->Capability[Slot],
> > +                            &Private->BaseClkFreq[Slot]
> > +                            );
> >        if (EFI_ERROR (Status)) {
> >          DEBUG ((DEBUG_WARN, "%a: Failed to override capability - %r\n",
> >            __FUNCTION__, Status));
> >          continue;
> >        }
> >      }
> > -    DumpCapabilityReg (Slot, &Private->Capability[Slot]);
> > +
> > +    //
> > +    // 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.
> > +    //
> > +    ASSERT (Private->BaseClkFreq[Slot] != 0);
>
> I think the ASSERT here can be dropped. This value will be handled within
> function SdMmcHcInitClockFreq(). And the above newly added comments can be used
> in SdMmcHcInitClockFreq() instead.

Right, I missed it - I will remove the ASSERT here, as the protection
against 0 value is already there.

>
> > +
> > +    DumpCapabilityReg (Slot, &Private->Capability[Slot], Private-
> > >BaseClkFreq[Slot]);
>
> My thought on this one is that you can leave DumpCapabilityReg() unchanged and
> append a DEBUG() call with DEBUG_INFO level to output the value of
> Private->BaseClkFreq for a slot.

Ok, I will append the print separately.

Thanks,
Marcin

> >
> >      Support64BitDma &= Private->Capability[Slot].SysBus64;
> >
> > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > index 38d6202..9f50754 100644
> > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> > @@ -22,12 +22,14 @@
> >
> >    @param[in]  Slot            The slot number of the SD card to send the
> > command to.
> >    @param[in]  Capability      The buffer to store the capability data.
> > +  @param[in]  BaseClkFreq     The base clock frequency of host controller in
> > MHz.
> >
> >  **/
> >  VOID
> >  DumpCapabilityReg (
> >    IN UINT8                Slot,
> > -  IN SD_MMC_HC_SLOT_CAP   *Capability
> > +  IN SD_MMC_HC_SLOT_CAP   *Capability,
> > +  IN UINT32               BaseClkFreq
> >    )
> >  {
> >    //
> > @@ -35,7 +37,10 @@ DumpCapabilityReg (
> >    //
> >    DEBUG ((DEBUG_INFO, " == Slot [%d] Capability is 0x%x ==\n", Slot,
> > Capability));
> >    DEBUG ((DEBUG_INFO, "   Timeout Clk Freq  %d%a\n", Capability-
> > >TimeoutFreq, (Capability->TimeoutUnit) ? "MHz" : "KHz"));
> > -  DEBUG ((DEBUG_INFO, "   Base Clk Freq     %dMHz\n", Capability-
> > >BaseClkFreq));
> > +  if (Capability->BaseClkFreq != BaseClkFreq) {
> > +    DEBUG ((DEBUG_INFO, "   Controller register value overriden:\n"));
> > +  }
> > +  DEBUG ((DEBUG_INFO, "   Base Clk Freq     %dMHz\n", BaseClkFreq));
> >    DEBUG ((DEBUG_INFO, "   Max Blk Len       %dbytes\n", 512 * (1 <<
> > Capability->MaxBlkLen)));
> >    DEBUG ((DEBUG_INFO, "   8-bit Support     %a\n", Capability->BusWidth8 ?
> > "TRUE" : "FALSE"));
> >    DEBUG ((DEBUG_INFO, "   ADMA2 Support     %a\n", Capability->Adma2 ?
> > "TRUE" : "FALSE"));
> > @@ -721,7 +726,7 @@ SdMmcHcStopClock (
> >    @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] Capability     The capability of the slot.
> > +  @param[in] BaseClkFreq    The base clock frequency of host controller in
> > MHz.
> >
> >    @retval EFI_SUCCESS       The clock is supplied successfully.
> >    @retval Others            The clock isn't supplied successfully.
> > @@ -732,11 +737,10 @@ SdMmcHcClockSupply (
> >    IN EFI_PCI_IO_PROTOCOL    *PciIo,
> >    IN UINT8                  Slot,
> >    IN UINT64                 ClockFreq,
> > -  IN SD_MMC_HC_SLOT_CAP     Capability
> > +  IN UINT32                 BaseClkFreq
> >    )
> >  {
> >    EFI_STATUS                Status;
> > -  UINT32                    BaseClkFreq;
> >    UINT32                    SettingFreq;
> >    UINT32                    Divisor;
> >    UINT32                    Remainder;
> > @@ -746,9 +750,8 @@ SdMmcHcClockSupply (
> >    //
> >    // Calculate a divisor for SD clock frequency
> >    //
> > -  ASSERT (Capability.BaseClkFreq != 0);
> > +  ASSERT (BaseClkFreq != 0);
> >
> > -  BaseClkFreq = Capability.BaseClkFreq;
> >    if (ClockFreq == 0) {
> >      return EFI_INVALID_PARAMETER;
> >    }
> > @@ -939,7 +942,7 @@ SdMmcHcSetBusWidth (
> >
> >    @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] Capability     The capability of the slot.
> > +  @param[in] BaseClkFreq    The base clock frequency of host controller in
> > MHz.
> >
> >    @retval EFI_SUCCESS       The clock is supplied successfully.
> >    @retval Others            The clock isn't supplied successfully.
> > @@ -949,7 +952,7 @@ EFI_STATUS
> >  SdMmcHcInitClockFreq (
> >    IN EFI_PCI_IO_PROTOCOL    *PciIo,
> >    IN UINT8                  Slot,
> > -  IN SD_MMC_HC_SLOT_CAP     Capability
> > +  IN UINT32                 BaseClkFreq
> >    )
> >  {
> >    EFI_STATUS                Status;
> > @@ -958,7 +961,7 @@ SdMmcHcInitClockFreq (
> >    //
> >    // Calculate a divisor for SD clock frequency
> >    //
> > -  if (Capability.BaseClkFreq == 0) {
> > +  if (BaseClkFreq == 0) {
> >      //
> >      // Don't support get Base Clock Frequency information via another
> > method
> >      //
> > @@ -968,7 +971,7 @@ SdMmcHcInitClockFreq (
> >    // Supply 400KHz clock frequency at initialization phase.
> >    //
> >    InitFreq = 400;
> > -  Status = SdMmcHcClockSupply (PciIo, Slot, InitFreq, Capability);
> > +  Status = SdMmcHcClockSupply (PciIo, Slot, InitFreq, BaseClkFreq);
> >    return Status;
> >  }
> >
> > @@ -1102,7 +1105,7 @@ SdMmcHcInitHost (
> >    PciIo = Private->PciIo;
> >    Capability = Private->Capability[Slot];
> >
> > -  Status = SdMmcHcInitClockFreq (PciIo, Slot, Capability);
> > +  Status = SdMmcHcInitClockFreq (PciIo, Slot, Private->BaseClkFreq[Slot]);
> >    if (EFI_ERROR (Status)) {
> >      return Status;
> >    }
> > --
> > 2.7.4
>