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

[Simon Glass]

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.

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

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

Regards,
Simon