From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.136, mailfrom: hao.a.wu@intel.com) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by groups.io with SMTP; Wed, 25 Sep 2019 18:35:36 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Sep 2019 18:35:36 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,549,1559545200"; d="scan'208";a="214245123" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga004.fm.intel.com with ESMTP; 25 Sep 2019 18:35:36 -0700 Received: from fmsmsx114.amr.corp.intel.com (10.18.116.8) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 25 Sep 2019 18:35:35 -0700 Received: from shsmsx154.ccr.corp.intel.com (10.239.6.54) by FMSMSX114.amr.corp.intel.com (10.18.116.8) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 25 Sep 2019 18:35:35 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.32]) by SHSMSX154.ccr.corp.intel.com ([169.254.7.195]) with mapi id 14.03.0439.000; Thu, 26 Sep 2019 09:35:32 +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/ykuom8W5GHGiJKc6gc0AgAF2VQCAAScZIA== Date: Thu, 26 Sep 2019 01:35:32 +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: 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 >=20 >=20 >=20 > > -----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 switchin= g > > > bus timing in controller. Emmc driver used to do this switch other wa= y > > > around. This commit adds controller timing switch 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 functi= ons: > > EmmcSwitchToHighSpeed() > > EmmcSwitchToHS200() > > EmmcSwitchToHS400() > > > > The GCC compiler seems not happy with it, could you help to resolve it? >=20 > 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? >=20 > 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. > > >=20 > 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 o= ne > 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 resum= ing codes (before and after the call of function SdMmcHcUhsSignaling()) into th= e 1st patch of the series. Then a 2nd patch will relocate the call of functio= n SdMmcHcUhsSignaling() to fix the bus mode switching sequence issue. I think from your description in the cover-letter, such bus clock stopping = and resuming is only required when the preset values are being used (which is n= ot the case for the current implementation of the driver). What do you think of this? >=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, &BusMode)= ; > > } else if (BusMode.BusTiming =3D=3D SdMmcMmcHs200) { > > Status =3D EmmcSwitchToHS200 (PciIo, PassThru, Slot, Rca, &BusMode)= ; > > } 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 > > >=20 > That is actually another bug introduced by the previous patch, right? I w= ill 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[S= lot].HighSpeed !=3D 0)) { HsTiming =3D 1; IsDdr =3D FALSE; ClockFreq =3D 26; } =20 ... =20 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, Bu= sWidth); } else { // // Execute High Speed timing switch procedure // Status =3D EmmcSwitchToHighSpeed (PciIo, PassThru, Slot, Rca, ClockFreq= , IsDdr, BusWidth); } Also, there is logic for handling the case that no high speed mode is suppo= rted: 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 prope= r (parameters like bus width and working clock frequency will not get customi= zed for the Legacy MMC mode). I think we can add a 3rd patch to: A. Match the check in EmmcIsBusTimingSupported() for the SdMmcMmcLegacy cas= e with the above shown code. B. Restore to the previous handling approach that let the EmmcSwitchToHighS= peed() 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 > > > > > > > > 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 SdMmcMmcHsD= dr) > { > > > + 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, PciIo, > > > + 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, PciIo, > > > 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, PciIo, > > > 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, BusMod= e- > > > >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, PciIo, > > > Slot, SdMmcMmcHsSdr); > > > - if (EFI_ERROR (Status)) { > > > - return Status; > > > - } > > > - > > > HsFreq =3D BusMode->ClockFreq < 52 ? BusMode->ClockFreq : 52; > > > Status =3D EmmcSwitchBusTiming (PciIo, PassThru, Slot, Rca, BusMod= e- > > > >DriverStrength, SdMmcMmcHsSdr, HsFreq); > > > if (EFI_ERROR (Status)) { > > > @@ -961,11 +927,6 @@ EmmcSwitchToHS400 ( > > > return Status; > > > } > > > > > > - Status =3D SdMmcHcUhsSignaling (Private->ControllerHandle, PciIo, > > > 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