From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by mx.groups.io with SMTP id smtpd.web12.920.1577155947590584673 for ; Mon, 23 Dec 2019 18:52:27 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.100, mailfrom: hao.a.wu@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Dec 2019 18:52:27 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,350,1571727600"; d="scan'208";a="214060057" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga007.fm.intel.com with ESMTP; 23 Dec 2019 18:52:26 -0800 Received: from fmsmsx113.amr.corp.intel.com (10.18.116.7) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 23 Dec 2019 18:52:26 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by FMSMSX113.amr.corp.intel.com (10.18.116.7) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 23 Dec 2019 18:52:26 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.90]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.222]) with mapi id 14.03.0439.000; Tue, 24 Dec 2019 10:52:22 +0800 From: "Wu, Hao A" To: "Albecki, Mateusz" , "devel@edk2.groups.io" CC: Marcin Wojtas , "Gao, Zhichao" , "Gao, Liming" , Ard Biesheuvel Subject: Re: [PATCH 1/2] SdMmcPciHcDxe: Send EdkiiSdMmcSwitchClockFreq after SD clock start Thread-Topic: [PATCH 1/2] SdMmcPciHcDxe: Send EdkiiSdMmcSwitchClockFreq after SD clock start Thread-Index: AQHVt1j/v0tXok7XkECWWPHXwbo4wafIknUw Date: Tue, 24 Dec 2019 02:52:22 +0000 Message-ID: References: <20191220171312.3120-1-mateusz.albecki@intel.com> <20191220171312.3120-2-mateusz.albecki@intel.com> In-Reply-To: <20191220171312.3120-2-mateusz.albecki@intel.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 Return-Path: hao.a.wu@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Albecki, Mateusz > Sent: Saturday, December 21, 2019 1:13 AM > To: devel@edk2.groups.io > Cc: Albecki, Mateusz; Wu, Hao A; Marcin Wojtas; Gao, Zhichao; Gao, Liming > Subject: [PATCH 1/2] SdMmcPciHcDxe: Send EdkiiSdMmcSwitchClockFreq after > SD clock start Hello Mateusz, Just a minor format comment, how about changing the subject to: MdeModulePkg/SdMmcPciHcDxe: Hook SwitchClockFreq after SD clock start If there is no other major concern from other reviewers for the patch, I wi= ll handle this when pushing the patch. Reviewed-by: Hao A Wu Best Regards, Hao Wu >=20 > For eMMC modules we used to notify the platform about frequency > change only after sending CMD13 which meant that platform > might not get a chance to apply required post frequency > change fixes to get the clock stable. To fix this > notification has been moved to SdMmcHcClockSupply function > just after we start the SD clock. During first time setup > the notification won't be sent to avoid changing old behavior. >=20 > Cc: Hao A Wu > Cc: Marcin Wojtas > Cc: Zhichao Gao > Cc: Liming Gao >=20 > Signed-off-by: Mateusz Albecki > --- > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 20 +--- > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c | 28 ++---- > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h | 24 +++++ > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 107 +++++++++-- > ---------- > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h | 44 --------- > 5 files changed, 81 insertions(+), 142 deletions(-) >=20 > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > index 082904ccc5..776c0e796c 100644 > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > @@ -727,7 +727,7 @@ EmmcSwitchBusTiming ( > // > // Convert the clock freq unit from MHz to KHz. > // > - Status =3D SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private- > >BaseClkFreq[Slot], Private->ControllerVersion[Slot]); > + Status =3D SdMmcHcClockSupply (Private, Slot, BusTiming, FALSE, ClockF= req * > 1000); > if (EFI_ERROR (Status)) { > return Status; > } > @@ -745,24 +745,6 @@ EmmcSwitchBusTiming ( > return EFI_DEVICE_ERROR; > } >=20 > - if (mOverride !=3D NULL && mOverride->NotifyPhase !=3D NULL) { > - Status =3D mOverride->NotifyPhase ( > - Private->ControllerHandle, > - Slot, > - EdkiiSdMmcSwitchClockFreqPost, > - &BusTiming > - ); > - if (EFI_ERROR (Status)) { > - DEBUG (( > - DEBUG_ERROR, > - "%a: SD/MMC switch clock freq post notifier callback failed - %r= \n", > - __FUNCTION__, > - Status > - )); > - return Status; > - } > - } > - > return Status; > } >=20 > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c > index 336baade9e..d63dc54e8c 100644 > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdDevice.c > @@ -1145,29 +1145,11 @@ SdCardSetBusMode ( > return Status; > } >=20 > - Status =3D SdMmcHcClockSupply (PciIo, Slot, BusMode.ClockFreq * 1000, > Private->BaseClkFreq[Slot], Private->ControllerVersion[Slot]); > + Status =3D SdMmcHcClockSupply (Private, Slot, BusMode.BusTiming, FALSE= , > BusMode.ClockFreq * 1000); > if (EFI_ERROR (Status)) { > return Status; > } >=20 > - if (mOverride !=3D NULL && mOverride->NotifyPhase !=3D NULL) { > - Status =3D mOverride->NotifyPhase ( > - Private->ControllerHandle, > - Slot, > - EdkiiSdMmcSwitchClockFreqPost, > - &BusMode.BusTiming > - ); > - if (EFI_ERROR (Status)) { > - DEBUG (( > - DEBUG_ERROR, > - "%a: SD/MMC switch clock freq post notifier callback failed - %r= \n", > - __FUNCTION__, > - Status > - )); > - return Status; > - } > - } > - > if ((BusMode.BusTiming =3D=3D SdMmcUhsSdr104) || ((BusMode.BusTiming = =3D=3D > SdMmcUhsSdr50) && (Capability->TuningSDR50 !=3D 0))) { > Status =3D SdCardTuningClock (PciIo, PassThru, Slot); > if (EFI_ERROR (Status)) { > @@ -1345,7 +1327,13 @@ SdCardIdentification ( > goto Error; > } >=20 > - SdMmcHcInitClockFreq (PciIo, Slot, Private->BaseClkFreq[Slot], Pri= vate- > >ControllerVersion[Slot]); > + // > + // Restart the clock with first time parameters. > + // NOTE: it is not required to actually restart the clock > + // and go through internal clock setup again. Some time > + // could be saved if we simply started the SD clock. > + // > + SdMmcHcClockSupply (Private, Slot, 0, TRUE, 400); >=20 > gBS->Stall (1000); >=20 > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h > index c29e48767e..0304960132 100644 > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h > @@ -796,6 +796,30 @@ SdCardIdentification ( > IN UINT8 Slot > ); >=20 > +/** > + SD/MMC card clock supply. > + > + Refer to SD Host Controller Simplified spec 3.0 Section 3.2.1 for deta= ils. > + > + @param[in] Private A pointer to the SD_MMC_HC_PRIVATE_DATA > instance. > + @param[in] Slot The slot number of the SD card to send the = command > to. > + @param[in] BusTiming BusTiming at which the frequency change is = done. > + @param[in] FirstTimeSetup Flag to indicate whether the clock is being= setup > for the first time. > + @param[in] ClockFreq The max clock frequency to be set. The unit= is KHz. > + > + @retval EFI_SUCCESS The clock is supplied successfully. > + @retval Others The clock isn't supplied successfully. > + > +**/ > +EFI_STATUS > +SdMmcHcClockSupply ( > + IN SD_MMC_HC_PRIVATE_DATA *Private, > + IN UINT8 Slot, > + IN SD_MMC_BUS_MODE BusTiming, > + IN BOOLEAN FirstTimeSetup, > + IN UINT64 ClockFreq > + ); > + > /** > Software reset the specified SD/MMC host controller. >=20 > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > index b9d04e0f17..f667264c5e 100644 > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > @@ -763,11 +763,11 @@ SdMmcHcStopClock ( >=20 > Refer to SD Host Controller Simplified spec 3.0 Section 3.2.1 for deta= ils. >=20 > - @param[in] PciIo The PCI IO protocol instance. > - @param[in] Slot The slot number of the SD card to send the c= ommand > to. > - @param[in] ClockFreq The max clock frequency to be set. The unit = is KHz. > - @param[in] BaseClkFreq The base clock frequency of host controller = in > MHz. > - @param[in] ControllerVer The version of host controller. > + @param[in] Private A pointer to the SD_MMC_HC_PRIVATE_DATA > instance. > + @param[in] Slot The slot number of the SD card to send the = command > to. > + @param[in] BusTiming BusTiming at which the frequency change is = done. > + @param[in] FirstTimeSetup Flag to indicate whether the clock is being= setup > for the first time. > + @param[in] ClockFreq The max clock frequency to be set. The unit= is KHz. >=20 > @retval EFI_SUCCESS The clock is supplied successfully. > @retval Others The clock isn't supplied successfully. > @@ -775,11 +775,11 @@ SdMmcHcStopClock ( > **/ > EFI_STATUS > SdMmcHcClockSupply ( > - IN EFI_PCI_IO_PROTOCOL *PciIo, > - IN UINT8 Slot, > - IN UINT64 ClockFreq, > - IN UINT32 BaseClkFreq, > - IN UINT16 ControllerVer > + IN SD_MMC_HC_PRIVATE_DATA *Private, > + IN UINT8 Slot, > + IN SD_MMC_BUS_MODE BusTiming, > + IN BOOLEAN FirstTimeSetup, > + IN UINT64 ClockFreq > ) > { > EFI_STATUS Status; > @@ -787,13 +787,15 @@ SdMmcHcClockSupply ( > UINT32 Divisor; > UINT32 Remainder; > UINT16 ClockCtrl; > + UINT32 BaseClkFreq; > + UINT16 ControllerVer; > + EFI_PCI_IO_PROTOCOL *PciIo; >=20 > - // > - // Calculate a divisor for SD clock frequency > - // > - ASSERT (BaseClkFreq !=3D 0); > + PciIo =3D Private->PciIo; > + BaseClkFreq =3D Private->BaseClkFreq[Slot]; > + ControllerVer =3D Private->ControllerVersion[Slot]; >=20 > - if (ClockFreq =3D=3D 0) { > + if (BaseClkFreq =3D=3D 0 || ClockFreq =3D=3D 0) { > return EFI_INVALID_PARAMETER; > } >=20 > @@ -883,6 +885,29 @@ SdMmcHcClockSupply ( > ClockCtrl =3D BIT2; > Status =3D SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_CLOCK_CTRL, sizeof > (ClockCtrl), &ClockCtrl); >=20 > + // > + // We don't notify the platform on first time setup to avoid changing > + // legacy behavior. During first time setup we also don't know what ty= pe > + // of the card slot it is and which enum value of BusTiming applies. > + // > + if (!FirstTimeSetup && mOverride !=3D NULL && mOverride->NotifyPhase != =3D > NULL) { > + Status =3D mOverride->NotifyPhase ( > + Private->ControllerHandle, > + Slot, > + EdkiiSdMmcSwitchClockFreqPost, > + &BusTiming > + ); > + if (EFI_ERROR (Status)) { > + DEBUG (( > + DEBUG_ERROR, > + "%a: SD/MMC switch clock freq post notifier callback failed - %r= \n", > + __FUNCTION__, > + Status > + )); > + return Status; > + } > + } > + > return Status; > } >=20 > @@ -1038,49 +1063,6 @@ SdMmcHcInitV4Enhancements ( > return EFI_SUCCESS; > } >=20 > -/** > - Supply SD/MMC card with lowest clock frequency at initialization. > - > - @param[in] PciIo The PCI IO protocol instance. > - @param[in] Slot The slot number of the SD card to send the c= ommand > to. > - @param[in] BaseClkFreq The base clock frequency of host controller = in > MHz. > - @param[in] ControllerVer The version of host controller. > - > - @retval EFI_SUCCESS The clock is supplied successfully. > - @retval Others The clock isn't supplied successfully. > - > -**/ > -EFI_STATUS > -SdMmcHcInitClockFreq ( > - IN EFI_PCI_IO_PROTOCOL *PciIo, > - IN UINT8 Slot, > - IN UINT32 BaseClkFreq, > - IN UINT16 ControllerVer > - ) > -{ > - EFI_STATUS Status; > - UINT32 InitFreq; > - > - // > - // According to SDHCI specification ver. 4.2, BaseClkFreq field value = of > - // the Capability Register 1 can be zero, which means a need for obtai= ning > - // the clock frequency via another method. Fail in case it is not upda= ted > - // by SW at this point. > - // > - if (BaseClkFreq =3D=3D 0) { > - // > - // Don't support get Base Clock Frequency information via another me= thod > - // > - return EFI_UNSUPPORTED; > - } > - // > - // Supply 400KHz clock frequency at initialization phase. > - // > - InitFreq =3D 400; > - Status =3D SdMmcHcClockSupply (PciIo, Slot, InitFreq, BaseClkFreq, > ControllerVer); > - return Status; > -} > - > /** > Supply SD/MMC card with maximum voltage at initialization. >=20 > @@ -1216,7 +1198,14 @@ SdMmcHcInitHost ( > return Status; > } >=20 > - Status =3D SdMmcHcInitClockFreq (PciIo, Slot, Private->BaseClkFreq[Slo= t], > Private->ControllerVersion[Slot]); > + // > + // Perform first time clock setup with 400 KHz frequency. > + // We send the 0 as the BusTiming value because at this time > + // we still do not know the slot type and which enum value will apply. > + // Since it is a first time setup SdMmcHcClockSupply won't notify > + // the platofrm driver anyway so it doesn't matter. > + // > + Status =3D SdMmcHcClockSupply (Private, Slot, 0, TRUE, 400); > if (EFI_ERROR (Status)) { > return Status; > } > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h > index 088c70451c..826e851b04 100644 > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.h > @@ -478,30 +478,6 @@ SdMmcHcStopClock ( > IN UINT8 Slot > ); >=20 > -/** > - SD/MMC card clock supply. > - > - Refer to SD Host Controller Simplified spec 3.0 Section 3.2.1 for deta= ils. > - > - @param[in] PciIo The PCI IO protocol instance. > - @param[in] Slot The slot number of the SD card to send the c= ommand > to. > - @param[in] ClockFreq The max clock frequency to be set. The unit = is KHz. > - @param[in] BaseClkFreq The base clock frequency of host controller = in > MHz. > - @param[in] ControllerVer The version of host controller. > - > - @retval EFI_SUCCESS The clock is supplied successfully. > - @retval Others The clock isn't supplied successfully. > - > -**/ > -EFI_STATUS > -SdMmcHcClockSupply ( > - IN EFI_PCI_IO_PROTOCOL *PciIo, > - IN UINT8 Slot, > - IN UINT64 ClockFreq, > - IN UINT32 BaseClkFreq, > - IN UINT16 ControllerVer > - ); > - > /** > SD/MMC bus power control. >=20 > @@ -542,26 +518,6 @@ SdMmcHcSetBusWidth ( > IN UINT16 BusWidth > ); >=20 > -/** > - Supply SD/MMC card with lowest clock frequency at initialization. > - > - @param[in] PciIo The PCI IO protocol instance. > - @param[in] Slot The slot number of the SD card to send the c= ommand > to. > - @param[in] BaseClkFreq The base clock frequency of host controller = in > MHz. > - @param[in] ControllerVer The version of host controller. > - > - @retval EFI_SUCCESS The clock is supplied successfully. > - @retval Others The clock isn't supplied successfully. > - > -**/ > -EFI_STATUS > -SdMmcHcInitClockFreq ( > - IN EFI_PCI_IO_PROTOCOL *PciIo, > - IN UINT8 Slot, > - IN UINT32 BaseClkFreq, > - IN UINT16 ControllerVer > - ); > - > /** > Supply SD/MMC card with maximum voltage at initialization. >=20 > -- > 2.14.1.windows.1