From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web12.6265.1585651379552528814 for ; Tue, 31 Mar 2020 03:42:59 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 04804113E for ; Tue, 31 Mar 2020 03:42:59 -0700 (PDT) Received: from mail-wm1-f43.google.com (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C4A2D3F99C for ; Tue, 31 Mar 2020 03:42:58 -0700 (PDT) Received: by mail-wm1-f43.google.com with SMTP id a81so1998842wmf.5 for ; Tue, 31 Mar 2020 03:42:58 -0700 (PDT) X-Gm-Message-State: ANhLgQ0wEcyJE6GD7ryX9P0kIpM4JndOi5hENrfrKalIr7mAcrYSOt0d PwZi24BMehua5jmAOVEZ6AP/LHl46j1oKAWSqldctQ== X-Google-Smtp-Source: ADFU+vvzz5mo+Q4hm74IJuu8ZYjcEVe87gPXCJWG1jXkUQWTChFEt5T9FDmUSnXQKoYSGX+foSb2i3eD41jnX1KcsvQ= X-Received: by 2002:a1c:6285:: with SMTP id w127mr2949693wmb.133.1585651373449; Tue, 31 Mar 2020 03:42:53 -0700 (PDT) MIME-Version: 1.0 References: <20200325105252.13905-1-aditya.angadi@arm.com> <20200325105252.13905-2-aditya.angadi@arm.com> In-Reply-To: From: "Ard Biesheuvel" Date: Tue, 31 Mar 2020 12:42:42 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-platforms][PATCH v3 1/9] Platform/ARM/SgiPkg: create individual To: Aditya Angadi Cc: edk2-devel-groups-io , Vijayenthiran Subramaniam , Leif Lindholm Content-Type: text/plain; charset="UTF-8" On Tue, 31 Mar 2020 at 12:33, Ard Biesheuvel wrote: > > Hello Aditya, > > Could you please use a meaningful subject line for this patch? > > On Wed, 25 Mar 2020 at 11:53, Aditya Angadi wrote: > > > > From: Vijayenthiran Subramaniam > > > > In preparation for adding support for Reference Design (RD) platforms > > that have different base addresses for GIC distributor or redistributor, > > create individual platform description files for all SGI/RD platforms > > and move GIC related base addresses from the common SGI/RD platform > > description file to individual platform description files. > > The existing platform description is then included by individual > > platform description files. > > > > Cc: Leif Lindholm > > Cc: Ard Biesheuvel > > Signed-off-by: Aditya Angadi > > --- > > Platform/ARM/SgiPkg/Include/SgiPlatform.h | 7 +--- > > Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf | 3 +- > > Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c | 8 ++--- > > Platform/ARM/SgiPkg/RdE1Edge.dsc | 37 ++++++++++++++++++++ > > Platform/ARM/SgiPkg/RdN1Edge.dsc | 37 ++++++++++++++++++++ > > Platform/ARM/SgiPkg/Sgi575.dsc | 37 ++++++++++++++++++++ > > Platform/ARM/SgiPkg/SgiPlatform.dec | 5 ++- > > Platform/ARM/SgiPkg/SgiPlatform.dsc | 25 ++----------- > > 8 files changed, 124 insertions(+), 35 deletions(-) > > > > diff --git a/Platform/ARM/SgiPkg/Include/SgiPlatform.h b/Platform/ARM/SgiPkg/Include/SgiPlatform.h > > index e36a412155ff..d87fb2b5409f 100644 > > --- a/Platform/ARM/SgiPkg/Include/SgiPlatform.h > > +++ b/Platform/ARM/SgiPkg/Include/SgiPlatform.h > > @@ -1,6 +1,6 @@ > > /** @file > > * > > -* Copyright (c) 2018, ARM Limited. All rights reserved. > > +* Copyright (c) 2018-2020, ARM Limited. All rights reserved. > > * > > * SPDX-License-Identifier: BSD-2-Clause-Patent > > * > > @@ -45,11 +45,6 @@ > > #define SGI_SUBSYS_GENERIC_WDOG_BASE 0x2A440000 > > #define SGI_SUBSYS_GENERIC_WDOG_SZ SIZE_128KB > > > > -// Sub System Peripherals - GIC > > -#define SGI_SUBSYS_GENERIC_GIC_BASE 0x30000000 > > -#define SGI_SUBSYS_GENERIC_GICR_BASE 0x300C0000 > > -#define SGI_SUBSYS_GENERIC_GIC_SZ SIZE_1MB > > - > > // Expansion AXI - Platform Peripherals - HDLCD1 > > #define SGI_EXP_PLAT_PERIPH_HDLCD1_BASE 0x7FF60000 > > #define SGI_EXP_PLAT_PERIPH_HDLCD1_SZ SIZE_64KB > > diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf > > index 3db70e900d61..a918afef5fba 100644 > > --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf > > +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf > > @@ -1,5 +1,5 @@ > > # > > -# Copyright (c) 2018, ARM Limited. All rights reserved. > > +# Copyright (c) 2018-2020, ARM Limited. All rights reserved. > > # > > # SPDX-License-Identifier: BSD-2-Clause-Patent > > # > > @@ -42,6 +42,7 @@ [FixedPcd] > > > > gArmSgiTokenSpaceGuid.PcdDramBlock2Base > > gArmSgiTokenSpaceGuid.PcdDramBlock2Size > > + gArmSgiTokenSpaceGuid.PcdGicSize > > > > gArmTokenSpaceGuid.PcdSystemMemoryBase > > gArmTokenSpaceGuid.PcdSystemMemorySize > > diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c > > index 845aeaf4dd49..8d0ad4ec9c84 100644 > > --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c > > +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c > > @@ -1,6 +1,6 @@ > > /** @file > > * > > -* Copyright (c) 2018, ARM Limited. All rights reserved. > > +* Copyright (c) 2018-2020, ARM Limited. All rights reserved. > > * > > * SPDX-License-Identifier: BSD-2-Clause-Patent > > * > > @@ -93,9 +93,9 @@ ArmPlatformGetVirtualMemoryMap ( > > VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; > > > > // Sub System Peripherals - GIC-600 > > - VirtualMemoryTable[++Index].PhysicalBase = SGI_SUBSYS_GENERIC_GIC_BASE; > > - VirtualMemoryTable[Index].VirtualBase = SGI_SUBSYS_GENERIC_GIC_BASE; > > - VirtualMemoryTable[Index].Length = SGI_SUBSYS_GENERIC_GIC_SZ; > > + VirtualMemoryTable[++Index].PhysicalBase = FixedPcdGet64(PcdGicDistributorBase); > > + VirtualMemoryTable[Index].VirtualBase = FixedPcdGet64(PcdGicDistributorBase); > > + VirtualMemoryTable[Index].Length = FixedPcdGet64(PcdGicSize); > > Please use a space before '(' > > > VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; > > > > // Expansion AXI - Platform Peripherals - HDLCD1 > > diff --git a/Platform/ARM/SgiPkg/RdE1Edge.dsc b/Platform/ARM/SgiPkg/RdE1Edge.dsc > > new file mode 100644 > > index 000000000000..082cbb0157f7 > > --- /dev/null > > +++ b/Platform/ARM/SgiPkg/RdE1Edge.dsc > > @@ -0,0 +1,37 @@ > > +# > > +# Copyright (c) 2020, ARM Limited. All rights reserved. > > +# > > +# SPDX-License-Identifier: BSD-2-Clause-Patent > > +# > > + > > +################################################################################ > > +# > > +# Defines Section - statements that will be processed to create a Makefile. > > +# > > +################################################################################ > > +[Defines] > > + PLATFORM_NAME = ArmSgi > > + PLATFORM_GUID = c834de39-c5b0-458b-8ea3-882427179b8a > > + PLATFORM_VERSION = 0.1 > > + DSC_SPECIFICATION = 0x0001001B > > + OUTPUT_DIRECTORY = Build/$(PLATFORM_NAME) > > + SUPPORTED_ARCHITECTURES = AARCH64|ARM > > + BUILD_TARGETS = NOOPT|DEBUG|RELEASE > > + SKUID_IDENTIFIER = DEFAULT > > + FLASH_DEFINITION = Platform/ARM/SgiPkg/SgiPlatform.fdf > > If you are going to split these into separate DSCs, please do this > first in a separate patch, and update the PLATFORM_NAME accordingly. > > If there is no need to use separate FDFs than you can keep using a > shared one. If there is, please do the split in the same patch. > > > + BUILD_NUMBER = 1 > > + > > +# include common definitions from SgiPlatform.dsc > > +!include Platform/ARM/SgiPkg/SgiPlatform.dsc > > + > > Please rename that file to use a .dsc.inc extension to make it clear > this is an include file > Actually, now that I looked more closely: is it the case that formerly, the same build could run on any SGI platforms, but after this change, you have separate builds for separate platforms? Because the FDF contains all the ACPI tables for all the platforms, right? Please consider carefully whether you really want to depart from this, and if you do, clean up all the overlap between the platforms after you split them into separate ones. > > +################################################################################ > > +# > > +# Pcd Section - list of all EDK II PCD Entries defined by this Platform > > +# > > +################################################################################ > > + > > +[PcdsFixedAtBuild.common] > > + # GIC Base Addresses > > + gArmTokenSpaceGuid.PcdGicDistributorBase|0x30000000 > > + gArmTokenSpaceGuid.PcdGicRedistributorsBase|0x300C0000 > > + gArmSgiTokenSpaceGuid.PcdGicSize|0x100000 > > diff --git a/Platform/ARM/SgiPkg/RdN1Edge.dsc b/Platform/ARM/SgiPkg/RdN1Edge.dsc > > new file mode 100644 > > index 000000000000..6774990ad6f6 > > --- /dev/null > > +++ b/Platform/ARM/SgiPkg/RdN1Edge.dsc > > @@ -0,0 +1,37 @@ > > +# > > +# Copyright (c) 2020, ARM Limited. All rights reserved. > > +# > > +# SPDX-License-Identifier: BSD-2-Clause-Patent > > +# > > + > > +################################################################################ > > +# > > +# Defines Section - statements that will be processed to create a Makefile. > > +# > > +################################################################################ > > +[Defines] > > + PLATFORM_NAME = ArmSgi > > + PLATFORM_GUID = dbc75915-03df-4640-8f3d-3d3abf7c119b > > + PLATFORM_VERSION = 0.1 > > + DSC_SPECIFICATION = 0x0001001B > > + OUTPUT_DIRECTORY = Build/$(PLATFORM_NAME) > > + SUPPORTED_ARCHITECTURES = AARCH64|ARM > > + BUILD_TARGETS = NOOPT|DEBUG|RELEASE > > + SKUID_IDENTIFIER = DEFAULT > > + FLASH_DEFINITION = Platform/ARM/SgiPkg/SgiPlatform.fdf > > + BUILD_NUMBER = 1 > > + > > +# include common definitions from SgiPlatform.dsc > > +!include Platform/ARM/SgiPkg/SgiPlatform.dsc > > + > > +################################################################################ > > +# > > +# Pcd Section - list of all EDK II PCD Entries defined by this Platform > > +# > > +################################################################################ > > + > > +[PcdsFixedAtBuild.common] > > + # GIC Base Addresses > > + gArmTokenSpaceGuid.PcdGicDistributorBase|0x30000000 > > + gArmTokenSpaceGuid.PcdGicRedistributorsBase|0x300C0000 > > + gArmSgiTokenSpaceGuid.PcdGicSize|0x100000 > > diff --git a/Platform/ARM/SgiPkg/Sgi575.dsc b/Platform/ARM/SgiPkg/Sgi575.dsc > > new file mode 100644 > > index 000000000000..3c1904c2da91 > > --- /dev/null > > +++ b/Platform/ARM/SgiPkg/Sgi575.dsc > > @@ -0,0 +1,37 @@ > > +# > > +# Copyright (c) 2020, ARM Limited. All rights reserved. > > +# > > +# SPDX-License-Identifier: BSD-2-Clause-Patent > > +# > > + > > +################################################################################ > > +# > > +# Defines Section - statements that will be processed to create a Makefile. > > +# > > +################################################################################ > > +[Defines] > > + PLATFORM_NAME = ArmSgi > > + PLATFORM_GUID = 3a6b2eae-0275-4b6e-a5d1-bd2ba1ce1fae > > + PLATFORM_VERSION = 0.1 > > + DSC_SPECIFICATION = 0x0001001B > > + OUTPUT_DIRECTORY = Build/$(PLATFORM_NAME) > > + SUPPORTED_ARCHITECTURES = AARCH64|ARM > > + BUILD_TARGETS = NOOPT|DEBUG|RELEASE > > + SKUID_IDENTIFIER = DEFAULT > > + FLASH_DEFINITION = Platform/ARM/SgiPkg/SgiPlatform.fdf > > + BUILD_NUMBER = 1 > > + > > +# include common definitions from SgiPlatform.dsc > > +!include Platform/ARM/SgiPkg/SgiPlatform.dsc > > + > > +################################################################################ > > +# > > +# Pcd Section - list of all EDK II PCD Entries defined by this Platform > > +# > > +################################################################################ > > + > > +[PcdsFixedAtBuild.common] > > + # GIC Base Addresses > > + gArmTokenSpaceGuid.PcdGicDistributorBase|0x30000000 > > + gArmTokenSpaceGuid.PcdGicRedistributorsBase|0x300C0000 > > + gArmSgiTokenSpaceGuid.PcdGicSize|0x100000 > > diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec b/Platform/ARM/SgiPkg/SgiPlatform.dec > > index 9d70ec677776..4ac3dec91e3d 100644 > > --- a/Platform/ARM/SgiPkg/SgiPlatform.dec > > +++ b/Platform/ARM/SgiPkg/SgiPlatform.dec > > @@ -1,5 +1,5 @@ > > # > > -# Copyright (c) 2018, ARM Limited. All rights reserved. > > +# Copyright (c) 2018-2020, ARM Limited. All rights reserved. > > # > > # SPDX-License-Identifier: BSD-2-Clause-Patent > > # > > @@ -49,5 +49,8 @@ [PcdsFixedAtBuild] > > gArmSgiTokenSpaceGuid.PcdVirtioNetSize|0x00000000|UINT32|0x00000008 > > gArmSgiTokenSpaceGuid.PcdVirtioNetInterrupt|0x00000000|UINT32|0x00000009 > > > > + # GIC > > + gArmSgiTokenSpaceGuid.PcdGicSize|0|UINT64|0x0000000A > > Wouldn't a UINT32 be sufficient to describe the size of a MMIO block? > > > + > > [Ppis] > > gNtFwConfigDtInfoPpiGuid = { 0x6f606eb3, 0x9123, 0x4e15, { 0xa8, 0x9b, 0x0f, 0xac, 0x66, 0xef, 0xd0, 0x17 } } > > diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc b/Platform/ARM/SgiPkg/SgiPlatform.dsc > > index 5226c5751e98..4e1fcefb1442 100644 > > --- a/Platform/ARM/SgiPkg/SgiPlatform.dsc > > +++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc > > @@ -1,26 +1,9 @@ > > # > > -# Copyright (c) 2018, ARM Limited. All rights reserved. > > +# Copyright (c) 2018-2020, ARM Limited. All rights reserved. > > # > > # SPDX-License-Identifier: BSD-2-Clause-Patent > > # > > > > -################################################################################ > > -# > > -# Defines Section - statements that will be processed to create a Makefile. > > -# > > -################################################################################ > > -[Defines] > > - PLATFORM_NAME = ArmSgi > > - PLATFORM_GUID = 3a6b2eae-0275-4b6e-a5d1-bd2ba1ce1fae > > - PLATFORM_VERSION = 0.1 > > - DSC_SPECIFICATION = 0x0001001B > > - OUTPUT_DIRECTORY = Build/$(PLATFORM_NAME) > > - SUPPORTED_ARCHITECTURES = AARCH64|ARM > > - BUILD_TARGETS = NOOPT|DEBUG|RELEASE > > - SKUID_IDENTIFIER = DEFAULT > > - FLASH_DEFINITION = Platform/ARM/SgiPkg/SgiPlatform.fdf > > - BUILD_NUMBER = 1 > > - > > !include Platform/ARM/VExpressPkg/ArmVExpress.dsc.inc > > > > [BuildOptions] > > @@ -93,7 +76,7 @@ [LibraryClasses.common.UEFI_DRIVER, LibraryClasses.common.UEFI_APPLICATION, Libr > > > > ################################################################################ > > # > > -# Pcd Section - list of all EDK II PCD Entries defined by this Platform > > +# Pcd Section - list of all EDK II PCD Entries common to all SGI/RD platforms > > # > > ################################################################################ > > > > @@ -126,10 +109,6 @@ [PcdsFixedAtBuild.common] > > gArmTokenSpaceGuid.PcdSystemMemoryBase|0x80000000 > > gArmTokenSpaceGuid.PcdSystemMemorySize|0x7F000000 > > > > - # GIC Base Addresses > > - gArmTokenSpaceGuid.PcdGicDistributorBase|0x30000000 > > - gArmTokenSpaceGuid.PcdGicRedistributorsBase|0x300C0000 > > - > > # > > # PCIe > > # > > -- > > 2.17.1 > >