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

[Julien Grall <julien@xxxxxxx>]

Hi Rahul,

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:
"
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,

--
Julien Grall