[PATCH 0/3] Support reading an u32 from a multi-value property

[PATCH 0/3] Support reading an u32 from a multi-value property

[Dario Binacchi]

The main part of the series consists of adding the dev_read_u32_index
and dev_read_u32_index_default functions. During implementation and
testing I noticed the lack of testing for the 64-bit data access
functions.
I decided to add the 64-bit test patch to the series to avoid merge
conflicts that could have occurred with the submission of a single
separated patch. In fact, it also contains changes to the
test/dm/test-fdt.c file.

If this series is accepted, it will be possible to evaluate the
addition of functions for indexed access also for the dev_read_s32 and
dev_read_u32u functions. Maybe to add in a second version of this
series.



Dario Binacchi (3):
  dm: test: add test case for dev_read_u64 function
  dm: core: support reading a single indexed u32 value
  dm: core: refactor functions reading an u32 from dt

 arch/sandbox/dts/test.dts |  2 ++
 drivers/core/of_access.c  | 38 ++++++++++++++++++------------
 drivers/core/ofnode.c     | 49 ++++++++++++++++++++++++++++-----------
 drivers/core/read.c       | 13 +++++++++++
 include/dm/of_access.h    | 19 +++++++++++++++
 include/dm/ofnode.h       | 25 ++++++++++++++++++++
 include/dm/read.h         | 40 ++++++++++++++++++++++++++++++++
 include/test/ut.h         | 16 +++++++++++++
 test/dm/test-fdt.c        | 39 +++++++++++++++++++++++++++++++
 9 files changed, 212 insertions(+), 29 deletions(-)

-- 
2.17.1

[PATCH 1/3] dm: test: add test case for dev_read_u64 function

[Dario Binacchi]

Add test case to cover dev_read_u64 and dev_read_u64_default functions.

Signed-off-by: Dario Binacchi <dariobin@libero.it>
---

 arch/sandbox/dts/test.dts |  1 +
 include/test/ut.h         | 16 ++++++++++++++++
 test/dm/test-fdt.c        | 10 ++++++++++
 3 files changed, 27 insertions(+)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 4a277934a7..6664adb385 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -93,6 +93,7 @@
 			<&gpio_b 9 0xc 3 2 1>;
 		int-value = <1234>;
 		uint-value = <(-1234)>;
+		int64-value = /bits/ 64 <0x1111222233334444>;
 		interrupts-extended = <&irq 3 0>;
 	};
 
diff --git a/include/test/ut.h b/include/test/ut.h
index 04df8ba3af..ab861588a8 100644
--- a/include/test/ut.h
+++ b/include/test/ut.h
@@ -104,6 +104,22 @@ int ut_check_console_dump(struct unit_test_state *uts, int total_bytes);
 	}								\
 }
 
+/* Assert that two 64 int expressions are equal */
+#define ut_asserteq_64(expr1, expr2) {					\
+	u64 _val1 = (expr1), _val2 = (expr2);				\
+									\
+	if (_val1 != _val2) {						\
+		ut_failf(uts, __FILE__, __LINE__, __func__,		\
+			 #expr1 " == " #expr2,				\
+			 "Expected %#llx (%lld), got %#llx (%lld)",	\
+			 (unsigned long long)_val1,			\
+			 (unsigned long long)_val1,			\
+			 (unsigned long long)_val2,			\
+			 (unsigned long long)_val2);			\
+		return CMD_RET_FAILURE;					\
+	}								\
+}
+
 /* Assert that two string expressions are equal */
 #define ut_asserteq_str(expr1, expr2) {					\
 	const char *_val1 = (expr1), *_val2 = (expr2);			\
diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
index 75ae08081c..50bff4fdfb 100644
--- a/test/dm/test-fdt.c
+++ b/test/dm/test-fdt.c
@@ -867,6 +867,7 @@ static int dm_test_read_int(struct unit_test_state *uts)
 	u32 val32;
 	s32 sval;
 	uint val;
+	u64 val64;
 
 	ut_assertok(uclass_first_device_err(UCLASS_TEST_FDT, &dev));
 	ut_asserteq_str("a-test", dev->name);
@@ -891,6 +892,15 @@ static int dm_test_read_int(struct unit_test_state *uts)
 	ut_assertok(dev_read_u32u(dev, "uint-value", &val));
 	ut_asserteq(-1234, val);
 
+	ut_assertok(dev_read_u64(dev, "int64-value", &val64));
+	ut_asserteq_64(0x1111222233334444, val64);
+
+	ut_asserteq_64(-EINVAL, dev_read_u64(dev, "missing", &val64));
+	ut_asserteq_64(6, dev_read_u64_default(dev, "missing", 6));
+
+	ut_asserteq_64(0x1111222233334444,
+		       dev_read_u64_default(dev, "int64-value", 6));
+
 	return 0;
 }
 DM_TEST(dm_test_read_int, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
-- 
2.17.1

[PATCH 2/3] dm: core: support reading a single indexed u32 value

[Dario Binacchi]

The patch adds helper functions to allow reading a single indexed u32
value from a device-tree property containing multiple u32 values, that
is an array of integers.

Signed-off-by: Dario Binacchi <dariobin@libero.it>
---

 arch/sandbox/dts/test.dts |  1 +
 drivers/core/of_access.c  | 22 +++++++++++++++++++++
 drivers/core/ofnode.c     | 40 +++++++++++++++++++++++++++++++++++++++
 drivers/core/read.c       | 13 +++++++++++++
 include/dm/of_access.h    | 19 +++++++++++++++++++
 include/dm/ofnode.h       | 25 ++++++++++++++++++++++++
 include/dm/read.h         | 40 +++++++++++++++++++++++++++++++++++++++
 test/dm/test-fdt.c        | 29 ++++++++++++++++++++++++++++
 8 files changed, 189 insertions(+)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 6664adb385..270eb3d87d 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -94,6 +94,7 @@
 		int-value = <1234>;
 		uint-value = <(-1234)>;
 		int64-value = /bits/ 64 <0x1111222233334444>;
+		int-array = <5678 9123 4567>;
 		interrupts-extended = <&irq 3 0>;
 	};
 
