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

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

[Swapnil Paratey]

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 <swapnil.paratey@amd.com>i

---
Changed since v4:
 * Changes from [PATCH v4] code review

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.
 * Implementation changes from [PATCH v3] code review

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(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 450b222..53052b2 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..89e0e4d 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -38,11 +38,28 @@
  * 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,
+    bridge_bdf,
+    clock_hz,
+    data_bits,
+    device,
+    io_base,
+    irq,
+    parity,
+    port_bdf,
+    reg_shift,
+    reg_width,
+    stop_bits,
+    /* 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 +94,30 @@ static struct ns16550 {
 #endif
 } ns16550_com[2] = { { 0 } };
 
+struct serial_param_var {
+    const 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},
+    {"bridge", bridge_bdf},
+    {"clock-hz", clock_hz},
+    {"data-bits", data_bits},
+    {"dev", device},
+    {"io-base", io_base},
+    {"irq", irq},
+    {"parity", parity},
+    {"port", port_bdf},
+    {"reg-shift", reg_shift},
+    {"reg-width", reg_width},
+    {"stop-bits", stop_bits},
+};
+
 #ifdef CONFIG_HAS_PCI
 struct ns16550_config {
     u16 vendor_id;
@@ -1083,26 +1124,77 @@ 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 - ext_value.
+ */
+static enum __init serial_param_type get_token(char *token, char *ext_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 )
+        {
+            /*
+             * Token has the param value after the equal to sign.
+             * ext_value is a 16-byte buffer - param_value[16].
+             */
+            strlcpy(ext_value, token, 16);
+            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[16];
+    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;
+
+        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 = 1;
+            break;
+
+        case clock_hz:
+            uart->clock_hz = (simple_strtoul(param_value, NULL, 0) << 4);
+            break;
+
+        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;
+
+        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;
+
+        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;
+
+        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);
+            break;
+
+        }
+    } 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 )
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 19.04.17 at 20:57, <swapnil.paratey@amd.com> wrote:
> Changed since v4:
>  * Changes from [PATCH v4] code review

I'm sorry, but this is not enough.

> @@ -77,6 +94,30 @@ static struct ns16550 {
>  #endif
>  } ns16550_com[2] = { { 0 } };
>  
> +struct serial_param_var {
> +    const char name[12];

Pointless const.

> +static enum __init serial_param_type get_token(char *token, char *ext_value)

I don't see the need for the ext_ prefix.

> +{
> +    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 )
> +        {
> +            /*
> +             * Token has the param value after the equal to sign.
> +             * ext_value is a 16-byte buffer - param_value[16].
> +             */
> +            strlcpy(ext_value, token, 16);

So why do you copy here, instead of simply passing the pointer
back? This is the more that the caller didn't tell you it 16 bytes of
space allocated for the output value.

> +static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
> +{
> +    char *token, *start = str;
> +    char param_value[16];
> +    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) ) {

Coding style (missing blank and brace on its own line).

> +        case baud:
> +            uart->baud = simple_strtoul(param_value, NULL, 0);
> +            break;
> +
> +        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 = 1;
> +            break;
> +
> +        case clock_hz:
> +            uart->clock_hz = (simple_strtoul(param_value, NULL, 0) << 4);

Pointless parentheses.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel