[RFC] dm: uclass: add functions to get device by platdata
Walter Lozano
walter.lozano at collabora.com
Wed May 6 14:57:11 CEST 2020
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 at 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 at 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.lozano at 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.lozano at 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.lozano at 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.lozano at 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.lozano at 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.lozano at 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*
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
More information about the U-Boot
mailing list