[RFC] dm: uclass: add functions to get device by platdata
Simon Glass
sjg at chromium.org
Thu Apr 9 21:36:31 CEST 2020
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().
Regards,
Simon
>
> >>>> 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