[PATCH v2] ns16550: Add command line parsing adjustments

[PATCH v2] ns16550: Add command line parsing adjustments

[Swapnil Paratey]

Add parsing options for reg_width and reg_shift in bootup command line
parameters. This adds flexibility in setting register values
for MMIO UART devices.

Increase length of opt_com1 and opt_com2 buffer to accommodate more
command line parameters.

eg. com1=115200,8n1,0x3f8,4 (legacy IO)
eg. com1=115200/3000000/4/2,8n1,0xfedc9000,4 (MMIO adjustments)

Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Swapnil Paratey <swapnil.paratey@amd.com>

---
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 | 11 ++++++++++-
 xen/drivers/char/ns16550.c          | 20 +++++++++++++++++---
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 0138978..9ab7ead 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -291,7 +291,7 @@ Flag to indicate whether to probe for a CMOS Real Time Clock irrespective of
 ACPI indicating none to be there.
 
 ### com1,com2
-> `= <baud>[/<base-baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>][,[<port-bdf>][,[<bridge-bdf>]]]]]]`
+> `= <baud>[/[<base-baud>][/[<reg-width>][/[<reg-shift>]]]][,DPS[,<io-base>[,<irq>[,<port-bdf>[,<bridge-bdf>]]]]]`
 
 Both option `com1` and `com2` follow the same format.
 
@@ -299,6 +299,15 @@ Both option `com1` and `com2` follow the same format.
   the bootloader or other earlier firmware has already set it up.
 * Optionally, the base baud rate (usually the highest baud rate the
   device can communicate at) can be specified.
