[U-Boot] [PATCH 0/8] dm: core: abolish "u-boot, dm-pre-reloc" property and various refactorings

[U-Boot] [PATCH 0/8] dm: core: abolish "u-boot, dm-pre-reloc" property and various refactorings

[Masahiro Yamada]

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(-)

-- 
1.9.1

[U-Boot] [PATCH 1/8] dm: core: a trivial clean up

[Masahiro Yamada]

Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
---

 drivers/core/root.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/core/root.c b/drivers/core/root.c
index a328a48..47b3acf 100644
--- a/drivers/core/root.c
+++ b/drivers/core/root.c
@@ -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
-- 
1.9.1

[U-Boot] [PATCH 2/8] dm: core: remove meaningless if conditional

[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 <yamada.m@jp.panasonic.com>
---

 drivers/core/lists.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/core/lists.c b/drivers/core/lists.c
index 3a1ea85..0aad56d 100644
--- a/drivers/core/lists.c
+++ b/drivers/core/lists.c
@@ -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;
 		}
 
-- 
1.9.1

[U-Boot] [PATCH 3/8] dm: core: remove unnecessary return condition in driver lookup

[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 <yamada.m@jp.panasonic.com>
---

 drivers/core/lists.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/core/lists.c b/drivers/core/lists.c
index 0aad56d..0c58ec4 100644
--- a/drivers/core/lists.c
+++ b/drivers/core/lists.c
@@ -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;
-- 
1.9.1

[U-Boot] [PATCH 4/8] dm: core: remove unnecessary return condition in uclass lookup

[Masahiro Yamada]

These 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.

Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
---

 drivers/core/lists.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/core/lists.c b/drivers/core/lists.c
index 0c58ec4..ddbac38 100644
--- a/drivers/core/lists.c
+++ b/drivers/core/lists.c
@@ -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;
-- 
1.9.1

[U-Boot] [PATCH 5/8] dm: core: refactor linker lists lookup code

[Masahiro Yamada]

It 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.)

Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
---

 drivers/core/lists.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/core/lists.c b/drivers/core/lists.c
index ddbac38..1476715 100644
--- a/drivers/core/lists.c
+++ b/drivers/core/lists.c
@@ -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) {
-- 
1.9.1

[U-Boot] [PATCH 6/8] dm: core: look up drivers more efficiently

[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_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);
 	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

[U-Boot] [PATCH 7/8] dm: core: declare pre-reloc drivers with U_BOOT_DRIVER_F

[Masahiro Yamada]

Replace U_BOOT_DRIVER with U_BOOT_DRIVER_F where the DM_FLAG_PRE_RELOC
flag is set.

Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
---

 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(-)

diff --git a/drivers/serial/sandbox.c b/drivers/serial/sandbox.c
index cd2f91e..b7e9f4e 100644
--- a/drivers/serial/sandbox.c
+++ b/drivers/serial/sandbox.c
@@ -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 = {
diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
index d6cf1d8..e93d3e2 100644
--- a/drivers/serial/serial_mxc.c
+++ b/drivers/serial/serial_mxc.c
@@ -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
diff --git a/drivers/serial/serial_omap.c b/drivers/serial/serial_omap.c
index 265fe00..0dffba3 100644
--- a/drivers/serial/serial_omap.c
+++ b/drivers/serial/serial_omap.c
@@ -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,
 };
diff --git a/drivers/serial/serial_pl01x.c b/drivers/serial/serial_pl01x.c
index 38dda91..53eab9a 100644
--- a/drivers/serial/serial_pl01x.c
+++ b/drivers/serial/serial_pl01x.c
@@ -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
diff --git a/drivers/serial/serial_s5p.c b/drivers/serial/serial_s5p.c
index 8469afd..abab286 100644
--- a/drivers/serial/serial_s5p.c
+++ b/drivers/serial/serial_s5p.c
@@ -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,
 };
diff --git a/drivers/serial/serial_uniphier.c b/drivers/serial/serial_uniphier.c
index 6046efb..6fe68c7 100644
--- a/drivers/serial/serial_uniphier.c
+++ b/drivers/serial/serial_uniphier.c
@@ -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,
 };
diff --git a/include/dm/device.h b/include/dm/device.h
index a2fd7b6..ed14af9 100644
--- a/include/dm/device.h
+++ b/include/dm/device.h
@@ -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
  *
diff --git a/test/dm/test-driver.c b/test/dm/test-driver.c
index bc6a6e7..065bcbf 100644
--- a/test/dm/test-driver.c
+++ b/test/dm/test-driver.c
@@ -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,
 };
