[RFC] dm: uclass: add functions to get device by platdata
Walter Lozano
walter.lozano at collabora.com
Tue Apr 7 22:05:14 CEST 2020
Hi Simon,
On 6/4/20 00:43, Simon Glass wrote:
> Hi Walter,
>
> On Mon, 9 Mar 2020 at 12:27, Walter Lozano<walter.lozano at collabora.com> wrote:
>> 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?
> No, I was thinking that it would be the same CLK_ID_SPI value from the binding.
>
> We currently have (see the 'rock' board):
>
> struct dtd_rockchip_rk3188_uart {
> fdt32_t clock_frequency;
> struct phandle_1_arg clocks[2];
> fdt32_t interrupts[3];
> fdt32_t reg[2];
> fdt32_t reg_io_width;
> fdt32_t reg_shift;
> };
>
> So the phandle_1_arg is similar to what you want. It could use phandle_2_arg.
>
> Since the array has two elements, there is room for two clocks.
Now is clear, thanks.
>>>> 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.
> I think at present it is very simple. E.g. see
> clk_get_by_index_platdata() which only supports a 1-arg phandle and
> always uses the first available clock device.
>
>> 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.
> Well you don't have the device tree with of-platdata, so you cannot
> look up a phandle. I suppose we could add the phandle into the
> structures but we would need to know how to find it generically.
>
>> 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.
> OK I see.
>
> DM_GET_DRIVER() is a compile-time way to find a driver. I wonder if we
> could have a compile-time way to find a device?
>
> It should be possible since each U_BOOT_DEVICE() get s a symbol and
> the symbols are named methodically. So we can actually find the device
> at compile time.
>
> So how about we have DM_GET_DEVICE(name) in that field. That way you
> have the device pointer available, with no lookup needed.
I found this approach very interesting. Let me investigate it and I'll
get back to you. I really appreciate this suggestion, I hope to come
with something useful.
>
>> 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.
> It isn't clear to me either. It needs some thought. But hopefully what
> I said above puts you on the right track?
Yes, indeed. Please let me investigate your suggestion about
DM_GET_DEVICE(name), and then we can discuss further.
>
>>>>>>> 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.
> OK.
Thank you.
> Regards,
> Simon
Regards,
Walter
More information about the U-Boot
mailing list