dm: core: abolish "u-boot, dm-pre-reloc" property and various refactorings
Subsystems (3)
cover dm: core: abolish "u-boot, dm-pre-reloc" property and various refactorings
I am implemting Device Tree support for Panasonic UniPhier platform, but while I was working on that, I noticed some problems in DM core implementation.
What I want to solve the most here is the inconsistency in terms of how to pass the pre-reloc attribute.
The driver model provides two ways to pass the device information, platform data and device tree.
In the platform data way, the pre-reloc DM scan checks if each driver has DM_FLAG_PRE_RELOC flag, that is, each **driver** has the pre-reloc attribute.
In the device tree control, the existence of "u-boot,dm-pre-reloc" is checked for each device node. The driver flag "DM_FLAG_PRE_RELOC" is never checked. That is, each **device** owns the pre-reloc attribute.
Drivers should generally work both with platform data and device tree, but this inconsitency has made our life difficult.
This series aims to abolish "u-boot,dm-pre-reloc" property because:
- Having a U-Boot specific property makes it difficult to share the device tree sources between Linux and U-Boot.
- The number of devices is generally larger than that of drivers. Each driver often has multiple devices with different base addresses. It seems more reasonable to add the pre-reloc attribute to drivers than devices.
8/8 commit abolishes "u-boot,dm-pre-reloc" property and replaces it with some *driver* attribute.
When I am working on that, I came up with another idea. I thought it is unefficient to scan all the drivers and then check the DM_FLAG_PRE_RELOC. Becuase the DM_FLAG_PRE_RELOC is a static data, we know if each driver is a pre-reloc one or not. So, 6/8 introduces more efficient driver scanning.
1/8 thru 5/8 are some cleanups before I do new things.
Masahiro Yamada (8):
dm: core: a trivial clean up
dm: core: remove meaningless if conditional
dm: core: remove unnecessary return condition in driver lookup
dm: core: remove unnecessary return condition in uclass lookup
dm: core: refactor linker lists lookup code
dm: core: look up drivers more efficiently
dm: core: declare pre-reloc drivers with U_BOOT_DRIVER_F
dm: core: abolish u-boot,dm-pre-reloc property
arch/arm/dts/exynos5.dtsi | 1 - doc/driver-model/README.txt | 5 ++-- drivers/core/device.c | 4 +-- drivers/core/lists.c | 57 ++++++++++++++++++---------------------- drivers/core/root.c | 13 ++++----- drivers/core/simple-bus.c | 2 +- drivers/serial/sandbox.c | 3 +-- drivers/serial/serial_mxc.c | 3 +-- drivers/serial/serial_omap.c | 3 +-- drivers/serial/serial_pl01x.c | 3 +-- drivers/serial/serial_s5p.c | 3 +-- drivers/serial/serial_uniphier.c | 3 +-- include/dm/device.h | 11 ++++---- include/dm/lists.h | 29 +++++++++++++++----- include/dm/root.h | 20 +++++++------- test/dm/test-driver.c | 3 +-- test/dm/test-fdt.c | 2 +- test/dm/test.dts | 2 -- 18 files changed, 80 insertions(+), 87 deletions(-)
Signature · 1 line
1.9.1
PATCH 1/8 dm: core: a trivial clean up
---
drivers/core/root.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/core/root.c +1 -3
@@ -73,10 +73,8 @@ int dm_scan_platdata(bool pre_reloc_only)dm_warn ("Some drivers were not found\n" ); ret =0 ; } -if (ret) -return ret; -return 0 ; +return ret; }#ifdef CONFIG_OF_CONTROL
Signature · 1 line
1.9.1
PATCH 1/8 dm: core: a trivial clean up
Quoted 1 line · Masahiro Yamada
Signed-off-by Masahiro Yamada <[email protected]>
PATCH 1/8 dm: core: a trivial clean up
Quoted 4 lines · Simon Glass
Quoted 1 line · Masahiro Yamada
Signed-off-by Masahiro Yamada <[email protected]>Acked-by Simon Glass <[email protected]>
Applied to u-boot-dm.
PATCH 2/8 dm: core: remove meaningless if conditional
In reply to Masahiro YamadaIf the variable "ret" is equal to "-ENOENT", it is trapped at [1] and never reaches [2]. At [3], the condition "ret != -ENOENT" is always true.
if (ret == -ENOENT) { <------------------ [1]
continue;
} else if (ret == -ENODEV) {
dm_dbg("Device '%s' has no compatible string\n", name);
break;
} else if (ret) { <------------------ [2]
dm_warn("Device tree error at offset %d\n", offset);
if (!result || ret != -ENOENT) <------------------ [3]
result = ret;
break;
}---
drivers/core/lists.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/core/lists.c +1 -2
@@ -136,8 +136,7 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,break ; }else if (ret) {dm_warn ("Device tree error at offset %d\n" , offset); -if (!result || ret != -ENOENT ) - result = ret; + result = ret;break ; }
Signature · 1 line
1.9.1
PATCH 2/8 dm: core: remove meaningless if conditional
Hi Masahiro,
Quoted 27 lines · Masahiro Yamada
If the variable "ret" is equal to "
-ENOENT", it is trapped at [1] and never reaches [2]. At [3], the condition "ret !=-ENOENT" is always true.if (ret == -ENOENT) { <------------------ [1] continue; } else if (ret == -ENODEV) { dm_dbg("Device '%s' has no compatible string\n", name); break; } else if (ret) { <------------------ [2] dm_warn("Device tree error at offset %d\n", offset); if (!result || ret != -ENOENT) <------------------ [3] result = ret; break; }Signed-off-by Masahiro Yamada <[email protected]>---
drivers/core/lists.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/core/lists.c +0
-0
drivers/core/lists.c int lists_bind_fdt(struct udevice *parent, const void *blob, int offset, · line 139
Quoted 6 lines
drivers/core/lists.c +1
-2@@ -136,8 +136,7 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,break ; }else if (ret) {dm_warn ("Device tree error at offset %d\n" , offset); -if (!result || ret != -ENOENT ) - result = ret; + result = ret;
Thanks for the clear explanation.
It looks good, except my intent was to return the first error, hence the 'if (!result ...'.
Your code will return the last error. Can we preserve the current bahaviour?
Regards,
Simon
PATCH 2/8 dm: core: remove meaningless if conditional
Hi Simon,
Quoted 41 lines · On Mon, 17 Nov 2014 09:14:15 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 27 lines · Masahiro Yamada
If the variable "ret" is equal to "
-ENOENT", it is trapped at [1] and never reaches [2]. At [3], the condition "ret !=-ENOENT" is always true.if (ret == -ENOENT) { <------------------ [1] continue; } else if (ret == -ENODEV) { dm_dbg("Device '%s' has no compatible string\n", name); break; } else if (ret) { <------------------ [2] dm_warn("Device tree error at offset %d\n", offset); if (!result || ret != -ENOENT) <------------------ [3] result = ret; break; }Signed-off-by Masahiro Yamada <[email protected]>---
drivers/core/lists.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/core/lists.c +0
-0drivers/core/lists.c int lists_bind_fdt(struct udevice *parent, const void *blob, int offset, · line 139
Quoted 6 lines
drivers/core/lists.c +1
-2@@ -136,8 +136,7 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,break ; }else if (ret) {dm_warn ("Device tree error at offset %d\n" , offset); -if (!result || ret != -ENOENT ) - result = ret; + result = ret;Thanks for the clear explanation.
It looks good, except my intent was to return the first error, hence the 'if (!result ...'.
Quoted 1 line
Your code will return the last error. Can we preserve the current bahaviour?
Do you mean, like this?
dm_warn("Device tree error at offset %d\n", offset);
if (!result)
result = ret;
break;I think there is no difference in the behaviour, because this "for" loop is escaped out at the first encounter with an error except -ENOENT.
Here is the only line to set "result" and it is followed by "break" statement, so the last error is also the first error, I think.
Best Regards
Masahiro Yamada
PATCH 2/8 dm: core: remove meaningless if conditional
Hi Masahiro,
Quoted 59 lines · Masahiro Yamada
Hi Simon,
Quoted 41 lines · On Mon, 17 Nov 2014 09:14:15 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 27 lines · Masahiro Yamada
If the variable "ret" is equal to "
-ENOENT", it is trapped at [1] and never reaches [2]. At [3], the condition "ret !=-ENOENT" is always true.if (ret == -ENOENT) { <------------------ [1] continue; } else if (ret == -ENODEV) { dm_dbg("Device '%s' has no compatible string\n", name); break; } else if (ret) { <------------------ [2] dm_warn("Device tree error at offset %d\n", offset); if (!result || ret != -ENOENT) <------------------ [3] result = ret; break; }Signed-off-by Masahiro Yamada <[email protected]>---
drivers/core/lists.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/core/lists.c +0
-0drivers/core/lists.c int lists_bind_fdt(struct udevice *parent, const void *blob, int offset, · line 139
Quoted 6 lines
drivers/core/lists.c +1
-2@@ -136,8 +136,7 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,break ; }else if (ret) {dm_warn ("Device tree error at offset %d\n" , offset); -if (!result || ret != -ENOENT ) - result = ret; + result = ret;Thanks for the clear explanation.
It looks good, except my intent was to return the first error, hence the 'if (!result ...'.Quoted 1 line
Your code will return the last error. Can we preserve the current bahaviour?
Do you mean, like this?
dm_warn("Device tree error at offset %d\n", offset); if (!result) result = ret; break;
Quoted 6 lines
I think there is no difference in the behaviour, because this "for" loop is escaped out at the first encounter with an error except
-ENOENT.Here is the only line to set "result" and it is followed by "break" statement, so the last error is also the first error, I think.
Ah yes, OK. The intent here is to provide a useful error message without adding a lot of noise. It's a bit of a pain but we should keep it I think. Your patch looks good.
Regards,
Simon
PATCH 2/8 dm: core: remove meaningless if conditional
Quoted 66 lines · Simon Glass
Hi Masahiro,
Quoted 63 lines · Masahiro Yamada
Hi Simon,
Quoted 41 lines · On Mon, 17 Nov 2014 09:14:15 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 27 lines · Masahiro Yamada
If the variable "ret" is equal to "
-ENOENT", it is trapped at [1] and never reaches [2]. At [3], the condition "ret !=-ENOENT" is always true.if (ret == -ENOENT) { <------------------ [1] continue; } else if (ret == -ENODEV) { dm_dbg("Device '%s' has no compatible string\n", name); break; } else if (ret) { <------------------ [2] dm_warn("Device tree error at offset %d\n", offset); if (!result || ret != -ENOENT) <------------------ [3] result = ret; break; }Signed-off-by Masahiro Yamada <[email protected]>---
drivers/core/lists.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/core/lists.c +0
-0drivers/core/lists.c int lists_bind_fdt(struct udevice *parent, const void *blob, int offset, · line 139
Quoted 6 lines
drivers/core/lists.c +1
-2@@ -136,8 +136,7 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,break ; }else if (ret) {dm_warn ("Device tree error at offset %d\n" , offset); -if (!result || ret != -ENOENT ) - result = ret; + result = ret;Thanks for the clear explanation.
It looks good, except my intent was to return the first error, hence the 'if (!result ...'.Quoted 1 line
Your code will return the last error. Can we preserve the current bahaviour?
Do you mean, like this?
dm_warn("Device tree error at offset %d\n", offset); if (!result) result = ret; break;I think there is no difference in the behaviour, because this "for" loop is escaped out at the first encounter with an error except
-ENOENT.
Quoted 8 lines
Quoted 2 lines
Here is the only line to set "result" and it is followed by "break" statement, so the last error is also the first error, I think.
Ah yes, OK. The intent here is to provide a useful error message without adding a lot of noise. It's a bit of a pain but we should keep it I think. Your patch looks good.
Acked-by Simon Glass <[email protected]>
Applied to u-boot-dm.
PATCH 3/8 dm: core: remove unnecessary return condition in driver lookup
In reply to Masahiro YamadaThe variable "drv" never becomes NULL because ll_entry_start() always returns a valid pointer even if there are no entries.
The case "n_ents == 0" is covered by the following "for" loop.
---
drivers/core/lists.c | 3 --- 1 file changed, 3 deletions(-)
drivers/core/lists.c +0 -3
@@ -25,9 +25,6 @@ struct driver *lists_driver_lookup_name(const char *name) const int n_ents = ll_entry_count(struct driver, driver);struct driver *entry; -if (!drv || !n_ents) -return NULL ; -for (entry = drv; entry != drv + n_ents; entry++) {if (!strcmp (name, entry->name))return entry;
Signature · 1 line
1.9.1
PATCH 3/8 dm: core: remove unnecessary return condition in driver lookup
Quoted 6 lines · Masahiro Yamada
The variable "drv" never becomes NULL because
ll_entry_start()always returns a valid pointer even if there are no entries.The case "
n_ents== 0" is covered by the following "for" loop.Signed-off-by Masahiro Yamada <[email protected]>
PATCH 3/8 dm: core: remove unnecessary return condition in driver lookup
Quoted 9 lines · Simon Glass
Quoted 6 lines · Masahiro Yamada
The variable "drv" never becomes NULL because
ll_entry_start()always returns a valid pointer even if there are no entries.The case "
n_ents== 0" is covered by the following "for" loop.Signed-off-by Masahiro Yamada <[email protected]>Acked-by Simon Glass <[email protected]>
Applied to u-boot-dm.
PATCH 4/8 dm: core: remove unnecessary return condition in uclass lookup
In reply to Masahiro YamadaThese conditions never happen.
- There is no real uclass with UCLASS_INVALID id.
- uclass never becomes NULL because ll_entry_start() always returns a valid pointer.
---
drivers/core/lists.c | 3 --- 1 file changed, 3 deletions(-)
drivers/core/lists.c +0 -3
@@ -41,9 +41,6 @@ struct uclass_driver *lists_uclass_lookup(enum uclass_id id) const int n_ents = ll_entry_count(struct uclass_driver, uclass);struct uclass_driver *entry; -if ((id ==UCLASS_INVALID ) || !uclass) -return NULL ; -for (entry = uclass; entry != uclass + n_ents; entry++) {if (entry->id == id)return entry;
Signature · 1 line
1.9.1
PATCH 4/8 dm: core: remove unnecessary return condition in uclass lookup
Quoted 6 lines · Masahiro Yamada
These conditions never happen.
- There is no real uclass withUCLASS_INVALIDid.
- uclass never becomes NULL becausell_entry_start()always returns a valid pointer.Signed-off-by Masahiro Yamada <[email protected]>
Yes we now use proper error codes to detect an invalid uclass, so it is good to clean this up.
PATCH 4/8 dm: core: remove unnecessary return condition in uclass lookup
Quoted 6 lines · Simon Glass
Quoted 5 lines · Masahiro Yamada
These conditions never happen.
- There is no real uclass withUCLASS_INVALIDid.
- uclass never becomes NULL becausell_entry_start()always returns a valid pointer.
Quoted 6 lines
Quoted 1 line
Signed-off-by Masahiro Yamada <[email protected]>Yes we now use proper error codes to detect an invalid uclass, so it is good to clean this up.
Acked-by Simon Glass <[email protected]>
Applied to u-boot-dm.
PATCH 5/8 dm: core: refactor linker lists lookup code
In reply to Masahiro YamadaIt looks simpler to use ll_entry_end() than ll_entry_count(). We can save one variable. (and it will be helpful for the upcoming commit.)
---
drivers/core/lists.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-)
drivers/core/lists.c +13 -17
@@ -20,12 +20,10 @@struct driver *lists_driver_lookup_name (const char *name) { -struct driver *drv = - ll_entry_start(struct driver, driver); - const int n_ents = ll_entry_count(struct driver, driver); -struct driver *entry; + struct driver *entry = ll_entry_start(struct driver, driver); + struct driver *end = ll_entry_end(struct driver, driver); -for (entry = drv; entry != drv + n_ents; entry++) { +for (; entry < end; entry++) {if (!strcmp (name, entry->name))return entry; }
@@ -36,12 +34,11 @@ struct driver *lists_driver_lookup_name(const char *name)struct uclass_driver *lists_uclass_lookup (enum uclass_id id) { -struct uclass_driver *uclass = +struct uclass_driver *entry = ll_entry_start(struct uclass_driver, uclass); - const int n_ents = ll_entry_count(struct uclass_driver, uclass); -struct uclass_driver *entry; + struct uclass_driver *end = ll_entry_end(struct uclass_driver, uclass); -for (entry = uclass; entry != uclass + n_ents; entry++) { +for (; entry < end; entry++) {if (entry->id == id)return entry; }
@@ -51,15 +48,15 @@ struct uclass_driver *lists_uclass_lookup(enum uclass_id id)int lists_bind_drivers (struct udevice *parent,bool pre_reloc_only) { -struct driver_info *info = +struct driver_info *entry = ll_entry_start(struct driver_info, driver_info); - const int n_ents = ll_entry_count(struct driver_info, driver_info); -struct driver_info *entry; +struct driver_info *end = + ll_entry_end(struct driver_info, driver_info);struct udevice *dev;int result =0 ;int ret; -for (entry = info; entry != info + n_ents; entry++) { +for (; entry < end; entry++) { ret =device_bind_by_name (parent, pre_reloc_only, entry, &dev);if (ret && ret != -EPERM ) {dm_warn ("No match for driver '%s'\n" , entry->name);
@@ -108,9 +105,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 *driver = ll_entry_start(struct driver, driver); - const int n_ents = ll_entry_count(struct driver, driver); -struct driver *entry; + struct driver *entry = ll_entry_start(struct driver, driver); + struct driver *end = ll_entry_end(struct driver, driver);struct udevice *dev;bool found = false;const char *name;
@@ -120,7 +116,7 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,dm_dbg ("bind node %s\n" ,fdt_get_name (blob, offset,NULL ));if (devp) *devp =NULL ; -for (entry = driver; entry != driver + n_ents; entry++) { +for (; entry < end; entry++) { ret =driver_check_compatible (blob, offset, entry->of_match); name =fdt_get_name (blob, offset,NULL );if (ret == -ENOENT ) {
Signature · 1 line
1.9.1
PATCH 5/8 dm: core: refactor linker lists lookup code
Quoted 5 lines · Masahiro Yamada
It looks simpler to use
ll_entry_end()thanll_entry_count(). We can save one variable. (and it will be helpful for the upcoming commit.)Signed-off-by Masahiro Yamada <[email protected]>
PATCH 6/8 dm: core: look up drivers more efficiently
In reply to Masahiro YamadaThe 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.
---
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(-)
doc/driver-model/README.txt +1 -1
@@ -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.
drivers/core/device.c +1 -3
@@ -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);
drivers/core/lists.c +13 -9
@@ -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;
drivers/core/root.c +2 -1
@@ -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 ; }
include/dm/device.h +6 -2
@@ -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
include/dm/lists.h +12 -4
@@ -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);
include/dm/root.h +8 -8
@@ -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);
Signature · 1 line
1.9.1
PATCH 6/8 dm: core: look up drivers more efficiently
HI Masahiro,
Quoted 71 lines · Masahiro Yamada
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_RELOCflag is checked. In the pre-reloc DM scan, drivers withoutDM_FLAG_PRE_RELOCare 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 withU_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 <[email protected]>---
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(-)doc/driver-model/README.txt +1
-1@@ -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.drivers/core/device.c +0
-0
drivers/core/device.c int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, · line 160
Quoted 5 lines
drivers/core/device.c +1
-1@@ -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?
Quoted 180 lines
drivers/core/device.c +0
-2if (!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);drivers/core/lists.c +13
-9@@ -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;drivers/core/root.c +2
-1@@ -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 ; }include/dm/device.h +6
-2@@ -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 include/dm/lists.h +12
-4@@ -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);include/dm/root.h +8
-9@@ -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); --
Quoted 2 lines
1.9.1
Regards,
Simon
PATCH 6/8 dm: core: look up drivers more efficiently
Hi Simon,
Quoted 6 lines · On Mon, 17 Nov 2014 09:22:19 +0000 Simon Glass <[email protected]> wrote:
Quoted 6 lines
drivers/core/device.c +0
-0@@ -157,11 +157,9 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, {struct driver *drv;
Quoted 7 lines
drivers/core/device.c int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, · line 160
Quoted 2 lines
drivers/core/device.c +1
-1- 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
PATCH 6/8 dm: core: look up drivers more efficiently
Hi Masahiro,
Quoted 23 lines · Masahiro Yamada
Hi Simon,
Quoted 6 lines · On Mon, 17 Nov 2014 09:22:19 +0000 Simon Glass <[email protected]> wrote:
Quoted 6 lines
drivers/core/device.c +0
-0@@ -157,11 +157,9 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, {struct driver *drv;Quoted 7 lines
drivers/core/device.c int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, · line 160
Quoted 2 lines
drivers/core/device.c +1
-1- 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.
Quoted 8 lines
I often see "__" prefixes in Linux and in my understanding,
__foo()is a "use it carefully" version or locally used interface offoo()(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__prerelocis 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.
Regards,
Simon
PATCH 7/8 dm: core: declare pre-reloc drivers with U_BOOT_DRIVER_F
In reply to Masahiro YamadaReplace U_BOOT_DRIVER with U_BOOT_DRIVER_F where the DM_FLAG_PRE_RELOC flag is set.
---
drivers/serial/sandbox.c | 3 +-- drivers/serial/serial_mxc.c | 3 +-- drivers/serial/serial_omap.c | 3 +-- drivers/serial/serial_pl01x.c | 3 +-- drivers/serial/serial_s5p.c | 3 +-- drivers/serial/serial_uniphier.c | 3 +-- include/dm/device.h | 3 --- test/dm/test-driver.c | 3 +-- 8 files changed, 7 insertions(+), 17 deletions(-)
drivers/serial/sandbox.c +1 -2
@@ -176,7 +176,7 @@ static const struct udevice_id sandbox_serial_ids[] = { { } }; -U_BOOT_DRIVER (serial_sandbox) = { +U_BOOT_DRIVER_F (serial_sandbox) = { .name ="serial_sandbox" , .id =UCLASS_SERIAL , .of_match = sandbox_serial_ids,
@@ -186,7 +186,6 @@ U_BOOT_DRIVER(serial_sandbox) = { .probe = sandbox_serial_probe, .remove = sandbox_serial_remove, .ops = &sandbox_serial_ops, - .flags = DM_FLAG_PRE_RELOC, };static const struct sandbox_serial_platdata platdata_non_fdt = {
drivers/serial/serial_mxc.c +1 -2
@@ -334,11 +334,10 @@ static const struct dm_serial_ops mxc_serial_ops = { .setbrg = mxc_serial_setbrg, }; -U_BOOT_DRIVER (serial_mxc) = { +U_BOOT_DRIVER_F (serial_mxc) = { .name ="serial_mxc" , .id =UCLASS_SERIAL , .probe = mxc_serial_probe, .ops = &mxc_serial_ops, - .flags =DM_FLAG_PRE_RELOC , };#endif
drivers/serial/serial_omap.c +1 -2
@@ -34,7 +34,7 @@ static int omap_serial_ofdata_to_platdata(struct udevice *dev) }#endif -U_BOOT_DRIVER (serial_omap_ns16550) = { +U_BOOT_DRIVER_F (serial_omap_ns16550) = { .name ="serial_omap" , .id =UCLASS_SERIAL , .of_match =of_match_ptr (omap_serial_ids),
@@ -43,5 +43,4 @@ U_BOOT_DRIVER(serial_omap_ns16550) = { .priv_auto_alloc_size = sizeof(struct NS16550), .probe = ns16550_serial_probe, .ops = &ns16550_serial_ops, - .flags = DM_FLAG_PRE_RELOC, };
drivers/serial/serial_pl01x.c +1 -2
@@ -338,12 +338,11 @@ static const struct dm_serial_ops pl01x_serial_ops = { .setbrg = pl01x_serial_setbrg, }; -U_BOOT_DRIVER (serial_pl01x) = { +U_BOOT_DRIVER_F (serial_pl01x) = { .name ="serial_pl01x" , .id =UCLASS_SERIAL , .probe = pl01x_serial_probe, .ops = &pl01x_serial_ops, - .flags =DM_FLAG_PRE_RELOC , };#endif
drivers/serial/serial_s5p.c +1 -2
@@ -178,7 +178,7 @@ static const struct udevice_id s5p_serial_ids[] = { { } }; -U_BOOT_DRIVER (serial_s5p) = { +U_BOOT_DRIVER_F (serial_s5p) = { .name ="serial_s5p" , .id =UCLASS_SERIAL , .of_match = s5p_serial_ids,
@@ -186,5 +186,4 @@ U_BOOT_DRIVER(serial_s5p) = { .platdata_auto_alloc_size = sizeof(struct s5p_serial_platdata), .probe = s5p_serial_probe, .ops = &s5p_serial_ops, - .flags = DM_FLAG_PRE_RELOC, };
drivers/serial/serial_uniphier.c +1 -2
@@ -136,7 +136,7 @@ static const struct dm_serial_ops uniphier_serial_ops = { .pending = uniphier_serial_pending, }; -U_BOOT_DRIVER (uniphier_serial) = { +U_BOOT_DRIVER_F (uniphier_serial) = { .name =DRIVER_NAME , .id =UCLASS_SERIAL , .of_match =of_match_ptr (uniphier_uart_of_match),
@@ -147,5 +147,4 @@ U_BOOT_DRIVER(uniphier_serial) = { .platdata_auto_alloc_size = sizeof(struct uniphier_serial_platform_data), .ops = &uniphier_serial_ops, - .flags = DM_FLAG_PRE_RELOC, };
include/dm/device.h +0 -3
@@ -23,9 +23,6 @@ struct driver_info;/* DM is responsible for allocating and freeing platdata */ #define DM_FLAG_ALLOC_PDATA (1 << 1) -/* DM should init this device prior to relocation */ -#define DM_FLAG_PRE_RELOC (1 << 2) -/** * struct udevice - An instance of a driver *
test/dm/test-driver.c +1 -2
@@ -145,7 +145,7 @@ U_BOOT_DRIVER(test_manual_drv) = { .unbind = test_manual_unbind, }; -U_BOOT_DRIVER (test_pre_reloc_drv) = { +U_BOOT_DRIVER_F (test_pre_reloc_drv) = { .name ="test_pre_reloc_drv" , .id =UCLASS_TEST , .ops = &test_manual_ops,
@@ -153,5 +153,4 @@ U_BOOT_DRIVER(test_pre_reloc_drv) = { .probe = test_manual_probe, .remove = test_manual_remove, .unbind = test_manual_unbind, - .flags = DM_FLAG_PRE_RELOC, };
Signature · 1 line
1.9.1
PATCH 7/8 dm: core: declare pre-reloc drivers with U_BOOT_DRIVER_F
Quoted 4 lines · Masahiro Yamada
Replace
U_BOOT_DRIVERwithU_BOOT_DRIVER_Fwhere theDM_FLAG_PRE_RELOCflag is set.Signed-off-by Masahiro Yamada <[email protected]>
PATCH 8/8 dm: core: abolish u-boot, dm-pre-reloc property
In reply to Masahiro YamadaThe driver model provides two ways to pass the device information, platform data and device tree. Either way works to bind devices and drivers, but there is inconsistency in terms of how to pass the pre-reloc flag.
In the platform data way, the pre-reloc DM scan checks if each driver has DM_FLAG_PRE_RELOC flag (this was changed to use U_BOOT_DRIVER_F just before). That is, each **driver** has the pre-reloc attribute.
In the device tree control, the existence of "u-boot,dm-pre-reloc" is checked for each device node. The driver flag "DM_FLAG_PRE_RELOC" is never checked. That is, each **device** owns the pre-reloc attribute.
Drivers should generally work both with platform data and device tree, but this inconsistency has made our life difficult.
This commit abolishes "u-boot,dm-pre-reloc" property because:
- Having a U-Boot specific property makes it difficult to share the device tree sources between Linux and U-Boot.
- The number of devices is generally larger than that of drivers. Each driver often has multiple devices with different base addresses. It seems more reasonable to add the pre-reloc attribute to drivers than devices.
---
arch/arm/dts/exynos5.dtsi | 1 - doc/driver-model/README.txt | 5 ++--- drivers/core/lists.c | 6 +++--- drivers/core/root.c | 6 ++---- drivers/core/simple-bus.c | 2 +- include/dm/lists.h | 13 ++++++++++--- include/dm/root.h | 4 ++-- test/dm/test-fdt.c | 2 +- test/dm/test.dts | 2 -- 9 files changed, 21 insertions(+), 20 deletions(-)
arch/arm/dts/exynos5.dtsi +0 -1
@@ -244,7 +244,6 @@ compatible = "samsung,exynos4210-uart"; reg = <0x12C30000 0x100>; interrupts = <0 54 0>; - u-boot,dm-pre-reloc; id = <3>; };
doc/driver-model/README.txt +2 -3
@@ -739,9 +739,8 @@ Pre-Relocation Support ---------------------- For pre-relocation we simply call the driver model init function. Only -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. +drivers declared with U_BOOT_DRIVER_F are initialised prior to relocation. +This helps to reduce the driver model overhead. Then post relocation we throw that away and re-init driver model again. For drivers which require some sort of continuity between pre- and
drivers/core/lists.c +3 -3
@@ -106,11 +106,11 @@ static int driver_check_compatible(const void *blob, int offset,return -ENOENT ; } -int lists_bind_fdt (struct udevice *parent,const void *blob,int offset, -struct udevice **devp) +int __lists_bind_fdt (struct udevice *parent,const void *blob,int offset, +struct udevice **devp,bool pre_reloc_only) { struct driver *entry = ll_entry_start(struct driver, driver1); - struct driver *end = ll_entry_end(struct driver, driver2); +struct driver *end =driver_entry_end (pre_reloc_only);struct udevice *dev;bool found = false;const char *name;
drivers/core/root.c +2 -4
@@ -87,10 +87,8 @@ int dm_scan_fdt_node(struct udevice *parent, const void *blob, int offset,for (offset =fdt_first_subnode (blob, offset); offset >0 ; offset =fdt_next_subnode (blob, offset)) { -if (pre_reloc_only && - !fdt_getprop (blob, offset,"u-boot,dm-pre-reloc" ,NULL )) -continue ; - err =lists_bind_fdt (parent, blob, offset,NULL ); + err =__lists_bind_fdt (parent, blob, offset,NULL , + pre_reloc_only);if (err && !ret) ret = err; }
drivers/core/simple-bus.c +1 -1
@@ -26,7 +26,7 @@ static const struct udevice_id generic_simple_bus_ids[] = { { } }; -U_BOOT_DRIVER (simple_bus_drv) = { +U_BOOT_DRIVER_F (simple_bus_drv) = { .name ="generic_simple_bus" , .id =UCLASS_SIMPLE_BUS , .of_match = generic_simple_bus_ids,
include/dm/lists.h +10 -3
@@ -53,7 +53,7 @@ struct uclass_driver *lists_uclass_lookup(enum uclass_id id);int lists_bind_drivers (struct udevice *parent,bool pre_reloc_only);/** -* lists_bind_fdt() - bind a device tree node +* __lists_bind_fdt() - bind a device tree node * * This creates a new device bound to the given device tree node, with * @parent as its parent.
@@ -62,10 +62,17 @@ int lists_bind_drivers(struct udevice *parent, bool pre_reloc_only);* @blob: device tree blob * @offset: offset of this device tree node * @devp: if non-NULL, returns a pointer to the bound device +* @pre_reloc_only: If true, bind only drivers declared with U_BOOT_DRIVER_F. +* If false bind all drivers. * @return 0 if device was bound, -EINVAL if the device tree is invalid, * other -ve value on error */ -int lists_bind_fdt (struct udevice *parent,const void *blob,int offset, -struct udevice **devp); +int __lists_bind_fdt (struct udevice *parent,const void *blob,int offset, +struct udevice **devp,bool pre_reloc_only); +static inline int lists_bind_fdt (struct udevice *parent,const void *blob, +int offset,struct udevice **devp) +{ +return __lists_bind_fdt (parent, blob, offset, devp, false); +}#endif
include/dm/root.h +2 -2
@@ -39,8 +39,8 @@ int dm_scan_platdata(bool pre_reloc_only);* the top-level subnodes are examined. * * @blob: Pointer to device tree blob -* @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 (const void *blob,bool pre_reloc_only);
test/dm/test-fdt.c +1 -1
@@ -64,7 +64,7 @@ static const struct udevice_id testfdt_ids[] = { { } }; -U_BOOT_DRIVER (testfdt_drv) = { +U_BOOT_DRIVER_F (testfdt_drv) = { .name ="testfdt_drv" , .of_match = testfdt_ids, .id =UCLASS_TEST_FDT ,
test/dm/test.dts +0 -2
@@ -13,7 +13,6 @@ uart0: serial { compatible = "sandbox,serial"; - u-boot,dm-pre-reloc; }; a-test {
@@ -21,7 +20,6 @@ compatible = "denx,u-boot-fdt-test"; ping-expect = <0>; ping-add = <0>; - u-boot,dm-pre-reloc; }; junk {
Signature · 1 line
1.9.1
PATCH 8/8 dm: core: abolish u-boot, dm-pre-reloc property
Hi Masahiro,
Quoted 9 lines · Masahiro Yamada
The driver model provides two ways to pass the device information, platform data and device tree. Either way works to bind devices and drivers, but there is inconsistency in terms of how to pass the pre-reloc flag.
In the platform data way, the pre-reloc DM scan checks if each driver has
DM_FLAG_PRE_RELOCflag (this was changed to useU_BOOT_DRIVER_Fjust before). That is, each **driver** has the pre-reloc attribute.
Quoted 6 lines
In the device tree control, the existence of "u-boot,dm-pre-reloc" is checked for each device node. The driver flag "
DM_FLAG_PRE_RELOC" is never checked. That is, each **device** owns the pre-reloc attribute.Drivers should generally work both with platform data and device tree, but this inconsistency has made our life difficult.
I feel we should use device tree where available, and only fall back to platform data when necessary (no device tree available for platform, for example).
Quoted 10 lines
This commit abolishes "u-boot,dm-pre-reloc" property because:
- Having a U-Boot specific property makes it difficult to share the device tree sources between Linux and U-Boot.
- The number of devices is generally larger than that of drivers. Each driver often has multiple devices with different base addresses. It seems more reasonable to add the pre-reloc attribute to drivers than devices.
The inability for platform data to specify which devices need to be pre-relocation is certainly a limitation. But I'm not sure that the solution is to remove that feature from the device tree. Prior to relocation memory may be severely limited. Things like GPIO and serial can create quite a few devices (e.g. Tegra has 16 for GPIO and 4 for serial), but only a subset may be needed before relocation (on Tegra only 2!).
I'm actually pretty comfortable with platform data having a limited subset of functionality, since I believe most platforms will use device tree for one reason or another.
Thoughts?
Regards,
Simon
PATCH 8/8 dm: core: abolish u-boot, dm-pre-reloc property
Hi Simon,
Quoted 16 lines · On Mon, 17 Nov 2014 18:17:43 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 13 lines · Masahiro Yamada
The driver model provides two ways to pass the device information, platform data and device tree. Either way works to bind devices and drivers, but there is inconsistency in terms of how to pass the pre-reloc flag.
In the platform data way, the pre-reloc DM scan checks if each driver has
DM_FLAG_PRE_RELOCflag (this was changed to useU_BOOT_DRIVER_Fjust before). That is, each **driver** has the pre-reloc attribute.In the device tree control, the existence of "u-boot,dm-pre-reloc" is checked for each device node. The driver flag "
DM_FLAG_PRE_RELOC" is never checked. That is, each **device** owns the pre-reloc attribute.
Quoted 6 lines
Quoted 2 lines
Drivers should generally work both with platform data and device tree, but this inconsistency has made our life difficult.
I feel we should use device tree where available, and only fall back to platform data when necessary (no device tree available for platform, for example).
No, it is true that device tree is a useful tool, but it should be optional.
All the infrastructures of drivers must work perfectly without device tree.
The device tree is just one choice of how to give device information.
Quoted 19 lines
Quoted 10 lines
This commit abolishes "u-boot,dm-pre-reloc" property because:
- Having a U-Boot specific property makes it difficult to share the device tree sources between Linux and U-Boot.
- The number of devices is generally larger than that of drivers. Each driver often has multiple devices with different base addresses. It seems more reasonable to add the pre-reloc attribute to drivers than devices.
The inability for platform data to specify which devices need to be pre-relocation is certainly a limitation. But I'm not sure that the solution is to remove that feature from the device tree. Prior to relocation memory may be severely limited. Things like GPIO and serial can create quite a few devices (e.g. Tegra has 16 for GPIO and 4 for serial), but only a subset may be needed before relocation (on Tegra only 2!).
Quoted 6 lines
I'm actually pretty comfortable with platform data having a limited subset of functionality, since I believe most platforms will use device tree for one reason or another.
Thoughts?
No, it is not justified to compel to use device tree unless Linux is the target OS.
Even in Linux, limited numbers of architrectures use device trees.
Best Regards
Masahiro Yamada
PATCH 8/8 dm: core: abolish u-boot, dm-pre-reloc property
Hi Masahiro,
Quoted 30 lines · Masahiro Yamada
Hi Simon,
Quoted 16 lines · On Mon, 17 Nov 2014 18:17:43 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 13 lines · Masahiro Yamada
The driver model provides two ways to pass the device information, platform data and device tree. Either way works to bind devices and drivers, but there is inconsistency in terms of how to pass the pre-reloc flag.
In the platform data way, the pre-reloc DM scan checks if each driver has
DM_FLAG_PRE_RELOCflag (this was changed to useU_BOOT_DRIVER_Fjust before). That is, each **driver** has the pre-reloc attribute.In the device tree control, the existence of "u-boot,dm-pre-reloc" is checked for each device node. The driver flag "
DM_FLAG_PRE_RELOC" is never checked. That is, each **device** owns the pre-reloc attribute.Quoted 6 lines
Quoted 2 lines
Drivers should generally work both with platform data and device tree, but this inconsistency has made our life difficult.
I feel we should use device tree where available, and only fall back to platform data when necessary (no device tree available for platform, for example).
Quoted 6 lines
No, it is true that device tree is a useful tool, but it should be optional.
All the infrastructures of drivers must work perfectly without device tree.
The device tree is just one choice of how to give device information.
Which platform(s) are we talking about here?
Quoted 26 lines
Quoted 26 lines
Quoted 6 lines
This commit abolishes "u-boot,dm-pre-reloc" property because:
Quoted 7 lines
- Having a U-Boot specific property makes it difficult to share the device tree sources between Linux and U-Boot.
- The number of devices is generally larger than that of drivers. Each driver often has multiple devices with different base addresses. It seems more reasonable to add the pre-reloc attribute to drivers than devices.
The inability for platform data to specify which devices need to be pre-relocation is certainly a limitation. But I'm not sure that the solution is to remove that feature from the device tree. Prior to relocation memory may be severely limited. Things like GPIO and serial can create quite a few devices (e.g. Tegra has 16 for GPIO and 4 for serial), but only a subset may be needed before relocation (on Tegra only 2!).
I'm actually pretty comfortable with platform data having a limited subset of functionality, since I believe most platforms will use device tree for one reason or another.
Quoted 7 lines
Quoted 2 lines
Thoughts?
No, it is not justified to compel to use device tree unless Linux is the target OS.
Even in Linux, limited numbers of architrectures use device trees.
Fair enough, but let's look at this when the case comes up. So far the platforms that use I2C and SPI with DM do use device tree in Linux and probably should do in U-Boot.
Regards,
Simon
PATCH 8/8 dm: core: abolish u-boot, dm-pre-reloc property
Hi Simon,
Quoted 33 lines · On Tue, 18 Nov 2014 14:37:33 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 30 lines · Masahiro Yamada
Hi Simon,
Quoted 22 lines · On Mon, 17 Nov 2014 18:17:43 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 9 lines · Masahiro Yamada
The driver model provides two ways to pass the device information, platform data and device tree. Either way works to bind devices and drivers, but there is inconsistency in terms of how to pass the pre-reloc flag.
In the platform data way, the pre-reloc DM scan checks if each driver has
DM_FLAG_PRE_RELOCflag (this was changed to useU_BOOT_DRIVER_Fjust before). That is, each **driver** has the pre-reloc attribute.Quoted 6 lines
In the device tree control, the existence of "u-boot,dm-pre-reloc" is checked for each device node. The driver flag "
DM_FLAG_PRE_RELOC" is never checked. That is, each **device** owns the pre-reloc attribute.Drivers should generally work both with platform data and device tree, but this inconsistency has made our life difficult.
I feel we should use device tree where available, and only fall back to platform data when necessary (no device tree available for platform, for example).
Quoted 8 lines
Quoted 6 lines
No, it is true that device tree is a useful tool, but it should be optional.
All the infrastructures of drivers must work perfectly without device tree.
The device tree is just one choice of how to give device information.
Which platform(s) are we talking about here?
I am talking about the general design policy of drivers in U-Boot and Linux.
Quoted 28 lines
Quoted 28 lines
Quoted 27 lines
Quoted 5 lines
This commit abolishes "u-boot,dm-pre-reloc" property because:
Quoted 7 lines
- Having a U-Boot specific property makes it difficult to share the device tree sources between Linux and U-Boot.
- The number of devices is generally larger than that of drivers. Each driver often has multiple devices with different base addresses. It seems more reasonable to add the pre-reloc attribute to drivers than devices.
The inability for platform data to specify which devices need to be pre-relocation is certainly a limitation. But I'm not sure that the solution is to remove that feature from the device tree. Prior to relocation memory may be severely limited. Things like GPIO and serial can create quite a few devices (e.g. Tegra has 16 for GPIO and 4 for serial), but only a subset may be needed before relocation (on Tegra only 2!).
I'm actually pretty comfortable with platform data having a limited subset of functionality, since I believe most platforms will use device tree for one reason or another.
Thoughts?
Quoted 8 lines
Quoted 4 lines
No, it is not justified to compel to use device tree unless Linux is the target OS.
Even in Linux, limited numbers of architrectures use device trees.
Fair enough, but let's look at this when the case comes up. So far the platforms that use I2C and SPI with DM do use device tree in Linux and probably should do in U-Boot.
OK, so let's think about it when a problem happens.
Let's get back talking about this patch.
If 8/8 is not acceptable, I do not have motivation for 6/8 and 7/8, either.
I still believe that the top priority of the design policy is to share the same device tree source between U-Boot and Linux.
I am really unhappy about having such a u-boot specific property.
So, my suggestion is this patch, and one possible alternative is to bind all the devices even before relocation.
Only binding won't use much memory because U-Boot does not probe devices until they are actually used.
Both "u-boot,dm-pre-reloc" and DM_FLAG_PRE_RELOC will go away.
What do you think?
Best Regards
Masahiro Yamada
PATCH 8/8 dm: core: abolish u-boot, dm-pre-reloc property
Hi Masahiro,
Quoted 44 lines · Masahiro Yamada
Hi Simon,
Quoted 37 lines · On Tue, 18 Nov 2014 14:37:33 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 34 lines · Masahiro Yamada
Hi Simon,
Quoted 16 lines · On Mon, 17 Nov 2014 18:17:43 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 13 lines · Masahiro Yamada
The driver model provides two ways to pass the device information, platform data and device tree. Either way works to bind devices and drivers, but there is inconsistency in terms of how to pass the pre-reloc flag.
In the platform data way, the pre-reloc DM scan checks if each driver has
DM_FLAG_PRE_RELOCflag (this was changed to useU_BOOT_DRIVER_Fjust before). That is, each **driver** has the pre-reloc attribute.In the device tree control, the existence of "u-boot,dm-pre-reloc" is checked for each device node. The driver flag "
DM_FLAG_PRE_RELOC" is never checked. That is, each **device** owns the pre-reloc attribute.Quoted 6 lines
Quoted 2 lines
Drivers should generally work both with platform data and device tree, but this inconsistency has made our life difficult.
I feel we should use device tree where available, and only fall back to platform data when necessary (no device tree available for platform, for example).
No, it is true that device tree is a useful tool, but it should be optional.
All the infrastructures of drivers must work perfectly without device tree.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
The device tree is just one choice of how to give device information.
Which platform(s) are we talking about here?
I am talking about the general design policy of drivers in U-Boot and Linux.
Well Linux has moved away from platform data, right?
Quoted 43 lines
Quoted 31 lines
Quoted 31 lines
Quoted 30 lines
Quoted 8 lines
This commit abolishes "u-boot,dm-pre-reloc" property because:
Quoted 7 lines
- Having a U-Boot specific property makes it difficult to share the device tree sources between Linux and U-Boot.
- The number of devices is generally larger than that of drivers. Each driver often has multiple devices with different base addresses. It seems more reasonable to add the pre-reloc attribute to drivers than devices.
The inability for platform data to specify which devices need to be pre-relocation is certainly a limitation. But I'm not sure that the solution is to remove that feature from the device tree. Prior to relocation memory may be severely limited. Things like GPIO and serial can create quite a few devices (e.g. Tegra has 16 for GPIO and 4 for serial), but only a subset may be needed before relocation (on Tegra only 2!).
I'm actually pretty comfortable with platform data having a limited subset of functionality, since I believe most platforms will use device tree for one reason or another.
Thoughts?
Quoted 8 lines
Quoted 4 lines
No, it is not justified to compel to use device tree unless Linux is the target OS.
Even in Linux, limited numbers of architrectures use device trees.
Fair enough, but let's look at this when the case comes up. So far the platforms that use I2C and SPI with DM do use device tree in Linux and probably should do in U-Boot.
OK, so let's think about it when a problem happens.
Quoted 6 lines
Let's get back talking about this patch.
If 8/8 is not acceptable, I do not have motivation for 6/8 and 7/8, either.I still believe that the top priority of the design policy is to share the same device tree source between U-Boot and Linux.
Agreed, and we really need to line up so we are using the same source. I do want to point out that we mostly do, the differences are small.
Quoted 11 lines
I am really unhappy about having such a u-boot specific property.
So, my suggestion is this patch, and one possible alternative is to bind all the devices even before relocation.
Only binding won't use much memory because U-Boot does not probe devices until they are actually used.
Both "u-boot,dm-pre-reloc" andDM_FLAG_PRE_RELOCwill go away.What do you think?
That's a waste of time since we won't use them and the goal is to do as little as possible before relocation.
I don't see that the pre-reloc property is a huge problem. In the case of serial I found a way around it (using aliases). I hope that it will be possible more generally and we can review that at some point in the future. There are bigger fish to fry in driver model I think - so many uclasses to write.
Regards,
Simon
PATCH 8/8 dm: core: abolish u-boot, dm-pre-reloc property
Quoted 50 lines · Simon Glass
Hi Masahiro,
Quoted 47 lines · Masahiro Yamada
Hi Simon,
Quoted 40 lines · On Tue, 18 Nov 2014 14:37:33 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 36 lines · Masahiro Yamada
Hi Simon,
Quoted 16 lines · On Mon, 17 Nov 2014 18:17:43 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 13 lines · Masahiro Yamada
The driver model provides two ways to pass the device information, platform data and device tree. Either way works to bind devices and drivers, but there is inconsistency in terms of how to pass the pre-reloc flag.
In the platform data way, the pre-reloc DM scan checks if each driver has
DM_FLAG_PRE_RELOCflag (this was changed to useU_BOOT_DRIVER_Fjust before). That is, each **driver** has the pre-reloc attribute.In the device tree control, the existence of "u-boot,dm-pre-reloc" is checked for each device node. The driver flag "
DM_FLAG_PRE_RELOC" is never checked. That is, each **device** owns the pre-reloc attribute.Quoted 6 lines
Quoted 2 lines
Drivers should generally work both with platform data and device tree, but this inconsistency has made our life difficult.
I feel we should use device tree where available, and only fall back to platform data when necessary (no device tree available for platform, for example).
No, it is true that device tree is a useful tool, but it should be optional.
All the infrastructures of drivers must work perfectly without device tree.
The device tree is just one choice of how to give device information.
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
Which platform(s) are we talking about here?
I am talking about the general design policy of drivers in U-Boot and Linux.
Well Linux has moved away from platform data, right?
As a blanket statement, that isn't true.
Some architectures (e.g. ARM) have moved to DT. That move is something done by the ARM maintainers, not something that's necessarily being forced upon every single architecture. I know of no move to force everyone to convert to DT.
What makes sense for Linux doesn't always make sense for everything else anyway.
In my opinion, DT in Linux isn't actually doing much that's useful, and DT in a boot loader is likely to be even less useful.
PATCH 8/8 dm: core: abolish u-boot, dm-pre-reloc property
In reply to Simon GlassHi Simon,
Quoted 123 lines · On Thu, 20 Nov 2014 16:44:22 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 44 lines · Masahiro Yamada
Hi Simon,
Quoted 37 lines · On Tue, 18 Nov 2014 14:37:33 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 34 lines · Masahiro Yamada
Hi Simon,
Quoted 16 lines · On Mon, 17 Nov 2014 18:17:43 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 13 lines · Masahiro Yamada
The driver model provides two ways to pass the device information, platform data and device tree. Either way works to bind devices and drivers, but there is inconsistency in terms of how to pass the pre-reloc flag.
In the platform data way, the pre-reloc DM scan checks if each driver has
DM_FLAG_PRE_RELOCflag (this was changed to useU_BOOT_DRIVER_Fjust before). That is, each **driver** has the pre-reloc attribute.In the device tree control, the existence of "u-boot,dm-pre-reloc" is checked for each device node. The driver flag "
DM_FLAG_PRE_RELOC" is never checked. That is, each **device** owns the pre-reloc attribute.Quoted 6 lines
Quoted 2 lines
Drivers should generally work both with platform data and device tree, but this inconsistency has made our life difficult.
I feel we should use device tree where available, and only fall back to platform data when necessary (no device tree available for platform, for example).
No, it is true that device tree is a useful tool, but it should be optional.
All the infrastructures of drivers must work perfectly without device tree.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
The device tree is just one choice of how to give device information.
Which platform(s) are we talking about here?
I am talking about the general design policy of drivers in U-Boot and Linux.
Well Linux has moved away from platform data, right?
Quoted 43 lines
Quoted 31 lines
Quoted 31 lines
Quoted 30 lines
Quoted 8 lines
This commit abolishes "u-boot,dm-pre-reloc" property because:
Quoted 7 lines
- Having a U-Boot specific property makes it difficult to share the device tree sources between Linux and U-Boot.
- The number of devices is generally larger than that of drivers. Each driver often has multiple devices with different base addresses. It seems more reasonable to add the pre-reloc attribute to drivers than devices.
The inability for platform data to specify which devices need to be pre-relocation is certainly a limitation. But I'm not sure that the solution is to remove that feature from the device tree. Prior to relocation memory may be severely limited. Things like GPIO and serial can create quite a few devices (e.g. Tegra has 16 for GPIO and 4 for serial), but only a subset may be needed before relocation (on Tegra only 2!).
I'm actually pretty comfortable with platform data having a limited subset of functionality, since I believe most platforms will use device tree for one reason or another.
Thoughts?
Quoted 8 lines
Quoted 4 lines
No, it is not justified to compel to use device tree unless Linux is the target OS.
Even in Linux, limited numbers of architrectures use device trees.
Fair enough, but let's look at this when the case comes up. So far the platforms that use I2C and SPI with DM do use device tree in Linux and probably should do in U-Boot.
OK, so let's think about it when a problem happens.
Quoted 6 lines
Let's get back talking about this patch.
If 8/8 is not acceptable, I do not have motivation for 6/8 and 7/8, either.I still believe that the top priority of the design policy is to share the same device tree source between U-Boot and Linux.
Agreed, and we really need to line up so we are using the same source. I do want to point out that we mostly do, the differences are small.
Quoted 11 lines
I am really unhappy about having such a u-boot specific property.
So, my suggestion is this patch, and one possible alternative is to bind all the devices even before relocation.
Only binding won't use much memory because U-Boot does not probe devices until they are actually used.
Both "u-boot,dm-pre-reloc" andDM_FLAG_PRE_RELOCwill go away.What do you think?
Quoted 8 lines
That's a waste of time since we won't use them and the goal is to do as little as possible before relocation.
I don't see that the pre-reloc property is a huge problem. In the case of serial I found a way around it (using aliases). I hope that it will be possible more generally and we can review that at some point in the future. There are bigger fish to fry in driver model I think - so many uclasses to write.
OK. I've marked 6/8 thru 8/8 as Rejected.
No point for 6/8 and 7/8 without 8/8, I think.
Best Regards
Masahiro Yamada
PATCH 8/8 dm: core: abolish u-boot, dm-pre-reloc property
HI Masahiro,
Quoted 141 lines · Masahiro Yamada
Hi Simon,
Quoted 123 lines · On Thu, 20 Nov 2014 16:44:22 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 44 lines · Masahiro Yamada
Hi Simon,
Quoted 37 lines · On Tue, 18 Nov 2014 14:37:33 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 34 lines · Masahiro Yamada
Hi Simon,
Quoted 16 lines · On Mon, 17 Nov 2014 18:17:43 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 13 lines · Masahiro Yamada
The driver model provides two ways to pass the device information, platform data and device tree. Either way works to bind devices and drivers, but there is inconsistency in terms of how to pass the pre-reloc flag.
In the platform data way, the pre-reloc DM scan checks if each driver has
DM_FLAG_PRE_RELOCflag (this was changed to useU_BOOT_DRIVER_Fjust before). That is, each **driver** has the pre-reloc attribute.In the device tree control, the existence of "u-boot,dm-pre-reloc" is checked for each device node. The driver flag "
DM_FLAG_PRE_RELOC" is never checked. That is, each **device** owns the pre-reloc attribute.Quoted 6 lines
Quoted 2 lines
Drivers should generally work both with platform data and device tree, but this inconsistency has made our life difficult.
I feel we should use device tree where available, and only fall back to platform data when necessary (no device tree available for platform, for example).
No, it is true that device tree is a useful tool, but it should be optional.
All the infrastructures of drivers must work perfectly without device tree.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
The device tree is just one choice of how to give device information.
Which platform(s) are we talking about here?
I am talking about the general design policy of drivers in U-Boot and Linux.
Well Linux has moved away from platform data, right?
Quoted 43 lines
Quoted 31 lines
Quoted 31 lines
Quoted 30 lines
Quoted 8 lines
This commit abolishes "u-boot,dm-pre-reloc" property because:
Quoted 7 lines
- Having a U-Boot specific property makes it difficult to share the device tree sources between Linux and U-Boot.
- The number of devices is generally larger than that of drivers. Each driver often has multiple devices with different base addresses. It seems more reasonable to add the pre-reloc attribute to drivers than devices.
The inability for platform data to specify which devices need to be pre-relocation is certainly a limitation. But I'm not sure that the solution is to remove that feature from the device tree. Prior to relocation memory may be severely limited. Things like GPIO and serial can create quite a few devices (e.g. Tegra has 16 for GPIO and 4 for serial), but only a subset may be needed before relocation (on Tegra only 2!).
I'm actually pretty comfortable with platform data having a limited subset of functionality, since I believe most platforms will use device tree for one reason or another.
Thoughts?
Quoted 8 lines
Quoted 4 lines
No, it is not justified to compel to use device tree unless Linux is the target OS.
Even in Linux, limited numbers of architrectures use device trees.
Fair enough, but let's look at this when the case comes up. So far the platforms that use I2C and SPI with DM do use device tree in Linux and probably should do in U-Boot.
OK, so let's think about it when a problem happens.
Quoted 6 lines
Let's get back talking about this patch.
If 8/8 is not acceptable, I do not have motivation for 6/8 and 7/8, either.I still believe that the top priority of the design policy is to share the same device tree source between U-Boot and Linux.
Agreed, and we really need to line up so we are using the same source. I do want to point out that we mostly do, the differences are small.
Quoted 11 lines
I am really unhappy about having such a u-boot specific property.
So, my suggestion is this patch, and one possible alternative is to bind all the devices even before relocation.
Only binding won't use much memory because U-Boot does not probe devices until they are actually used.
Both "u-boot,dm-pre-reloc" andDM_FLAG_PRE_RELOCwill go away.What do you think?
Quoted 8 lines
That's a waste of time since we won't use them and the goal is to do as little as possible before relocation.
I don't see that the pre-reloc property is a huge problem. In the case of serial I found a way around it (using aliases). I hope that it will be possible more generally and we can review that at some point in the future. There are bigger fish to fry in driver model I think - so many uclasses to write.
Quoted 2 lines
OK. I've marked 6/8 thru 8/8 as Rejected.
No point for 6/8 and 7/8 without 8/8, I think.
I'm not so sure. Your method reduces the number of drivers that are considered for pre-reloc use which is a benefit.
Maybe we should require that all pre-reloc devices have an alias. Then we can bind devices as they are needed as we do for serial. Needs more thought though.
Regards,
Simon
PATCH 8/8 dm: core: abolish u-boot, dm-pre-reloc property
Hi Simon,
Quoted 144 lines · On Mon, 24 Nov 2014 15:29:23 -0700 Simon Glass <[email protected]> wrote:
HI Masahiro,
Quoted 141 lines · Masahiro Yamada
Hi Simon,
Quoted 131 lines · On Thu, 20 Nov 2014 16:44:22 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 44 lines · Masahiro Yamada
Hi Simon,
Quoted 37 lines · On Tue, 18 Nov 2014 14:37:33 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 34 lines · Masahiro Yamada
Hi Simon,
Quoted 16 lines · On Mon, 17 Nov 2014 18:17:43 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 13 lines · Masahiro Yamada
The driver model provides two ways to pass the device information, platform data and device tree. Either way works to bind devices and drivers, but there is inconsistency in terms of how to pass the pre-reloc flag.
In the platform data way, the pre-reloc DM scan checks if each driver has
DM_FLAG_PRE_RELOCflag (this was changed to useU_BOOT_DRIVER_Fjust before). That is, each **driver** has the pre-reloc attribute.In the device tree control, the existence of "u-boot,dm-pre-reloc" is checked for each device node. The driver flag "
DM_FLAG_PRE_RELOC" is never checked. That is, each **device** owns the pre-reloc attribute.Quoted 6 lines
Quoted 2 lines
Drivers should generally work both with platform data and device tree, but this inconsistency has made our life difficult.
I feel we should use device tree where available, and only fall back to platform data when necessary (no device tree available for platform, for example).
No, it is true that device tree is a useful tool, but it should be optional.
All the infrastructures of drivers must work perfectly without device tree.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
The device tree is just one choice of how to give device information.
Which platform(s) are we talking about here?
I am talking about the general design policy of drivers in U-Boot and Linux.
Well Linux has moved away from platform data, right?
Quoted 43 lines
Quoted 31 lines
Quoted 31 lines
Quoted 30 lines
Quoted 8 lines
This commit abolishes "u-boot,dm-pre-reloc" property because:
Quoted 7 lines
- Having a U-Boot specific property makes it difficult to share the device tree sources between Linux and U-Boot.
- The number of devices is generally larger than that of drivers. Each driver often has multiple devices with different base addresses. It seems more reasonable to add the pre-reloc attribute to drivers than devices.
The inability for platform data to specify which devices need to be pre-relocation is certainly a limitation. But I'm not sure that the solution is to remove that feature from the device tree. Prior to relocation memory may be severely limited. Things like GPIO and serial can create quite a few devices (e.g. Tegra has 16 for GPIO and 4 for serial), but only a subset may be needed before relocation (on Tegra only 2!).
I'm actually pretty comfortable with platform data having a limited subset of functionality, since I believe most platforms will use device tree for one reason or another.
Thoughts?
Quoted 8 lines
Quoted 4 lines
No, it is not justified to compel to use device tree unless Linux is the target OS.
Even in Linux, limited numbers of architrectures use device trees.
Fair enough, but let's look at this when the case comes up. So far the platforms that use I2C and SPI with DM do use device tree in Linux and probably should do in U-Boot.
OK, so let's think about it when a problem happens.
Quoted 6 lines
Let's get back talking about this patch.
If 8/8 is not acceptable, I do not have motivation for 6/8 and 7/8, either.I still believe that the top priority of the design policy is to share the same device tree source between U-Boot and Linux.
Agreed, and we really need to line up so we are using the same source. I do want to point out that we mostly do, the differences are small.
Quoted 11 lines
I am really unhappy about having such a u-boot specific property.
So, my suggestion is this patch, and one possible alternative is to bind all the devices even before relocation.
Only binding won't use much memory because U-Boot does not probe devices until they are actually used.
Both "u-boot,dm-pre-reloc" andDM_FLAG_PRE_RELOCwill go away.What do you think?
That's a waste of time since we won't use them and the goal is to do as little as possible before relocation.
I don't see that the pre-reloc property is a huge problem. In the case of serial I found a way around it (using aliases). I hope that it will be possible more generally and we can review that at some point in the future. There are bigger fish to fry in driver model I think - so many uclasses to write.
Quoted 5 lines
Quoted 2 lines
OK. I've marked 6/8 thru 8/8 as Rejected.
No point for 6/8 and 7/8 without 8/8, I think.I'm not so sure. Your method reduces the number of drivers that are considered for pre-reloc use which is a benefit.
If you insist on "u-boot,dm-pre-reloc" property (= each device has the pre-reloc attribute), I think the DM_FLAG_PRE_RELOC flag should be moved from U_BOOT_DRIVER to U_BOOT_DEVICE for consistency. (Or 6/8 should introduce U_BOOT_DEVICE_F instead of U_BOOT_DRIVER_F)
That is why I thought 6/8 and 7/8 should not be applied.
Quoted 3 lines
Maybe we should require that all pre-reloc devices have an alias. Then we can bind devices as they are needed as we do for serial. Needs more thought though.
I want to see first how this will work
because I believe platform data should be implemented consistently with that.
Best Regards
Masahiro Yamada
PATCH 8/8 dm: core: abolish u-boot, dm-pre-reloc property
Hi Masahiro,
Quoted 158 lines · Masahiro Yamada
Hi Simon,
Quoted 144 lines · On Mon, 24 Nov 2014 15:29:23 -0700 Simon Glass <[email protected]> wrote:
HI Masahiro,
Quoted 141 lines · Masahiro Yamada
Hi Simon,
Quoted 131 lines · On Thu, 20 Nov 2014 16:44:22 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 44 lines · Masahiro Yamada
Hi Simon,
Quoted 37 lines · On Tue, 18 Nov 2014 14:37:33 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 34 lines · Masahiro Yamada
Hi Simon,
Quoted 16 lines · On Mon, 17 Nov 2014 18:17:43 +0000 Simon Glass <[email protected]> wrote:
Hi Masahiro,
Quoted 13 lines · Masahiro Yamada
The driver model provides two ways to pass the device information, platform data and device tree. Either way works to bind devices and drivers, but there is inconsistency in terms of how to pass the pre-reloc flag.
In the platform data way, the pre-reloc DM scan checks if each driver has
DM_FLAG_PRE_RELOCflag (this was changed to useU_BOOT_DRIVER_Fjust before). That is, each **driver** has the pre-reloc attribute.In the device tree control, the existence of "u-boot,dm-pre-reloc" is checked for each device node. The driver flag "
DM_FLAG_PRE_RELOC" is never checked. That is, each **device** owns the pre-reloc attribute.Quoted 6 lines
Quoted 2 lines
Drivers should generally work both with platform data and device tree, but this inconsistency has made our life difficult.
I feel we should use device tree where available, and only fall back to platform data when necessary (no device tree available for platform, for example).
No, it is true that device tree is a useful tool, but it should be optional.
All the infrastructures of drivers must work perfectly without device tree.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
The device tree is just one choice of how to give device information.
Which platform(s) are we talking about here?
I am talking about the general design policy of drivers in U-Boot and Linux.
Well Linux has moved away from platform data, right?
Quoted 43 lines
Quoted 31 lines
Quoted 31 lines
Quoted 30 lines
Quoted 8 lines
This commit abolishes "u-boot,dm-pre-reloc" property because:
Quoted 7 lines
- Having a U-Boot specific property makes it difficult to share the device tree sources between Linux and U-Boot.
- The number of devices is generally larger than that of drivers. Each driver often has multiple devices with different base addresses. It seems more reasonable to add the pre-reloc attribute to drivers than devices.
The inability for platform data to specify which devices need to be pre-relocation is certainly a limitation. But I'm not sure that the solution is to remove that feature from the device tree. Prior to relocation memory may be severely limited. Things like GPIO and serial can create quite a few devices (e.g. Tegra has 16 for GPIO and 4 for serial), but only a subset may be needed before relocation (on Tegra only 2!).
I'm actually pretty comfortable with platform data having a limited subset of functionality, since I believe most platforms will use device tree for one reason or another.
Thoughts?
Quoted 8 lines
Quoted 4 lines
No, it is not justified to compel to use device tree unless Linux is the target OS.
Even in Linux, limited numbers of architrectures use device trees.
Fair enough, but let's look at this when the case comes up. So far the platforms that use I2C and SPI with DM do use device tree in Linux and probably should do in U-Boot.
OK, so let's think about it when a problem happens.
Quoted 6 lines
Let's get back talking about this patch.
If 8/8 is not acceptable, I do not have motivation for 6/8 and 7/8, either.I still believe that the top priority of the design policy is to share the same device tree source between U-Boot and Linux.
Agreed, and we really need to line up so we are using the same source. I do want to point out that we mostly do, the differences are small.
Quoted 11 lines
I am really unhappy about having such a u-boot specific property.
So, my suggestion is this patch, and one possible alternative is to bind all the devices even before relocation.
Only binding won't use much memory because U-Boot does not probe devices until they are actually used.
Both "u-boot,dm-pre-reloc" andDM_FLAG_PRE_RELOCwill go away.What do you think?
That's a waste of time since we won't use them and the goal is to do as little as possible before relocation.
I don't see that the pre-reloc property is a huge problem. In the case of serial I found a way around it (using aliases). I hope that it will be possible more generally and we can review that at some point in the future. There are bigger fish to fry in driver model I think - so many uclasses to write.
Quoted 5 lines
Quoted 2 lines
OK. I've marked 6/8 thru 8/8 as Rejected.
No point for 6/8 and 7/8 without 8/8, I think.I'm not so sure. Your method reduces the number of drivers that are considered for pre-reloc use which is a benefit.
Quoted 6 lines
If you insist on "u-boot,dm-pre-reloc" property (= each device has the pre-reloc attribute), I think the
DM_FLAG_PRE_RELOCflag should be moved fromU_BOOT_DRIVERtoU_BOOT_DEVICEfor consistency. (Or 6/8 should introduceU_BOOT_DEVICE_Finstead ofU_BOOT_DRIVER_F)That is why I thought 6/8 and 7/8 should not be applied.
Let's do nothing for now. I don't like that property and would to get rid of it too.
Quoted 8 lines
Quoted 5 lines
Maybe we should require that all pre-reloc devices have an alias. Then we can bind devices as they are needed as we do for serial. Needs more thought though.
I want to see first how this will work
because I believe platform data should be implemented consistently with that.
I'm thinking about it. If I take a look at adding more functions to the GPIO uclass (automatically handing gpio nodes in the device tree) then I might try to solve this one too.
For platform data I suppose it is just the order in the executable that determines the numbering. Pretty ugly.
Regards,
Simon