[RFC] dm: uclass: add functions to get device by platdata
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@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(+)
diff --git a/drivers/core/device.c b/drivers/core/device.c index 89ea820d48..54a3a8d870 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -591,6 +591,25 @@ static int device_find_by_ofnode(ofnode node, struct udevice **devp) } #endif
+ +int device_find_by_platdata(void *platdata, struct udevice **devp) +{ + struct uclass *uc; + struct udevice *dev; + int ret; + + list_for_each_entry(uc, &gd->uclass_root, sibling_node) { + ret = uclass_find_device_by_platdata(uc->uc_drv->id, platdata, + &dev); + if (!ret || dev) { + *devp = dev; + return 0; + } + } + + return -ENODEV; +} + int device_get_child(const struct udevice *parent, int index, struct udevice **devp) { diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index 58b19a4210..7b0ae5b122 100644 --- a/drivers/core/uclass.c +++ b/drivers/core/uclass.c @@ -271,6 +271,29 @@ int uclass_find_device_by_name(enum uclass_id id, const char *name, return -ENODEV; }
+int uclass_find_device_by_platdata(enum uclass_id id, void * platdata, struct udevice **devp) +{ + struct uclass *uc; + struct udevice *dev; + int ret; + + *devp = NULL; + ret = uclass_get(id, &uc); + if (ret) + return ret; + if (list_empty(&uc->dev_head)) + return -ENODEV; + + uclass_foreach_dev(dev, uc) { + if (dev->platdata == platdata) { + *devp = dev; + return 0; + } + } + + return -ENODEV; +} + int uclass_find_next_free_req_seq(enum uclass_id id) { struct uclass *uc; @@ -466,6 +489,17 @@ int uclass_get_device_by_name(enum uclass_id id, const char *name, return uclass_get_device_tail(dev, ret, devp); }
+int uclass_get_device_by_platdata(enum uclass_id id, void * platdata, + struct udevice **devp) +{ + struct udevice *dev; + int ret; + + *devp = NULL; + ret = uclass_find_device_by_platdata(id, platdata, &dev); + return uclass_get_device_tail(dev, ret, devp); +} + int uclass_get_device_by_seq(enum uclass_id id, int seq, struct udevice **devp) { struct udevice *dev; diff --git a/include/dm/device.h b/include/dm/device.h index ab806d0b7e..3c043a3127 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -396,6 +396,8 @@ enum uclass_id device_get_uclass_id(const struct udevice *dev); */ const char *dev_get_uclass_name(const struct udevice *dev);
+int device_find_by_platdata(void *platdata, struct udevice **devp); + /** * device_get_child() - Get the child of a device by index * diff --git a/include/dm/uclass-internal.h b/include/dm/uclass-internal.h index 6e3f15c2b0..f643fc95da 100644 --- a/include/dm/uclass-internal.h +++ b/include/dm/uclass-internal.h @@ -100,6 +100,9 @@ int uclass_find_next_device(struct udevice **devp); int uclass_find_device_by_name(enum uclass_id id, const char *name, struct udevice **devp);
+int uclass_find_device_by_platdata(enum uclass_id id, void *platdata, + struct udevice **devp); + /** * uclass_find_device_by_seq() - Find uclass device based on ID and sequence * diff --git a/include/dm/uclass.h b/include/dm/uclass.h index 70fca79b44..92c07f8426 100644 --- a/include/dm/uclass.h +++ b/include/dm/uclass.h @@ -167,6 +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); /** * uclass_get_device_by_seq() - Get a uclass device based on an ID and sequence *
Hi Walter,
On Wed, 4 Mar 2020 at 12:40, Walter Lozano walter.lozano@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@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.
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.
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?
Regards, Simon
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@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@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?
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,
Walter
Hi Simon,
On 5/3/20 10:54, Walter Lozano wrote:
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?
Sorry, it looks like the last part of the email test got screw.
If I understand correctly, your suggestion is to change
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 trying to cover with this approach?
Regards,
Walter
Hi Walter,
On Thu, 5 Mar 2020 at 06:54, Walter Lozano walter.lozano@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@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@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.
What driver is the above code in? Is it for MMC that needs a GPIO to function? I'll assume it is for now.
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()?
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.
Regards, Simon
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@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@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@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?
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.
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?
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?
Regards, Simon
Hi Walter,
On Fri, 6 Mar 2020 at 09:10, Walter Lozano walter.lozano@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@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@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@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.
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)
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.
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.
Regards, Simon
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@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@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@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@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?
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)
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.
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.
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.
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.
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); -intuclass_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.
Regards,
Walter
Hi Walter,
On Mon, 9 Mar 2020 at 12:27, Walter Lozano walter.lozano@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@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@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@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@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.
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)
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.
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?
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); -intuclass_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
Hi Simon,
On 6/4/20 00:43, Simon Glass wrote:
Hi Walter,
On Mon, 9 Mar 2020 at 12:27, Walter Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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)
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.
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); -intuclass_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.
Thank you.
Regards, Simon
Regards,
Walter
Hi Walter,
On Tue, 7 Apr 2020 at 14:05, Walter Lozano walter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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)
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.
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
Hi Simon,
On 8/4/20 00:14, Simon Glass wrote:
Hi Walter,
On Tue, 7 Apr 2020 at 14:05, Walter Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 canonly 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)
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?
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
HI Walter,
On Thu, 9 Apr 2020 at 12:57, Walter Lozano walter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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().
Regards, Simon
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
On 9/4/20 16:36, Simon Glass wrote:
HI Walter,
On Thu, 9 Apr 2020 at 12:57, Walter Lozano walter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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().
What I was thinking in adding a pointer to platdata structure to the device it owns it, which should be filled in device_bind(), but after re thinking about it, it is clear that you approach is more elegant.
I'll investigate this and come back to you. Thanks for sharing.
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
Regards,
Walter
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@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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?
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
Hi Walter,
On Fri, 17 Apr 2020 at 15:18, Walter Lozano walter.lozano@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@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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.
Regards, Simon
[..]
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@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@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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 Lozanowalter.lozano@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.
Regards,
Walter
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@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@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 Lozanowalter.lozano@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 >> Lozanowalter.lozano@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 >>>> Lozanowalter.lozano@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 >>>>>> Lozanowalter.lozano@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 >>>>>>>> Lozanowalter.lozano@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 Lozanowalter.lozano@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
Hi Walter,
On Wed, 6 May 2020 at 06:57, Walter Lozano walter.lozano@collabora.com wrote:
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@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@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 > Lozanowalter.lozano@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 >>> Lozanowalter.lozano@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 >>>>> Lozanowalter.lozano@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 >>>>>>> Lozanowalter.lozano@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 >>>>>>>>> Lozanowalter.lozano@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 Lozanowalter.lozano@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*
Yes...unfortunately the devices are allocated at run-time. It is the driver struct that is allocated at build-time.
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
Yes that seems like the right idea. The 'const' is probably not a good idea anyway, if it stops us linking up the data structures.
If you'd like me to fiddle with it a bit, point me to your patches and I can have a try. It seems like you are close though.
3- Keep my original approach
I would like, if possible, to know your opinions and suggestions. Thanks in advance.
Regards, Simon
Hi Simon,
On 7/5/20 22:36, Simon Glass wrote:
Hi Walter,
On Wed, 6 May 2020 at 06:57, Walter Lozano walter.lozano@collabora.com wrote:
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@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@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 >> Lozanowalter.lozano@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 >>>> Lozanowalter.lozano@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 >>>>>> Lozanowalter.lozano@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 >>>>>>>> Lozanowalter.lozano@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 >>>>>>>>>> Lozanowalter.lozano@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 Lozanowalter.lozano@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*
Yes...unfortunately the devices are allocated at run-time. It is the driver struct that is allocated at build-time.
Yes, we are in sync.
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
Yes that seems like the right idea. The 'const' is probably not a good idea anyway, if it stops us linking up the data structures.
If you'd like me to fiddle with it a bit, point me to your patches and I can have a try. It seems like you are close though.
Thanks for your check my comments and confirm that I'm on the right track. I'll prepare a new version and send a series. At this point I would like to ask if you think it this better to move this patches to its own series, and then prepare a specific series about OF_PLATDATA on iMX6/Cuboxi.
It is also interesting to think if there are some other improvements we can do generating more specific code with dtoc. I'll keep thinking in that.
3- Keep my original approach
I would like, if possible, to know your opinions and suggestions. Thanks in advance.
Once again, thanks for your help and time.
Regards,
Walter
Hi Walter,
On Fri, 8 May 2020 at 04:08, Walter Lozano walter.lozano@collabora.com wrote:
Hi Simon,
On 7/5/20 22:36, Simon Glass wrote:
Hi Walter,
On Wed, 6 May 2020 at 06:57, Walter Lozano walter.lozano@collabora.com wrote:
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@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@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 >>> Lozanowalter.lozano@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 >>>>> Lozanowalter.lozano@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 >>>>>>> Lozanowalter.lozano@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 >>>>>>>>> Lozanowalter.lozano@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 >>>>>>>>>>> Lozanowalter.lozano@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 Lozanowalter.lozano@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*
Yes...unfortunately the devices are allocated at run-time. It is the driver struct that is allocated at build-time.
Yes, we are in sync.
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
Yes that seems like the right idea. The 'const' is probably not a good idea anyway, if it stops us linking up the data structures.
If you'd like me to fiddle with it a bit, point me to your patches and I can have a try. It seems like you are close though.
Thanks for your check my comments and confirm that I'm on the right track. I'll prepare a new version and send a series. At this point I would like to ask if you think it this better to move this patches to its own series, and then prepare a specific series about OF_PLATDATA on iMX6/Cuboxi.
Yes I think it should have its own series.
It is also interesting to think if there are some other improvements we can do generating more specific code with dtoc. I'll keep thinking in that.
Yes indeed.
3- Keep my original approach
I would like, if possible, to know your opinions and suggestions. Thanks in advance.
Once again, thanks for your help and time.
You're welcome, good luck!
Regards, Simon
participants (2)
-
Simon Glass -
Walter Lozano