Re: [edk2-devel] [edk2-platforms][PATCH V1 1/1] Platform/ARM/N1Sdp: Modify IRQ ID of Debug UART and routing to IOFPGA UART1

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Sahil, Thomas, 

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

On 25/01/2023, 19:53, "Thomas Abraham" <thomas.abraham@arm.com> wrote:

    Hi Sahil,

    On 06/01/2023 06:09, sahil via groups.io wrote:
    > From: Himanshu Sharma <Himanshu.Sharma@arm.com>
    > 
    > In DBG2 table, IRQ ID was set as 0 for the UART. This overwrote the
[SAMI] The above line is incorrect. It is not the DBG2 table where the IRQ ID is set. 
It is the AML description for the UART that is configured as the serial debug port where the IRQ ID needs to be configured correctly.
[/SAMI]

    > IPI0 trigger method to "level", which prevented SGI0 to be enabled
    > again after a CPU offline/online cycle.
    > 
    > This patch fixes the above issue by assigning a reserved IRQ ID
[SAMI] I am not sure what you mean here. Is the IRQ ID 250 (configured in code below) not for the UART?

    > for the Debug UART, other than 0 and also routing it to use IOFPGA
    > UART1 by unsharing it from currently using serial terminal.
    > 
    > Signed-off-by: Himanshu Sharma <Himanshu.Sharma@arm.com>
[SAMI] This should be signed off only by the person who sends the patch. 

    > ---
    >   Platform/ARM/N1Sdp/N1SdpPlatform.dsc                                                   | 8 ++++----
    >   Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c | 2 +-
    >   2 files changed, 5 insertions(+), 5 deletions(-)
    > 
    > diff --git a/Platform/ARM/N1Sdp/N1SdpPlatform.dsc b/Platform/ARM/N1Sdp/N1SdpPlatform.dsc
    > index d04b22d3ef51..676ab677257a 100644
    > --- a/Platform/ARM/N1Sdp/N1SdpPlatform.dsc
    > +++ b/Platform/ARM/N1Sdp/N1SdpPlatform.dsc
    > @@ -4,7 +4,7 @@
    >   # This provides platform specific component descriptions and libraries that
    > 
    >   # conform to EFI/Framework standards.
    > 
    >   #
    > 
    > -# Copyright (c) 2018 - 2021, ARM Limited. All rights reserved.<BR>
    > 
    > +# Copyright (c) 2018 - 2023, ARM Limited. All rights reserved.<BR>
    > 
    >   #
    > 
    >   # SPDX-License-Identifier: BSD-2-Clause-Patent
    > 
    >   #
    > 
    > @@ -136,9 +136,9 @@ [PcdsFixedAtBuild.common]
    >     gArmPlatformTokenSpaceGuid.PL011UartInterrupt|95
    > 
    >   
    > 
    >     # PL011 Serial Debug UART (DBG2)
    > 
    > -  gArmPlatformTokenSpaceGuid.PcdSerialDbgRegisterBase|gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterBase
    > 
    > -  gArmPlatformTokenSpaceGuid.PcdSerialDbgUartBaudRate|gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate
    > 
    > -  gArmPlatformTokenSpaceGuid.PcdSerialDbgUartClkInHz|50000000
    > 
    > +  gArmPlatformTokenSpaceGuid.PcdSerialDbgRegisterBase|0x1C0A0000
    > 
    > +  gArmPlatformTokenSpaceGuid.PcdSerialDbgUartBaudRate|115200
    > 
    > +  gArmPlatformTokenSpaceGuid.PcdSerialDbgUartClkInHz|24000000
    > 
    >   
    > 
    >     # SBSA Watchdog
    > 
    >     gArmTokenSpaceGuid.PcdGenericWatchdogEl2IntrNum|93
    > 
    > diff --git a/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c b/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c
    > index b11c0425fe25..44046a0026bb 100644
    > --- a/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c
    > +++ b/Platform/ARM/N1Sdp/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c
    > @@ -320,7 +320,7 @@ EDKII_PLATFORM_REPOSITORY_INFO N1sdpRepositoryInfo = {
    >     // Debug Serial Port
    > 
    >     {
    > 
    >       FixedPcdGet64 (PcdSerialDbgRegisterBase),               // BaseAddress
    > 
    > -    0,                                                      // Interrupt -unused
    > 
    > +    250,                                                    // Interrupt (reserved)

    I have not looked into ConfigurationManager but why would IRQ number be 
    mandatory for debug UART? Isn't there a way to just avoid specifying a 
    interrupt number?
[SAMI] This information is used for:
1. Generating the DBG2 table, which does not require the interrupt number (and is hence ignored).
2. Generating the AML description for the UART port which requires the Interrupt to be specified, see example below:
   // UART PL011
    Device(COM0) {
      Name(_HID, "ARMH0011")
      Name(_UID, Zero)
      Name(_CRS, ResourceTemplate() {
        Memory32Fixed(ReadWrite, 0x7FF80000, 0x1000)
        Interrupt(ResourceConsumer, Level, ActiveHigh, Exclusive) { 115 }
      })
    }

The commit message is misleading and needs to be fixed.

[/SAMI]
    Thanks,
    Thomas.

    > 
    >       FixedPcdGet64 (PcdSerialDbgUartBaudRate),               // BaudRate
    > 
    >       FixedPcdGet32 (PcdSerialDbgUartClkInHz),                // Clock
    > 
    >       EFI_ACPI_DBG2_PORT_SUBTYPE_SERIAL_ARM_PL011_UART        // Port subtype
    >