| Message ID | 955996aa8cd7f17f9f39c60bd3b9b74ffaa5c5f7.1605527997.git.rahul.singh@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Make PCI passthrough code non-x86 specific | expand |
On Mon, 16 Nov 2020, 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. > > Signed-off-by: Rahul Singh <rahul.singh@arm.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > > Changes in v3: > - remove help text from the Kconfig file because of prompt-less option. > > --- > xen/drivers/char/Kconfig | 4 ++++ > xen/drivers/char/ns16550.c | 32 ++++++++++++++++---------------- > 2 files changed, 20 insertions(+), 16 deletions(-) > > diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig > index b572305657..abb59fdb0f 100644 > --- a/xen/drivers/char/Kconfig > +++ b/xen/drivers/char/Kconfig > @@ -4,6 +4,10 @@ config HAS_NS16550 > help > This selects the 16550-series UART support. For most systems, say Y. > > +config HAS_NS16550_PCI > + def_bool y > + depends on X86 && HAS_NS16550 && HAS_PCI > + > config HAS_CADENCE_UART > bool "Xilinx Cadence UART driver" > default y > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c > index d8b52eb813..bd1c2af956 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 > +#ifdef CONFIG_HAS_NS16550_PCI > #include <xen/pci.h> > #include <xen/pci_regs.h> > #include <xen/pci_ids.h> > @@ -54,7 +54,7 @@ enum serial_param_type { > reg_shift, > reg_width, > stop_bits, > -#ifdef CONFIG_HAS_PCI > +#ifdef CONFIG_HAS_NS16550_PCI > bridge_bdf, > device, > port_bdf, > @@ -83,7 +83,7 @@ static struct ns16550 { > unsigned int timeout_ms; > bool_t intr_works; > bool_t dw_usr_bsy; > -#ifdef CONFIG_HAS_PCI > +#ifdef CONFIG_HAS_NS16550_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 */ > @@ -117,14 +117,14 @@ static const struct serial_param_var __initconst sp_vars[] = { > {"reg-shift", reg_shift}, > {"reg-width", reg_width}, > {"stop-bits", stop_bits}, > -#ifdef CONFIG_HAS_PCI > +#ifdef CONFIG_HAS_NS16550_PCI > {"bridge", bridge_bdf}, > {"dev", device}, > {"port", port_bdf}, > #endif > }; > > -#ifdef CONFIG_HAS_PCI > +#ifdef CONFIG_HAS_NS16550_PCI > struct ns16550_config { > u16 vendor_id; > u16 dev_id; > @@ -620,7 +620,7 @@ static int ns16550_getc(struct serial_port *port, char *pc) > > static void pci_serial_early_init(struct ns16550 *uart) > { > -#ifdef CONFIG_HAS_PCI > +#ifdef CONFIG_HAS_NS16550_PCI > if ( !uart->ps_bdf_enable || uart->io_base >= 0x10000 ) > return; > > @@ -719,7 +719,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 > +#ifdef CONFIG_HAS_NS16550_PCI > struct ns16550 *uart = port->uart; > > if ( uart->msi ) > @@ -761,7 +761,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 > +#ifdef CONFIG_HAS_NS16550_PCI > if ( uart->bar || uart->ps_bdf_enable ) > { > if ( uart->param && uart->param->mmio && > @@ -841,7 +841,7 @@ static void ns16550_suspend(struct serial_port *port) > > stop_timer(&uart->timer); > > -#ifdef CONFIG_HAS_PCI > +#ifdef CONFIG_HAS_NS16550_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); > @@ -850,7 +850,7 @@ static void ns16550_suspend(struct serial_port *port) > > static void _ns16550_resume(struct serial_port *port) > { > -#ifdef CONFIG_HAS_PCI > +#ifdef CONFIG_HAS_NS16550_PCI > struct ns16550 *uart = port->uart; > > if ( uart->bar ) > @@ -1013,7 +1013,7 @@ static int __init check_existence(struct ns16550 *uart) > return 1; /* Everything is MMIO */ > #endif > > -#ifdef CONFIG_HAS_PCI > +#ifdef CONFIG_HAS_NS16550_PCI > pci_serial_early_init(uart); > #endif > > @@ -1044,7 +1044,7 @@ static int __init check_existence(struct ns16550 *uart) > return (status == 0x90); > } > > -#ifdef CONFIG_HAS_PCI > +#ifdef CONFIG_HAS_NS16550_PCI > static int __init > pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx) > { > @@ -1305,7 +1305,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str) > > if ( *conf == ',' && *++conf != ',' ) > { > -#ifdef CONFIG_HAS_PCI > +#ifdef CONFIG_HAS_NS16550_PCI > if ( strncmp(conf, "pci", 3) == 0 ) > { > if ( pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) ) > @@ -1327,7 +1327,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str) > > if ( *conf == ',' && *++conf != ',' ) > { > -#ifdef CONFIG_HAS_PCI > +#ifdef CONFIG_HAS_NS16550_PCI > if ( strncmp(conf, "msi", 3) == 0 ) > { > conf += 3; > @@ -1339,7 +1339,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str) > uart->irq = simple_strtol(conf, &conf, 10); > } > > -#ifdef CONFIG_HAS_PCI > +#ifdef CONFIG_HAS_NS16550_PCI > if ( *conf == ',' && *++conf != ',' ) > { > conf = parse_pci(conf, NULL, &uart->ps_bdf[0], > @@ -1419,7 +1419,7 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart) > uart->reg_width = simple_strtoul(param_value, NULL, 0); > break; > > -#ifdef CONFIG_HAS_PCI > +#ifdef CONFIG_HAS_NS16550_PCI > case bridge_bdf: > if ( !parse_pci(param_value, NULL, &uart->ps_bdf[0], > &uart->ps_bdf[1], &uart->ps_bdf[2]) ) > -- > 2.17.1 >
On 16.11.2020 13: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. While you've extended the sentence, it remains unclear to me what compilation error it is that results here. I've requested such clarification for v2 already. > --- a/xen/drivers/char/Kconfig > +++ b/xen/drivers/char/Kconfig > @@ -4,6 +4,10 @@ config HAS_NS16550 > help > This selects the 16550-series UART support. For most systems, say Y. > > +config HAS_NS16550_PCI > + def_bool y > + depends on X86 && HAS_NS16550 && HAS_PCI Looking at this again (in particular at all the #ifdef changes in the actual source file), I wonder whether an approach with less code churn and without such an extra Kconfig setting (with, as said, a bogus dependency on x86) couldn't be found. For example, how about ... > --- 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 > +#ifdef CONFIG_HAS_NS16550_PCI > #include <xen/pci.h> > #include <xen/pci_regs.h> > #include <xen/pci_ids.h> ... #undef-ining CONFIG_HAS_PCI at a suitable position in this file (e.g. after all #include-s, to make sure all structure layouts remain correct)? This would then be far easier to revert down the road, and would confine the oddity to a single file (and there a single place) in the code base. Jan
Hello Jan, > On 17 Nov 2020, at 10:55 am, Jan Beulich <jbeulich@suse.com> wrote: > > On 16.11.2020 13: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. > > While you've extended the sentence, it remains unclear to me what > compilation error it is that results here. I've requested such > clarification for v2 already. Compilation error is related to the code that refer to x86 functions (create_irq()..) and MSI implementation related error. For more details please find the attached file for compilation error. > >> --- a/xen/drivers/char/Kconfig >> +++ b/xen/drivers/char/Kconfig >> @@ -4,6 +4,10 @@ config HAS_NS16550 >> help >> This selects the 16550-series UART support. For most systems, say Y. >> >> +config HAS_NS16550_PCI >> + def_bool y >> + depends on X86 && HAS_NS16550 && HAS_PCI > > Looking at this again (in particular at all the #ifdef changes in > the actual source file), I wonder whether an approach with less > code churn and without such an extra Kconfig setting (with, as > said, a bogus dependency on x86) couldn't be found. For example, > how about ... > >> --- 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 >> +#ifdef CONFIG_HAS_NS16550_PCI >> #include <xen/pci.h> >> #include <xen/pci_regs.h> >> #include <xen/pci_ids.h> > > ... #undef-ining CONFIG_HAS_PCI at a suitable position in this > file (e.g. after all #include-s, to make sure all structure > layouts remain correct)? This would then be far easier to revert > down the road, and would confine the oddity to a single file > (and there a single place) in the code base. > As for ARM platforms, PCI implementation is in the development process and I am not sure if after completion of PCI work, ns16500 PCI part of code will work out of the box. I think there is some effort required to test the ns16550 PCI part of the code on ARM. As this code is tested on X86 only so I make the options depends on X86 and enable it by default for x86. I feel that adding a new Kconfig options is ok to enable/disable the PCI NS16550 support as compared to #undef CONFIG_HAS_PCI in the particular file. If in future other architecture wants to implement the PCI they will face the similar compilation error issues. Please suggest how we can proceed on this. > Jan Regards, Rahul
On 18.11.2020 16:02, Rahul Singh wrote: > Hello Jan, > >> On 17 Nov 2020, at 10:55 am, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 16.11.2020 13: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. >> >> While you've extended the sentence, it remains unclear to me what >> compilation error it is that results here. I've requested such >> clarification for v2 already. > > Compilation error is related to the code that refer to x86 functions (create_irq()..) and MSI implementation related error. > For more details please find the attached file for compilation error. The use of mmio_ro_ranges is almost quite possibly going to remain x86-specific, but then I guess this wants abstracting in a suitable way. The remaining look to all be MSI-related, so perhaps what you want to avoid is just that part rather than everything PCI-ish? >>> --- 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 >>> +#ifdef CONFIG_HAS_NS16550_PCI >>> #include <xen/pci.h> >>> #include <xen/pci_regs.h> >>> #include <xen/pci_ids.h> >> >> ... #undef-ining CONFIG_HAS_PCI at a suitable position in this >> file (e.g. after all #include-s, to make sure all structure >> layouts remain correct)? This would then be far easier to revert >> down the road, and would confine the oddity to a single file >> (and there a single place) in the code base. >> > > As for ARM platforms, PCI implementation is in the development process and I am not sure if after completion of PCI work, ns16500 PCI part of code will work out of the box. I think there is some effort required to test the ns16550 PCI part of the code on ARM. > As this code is tested on X86 only so I make the options depends on X86 and enable it by default for x86. > > I feel that adding a new Kconfig options is ok to enable/disable the PCI NS16550 support as compared to #undef CONFIG_HAS_PCI in the particular file. If in future other architecture wants to implement the PCI they will face the similar compilation error issues. > > Please suggest how we can proceed on this. Introduce CONFIG_HAS_PCI_MSI (selected only by x86), if there's no immediate plan to support it on Arm together with the rest of PCI? Jan
Hi, On 18/11/2020 15:16, Jan Beulich wrote: > On 18.11.2020 16:02, Rahul Singh wrote: >> Hello Jan, >> >>> On 17 Nov 2020, at 10:55 am, Jan Beulich <jbeulich@suse.com> wrote: >>> >>> On 16.11.2020 13: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. >>> >>> While you've extended the sentence, it remains unclear to me what >>> compilation error it is that results here. I've requested such >>> clarification for v2 already. >> >> Compilation error is related to the code that refer to x86 functions (create_irq()..) and MSI implementation related error. >> For more details please find the attached file for compilation error. > > The use of mmio_ro_ranges is almost quite possibly going to remain > x86-specific, but then I guess this wants abstracting in a suitable > way. > > The remaining look to all be MSI-related, so perhaps what you want > to avoid is just that part rather than everything PCI-ish? Not really (see more above). > >>>> --- 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 >>>> +#ifdef CONFIG_HAS_NS16550_PCI >>>> #include <xen/pci.h> >>>> #include <xen/pci_regs.h> >>>> #include <xen/pci_ids.h> >>> >>> ... #undef-ining CONFIG_HAS_PCI at a suitable position in this >>> file (e.g. after all #include-s, to make sure all structure >>> layouts remain correct)? This would then be far easier to revert >>> down the road, and would confine the oddity to a single file >>> (and there a single place) in the code base. >>> >> >> As for ARM platforms, PCI implementation is in the development process and I am not sure if after completion of PCI work, ns16500 PCI part of code will work out of the box. I think there is some effort required to test the ns16550 PCI part of the code on ARM. >> As this code is tested on X86 only so I make the options depends on X86 and enable it by default for x86. >> >> I feel that adding a new Kconfig options is ok to enable/disable the PCI NS16550 support as compared to #undef CONFIG_HAS_PCI in the particular file. If in future other architecture wants to implement the PCI they will face the similar compilation error issues. >> >> Please suggest how we can proceed on this. > > Introduce CONFIG_HAS_PCI_MSI (selected only by x86), if there's no > immediate plan to support it on Arm together with the rest of PCI? So even we are going to enable PCI on Arm and fix compilation issue, there are no way the NS16550 PCI would be usuable without effort for a few reasons: 1) com1/com2 is x86 specific 2) ns16550_init() is not used by Arm and the only way to use a PCI UART 3) UART is discovered through the device-tree/ACPI tables on Arm So I think CONFIG_HAS_NS16550_PCI is most suitable solution and we should probably guard more code (e.g. ns16550_init(), com1, com2...). Note that's not a request for doing it in this patch :). Cheers,
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@arm.com> [julieng: Commit message] Signed-off-by: Julien Grall <jgrall@amazon.com> " Cheers,
On 18.11.2020 16:35, Julien Grall wrote: > So even we are going to enable PCI on Arm and fix compilation issue, > there are no way the NS16550 PCI would be usuable without effort for a > few reasons: > > 1) com1/com2 is x86 specific > 2) ns16550_init() is not used by Arm and the only way to use a PCI UART This is a good observation, and I wasn't aware of this. I'm sending a patch which ... > 3) UART is discovered through the device-tree/ACPI tables on Arm > > So I think CONFIG_HAS_NS16550_PCI is most suitable solution and we > should probably guard more code (e.g. ns16550_init(), com1, com2...). ... hopefully fulfills this (to be considered a prereq to Rahul's series then). Jan
On 18.11.2020 16:35, Julien Grall wrote: > On 18/11/2020 15:16, Jan Beulich wrote: >> On 18.11.2020 16:02, Rahul Singh wrote: >>> Hello Jan, >>> >>>> On 17 Nov 2020, at 10:55 am, Jan Beulich <jbeulich@suse.com> wrote: >>>> >>>> On 16.11.2020 13: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. >>>> >>>> While you've extended the sentence, it remains unclear to me what >>>> compilation error it is that results here. I've requested such >>>> clarification for v2 already. >>> >>> Compilation error is related to the code that refer to x86 functions (create_irq()..) and MSI implementation related error. >>> For more details please find the attached file for compilation error. >> >> The use of mmio_ro_ranges is almost quite possibly going to remain >> x86-specific, but then I guess this wants abstracting in a suitable >> way. >> >> The remaining look to all be MSI-related, so perhaps what you want >> to avoid is just that part rather than everything PCI-ish? > > Not really (see more above). Did you really mean "above", not "below"? If so, I guess I need some clarification. If not, I suppose I've addressed your concern by the 2-patch series I've just sent. Jan
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@arm.com> > [julieng: Commit message] > Signed-off-by: Julien Grall <jgrall@amazon.com> > " > > Cheers, >
Hi Jan, On 19/11/2020 09:05, Jan Beulich wrote: > 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. I am yet to see an argument into why we should keep the PCI code compiled on Arm when there will be no-use.... > 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. I would be happy with CONFIG_X86, despite the fact that this is only deferring the problem. Regarding HAS_PCI_MSI, I don't really see the point of introducing given that we are not going to use NS16550 PCI on Arm in the forseeable future. So why do we need a finer graine Kconfig? Cheers,
Hi Jan, On 19/11/2020 09:00, Jan Beulich wrote: > On 18.11.2020 16:35, Julien Grall wrote: >> On 18/11/2020 15:16, Jan Beulich wrote: >>> On 18.11.2020 16:02, Rahul Singh wrote: >>>> Hello Jan, >>>> >>>>> On 17 Nov 2020, at 10:55 am, Jan Beulich <jbeulich@suse.com> wrote: >>>>> >>>>> On 16.11.2020 13: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. >>>>> >>>>> While you've extended the sentence, it remains unclear to me what >>>>> compilation error it is that results here. I've requested such >>>>> clarification for v2 already. >>>> >>>> Compilation error is related to the code that refer to x86 functions (create_irq()..) and MSI implementation related error. >>>> For more details please find the attached file for compilation error. >>> >>> The use of mmio_ro_ranges is almost quite possibly going to remain >>> x86-specific, but then I guess this wants abstracting in a suitable >>> way. >>> >>> The remaining look to all be MSI-related, so perhaps what you want >>> to avoid is just that part rather than everything PCI-ish? >> >> Not really (see more above). > > Did you really mean "above", not "below"? If so, I guess I need some > clarification. If not, I suppose I've addressed your concern by the > 2-patch series I've just sent. This was meant to be "below". Cheers,
On 19.11.2020 10:21, Julien Grall wrote: > Hi Jan, > > On 19/11/2020 09:05, Jan Beulich wrote: >> 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. > > I am yet to see an argument into why we should keep the PCI code > compiled on Arm when there will be no-use.... Well, see my patch suppressing building of quite a part of it. >> 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. > > I would be happy with CONFIG_X86, despite the fact that this is only > deferring the problem. > > Regarding HAS_PCI_MSI, I don't really see the point of introducing given > that we are not going to use NS16550 PCI on Arm in the forseeable > future. And I continue to fail to see what would guarantee this: As soon as you can plug in such a card into an Arm system, people will want to be able use it. That's why we had to add support for it on x86, after all. > So why do we need a finer graine Kconfig? Because most of the involved code is indeed MSI-related? Jan
On 19/11/2020 09:53, Jan Beulich wrote: > On 19.11.2020 10:21, Julien Grall wrote: >> Hi Jan, >> >> On 19/11/2020 09:05, Jan Beulich wrote: >>> 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. >> >> I am yet to see an argument into why we should keep the PCI code >> compiled on Arm when there will be no-use.... > > Well, see my patch suppressing building of quite a part of it. I will let Rahul figuring out whether your patch series is sufficient to fix compilation issues (this is what matters right now). > >>> 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. >> >> I would be happy with CONFIG_X86, despite the fact that this is only >> deferring the problem. >> >> Regarding HAS_PCI_MSI, I don't really see the point of introducing given >> that we are not going to use NS16550 PCI on Arm in the forseeable >> future. > > And I continue to fail to see what would guarantee this: As soon > as you can plug in such a card into an Arm system, people will > want to be able use it. That's why we had to add support for it > on x86, after all. Well, plug-in PCI cards on Arm has been available for quite a while... Yet I haven't heard anyone asking for NS16550 PCI support. This is probably because SBSA compliant server should always provide an SBSA UART (a cut-down version of the PL011). So why would bother to lose a PCI slot for yet another UART? > >> So why do we need a finer graine Kconfig? > > Because most of the involved code is indeed MSI-related? Possibly, yet it would not be necessary if we don't want NS16550 PCI support... Cheers,
Hello, > On 19 Nov 2020, at 10:16 am, Julien Grall <julien@xen.org> wrote: > > > > On 19/11/2020 09:53, Jan Beulich wrote: >> On 19.11.2020 10:21, Julien Grall wrote: >>> Hi Jan, >>> >>> On 19/11/2020 09:05, Jan Beulich wrote: >>>> 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. >>> >>> I am yet to see an argument into why we should keep the PCI code >>> compiled on Arm when there will be no-use.... >> Well, see my patch suppressing building of quite a part of it. > > I will let Rahul figuring out whether your patch series is sufficient to fix compilation issues (this is what matters right now). I just checked the compilation error for ARM after enabling the HAS_PCI on ARM. I am observing the same compilation error what I observed previously. There are two new errors related to struct uart_config and struct part_param as those struct defined globally but used under X86 flags. At top level: ns16550.c:179:48: error: ‘uart_config’ defined but not used [-Werror=unused-const-variable=] static const struct ns16550_config __initconst uart_config[] = ^~~~~~~~~~~ ns16550.c:104:54: error: ‘uart_param’ defined but not used [-Werror=unused-const-variable=] static const struct ns16550_config_param __initconst uart_param[] = { > >>>> 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. >>> >>> I would be happy with CONFIG_X86, despite the fact that this is only >>> deferring the problem. >>> >>> Regarding HAS_PCI_MSI, I don't really see the point of introducing given >>> that we are not going to use NS16550 PCI on Arm in the forseeable >>> future. >> And I continue to fail to see what would guarantee this: As soon >> as you can plug in such a card into an Arm system, people will >> want to be able use it. That's why we had to add support for it >> on x86, after all. > > Well, plug-in PCI cards on Arm has been available for quite a while... Yet I haven't heard anyone asking for NS16550 PCI support. > > This is probably because SBSA compliant server should always provide an SBSA UART (a cut-down version of the PL011). So why would bother to lose a PCI slot for yet another UART? > >> >> So why do we need a finer graine Kconfig? >> Because most of the involved code is indeed MSI-related? > > Possibly, yet it would not be necessary if we don't want NS16550 PCI support... > To fix compilation error on ARM as per the discussion there are below options please suggest which one to use to proceed further. 1. Use the newly introduced CONFIG_HAS_NS16550_PCI config options. This helps also non-x86 architecture in the future not to have compilation error what we are observing now when HAS_PCI is enabled. 2. Guard the remaining x86 specific code with CONFIG_X86 and introduce the new CONFIG_HAS_PCI_MSI options to fix the MSI related compilation error. Once we have proper support for MSI and PCI for ARM (HAS_PCI_MSI and HAS_PCI enabled for ARM in Kconfig ) I am not sure if NS16550 PCI will work out of the box on ARM .In that case, we might need to come back again to fix NS16550 driver. > Cheers, > > -- > Julien Grall Regards, Rahul
On 19.11.2020 16:54, Rahul Singh wrote: >> On 19 Nov 2020, at 10:16 am, Julien Grall <julien@xen.org> wrote: >> On 19/11/2020 09:53, Jan Beulich wrote: >>> Well, see my patch suppressing building of quite a part of it. >> >> I will let Rahul figuring out whether your patch series is sufficient to fix compilation issues (this is what matters right now). > > I just checked the compilation error for ARM after enabling the HAS_PCI on ARM. I am observing the same compilation error what I observed previously. > There are two new errors related to struct uart_config and struct part_param as those struct defined globally but used under X86 flags. > > At top level: > ns16550.c:179:48: error: ‘uart_config’ defined but not used [-Werror=unused-const-variable=] > static const struct ns16550_config __initconst uart_config[] = > ^~~~~~~~~~~ > ns16550.c:104:54: error: ‘uart_param’ defined but not used [-Werror=unused-const-variable=] > static const struct ns16550_config_param __initconst uart_param[] = { Looks like more code movement I need to do in my patch then. I was never happy about the disconnected placement of these array and the functions using them ... Jan
On Thu, 19 Nov 2020, Rahul Singh wrote: > > On 19/11/2020 09:53, Jan Beulich wrote: > >> On 19.11.2020 10:21, Julien Grall wrote: > >>> Hi Jan, > >>> > >>> On 19/11/2020 09:05, Jan Beulich wrote: > >>>> 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. > >>> > >>> I am yet to see an argument into why we should keep the PCI code > >>> compiled on Arm when there will be no-use.... > >> Well, see my patch suppressing building of quite a part of it. > > > > I will let Rahul figuring out whether your patch series is sufficient to fix compilation issues (this is what matters right now). > > I just checked the compilation error for ARM after enabling the HAS_PCI on ARM. I am observing the same compilation error what I observed previously. > There are two new errors related to struct uart_config and struct part_param as those struct defined globally but used under X86 flags. > > At top level: > ns16550.c:179:48: error: ‘uart_config’ defined but not used [-Werror=unused-const-variable=] > static const struct ns16550_config __initconst uart_config[] = > ^~~~~~~~~~~ > ns16550.c:104:54: error: ‘uart_param’ defined but not used [-Werror=unused-const-variable=] > static const struct ns16550_config_param __initconst uart_param[] = { > > > > > >>>> 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. > >>> > >>> I would be happy with CONFIG_X86, despite the fact that this is only > >>> deferring the problem. > >>> > >>> Regarding HAS_PCI_MSI, I don't really see the point of introducing given > >>> that we are not going to use NS16550 PCI on Arm in the forseeable > >>> future. > >> And I continue to fail to see what would guarantee this: As soon > >> as you can plug in such a card into an Arm system, people will > >> want to be able use it. That's why we had to add support for it > >> on x86, after all. > > > > Well, plug-in PCI cards on Arm has been available for quite a while... Yet I haven't heard anyone asking for NS16550 PCI support. > > > > This is probably because SBSA compliant server should always provide an SBSA UART (a cut-down version of the PL011). So why would bother to lose a PCI slot for yet another UART? > > > >> >> So why do we need a finer graine Kconfig? > >> Because most of the involved code is indeed MSI-related? > > > > Possibly, yet it would not be necessary if we don't want NS16550 PCI support... > > To fix compilation error on ARM as per the discussion there are below options please suggest which one to use to proceed further. > > 1. Use the newly introduced CONFIG_HAS_NS16550_PCI config options. This helps also non-x86 architecture in the future not to have compilation error > what we are observing now when HAS_PCI is enabled. > > 2. Guard the remaining x86 specific code with CONFIG_X86 and introduce the new CONFIG_HAS_PCI_MSI options to fix the MSI related compilation error. > Once we have proper support for MSI and PCI for ARM (HAS_PCI_MSI and HAS_PCI enabled for ARM in Kconfig ) I am not sure if NS16550 PCI will work out of the box on ARM .In that case, we might need to come back again to fix NS16550 driver. It doesn't matter too much to me, let's just choose one option so that you get unblocked soon. It looks like Jan prefers option 2) and both Julien and I are OK with it. So let's do 2). Jan, please confirm too :-)
On Thu, 19 Nov 2020, 23:38 Stefano Stabellini, <sstabellini@kernel.org> wrote: > On Thu, 19 Nov 2020, Rahul Singh wrote: > > > On 19/11/2020 09:53, Jan Beulich wrote: > > >> On 19.11.2020 10:21, Julien Grall wrote: > > >>> Hi Jan, > > >>> > > >>> On 19/11/2020 09:05, Jan Beulich wrote: > > >>>> 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. > > >>> > > >>> I am yet to see an argument into why we should keep the PCI code > > >>> compiled on Arm when there will be no-use.... > > >> Well, see my patch suppressing building of quite a part of it. > > > > > > I will let Rahul figuring out whether your patch series is sufficient > to fix compilation issues (this is what matters right now). > > > > I just checked the compilation error for ARM after enabling the HAS_PCI > on ARM. I am observing the same compilation error what I observed > previously. > > There are two new errors related to struct uart_config and struct > part_param as those struct defined globally but used under X86 flags. > > > > At top level: > > ns16550.c:179:48: error: ‘uart_config’ defined but not used > [-Werror=unused-const-variable=] > > static const struct ns16550_config __initconst uart_config[] = > > ^~~~~~~~~~~ > > ns16550.c:104:54: error: ‘uart_param’ defined but not used > [-Werror=unused-const-variable=] > > static const struct ns16550_config_param __initconst uart_param[] = { > > > > > > > > > >>>> 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. > > >>> > > >>> I would be happy with CONFIG_X86, despite the fact that this is only > > >>> deferring the problem. > > >>> > > >>> Regarding HAS_PCI_MSI, I don't really see the point of introducing > given > > >>> that we are not going to use NS16550 PCI on Arm in the forseeable > > >>> future. > > >> And I continue to fail to see what would guarantee this: As soon > > >> as you can plug in such a card into an Arm system, people will > > >> want to be able use it. That's why we had to add support for it > > >> on x86, after all. > > > > > > Well, plug-in PCI cards on Arm has been available for quite a while... > Yet I haven't heard anyone asking for NS16550 PCI support. > > > > > > This is probably because SBSA compliant server should always provide > an SBSA UART (a cut-down version of the PL011). So why would bother to lose > a PCI slot for yet another UART? > > > > > >> >> So why do we need a finer graine Kconfig? > > >> Because most of the involved code is indeed MSI-related? > > > > > > Possibly, yet it would not be necessary if we don't want NS16550 PCI > support... > > > > To fix compilation error on ARM as per the discussion there are below > options please suggest which one to use to proceed further. > > > > 1. Use the newly introduced CONFIG_HAS_NS16550_PCI config options. This > helps also non-x86 architecture in the future not to have compilation error > > what we are observing now when HAS_PCI is enabled. > > > > 2. Guard the remaining x86 specific code with CONFIG_X86 and introduce > the new CONFIG_HAS_PCI_MSI options to fix the MSI related compilation > error. > > Once we have proper support for MSI and PCI for ARM (HAS_PCI_MSI and > HAS_PCI enabled for ARM in Kconfig ) I am not sure if NS16550 PCI will work > out of the box on ARM .In that case, we might need to come back again to > fix NS16550 driver. > > > It doesn't matter too much to me, let's just choose one option so that you > get unblocked soon. > > It looks like Jan prefers option 2) and both Julien and I are OK with > it. So let's do 2). Jan, please confirm too :-) Please don't put words in my mouth... I think introducing HAS_PCI_MSI is short sighted. There are no clear benefits of it when NS16550 PCI support is not going to be enable in the foreseeable future. I would be ok with moving everything under CONFIG_X86. IHMO this is still shortsighted but at least we don't introduce a config that's not going to help Arm or other any architecture to disable completely PCI support in NS16550. Cheers,
On Thu, 19 Nov 2020, Julien Grall wrote: > On Thu, 19 Nov 2020, 23:38 Stefano Stabellini, <sstabellini@kernel.org> wrote: > On Thu, 19 Nov 2020, Rahul Singh wrote: > > > On 19/11/2020 09:53, Jan Beulich wrote: > > >> On 19.11.2020 10:21, Julien Grall wrote: > > >>> Hi Jan, > > >>> > > >>> On 19/11/2020 09:05, Jan Beulich wrote: > > >>>> 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. > > >>> > > >>> I am yet to see an argument into why we should keep the PCI code > > >>> compiled on Arm when there will be no-use.... > > >> Well, see my patch suppressing building of quite a part of it. > > > > > > I will let Rahul figuring out whether your patch series is sufficient to fix compilation issues (this is what matters right > now). > > > > I just checked the compilation error for ARM after enabling the HAS_PCI on ARM. I am observing the same compilation error > what I observed previously. > > There are two new errors related to struct uart_config and struct part_param as those struct defined globally but used under > X86 flags. > > > > At top level: > > ns16550.c:179:48: error: ‘uart_config’ defined but not used [-Werror=unused-const-variable=] > > static const struct ns16550_config __initconst uart_config[] = > > ^~~~~~~~~~~ > > ns16550.c:104:54: error: ‘uart_param’ defined but not used [-Werror=unused-const-variable=] > > static const struct ns16550_config_param __initconst uart_param[] = { > > > > > > > > > >>>> 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. > > >>> > > >>> I would be happy with CONFIG_X86, despite the fact that this is only > > >>> deferring the problem. > > >>> > > >>> Regarding HAS_PCI_MSI, I don't really see the point of introducing given > > >>> that we are not going to use NS16550 PCI on Arm in the forseeable > > >>> future. > > >> And I continue to fail to see what would guarantee this: As soon > > >> as you can plug in such a card into an Arm system, people will > > >> want to be able use it. That's why we had to add support for it > > >> on x86, after all. > > > > > > Well, plug-in PCI cards on Arm has been available for quite a while... Yet I haven't heard anyone asking for NS16550 PCI > support. > > > > > > This is probably because SBSA compliant server should always provide an SBSA UART (a cut-down version of the PL011). So why > would bother to lose a PCI slot for yet another UART? > > > > > >> >> So why do we need a finer graine Kconfig? > > >> Because most of the involved code is indeed MSI-related? > > > > > > Possibly, yet it would not be necessary if we don't want NS16550 PCI support... > > > > To fix compilation error on ARM as per the discussion there are below options please suggest which one to use to proceed > further. > > > > 1. Use the newly introduced CONFIG_HAS_NS16550_PCI config options. This helps also non-x86 architecture in the future not to > have compilation error > > what we are observing now when HAS_PCI is enabled. > > > > 2. Guard the remaining x86 specific code with CONFIG_X86 and introduce the new CONFIG_HAS_PCI_MSI options to fix the MSI > related compilation error. > > Once we have proper support for MSI and PCI for ARM (HAS_PCI_MSI and HAS_PCI enabled for ARM in Kconfig ) I am not sure if > NS16550 PCI will work out of the box on ARM .In that case, we might need to come back again to fix NS16550 driver. > > > It doesn't matter too much to me, let's just choose one option so that you > get unblocked soon. > > It looks like Jan prefers option 2) and both Julien and I are OK with > it. So let's do 2). Jan, please confirm too :-) > > > Please don't put words in my mouth... Sorry Julien, I misinterpreted one of your previous comments. Sometimes it is difficult to do things by email. It is good that you clarified as my goal was to reach an agreement. > I think introducing HAS_PCI_MSI is short sighted. > > There are no clear benefits of it when NS16550 PCI support is not going to be enable in the foreseeable future. I agree > I would be ok with moving everything under CONFIG_X86. IHMO this is still shortsighted but at least we don't introduce a config that's not > going to help Arm or other any architecture to disable completely PCI support in NS16550. So you are suggesting a new option: 3. Guard the remaining x86 specific code *and* the MSI related compilation errors with CONFIG_X86 Is that right? My preference is actually option 1) but this series is already at v3 and I don't think this decision is as important as much as unblocking Rahul, so I am OK with the other alternatives too. I tend to agree with you that 3) is better than 2) for the reasons you wrote above.
Hello Jan, > On 20 Nov 2020, at 12:14 am, Stefano Stabellini <sstabellini@kernel.org> wrote: > > On Thu, 19 Nov 2020, Julien Grall wrote: >> On Thu, 19 Nov 2020, 23:38 Stefano Stabellini, <sstabellini@kernel.org> wrote: >> On Thu, 19 Nov 2020, Rahul Singh wrote: >>>> On 19/11/2020 09:53, Jan Beulich wrote: >>>>> On 19.11.2020 10:21, Julien Grall wrote: >>>>>> Hi Jan, >>>>>> >>>>>> On 19/11/2020 09:05, Jan Beulich wrote: >>>>>>> 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. >>>>>> >>>>>> I am yet to see an argument into why we should keep the PCI code >>>>>> compiled on Arm when there will be no-use.... >>>>> Well, see my patch suppressing building of quite a part of it. >>>> >>>> I will let Rahul figuring out whether your patch series is sufficient to fix compilation issues (this is what matters right >> now). >>> >>> I just checked the compilation error for ARM after enabling the HAS_PCI on ARM. I am observing the same compilation error >> what I observed previously. >>> There are two new errors related to struct uart_config and struct part_param as those struct defined globally but used under >> X86 flags. >>> >>> At top level: >>> ns16550.c:179:48: error: ‘uart_config’ defined but not used [-Werror=unused-const-variable=] >>> static const struct ns16550_config __initconst uart_config[] = >>> ^~~~~~~~~~~ >>> ns16550.c:104:54: error: ‘uart_param’ defined but not used [-Werror=unused-const-variable=] >>> static const struct ns16550_config_param __initconst uart_param[] = { >>> >>> >>>> >>>>>>> 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. >>>>>> >>>>>> I would be happy with CONFIG_X86, despite the fact that this is only >>>>>> deferring the problem. >>>>>> >>>>>> Regarding HAS_PCI_MSI, I don't really see the point of introducing given >>>>>> that we are not going to use NS16550 PCI on Arm in the forseeable >>>>>> future. >>>>> And I continue to fail to see what would guarantee this: As soon >>>>> as you can plug in such a card into an Arm system, people will >>>>> want to be able use it. That's why we had to add support for it >>>>> on x86, after all. >>>> >>>> Well, plug-in PCI cards on Arm has been available for quite a while... Yet I haven't heard anyone asking for NS16550 PCI >> support. >>>> >>>> This is probably because SBSA compliant server should always provide an SBSA UART (a cut-down version of the PL011). So why >> would bother to lose a PCI slot for yet another UART? >>>> >>>>>>> So why do we need a finer graine Kconfig? >>>>> Because most of the involved code is indeed MSI-related? >>>> >>>> Possibly, yet it would not be necessary if we don't want NS16550 PCI support... >>> >>> To fix compilation error on ARM as per the discussion there are below options please suggest which one to use to proceed >> further. >>> >>> 1. Use the newly introduced CONFIG_HAS_NS16550_PCI config options. This helps also non-x86 architecture in the future not to >> have compilation error >>> what we are observing now when HAS_PCI is enabled. >>> >>> 2. Guard the remaining x86 specific code with CONFIG_X86 and introduce the new CONFIG_HAS_PCI_MSI options to fix the MSI >> related compilation error. >>> Once we have proper support for MSI and PCI for ARM (HAS_PCI_MSI and HAS_PCI enabled for ARM in Kconfig ) I am not sure if >> NS16550 PCI will work out of the box on ARM .In that case, we might need to come back again to fix NS16550 driver. >> >> >> It doesn't matter too much to me, let's just choose one option so that you >> get unblocked soon. >> >> It looks like Jan prefers option 2) and both Julien and I are OK with >> it. So let's do 2). Jan, please confirm too :-) >> >> >> Please don't put words in my mouth... > > Sorry Julien, I misinterpreted one of your previous comments. Sometimes > it is difficult to do things by email. It is good that you clarified as > my goal was to reach an agreement. > > >> I think introducing HAS_PCI_MSI is short sighted. >> >> There are no clear benefits of it when NS16550 PCI support is not going to be enable in the foreseeable future. > > I agree > > >> I would be ok with moving everything under CONFIG_X86. IHMO this is still shortsighted but at least we don't introduce a config that's not >> going to help Arm or other any architecture to disable completely PCI support in NS16550. > > So you are suggesting a new option: > > 3. Guard the remaining x86 specific code *and* the MSI related > compilation errors with CONFIG_X86 > > Is that right? > > > My preference is actually option 1) but this series is already at v3 and > I don't think this decision is as important as much as unblocking > Rahul, so I am OK with the other alternatives too. > > I tend to agree with you that 3) is better than 2) for the reasons you > wrote above. Can you please provide your suggestion how to proceed on this so that I can send my next patch. I am waiting for your reply if you are also ok for the options 3. Thanks in advance. Regards, Rahul
Rahul, On 23.11.2020 12:54, Rahul Singh wrote: > Hello Jan, as an aside - it helps if you also put the addressee of your mail on the To list. >> On 20 Nov 2020, at 12:14 am, Stefano Stabellini <sstabellini@kernel.org> wrote: >> >> On Thu, 19 Nov 2020, Julien Grall wrote: >>> On Thu, 19 Nov 2020, 23:38 Stefano Stabellini, <sstabellini@kernel.org> wrote: >>> On Thu, 19 Nov 2020, Rahul Singh wrote: >>>>> On 19/11/2020 09:53, Jan Beulich wrote: >>>>>> On 19.11.2020 10:21, Julien Grall wrote: >>>>>>> Hi Jan, >>>>>>> >>>>>>> On 19/11/2020 09:05, Jan Beulich wrote: >>>>>>>> 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. >>>>>>> >>>>>>> I am yet to see an argument into why we should keep the PCI code >>>>>>> compiled on Arm when there will be no-use.... >>>>>> Well, see my patch suppressing building of quite a part of it. >>>>> >>>>> I will let Rahul figuring out whether your patch series is sufficient to fix compilation issues (this is what matters right >>> now). >>>> >>>> I just checked the compilation error for ARM after enabling the HAS_PCI on ARM. I am observing the same compilation error >>> what I observed previously. >>>> There are two new errors related to struct uart_config and struct part_param as those struct defined globally but used under >>> X86 flags. >>>> >>>> At top level: >>>> ns16550.c:179:48: error: ‘uart_config’ defined but not used [-Werror=unused-const-variable=] >>>> static const struct ns16550_config __initconst uart_config[] = >>>> ^~~~~~~~~~~ >>>> ns16550.c:104:54: error: ‘uart_param’ defined but not used [-Werror=unused-const-variable=] >>>> static const struct ns16550_config_param __initconst uart_param[] = { >>>> >>>> >>>>> >>>>>>>> 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. >>>>>>> >>>>>>> I would be happy with CONFIG_X86, despite the fact that this is only >>>>>>> deferring the problem. >>>>>>> >>>>>>> Regarding HAS_PCI_MSI, I don't really see the point of introducing given >>>>>>> that we are not going to use NS16550 PCI on Arm in the forseeable >>>>>>> future. >>>>>> And I continue to fail to see what would guarantee this: As soon >>>>>> as you can plug in such a card into an Arm system, people will >>>>>> want to be able use it. That's why we had to add support for it >>>>>> on x86, after all. >>>>> >>>>> Well, plug-in PCI cards on Arm has been available for quite a while... Yet I haven't heard anyone asking for NS16550 PCI >>> support. >>>>> >>>>> This is probably because SBSA compliant server should always provide an SBSA UART (a cut-down version of the PL011). So why >>> would bother to lose a PCI slot for yet another UART? >>>>> >>>>>>>> So why do we need a finer graine Kconfig? >>>>>> Because most of the involved code is indeed MSI-related? >>>>> >>>>> Possibly, yet it would not be necessary if we don't want NS16550 PCI support... >>>> >>>> To fix compilation error on ARM as per the discussion there are below options please suggest which one to use to proceed >>> further. >>>> >>>> 1. Use the newly introduced CONFIG_HAS_NS16550_PCI config options. This helps also non-x86 architecture in the future not to >>> have compilation error >>>> what we are observing now when HAS_PCI is enabled. >>>> >>>> 2. Guard the remaining x86 specific code with CONFIG_X86 and introduce the new CONFIG_HAS_PCI_MSI options to fix the MSI >>> related compilation error. >>>> Once we have proper support for MSI and PCI for ARM (HAS_PCI_MSI and HAS_PCI enabled for ARM in Kconfig ) I am not sure if >>> NS16550 PCI will work out of the box on ARM .In that case, we might need to come back again to fix NS16550 driver. >>> >>> >>> It doesn't matter too much to me, let's just choose one option so that you >>> get unblocked soon. >>> >>> It looks like Jan prefers option 2) and both Julien and I are OK with >>> it. So let's do 2). Jan, please confirm too :-) >>> >>> >>> Please don't put words in my mouth... >> >> Sorry Julien, I misinterpreted one of your previous comments. Sometimes >> it is difficult to do things by email. It is good that you clarified as >> my goal was to reach an agreement. >> >> >>> I think introducing HAS_PCI_MSI is short sighted. >>> >>> There are no clear benefits of it when NS16550 PCI support is not going to be enable in the foreseeable future. >> >> I agree >> >> >>> I would be ok with moving everything under CONFIG_X86. IHMO this is still shortsighted but at least we don't introduce a config that's not >>> going to help Arm or other any architecture to disable completely PCI support in NS16550. >> >> So you are suggesting a new option: >> >> 3. Guard the remaining x86 specific code *and* the MSI related >> compilation errors with CONFIG_X86 >> >> Is that right? >> >> >> My preference is actually option 1) but this series is already at v3 and >> I don't think this decision is as important as much as unblocking >> Rahul, so I am OK with the other alternatives too. >> >> I tend to agree with you that 3) is better than 2) for the reasons you >> wrote above. > > > Can you please provide your suggestion how to proceed on this so that I can send my next patch. > I am waiting for your reply if you are also ok for the options 3. I can live with 3, I guess, but I still think a separate PCI_MSI control would be better. Please realize though that things also depend on how the change is going to look like in the end, i.e. I'm not going to assure you this is my final view on it. In any event I've just sent v2 of my series, which I consider a prereq of yours. Jan
Hi Stefano, On 20/11/2020 00:14, Stefano Stabellini wrote: > On Thu, 19 Nov 2020, Julien Grall wrote: >> On Thu, 19 Nov 2020, 23:38 Stefano Stabellini, <sstabellini@kernel.org> wrote: >> On Thu, 19 Nov 2020, Rahul Singh wrote: >> > > On 19/11/2020 09:53, Jan Beulich wrote: >> > >> On 19.11.2020 10:21, Julien Grall wrote: >> > >>> Hi Jan, >> > >>> >> > >>> On 19/11/2020 09:05, Jan Beulich wrote: >> > >>>> 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. >> > >>> >> > >>> I am yet to see an argument into why we should keep the PCI code >> > >>> compiled on Arm when there will be no-use.... >> > >> Well, see my patch suppressing building of quite a part of it. >> > > >> > > I will let Rahul figuring out whether your patch series is sufficient to fix compilation issues (this is what matters right >> now). >> > >> > I just checked the compilation error for ARM after enabling the HAS_PCI on ARM. I am observing the same compilation error >> what I observed previously. >> > There are two new errors related to struct uart_config and struct part_param as those struct defined globally but used under >> X86 flags. >> > >> > At top level: >> > ns16550.c:179:48: error: ‘uart_config’ defined but not used [-Werror=unused-const-variable=] >> > static const struct ns16550_config __initconst uart_config[] = >> > ^~~~~~~~~~~ >> > ns16550.c:104:54: error: ‘uart_param’ defined but not used [-Werror=unused-const-variable=] >> > static const struct ns16550_config_param __initconst uart_param[] = { >> > >> > >> > > >> > >>>> 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. >> > >>> >> > >>> I would be happy with CONFIG_X86, despite the fact that this is only >> > >>> deferring the problem. >> > >>> >> > >>> Regarding HAS_PCI_MSI, I don't really see the point of introducing given >> > >>> that we are not going to use NS16550 PCI on Arm in the forseeable >> > >>> future. >> > >> And I continue to fail to see what would guarantee this: As soon >> > >> as you can plug in such a card into an Arm system, people will >> > >> want to be able use it. That's why we had to add support for it >> > >> on x86, after all. >> > > >> > > Well, plug-in PCI cards on Arm has been available for quite a while... Yet I haven't heard anyone asking for NS16550 PCI >> support. >> > > >> > > This is probably because SBSA compliant server should always provide an SBSA UART (a cut-down version of the PL011). So why >> would bother to lose a PCI slot for yet another UART? >> > > >> > >> >> So why do we need a finer graine Kconfig? >> > >> Because most of the involved code is indeed MSI-related? >> > > >> > > Possibly, yet it would not be necessary if we don't want NS16550 PCI support... >> > >> > To fix compilation error on ARM as per the discussion there are below options please suggest which one to use to proceed >> further. >> > >> > 1. Use the newly introduced CONFIG_HAS_NS16550_PCI config options. This helps also non-x86 architecture in the future not to >> have compilation error >> > what we are observing now when HAS_PCI is enabled. >> > >> > 2. Guard the remaining x86 specific code with CONFIG_X86 and introduce the new CONFIG_HAS_PCI_MSI options to fix the MSI >> related compilation error. >> > Once we have proper support for MSI and PCI for ARM (HAS_PCI_MSI and HAS_PCI enabled for ARM in Kconfig ) I am not sure if >> NS16550 PCI will work out of the box on ARM .In that case, we might need to come back again to fix NS16550 driver. >> >> >> It doesn't matter too much to me, let's just choose one option so that you >> get unblocked soon. >> >> It looks like Jan prefers option 2) and both Julien and I are OK with >> it. So let's do 2). Jan, please confirm too :-) >> >> >> Please don't put words in my mouth... > > Sorry Julien, I misinterpreted one of your previous comments. Sometimes > it is difficult to do things by email. It is good that you clarified as > my goal was to reach an agreement. No worries. I would like to apologies for been harsher than I would have wanted on my reply. The thread had a lot of back and forth so far. > > >> I think introducing HAS_PCI_MSI is short sighted. >> >> There are no clear benefits of it when NS16550 PCI support is not going to be enable in the foreseeable future. > > I agree > > >> I would be ok with moving everything under CONFIG_X86. IHMO this is still shortsighted but at least we don't introduce a config that's not >> going to help Arm or other any architecture to disable completely PCI support in NS16550. > > So you are suggesting a new option: > > 3. Guard the remaining x86 specific code *and* the MSI related > compilation errors with CONFIG_X86 > > Is that right? That's correct. > My preference is actually option 1) but this series is already at v3 and > I don't think this decision is as important as much as unblocking > Rahul, so I am OK with the other alternatives too. In order, my preferences are 1) 3) 2). AFAICT... > > I tend to agree with you that 3) is better than 2) for the reasons you > wrote above. ... this is the same order as you. Although I probably have a stronger dislike for 2) because I feel it is pushed for the wrong reasons (e.g. matter of taste) so far. My view on 2) can change if Jan provides enough information into why one would want NS1650 PCI enabled by default on Arm but disable MSI. Cheers,
On Mon, 23 Nov 2020, Jan Beulich wrote: > Rahul, > > On 23.11.2020 12:54, Rahul Singh wrote: > > Hello Jan, > > as an aside - it helps if you also put the addressee of your mail > on the To list. > > >> On 20 Nov 2020, at 12:14 am, Stefano Stabellini <sstabellini@kernel.org> wrote: > >> > >> On Thu, 19 Nov 2020, Julien Grall wrote: > >>> On Thu, 19 Nov 2020, 23:38 Stefano Stabellini, <sstabellini@kernel.org> wrote: > >>> On Thu, 19 Nov 2020, Rahul Singh wrote: > >>>>> On 19/11/2020 09:53, Jan Beulich wrote: > >>>>>> On 19.11.2020 10:21, Julien Grall wrote: > >>>>>>> Hi Jan, > >>>>>>> > >>>>>>> On 19/11/2020 09:05, Jan Beulich wrote: > >>>>>>>> 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. > >>>>>>> > >>>>>>> I am yet to see an argument into why we should keep the PCI code > >>>>>>> compiled on Arm when there will be no-use.... > >>>>>> Well, see my patch suppressing building of quite a part of it. > >>>>> > >>>>> I will let Rahul figuring out whether your patch series is sufficient to fix compilation issues (this is what matters right > >>> now). > >>>> > >>>> I just checked the compilation error for ARM after enabling the HAS_PCI on ARM. I am observing the same compilation error > >>> what I observed previously. > >>>> There are two new errors related to struct uart_config and struct part_param as those struct defined globally but used under > >>> X86 flags. > >>>> > >>>> At top level: > >>>> ns16550.c:179:48: error: ‘uart_config’ defined but not used [-Werror=unused-const-variable=] > >>>> static const struct ns16550_config __initconst uart_config[] = > >>>> ^~~~~~~~~~~ > >>>> ns16550.c:104:54: error: ‘uart_param’ defined but not used [-Werror=unused-const-variable=] > >>>> static const struct ns16550_config_param __initconst uart_param[] = { > >>>> > >>>> > >>>>> > >>>>>>>> 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. > >>>>>>> > >>>>>>> I would be happy with CONFIG_X86, despite the fact that this is only > >>>>>>> deferring the problem. > >>>>>>> > >>>>>>> Regarding HAS_PCI_MSI, I don't really see the point of introducing given > >>>>>>> that we are not going to use NS16550 PCI on Arm in the forseeable > >>>>>>> future. > >>>>>> And I continue to fail to see what would guarantee this: As soon > >>>>>> as you can plug in such a card into an Arm system, people will > >>>>>> want to be able use it. That's why we had to add support for it > >>>>>> on x86, after all. > >>>>> > >>>>> Well, plug-in PCI cards on Arm has been available for quite a while... Yet I haven't heard anyone asking for NS16550 PCI > >>> support. > >>>>> > >>>>> This is probably because SBSA compliant server should always provide an SBSA UART (a cut-down version of the PL011). So why > >>> would bother to lose a PCI slot for yet another UART? > >>>>> > >>>>>>>> So why do we need a finer graine Kconfig? > >>>>>> Because most of the involved code is indeed MSI-related? > >>>>> > >>>>> Possibly, yet it would not be necessary if we don't want NS16550 PCI support... > >>>> > >>>> To fix compilation error on ARM as per the discussion there are below options please suggest which one to use to proceed > >>> further. > >>>> > >>>> 1. Use the newly introduced CONFIG_HAS_NS16550_PCI config options. This helps also non-x86 architecture in the future not to > >>> have compilation error > >>>> what we are observing now when HAS_PCI is enabled. > >>>> > >>>> 2. Guard the remaining x86 specific code with CONFIG_X86 and introduce the new CONFIG_HAS_PCI_MSI options to fix the MSI > >>> related compilation error. > >>>> Once we have proper support for MSI and PCI for ARM (HAS_PCI_MSI and HAS_PCI enabled for ARM in Kconfig ) I am not sure if > >>> NS16550 PCI will work out of the box on ARM .In that case, we might need to come back again to fix NS16550 driver. > >>> > >>> > >>> It doesn't matter too much to me, let's just choose one option so that you > >>> get unblocked soon. > >>> > >>> It looks like Jan prefers option 2) and both Julien and I are OK with > >>> it. So let's do 2). Jan, please confirm too :-) > >>> > >>> > >>> Please don't put words in my mouth... > >> > >> Sorry Julien, I misinterpreted one of your previous comments. Sometimes > >> it is difficult to do things by email. It is good that you clarified as > >> my goal was to reach an agreement. > >> > >> > >>> I think introducing HAS_PCI_MSI is short sighted. > >>> > >>> There are no clear benefits of it when NS16550 PCI support is not going to be enable in the foreseeable future. > >> > >> I agree > >> > >> > >>> I would be ok with moving everything under CONFIG_X86. IHMO this is still shortsighted but at least we don't introduce a config that's not > >>> going to help Arm or other any architecture to disable completely PCI support in NS16550. > >> > >> So you are suggesting a new option: > >> > >> 3. Guard the remaining x86 specific code *and* the MSI related > >> compilation errors with CONFIG_X86 > >> > >> Is that right? > >> > >> > >> My preference is actually option 1) but this series is already at v3 and > >> I don't think this decision is as important as much as unblocking > >> Rahul, so I am OK with the other alternatives too. > >> > >> I tend to agree with you that 3) is better than 2) for the reasons you > >> wrote above. > > > > > > Can you please provide your suggestion how to proceed on this so that I can send my next patch. > > I am waiting for your reply if you are also ok for the options 3. > > I can live with 3, I guess, but I still think a separate PCI_MSI > control would be better. Please realize though that things also > depend on how the change is going to look like in the end, i.e. > I'm not going to assure you this is my final view on it. In any > event I've just sent v2 of my series, which I consider a prereq > of yours. It is great that we have a way forward. I'll try to have a look at your series -- it looks pretty straightforward.
On 23.11.2020 18:13, Julien Grall wrote: > My view on 2) can change if Jan provides enough information into why one > would want NS1650 PCI enabled by default on Arm but disable MSI. Because, like it was on x86, initially there may be no support for MSI? I have no idea what the plans are ... Jan
Hi Jan, On 24/11/2020 09:47, Jan Beulich wrote: > On 23.11.2020 18:13, Julien Grall wrote: >> My view on 2) can change if Jan provides enough information into why one >> would want NS1650 PCI enabled by default on Arm but disable MSI. > > Because, like it was on x86, initially there may be no support for > MSI? "no support for MSI" implies that there will be at least support for NS16550 PCI. > I have no idea what the plans are ... There are no such plan on Arm for the forseeable future (read as we haven't seen any interested from the Arm community). The NS16550 PCI code will stay unusable until someone effectively send a patch to plumb it correctly. While I agree that disabling MSI may be nice to have to do in the future, this doesn't address the need for Arm. I don't want to get in our way the NS16550 PCI code in our way when implementing PCI (with or without MSI) on Arm. Even if there were an interest, I would still expect some users (e.g. embedded folks) to want to compile-out unused feature (you may have a platform with a embedded NS16550). So the path forward will stay either 1) or 3) for me. Cheers,
On 24.11.2020 11:22, Julien Grall wrote: > On 24/11/2020 09:47, Jan Beulich wrote: >> On 23.11.2020 18:13, Julien Grall wrote: >>> My view on 2) can change if Jan provides enough information into why one >>> would want NS1650 PCI enabled by default on Arm but disable MSI. >> >> Because, like it was on x86, initially there may be no support for >> MSI? > > "no support for MSI" implies that there will be at least support for > NS16550 PCI. > >> I have no idea what the plans are ... > > There are no such plan on Arm for the forseeable future (read as we > haven't seen any interested from the Arm community). Okay, so you're question wasn't so much about the "but" in there, but the "PCI" in the first place. > The NS16550 PCI code will stay unusable until someone effectively send a > patch to plumb it correctly. > > While I agree that disabling MSI may be nice to have to do in the > future, this doesn't address the need for Arm. I don't want to get in > our way the NS16550 PCI code in our way when implementing PCI (with or > without MSI) on Arm. > > Even if there were an interest, I would still expect some users (e.g. > embedded folks) to want to compile-out unused feature (you may have a > platform with a embedded NS16550). > > So the path forward will stay either 1) or 3) for me. Well, as said elsewhere - 3) it is then afaic, for making more obvious that this is really a hack. Jan
diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig index b572305657..abb59fdb0f 100644 --- a/xen/drivers/char/Kconfig +++ b/xen/drivers/char/Kconfig @@ -4,6 +4,10 @@ config HAS_NS16550 help This selects the 16550-series UART support. For most systems, say Y. +config HAS_NS16550_PCI + def_bool y + depends on X86 && HAS_NS16550 && HAS_PCI + config HAS_CADENCE_UART bool "Xilinx Cadence UART driver" default y diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c index d8b52eb813..bd1c2af956 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 +#ifdef CONFIG_HAS_NS16550_PCI #include <xen/pci.h> #include <xen/pci_regs.h> #include <xen/pci_ids.h> @@ -54,7 +54,7 @@ enum serial_param_type { reg_shift, reg_width, stop_bits, -#ifdef CONFIG_HAS_PCI +#ifdef CONFIG_HAS_NS16550_PCI bridge_bdf, device, port_bdf, @@ -83,7 +83,7 @@ static struct ns16550 { unsigned int timeout_ms; bool_t intr_works; bool_t dw_usr_bsy; -#ifdef CONFIG_HAS_PCI +#ifdef CONFIG_HAS_NS16550_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 */ @@ -117,14 +117,14 @@ static const struct serial_param_var __initconst sp_vars[] = { {"reg-shift", reg_shift}, {"reg-width", reg_width}, {"stop-bits", stop_bits}, -#ifdef CONFIG_HAS_PCI +#ifdef CONFIG_HAS_NS16550_PCI {"bridge", bridge_bdf}, {"dev", device}, {"port", port_bdf}, #endif }; -#ifdef CONFIG_HAS_PCI +#ifdef CONFIG_HAS_NS16550_PCI struct ns16550_config { u16 vendor_id; u16 dev_id; @@ -620,7 +620,7 @@ static int ns16550_getc(struct serial_port *port, char *pc) static void pci_serial_early_init(struct ns16550 *uart) { -#ifdef CONFIG_HAS_PCI +#ifdef CONFIG_HAS_NS16550_PCI if ( !uart->ps_bdf_enable || uart->io_base >= 0x10000 ) return; @@ -719,7 +719,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 +#ifdef CONFIG_HAS_NS16550_PCI struct ns16550 *uart = port->uart; if ( uart->msi ) @@ -761,7 +761,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 +#ifdef CONFIG_HAS_NS16550_PCI if ( uart->bar || uart->ps_bdf_enable ) { if ( uart->param && uart->param->mmio && @@ -841,7 +841,7 @@ static void ns16550_suspend(struct serial_port *port) stop_timer(&uart->timer); -#ifdef CONFIG_HAS_PCI +#ifdef CONFIG_HAS_NS16550_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); @@ -850,7 +850,7 @@ static void ns16550_suspend(struct serial_port *port) static void _ns16550_resume(struct serial_port *port) { -#ifdef CONFIG_HAS_PCI +#ifdef CONFIG_HAS_NS16550_PCI struct ns16550 *uart = port->uart; if ( uart->bar ) @@ -1013,7 +1013,7 @@ static int __init check_existence(struct ns16550 *uart) return 1; /* Everything is MMIO */ #endif -#ifdef CONFIG_HAS_PCI +#ifdef CONFIG_HAS_NS16550_PCI pci_serial_early_init(uart); #endif @@ -1044,7 +1044,7 @@ static int __init check_existence(struct ns16550 *uart) return (status == 0x90); } -#ifdef CONFIG_HAS_PCI +#ifdef CONFIG_HAS_NS16550_PCI static int __init pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx) { @@ -1305,7 +1305,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str) if ( *conf == ',' && *++conf != ',' ) { -#ifdef CONFIG_HAS_PCI +#ifdef CONFIG_HAS_NS16550_PCI if ( strncmp(conf, "pci", 3) == 0 ) { if ( pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) ) @@ -1327,7 +1327,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str) if ( *conf == ',' && *++conf != ',' ) { -#ifdef CONFIG_HAS_PCI +#ifdef CONFIG_HAS_NS16550_PCI if ( strncmp(conf, "msi", 3) == 0 ) { conf += 3; @@ -1339,7 +1339,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str) uart->irq = simple_strtol(conf, &conf, 10); } -#ifdef CONFIG_HAS_PCI +#ifdef CONFIG_HAS_NS16550_PCI if ( *conf == ',' && *++conf != ',' ) { conf = parse_pci(conf, NULL, &uart->ps_bdf[0], @@ -1419,7 +1419,7 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart) uart->reg_width = simple_strtoul(param_value, NULL, 0); break; -#ifdef CONFIG_HAS_PCI +#ifdef CONFIG_HAS_NS16550_PCI case bridge_bdf: if ( !parse_pci(param_value, NULL, &uart->ps_bdf[0], &uart->ps_bdf[1], &uart->ps_bdf[2]) )
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. Signed-off-by: Rahul Singh <rahul.singh@arm.com> --- Changes in v3: - remove help text from the Kconfig file because of prompt-less option. --- xen/drivers/char/Kconfig | 4 ++++ xen/drivers/char/ns16550.c | 32 ++++++++++++++++---------------- 2 files changed, 20 insertions(+), 16 deletions(-)