[v8] ns16550: Add support for UART parameters to be specifed with name-value pairs

Commit Message

Swapnil Paratey June 2, 2017, 8:07 p.m. UTC

Add name=value parsing options for com1 and com2 to add flexibility
in setting register values for MMIO UART devices.

Maintain backward compatibility with previous positional parameter
specfications.

eg. com1=115200,8n1,0x3f8,4
eg. com1=115200,8n1,0x3f8,4,reg_width=4,reg_shift=2
eg. com1=baud=115200,parity=n,reg_width=4,reg_shift=2,irq=4

Signed-off-by: Swapnil Paratey <[e-mail]>
Reviewed-by: Jan Beulich <[e-mail]>

---
Changed since v7:
 * Added CONFIG_HAS_PCI macros for bridge_bdf, device and port_bdf case
   statements inside parse_namevalue_pairs() that caused ARM build fails

Changed since v6:
 * Changed '1' to bool true in bridge_bdf case statement for uart->ps_bdf_enable
 * Removed break for default case in switch block in parse_namevalue_pairs()

Changed since v5:
 * Extended changelist for PATCH v4 and v5
 * Removed const for name[12] in serial_param_var
 * Changed "ext_value" pointer to "value" double pointer in get_token()
 * Removed parenthesis in clock_hz case statement

Changed since v4:
 * Changed max_serial_params to num_serial_params in enum e_serial_param_type
 * Changed *sp_name in struct serial_param_var to a 12 byte buffer
 * Removed sp_ prefix in sp_name in struct serial_param_var
 * Removed if ( name_val_pos ) check before while() loop in parse_positional()
 * Added blank lines for non-fall-through case blocks in switch statement
   inside parse_namevalue_pairs
 * Changed comparison of a bool with "true" for dev_set in io_base case.
 * Changed cmdline to com_console_options in ns16550_parse_port_config()
 * Changed io_base setting after pci device specification to give an error
   message instead of PARSE_ERR_RET
 * Removed config_parsed goto statements outside of error cleanup
 * Removed the full-stop (.) in sanity checks for reg_width at the end of
   ns16550_parse_port_config

Changed since v3:
 * Changed subject/title of the patch
   Previous name: ns16550-Add-command-line-parsing-adjustments
 * Increased length of opt_com1 and opt_com2 buffers to 128 bytes.
 * Changed wrongly used bus:device:function to bus:device.function in
   xen-command-line.markdown
 * Using '-' instead of '_' for name-value pairs
   eg. reg-width instead of reg_width, stop-bits instead of stop_bits
 * Used __initconst for serial_param_var struct
 * Changed get_token_value function name to get_token
 * Values for name-value pairs in get_token are extracted only when a match
   is found in the table for valid name-value pairs
 * Removed intermediate return variable for get_token function and used
   it directly in the switch statement for setting UART configs in
   parse_namevalue_pairs

Changed since v2:
 * Added name=value specification for com1 and com2 command line
   parameter input for UART devices
   Syntax: "com1=(positional parameters),(name-value parameters)"
 * Maintained previous positional specification for UART parameters
 * All parameters should be comma-separated

Changed since v1:
 * Changed opt_com1 and opt_com2 array size to 64 (power of 2).
 * Added descriptions for reg_width and reg_shift in
   docs/misc/xen-command-line.markdown
 * Changed subject to ns16550 from 16550 for better tracking.
---
 docs/misc/xen-command-line.markdown |  38 ++++++
 xen/common/kernel.c                 |   2 +-
 xen/drivers/char/ns16550.c          | 248 +++++++++++++++++++++++++++++++++---
 3 files changed, 271 insertions(+), 17 deletions(-)

Comments

Jan Beulich June 6, 2017, 3:39 p.m. UTC | #1

>>> On 02.06.17 at 22:07, <[e-mail]> wrote:
> Add name=value parsing options for com1 and com2 to add flexibility
> in setting register values for MMIO UART devices.
> 
> Maintain backward compatibility with previous positional parameter
> specfications.
> 
> eg. com1=115200,8n1,0x3f8,4
> eg. com1=115200,8n1,0x3f8,4,reg_width=4,reg_shift=2
> eg. com1=baud=115200,parity=n,reg_width=4,reg_shift=2,irq=4
> 
> Signed-off-by: Swapnil Paratey <[e-mail]>
> Reviewed-by: Jan Beulich <[e-mail]>

For the future, please drop prior tags when fixing bugs or doing
any other not purely mechanical changes.

> +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
> +};

Just like you've done here and ...

> @@ -77,6 +96,32 @@ 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
> +};

... here I would also have expected you to group PCI things ...

> +static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
> +{
> +    char *token, *start = str;
> +    char *param_value = NULL;
> +    bool dev_set = false;
> +
> +    if ( (str == NULL) || (*str == '\0') )
> +        return true;
> +
> +    do
> +    {
> +        /* When no tokens are found, start will be NULL */
> +        token = strsep(&start, ",");
> +
> +        switch ( get_token(token, &param_value) )
> +        {
> +        case baud:
> +            uart->baud = simple_strtoul(param_value, NULL, 0);
> +            break;
> +#ifdef CONFIG_HAS_PCI
> +        case bridge_bdf:
> +            if ( !parse_pci(param_value, NULL, &uart->ps_bdf[0],
> +                            &uart->ps_bdf[1], &uart->ps_bdf[2]) )
> +                PARSE_ERR_RET("Bad port PCI coordinates\n");
> +            uart->ps_bdf_enable = true;
> +            break;
> +#endif
> +        case clock_hz:
> +            uart->clock_hz = simple_strtoul(param_value, NULL, 0) << 4;
> +            break;
> +#ifdef CONFIG_HAS_PCI
> +        case device:
> +            if ( strncmp(param_value, "pci", 3) == 0 )
> +            {
> +                pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com);
> +                dev_set = true;
> +            }
> +            else if ( strncmp(param_value, "amt", 3) == 0 )
> +            {
> +                pci_uart_config(uart, 0, uart - ns16550_com);
> +                dev_set = true;
> +            }
> +            break;
> +#endif
> +        case io_base:
> +            if ( dev_set )
> +            {
> +                printk(XENLOG_WARNING
> +                       "Can't use io_base with dev=pci or dev=amt options\n");
> +                break;
> +            }
> +            uart->io_base = simple_strtoul(param_value, NULL, 0);
> +            break;
> +
> +        case irq:
> +            uart->irq = simple_strtoul(param_value, NULL, 0);
> +            break;
> +
> +        case data_bits:
> +            uart->data_bits = simple_strtoul(param_value, NULL, 0);
> +            break;
> +
> +        case parity:
> +            uart->parity = parse_parity_char(*param_value);
> +            break;
> +#ifdef CONFIG_HAS_PCI
> +        case port_bdf:
> +            if ( !parse_pci(param_value, NULL, &uart->pb_bdf[0],
> +                            &uart->pb_bdf[1], &uart->pb_bdf[2]) )
> +                PARSE_ERR_RET("Bad port PCI coordinates\n");
> +            uart->pb_bdf_enable = true;
> +            break;
> +#endif
> +        case stop_bits:
> +            uart->stop_bits = simple_strtoul(param_value, NULL, 0);
> +            break;
> +
> +        case reg_shift:
> +            uart->reg_shift = simple_strtoul(param_value, NULL, 0);
> +            break;
> +
> +        case reg_width:
> +            uart->reg_width = simple_strtoul(param_value, NULL, 0);
> +            break;
> +
> +        default:
> +            PARSE_ERR_RET("Invalid parameter: %s\n", token);
> +
> +        }
> +    } while ( start != NULL );
> +
> +    return true;
> +}

