[RFC] dm: uclass: add functions to get device by platdata

[Walter Lozano]

Hi Simon

On 6/3/20 17:32, Simon Glass wrote:
> Hi Walter,
>
> On Fri, 6 Mar 2020 at 09:10, Walter Lozano <walter.lozano at collabora.com> wrote:
>> Hi Simon,
>>
>> Thanks again for taking the time to check my comments.
>>
>> On 6/3/20 10:17, Simon Glass wrote:
>>> Hi Walter,
>>>
>>> On Thu, 5 Mar 2020 at 06:54, Walter Lozano <walter.lozano at collabora.com> wrote:
>>>> Hi Simon,
>>>>
>>>> Thanks for taking the time to check for my comments
>>>>
>>>> On 4/3/20 20:11, Simon Glass wrote:
>>>>
>>>>> Hi Walter,
>>>>>
>>>>> On Wed, 4 Mar 2020 at 12:40, Walter Lozano <walter.lozano at collabora.com> wrote:
>>>>>> When OF_PLATDATA is enabled DT information is parsed and platdata
>>>>>> structures are populated. In this context the links between DT nodes are
>>>>>> represented as pointers to platdata structures, and there is no clear way
>>>>>> to access to the device which owns the structure.
>>>>>>
>>>>>> This patch implements a set of functions:
>>>>>>
>>>>>> - device_find_by_platdata
>>>>>> - uclass_find_device_by_platdata
>>>>>>
>>>>>> to access to the device.
>>>>>>
>>>>>> Signed-off-by: Walter Lozano <walter.lozano at collabora.com>
>>>>>> ---
>>>>>>     drivers/core/device.c        | 19 +++++++++++++++++++
>>>>>>     drivers/core/uclass.c        | 34 ++++++++++++++++++++++++++++++++++
>>>>>>     include/dm/device.h          |  2 ++
>>>>>>     include/dm/uclass-internal.h |  3 +++
>>>>>>     include/dm/uclass.h          |  2 ++
>>>>>>     5 files changed, 60 insertions(+)
>>>>> This is interesting. Could you also add the motivation for this? It's
>>>>> not clear to me who would call this function.
>>>> I have been reviewing the OF_PLATDATA support as an R&D project, in this context, in order to have
>>>> a better understanding on the possibilities and limitations I decided to add its support to iMX6,
>>>> more particularly to the MMC drivers. The link issue arises when I tried to setup the GPIO for
>>>> Card Detection, which is trivial when DT is available. However, when OF_PLATDATA is enabled
>>>> this seems, at least for me, not straightforward.
>>>>
>>>> In order to overcome this limitation I think that having a set of functions to find/get devices
>>>> based on platdata could be useful. Of course, there might be a better approach/idea, so that is
>>>> the motivation for this RFC.
>>>>
>>>> An example of the usage could be
>>>>
>>>> #if CONFIG_IS_ENABLED(DM_GPIO)
>>>>
>>>>            struct udevice *gpiodev;
>>>>
>>>>            ret = uclass_get_device_by_platdata(UCLASS_GPIO, (void *)dtplat->cd_gpios->node, &gpiodev);
>>>>
>>>>            if (ret)
>>>>                    return ret;
>>>>
>>>>            ret = gpio_dev_request_index(gpiodev, gpiodev->name, "cd-gpios",
>>>>                                         dtplat->cd_gpios->arg[0], GPIOD_IS_IN,
>>>>                                         dtplat->cd_gpios->arg[1], &priv->cd_gpio);
>>>>
>>>>            if (ret)
>>>>                    return ret;
>>>>
>>>> #endif
>>>>
>>>> This is part of my current work, a series of patches to add OF_PLATDATA support as explained.
>>>>
>>>> Does this make sense to you?
>>> Not yet :-)
>>>
>>> What is the context of this call? Typically dtplat is only available
>>> in the driver that includes it.
>> Sorry for not being clear enough. I'm working in a patchset that needs
>> some clean up, that is the reason I didn't send it yet. I'll try to
>> clarify, but if you think it could be useful to share it, please let me
>> know.
>>
>>> What driver is the above code in? Is it for MMC that needs a GPIO to
>>> function? I'll assume it is for now.
>> The driver on which I'm working in is drivers/mmc/fsl_esdhc_imx.c, I'm
>> adding support for OF_PLATDATA to it, and in this sense trying to get
>> the GPIOs used for CD to be requested.
>>
>>> Then the weird thing is that we are accessing the dtplat of another
>>> device. It's a clever technique but I wonder if we can find another
>>> way.
>>>
>>> If you see drivers/mmc/rockchip_sdhci.c it has:
>>>
>>> ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &clk);
>>>
>>> So I wonder if we need gpio_dev_request_by_platdata()?
>> Thanks for pointing to this example, as I saw it before starting to work
>> on these functions and had some doubts. I'll use it in the next
>> paragraph to share my thoughts and the motivation of my work.
>>
>>   From my understanding, clk_get_by_index_platdata in this context can
>> only get a UCLASS_CLK device with id == 0. Is this correct?
>>
>> If it is so, this will only allow us to use this function if we know in
>> advance that the UCLASS_CLK device has index 0.
>>
>> How can we get the correct UCLASS_CLK device in case of multiple instances?
> We actually can't support that at present. I think we would need to
> change the property to be an array, like:
>
>     clocks = [
>         [&clk1, CLK_ID_SPI],
>         [&clk1, CLK_ID_I2C, 23],
>       ]
>
> which would need a fancier dtoc. Perhaps we should start by
> upstreaming that tool.

