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

[Jan Beulich <jbeulich@xxxxxxxx>]

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. 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.

Jan

> "
> xen/char: ns16550: Gate all PCI code with a new Kconfig HAS_NS16550_PCI
> 
> The NS16550 driver is assuming that NS16550 PCI card are usable if the 
> architecture supports PCI (i.e. CONFIG_HAS_PCI=y). However, the code is 
> very x86 focus and will fail to build on Arm (/!\ it is not all the errors):
> 
>   ns16550.c: In function ‘ns16550_init_irq’:
> ns16550.c:726:21: error: implicit declaration of function ‘create_irq’; 
> did you mean ‘release_irq’? [-Werror=implicit-function-declaration]
>           uart->irq = create_irq(0, false);
>                       ^~~~~~~~~~
>                       release_irq
> ns16550.c:726:21: error: nested extern declaration of ‘create_irq’ 
> [-Werror=nested-externs]
> ns16550.c: In function ‘ns16550_init_postirq’:
> ns16550.c:768:33: error: ‘mmio_ro_ranges’ undeclared (first use in this 
> function); did you mean ‘mmio_handler’?
>                rangeset_add_range(mmio_ro_ranges, uart->io_base,
>                                   ^~~~~~~~~~~~~~
>                                   mmio_handler
> ns16550.c:768:33: note: each undeclared identifier is reported only once 
> for each function it appears in
> ns16550.c:780:20: error: variable ‘msi’ has initializer but incomplete type
>               struct msi_info msi = {
>                      ^~~~~~~~
> ns16550.c:781:18: error: ‘struct msi_info’ has no member named ‘bus’
>                   .bus = uart->ps_bdf[0],
>                    ^~~
> ns16550.c:781:24: error: excess elements in struct initializer [-Werror]
>                   .bus = uart->ps_bdf[0],
>                          ^~~~
> ns16550.c:781:24: note: (near initialization for ‘msi’)
> ns16550.c:782:18: error: ‘struct msi_info’ has no member named ‘devfn’
>                   .devfn = PCI_DEVFN(uart->ps_bdf[1], uart->ps_bdf[2]),
> 
> Enabling support for NS16550 PCI card on Arm would require more plumbing 
> in addition to fixing the compilation error.
> 
> Arm systems tend to have platform UART available such as NS16550, PL011. 
> So there are limited reasons to get NS16550 PCI support for now on Arm.
> 
> A new Kconfig option CONFIG_HAS_NS16550_PCI is introduced to gate all 
> the PCI code.
> 
> This option will be select automically for x86 platform and left 
> unselectable on Arm.
> 
> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> [julieng: Commit message]
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> "
> 
> Cheers,
>