From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by mx.groups.io with SMTP id smtpd.web09.9242.1581424137576696865 for ; Tue, 11 Feb 2020 04:28:57 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=FF4aeets; spf=pass (domain: nuviainc.com, ip: 209.85.221.67, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f67.google.com with SMTP id m16so12104085wrx.11 for ; Tue, 11 Feb 2020 04:28:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=7tH9H3WGOHsZKIG+hb7U+eGkFHhuT8IonfPuNYg5Bu4=; b=FF4aeetsROzOz+3xG47LmZEey1iL23w4kep5x5a1ohlvbFnXYAgcIzSxtbZN8xATig AEri8mF8L/88kotRHpPMev9Nz/JU95qSkB2locL9034x7YmvKIsnMCL7uFvmY/sbUjiK Wg1vAUSVnsysfLs9qqTFjimWFZLSm/w8R+7OiOuJIiwigBspeCY3xXo643XwvdZFmOSu BxjiAZ7vG5sQ6dCNhSLtwHSEjk9mRYTEj+pS8dCFnA2QyYt2fycoJX98jctnVGkDH7om nW6pe/d/aI9cFGwTzi2PsHyEWuufWO6ZEBaQ7FLy4TiHAakGdiS6BYkxzSvItKXXWMk5 gzeg== 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=7tH9H3WGOHsZKIG+hb7U+eGkFHhuT8IonfPuNYg5Bu4=; b=X8GPOKS4kl4Ur3yL/UVq5yN3puFJfC6lzA+TC6uc+P6P+BxnZe/oLnIn5TduVyVTcH Z4ZrZyQW950b1L3s2pKlpOCdZmnD2G1huUMaaEiJN56uhBeRUQwrcjE+YuS8lQEU2pLM BLvtzvqssRT4IsNcgVr18e8v+0yiex8a8mmYt1eNuQC40hbUOJTyteBzPEFVBcCbR+xw LOnrJwhxfuOIu6Zl35i1igY5R71vdav3kBVT0lwXZpLVVnYu3Xqtm/ecKKPe7cp5NML9 RTOqfAtty7qgnUCPEfvlpwBcwi6floC36HDjnGKv4eD7WSEK75eytN6vY9B9JZBSLxbf MBAg== X-Gm-Message-State: APjAAAUBr9PF+qOl4Z2l+6ipHx5s+59NxSqZNn1BG4jIfFZJP8qNv/eq zx0DqdwgpXW/Bza51a/YNGH6YQ== X-Google-Smtp-Source: APXvYqxAi6mKIGyjLc2DPBIpNLy+bNZu/xR5IMKu8rcQOyh0SEU+QGyv6IQMDqSmru7jgslbqGYu/w== X-Received: by 2002:a5d:4e91:: with SMTP id e17mr7928381wru.233.1581424135935; Tue, 11 Feb 2020 04:28:55 -0800 (PST) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id v22sm3478378wml.11.2020.02.11.04.28.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 11 Feb 2020 04:28:55 -0800 (PST) Date: Tue, 11 Feb 2020 12:28:53 +0000 From: "Leif Lindholm" To: Pankaj Bansal Cc: Meenakshi Aggarwal , Michael D Kinney , Varun Sethi , devel@edk2.groups.io Subject: Re: [PATCH 11/19] Silicon/NXP: Add Chassis Lib for Chassis2 Message-ID: <20200211122853.GU23627@bivouac.eciton.net> References: <20200207124328.8723-1-pankaj.bansal@nxp.com> <20200207124328.8723-12-pankaj.bansal@nxp.com> MIME-Version: 1.0 In-Reply-To: <20200207124328.8723-12-pankaj.bansal@nxp.com> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Feb 07, 2020 at 18:13:20 +0530, Pankaj Bansal wrote: > Add ChassisLib for Chassis2. What is a Chassis2? This adds a new package, a new library class - it needs more description. Can we expect to see Silicon/NXP/Library/SocLib/Chassis.c broken out into its own package in future? > Signed-off-by: Pankaj Bansal > --- > Silicon/NXP/Chassis2/Chassis2.dec | 23 +++ > Silicon/NXP/Chassis2/Chassis2.dsc.inc | 10 + > Silicon/NXP/Chassis2/Include/Chassis.h | 42 ++++ > .../Chassis2/Library/ChassisLib/ChassisLib.c | 186 ++++++++++++++++++ > .../Library/ChassisLib/ChassisLib.inf | 41 ++++ > Silicon/NXP/Include/Library/ChassisLib.h | 41 ++++ > Silicon/NXP/NxpQoriqLs.dec | 4 + > 7 files changed, 347 insertions(+) > create mode 100644 Silicon/NXP/Chassis2/Chassis2.dec > create mode 100644 Silicon/NXP/Chassis2/Chassis2.dsc.inc > create mode 100644 Silicon/NXP/Chassis2/Include/Chassis.h > create mode 100644 Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c > create mode 100644 Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf > create mode 100644 Silicon/NXP/Include/Library/ChassisLib.h > > diff --git a/Silicon/NXP/Chassis2/Chassis2.dec b/Silicon/NXP/Chassis2/Chassis2.dec > new file mode 100644 > index 0000000000..106b118188 > --- /dev/null > +++ b/Silicon/NXP/Chassis2/Chassis2.dec > @@ -0,0 +1,23 @@ > +#/** @file > +# NXP Layerscape processor package. > +# > +# Copyright 2020 NXP > +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > +#**/ > + > +[Defines] > + DEC_SPECIFICATION = 1.27 > + PACKAGE_VERSION = 0.1 > + > +################################################################################ > +# > +# Include Section - list of Include Paths that are provided by this package. > +# Comments are used for Keywords and Module Types. > +# > +# > +################################################################################ > +[Includes.common] > + Include # Root include for the package > + > diff --git a/Silicon/NXP/Chassis2/Chassis2.dsc.inc b/Silicon/NXP/Chassis2/Chassis2.dsc.inc > new file mode 100644 > index 0000000000..db8e5a92ea > --- /dev/null > +++ b/Silicon/NXP/Chassis2/Chassis2.dsc.inc > @@ -0,0 +1,10 @@ > +# @file > +# > +# Copyright 2020 NXP > +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > +# > + > +[LibraryClasses.common] > + ChassisLib|Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf > diff --git a/Silicon/NXP/Chassis2/Include/Chassis.h b/Silicon/NXP/Chassis2/Include/Chassis.h > new file mode 100644 > index 0000000000..48ba2e7bfb > --- /dev/null > +++ b/Silicon/NXP/Chassis2/Include/Chassis.h > @@ -0,0 +1,42 @@ > +/** @file > + > + Copyright 2020 NXP > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > +#ifndef __CHASSIS_H__ > +#define __CHASSIS_H__ Please drop leading __ from incude guards. > + > +#define NXP_LAYERSCAPE_CHASSIS2_DCFG_ADDRESS 0x1EE0000 > + > +#define TP_CLUSTER_ITYPE_IDX 0x3f > +#define TP_CLUSTER_EOC BIT31 > +#define TP_ITYPE_AVAILABLE BIT0 > +#define TP_ITYPE_TYPE(x) (((x) & 0x06) >> 1) > +#define TP_ITYPE_ARM 0x0 > +#define TP_ITYPE_VERSION(x) (((x) & 0xe0) >> 5) > + > +#define TP_ITYPE_VERSION_A53 0x2 > +#define TP_ITYPE_VERSION_A72 0x4 > + > +/** > + The Device Configuration Unit provides general purpose configuration and status for the > + device. These registers only support 32-bit accesses. > +**/ > +#pragma pack(1) > +typedef struct { > + UINT8 Reserved0[0x100 - 0x0]; > + UINT32 RcwSr[16]; // Reset Control Word Status Register > + UINT8 Reserved140[0x200 - 0x140]; > + UINT32 ScratchRw[16]; /// Scratch Read / Write Register > + UINT8 Reserved240[0x740-0x240]; > + UINT32 TpItyp[65]; /// Topology Initiator Type Register > + struct { > + UINT32 Lower; > + UINT32 Upper; > + }TpCluster[8]; > +} NXP_LAYERSCAPE_CHASSIS2_DEVICE_CONFIG; > +#pragma pack() > + > +#endif > diff --git a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c > new file mode 100644 > index 0000000000..fa6a36e96f > --- /dev/null > +++ b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c > @@ -0,0 +1,186 @@ > +/** @file > + Chassis specific functions common to all SOCs based on a specific Chessis > + > + Copyright 2020 NXP > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#include > +#include > +#include > +#include Plese sort includes alphabetically. > +#include > +#include > +#include > + > +UINT32 > +EFIAPI > +DcfgRead32 ( OK, so after 3 years of review, I failed to spot the set I merged doesn't actually make use of the GetMmioOperations* functions introduced in IoAccessLib to get away from this needless code duplication. Ideally, I would like to see some patches that remedy this situation in the alredy merged code. But it certainly needs to be used for any new additions. > + IN UINTN Address > + ) > +{ > + if (FeaturePcdGet (PcdDcfgBigEndian)) { > + return SwapMmioRead32 (Address); > + } else { > + return MmioRead32 (Address); > + } > +} > + > +UINT32 > +EFIAPI > +DcfgWrite32 ( > + IN UINTN Address, > + IN UINT32 Value > + ) > +{ > + if (FeaturePcdGet (PcdDcfgBigEndian)) { > + return SwapMmioWrite32 (Address, Value); > + } else { > + return MmioWrite32 (Address, Value); > + } > +} > + > +/** > + Get the type of core in cluster > + > + The core can be of type ARM or PowerPC or Hardware Accelerator. > + If the core is enabled and of type ARM EFI_SUCCESS is returned and a code for type of ARM core is returned Please wrap long lines. > + > + @param[in] TpItypeIdx Index of Core to be searched in TpItyp in Device Config Registers. TpItype or TpItyp? Neither Tp nor Itype are known abbreviations, so need to be explained in file comment header, unless they can be given more generically descriptive names. > + @param[out] CoreType If the core is ARM core then the type of core i.e. A53/A72 etc. > + These cores are identified based on their codes like TP_ITYPE_VERSION_A72 > + > + @return EFI_NOT_FOUND No enabled ARM core found > + @return EFI_SUCCESS An enabled ARM core found > +**/ > +STATIC > +EFI_STATUS > +SocGetCoreType ( > + IN UINT8 TpItypeIdx, > + OUT UINT8 *CoreType > + ) > +{ > + NXP_LAYERSCAPE_CHASSIS2_DEVICE_CONFIG *Dcfg; > + UINT32 TpItype; > + > + Dcfg = (NXP_LAYERSCAPE_CHASSIS2_DEVICE_CONFIG *)NXP_LAYERSCAPE_CHASSIS2_DCFG_ADDRESS; > + TpItype = MmioRead32 ((UINTN)&Dcfg->TpItyp[TpItypeIdx]); > + if (TpItype & TP_ITYPE_AVAILABLE) { > + if (TP_ITYPE_TYPE (TpItype) == TP_ITYPE_ARM) { > + *CoreType = TP_ITYPE_VERSION (TpItype); > + } else { > + return EFI_NOT_FOUND; > + } > + } else { > + return EFI_NOT_FOUND; > + } > + > + return EFI_SUCCESS; > +} > + > +/** > + Return the number of cores present in SOC > + > + This function returns the number of cores present in SOC. > + and also their position (cluster number and core number) in the form of ARM_CORE_INFO array Please wrap long lines. > + and NxpCoreTable array. > + NxpCoreTable array can be used to find out the type of core. it's values are of type > + TP_ITYPE_VERSION_*. > + The number of cores present in SOC can vary depending on which flavour of SOC is being used. > + This function doesn't allocte any memory and must be provided memory for array of ARM_CORE_INFO > + and NxpCoreTable for maximum number of cores the SOC can have. > + > + @param[out] NxpCoreTable array of UINT8 for maximum number of cores the SOC can have. > + @param[out] ArmCoreTable array of ARM_CORE_INFO for maximum number of cores the SOC can have. > + @param[in] ArmCoreTableSize Size of ArmCoreTable > + > + @return Actual number of cores present in SOC. After calling this function only the returned value number of > + entries in ArmCoreTable are valid entries. > +**/ > +UINTN > +__attribute__((weak)) There is nothing *wrong* about using weak symbols, but it usually done either for some specific workarouns, or as part of a defined usage model to override things at different levels of software. It is not clear to me what that usage model is here - could you document it please? > +SocGetMpCoreInfo ( > + OUT UINT8 *NxpCoreTable, > + OUT ARM_CORE_INFO *ArmCoreTable, > + IN UINTN CoreTableSize > + ) > +{ > + NXP_LAYERSCAPE_CHASSIS2_DEVICE_CONFIG *Dcfg; > + UINT32 TpClusterLower; > + UINT8 TpClusterParser; TpCusterLower/TpClusterParser are not generic terms - could they be documented or renamed more generically. > + UINT8 ClusterIndex; > + UINT8 CoreIndex; > + UINTN CoreCount; > + UINT8 CoreType; > + EFI_STATUS Status; > + > + Dcfg = (NXP_LAYERSCAPE_CHASSIS2_DEVICE_CONFIG *)NXP_LAYERSCAPE_CHASSIS2_DCFG_ADDRESS; > + ClusterIndex = 0; > + CoreCount = 0; > + while (TRUE) { > + TpClusterLower = MmioRead32 ((UINTN)&Dcfg->TpCluster[ClusterIndex].Lower); > + for (CoreIndex = 0; CoreIndex < (sizeof (TpClusterLower) / sizeof (TpClusterParser)); CoreIndex++) { I don't see a value in calculating sizeof(UINT32)/sizeof(UINT8). A strategically placed #define would be more clear (and shorten the line length). > + TpClusterParser = (TpClusterLower >> (8 * CoreIndex)); And I would prefer to see that 8 replaced by another #define. > + Status = SocGetCoreType (TpClusterParser & TP_CLUSTER_ITYPE_IDX, &CoreType); > + if (Status != EFI_NOT_FOUND) { > + ArmCoreTable[CoreCount].ClusterId = ClusterIndex; > + ArmCoreTable[CoreCount].CoreId = CoreIndex; > + ArmCoreTable[CoreCount].MailboxSetAddress = 0; > + ArmCoreTable[CoreCount].MailboxGetAddress = 0; > + ArmCoreTable[CoreCount].MailboxClearAddress = 0; > + ArmCoreTable[CoreCount].MailboxClearValue = ~0; > + > + NxpCoreTable[CoreCount] = CoreType; > + CoreCount++; > + if (CoreCount == CoreTableSize) { > + break; > + } > + } > + } > + if (TpClusterLower & TP_CLUSTER_EOC) { > + break; > + } > + if (CoreCount == CoreTableSize) { > + break; > + } > + ClusterIndex++; > + } > + > + return CoreCount; > +} > + > +/** > + Function to initialize Chassis Specific functions > + **/ > +VOID > +ChassisInit ( > + VOID > + ) > +{ > + UINT64 BaudRate; > + UINT32 ReceiveFifoDepth; > + EFI_PARITY_TYPE Parity; > + UINT8 DataBits; > + EFI_STOP_BITS_TYPE StopBits; > + UINT32 Timeout; > + > + BaudRate = FixedPcdGet64 (PcdUartDefaultBaudRate); > + ReceiveFifoDepth = FixedPcdGet16 (PcdUartDefaultReceiveFifoDepth); // Use default FIFO depth > + Timeout = 0; > + Parity = (EFI_PARITY_TYPE)FixedPcdGet8 (PcdUartDefaultParity); > + DataBits = FixedPcdGet8 (PcdUartDefaultDataBits); > + StopBits = (EFI_STOP_BITS_TYPE) FixedPcdGet8 (PcdUartDefaultStopBits); > + > + // > + // Early init serial Port to get board information. > + // > + SerialPortSetAttributes ( > + &BaudRate, > + &ReceiveFifoDepth, > + &Timeout, > + &Parity, > + &DataBits, > + &StopBits > + ); > +} > diff --git a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf > new file mode 100644 > index 0000000000..4964bb4e82 > --- /dev/null > +++ b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf > @@ -0,0 +1,41 @@ > +#/** @file > +# > +# Copyright 2020 NXP > +# > +# SPDX-License-Identifier: BSD-2-Clause > +# > +#**/ > + > +[Defines] > + INF_VERSION = 0x0001001A > + BASE_NAME = Chassis2Lib > + FILE_GUID = fae0d077-5fc2-494f-b8e1-c51a3023ee3e > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = ChassisLib > + > +[Packages] > + MdePkg/MdePkg.dec > + ArmPkg/ArmPkg.dec > + Silicon/NXP/NxpQoriqLs.dec > + Silicon/NXP/Chassis2/Chassis2.dec Please sort packages alphabetically. > + > +[LibraryClasses] > + IoLib > + IoAccessLib Please sort library classes alphabetically. > + PcdLib > + SerialPortLib > + > +[Sources.common] > + ChassisLib.c > + > +[FeaturePcd] > + gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian > + > +[FixedPcd] > + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate > + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits > + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity > + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits > + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultReceiveFifoDepth Please sort Pcds alphabetically (i.e. swap final two lines). > + > diff --git a/Silicon/NXP/Include/Library/ChassisLib.h b/Silicon/NXP/Include/Library/ChassisLib.h > new file mode 100644 > index 0000000000..b51b024374 > --- /dev/null > +++ b/Silicon/NXP/Include/Library/ChassisLib.h > @@ -0,0 +1,41 @@ > +/** @file > + I2c Lib to control I2c controller. > + > + Copyright 2020 NXP > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#ifndef __CHASSIS_LIB_H__ > +#define __CHASSIS_LIB_H__ Please drop leading __ from include guards. > + > +#include Include files should only include those files needed for their own definitions. > + > +/** > + Read Dcfg register > +**/ > +UINT32 > +EFIAPI > +DcfgRead32 ( > + IN UINTN Address > + ); > + > +/** > + Write Dcfg register > +**/ > +UINT32 > +EFIAPI > +DcfgWrite32 ( > + IN UINTN Address, > + IN UINT32 Value > + ); > + The above two prototypes can be deleted. > +/** > + Function to initialize Chassis Specific functions > + **/ > +VOID > +ChassisInit ( > + VOID > + ); > + > +#endif > diff --git a/Silicon/NXP/NxpQoriqLs.dec b/Silicon/NXP/NxpQoriqLs.dec > index b478560450..d8989657e6 100644 > --- a/Silicon/NXP/NxpQoriqLs.dec > +++ b/Silicon/NXP/NxpQoriqLs.dec > @@ -14,6 +14,9 @@ > Include > > [LibraryClasses] > + ## @libraryclass Provides Chassis specific functions to other modules > + ChassisLib|Include/Library/ChassisLib.h > + > ## @libraryclass Provides services to read/write to I2c devices > I2cLib|Include/Library/I2cLib.h > > @@ -34,4 +37,5 @@ > > [PcdsFeatureFlag] > gNxpQoriqLsTokenSpaceGuid.PcdI2cErratumA009203|FALSE|BOOLEAN|0x00000315 > + gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian|FALSE|BOOLEAN|0x00000316 I would prefer for this flag to be added with the other *BigEndian flags in the same file. Looping back to a previous patch - looking at the Pcd tokens used, it would be more convenient if PcdI2cErratumA009203 was kept as part of a separate group and token 0x315 was given to PcdDcfgBigEndian. / Leif > > -- > 2.17.1 >