From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.100, mailfrom: hao.a.wu@intel.com) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by groups.io with SMTP; Fri, 27 Sep 2019 01:01:58 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Sep 2019 01:01:58 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,554,1559545200"; d="scan'208";a="201939329" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga002.jf.intel.com with ESMTP; 27 Sep 2019 01:01:57 -0700 Received: from fmsmsx607.amr.corp.intel.com (10.18.126.87) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Fri, 27 Sep 2019 01:01:57 -0700 Received: from fmsmsx607.amr.corp.intel.com (10.18.126.87) by fmsmsx607.amr.corp.intel.com (10.18.126.87) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Fri, 27 Sep 2019 01:01:56 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx607.amr.corp.intel.com (10.18.126.87) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Fri, 27 Sep 2019 01:01:56 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.32]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.53]) with mapi id 14.03.0439.000; Fri, 27 Sep 2019 16:01:54 +0800 From: "Wu, Hao A" To: "Albecki, Mateusz" , "devel@edk2.groups.io" CC: Marcin Wojtas Subject: Re: [PATCH 1/1] MdeModulePkg/SdMmcPciHcDxe: Fix bus timing switch sequence Thread-Topic: [PATCH 1/1] MdeModulePkg/SdMmcPciHcDxe: Fix bus timing switch sequence Thread-Index: AQHVceotbQiUvge/ykuom8W5GHGiJKc6gc0AgAF2VQCAAScZIIAAU5yAgAFZAfA= Date: Fri, 27 Sep 2019 08:01:54 +0000 Message-ID: References: <20190923083701.1496-1-mateusz.albecki@intel.com> <20190923083701.1496-2-mateusz.albecki@intel.com> In-Reply-To: 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: Thursday, September 26, 2019 9:29 PM > To: Wu, Hao A; devel@edk2.groups.io > Cc: Marcin Wojtas > Subject: RE: [PATCH 1/1] MdeModulePkg/SdMmcPciHcDxe: Fix bus timing > switch sequence >=20 >=20 >=20 > > -----Original Message----- > > From: Wu, Hao A > > Sent: Thursday, September 26, 2019 3:36 AM > > To: Albecki, Mateusz ; devel@edk2.groups.io > > Cc: Marcin Wojtas > > Subject: RE: [PATCH 1/1] MdeModulePkg/SdMmcPciHcDxe: Fix bus timing > > switch sequence > > > > > -----Original Message----- > > > From: Albecki, Mateusz > > > Sent: Wednesday, September 25, 2019 10:54 PM > > > To: Wu, Hao A; devel@edk2.groups.io > > > Cc: Marcin Wojtas > > > Subject: RE: [PATCH 1/1] MdeModulePkg/SdMmcPciHcDxe: Fix bus > timing > > > switch sequence > > > > > > > > > > > > > -----Original Message----- > > > > From: Wu, Hao A > > > > Sent: Wednesday, September 25, 2019 5:34 AM > > > > To: Albecki, Mateusz ; > > > > devel@edk2.groups.io > > > > Cc: Marcin Wojtas > > > > Subject: RE: [PATCH 1/1] MdeModulePkg/SdMmcPciHcDxe: Fix bus > > timing > > > > switch sequence > > > > > > > > > -----Original Message----- > > > > > From: Albecki, Mateusz > > > > > Sent: Monday, September 23, 2019 4:37 PM > > > > > To: devel@edk2.groups.io > > > > > Cc: Albecki, Mateusz; Wu, Hao A; Marcin Wojtas > > > > > Subject: [PATCH 1/1] MdeModulePkg/SdMmcPciHcDxe: Fix bus > timing > > > > switch > > > > > sequence > > > > > > > > > > SD specification recommends switching card bus timing before > > > > > switching bus timing in controller. Emmc driver used to do this > > > > > switch other way around. This commit adds controller timing switc= h > > > > > in EmmcSwitchBusTiming function to enforce this order and removes > > > > > all controller timing programing from EmmcSwitchToXXX functions. > > > > > > > > > > > > Hello Mateusz, > > > > > > > > I think the changes in the patch look good. > > > > > > > > Could you help to address the below things: > > > > > > > > 1. The 'Private' local variable is no longer being used by below fu= nctions: > > > > EmmcSwitchToHighSpeed() > > > > EmmcSwitchToHS200() > > > > EmmcSwitchToHS400() > > > > > > > > The GCC compiler seems not happy with it, could you help to resolve= it? > > > > > > Sure > > > > > > > > 2. I have submitted a Bugzilla tracker for this issue at: > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=3D2218 > > > > > > > > Could you help to add this information in the commit log message? > > > > > > Sure > > > > > > > > 3. For the removal of bus clock stopping codes in > > > > EmmcSwitchToHS200(), could you split them into another separate > > > > patch? I think they are not > > > directly > > > > related with the bus speed mode changing sequence. > > > > > > > > > > I could do that sure, but since I have to remove the call to > > > SdMmcHcUhsSignaling from this function stopping and starting clock no > > > longer makes any sense so it would result in a nonsensical code in > > > this one patch. Anyway let me know if you still want to split it and = I will. > > > > > > My initial thought was to split the removal of bus clock stopping and > > resuming codes (before and after the call of function > > SdMmcHcUhsSignaling()) into the 1st patch of the series. Then a 2nd pat= ch > > will relocate the call of function > > SdMmcHcUhsSignaling() to fix the bus mode switching sequence issue. > > > > I think from your description in the cover-letter, such bus clock stopp= ing > and > > resuming is only required when the preset values are being used (which = is > > not the case for the current implementation of the driver). > > > > What do you think of this? > > >=20 > Yeah that makes sense. I will split it like that. >=20 > > > > > > > > > 4. I have performed the below tests on the eMMC device on my side, > > > > for different eMMC bus modes: > > > > HS400 (good) > > > > HS200 (good) > > > > High Speed DDR (good) > > > > High Speed SDR (good) > > > > Legacy MMC (NOT good) > > > > > > > > For the Legacy MMC mode case, the error comes from the > > > > EmmcSwitchToHighSpeed() function returning > EFI_INVALID_PARAMETER > > > for > > > > bus mode 'SdMmcMmcLegacy'. I think for the EmmcSetBusMode() > > > function, > > > > the below updates can be made: > > > > > > > > if (BusMode.BusTiming =3D=3D SdMmcMmcHs400) { > > > > Status =3D EmmcSwitchToHS400 (PciIo, PassThru, Slot, Rca, &BusM= ode); > > > > } else if (BusMode.BusTiming =3D=3D SdMmcMmcHs200) { > > > > Status =3D EmmcSwitchToHS200 (PciIo, PassThru, Slot, Rca, &BusM= ode); > > > > } else { <=3D=3D should be updated to: > > > > } else if (BusMode.BusTiming =3D=3D SdMmcMmcHsSdr || > > > BusMode.BusTiming > > > > =3D=3D SdMmcMmcHsDdr) { > > > > Status =3D EmmcSwitchToHighSpeed (PciIo, PassThru, Slot, Rca, > > > &BusMode); > > > > } > > > > > > > > think for the Legacy MMC mode, no additional bus mode switch is > > needed. > > > If > > > > you agree with this, could you help to add another patch to change > > > > the above logic (also the debug message after the above-shown code)= ? > > > > > > > > Best Regards, > > > > Hao Wu > > > > > > > > > > That is actually another bug introduced by the previous patch, right? > > > I will fix it in the separate patch(but in this series). > > > > > > Before the previous patch, I found that in EmmcSetBusMode(), the Legacy > > MMC mode will be handled in EmmcSwitchToHighSpeed(): > > > > } else if (((ExtCsd.DeviceType & BIT0) !=3D 0) && (Private- > > >Capability[Slot].HighSpeed !=3D 0)) { > > HsTiming =3D 1; > > IsDdr =3D FALSE; > > ClockFreq =3D 26; > > } > > > > ... > > > > if (HsTiming =3D=3D 3) { > > // > > // Execute HS400 timing switch procedure > > // > > Status =3D EmmcSwitchToHS400 (PciIo, PassThru, Slot, Rca, ClockFreq= ); > > } else if (HsTiming =3D=3D 2) { > > // > > // Execute HS200 timing switch procedure > > // > > Status =3D EmmcSwitchToHS200 (PciIo, PassThru, Slot, Rca, ClockFreq= , > > BusWidth); > > } else { > > // > > // Execute High Speed timing switch procedure > > // > > Status =3D EmmcSwitchToHighSpeed (PciIo, PassThru, Slot, Rca, Clock= Freq, > > IsDdr, BusWidth); > > } > > > > Also, there is logic for handling the case that no high speed mode is > > supported: > > > > if ((ClockFreq =3D=3D 0) || (HsTiming =3D=3D 0)) { > > // > > // Continue using default setting. > > // > > return EFI_SUCCESS; > > } > > > > After a second thought, the previous suggested solution does not seem > > proper (parameters like bus width and working clock frequency will not = get > > customized for the Legacy MMC mode). > > > > I think we can add a 3rd patch to: > > > > A. Match the check in EmmcIsBusTimingSupported() for the > > SdMmcMmcLegacy case > > with the above shown code. > > B. Restore to the previous handling approach that let the > > EmmcSwitchToHighSpeed() > > function to handle Legacy MMC mode. > > C. Add back codes to return directly when no high speed mode is > supported. > > > > Best Regards, > > Hao Wu > > >=20 > After closer inspection I think the problem might be a little more compli= cated. > According to eMMC 5.1 specification section 5.3.2 high speed SDR mode > supports clock frequency in the range 0-52MHz while 0-26MHz is reserved f= or > the backwards compatibility with MMC card(SdMmcMmcLegacy). This > suggests that frequencies <=3D26MHz are not high speed so we do not have = to > switch link to high speed timing. In the same specification it says that = if BIT0 in > DEVICE_TYPE field in EXT_CSD register is set then the card is "High speed > eMMC at 26MHz" which would suggest that high speed mode should be > enabled for those cards and we should just be careful to not exceed the > clock frequency of 26MHz. Yes. The mention of the: High-Speed eMMC at 26 MHz - at rated device voltage(s) by the EMMC spec under the description for Extended CSD register looks inconsistent with the high speed bus mode defined in other places in the sp= ec. >=20 > That said in section 6.6.2 which actually talks about high speed mode > selection spec is very clear that host should switch the card to high spe= ed > only if it wishes to operate at frequencies greater then 26MHz. Given tha= t I > think we should follow your proposition with minor modification. We will > keep treating the eMMC devices that have only set BIT0 in DEVICE_TYPE as > legacy devices and we will not switch the to high speed. The reason for t= hat is > that we will never exceed legacy clock frequencies anyway. The change wil= l > be basically limited to allowing SdMmcMmcLegacy bus timing in > EmmcSwitchToHighSpeed since I think EmmcSwitchBusTiming function is > ready to handle SdMmcMmcLegacy argument. Before commit adec1f5deb89c68d82598074500006bd575e8f58: * MdeModulePkg/SdMmcHcDxe: Implement revision 3 of SdMmcOverrideProtocol The driver attempted to switch to high speed bus mode for device whose maxi= mum working frequency is 26MHz (which maps to 'SdMmcMmcLegacy' bus timing in cu= rrent driver implementation): } else if (((ExtCsd.DeviceType & BIT0) !=3D 0) && (Private->Capability[S= lot].HighSpeed !=3D 0)) { HsTiming =3D 1; IsDdr =3D FALSE; ClockFreq =3D 26; } =20 (where setting 'HsTiming' to 1 means switching to high speed bus mode) However, I think the above codes do not have a chance to be executed, due t= o the lack of eMMC device that ONLY supports this bus timing mode. So based on the above findings, I agree with your proposal to NOT switching= to the high speed mode for SdMmcMmcLegacy bus timing. Best Regards, Hao Wu >=20 > > > > > > > > > > > > > > > > > > > Signed-off-by: Mateusz Albecki > > > > > Cc: Hao A Wu > > > > > Cc: Marcin Wojtas > > > > > --- > > > > > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 79 > > +++++++- > > > -- > > > > ---- > > > > > ----------- > > > > > 1 file changed, 20 insertions(+), 59 deletions(-) > > > > > > > > > > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > > > > > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > > > > > index 3f4a8e5413..06ee1208be 100644 > > > > > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > > > > > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > > > > > @@ -671,6 +671,7 @@ EmmcSwitchBusTiming ( > > > > > UINT8 CmdSet; > > > > > UINT32 DevStatus; > > > > > SD_MMC_HC_PRIVATE_DATA *Private; > > > > > + UINT8 HostCtrl1; > > > > > > > > > > Private =3D SD_MMC_HC_PRIVATE_FROM_THIS (PassThru); > > > > > // > > > > > @@ -704,6 +705,25 @@ EmmcSwitchBusTiming ( > > > > > return Status; > > > > > } > > > > > > > > > > + if (BusTiming =3D=3D SdMmcMmcHsSdr || BusTiming =3D=3D > > SdMmcMmcHsDdr) > > > { > > > > > + HostCtrl1 =3D BIT2; > > > > > + Status =3D SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL1, > > > > sizeof > > > > > (HostCtrl1), &HostCtrl1); > > > > > + if (EFI_ERROR (Status)) { > > > > > + return Status; > > > > > + } > > > > > + } else { > > > > > + HostCtrl1 =3D (UINT8)~BIT2; > > > > > + Status =3D SdMmcHcAndMmio (PciIo, Slot, > SD_MMC_HC_HOST_CTRL1, > > > > > sizeof (HostCtrl1), &HostCtrl1); > > > > > + if (EFI_ERROR (Status)) { > > > > > + return Status; > > > > > + } > > > > > + } > > > > > + > > > > > + Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, Pci= Io, > > > > > + Slot, > > > > > BusTiming); > > > > > + if (EFI_ERROR (Status)) { > > > > > + return Status; > > > > > + } > > > > > + > > > > > // > > > > > // Convert the clock freq unit from MHz to KHz. > > > > > // > > > > > @@ -772,7 +792,6 @@ EmmcSwitchToHighSpeed ( > > > > > ) > > > > > { > > > > > EFI_STATUS Status; > > > > > - UINT8 HostCtrl1; > > > > > SD_MMC_HC_PRIVATE_DATA *Private; > > > > > BOOLEAN IsDdr; > > > > > > > > > > @@ -794,20 +813,6 @@ EmmcSwitchToHighSpeed ( > > > > > return Status; > > > > > } > > > > > > > > > > - // > > > > > - // Set to High Speed timing > > > > > - // > > > > > - HostCtrl1 =3D BIT2; > > > > > - Status =3D SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_HOST_CTRL1, > > > > sizeof > > > > > (HostCtrl1), &HostCtrl1); > > > > > - if (EFI_ERROR (Status)) { > > > > > - return Status; > > > > > - } > > > > > - > > > > > - Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, Pci= Io, > > > > > Slot, > > > > > BusMode->BusTiming); > > > > > - if (EFI_ERROR (Status)) { > > > > > - return Status; > > > > > - } > > > > > - > > > > > return EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, > > > > > BusMode- > > > > > >DriverStrength, BusMode->BusTiming, BusMode->ClockFreq); > > > > > } > > > > > > > > > > @@ -837,7 +842,6 @@ EmmcSwitchToHS200 ( > > > > > ) > > > > > { > > > > > EFI_STATUS Status; > > > > > - UINT16 ClockCtrl; > > > > > SD_MMC_HC_PRIVATE_DATA *Private; > > > > > > > > > > Private =3D SD_MMC_HC_PRIVATE_FROM_THIS (PassThru); @@ - > > 851,39 > > > > +855,6 > > > > > @@ EmmcSwitchToHS200 ( > > > > > if (EFI_ERROR (Status)) { > > > > > return Status; > > > > > } > > > > > - // > > > > > - // Stop bus clock at first > > > > > - // > > > > > - Status =3D SdMmcHcStopClock (PciIo, Slot); > > > > > - if (EFI_ERROR (Status)) { > > > > > - return Status; > > > > > - } > > > > > - > > > > > - Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, Pci= Io, > > > > > Slot, > > > > > BusMode->BusTiming); > > > > > - if (EFI_ERROR (Status)) { > > > > > - return Status; > > > > > - } > > > > > - > > > > > - // > > > > > - // Wait Internal Clock Stable in the Clock Control register to > > > > > be 1 before set SD Clock Enable bit > > > > > - // > > > > > - Status =3D SdMmcHcWaitMmioSet ( > > > > > - PciIo, > > > > > - Slot, > > > > > - SD_MMC_HC_CLOCK_CTRL, > > > > > - sizeof (ClockCtrl), > > > > > - BIT1, > > > > > - BIT1, > > > > > - SD_MMC_HC_GENERIC_TIMEOUT > > > > > - ); > > > > > - if (EFI_ERROR (Status)) { > > > > > - return Status; > > > > > - } > > > > > - // > > > > > - // Set SD Clock Enable in the Clock Control register to 1 > > > > > - // > > > > > - ClockCtrl =3D BIT2; > > > > > - Status =3D SdMmcHcOrMmio (PciIo, Slot, SD_MMC_HC_CLOCK_CTRL, > > > > sizeof > > > > > (ClockCtrl), &ClockCtrl); > > > > > > > > > > Status =3D EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, > > > > > BusMode- > > > > > >DriverStrength, BusMode->BusTiming, BusMode->ClockFreq); > > > > > if (EFI_ERROR (Status)) { > > > > > @@ -945,11 +916,6 @@ EmmcSwitchToHS400 ( > > > > > // Set to High Speed timing and set the clock frequency to a > > > > > value less than or equal to 52MHz. > > > > > // This step is necessary to be able to switch Bus into 8 bit > > > > > DDR mode which is unsupported in HS200. > > > > > // > > > > > - Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, Pci= Io, > > > > > Slot, SdMmcMmcHsSdr); > > > > > - if (EFI_ERROR (Status)) { > > > > > - return Status; > > > > > - } > > > > > - > > > > > HsFreq =3D BusMode->ClockFreq < 52 ? BusMode->ClockFreq : 52; > > > > > Status =3D EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, > > > > > BusMode- > > > > > >DriverStrength, SdMmcMmcHsSdr, HsFreq); > > > > > if (EFI_ERROR (Status)) { > > > > > @@ -961,11 +927,6 @@ EmmcSwitchToHS400 ( > > > > > return Status; > > > > > } > > > > > > > > > > - Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, Pci= Io, > > > > > Slot, > > > > > BusMode->BusTiming); > > > > > - if (EFI_ERROR (Status)) { > > > > > - return Status; > > > > > - } > > > > > - > > > > > return EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, > > > > > BusMode- > > > > > >DriverStrength, BusMode->BusTiming, BusMode->ClockFreq); > > > > > } > > > > > > > > > > -- > > > > > 2.14.1.windows.1 > > > > > > > > > >=20