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.loz...@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.loz...@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.loz...@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.loz...@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.loz...@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.loz...@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.loz...@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.loz...@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
