From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qk1-f193.google.com (mail-qk1-f193.google.com [209.85.222.193]) by mx.groups.io with SMTP id smtpd.web12.1026.1577695469208357489 for ; Mon, 30 Dec 2019 00:44:29 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@semihalf-com.20150623.gappssmtp.com header.s=20150623 header.b=Yc5JLIiF; spf=none, err=SPF record not found (domain: semihalf.com, ip: 209.85.222.193, mailfrom: mw@semihalf.com) Received: by mail-qk1-f193.google.com with SMTP id x1so25770103qkl.12 for ; Mon, 30 Dec 2019 00:44:29 -0800 (PST) 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=kAfVWZoKMhWsac/0JJzOy2pPtQWp0oyO7bAyQ45moA0=; b=Yc5JLIiF4+MAn35T08GNJtfQ4rlrXgSN3But+MMSIMlaepCTeHzxswKX/maE+WCxMt LzpyHfkfR3gejUR/H0t28hCe7cZJs3mI7th1GY491Kn4RkYuKKSiTSSFm4Rjiud79e1h qQSTyH0bs+zMiWr42293o8TXswyEBXL2pz58JiAWhO7LGalN5F4glv+skfyA4C9a6u4p 6lTCHqGYLyZ34armjNTJQj732n9xGDvS2zdkECF7gX268qsB5e2L6qhj6Ue+qP2cIQyu w7mfQBxmZzm1qZHqE3JoKavbqScmmAoAxWs9O+866qc/TFsO0qULdILhgo0qB+Pq/Mpb kEeg== 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=kAfVWZoKMhWsac/0JJzOy2pPtQWp0oyO7bAyQ45moA0=; b=DPi1Tr6Ttze/e4EwyWNVnfFzMJ+l6GzRLVBYP1Bzh05ru0MODhZ0NsUSSuecXzU8uR GbZOsBigPTA09cCHqO1hlY8UrNltEi8YoFiuFYlmcaY2VumsuF0MDRETWtClhjDM5MSq LrdKLFyacm+V7jTLMf487NVQ7yO0sJa+rm0ClknEvX9FZz8RuEFvLywmsqtP8JQpyBNy ebyBA85fOEHLxjYDfhFR5clm9dz0fgZ91eJ5/TvHkmuJOGVwI+G1wNJWL0+NoqfhjWAt 1lyPisRNqmVB4tknhWPq5vQpaRzeEfwAnfhvVle9qmOxu2QoTkOaManX4q5shsouZWsT cAgg== X-Gm-Message-State: APjAAAViZ2YRwQplsrzTpTcnFonXsM8Pj3ihI8GE+gyqWDniB2kzWpl6 XYFnOYOMDHMk0OAue5bP9yFzlehKFHpJAZC0X1N4/A== X-Google-Smtp-Source: APXvYqyRiqzJCMWhJ2a+J1i0YA4dBhaOl9mcVCmTZ3wYJvwiIMTC/jE/BWnnajmgB6DXg750s8KzLd2S61iM1vA8P+I= X-Received: by 2002:a05:620a:1114:: with SMTP id o20mr52947148qkk.128.1577695468230; Mon, 30 Dec 2019 00:44:28 -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: "Marcin Wojtas" Date: Mon, 30 Dec 2019 09:44:16 +0100 Message-ID: Subject: Re: [PATCH 1/2] SdMmcPciHcDxe: Send EdkiiSdMmcSwitchClockFreq after SD clock start To: Ard Biesheuvel Cc: "Wu, Hao A" , "Albecki, Mateusz" , "devel@edk2.groups.io" , "Gao, Zhichao" , "Gao, Liming" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi, wt., 24 gru 2019 o 10:54 Ard Biesheuvel napisa= =C5=82(a): > > 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, Li= ming > > > Subject: [PATCH 1/2] SdMmcPciHcDxe: Send EdkiiSdMmcSwitchClockFreq af= ter > > > 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. > I'll check the branch today or tomorrow. Best regards, Marcin > > > > > > > > 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 =3D SdMmcHcClockSupply (PciIo, Slot, ClockFreq * 1000, Priv= ate- > > > >BaseClkFreq[Slot], Private->ControllerVersion[Slot]); > > > + Status =3D SdMmcHcClockSupply (Private, Slot, BusTiming, FALSE, Cl= ockFreq * > > > 1000); > > > if (EFI_ERROR (Status)) { > > > return Status; > > > } > > > @@ -745,24 +745,6 @@ EmmcSwitchBusTiming ( > > > return EFI_DEVICE_ERROR; > > > } > > > > > > - 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; > > > } > > > > > > 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 =3D SdMmcHcClockSupply (PciIo, Slot, BusMode.ClockFreq * 10= 00, > > > Private->BaseClkFreq[Slot], Private->ControllerVersion[Slot]); > > > + Status =3D SdMmcHcClockSupply (Private, Slot, BusMode.BusTiming, F= ALSE, > > > BusMode.ClockFreq * 1000); > > > if (EFI_ERROR (Status)) { > > > return Status; > > > } > > > > > > - 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.BusTimi= ng =3D=3D > > > SdMmcUhsSdr50) && (Capability->TuningSDR50 !=3D 0))) { > > > Status =3D 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 b= eing 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 t= he command > > > to. > > > - @param[in] ClockFreq The max clock frequency to be set. The u= nit is KHz. > > > - @param[in] BaseClkFreq The base clock frequency of host control= ler 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 b= eing 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 !=3D 0); > > > + PciIo =3D Private->PciIo; > > > + BaseClkFreq =3D Private->BaseClkFreq[Slot]; > > > + ControllerVer =3D Private->ControllerVersion[Slot]; > > > > > > - if (ClockFreq =3D=3D 0) { > > > + if (BaseClkFreq =3D=3D 0 || ClockFreq =3D=3D 0) { > > > return EFI_INVALID_PARAMETER; > > > } > > > > > > @@ -883,6 +885,29 @@ SdMmcHcClockSupply ( > > > ClockCtrl =3D BIT2; > > > Status =3D SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_CLOCK_CTRL, sizeo= f > > > (ClockCtrl), &ClockCtrl); > > > > > > + // > > > + // We don't notify the platform on first time setup to avoid chang= ing > > > + // legacy behavior. During first time setup we also don't know wha= t type > > > + // of the card slot it is and which enum value of BusTiming applie= s. > > > + // > > > + if (!FirstTimeSetup && mOverride !=3D NULL && mOverride->NotifyPha= se !=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; > > > } > > > > > > @@ -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 t= he command > > > to. > > > - @param[in] BaseClkFreq The base clock frequency of host control= ler 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 va= lue of > > > - // the Capability Register 1 can be zero, which means a need for o= btaining > > > - // the clock frequency via another method. Fail in case it is not = updated > > > - // by SW at this point. > > > - // > > > - if (BaseClkFreq =3D=3D 0) { > > > - // > > > - // Don't support get Base Clock Frequency information via anothe= r method > > > - // > > > - 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. > > > > > > @@ -1216,7 +1198,14 @@ SdMmcHcInitHost ( > > > return Status; > > > } > > > > > > - Status =3D 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 ap= ply. > > > + // 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 > > > ); > > > > > > -/** > > > - 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 t= he command > > > to. > > > - @param[in] ClockFreq The max clock frequency to be set. The u= nit is KHz. > > > - @param[in] BaseClkFreq The base clock frequency of host control= ler 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 t= he command > > > to. > > > - @param[in] BaseClkFreq The base clock frequency of host control= ler 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 > >