From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=217.140.101.70; helo=foss.arm.com; envelope-from=vijayenthiran.subramaniam@arm.com; receiver=edk2-devel@lists.01.org Received: from foss.arm.com (foss.arm.com [217.140.101.70]) by ml01.01.org (Postfix) with ESMTP id 08BD421BADAB2 for ; Thu, 6 Dec 2018 22:14:19 -0800 (PST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 9E6BA15BE for ; Thu, 6 Dec 2018 22:14:19 -0800 (PST) Received: from mail-lf1-f52.google.com (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 377B93F5AF for ; Thu, 6 Dec 2018 22:14:19 -0800 (PST) Received: by mail-lf1-f52.google.com with SMTP id n18so2176981lfh.6 for ; Thu, 06 Dec 2018 22:14:19 -0800 (PST) X-Gm-Message-State: AA+aEWbaalxfOoglm8+ASatPoKVpaIlOqOphLc5NYuPlYRkswpoSQt/j O/UJrXOoXMJJI2GEYC8QI32RijsNA1QT48hQJQY= X-Google-Smtp-Source: AFSGD/Xv+I4Qh3S79riWw+E11xRHXLdxvzKNEqcAkgdD4NtabE5tZpJkw4eToEKaK8SiF2s3xgVFsF9PaXlVZ/KTUAQ= X-Received: by 2002:a19:1bca:: with SMTP id b193mr472963lfb.153.1544163257335; Thu, 06 Dec 2018 22:14:17 -0800 (PST) MIME-Version: 1.0 References: <1544100176-13374-1-git-send-email-vijayenthiran.subramaniam@arm.com> In-Reply-To: From: Vijayenthiran Subramaniam Date: Fri, 7 Dec 2018 06:13:38 +0000 X-Gmail-Original-Message-ID: Message-ID: To: Ard Biesheuvel Cc: edk2-devel@lists.01.org Subject: Re: [PATCH] Platform/ARM/SgiPkg: Add support for HDLCD X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 07 Dec 2018 06:14:20 -0000 Content-Type: text/plain; charset="UTF-8" On Thu, Dec 6, 2018 at 5:08 PM Ard Biesheuvel wrote: > > On Thu, 6 Dec 2018 at 13:43, Vijayenthiran Subramaniam > wrote: > > > > Add HDLCD platform library for SGI platform that implements platform > > callbacks for the Arm HDLCD driver. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Vijayenthiran Subramaniam > > --- > > Platform/ARM/SgiPkg/SgiPlatform.dsc | 6 + > > Platform/ARM/SgiPkg/SgiPlatform.fdf | 6 + > > Platform/ARM/SgiPkg/Library/HdLcdArmSgiLib/HdLcdArmSgiLib.inf | 37 +++ > > Platform/ARM/SgiPkg/Include/SgiPlatform.h | 4 + > > Platform/ARM/SgiPkg/Library/HdLcdArmSgiLib/HdLcdArmSgi.c | 262 ++++++++++++++++++++ > > Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c | 8 +- > > 6 files changed, 322 insertions(+), 1 deletion(-) > > > > diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc b/Platform/ARM/SgiPkg/SgiPlatform.dsc > > index 0c794c6b299d..7995c7d132d6 100644 > > --- a/Platform/ARM/SgiPkg/SgiPlatform.dsc > > +++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc > > @@ -37,6 +37,8 @@ [LibraryClasses.common] > > ArmPlatformLib|Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf > > BasePathLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf > > EfiResetSystemLib|ArmPkg/Library/ArmPsciResetSystemLib/ArmPsciResetSystemLib.inf > > + LcdHwLib|ArmPlatformPkg/Library/HdLcd/HdLcd.inf > > + LcdPlatformLib|Platform/ARM/SgiPkg/Library/HdLcdArmSgiLib/HdLcdArmSgiLib.inf > > NorFlashPlatformLib|Platform/ARM/SgiPkg/Library/NorFlashLib/NorFlashLib.inf > > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > > TimerLib|ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf > > @@ -155,6 +157,9 @@ [PcdsFixedAtBuild.common] > > gArmPlatformTokenSpaceGuid.PL011UartInteger|4 > > gArmPlatformTokenSpaceGuid.PL011UartFractional|0 > > > > + ## PL370 - HDLCD1 > > + gArmPlatformTokenSpaceGuid.PcdArmHdLcdBase|0x7FF60000 > > + > > ## PL011 - Serial Debug UART > > gArmPlatformTokenSpaceGuid.PcdSerialDbgRegisterBase|0x7FF80000 > > gArmPlatformTokenSpaceGuid.PcdSerialDbgUartClkInHz|7372800 > > @@ -235,6 +240,7 @@ [Components.common] > > ArmPkg/Drivers/ArmGic/ArmGicDxe.inf > > ArmPkg/Drivers/TimerDxe/TimerDxe.inf > > ArmPkg/Drivers/GenericWatchdogDxe/GenericWatchdogDxe.inf > > + ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe.inf > > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf > > EmbeddedPkg/RealTimeClockRuntimeDxe/RealTimeClockRuntimeDxe.inf > > EmbeddedPkg/MetronomeDxe/MetronomeDxe.inf > > diff --git a/Platform/ARM/SgiPkg/SgiPlatform.fdf b/Platform/ARM/SgiPkg/SgiPlatform.fdf > > index ddf1fda5a16e..80c3412fd4ad 100644 > > --- a/Platform/ARM/SgiPkg/SgiPlatform.fdf > > +++ b/Platform/ARM/SgiPkg/SgiPlatform.fdf > > @@ -133,6 +133,9 @@ [FV.FvMain] > > # > > # Multiple Console IO support > > # > > + INF MdeModulePkg/Universal/Console/ConPlatformDxe/ConPlatformDxe.inf > > + INF MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitterDxe.inf > > + INF MdeModulePkg/Universal/Console/GraphicsConsoleDxe/GraphicsConsoleDxe.inf > > INF MdeModulePkg/Universal/Console/TerminalDxe/TerminalDxe.inf > > INF MdeModulePkg/Universal/SerialDxe/SerialDxe.inf > > > > @@ -144,6 +147,9 @@ [FV.FvMain] > > INF ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf > > INF OvmfPkg/VirtioBlkDxe/VirtioBlk.inf > > > > + # Graphics Output Protocol > > + INF ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/LcdGraphicsOutputDxe.inf > > + > > INF Platform/ARM/Drivers/BootMonFs/BootMonFs.inf > > INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf > > > > diff --git a/Platform/ARM/SgiPkg/Library/HdLcdArmSgiLib/HdLcdArmSgiLib.inf b/Platform/ARM/SgiPkg/Library/HdLcdArmSgiLib/HdLcdArmSgiLib.inf > > new file mode 100644 > > index 000000000000..25efbea5fb83 > > --- /dev/null > > +++ b/Platform/ARM/SgiPkg/Library/HdLcdArmSgiLib/HdLcdArmSgiLib.inf > > @@ -0,0 +1,37 @@ > > +# > > +# Copyright (c) 2018, 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 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 = 0x0001001A > > + BASE_NAME = HdLcdArmSgiLib > > + FILE_GUID = 0C77342C-7895-4DE1-A9C8-1DBBFA71AF34 > > + MODULE_TYPE = BASE > > + VERSION_STRING = 1.0 > > + LIBRARY_CLASS = LcdPlatformLib > > + > > +[Sources.common] > > + HdLcdArmSgi.c > > + > > +[Packages] > > + ArmPkg/ArmPkg.dec > > + ArmPlatformPkg/ArmPlatformPkg.dec > > + MdePkg/MdePkg.dec > > + Platform/ARM/SgiPkg/SgiPlatform.dec > > + > > +[LibraryClasses] > > + BaseLib > > + > > +[FixedPcd] > > + gArmPlatformTokenSpaceGuid.PcdArmHdLcdBase > > + gArmPlatformTokenSpaceGuid.PcdArmLcdDdrFrameBufferBase > > + gArmPlatformTokenSpaceGuid.PcdArmLcdDdrFrameBufferSize > > + gArmPlatformTokenSpaceGuid.PcdGopPixelFormat > > diff --git a/Platform/ARM/SgiPkg/Include/SgiPlatform.h b/Platform/ARM/SgiPkg/Include/SgiPlatform.h > > index 550189565752..b9a662ae41a1 100644 > > --- a/Platform/ARM/SgiPkg/Include/SgiPlatform.h > > +++ b/Platform/ARM/SgiPkg/Include/SgiPlatform.h > > @@ -56,6 +56,10 @@ > > #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 > > + > > // Expansion AXI - Platform Peripherals - UART0 > > #define SGI_EXP_PLAT_PERIPH_UART0_BASE 0x7FF70000 > > #define SGI_EXP_PLAT_PERIPH_UART0_SZ SIZE_64KB > > diff --git a/Platform/ARM/SgiPkg/Library/HdLcdArmSgiLib/HdLcdArmSgi.c b/Platform/ARM/SgiPkg/Library/HdLcdArmSgiLib/HdLcdArmSgi.c > > new file mode 100644 > > index 000000000000..e1793c6d87c9 > > --- /dev/null > > +++ b/Platform/ARM/SgiPkg/Library/HdLcdArmSgiLib/HdLcdArmSgi.c > > @@ -0,0 +1,262 @@ > > +/** @file > > +* > > +* Copyright (c) 2018, 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 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 > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +typedef struct { > > + UINT32 OscFreq; > > + SCAN_TIMINGS Horizontal; > > + SCAN_TIMINGS Vertical; > > +} DISPLAY_MODE; > > + > > +STATIC DISPLAY_MODE mDisplayModes[] = { > > + { > > + // SVGA : 800 x 600 x 24 bpp. > > + 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} > > + } > > +}; > > + > > +STATIC CONST UINT32 mMaxMode = sizeof (mDisplayModes) / sizeof (DISPLAY_MODE); > > + > > Please use ARARY_SIZE() here > > > +/** 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 EFI_HANDLE Handle > > + ) > > +{ > > + (VOID)Handle; > > Drop this please > > > + 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); > > + return EFI_UNSUPPORTED; > > + } > > + > > + return EFI_SUCCESS; > > +} > > + > > +/** Allocate VRAM memory in DRAM for the framebuffer > > + > > + The allocated address can be used to set the framebuffer. > > + > > + @param[out] VramBaseAddress A pointer to the framebuffer address. > > + @param[out] VramSize A pointer to the size of the framebuffer > > + in bytes > > + > > + @retval EFI_SUCCESS Framebuffer memory allocated successfully. > > + @retval !(EFI_SUCCESS) Other errors. > > Just say 'other': the boolean negation of EFI_SUCCESS makes no sense here > > > +**/ > > +EFI_STATUS > > +LcdPlatformGetVram ( > > + OUT EFI_PHYSICAL_ADDRESS *VramBaseAddress, > > + OUT UINTN *VramSize > > + ) > > +{ > > + EFI_STATUS Status; > > + EFI_CPU_ARCH_PROTOCOL *Cpu; > > + > > + ASSERT (VramBaseAddress != NULL); > > + ASSERT (VramSize != NULL); > > + > > + // Set the VRAM size. > > + *VramSize = (UINTN)FixedPcdGet32 (PcdArmLcdDdrFrameBufferSize); > > + > > + // Check if base address is already reserved for the framebuffer. > > + *VramBaseAddress = > > + (EFI_PHYSICAL_ADDRESS)FixedPcdGet64 (PcdArmLcdDdrFrameBufferBase); > > + > > + if (*VramBaseAddress == 0) { > > + // If not already reserved, attempt to allocate the VRAM from the DRAM. > > + Status = gBS->AllocatePages ( > > + AllocateAnyPages, > > + EfiReservedMemoryType, > > + EFI_SIZE_TO_PAGES (*VramSize), > > + VramBaseAddress); > > + if (EFI_ERROR (Status)) { > > + DEBUG ((DEBUG_ERROR, "HdLcdArmSgi: Failed to allocate memory for" > > + "frame buffer.\n")); > > + ASSERT_EFI_ERROR (Status); > > + return Status; > > + } > > + } > > + > > + // Ensure the Cpu architectural protocol is already installed > > + Status = gBS->LocateProtocol ( > > + &gEfiCpuArchProtocolGuid, > > + NULL, > > + (VOID **)&Cpu); > > + if (EFI_ERROR (Status)) { > > + ASSERT_EFI_ERROR (Status); > > + gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize)); > > + return Status; > > + } > > + > > If you require this protocol, add it to the [depex] section, and drop > the error handling (but leave the ASSERT()) > > > + // The VRAM is inside the DRAM, which is cacheable. > > + // Mark the VRAM as write-combining (uncached) and non-executable. > > + Status = Cpu->SetMemoryAttributes ( > > + Cpu, > > + *VramBaseAddress, > > + *VramSize, > > + EFI_MEMORY_WC | EFI_MEMORY_XP); > > + if (EFI_ERROR (Status)) { > > + ASSERT_EFI_ERROR (Status); > > + gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize)); > > + } > > + > > + return Status; > > +} > > + > > +/** Return total number of modes supported. > > + > > + @retval UINT32 Mode Number. > > +**/ > > +UINT32 > > +LcdPlatformGetMaxMode (VOID) > > +{ > > + return mMaxMode; > > +} > > + > > +/** Set the requested display mode. > > + > > + @param[in] ModeNumber Mode Number. > > + > > + @retval EFI_SUCCESS Mode set successfully. > > + @retval EFI_INVALID_PARAMETER Requested mode not found. > > +**/ > > +EFI_STATUS > > +LcdPlatformSetMode ( > > + IN UINT32 ModeNumber > > + ) > > +{ > > + if (ModeNumber >= mMaxMode) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + return EFI_SUCCESS; > > +} > > + > > +/** 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 UINT32 ModeNumber, > > + OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION *Info > > + ) > > +{ > > + if (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 SCAN_TIMINGS **Horizontal, > > + OUT SCAN_TIMINGS **Vertical > > + ) > > +{ > > + if (ModeNumber >= mMaxMode) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + ASSERT (Horizontal != NULL); > > + ASSERT (Vertical != NULL); > > + > > + *Horizontal = &mDisplayModes[ModeNumber].Horizontal; > > + *Vertical = &mDisplayModes[ModeNumber].Vertical; > > + > > + // Pretend that clock probing and get timings are successful for SGI FVP > > + return EFI_SUCCESS; > > +} > > + > > +/** Return bits per pixel information for a mode number. > > + > > + @param[in] ModeNumber Mode Number. > > + > > + @param[out] Bpp Pointer to bits per pixel information. > > + > > + @retval EFI_SUCCESS Bits per pixel information for the requested > > + mode returned successfully. > > + @retval EFI_INVALID_PARAMETER Requested mode not found. > > +**/ > > +EFI_STATUS > > +LcdPlatformGetBpp ( > > + IN UINT32 ModeNumber, > > + OUT LCD_BPP *Bpp > > + ) > > +{ > > + if (ModeNumber >= mMaxMode) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + ASSERT (Bpp != NULL); > > + > > + *Bpp = LCD_BITS_PER_PIXEL_24; > > + > > + return EFI_SUCCESS; > > +} > > diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c > > index 3314a26e8f41..6ec2e8a7096d 100644 > > --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c > > +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c > > @@ -22,7 +22,7 @@ > > #include > > > > // Total number of descriptors, including the final "end-of-table" descriptor. > > -#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 11 > > +#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS 12 > > > > /** > > Returns the Virtual Memory Map of the platform. > > @@ -104,6 +104,12 @@ ArmPlatformGetVirtualMemoryMap ( > > VirtualMemoryTable[Index].Length = SGI_SUBSYS_GENERIC_GIC_SZ; > > VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; > > > > + // Expansion AXI - Platform Peripherals - HDLCD1 > > + VirtualMemoryTable[++Index].PhysicalBase = SGI_EXP_PLAT_PERIPH_HDLCD1_BASE; > > + VirtualMemoryTable[Index].VirtualBase = SGI_EXP_PLAT_PERIPH_HDLCD1_BASE; > > + VirtualMemoryTable[Index].Length = SGI_EXP_PLAT_PERIPH_HDLCD1_SZ; > > + VirtualMemoryTable[Index].Attributes = ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; > > + > > // Expansion AXI - Platform Peripherals - UART1 > > VirtualMemoryTable[++Index].PhysicalBase = SGI_EXP_PLAT_PERIPH_UART1_BASE; > > VirtualMemoryTable[Index].VirtualBase = SGI_EXP_PLAT_PERIPH_UART1_BASE; > > -- > > 2.7.4 > > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel Thanks for the review Ard. Posted a new patch addressing the comments. Regards, Vijay.