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; Tue, 24 Sep 2019 20:33:50 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Sep 2019 20:33:49 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,546,1559545200"; d="scan'208";a="390078840" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga006.fm.intel.com with ESMTP; 24 Sep 2019 20:33:49 -0700 Received: from fmsmsx154.amr.corp.intel.com (10.18.116.70) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 24 Sep 2019 20:33:49 -0700 Received: from shsmsx153.ccr.corp.intel.com (10.239.6.53) by FMSMSX154.amr.corp.intel.com (10.18.116.70) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 24 Sep 2019 20:33:49 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.32]) by SHSMSX153.ccr.corp.intel.com ([169.254.12.235]) with mapi id 14.03.0439.000; Wed, 25 Sep 2019 11:33:46 +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/ykuom8W5GHGiJKc6gc0A Date: Wed, 25 Sep 2019 03:33:45 +0000 Message-ID: References: <20190923083701.1496-1-mateusz.albecki@intel.com> <20190923083701.1496-2-mateusz.albecki@intel.com> In-Reply-To: <20190923083701.1496-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: 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 >=20 > 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 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 functions: EmmcSwitchToHighSpeed() EmmcSwitchToHS200() EmmcSwitchToHS400() The GCC compiler seems not happy with it, could you help to resolve it? 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? 3. For the removal of bus clock stopping codes in EmmcSwitchToHS200(), coul= d you split them into another separate patch? I think they are not directly related with the bus speed mode changing sequence. 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 EmmcSwitchToHighSpee= d() function returning EFI_INVALID_PARAMETER for bus mode 'SdMmcMmcLegacy'. I t= hink 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 abov= e logic (also the debug message after the above-shown code)? 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(-) >=20 > 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; >=20 > Private =3D SD_MMC_HC_PRIVATE_FROM_THIS (PassThru); > // > @@ -704,6 +705,25 @@ EmmcSwitchBusTiming ( > return Status; > } >=20 > + 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, 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; >=20 > @@ -794,20 +813,6 @@ EmmcSwitchToHighSpeed ( > return Status; > } >=20 > - // > - // 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); > } >=20 > @@ -837,7 +842,6 @@ EmmcSwitchToHS200 ( > ) > { > EFI_STATUS Status; > - UINT16 ClockCtrl; > SD_MMC_HC_PRIVATE_DATA *Private; >=20 > 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 be= fore > 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); >=20 > 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 les= s 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, BusMode- > >DriverStrength, SdMmcMmcHsSdr, HsFreq); > if (EFI_ERROR (Status)) { > @@ -961,11 +927,6 @@ EmmcSwitchToHS400 ( > return Status; > } >=20 > - 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); > } >=20 > -- > 2.14.1.windows.1