Re: [RFC PATCH 1/3] of: base: Introduce of_alias_check_id() to check alias IDs

[Michal Simek]

On 27.4.2018 15:02, Rob Herring wrote:
> On Fri, Apr 27, 2018 at 1:10 AM, Michal Simek <michal.simek@xxxxxxxxxx> wrote:
>> On 27.4.2018 04:39, Rob Herring wrote:
>>> On Thu, Apr 26, 2018 at 9:08 AM, Michal Simek <michal.simek@xxxxxxxxxx> wrote:
>>>> The function travers the lookup table to check if the request alias
>>>> id is compatible with the device driver match structure.
>>>> This function will be used by serial drivers to check if requested alias
>>>> is allocated or free to use.
>>>>
>>>> Signed-off-by: Michal Simek <michal.simek@xxxxxxxxxx>
>>>> ---
>>>>
>>>>  drivers/of/base.c  | 49 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/linux/of.h |  2 ++
>>>>  2 files changed, 51 insertions(+)
>>>>
>>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>>> index 848f549164cd..382de01acc72 100644
>>>> --- a/drivers/of/base.c
>>>> +++ b/drivers/of/base.c
>>>> @@ -1892,6 +1892,55 @@ int of_alias_get_id(struct device_node *np, const char *stem)
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(of_alias_get_id);
>>>>
>>>> +/**
>>>> + * of_alias_check_id - Check alias id for the give compatibility
>>>> + * @matches:   Array of of device match structures to search in
>>>> + * @stem:      Alias stem of the given device_node
>>>> + * @id:                Alias ID for checking
>>>> + *
>>>> + * The function travers the lookup table to check if the request alias id
>>>> + * is compatible with the device driver match structure
>>>> + *
>>>> + * Return true if ID is allocated, return false if not
>>>> + */
>>>> +bool of_alias_check_id(const struct of_device_id *matches, const char *stem,
>>>> +                      int id)
>>>
>>> Wouldn't it be simpler to just return a bitmap of all allocated ids
>>> that match rather than trying to build that up 1 bit at a time?
>>
>> Is alias list stable or can dt overlay change it?
> 
> Stable. If you use an id and then an overlay adds that id as an alias,
> what should the kernel do? The only choice is ignore the suggestion.

I didn't play with that for a while but on fpga case you have for
example serial0/serial1 fixed to hard IPs/PS part and then you add 20
uartlites or 16550 IPs to PL via overlays and you want to make sure
proper order for user application.
It means it is not rewriting current but adding new one exactly how you
should do that when fixed and fpga parts are together.


> 
>> What should be the expected flow? Find out maximum number of aliases of
>> the same kind and allocate bitmap and return it with length.
> 
> I think we can assume <32 so just return a bitmap in a unsigned long
> or u32. Use that to initialize a bitmap in the driver. For un-aliased
> devices, find the first unset bit, use that id and set the bit.

Ok if this is in driver field can be allocated in the driver and passed
also maximum len. It should be better then expecting maximum of 32
devices which ends with max serial31 alias.

Or at least expect u64 with max serial63.


>> Anyway if you look at that patches I sent then I call in the driver
>> of_alias_get_highest_id("serial") which doesn't take care if alias match
>> with actual driver. It means having information about max alias ID which
>> match actual driver that would be helpful but I am not quite sure what
>> should be the flow.
> 
> I'm a bit confused as to why you use of_alias_get_highest_id and
> of_alias_check_id. of_alias_get_highest_id is really intended if  you
> want to start dynamic allocations above all the alias ids.

I need to get maximum number of ports for passing it to
uart_register_driver which is called only once.
And using of_alias_get_highest_id() return me reasonable number for all
listed serial ports.
It is not correct because some IPs don't need to be listed in that list
but it is reasonable expectation for this RFC.
Even alias bitfield won't help with getting correct number.


The best will be to get number of devices which match that driver
because they don't need to be listed in aliases list.

You get 20 of devices and will allocate space for 20. If there is alias
below 20 you will use numbers. If there is alias >= 20 it will be
ignored in probe and number will be assigned based on empty position.

But this is also no covering overlay cases where others devices can be
added at run time.


> I guess the need to match devices is based on each serial driver
> having it's own major and /dev name. IMO, we should get rid of those
> and every serial driver use ttySn (with the exception of things like
> USB serial). Then you'd just need to find all serial aliases. But you
> can support both with this function. Just return a bitmap of all
> aliases if match is NULL.

This will break a lot of things which are stable today.


>> Any link to similar function would be good to understand how the flow is
>> supposed to work.
> 
> I don't think there's a good model. The pl011 code looks broken to me.

yes, it is broken.

> The warning about devices with and without aliases should be fixed.

The same solution which

> That is perfectly valid and should be supported. You should really
> only need an alias for your console.

Having aliases is quite good in case of multiple the same uarts where if
you have more then 2 it is hard to find out which one is the one
connected to certain devices.
With uartlite or ns16550 and PL where you can have unlimited number of
devices this is really must to be able to work with this system.

Thanks,
Michal