-- 
1.9.1

[U-Boot] [PATCH 8/8] dm: core: abolish u-boot, dm-pre-reloc property

[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_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.

Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
---

 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(-)

diff --git a/arch/arm/dts/exynos5.dtsi b/arch/arm/dts/exynos5.dtsi
index e539068..dc5405b 100644
--- a/arch/arm/dts/exynos5.dtsi
+++ b/arch/arm/dts/exynos5.dtsi
@@ -244,7 +244,6 @@
 		compatible = "samsung,exynos4210-uart";
 		reg = <0x12C30000 0x100>;
 		interrupts = <0 54 0>;
-		u-boot,dm-pre-reloc;
 		id = <3>;
 	};
 
diff --git a/doc/driver-model/README.txt b/doc/driver-model/README.txt
index e4114d5..06f6515 100644
--- a/doc/driver-model/README.txt
+++ b/doc/driver-model/README.txt
@@ -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
diff --git a/drivers/core/lists.c b/drivers/core/lists.c
index 5b61ab5..2109a9f 100644
--- a/drivers/core/lists.c
+++ b/drivers/core/lists.c
@@ -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;
diff --git a/drivers/core/root.c b/drivers/core/root.c
index 02970c8..2f0c409 100644
--- a/drivers/core/root.c
+++ b/drivers/core/root.c
@@ -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;
 	}
diff --git a/drivers/core/simple-bus.c b/drivers/core/simple-bus.c
index 3ea4d82..483f8e2 100644
--- a/drivers/core/simple-bus.c
+++ b/drivers/core/simple-bus.c
@@ -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,
diff --git a/include/dm/lists.h b/include/dm/lists.h
index fbd428d..56bf6a7 100644
--- a/include/dm/lists.h
+++ b/include/dm/lists.h
@@ -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
diff --git a/include/dm/root.h b/include/dm/root.h
index 61d1cd8..960e888 100644
--- a/include/dm/root.h
+++ b/include/dm/root.h
@@ -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);
diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
index cd2c389..8cb1f76 100644
--- a/test/dm/test-fdt.c
+++ b/test/dm/test-fdt.c
@@ -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,
diff --git a/test/dm/test.dts b/test/dm/test.dts
index 1fba792..86f024f 100644
--- a/test/dm/test.dts
+++ b/test/dm/test.dts
@@ -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 {
-- 
1.9.1

[U-Boot] [PATCH 1/8] dm: core: a trivial clean up

[Simon Glass]

On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 2/8] dm: core: remove meaningless if conditional

[Simon Glass]

Hi Masahiro,

On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> 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 <yamada.m@jp.panasonic.com>
> ---
>
>  drivers/core/lists.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/core/lists.c b/drivers/core/lists.c
> index 3a1ea85..0aad56d 100644
> --- a/drivers/core/lists.c
> +++ b/drivers/core/lists.c
> @@ -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

[U-Boot] [PATCH 3/8] dm: core: remove unnecessary return condition in driver lookup

[Simon Glass]

On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> 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 <yamada.m@jp.panasonic.com>

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 4/8] dm: core: remove unnecessary return condition in uclass lookup

[Simon Glass]

On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> These 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.
>
> Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>

Yes we now use proper error codes to detect an invalid uclass, so it
is good to clean this up.

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 5/8] dm: core: refactor linker lists lookup code

[Simon Glass]

On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> It 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.)
>
> Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 6/8] dm: core: look up drivers more efficiently

[Simon Glass]

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

[U-Boot] [PATCH 7/8] dm: core: declare pre-reloc drivers with U_BOOT_DRIVER_F

[Simon Glass]

On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> Replace U_BOOT_DRIVER with U_BOOT_DRIVER_F where the DM_FLAG_PRE_RELOC
> flag is set.
>
> Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 2/8] dm: core: remove meaningless if conditional

[Masahiro Yamada]

Hi Simon,



On Mon, 17 Nov 2014 09:14:15 +0000
Simon Glass <sjg@chromium.org> wrote:

> Hi Masahiro,
> 
> On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> > 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 <yamada.m@jp.panasonic.com>
> > ---
> >
> >  drivers/core/lists.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/core/lists.c b/drivers/core/lists.c
> > index 3a1ea85..0aad56d 100644
> > --- a/drivers/core/lists.c
> > +++ b/drivers/core/lists.c
> > @@ -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?


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

