Re: [PATCH v4 3/3] ns16550: Gate all PCI code with CONFIG_X86

[Bertrand Marquis <Bertrand.Marquis@arm.com>]

> On 25 Nov 2020, at 18:21, Rahul Singh <Rahul.Singh@arm.com> wrote:
> 
> Hello Julien,
> 
>> On 25 Nov 2020, at 6:16 pm, Rahul Singh <rahul.singh@arm.com> wrote:
>> 
>> 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 = {
>>                    ^~~~~~~~
>> 
>> 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.
>> 
>> Guard all remaining PCI code that is not under x86 flag with CONFIG_X86.
>> 
>> No functional change intended.
>> 
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

> 
> Sorry I missed to add the signed off for the commit msg. I will send the next version once reviewed done.
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 

I guess the commiter can add this while commiting this patch ?

Regards
Bertrand

> Regards,
> Rahul
>> ---
>> 
>> Changes in v4:
>> - As per the discussion guard all remaining PCI code with CONFIG_X86
>> 
>> ---
>> xen/drivers/char/ns16550.c | 16 ++++++++--------
>> 1 file changed, 8 insertions(+), 8 deletions(-)
>> 
>> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
>> index 9235d854fe..26e601857a 100644
>> --- a/xen/drivers/char/ns16550.c
>> +++ b/xen/drivers/char/ns16550.c
>> @@ -16,7 +16,7 @@
>> #include <xen/timer.h>
>> #include <xen/serial.h>
>> #include <xen/iocap.h>
>> -#ifdef CONFIG_HAS_PCI
>> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
>> #include <xen/pci.h>
>> #include <xen/pci_regs.h>
>> #include <xen/pci_ids.h>
>> @@ -51,7 +51,7 @@ static struct ns16550 {
>>    unsigned int timeout_ms;
>>    bool_t intr_works;
>>    bool_t dw_usr_bsy;
>> -#ifdef CONFIG_HAS_PCI
>> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
>>    /* PCI card parameters. */
>>    bool_t pb_bdf_enable;   /* if =1, pb-bdf effective, port behind bridge */
>>    bool_t ps_bdf_enable;   /* if =1, ps_bdf effective, port on pci card */
>> @@ -66,7 +66,7 @@ static struct ns16550 {
>> #endif
>> } ns16550_com[2] = { { 0 } };
>> 
>> -#ifdef CONFIG_HAS_PCI
>> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
>> struct ns16550_config {
>>    u16 vendor_id;
>>    u16 dev_id;
>> @@ -256,7 +256,7 @@ static int ns16550_getc(struct serial_port *port, char *pc)
>> 
>> static void pci_serial_early_init(struct ns16550 *uart)
>> {
>> -#ifdef CONFIG_HAS_PCI
>> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
>>    if ( !uart->ps_bdf_enable || uart->io_base >= 0x10000 )
>>        return;
>> 
>> @@ -355,7 +355,7 @@ static void __init ns16550_init_preirq(struct serial_port *port)
>> 
>> static void __init ns16550_init_irq(struct serial_port *port)
>> {
>> -#ifdef CONFIG_HAS_PCI
>> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
>>    struct ns16550 *uart = port->uart;
>> 
>>    if ( uart->msi )
>> @@ -397,7 +397,7 @@ static void __init ns16550_init_postirq(struct serial_port *port)
>>    uart->timeout_ms = max_t(
>>        unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
>> 
>> -#ifdef CONFIG_HAS_PCI
>> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
>>    if ( uart->bar || uart->ps_bdf_enable )
>>    {
>>        if ( uart->param && uart->param->mmio &&
>> @@ -477,7 +477,7 @@ static void ns16550_suspend(struct serial_port *port)
>> 
>>    stop_timer(&uart->timer);
>> 
>> -#ifdef CONFIG_HAS_PCI
>> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
>>    if ( uart->bar )
>>       uart->cr = pci_conf_read16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
>>                                  uart->ps_bdf[2]), PCI_COMMAND);
>> @@ -486,7 +486,7 @@ static void ns16550_suspend(struct serial_port *port)
>> 
>> static void _ns16550_resume(struct serial_port *port)
>> {
>> -#ifdef CONFIG_HAS_PCI
>> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
>>    struct ns16550 *uart = port->uart;
>> 
>>    if ( uart->bar )
>> --
>> 2.17.1