Re: [edk2-devel] [PATCH v1 10/19] DynamicTablesPkg: Serial debug port initialisation

["Leif Lindholm" <leif.lindholm@linaro.org>]

On Thu, Nov 21, 2019 at 16:29:16 +0100, Philippe Mathieu-Daudé wrote:
> On 11/21/19 4:23 PM, Leif Lindholm wrote:
> > On Thu, Nov 21, 2019 at 16:20:31 +0100, Philippe Mathieu-Daudé wrote:
> > > On 8/23/19 12:55 PM, Sami Mujawar wrote:
> > > > The ARM DCC serial port subtype is an option that is
> > > > supported by the DBG2 generator. However, the serial
> > > > port initialisation should only be done for PL011/SBSA
> > > > compatible UARTs.
> > > > 
> > > > Add check to conditionally initialise the serial port.
> > > > 
> > > > Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> > > > ---
> > > >    DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c | 27 ++++++++++++--------
> > > >    1 file changed, 17 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
> > > > index 346ab5b22f5402bf87c385558f68f080d1b454ed..51c843d25f75388104694855ce133b3d61860196 100644
> > > > --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
> > > > +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiDbg2LibArm/Dbg2Generator.c
> > > > @@ -175,7 +175,7 @@ GET_OBJECT_LIST (
> > > >      CM_ARM_SERIAL_PORT_INFO
> > > >      );
> > > > -/** Initialize the PL011 UART with the parameters obtained from
> > > > +/** Initialize the PL011/SBSA UART with the parameters obtained from
> > > >        the Configuration Manager.
> > > 
> > > Isn't the SBSA UART a PL011?
> > 
> > No. It's a compatible subset.
> > So a PL011 can be used as an SBSA UART.
> 
> OK thanks.
> 
> Can you update the comment? Maybe:
> 
> "Initialize the PL011 compatible UART with the parameters ..."

The original is correct, the suggested alternative is not (an SBSA
UART is a subset, so not fully compatible).

If the comment was to change, I would suggest that dropping the model
name completely and simply refer to it as "the UART" would be
preferable.

/
    Leif

> Regardless:
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
> 
> > 
> > /
> >      Leif