diff --git a/drivers/core/of_access.c b/drivers/core/of_access.c
index acd745c121..55e4a38fc5 100644
--- a/drivers/core/of_access.c
+++ b/drivers/core/of_access.c
@@ -485,6 +485,28 @@ int of_read_u32_array(const struct device_node *np, const char *propname,
 	return 0;
 }
 
+int of_read_u32_index(const struct device_node *np, const char *propname,
+		      int index, u32 *outp)
+{
+	const __be32 *val;
+
+	debug("%s: %s: ", __func__, propname);
+	if (!np)
+		return -EINVAL;
+
+	val = of_find_property_value_of_size(np, propname,
+					     sizeof(*outp) * (index + 1));
+	if (IS_ERR(val)) {
+		debug("(not found)\n");
+		return PTR_ERR(val);
+	}
+
+	*outp = be32_to_cpup(val + index);
+	debug("%#x (%d)\n", *outp, *outp);
+
+	return 0;
+}
+
 int of_read_u64(const struct device_node *np, const char *propname, u64 *outp)
 {
 	const __be64 *val;
diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
index 96a5dd20bd..5bc3b02996 100644
--- a/drivers/core/ofnode.c
+++ b/drivers/core/ofnode.c
@@ -48,6 +48,46 @@ u32 ofnode_read_u32_default(ofnode node, const char *propname, u32 def)
 	return def;
 }
 
