Hi Walter, On Fri, 8 May 2020 at 04:08, Walter Lozano <walter.loz...@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.loz...@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.loz...@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.loz...@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.loz...@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.loz...@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.loz...@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.loz...@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.loz...@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.loz...@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