[U-Boot] [PATCH 6/8] dm: core: look up drivers more efficiently

[Masahiro Yamada]

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

[U-Boot] [PATCH 8/8] dm: core: abolish u-boot, dm-pre-reloc property

[Simon Glass]

Hi Masahiro,

On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> 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_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.

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).

>
> 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

[U-Boot] [PATCH 2/8] dm: core: remove meaningless if conditional

[Simon Glass]

Hi Masahiro,

On 17 November 2014 11:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> Hi Simon,
>
>
>
> On Mon, 17 Nov 2014 09:14:15 +0000
> Simon Glass <sjg@chromium.org> wrote:
>
>> Hi Masahiro,
>>
>> On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>> > 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 <yamada.m@jp.panasonic.com>
>> > ---
>> >
>> >  drivers/core/lists.c | 3 +--
>> >  1 file changed, 1 insertion(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/core/lists.c b/drivers/core/lists.c
>> > index 3a1ea85..0aad56d 100644
>> > --- a/drivers/core/lists.c
>> > +++ b/drivers/core/lists.c
>> > @@ -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?
>
>
> 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.

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 <sjg@chromium.org>

Regards,
Simon

[U-Boot] [PATCH 6/8] dm: core: look up drivers more efficiently

[Simon Glass]

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

[U-Boot] [PATCH 8/8] dm: core: abolish u-boot, dm-pre-reloc property

[Masahiro Yamada]

Hi Simon,



On Mon, 17 Nov 2014 18:17:43 +0000
Simon Glass <sjg@chromium.org> wrote:

> Hi Masahiro,
> 
> On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> > 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_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.
> 
> 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.




> >
> > 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?
> 

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

[U-Boot] [PATCH 8/8] dm: core: abolish u-boot, dm-pre-reloc property

[Simon Glass]

Hi Masahiro,

On 18 November 2014 12:51, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> Hi Simon,
>
>
>
> On Mon, 17 Nov 2014 18:17:43 +0000
> Simon Glass <sjg@chromium.org> wrote:
>
>> Hi Masahiro,
>>
>> On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>> > 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_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.
>>
>> 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.
>

Which platform(s) are we talking about here?

>
>
>
>> >
>> > 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?
>>
>
> 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

[U-Boot] [PATCH 8/8] dm: core: abolish u-boot, dm-pre-reloc property

[Masahiro Yamada]

Hi Simon,



On Tue, 18 Nov 2014 14:37:33 +0000
Simon Glass <sjg@chromium.org> wrote:

> Hi Masahiro,
> 
> On 18 November 2014 12:51, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> > Hi Simon,
> >
> >
> >
> > On Mon, 17 Nov 2014 18:17:43 +0000
> > Simon Glass <sjg@chromium.org> wrote:
> >
> >> Hi Masahiro,
> >>
> >> On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> >> > 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_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.
> >>
> >> 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.
> >
> 
> Which platform(s) are we talking about here?


I am talking about the general design policy of drivers
in U-Boot and Linux.



> >
> >
> >> >
> >> > 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?
> >>
> >
> > 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

[U-Boot] [PATCH 8/8] dm: core: abolish u-boot, dm-pre-reloc property

[Simon Glass]

Hi Masahiro,

On 19 November 2014 09:21, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> Hi Simon,
>
>
>
> On Tue, 18 Nov 2014 14:37:33 +0000
> Simon Glass <sjg@chromium.org> wrote:
>
>> Hi Masahiro,
>>
>> On 18 November 2014 12:51, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>> > Hi Simon,
>> >
>> >
>> >
>> > On Mon, 17 Nov 2014 18:17:43 +0000
>> > Simon Glass <sjg@chromium.org> wrote:
>> >
>> >> Hi Masahiro,
>> >>
>> >> On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>> >> > 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_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.
>> >>
>> >> 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.
>> >
>>
>> 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?

>
>
>
>> >
>> >
>> >> >
>> >> > 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?
>> >>
>> >
>> > 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.

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.

>
> 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?

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

[U-Boot] [PATCH 8/8] dm: core: abolish u-boot, dm-pre-reloc property

[Stephen Warren]

On 11/20/2014 09:44 AM, Simon Glass wrote:
> Hi Masahiro,
>
> On 19 November 2014 09:21, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>> Hi Simon,
>>
>>
>>
>> On Tue, 18 Nov 2014 14:37:33 +0000
>> Simon Glass <sjg@chromium.org> wrote:
>>
>>> Hi Masahiro,
>>>
>>> On 18 November 2014 12:51, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>>>> Hi Simon,
>>>>
>>>>
>>>>
>>>> On Mon, 17 Nov 2014 18:17:43 +0000
>>>> Simon Glass <sjg@chromium.org> wrote:
>>>>
>>>>> Hi Masahiro,
>>>>>
>>>>> On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>>>>>> 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_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.
>>>>>
>>>>> 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.
>>>>
>>>
>>> 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.

[U-Boot] [PATCH 8/8] dm: core: abolish u-boot, dm-pre-reloc property

[Masahiro Yamada]

Hi Simon,



On Thu, 20 Nov 2014 16:44:22 +0000
Simon Glass <sjg@chromium.org> wrote:

> Hi Masahiro,
> 
> On 19 November 2014 09:21, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> > Hi Simon,
> >
> >
> >
> > On Tue, 18 Nov 2014 14:37:33 +0000
> > Simon Glass <sjg@chromium.org> wrote:
> >
> >> Hi Masahiro,
> >>
> >> On 18 November 2014 12:51, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> >> > Hi Simon,
> >> >
> >> >
> >> >
> >> > On Mon, 17 Nov 2014 18:17:43 +0000
> >> > Simon Glass <sjg@chromium.org> wrote:
> >> >
> >> >> Hi Masahiro,
> >> >>
> >> >> On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> >> >> > 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_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.
> >> >>
> >> >> 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.
> >> >
> >>
> >> 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?
> 
> >
> >
> >
> >> >
> >> >
> >> >> >
> >> >> > 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?
> >> >>
> >> >
> >> > 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.
> 
> 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.
> 
> >
> > 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?
> 
> 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

[U-Boot] [PATCH 1/8] dm: core: a trivial clean up

[Simon Glass]

On 17 November 2014 at 10:08, Simon Glass <sjg@chromium.org> wrote:
> On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>> Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
>
> Acked-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-dm.

[U-Boot] [PATCH 2/8] dm: core: remove meaningless if conditional

[Simon Glass]

On 17 November 2014 at 19:23, Simon Glass <sjg@chromium.org> wrote:
> Hi Masahiro,
>
> On 17 November 2014 11:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>> Hi Simon,
>>
>>
>>
>> On Mon, 17 Nov 2014 09:14:15 +0000
>> Simon Glass <sjg@chromium.org> wrote:
>>
>>> Hi Masahiro,
>>>
>>> On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>>> > 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 <yamada.m@jp.panasonic.com>
>>> > ---
>>> >
>>> >  drivers/core/lists.c | 3 +--
>>> >  1 file changed, 1 insertion(+), 2 deletions(-)
>>> >
>>> > diff --git a/drivers/core/lists.c b/drivers/core/lists.c
>>> > index 3a1ea85..0aad56d 100644
>>> > --- a/drivers/core/lists.c
>>> > +++ b/drivers/core/lists.c
>>> > @@ -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?
>>
>>
>> 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.
>
> 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 <sjg@chromium.org>

Applied to u-boot-dm.

[U-Boot] [PATCH 3/8] dm: core: remove unnecessary return condition in driver lookup

[Simon Glass]

On 17 November 2014 at 02:14, Simon Glass <sjg@chromium.org> wrote:
> On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>> 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 <yamada.m@jp.panasonic.com>
>
> Acked-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-dm.

[U-Boot] [PATCH 4/8] dm: core: remove unnecessary return condition in uclass lookup

[Simon Glass]

On 17 November 2014 at 10:16, Simon Glass <sjg@chromium.org> wrote:
> On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>> These 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.
>>
>> Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
>
> Yes we now use proper error codes to detect an invalid uclass, so it
> is good to clean this up.
>
> Acked-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-dm.

[U-Boot] [PATCH 8/8] dm: core: abolish u-boot, dm-pre-reloc property

[Simon Glass]

HI Masahiro,

On 21 November 2014 at 02:59, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> Hi Simon,
>
>
>
> On Thu, 20 Nov 2014 16:44:22 +0000
> Simon Glass <sjg@chromium.org> wrote:
>
>> Hi Masahiro,
>>
>> On 19 November 2014 09:21, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>> > Hi Simon,
>> >
>> >
>> >
>> > On Tue, 18 Nov 2014 14:37:33 +0000
>> > Simon Glass <sjg@chromium.org> wrote:
>> >
>> >> Hi Masahiro,
>> >>
>> >> On 18 November 2014 12:51, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>> >> > Hi Simon,
>> >> >
>> >> >
>> >> >
>> >> > On Mon, 17 Nov 2014 18:17:43 +0000
>> >> > Simon Glass <sjg@chromium.org> wrote:
>> >> >
>> >> >> Hi Masahiro,
>> >> >>
>> >> >> On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>> >> >> > 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_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.
>> >> >>
>> >> >> 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.
>> >> >
>> >>
>> >> 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?
>>
>> >
>> >
>> >
>> >> >
>> >> >
>> >> >> >
>> >> >> > 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?
>> >> >>
>> >> >
>> >> > 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.
>>
>> 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.
>>
>> >
>> > 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?
>>
>> 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.

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

[U-Boot] [PATCH 8/8] dm: core: abolish u-boot, dm-pre-reloc property

[Masahiro Yamada]

Hi Simon,



On Mon, 24 Nov 2014 15:29:23 -0700
Simon Glass <sjg@chromium.org> wrote:

> HI Masahiro,
> 
> On 21 November 2014 at 02:59, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> > Hi Simon,
> >
> >
> >
> > On Thu, 20 Nov 2014 16:44:22 +0000
> > Simon Glass <sjg@chromium.org> wrote:
> >
> >> Hi Masahiro,
> >>
> >> On 19 November 2014 09:21, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> >> > Hi Simon,
> >> >
> >> >
> >> >
> >> > On Tue, 18 Nov 2014 14:37:33 +0000
> >> > Simon Glass <sjg@chromium.org> wrote:
> >> >
> >> >> Hi Masahiro,
> >> >>
> >> >> On 18 November 2014 12:51, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> >> >> > Hi Simon,
> >> >> >
> >> >> >
> >> >> >
> >> >> > On Mon, 17 Nov 2014 18:17:43 +0000
> >> >> > Simon Glass <sjg@chromium.org> wrote:
> >> >> >
> >> >> >> Hi Masahiro,
> >> >> >>
> >> >> >> On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> >> >> >> > 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_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.
> >> >> >>
> >> >> >> 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.
> >> >> >
> >> >>
> >> >> 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?
> >>
> >> >
> >> >
> >> >
> >> >> >
> >> >> >
> >> >> >> >
> >> >> >> > 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?
> >> >> >>
> >> >> >
> >> >> > 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.
> >>
> >> 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.
> >>
> >> >
> >> > 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?
> >>
> >> 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.
> 
> 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.


> 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

[U-Boot] [PATCH 8/8] dm: core: abolish u-boot, dm-pre-reloc property

[Simon Glass]

Hi Masahiro,

On 24 November 2014 at 20:18, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> Hi Simon,
>
>
>
> On Mon, 24 Nov 2014 15:29:23 -0700
> Simon Glass <sjg@chromium.org> wrote:
>
>> HI Masahiro,
>>
>> On 21 November 2014 at 02:59, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>> > Hi Simon,
>> >
>> >
>> >
>> > On Thu, 20 Nov 2014 16:44:22 +0000
>> > Simon Glass <sjg@chromium.org> wrote:
>> >
>> >> Hi Masahiro,
>> >>
>> >> On 19 November 2014 09:21, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>> >> > Hi Simon,
>> >> >
>> >> >
>> >> >
>> >> > On Tue, 18 Nov 2014 14:37:33 +0000
>> >> > Simon Glass <sjg@chromium.org> wrote:
>> >> >
>> >> >> Hi Masahiro,
>> >> >>
>> >> >> On 18 November 2014 12:51, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>> >> >> > Hi Simon,
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >> > On Mon, 17 Nov 2014 18:17:43 +0000
>> >> >> > Simon Glass <sjg@chromium.org> wrote:
>> >> >> >
>> >> >> >> Hi Masahiro,
>> >> >> >>
>> >> >> >> On 17 November 2014 08:19, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>> >> >> >> > 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_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.
>> >> >> >>
>> >> >> >> 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.
>> >> >> >
>> >> >>
>> >> >> 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?
>> >>
>> >> >
>> >> >
>> >> >
>> >> >> >
>> >> >> >
>> >> >> >> >
>> >> >> >> > 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?
>> >> >> >>
>> >> >> >
>> >> >> > 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.
>> >>
>> >> 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.
>> >>
>> >> >
>> >> > 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?
>> >>
>> >> 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.
>>
>> 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.

Let's do nothing for now. I don't like that property and would to get
rid of it too.

>
>
>> 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