From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.77621.1675750222609994839 for ; Mon, 06 Feb 2023 22:10:22 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: vivek.gautam@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 8A6471063; Mon, 6 Feb 2023 22:11:04 -0800 (PST) Received: from [10.162.40.135] (a077843.arm.com [10.162.40.135]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id B902C3F8C6; Mon, 6 Feb 2023 22:10:20 -0800 (PST) Message-ID: Date: Tue, 7 Feb 2023 11:40:10 +0530 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 Subject: Re: [edk2-devel] [edk2-platforms][PATCH V2 4/5] Platform/Sgi: Initialize additional UART controllers To: devel@edk2.groups.io, pierre.gondois@arm.com Cc: ardb+tianocore@kernel.org, leif@nuviainc.com, Sami.Mujawar@arm.com References: <20230127092338.72056-1-vivek.gautam@arm.com> <20230127092338.72056-5-vivek.gautam@arm.com> From: "Vivek Kumar Gautam" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Hi Pierre, On 2/3/23 21:25, PierreGondois via groups.io wrote: > Hello Vivek, Thanks for your review. Please find my responses inline below. >=20 > On 1/27/23 10:23, Vivek Gautam wrote: >> From: Shriram K >> >> The IO virtualization block on reference design platforms allow >> connecting SoC expansion devices such as PL011 UART. On platforms >> that support this, initialize the UART controller connected to the >> IO virtualization block. >> >> Signed-off-by: Shriram K >> Signed-off-by: Vivek Gautam >> --- >> =C2=A0 Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf=C2=A0 |= 10 ++- >> =C2=A0 Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf=C2=A0 |= =C2=A0 7 ++- >> =C2=A0 Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c=C2=A0=C2=A0= =C2=A0 | 64=20 >> +++++++++++++++++++- >> =C2=A0 Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c | 43=20 >> ++++++++++++- >> =C2=A0 Platform/ARM/SgiPkg/SgiPlatform.dec=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 |=C2=A0 1 + >> =C2=A0 5 files changed, 118 insertions(+), 7 deletions(-) >> [snip] >> diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c=20 >> b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c >> index 8139b75d8ee4..08aa9bf64940 100644 >> --- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c >> +++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c >> @@ -1,6 +1,6 @@ >> =C2=A0 /** @file >> =C2=A0 * >> -*=C2=A0 Copyright (c) 2018-2020, ARM Limited. All rights reserved. >> +*=C2=A0 Copyright (c) 2018 - 2023, Arm Limited. All rights reserved. >> =C2=A0 * >> =C2=A0 *=C2=A0 SPDX-License-Identifier: BSD-2-Clause-Patent >> =C2=A0 * >> @@ -13,11 +13,23 @@ >> =C2=A0 #include >> =C2=A0 #include >> +#include >> =C2=A0 #include >> =C2=A0 // Total number of descriptors, including the final "end-of-tab= le"=20 >> descriptor. >> -#define MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 \ >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (14 + (FixedPc= dGet32 (PcdChipCount) * 2)) >> +#define=20 >> MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 \ >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ((14 + (FixedP= cdGet32 (PcdChipCount) * 2))=20 >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= \ >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (FixedPc= dGet32 (PcdIoVirtSocExpBlkUartEnable)=20 >> *=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 \ >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Fi= xedPcdGet32 (PcdChipCount) * 2)) >> + >> +// Memory Map descriptor for IO Virtualization SoC Expansion Block UA= RT >> +#define IO_VIRT_SOC_EXP_BLK_UART_MMAP(UartIdx,=20 >> ChipIdx)=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= \ >> +=C2=A0 VirtualMemoryTable[++Index].PhysicalBase=20 >> =3D=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 \ >> +=C2=A0=C2=A0=C2=A0 SGI_REMOTE_CHIP_MEM_OFFSET(ChipIdx) +=20 >> UART_START(UartIdx);=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 \ >> +=C2=A0 VirtualMemoryTable[Index].VirtualBase =20 >> =3D=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 \ >> +=C2=A0=C2=A0=C2=A0 SGI_REMOTE_CHIP_MEM_OFFSET(ChipIdx) +=20 >> UART_START(UartIdx);=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 \ >> +=C2=A0 VirtualMemoryTable[Index].Length=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 =3D=20 >> SIZE_64KB;=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 \ >> +=C2=A0 VirtualMemoryTable[Index].Attributes=C2=A0=C2=A0=C2=A0=C2=A0 =3D= =20 >> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; >> =C2=A0 /** >> =C2=A0=C2=A0=C2=A0 Returns the Virtual Memory Map of the platform. >> @@ -171,6 +183,31 @@ ArmPlatformGetVirtualMemoryMap ( >> =C2=A0=C2=A0=C2=A0 VirtualMemoryTable[Index].Length=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =3D SIZE_64KB; >> =C2=A0=C2=A0=C2=A0 VirtualMemoryTable[Index].Attributes=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 =3D=20 >> ARM_MEMORY_REGION_ATTRIBUTE_DEVICE; >> +#if (FixedPcdGet32 (PcdIoVirtSocExpBlkUartEnable) =3D=3D 1) >> +=C2=A0 // Chip-0 IO Virtualization SoC Expansion Block - UART0 >> +=C2=A0 IO_VIRT_SOC_EXP_BLK_UART_MMAP(0, 0) >> +=C2=A0 // Chip-0 IO Virtualization SoC Expansion Block - UART0 >=20 > NIT: shouldn't it be UART1 ? Yes, it should be UART1. Will correct it. >=20 >> +=C2=A0 IO_VIRT_SOC_EXP_BLK_UART_MMAP(1, 0) >> +#if (FixedPcdGet32 (PcdChipCount) > 1) >> +=C2=A0 // Chip-1 IO Virtualization SoC Expansion Block - UART0 >> +=C2=A0 IO_VIRT_SOC_EXP_BLK_UART_MMAP(0, 1) >> +=C2=A0 // Chip-1 IO Virtualization SoC Expansion Block - UART1 >> +=C2=A0 IO_VIRT_SOC_EXP_BLK_UART_MMAP(1, 1) >> +#if (FixedPcdGet32 (PcdChipCount) > 2) >> +=C2=A0 // Chip-2 IO Virtualization SoC Expansion Block - UART0 >> +=C2=A0 IO_VIRT_SOC_EXP_BLK_UART_MMAP(0, 2) >> +=C2=A0 // Chip-2 IO Virtualization SoC Expansion Block - UART1 >> +=C2=A0 IO_VIRT_SOC_EXP_BLK_UART_MMAP(1, 2) >> +#if (FixedPcdGet32 (PcdChipCount) > 3) >> +=C2=A0 // Chip-3 IO Virtualization SoC Expansion Block - UART0 >> +=C2=A0 IO_VIRT_SOC_EXP_BLK_UART_MMAP(0, 3) >> +=C2=A0 // Chip-3 IO Virtualization SoC Expansion Block - UART1 >> +=C2=A0 IO_VIRT_SOC_EXP_BLK_UART_MMAP(1, 3) >> +#endif >> +#endif >> +#endif >> +#endif >> + >> =C2=A0=C2=A0=C2=A0 // DDR - (2GB - 16MB) >> =C2=A0=C2=A0=C2=A0 VirtualMemoryTable[++Index].PhysicalBase=C2=A0 =3D = PcdGet64=20 >> (PcdSystemMemoryBase); >> =C2=A0=C2=A0=C2=A0 VirtualMemoryTable[Index].VirtualBase=C2=A0=C2=A0=C2= =A0=C2=A0 =3D PcdGet64=20 >> (PcdSystemMemoryBase); >> diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dec=20 >> b/Platform/ARM/SgiPkg/SgiPlatform.dec >> index 407f03c1c3e8..43d350ec48bb 100644 >> --- a/Platform/ARM/SgiPkg/SgiPlatform.dec >> +++ b/Platform/ARM/SgiPkg/SgiPlatform.dec >> @@ -102,6 +102,7 @@ >> =C2=A0=C2=A0=C2=A0 gArmSgiTokenSpaceGuid.PcdIoVirtSocExpBlk0Base|0|UIN= T64|0x0000002B >> =20 >> gArmSgiTokenSpaceGuid.PcdIoVirtSocExpBlkPeriOffset|0|UINT32|0x0000002C >> =20 >> gArmSgiTokenSpaceGuid.PcdIoVirtSocExpBlkResourceSize|0|UINT32|0x000000= 2D >> +=C2=A0 gArmSgiTokenSpaceGuid.PcdIoVirtSocExpBlkUartEnable|0|UINT32|0x= 0000002E >=20 > PcdIoVirtSocExpBlkUartEnable isn't set for any platform, is it normal ? Yes, by default we are keeping these Uarts disabled as these are not=20 used for system debug. Platforms that plan to use them can enable these=20 in debug/production as needed. Best regards Vivek