[RFC] dm: uclass: add functions to get device by platdata

[Simon Glass]

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.

>
>
> >> 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.

>
> 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?

>
>
> >>>>> 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