| Message ID | 1416212385-23800-7-git-send-email-yamada.m@jp.panasonic.com |
|---|---|
| State | Rejected |
| Delegated to: | Simon Glass |
| Headers | show |
HI Masahiro, On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote: > The DM initialization function, dm_init_and_scan() is called twice, > before and after relocation. > > In the first DM scan, only some of drivers are bound for the use in > pre-reloc code (mostly UART drivers). In the second scan, all the > drivers are bound. > > In the current implementation, all the drivers are collected into a > single array. Every driver is searched through the array and then > the DM_FLAG_PRE_RELOC flag is checked. In the pre-reloc DM scan, > drivers without DM_FLAG_PRE_RELOC are ignored. This algorithm works > enough, but is not very efficient. > > This commit aims to split drivers into two contiguous arrays, > pre-reloc drivers and post-reloc drivers. The drivers in the former > group are declared with U_BOOT_DRIVER_F. The others are declared with > U_BOOT_DRIVER. > > pre post > _____________________ reloc reloc > .u_boot_list_2_driver1_1 | | || || > | | || || > | | || || > | U_BOOT_DRIVER_F | || || > | (driver1) | || || > | | _||_ || > | | \ / || > .u_boot_list_2_driver1_3 |___________________| \/ || > .u_boot_list_2_driver2_1 | | || > | | || > | | || > | U_BOOT_DRIVER | || > | (driver2) | || > | | _||_ > | | \ / > .u_boot_list_2_driver2_3 |___________________| \/ > > The pre-reloc DM scan searches drivers from .u_boot_list_2_driver1_1 > to .u_boot_list_2_driver1_3. The post-reloc scan searches ones from > .u_boot_list_2_driver1_1 and .u_boot_list_2_driver2_3. > > Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com> > --- > > doc/driver-model/README.txt | 2 +- > drivers/core/device.c | 4 +--- > drivers/core/lists.c | 22 +++++++++++++--------- > drivers/core/root.c | 3 ++- > include/dm/device.h | 8 ++++++-- > include/dm/lists.h | 16 ++++++++++++---- > include/dm/root.h | 16 ++++++++-------- > 7 files changed, 43 insertions(+), 28 deletions(-) > > diff --git a/doc/driver-model/README.txt b/doc/driver-model/README.txt > index 0278dda..e4114d5 100644 > --- a/doc/driver-model/README.txt > +++ b/doc/driver-model/README.txt > @@ -739,7 +739,7 @@ Pre-Relocation Support > ---------------------- > > For pre-relocation we simply call the driver model init function. Only > -drivers marked with DM_FLAG_PRE_RELOC or the device tree > +drivers declared with U_BOOT_DRIVER_F or the device tree > 'u-boot,dm-pre-reloc' flag are initialised prior to relocation. This helps > to reduce the driver model overhead. > > diff --git a/drivers/core/device.c b/drivers/core/device.c > index 49faa29..6fd2f07 100644 > --- a/drivers/core/device.c > +++ b/drivers/core/device.c > @@ -157,11 +157,9 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, > { > struct driver *drv; > > - drv = lists_driver_lookup_name(info->name); > + drv = __lists_driver_lookup_name(info->name, pre_reloc_only); This patch looks good, except that I would prefer lists_driver_lookup_name_prereloc() to __lists_driver_lookup_name(). The __ seems like an internal compiler name to me. So can you rename it? > if (!drv) > return -ENOENT; > - if (pre_reloc_only && !(drv->flags & DM_FLAG_PRE_RELOC)) > - return -EPERM; > > return device_bind(parent, drv, info->name, (void *)info->platdata, > -1, devp); > diff --git a/drivers/core/lists.c b/drivers/core/lists.c > index 1476715..5b61ab5 100644 > --- a/drivers/core/lists.c > +++ b/drivers/core/lists.c > @@ -18,10 +18,13 @@ > #include <fdtdec.h> > #include <linux/compiler.h> > > -struct driver *lists_driver_lookup_name(const char *name) > +#define driver_entry_end(p) (p) ? ll_entry_end(struct driver, driver1) : \ > + ll_entry_end(struct driver, driver2); > + > +struct driver *__lists_driver_lookup_name(const char *name, bool pre_reloc_only) > { > - struct driver *entry = ll_entry_start(struct driver, driver); > - struct driver *end = ll_entry_end(struct driver, driver); > + struct driver *entry = ll_entry_start(struct driver, driver1); > + struct driver *end = driver_entry_end(pre_reloc_only); > > for (; entry < end; entry++) { > if (!strcmp(name, entry->name)) > @@ -58,11 +61,12 @@ int lists_bind_drivers(struct udevice *parent, bool pre_reloc_only) > > for (; entry < end; entry++) { > ret = device_bind_by_name(parent, pre_reloc_only, entry, &dev); > - if (ret && ret != -EPERM) { > + > + if (!pre_reloc_only && ret == -ENOENT) > dm_warn("No match for driver '%s'\n", entry->name); > - if (!result || ret != -ENOENT) > - result = ret; > - } > + > + if (!result || ret != -ENOENT) > + result = ret; > } > > return result; > @@ -105,8 +109,8 @@ static int driver_check_compatible(const void *blob, int offset, > int lists_bind_fdt(struct udevice *parent, const void *blob, int offset, > struct udevice **devp) > { > - struct driver *entry = ll_entry_start(struct driver, driver); > - struct driver *end = ll_entry_end(struct driver, driver); > + struct driver *entry = ll_entry_start(struct driver, driver1); > + struct driver *end = ll_entry_end(struct driver, driver2); > struct udevice *dev; > bool found = false; > const char *name; > diff --git a/drivers/core/root.c b/drivers/core/root.c > index 47b3acf..02970c8 100644 > --- a/drivers/core/root.c > +++ b/drivers/core/root.c > @@ -70,7 +70,8 @@ int dm_scan_platdata(bool pre_reloc_only) > > ret = lists_bind_drivers(DM_ROOT_NON_CONST, pre_reloc_only); > if (ret == -ENOENT) { > - dm_warn("Some drivers were not found\n"); > + if (!pre_reloc_only) > + dm_warn("Some drivers were not found\n"); > ret = 0; > } > > diff --git a/include/dm/device.h b/include/dm/device.h > index 9ce95a8..a2fd7b6 100644 > --- a/include/dm/device.h > +++ b/include/dm/device.h > @@ -167,9 +167,13 @@ struct driver { > uint32_t flags; > }; > > -/* Declare a new U-Boot driver */ > +/* Drivers bound before and after relocation */ > +#define U_BOOT_DRIVER_F(__name) \ > + ll_entry_declare(struct driver, __name, driver1) > + > +/* Drivers bound only after relocation */ > #define U_BOOT_DRIVER(__name) \ > - ll_entry_declare(struct driver, __name, driver) > + ll_entry_declare(struct driver, __name, driver2) > > /** > * dev_get_platdata() - Get the platform data for a device > diff --git a/include/dm/lists.h b/include/dm/lists.h > index 704e33e..fbd428d 100644 > --- a/include/dm/lists.h > +++ b/include/dm/lists.h > @@ -13,15 +13,23 @@ > #include <dm/uclass-id.h> > > /** > - * lists_driver_lookup_name() - Return u_boot_driver corresponding to name > + * __lists_driver_lookup_name() - Return u_boot_driver corresponding to name > * > * This function returns a pointer to a driver given its name. This is used > * for binding a driver given its name and platdata. > * > * @name: Name of driver to look up > + * @pre_reloc_only: If true, searches in drivers declared with U_BOOT_DRIVER_F. > + * If false searches in all drivers. > * @return pointer to driver, or NULL if not found > */ > -struct driver *lists_driver_lookup_name(const char *name); > +struct driver *__lists_driver_lookup_name(const char *name, > + bool pre_reloc_only); > + > +static inline struct driver *lists_driver_lookup_name(const char *name) > +{ > + return __lists_driver_lookup_name(name, false); > +} > > /** > * lists_uclass_lookup() - Return uclass_driver based on ID of the class > @@ -39,8 +47,8 @@ struct uclass_driver *lists_uclass_lookup(enum uclass_id id); > * each one. The devices will have @parent as their parent. > * > * @parent: parent device (root) > - * @early_only: If true, bind only drivers with the DM_INIT_F flag. If false > - * bind all drivers. > + * @pre_reloc_only: If true, bind only drivers declared with U_BOOT_DRIVER_F. > + * If false bind all drivers. > */ > int lists_bind_drivers(struct udevice *parent, bool pre_reloc_only); > > diff --git a/include/dm/root.h b/include/dm/root.h > index c7f0c1d..61d1cd8 100644 > --- a/include/dm/root.h > +++ b/include/dm/root.h > @@ -26,8 +26,8 @@ struct udevice *dm_root(void); > * > * This scans all available platdata and creates drivers for each > * > - * @pre_reloc_only: If true, bind only drivers with the DM_FLAG_PRE_RELOC > - * flag. If false bind all drivers. > + * @pre_reloc_only: If true, bind only drivers declared with U_BOOT_DRIVER_F. > + * If false bind all drivers. > * @return 0 if OK, -ve on error > */ > int dm_scan_platdata(bool pre_reloc_only); > @@ -54,8 +54,8 @@ int dm_scan_fdt(const void *blob, bool pre_reloc_only); > * @parent: Parent device for the devices that will be created > * @blob: Pointer to device tree blob > * @offset: Offset of node to scan > - * @pre_reloc_only: If true, bind only drivers with the DM_FLAG_PRE_RELOC > - * flag. If false bind all drivers. > + * @pre_reloc_only: If true, bind only drivers declared with U_BOOT_DRIVER_F. > + * If false bind all drivers. > * @return 0 if OK, -ve on error > */ > int dm_scan_fdt_node(struct udevice *parent, const void *blob, int offset, > @@ -69,8 +69,8 @@ int dm_scan_fdt_node(struct udevice *parent, const void *blob, int offset, > * programmaticaly. They should do this by calling device_bind() on each > * device. > * > - * @pre_reloc_only: If true, bind only drivers with the DM_FLAG_PRE_RELOC > - * flag. If false bind all drivers. > + * @pre_reloc_only: If true, bind only drivers declared with U_BOOT_DRIVER_F. > + * If false bind all drivers. > */ > int dm_scan_other(bool pre_reloc_only); > > @@ -81,8 +81,8 @@ int dm_scan_other(bool pre_reloc_only); > * then scans and binds available devices from platform data and the FDT. > * This calls dm_init() to set up Driver Model structures. > * > - * @pre_reloc_only: If true, bind only drivers with the DM_FLAG_PRE_RELOC > - * flag. If false bind all drivers. > + * @pre_reloc_only: If true, bind only drivers declared with U_BOOT_DRIVER_F. > + * If false bind all drivers. > * @return 0 if OK, -ve on error > */ > int dm_init_and_scan(bool pre_reloc_only); > -- > 1.9.1 > Regards, Simon
Hi Simon, On Mon, 17 Nov 2014 09:22:19 +0000 Simon Glass <sjg@chromium.org> wrote: > > --- a/drivers/core/device.c > > +++ b/drivers/core/device.c > > @@ -157,11 +157,9 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, > > { > > struct driver *drv; > > > > - drv = lists_driver_lookup_name(info->name); > > + drv = __lists_driver_lookup_name(info->name, pre_reloc_only); > > This patch looks good, except that I would prefer > lists_driver_lookup_name_prereloc() to __lists_driver_lookup_name(). > The __ seems like an internal compiler name to me. So can you rename > it? Indeed. I think __ should be used carefully especially when it comes to host programs, but I think we can play it by ear in standalone binaries such as the kernel and U-boot code. I often see "__" prefixes in Linux and in my understanding, __foo() is a "use it carefully" version or locally used interface of foo() (for example, when the resources are not protected by spinlocks, etc.). So, I often use it when I cannot invent a good func name. Hmm, lists_driver_lookup_name_prereloc() is already too long and __prereloc is a bit misleading because it is used both before and after relocation, isn't it? Best Regards Masahiro Yamada
Hi Masahiro, On 17 November 2014 11:49, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote: > Hi Simon, > > > On Mon, 17 Nov 2014 09:22:19 +0000 > Simon Glass <sjg@chromium.org> wrote: >> > --- a/drivers/core/device.c >> > +++ b/drivers/core/device.c >> > @@ -157,11 +157,9 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, >> > { >> > struct driver *drv; >> > >> > - drv = lists_driver_lookup_name(info->name); >> > + drv = __lists_driver_lookup_name(info->name, pre_reloc_only); >> >> This patch looks good, except that I would prefer >> lists_driver_lookup_name_prereloc() to __lists_driver_lookup_name(). >> The __ seems like an internal compiler name to me. So can you rename >> it? > > Indeed. I think __ should be used carefully especially when it comes to > host programs, but I think we can play it by ear in standalone binaries > such as the kernel and U-boot code. > > I often see "__" prefixes in Linux and in my understanding, > __foo() is a "use it carefully" version or locally used interface of foo() > (for example, when the resources are not protected by spinlocks, etc.). > So, I often use it when I cannot invent a good func name. > > Hmm, lists_driver_lookup_name_prereloc() is already too long > and __prereloc is a bit misleading because it is used both before and after relocation, > isn't it? OK that seems fair enough if used sparingly. I can't think of a great name either and we don't really want to pollute the code with the pre_reloc_only parameter since it will be used in many places. Acked-by: Simon Glass <sjg@chromium.org> Regards, Simon
diff --git a/doc/driver-model/README.txt b/doc/driver-model/README.txt index 0278dda..e4114d5 100644 --- a/doc/driver-model/README.txt +++ b/doc/driver-model/README.txt @@ -739,7 +739,7 @@ Pre-Relocation Support ---------------------- For pre-relocation we simply call the driver model init function. Only -drivers marked with DM_FLAG_PRE_RELOC or the device tree +drivers declared with U_BOOT_DRIVER_F or the device tree 'u-boot,dm-pre-reloc' flag are initialised prior to relocation. This helps to reduce the driver model overhead. diff --git a/drivers/core/device.c b/drivers/core/device.c index 49faa29..6fd2f07 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -157,11 +157,9 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, { struct driver *drv; - drv = lists_driver_lookup_name(info->name); + drv = __lists_driver_lookup_name(info->name, pre_reloc_only); if (!drv) return -ENOENT; - if (pre_reloc_only && !(drv->flags & DM_FLAG_PRE_RELOC)) - return -EPERM; return device_bind(parent, drv, info->name, (void *)info->platdata, -1, devp); diff --git a/drivers/core/lists.c b/drivers/core/lists.c index 1476715..5b61ab5 100644 --- a/drivers/core/lists.c +++ b/drivers/core/lists.c @@ -18,10 +18,13 @@ #include <fdtdec.h> #include <linux/compiler.h> -struct driver *lists_driver_lookup_name(const char *name) +#define driver_entry_end(p) (p) ? ll_entry_end(struct driver, driver1) : \ + ll_entry_end(struct driver, driver2); + +struct driver *__lists_driver_lookup_name(const char *name, bool pre_reloc_only) { - struct driver *entry = ll_entry_start(struct driver, driver); - struct driver *end = ll_entry_end(struct driver, driver); + struct driver *entry = ll_entry_start(struct driver, driver1); + struct driver *end = driver_entry_end(pre_reloc_only); for (; entry < end; entry++) { if (!strcmp(name, entry->name)) @@ -58,11 +61,12 @@ int lists_bind_drivers(struct udevice *parent, bool pre_reloc_only) for (; entry < end; entry++) { ret = device_bind_by_name(parent, pre_reloc_only, entry, &dev); - if (ret && ret != -EPERM) { + + if (!pre_reloc_only && ret == -ENOENT) dm_warn("No match for driver '%s'\n", entry->name); - if (!result || ret != -ENOENT) - result = ret; - } + + if (!result || ret != -ENOENT) + result = ret; } return result; @@ -105,8 +109,8 @@ static int driver_check_compatible(const void *blob, int offset, int lists_bind_fdt(struct udevice *parent, const void *blob, int offset, struct udevice **devp) { - struct driver *entry = ll_entry_start(struct driver, driver); - struct driver *end = ll_entry_end(struct driver, driver); + struct driver *entry = ll_entry_start(struct driver, driver1); + struct driver *end = ll_entry_end(struct driver, driver2); struct udevice *dev; bool found = false; const char *name; diff --git a/drivers/core/root.c b/drivers/core/root.c index 47b3acf..02970c8 100644 --- a/drivers/core/root.c +++ b/drivers/core/root.c @@ -70,7 +70,8 @@ int dm_scan_platdata(bool pre_reloc_only) ret = lists_bind_drivers(DM_ROOT_NON_CONST, pre_reloc_only); if (ret == -ENOENT) { - dm_warn("Some drivers were not found\n"); + if (!pre_reloc_only) + dm_warn("Some drivers were not found\n"); ret = 0; } diff --git a/include/dm/device.h b/include/dm/device.h index 9ce95a8..a2fd7b6 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -167,9 +167,13 @@ struct driver { uint32_t flags; }; -/* Declare a new U-Boot driver */ +/* Drivers bound before and after relocation */ +#define U_BOOT_DRIVER_F(__name) \ + ll_entry_declare(struct driver, __name, driver1) + +/* Drivers bound only after relocation */ #define U_BOOT_DRIVER(__name) \ - ll_entry_declare(struct driver, __name, driver) + ll_entry_declare(struct driver, __name, driver2) /** * dev_get_platdata() - Get the platform data for a device diff --git a/include/dm/lists.h b/include/dm/lists.h index 704e33e..fbd428d 100644 --- a/include/dm/lists.h +++ b/include/dm/lists.h @@ -13,15 +13,23 @@ #include <dm/uclass-id.h> /** - * lists_driver_lookup_name() - Return u_boot_driver corresponding to name + * __lists_driver_lookup_name() - Return u_boot_driver corresponding to name * * This function returns a pointer to a driver given its name. This is used * for binding a driver given its name and platdata. * * @name: Name of driver to look up + * @pre_reloc_only: If true, searches in drivers declared with U_BOOT_DRIVER_F. + * If false searches in all drivers. * @return pointer to driver, or NULL if not found */ -struct driver *lists_driver_lookup_name(const char *name); +struct driver *__lists_driver_lookup_name(const char *name, + bool pre_reloc_only); + +static inline struct driver *lists_driver_lookup_name(const char *name) +{ + return __lists_driver_lookup_name(name, false); +} /** * lists_uclass_lookup() - Return uclass_driver based on ID of the class @@ -39,8 +47,8 @@ struct uclass_driver *lists_uclass_lookup(enum uclass_id id); * each one. The devices will have @parent as their parent. * * @parent: parent device (root) - * @early_only: If true, bind only drivers with the DM_INIT_F flag. If false - * bind all drivers. + * @pre_reloc_only: If true, bind only drivers declared with U_BOOT_DRIVER_F. + * If false bind all drivers. */ int lists_bind_drivers(struct udevice *parent, bool pre_reloc_only); diff --git a/include/dm/root.h b/include/dm/root.h index c7f0c1d..61d1cd8 100644 --- a/include/dm/root.h +++ b/include/dm/root.h @@ -26,8 +26,8 @@ struct udevice *dm_root(void); * * This scans all available platdata and creates drivers for each * - * @pre_reloc_only: If true, bind only drivers with the DM_FLAG_PRE_RELOC - * flag. If false bind all drivers. + * @pre_reloc_only: If true, bind only drivers declared with U_BOOT_DRIVER_F. + * If false bind all drivers. * @return 0 if OK, -ve on error */ int dm_scan_platdata(bool pre_reloc_only); @@ -54,8 +54,8 @@ int dm_scan_fdt(const void *blob, bool pre_reloc_only); * @parent: Parent device for the devices that will be created * @blob: Pointer to device tree blob * @offset: Offset of node to scan - * @pre_reloc_only: If true, bind only drivers with the DM_FLAG_PRE_RELOC - * flag. If false bind all drivers. + * @pre_reloc_only: If true, bind only drivers declared with U_BOOT_DRIVER_F. + * If false bind all drivers. * @return 0 if OK, -ve on error */ int dm_scan_fdt_node(struct udevice *parent, const void *blob, int offset, @@ -69,8 +69,8 @@ int dm_scan_fdt_node(struct udevice *parent, const void *blob, int offset, * programmaticaly. They should do this by calling device_bind() on each * device. * - * @pre_reloc_only: If true, bind only drivers with the DM_FLAG_PRE_RELOC - * flag. If false bind all drivers. + * @pre_reloc_only: If true, bind only drivers declared with U_BOOT_DRIVER_F. + * If false bind all drivers. */ int dm_scan_other(bool pre_reloc_only); @@ -81,8 +81,8 @@ int dm_scan_other(bool pre_reloc_only); * then scans and binds available devices from platform data and the FDT. * This calls dm_init() to set up Driver Model structures. * - * @pre_reloc_only: If true, bind only drivers with the DM_FLAG_PRE_RELOC - * flag. If false bind all drivers. + * @pre_reloc_only: If true, bind only drivers declared with U_BOOT_DRIVER_F. + * If false bind all drivers. * @return 0 if OK, -ve on error */ int dm_init_and_scan(bool pre_reloc_only);
The DM initialization function, dm_init_and_scan() is called twice, before and after relocation. In the first DM scan, only some of drivers are bound for the use in pre-reloc code (mostly UART drivers). In the second scan, all the drivers are bound. In the current implementation, all the drivers are collected into a single array. Every driver is searched through the array and then the DM_FLAG_PRE_RELOC flag is checked. In the pre-reloc DM scan, drivers without DM_FLAG_PRE_RELOC are ignored. This algorithm works enough, but is not very efficient. This commit aims to split drivers into two contiguous arrays, pre-reloc drivers and post-reloc drivers. The drivers in the former group are declared with U_BOOT_DRIVER_F. The others are declared with U_BOOT_DRIVER. pre post _____________________ reloc reloc .u_boot_list_2_driver1_1 | | || || | | || || | | || || | U_BOOT_DRIVER_F | || || | (driver1) | || || | | _||_ || | | \ / || .u_boot_list_2_driver1_3 |___________________| \/ || .u_boot_list_2_driver2_1 | | || | | || | | || | U_BOOT_DRIVER | || | (driver2) | || | | _||_ | | \ / .u_boot_list_2_driver2_3 |___________________| \/ The pre-reloc DM scan searches drivers from .u_boot_list_2_driver1_1 to .u_boot_list_2_driver1_3. The post-reloc scan searches ones from .u_boot_list_2_driver1_1 and .u_boot_list_2_driver2_3. Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com> --- doc/driver-model/README.txt | 2 +- drivers/core/device.c | 4 +--- drivers/core/lists.c | 22 +++++++++++++--------- drivers/core/root.c | 3 ++- include/dm/device.h | 8 ++++++-- include/dm/lists.h | 16 ++++++++++++---- include/dm/root.h | 16 ++++++++-------- 7 files changed, 43 insertions(+), 28 deletions(-)