From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: None (no SPF record) identity=mailfrom; client-ip=2607:f8b0:4864:20::d44; helo=mail-io1-xd44.google.com; envelope-from=mw@semihalf.com; receiver=edk2-devel@lists.01.org Received: from mail-io1-xd44.google.com (mail-io1-xd44.google.com [IPv6:2607:f8b0:4864:20::d44]) (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 F3E9C2118AA97 for ; Fri, 2 Nov 2018 02:52:46 -0700 (PDT) Received: by mail-io1-xd44.google.com with SMTP id y22-v6so925016ioj.13 for ; Fri, 02 Nov 2018 02:52:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=Tlgb1h4ahRAW/8y3snX5vcRI6wRZdIn3VkYVHzJFnzs=; b=Y5LnSTJ62UBla8VjUDpvy0+Yd2Jt32ttRz3Avl05VbI7D6fdWPDwQ0iK8Uef26ZEox IlfeZBWf+rHaIHJD2WxIEkZjAXK/pkaReDDlKUQxgsQwRRi1bJxGL+HBQWhaGsuoKEYm wJnYcCNRa9t4CMDGrAtQXzTU2ofL0EPXnhVBmtVGjKU6pamy9daL1s5RziYebTB4+YAW //C90LlZVH9T4mwgbcsn0TVKMkhBFLC0LK0OE6TXni9QAodrtfN7VZgsZOWVsRCtlu2O QXAbiHaI5/I7lRKtvfrArtC4RTF5Azu15WRf09D13mlIGY6S7pOKsc+ga7TXk79t1H6K l2Bg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=Tlgb1h4ahRAW/8y3snX5vcRI6wRZdIn3VkYVHzJFnzs=; b=rqjxQocJtjnhJtapo/tzUtkcjgEaI2sFsPhCsEIpeWV42PZJcLRQBbGPeWNnmRIf0g 1HctErsPNwmNZ7cD3O76wOKu6LBbQXPfomxdpfygPkmcoymgdgKzi5gy6RxVcIPQbn// NxSDdxAdHg5nJmvNJDIHGdSASYYCGkZEGXaIeL0z8MKZMNQD+ArhtcZ825MHvGVs5cmt jLTzvT+GldkupasYV0jkdpRKScNc0ulg95ACgXjFdbK1KwudGDu7y2aCWbfXUbKhMUK4 592Zotc6yKTdfesP2tF+oA9lpjYLzvN22uvpbQEI7tGh5b7EjQDLL0QOkdumkvokNmpL CH4g== X-Gm-Message-State: AGRZ1gL8xtQBjH+5g0Te7wuEyuLC9PY20Tp+E03tFCkKGJtgvpaGK2tV dcp+goHH4KzPEx0Z62njVe1KnGxH+cqv0e9dn8+gBQ== X-Google-Smtp-Source: AJdET5crye/GGz3ZOfEM9TGPbYDtGAjRm/SmO1/nrXAOlcsXdSz9jPyEK4STZ9+A2SVKKXWR+Pd3QS4oD5L0BgoYJWM= X-Received: by 2002:a6b:8ec9:: with SMTP id q192-v6mr8170063iod.248.1541152366021; Fri, 02 Nov 2018 02:52:46 -0700 (PDT) MIME-Version: 1.0 References: <1538745911-22484-1-git-send-email-mw@semihalf.com> <1538745911-22484-5-git-send-email-mw@semihalf.com> In-Reply-To: From: Marcin Wojtas Date: Fri, 2 Nov 2018 10:52:35 +0100 Message-ID: To: hao.a.wu@intel.com Cc: edk2-devel-01 , "Kinney, Michael D" , "Gao, Liming" , Leif Lindholm , Ard Biesheuvel , nadavh@marvell.com, "jsd@semihalf.com" , 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: Fri, 02 Nov 2018 09:52:47 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Hao, czw., 1 lis 2018 o 08:11 Wu, Hao A napisa=C5=82(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.or= g; 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 > > --- > > 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 t= he > > command to. > > @param[in] Capability The buffer to store the capability data. > > + @param[in] BaseClkFreq The base clock frequency of host control= ler 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 uni= t is KHz. > > - @param[in] Capability The capability of the slot. > > + @param[in] BaseClkFreq The base clock frequency of host controlle= r 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 controlle= r 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 controll= er. > > @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 succes= sfully. > > @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 =3D SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Privat= e- > > >Capability[Slot]); > > + Status =3D SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Privat= e- > > >BaseClkFreq[Slot]); > > > > return Status; > > } > > @@ -1099,7 +1099,7 @@ EmmcSetBusMode ( > > return Status; > > } > > > > - 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 ( > > } > > } > > > > - Status =3D SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, *Capab= ility); > > + Status =3D SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Privat= e- > > >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] =3D Private->Capability[Slot].BaseClkFr= eq; > > + > > 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 va= lue 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] !=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 b= e 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() unchange= d 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 &=3D 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 t= he > > command to. > > @param[in] Capability The buffer to store the capability data. > > + @param[in] BaseClkFreq The base clock frequency of host control= ler 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, " =3D=3D Slot [%d] Capability is 0x%x =3D=3D\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 !=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->BusWidt= h8 ? > > "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 uni= t is KHz. > > - @param[in] Capability The capability of the slot. > > + @param[in] BaseClkFreq The base clock frequency of host controlle= r 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 !=3D 0); > > + ASSERT (BaseClkFreq !=3D 0); > > > > - BaseClkFreq =3D Capability.BaseClkFreq; > > if (ClockFreq =3D=3D 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 controlle= r 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 =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; > > } > > > > @@ -1102,7 +1105,7 @@ SdMmcHcInitHost ( > > PciIo =3D Private->PciIo; > > Capability =3D Private->Capability[Slot]; > > > > - Status =3D SdMmcHcInitClockFreq (PciIo, Slot, Capability); > > + Status =3D SdMmcHcInitClockFreq (PciIo, Slot, Private->BaseClkFreq[S= lot]); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > -- > > 2.7.4 >