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

[Swapnil Paratey <swapnil.paratey@xxxxxxx>]

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

> > > > On 06.01.17 at 17:24, <swapnil.paratey@xxxxxxx> wrote:
> > On 01/06/2017 08:58 AM, Jan Beulich wrote:
> > > > > > On 05.01.17 at 23:39, <swapnil.paratey@xxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel