From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by mx.groups.io with SMTP id smtpd.web11.17719.1582527668384599639 for ; Sun, 23 Feb 2020 23:01:08 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.24, mailfrom: hao.a.wu@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Feb 2020 23:01:07 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,479,1574150400"; d="scan'208";a="225885286" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga007.jf.intel.com with ESMTP; 23 Feb 2020 23:01:06 -0800 Received: from fmsmsx608.amr.corp.intel.com (10.18.126.88) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.439.0; Sun, 23 Feb 2020 23:01:03 -0800 Received: from fmsmsx608.amr.corp.intel.com (10.18.126.88) by fmsmsx608.amr.corp.intel.com (10.18.126.88) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Sun, 23 Feb 2020 23:01:03 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by fmsmsx608.amr.corp.intel.com (10.18.126.88) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Sun, 23 Feb 2020 23:01:03 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.5]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.196]) with mapi id 14.03.0439.000; Mon, 24 Feb 2020 15:01:00 +0800 From: "Wu, Hao A" To: "Albecki, Mateusz" , "devel@edk2.groups.io" CC: Marcin Wojtas , "Gao, Zhichao" , "Gao, Liming" Subject: Re: [PATCH 1/1] MdeModulePkg/SdMmcPciHcDxe: Send SEND_STATUS at lower frequency Thread-Topic: [PATCH 1/1] MdeModulePkg/SdMmcPciHcDxe: Send SEND_STATUS at lower frequency Thread-Index: AQHV6MoSCFL6no1++0OetyZ2GHO/Bqgp42Ng Date: Mon, 24 Feb 2020 07:00:59 +0000 Message-ID: References: <20200221151719.4248-1-mateusz.albecki@intel.com> <20200221151719.4248-2-mateusz.albecki@intel.com> In-Reply-To: <20200221151719.4248-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: Friday, February 21, 2020 11:17 PM > To: devel@edk2.groups.io > Cc: Albecki, Mateusz; Wu, Hao A; Marcin Wojtas; Gao, Zhichao; Gao, Liming > Subject: [PATCH 1/1] MdeModulePkg/SdMmcPciHcDxe: Send SEND_STATUS > at lower frequency >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1140 >=20 > To avoid stability issues on some designs the driver > will now send SEND_STATUS at previous, lower, frequency > when upgrading the bus timing. Hello Mateusz, The patch looks like a refinement. So I plan to push it (after the reviewin= g process) after the upcoming stable tag. Does it work for you? A couple of inline comments below: >=20 > Cc: Hao A Wu > Cc: Marcin Wojtas > Cc: Zhichao Gao > Cc: Liming Gao >=20 > Signed-off-by: Mateusz Albecki > --- > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c | 124 > +++++++++++++++++---- > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 2 +- > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h | 1 + > MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c | 2 + > 4 files changed, 105 insertions(+), 24 deletions(-) >=20 > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > index 776c0e796c..c2ebd37623 100644 > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/EmmcDevice.c > @@ -558,6 +558,41 @@ EmmcTuningClkForHs200 ( > return EFI_DEVICE_ERROR; > } >=20 > +/** > + Check the SWITCH operation status. > + > + @param[in] PassThru A pointer to the > EFI_SD_MMC_PASS_THRU_PROTOCOL instance. Please help to add the comments for parameters 'Slot' and 'Rca'. > + > + @retval EFI_SUCCESS The SWITCH finished siccessfully. > + @retval others The SWITCH failed. > +**/ > +EFI_STATUS > +EmmcCheckSwitchStatus ( > + IN EFI_SD_MMC_PASS_THRU_PROTOCOL *PassThru, > + IN UINT8 Slot, > + IN UINT16 Rca > + ) > +{ > + EFI_STATUS Status; > + UINT32 DevStatus; > + > + Status =3D EmmcSendStatus (PassThru, Slot, Rca, &DevStatus); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "EmmcCheckSwitchStatus: Send status fails > with %r\n", Status)); > + return Status; > + } > + > + // > + // Check the switch operation is really successful or not. > + // > + if ((DevStatus & BIT7) !=3D 0) { > + DEBUG ((DEBUG_ERROR, "EmmcCheckSwitchStatus: The switch > operation fails as DevStatus is 0x%08x\n", DevStatus)); > + return EFI_DEVICE_ERROR; > + } > + > + return EFI_SUCCESS; > +} > + > /** > Switch the bus width to specified width. >=20 > @@ -591,7 +626,6 @@ EmmcSwitchBusWidth ( > UINT8 Index; > UINT8 Value; > UINT8 CmdSet; > - UINT32 DevStatus; >=20 > // > // Write Byte, the Value field is written into the byte pointed by Ind= ex. > @@ -617,24 +651,53 @@ EmmcSwitchBusWidth ( > return Status; > } >=20 > - Status =3D EmmcSendStatus (PassThru, Slot, Rca, &DevStatus); > + Status =3D EmmcCheckSwitchStatus (PassThru, Slot, Rca); > if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, "EmmcSwitchBusWidth: Send status fails > with %r\n", Status)); > return Status; > } > - // > - // Check the switch operation is really successful or not. > - // > - if ((DevStatus & BIT7) !=3D 0) { > - DEBUG ((DEBUG_ERROR, "EmmcSwitchBusWidth: The switch operation > fails as DevStatus is 0x%08x\n", DevStatus)); > - return EFI_DEVICE_ERROR; > - } >=20 > Status =3D SdMmcHcSetBusWidth (PciIo, Slot, BusWidth); >=20 > return Status; > } >=20 > +/** > + Checks if given clock frequency is supported on > + given bus timing. > + > + @param[in] ClockFreq Clock frequency to check in KHz. > + @param[in] BusTiming Bus timing against which frequency will be > compared. > + > + @retval TRUE Frequency is valid for given bus timing. > + @retval FALSE Frequency is invalid for given bus timing. > +**/ > +BOOLEAN > +EmmcIsFrequencySupportedOnBusTiming ( > + IN UINT64 ClockFreq, > + IN SD_MMC_BUS_MODE BusTiming > + ) > +{ > + UINT32 MaxFreq; > + > + switch (BusTiming) { > + case SdMmcMmcLegacy: > + MaxFreq =3D 26000; > + break; > + case SdMmcMmcHsSdr: > + case SdMmcMmcHsDdr: > + MaxFreq =3D 52000; > + break; > + case SdMmcMmcHs200: > + case SdMmcMmcHs400: > + MaxFreq =3D 200000; > + break; > + default: > + return FALSE; > + } > + > + return (ClockFreq <=3D MaxFreq); > +} > + > /** > Switch the bus timing and clock frequency. >=20 > @@ -669,9 +732,9 @@ EmmcSwitchBusTiming ( > UINT8 Index; > UINT8 Value; > UINT8 CmdSet; > - UINT32 DevStatus; > SD_MMC_HC_PRIVATE_DATA *Private; > UINT8 HostCtrl1; > + BOOLEAN DelaySendStatus; >=20 > Private =3D SD_MMC_HC_PRIVATE_FROM_THIS (PassThru); > // > @@ -695,7 +758,7 @@ EmmcSwitchBusTiming ( > Value =3D 0; > break; > default: > - DEBUG ((DEBUG_ERROR, "EmmcSwitchBusTiming: Unsupported > BusTiming(%d\n)", BusTiming)); > + DEBUG ((DEBUG_ERROR, "EmmcSwitchBusTiming: Unsupported > BusTiming(%d)\n", BusTiming)); > return EFI_INVALID_PARAMETER; > } >=20 > @@ -724,6 +787,27 @@ EmmcSwitchBusTiming ( > return Status; > } >=20 > + // > + // For cases when we switch bus timing to higher mode from current we > want to > + // send SEND_STATUS at current, lower, frequency then the target > frequency to avoid > + // stability issues. It has been observed that some designs are unable= to > process the > + // SEND_STATUS at higher frequency during switch to HS200 @200MHz > irrespective of the number of retries > + // and only running the clock tuning is able to make them work on targ= et > frequency. > + // > + // For cases when we are downgrading the frequency and current high > frequency is invalid > + // we have to first change the frequency to target frequency and then = send > the SEND_STATUS. > + // > + if (EmmcIsFrequencySupportedOnBusTiming (Private- > >Slot[Slot].CurrentFreq, BusTiming) && One question here. I am not sure if we need to perform the above frequency check by EmmcIsFrequencySupportedOnBusTiming(). My understanding is that the below functions call sequence: EmmcSetBusMode() -> EmmcGetTargetBusMode() -> EmmcGetTargetClockFreq() can ensure target frequency specified by 'ClockFreq' is supported by the ta= rget bus mode specified by 'BusTiming'. If current working frequency is smaller = than the target frequency, it should be supported by the target bus mode as well= . Best Regards, Hao Wu > + (Private->Slot[Slot].CurrentFreq < (ClockFreq * 1000))) { > + Status =3D EmmcCheckSwitchStatus (PassThru, Slot, Rca); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + DelaySendStatus =3D FALSE; > + } else { > + DelaySendStatus =3D TRUE; > + } > + > // > // Convert the clock freq unit from MHz to KHz. > // > @@ -732,17 +816,11 @@ EmmcSwitchBusTiming ( > return Status; > } >=20 > - Status =3D EmmcSendStatus (PassThru, Slot, Rca, &DevStatus); > - if (EFI_ERROR (Status)) { > - DEBUG ((DEBUG_ERROR, "EmmcSwitchBusTiming: Send status fails > with %r\n", Status)); > - return Status; > - } > - // > - // Check the switch operation is really successful or not. > - // > - if ((DevStatus & BIT7) !=3D 0) { > - DEBUG ((DEBUG_ERROR, "EmmcSwitchBusTiming: The switch operation > fails as DevStatus is 0x%08x\n", DevStatus)); > - return EFI_DEVICE_ERROR; > + if (DelaySendStatus) { > + Status =3D EmmcCheckSwitchStatus (PassThru, Slot, Rca); > + if (EFI_ERROR (Status)) { > + return Status; > + } > } >=20 > return Status; > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c > index b18ff3e972..57f4cf329a 100644 > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c > @@ -28,7 +28,7 @@ EFI_DRIVER_BINDING_PROTOCOL > gSdMmcPciHcDriverBinding =3D { > NULL > }; >=20 > -#define SLOT_INIT_TEMPLATE {0, UnknownSlot, 0, 0, 0, \ > +#define SLOT_INIT_TEMPLATE {0, UnknownSlot, 0, 0, 0, 0, \ > {EDKII_SD_MMC_BUS_WIDTH_IGNORE,\ > EDKII_SD_MMC_CLOCK_FREQ_IGNORE,\ > {EDKII_SD_MMC_DRIVER_STRENGTH_IGNORE}}} > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h > index 5bc3577ba2..bb3d38482f 100644 > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h > @@ -83,6 +83,7 @@ typedef struct { > BOOLEAN MediaPresent; > BOOLEAN Initialized; > SD_MMC_CARD_TYPE CardType; > + UINT64 CurrentFreq; > EDKII_SD_MMC_OPERATING_PARAMETERS OperatingParameters; > } SD_MMC_HC_SLOT; >=20 > diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > index 43626fff48..7971196a25 100644 > --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHci.c > @@ -931,6 +931,8 @@ SdMmcHcClockSupply ( > } > } >=20 > + Private->Slot[Slot].CurrentFreq =3D ClockFreq; > + > return Status; > } >=20 > -- > 2.14.1.windows.1