[RFC] dm: uclass: add functions to get device by platdata
Simon Glass
sjg at chromium.org
Tue Apr 21 19:36:08 CEST 2020
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.
Regards,
Simon
[..]
More information about the U-Boot
mailing list