Hi Walter, On Fri, 17 Apr 2020 at 15:18, Walter Lozano <[email protected]> wrote: > > Hi Simon, > > On 9/4/20 16:36, Simon Glass wrote: > > HI Walter, > > > > On Thu, 9 Apr 2020 at 12:57, Walter Lozano <[email protected]> > > wrote: > >> Hi Simon, > >> > >> On 8/4/20 00:14, Simon Glass wrote: > >>> Hi Walter, > >>> > >>> On Tue, 7 Apr 2020 at 14:05, Walter Lozano<[email protected]> > >>> wrote: > >>>> Hi Simon, > >>>> > >>>> On 6/4/20 00:43, Simon Glass wrote: > >>>>> Hi Walter, > >>>>> > >>>>> On Mon, 9 Mar 2020 at 12:27, Walter Lozano<[email protected]> > >>>>> wrote: > >>>>>> Hi Simon > >>>>>> > >>>>>> On 6/3/20 17:32, Simon Glass wrote: > >>>>>>> Hi Walter, > >>>>>>> > >>>>>>> On Fri, 6 Mar 2020 at 09:10, Walter > >>>>>>> Lozano<[email protected]> 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<[email protected]> 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<[email protected]> 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<[email protected]> > >>>>>>>>>>>> --- > >>>>>>>>>>>> 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. Regards, Simon [..]
