On Thu, 19 Nov 2020, Julien Grall wrote:
> On Thu, 19 Nov 2020, 23:38 Stefano Stabellini, <sstabell...@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...
Sorry Julien, I misinterpreted one of your previous comments. Sometimes
it is difficult to do things by email. It is good that you clarified as
my goal was to reach an agreement.
> 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 agree
> 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.
So you are suggesting a new option:
3. Guard the remaining x86 specific code *and* the MSI related
compilation errors with CONFIG_X86
Is that right?
My preference is actually option 1) but this series is already at v3 and
I don't think this decision is as important as much as unblocking
Rahul, so I am OK with the other alternatives too.
I tend to agree with you that 3) is better than 2) for the reasons you
wrote above.
