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

[swapnil <swapnil.paratey@xxxxxxx>]

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

> > > > On 05.01.17 at 23:39, <swapnil.paratey@xxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel