[RFC] dm: uclass: add functions to get device by platdata
Simon Glass
sjg at chromium.org
Wed Apr 8 05:14:33 CEST 2020
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.
>
> >
> >> 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
More information about the U-Boot
mailing list