[RFC] dm: uclass: add functions to get device by platdata
Walter Lozano
walter.lozano at collabora.com
Fri May 8 12:08:34 CEST 2020
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.
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.
>> 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.
Regards,
Walter
More information about the U-Boot
mailing list