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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 19 Nov 2020, Rahul Singh wrote:
> > 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).
> 
> I just checked the compilation error for ARM after enabling the HAS_PCI on 
> ARM. I am observing the same compilation error what I observed previously. 
> There are two new errors related to struct uart_config and struct part_param 
> as those struct defined globally but used under X86 flags.
> 
> At top level:
> ns16550.c:179:48: error: ‘uart_config’ defined but not used 
> [-Werror=unused-const-variable=]
>  static const struct ns16550_config __initconst uart_config[] =
>                                                 ^~~~~~~~~~~
> ns16550.c:104:54: error: ‘uart_param’ defined but not used 
> [-Werror=unused-const-variable=]
>  static const struct ns16550_config_param __initconst uart_param[] = { 
> 
> 
> > 
> >>>> 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...
> 
> To fix compilation error on ARM as per the discussion there are below options 
> please suggest which one to use to proceed further.
> 
> 1. Use the newly introduced CONFIG_HAS_NS16550_PCI config options. This helps 
> also non-x86 architecture in the future not to have compilation error 
> what we are observing now when HAS_PCI is enabled.
> 
> 2. Guard the remaining x86 specific code with CONFIG_X86 and introduce the 
> new CONFIG_HAS_PCI_MSI options to fix the MSI related compilation error. 
> Once we have proper support for MSI and PCI for ARM  (HAS_PCI_MSI and HAS_PCI 
> enabled for ARM in Kconfig ) I am not sure if NS16550 PCI will work out of 
> the box on ARM .In that case, we might need to come back again to fix NS16550 
> driver.  


It doesn't matter too much to me, let's just choose one option so that you
get unblocked soon.

It looks like Jan prefers option 2) and both Julien and I are OK with
it. So let's do 2). Jan, please confirm too :-)