From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=40.107.76.121; helo=nam02-cy1-obe.outbound.protection.outlook.com; envelope-from=christopher.co@microsoft.com; receiver=edk2-devel@lists.01.org Received: from NAM02-CY1-obe.outbound.protection.outlook.com (mail-eopbgr760121.outbound.protection.outlook.com [40.107.76.121]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 28F67211958E1 for ; Mon, 3 Dec 2018 17:41:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=3mY5wlDLSLsqoNhoRlWVGiDAliz13bBIoom8F2SKco0=; b=cjw0cdkNMD6tRyY5HYOccqa69JlkOPzkXH+WZp98qy7RgVuLWnfLvei1wJ6E7eexbpIrvjH0tKtoc+jxbGa7tkjpizwmzJUnIBdaT+ZikVUVjDOaRxORIU/VxwFUbgOENNTHzxUdmVVTRLRlREVDWZwOucFzm2VF0e9CPuTMFoU= Received: from DM5PR21MB0186.namprd21.prod.outlook.com (10.173.173.137) by DM5PR21MB0140.namprd21.prod.outlook.com (10.173.173.15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1425.4; Tue, 4 Dec 2018 01:41:18 +0000 Received: from DM5PR21MB0186.namprd21.prod.outlook.com ([fe80::9115:7193:d145:d0f5]) by DM5PR21MB0186.namprd21.prod.outlook.com ([fe80::9115:7193:d145:d0f5%6]) with mapi id 15.20.1425.005; Tue, 4 Dec 2018 01:41:18 +0000 From: Chris Co To: Leif Lindholm CC: "edk2-devel@lists.01.org" , Ard Biesheuvel , Michael D Kinney Thread-Topic: [PATCH edk2-platforms 12/27] Silicon/NXP: Add i.MX6 I/O MUX library Thread-Index: AQHUUYS8K8ZRkLJZhEm6rexKIPRBDKVGdsiAgCfJKWA= Date: Tue, 4 Dec 2018 01:41:17 +0000 Message-ID: References: <20180921082542.35768-1-christopher.co@microsoft.com> <20180921082542.35768-13-christopher.co@microsoft.com> <20181108180018.mqi6yvadi7nkiolt@bivouac.eciton.net> In-Reply-To: <20181108180018.mqi6yvadi7nkiolt@bivouac.eciton.net> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: msip_labels: MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Enabled=True; MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SiteId=72f988bf-86f1-41af-91ab-2d7cd011db47; MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Owner=chrco@microsoft.com; MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SetDate=2018-12-04T01:41:15.8756362Z; MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Name=General; MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Application=Microsoft Azure Information Protection; MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Extended_MSFT_Method=Automatic; Sensitivity=General x-originating-ip: [2001:4898:80e8:1:6928:7d3d:7cac:2222] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; DM5PR21MB0140; 6:JS24YfhAm5h4vbXhHoTHrxH57CcfPs/V5vuLRVlGhvq02z73akmrj7hPtxeYy7eefar9TTXZzT4WLaxXzPXaaDg3+0iPrwRxfsY1IVOdOW14y7VK9m84rm8v/vKipU1e6TKXqNDu15ZOwdWn3GCR9oeCgBGH3MxLUXom+xjqjEnkTveLK+bXKDpdEX6lY3lrL+wuxZ/lqZKFYhvprktTjnXm+d5jp0Gm0qiUO6BDQYNCz3WGl1RUd1L94a+lnGTIKuUFrNQyFRsdPak95XjOIn1Bzig7KEnSp5HV/vxRzJsSKZdq4NfXHyeU/cZiuR2UKZsO7MILUBgFD1DK1JBoTyVaMvcEphCQbbHzCjWXZ/aEhRk7jHZhG3H4NyzEZYf2FpcABe3tevpaEq8Sg7gWaCjrMKWda+abG0hXcYC8sCL2jAxm96ISDWUXzixqVs3LEezL3DfTVu/BLicUU0sVAg==; 5:uMeFQwBomd2OrxnA+gcldtvxIFpFaVJsH/Fijl07U5GE5MCbU19RQRCXG2qIwXPHikH66AP1UJ1T9/09eKR77KibEMGtsZpN/wR8syoY0biSMJWp2RhHSB+kqwAUL4XRsXX60fcGQ4aVu/O8sU+mkhBlUW3p1opcsPd9ZfRBLi0=; 7:Iuc/yMNCpV2SPY/QFfWYFA4KsBvP6esl54ohElYnNNNKLAVTnnOOkfdQAkIAq7AhOuyC4HxMB6iDR61Qg2lTWGdw2qfjk9vyjSSzpfyp9CKPIThaqs71Cx28zDDAYS78QfYmh4TJ7KTH39DcoA6xeg== x-ms-exchange-antispam-srfa-diagnostics: SOS; x-ms-office365-filtering-correlation-id: 31057416-ca8b-4c6c-4427-08d6598995fd x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0; PCL:0; RULEID:(2390098)(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600074)(711020)(4618075)(2017052603328)(7193020); SRVR:DM5PR21MB0140; x-ms-traffictypediagnostic: DM5PR21MB0140: x-ms-exchange-purlcount: 2 authentication-results: spf=none (sender IP is ) smtp.mailfrom=Christopher.Co@microsoft.com; x-microsoft-antispam-prvs: x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(8220035)(2401047)(8121501046)(5005006)(10201501046)(3002001)(3231455)(999002)(944501505)(4982022)(2018427008)(93006095)(93001095)(6055026)(148016)(149066)(150057)(6041310)(20161123560045)(20161123564045)(20161123558120)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123562045)(201708071742011)(7699051)(76991095); SRVR:DM5PR21MB0140; BCL:0; PCL:0; RULEID:; SRVR:DM5PR21MB0140; x-forefront-prvs: 0876988AF0 x-forefront-antispam-report: SFV:NSPM; SFS:(10019020)(376002)(39860400002)(346002)(366004)(396003)(136003)(13464003)(199004)(189003)(68736007)(99286004)(486006)(53546011)(106356001)(5660300001)(6916009)(6306002)(105586002)(186003)(11346002)(9686003)(97736004)(6506007)(446003)(53936002)(229853002)(476003)(76176011)(25786009)(2906002)(16799955002)(7696005)(86362001)(71190400001)(71200400001)(575784001)(86612001)(8990500004)(14454004)(72206003)(7736002)(74316002)(19627235002)(10090500001)(305945005)(10290500003)(478600001)(102836004)(46003)(14444005)(256004)(4326008)(6116002)(81156014)(8936002)(8676002)(6246003)(54906003)(316002)(81166006)(22452003)(33656002)(55016002)(6436002); DIR:OUT; SFP:1102; SCL:1; SRVR:DM5PR21MB0140; H:DM5PR21MB0186.namprd21.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; received-spf: None (protection.outlook.com: microsoft.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: COSyFKpBhJFbcpdxTmmyB+0Aj60tNHgG3RST+tcuwx/qFj/qXtCarKrNLt0IOCQYNiT7zj7wLDBpEvNoRy8oDtWUg9eDum48DYxfR+l//5y0xSEFzC/PrgdvMVIXiavK1RIKEyYZ92lsE1LL/OGhEIgnTmYPuKYv+CBFHNS9FV+rJTmAK+HT4sxPJk+Q3isfrKJsfPjAFQrSJIV9axcdryTGd6xqAmK2UaR4K424cnV27nfSdzx9zETfcz8L/rMuQQ/UVDGpAPfljEd3AbSTG7vaq5rMNyN4lc5rLPPpTzFx9gIy6l9SJai9aiuEWRvGXjd3ic7St/tLRZWjyzYgK3F9U3cW3HLqhn6y4cqqwsU= spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: microsoft.com X-MS-Exchange-CrossTenant-Network-Message-Id: 31057416-ca8b-4c6c-4427-08d6598995fd X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Dec 2018 01:41:18.0247 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47 X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR21MB0140 Subject: Re: [PATCH edk2-platforms 12/27] Silicon/NXP: Add i.MX6 I/O MUX library X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 04 Dec 2018 01:41:20 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Leif, > -----Original Message----- > From: Leif Lindholm > Sent: Thursday, November 8, 2018 10:00 AM > To: Chris Co > Cc: edk2-devel@lists.01.org; Ard Biesheuvel ; > Michael D Kinney > Subject: Re: [PATCH edk2-platforms 12/27] Silicon/NXP: Add i.MX6 I/O MUX > library >=20 > On Fri, Sep 21, 2018 at 08:26:03AM +0000, Chris Co wrote: > > This adds support for initializing and manipulating the I/O Pads on > > NXP i.MX6 SoCs. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Christopher Co > > Cc: Ard Biesheuvel > > Cc: Leif Lindholm > > Cc: Michael D Kinney > > --- > > Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMux.c | 151 > ++++++++++++++++++++ > > Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMuxLib.inf | 41 > > ++++++ > > 2 files changed, 192 insertions(+) > > > > diff --git a/Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMux.c > > b/Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMux.c > > new file mode 100644 > > index 000000000000..7c0c7b54a2fe > > --- /dev/null > > +++ b/Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMux.c > > @@ -0,0 +1,151 @@ > > +/** @file > > +* > > +* Copyright (c) 2018 Microsoft Corporation. All rights reserved. > > +* > > +* This program and the accompanying materials > > +* are licensed and made available under the terms and conditions of > > +the BSD License > > +* which accompanies this distribution. The full text of the license > > +may be found at > > +* > > +https://na01.safelinks.protection.outlook.com/?url=3Dhttp%3A%2F%2Fopen= s > > +ource.org%2Flicenses%2Fbsd- > license.php&data=3D02%7C01%7CChristopher > > > +.Co%40microsoft.com%7C416f86ef76c04f1cac6408d645a40e53%7C72f988b > f86f1 > > > +41af91ab2d7cd011db47%7C1%7C0%7C636772968261577670&sdata=3D > vkaVmLhCg > > +d0X80xpryCJ4kHPUTUtcv6W6MFg3a082nk%3D&reserved=3D0 > > +* > > +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" > > +BASIS, > > +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER > EXPRESS OR IMPLIED. > > +* > > +**/ > > + > > +#include > > + > > +#include > > +#include > > + > > +#include > > +#include > > + > > +// Muxing functions > > +VOID > > +ImxPadConfig ( > > + IN IMX_PAD Pad, > > + IN IMX_PADCFG PadConfig >=20 > I'll get back to reviewing patch 11 at some point, but that one is hard g= oing. So > I'll point out here what I'll mention then: > Please just use UINT64. Typedefs are useful to reduce pointless repetitio= n for > structs, but here it just obscures what is programatically going on. >=20 I'm thinking to rework the PADCFG to actually use structs properly instead = of custom-packing with macros. It seems the previous dev was very macro hap= py and I am the opposite... The remaining feedback makes sense. Will address for v2. Thanks, Chris > > + ) > > +{ > > + // Configure Mux Control > > + MmioWrite32 ( > > + IMX_IOMUXC_BASE + _IMX_PAD_MUX_OFFSET (Pad), > > + _IMX_PADCFG_MUX_CTL (PadConfig)); >=20 > It would be worth adding macros or simple accessor functions for these - = there's > a lot of text in this file that has no semantic value and needs to be man= ually > filtered when reading. >=20 > I.e. IomuxWrite32 (Pad, ...), IomuxRead32 (Pad), ... >=20 > Other comment really belonging to 11: > Please drop leading _ from macros. Such macros are intended for internal = use > by toolchains. >=20 > > + > > + // Configure Select Input Control > > + if (_IMX_PADCFG_SEL_INP (PadConfig) !=3D 0) { > > + DEBUG ((DEBUG_INFO, "Setting INPUT_SELECT %x value %x\n", > > + _IMX_SEL_INP_REGISTER (_IMX_PADCFG_SEL_INP (PadConfig)), > > + _IMX_SEL_INP_VALUE (_IMX_PADCFG_SEL_INP (PadConfig)))); > > + > > + MmioWrite32 ( > > + _IMX_SEL_INP_REGISTER (_IMX_PADCFG_SEL_INP (PadConfig)), > > + _IMX_SEL_INP_VALUE (_IMX_PADCFG_SEL_INP (PadConfig))); } > > + > > + // Configure Pad Control > > + MmioWrite32 ( > > + IMX_IOMUXC_BASE + _IMX_PAD_CTL_OFFSET (Pad), > > + _IMX_PADCFG_PAD_CTL (PadConfig)); } > > + > > +VOID > > +ImxPadDumpConfig ( > > + IN CHAR8 *SignalFriendlyName, > > + IN IMX_PAD Pad > > + ) > > +{ > > + IMX_IOMUXC_MUX_CTL MuxCtl; > > + IMX_IOMUXC_PAD_CTL PadCtl; > > + > > + MuxCtl.AsUint32 =3D MmioRead32 ( > > + IMX_IOMUXC_BASE + _IMX_PAD_MUX_OFFSET (Pad)); > > + > > + DEBUG (( > > + DEBUG_INIT, > > + "- %a MUX_CTL(0x%p)=3D0x%08x: MUX_MODE:%d SION:%d | ", > > + SignalFriendlyName, > > + IMX_IOMUXC_BASE + _IMX_PAD_MUX_OFFSET (Pad), > > + MuxCtl.AsUint32, > > + MuxCtl.Fields.MUX_MODE, > > + MuxCtl.Fields.SION)); > > + > > + PadCtl.AsUint32 =3D MmioRead32 ( > > + IMX_IOMUXC_BASE + _IMX_PAD_CTL_OFFSET (Pad)); > > + > > + DEBUG (( > > + DEBUG_INIT, > > + "PAD_CTL(0x%p)=3D0x%08x: SRE:%d DSE:%d SPEED:%d ODE:%d PKE:= %d > PUE:%d PUS:%d HYS:%d\n", > > + IMX_IOMUXC_BASE + _IMX_PAD_CTL_OFFSET (Pad), > > + PadCtl.AsUint32, > > + PadCtl.Fields.SRE, > > + PadCtl.Fields.DSE, > > + PadCtl.Fields.SPEED, > > + PadCtl.Fields.ODE, > > + PadCtl.Fields.PKE, > > + PadCtl.Fields.PUE, > > + PadCtl.Fields.PUS, > > + PadCtl.Fields.HYS)); > > +} > > + > > +// GPIO functions > > +VOID > > +ImxGpioDirection ( > > + IN IMX_GPIO_BANK Bank, > > + IN UINT32 IoNumber, > > + IN IMX_GPIO_DIR Direction > > + ) > > +{ > > + volatile IMX_GPIO_REGISTERS *gpioRegisters; >=20 > What makes this pointer volatile? > (Hint: drop it, it does nothing useful here.) >=20 > That initial 'g', following EDK2 naming rules, says that this is a variab= le in the > global namespace, exported from this module. It should be GpioRegisters. >=20 > > + > > + ASSERT (IoNumber < 32); >=20 > That 32 needs a #define. >=20 > > + > > + gpioRegisters =3D (IMX_GPIO_REGISTERS *) IMX_GPIO_BASE; if > > + (Direction =3D=3D IMX_GPIO_DIR_INPUT) { > > + MmioAnd32 ((UINTN) &gpioRegisters->Banks[Bank - 1].GDIR, ~ (1 << > > + IoNumber)); >=20 > This 'Bank - 1' stuff looks a bit scary. Why aren't we passing the inde t= o use? A > comment block before the function could explain what the inputs are meant= to > be. >=20 > > + } else { > > + MmioOr32 ((UINTN) &gpioRegisters->Banks[Bank - 1].GDIR, 1 << > > + IoNumber); >=20 > Since we're doing 1 << IoNumber on all possible routes through this funct= ion, > could we assign it to a temporary variable? And we're doing it to the sam= e bank. >=20 > If I wrote this function, I'd probably do something more like >=20 > UINTN DirectionRegister; > UINT32 Pin; > DirectionRegister =3D (UINTN)&((IMX_GPIO_REGISTERS *)IMX_GPIO_BASE)- > >Banks[Bank - 1].GDIR; > Pin =3D 1 << IoNumber; >=20 > if (Direction =3D=3D IMX_GPIO_DIR_INPUT) { > MmioAnd32 (DirectionRegister, ~Pin); > } else { > MmioOr32 (DirectionRegister, Pin); > } >=20 > > + } > > +} > > + > > +VOID > > +ImxGpioWrite ( > > + IN IMX_GPIO_BANK Bank, > > + IN UINT32 IoNumber, > > + IN IMX_GPIO_VALUE Value > > + ) > > +{ > > + volatile IMX_GPIO_REGISTERS *gpioRegisters; > > + > > + ASSERT (IoNumber < 32); > > + > > + gpioRegisters =3D (IMX_GPIO_REGISTERS *) IMX_GPIO_BASE; if (Value = =3D=3D > > + IMX_GPIO_LOW) { > > + MmioAnd32 ((UINTN) &gpioRegisters->Banks[Bank - 1].DR, ~ (1 << > > + IoNumber)); } else { > > + MmioOr32 ((UINTN) &gpioRegisters->Banks[Bank - 1].DR, 1 << > > + IoNumber); } >=20 > All the same comments as above. >=20 > > +} > > + > > +IMX_GPIO_VALUE > > +ImxGpioRead ( > > + IN IMX_GPIO_BANK Bank, > > + IN UINT32 IoNumber > > + ) > > +{ > > + volatile IMX_GPIO_REGISTERS *gpioRegisters; > > + UINT32 Mask; > > + UINT32 Psr; > > + > > + ASSERT (IoNumber < 32); > > + > > + gpioRegisters =3D (IMX_GPIO_REGISTERS *) IMX_GPIO_BASE; Mask =3D (1= << > > + IoNumber); Psr =3D MmioRead32 ((UINTN) &gpioRegisters->Banks[Bank - > > + 1].PSR); > > + > > + if (Psr & Mask) { > > + return IMX_GPIO_HIGH; > > + } else { > > + return IMX_GPIO_LOW; > > + } >=20 > Some of the same comments as above :) > (I.e., those that apply.) >=20 > / > Leif >=20 > > +} > > diff --git a/Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMuxLib.inf > > b/Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMuxLib.inf > > new file mode 100644 > > index 000000000000..84bbbee5c1db > > --- /dev/null > > +++ b/Silicon/NXP/iMX6Pkg/Library/iMX6IoMuxLib/iMX6IoMuxLib.inf > > @@ -0,0 +1,41 @@ > > +## @file > > +# > > +# Copyright (c) 2018 Microsoft Corporation. All rights reserved. > > +# > > +# This program and the accompanying materials # are licensed and > > +made available under the terms and conditions of the BSD License # > > +which accompanies this distribution. The full text of the license > > +may be found at # > > +https://na01.safelinks.protection.outlook.com/?url=3Dhttp%3A%2F%2Fopen= s > > +ource.org%2Flicenses%2Fbsd- > license.php&data=3D02%7C01%7CChristopher > > > +.Co%40microsoft.com%7C416f86ef76c04f1cac6408d645a40e53%7C72f988b > f86f1 > > > +41af91ab2d7cd011db47%7C1%7C0%7C636772968261577670&sdata=3D > vkaVmLhCg > > +d0X80xpryCJ4kHPUTUtcv6W6MFg3a082nk%3D&reserved=3D0 > > +# > > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" > > +BASIS, # WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, > EITHER EXPRESS OR IMPLIED. > > +# > > +## > > + > > +[Defines] > > + INF_VERSION =3D 0x0001001A > > + BASE_NAME =3D iMX6IoMuxLib > > + FILE_GUID =3D FA41BEF0-0666-4C07-9EC3-47F61C36E= DBE > > + MODULE_TYPE =3D BASE > > + VERSION_STRING =3D 1.0 > > + LIBRARY_CLASS =3D iMX6IoMuxLib > > + > > +[Packages] > > + ArmPkg/ArmPkg.dec > > + EmbeddedPkg/EmbeddedPkg.dec > > + MdeModulePkg/MdeModulePkg.dec > > + MdePkg/MdePkg.dec > > + Silicon/NXP/iMX6Pkg/iMX6Pkg.dec > > + Silicon/NXP/iMXPlatformPkg/iMXPlatformPkg.dec > > + > > +[LibraryClasses] > > + BaseMemoryLib > > + DebugLib > > + IoLib > > + TimerLib > > + > > +[Sources.common] > > + iMX6IoMux.c > > + > > + [FixedPcd] > > + giMXPlatformTokenSpaceGuid.PcdGpioBankMemoryRange > > -- > > 2.16.2.gvfs.1.33.gf5370f1 > >