Re: [edk2-devel] [edk2-platforms][PATCH 2/8] Platform/RPi: Replace Bcm283x SoC base register address with a PCD

[samer.el-haj-mahmoud@arm.com]

> I don't actually have a strong opinion here, but would appreciate an explicit statement that the code is being submitted
> as copyright ARM with author @elhajmahmoud.com and not @arm.com?

Leif, I use @elhajmahmoud.com for Github. But the code is Arm copyrighted (since I am doing this for work). Please let me know if you have any concerns .

Thanks for your review!

--Samer

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Leif Lindholm via Groups.Io
Sent: Monday, November 18, 2019 10:49 AM
To: Pete Batard <pete@akeo.ie>
Cc: devel@edk2.groups.io; ard.biesheuvel@linaro.org; philmd@redhat.com; Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>
Subject: Re: [edk2-devel] [edk2-platforms][PATCH 2/8] Platform/RPi: Replace Bcm283x SoC base register address with a PCD

On Thu, Nov 14, 2019 at 04:07:34PM +0000, Pete Batard wrote:
> From: Samer El-Haj-Mahmoud <samer@elhajmahmoud.com>
>
> Define BCM2836_SOC_REGISTERS from PcdBcm283xRegistersAddress. This is
> needed in preparation for adding Raspberry Pi 4 support, since the two
> Pi's have a different base addresses for the Bcm283x specific registers.

Minor style comments below, would be nice if you could fold into any
v2 coming based on Phil's comments. (Although from my reading the discussion, most of these intances may in fact fall out in a v2.)

Also adding Samer to cc on this reply so he sees the feedback.

> Signed-off-by: Pete Batard <pete@akeo.ie>
> ---
>  Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.inf | 2 ++
>  Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf               | 2 ++
>  Platform/RaspberryPi/Drivers/DwUsbHostDxe/DwUsbHostDxe.inf         | 4 ++++
>  Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.inf     | 5 ++++-
>  Platform/RaspberryPi/Drivers/SdHostDxe/SdHostDxe.inf               | 2 ++
>  Platform/RaspberryPi/Library/PlatformLib/PlatformLib.inf           | 3 ++-
>  Platform/RaspberryPi/RPi3/RPi3.dsc                                 | 6 +++++-
>  Silicon/Broadcom/Bcm283x/Bcm283x.dec                               | 7 +++++++
>  Silicon/Broadcom/Bcm283x/Drivers/InterruptDxe/InterruptDxe.inf     | 4 +++-
>  Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.inf                 | 5 +++++
>  Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h        | 3 ++-
>  Silicon/Broadcom/Bcm283x/Library/GpioLib/GpioLib.inf               | 4 ++++
>  12 files changed, 42 insertions(+), 5 deletions(-)
>
> diff --git
> a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.inf
> b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.inf
> index 3f0d7b6b9e9d..034c8c449f00 100644
> ---
> a/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.inf
> +++ b/Platform/RaspberryPi/Drivers/ArasanMmcHostDxe/ArasanMmcHostDxe.i
> +++ nf
> @@ -1,5 +1,6 @@
>  #/** @file
>  #
> +#  Copyright (c) 2019, ARM Limited. All rights reserved.

I don't actually have a strong opinion here, but would appreciate an explicit statement that the code is being submitted as copyright ARM with author @elhajmahmoud.com and not @arm.com?

>  #  Copyright (c) 2017, Andrei Warkentin <andrey.warkentin@gmail.com>
> #  Copyright (c) Microsoft Corporation. All rights reserved.
>  #
> @@ -42,6 +43,7 @@ [Protocols]
>
>  [Pcd]
>    gRaspberryPiTokenSpaceGuid.PcdSdIsArasan
> +  gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress

When inserting new items like this, please do so alphabetically.

