From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::442; helo=mail-wr1-x442.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr1-x442.google.com (mail-wr1-x442.google.com [IPv6:2a00:1450:4864:20::442]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A335621B02822 for ; Thu, 8 Nov 2018 10:00:23 -0800 (PST) Received: by mail-wr1-x442.google.com with SMTP id j26-v6so22263021wre.1 for ; Thu, 08 Nov 2018 10:00:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=HNEnQSK24bdDv7Hf0uZxgcNLEecOU1qbRUox7gh8vzo=; b=CirXyHHLyx0q9sRf2IV2ghtgBahC74jZEsUJIeiSsKjKGJMoGI5fjRSoXDXhtGYQJ3 D33HmYNYwsGQeH/oXajKipjD/TGQ211eFF4okCLizmGL+M256T8cdZjufNO1JHescphJ YjUqSniX6DLWcAxJyn935iZ10nhiHR2qqJNDo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=HNEnQSK24bdDv7Hf0uZxgcNLEecOU1qbRUox7gh8vzo=; b=Uy+AKmUgHTr1xpRP7AM4o8klEf8PtrCqjc/8ZSTcLPqJuVqY4+p8E/5pZit5aECO2v yF8rCuM8Wuilf5lIAxAHV45gPlLGrwT9Guo/0O/iWZZAiUbV/poE40v60/gU/wETkIiC 7vNvQyoqtDEo216crX83ywIDS7rWdrfFtTSm0XuCF5sSwUiOhD/XDgEdYBhrN1sF6fig 3XJxmQu7+eER7CVMgrBo8D8fUAaZx7wf/mGTxE3SxuDSQbPF3oNyE2LzJBYTA/ZEpAsa FFsqwUeOLWvP5OIzWA5vB8v8ysLrccuDaWy7iN8OyEmjsC4uDyx6gilqbFqdE63Q6qe0 p8og== X-Gm-Message-State: AGRZ1gLQHOb815gCgVtb9WiCbWNbRRWhYsJtg/aQGW3AQ8F0J96VzQo8 z+JPohGSw4n3bGnfKwWdR7nwew== X-Google-Smtp-Source: AJdET5cZguoSCFhcYamnxa5tcB7mTHkiupp+VPega3aQh8AWORefDzl/1CmxFvMIOAH+IPBiEsqsoQ== X-Received: by 2002:adf:a386:: with SMTP id l6-v6mr5371254wrb.110.1541700021833; Thu, 08 Nov 2018 10:00:21 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id r188-v6sm6066636wmg.19.2018.11.08.10.00.20 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 08 Nov 2018 10:00:20 -0800 (PST) Date: Thu, 8 Nov 2018 18:00:19 +0000 From: Leif Lindholm To: Chris Co Cc: "edk2-devel@lists.01.org" , Ard Biesheuvel , Michael D Kinney Message-ID: <20181108180018.mqi6yvadi7nkiolt@bivouac.eciton.net> References: <20180921082542.35768-1-christopher.co@microsoft.com> <20180921082542.35768-13-christopher.co@microsoft.com> MIME-Version: 1.0 In-Reply-To: <20180921082542.35768-13-christopher.co@microsoft.com> User-Agent: NeoMutt/20170113 (1.7.2) 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: Thu, 08 Nov 2018 18:00:24 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > +* http://opensource.org/licenses/bsd-license.php > +* > +* 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 I'll get back to reviewing patch 11 at some point, but that one is hard going. So I'll point out here what I'll mention then: Please just use UINT64. Typedefs are useful to reduce pointless repetition for structs, but here it just obscures what is programatically going on. > + ) > +{ > + // Configure Mux Control > + MmioWrite32 ( > + IMX_IOMUXC_BASE + _IMX_PAD_MUX_OFFSET (Pad), > + _IMX_PADCFG_MUX_CTL (PadConfig)); 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 manually filtered when reading. I.e. IomuxWrite32 (Pad, ...), IomuxRead32 (Pad), ... Other comment really belonging to 11: Please drop leading _ from macros. Such macros are intended for internal use by toolchains. > + > + // Configure Select Input Control > + if (_IMX_PADCFG_SEL_INP (PadConfig) != 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 = MmioRead32 ( > + IMX_IOMUXC_BASE + _IMX_PAD_MUX_OFFSET (Pad)); > + > + DEBUG (( > + DEBUG_INIT, > + "- %a MUX_CTL(0x%p)=0x%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 = MmioRead32 ( > + IMX_IOMUXC_BASE + _IMX_PAD_CTL_OFFSET (Pad)); > + > + DEBUG (( > + DEBUG_INIT, > + "PAD_CTL(0x%p)=0x%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; What makes this pointer volatile? (Hint: drop it, it does nothing useful here.) That initial 'g', following EDK2 naming rules, says that this is a variable in the global namespace, exported from this module. It should be GpioRegisters. > + > + ASSERT (IoNumber < 32); That 32 needs a #define. > + > + gpioRegisters = (IMX_GPIO_REGISTERS *) IMX_GPIO_BASE; > + if (Direction == IMX_GPIO_DIR_INPUT) { > + MmioAnd32 ((UINTN) &gpioRegisters->Banks[Bank - 1].GDIR, ~ (1 << IoNumber)); This 'Bank - 1' stuff looks a bit scary. Why aren't we passing the inde to use? A comment block before the function could explain what the inputs are meant to be. > + } else { > + MmioOr32 ((UINTN) &gpioRegisters->Banks[Bank - 1].GDIR, 1 << IoNumber); Since we're doing 1 << IoNumber on all possible routes through this function, could we assign it to a temporary variable? And we're doing it to the same bank. If I wrote this function, I'd probably do something more like UINTN DirectionRegister; UINT32 Pin; DirectionRegister = (UINTN)&((IMX_GPIO_REGISTERS *)IMX_GPIO_BASE)->Banks[Bank - 1].GDIR; Pin = 1 << IoNumber; if (Direction == IMX_GPIO_DIR_INPUT) { MmioAnd32 (DirectionRegister, ~Pin); } else { MmioOr32 (DirectionRegister, Pin); } > + } > +} > + > +VOID > +ImxGpioWrite ( > + IN IMX_GPIO_BANK Bank, > + IN UINT32 IoNumber, > + IN IMX_GPIO_VALUE Value > + ) > +{ > + volatile IMX_GPIO_REGISTERS *gpioRegisters; > + > + ASSERT (IoNumber < 32); > + > + gpioRegisters = (IMX_GPIO_REGISTERS *) IMX_GPIO_BASE; > + if (Value == IMX_GPIO_LOW) { > + MmioAnd32 ((UINTN) &gpioRegisters->Banks[Bank - 1].DR, ~ (1 << IoNumber)); > + } else { > + MmioOr32 ((UINTN) &gpioRegisters->Banks[Bank - 1].DR, 1 << IoNumber); > + } All the same comments as above. > +} > + > +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 = (IMX_GPIO_REGISTERS *) IMX_GPIO_BASE; > + Mask = (1 << IoNumber); > + Psr = MmioRead32 ((UINTN) &gpioRegisters->Banks[Bank - 1].PSR); > + > + if (Psr & Mask) { > + return IMX_GPIO_HIGH; > + } else { > + return IMX_GPIO_LOW; > + } Some of the same comments as above :) (I.e., those that apply.) / Leif > +} > 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 > +# http://opensource.org/licenses/bsd-license.php > +# > +# 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 = 0x0001001A > + BASE_NAME = iMX6IoMuxLib > + FILE_GUID = FA41BEF0-0666-4C07-9EC3-47F61C36EDBE > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = 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 >