diff mbox series

[v1,1/4] xen/ns16550: solve compilation error on ARM with CONFIG_HAS_PCI enabled.

Message ID d1df24d48508c0c01c0b1130ed22f2b4585d04db.1603731279.git.rahul.singh@arm.com (mailing list archive)
State Superseded
Headers show
Series xen/arm: Make PCI passthrough code non-x86 specific | expand

Commit Message

Rahul Singh Oct. 26, 2020, 5:17 p.m. UTC
ARM platforms does not support ns16550 PCI support. When CONFIG_HAS_PCI
is enabled for ARM a compilation error is observed.

Fixed compilation error after introducing new kconfig option
CONFIG_HAS_NS16550_PCI for x86 platforms to support ns16550 PCI.

No functional change.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/drivers/char/Kconfig   |  7 +++++++
 xen/drivers/char/ns16550.c | 32 ++++++++++++++++----------------
 2 files changed, 23 insertions(+), 16 deletions(-)

Comments

Stefano Stabellini Oct. 27, 2020, 11:32 p.m. UTC | #1
On Mon, 26 Oct 2020, Rahul Singh wrote:
> ARM platforms does not support ns16550 PCI support. When CONFIG_HAS_PCI
                ^ do

> is enabled for ARM a compilation error is observed.
> 
> Fixed compilation error after introducing new kconfig option
> CONFIG_HAS_NS16550_PCI for x86 platforms to support ns16550 PCI.
>
> No functional change.

Written like that it would seem that ARM platforms do not support
NS16550 on the PCI bus, but actually, it would be theoretically possible
to have an NS16550 card slotted in a PCI bus on ARM, right?

The problem is the current limitation in terms of Xen internal
plumbings, such as lack of MSI support. Is that correct?

If so, I'd update the commit message to reflect the situation a bit
better.


> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
>  xen/drivers/char/Kconfig   |  7 +++++++
>  xen/drivers/char/ns16550.c | 32 ++++++++++++++++----------------
>  2 files changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
> index b572305657..8887e86afe 100644
> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -4,6 +4,13 @@ config HAS_NS16550
>  	help
>  	  This selects the 16550-series UART support. For most systems, say Y.
>  
> +config HAS_NS16550_PCI
> +	bool "NS16550 UART PCI support" if X86
> +	default y
> +	depends on X86 && HAS_NS16550 && HAS_PCI
> +	help
> +	  This selects the 16550-series UART PCI support. For most systems, say Y.

I think that this should be a silent option:
if HAS_NS16550 && HAS_PCI && X86 -> automatically enable
otherwise -> automatically disable

No need to show it to the user.

The rest of course is fine.


>  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
>
Jan Beulich Oct. 28, 2020, 7:18 a.m. UTC | #2
On 28.10.2020 00:32, Stefano Stabellini wrote:
> On Mon, 26 Oct 2020, Rahul Singh wrote:
>> --- a/xen/drivers/char/Kconfig
>> +++ b/xen/drivers/char/Kconfig
>> @@ -4,6 +4,13 @@ config HAS_NS16550
>>  	help
>>  	  This selects the 16550-series UART support. For most systems, say Y.
>>  
>> +config HAS_NS16550_PCI
>> +	bool "NS16550 UART PCI support" if X86
>> +	default y
>> +	depends on X86 && HAS_NS16550 && HAS_PCI
>> +	help
>> +	  This selects the 16550-series UART PCI support. For most systems, say Y.
> 
> I think that this should be a silent option:
> if HAS_NS16550 && HAS_PCI && X86 -> automatically enable
> otherwise -> automatically disable
> 
> No need to show it to the user.

I agree in principle, but I don't see why an X86 dependency gets
added here. HAS_PCI really should be all that's needed.

Jan
Rahul Singh Oct. 28, 2020, 10:38 a.m. UTC | #3
Hello Stefano,

> On 27 Oct 2020, at 11:32 pm, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Mon, 26 Oct 2020, Rahul Singh wrote:
>> ARM platforms does not support ns16550 PCI support. When CONFIG_HAS_PCI
>                ^ do

Ok I will fix that in next version.
> 
>> is enabled for ARM a compilation error is observed.
>> 
>> Fixed compilation error after introducing new kconfig option
>> CONFIG_HAS_NS16550_PCI for x86 platforms to support ns16550 PCI.
>> 
>> No functional change.
> 
> Written like that it would seem that ARM platforms do not support
> NS16550 on the PCI bus, but actually, it would be theoretically possible
> to have an NS16550 card slotted in a PCI bus on ARM, right?
> 
> The problem is the current limitation in terms of Xen internal
> plumbings, such as lack of MSI support. Is that correct?
> 
> If so, I'd update the commit message to reflect the situation a bit
> better.
> 

Yes you are right on ARM platforms PCI is not fully supported because of that 
I decided to disable it for ARM. Once we have full support for PCI device we can enable 
it for ARM also. I will modify the commit msg to reflect the same.

> 
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> xen/drivers/char/Kconfig   |  7 +++++++
>> xen/drivers/char/ns16550.c | 32 ++++++++++++++++----------------
>> 2 files changed, 23 insertions(+), 16 deletions(-)
>> 
>> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
>> index b572305657..8887e86afe 100644
>> --- a/xen/drivers/char/Kconfig
>> +++ b/xen/drivers/char/Kconfig
>> @@ -4,6 +4,13 @@ config HAS_NS16550
>> 	help
>> 	  This selects the 16550-series UART support. For most systems, say Y.
>> 
>> +config HAS_NS16550_PCI
>> +	bool "NS16550 UART PCI support" if X86
>> +	default y
>> +	depends on X86 && HAS_NS16550 && HAS_PCI
>> +	help
>> +	  This selects the 16550-series UART PCI support. For most systems, say Y.
> 
> I think that this should be a silent option:
> if HAS_NS16550 && HAS_PCI && X86 -> automatically enable
> otherwise -> automatically disable
> 
> No need to show it to the user.
> 
> The rest of course is fine.

Ok I will fix that. I will do the same as done for HAS_NS16550 , 
x86:  silent option depend on HAS_NS16550 && HAS_PCI
ARM: user can decide to enable or disable via menuconfig depend on  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

Regards,
Rahul
Rahul Singh Oct. 28, 2020, 10:41 a.m. UTC | #4
Hello Jan,

> On 28 Oct 2020, at 7:18 am, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 28.10.2020 00:32, Stefano Stabellini wrote:
>> On Mon, 26 Oct 2020, Rahul Singh wrote:
>>> --- a/xen/drivers/char/Kconfig
>>> +++ b/xen/drivers/char/Kconfig
>>> @@ -4,6 +4,13 @@ config HAS_NS16550
>>> 	help
>>> 	  This selects the 16550-series UART support. For most systems, say Y.
>>> 
>>> +config HAS_NS16550_PCI
>>> +	bool "NS16550 UART PCI support" if X86
>>> +	default y
>>> +	depends on X86 && HAS_NS16550 && HAS_PCI
>>> +	help
>>> +	  This selects the 16550-series UART PCI support. For most systems, say Y.
>> 
>> I think that this should be a silent option:
>> if HAS_NS16550 && HAS_PCI && X86 -> automatically enable
>> otherwise -> automatically disable
>> 
>> No need to show it to the user.
> 
> I agree in principle, but I don't see why an X86 dependency gets
> added here. HAS_PCI really should be all that's needed.
> 

Yes you are right . I will remove the X86 and make HAS_NS16550_PCI depend on "HAS_NS16550 && HAS_PCI"
> Jan

Regards,
Rahul
Julien Grall Oct. 28, 2020, 11:32 a.m. UTC | #5
Hi,

On 28/10/2020 10:41, Rahul Singh wrote:
>> On 28 Oct 2020, at 7:18 am, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 28.10.2020 00:32, Stefano Stabellini wrote:
>>> On Mon, 26 Oct 2020, Rahul Singh wrote:
>>>> --- a/xen/drivers/char/Kconfig
>>>> +++ b/xen/drivers/char/Kconfig
>>>> @@ -4,6 +4,13 @@ config HAS_NS16550
>>>> 	help
>>>> 	  This selects the 16550-series UART support. For most systems, say Y.
>>>>
>>>> +config HAS_NS16550_PCI
>>>> +	bool "NS16550 UART PCI support" if X86
>>>> +	default y
>>>> +	depends on X86 && HAS_NS16550 && HAS_PCI
>>>> +	help
>>>> +	  This selects the 16550-series UART PCI support. For most systems, say Y.
>>>
>>> I think that this should be a silent option:
>>> if HAS_NS16550 && HAS_PCI && X86 -> automatically enable
>>> otherwise -> automatically disable
>>>
>>> No need to show it to the user.
>>
>> I agree in principle, but I don't see why an X86 dependency gets
>> added here. HAS_PCI really should be all that's needed.
>>
> 
> Yes you are right . I will remove the X86 and make HAS_NS16550_PCI depend on "HAS_NS16550 && HAS_PCI"

There are quite a bit of work to make the PCI part of the implementation 
build on Arm because the code refer to x86 functions.

While in theory, an NS16550 PCI card could be used on Arm, there is only 
a slim chance for an actual users. So I am not convinced the effort is 
worth it here.

Cheers,
Rahul Singh Oct. 28, 2020, 3:47 p.m. UTC | #6
Hello Julen,

> On 28 Oct 2020, at 11:32 am, Julien Grall <julien@xen.org> wrote:
> 
> Hi,
> 
> On 28/10/2020 10:41, Rahul Singh wrote:
>>> On 28 Oct 2020, at 7:18 am, Jan Beulich <jbeulich@suse.com> wrote:
>>> 
>>> On 28.10.2020 00:32, Stefano Stabellini wrote:
>>>> On Mon, 26 Oct 2020, Rahul Singh wrote:
>>>>> --- a/xen/drivers/char/Kconfig
>>>>> +++ b/xen/drivers/char/Kconfig
>>>>> @@ -4,6 +4,13 @@ config HAS_NS16550
>>>>> 	help
>>>>> 	  This selects the 16550-series UART support. For most systems, say Y.
>>>>> 
>>>>> +config HAS_NS16550_PCI
>>>>> +	bool "NS16550 UART PCI support" if X86
>>>>> +	default y
>>>>> +	depends on X86 && HAS_NS16550 && HAS_PCI
>>>>> +	help
>>>>> +	  This selects the 16550-series UART PCI support. For most systems, say Y.
>>>> 
>>>> I think that this should be a silent option:
>>>> if HAS_NS16550 && HAS_PCI && X86 -> automatically enable
>>>> otherwise -> automatically disable
>>>> 
>>>> No need to show it to the user.
>>> 
>>> I agree in principle, but I don't see why an X86 dependency gets
>>> added here. HAS_PCI really should be all that's needed.
>>> 
>> Yes you are right . I will remove the X86 and make HAS_NS16550_PCI depend on "HAS_NS16550 && HAS_PCI"
> 
> There are quite a bit of work to make the PCI part of the implementation build on Arm because the code refer to x86 functions.
> 
> While in theory, an NS16550 PCI card could be used on Arm, there is only a slim chance for an actual users. So I am not convinced the effort is worth it here.
> 
> Cheers,

Ok. I will enable by default HAS_NS16550_PCI on X86 only. Once we have proper support for NS16550 PCI on ARM we can enable it at that time.
I will modify as follows:

config HAS_NS16550_PCI                                                          
    bool                                                                        
    default y                                                                   
    depends on X86 && HAS_NS16550 && HAS_PCI                                    
    help                                                                        
      This selects the 16550-series UART PCI support. For most systems, say Y.


> 
> -- 
> Julien Grall

Thanks,
Rahul
Jan Beulich Oct. 28, 2020, 3:49 p.m. UTC | #7
On 28.10.2020 16:47, Rahul Singh wrote:
> Ok. I will enable by default HAS_NS16550_PCI on X86 only. Once we have proper support for NS16550 PCI on ARM we can enable it at that time.
> I will modify as follows:
> 
> config HAS_NS16550_PCI                                                          
>     bool                                                                        
>     default y                                                                   

def_bool y

Jan
diff mbox series

Patch

diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
index b572305657..8887e86afe 100644
--- a/xen/drivers/char/Kconfig
+++ b/xen/drivers/char/Kconfig
@@ -4,6 +4,13 @@  config HAS_NS16550
 	help
 	  This selects the 16550-series UART support. For most systems, say Y.
 
+config HAS_NS16550_PCI
+	bool "NS16550 UART PCI support" if X86
+	default y
+	depends on X86 && HAS_NS16550 && HAS_PCI
+	help
+	  This selects the 16550-series UART PCI support. For most systems, say Y.
+
 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]) )