+int ofnode_read_u32_index(ofnode node, const char *propname, int index,
+			  u32 *outp)
+{
+	const fdt32_t *cell;
+	int len;
+
+	assert(ofnode_valid(node));
+	debug("%s: %s: ", __func__, propname);
+
+	if (ofnode_is_np(node))
+		return of_read_u32_index(ofnode_to_np(node), propname, index,
+					 outp);
+
+	cell = fdt_getprop(gd->fdt_blob, ofnode_to_offset(node), propname,
+			   &len);
+	if (!cell) {
+		debug("(not found)\n");
+		return -EINVAL;
+	}
+
+	if (len < (sizeof(int) * (index + 1))) {
+		debug("(not large enough)\n");
+		return -EOVERFLOW;
+	}
+
+	*outp = fdt32_to_cpu(cell[index]);
+	debug("%#x (%d)\n", *outp, *outp);
+
+	return 0;
+}
+
+u32 ofnode_read_u32_index_default(ofnode node, const char *propname, int index,
+				  u32 def)
+{
+	assert(ofnode_valid(node));
+	ofnode_read_u32_index(node, propname, index, &def);
+
+	return def;
+}
+
 int ofnode_read_s32_default(ofnode node, const char *propname, s32 def)
 {
 	assert(ofnode_valid(node));
diff --git a/drivers/core/read.c b/drivers/core/read.c
index 1f999b1b31..ce78f09d28 100644
--- a/drivers/core/read.c
+++ b/drivers/core/read.c
@@ -22,6 +22,19 @@ int dev_read_u32_default(const struct udevice *dev, const char *propname,
 	return ofnode_read_u32_default(dev_ofnode(dev), propname, def);
 }
 
+int dev_read_u32_index(struct udevice *dev, const char *propname, int index,
+		       u32 *outp)
+{
+	return ofnode_read_u32_index(dev_ofnode(dev), propname, index, outp);
+}
+
+u32 dev_read_u32_index_default(struct udevice *dev, const char *propname,
+			       int index, u32 def)
+{
+	return ofnode_read_u32_index_default(dev_ofnode(dev), propname, index,
+					     def);
+}
+
 int dev_read_s32(const struct udevice *dev, const char *propname, s32 *outp)
 {
 	return ofnode_read_u32(dev_ofnode(dev), propname, (u32 *)outp);
diff --git a/include/dm/of_access.h b/include/dm/of_access.h
index 13fedb7cf5..92876b3ecb 100644
--- a/include/dm/of_access.h
+++ b/include/dm/of_access.h
@@ -234,6 +234,25 @@ struct device_node *of_find_node_by_phandle(phandle handle);
  */
 int of_read_u32(const struct device_node *np, const char *propname, u32 *outp);
 
+/**
+ * of_read_u32_index() - Find and read a 32-bit value from a multi-value
+ *                       property
+ *
+ * Search for a property in a device node and read a 32-bit value from
+ * it.
+ *
+ * @np:		device node from which the property value is to be read.
+ * @propname:	name of the property to be searched.
+ * @index:	index of the u32 in the list of values
+ * @outp:	pointer to return value, modified only if return value is 0.
+ *
+ * @return 0 on success, -EINVAL if the property does not exist,
+ * -ENODATA if property does not have a value, and -EOVERFLOW if the
+ * property data isn't large enough.
+ */
+int of_read_u32_index(const struct device_node *np, const char *propname,
+		      int index, u32 *outp);
+
 /**
  * of_read_u64() - Find and read a 64-bit integer from a property
  *
diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
index b5a50e8849..ce5e366c06 100644
--- a/include/dm/ofnode.h
+++ b/include/dm/ofnode.h
@@ -202,6 +202,18 @@ static inline ofnode ofnode_null(void)
  */
 int ofnode_read_u32(ofnode node, const char *propname, u32 *outp);
 
+/**
+ * ofnode_read_u32_index() - Read a 32-bit integer from a multi-value property
+ *
+ * @ref:	valid node reference to read property from
+ * @propname:	name of the property to read from
+ * @index:	index of the integer to return
+ * @outp:	place to put value (if found)
+ * @return 0 if OK, -ve on error
+ */
+int ofnode_read_u32_index(ofnode node, const char *propname, int index,
+			  u32 *outp);
+
 /**
  * ofnode_read_s32() - Read a 32-bit integer from a property
  *
@@ -226,6 +238,19 @@ static inline int ofnode_read_s32(ofnode node, const char *propname,
  */
 u32 ofnode_read_u32_default(ofnode ref, const char *propname, u32 def);
 
+/**
+ * ofnode_read_u32_index_default() - Read a 32-bit integer from a multi-value
+ *                                   property
+ *
+ * @ref:	valid node reference to read property from
+ * @propname:	name of the property to read from
+ * @index:	index of the integer to return
+ * @def:	default value to return if the property has no value
+ * @return property value, or @def if not found
+ */
+u32 ofnode_read_u32_index_default(ofnode ref, const char *propname, int index,
+				  u32 def);
+
 /**
  * ofnode_read_s32_default() - Read a 32-bit integer from a property
  *
diff --git a/include/dm/read.h b/include/dm/read.h
index da8c7f25e7..77d3bc8db5 100644
--- a/include/dm/read.h
+++ b/include/dm/read.h
@@ -66,6 +66,32 @@ int dev_read_u32(const struct udevice *dev, const char *propname, u32 *outp);
 int dev_read_u32_default(const struct udevice *dev, const char *propname,
 			 int def);
 
+/**
+ * dev_read_u32_index() - read an indexed 32-bit integer from a device's DT
+ *                        property
+ *
+ * @dev:	device to read DT property from
+ * @propname:	name of the property to read from
+ * @index:	index of the integer to return
+ * @outp:	place to put value (if found)
+ * @return 0 if OK, -ve on error
+ */
+int dev_read_u32_index(struct udevice *dev, const char *propname, int index,
+		       u32 *outp);
+
+/**
+ * dev_read_u32_index_default() - read an indexed 32-bit integer from a device's
+ *                                DT property
+ *
+ * @dev:	device to read DT property from
+ * @propname:	name of the property to read from
+ * @index:	index of the integer to return
+ * @def:	default value to return if the property has no value
+ * @return property value, or @def if not found
+ */
+u32 dev_read_u32_index_default(struct udevice *dev, const char *propname,
+			       int index, u32 def);
+
 /**
  * dev_read_s32() - read a signed 32-bit integer from a device's DT property
  *
@@ -621,6 +647,20 @@ static inline int dev_read_u32_default(const struct udevice *dev,
 	return ofnode_read_u32_default(dev_ofnode(dev), propname, def);
 }
 
+static inline int dev_read_u32_index(struct udevice *dev,
+				     const char *propname, int index, u32 *outp)
+{
+	return ofnode_read_u32_index(dev_ofnode(dev), propname, index, outp);
+}
+
+static inline u32 dev_read_u32_index_default(struct udevice *dev,
+					     const char *propname, int index,
+					     u32 def)
+{
+	return ofnode_read_u32_index_default(dev_ofnode(dev), propname, index,
+					     def);
+}
+
 static inline int dev_read_s32(const struct udevice *dev,
 			       const char *propname, s32 *outp)
 {
diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
index 50bff4fdfb..5fa1238d94 100644
--- a/test/dm/test-fdt.c
+++ b/test/dm/test-fdt.c
@@ -905,6 +905,35 @@ static int dm_test_read_int(struct unit_test_state *uts)
 }
 DM_TEST(dm_test_read_int, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
 
+static int dm_test_read_int_index(struct unit_test_state *uts)
+{
+	struct udevice *dev;
+	u32 val32;
+
+	ut_assertok(uclass_first_device_err(UCLASS_TEST_FDT, &dev));
+	ut_asserteq_str("a-test", dev->name);
+
+	ut_asserteq(-EINVAL, dev_read_u32_index(dev, "missing", 0, &val32));
+	ut_asserteq(19, dev_read_u32_index_default(dev, "missing", 0, 19));
+
+	ut_assertok(dev_read_u32_index(dev, "int-array", 0, &val32));
+	ut_asserteq(5678, val32);
+	ut_assertok(dev_read_u32_index(dev, "int-array", 1, &val32));
+	ut_asserteq(9123, val32);
+	ut_assertok(dev_read_u32_index(dev, "int-array", 2, &val32));
+	ut_asserteq(4567, val32);
+	ut_asserteq(-EOVERFLOW, dev_read_u32_index(dev, "int-array", 3,
+						   &val32));
+
+	ut_asserteq(5678, dev_read_u32_index_default(dev, "int-array", 0, 2));
+	ut_asserteq(9123, dev_read_u32_index_default(dev, "int-array", 1, 2));
+	ut_asserteq(4567, dev_read_u32_index_default(dev, "int-array", 2, 2));
+	ut_asserteq(2, dev_read_u32_index_default(dev, "int-array", 3, 2));
+
+	return 0;
+}
+DM_TEST(dm_test_read_int_index, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
+
 /* Test iteration through devices by drvdata */
 static int dm_test_uclass_drvdata(struct unit_test_state *uts)
 {
-- 
2.17.1

[PATCH 3/3] dm: core: refactor functions reading an u32 from dt

[Dario Binacchi]

Now reading a 32 bit value from a device-tree property can be expressed
as reading the first element of an array with a single value.

Signed-off-by: Dario Binacchi <dariobin@libero.it>

---

 drivers/core/of_access.c | 16 +---------------
 drivers/core/ofnode.c    | 23 ++---------------------
 2 files changed, 3 insertions(+), 36 deletions(-)

diff --git a/drivers/core/of_access.c b/drivers/core/of_access.c
index 55e4a38fc5..d116d106d9 100644
--- a/drivers/core/of_access.c
+++ b/drivers/core/of_access.c
@@ -449,21 +449,7 @@ static void *of_find_property_value_of_size(const struct device_node *np,
 
 int of_read_u32(const struct device_node *np, const char *propname, u32 *outp)
 {
-	const __be32 *val;
-
-	debug("%s: %s: ", __func__, propname);
-	if (!np)
-		return -EINVAL;
-	val = of_find_property_value_of_size(np, propname, sizeof(*outp));
-	if (IS_ERR(val)) {
-		debug("(not found)\n");
-		return PTR_ERR(val);
-	}
-
-	*outp = be32_to_cpup(val);
-	debug("%#x (%d)\n", *outp, *outp);
-
-	return 0;
+	return of_read_u32_index(np, propname, 0, outp);
 }
 
 int of_read_u32_array(const struct device_node *np, const char *propname,
diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
index 5bc3b02996..b0be7cbe19 100644
--- a/drivers/core/ofnode.c
+++ b/drivers/core/ofnode.c
@@ -18,32 +18,13 @@
 
 int ofnode_read_u32(ofnode node, const char *propname, u32 *outp)
 {
-	assert(ofnode_valid(node));
-	debug("%s: %s: ", __func__, propname);
-
-	if (ofnode_is_np(node)) {
-		return of_read_u32(ofnode_to_np(node), propname, outp);
-	} else {
-		const fdt32_t *cell;
-		int len;
-
-		cell = fdt_getprop(gd->fdt_blob, ofnode_to_offset(node),
-				   propname, &len);
-		if (!cell || len < sizeof(int)) {
-			debug("(not found)\n");
-			return -EINVAL;
-		}
-		*outp = fdt32_to_cpu(cell[0]);
-	}
-	debug("%#x (%d)\n", *outp, *outp);
-
-	return 0;
+	return ofnode_read_u32_index(node, propname, 0, outp);
 }
 
 u32 ofnode_read_u32_default(ofnode node, const char *propname, u32 def)
 {
 	assert(ofnode_valid(node));
-	ofnode_read_u32(node, propname, &def);
+	ofnode_read_u32_index(node, propname, 0, &def);
 
 	return def;
 }
-- 
2.17.1

[PATCH 1/3] dm: test: add test case for dev_read_u64 function

[Simon Glass]

On Sun, 29 Mar 2020 at 10:05, Dario Binacchi <dariobin@libero.it> wrote:
>
> Add test case to cover dev_read_u64 and dev_read_u64_default functions.
>
> Signed-off-by: Dario Binacchi <dariobin@libero.it>
> ---
>
>  arch/sandbox/dts/test.dts |  1 +
>  include/test/ut.h         | 16 ++++++++++++++++
>  test/dm/test-fdt.c        | 10 ++++++++++
>  3 files changed, 27 insertions(+)

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

[PATCH 2/3] dm: core: support reading a single indexed u32 value

[Simon Glass]

On Sun, 29 Mar 2020 at 10:05, Dario Binacchi <dariobin@libero.it> wrote:
>
> The patch adds helper functions to allow reading a single indexed u32
> value from a device-tree property containing multiple u32 values, that
> is an array of integers.
>
> Signed-off-by: Dario Binacchi <dariobin@libero.it>
> ---
>
>  arch/sandbox/dts/test.dts |  1 +
>  drivers/core/of_access.c  | 22 +++++++++++++++++++++
>  drivers/core/ofnode.c     | 40 +++++++++++++++++++++++++++++++++++++++
>  drivers/core/read.c       | 13 +++++++++++++
>  include/dm/of_access.h    | 19 +++++++++++++++++++
>  include/dm/ofnode.h       | 25 ++++++++++++++++++++++++
>  include/dm/read.h         | 40 +++++++++++++++++++++++++++++++++++++++
>  test/dm/test-fdt.c        | 29 ++++++++++++++++++++++++++++
>  8 files changed, 189 insertions(+)
>

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

Very nice

[PATCH 3/3] dm: core: refactor functions reading an u32 from dt

[Simon Glass]

On Sun, 29 Mar 2020 at 10:05, Dario Binacchi <dariobin@libero.it> wrote:
>
> Now reading a 32 bit value from a device-tree property can be expressed
> as reading the first element of an array with a single value.
>
> Signed-off-by: Dario Binacchi <dariobin@libero.it>
>
> ---
>
>  drivers/core/of_access.c | 16 +---------------
>  drivers/core/ofnode.c    | 23 ++---------------------
>  2 files changed, 3 insertions(+), 36 deletions(-)

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

Can you please check the code-size delta in SPL on a suitable board?

[PATCH 3/3] dm: core: refactor functions reading an u32 from dt

[dariobin at libero.it]

> Il 31 marzo 2020 alle 1.57 Simon Glass <sjg@chromium.org> ha scritto:
> 
> 
> On Sun, 29 Mar 2020 at 10:05, Dario Binacchi <dariobin@libero.it> wrote:
> >
> > Now reading a 32 bit value from a device-tree property can be expressed
> > as reading the first element of an array with a single value.
> >
> > Signed-off-by: Dario Binacchi <dariobin@libero.it>
> >
> > ---
> >
> >  drivers/core/of_access.c | 16 +---------------
> >  drivers/core/ofnode.c    | 23 ++---------------------
> >  2 files changed, 3 insertions(+), 36 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Can you please check the code-size delta in SPL on a suitable board?

I have a black beaglebone available (am335x_evm_defconfig).

u-boot-spl.map generated without applying the refactoring patch
....
 .text.ofnode_read_u32
                0x0000000000000000       0x2e drivers/built-in.o
 .text.ofnode_read_u32_index
                0x0000000000000000       0x38 drivers/built-in.o
 .text.ofnode_read_u32_default
                0x0000000000000000       0x12 drivers/built-in.o

u-boot-spl.map genarated with the refactoring patch applied
....
 .text.ofnode_read_u32_index
                0x0000000000000000       0x38 drivers/built-in.o
 .text.ofnode_read_u32
                0x0000000000000000        0x8 drivers/built-in.o
 .text.ofnode_read_u32_default
                0x0000000000000000       0x14 drivers/built-in.o

I hope I have correctly answered what you asked me. Otherwise I am available to perform the right checks on your indications.

Thanks and regards,
Dario Binacchi

[PATCH 2/3] dm: core: support reading a single indexed u32 value

[dariobin at libero.it]

Do you think it would make sense to add indexed access also for the s32, u32u and u64 types or at least some of those ?

Thanks and regards
Dario Binacchi

> Il 31 marzo 2020 alle 1.57 Simon Glass <sjg@chromium.org> ha scritto:
> 
> 
> On Sun, 29 Mar 2020 at 10:05, Dario Binacchi <dariobin@libero.it> wrote:
> >
> > The patch adds helper functions to allow reading a single indexed u32
> > value from a device-tree property containing multiple u32 values, that
> > is an array of integers.
> >
> > Signed-off-by: Dario Binacchi <dariobin@libero.it>
> > ---
> >
> >  arch/sandbox/dts/test.dts |  1 +
> >  drivers/core/of_access.c  | 22 +++++++++++++++++++++
> >  drivers/core/ofnode.c     | 40 +++++++++++++++++++++++++++++++++++++++
> >  drivers/core/read.c       | 13 +++++++++++++
> >  include/dm/of_access.h    | 19 +++++++++++++++++++
> >  include/dm/ofnode.h       | 25 ++++++++++++++++++++++++
> >  include/dm/read.h         | 40 +++++++++++++++++++++++++++++++++++++++
> >  test/dm/test-fdt.c        | 29 ++++++++++++++++++++++++++++
> >  8 files changed, 189 insertions(+)
> >
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Very nice

[PATCH 3/3] dm: core: refactor functions reading an u32 from dt

[Simon Glass]

Hi Dario,

On Wed, 1 Apr 2020 at 13:34, <dariobin@libero.it> wrote:
>
>
> > Il 31 marzo 2020 alle 1.57 Simon Glass <sjg@chromium.org> ha scritto:
> >
> >
> > On Sun, 29 Mar 2020 at 10:05, Dario Binacchi <dariobin@libero.it> wrote:
> > >
> > > Now reading a 32 bit value from a device-tree property can be expressed
> > > as reading the first element of an array with a single value.
> > >
> > > Signed-off-by: Dario Binacchi <dariobin@libero.it>
> > >
> > > ---
> > >
> > >  drivers/core/of_access.c | 16 +---------------
> > >  drivers/core/ofnode.c    | 23 ++---------------------
> > >  2 files changed, 3 insertions(+), 36 deletions(-)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > Can you please check the code-size delta in SPL on a suitable board?
>
> I have a black beaglebone available (am335x_evm_defconfig).
>
> u-boot-spl.map generated without applying the refactoring patch
> ....
>  .text.ofnode_read_u32
>                 0x0000000000000000       0x2e drivers/built-in.o
>  .text.ofnode_read_u32_index
>                 0x0000000000000000       0x38 drivers/built-in.o
>  .text.ofnode_read_u32_default
>                 0x0000000000000000       0x12 drivers/built-in.o
>
> u-boot-spl.map genarated with the refactoring patch applied
> ....
>  .text.ofnode_read_u32_index
>                 0x0000000000000000       0x38 drivers/built-in.o
>  .text.ofnode_read_u32
>                 0x0000000000000000        0x8 drivers/built-in.o
>  .text.ofnode_read_u32_default
>                 0x0000000000000000       0x14 drivers/built-in.o

Possibly, but a better test is to build your branch with the patch in:

buildman -b <branch> <board>

Then check the size:

buildman -b <branch> -sS

or function detail:

buildman -b <branch> -sSB

That will give us a true picture for SPL. It will show incremental
size increase with your patch.

See buildman docs for more info.

Regards,
Simon

[PATCH 2/3] dm: core: support reading a single indexed u32 value

[Simon Glass]

Hi Dario,

On Wed, 1 Apr 2020 at 13:43, <dariobin@libero.it> wrote:
>
> Do you think it would make sense to add indexed access also for the s32, u32u and u64 types or at least some of those ?
>

I really don't know. I suspect it would be used at some point.

Regards,
Simon

> Thanks and regards
> Dario Binacchi
>
> > Il 31 marzo 2020 alle 1.57 Simon Glass <sjg@chromium.org> ha scritto:
> >
> >
> > On Sun, 29 Mar 2020 at 10:05, Dario Binacchi <dariobin@libero.it> wrote:
> > >
> > > The patch adds helper functions to allow reading a single indexed u32
> > > value from a device-tree property containing multiple u32 values, that
> > > is an array of integers.
> > >
> > > Signed-off-by: Dario Binacchi <dariobin@libero.it>
> > > ---
> > >
> > >  arch/sandbox/dts/test.dts |  1 +
> > >  drivers/core/of_access.c  | 22 +++++++++++++++++++++
> > >  drivers/core/ofnode.c     | 40 +++++++++++++++++++++++++++++++++++++++
> > >  drivers/core/read.c       | 13 +++++++++++++
> > >  include/dm/of_access.h    | 19 +++++++++++++++++++
> > >  include/dm/ofnode.h       | 25 ++++++++++++++++++++++++
> > >  include/dm/read.h         | 40 +++++++++++++++++++++++++++++++++++++++
> > >  test/dm/test-fdt.c        | 29 ++++++++++++++++++++++++++++
> > >  8 files changed, 189 insertions(+)
> > >
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > Very nice

[PATCH 3/3] dm: core: refactor functions reading an u32 from dt

[dariobin at libero.it]

> Il 2 aprile 2020 alle 20.54 Simon Glass <sjg@chromium.org> ha scritto:
> 
> 
> Hi Dario,
> 
> On Wed, 1 Apr 2020 at 13:34, <dariobin@libero.it> wrote:
> >
> >
> > > Il 31 marzo 2020 alle 1.57 Simon Glass <sjg@chromium.org> ha scritto:
> > >
> > >
> > > On Sun, 29 Mar 2020 at 10:05, Dario Binacchi <dariobin@libero.it> wrote:
> > > >
> > > > Now reading a 32 bit value from a device-tree property can be expressed
> > > > as reading the first element of an array with a single value.
> > > >
> > > > Signed-off-by: Dario Binacchi <dariobin@libero.it>
> > > >
> > > > ---
> > > >
> > > >  drivers/core/of_access.c | 16 +---------------
> > > >  drivers/core/ofnode.c    | 23 ++---------------------
> > > >  2 files changed, 3 insertions(+), 36 deletions(-)
> > >
> > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > >
> > > Can you please check the code-size delta in SPL on a suitable board?
> >
> > I have a black beaglebone available (am335x_evm_defconfig).
> >
> > u-boot-spl.map generated without applying the refactoring patch
> > ....
> >  .text.ofnode_read_u32
> >                 0x0000000000000000       0x2e drivers/built-in.o
> >  .text.ofnode_read_u32_index
> >                 0x0000000000000000       0x38 drivers/built-in.o
> >  .text.ofnode_read_u32_default
> >                 0x0000000000000000       0x12 drivers/built-in.o
> >
> > u-boot-spl.map genarated with the refactoring patch applied
> > ....
> >  .text.ofnode_read_u32_index
> >                 0x0000000000000000       0x38 drivers/built-in.o
> >  .text.ofnode_read_u32
> >                 0x0000000000000000        0x8 drivers/built-in.o
> >  .text.ofnode_read_u32_default
> >                 0x0000000000000000       0x14 drivers/built-in.o
> 
> Possibly, but a better test is to build your branch with the patch in:
> 
> buildman -b <branch> <board>
> 
> Then check the size:
> 
> buildman -b <branch> -sS
> 
> or function detail:
> 
> buildman -b <branch> -sSB
> 
> That will give us a true picture for SPL. It will show incremental
> size increase with your patch.

Hi Simon, 
this is the buildman response:
...
03: dm: core: support reading a single indexed u32 value
04: dm: core: refactor functions reading an u32 from dt
       arm: (for 4/708 boards) all +11.5 bss -10.0 text +21.5
            am335x_hs_evm  : all +24 text +24
               u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26)
                 function                                   old     new   delta
                 ofnode_read_u32_index                        -      62     +62
                 ofnode_read_u32_default                     18      20      +2
                 ofnode_read_u32                             46       8     -38
            am335x_hs_evm_uart: all +24 text +24
               u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26)
                 function                                   old     new   delta
                 ofnode_read_u32_index                        -      62     +62
                 ofnode_read_u32_default                     18      20      +2
                 ofnode_read_u32                             46       8     -38
            am335x_evm     : bss -24 text +24
               u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26)
                 function                                   old     new   delta
                 ofnode_read_u32_index                        -      62     +62
                 ofnode_read_u32_default                     18      20      +2
                 ofnode_read_u32                             46       8     -38
            am335x_boneblack_vboot: all -2 bss -16 text +14
               u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26)
                 function                                   old     new   delta
                 ofnode_read_u32_index                        -      62     +62
                 ofnode_read_u32_default                     18      20      +2
                 ofnode_read_u32                             46       8     -38

Regards,
Dario
> 
> See buildman docs for more info.
> 
> Regards,
> Simon

[PATCH 3/3] dm: core: refactor functions reading an u32 from dt

[Simon Glass]

Hi Dario,

On Sat, 4 Apr 2020 at 06:49, <dariobin@libero.it> wrote:
>
> > Il 2 aprile 2020 alle 20.54 Simon Glass <sjg@chromium.org> ha scritto:
> >
> >
> > Hi Dario,
> >
> > On Wed, 1 Apr 2020 at 13:34, <dariobin@libero.it> wrote:
> > >
> > >
> > > > Il 31 marzo 2020 alle 1.57 Simon Glass <sjg@chromium.org> ha scritto:
> > > >
> > > >
> > > > On Sun, 29 Mar 2020 at 10:05, Dario Binacchi <dariobin@libero.it> wrote:
> > > > >
> > > > > Now reading a 32 bit value from a device-tree property can be expressed
> > > > > as reading the first element of an array with a single value.
> > > > >
> > > > > Signed-off-by: Dario Binacchi <dariobin@libero.it>
> > > > >
> > > > > ---
> > > > >
> > > > >  drivers/core/of_access.c | 16 +---------------
> > > > >  drivers/core/ofnode.c    | 23 ++---------------------
> > > > >  2 files changed, 3 insertions(+), 36 deletions(-)
> > > >
> > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > >
> > > > Can you please check the code-size delta in SPL on a suitable board?
> > >
> > > I have a black beaglebone available (am335x_evm_defconfig).
> > >
> > > u-boot-spl.map generated without applying the refactoring patch
> > > ....
> > >  .text.ofnode_read_u32
> > >                 0x0000000000000000       0x2e drivers/built-in.o
> > >  .text.ofnode_read_u32_index
> > >                 0x0000000000000000       0x38 drivers/built-in.o
> > >  .text.ofnode_read_u32_default
> > >                 0x0000000000000000       0x12 drivers/built-in.o
> > >
> > > u-boot-spl.map genarated with the refactoring patch applied
> > > ....
> > >  .text.ofnode_read_u32_index
> > >                 0x0000000000000000       0x38 drivers/built-in.o
> > >  .text.ofnode_read_u32
> > >                 0x0000000000000000        0x8 drivers/built-in.o
> > >  .text.ofnode_read_u32_default
> > >                 0x0000000000000000       0x14 drivers/built-in.o
> >
> > Possibly, but a better test is to build your branch with the patch in:
> >
> > buildman -b <branch> <board>
> >
> > Then check the size:
> >
> > buildman -b <branch> -sS
> >
> > or function detail:
> >
> > buildman -b <branch> -sSB
> >
> > That will give us a true picture for SPL. It will show incremental
> > size increase with your patch.
>
> Hi Simon,
> this is the buildman response:
> ...
> 03: dm: core: support reading a single indexed u32 value
> 04: dm: core: refactor functions reading an u32 from dt
>        arm: (for 4/708 boards) all +11.5 bss -10.0 text +21.5
>             am335x_hs_evm  : all +24 text +24
>                u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26)
>                  function                                   old     new   delta
>                  ofnode_read_u32_index                        -      62     +62
>                  ofnode_read_u32_default                     18      20      +2
>                  ofnode_read_u32                             46       8     -38
>             am335x_hs_evm_uart: all +24 text +24
>                u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26)
>                  function                                   old     new   delta
>                  ofnode_read_u32_index                        -      62     +62
>                  ofnode_read_u32_default                     18      20      +2
>                  ofnode_read_u32                             46       8     -38
>             am335x_evm     : bss -24 text +24
>                u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26)
>                  function                                   old     new   delta
>                  ofnode_read_u32_index                        -      62     +62
>                  ofnode_read_u32_default                     18      20      +2
>                  ofnode_read_u32                             46       8     -38
>             am335x_boneblack_vboot: all -2 bss -16 text +14
>                u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26)
>                  function                                   old     new   delta
>                  ofnode_read_u32_index                        -      62     +62
>                  ofnode_read_u32_default                     18      20      +2
>                  ofnode_read_u32                             46       8     -38

