[RFC] dm: uclass: add functions to get device by platdata
Simon Glass
sjg at chromium.org
Fri May 8 15:18:01 CEST 2020
Hi Walter,
On Fri, 8 May 2020 at 04:08, Walter Lozano <walter.lozano at collabora.com> wrote:
>
> Hi Simon,
>
> On 7/5/20 22:36, Simon Glass wrote:
> > Hi Walter,
> >
> > On Wed, 6 May 2020 at 06:57, Walter Lozano <walter.lozano at collabora.com> wrote:
> >> Hi Simon,
> >>
> >> On 23/4/20 00:38, Walter Lozano wrote:
> >>> Hi Simon,
> >>>
> >>> On 21/4/20 14:36, Simon Glass wrote:
> >>>> 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.
> >>> Thanks for the explanation, I think I now fully understand your
> >>> approach. I will work on it and let you know.
> >>>
> >>>
> >>> Again, thanks for your time.
> >>>
> >> I have finally managed to have some time to work on this feature, sorry
> >> for the long delay.
> >>
> >>
> >> In order to test this approach I've
> >>
> >> - added DM_GET_DEVICE
> >>
> >> - updated dtoc in order to use DM_GET_DEVICE when populated phandle
> >>
> >> with this in new features the spl/dts/dt-platdata.c looks like
> >>
> >> static const struct dtd_rockchip_rk3188_dmc dtv_dmc_at_20020000 = {
> >> .clocks = {
> >> {DM_GET_DEVICE(clock_controller_at_20000000), {352}},
> >> {DM_GET_DEVICE(clock_controller_at_20000000), {353}},},
> >> .reg = {0x20020000, 0x3fc, 0x20040000, 0x294},
> >> .rockchip_cru = 0x2,
> >> .rockchip_grf = 0x7,
> >> .rockchip_noc = 0x14,
> >> .rockchip_pctl_timing = {0x12c, 0xc8, 0x1f4, 0x1e, 0x4e, 0x4, 0x69, 0x6,
> >> 0x3, 0x0, 0x6, 0x5, 0xc, 0x10, 0x6, 0x4,
> >> 0x4, 0x5, 0x4, 0x200, 0x3, 0xa, 0x40, 0x0,
> >> 0x1, 0x5, 0x5, 0x3, 0xc, 0x1e, 0x100, 0x0,
> >> 0x4, 0x0},
> >> .rockchip_phy_timing = {0x208c6690, 0x690878, 0x10022a00, 0x220, 0x40, 0x0, 0x0},
> >> .rockchip_pmu = 0x13,
> >> .rockchip_sdram_params = {0x24716310, 0x0, 0x2, 0x11e1a300, 0x3, 0x9, 0x0},
> >> };
> >>
> >> which I think is what you suggested or at least in that direction.
> >>
> >> However, this code does not compile due to
> >>
> >> In file included from include/command.h:14:0,
> >> from include/image.h:45,
> >> from include/common.h:40,
> >> from spl/dts/dt-platdata.c:7:
> >> include/linker_lists.h:206:2: error: braced-group within expression allowed only inside a function
> >> ({ \
> >> ^
> >> include/dm/platdata.h:48:2: note: in expansion of macro ‘ll_entry_get’
> >> ll_entry_get(struct driver_info, __name, driver_info)
> >> ^~~~~~~~~~~~
> >> spl/dts/dt-platdata.c:50:5: note: in expansion of macro ‘DM_GET_DEVICE’
> >> {DM_GET_DEVICE(clock_controller_at_20000000), {352}},
> >> ^~~~~~~~~~~~~
> >>
> >> which is clear when examining the code for *ll_entry_get*
> > Yes...unfortunately the devices are allocated at run-time. It is the
> > driver struct that is allocated at build-time.
>
> Yes, we are in sync.
>
> >> I haven't found a proper fix for this, the options I currently see are:
> >>
> >> 1- Populate phandle data with the device name which matches the
> >> U_BOOT_DEVICE entry, and do a runtime lookup. This is not a nice
> >> approach as it uses strings, as we previously discussed.
> >>
> >> 2- Populate the phandle with the struct driver_info at runtime, with
> >> some code generated by dtoc on spl/dts/dt-platdata.c, something like
> >>
> >> void populate_phandle(void) {
> >> dtv_dmc_at_20020000.clocks[0].node = DM_GET_DEVICE(clock_controller_at_20000000);
> >> dtv_dmc_at_20020000.clocks[1].node = DM_GET_DEVICE(clock_controller_at_20000000);
> >> }
> >>
> >>
> >> the additional issue is that I would need to drop then const from
> >> dtv_dmc_at_20020000
> > Yes that seems like the right idea. The 'const' is probably not a good
> > idea anyway, if it stops us linking up the data structures.
> >
> > If you'd like me to fiddle with it a bit, point me to your patches and
> > I can have a try. It seems like you are close though.
>
> Thanks for your check my comments and confirm that I'm on the right
> track. I'll prepare a new version and send a series. At this point I
> would like to ask if you think it this better to move this patches to
> its own series, and then prepare a specific series about OF_PLATDATA on
> iMX6/Cuboxi.
Yes I think it should have its own series.
>
> It is also interesting to think if there are some other improvements we
> can do generating more specific code with dtoc. I'll keep thinking in that.
>
Yes indeed.
> >> 3- Keep my original approach
> >>
> >>
> >> I would like, if possible, to know your opinions and suggestions. Thanks
> >> in advance.
>
> Once again, thanks for your help and time.
You're welcome, good luck!
Regards,
Simon
More information about the U-Boot
mailing list