From: "Leif Lindholm" <leif@nuviainc.com>
To: Pankaj Bansal <pankaj.bansal@nxp.com>
Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>,
Michael D Kinney <michael.d.kinney@intel.com>,
Varun Sethi <V.Sethi@nxp.com>,
devel@edk2.groups.io
Subject: Re: [PATCH 08/19] Silicon/NXP: Remove unnecessary PCDs
Date: Mon, 10 Feb 2020 17:32:00 +0000 [thread overview]
Message-ID: <20200210173200.GM23627@bivouac.eciton.net> (raw)
In-Reply-To: <20200207124328.8723-9-pankaj.bansal@nxp.com>
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 <pankaj.bansal@nxp.com>
> ---
> .../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 <Library/PcdLib.h>
> #include <Library/UefiBootServicesTableLib.h>
> #include <Library/UefiLib.h>
> +#include <Soc.h>
>
> #include <Protocol/NonDiscoverableDevice.h>
>
> @@ -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 <Library/DebugLib.h>
> #include <Library/PcdLib.h>
> #include <Library/MemoryAllocationLib.h>
> -#include <DramInfo.h>
> +#include <Soc.h>
>
> #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
>
next prev parent reply other threads:[~2020-02-10 17:32 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-07 12:43 [PATCH 00/19] ADD LX2160ARDB Platform Support Pankaj Bansal
2020-02-07 12:43 ` [PATCH 01/19] Silicon/NXP: Add I2c lib Pankaj Bansal
2020-02-08 17:13 ` Leif Lindholm
2020-02-09 11:49 ` [edk2-devel] " Ard Biesheuvel
2020-02-07 12:43 ` [PATCH 02/19] Silicon/NXP: changes to use I2clib in i2cdxe Pankaj Bansal
2020-02-08 17:23 ` Leif Lindholm
2020-02-07 12:43 ` [PATCH 03/19] NXP/LS1043aRdb: Move Soc specific components to soc files Pankaj Bansal
2020-02-08 17:27 ` Leif Lindholm
2020-02-07 12:43 ` [PATCH 04/19] Silicon/NXP: Remove DuartLib and use BaseSerialPortLib16550 Pankaj Bansal
2020-02-08 17:46 ` Leif Lindholm
2020-02-10 5:48 ` Pankaj Bansal
2020-02-12 23:27 ` Leif Lindholm
2020-02-07 12:43 ` [PATCH 05/19] NXP/BaseSerialPortLib16550: remove SerialPortInitalize functionality Pankaj Bansal
2020-02-07 12:43 ` [PATCH 06/19] Silicon/NXP: remove print information from Soc lib Pankaj Bansal
2020-02-10 17:09 ` [EXTERNAL] " Leif Lindholm
2020-02-07 12:43 ` [PATCH 07/19] Silicon/NXP: remove not needed components Pankaj Bansal
2020-02-10 17:11 ` Leif Lindholm
2020-02-11 7:24 ` Pankaj Bansal
2020-02-20 19:05 ` Leif Lindholm
2020-02-07 12:43 ` [PATCH 08/19] Silicon/NXP: Remove unnecessary PCDs Pankaj Bansal
2020-02-10 17:32 ` Leif Lindholm [this message]
2020-02-11 8:45 ` Pankaj Bansal
2020-02-20 18:56 ` Leif Lindholm
2020-02-07 12:43 ` [PATCH 09/19] Silicon/NXP: Move dsc file Pankaj Bansal
2020-02-11 11:35 ` Leif Lindholm
2020-02-07 12:43 ` [PATCH 10/19] Platform/NXP: rename the ArmPlatformLib as per ArmPlatformPkg Pankaj Bansal
2020-02-11 11:40 ` Leif Lindholm
2020-02-07 12:43 ` [PATCH 11/19] Silicon/NXP: Add Chassis Lib for Chassis2 Pankaj Bansal
2020-02-11 12:28 ` Leif Lindholm
2020-02-07 12:43 ` [PATCH 12/19] Silicon/NXP/LS1043A: Add SocLib Pankaj Bansal
2020-02-11 12:38 ` Leif Lindholm
2020-02-07 12:43 ` [PATCH 13/19] Silicon/NXP: Move RAM retrieval from SocLib Pankaj Bansal
2020-02-11 13:28 ` Leif Lindholm
2020-02-07 12:43 ` [PATCH 14/19] Silicon/NXP/LS1043A: Replce SocLib Pankaj Bansal
2020-02-11 13:35 ` Leif Lindholm
2020-02-12 9:37 ` Pankaj Bansal
2020-02-12 22:50 ` Leif Lindholm
2020-02-13 11:00 ` Pankaj Bansal
2020-02-20 18:45 ` Leif Lindholm
2020-02-07 12:43 ` [PATCH 15/19] Platform/NXP/LS1043ARDB: introduce PEI Phase Pankaj Bansal
2020-02-12 20:24 ` Leif Lindholm
2020-02-07 12:43 ` [PATCH 16/19] Silicon/NXP: Add Pl011 Serial port lib Pankaj Bansal
2020-02-12 20:26 ` Leif Lindholm
2020-02-07 12:43 ` [PATCH 17/19] Silicon/NXP: Add Chassis3V2 Pankaj Bansal
2020-02-12 20:33 ` Leif Lindholm
2020-02-07 12:43 ` [PATCH 18/19] Silicon/NXP: Add LX2160A SocLib Pankaj Bansal
2020-02-12 21:39 ` Leif Lindholm
2020-02-07 12:43 ` [PATCH 19/19] Platform/NXP: Add LX2160ARDBPKG Pankaj Bansal
2020-02-12 21:36 ` Leif Lindholm
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200210173200.GM23627@bivouac.eciton.net \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox