From mboxrd@z Thu Jan 1 00:00:00 1970 From: Walter Lozano Date: Wed, 6 May 2020 09:57:11 -0300 Subject: [RFC] dm: uclass: add functions to get device by platdata In-Reply-To: <0685e305-7968-0452-938c-1410a614df94@collabora.com> References: <20200304194006.30924-1-walter.lozano@collabora.com> <3be13b3e-0273-f66d-1c3c-7f8da45cbc59@collabora.com> <98f7f566-5501-d5bf-3f07-35497d3ffa32@collabora.com> <0685e305-7968-0452-938c-1410a614df94@collabora.com> Message-ID: <590d9686-5ee9-e6bf-68a3-5cb57686d9ab@collabora.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >> wrote: >>> Hi Simon, >>> >>> On 9/4/20 16:36, Simon Glass wrote: >>>> HI Walter, >>>> >>>> On Thu, 9 Apr 2020 at 12:57, Walter Lozano >>>> wrote: >>>>> Hi Simon, >>>>> >>>>> On 8/4/20 00:14, Simon Glass wrote: >>>>>> Hi Walter, >>>>>> >>>>>> On Tue, 7 Apr 2020 at 14:05, Walter >>>>>> Lozano? wrote: >>>>>>> Hi Simon, >>>>>>> >>>>>>> On 6/4/20 00:43, Simon Glass wrote: >>>>>>>> Hi Walter, >>>>>>>> >>>>>>>> On Mon, 9 Mar 2020 at 12:27, Walter >>>>>>>> Lozano?? wrote: >>>>>>>>> Hi Simon >>>>>>>>> >>>>>>>>> On 6/3/20 17:32, Simon Glass wrote: >>>>>>>>>> Hi Walter, >>>>>>>>>> >>>>>>>>>> On Fri, 6 Mar 2020 at 09:10, Walter >>>>>>>>>> Lozano 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 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 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 >>>>>>>>>>>>>>> --- >>>>>>>>>>>>>>> ??????? 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* 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 3- Keep my original approach I would like, if possible, to know your opinions and suggestions. Thanks in advance. Regards, Walter