From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by mx.groups.io with SMTP id smtpd.web10.1384.1581355924660877675 for ; Mon, 10 Feb 2020 09:32:05 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=RKh6kNwA; spf=pass (domain: nuviainc.com, ip: 209.85.221.66, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f66.google.com with SMTP id t3so8836164wru.7 for ; Mon, 10 Feb 2020 09:32:04 -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=30UZDK4j19L/8aZqey/kc6CoM9fW6hnBNt+PSNkE5Wg=; b=RKh6kNwAVnZSJH3A12nwN2sIrTIQdlJhZKRzdqwWsfDO2ow0hpdMmE8hufBbmfOFa4 qNBnWsXszaNMrhBj10K9e5CAWHrbtwHYD3mjEfITh64I12plIJiwqgB7n6ql8Nv4SIYu wFj7eAPN0NSJDejP/jl1C2L/4yLVVrMPA3R4WUAabfR1KURE3fZLiVtwqVC6Q21HFlBU E9KkOXdfaSSqba64DLP6RzbLoIyZkFd4hMU76rThaRjnofQhKw77LQF8HhTYLqq5mdk0 oy12KycdTgBPAIDGOzt5Iyu4Wdf4l+Pa2zNHI9wc5jDVX+AXEXkkhfj7yLvk95RFw/LW j38g== 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=30UZDK4j19L/8aZqey/kc6CoM9fW6hnBNt+PSNkE5Wg=; b=b3por6o5qlpKo7Tw3vfGnB7hp6NEL+bI4BqmSJJlPhtNn4jcYl4PV9Xhmkv/qONGzW 7ejknz1/QMxap0A3jJEFKCps9tEr6C+LUWw2Pd4HmzVs6h6ZFopuhX3bQuxi4GxF4E5c EN9cHRtmAe4ZdeifzB5YEycSG9njH2/oplni5jrcyv4CYe0OpQx0w5bkLcK+F6hqi/1V BSDbEBAn627iAGdWQu/uc3H8VY9anUCu757foUl3unQOyTNt5BmHSywPUVX5UpG1t83J 0tlpDg9YmE7JIzeZjxU0X/E+JpfQJXsTs1USGt69tXA87IztGBEA8vCDzz9f4tXUTi6a /dGA== X-Gm-Message-State: APjAAAXlbRhX3X6dZ3670rM5cwQUNFpnVJl/8gPcZkjSw8qsP0Tm8lZc LBehSvGfFbSH1xBWRZfvAs1VXA== X-Google-Smtp-Source: APXvYqwhqDGFDuEsLbkwYSKJbeVbGIbjs6X0vf3V5zAATqRcKGhjYVQxSAVhyZEmKBzLa7JOuHEj5A== X-Received: by 2002:adf:a35e:: with SMTP id d30mr2842730wrb.33.1581355922928; Mon, 10 Feb 2020 09:32:02 -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 b16sm38943wmj.39.2020.02.10.09.32.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 10 Feb 2020 09:32:02 -0800 (PST) Date: Mon, 10 Feb 2020 17:32:00 +0000 From: "Leif Lindholm" To: Pankaj Bansal Cc: Meenakshi Aggarwal , Michael D Kinney , Varun Sethi , devel@edk2.groups.io Subject: Re: [PATCH 08/19] Silicon/NXP: Remove unnecessary PCDs Message-ID: <20200210173200.GM23627@bivouac.eciton.net> References: <20200207124328.8723-1-pankaj.bansal@nxp.com> <20200207124328.8723-9-pankaj.bansal@nxp.com> MIME-Version: 1.0 In-Reply-To: <20200207124328.8723-9-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:17 +0530, Pankaj Bansal wrote: > There is no need to keep SOC specific PCDs defined for each SOC. That sound like the definition of why we have SoC-specific Pcds, so I don't follow. > we can do away with these PCDs. After looking through this patchset, it looks like: 1) Initial implementation defined a bunch of things as Pcds which really cannot vary between different platforms using the same SoC. 2) This patch moves several things that can differ between platforms into #defines, when they should be Pcds. I am OK with the 1)s, but there are some you would need to convince me are not 2:s. > Signed-off-by: Pankaj Bansal > --- > .../Drivers/PlatformDxe/PlatformDxe.c | 15 +-- > .../Drivers/PlatformDxe/PlatformDxe.inf | 8 +- > Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.dsc | 1 - > .../Library/PlatformLib/ArmPlatformLib.inf | 21 +--- > .../Library/PlatformLib/NxpQoriqLsMem.c | 103 +++++++++--------- > Silicon/NXP/Drivers/I2cDxe/I2cDxe.inf | 6 - > Silicon/NXP/Include/Chassis2/NxpSoc.h | 2 + > Silicon/NXP/LS1043A/Include/Soc.h | 44 ++++++++ > Silicon/NXP/LS1043A/LS1043A.dsc.inc | 32 ------ > Silicon/NXP/Library/SocLib/Chassis2/Soc.c | 2 +- > Silicon/NXP/Library/SocLib/LS1043aSocLib.inf | 4 - > Silicon/NXP/NxpQoriqLs.dec | 74 ------------- > 12 files changed, 108 insertions(+), 204 deletions(-) > create mode 100644 Silicon/NXP/LS1043A/Include/Soc.h > > diff --git a/Platform/NXP/LS1043aRdbPkg/Drivers/PlatformDxe/PlatformDxe.c b/Platform/NXP/LS1043aRdbPkg/Drivers/PlatformDxe/PlatformDxe.c > index f89dcdeff3..62c400eb1a 100644 > --- a/Platform/NXP/LS1043aRdbPkg/Drivers/PlatformDxe/PlatformDxe.c > +++ b/Platform/NXP/LS1043aRdbPkg/Drivers/PlatformDxe/PlatformDxe.c > @@ -1,7 +1,7 @@ > /** @file > LS1043 DXE platform driver. > > - Copyright 2018-2019 NXP > + Copyright 2018-2020 NXP > > SPDX-License-Identifier: BSD-2-Clause-Patent > > @@ -14,6 +14,7 @@ > #include > #include > #include > +#include > > #include > > @@ -22,7 +23,7 @@ typedef struct { > UINT8 EndDesc; > } ADDRESS_SPACE_DESCRIPTOR; > > -STATIC ADDRESS_SPACE_DESCRIPTOR mI2cDesc[FixedPcdGet64 (PcdNumI2cController)]; > +STATIC ADDRESS_SPACE_DESCRIPTOR mI2cDesc[LS1043A_I2C_NUM_CONTROLLERS]; Sure, this one sounds like something not configurable in software or board design. > > STATIC > EFI_STATUS > @@ -65,19 +66,19 @@ PopulateI2cInformation ( > { > UINT32 Index; > > - for (Index = 0; Index < FixedPcdGet32 (PcdNumI2cController); Index++) { > + for (Index = 0; Index < ARRAY_SIZE (mI2cDesc); Index++) { > mI2cDesc[Index].StartDesc.Desc = ACPI_ADDRESS_SPACE_DESCRIPTOR; > mI2cDesc[Index].StartDesc.Len = sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3; > mI2cDesc[Index].StartDesc.ResType = ACPI_ADDRESS_SPACE_TYPE_MEM; > mI2cDesc[Index].StartDesc.GenFlag = 0; > mI2cDesc[Index].StartDesc.SpecificFlag = 0; > mI2cDesc[Index].StartDesc.AddrSpaceGranularity = 32; > - mI2cDesc[Index].StartDesc.AddrRangeMin = FixedPcdGet64 (PcdI2c0BaseAddr) + > - (Index * FixedPcdGet32 (PcdI2cSize)); As does this. > + mI2cDesc[Index].StartDesc.AddrRangeMin = LS1043A_I2C0_PHYS_ADDRESS + > + (Index * LS1043A_I2C_SIZE); > mI2cDesc[Index].StartDesc.AddrRangeMax = mI2cDesc[Index].StartDesc.AddrRangeMin + > - FixedPcdGet32 (PcdI2cSize) - 1; > + LS1043A_I2C_SIZE - 1; > mI2cDesc[Index].StartDesc.AddrTranslationOffset = 0; > - mI2cDesc[Index].StartDesc.AddrLen = FixedPcdGet32 (PcdI2cSize); > + mI2cDesc[Index].StartDesc.AddrLen = LS1043A_I2C_SIZE; > > mI2cDesc[Index].EndDesc = ACPI_END_TAG_DESCRIPTOR; > } > diff --git a/Platform/NXP/LS1043aRdbPkg/Drivers/PlatformDxe/PlatformDxe.inf b/Platform/NXP/LS1043aRdbPkg/Drivers/PlatformDxe/PlatformDxe.inf > index d689cf4db5..126a1174fa 100644 > --- a/Platform/NXP/LS1043aRdbPkg/Drivers/PlatformDxe/PlatformDxe.inf > +++ b/Platform/NXP/LS1043aRdbPkg/Drivers/PlatformDxe/PlatformDxe.inf > @@ -2,7 +2,7 @@ > # > # Component description file for LS1043 DXE platform driver. > # > -# Copyright 2018-2019 NXP > +# Copyright 2018-2020 NXP > # > # SPDX-License-Identifier: BSD-2-Clause-Patent > # > @@ -25,6 +25,7 @@ > MdeModulePkg/MdeModulePkg.dec > Silicon/Maxim/Library/Ds1307RtcLib/Ds1307RtcLib.dec > Silicon/NXP/NxpQoriqLs.dec > + Silicon/NXP/LS1043A/LS1043A.dec Please insert sorted alphabetically. > > [LibraryClasses] > BaseLib > @@ -43,10 +44,5 @@ > gEdkiiNonDiscoverableDeviceProtocolGuid ## PRODUCES > gDs1307RealTimeClockLibI2cMasterProtocolGuid ## PRODUCES > > -[FixedPcd] > - gNxpQoriqLsTokenSpaceGuid.PcdI2c0BaseAddr > - gNxpQoriqLsTokenSpaceGuid.PcdI2cSize > - gNxpQoriqLsTokenSpaceGuid.PcdNumI2cController Still good. > - > [Depex] > TRUE > diff --git a/Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.dsc b/Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.dsc > index 802cccdce6..74a1948fc6 100644 > --- a/Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.dsc > +++ b/Platform/NXP/LS1043aRdbPkg/LS1043aRdbPkg.dsc > @@ -42,7 +42,6 @@ > # > # Board Specific Pcds > # > - gNxpQoriqLsTokenSpaceGuid.PcdSerdes2Enabled|FALSE But this sounds exactly like the kind of thing that would be configured differently for different boards. Can you please explain why this should be removed? > gNxpQoriqLsTokenSpaceGuid.PcdPlatformFreqDiv|0x1 > > # > diff --git a/Platform/NXP/LS1043aRdbPkg/Library/PlatformLib/ArmPlatformLib.inf b/Platform/NXP/LS1043aRdbPkg/Library/PlatformLib/ArmPlatformLib.inf > index f7ae74afc6..054dc4d003 100644 > --- a/Platform/NXP/LS1043aRdbPkg/Library/PlatformLib/ArmPlatformLib.inf > +++ b/Platform/NXP/LS1043aRdbPkg/Library/PlatformLib/ArmPlatformLib.inf > @@ -1,7 +1,7 @@ > # @file > # > # Copyright (c) 2016, Freescale Semiconductor, Inc. All rights reserved. > -# Copyright 2017, 2019 NXP > +# Copyright 2017, 2019-2020 NXP > # > # SPDX-License-Identifier: BSD-2-Clause-Patent > # > @@ -20,6 +20,7 @@ > EmbeddedPkg/EmbeddedPkg.dec > MdePkg/MdePkg.dec > Silicon/NXP/NxpQoriqLs.dec > + Silicon/NXP/LS1043A/LS1043A.dec Insert sorted. > > [LibraryClasses] > ArmLib > @@ -35,21 +36,3 @@ > > [FixedPcd] > gArmTokenSpaceGuid.PcdArmPrimaryCore > - gNxpQoriqLsTokenSpaceGuid.PcdCcsrBaseAddr > - gNxpQoriqLsTokenSpaceGuid.PcdCcsrSize > - gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion1BaseAddr > - gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion1Size > - gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion2BaseAddr > - gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion2Size > - gNxpQoriqLsTokenSpaceGuid.PcdQmanSwpBaseAddr > - gNxpQoriqLsTokenSpaceGuid.PcdQmanSwpSize > - gNxpQoriqLsTokenSpaceGuid.PcdBmanSwpBaseAddr > - gNxpQoriqLsTokenSpaceGuid.PcdBmanSwpSize > - gNxpQoriqLsTokenSpaceGuid.PcdPciExp1BaseAddr > - gNxpQoriqLsTokenSpaceGuid.PcdPciExp1BaseSize > - gNxpQoriqLsTokenSpaceGuid.PcdPciExp2BaseAddr > - gNxpQoriqLsTokenSpaceGuid.PcdPciExp2BaseSize > - gNxpQoriqLsTokenSpaceGuid.PcdPciExp3BaseAddr > - gNxpQoriqLsTokenSpaceGuid.PcdPciExp3BaseSize > - gNxpQoriqLsTokenSpaceGuid.PcdQspiRegionBaseAddr > - gNxpQoriqLsTokenSpaceGuid.PcdQspiRegionSize > diff --git a/Platform/NXP/LS1043aRdbPkg/Library/PlatformLib/NxpQoriqLsMem.c b/Platform/NXP/LS1043aRdbPkg/Library/PlatformLib/NxpQoriqLsMem.c > index c6c256da07..3a72c8bdd8 100644 > --- a/Platform/NXP/LS1043aRdbPkg/Library/PlatformLib/NxpQoriqLsMem.c > +++ b/Platform/NXP/LS1043aRdbPkg/Library/PlatformLib/NxpQoriqLsMem.c > @@ -6,7 +6,7 @@ > * > * Copyright (c) 2011, ARM Limited. All rights reserved. > * Copyright (c) 2016, Freescale Semiconductor, Inc. All rights reserved. > -* Copyright 2017, 2019 NXP > +* Copyright 2017, 2019-2020 NXP > * > * SPDX-License-Identifier: BSD-2-Clause-Patent > * > @@ -16,7 +16,7 @@ > #include > #include > #include > -#include > +#include > > #define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 25 > > @@ -38,7 +38,6 @@ ArmPlatformGetVirtualMemoryMap ( > { > UINTN Index; > ARM_MEMORY_REGION_DESCRIPTOR *VirtualMemoryTable; > - DRAM_INFO DramInfo; > > Index = 0; > > @@ -51,25 +50,21 @@ ArmPlatformGetVirtualMemoryMap ( > return; > } > > - if (GetDramBankInfo (&DramInfo)) { > - DEBUG ((DEBUG_ERROR, "Failed to get DRAM information, exiting...\n")); > - return; > - } > + VirtualMemoryTable[Index].PhysicalBase = LS1043A_DRAM0_PHYS_ADDRESS; > + VirtualMemoryTable[Index].VirtualBase = LS1043A_DRAM0_PHYS_ADDRESS; > + VirtualMemoryTable[Index].Length = LS1043A_DRAM0_SIZE; > + VirtualMemoryTable[Index++].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK; > > - > - for (Index = 0; Index < DramInfo.NumOfDrams; Index++) { > - // DRAM1 (Must be 1st entry) > - VirtualMemoryTable[Index].PhysicalBase = DramInfo.DramRegion[Index].BaseAddress; > - VirtualMemoryTable[Index].VirtualBase = DramInfo.DramRegion[Index].BaseAddress; > - VirtualMemoryTable[Index].Length = DramInfo.DramRegion[Index].Size; > - VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK; > - } > + VirtualMemoryTable[Index].PhysicalBase = LS1043A_DRAM1_PHYS_ADDRESS; > + VirtualMemoryTable[Index].VirtualBase = LS1043A_DRAM1_PHYS_ADDRESS; > + VirtualMemoryTable[Index].Length = LS1043A_DRAM1_SIZE; > + VirtualMemoryTable[Index++].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK; No changes to this file so far appear to have anything to do with what the commit message says the patch does. > > // CCSR Space > - VirtualMemoryTable[Index].PhysicalBase = FixedPcdGet64 (PcdCcsrBaseAddr); > - VirtualMemoryTable[Index].VirtualBase = FixedPcdGet64 (PcdCcsrBaseAddr); > - VirtualMemoryTable[Index].Length = FixedPcdGet64 (PcdCcsrSize); > - VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; > + VirtualMemoryTable[Index].PhysicalBase = LS1043A_CCSR_PHYS_ADDRESS; > + VirtualMemoryTable[Index].VirtualBase = LS1043A_CCSR_PHYS_ADDRESS; > + VirtualMemoryTable[Index].Length = LS1043A_CCSR_SIZE; > + VirtualMemoryTable[Index++].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; If these are not reconfigurable for different platforms, sure. > > // IFC region 1 > // > @@ -85,60 +80,60 @@ ArmPlatformGetVirtualMemoryMap ( > // For write transactions from non-core masters (like system DMA), the address > // should be 16 byte aligned and the data size should be multiple of 16 bytes. > // > - VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet64 (PcdIfcRegion1BaseAddr); > - VirtualMemoryTable[Index].VirtualBase = FixedPcdGet64 (PcdIfcRegion1BaseAddr); > - VirtualMemoryTable[Index].Length = FixedPcdGet64 (PcdIfcRegion1Size); > - VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; > + VirtualMemoryTable[Index].PhysicalBase = LS1043A_IFC0_PHYS_ADDRESS; > + VirtualMemoryTable[Index].VirtualBase = LS1043A_IFC0_PHYS_ADDRESS; > + VirtualMemoryTable[Index].Length = LS1043A_IFC0_SIZE; > + VirtualMemoryTable[Index++].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; If these are not reconfigurable for different platforms, sure. > > // QMAN SWP > - VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet64 (PcdQmanSwpBaseAddr); > - VirtualMemoryTable[Index].VirtualBase = FixedPcdGet64 (PcdQmanSwpBaseAddr); > - VirtualMemoryTable[Index].Length = FixedPcdGet64 (PcdQmanSwpSize); > - VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED; > + VirtualMemoryTable[Index].PhysicalBase = LS1043A_QMAN_SW_PORTAL_PHYS_ADDRESS; > + VirtualMemoryTable[Index].VirtualBase = LS1043A_QMAN_SW_PORTAL_PHYS_ADDRESS; > + VirtualMemoryTable[Index].Length = LS1043A_QMAN_SW_PORTAL_SIZE; > + VirtualMemoryTable[Index++].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED; > If these are not reconfigurable for different platforms, sure. > // BMAN SWP > - VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet64 (PcdBmanSwpBaseAddr); > - VirtualMemoryTable[Index].VirtualBase = FixedPcdGet64 (PcdBmanSwpBaseAddr); > - VirtualMemoryTable[Index].Length = FixedPcdGet64 (PcdBmanSwpSize); > - VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED; > + VirtualMemoryTable[Index].PhysicalBase = LS1043A_BMAN_SW_PORTAL_PHYS_ADDRESS; > + VirtualMemoryTable[Index].VirtualBase = LS1043A_BMAN_SW_PORTAL_PHYS_ADDRESS; > + VirtualMemoryTable[Index].Length = LS1043A_QMAN_SW_PORTAL_SIZE; > + VirtualMemoryTable[Index++].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED; > If these are not reconfigurable for different platforms, sure. > // IFC region 2 > - VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet64 (PcdIfcRegion2BaseAddr); > - VirtualMemoryTable[Index].VirtualBase = FixedPcdGet64 (PcdIfcRegion2BaseAddr); > - VirtualMemoryTable[Index].Length = FixedPcdGet64 (PcdIfcRegion2Size); > - VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; > + VirtualMemoryTable[Index].PhysicalBase = LS1043A_IFC1_PHYS_ADDRESS; > + VirtualMemoryTable[Index].VirtualBase = LS1043A_IFC1_PHYS_ADDRESS; > + VirtualMemoryTable[Index].Length = LS1043A_IFC1_SIZE; > + VirtualMemoryTable[Index++].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; > If these are not reconfigurable for different platforms, sure. > // PCIe1 > - VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet64 (PcdPciExp1BaseAddr); > - VirtualMemoryTable[Index].VirtualBase = FixedPcdGet64 (PcdPciExp1BaseAddr); > - VirtualMemoryTable[Index].Length = FixedPcdGet64 (PcdPciExp1BaseSize); > - VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; > + VirtualMemoryTable[Index].PhysicalBase = LS1043A_PCI0_PHYS_ADDRESS; > + VirtualMemoryTable[Index].VirtualBase = LS1043A_PCI0_PHYS_ADDRESS; > + VirtualMemoryTable[Index].Length = LS1043A_PCI_SIZE; > + VirtualMemoryTable[Index++].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; > If these are not reconfigurable for different platforms, sure. > // PCIe2 > - VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet64 (PcdPciExp2BaseAddr); > - VirtualMemoryTable[Index].VirtualBase = FixedPcdGet64 (PcdPciExp2BaseAddr); > - VirtualMemoryTable[Index].Length = FixedPcdGet64 (PcdPciExp2BaseSize); > - VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; > + VirtualMemoryTable[Index].PhysicalBase = LS1043A_PCI1_PHYS_ADDRESS; > + VirtualMemoryTable[Index].VirtualBase = LS1043A_PCI1_PHYS_ADDRESS; > + VirtualMemoryTable[Index].Length = LS1043A_PCI_SIZE; > + VirtualMemoryTable[Index++].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; > If these are not reconfigurable for different platforms, sure. > // PCIe3 > - VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet64 (PcdPciExp3BaseAddr); > - VirtualMemoryTable[Index].VirtualBase = FixedPcdGet64 (PcdPciExp3BaseAddr); > - VirtualMemoryTable[Index].Length = FixedPcdGet64 (PcdPciExp3BaseSize); > - VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; > + VirtualMemoryTable[Index].PhysicalBase = LS1043A_PCI2_PHYS_ADDRESS; > + VirtualMemoryTable[Index].VirtualBase = LS1043A_PCI2_PHYS_ADDRESS; > + VirtualMemoryTable[Index].Length = LS1043A_PCI_SIZE; > + VirtualMemoryTable[Index++].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; If these are not reconfigurable for different platforms, sure. > > // QSPI region > - VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet64 (PcdQspiRegionBaseAddr); > - VirtualMemoryTable[Index].VirtualBase = FixedPcdGet64 (PcdQspiRegionBaseAddr); > - VirtualMemoryTable[Index].Length = FixedPcdGet64 (PcdQspiRegionSize); > - VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED; > + VirtualMemoryTable[Index].PhysicalBase = LS1043A_QSPI_PHYS_ADDRESS; > + VirtualMemoryTable[Index].VirtualBase = LS1043A_QSPI_PHYS_ADDRESS; > + VirtualMemoryTable[Index].Length = LS1043A_QSPI_SIZE; > + VirtualMemoryTable[Index++].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED; If these are not reconfigurable for different platforms, sure. > > // End of Table > - VirtualMemoryTable[++Index].PhysicalBase = 0; > + VirtualMemoryTable[Index].PhysicalBase = 0; Move that ++ back where it belongs - there is no functional change here. > VirtualMemoryTable[Index].VirtualBase = 0; > VirtualMemoryTable[Index].Length = 0; > - VirtualMemoryTable[Index].Attributes = (ARM_MEMORY_REGION_ATTRIBUTES)0; > + VirtualMemoryTable[Index++].Attributes = (ARM_MEMORY_REGION_ATTRIBUTES)0; Move that ++ back where it belongs - there is no functional change here. > > - ASSERT ((Index + 1) <= MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS); > + ASSERT (Index < MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS); Drop that +1 = - there is no functional change here. If you want to submit separate patches for coding style changes, that's fine. But given that this set looks like it's making up for not synchronising internal development with the (very long) upstreaming effort so far, I am not interested in those until all functional changes are in. > > *VirtualMemoryMap = VirtualMemoryTable; > } > diff --git a/Silicon/NXP/Drivers/I2cDxe/I2cDxe.inf b/Silicon/NXP/Drivers/I2cDxe/I2cDxe.inf > index 784139065f..fc4bb618fa 100644 > --- a/Silicon/NXP/Drivers/I2cDxe/I2cDxe.inf > +++ b/Silicon/NXP/Drivers/I2cDxe/I2cDxe.inf > @@ -49,11 +49,5 @@ > gEdkiiNonDiscoverableDeviceProtocolGuid ## TO_START > gEfiI2cMasterProtocolGuid ## BY_START > > -[Pcd] > - gNxpQoriqLsTokenSpaceGuid.PcdI2cSpeed > - gNxpQoriqLsTokenSpaceGuid.PcdI2c0BaseAddr > - gNxpQoriqLsTokenSpaceGuid.PcdI2cSize > - gNxpQoriqLsTokenSpaceGuid.PcdNumI2cController > - > [Depex] > TRUE > diff --git a/Silicon/NXP/Include/Chassis2/NxpSoc.h b/Silicon/NXP/Include/Chassis2/NxpSoc.h > index 74330b6205..6812beafe4 100644 > --- a/Silicon/NXP/Include/Chassis2/NxpSoc.h > +++ b/Silicon/NXP/Include/Chassis2/NxpSoc.h > @@ -12,6 +12,8 @@ > > #define CLK_FREQ 100000000 > > +#define CHASSIS2_DCFG_ADDRESS 0x1EE0000 > + > /* SMMU Defintions */ > #define SMMU_BASE_ADDR 0x09000000 > #define SMMU_REG_SCR0 (SMMU_BASE_ADDR + 0x0) > diff --git a/Silicon/NXP/LS1043A/Include/Soc.h b/Silicon/NXP/LS1043A/Include/Soc.h > new file mode 100644 > index 0000000000..c1e00394af > --- /dev/null > +++ b/Silicon/NXP/LS1043A/Include/Soc.h > @@ -0,0 +1,44 @@ > +/** @file > + > + Copyright 2020 NXP > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > +#ifndef __SOC_H__ > +#define __SOC_H__ > + > +/** > + Soc Memory Map > +**/ > +#define LS1043A_DRAM0_PHYS_ADDRESS 0x80000000 > +#define LS1043A_DRAM0_SIZE SIZE_2GB > +#define LS1043A_DRAM1_PHYS_ADDRESS 0x880000000 > +#define LS1043A_DRAM1_SIZE 0x780000000 // 30 GB > + > +#define LS1043A_CCSR_PHYS_ADDRESS 0x1000000 > +#define LS1043A_CCSR_SIZE 0xF000000 > + > +#define LS1043A_IFC0_PHYS_ADDRESS 0x60000000 > +#define LS1043A_IFC0_SIZE SIZE_512MB > +#define LS1043A_IFC1_PHYS_ADDRESS 0x620000000 > +#define LS1043A_IFC1_SIZE 0xE0000000 // 3.5 GB > + > +#define LS1043A_QSPI_PHYS_ADDRESS 0x40000000 > +#define LS1043A_QSPI_SIZE SIZE_512MB > + > +#define LS1043A_QMAN_SW_PORTAL_PHYS_ADDRESS 0x500000000 > +#define LS1043A_QMAN_SW_PORTAL_SIZE SIZE_128MB > +#define LS1043A_BMAN_SW_PORTAL_PHYS_ADDRESS 0x508000000 > +#define LS1043A_BMAN_SW_PORTAL_SIZE SIZE_128MB > + > +#define LS1043A_PCI0_PHYS_ADDRESS 0x4000000000 > +#define LS1043A_PCI1_PHYS_ADDRESS 0x4800000000 > +#define LS1043A_PCI2_PHYS_ADDRESS 0x5000000000 > +#define LS1043A_PCI_SIZE SIZE_32GB > + > +#define LS1043A_I2C0_PHYS_ADDRESS 0x2180000 > +#define LS1043A_I2C_SIZE 0x10000 > +#define LS1043A_I2C_NUM_CONTROLLERS 4 > + > +#endif > diff --git a/Silicon/NXP/LS1043A/LS1043A.dsc.inc b/Silicon/NXP/LS1043A/LS1043A.dsc.inc > index 754eff396a..7ebbb1a495 100644 > --- a/Silicon/NXP/LS1043A/LS1043A.dsc.inc > +++ b/Silicon/NXP/LS1043A/LS1043A.dsc.inc > @@ -26,40 +26,8 @@ > [PcdsFixedAtBuild.common] > gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterBase|0x021c0500 > > - # > - # CCSR Address Space and other attached Memories > - # > - gNxpQoriqLsTokenSpaceGuid.PcdCcsrBaseAddr|0x01000000 > - gNxpQoriqLsTokenSpaceGuid.PcdCcsrSize|0x0F000000 > - gNxpQoriqLsTokenSpaceGuid.PcdClkBaseAddr|0x01EE1000 > - gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion1BaseAddr|0x60000000 > - gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion1Size|0x20000000 > - gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion2BaseAddr|0x0620000000 > - gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion2Size|0x00E0000000 > - gNxpQoriqLsTokenSpaceGuid.PcdIfcNandReservedSize|0x2EA > - gNxpQoriqLsTokenSpaceGuid.PcdQmanSwpBaseAddr|0x0500000000 > - gNxpQoriqLsTokenSpaceGuid.PcdQmanSwpSize|0x0080000000 > - gNxpQoriqLsTokenSpaceGuid.PcdBmanSwpBaseAddr|0x0508000000 > - gNxpQoriqLsTokenSpaceGuid.PcdBmanSwpSize|0x0080000000 > - gNxpQoriqLsTokenSpaceGuid.PcdPciExp1BaseAddr|0x4000000000 > - gNxpQoriqLsTokenSpaceGuid.PcdPciExp1BaseSize|0x800000000 > - gNxpQoriqLsTokenSpaceGuid.PcdPciExp2BaseAddr|0x4800000000 > - gNxpQoriqLsTokenSpaceGuid.PcdPciExp2BaseSize|0x800000000 > - gNxpQoriqLsTokenSpaceGuid.PcdPciExp3BaseAddr|0x5000000000 > - gNxpQoriqLsTokenSpaceGuid.PcdPciExp3BaseSize|0x800000000 > - gNxpQoriqLsTokenSpaceGuid.PcdScfgBaseAddr|0x1570000 > - gNxpQoriqLsTokenSpaceGuid.PcdGutsBaseAddr|0x01EE0000 > - gNxpQoriqLsTokenSpaceGuid.PcdWatchdog1BaseAddr|0x02AD0000 > - gNxpQoriqLsTokenSpaceGuid.PcdSdxcBaseAddr|0x01560000 > - gNxpQoriqLsTokenSpaceGuid.PcdI2c0BaseAddr|0x02180000 > - gNxpQoriqLsTokenSpaceGuid.PcdI2cSize|0x10000 > - gNxpQoriqLsTokenSpaceGuid.PcdNumI2cController|4 > - gNxpQoriqLsTokenSpaceGuid.PcdQspiRegionBaseAddr|0x40000000 > - gNxpQoriqLsTokenSpaceGuid.PcdQspiRegionSize|0x20000000 > - > # > # Big Endian IPs > # > gNxpQoriqLsTokenSpaceGuid.PcdGurBigEndian|TRUE > - gNxpQoriqLsTokenSpaceGuid.PcdWatchdogBigEndian|TRUE > ## > diff --git a/Silicon/NXP/Library/SocLib/Chassis2/Soc.c b/Silicon/NXP/Library/SocLib/Chassis2/Soc.c > index 9baeb17ecf..a3dabc93d1 100644 > --- a/Silicon/NXP/Library/SocLib/Chassis2/Soc.c > +++ b/Silicon/NXP/Library/SocLib/Chassis2/Soc.c > @@ -34,7 +34,7 @@ GetSysInfo ( > CCSR_GUR *GurBase; > UINTN SysClk; > > - GurBase = (VOID *)PcdGet64 (PcdGutsBaseAddr); > + GurBase = (CCSR_GUR *)CHASSIS2_DCFG_ADDRESS; > SysClk = CLK_FREQ; > > SetMem (PtrSysInfo, sizeof (SYS_INFO), 0); > diff --git a/Silicon/NXP/Library/SocLib/LS1043aSocLib.inf b/Silicon/NXP/Library/SocLib/LS1043aSocLib.inf > index fe77717337..d8707927b7 100644 > --- a/Silicon/NXP/Library/SocLib/LS1043aSocLib.inf > +++ b/Silicon/NXP/Library/SocLib/LS1043aSocLib.inf > @@ -40,9 +40,5 @@ > gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity > gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits > gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString > - gNxpQoriqLsTokenSpaceGuid.PcdClkBaseAddr > gNxpQoriqLsTokenSpaceGuid.PcdGurBigEndian > - gNxpQoriqLsTokenSpaceGuid.PcdGutsBaseAddr > gNxpQoriqLsTokenSpaceGuid.PcdPlatformFreqDiv > - gNxpQoriqLsTokenSpaceGuid.PcdScfgBaseAddr > - gNxpQoriqLsTokenSpaceGuid.PcdSerdes2Enabled > diff --git a/Silicon/NXP/NxpQoriqLs.dec b/Silicon/NXP/NxpQoriqLs.dec > index 4a1cfb3e27..b478560450 100644 > --- a/Silicon/NXP/NxpQoriqLs.dec > +++ b/Silicon/NXP/NxpQoriqLs.dec > @@ -22,89 +22,15 @@ > gNxpNonDiscoverableI2cMasterGuid = { 0x5f2c099c, 0x54a3, 0x4dd4, {0x9e, 0xc5, 0xe9, 0x12, 0x8c, 0x36, 0x81, 0x6a}} > > [PcdsFixedAtBuild.common] > - # > - # Pcds for I2C Controller > - # > - gNxpQoriqLsTokenSpaceGuid.PcdI2cSpeed|0|UINT32|0x00000001 > - gNxpQoriqLsTokenSpaceGuid.PcdNumI2cController|0|UINT32|0x00000002 > - > - # > - # Pcds for base address and size > - # > - gNxpQoriqLsTokenSpaceGuid.PcdGutsBaseAddr|0x0|UINT64|0x00000100 > - gNxpQoriqLsTokenSpaceGuid.PcdPiFdSize|0x0|UINT32|0x00000101 > - gNxpQoriqLsTokenSpaceGuid.PcdPiFdBaseAddress|0x0|UINT64|0x00000102 > - gNxpQoriqLsTokenSpaceGuid.PcdClkBaseAddr|0x0|UINT64|0x00000103 > - gNxpQoriqLsTokenSpaceGuid.PcdWatchdog1BaseAddr|0x0|UINT64|0x00000104 > - gNxpQoriqLsTokenSpaceGuid.PcdDdrBaseAddr|0x0|UINT64|0x00000105 > - gNxpQoriqLsTokenSpaceGuid.PcdSdxcBaseAddr|0x0|UINT64|0x00000106 > - gNxpQoriqLsTokenSpaceGuid.PcdScfgBaseAddr|0x0|UINT64|0x00000107 > - gNxpQoriqLsTokenSpaceGuid.PcdI2c0BaseAddr|0x0|UINT64|0x00000108 > - gNxpQoriqLsTokenSpaceGuid.PcdI2cSize|0x0|UINT32|0x00000109 > - gNxpQoriqLsTokenSpaceGuid.PcdDcsrBaseAddr|0x0|UINT64|0x0000010A > - gNxpQoriqLsTokenSpaceGuid.PcdDcsrSize|0x0|UINT64|0x0000010B > - gNxpQoriqLsTokenSpaceGuid.PcdSataBaseAddr|0x0|UINT32|0x0000010C > - gNxpQoriqLsTokenSpaceGuid.PcdSataSize|0x0|UINT32|0x0000010D > - gNxpQoriqLsTokenSpaceGuid.PcdQmanSwpBaseAddr|0x0|UINT64|0x0000010E > - gNxpQoriqLsTokenSpaceGuid.PcdQmanSwpSize|0x0|UINT64|0x0000010F > - gNxpQoriqLsTokenSpaceGuid.PcdBmanSwpBaseAddr|0x0|UINT64|0x00000110 > - gNxpQoriqLsTokenSpaceGuid.PcdBmanSwpSize|0x0|UINT64|0x00000111 > - gNxpQoriqLsTokenSpaceGuid.PcdPciExp1BaseAddr|0x0|UINT64|0x00000112 > - gNxpQoriqLsTokenSpaceGuid.PcdPciExp1BaseSize|0x0|UINT64|0x00000113 > - gNxpQoriqLsTokenSpaceGuid.PcdPciExp2BaseAddr|0x0|UINT64|0x00000114 > - gNxpQoriqLsTokenSpaceGuid.PcdPciExp2BaseSize|0x0|UINT64|0x00000115 > - gNxpQoriqLsTokenSpaceGuid.PcdPciExp3BaseAddr|0x0|UINT64|0x00000116 > - gNxpQoriqLsTokenSpaceGuid.PcdPciExp3BaseSize|0x0|UINT64|0x00000117 > - gNxpQoriqLsTokenSpaceGuid.PcdPciExp4BaseAddr|0x0|UINT64|0x0000118 > - gNxpQoriqLsTokenSpaceGuid.PcdPciExp4BaseSize|0x0|UINT64|0x0000119 > - gNxpQoriqLsTokenSpaceGuid.PcdQspiRegionBaseAddr|0x0|UINT64|0x0000011A > - gNxpQoriqLsTokenSpaceGuid.PcdQspiRegionSize|0x0|UINT64|0x0000011B > - gNxpQoriqLsTokenSpaceGuid.PcdQspiRegion2BaseAddr|0x0|UINT64|0x0000011C > - gNxpQoriqLsTokenSpaceGuid.PcdQspiRegion2Size|0x0|UINT64|0x0000011D > - gNxpQoriqLsTokenSpaceGuid.PcdSystemMemoryExBase|0x0|UINT64|0x0000011E > - gNxpQoriqLsTokenSpaceGuid.PcdSystemMemoryExSize|0x0|UINT64|0x0000011F > - gNxpQoriqLsTokenSpaceGuid.PcdUsbBaseAddr|0x0|UINT32|0x00000120 > - gNxpQoriqLsTokenSpaceGuid.PcdUsbSize|0x0|UINT32|0x00000121 > - gNxpQoriqLsTokenSpaceGuid.PcdCcsrBaseAddr|0x0|UINT64|0x00000122 > - gNxpQoriqLsTokenSpaceGuid.PcdCcsrSize|0x0|UINT64|0x00000123 > - > - # > - # IFC PCDs > - # > - gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion1BaseAddr|0x0|UINT64|0x00000190 > - gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion1Size|0x0|UINT64|0x00000191 > - gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion2BaseAddr|0x0|UINT64|0x00000192 > - gNxpQoriqLsTokenSpaceGuid.PcdIfcRegion2Size|0x0|UINT64|0x00000193 > - gNxpQoriqLsTokenSpaceGuid.PcdIfcNandReservedSize|0x0|UINT32|0x00000194 > - gNxpQoriqLsTokenSpaceGuid.PcdFlashDeviceBase64|0x0|UINT64|0x00000195 > - gNxpQoriqLsTokenSpaceGuid.PcdFlashReservedRegionBase64|0x0|UINT64|0x00000196 > - > - # > - # NV Pcd > - # > - gNxpQoriqLsTokenSpaceGuid.PcdNvFdBase|0x0|UINT64|0x00000210 > - gNxpQoriqLsTokenSpaceGuid.PcdNvFdSize|0x0|UINT64|0x00000211 Not able to be different between different platforms using the same SoC? > - > # > # Platform PCDs > # > gNxpQoriqLsTokenSpaceGuid.PcdPlatformFreqDiv|0x0|UINT32|0x00000250 > - gNxpQoriqLsTokenSpaceGuid.PcdSerdes2Enabled|FALSE|BOOLEAN|0x00000251 > - > - # > - # Clock PCDs > - # > - gNxpQoriqLsTokenSpaceGuid.PcdSysClk|0x0|UINT64|0x000002A0 > - gNxpQoriqLsTokenSpaceGuid.PcdDdrClk|0x0|UINT64|0x000002A1 And clocks also sound very much like something that can change per platform? / Leif > > # > # Pcds to support Big Endian IPs > # > - gNxpQoriqLsTokenSpaceGuid.PcdMmcBigEndian|FALSE|BOOLEAN|0x0000310 > gNxpQoriqLsTokenSpaceGuid.PcdGurBigEndian|FALSE|BOOLEAN|0x0000311 > - gNxpQoriqLsTokenSpaceGuid.PcdPciLutBigEndian|FALSE|BOOLEAN|0x00000312 > - gNxpQoriqLsTokenSpaceGuid.PcdWatchdogBigEndian|FALSE|BOOLEAN|0x00000313 > - gNxpQoriqLsTokenSpaceGuid.PcdIfcBigEndian|FALSE|BOOLEAN|0x00000314 > > [PcdsFeatureFlag] > gNxpQoriqLsTokenSpaceGuid.PcdI2cErratumA009203|FALSE|BOOLEAN|0x00000315 > -- > 2.17.1 >