This does not actually look like SPL though, only U-Boot. I hope that
means that SPL doesn't increase in size.

Anyway for U-Boot proper it looks like it adds about 24 bytes of code.
I think it is worth it.

Regards,
Simon

[PATCH 3/3] dm: core: refactor functions reading an u32 from dt

[sjg at google.com]

Hi Dario,

On Sat, 4 Apr 2020 at 06:49, <dariobin@libero.it> wrote:
>
> > Il 2 aprile 2020 alle 20.54 Simon Glass <sjg@chromium.org> ha scritto:
> >
> >
> > Hi Dario,
> >
> > On Wed, 1 Apr 2020 at 13:34, <dariobin@libero.it> wrote:
> > >
> > >
> > > > Il 31 marzo 2020 alle 1.57 Simon Glass <sjg@chromium.org> ha scritto:
> > > >
> > > >
> > > > On Sun, 29 Mar 2020 at 10:05, Dario Binacchi <dariobin@libero.it> wrote:
> > > > >
> > > > > Now reading a 32 bit value from a device-tree property can be expressed
> > > > > as reading the first element of an array with a single value.
> > > > >
> > > > > Signed-off-by: Dario Binacchi <dariobin@libero.it>
> > > > >
> > > > > ---
> > > > >
> > > > >  drivers/core/of_access.c | 16 +---------------
> > > > >  drivers/core/ofnode.c    | 23 ++---------------------
> > > > >  2 files changed, 3 insertions(+), 36 deletions(-)
> > > >
> > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > >
> > > > Can you please check the code-size delta in SPL on a suitable board?
> > >
> > > I have a black beaglebone available (am335x_evm_defconfig).
> > >
> > > u-boot-spl.map generated without applying the refactoring patch
> > > ....
> > >  .text.ofnode_read_u32
> > >                 0x0000000000000000       0x2e drivers/built-in.o
> > >  .text.ofnode_read_u32_index
> > >                 0x0000000000000000       0x38 drivers/built-in.o
> > >  .text.ofnode_read_u32_default
> > >                 0x0000000000000000       0x12 drivers/built-in.o
> > >
> > > u-boot-spl.map genarated with the refactoring patch applied
> > > ....
> > >  .text.ofnode_read_u32_index
> > >                 0x0000000000000000       0x38 drivers/built-in.o
> > >  .text.ofnode_read_u32
> > >                 0x0000000000000000        0x8 drivers/built-in.o
> > >  .text.ofnode_read_u32_default
> > >                 0x0000000000000000       0x14 drivers/built-in.o
> >
> > Possibly, but a better test is to build your branch with the patch in:
> >
> > buildman -b <branch> <board>
> >
> > Then check the size:
> >
> > buildman -b <branch> -sS
> >
> > or function detail:
> >
> > buildman -b <branch> -sSB
> >
> > That will give us a true picture for SPL. It will show incremental
> > size increase with your patch.
>
> Hi Simon,
> this is the buildman response:
> ...
> 03: dm: core: support reading a single indexed u32 value
> 04: dm: core: refactor functions reading an u32 from dt
>        arm: (for 4/708 boards) all +11.5 bss -10.0 text +21.5
>             am335x_hs_evm  : all +24 text +24
>                u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26)
>                  function                                   old     new   delta
>                  ofnode_read_u32_index                        -      62     +62
>                  ofnode_read_u32_default                     18      20      +2
>                  ofnode_read_u32                             46       8     -38
>             am335x_hs_evm_uart: all +24 text +24
>                u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26)
>                  function                                   old     new   delta
>                  ofnode_read_u32_index                        -      62     +62
>                  ofnode_read_u32_default                     18      20      +2
>                  ofnode_read_u32                             46       8     -38
>             am335x_evm     : bss -24 text +24
>                u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26)
>                  function                                   old     new   delta
>                  ofnode_read_u32_index                        -      62     +62
>                  ofnode_read_u32_default                     18      20      +2
>                  ofnode_read_u32                             46       8     -38
>             am335x_boneblack_vboot: all -2 bss -16 text +14
>                u-boot: add: 1/0, grow: 1/-1 bytes: 64/-38 (26)
>                  function                                   old     new   delta
>                  ofnode_read_u32_index                        -      62     +62
>                  ofnode_read_u32_default                     18      20      +2
>                  ofnode_read_u32                             46       8     -38

This does not actually look like SPL though, only U-Boot. I hope that
means that SPL doesn't increase in size.

Anyway for U-Boot proper it looks like it adds about 24 bytes of code.
I think it is worth it.

Regards,
Simon

Applied to u-boot-dm/next, thanks!

[PATCH 1/3] dm: test: add test case for dev_read_u64 function

[sjg at google.com]

On Sun, 29 Mar 2020 at 10:05, Dario Binacchi <dariobin@libero.it> wrote:
>
> Add test case to cover dev_read_u64 and dev_read_u64_default functions.
>
> Signed-off-by: Dario Binacchi <dariobin@libero.it>
> ---
>
>  arch/sandbox/dts/test.dts |  1 +
>  include/test/ut.h         | 16 ++++++++++++++++
>  test/dm/test-fdt.c        | 10 ++++++++++
>  3 files changed, 27 insertions(+)

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

Applied to u-boot-dm/next, thanks!