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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.01.17 at 18:28, <swapnil.paratey@xxxxxxx> wrote:
> 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?

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

Jan


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