From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.65; helo=mga03.intel.com; envelope-from=hao.a.wu@intel.com; receiver=edk2-devel@lists.01.org Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id EBFA72117B577 for ; Thu, 1 Nov 2018 00:11:07 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 01 Nov 2018 00:11:06 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,451,1534834800"; d="scan'208";a="87750553" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga006.jf.intel.com with ESMTP; 01 Nov 2018 00:11:06 -0700 Received: from fmsmsx112.amr.corp.intel.com (10.18.116.6) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 1 Nov 2018 00:11:06 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by FMSMSX112.amr.corp.intel.com (10.18.116.6) with Microsoft SMTP Server (TLS) id 14.3.408.0; Thu, 1 Nov 2018 00:11:05 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.117]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.214]) with mapi id 14.03.0415.000; Thu, 1 Nov 2018 15:11:04 +0800 From: "Wu, Hao A" To: 'Marcin Wojtas' , "edk2-devel@lists.01.org" CC: "Kinney, Michael D" , "Gao, Liming" , "leif.lindholm@linaro.org" , "ard.biesheuvel@linaro.org" , "nadavh@marvell.com" , "jsd@semihalf.com" , "tm@semihalf.com" Thread-Topic: [PATCH v2 4/4] MdeModulePkg/SdMmcPciHcDxe: Allow overriding base clock frequency Thread-Index: AQHUXK7vb38dSrbJ+kCpzZmZD+e6l6U3aumg Date: Thu, 1 Nov 2018 07:11:03 +0000 Message-ID: References: <1538745911-22484-1-git-send-email-mw@semihalf.com> <1538745911-22484-5-git-send-email-mw@semihalf.com> In-Reply-To: <1538745911-22484-5-git-send-email-mw@semihalf.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 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: Thu, 01 Nov 2018 07:11:08 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 >=20 > 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. >=20 > 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. >=20 > 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. >=20 > This patch also adds new IN OUT BaseClockFreq field > in the Capability callback of the SdMmcOverride, > protocol which allows to update BaseClkFreq value. >=20 > The patch reuses original commit from edk2-platforms: > 20f6f144d3a8 ("Marvell/Drivers: XenonDxe: Allow overriding base clock > frequency") >=20 > 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(-) >=20 > 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]; >=20 > 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; >=20 > #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 { >=20 > @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 controlle= r in > MHz. >=20 > **/ > VOID > DumpCapabilityReg ( > IN UINT8 Slot, > - IN SD_MMC_HC_SLOT_CAP *Capability > + IN SD_MMC_HC_SLOT_CAP *Capability, > + IN UINT32 BaseClkFreq > ); >=20 > /** > @@ -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. >=20 > @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 > ); >=20 > /** > @@ -490,7 +492,7 @@ SdMmcHcSetBusWidth ( >=20 > @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. >=20 > @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 > ); >=20 > /** > 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 t= hat > + optionally can be updated. >=20 > @retval EFI_SUCCESS The override function completed successf= ully. > @retval EFI_NOT_FOUND The specified controller or slot does no= t > 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 > ); >=20 > /** > 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 =3D SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private- > >Capability[Slot]); > + Status =3D SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private- > >BaseClkFreq[Slot]); >=20 > return Status; > } > @@ -1099,7 +1099,7 @@ EmmcSetBusMode ( > return Status; > } >=20 > - ASSERT (Private->Capability[Slot].BaseClkFreq !=3D 0); > + ASSERT (Private->BaseClkFreq[Slot] !=3D 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 ( > } > } >=20 > - Status =3D SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, *Capabil= ity); > + Status =3D SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private- > >BaseClkFreq[Slot]); > if (EFI_ERROR (Status)) { > return Status; > } > @@ -1081,7 +1081,7 @@ SdCardIdentification ( > goto Error; > } >=20 > - SdMmcHcInitClockFreq (PciIo, Slot, Private->Capability[Slot]); > + SdMmcHcInitClockFreq (PciIo, Slot, Private->BaseClkFreq[Slot]); >=20 > gBS->Stall (1000); >=20 > 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] =3D Private->Capability[Slot].BaseClkFreq= ; > + > if (mOverride !=3D NULL && mOverride->Capability !=3D NULL) { > Status =3D 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 valu= e 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 up= dated > + // by SW at this point. > + // > + ASSERT (Private->BaseClkFreq[Slot] !=3D 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. > + > + 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. Best Regards, Hao Wu >=20 > Support64BitDma &=3D Private->Capability[Slot].SysBus64; >=20 > 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 @@ >=20 > @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 controlle= r in > MHz. >=20 > **/ > 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, " =3D=3D Slot [%d] Capability is 0x%x =3D=3D\n", S= lot, > 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 !=3D 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. >=20 > @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 !=3D 0); > + ASSERT (BaseClkFreq !=3D 0); >=20 > - BaseClkFreq =3D Capability.BaseClkFreq; > if (ClockFreq =3D=3D 0) { > return EFI_INVALID_PARAMETER; > } > @@ -939,7 +942,7 @@ SdMmcHcSetBusWidth ( >=20 > @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. >=20 > @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 =3D=3D 0) { > + if (BaseClkFreq =3D=3D 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 =3D 400; > - Status =3D SdMmcHcClockSupply (PciIo, Slot, InitFreq, Capability); > + Status =3D SdMmcHcClockSupply (PciIo, Slot, InitFreq, BaseClkFreq); > return Status; > } >=20 > @@ -1102,7 +1105,7 @@ SdMmcHcInitHost ( > PciIo =3D Private->PciIo; > Capability =3D Private->Capability[Slot]; >=20 > - Status =3D SdMmcHcInitClockFreq (PciIo, Slot, Capability); > + Status =3D SdMmcHcInitClockFreq (PciIo, Slot, Private->BaseClkFreq[Slo= t]); > if (EFI_ERROR (Status)) { > return Status; > } > -- > 2.7.4