From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@intel.onmicrosoft.com header.s=selector2-intel-onmicrosoft-com header.b=wrny8EAV; spf=pass (domain: intel.com, ip: 134.134.136.126, mailfrom: mateusz.albecki@intel.com) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by groups.io with SMTP; Wed, 25 Sep 2019 07:54:10 -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 orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Sep 2019 07:54:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,548,1559545200"; d="scan'208";a="390235255" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga006.fm.intel.com with ESMTP; 25 Sep 2019 07:54:07 -0700 Received: from fmsmsx116.amr.corp.intel.com (10.18.116.20) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 25 Sep 2019 07:54:07 -0700 Received: from FMSEDG002.ED.cps.intel.com (10.1.192.134) by fmsmsx116.amr.corp.intel.com (10.18.116.20) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 25 Sep 2019 07:54:06 -0700 Received: from NAM01-BN3-obe.outbound.protection.outlook.com (104.47.33.55) by edgegateway.intel.com (192.55.55.69) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 25 Sep 2019 07:54:07 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=dAC6sCU9iaJM/ctul4FQ0QNV0X7H6OM985PlDqBn/YM8NS5lvxWzgGz2+Ksi7H7aH98aX+/Ela5PMlgozgD0Qh1uqmDIpnC2DuTHfNEc47/tJzOZpqeC++z1kRARzRGRfTE1t/0KgOy2i3AYJXFRJvgfq2Qu8auBmDIP90MdTDah1L+VNlayz7YLaLygHRn7pSF3Jjnzl/vkqh66benSBVZYbG+YYX4l9XMNusoZmvGTyYWPnW9R9gDt/j11Ptvm0e29Z/Hhgyz7cM2+1+CliprvGsGZ6tgyePW0dFLly8b0ADeaLNVdymgcEnTsbKcMGFekQHep0UBBT/Xwn4P9Yw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=0EHG4QPJ7l1YS2VqSxCAAeEzPmGElz09Q950wVDd1Q0=; b=ZuZqzz/AG3/Zaq0L6iA/Ona23a+Ncm1qVcyaAzVGBceFDHSlqV595NrsWV7f6BVBKaDKoV1g01KR5ll5V34Cmu/OIvoqZWTklR+tB3jjFvwTE4b8b7BiQ/dG3INoJHm2gbyPwIV6xwO7+GEJL7sPw2nFfcthr63hSWoTtwhck7q3fA2ITyTGOXuIJGa6xbOT4d26TSZC3dr3dTPoXQgqa4WY3JKShceseIV1Qc+PILLBFy83gBTluOlES1H7M6t1ihO2TanWZiMixnJiTMzGKYh9gMsSFouw83JR0BDPUupW40CX5d+0wqPpq8IZsDy9zv+4zwPxgynUZZKdhjC8cw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel.onmicrosoft.com; s=selector2-intel-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=0EHG4QPJ7l1YS2VqSxCAAeEzPmGElz09Q950wVDd1Q0=; b=wrny8EAVr1hypL99V/k5J9PWCGu64Yygjf4leNKi5uM63AiCZN+Zwjhy75p3exzGs+DkazHEwy5c4uSW6ScEJCwBVh+mpztTcH7hJEQ/HMtyKkfoWwTH8c8wc3JsTNvrVGwxk2LiV3M0igGxnou0/aF5dFpyvdzCWWWDn0bF6/8= Received: from CY4PR11MB1302.namprd11.prod.outlook.com (10.173.16.20) by CY4PR11MB1445.namprd11.prod.outlook.com (10.172.67.141) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2284.26; Wed, 25 Sep 2019 14:53:48 +0000 Received: from CY4PR11MB1302.namprd11.prod.outlook.com ([fe80::292e:5325:2ae2:2be8]) by CY4PR11MB1302.namprd11.prod.outlook.com ([fe80::292e:5325:2ae2:2be8%8]) with mapi id 15.20.2284.023; Wed, 25 Sep 2019 14:53:48 +0000 From: "Albecki, Mateusz" To: "Wu, Hao A" , "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: AQHVceo0az7spkfcA0CcYZl+L8fZh6c7wD2AgAC7+BA= Date: Wed, 25 Sep 2019 14:53:48 +0000 Message-ID: References: <20190923083701.1496-1-mateusz.albecki@intel.com> <20190923083701.1496-2-mateusz.albecki@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYTJkNWQ2MmYtY2I3Zi00ZDljLTk0MjgtMDQyNWRmYTNmOTQyIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiN3ErXC95ZUVud3Y2Q1ErdkR6MGQrbXZwa1wvUFZXYXVMcWFxNHc3cXZhbXRSTnVsWHRiWXVxQktvZlFrY3o5elZOIn0= dlp-product: dlpe-windows x-ctpclassification: CTP_NT dlp-reaction: no-action dlp-version: 11.2.0.6 authentication-results: spf=none (sender IP is ) smtp.mailfrom=mateusz.albecki@intel.com; x-originating-ip: [192.55.79.104] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 15fd2f20-dd25-4b4d-95d6-08d741c82c27 x-microsoft-antispam: BCL:0;PCL:0;RULEID:(2390118)(7020095)(4652040)(8989299)(5600167)(711020)(4605104)(1401327)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(2017052603328)(7193020);SRVR:CY4PR11MB1445; x-ms-traffictypediagnostic: CY4PR11MB1445: x-ms-exchange-purlcount: 1 x-ld-processed: 46c98d88-e344-4ed4-8496-4ed7712e255d,ExtAddr x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:8882; x-forefront-prvs: 01713B2841 x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(6029001)(366004)(376002)(396003)(39860400002)(136003)(346002)(199004)(189003)(13464003)(52536014)(2501003)(76116006)(6246003)(66476007)(66446008)(64756008)(66556008)(8936002)(33656002)(25786009)(66066001)(4326008)(9686003)(66946007)(55016002)(81156014)(81166006)(6306002)(74316002)(316002)(110136005)(305945005)(8676002)(86362001)(229853002)(6436002)(446003)(14444005)(2906002)(256004)(7736002)(53546011)(6506007)(102836004)(5660300002)(26005)(186003)(3846002)(6116002)(14454004)(966005)(71200400001)(71190400001)(478600001)(476003)(76176011)(99286004)(486006)(11346002)(7696005);DIR:OUT;SFP:1102;SCL:1;SRVR:CY4PR11MB1445;H:CY4PR11MB1302.namprd11.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;A:1;MX:1; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam-message-info: zekPqMYYYJZBLw096mOd1GUWwlW1CRvEsAGGxjX0TSDttck4dSqwZKv4iJH3fMrPzfGuJS344oPa7qRe+ShMKlQPJZPbjdzdSBsHLVvpM12beu0XzJeUloTAQfQ82mvJ3m++T1ejCoQE29w1Vrr2JA56TUyOPGuuMqKQdpC//J7bSOSGF2wk75wG3d9CwzUfrS4W3AxH3XlWSg+VFAEUP7rmZa0Xi/rVfJZuWZCH7plF8YceJnv8JNo51+AWfTLlwMn3avraEZKvOQkiNDUvFijmpCVgAD/bYufIr5TRFgQXIuGI2Mgm9yDdKJdrp6GKZBOONj+mreLndOmguKozkyPlBfQoVqts4SPNehk2HujvLq/DWqEIL+yBvRhwyE50mXcwcuzQ2bcGm/vQeGvTVd0YbayNQ7X/E+vkIGNcPf7RbbOMasdj8mB4G9czTRLm3kJkV4QBeDLkTpjGPVywXQ== MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: 15fd2f20-dd25-4b4d-95d6-08d741c82c27 X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Sep 2019 14:53:48.3532 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: jP0/6QddFt0YLq5b/DuKi6hF23mxn7l3rxSHDKb44QCWMvsv0Hn2CIu9+awwRcCZa/bPRP8V3hY+0d9etctRNax68fCjRe/wwqyuMHJAw+o= X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR11MB1445 Return-Path: mateusz.albecki@intel.com X-OriginatorOrg: intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----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 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 function= s: > 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 di= rectly > related with the bus speed mode changing sequence. > = I could do that sure, but since I have to remove the call to SdMmcHcUhsSign= aling from this function stopping and starting clock no longer makes any se= nse 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. = > 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, &BusMod= e); > } > = > 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 wil= l fix it in the separate patch(but in this series). > = > > > > 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, 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, 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, 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; > > } > > > > - 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 > = -------------------------------------------------------------------- Intel Technology Poland sp. z o.o. ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydz= ial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-31= 6 | Kapital zakladowy 200.000 PLN. Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata= i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wi= adomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiek= olwiek przegladanie lub rozpowszechnianie jest zabronione. This e-mail and any attachments may contain confidential material for the s= ole use of the intended recipient(s). If you are not the intended recipient= , please contact the sender and delete all copies; any review or distributi= on by others is strictly prohibited.