Re: [PATCH edk2-platforms v2 18/18] ARM/JunoPkg: Add HDLCD platform library

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 22 December 2017 at 19:08,  <evan.lloyd@arm.com> wrote:
> From: Girish Pathak <girish.pathak@arm.com>
>
> This change adds the HDLCD platform lib for the Juno plaform. This
> library will be instantiated as a LcdPlatformLib to link with
> LcdGraphicsOutputDxe for the Juno platform.
>
> HDLCD platform library depends on the Arm SCMI DXE driver for
> communication with the SCP for clock setting. Therefore this change also
> enables building of Arm SCMI DXE driver for the Juno platform.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Girish Pathak <girish.pathak@arm.com>

Missing signoff?

> ---
>  Platform/ARM/JunoPkg/ArmJuno.dec                                 |   8 +
>  Platform/ARM/JunoPkg/ArmJuno.dsc                                 |  29 +
>  Platform/ARM/JunoPkg/ArmJuno.fdf                                 |  12 +-
>  Platform/ARM/JunoPkg/Library/ArmJunoLib/ArmJunoLib.inf           |   5 +-
>  Platform/ARM/JunoPkg/Library/HdLcdArmJunoLib/HdLcdArmJunoLib.inf |  40 ++
>  Platform/ARM/JunoPkg/Library/ArmJunoLib/ArmJunoMem.c             |  18 +-
>  Platform/ARM/JunoPkg/Library/HdLcdArmJunoLib/HdLcdArmJuno.c      | 559 ++++++++++++++++++++
>  7 files changed, 668 insertions(+), 3 deletions(-)
>
> diff --git a/Platform/ARM/JunoPkg/ArmJuno.dec b/Platform/ARM/JunoPkg/ArmJuno.dec
> index b733480c3198d135df16ca024b5e85ff350e11c7..cd6710feb2faf0bd17b5ea39a21dbe5406cd4ffd 100644
> --- a/Platform/ARM/JunoPkg/ArmJuno.dec
> +++ b/Platform/ARM/JunoPkg/ArmJuno.dec
> @@ -53,3 +53,11 @@ [PcdsFixedAtBuild.common]
>    gArmJunoTokenSpaceGuid.PcdArmMtlMailBoxBase|0x2E000000|UINT64|0x00000025
>    gArmJunoTokenSpaceGuid.PcdArmMtlMailBoxSize|0x80|UINT32|0x00000026
>
> +  # MaxMode must be one number higher than the actual max mode,
> +  # i.e. for actual maximum mode 2, set the value to 3.
> +  #
> +  # Default value zero allows platform to enumerate maximum supported mode.
> +  #
> +  # For a list of mode numbers look in HdLcdArmJuno.c
> +  gArmJunoTokenSpaceGuid.PcdArmHdLcdMaxMode|0|UINT32|0x00000017
> +
> diff --git a/Platform/ARM/JunoPkg/ArmJuno.dsc b/Platform/ARM/JunoPkg/ArmJuno.dsc
> index fe860956a4dc497cac52be70bab3657246a08bd0..9027c5b0728a6941f850636b3bc315fd33b867fb 100644
> --- a/Platform/ARM/JunoPkg/ArmJuno.dsc
> +++ b/Platform/ARM/JunoPkg/ArmJuno.dsc
> @@ -50,6 +50,11 @@ [LibraryClasses.common]
>    # SCMI Mailbox Transport Layer
>    ArmMtl|Platform/ARM/JunoPkg/Library/ArmMtl/ArmMtl.inf
>
> +!ifndef HEADLESS_PLATFORM

Wouldn't it make more sense to add a macro ENABLE_HDLCD, rather than
inverting the logic?

