| Message ID | 1483655974-3280-1-git-send-email-swapnil.paratey@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> 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
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
>>> 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
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
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.
>>> 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
>>> 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
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
>>> 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
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.
>>> 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
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.
>>> 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
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);