[RFC] dm: uclass: add functions to get device by platdata
Walter Lozano
walter.lozano at collabora.com
Thu Apr 23 05:38:26 CEST 2020
Hi Simon,
On 21/4/20 14:36, Simon Glass wrote:
> Hi Walter,
>
> On Fri, 17 Apr 2020 at 15:18, Walter Lozano <walter.lozano at collabora.com> wrote:
>> Hi Simon,
>>
>> On 9/4/20 16:36, Simon Glass wrote:
>>> HI Walter,
>>>
>>> On Thu, 9 Apr 2020 at 12:57, Walter Lozano <walter.lozano at collabora.com> wrote:
>>>> Hi Simon,
>>>>
>>>> On 8/4/20 00:14, Simon Glass wrote:
>>>>> Hi Walter,
>>>>>
>>>>> On Tue, 7 Apr 2020 at 14:05, Walter Lozano<walter.lozano at collabora.com> wrote:
>>>>>> 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.
>>>>> Yes me too...
>>>>>
>>>>> But sadly I don't think it is enough. It points to the driver data,
>>>>> the data *used* to create the device, but not the device itself, which
>>>>> is dynamically allocated with malloc().
>>>>>
>>>>> The good news is that it is a compile-time check, so there is some
>>>>> value in the idea. Good to avoid runtime failure if possible.
>>>>>
>>>>> One option would be to add a pointer at run-time from the driver data
>>>>> to the device, for of-platdata. That way we could follow a pointer and
>>>>> find the device. It would be easy enough to do.
>>>> Thank you so much for sharing all these ideas. I hope to make good use
>>>> of these suggestions. I think I'm following your idea, however this will
>>>> be clearer when I start to work on this, hopefully next week, and
>>>> probably will come back to you with some silly questions.
>>>>
>>>> At this point what I see
>>>>
>>>> - I like the compile-time check, you have showed me that benefits with
>>>> several of your previous patches, thanks for that.
>>>>
>>>> - If we need to add a pointer from driver data to the device, why not
>>>> add this pointer to struct platdata instead?
>>> Unfortunately struct udevice is allocated at runtime and so the
>>> address is not available at compile-time.
>>>
>>> I suppose we could statically allocate the 'struct udevice' things in
>>> the dt-platdata.c file and then track them down in device_bind(),
>>> avoiding the malloc().
>>>
>>> But it might be easier (and less different) to add a pointer at
>>> runtime in device_bind().
>> Let me check if I understand you correctly, as I might I have
>> misunderstood you previously. Again I will use your example to have a
>> reference
>>
>> What I see is that I have access to a pointer to
>> dtd_rockchip_rk3188_cru, by &dtv_clock_controller_at_20000000, so I
>> understand that the idea is to extend the dtd_rockchip_rk3188_cru to
>> include a pointer to the device which uses it. This pointer, as you
>> described should be filled at runtime, in device_bind(). So in order to
>> to this we have to
>>
>> - Tweak dtoc to add this new pointer
>>
>> - Populate this data on device_bind()
>>
>> If this is not correct, could you please point me to the correct
>> suggestion using your example?
> I am not suggesting extending dtd_rockchip_rk3188_cru, but to struct
> driver_info. Something like:
>
> struct udevice *dev;
>
> which points to the actual device. It would not be set by dtoc, but
> device_bind() could assign it.
Thanks for the explanation, I think I now fully understand your
approach. I will work on it and let you know.
Again, thanks for your time.
Regards,
Walter
More information about the U-Boot
mailing list