From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Marcin Wojtas <mw@semihalf.com>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
"Leif Lindholm" <leif.lindholm@linaro.org>,
"Wu, Hao A" <hao.a.wu@intel.com>,
"Kinney, Michael D" <michael.d.kinney@intel.com>,
"Gao, Liming" <liming.gao@intel.com>,
"Nadav Haklai" <nadavh@marvell.com>,
"Jan Dąbroś" <jsd@semihalf.com>,
"Tomasz Michalec" <tm@semihalf.com>,
"Grzegorz Jaszczyk" <jaz@semihalf.com>
Subject: Re: [PATCH v3 4/4] MdeModulePkg/SdMmcPciHcDxe: Allow overriding base clock frequency
Date: Thu, 8 Nov 2018 12:10:43 +0100 [thread overview]
Message-ID: <CAKv+Gu959Qs04YqwM2b84FrvTjxZ8OHNatxkXv4tjzgpVxrPBQ@mail.gmail.com> (raw)
In-Reply-To: <1541642255-15602-5-git-send-email-mw@semihalf.com>
On 8 November 2018 at 02:57, Marcin Wojtas <mw@semihalf.com> wrote:
> 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>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h | 6 +++++
> MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h | 8 +++----
> MdeModulePkg/Include/Protocol/SdMmcOverride.h | 7 ++++--
> MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 4 ++--
> MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c | 4 ++--
> MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 13 ++++++++++-
> MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 23 ++++++++++----------
> 7 files changed, 43 insertions(+), 22 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 1a11d51..8eefc31 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h
> @@ -423,7 +423,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.
> @@ -434,7 +434,7 @@ SdMmcHcClockSupply (
> IN EFI_PCI_IO_PROTOCOL *PciIo,
> IN UINT8 Slot,
> IN UINT64 ClockFreq,
> - IN SD_MMC_HC_SLOT_CAP Capability
> + IN UINT32 BaseClkFreq
> );
>
> /**
> @@ -482,7 +482,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.
> @@ -492,7 +492,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 6160b5b..0aaf258 100644
> --- a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> @@ -22,7 +22,7 @@
> #define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
> { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83, 0x23, 0x23 } }
>
> -#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION 0x1
> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION 0x2
>
> typedef struct _EDKII_SD_MMC_OVERRIDE EDKII_SD_MMC_OVERRIDE;
>
> @@ -58,6 +58,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.
> @@ -69,7 +71,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 6fc6871..0393fd4 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;
> }
> @@ -1098,7 +1098,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 5408bbc..a9d0d3a 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;
> }
> @@ -1082,7 +1082,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..a87f8de 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> @@ -625,11 +625,16 @@ 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));
> @@ -637,6 +642,12 @@ SdMmcPciHcDriverBindingStart (
> }
> }
> DumpCapabilityReg (Slot, &Private->Capability[Slot]);
> + DEBUG ((
> + DEBUG_INFO,
> + "Slot[%d] Base Clock Frequency: %dMHz\n",
> + Slot,
> + Private->BaseClkFreq[Slot]
> + ));
>
> Support64BitDma &= Private->Capability[Slot].SysBus64;
>
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> index 85aa625..040a959 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c
> @@ -721,7 +721,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 +732,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 +745,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;
> }
> @@ -940,7 +938,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.
> @@ -950,16 +948,19 @@ 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;
> UINT32 InitFreq;
>
> //
> - // Calculate a divisor for SD clock frequency
> + // 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 (Capability.BaseClkFreq == 0) {
> + if (BaseClkFreq == 0) {
> //
> // Don't support get Base Clock Frequency information via another method
> //
> @@ -969,7 +970,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;
> }
>
> @@ -1103,7 +1104,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
>
prev parent reply other threads:[~2018-11-08 11:10 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-08 1:57 [PATCH v3 0/4] SdMmcOverride extension Marcin Wojtas
2018-11-08 1:57 ` [PATCH v3 1/4] MdeModulePkg/SdMmcPciHcDxe: Add an optional parameter in NotifyPhase Marcin Wojtas
2018-11-08 1:57 ` [PATCH v3 2/4] MdeModulePkg/SdMmcPciHcDxe: Add UhsSignaling to SdMmcOverride protocol Marcin Wojtas
2018-11-08 11:06 ` Ard Biesheuvel
2018-11-08 11:15 ` Marcin Wojtas
2018-11-08 1:57 ` [PATCH v3 3/4] MdeModulePkg/SdMmcPciHcDxe: Add SwitchClockFreqPost to SdMmcOverride Marcin Wojtas
2018-11-08 11:09 ` Ard Biesheuvel
2018-11-08 11:18 ` Marcin Wojtas
2018-11-08 1:57 ` [PATCH v3 4/4] MdeModulePkg/SdMmcPciHcDxe: Allow overriding base clock frequency Marcin Wojtas
2018-11-08 11:10 ` Ard Biesheuvel [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKv+Gu959Qs04YqwM2b84FrvTjxZ8OHNatxkXv4tjzgpVxrPBQ@mail.gmail.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox