Re: [Xen-devel] [PATCH 2/3 v3] xen: Add support for initializing 16550 UART using ACPI

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi,

On 24/11/17 15:11, Andre Przywara wrote:
> On 24/11/17 13:50, Konrad Rzeszutek Wilk wrote:
> > On Fri, Nov 24, 2017 at 05:09:11PM +0530, Bhupinder Thakur wrote:
> > > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> > > index c5dfc1e..af4712f 100644
> > > --- a/xen/drivers/char/ns16550.c
> > > +++ b/xen/drivers/char/ns16550.c
> > > @@ -29,6 +29,10 @@
> > >   #ifdef CONFIG_X86
> > >   #include <asm/fixmap.h>
> > >   #endif
> > > +#ifdef CONFIG_ACPI
> > > +#include <xen/acpi.h>
> > > +#endif
> > > +
> > >   
> > >   /*
> > >    * Configure serial port with a string:
> > > @@ -1565,6 +1569,69 @@ DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL)
> > >   DT_DEVICE_END
> > >   
> > >   #endif /* HAS_DEVICE_TREE */
> > > +
> > > +#if defined(CONFIG_ACPI) && defined(CONFIG_ARM)
> > I would remove the CONFIG_ARM as the spec says it is possible to use
> > this on ARM _and_ x86 hardware.
> I was thinking the same. Originally the SPCR was x86 only.
Yes but Xen does not use SPCR on x86 today. So this will likely lead to 
a build failure as the function would not be used.
Unless someone decided to plug it for x86, the right solution is to 
ifdef CONFIG_ARM that code.
> > But I guess you can't as ACPI_DEVICE_START is defined to be only
> > on ARM?
> We could move the #ifdef there.
>
> > > +
> > > +static int ns16550_init_acpi(struct ns16550 **puart)
> > > +{
> > > +    struct acpi_table_spcr *spcr;
> > > +    int status;
> > hmm, not acpi_status ?
> > > +    struct ns16550 *uart = &ns16550_com[0];
> > > +
> > > +    ns16550_init_common(uart);
> > I would move this below the error check below..
> > > +
> > > +    status = acpi_get_table(ACPI_SIG_SPCR, 0,
> > > +                            (struct acpi_table_header **)&spcr);
> > > +
> > > +    if ( ACPI_FAILURE(status) )
> > > +    {
> > > +        printk("ns16550: Failed to get SPCR table\n");
> > > +        return -EINVAL;
> > > +    }
> > > +
> > > +    uart->baud      = BAUD_AUTO;
> > > +    uart->data_bits = 8;
> > Are those two assumed from the ACPI spec?
> I can't find a field for the number of data bits, so I assume it's
> indeed fixed to 8.
>
> > Wait a minute. The
> > https://msdn.microsoft.com/en-us/library/windows/hardware/dn639132(v=vs.85).aspx
> > has Baud Rate at Offset 58?
> Yes, I see the same. We should use that table.
> Just wondering what the Moonshot gives you in this table? Is it "7"?
What is the point to use the baud rate from the table? It should have 
been configured by the firmware and there are no point for the driver to 
reconfigure it. It will likely make it worst as AFAICT we don't have the 
clock frequency in hand.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel