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

[Julien Grall <julien.grall.oss@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5642 bytes --]

On Thu, 19 Nov 2020, 23:38 Stefano Stabellini, <sstabellini@kernel.org>
wrote:

> 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 :-)


Please don't put words in my mouth... I think introducing HAS_PCI_MSI is
short sighted.

There are no clear benefits of it when NS16550 PCI support is not going to
be enable in the foreseeable future.

I would be ok with moving everything under CONFIG_X86. IHMO this is still
shortsighted but at least we don't introduce a config that's not going to
help Arm or other any architecture to disable completely PCI support in
NS16550.

Cheers,

[-- Attachment #2: Type: text/html, Size: 7343 bytes --]