From: "Ard Biesheuvel" <ard.biesheuvel@arm.com>
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-platforms][PATCH v3 1/9] Platform/ARM/SgiPkg: create individual
Date: Tue, 31 Mar 2020 12:42:42 +0200 [thread overview]
Message-ID: <CAKv+Gu9zPvo4=UcnoFT4dJd8LcE3AyfVmhi_ywkAvsjQw4P7+w@mail.gmail.com> (raw)
In-Reply-To: <CAKv+Gu9zXdhbLnwG_93gQuy39-yNfnQJY-LaapuM9n2vRA3+=g@mail.gmail.com>
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.
> > +################################################################################
> > +#
> > +# 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
> >
next prev parent reply other threads:[~2020-03-31 10:42 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 [this message]
2020-04-13 12:31 ` [edk2-devel] " Aditya Angadi
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='CAKv+Gu9zPvo4=UcnoFT4dJd8LcE3AyfVmhi_ywkAvsjQw4P7+w@mail.gmail.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