>
>  [Depex]
>    gRaspberryPiFirmwareProtocolGuid AND
> gRaspberryPiConfigAppliedProtocolGuid
> diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
> b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
> index 28fc2682b585..4f4fdef4e003 100644
> --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
> +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.inf
> @@ -1,5 +1,6 @@
>  #/** @file
>  #
> +#  Copyright (c) 2019, ARM Limited. All rights reserved.
>  #  Copyright (c) 2018, Andrei Warkentin <andrey.warkentin@gmail.com>
> #  #  SPDX-License-Identifier: BSD-2-Clause-Patent @@ -66,6 +67,7 @@
> [Pcd]
>    gRaspberryPiTokenSpaceGuid.PcdDebugShowUEFIExit
>    gRaspberryPiTokenSpaceGuid.PcdDisplayEnableScaledVModes
>    gRaspberryPiTokenSpaceGuid.PcdDisplayEnableSShot
> +  gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress

Also here.

>
>  [FeaturePcd]
>
> diff --git
> a/Platform/RaspberryPi/Drivers/DwUsbHostDxe/DwUsbHostDxe.inf
> b/Platform/RaspberryPi/Drivers/DwUsbHostDxe/DwUsbHostDxe.inf
> index e880c2fb0261..8817f20622d6 100644
> --- a/Platform/RaspberryPi/Drivers/DwUsbHostDxe/DwUsbHostDxe.inf
> +++ b/Platform/RaspberryPi/Drivers/DwUsbHostDxe/DwUsbHostDxe.inf
> @@ -1,5 +1,6 @@
>  #/** @file
>  #
> +#  Copyright (c) 2019, ARM Limited. All rights reserved.
>  #  Copyright (c) 2017-2018, Andrei Warkentin
> <andrey.warkentin@gmail.com>  #  Copyright (c) 2015-2016, Linaro Limited. All rights reserved.
>  #
> @@ -51,5 +52,8 @@ [Protocols]
>    gEfiUsb2HcProtocolGuid
>    gRaspberryPiFirmwareProtocolGuid
>
> +[FixedPcd]
> +  gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress
> +
>  [Depex]
>    gRaspberryPiFirmwareProtocolGuid
> diff --git
> a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.inf
> b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.inf
> index 87bca98fec28..a3fc0fa49a3c 100644
> --- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.inf
> +++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.inf
> @@ -1,5 +1,5 @@
>  #/** @file
> -#
> +#  Copyright (c) 2019, ARM Limited. All rights reserved.
>  #  Copyright (c) 2017-2018, Andrei Warkentin
> <andrey.warkentin@gmail.com>  #  Copyright (c) 2016, Linaro, Ltd. All rights reserved.
>  #
> @@ -40,5 +40,8 @@ [LibraryClasses]
>  [Protocols]
>    gRaspberryPiFirmwareProtocolGuid    ## PRODUCES
>
> +[FixedPcd]
> +  gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress
> +
>  [Depex]
>    TRUE
> diff --git a/Platform/RaspberryPi/Drivers/SdHostDxe/SdHostDxe.inf
> b/Platform/RaspberryPi/Drivers/SdHostDxe/SdHostDxe.inf
> index 7386ff251864..b99f197bb007 100644
> --- a/Platform/RaspberryPi/Drivers/SdHostDxe/SdHostDxe.inf
> +++ b/Platform/RaspberryPi/Drivers/SdHostDxe/SdHostDxe.inf
> @@ -1,5 +1,6 @@
>  #/** @file
>  #
> +#  Copyright (c) 2019, ARM Limited. All rights reserved.
>  #  Copyright (c) 2017, Andrei Warkentin <andrey.warkentin@gmail.com>
> #  Copyright (c) Microsoft Corporation. All rights reserved.
>  #
> @@ -44,6 +45,7 @@ [Protocols]
>
>  [Pcd]
>    gRaspberryPiTokenSpaceGuid.PcdSdIsArasan
> +  gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress

Also here.

