From mboxrd@z Thu Jan 1 00:00:00 1970 From: Walter Lozano Date: Fri, 17 Apr 2020 18:18:18 -0300 Subject: [RFC] dm: uclass: add functions to get device by platdata In-Reply-To: References: <20200304194006.30924-1-walter.lozano@collabora.com> <3be13b3e-0273-f66d-1c3c-7f8da45cbc59@collabora.com> <98f7f566-5501-d5bf-3f07-35497d3ffa32@collabora.com> Message-ID: 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 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? Regards, Walter >>>>>> Before you suggested me to add more information to dtoc, but it is not >>>>>> clear to me what info to add to have a generic solution related to >>>>>> linked nodes and phandles. >>>>> It isn't clear to me either. It needs some thought. But hopefully what >>>>> I said above puts you on the right track? >>>> Yes, indeed. Please let me investigate your suggestion about >>>> DM_GET_DEVICE(name), and then we can discuss further. >>>> >>>> >>>>>>>>>>> Also it relates to another thing I've been thinking about for a while, >>>>>>>>>>> which is to validate that all the structs pointed to are correct. >>>>>>>>>>> >>>>>>>>>>> E.g. if every struct had a magic number like: >>>>>>>>>>> >>>>>>>>>>> struct tpm_platdata { >>>>>>>>>>> DM_STRUCT(UCLASS_TPM, DM_STRUCT_PLATDATA, ...) >>>>>>>>>>> fields here >>>>>>>>>>> }; >>>>>>>>>>> >>>>>>>>>>> then we could check the structure pointers are correct. >>>>>>>>>>> >>>>>>>>>>> DM_STRUCT() would define to nothing if we were not building with >>>>>>>>>>> CONFIG_DM_DEBUG or similar. >>>>>>>>>> Interesting, I think it could be useful and save us from headaches while debugging. >>>>>>>>>> >>>>>>>>>> Thanks for sharing this idea. >>>>>>>>>> >>>>>>>>>>> Anyway, I wonder whether you could expand your definition a bit so you >>>>>>>>>>> have an enum for the different types of struct you can request: >>>>>>>>>>> >>>>>>>>>>> enum dm_struct_t { >>>>>>>>>>> DM_STRUCT_PLATDATA, >>>>>>>>>>> ... >>>>>>>>>>> >>>>>>>>>>> DM_STRUCT_COUNT, >>>>>>>>>>> }; >>>>>>>>>>> >>>>>>>>>>> and modify the function so it can request it via the enum? >>>>>>>>>> Let me check if I understand correctly, your suggestion is to do >>>>>>>>>> something like diff --git a/include/dm/uclass.h b/include/dm/uclass.h >>>>>>>>>> index 92c07f8426..bf09dadf3f 100644 --- a/include/dm/uclass.h +++ >>>>>>>>>> b/include/dm/uclass.h >>>>>>>>>> >>>>>>>>>> @@ -167,8 +167,8 @@ int uclass_get_device(enum uclass_id id, int index, >>>>>>>>>> struct udevice **devp); >>>>>>>>>> >>>>>>>>>> int uclass_get_device_by_name(enum uclass_id id, const char *name, >>>>>>>>>> struct udevice **devp); -int >>>>>>>>>> uclass_get_device_by_platdata(enum uclass_id id, void *platdata, >>>>>>>>>> - struct udevice **devp); >>>>>>>>>> >>>>>>>>>> +int uclass_get_device_by_struct(enum uclass_id id, enum dm_struct_t >>>>>>>>>> struct_id, + void *struct_pointer, struct >>>>>>>>>> udevice **devp); /** * uclass_get_device_by_seq() - Get a uclass >>>>>>>>>> device based on an ID and sequence * >>>>>>>>>> >>>>>>>>>> If that is the case, I would be happy to help. >>>>>>>>>> >>>>>>>>>> Also, if my understanding is correct, could you elaborate which cases >>>>>>>>>> are you trying to cover with this approach? Regards, >>>>>>>>> This is just so that in dev_get_priv(), for example, we can write a >>>>>>>>> check that dev->privdata is actually the expected struct. We can check >>>>>>>>> the uclass and the dm_struct_t. >>>>>>>>> >>>>>>>>> It is really just a run-time assert to help with getting these things mixed up. >>>>>>>> So if I understand correctly I think that if that the approach that I >>>>>>>> have in mind is really useful, which I intend to validate with this RFC, >>>>>>>> your suggestion is to add some run-time checks, to make sure that in the >>>>>>>> above example cells->node is a valid platdata? Is my understanding is >>>>>>>> correct? >>>>>>> Yes, the purpose of the checks is completely different from your goal. >>>>>>> It just happens that your function is something that would help there. >>>>>> Thanks for clarifying, now I get a better understanding of you comments. >>>>> OK. >>> Regards, >>> Simon >> Regards, >> >> Walter >>