From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f67.google.com (mail-wm1-f67.google.com [209.85.128.67]) by mx.groups.io with SMTP id smtpd.web09.1244.1577181257400866278 for ; Tue, 24 Dec 2019 01:54:17 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=DiTUicuK; spf=pass (domain: linaro.org, ip: 209.85.128.67, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wm1-f67.google.com with SMTP id q9so409394wmj.5 for ; Tue, 24 Dec 2019 01:54:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=nhwA9PBOpYdeaYvChhBP8JDEXTlqlHivJ4Q0drPvtI8=; b=DiTUicuKXqTKeJ9aG/dD/LLLUlihh8L80ON+mLs/JjBXdok79FwTQ5G+GS99vjXzMk XCp74psKkLQ4IeGZ6c0zj4E7H3iek9x9Cl1rhHN+0flZDEfTAnUivCbkseqyw832QTGv XvUdyE+MQh8j2enBc/kpiI942QC8P4NQ8gZNk/B9ti4Js13OtTQtR/Aap+r75apc4lWy weKzPiCmOsQxvmNStcdB4F0neAwrenjZuCboh2uKOawrWkSXu5QulDf5E6FWCCkrzrTf G0hJB5Smal+CeHVkPpabQFl/KuUVlOn1KUlv3fJQOc8U6UMFWWQ/4PwCU5CIrwukPaAQ w22w== 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; bh=nhwA9PBOpYdeaYvChhBP8JDEXTlqlHivJ4Q0drPvtI8=; b=gWI9eI12iOp8pTqBMKfkqvpXlr1gZa1Akd85LDsAXA634hEjpATVDVd5cC0u/j3uB3 gzGVN7ioKV/MIDdT7hlZjxGido1VvlbZFCharZMS/Lw66B0a8/nWJ21d4ytfh2R1wY5n 4uP0lDxqAzeEm9KQpEjDaJ5vLF4FFtJarB+Fo746y4MRtiTnIuIiMtMkXNz4VlVAMFJI 99jgPy2lpgTylEd4qjRiOEzE3QbVzK0m+UcT1yy4x0Jr/qIgcWWxlvVVzG0eX8ECbKq7 DY7gzTgDdvzmgGQeZ/1S76+J4igO8qB7ijevsHmjEZIC82dfePugy6en0i3/6sgB3s/H BT+w== X-Gm-Message-State: APjAAAXmRBWChi7dYAcXDpJw1hleRXCglmg3zroV363UnIzhWGisvSsK vfYL9rwUA2WwLOB1HYYul0v6rB8ovpaiWHK7QYdsNA== X-Google-Smtp-Source: APXvYqyqcDgNN8tKA6qeyi/1xuBKy+hZozG7VFNr5ymw3xsp7UtV7ODGP0Wo54Jm8jXtBNQCyHKjt7Isl89fSJvkaYI= X-Received: by 2002:a1c:3dc3:: with SMTP id k186mr3265202wma.95.1577181255897; Tue, 24 Dec 2019 01:54:15 -0800 (PST) MIME-Version: 1.0 References: <20191220171312.3120-1-mateusz.albecki@intel.com> <20191220171312.3120-2-mateusz.albecki@intel.com> In-Reply-To: From: "Ard Biesheuvel" Date: Tue, 24 Dec 2019 10:54:15 +0100 Message-ID: Subject: Re: [PATCH 1/2] SdMmcPciHcDxe: Send EdkiiSdMmcSwitchClockFreq after SD clock start To: "Wu, Hao A" , Marcin Wojtas Cc: "Albecki, Mateusz" , "devel@edk2.groups.io" , "Gao, Zhichao" , "Gao, Liming" Content-Type: text/plain; charset="UTF-8" On Tue, 24 Dec 2019 at 03:52, Wu, Hao A wrote: > > > -----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 will > handle this when pushing the patch. > > Reviewed-by: Hao A Wu > > Best Regards, > Hao Wu > I don't have any objections to this patch, but I'd like Marcin to confirm, given that this override callback is used on the Marvell platforms. > > > > > 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. > > > > Cc: Hao A Wu > > Cc: Marcin Wojtas > > Cc: Zhichao Gao > > Cc: Liming Gao > > > > 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(-) > > > > 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 = SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Private- > > >BaseClkFreq[Slot], Private->ControllerVersion[Slot]); > > + Status = SdMmcHcClockSupply (Private, Slot, BusTiming, FALSE, ClockFreq * > > 1000); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > @@ -745,24 +745,6 @@ EmmcSwitchBusTiming ( > > return EFI_DEVICE_ERROR; > > } > > > > - if (mOverride != NULL && mOverride->NotifyPhase != NULL) { > > - Status = 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; > > } > > > > 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; > > } > > > > - Status = SdMmcHcClockSupply (PciIo, Slot, BusMode.ClockFreq * 1000, > > Private->BaseClkFreq[Slot], Private->ControllerVersion[Slot]); > > + Status = SdMmcHcClockSupply (Private, Slot, BusMode.BusTiming, FALSE, > > BusMode.ClockFreq * 1000); > > if (EFI_ERROR (Status)) { > > return Status; > > } > > > > - if (mOverride != NULL && mOverride->NotifyPhase != NULL) { > > - Status = 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 == SdMmcUhsSdr104) || ((BusMode.BusTiming == > > SdMmcUhsSdr50) && (Capability->TuningSDR50 != 0))) { > > Status = SdCardTuningClock (PciIo, PassThru, Slot); > > if (EFI_ERROR (Status)) { > > @@ -1345,7 +1327,13 @@ SdCardIdentification ( > > goto Error; > > } > > > > - SdMmcHcInitClockFreq (PciIo, Slot, Private->BaseClkFreq[Slot], Private- > > >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); > > > > gBS->Stall (1000); > > > > 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 > > ); > > > > +/** > > + SD/MMC card clock supply. > > + > > + Refer to SD Host Controller Simplified spec 3.0 Section 3.2.1 for details. > > + > > + @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. > > > > 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 ( > > > > Refer to SD Host Controller Simplified spec 3.0 Section 3.2.1 for details. > > > > - @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] 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. > > > > @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; > > > > - // > > - // Calculate a divisor for SD clock frequency > > - // > > - ASSERT (BaseClkFreq != 0); > > + PciIo = Private->PciIo; > > + BaseClkFreq = Private->BaseClkFreq[Slot]; > > + ControllerVer = Private->ControllerVersion[Slot]; > > > > - if (ClockFreq == 0) { > > + if (BaseClkFreq == 0 || ClockFreq == 0) { > > return EFI_INVALID_PARAMETER; > > } > > > > @@ -883,6 +885,29 @@ SdMmcHcClockSupply ( > > ClockCtrl = BIT2; > > Status = SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_CLOCK_CTRL, sizeof > > (ClockCtrl), &ClockCtrl); > > > > + // > > + // 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 type > > + // of the card slot it is and which enum value of BusTiming applies. > > + // > > + if (!FirstTimeSetup && mOverride != NULL && mOverride->NotifyPhase != > > NULL) { > > + Status = 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; > > } > > > > @@ -1038,49 +1063,6 @@ SdMmcHcInitV4Enhancements ( > > return EFI_SUCCESS; > > } > > > > -/** > > - 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 command > > 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 obtaining > > - // the clock frequency via another method. Fail in case it is not updated > > - // by SW at this point. > > - // > > - if (BaseClkFreq == 0) { > > - // > > - // Don't support get Base Clock Frequency information via another method > > - // > > - return EFI_UNSUPPORTED; > > - } > > - // > > - // Supply 400KHz clock frequency at initialization phase. > > - // > > - InitFreq = 400; > > - Status = SdMmcHcClockSupply (PciIo, Slot, InitFreq, BaseClkFreq, > > ControllerVer); > > - return Status; > > -} > > - > > /** > > Supply SD/MMC card with maximum voltage at initialization. > > > > @@ -1216,7 +1198,14 @@ SdMmcHcInitHost ( > > return Status; > > } > > > > - Status = SdMmcHcInitClockFreq (PciIo, Slot, Private->BaseClkFreq[Slot], > > 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 = 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 > > ); > > > > -/** > > - SD/MMC card clock supply. > > - > > - Refer to SD Host Controller Simplified spec 3.0 Section 3.2.1 for details. > > - > > - @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] 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. > > > > @@ -542,26 +518,6 @@ SdMmcHcSetBusWidth ( > > IN UINT16 BusWidth > > ); > > > > -/** > > - 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 command > > 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. > > > > -- > > 2.14.1.windows.1 >