[RFC] dm: uclass: add functions to get device by platdata
Walter Lozano
walter.lozano at collabora.com
Fri Apr 17 23:18:18 CEST 2020
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?
Regards,
Walter
>>>>>> 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.
>>> Regards,
>>> Simon
>> Regards,
>>
>> Walter
>>
More information about the U-Boot
mailing list