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

[Julien Grall <julien@xxxxxxx>]

On 19/11/2020 09:53, Jan Beulich wrote:
> On 19.11.2020 10:21, Julien Grall wrote:
> > Hi Jan,
> >
> > On 19/11/2020 09:05, Jan Beulich wrote:
> > > On 18.11.2020 16:50, Julien Grall wrote:
> > > > On 16/11/2020 12: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.
> > > >    >
> > > > > Introducing new kconfig option CONFIG_HAS_NS16550_PCI to support
> > > > > ns16550 PCI for X86.
> > > > >
> > > > > For X86 platforms it is enabled by default. For ARM platforms it is
> > > > > disabled by default, once we have proper support for NS16550 PCI for
> > > > > ARM we can enable it.
> > > > >
> > > > > No functional change.
> > > > NIT: I would say "No functional change intended" to make clear this is
> > > > an expectation and hopefully will be correct :).
> > > >
> > > > Regarding the commit message itself, I would suggest the following to
> > > > address Jan's concern:
> > > While indeed this is a much better description, I continue to think
> > > that the proposed Kconfig option is undesirable to have.
> > I am yet to see an argument into why we should keep the PCI code
> > compiled on Arm when there will be no-use....
> Well, see my patch suppressing building of quite a part of it.
I will let Rahul figuring out whether your patch series is sufficient to 
fix compilation issues (this is what matters right now).
> > > Either,
> > > following the patch I've just sent, truly x86-specific things (at
> > > least as far as current state goes - if any of this was to be
> > > re-used by a future port, suitable further abstraction may be
> > > needed) should be guarded by CONFIG_X86 (or abstracted into arch
> > > hooks), or the HAS_PCI_MSI proposal would at least want further
> > > investigating as to its feasibility to address the issues at hand.
> > I would be happy with CONFIG_X86, despite the fact that this is only
> > deferring the problem.
> >
> > Regarding HAS_PCI_MSI, I don't really see the point of introducing given
> > that we are not going to use NS16550 PCI on Arm in the forseeable
> > future.
> And I continue to fail to see what would guarantee this: As soon
> as you can plug in such a card into an Arm system, people will
> want to be able use it. That's why we had to add support for it
> on x86, after all.
Well, plug-in PCI cards on Arm has been available for quite a while... 
Yet I haven't heard anyone asking for NS16550 PCI support.
This is probably because SBSA compliant server should always provide an 
SBSA UART (a cut-down version of the PL011). So why would bother to lose 
a PCI slot for yet another UART?
> >> So why do we need a finer graine Kconfig?
>
> Because most of the involved code is indeed MSI-related?
Possibly, yet it would not be necessary if we don't want NS16550 PCI 
support...
Cheers,

--
Julien Grall