... here. Furthermore please retain the blank lines you had between
individual case blocks (instead of replacing them by #if-s/#else-s).
And please get rid of the stray blank line at the bottom of above
switch(). With all of this you may then retain my R-b.

Jan

Patch

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 44d9985..8c939c4 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -324,6 +324,44 @@  Both option `com1` and `com2` follow the same format.
 
 A typical setup for most situations might be `com1=115200,8n1`
 
+In addition to the above positional specification for UART parameters,
+name=value pair specfications are also supported. This is used to add
+flexibility for UART devices which require additional UART parameter
+configurations.
+
+The comma separation still delineates positional parameters. Hence,
+unless the parameter is explicitly specified with name=value option, it
+will be considered a positional parameter.
+
+The syntax consists of
+com1=(comma-separated positional parameters),(comma separated name-value pairs)
+
+The accepted name keywords for name=value pairs are
+ * `baud` - accepts integer baud rate (eg. 115200) or `auto`
+ * `bridge`- Similar to bridge-bdf in positional parameters.
+             Used to determine the PCI bridge to access the UART device.
+             Notation is xx:xx.xx <bus>:<device>.<function>
+ * `clock-hz`- accepts large integers to setup UART clock frequencies.
+               Do note - these values are multiplied by 16.
+ * `data-bits` - integer between 5 and 8
+ * `dev` - accepted values are `pci` OR `amt`. If this option
+           is used to specify if the serial device is pci-based. The io_base
+           cannot be specified when `dev=pci` or `dev=amt` is used.
+ * `io-base` - accepts integer which specified IO base port for UART registers
+ * `irq` - IRQ number to use
+ * `parity` - accepted values are same as positional parameters
+ * `port` - Used to specify which port the PCI serial device is located on
+            Notation is xx:xx.xx <bus>:<device>.<function>
+ * `reg-shift` - register shifts required to set UART registers
+ * `reg-width` - register width required to set UART registers
+                 (only accepts 1 and 4)
+ * `stop-bits` - only accepts 1 or 2 for the number of stop bits
+
+The following are examples of correct specifications:
+`com1=115200,8n1,0x3f8,4`
+`com1=115200,8n1,0x3f8,4,reg_width=4,reg_shift=2`
+`com1=baud=115200,parity=n,stop_bits=1,io_base=0x3f8,reg_width=4`
+
 ### conring\_size
 > `= <size>`
 
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 8461871..e1ebb0b 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -49,7 +49,7 @@  static void __init assign_integer_param(
 
 static void __init _cmdline_parse(const char *cmdline)
 {
-    char opt[100], *optval, *optkey, *q;
+    char opt[128], *optval, *optkey, *q;
     const char *p = cmdline;
     const struct kernel_param *param;
     int bool_assert;
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index e4de3b4..0531135 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -38,11 +38,30 @@ 
  * can be specified in place of a numeric baud rate. Polled mode is specified
  * by requesting irq 0.
  */
-static char __initdata opt_com1[30] = "";
-static char __initdata opt_com2[30] = "";
+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. */
@@ -77,6 +96,32 @@  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;
@@ -1083,26 +1128,73 @@  pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
 }
 #endif
 
+/*
+ * Used to parse name value pairs and return which value it is along with
+ * pointer for the extracted value.
+ */
+static enum __init serial_param_type get_token(char *token, char **value)
+{
+    const char *param_name;
+    unsigned int i;
+
+    param_name = strsep(&token, "=");
+    if ( param_name == NULL )
+        return num_serial_params;
+
+    /* Linear search for the parameter. */
+    for ( i = 0; i < ARRAY_SIZE(sp_vars); i++ )
+    {
+        if ( strcmp(sp_vars[i].name, param_name) == 0 )
+        {
+            *value = token;
+            return sp_vars[i].type;
+        }
+    }
+
+    return num_serial_params;
+}
+
 #define PARSE_ERR(_f, _a...)                 \
     do {                                     \
         printk( "ERROR: " _f "\n" , ## _a ); \
         return;                              \
     } while ( 0 )
 
-static void __init ns16550_parse_port_config(
-    struct ns16550 *uart, const char *conf)
+#define PARSE_ERR_RET(_f, _a...)             \
+    do {                                     \
+        printk( "ERROR: " _f "\n" , ## _a ); \
+        return false;                        \
+    } while ( 0 )
+
+
+static bool __init parse_positional(struct ns16550 *uart, char **str)
 {
     int baud;
+    const char *conf;
+    char *name_val_pos;
 
-    /* No user-specified configuration? */
-    if ( (conf == NULL) || (*conf == '\0') )
+    conf = *str;
+    name_val_pos = strchr(conf, '=');
+
+    /* Finding the end of the positional parameters. */
+    while ( name_val_pos > *str )
     {
-        /* Some platforms may automatically probe the UART configuartion. */
-        if ( uart->baud != 0 )
-            goto config_parsed;
-        return;
+        /* Working backwards from the '=' sign. */
+        name_val_pos--;
+        if ( *name_val_pos == ',' )
+        {
+            *name_val_pos = '\0';
+            name_val_pos++;
+            break;
+        }
     }
 
+    *str = name_val_pos;
+    /* When there are no positional parameters, we return from the function. */
+    if ( conf == *str )
+        return true;
+
+    /* Parse positional parameters here. */
     if ( strncmp(conf, "auto", 4) == 0 )
     {
         uart->baud = BAUD_AUTO;
@@ -1132,13 +1224,13 @@  static void __init ns16550_parse_port_config(
         if ( strncmp(conf, "pci", 3) == 0 )
         {
             if ( pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
-                return;
+                return true;
             conf += 3;
         }
         else if ( strncmp(conf, "amt", 3) == 0 )
         {
             if ( pci_uart_config(uart, 0, uart - ns16550_com) )
-                return;
+                return true;
             conf += 3;
         }
         else
@@ -1157,19 +1249,141 @@  static void __init ns16550_parse_port_config(
         conf = parse_pci(conf, NULL, &uart->ps_bdf[0],
                          &uart->ps_bdf[1], &uart->ps_bdf[2]);
         if ( !conf )
-            PARSE_ERR("Bad port PCI coordinates");
-        uart->ps_bdf_enable = 1;
+            PARSE_ERR_RET("Bad port PCI coordinates");
+        uart->ps_bdf_enable = true;
     }
 
     if ( *conf == ',' && *++conf != ',' )
     {
         if ( !parse_pci(conf, NULL, &uart->pb_bdf[0],
                         &uart->pb_bdf[1], &uart->pb_bdf[2]) )
-            PARSE_ERR("Bad bridge PCI coordinates");
-        uart->pb_bdf_enable = 1;
+            PARSE_ERR_RET("Bad bridge PCI coordinates");
+        uart->pb_bdf_enable = true;
     }
 #endif
 
+    return true;
+}
+
+static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
+{
+    char *token, *start = str;
+    char *param_value = NULL;
+    bool dev_set = false;
+
+    if ( (str == NULL) || (*str == '\0') )
+        return true;
+
+    do
+    {
+        /* When no tokens are found, start will be NULL */
+        token = strsep(&start, ",");
+
+        switch ( get_token(token, &param_value) )
+        {
+        case baud:
+            uart->baud = simple_strtoul(param_value, NULL, 0);
+            break;
+#ifdef CONFIG_HAS_PCI
+        case bridge_bdf:
+            if ( !parse_pci(param_value, NULL, &uart->ps_bdf[0],
+                            &uart->ps_bdf[1], &uart->ps_bdf[2]) )
+                PARSE_ERR_RET("Bad port PCI coordinates\n");
+            uart->ps_bdf_enable = true;
+            break;
+#endif
+        case clock_hz:
+            uart->clock_hz = simple_strtoul(param_value, NULL, 0) << 4;
+            break;
+#ifdef CONFIG_HAS_PCI
+        case device:
+            if ( strncmp(param_value, "pci", 3) == 0 )
+            {
+                pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com);
+                dev_set = true;
+            }
+            else if ( strncmp(param_value, "amt", 3) == 0 )
+            {
+                pci_uart_config(uart, 0, uart - ns16550_com);
+                dev_set = true;
+            }
+            break;
+#endif
+        case io_base:
+            if ( dev_set )
+            {
+                printk(XENLOG_WARNING
+                       "Can't use io_base with dev=pci or dev=amt options\n");
+                break;
+            }
+            uart->io_base = simple_strtoul(param_value, NULL, 0);
+            break;
+
+        case irq:
+            uart->irq = simple_strtoul(param_value, NULL, 0);
+            break;
+
+        case data_bits:
+            uart->data_bits = simple_strtoul(param_value, NULL, 0);
+            break;
+
+        case parity:
+            uart->parity = parse_parity_char(*param_value);
+            break;
+#ifdef CONFIG_HAS_PCI
+        case port_bdf:
+            if ( !parse_pci(param_value, NULL, &uart->pb_bdf[0],
+                            &uart->pb_bdf[1], &uart->pb_bdf[2]) )
+                PARSE_ERR_RET("Bad port PCI coordinates\n");
+            uart->pb_bdf_enable = true;
+            break;
+#endif
+        case stop_bits:
+            uart->stop_bits = simple_strtoul(param_value, NULL, 0);
+            break;
+
+        case reg_shift:
+            uart->reg_shift = simple_strtoul(param_value, NULL, 0);
+            break;
+
+        case reg_width:
+            uart->reg_width = simple_strtoul(param_value, NULL, 0);
+            break;
+
+        default:
+            PARSE_ERR_RET("Invalid parameter: %s\n", token);
+
+        }
+    } while ( start != NULL );
+
+    return true;
+}
+
+static void __init ns16550_parse_port_config(
+    struct ns16550 *uart, const char *conf)
+{
+    char com_console_options[128];
+    char *str;
+
+    /* No user-specified configuration? */
+    if ( (conf == NULL) || (*conf == '\0') )
+    {
+        /* Some platforms may automatically probe the UART configuartion. */
+        if ( uart->baud != 0 )
+            goto config_parsed;
+        return;
+    }
+
+    strlcpy(com_console_options, conf, ARRAY_SIZE(com_console_options));
+    str = com_console_options;
+
+    /* parse positional parameters and get pointer for name-value pairs */
+    if ( !parse_positional(uart, &str) )
+        return;
+
+    if ( !parse_namevalue_pairs(str, uart) )
+        return;
+
  config_parsed:
     /* Sanity checks. */
     if ( (uart->baud != BAUD_AUTO) &&
@@ -1177,6 +1391,8 @@  static void __init ns16550_parse_port_config(
         PARSE_ERR("Baud rate %d outside supported range.", uart->baud);
     if ( (uart->data_bits < 5) || (uart->data_bits > 8) )
         PARSE_ERR("%d data bits are unsupported.", uart->data_bits);
+    if ( (uart->reg_width != 1) && (uart->reg_width != 4) )
+        PARSE_ERR("Accepted values of reg_width are 1 and 4 only");
     if ( (uart->stop_bits < 1) || (uart->stop_bits > 2) )
         PARSE_ERR("%d stop bits are unsupported.", uart->stop_bits);
     if ( uart->io_base == 0 )