>
>  [Depex]
>    gRaspberryPiFirmwareProtocolGuid AND
> gRaspberryPiConfigAppliedProtocolGuid
> diff --git a/Platform/RaspberryPi/Library/PlatformLib/PlatformLib.inf
> b/Platform/RaspberryPi/Library/PlatformLib/PlatformLib.inf
> index ed986034b957..85462febdd8d 100644
> --- a/Platform/RaspberryPi/Library/PlatformLib/PlatformLib.inf
> +++ b/Platform/RaspberryPi/Library/PlatformLib/PlatformLib.inf
> @@ -2,7 +2,7 @@
>  #
>  #  Copyright (c) 2017-2018, Andrei Warkentin
> <andrey.warkentin@gmail.com>  #  Copyright (c) 2014-2016, Linaro Limited. All rights reserved.
> -#  Copyright (c) 2011-2014, ARM Limited. All rights reserved.
> +#  Copyright (c) 2011-2019, ARM Limited. All rights reserved.
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent  # @@ -54,6 +54,7 @@
> [FixedPcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
> +  gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress

Also here.

>
>  [Ppis]
>    gArmMpCoreInfoPpiGuid
> diff --git a/Platform/RaspberryPi/RPi3/RPi3.dsc b/Platform/RaspberryPi/RPi3/RPi3.dsc
> index a0365c5cf606..4e5a9f0b05e6 100644
> --- a/Platform/RaspberryPi/RPi3/RPi3.dsc
> +++ b/Platform/RaspberryPi/RPi3/RPi3.dsc
> @@ -1,6 +1,6 @@
>  # @file
>  #
> -#  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
> +#  Copyright (c) 2011 - 2019, ARM Limited. All rights reserved.
>  #  Copyright (c) 2014, Linaro Limited. All rights reserved.
>  #  Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.
>  #  Copyright (c) 2017 - 2018, Andrei Warkentin <andrey.warkentin@gmail.com>
> @@ -372,6 +372,10 @@ [PcdsFixedAtBuild.common]
>    gArmTokenSpaceGuid.PcdSystemMemoryBase|0x00400000
>    gArmTokenSpaceGuid.PcdSystemMemorySize|0x3FC00000
>
> +  #
> +  # Device specific addresses
> +  #
> +  gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress|0x3f000000
>    ## NS16550 compatible UART
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterBase|0x3f215040
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio|TRUE
> diff --git a/Silicon/Broadcom/Bcm283x/Bcm283x.dec b/Silicon/Broadcom/Bcm283x/Bcm283x.dec
> index ec62ff27fbb3..5b839b00d286 100644
> --- a/Silicon/Broadcom/Bcm283x/Bcm283x.dec
> +++ b/Silicon/Broadcom/Bcm283x/Bcm283x.dec
> @@ -1,5 +1,6 @@
>  ## @file
>  #
> +#  Copyright (c) 2019, ARM Limited. All rights reserved.
>  #  Copyright (c) 2019, Pete Batard <pete@akeo.ie>
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -14,3 +15,9 @@ [Defines]
>
>  [Includes]
>    Include
> +
> +[Guids]
> +  gBcm283xTokenSpaceGuid = {0x82f36a92, 0xfb7e, 0x43a1, {0xb9, 0x9e, 0x49, 0x13, 0x3f, 0xc7, 0xa4, 0x2e}}
> +
> +[PcdsFixedAtBuild.common]
> +  gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress|0x0|UINT32|0x00000001
> diff --git a/Silicon/Broadcom/Bcm283x/Drivers/InterruptDxe/InterruptDxe.inf b/Silicon/Broadcom/Bcm283x/Drivers/InterruptDxe/InterruptDxe.inf
> index cdce11a51e14..6c58df5c3285 100644
> --- a/Silicon/Broadcom/Bcm283x/Drivers/InterruptDxe/InterruptDxe.inf
> +++ b/Silicon/Broadcom/Bcm283x/Drivers/InterruptDxe/InterruptDxe.inf
> @@ -1,5 +1,5 @@
>  #/** @file
> -#
> +#  Copyright (c) 2019, ARM Limited. All rights reserved.
>  #  Copyright (c) 2017, Andrei Warkentin <andrey.warkentin@gmail.com>
>  #  Copyright (c) 2016 Linaro, Ltd. All rights reserved.
>  #
> @@ -30,6 +30,7 @@ [LibraryClasses]
>    UefiBootServicesTableLib
>    UefiLib
>    UefiDriverEntryPoint
> +  PcdLib

Also here.

>
>  [Protocols]
>    gHardwareInterruptProtocolGuid  ## PRODUCES
> @@ -37,6 +38,7 @@ [Protocols]
>
>  [FixedPcd]
>    gEmbeddedTokenSpaceGuid.PcdInterruptBaseAddress
> +  gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress

Also here.

>
>  [Depex]
>    TRUE
> diff --git a/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.inf b/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.inf
> index cb1695bd2dfc..4481d71aaff0 100644
> --- a/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.inf
> +++ b/Silicon/Broadcom/Bcm283x/Drivers/RngDxe/RngDxe.inf
> @@ -1,5 +1,6 @@
>  #/** @file
>  #
> +#  Copyright (c) 2019, ARM Limited. All rights reserved.
>  #  Copyright (c) 2019 Linaro, Ltd. All rights reserved.
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -28,6 +29,7 @@ [LibraryClasses]
>    IoLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
> +  PcdLib

Also here.

/
    Leif

>
>  [Protocols]
>    gEfiRngProtocolGuid              ## PRODUCES
> @@ -35,5 +37,8 @@ [Protocols]
>  [Guids]
>    gEfiRngAlgorithmRaw
>
> +[FixedPcd]
> +  gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress
> +
>  [Depex]
>    TRUE
> diff --git a/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h b/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h
> index 4007301228be..8bd68c234bfd 100644
> --- a/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h
> +++ b/Silicon/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2836.h
> @@ -1,5 +1,6 @@
>  /** @file
>   *
> + *  Copyright (c) 2019, ARM Limited. All rights reserved.
>   *  Copyright (c) 2017, Andrei Warkentin <andrey.warkentin@gmail.com>
>   *  Copyright (c) 2016, Linaro Limited. All rights reserved.
>   *
> @@ -13,7 +14,7 @@
>  /*
>   * Both "core" and SoC perpherals (1M each).
>   */
> -#define BCM2836_SOC_REGISTERS                               0x3f000000
> +#define BCM2836_SOC_REGISTERS                               (FixedPcdGet64 (PcdBcm283xRegistersAddress))
>  #define BCM2836_SOC_REGISTER_LENGTH                         0x02000000
>
>  /*
> diff --git a/Silicon/Broadcom/Bcm283x/Library/GpioLib/GpioLib.inf b/Silicon/Broadcom/Bcm283x/Library/GpioLib/GpioLib.inf
> index 50da4eb771f3..ff1b5af6db6e 100644
> --- a/Silicon/Broadcom/Bcm283x/Library/GpioLib/GpioLib.inf
> +++ b/Silicon/Broadcom/Bcm283x/Library/GpioLib/GpioLib.inf
> @@ -2,6 +2,7 @@
>  #
>  #  Manipulate GPIOs.
>  #
> +#  Copyright (c) 2019, ARM Limited. All rights reserved.
>  #  Copyright (c) 2018, Andrei Warkentin <andrey.warkentin@gmail.com>
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -30,4 +31,7 @@ [LibraryClasses]
>    DebugLib
>    IoLib
>
> +[FixedPcd]
> +  gBcm283xTokenSpaceGuid.PcdBcm283xRegistersAddress
> +
>  [Guids]
> --
> 2.21.0.windows.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.