[RFC] dm: uclass: add functions to get device by platdata
Walter Lozano
walter.lozano at collabora.com
Thu Apr 9 20:57:36 CEST 2020
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?
>>>> 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