| Message ID | 20200513201345.26769-3-walter.lozano@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | improve OF_PLATDATA support | expand |
Hi Walter, On Wed, 13 May 2020 at 14:14, Walter Lozano <walter.lozano at collabora.com> wrote: > > Currently when creating an U_BOOT_DEVICE entry a struct driver_info > is declared, which contains the data needed to instantiate the device. > However, the actual device is created at runtime and there is no proper > way to get the device based on its struct driver_info. > > This patch extends struct driver_info adding a pointer to udevice which > is populated during the bind process, allowing to generate a set of > functions to get the device based on its struct driver_info. > > Signed-off-by: Walter Lozano <walter.lozano at collabora.com> > --- > drivers/core/device.c | 25 +++++++++++++++++++++++-- > drivers/core/root.c | 6 +++++- > include/dm/device-internal.h | 2 +- > include/dm/device.h | 13 +++++++++++++ > include/dm/platdata.h | 6 ++++++ > 5 files changed, 48 insertions(+), 4 deletions(-) > > diff --git a/drivers/core/device.c b/drivers/core/device.c > index 0157bb1fe0..2476f170a5 100644 > --- a/drivers/core/device.c > +++ b/drivers/core/device.c > @@ -246,10 +246,11 @@ int device_bind_ofnode(struct udevice *parent, const struct driver *drv, > } > > int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, > - const struct driver_info *info, struct udevice **devp) > + struct driver_info *info, struct udevice **devp) > { > struct driver *drv; > uint platdata_size = 0; > + int ret = 0; > > drv = lists_driver_lookup_name(info->name); > if (!drv) > @@ -260,9 +261,19 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, > #if CONFIG_IS_ENABLED(OF_PLATDATA) > platdata_size = info->platdata_size; > #endif > - return device_bind_common(parent, drv, info->name, > + ret = device_bind_common(parent, drv, info->name, > (void *)info->platdata, 0, ofnode_null(), platdata_size, > devp); > + drop blank line > + if (ret) > + return ret; > + > +#if CONFIG_IS_ENABLED(OF_PLATDATA) > + info->dev = *devp; > +#endif > + > + return ret; > + and here > } > > static void *alloc_priv(int size, uint flags) > @@ -727,6 +738,16 @@ int device_get_global_by_ofnode(ofnode ofnode, struct udevice **devp) > return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp); > } > > +#if CONFIG_IS_ENABLED(OF_PLATDATA) > +int device_get_by_driver_info(struct driver_info * info, struct udevice **devp) > +{ > + struct udevice *dev; > + > + dev = info->dev; blank line before return:-) > + return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp); > +} > +#endif > + > int device_find_first_child(const struct udevice *parent, struct udevice **devp) > { > if (list_empty(&parent->child_head)) { > diff --git a/drivers/core/root.c b/drivers/core/root.c > index 14df16c280..8f47a6b356 100644 > --- a/drivers/core/root.c > +++ b/drivers/core/root.c > @@ -25,7 +25,7 @@ > > DECLARE_GLOBAL_DATA_PTR; > > -static const struct driver_info root_info = { > +static struct driver_info root_info = { > .name = "root_driver", > }; > > @@ -346,6 +346,10 @@ int dm_init_and_scan(bool pre_reloc_only) > { > int ret; > > +#if CONFIG_IS_ENABLED(OF_PLATDATA) This one can use if() I think > + populate_phandle_data(); > +#endif > + > ret = dm_init(IS_ENABLED(CONFIG_OF_LIVE)); > if (ret) { > debug("dm_init() failed: %d\n", ret); > diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h > index 294d6c1810..5145fb4e14 100644 > --- a/include/dm/device-internal.h > +++ b/include/dm/device-internal.h > @@ -81,7 +81,7 @@ int device_bind_with_driver_data(struct udevice *parent, > * @return 0 if OK, -ve on error > */ > int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, > - const struct driver_info *info, struct udevice **devp); > + struct driver_info *info, struct udevice **devp); That change should really be a separate patch I think. > > /** > * device_ofdata_to_platdata() - Read platform data for a device > diff --git a/include/dm/device.h b/include/dm/device.h > index d5b3e732c3..590c1f99ce 100644 > --- a/include/dm/device.h > +++ b/include/dm/device.h > @@ -537,6 +537,19 @@ int device_find_global_by_ofnode(ofnode node, struct udevice **devp); > */ > int device_get_global_by_ofnode(ofnode node, struct udevice **devp); > > +/** > + * device_get_by_driver_info() - Get a device based on driver_info > + * > + * Locates a device by its struct driver_info. > + * > + * The device is probed to activate it ready for use. > + * > + * @info: Struct driver_info > + * @devp: Returns pointer to device if found, otherwise this is set to NULL > + * @return 0 if OK, -ve on error > + */ > +int device_get_by_driver_info(struct driver_info * info, struct udevice **devp); > + > /** > * device_find_first_child() - Find the first child of a device > * > diff --git a/include/dm/platdata.h b/include/dm/platdata.h > index c972fa6936..17eef7c7a3 100644 > --- a/include/dm/platdata.h > +++ b/include/dm/platdata.h > @@ -28,6 +28,7 @@ struct driver_info { > const void *platdata; > #if CONFIG_IS_ENABLED(OF_PLATDATA) > uint platdata_size; > + struct udevice *dev; Please also update comment for this struct > #endif > }; > > @@ -43,4 +44,9 @@ struct driver_info { > #define U_BOOT_DEVICES(__name) \ > ll_entry_declare_list(struct driver_info, __name, driver_info) > > +/* Get a pointer to a given driver */ > +#define DM_GET_DEVICE(__name) \ > + ll_entry_get(struct driver_info, __name, driver_info) > + > +void populate_phandle_data(void); Function comment. Also can this ever fail? > #endif > -- > 2.20.1 > Regards, SImon
Hi Simon, On 20/5/20 00:07, Simon Glass wrote: > Hi Walter, > > On Wed, 13 May 2020 at 14:14, Walter Lozano<walter.lozano at collabora.com> wrote: >> Currently when creating an U_BOOT_DEVICE entry a struct driver_info >> is declared, which contains the data needed to instantiate the device. >> However, the actual device is created at runtime and there is no proper >> way to get the device based on its struct driver_info. >> >> This patch extends struct driver_info adding a pointer to udevice which >> is populated during the bind process, allowing to generate a set of >> functions to get the device based on its struct driver_info. >> >> Signed-off-by: Walter Lozano<walter.lozano at collabora.com> >> --- >> drivers/core/device.c | 25 +++++++++++++++++++++++-- >> drivers/core/root.c | 6 +++++- >> include/dm/device-internal.h | 2 +- >> include/dm/device.h | 13 +++++++++++++ >> include/dm/platdata.h | 6 ++++++ >> 5 files changed, 48 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/core/device.c b/drivers/core/device.c >> index 0157bb1fe0..2476f170a5 100644 >> --- a/drivers/core/device.c >> +++ b/drivers/core/device.c >> @@ -246,10 +246,11 @@ int device_bind_ofnode(struct udevice *parent, const struct driver *drv, >> } >> >> int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, >> - const struct driver_info *info, struct udevice **devp) >> + struct driver_info *info, struct udevice **devp) >> { >> struct driver *drv; >> uint platdata_size = 0; >> + int ret = 0; >> >> drv = lists_driver_lookup_name(info->name); >> if (!drv) >> @@ -260,9 +261,19 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, >> #if CONFIG_IS_ENABLED(OF_PLATDATA) >> platdata_size = info->platdata_size; >> #endif >> - return device_bind_common(parent, drv, info->name, >> + ret = device_bind_common(parent, drv, info->name, >> (void *)info->platdata, 0, ofnode_null(), platdata_size, >> devp); >> + > drop blank line Sure. >> + if (ret) >> + return ret; >> + >> +#if CONFIG_IS_ENABLED(OF_PLATDATA) >> + info->dev = *devp; >> +#endif >> + >> + return ret; >> + > and here Sure. >> } >> >> static void *alloc_priv(int size, uint flags) >> @@ -727,6 +738,16 @@ int device_get_global_by_ofnode(ofnode ofnode, struct udevice **devp) >> return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp); >> } >> >> +#if CONFIG_IS_ENABLED(OF_PLATDATA) >> +int device_get_by_driver_info(struct driver_info * info, struct udevice **devp) >> +{ >> + struct udevice *dev; >> + >> + dev = info->dev; > blank line before return:-) Sure, thanks! >> + return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp); >> +} >> +#endif >> + >> int device_find_first_child(const struct udevice *parent, struct udevice **devp) >> { >> if (list_empty(&parent->child_head)) { >> diff --git a/drivers/core/root.c b/drivers/core/root.c >> index 14df16c280..8f47a6b356 100644 >> --- a/drivers/core/root.c >> +++ b/drivers/core/root.c >> @@ -25,7 +25,7 @@ >> >> DECLARE_GLOBAL_DATA_PTR; >> >> -static const struct driver_info root_info = { >> +static struct driver_info root_info = { >> .name = "root_driver", >> }; >> >> @@ -346,6 +346,10 @@ int dm_init_and_scan(bool pre_reloc_only) >> { >> int ret; >> >> +#if CONFIG_IS_ENABLED(OF_PLATDATA) > This one can use if() I think I think that as populate_phandle_data() is only defined when OF_PLATDATA is enabled (as it is created by dtoc), this will trigger and link error on those board which don't use this config. Is my understanding correct? >> + populate_phandle_data(); >> +#endif >> + >> ret = dm_init(IS_ENABLED(CONFIG_OF_LIVE)); >> if (ret) { >> debug("dm_init() failed: %d\n", ret); >> diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h >> index 294d6c1810..5145fb4e14 100644 >> --- a/include/dm/device-internal.h >> +++ b/include/dm/device-internal.h >> @@ -81,7 +81,7 @@ int device_bind_with_driver_data(struct udevice *parent, >> * @return 0 if OK, -ve on error >> */ >> int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, >> - const struct driver_info *info, struct udevice **devp); >> + struct driver_info *info, struct udevice **devp); > That change should really be a separate patch I think. If we move this change to a separate patch, this will fail to compile as struct driver_info needs to be updated by device_bind_by_name to fill info->dev. I can move both things to a different patch, in that case. Please advice. >> /** >> * device_ofdata_to_platdata() - Read platform data for a device >> diff --git a/include/dm/device.h b/include/dm/device.h >> index d5b3e732c3..590c1f99ce 100644 >> --- a/include/dm/device.h >> +++ b/include/dm/device.h >> @@ -537,6 +537,19 @@ int device_find_global_by_ofnode(ofnode node, struct udevice **devp); >> */ >> int device_get_global_by_ofnode(ofnode node, struct udevice **devp); >> >> +/** >> + * device_get_by_driver_info() - Get a device based on driver_info >> + * >> + * Locates a device by its struct driver_info. >> + * >> + * The device is probed to activate it ready for use. >> + * >> + * @info: Struct driver_info >> + * @devp: Returns pointer to device if found, otherwise this is set to NULL >> + * @return 0 if OK, -ve on error >> + */ >> +int device_get_by_driver_info(struct driver_info * info, struct udevice **devp); >> + >> /** >> * device_find_first_child() - Find the first child of a device >> * >> diff --git a/include/dm/platdata.h b/include/dm/platdata.h >> index c972fa6936..17eef7c7a3 100644 >> --- a/include/dm/platdata.h >> +++ b/include/dm/platdata.h >> @@ -28,6 +28,7 @@ struct driver_info { >> const void *platdata; >> #if CONFIG_IS_ENABLED(OF_PLATDATA) >> uint platdata_size; >> + struct udevice *dev; > Please also update comment for this struct Sure >> #endif >> }; >> >> @@ -43,4 +44,9 @@ struct driver_info { >> #define U_BOOT_DEVICES(__name) \ >> ll_entry_declare_list(struct driver_info, __name, driver_info) >> >> +/* Get a pointer to a given driver */ >> +#define DM_GET_DEVICE(__name) \ >> + ll_entry_get(struct driver_info, __name, driver_info) >> + >> +void populate_phandle_data(void); > Function comment. Also can this ever fail? It shouldn't. It basically does something like this void populate_phandle_data(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); dtv_serial_at_20064000.clocks[0].node = DM_GET_DEVICE(clock_controller_at_20000000); dtv_serial_at_20064000.clocks[1].node = DM_GET_DEVICE(clock_controller_at_20000000); } as we discuss previously, it is all about to be able to use DM_GET_DEVICE. >> #endif >> -- >> 2.20.1 >> Regards, Walter
Hi Walter, On Thu, 21 May 2020 at 07:15, Walter Lozano <walter.lozano at collabora.com> wrote: > > Hi Simon, > > On 20/5/20 00:07, Simon Glass wrote: > > Hi Walter, > > > > On Wed, 13 May 2020 at 14:14, Walter Lozano<walter.lozano at collabora.com> wrote: > >> Currently when creating an U_BOOT_DEVICE entry a struct driver_info > >> is declared, which contains the data needed to instantiate the device. > >> However, the actual device is created at runtime and there is no proper > >> way to get the device based on its struct driver_info. > >> > >> This patch extends struct driver_info adding a pointer to udevice which > >> is populated during the bind process, allowing to generate a set of > >> functions to get the device based on its struct driver_info. > >> > >> Signed-off-by: Walter Lozano<walter.lozano at collabora.com> > >> --- > >> drivers/core/device.c | 25 +++++++++++++++++++++++-- > >> drivers/core/root.c | 6 +++++- > >> include/dm/device-internal.h | 2 +- > >> include/dm/device.h | 13 +++++++++++++ > >> include/dm/platdata.h | 6 ++++++ > >> 5 files changed, 48 insertions(+), 4 deletions(-) > >> [..] > >> @@ -346,6 +346,10 @@ int dm_init_and_scan(bool pre_reloc_only) > >> { > >> int ret; > >> > >> +#if CONFIG_IS_ENABLED(OF_PLATDATA) > > This one can use if() I think > > I think that as populate_phandle_data() is only defined when OF_PLATDATA > is enabled (as it is created by dtoc), this will trigger and link error > on those board which don't use this config. Is my understanding correct? Hopefully not. The linker should drop code that is not called, so it won't become an issue. > > >> + populate_phandle_data(); > >> +#endif > >> + > >> ret = dm_init(IS_ENABLED(CONFIG_OF_LIVE)); > >> if (ret) { > >> debug("dm_init() failed: %d\n", ret); > >> diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h > >> index 294d6c1810..5145fb4e14 100644 > >> --- a/include/dm/device-internal.h > >> +++ b/include/dm/device-internal.h > >> @@ -81,7 +81,7 @@ int device_bind_with_driver_data(struct udevice *parent, > >> * @return 0 if OK, -ve on error > >> */ > >> int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, > >> - const struct driver_info *info, struct udevice **devp); > >> + struct driver_info *info, struct udevice **devp); > > That change should really be a separate patch I think. > > If we move this change to a separate patch, this will fail to compile as > struct driver_info needs to be updated by device_bind_by_name to fill > info->dev. I can move both things to a different patch, in that case. > > Please advice. Yes I just mean that here you are taking away the 'const' and I think that little bit should be in its own patch. So take away the const, but do nothing else. [..] Regards. Simon
diff --git a/drivers/core/device.c b/drivers/core/device.c index 0157bb1fe0..2476f170a5 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -246,10 +246,11 @@ int device_bind_ofnode(struct udevice *parent, const struct driver *drv, } int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, - const struct driver_info *info, struct udevice **devp) + struct driver_info *info, struct udevice **devp) { struct driver *drv; uint platdata_size = 0; + int ret = 0; drv = lists_driver_lookup_name(info->name); if (!drv) @@ -260,9 +261,19 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, #if CONFIG_IS_ENABLED(OF_PLATDATA) platdata_size = info->platdata_size; #endif - return device_bind_common(parent, drv, info->name, + ret = device_bind_common(parent, drv, info->name, (void *)info->platdata, 0, ofnode_null(), platdata_size, devp); + + if (ret) + return ret; + +#if CONFIG_IS_ENABLED(OF_PLATDATA) + info->dev = *devp; +#endif + + return ret; + } static void *alloc_priv(int size, uint flags) @@ -727,6 +738,16 @@ int device_get_global_by_ofnode(ofnode ofnode, struct udevice **devp) return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp); } +#if CONFIG_IS_ENABLED(OF_PLATDATA) +int device_get_by_driver_info(struct driver_info * info, struct udevice **devp) +{ + struct udevice *dev; + + dev = info->dev; + return device_get_device_tail(dev, dev ? 0 : -ENOENT, devp); +} +#endif + int device_find_first_child(const struct udevice *parent, struct udevice **devp) { if (list_empty(&parent->child_head)) { diff --git a/drivers/core/root.c b/drivers/core/root.c index 14df16c280..8f47a6b356 100644 --- a/drivers/core/root.c +++ b/drivers/core/root.c @@ -25,7 +25,7 @@ DECLARE_GLOBAL_DATA_PTR; -static const struct driver_info root_info = { +static struct driver_info root_info = { .name = "root_driver", }; @@ -346,6 +346,10 @@ int dm_init_and_scan(bool pre_reloc_only) { int ret; +#if CONFIG_IS_ENABLED(OF_PLATDATA) + populate_phandle_data(); +#endif + ret = dm_init(IS_ENABLED(CONFIG_OF_LIVE)); if (ret) { debug("dm_init() failed: %d\n", ret); diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h index 294d6c1810..5145fb4e14 100644 --- a/include/dm/device-internal.h +++ b/include/dm/device-internal.h @@ -81,7 +81,7 @@ int device_bind_with_driver_data(struct udevice *parent, * @return 0 if OK, -ve on error */ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, - const struct driver_info *info, struct udevice **devp); + struct driver_info *info, struct udevice **devp); /** * device_ofdata_to_platdata() - Read platform data for a device diff --git a/include/dm/device.h b/include/dm/device.h index d5b3e732c3..590c1f99ce 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -537,6 +537,19 @@ int device_find_global_by_ofnode(ofnode node, struct udevice **devp); */ int device_get_global_by_ofnode(ofnode node, struct udevice **devp); +/** + * device_get_by_driver_info() - Get a device based on driver_info + * + * Locates a device by its struct driver_info. + * + * The device is probed to activate it ready for use. + * + * @info: Struct driver_info + * @devp: Returns pointer to device if found, otherwise this is set to NULL + * @return 0 if OK, -ve on error + */ +int device_get_by_driver_info(struct driver_info * info, struct udevice **devp); + /** * device_find_first_child() - Find the first child of a device * diff --git a/include/dm/platdata.h b/include/dm/platdata.h index c972fa6936..17eef7c7a3 100644 --- a/include/dm/platdata.h +++ b/include/dm/platdata.h @@ -28,6 +28,7 @@ struct driver_info { const void *platdata; #if CONFIG_IS_ENABLED(OF_PLATDATA) uint platdata_size; + struct udevice *dev; #endif }; @@ -43,4 +44,9 @@ struct driver_info { #define U_BOOT_DEVICES(__name) \ ll_entry_declare_list(struct driver_info, __name, driver_info) +/* Get a pointer to a given driver */ +#define DM_GET_DEVICE(__name) \ + ll_entry_get(struct driver_info, __name, driver_info) + +void populate_phandle_data(void); #endif
Currently when creating an U_BOOT_DEVICE entry a struct driver_info is declared, which contains the data needed to instantiate the device. However, the actual device is created at runtime and there is no proper way to get the device based on its struct driver_info. This patch extends struct driver_info adding a pointer to udevice which is populated during the bind process, allowing to generate a set of functions to get the device based on its struct driver_info. Signed-off-by: Walter Lozano <walter.lozano at collabora.com> --- drivers/core/device.c | 25 +++++++++++++++++++++++-- drivers/core/root.c | 6 +++++- include/dm/device-internal.h | 2 +- include/dm/device.h | 13 +++++++++++++ include/dm/platdata.h | 6 ++++++ 5 files changed, 48 insertions(+), 4 deletions(-)