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



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



 


Rackspace

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