| Message ID | 6d64bb35a6ce247faaa3df2ebae27b6bfa1d969e.1606326929.git.rahul.singh@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Make PCI passthrough code non-x86 specific | expand |
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> 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> 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 >
On Wed, 25 Nov 2020, Rahul Singh 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> Acked-by: Stefano Stabellini <sstabellini@kernel.org> > --- > > 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 >
> 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
On 25.11.2020 19:16, Rahul Singh wrote: > --- 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) I'm sorry to be picky, but this being a hack wants, imo, also calling it so, by way of a code comment. Clearly this should go at one of the first instances, yet neither of the two above are really suitable imo. Hence I'm coming back to my prior suggestion of introducing a consolidated #define without this becoming a Kconfig setting: /* * The PCI part of the code in this file currently is only known to * work on x86. Undo this hack once the logic has been suitably * abstracted. */ #if defined(CONFIG_HAS_PCI) && defined(CONFIG_X86) # define NS16550_PCI #endif And then use NS16550_PCI everywhere. I'd be fine making this adjustment while committing, if I knew that (a) you're okay with it and (b) the R-b and A-b you've already got can be kept. Jan
Hi Jan, > On 27 Nov 2020, at 13:58, Jan Beulich <jbeulich@suse.com> wrote: > > On 25.11.2020 19:16, Rahul Singh wrote: >> --- 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) > > I'm sorry to be picky, but this being a hack wants, imo, also calling > it so, by way of a code comment. Clearly this should go at one of the > first instances, yet neither of the two above are really suitable imo. > Hence I'm coming back to my prior suggestion of introducing a > consolidated #define without this becoming a Kconfig setting: > > /* > * The PCI part of the code in this file currently is only known to > * work on x86. Undo this hack once the logic has been suitably > * abstracted. > */ > #if defined(CONFIG_HAS_PCI) && defined(CONFIG_X86) > # define NS16550_PCI > #endif > > And then use NS16550_PCI everywhere. I'd be fine making this > adjustment while committing, if I knew that (a) you're okay with it > and (b) the R-b and A-b you've already got can be kept. > Sounds ok to me so you can keep my R-b if you go this way. Cheers Bertrand > Jan >
Hello Jan, > On 27 Nov 2020, at 1:58 pm, Jan Beulich <jbeulich@suse.com> wrote: > > On 25.11.2020 19:16, Rahul Singh wrote: >> --- 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) > > I'm sorry to be picky, but this being a hack wants, imo, also calling > it so, by way of a code comment. Clearly this should go at one of the > first instances, yet neither of the two above are really suitable imo. > Hence I'm coming back to my prior suggestion of introducing a > consolidated #define without this becoming a Kconfig setting: > > /* > * The PCI part of the code in this file currently is only known to > * work on x86. Undo this hack once the logic has been suitably > * abstracted. > */ > #if defined(CONFIG_HAS_PCI) && defined(CONFIG_X86) > # define NS16550_PCI > #endif > > And then use NS16550_PCI everywhere. I'd be fine making this > adjustment while committing, if I knew that (a) you're okay with it > and Thanks for reviewing the code.I am ok with it. > (b) the R-b and A-b you've already got can be kept. > > Jan >
On Fri, 27 Nov 2020, Bertrand Marquis wrote: > Hi Jan, > > > On 27 Nov 2020, at 13:58, Jan Beulich <jbeulich@suse.com> wrote: > > > > On 25.11.2020 19:16, Rahul Singh wrote: > >> --- 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) > > > > I'm sorry to be picky, but this being a hack wants, imo, also calling > > it so, by way of a code comment. Clearly this should go at one of the > > first instances, yet neither of the two above are really suitable imo. > > Hence I'm coming back to my prior suggestion of introducing a > > consolidated #define without this becoming a Kconfig setting: > > > > /* > > * The PCI part of the code in this file currently is only known to > > * work on x86. Undo this hack once the logic has been suitably > > * abstracted. > > */ > > #if defined(CONFIG_HAS_PCI) && defined(CONFIG_X86) > > # define NS16550_PCI > > #endif > > > > And then use NS16550_PCI everywhere. I'd be fine making this > > adjustment while committing, if I knew that (a) you're okay with it > > and (b) the R-b and A-b you've already got can be kept. > > > > Sounds ok to me so you can keep my R-b if you go this way. I am OK with that too
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 )
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> --- 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(-)