+* `<reg-width>` is the access size, or width, for programming
+  the UART device registers.  Accepted values are 1 and 4 (bytes).
+  The UART device datasheet defines the register width to be used when
+  reading or writing the registers. This field is optional.
+  The default value is 1.
+* `<reg-shift>` is the number of bits to shift the register offset value
+  for programming the UART device registers. The UART device datasheet
+  defines the register shift needed to access the registers properly.
+  This field is optional. The default value is 0.
 * `DPS` represents the number of data bits, the parity, and the number
   of stop bits.
   * `D` is an integer between 5 and 8 for the number of data bits.
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 1da103a..0e80bce 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -33,14 +33,14 @@
 
 /*
  * Configure serial port with a string:
- *   <baud>[/<base_baud>][,DPS[,<io-base>[,<irq>[,<port-bdf>[,<bridge-bdf>]]]]].
+ *   <baud>[/[<base-baud>][/[<reg-width>][/[<reg-shift>]]]][,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[30] = "";
-static char __initdata opt_com2[30] = "";
+static char __initdata opt_com1[64] = "";
+static char __initdata opt_com2[64] = "";
 string_param("com1", opt_com1);
 string_param("com2", opt_com2);
 
@@ -1118,6 +1118,20 @@ static void __init ns16550_parse_port_config(
         uart->clock_hz = simple_strtoul(conf, &conf, 0) << 4;
     }
 
+    if ( *conf == '/' )
+    {
+        conf++;
+        if ( *conf != '/' && *conf != ',' )
+            uart->reg_width = simple_strtol(conf, &conf, 0);
+    }
+
+    if ( *conf == '/' )
+    {
+        conf++;
+        if ( *conf != '/' && *conf != ',' )
+            uart->reg_shift = simple_strtol(conf, &conf, 0);
+    }
+
     if ( *conf == ',' && *++conf != ',' )
     {
         uart->data_bits = simple_strtoul(conf, &conf, 10);
-- 
1.9.1


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

Re: [PATCH v2] ns16550: Add command line parsing adjustments

[Andrew Cooper]

On 05/01/17 22:39, Swapnil Paratey wrote:
> Add parsing options for reg_width and reg_shift in bootup command line
> parameters. This adds flexibility in setting register values
> for MMIO UART devices.
>
> Increase length of opt_com1 and opt_com2 buffer to accommodate more
> command line parameters.
>
> eg. com1=115200,8n1,0x3f8,4 (legacy IO)
> eg. com1=115200/3000000/4/2,8n1,0xfedc9000,4 (MMIO adjustments)
>
> Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Swapnil Paratey <swapnil.paratey@amd.com>

Much better, thanks.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v2] ns16550: Add command line parsing adjustments

[Jan Beulich]

>>> On 05.01.17 at 23:39, <swapnil.paratey@amd.com> wrote:
> @@ -299,6 +299,15 @@ Both option `com1` and `com2` follow the same format.
>    the bootloader or other earlier firmware has already set it up.
>  * Optionally, the base baud rate (usually the highest baud rate the
>    device can communicate at) can be specified.
> +* `<reg-width>` is the access size, or width, for programming
> +  the UART device registers.  Accepted values are 1 and 4 (bytes).
> +  The UART device datasheet defines the register width to be used when
> +  reading or writing the registers. This field is optional.
> +  The default value is 1.
> +* `<reg-shift>` is the number of bits to shift the register offset value
> +  for programming the UART device registers. The UART device datasheet
> +  defines the register shift needed to access the registers properly.
> +  This field is optional. The default value is 0.

I was about to point out that the two defaults listed here aren't
really correct for all cases, but the situation is worse:

> @@ -1118,6 +1118,20 @@ static void __init ns16550_parse_port_config(
>          uart->clock_hz = simple_strtoul(conf, &conf, 0) << 4;
>      }
>  
> +    if ( *conf == '/' )
> +    {
> +        conf++;
> +        if ( *conf != '/' && *conf != ',' )
> +            uart->reg_width = simple_strtol(conf, &conf, 0);
> +    }
> +
> +    if ( *conf == '/' )
> +    {
> +        conf++;
> +        if ( *conf != '/' && *conf != ',' )
> +            uart->reg_shift = simple_strtol(conf, &conf, 0);
> +    }

Putting the new override settings here is not only undesirable
because the / separator so far really separates two similar
things (while you now make it also separate dissimilar ones),
but it also means one won't be able to override anything
coming out of pci_uart_config(). Therefore I think you need
to move this further down (and use , as the separator), in
turn requiring an adjustment to the doc change.

Jan


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

Re: [PATCH v2] ns16550: Add command line parsing adjustments

[swapnil]

On 01/06/2017 08:58 AM, Jan Beulich wrote:

>>>> On 05.01.17 at 23:39, <swapnil.paratey@amd.com> wrote:
>> @@ -299,6 +299,15 @@ Both option `com1` and `com2` follow the same format.
>>     the bootloader or other earlier firmware has already set it up.
>>   * Optionally, the base baud rate (usually the highest baud rate the
>>     device can communicate at) can be specified.
>> +* `<reg-width>` is the access size, or width, for programming
>> +  the UART device registers.  Accepted values are 1 and 4 (bytes).
>> +  The UART device datasheet defines the register width to be used when
>> +  reading or writing the registers. This field is optional.
>> +  The default value is 1.
>> +* `<reg-shift>` is the number of bits to shift the register offset value
>> +  for programming the UART device registers. The UART device datasheet
>> +  defines the register shift needed to access the registers properly.
>> +  This field is optional. The default value is 0.
> I was about to point out that the two defaults listed here aren't
> really correct for all cases, but the situation is worse:
>
>> @@ -1118,6 +1118,20 @@ static void __init ns16550_parse_port_config(
>>           uart->clock_hz = simple_strtoul(conf, &conf, 0) << 4;
>>       }
>>   
>> +    if ( *conf == '/' )
>> +    {
>> +        conf++;
>> +        if ( *conf != '/' && *conf != ',' )
>> +            uart->reg_width = simple_strtol(conf, &conf, 0);
>> +    }
>> +
>> +    if ( *conf == '/' )
>> +    {
>> +        conf++;
>> +        if ( *conf != '/' && *conf != ',' )
>> +            uart->reg_shift = simple_strtol(conf, &conf, 0);
>> +    }
> Putting the new override settings here is not only undesirable
> because the / separator so far really separates two similar
> things (while you now make it also separate dissimilar ones),
> but it also means one won't be able to override anything
> coming out of pci_uart_config(). Therefore I think you need
> to move this further down (and use , as the separator), in
> turn requiring an adjustment to the doc change.
>
> Jan

Just to understand this correctly, is this the expected syntax?
<baud>[/<base-baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>][,[<port-bdf>]
[,[<bridge-bdf>][,[<reg_width>][,[<reg_shift>]]]]]]]]

Can I add these override settings (reg_width and reg_shift) just before
the sanity checks?

Should I have sanity checks for reg_width and reg_shift?
If so, should reg_width just check for values 1 and 4?
And should reg_shift have a max shift value as a sanity check?

Swapnil


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

Re: [PATCH v2] ns16550: Add command line parsing adjustments

[Jan Beulich]

>>> On 06.01.17 at 17:24, <swapnil.paratey@amd.com> wrote:
> On 01/06/2017 08:58 AM, Jan Beulich wrote:
>>>>> On 05.01.17 at 23:39, <swapnil.paratey@amd.com> wrote:
>>> @@ -1118,6 +1118,20 @@ static void __init ns16550_parse_port_config(
>>>           uart->clock_hz = simple_strtoul(conf, &conf, 0) << 4;
>>>       }
>>>   
>>> +    if ( *conf == '/' )
>>> +    {
>>> +        conf++;
>>> +        if ( *conf != '/' && *conf != ',' )
>>> +            uart->reg_width = simple_strtol(conf, &conf, 0);
>>> +    }
>>> +
>>> +    if ( *conf == '/' )
>>> +    {
>>> +        conf++;
>>> +        if ( *conf != '/' && *conf != ',' )
>>> +            uart->reg_shift = simple_strtol(conf, &conf, 0);
>>> +    }
>> Putting the new override settings here is not only undesirable
>> because the / separator so far really separates two similar
>> things (while you now make it also separate dissimilar ones),
>> but it also means one won't be able to override anything
>> coming out of pci_uart_config(). Therefore I think you need
>> to move this further down (and use , as the separator), in
>> turn requiring an adjustment to the doc change.
> 
> Just to understand this correctly, is this the expected syntax?
> <baud>[/<base-baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>][,[<port-bdf>]
> [,[<bridge-bdf>][,[<reg_width>][,[<reg_shift>]]]]]]]]
> 
> Can I add these override settings (reg_width and reg_shift) just before
> the sanity checks?

Well, as you may have seen, things are getting complicated here:
The two currently-last elements are permitted only with
CONFIG_HAS_PCI, so by adding the new fields to the end we'd
end up having two syntaxes (one with and one without PCI
support). I therefore have to modify my original proposal, and
ask for the addition to be done earlier, perhaps - using a
separator other than comma (maybe colon or semicolon) - with
the [<io-base>|pci|amt] element (as that's really the item
(possibly implicitly) specifying the I/O range, which you mean to
amend.

> Should I have sanity checks for reg_width and reg_shift?
> If so, should reg_width just check for values 1 and 4?
> And should reg_shift have a max shift value as a sanity check?

Yeah, considering the other consistency checks, it would probably
be a good idea to have this.

Jan


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

Re: [PATCH v2] ns16550: Add command line parsing adjustments

[Swapnil Paratey]

On 01/06/2017 10:43 AM, Jan Beulich wrote:

>>>> On 06.01.17 at 17:24, <swapnil.paratey@amd.com> wrote:
>> On 01/06/2017 08:58 AM, Jan Beulich wrote:
>>>>>> On 05.01.17 at 23:39, <swapnil.paratey@amd.com> wrote:
>>>> @@ -1118,6 +1118,20 @@ static void __init ns16550_parse_port_config(
>>>>            uart->clock_hz = simple_strtoul(conf, &conf, 0) << 4;
>>>>        }
>>>>    
>>>> +    if ( *conf == '/' )
>>>> +    {
>>>> +        conf++;
>>>> +        if ( *conf != '/' && *conf != ',' )
>>>> +            uart->reg_width = simple_strtol(conf, &conf, 0);
>>>> +    }
>>>> +
>>>> +    if ( *conf == '/' )
>>>> +    {
>>>> +        conf++;
>>>> +        if ( *conf != '/' && *conf != ',' )
>>>> +            uart->reg_shift = simple_strtol(conf, &conf, 0);
>>>> +    }
>>> Putting the new override settings here is not only undesirable
>>> because the / separator so far really separates two similar
>>> things (while you now make it also separate dissimilar ones),
>>> but it also means one won't be able to override anything
>>> coming out of pci_uart_config(). Therefore I think you need
>>> to move this further down (and use , as the separator), in
>>> turn requiring an adjustment to the doc change.
>> Just to understand this correctly, is this the expected syntax?
>> <baud>[/<base-baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>][,[<port-bdf>]
>> [,[<bridge-bdf>][,[<reg_width>][,[<reg_shift>]]]]]]]]
>>
>> Can I add these override settings (reg_width and reg_shift) just before
>> the sanity checks?
> Well, as you may have seen, things are getting complicated here:
> The two currently-last elements are permitted only with
> CONFIG_HAS_PCI, so by adding the new fields to the end we'd
> end up having two syntaxes (one with and one without PCI
> support). I therefore have to modify my original proposal, and
> ask for the addition to be done earlier, perhaps - using a
> separator other than comma (maybe colon or semicolon) - with
> the [<io-base>|pci|amt] element (as that's really the item
> (possibly implicitly) specifying the I/O range, which you mean to
> amend.

Yes, that's true. We have a MMIO-based (non-PCI) UART device which needs a
4-byte wide access (reg_width) and a reg_shift for specifying offsets. So,
adding options for reg_width and reg_shift with a semi-colon/colon after the
<io_base> is a good option.

So, just for clarification, PCI based serial devices should definitely not
have the reg_width and reg_shift overrides? They should be options available
only with the <io_base> mentioned?

Would this syntax be appropriate for coding the solution and documentation?
<baud>[/<base-baud>]
[,[DPS][,[<io-base>[:[<reg_width>][:[<reg_shift>]]]|pci|amt]]
[,[<irq>][,[<port-bdf>][,[<bridge-bdf>]]]]]]

>
>> Should I have sanity checks for reg_width and reg_shift?
>> If so, should reg_width just check for values 1 and 4?
>> And should reg_shift have a max shift value as a sanity check?
> Yeah, considering the other consistency checks, it would probably
> be a good idea to have this.
>
> Jan

Thanks for your suggestions/feedback

Swapnil


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

Re: [PATCH v2] ns16550: Add command line parsing adjustments

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v2] ns16550: Add command line parsing adjustments"):
> On 06.01.17 at 17:24, <swapnil.paratey@amd.com> wrote:
> Well, as you may have seen, things are getting complicated here:
> The two currently-last elements are permitted only with
> CONFIG_HAS_PCI, so by adding the new fields to the end we'd
> end up having two syntaxes (one with and one without PCI
> support). I therefore have to modify my original proposal, and
> ask for the addition to be done earlier, perhaps - using a
> separator other than comma (maybe colon or semicolon) - with
> the [<io-base>|pci|amt] element (as that's really the item
> (possibly implicitly) specifying the I/O range, which you mean to
> amend.

Why not introduce the concept of names for these endlessly multiplying
parameters ?

Ian.

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

Re: [PATCH v2] ns16550: Add command line parsing adjustments

[Jan Beulich]

>>> On 06.01.17 at 18:28, <swapnil.paratey@amd.com> wrote:
> On 01/06/2017 10:43 AM, Jan Beulich wrote:
> 
>>>>> On 06.01.17 at 17:24, <swapnil.paratey@amd.com> wrote:
>>> On 01/06/2017 08:58 AM, Jan Beulich wrote:
>>>>>>> On 05.01.17 at 23:39, <swapnil.paratey@amd.com> wrote:
>>>>> @@ -1118,6 +1118,20 @@ static void __init ns16550_parse_port_config(
>>>>>            uart->clock_hz = simple_strtoul(conf, &conf, 0) << 4;
>>>>>        }
>>>>>    
>>>>> +    if ( *conf == '/' )
>>>>> +    {
>>>>> +        conf++;
>>>>> +        if ( *conf != '/' && *conf != ',' )
>>>>> +            uart->reg_width = simple_strtol(conf, &conf, 0);
>>>>> +    }
>>>>> +
>>>>> +    if ( *conf == '/' )
>>>>> +    {
>>>>> +        conf++;
>>>>> +        if ( *conf != '/' && *conf != ',' )
>>>>> +            uart->reg_shift = simple_strtol(conf, &conf, 0);
>>>>> +    }
>>>> Putting the new override settings here is not only undesirable
>>>> because the / separator so far really separates two similar
>>>> things (while you now make it also separate dissimilar ones),
>>>> but it also means one won't be able to override anything
>>>> coming out of pci_uart_config(). Therefore I think you need
>>>> to move this further down (and use , as the separator), in
>>>> turn requiring an adjustment to the doc change.
>>> Just to understand this correctly, is this the expected syntax?
>>> <baud>[/<base-baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>][,[<port-bdf>]
>>> [,[<bridge-bdf>][,[<reg_width>][,[<reg_shift>]]]]]]]]
>>>
>>> Can I add these override settings (reg_width and reg_shift) just before
>>> the sanity checks?
>> Well, as you may have seen, things are getting complicated here:
>> The two currently-last elements are permitted only with
>> CONFIG_HAS_PCI, so by adding the new fields to the end we'd
>> end up having two syntaxes (one with and one without PCI
>> support). I therefore have to modify my original proposal, and
>> ask for the addition to be done earlier, perhaps - using a
>> separator other than comma (maybe colon or semicolon) - with
>> the [<io-base>|pci|amt] element (as that's really the item
>> (possibly implicitly) specifying the I/O range, which you mean to
>> amend.
> 
> Yes, that's true. We have a MMIO-based (non-PCI) UART device which needs a
> 4-byte wide access (reg_width) and a reg_shift for specifying offsets. So,
> adding options for reg_width and reg_shift with a semi-colon/colon after the
> <io_base> is a good option.
> 
> So, just for clarification, PCI based serial devices should definitely not
> have the reg_width and reg_shift overrides? They should be options available
> only with the <io_base> mentioned?

No, I don't think so - PCI devices may well be matched by the
param_default entry.

Jan


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

Re: [PATCH v2] ns16550: Add command line parsing adjustments

[Jan Beulich]

>>> On 06.01.17 at 19:34, <ian.jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [PATCH v2] ns16550: Add command line parsing 
> adjustments"):
>> On 06.01.17 at 17:24, <swapnil.paratey@amd.com> wrote:
>> Well, as you may have seen, things are getting complicated here:
>> The two currently-last elements are permitted only with
>> CONFIG_HAS_PCI, so by adding the new fields to the end we'd
>> end up having two syntaxes (one with and one without PCI
>> support). I therefore have to modify my original proposal, and
>> ask for the addition to be done earlier, perhaps - using a
>> separator other than comma (maybe colon or semicolon) - with
>> the [<io-base>|pci|amt] element (as that's really the item
>> (possibly implicitly) specifying the I/O range, which you mean to
>> amend.
> 
> Why not introduce the concept of names for these endlessly multiplying
> parameters ?

That's a good ides - we should indeed consider this, but then for all
of the present sub-options (retaining the nameless variant just for
backwards compatibility, and maybe only for a couple of releases.
I'm not sure Swapnil is up to a full overhaul of this parsing ...

Jan


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

Re: [PATCH v2] ns16550: Add command line parsing adjustments

[Swapnil Paratey]

On 01/09/2017 02:10 AM, Jan Beulich wrote:

>>>> On 06.01.17 at 19:34, <ian.jackson@eu.citrix.com> wrote:
>> Jan Beulich writes ("Re: [PATCH v2] ns16550: Add command line parsing
>> adjustments"):
>>> On 06.01.17 at 17:24, <swapnil.paratey@amd.com> wrote:
>>> Well, as you may have seen, things are getting complicated here:
>>> The two currently-last elements are permitted only with
>>> CONFIG_HAS_PCI, so by adding the new fields to the end we'd
>>> end up having two syntaxes (one with and one without PCI
>>> support). I therefore have to modify my original proposal, and
>>> ask for the addition to be done earlier, perhaps - using a
>>> separator other than comma (maybe colon or semicolon) - with
>>> the [<io-base>|pci|amt] element (as that's really the item
>>> (possibly implicitly) specifying the I/O range, which you mean to
>>> amend.
>> Why not introduce the concept of names for these endlessly multiplying
>> parameters ?
> That's a good ides - we should indeed consider this, but then for all
> of the present sub-options (retaining the nameless variant just for
> backwards compatibility, and maybe only for a couple of releases.
> I'm not sure Swapnil is up to a full overhaul of this parsing ...
>
> Jan
>
I don't mind giving it a try.

Could you'll elaborate on the concept of names? A clear syntax of the command
line parsing options would be really helpful.

Does this mean we are adding another option? For example:
console = com1, vga
console_device = pci OR mmio OR legacyIO
com1 = 115200/3000000,8n1,0xfedc9000,4,<reg_width>,<reg_shift>

If you'll can elaborate on what to achieve, I would love to try it out.

Swapnil


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

Re: [PATCH v2] ns16550: Add command line parsing adjustments

[Jan Beulich]

>>> On 09.01.17 at 16:40, <swapnil.paratey@amd.com> wrote:
> On 01/09/2017 02:10 AM, Jan Beulich wrote:
> 
>>>>> On 06.01.17 at 19:34, <ian.jackson@eu.citrix.com> wrote:
>>> Jan Beulich writes ("Re: [PATCH v2] ns16550: Add command line parsing
>>> adjustments"):
>>>> On 06.01.17 at 17:24, <swapnil.paratey@amd.com> wrote:
>>>> Well, as you may have seen, things are getting complicated here:
>>>> The two currently-last elements are permitted only with
>>>> CONFIG_HAS_PCI, so by adding the new fields to the end we'd
>>>> end up having two syntaxes (one with and one without PCI
>>>> support). I therefore have to modify my original proposal, and
>>>> ask for the addition to be done earlier, perhaps - using a
>>>> separator other than comma (maybe colon or semicolon) - with
>>>> the [<io-base>|pci|amt] element (as that's really the item
>>>> (possibly implicitly) specifying the I/O range, which you mean to
>>>> amend.
>>> Why not introduce the concept of names for these endlessly multiplying
>>> parameters ?
>> That's a good ides - we should indeed consider this, but then for all
>> of the present sub-options (retaining the nameless variant just for
>> backwards compatibility, and maybe only for a couple of releases.
>> I'm not sure Swapnil is up to a full overhaul of this parsing ...
>>
> I don't mind giving it a try.
> 
> Could you'll elaborate on the concept of names? A clear syntax of the command
> line parsing options would be really helpful.
> 
> Does this mean we are adding another option? For example:
> console = com1, vga
> console_device = pci OR mmio OR legacyIO
> com1 = 115200/3000000,8n1,0xfedc9000,4,<reg_width>,<reg_shift>
> 
> If you'll can elaborate on what to achieve, I would love to try it out.

See e.g. the cpufreq=, i.e. we're talking of

com1=<name1>:<value1>,<name2>:<value2>,...

(unless Ian had something different in mind). I have to admit that
for some of the items here I'm not immediately seeing good choices
for <name> ...

Jan


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

Re: [PATCH v2] ns16550: Add command line parsing adjustments

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v2] ns16550: Add command line parsing adjustments"):
> See e.g. the cpufreq=, i.e. we're talking of
> 
> com1=<name1>:<value1>,<name2>:<value2>,...

I think we will have to use `=', so
  com1=<name1>=<value1>,<name2>=<value2>,...

Since we already have BDFs which I think contain `:', and that would
be more confusing than the `nested' equals signs.

Also it would be good to support a few positional parameters.  Most
people would want to write something like
   com1=115200,8n1,irq=7

Ian.

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

Re: [PATCH v2] ns16550: Add command line parsing adjustments

[Jan Beulich]

>>> On 09.01.17 at 17:34, <ian.jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [PATCH v2] ns16550: Add command line parsing 
> adjustments"):
>> See e.g. the cpufreq=, i.e. we're talking of
>> 
>> com1=<name1>:<value1>,<name2>:<value2>,...
> 
> I think we will have to use `=', so
>   com1=<name1>=<value1>,<name2>=<value2>,...
> 
> Since we already have BDFs which I think contain `:', and that would
> be more confusing than the `nested' equals signs.

Oh, indeed. Otoh these two sub-options then would b prefixed
by <name>= (or <name>:) too, so there would be visual
separation. Another option would be to allow both : and = as
top level separators.

> Also it would be good to support a few positional parameters.  Most
> people would want to write something like
>    com1=115200,8n1,irq=7

Right, I had been thinking of this too, but then wasn't sure to
suggest such a mixture. Perhaps the first two options could
even remain completely nameless then (I think those are the
ones which had been there from the beginning)?

Jan


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

Re: [PATCH v2] ns16550: Add command line parsing adjustments

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH v2] ns16550: Add command line parsing adjustments"):
> Right, I had been thinking of this too, but then wasn't sure to
> suggest such a mixture. Perhaps the first two options could
> even remain completely nameless then (I think those are the
> ones which had been there from the beginning)?

We have to support comma-separated lists for backward compatibility,
anyway.  It's probably easier to support
  ( existing positional parameter ',' )*  ( name '=' value ',' )*
than to split the whole thing into two parsers.

So I just suggested to Swapnil on irc that they should implement their
additional two parameters as named parameters which could be suffixed
onto the existing comma-separated string.  I don't want to ask them to
implement a complete conversion, but once the framework is there we
can easily support (for example) irq setting both positionally and by
name.

Ian.

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

Re: [PATCH v2] ns16550: Add command line parsing adjustments

[Jan Beulich]

>>> On 09.01.17 at 17:58, <ian.jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [PATCH v2] ns16550: Add command line parsing 
> adjustments"):
>> Right, I had been thinking of this too, but then wasn't sure to
>> suggest such a mixture. Perhaps the first two options could
>> even remain completely nameless then (I think those are the
>> ones which had been there from the beginning)?
> 
> We have to support comma-separated lists for backward compatibility,
> anyway.  It's probably easier to support
>   ( existing positional parameter ',' )*  ( name '=' value ',' )*
> than to split the whole thing into two parsers.

Just seen that, which of course is fine.

> So I just suggested to Swapnil on irc that they should implement their
> additional two parameters as named parameters which could be suffixed
> onto the existing comma-separated string.  I don't want to ask them to
> implement a complete conversion, but once the framework is there we
> can easily support (for example) irq setting both positionally and by
> name.

Right - limiting this to just their addition was my suggestion too,
if he's not up to doing the full thing.

Jan


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