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@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@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@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 >> Lozanowalter.lozano@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 >>>> Lozanowalter.lozano@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 >>>>>> Lozanowalter.lozano@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 >>>>>>>> Lozanowalter.lozano@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 >>>>>>>>>> Lozanowalter.lozano@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 Lozanowalter.lozano@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