From: "Aditya Angadi" <aditya.angadi@arm.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
Ard Biesheuvel <Ard.Biesheuvel@arm.com>
Cc: Vijayenthiran Subramaniam <Vijayenthiran.Subramaniam@arm.com>,
Leif Lindholm <leif@nuviainc.com>
Subject: Re: [edk2-devel] [edk2-platforms][PATCH v3 1/9] Platform/ARM/SgiPkg: create individual
Date: Mon, 13 Apr 2020 12:31:03 +0000 [thread overview]
Message-ID: <AM0PR08MB44992B6FABC6858CF6B9271CE1DD0@AM0PR08MB4499.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <CAKv+Gu9zPvo4=UcnoFT4dJd8LcE3AyfVmhi_ywkAvsjQw4P7+w@mail.gmail.com>
Hi Ard ,
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> Biesheuvel via Groups.Io
> Sent: 31 March 2020 16:13
> To: Aditya Angadi <Aditya.Angadi@arm.com>
> Cc: edk2-devel-groups-io <devel@edk2.groups.io>; Vijayenthiran
> Subramaniam <Vijayenthiran.Subramaniam@arm.com>; Leif Lindholm
> <leif@nuviainc.com>
> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v3 1/9]
> Platform/ARM/SgiPkg: create individual
>
> On Tue, 31 Mar 2020 at 12:33, Ard Biesheuvel <ard.biesheuvel@arm.com>
> wrote:
> >
> > Hello Aditya,
> >
> > Could you please use a meaningful subject line for this patch?
> >
> > On Wed, 25 Mar 2020 at 11:53, Aditya Angadi <aditya.angadi@arm.com>
> wrote:
> > >
> > > From: Vijayenthiran Subramaniam
> <vijayenthiran.subramaniam@arm.com>
> > >
> > > 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 <leif@nuviainc.com>
> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > Signed-off-by: Aditya Angadi <aditya.angadi@arm.com>
> > > ---
> > > 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.
Apologies in delay for replying to this email. We attempted to have a single build support for SgiPkg but there are other platform specific differences in the upcoming RD Platforms and that is causing additional software changes to maintain single build support and we do not have a requirement to for single build support at this point. So we are now moving away from single build. I have addressed your other comments and patchset V4 will be posted soon.
Thanks
Aditya
>
> > >
> +#########################################################
> ##########
> > > +#############
> > > +#
> > > +# 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|0x0000
> > > 0009
> > >
> > > + # 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
> > >
>
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
next prev parent reply other threads:[~2020-04-13 12:31 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-25 10:52 [edk2-platforms][PATCH v3 0/8] Add platform support for RD-Daniel Aditya Angadi
2020-03-25 10:52 ` [edk2-platforms][PATCH v3 1/9] Platform/ARM/SgiPkg: create individual Aditya Angadi
2020-03-31 10:33 ` Ard Biesheuvel
2020-03-31 10:42 ` Ard Biesheuvel
2020-04-13 12:31 ` Aditya Angadi [this message]
2020-03-25 10:52 ` [edk2-platforms][PATCH v3 2/9] Platform/ARM/SgiPkg: move the GIC Aditya Angadi
2020-03-31 10:35 ` Ard Biesheuvel
2020-03-25 10:52 ` [edk2-platforms][PATCH v3 3/9] Platform/ARM/SgiPkg: move common Aditya Angadi
2020-03-31 10:36 ` Ard Biesheuvel
2020-03-25 10:52 ` [edk2-platforms][PATCH v3 4/9] Platform/ARM/SgiPkg: remove Aditya Angadi
2020-03-31 10:37 ` Ard Biesheuvel
2020-03-25 10:52 ` [edk2-platforms][PATCH v3 5/9] Platform/ARM/SgiPkg: add ACPI tables Aditya Angadi
2020-03-31 10:40 ` Ard Biesheuvel
2020-03-25 10:52 ` [edk2-platforms][PATCH v3 6/9] Platform/ARM/Sgi: add initial support Aditya Angadi
2020-03-31 10:39 ` Ard Biesheuvel
2020-03-25 10:52 ` [edk2-platforms][PATCH v3 7/9] Platform/ARM/SgiPkg: add ACPI tables Aditya Angadi
2020-03-31 10:45 ` Ard Biesheuvel
2020-03-25 10:52 ` [edk2-platforms][PATCH v3 8/9] Platform/ARM/Sgi: add initial support Aditya Angadi
2020-03-31 10:46 ` Ard Biesheuvel
2020-03-25 10:52 ` [edk2-platforms][PATCH v3 9/9] Maintainers.txt: Update Arm platform Aditya Angadi
2020-03-31 10:48 ` Ard Biesheuvel
2020-03-29 16:41 ` [edk2-devel] [edk2-platforms][PATCH v3 0/8] Add platform support for RD-Daniel Thomas Abraham
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=AM0PR08MB44992B6FABC6858CF6B9271CE1DD0@AM0PR08MB4499.eurprd08.prod.outlook.com \
--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