[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





On Thu, 19 Nov 2020, 23:38 Stefano Stabellini, <sstabellini@xxxxxxxxxx> 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,



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.