> +  LcdPlatformLib|Platform/ARM/JunoPkg/Library/HdLcdArmJunoLib/HdLcdArmJunoLib.inf
> +  LcdHwLib|ArmPlatformPkg/Library/HdLcd/HdLcd.inf
> +!endif
> +
>  [LibraryClasses.common.SEC]
>    PrePiLib|EmbeddedPkg/Library/PrePiLib/PrePiLib.inf
>    ExtractGuidedSectionLib|EmbeddedPkg/Library/PrePiExtractGuidedSectionLib/PrePiExtractGuidedSectionLib.inf
> @@ -100,7 +105,15 @@ [PcdsFixedAtBuild.common]
>
>    # System Memory (2GB - 16MB of Trusted DRAM at the top of the 32bit address space)
>    gArmTokenSpaceGuid.PcdSystemMemoryBase|0x80000000
> +
> +!ifdef HEADLESS_PLATFORM
>    gArmTokenSpaceGuid.PcdSystemMemorySize|0x7F000000
> +!else
> +  gArmTokenSpaceGuid.PcdSystemMemorySize|0x7B000000
> +  gArmPlatformTokenSpaceGuid.PcdArmLcdDdrFrameBufferBase|0xFB000000
> +  gArmPlatformTokenSpaceGuid.PcdArmLcdDdrFrameBufferSize|0x04000000
> +  gArmPlatformTokenSpaceGuid.PcdArmHdLcdSwapBlueRedSelect|TRUE
> +!endif
>
>    # Juno Dual-Cluster profile
>    gArmPlatformTokenSpaceGuid.PcdCoreCount|6
> @@ -142,6 +155,11 @@ [PcdsFixedAtBuild.common]
>    gArmTokenSpaceGuid.PcdGicDistributorBase|0x2C010000
>    gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0x2C02F000
>
> +!ifndef HEADLESS_PLATFORM
> +  # ARM Juno HDLCD Base
> +  gArmPlatformTokenSpaceGuid.PcdArmHdLcdBase|0x7FF60000
> +!endif
> +
>    #
>    # PLDA PCI Root Complex
>    #
> @@ -314,6 +332,11 @@ [Components.common]
>    MdeModulePkg/Bus/Usb/UsbMassStorageDxe/UsbMassStorageDxe.inf
>    MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf
>
> +!ifndef HEADLESS_PLATFORM
> +  # Graphic Output Protocol
> +  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe.inf
> +!endif
> +
>    #
>    # Juno platform driver
>    #
> @@ -347,6 +370,12 @@ [Components.common]
>        BdsLib|Platform/ARM/Library/BdsLib/BdsLib.inf
>    }
>
> +  # SCMI Driver
> +  ArmPlatformPkg/Drivers/ArmScmiDxe/ArmScmiDxe.inf {
> +    <LibraryClasses>
> +      BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf

I take it your trusted SRAM does not tolerate unaligned memcpy()
because it is mapped as device memory. Couldn't you map it as
non-cacheable memory instead? (I meant to ask in response to the other
patch but I forgot)


> +  }
> +
>  [Components.AARCH64]
>    #
>    # EBC
> diff --git a/Platform/ARM/JunoPkg/ArmJuno.fdf b/Platform/ARM/JunoPkg/ArmJuno.fdf
> index ee9d0e7f4f6e6ac99ded6a14e88eb2c7854dd473..0b62760cbb3ff93490204ac636b41d5a867dfb80 100644
> --- a/Platform/ARM/JunoPkg/ArmJuno.fdf
> +++ b/Platform/ARM/JunoPkg/ArmJuno.fdf
> @@ -1,5 +1,5 @@
>  #
> -#  Copyright (c) 2013-2015, ARM Limited. All rights reserved.
> +#  Copyright (c) 2013-2017, ARM Limited. All rights reserved.
>  #
>  #  This program and the accompanying materials
>  #  are licensed and made available under the terms and conditions of the BSD License
> @@ -163,6 +163,13 @@ [FV.FvMain]
>    INF MdeModulePkg/Bus/Usb/UsbMouseDxe/UsbMouseDxe.inf
>    INF MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf
>
> +!ifndef HEADLESS_PLATFORM
> +  #
> +  # Graphics Output Protocol
> +  #
> +  INF ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe.inf
> +!endif
> +
>    #
>    # PCI Support
>    #
> @@ -223,6 +230,9 @@ [FV.FvMain]
>    # after the device drivers (eg: Ethernet) to ensure we have support for them.
>    INF Platform/ARM/Drivers/FdtPlatformDxe/FdtPlatformDxe.inf
>
> +  # SCMI Driver
> +  INF ArmPlatformPkg/Drivers/ArmScmiDxe/ArmScmiDxe.inf
> +
>  !if $(ARCH) == AARCH64
>    #
>    # EBC
> diff --git a/Platform/ARM/JunoPkg/Library/ArmJunoLib/ArmJunoLib.inf b/Platform/ARM/JunoPkg/Library/ArmJunoLib/ArmJunoLib.inf
> index 2dd384daba3d6076ba4898a0251ebc91bc0beee2..f131035be684f22e9f4c00417759b7845b29dcc6 100644
> --- a/Platform/ARM/JunoPkg/Library/ArmJunoLib/ArmJunoLib.inf
> +++ b/Platform/ARM/JunoPkg/Library/ArmJunoLib/ArmJunoLib.inf
> @@ -1,5 +1,5 @@
>  #
> -#  Copyright (c) 2013-2016, ARM Limited. All rights reserved.
> +#  Copyright (c) 2013-2017, ARM Limited. All rights reserved.
>  #
>  #  This program and the accompanying materials
>  #  are licensed and made available under the terms and conditions of the BSD License
> @@ -57,6 +57,9 @@ [FixedPcd]
>    gArmJunoTokenSpaceGuid.PcdPciConfigurationSpaceBaseAddress
>    gArmJunoTokenSpaceGuid.PcdPciConfigurationSpaceSize
>
> +  # Frame Buffer Memory
> +  gArmPlatformTokenSpaceGuid.PcdArmLcdDdrFrameBufferBase
> +  gArmPlatformTokenSpaceGuid.PcdArmLcdDdrFrameBufferSize
>
>    #
>    # PL011 Serial Debug UART
> diff --git a/Platform/ARM/JunoPkg/Library/HdLcdArmJunoLib/HdLcdArmJunoLib.inf b/Platform/ARM/JunoPkg/Library/HdLcdArmJunoLib/HdLcdArmJunoLib.inf
> new file mode 100644
> index 0000000000000000000000000000000000000000..a10bff135abf154484d36c67083b498846f24753
> --- /dev/null
> +++ b/Platform/ARM/JunoPkg/Library/HdLcdArmJunoLib/HdLcdArmJunoLib.inf
> @@ -0,0 +1,40 @@
> +#/** @file
> +#
> +#  Component description file for HdLcdArmJunoLib module
> +#
> +#  Copyright (c) 2013-2017, ARM Ltd. All rights reserved.<BR>
> +#
> +#  This program and the accompanying materials
> +#  are licensed and made available under the terms and conditions of the BSD License
> +#  which accompanies this distribution.  The full text of the license may be found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +#
> +#**/
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010019
> +  BASE_NAME                      = HdLcdArmJunoLib
> +  FILE_GUID                      = 7B1D26F7-7B88-47ED-B193-DD3BDF319006
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = LcdPlatformLib
> +
> +[Sources.common]
> +  HdLcdArmJuno.c
> +
> +[Packages]
> +  ArmPlatformPkg/ArmPlatformPkg.dec
> +  MdePkg/MdePkg.dec
> +  Platform/ARM/JunoPkg/ArmJuno.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +
> +[FixedPcd]
> +  gArmPlatformTokenSpaceGuid.PcdArmLcdDdrFrameBufferBase
> +  gArmPlatformTokenSpaceGuid.PcdArmLcdDdrFrameBufferSize
> +  gArmJunoTokenSpaceGuid.PcdArmHdLcdMaxMode
> +  gArmPlatformTokenSpaceGuid.PcdGopPixelFormat
> diff --git a/Platform/ARM/JunoPkg/Library/ArmJunoLib/ArmJunoMem.c b/Platform/ARM/JunoPkg/Library/ArmJunoLib/ArmJunoMem.c
> index afb2db0050c65b0d1b2b69c9038e168755c152c1..baa5221cb906ed5d077414475da006cf2e5cafc5 100644
> --- a/Platform/ARM/JunoPkg/Library/ArmJunoLib/ArmJunoMem.c
> +++ b/Platform/ARM/JunoPkg/Library/ArmJunoLib/ArmJunoMem.c
> @@ -21,8 +21,10 @@
>
>  #include <ArmPlatform.h>
>
> +#define FRAME_BUFFER_DESCRIPTOR ((FixedPcdGet32 (PcdArmLcdDdrFrameBufferSize) != 0) ? 1 : 0)
> +
>  // The total number of descriptors, including the final "end-of-table" descriptor.
> -#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 16
> +#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS (16 + FRAME_BUFFER_DESCRIPTOR)
>
>  // DDR attributes
>  #define DDR_ATTRIBUTES_CACHED           ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK
> @@ -151,6 +153,20 @@ ArmPlatformGetVirtualMemoryMap (
>    VirtualMemoryTable[Index].Length          = ARM_JUNO_SOC_PERIPHERALS_SZ;
>    VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
>
> +  // Frame Buffer Memory
> +#if (FixedPcdGet32 (PcdArmLcdDdrFrameBufferSize) != 0)

Please use a normal if()

> +  VirtualMemoryTable[++Index].PhysicalBase  = FixedPcdGet64 (PcdArmLcdDdrFrameBufferBase);
> +  VirtualMemoryTable[Index].VirtualBase     = FixedPcdGet64 (PcdArmLcdDdrFrameBufferBase);
> +  VirtualMemoryTable[Index].Length          = FixedPcdGet32 (PcdArmLcdDdrFrameBufferSize);
> +  // Map as Normal Non-Cacheable memory, so that we can use the accelerated
> +  // SetMem/CopyMem routines that may use unaligned accesses or
> +  // DC ZVA instructions. If mapped as device memory, these routine may cause
> +  // alignment faults.
> +  // NOTE: The attribute value is misleading, it indicates memory map type as
> +  // an un-cached, un-buffered but allows buffering and reordering.
> +  VirtualMemoryTable[Index].Attributes      = ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
> +#endif
> +
>    // DDR - 2GB
>    VirtualMemoryTable[++Index].PhysicalBase  = PcdGet64 (PcdSystemMemoryBase);
>    VirtualMemoryTable[Index].VirtualBase     = PcdGet64 (PcdSystemMemoryBase);
> diff --git a/Platform/ARM/JunoPkg/Library/HdLcdArmJunoLib/HdLcdArmJuno.c b/Platform/ARM/JunoPkg/Library/HdLcdArmJunoLib/HdLcdArmJuno.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..72be0a39846fb0a78ebcf3248b6c51377adf4f73
> --- /dev/null
> +++ b/Platform/ARM/JunoPkg/Library/HdLcdArmJunoLib/HdLcdArmJuno.c
> @@ -0,0 +1,559 @@
> +/** @file
> +
> +  Copyright (c) 2013-2017, ARM Ltd. All rights reserved.
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD License
> +  which accompanies this distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include <PiDxe.h>
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/DxeServicesTableLib.h>
> +#include <Library/LcdPlatformLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Drivers/ArmScmi.h>
> +#include <Drivers/ArmScmiClockProtocol.h>
> +
> +/* Display timings on Juno for 1920x1080.
> +  On Juno due to instability of the PLLs, we set OSC
> +  frequency to 138.5 MHz which is stable for most monitors.
> +  Frequency 148.5MHz does not work with some monitors.
> +  148.5 MHz is set by SCP firmware by default.
> +
> +#define JUNO_HD_OSC_FREQUENCY               148500000
> +*/
> +#define JUNO_HD_OSC_FREQUENCY               138500000
> +#define JUNO_HD_H_SYNC                      ( 32 - 1)
> +#define JUNO_HD_H_FRONT_PORCH               ( 48 - 1)
> +#define JUNO_HD_H_BACK_PORCH                ( 80 - 1)
> +#define JUNO_HD_V_SYNC                      (  5 - 1)
> +#define JUNO_HD_V_FRONT_PORCH               (  3 - 1)
> +#define JUNO_HD_V_BACK_PORCH                ( 23 - 1)
> +
> +/* SCMI defined clock device name and ID. This is not documented but
> +   obtained using clock management protocol's CLOCK_ATTRIBUTES command.
> +
> +   Generally we must discover clock device ID using clock name and then
> +   set/get rate using CLOCK_RATE_SET/CLOCK_RATE_GET commands. However
> +   because LcdGraphicsOutputDxe is a DXE driver, which gets initialized
> +   at boot time, for faster boot, in release build we will directly use
> +   this already known value as an argument to rate get/set functions.
> +
> +   We expect these values not to change in future SCP firmware releases.
> +
> +   DEBUG build however will probe SCP firmware and discover clock device
> +   ID for HDLCD.
> +*/
> +#define ARM_JUNO_CSS_CLK_NAME_HDLCD_0       "HDLCD_0"
> +#define ARM_JUNO_CSS_CLKID_HDLCD_0           3
> +
> +typedef struct {
> +  UINT32                      Mode;
> +  UINT32                      OscFreq;
> +  SCAN_TIMINGS                Horizontal;
> +  SCAN_TIMINGS                Vertical;
> +} DISPLAY_MODE;
> +
> +STATIC CONST DISPLAY_MODE mDisplayModes[] = {
> +  {
> +    // Mode 0 : VGA : 640 x 480 x 24 bpp.
> +    VGA,
> +    VGA_OSC_FREQUENCY,
> +    {VGA_H_RES_PIXELS, VGA_H_SYNC, VGA_H_BACK_PORCH, VGA_H_FRONT_PORCH},
> +    {VGA_V_RES_PIXELS, VGA_V_SYNC, VGA_V_BACK_PORCH, VGA_V_FRONT_PORCH}
> +  },
> +  {
> +    // Mode 1 : WVGA : 800 x 480 x 24 bpp.
> +    WVGA,
> +    WVGA_OSC_FREQUENCY,
> +    {WVGA_H_RES_PIXELS, WVGA_H_SYNC, WVGA_H_BACK_PORCH, WVGA_H_FRONT_PORCH},
> +    {WVGA_V_RES_PIXELS, WVGA_V_SYNC, WVGA_V_BACK_PORCH, WVGA_V_FRONT_PORCH}
> +  },
> +  {
> +    // Mode 2 : SVGA : 800 x 600 x 24 bpp.
> +    SVGA,
> +    SVGA_OSC_FREQUENCY,
> +    {SVGA_H_RES_PIXELS, SVGA_H_SYNC, SVGA_H_BACK_PORCH, SVGA_H_FRONT_PORCH},
> +    {SVGA_V_RES_PIXELS, SVGA_V_SYNC, SVGA_V_BACK_PORCH, SVGA_V_FRONT_PORCH}
> +  },
> +  {
> +    // Mode 3 : QHD : 960 x 540 x 24 bpp.
> +    QHD,
> +    QHD_OSC_FREQUENCY,
> +    {QHD_H_RES_PIXELS, QHD_H_SYNC, QHD_H_BACK_PORCH, QHD_H_FRONT_PORCH},
> +    {QHD_V_RES_PIXELS, QHD_V_SYNC, QHD_V_BACK_PORCH, QHD_V_FRONT_PORCH}
> +  },
> +  {
> +    // Mode 4 : WSVGA : 1024 x 600 x 24 bpp.
> +    WSVGA,
> +    WSVGA_OSC_FREQUENCY,
> +    {WSVGA_H_RES_PIXELS, WSVGA_H_SYNC, WSVGA_H_BACK_PORCH, WSVGA_H_FRONT_PORCH},
> +    {WSVGA_V_RES_PIXELS, WSVGA_V_SYNC, WSVGA_V_BACK_PORCH, WSVGA_V_FRONT_PORCH}
> +  },
> +  {
> +    // Mode 5 : XGA : 1024 x 768 x 24 bpp.
> +    XGA,
> +    XGA_OSC_FREQUENCY,
> +    {XGA_H_RES_PIXELS, XGA_H_SYNC, XGA_H_BACK_PORCH, XGA_H_FRONT_PORCH},
> +    {XGA_V_RES_PIXELS, XGA_V_SYNC, XGA_V_BACK_PORCH, XGA_V_FRONT_PORCH}
> +  },
> +  {
> +    // Mode 6 : HD : 1280 x 720 x 24 bpp.
> +    HD720,
> +    HD720_OSC_FREQUENCY,
> +    {HD720_H_RES_PIXELS, HD720_H_SYNC, HD720_H_BACK_PORCH, HD720_H_FRONT_PORCH},
> +    {HD720_V_RES_PIXELS, HD720_V_SYNC, HD720_V_BACK_PORCH, HD720_V_FRONT_PORCH}
> +  },
> +  {
> +    // Mode 7 : WXGA : 1280 x 800 x 24 bpp.
> +    WXGA,
> +    WXGA_OSC_FREQUENCY,
> +    {WXGA_H_RES_PIXELS, WXGA_H_SYNC, WXGA_H_BACK_PORCH, WXGA_H_FRONT_PORCH},
> +    {WXGA_V_RES_PIXELS, WXGA_V_SYNC, WXGA_V_BACK_PORCH, WXGA_V_FRONT_PORCH}
> +  },
> +  {
> +    // Mode 8 : SXGA : 1280 x 1024 x 24 bpp.
> +    SXGA,
> +    SXGA_OSC_FREQUENCY,
> +    {SXGA_H_RES_PIXELS, SXGA_H_SYNC, SXGA_H_BACK_PORCH, SXGA_H_FRONT_PORCH},
> +    {SXGA_V_RES_PIXELS, SXGA_V_SYNC, SXGA_V_BACK_PORCH, SXGA_V_FRONT_PORCH}
> +  },
> +  {
> +    // Mode 9 : WSXGA+ : 1680 x 1050 x 24 bpp.
> +    WSXGA,
> +    WSXGA_OSC_FREQUENCY,
> +    {WSXGA_H_RES_PIXELS, WSXGA_H_SYNC, WSXGA_H_BACK_PORCH, WSXGA_H_FRONT_PORCH},
> +    {WSXGA_V_RES_PIXELS, WSXGA_V_SYNC, WSXGA_V_BACK_PORCH, WSXGA_V_FRONT_PORCH}
> +  },
> +  {
> +    // Mode 10 : HD : 1920 x 1080 x 24 bpp.
> +    HD,
> +    JUNO_HD_OSC_FREQUENCY,
> +    {HD_H_RES_PIXELS, JUNO_HD_H_SYNC, JUNO_HD_H_BACK_PORCH, JUNO_HD_H_FRONT_PORCH},
> +    {HD_V_RES_PIXELS, JUNO_HD_V_SYNC, JUNO_HD_V_BACK_PORCH, JUNO_HD_V_FRONT_PORCH}
> +  }
> +};
> +
> +/* If PcdArmMaliDpMaxMode is 0, platform supports full range of modes
> +   else platform supports modes from 0 to PcdArmHdLcdMaxMode - 1
> +*/
> +STATIC CONST UINT32 mMaxMode = ((FixedPcdGet32 (PcdArmHdLcdMaxMode) != 0)
> +                                   ? FixedPcdGet32 (PcdArmHdLcdMaxMode)
> +                                   : sizeof (mDisplayModes) / sizeof (DISPLAY_MODE));
> +
> +/** HDLCD platform specific initialization function.
> +
> +  @param[in] Handle              Handle to the LCD device instance.
> +
> +  @retval EFI_SUCCESS            Plaform library initialized successfully.
> +  @retval EFI_UNSUPPORTED        PcdGopPixelFormat must be
> +                                 PixelRedGreenBlueReserved8BitPerColor OR
> +                                 PixelBlueGreenRedReserved8BitPerColor
> +                                 any other format is not supported.
> +  @retval !(EFI_SUCCESS)         Other errors.
> +**/
> +EFI_STATUS
> +LcdPlatformInitializeDisplay (
> +  IN CONST EFI_HANDLE   Handle
> +  )
> +{
> +  (VOID)Handle;
> +  EFI_GRAPHICS_PIXEL_FORMAT PixelFormat;
> +
> +  // PixelBitMask and PixelBltOnly pixel formats are not supported.
> +  PixelFormat = FixedPcdGet32 (PcdGopPixelFormat);
> +  if (PixelFormat != PixelRedGreenBlueReserved8BitPerColor
> +    && PixelFormat != PixelBlueGreenRedReserved8BitPerColor) {
> +
> +    ASSERT (PixelFormat == PixelRedGreenBlueReserved8BitPerColor
> +      ||  PixelFormat == PixelBlueGreenRedReserved8BitPerColor);

Please fix weird indentation

> +   return EFI_UNSUPPORTED;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/** Allocate VRAM memory in DRAM for the frame buffer
> +  (unless it is reserved already).
> +
> +  The allocated address can be used to set the frame buffer.
> +
> +  @param[out] VramBaseAddress     A pointer to the frame buffer address.
> +  @param[out] VramSize            A pointer to the size of the frame
> +                                  buffer in bytes
> +
> +  @retval EFI_SUCCESS             Frame buffer memory allocated successfully.
> +  @retval !(EFI_SUCCESS)          Other errors.
> +**/
> +EFI_STATUS
> +LcdPlatformGetVram (
> +  OUT EFI_PHYSICAL_ADDRESS * CONST VramBaseAddress,
> +  OUT UINTN                * CONST VramSize
> +  )
> +{
> +  EFI_STATUS      Status = EFI_SUCCESS;
> +
> +  ASSERT (VramBaseAddress != NULL);
> +  ASSERT (VramSize != NULL);
> +
> +  // Set the VRAM size.
> +  *VramSize = (UINTN)FixedPcdGet32 (PcdArmLcdDdrFrameBufferSize);
> +
> +  // Check if memory is already reserved for the frame buffer.
> +#if (FixedPcdGet64 (PcdArmLcdDdrFrameBufferBase) != 0)

Please don't use CPP conditionals for control flow

> +  *VramBaseAddress =
> +     (EFI_PHYSICAL_ADDRESS)FixedPcdGet64 (PcdArmLcdDdrFrameBufferBase);
> +#else
> +  // If not already reserved, attempt to allocate the VRAM from the DRAM.
> +  Status = gBS->AllocatePages (
> +                  AllocateAnyPages,
> +                  EfiBootServicesData,
> +                  EFI_SIZE_TO_PAGES (*VramSize),
> +                  VramBaseAddress
> +                  );
> +
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "HdLcdArmJuno: Failed to allocate frame buffer.\n"));
> +    ASSERT_EFI_ERROR (Status);
> +    return Status;
> +  }
> +
> +  /* Mark the VRAM as write-combining.
> +     The VRAM is inside the DRAM, which is cacheable.
> +  */
> +  Status = gDS->SetMemorySpaceAttributes (
> +                  *VramBaseAddress,
> +                  *VramSize,
> +                  EFI_MEMORY_WC
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize));
> +  }
> +#endif
> +
> +  return Status;
> +}
> +
> +/** Return total number of modes supported.
> +
> +  Note: Valid mode numbers are 0 to MaxMode - 1
> +  See Section 12.9 of the UEFI Specification 2.7
> +
> +  @retval UINT32             Mode Number.
> +**/
> +UINT32
> +LcdPlatformGetMaxMode (VOID)
> +{
> +  return  mMaxMode;
> +}
> +
> +#if !defined(MDEPKG_NDEBUG)
> +/** Probe Clock device ID of the HDLCD clock and current pixel clock frequency.
> +  NOTE: We will probe information only in DEBUG build.
> +
> +  @param[in]  ClockProtocol   A pointer to SCMI clock protocol
> +                              interface instance.
> +  @param[out] ClockId         ID of the clock device
> +
> +  @retval EFI_SUCCESS         Clock ID of the HDLCD device returned
> +                              successfully.
> +  @retval EFI_UNSUPPORTED     SCMI clock management protocol unsupported.
> +  @retval EFI_DEVICE_ERROR    SCMI error.
> +  @retval EFI_NOT_FOUND       Not found valid clock device ID of the HDLCD.
> +**/
> +STATIC
> +EFI_STATUS
> +ProbeHdLcdClock (
> +  IN  SCMI_CLOCK_PROTOCOL  *ClockProtocol,
> +  OUT UINT32               *ClockId
> +  )
> +{
> +  EFI_STATUS  Status;
> +  UINT64      CurrentHdLcdFreq;
> +
> +  UINT32      TotalClocks;
> +  UINT32      ClockProtocolVersion;
> +  BOOLEAN     Enabled;
> +  CHAR8       ClockName[SCMI_MAX_STR_LEN];
> +  BOOLEAN     ClockFound = FALSE;
> +
> +  UINT32                 TotalRates = 0;
> +  UINT32                 ClockRateSize;
> +  SCMI_CLOCK_RATE        ClockRate;
> +  SCMI_CLOCK_RATE_FORMAT ClockRateFormat;
> +
> +  Status = ClockProtocol->GetVersion (ClockProtocol, &ClockProtocolVersion);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT (FALSE);
> +    return Status;
> +  }
> +
> +  DEBUG ((DEBUG_ERROR, "SCMI clock management protocol version = %x\n",
> +    ClockProtocolVersion));
> +
> +  if (ClockProtocolVersion != SCMI_CLOCK_PROTOCOL_VERSION) {
> +    ASSERT (FALSE);
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  Status = ClockProtocol->GetTotalClocks (ClockProtocol, &TotalClocks);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  DEBUG ((DEBUG_ERROR, "Total number of clocks supported by SCMI clock management protocol = %d\n",
> +    TotalClocks));
> +
> +  for (*ClockId = 0; *ClockId < TotalClocks; (*ClockId)++) {
> +    Status = ClockProtocol->GetClockAttributes (
> +                              ClockProtocol,
> +                              *ClockId,
> +                              &Enabled,
> +                              ClockName
> +                              );
> +    if (EFI_ERROR (Status)) {
> +      // In current implementation of SCMI, some clocks are not accessible to
> +      // calling agents (in our case UEFI is an agent) which results in an
> +      // EFI_DEVICE_ERROR error. A bug fix for this is in discussions and will
> +      // be fixed in future versions of the SCP firmware. Irrespective of a fix
> +      // we must iterate over each clock to see if it matches with HDLCD.
> +      continue;
> +    }
> +
> +    if (AsciiStrnCmp ((CONST CHAR8*)ClockName,
> +          (CONST CHAR8*)ARM_JUNO_CSS_CLK_NAME_HDLCD_0,
> +          sizeof (ARM_JUNO_CSS_CLK_NAME_HDLCD_0)) == 0) {
> +      ClockFound = TRUE;
> +      break;
> +    }
> +  }
> +
> +  if (!ClockFound) {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  ClockRateSize = sizeof (ClockRate);
> +  Status = ClockProtocol->DescribeRates (
> +                            ClockProtocol,
> +                            *ClockId,
> +                            &ClockRateFormat,
> +                            &TotalRates,
> +                            &ClockRateSize,
> +                            &ClockRate
> +                            );
> +  if (EFI_ERROR (Status)) {
> +    ASSERT (FALSE);
> +    return Status;
> +  }
> +
> +  Status = ClockProtocol->RateGet (ClockProtocol, *ClockId, &CurrentHdLcdFreq);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT (FALSE);
> +    return Status;
> +  }
> +
> +  DEBUG ((DEBUG_ERROR, "Clock ID = %d Clock name = %a\n", *ClockId, ClockName));
> +  DEBUG ((DEBUG_ERROR, "Minimum frequency = %uHz\n", ClockRate.Min));
> +  DEBUG ((DEBUG_ERROR, "Maximum frequency = %uHz\n", ClockRate.Max));
> +  DEBUG ((DEBUG_ERROR, "Clock rate step = %uHz\n", ClockRate.Step));
> +
> +  DEBUG ((DEBUG_ERROR, "HDLCD Current frequency = %uHz\n", CurrentHdLcdFreq));
> +
> +  return EFI_SUCCESS;
> +}
> +#endif
> +
> +/** Set the requested display mode.
> +
> +  @param[in] ModeNumber             Mode Number.
> +
> +  @retval EFI_SUCCESS              Mode set successfully.
> +  @retval EFI_NOT_FOUND            Clock protocol instance not found.
> +  @retval EFI_DEVICE_ERROR         SCMI error.
> +  @retval EFI_INVALID_PARAMETER    Requested mode not found.
> +  @retval !(EFI_SUCCESS)            Other errors.
> +*/
> +EFI_STATUS
> +LcdPlatformSetMode (
> +  IN CONST UINT32  ModeNumber
> +  )
> +{
> +  EFI_STATUS          Status;
> +  SCMI_CLOCK_PROTOCOL *ClockProtocol;
> +  UINT32              ClockId;
> +
> +  EFI_GUID ClockProtocolGuid = ARM_SCMI_CLOCK_PROTOCOL_GUID;
> +
> +  if (ModeNumber >= mMaxMode) {
> +    ASSERT (ModeNumber < mMaxMode);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  // Display debug information in boot log.
> +  DEBUG ((DEBUG_ERROR, "HDLCD Display controller:\n"));
> +
> +  DEBUG ((DEBUG_ERROR, "Required frequency for resolution %dx%d = %uHz\n",
> +            mDisplayModes[ModeNumber].Horizontal.Resolution,
> +            mDisplayModes[ModeNumber].Vertical.Resolution,
> +            mDisplayModes[ModeNumber].OscFreq));
> +
> +  Status = gBS->LocateProtocol (
> +                  &ClockProtocolGuid,
> +                  NULL,
> +                  (VOID**)&ClockProtocol
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    ASSERT (FALSE);
> +    return Status;
> +  }
> +
> +#if !defined(MDEPKG_NDEBUG)
> +  /* Avoid probing clock device id in RELEASE build */
> +  Status = ProbeHdLcdClock (ClockProtocol, &ClockId);
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  ASSERT (ClockId == ARM_JUNO_CSS_CLKID_HDLCD_0);
> +#else
> +  ClockId = ARM_JUNO_CSS_CLKID_HDLCD_0;
> +#endif
> +
> +  // Set HDLCD clock required for the requested mode
> +  Status = ClockProtocol->RateSet (
> +                            ClockProtocol,
> +                            ClockId,
> +                            mDisplayModes[ModeNumber].OscFreq
> +                            );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "SCMI error: %r\n", Status));
> +    return Status;
> +  }
> +
> +#if !defined(MDEPKG_NDEBUG)
> +  UINT64  CurrentHdLcdFreq;
> +  // Actual value set can differ from requested frequency so verify.
> +  Status = ClockProtocol->RateGet (
> +                            ClockProtocol,
> +                            ARM_JUNO_CSS_CLKID_HDLCD_0,
> +                            &CurrentHdLcdFreq
> +                            );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "SCMI Error: %r\n", Status));
> +  } else {
> +    DEBUG ((DEBUG_ERROR, "Mode = %d, Requested frequency change = %uHz, Actual changed frequency = %uHz\n",
> +      ModeNumber,
> +      mDisplayModes[ModeNumber].OscFreq,
> +      CurrentHdLcdFreq
> +      ));
> +  }
> +#endif
> +
> +  return Status;
> +}
> +
> +/** Return information for the requested mode number.
> +
> +  @param[in]  ModeNumber          Mode Number.
> +
> +  @param[out] Info                Pointer for returned mode information
> +                                  (on success).
> +
> +  @retval EFI_SUCCESS             Mode information for the requested mode
> +                                  returned successfully.
> +  @retval EFI_INVALID_PARAMETER   Requested mode not found.
> +**/
> +EFI_STATUS
> +LcdPlatformQueryMode (
> +  IN  CONST UINT32                                  ModeNumber,
> +  OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST  Info
> +  )
> +{
> +  if (ModeNumber >= mMaxMode ){
> +    ASSERT (ModeNumber < mMaxMode);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  ASSERT (Info != NULL);
> +
> +  Info->Version = 0;
> +  Info->HorizontalResolution = mDisplayModes[ModeNumber].Horizontal.Resolution;
> +  Info->VerticalResolution = mDisplayModes[ModeNumber].Vertical.Resolution;
> +  Info->PixelsPerScanLine = mDisplayModes[ModeNumber].Horizontal.Resolution;
> +
> +  Info->PixelFormat = FixedPcdGet32 (PcdGopPixelFormat);
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/** Return display timing information for the requested mode number.
> +
> +  @param[in]  ModeNumber          Mode Number.
> +
> +  @param[out] Horizontal          Pointer to horizontal timing parameters.
> +                                  (Resolution, Sync, Back porch, Front porch)
> +  @param[out] Vertical            Pointer to vertical timing parameters.
> +                                  (Resolution, Sync, Back porch, Front porch)
> +
> +  @retval EFI_SUCCESS             Display timing information for the requested
> +                                  mode returned successfully.
> +  @retval EFI_INVALID_PARAMETER   Requested mode not found.
> +**/
> +EFI_STATUS
> +LcdPlatformGetTimings (
> +  IN  UINT32               ModeNumber,
> +  OUT CONST SCAN_TIMINGS   **Horizontal,
> +  OUT CONST SCAN_TIMINGS   **Vertical
> +  )
> +{
> +  if (ModeNumber >= mMaxMode ){
> +    ASSERT (ModeNumber < mMaxMode);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  ASSERT (Horizontal != NULL);
> +  ASSERT (Vertical != NULL);
> +
> +  *Horizontal = &mDisplayModes[ModeNumber].Horizontal;
> +  *Vertical   = &mDisplayModes[ModeNumber].Vertical;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/** Return bits per pixel information for a mode number.
> +
> +  @param[in]  ModeNumber          Mode Number.
> +
> +  @param[out] Bpp                 Pointer to value bits per pixel.
> +
> +  @retval EFI_SUCCESS             Bit per pixel information for the requested
> +                                  mode returned successfully.
> +  @retval EFI_INVALID_PARAMETER   Requested mode not found.
> +**/
> +EFI_STATUS
> +LcdPlatformGetBpp (
> +  IN  CONST UINT32         ModeNumber,
> +  OUT LCD_BPP      * CONST Bpp
> +  )
> +{
> +  if (ModeNumber >= mMaxMode) {
> +    // Check valid ModeNumber and Bpp.
> +    ASSERT (ModeNumber < mMaxMode);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  ASSERT (Bpp != NULL);
> +
> +  *Bpp = LCD_BITS_PER_PIXEL_24;
> +
> +  return EFI_SUCCESS;
> +}
> --
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>