In this case, are you suggesting to replace CLK_ID_SPI and CLK_ID_I2C 
with a integer calculated by dtoc based on the clocks entries available? 
If I understand correctly, what we need is to get the index parameter to 
feed the function uclass_find_device. Is this correct?


>> I understand that we need a way to use the link information present in
>> platdata. However I could not find a way to get the actual index of the
>> UCLASS_CLK device. In this context, I thought that the simplest but
>> still valid approach could be to find the right device based on the
>> struct platdata pointer it owns.
> The index should be the first value after the phandle. If you check
> the output of dtoc you should see it.
>
>> So in my understanding, your code could be more generic in this way
>>
>> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
>> index 71878474eb..61041bb3b8 100644
>> --- a/drivers/clk/clk-uclass.c
>> +++ b/drivers/clk/clk-uclass.c
>> @@ -25,14 +25,12 @@ static inline const struct clk_ops *clk_dev_ops(struct udevice *dev)
>>
>>    #if CONFIG_IS_ENABLED(OF_CONTROL)
>>    # if CONFIG_IS_ENABLED(OF_PLATDATA)
>> -int clk_get_by_index_platdata(struct udevice *dev, int index,
>> -                             struct phandle_1_arg *cells, struct clk *clk)
>> +int clk_get_by_platdata(struct udevice *dev, struct phandle_1_arg *cells,
>> +                       struct clk *clk)
>>    {
>>           int ret;
>>
>> -       if (index != 0)
>> -               return -ENOSYS;
>> -       ret = uclass_get_device(UCLASS_CLK, 0, &clk->dev);
>> +       ret = uclass_get_device_by_platdata(UCLASS_CLK (void *)cells->node, &clk->dev);
>>           if (ret)
>>                   return ret;
>>           clk->id = cells[0].arg[0];
>>
>>
>> I understand there could be a more elegant way, which I still don't see,
>> that is the motivation of this RFC.
>>
>> What do you think?
> Yes I see, but I think it is better to enhance dtoc if needed, to give
> us the info we need.

I understand. When I first reviewed the work to be done and dtoc tool 
source code I asked myself some questions. Let me share my thoughts 
using rock_defconfig as reference.

The link information regarding phandles is processed by dtoc tool. By 
default the phandle_id is converted to fdt32_t, but in case of clocks 
the function

get_phandle_argc(self, prop, node_name)

resolves the link and generates a pointer to the dt_struct of the linked 
node.

So without the special treatment for clocks a dt_struct looks like

static const struct dtd_rockchip_rk3188_dmc dtv_dmc_at_20020000 = {
         .clocks                 = {0x2, 0x160, 0x2, 0x161},

And with the special treatment the phandle_id gets converted to a pointer

static const struct dtd_rockchip_rk3188_dmc dtv_dmc_at_20020000 = {
         .clocks                 = {
                         {&dtv_clock_controller_at_20000000, {352}},
                         {&dtv_clock_controller_at_20000000, {353}},},


This approach was what encouraged me to try to find the device which 
owns dtv_clock_controller_at_20000000 pointer or something similar.

However, I was also thinking that other possibility is to keep dtoc 
simple and don't process this information at all, leaving the 
phandle_id, and also adding the phandle_id in each node dt_struct, in 
order to be able to get/find the device by phandle_id.

I understand that this approach is NOT what you thought it was the best 
for some reason I am not aware of.

So in my mind there should be a generic way to get/find a device based 
on some information in the dt_struct, valid for clocks, gpios and any 
other type of device/node as the phandle_id. In the specific case I'm 
working on, the cd-gpios property should allow us to get/find the gpio 
device to check for the status of the input gpio.

Before you suggested me to add more information to dtoc, but it is not 
clear to me what info to add to have a generic solution related to 
linked nodes and phandles.


>>>>> Also it relates to another thing I've been thinking about for a while,
>>>>> which is to validate that all the structs pointed to are correct.
>>>>>
>>>>> E.g. if every struct had a magic number like:
>>>>>
>>>>> struct tpm_platdata {
>>>>>        DM_STRUCT(UCLASS_TPM, DM_STRUCT_PLATDATA, ...)
>>>>>        fields here
>>>>> };
>>>>>
>>>>> then we could check the structure pointers are correct.
>>>>>
>>>>> DM_STRUCT() would define to nothing if we were not building with
>>>>> CONFIG_DM_DEBUG or similar.
>>>> Interesting, I think it could be useful and save us from headaches while debugging.
>>>>
>>>> Thanks for sharing this idea.
>>>>
>>>>> Anyway, I wonder whether you could expand your definition a bit so you
>>>>> have an enum for the different types of struct you can request:
>>>>>
>>>>> enum dm_struct_t {
>>>>>       DM_STRUCT_PLATDATA,
>>>>>     ...
>>>>>
>>>>>       DM_STRUCT_COUNT,
>>>>> };
>>>>>
>>>>> and modify the function so it can request it via the enum?
>>>> Let me check if I understand correctly, your suggestion is to do
>>>> something like diff --git a/include/dm/uclass.h b/include/dm/uclass.h
>>>> index 92c07f8426..bf09dadf3f 100644 --- a/include/dm/uclass.h +++
>>>> b/include/dm/uclass.h
>>>>
>>>> @@ -167,8 +167,8 @@ int uclass_get_device(enum uclass_id id, int index,
>>>> struct udevice **devp);
>>>>
>>>>     int uclass_get_device_by_name(enum uclass_id id, const char *name,
>>>>                                  struct udevice **devp); -int
>>>> uclass_get_device_by_platdata(enum uclass_id id, void *platdata,
>>>> -                             struct udevice **devp);
>>>>
>>>> +int uclass_get_device_by_struct(enum uclass_id id, enum dm_struct_t
>>>> struct_id, +                             void *struct_pointer, struct
>>>> udevice **devp);  /**   * uclass_get_device_by_seq() - Get a uclass
>>>> device based on an ID and sequence   *
>>>>
>>>> If that is the case, I would be happy to help.
>>>>
>>>> Also, if my understanding is correct, could you elaborate which cases
>>>> are you trying to cover with this approach? Regards,
>>> This is just so that in dev_get_priv(), for example, we can write a
>>> check that dev->privdata is actually the expected struct. We can check
>>> the uclass and the dm_struct_t.
>>>
>>> It is really just a run-time assert to help with getting these things mixed up.
>> So if I understand correctly I think that if that the approach that I
>> have in mind is really useful, which I intend to validate with this RFC,
>> your suggestion is to add some run-time checks, to make sure that in the
>> above example cells->node is a valid platdata? Is my understanding is
>> correct?
> Yes, the purpose of the checks is completely different from your goal.
> It just happens that your function is something that would help there.

Thanks for clarifying, now I get a better understanding of you comments.

Regards,

Walter