Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Julien Grall <julien@xxxxxxx>]

Hi,

On 18/11/2020 15:16, Jan Beulich wrote:
> On 18.11.2020 16:02, Rahul Singh wrote:
> > Hello Jan,
> >
> > > On 17 Nov 2020, at 10:55 am, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> > >
> > > On 16.11.2020 13:25, Rahul Singh wrote:
> > > > NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
> > > > is enabled for ARM, compilation error is observed for ARM architecture
> > > > because ARM platforms do not have full PCI support available.
> > > While you've extended the sentence, it remains unclear to me what
> > > compilation error it is that results here. I've requested such
> > > clarification for v2 already.
> > Compilation error is related to the code that refer to x86  functions 
> > (create_irq()..) and MSI implementation related error.
> > For more details please find the attached file for compilation error.
> The use of mmio_ro_ranges is almost quite possibly going to remain
> x86-specific, but then I guess this wants abstracting in a suitable
> way.
>
> The remaining look to all be MSI-related, so perhaps what you want
> to avoid is just that part rather than everything PCI-ish?
Not really (see more above).

> > > > --- a/xen/drivers/char/ns16550.c
> > > > +++ b/xen/drivers/char/ns16550.c
> > > > @@ -16,7 +16,7 @@
> > > > #include <xen/timer.h>
> > > > #include <xen/serial.h>
> > > > #include <xen/iocap.h>
> > > > -#ifdef CONFIG_HAS_PCI
> > > > +#ifdef CONFIG_HAS_NS16550_PCI
> > > > #include <xen/pci.h>
> > > > #include <xen/pci_regs.h>
> > > > #include <xen/pci_ids.h>
> > > ... #undef-ining CONFIG_HAS_PCI at a suitable position in this
> > > file (e.g. after all #include-s, to make sure all structure
> > > layouts remain correct)? This would then be far easier to revert
> > > down the road, and would confine the oddity to a single file
> > > (and there a single place) in the code base.
> > As for ARM platforms, PCI implementation is in the development process and I am 
> > not sure if after completion of PCI work, ns16500 PCI part of code will work 
> > out of the box. I think there is some effort required to test the ns16550 PCI 
> > part of the code on ARM.
> > As this code is tested on X86 only so I make the options depends on X86 and 
> > enable it by default for x86.
> >
> > I feel that adding a new Kconfig options is ok to enable/disable the PCI 
> > NS16550 support as compared to #undef CONFIG_HAS_PCI in the particular file. If 
> > in future other architecture wants to implement the PCI they will face the 
> > similar compilation error issues.
> >
> > Please suggest how we can proceed on this.
> Introduce CONFIG_HAS_PCI_MSI (selected only by x86), if there's no
> immediate plan to support it on Arm together with the rest of PCI?
So even we are going to enable PCI on Arm and fix compilation issue, 
there are no way the NS16550 PCI would be usuable without effort for a 
few reasons:
  1) com1/com2 is x86 specific
  2) ns16550_init() is not used by Arm and the only way to use a PCI UART
  3) UART is discovered through the device-tree/ACPI tables on Arm

So I think CONFIG_HAS_NS16550_PCI is most suitable solution and we 
should probably guard more code (e.g. ns16550_init(), com1, com2...).
Note that's not a request for doing it in this patch :).

Cheers,

--
Julien Grall