[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.