Re: [PATCH v2 2/3] ns16550: "com<N>=" command line options are x86-specific

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 23 Nov 2020, Jan Beulich wrote:
> Pure code motion (plus the addition of "#ifdef CONFIG_X86); no
> functional change intended.
> 
> Reported-by: Julien Grall <julien@xxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Great cleanup

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> ---
> v2: Re-base over new earlier patch.
> 
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -318,8 +318,8 @@ Interrupts.  Specifying zero disables CM
>  Flag to indicate whether to probe for a CMOS Real Time Clock irrespective of
>  ACPI indicating none to be there.
>  
> -### com1
> -### com2
> +### com1 (x86)
> +### com2 (x86)
>  > `= 
> <baud>[/<base-baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>|msi][,[<port-bdf>][,[<bridge-bdf>]]]]]]`
>  
>  Both option `com1` and `com2` follow the same format.
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -31,38 +31,6 @@
>  #include <asm/fixmap.h>
>  #endif
>  
> -/*
> - * Configure serial port with a string:
> - *   
> <baud>[/<base_baud>][,DPS[,<io-base>[,<irq>[,<port-bdf>[,<bridge-bdf>]]]]].
> - * The tail of the string can be omitted if platform defaults are sufficient.
> - * If the baud rate is pre-configured, perhaps by a bootloader, then 'auto'
> - * can be specified in place of a numeric baud rate. Polled mode is specified
> - * by requesting irq 0.
> - */
> -static char __initdata opt_com1[128] = "";
> -static char __initdata opt_com2[128] = "";
> -string_param("com1", opt_com1);
> -string_param("com2", opt_com2);
> -
> -enum serial_param_type {
> -    baud,
> -    clock_hz,
> -    data_bits,
> -    io_base,
> -    irq,
> -    parity,
> -    reg_shift,
> -    reg_width,
> -    stop_bits,
> -#ifdef CONFIG_HAS_PCI
> -    bridge_bdf,
> -    device,
> -    port_bdf,
> -#endif
> -    /* List all parameters before this line. */
> -    num_serial_params
> -};
> -
>  static struct ns16550 {
>      int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
>      u64 io_base;   /* I/O port or memory-mapped I/O address. */
> @@ -98,32 +66,6 @@ static struct ns16550 {
>  #endif
>  } ns16550_com[2] = { { 0 } };
>  
> -struct serial_param_var {
> -    char name[12];
> -    enum serial_param_type type;
> -};
> -
> -/*
> - * Enum struct keeping a table of all accepted parameter names for parsing
> - * com_console_options for serial port com1 and com2.
> - */
> -static const struct serial_param_var __initconst sp_vars[] = {
> -    {"baud", baud},
> -    {"clock-hz", clock_hz},
> -    {"data-bits", data_bits},
> -    {"io-base", io_base},
> -    {"irq", irq},
> -    {"parity", parity},
> -    {"reg-shift", reg_shift},
> -    {"reg-width", reg_width},
> -    {"stop-bits", stop_bits},
> -#ifdef CONFIG_HAS_PCI
> -    {"bridge", bridge_bdf},
> -    {"dev", device},
> -    {"port", port_bdf},
> -#endif
> -};
> -
>  #ifdef CONFIG_HAS_PCI
>  struct ns16550_config {
>      u16 vendor_id;
> @@ -674,6 +616,19 @@ static struct uart_driver __read_mostly
>  #endif
>  };
>  
> +static void ns16550_init_common(struct ns16550 *uart)
> +{
> +    uart->clock_hz  = UART_CLOCK_HZ;
> +
> +    /* Default is no transmit FIFO. */
> +    uart->fifo_size = 1;
> +
> +    /* Default lsr_mask = UART_LSR_THRE */
> +    uart->lsr_mask  = UART_LSR_THRE;
> +}
> +
> +#ifdef CONFIG_X86
> +
>  static int __init parse_parity_char(int c)
>  {
>      switch ( c )
> @@ -1217,6 +1172,64 @@ pci_uart_config(struct ns16550 *uart, bo
>  #endif /* CONFIG_HAS_PCI */
>  
>  /*
> + * Configure serial port with a string:
> + *   
> <baud>[/<base_baud>][,DPS[,<io-base>[,<irq>[,<port-bdf>[,<bridge-bdf>]]]]].
> + * The tail of the string can be omitted if platform defaults are sufficient.
> + * If the baud rate is pre-configured, perhaps by a bootloader, then 'auto'
> + * can be specified in place of a numeric baud rate. Polled mode is specified
> + * by requesting irq 0.
> + */
> +static char __initdata opt_com1[128] = "";
> +static char __initdata opt_com2[128] = "";
> +string_param("com1", opt_com1);
> +string_param("com2", opt_com2);
> +
> +enum serial_param_type {
> +    baud,
> +    clock_hz,
> +    data_bits,
> +    io_base,
> +    irq,
> +    parity,
> +    reg_shift,
> +    reg_width,
> +    stop_bits,
> +#ifdef CONFIG_HAS_PCI
> +    bridge_bdf,
> +    device,
> +    port_bdf,
> +#endif
> +    /* List all parameters before this line. */
> +    num_serial_params
> +};
> +
> +struct serial_param_var {
> +    char name[12];
> +    enum serial_param_type type;
> +};
> +
> +/*
> + * Enum struct keeping a table of all accepted parameter names for parsing
> + * com_console_options for serial port com1 and com2.
> + */
> +static const struct serial_param_var __initconst sp_vars[] = {
> +    {"baud", baud},
> +    {"clock-hz", clock_hz},
> +    {"data-bits", data_bits},
> +    {"io-base", io_base},
> +    {"irq", irq},
> +    {"parity", parity},
> +    {"reg-shift", reg_shift},
> +    {"reg-width", reg_width},
> +    {"stop-bits", stop_bits},
> +#ifdef CONFIG_HAS_PCI
> +    {"bridge", bridge_bdf},
> +    {"dev", device},
> +    {"port", port_bdf},
> +#endif
> +};
> +
> +/*
>   * Used to parse name value pairs and return which value it is along with
>   * pointer for the extracted value.
>   */
> @@ -1504,17 +1517,6 @@ static void __init ns16550_parse_port_co
>      serial_register_uart(uart - ns16550_com, &ns16550_driver, uart);
>  }
>  
> -static void ns16550_init_common(struct ns16550 *uart)
> -{
> -    uart->clock_hz  = UART_CLOCK_HZ;
> -
> -    /* Default is no transmit FIFO. */
> -    uart->fifo_size = 1;
> -
> -    /* Default lsr_mask = UART_LSR_THRE */
> -    uart->lsr_mask  = UART_LSR_THRE;
> -}
> -
>  void __init ns16550_init(int index, struct ns16550_defaults *defaults)
>  {
>      struct ns16550 *uart;
> @@ -1541,6 +1543,8 @@ void __init ns16550_init(int index, stru
>      ns16550_parse_port_config(uart, (index == 0) ? opt_com1 : opt_com2);
>  }
>  
> +#endif /* CONFIG_X86 */
> +
>  #ifdef CONFIG_HAS_DEVICE_TREE
>  static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
>                                         const void *data)
>