From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::d2b; helo=mail-io1-xd2b.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io1-xd2b.google.com (mail-io1-xd2b.google.com [IPv6:2607:f8b0:4864:20::d2b]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 0A0B621163277 for ; Mon, 8 Oct 2018 05:49:30 -0700 (PDT) Received: by mail-io1-xd2b.google.com with SMTP id l25-v6so15755813ioj.0 for ; Mon, 08 Oct 2018 05:49:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=W/Ur7jNc8c8X34Tdk59JqLpgPoFkmTYpWHD3wAYmbhg=; b=VrgtTLcAnftc1HcEIS6FlMTScsG3nV+doUrcKgK6Ji3sicE54PMyv5MpcbzzjitGwJ AKOAso7vLbb5gjdhE5K8Cm2v4JuxUxwkrzjn1gfKqV7j0y7RVrTUBgFJog5uk24Gz2su N/rV7USwIoFcjQ3FZKs6MTUozWee5p5wmOfXY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=W/Ur7jNc8c8X34Tdk59JqLpgPoFkmTYpWHD3wAYmbhg=; b=QqWX7qpP5BdN+2UNjSUM1ktStTVM+/0CmvZh3SjuaJ9Li3OI2137Hr6P0Ijiru3KjB qK98FcnytwlIF/TxBcejt0wVtKismfTGvOQdwvjDeJeZKvC+jXSqOsBWHvj8B4EvlMOS 7lSB1cN0z8NdtqlMP7lBnOAP4shs0KbaYNUNS/P2E/RRjmvCq1FJGI1rs9dau+cq/rg3 z4mcXjpi3p2GdmlGpoxyOZhJ90Gg54Yoqz8ieWitKHd9IfEAfEcdSFpvb82MPqIwmsn+ Qda94h1Tc1Ws4u3l0RPUqSSFIrPkwS4iZDtYY8Nlrrm4T4eoNWdEkd0svwscYbhzeKiR yH4A== X-Gm-Message-State: ABuFfoi4ifYo75CTkke/PwMybRLc0yuLFkQOHGC6Etlo38j7GdTQA8qR 8uWTCl+Ugnq/i1sSyPVROX/4dkpPGbLZ6pUccdo2DA== X-Google-Smtp-Source: ACcGV60x4PpOxOtEfdDCu/bCV00XIGRWL25u5Nbvu/iZkt3Oak9eM0WYFK4/S2rEU81i5kwX1OrC0KzyuKOr85pV4F4= X-Received: by 2002:a6b:5d12:: with SMTP id r18-v6mr15462133iob.170.1539002970066; Mon, 08 Oct 2018 05:49:30 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:5910:0:0:0:0:0 with HTTP; Mon, 8 Oct 2018 05:49:29 -0700 (PDT) In-Reply-To: <1538745911-22484-5-git-send-email-mw@semihalf.com> References: <1538745911-22484-1-git-send-email-mw@semihalf.com> <1538745911-22484-5-git-send-email-mw@semihalf.com> From: Ard Biesheuvel Date: Mon, 8 Oct 2018 14:49:29 +0200 Message-ID: To: Marcin Wojtas , "Zeng, Star" , Eric Dong , Ruiyu Ni Cc: "edk2-devel@lists.01.org" , "Tian, Feng" , "Kinney, Michael D" , "Gao, Liming" , Leif Lindholm , "Wu, Hao A" , Nadav Haklai , =?UTF-8?B?SmFuIETEhWJyb8Wb?= , Tomasz Michalec Subject: Re: [PATCH v2 4/4] MdeModulePkg/SdMmcPciHcDxe: Allow overriding base clock frequency X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 08 Oct 2018 12:49:31 -0000 Content-Type: text/plain; charset="UTF-8" (add MdeModulePkg maintainers) On 5 October 2018 at 15:25, Marcin Wojtas 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 > --- > 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 > ); > > /** Please bump the version number for this interface change. (just update the define, don't add new ones) > 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); > + > + DumpCapabilityReg (Slot, &Private->Capability[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 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")); 'overridden' However, could we just dump both values unconditionally? > + } > + 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 >