[PATCH] libfdt: add helpers to read address and size from reg

[PATCH] libfdt: add helpers to read address and size from reg

[Benjamin Fair]

This patch extends the capability of libfdt to parse the contents of device
trees in a similar manner to fdt_address_cells and fdt_size_cells.

It adds a helper function which reads the address and size of a device from
the reg property and performs basic sanity checks.

It does not perform translation to a physical address using the ranges
properties of parents, but this enhancement may be added as a separate
function in the future.

Signed-off-by: Benjamin Fair <b-fair-l0cyMroinI0@public.gmane.org>
---

The intent of this patch is similar to the commit "libfdt: Add helpers to read
#address-cells and #size-cells" [1].

It is related to "libfdt: add address translation support" [2] but does not
attempt to perform address translation and was written from scratch rather than
reusing GPL code. If the issues with that patch are resolved, that
functionality will complement what is added in this patch.

[1] http://www.spinics.net/lists/devicetree-compiler/msg00113.html
[2] http://www.spinics.net/lists/devicetree-compiler/msg00093.html

 libfdt/fdt_addresses.c | 62 ++++++++++++++++++++++++++++++++++++++++++
 libfdt/libfdt.h        | 29 ++++++++++++++++++++
 libfdt/version.lds     |  1 +
 tests/.gitignore       |  1 +
 tests/Makefile.tests   |  2 +-
 tests/addr_size.c      | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/addresses.dts    | 11 ++++++++
 tests/run_tests.sh     |  1 +
 8 files changed, 180 insertions(+), 1 deletion(-)
 create mode 100644 tests/addr_size.c

diff --git a/libfdt/fdt_addresses.c b/libfdt/fdt_addresses.c
index eff4dbc..92cbed9 100644
--- a/libfdt/fdt_addresses.c
+++ b/libfdt/fdt_addresses.c
@@ -55,6 +55,9 @@
 
 #include "libfdt_internal.h"
 
+#define BYTES_PER_CELL 4
+#define BITS_PER_CELL 32
+
 int fdt_address_cells(const void *fdt, int nodeoffset)
 {
 	const fdt32_t *ac;
@@ -94,3 +97,62 @@ int fdt_size_cells(const void *fdt, int nodeoffset)
 
 	return val;
 }
+
+static uint64_t _fdt_read_cells(const fdt32_t *cells, int n)
+{
+	int i;
+	uint64_t res;
+
+	/* TODO: Support values larger than 2 cells */
+	if (n > 2)
+		return -FDT_ERR_BADNCELLS;
+
+	res = 0;
+	for (i = 0; i < n; i++) {
+		res <<= BITS_PER_CELL;
+		res |= fdt32_to_cpu(cells[i]);
+	}
+
+	return res;
+}
+
+int fdt_simple_addr_size(const void *fdt, int nodeoffset, int idx,
+			 uintptr_t *addr, size_t *size)
+{
+	int parent;
+	int ac, sc, reg_stride;
+	int res;
+	const fdt32_t *reg;
+
+	reg = fdt_getprop(fdt, nodeoffset, "reg", &res);
+	if (res < 0)
+		return res;
+
+	parent = fdt_parent_offset(fdt, nodeoffset);
+	if (parent < 0)
+		return res;
+
+	ac = fdt_address_cells(fdt, parent);
+	if (ac < 0)
+		return ac;
+
+	sc = fdt_size_cells(fdt, parent);
+	if (sc < 0)
+		return sc;
+
+	reg_stride = ac + sc;
+	/*
+	 * res is the number of bytes read and must be an even multiple of the
+	 * sum of ac and sc.
+	 */
+	if ((res % (reg_stride * BYTES_PER_CELL)) != 0)
+		return -FDT_ERR_BADVALUE;
+
+	if (addr)
+		*addr = (uintptr_t) _fdt_read_cells(&reg[reg_stride * idx], ac);
+	if (size)
+		*size = (size_t) _fdt_read_cells(&reg[ac + reg_stride * idx],
+						 sc);
+
+	return 0;
+}
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index c69e918..1a184a3 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -1085,6 +1085,35 @@ int fdt_address_cells(const void *fdt, int nodeoffset);
  */
 int fdt_size_cells(const void *fdt, int nodeoffset);
 
+/**
+ *
+ * fdt_simple_addr_size - read address and/or size from the reg property of a
+ *                        device node.
+ * @fdt: pointer to the device tree blob
+ * @nodeoffset: offset of the node to find the address and/or size from
+ * @idx: which address/size pair to read
+ * @addrp: pointer to where address will be stored (will be overwritten) or NULL
+ * @sizep: pointer to where size will be stored (will be overwritten) or NULL
+ *
+ * When the node has a valid reg property, returns the address and/or size
+ * values stored there. It does not perform any type of translation based on
+ * the parent bus(es).
+ *
+ * returns:
+ *	0, on success
+ *	-FDT_ERR_BADVALUE, if there is an unexpected number of entries in the
+ *		reg property
+ *	-FDT_ERR_NOTFOUND, if the node does not have a reg property
+ *	-FDT_ERR_BADNCELLS, if the number of address or size cells is invalid
+ *		or greater than 2 (which is the maximum currently supported)
+ *	-FDT_ERR_BADMAGIC,
+ *	-FDT_ERR_BADSTATE,
+ *	-FDT_ERR_BADSTRUCTURE,
+ *	-FDT_ERR_BADVERSION,
+ *	-FDT_ERR_TRUNCATED, standard meanings
+ */
+int fdt_simple_addr_size(const void *fdt, int nodeoffset, int idx,
+			 uintptr_t *addrp, size_t *sizep);
 
 /**********************************************************************/
 /* Write-in-place functions                                           */
diff --git a/libfdt/version.lds b/libfdt/version.lds
index cff0358..075ffab 100644
--- a/libfdt/version.lds
+++ b/libfdt/version.lds
@@ -59,6 +59,7 @@ LIBFDT_1.2 {
 		fdt_next_subnode;
 		fdt_address_cells;
 		fdt_size_cells;
+		fdt_simple_addr_size;
 		fdt_stringlist_contains;
 		fdt_resize;
 		fdt_overlay_apply;
diff --git a/tests/.gitignore b/tests/.gitignore
index 354b565..9018414 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -42,6 +42,7 @@ tmp.*
 /path_offset
 /path_offset_aliases
 /phandle_format
+/addr_size
 /property_iterate
 /propname_escapes
 /references
diff --git a/tests/Makefile.tests b/tests/Makefile.tests
index eb039c5..c715421 100644
--- a/tests/Makefile.tests
+++ b/tests/Makefile.tests
@@ -8,7 +8,7 @@ LIB_TESTS_L = get_mem_rsv \
 	char_literal \
 	sized_cells \
 	notfound \
-	addr_size_cells \
+	addr_size_cells addr_size \
 	stringlist \
 	setprop_inplace nop_property nop_node \
 	sw_tree1 \
diff --git a/tests/addr_size.c b/tests/addr_size.c
new file mode 100644
index 0000000..8463efc
--- /dev/null
+++ b/tests/addr_size.c
@@ -0,0 +1,74 @@
+/*
+ * libfdt - Flat Device Tree manipulation
+ *	Testcase for address and size handling
+ * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * Based on addr_size_cells.c by David Gibson, <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation; either version 2.1 of the License, or (at
+ * your option) any later version.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any kind,
+ * whether express or implied; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+
+#include <libfdt.h>
+
+#include "tests.h"
+#include "testdata.h"
+
+static void check_node(const void *fdt, const char *path, int idx,
+		       uintptr_t addr, size_t size)
+{
+	int offset, res;
+	uintptr_t xaddr;
+	uintptr_t xsize;
+
+	offset = fdt_path_offset(fdt, path);
+	if (offset < 0)
+		FAIL("Couldn't find path %s", path);
+
+	res = fdt_simple_addr_size(fdt, offset, idx, &xaddr, &xsize);
+	if (res < 0)
+		FAIL("fdt_simple_addr_size gave error: %s", fdt_strerror(res));
+
+	if (xaddr != addr)
+		FAIL("Physical address for %s is %p instead of %p\n",
+		     path, (void *) xaddr, (void *) addr);
+
+	if (xsize != size)
+		FAIL("Size for %s is %zx instead of %zx\n",
+		     path, xsize, size);
+}
+
+int main(int argc, char *argv[])
+{
+	void *fdt;
+
+	if (argc != 2)
+		CONFIG("Usage: %s <dtb file>\n", argv[0]);
+
+	test_init(argc, argv);
+	fdt = load_blob(argv[1]);
+
+	check_node(fdt, "/identity-bus@0/id-device@400", 0,
+		   0x400, 0x100);
+	check_node(fdt, "/simple-bus@1000000/sb-device@8000000800", 0,
+		   0x8000000800, 0x200);
+	check_node(fdt, "/identity-bus@0/id-device@400", 1,
+		   0x400000000, 0x100000030);
+	check_node(fdt, "/simple-bus@1000000/sb-device@8000000800", 1,
+		   0x70000000, 0x700);
+	check_node(fdt, "/simple-bus@1000000/sb-device@8000000800", 2,
+		   0x1050000000, 0x20);
+	PASS();
+}
diff --git a/tests/addresses.dts b/tests/addresses.dts
index a2faaf5..32c4379 100644
--- a/tests/addresses.dts
+++ b/tests/addresses.dts
@@ -6,10 +6,21 @@
 	#size-cells = <2>;
 
 	identity-bus@0 {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		id-device@400 {
+			reg = <0x0 0x00000400 0x0 0x00000100>,
+			      <0x4 0x00000000 0x1 0x00000030>;
+		};
 	};
 
 	simple-bus@1000000 {
 		#address-cells = <2>;
 		#size-cells = <1>;
+		sb-device@8000000800 {
+			reg = <0x80 0x00000800 0x200>,
+			      <0x00 0x70000000 0x700>,
+			      <0x10 0x50000000 0x020>;
+		};
 	};
 };
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index e4139dd..96aca48 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -243,6 +243,7 @@ libfdt_tests () {
 
     run_dtc_test -I dts -O dtb -o addresses.test.dtb addresses.dts
     run_test addr_size_cells addresses.test.dtb
+    run_test addr_size addresses.test.dtb
 
     run_dtc_test -I dts -O dtb -o stringlist.test.dtb stringlist.dts
     run_test stringlist stringlist.test.dtb
-- 
1.9.1

Re: [PATCH] libfdt: add helpers to read address and size from reg

[Benjamin Fair]

On 11/09/2016 10:58 AM, Benjamin Fair wrote:
> This patch extends the capability of libfdt to parse the contents of device
> trees in a similar manner to fdt_address_cells and fdt_size_cells.
>
> It adds a helper function which reads the address and size of a device from
> the reg property and performs basic sanity checks.
>
> It does not perform translation to a physical address using the ranges
> properties of parents, but this enhancement may be added as a separate
> function in the future.
>
> Signed-off-by: Benjamin Fair <b-fair-l0cyMroinI0@public.gmane.org>
> ---
>
> The intent of this patch is similar to the commit "libfdt: Add helpers to read
> #address-cells and #size-cells" [1].
>
> It is related to "libfdt: add address translation support" [2] but does not
> attempt to perform address translation and was written from scratch rather than
> reusing GPL code. If the issues with that patch are resolved, that
> functionality will complement what is added in this patch.
>
> [1] http://www.spinics.net/lists/devicetree-compiler/msg00113.html
> [2] http://www.spinics.net/lists/devicetree-compiler/msg00093.html
>
>  libfdt/fdt_addresses.c | 62 ++++++++++++++++++++++++++++++++++++++++++
>  libfdt/libfdt.h        | 29 ++++++++++++++++++++
>  libfdt/version.lds     |  1 +
>  tests/.gitignore       |  1 +
>  tests/Makefile.tests   |  2 +-
>  tests/addr_size.c      | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/addresses.dts    | 11 ++++++++
>  tests/run_tests.sh     |  1 +
>  8 files changed, 180 insertions(+), 1 deletion(-)
>  create mode 100644 tests/addr_size.c

Hi David,

Did you see this patch?

-- 
Benjamin Fair

Re: [PATCH] libfdt: add helpers to read address and size from reg

[Nishanth Menon]

On 11/18/2016 11:37 AM, Benjamin Fair wrote:
> On 11/09/2016 10:58 AM, Benjamin Fair wrote:
>> This patch extends the capability of libfdt to parse the contents of device
>> trees in a similar manner to fdt_address_cells and fdt_size_cells.
>>
>> It adds a helper function which reads the address and size of a device from
>> the reg property and performs basic sanity checks.
>>
>> It does not perform translation to a physical address using the ranges
>> properties of parents, but this enhancement may be added as a separate
>> function in the future.
>>
>> Signed-off-by: Benjamin Fair <b-fair-l0cyMroinI0@public.gmane.org>
>> ---
>>
>> The intent of this patch is similar to the commit "libfdt: Add helpers to read
>> #address-cells and #size-cells" [1].
>>
>> It is related to "libfdt: add address translation support" [2] but does not
>> attempt to perform address translation and was written from scratch rather than
>> reusing GPL code. If the issues with that patch are resolved, that
>> functionality will complement what is added in this patch.
>>
>>
>>  libfdt/fdt_addresses.c | 62 ++++++++++++++++++++++++++++++++++++++++++
>>  libfdt/libfdt.h        | 29 ++++++++++++++++++++
>>  libfdt/version.lds     |  1 +
>>  tests/.gitignore       |  1 +
>>  tests/Makefile.tests   |  2 +-
>>  tests/addr_size.c      | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/addresses.dts    | 11 ++++++++
>>  tests/run_tests.sh     |  1 +
>>  8 files changed, 180 insertions(+), 1 deletion(-)
>>  create mode 100644 tests/addr_size.c

I think in a lot of places this simple translation of DT can easily be 
benefiting.

[1] http://www.spinics.net/lists/devicetree-compiler/msg00113.html
[2] http://www.spinics.net/lists/devicetree-compiler/msg00093.html

Rob, David,

The motivation started off by wanting to have a better integration of 
libfdt and dt support in ATF[2] - and basic stuff is to be able to 
read address from a node - not that we could'nt do something similar 
as a ATF specific library... but common might be better for all of 
libfdt users, no?

Do you folks have a suggestion as to what might be a better direction 
to take here?

Original patch for reference (since my google-fu failed to be able to 
find a devicetree-compiler patchworks)[1]:

[1] http://www.spinics.net/lists/devicetree-compiler/msg00844.html
[2] 
https://github.com/ARM-software/arm-trusted-firmware/pull/747#issuecomment-259727132
-- 
Regards,
Nishanth Menon

Re: [PATCH] libfdt: add helpers to read address and size from reg

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2084 bytes --]

On Fri, Nov 18, 2016 at 11:37:41AM -0600, Benjamin Fair wrote:
> On 11/09/2016 10:58 AM, Benjamin Fair wrote:
> > This patch extends the capability of libfdt to parse the contents of device
> > trees in a similar manner to fdt_address_cells and fdt_size_cells.
> > 
> > It adds a helper function which reads the address and size of a device from
> > the reg property and performs basic sanity checks.
> > 
> > It does not perform translation to a physical address using the ranges
> > properties of parents, but this enhancement may be added as a separate
> > function in the future.
> > 
> > Signed-off-by: Benjamin Fair <b-fair-l0cyMroinI0@public.gmane.org>
> > ---
> > 
> > The intent of this patch is similar to the commit "libfdt: Add helpers to read
> > #address-cells and #size-cells" [1].
> > 
> > It is related to "libfdt: add address translation support" [2] but does not
> > attempt to perform address translation and was written from scratch rather than
> > reusing GPL code. If the issues with that patch are resolved, that
> > functionality will complement what is added in this patch.
> > 
> > [1] http://www.spinics.net/lists/devicetree-compiler/msg00113.html
> > [2] http://www.spinics.net/lists/devicetree-compiler/msg00093.html
> > 
> >  libfdt/fdt_addresses.c | 62 ++++++++++++++++++++++++++++++++++++++++++
> >  libfdt/libfdt.h        | 29 ++++++++++++++++++++
> >  libfdt/version.lds     |  1 +
> >  tests/.gitignore       |  1 +
> >  tests/Makefile.tests   |  2 +-
> >  tests/addr_size.c      | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/addresses.dts    | 11 ++++++++
> >  tests/run_tests.sh     |  1 +
> >  8 files changed, 180 insertions(+), 1 deletion(-)
> >  create mode 100644 tests/addr_size.c
> 
> Hi David,
> 
> Did you see this patch?

Yes, I saw it, I've just been busy with other things. 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] libfdt: add helpers to read address and size from reg

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2951 bytes --]

On Fri, Nov 18, 2016 at 12:05:48PM -0600, Nishanth Menon wrote:
> On 11/18/2016 11:37 AM, Benjamin Fair wrote:
> > On 11/09/2016 10:58 AM, Benjamin Fair wrote:
> > > This patch extends the capability of libfdt to parse the contents of device
> > > trees in a similar manner to fdt_address_cells and fdt_size_cells.
> > > 
> > > It adds a helper function which reads the address and size of a device from
> > > the reg property and performs basic sanity checks.
> > > 
> > > It does not perform translation to a physical address using the ranges
> > > properties of parents, but this enhancement may be added as a separate
> > > function in the future.
> > > 
> > > Signed-off-by: Benjamin Fair <b-fair-l0cyMroinI0@public.gmane.org>
> > > ---
> > > 
> > > The intent of this patch is similar to the commit "libfdt: Add helpers to read
> > > #address-cells and #size-cells" [1].
> > > 
> > > It is related to "libfdt: add address translation support" [2] but does not
> > > attempt to perform address translation and was written from scratch rather than
> > > reusing GPL code. If the issues with that patch are resolved, that
> > > functionality will complement what is added in this patch.
> > > 
> > > 
> > >  libfdt/fdt_addresses.c | 62 ++++++++++++++++++++++++++++++++++++++++++
> > >  libfdt/libfdt.h        | 29 ++++++++++++++++++++
> > >  libfdt/version.lds     |  1 +
> > >  tests/.gitignore       |  1 +
> > >  tests/Makefile.tests   |  2 +-
> > >  tests/addr_size.c      | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/addresses.dts    | 11 ++++++++
> > >  tests/run_tests.sh     |  1 +
> > >  8 files changed, 180 insertions(+), 1 deletion(-)
> > >  create mode 100644 tests/addr_size.c
> 
> I think in a lot of places this simple translation of DT can easily be
> benefiting.
> 
> [1] http://www.spinics.net/lists/devicetree-compiler/msg00113.html
> [2] http://www.spinics.net/lists/devicetree-compiler/msg00093.html
> 
> Rob, David,
> 
> The motivation started off by wanting to have a better integration of libfdt
> and dt support in ATF[2] - and basic stuff is to be able to read address
> from a node - not that we could'nt do something similar as a ATF specific
> library... but common might be better for all of libfdt users, no?
> 
> Do you folks have a suggestion as to what might be a better direction to
> take here?

Sorry, I don't really follow what the question is.

> 
> Original patch for reference (since my google-fu failed to be able to find a
> devicetree-compiler patchworks)[1]:
> 
> [1] http://www.spinics.net/lists/devicetree-compiler/msg00844.html
> [2] https://github.com/ARM-software/arm-trusted-firmware/pull/747#issuecomment-259727132

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] libfdt: add helpers to read address and size from reg

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5413 bytes --]

On Wed, Nov 09, 2016 at 10:58:32AM -0600, Benjamin Fair wrote:
> This patch extends the capability of libfdt to parse the contents of device
> trees in a similar manner to fdt_address_cells and fdt_size_cells.
> 
> It adds a helper function which reads the address and size of a device from
> the reg property and performs basic sanity checks.
> 
> It does not perform translation to a physical address using the ranges
> properties of parents, but this enhancement may be added as a separate
> function in the future.

I like the concept of a helper to read entries from reg, but there
some things about the execution of it I think need some more thought.

> Signed-off-by: Benjamin Fair <b-fair-l0cyMroinI0@public.gmane.org>
> ---
> 
> The intent of this patch is similar to the commit "libfdt: Add helpers to read
> #address-cells and #size-cells" [1].
> 
> It is related to "libfdt: add address translation support" [2] but does not
> attempt to perform address translation and was written from scratch rather than
> reusing GPL code. If the issues with that patch are resolved, that
> functionality will complement what is added in this patch.
> 
> [1] http://www.spinics.net/lists/devicetree-compiler/msg00113.html
> [2] http://www.spinics.net/lists/devicetree-compiler/msg00093.html
> 
>  libfdt/fdt_addresses.c | 62 ++++++++++++++++++++++++++++++++++++++++++
>  libfdt/libfdt.h        | 29 ++++++++++++++++++++
>  libfdt/version.lds     |  1 +
>  tests/.gitignore       |  1 +
>  tests/Makefile.tests   |  2 +-
>  tests/addr_size.c      | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/addresses.dts    | 11 ++++++++
>  tests/run_tests.sh     |  1 +
>  8 files changed, 180 insertions(+), 1 deletion(-)
>  create mode 100644 tests/addr_size.c
> 
> diff --git a/libfdt/fdt_addresses.c b/libfdt/fdt_addresses.c
> index eff4dbc..92cbed9 100644
> --- a/libfdt/fdt_addresses.c
> +++ b/libfdt/fdt_addresses.c
> @@ -55,6 +55,9 @@
>  
>  #include "libfdt_internal.h"
>  
> +#define BYTES_PER_CELL 4
> +#define BITS_PER_CELL 32

You shouldn't need these.  BYTES_PER_CELL == sizeof(fdt32_t).

> +
>  int fdt_address_cells(const void *fdt, int nodeoffset)
>  {
>  	const fdt32_t *ac;
> @@ -94,3 +97,62 @@ int fdt_size_cells(const void *fdt, int nodeoffset)
>  
>  	return val;
>  }
> +
> +static uint64_t _fdt_read_cells(const fdt32_t *cells, int n)

This is a reasonable helper, but the name is bad.  "read_cells"
suggests it can read some arbitrary number of cells, but in fact all
it can do is read a 32-bit int or a 64-bit int.  Plus everything is
made up of cells, but more specifically what you're doing here is
interpreting several cells as an integer in the usual encoding.

> +{
> +	int i;
> +	uint64_t res;
> +
> +	/* TODO: Support values larger than 2 cells */

I don't really see any way you could support >2 cells without
completely changing the interface.

> +	if (n > 2)
> +		return -FDT_ERR_BADNCELLS;
> +
> +	res = 0;
> +	for (i = 0; i < n; i++) {
> +		res <<= BITS_PER_CELL;
> +		res |= fdt32_to_cpu(cells[i]);
> +	}
> +
> +	return res;
> +}
> +
> +int fdt_simple_addr_size(const void *fdt, int nodeoffset, int idx,
> +			 uintptr_t *addr, size_t *size)
> +{
> +	int parent;
> +	int ac, sc, reg_stride;
> +	int res;
> +	const fdt32_t *reg;
> +
> +	reg = fdt_getprop(fdt, nodeoffset, "reg", &res);
> +	if (res < 0)
> +		return res;
> +
> +	parent = fdt_parent_offset(fdt, nodeoffset);
> +	if (parent < 0)
> +		return res;

So, fdt_parent_offset() is very expensive, I wouldn't recommend it in
a function that's likely to be called a lot like this.  Instead I'd
suggest a function which takes the parent offset as a parameter, and
optionally a wrapper that uses fdt_parent_offset().

> +
> +	ac = fdt_address_cells(fdt, parent);
> +	if (ac < 0)
> +		return ac;
> +
> +	sc = fdt_size_cells(fdt, parent);
> +	if (sc < 0)
> +		return sc;
> +
> +	reg_stride = ac + sc;
> +	/*
> +	 * res is the number of bytes read and must be an even multiple of the
> +	 * sum of ac and sc.
> +	 */
> +	if ((res % (reg_stride * BYTES_PER_CELL)) != 0)
> +		return -FDT_ERR_BADVALUE;
> +
> +	if (addr)
> +		*addr = (uintptr_t) _fdt_read_cells(&reg[reg_stride * idx], ac);

I don't think uintptr_t makes sense here.  The addresses in the device
tree are in whatever bus they're in, and there are a whole stack of
reasons that could be unrelated to the pointer size of environment
libfdt is running in:
    - The device may be on a subordinate bus whose addresses need
      to be translated
    - Even at the top-level, the reg properties represent
      *physical* addresses, which may not be the same as virtual
      addresses  in code running on the system
    - libfdt may be running on a completely different system from the
      one the device tree in question is aimed at (bootloaders are
      only one use case for libfdt).

> +	if (size)
> +		*size = (size_t) _fdt_read_cells(&reg[ac + reg_stride * idx],
> +						 sc);

Likewise size_t isn't necessarily right here, although I suspect it's
less likely to break in practice.

> +	return 0;
> +}

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] libfdt: add helpers to read address and size from reg

[Benjamin Fair]

On 11/25/2016 04:51 AM, David Gibson wrote:
> On Wed, Nov 09, 2016 at 10:58:32AM -0600, Benjamin Fair wrote:
[...]
>
> I like the concept of a helper to read entries from reg, but there
> some things about the execution of it I think need some more thought.
>

Thanks for the review.

[...]
>> diff --git a/libfdt/fdt_addresses.c b/libfdt/fdt_addresses.c
>> index eff4dbc..92cbed9 100644
>> --- a/libfdt/fdt_addresses.c
>> +++ b/libfdt/fdt_addresses.c
>> @@ -55,6 +55,9 @@
>>
>>  #include "libfdt_internal.h"
>>
>> +#define BYTES_PER_CELL 4
>> +#define BITS_PER_CELL 32
>
> You shouldn't need these.  BYTES_PER_CELL == sizeof(fdt32_t).
>

Of course. Thanks. I'll get rid of them.

>> +
>>  int fdt_address_cells(const void *fdt, int nodeoffset)
>>  {
>>  	const fdt32_t *ac;
>> @@ -94,3 +97,62 @@ int fdt_size_cells(const void *fdt, int nodeoffset)
>>
>>  	return val;
>>  }
>> +
>> +static uint64_t _fdt_read_cells(const fdt32_t *cells, int n)
>
> This is a reasonable helper, but the name is bad.  "read_cells"
> suggests it can read some arbitrary number of cells, but in fact all
> it can do is read a 32-bit int or a 64-bit int.  Plus everything is
> made up of cells, but more specifically what you're doing here is
> interpreting several cells as an integer in the usual encoding.
>

Would "cells_to_integer" be a better name? Or would you recommend 
something else for this?

>> +{
>> +	int i;
>> +	uint64_t res;
>> +
>> +	/* TODO: Support values larger than 2 cells */
>
> I don't really see any way you could support >2 cells without
> completely changing the interface.
>

True. I wanted to have the result be a 128 bit integer, but couldn't 
find a portable way to do so. Is there a better way to go about this? Or 
is it fine to only support at most 2 cells, even though the rest of 
libfdt supports 4?

>> +	if (n > 2)
>> +		return -FDT_ERR_BADNCELLS;
>> +
>> +	res = 0;
>> +	for (i = 0; i < n; i++) {
>> +		res <<= BITS_PER_CELL;
>> +		res |= fdt32_to_cpu(cells[i]);
>> +	}
>> +
>> +	return res;
>> +}
>> +
>> +int fdt_simple_addr_size(const void *fdt, int nodeoffset, int idx,
>> +			 uintptr_t *addr, size_t *size)
>> +{
>> +	int parent;
>> +	int ac, sc, reg_stride;
>> +	int res;
>> +	const fdt32_t *reg;
>> +
>> +	reg = fdt_getprop(fdt, nodeoffset, "reg", &res);
>> +	if (res < 0)
>> +		return res;
>> +
>> +	parent = fdt_parent_offset(fdt, nodeoffset);
>> +	if (parent < 0)
>> +		return res;
>
> So, fdt_parent_offset() is very expensive, I wouldn't recommend it in
> a function that's likely to be called a lot like this.  Instead I'd
> suggest a function which takes the parent offset as a parameter, and
> optionally a wrapper that uses fdt_parent_offset().

Great idea, I'll do this in the next revision once we have a solution 
for the rest of the comments.

>
>> +
>> +	ac = fdt_address_cells(fdt, parent);
>> +	if (ac < 0)
>> +		return ac;
>> +
>> +	sc = fdt_size_cells(fdt, parent);
>> +	if (sc < 0)
>> +		return sc;
>> +
>> +	reg_stride = ac + sc;
>> +	/*
>> +	 * res is the number of bytes read and must be an even multiple of the
>> +	 * sum of ac and sc.
>> +	 */
>> +	if ((res % (reg_stride * BYTES_PER_CELL)) != 0)
>> +		return -FDT_ERR_BADVALUE;
>> +
>> +	if (addr)
>> +		*addr = (uintptr_t) _fdt_read_cells(&reg[reg_stride * idx], ac);
>
> I don't think uintptr_t makes sense here.  The addresses in the device
> tree are in whatever bus they're in, and there are a whole stack of
> reasons that could be unrelated to the pointer size of environment
> libfdt is running in:
>     - The device may be on a subordinate bus whose addresses need
>       to be translated
>     - Even at the top-level, the reg properties represent
>       *physical* addresses, which may not be the same as virtual
>       addresses  in code running on the system
>     - libfdt may be running on a completely different system from the
>       one the device tree in question is aimed at (bootloaders are
>       only one use case for libfdt).
>
>> +	if (size)
>> +		*size = (size_t) _fdt_read_cells(&reg[ac + reg_stride * idx],
>> +						 sc);
>
> Likewise size_t isn't necessarily right here, although I suspect it's
> less likely to break in practice.
>

Hmm... Is it fine to use uint64_t for both of these instead then?

>> +	return 0;
>> +}
>


-- 
Benjamin Fair

Re: [PATCH] libfdt: add helpers to read address and size from reg

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6174 bytes --]

On Wed, Nov 30, 2016 at 01:35:04PM -0600, Benjamin Fair wrote:
> On 11/25/2016 04:51 AM, David Gibson wrote:
> > On Wed, Nov 09, 2016 at 10:58:32AM -0600, Benjamin Fair wrote:
> [...]
> > 
> > I like the concept of a helper to read entries from reg, but there
> > some things about the execution of it I think need some more thought.
> > 
> 
> Thanks for the review.
> 
> [...]
> > > diff --git a/libfdt/fdt_addresses.c b/libfdt/fdt_addresses.c
> > > index eff4dbc..92cbed9 100644
> > > --- a/libfdt/fdt_addresses.c
> > > +++ b/libfdt/fdt_addresses.c
> > > @@ -55,6 +55,9 @@
> > > 
> > >  #include "libfdt_internal.h"
> > > 
> > > +#define BYTES_PER_CELL 4
> > > +#define BITS_PER_CELL 32
> > 
> > You shouldn't need these.  BYTES_PER_CELL == sizeof(fdt32_t).
> > 
> 
> Of course. Thanks. I'll get rid of them.
> 
> > > +
> > >  int fdt_address_cells(const void *fdt, int nodeoffset)
> > >  {
> > >  	const fdt32_t *ac;
> > > @@ -94,3 +97,62 @@ int fdt_size_cells(const void *fdt, int nodeoffset)
> > > 
> > >  	return val;
> > >  }
> > > +
> > > +static uint64_t _fdt_read_cells(const fdt32_t *cells, int n)
> > 
> > This is a reasonable helper, but the name is bad.  "read_cells"
> > suggests it can read some arbitrary number of cells, but in fact all
> > it can do is read a 32-bit int or a 64-bit int.  Plus everything is
> > made up of cells, but more specifically what you're doing here is
> > interpreting several cells as an integer in the usual encoding.
> > 
> 
> Would "cells_to_integer" be a better name? Or would you recommend something
> else for this?

cells_to_integer would be ok.  Or just fdt_read_integer().

> 
> > > +{
> > > +	int i;
> > > +	uint64_t res;
> > > +
> > > +	/* TODO: Support values larger than 2 cells */
> > 
> > I don't really see any way you could support >2 cells without
> > completely changing the interface.
> > 
> 
> True. I wanted to have the result be a 128 bit integer, but couldn't find a
> portable way to do so. Is there a better way to go about this? Or is it fine
> to only support at most 2 cells, even though the rest of libfdt supports 4?

I think just supporting 2 cells is ok - it's useful in enough
practical cases.  However, I'd suggest thinking of this in a more
layered way.

First, create a helper which just locates the relevant pieces of reg
entries, returning addresses and sizes as (fdt32_t *).  That's at
least somewhat useful for the >2 cells case, even though you have to
then parse the address/size yourself.  In the most common case for
this, PCI (address-cells == 3), you probably want to do that anyway.
This may also be useful for cases which are <= 2 cells, but the
encoding of the address is not just a plain integer.

You can then polish up fdt_read_integer() a bit and export it.

Finally you can add another helper which combines these to directly
get you the addr+size as u64 for buses where that's suitable.  Make
sure to return an error if #a or #s > 2, of course.

> > > +	if (n > 2)
> > > +		return -FDT_ERR_BADNCELLS;
> > > +
> > > +	res = 0;
> > > +	for (i = 0; i < n; i++) {
> > > +		res <<= BITS_PER_CELL;
> > > +		res |= fdt32_to_cpu(cells[i]);
> > > +	}
> > > +
> > > +	return res;
> > > +}
> > > +
> > > +int fdt_simple_addr_size(const void *fdt, int nodeoffset, int idx,
> > > +			 uintptr_t *addr, size_t *size)
> > > +{
> > > +	int parent;
> > > +	int ac, sc, reg_stride;
> > > +	int res;
> > > +	const fdt32_t *reg;
> > > +
> > > +	reg = fdt_getprop(fdt, nodeoffset, "reg", &res);
> > > +	if (res < 0)
> > > +		return res;
> > > +
> > > +	parent = fdt_parent_offset(fdt, nodeoffset);
> > > +	if (parent < 0)
> > > +		return res;
> > 
> > So, fdt_parent_offset() is very expensive, I wouldn't recommend it in
> > a function that's likely to be called a lot like this.  Instead I'd
> > suggest a function which takes the parent offset as a parameter, and
> > optionally a wrapper that uses fdt_parent_offset().
> 
> Great idea, I'll do this in the next revision once we have a solution for
> the rest of the comments.
> 
> > 
> > > +
> > > +	ac = fdt_address_cells(fdt, parent);
> > > +	if (ac < 0)
> > > +		return ac;
> > > +
> > > +	sc = fdt_size_cells(fdt, parent);
> > > +	if (sc < 0)
> > > +		return sc;
> > > +
> > > +	reg_stride = ac + sc;
> > > +	/*
> > > +	 * res is the number of bytes read and must be an even multiple of the
> > > +	 * sum of ac and sc.
> > > +	 */
> > > +	if ((res % (reg_stride * BYTES_PER_CELL)) != 0)
> > > +		return -FDT_ERR_BADVALUE;
> > > +
> > > +	if (addr)
> > > +		*addr = (uintptr_t) _fdt_read_cells(&reg[reg_stride * idx], ac);
> > 
> > I don't think uintptr_t makes sense here.  The addresses in the device
> > tree are in whatever bus they're in, and there are a whole stack of
> > reasons that could be unrelated to the pointer size of environment
> > libfdt is running in:
> >     - The device may be on a subordinate bus whose addresses need
> >       to be translated
> >     - Even at the top-level, the reg properties represent
> >       *physical* addresses, which may not be the same as virtual
> >       addresses  in code running on the system
> >     - libfdt may be running on a completely different system from the
> >       one the device tree in question is aimed at (bootloaders are
> >       only one use case for libfdt).
> > 
> > > +	if (size)
> > > +		*size = (size_t) _fdt_read_cells(&reg[ac + reg_stride * idx],
> > > +						 sc);
> > 
> > Likewise size_t isn't necessarily right here, although I suspect it's
> > less likely to break in practice.
> > 
> 
> Hmm... Is it fine to use uint64_t for both of these instead then?

Yes, I think that's reasonable.  Or you could even use unsigned long
or unsigned long long - as long as you error whenever #cells is too
big for the size of that type.  uint64_t is probably clearer and more
consistent, though.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] libfdt: add helpers to read address and size from reg

[Benjamin Fair]

On 12/01/2016 09:12 PM, David Gibson wrote:
> On Wed, Nov 30, 2016 at 01:35:04PM -0600, Benjamin Fair wrote:
>> On 11/25/2016 04:51 AM, David Gibson wrote:
>>> On Wed, Nov 09, 2016 at 10:58:32AM -0600, Benjamin Fair wrote:
>> [...]
>>>
>>> I like the concept of a helper to read entries from reg, but there
>>> some things about the execution of it I think need some more thought.
>>>
>>
>> Thanks for the review.
>>
>> [...]
>>>> diff --git a/libfdt/fdt_addresses.c b/libfdt/fdt_addresses.c
>>>> index eff4dbc..92cbed9 100644
>>>> --- a/libfdt/fdt_addresses.c
>>>> +++ b/libfdt/fdt_addresses.c
>>>> @@ -55,6 +55,9 @@
>>>>
>>>>  #include "libfdt_internal.h"
>>>>
>>>> +#define BYTES_PER_CELL 4
>>>> +#define BITS_PER_CELL 32
>>>
>>> You shouldn't need these.  BYTES_PER_CELL == sizeof(fdt32_t).
>>>
>>
>> Of course. Thanks. I'll get rid of them.
>>
>>>> +
>>>>  int fdt_address_cells(const void *fdt, int nodeoffset)
>>>>  {
>>>>  	const fdt32_t *ac;
>>>> @@ -94,3 +97,62 @@ int fdt_size_cells(const void *fdt, int nodeoffset)
>>>>
>>>>  	return val;
>>>>  }
>>>> +
>>>> +static uint64_t _fdt_read_cells(const fdt32_t *cells, int n)
>>>
>>> This is a reasonable helper, but the name is bad.  "read_cells"
>>> suggests it can read some arbitrary number of cells, but in fact all
>>> it can do is read a 32-bit int or a 64-bit int.  Plus everything is
>>> made up of cells, but more specifically what you're doing here is
>>> interpreting several cells as an integer in the usual encoding.
>>>
>>
>> Would "cells_to_integer" be a better name? Or would you recommend something
>> else for this?
>
> cells_to_integer would be ok.  Or just fdt_read_integer().
>

Thanks for the suggestion. I changed it to fdt_read_integer()

>>
>>>> +{
>>>> +	int i;
>>>> +	uint64_t res;
>>>> +
>>>> +	/* TODO: Support values larger than 2 cells */
>>>
>>> I don't really see any way you could support >2 cells without
>>> completely changing the interface.
>>>
>>
>> True. I wanted to have the result be a 128 bit integer, but couldn't find a
>> portable way to do so. Is there a better way to go about this? Or is it fine
>> to only support at most 2 cells, even though the rest of libfdt supports 4?
>
> I think just supporting 2 cells is ok - it's useful in enough
> practical cases.  However, I'd suggest thinking of this in a more
> layered way.
>
> First, create a helper which just locates the relevant pieces of reg
> entries, returning addresses and sizes as (fdt32_t *).  That's at
> least somewhat useful for the >2 cells case, even though you have to
> then parse the address/size yourself.  In the most common case for
> this, PCI (address-cells == 3), you probably want to do that anyway.
> This may also be useful for cases which are <= 2 cells, but the
> encoding of the address is not just a plain integer.
>
> You can then polish up fdt_read_integer() a bit and export it.
>
> Finally you can add another helper which combines these to directly
> get you the addr+size as u64 for buses where that's suitable.  Make
> sure to return an error if #a or #s > 2, of course.
>

That seems like a good idea. I implemented this for the next revision.

>>>> +	if (n > 2)
>>>> +		return -FDT_ERR_BADNCELLS;
>>>> +
>>>> +	res = 0;
>>>> +	for (i = 0; i < n; i++) {
>>>> +		res <<= BITS_PER_CELL;
>>>> +		res |= fdt32_to_cpu(cells[i]);
>>>> +	}
>>>> +
>>>> +	return res;
>>>> +}
>>>> +
>>>> +int fdt_simple_addr_size(const void *fdt, int nodeoffset, int idx,
>>>> +			 uintptr_t *addr, size_t *size)
>>>> +{
>>>> +	int parent;
>>>> +	int ac, sc, reg_stride;
>>>> +	int res;
>>>> +	const fdt32_t *reg;
>>>> +
>>>> +	reg = fdt_getprop(fdt, nodeoffset, "reg", &res);
>>>> +	if (res < 0)
>>>> +		return res;
>>>> +
>>>> +	parent = fdt_parent_offset(fdt, nodeoffset);
>>>> +	if (parent < 0)
>>>> +		return res;
>>>
>>> So, fdt_parent_offset() is very expensive, I wouldn't recommend it in
>>> a function that's likely to be called a lot like this.  Instead I'd
>>> suggest a function which takes the parent offset as a parameter, and
>>> optionally a wrapper that uses fdt_parent_offset().
>>
>> Great idea, I'll do this in the next revision once we have a solution for
>> the rest of the comments.
>>

Would it be OK to pass in the number of address cells and number of 
size cells directly instead of the parent offset?

I was thinking about this in order to minimize repeated work. Plus, 
the calling function needs to know this information in order to 
interpret the results anyway.

>>>
>>>> +
>>>> +	ac = fdt_address_cells(fdt, parent);
>>>> +	if (ac < 0)
>>>> +		return ac;
>>>> +
>>>> +	sc = fdt_size_cells(fdt, parent);
>>>> +	if (sc < 0)
>>>> +		return sc;
>>>> +
>>>> +	reg_stride = ac + sc;
>>>> +	/*
>>>> +	 * res is the number of bytes read and must be an even multiple of the
>>>> +	 * sum of ac and sc.
>>>> +	 */
>>>> +	if ((res % (reg_stride * BYTES_PER_CELL)) != 0)
>>>> +		return -FDT_ERR_BADVALUE;
>>>> +
>>>> +	if (addr)
>>>> +		*addr = (uintptr_t) _fdt_read_cells(&reg[reg_stride * idx], ac);
>>>
>>> I don't think uintptr_t makes sense here.  The addresses in the device
>>> tree are in whatever bus they're in, and there are a whole stack of
>>> reasons that could be unrelated to the pointer size of environment
>>> libfdt is running in:
>>>     - The device may be on a subordinate bus whose addresses need
>>>       to be translated
>>>     - Even at the top-level, the reg properties represent
>>>       *physical* addresses, which may not be the same as virtual
>>>       addresses  in code running on the system
>>>     - libfdt may be running on a completely different system from the
>>>       one the device tree in question is aimed at (bootloaders are
>>>       only one use case for libfdt).
>>>
>>>> +	if (size)
>>>> +		*size = (size_t) _fdt_read_cells(&reg[ac + reg_stride * idx],
>>>> +						 sc);
>>>
>>> Likewise size_t isn't necessarily right here, although I suspect it's
>>> less likely to break in practice.
>>>
>>
>> Hmm... Is it fine to use uint64_t for both of these instead then?
>
> Yes, I think that's reasonable.  Or you could even use unsigned long
> or unsigned long long - as long as you error whenever #cells is too
> big for the size of that type.  uint64_t is probably clearer and more
> consistent, though.
>

Thanks, I changed it to use uint64_t

-- 
Benjamin Fair

Re: [PATCH] libfdt: add helpers to read address and size from reg

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7626 bytes --]

On Mon, Dec 05, 2016 at 11:46:31AM -0600, Benjamin Fair wrote:
> On 12/01/2016 09:12 PM, David Gibson wrote:
> > On Wed, Nov 30, 2016 at 01:35:04PM -0600, Benjamin Fair wrote:
> > > On 11/25/2016 04:51 AM, David Gibson wrote:
> > > > On Wed, Nov 09, 2016 at 10:58:32AM -0600, Benjamin Fair wrote:
> > > [...]
> > > > 
> > > > I like the concept of a helper to read entries from reg, but there
> > > > some things about the execution of it I think need some more thought.
> > > > 
> > > 
> > > Thanks for the review.
> > > 
> > > [...]
> > > > > diff --git a/libfdt/fdt_addresses.c b/libfdt/fdt_addresses.c
> > > > > index eff4dbc..92cbed9 100644
> > > > > --- a/libfdt/fdt_addresses.c
> > > > > +++ b/libfdt/fdt_addresses.c
> > > > > @@ -55,6 +55,9 @@
> > > > > 
> > > > >  #include "libfdt_internal.h"
> > > > > 
> > > > > +#define BYTES_PER_CELL 4
> > > > > +#define BITS_PER_CELL 32
> > > > 
> > > > You shouldn't need these.  BYTES_PER_CELL == sizeof(fdt32_t).
> > > > 
> > > 
> > > Of course. Thanks. I'll get rid of them.
> > > 
> > > > > +
> > > > >  int fdt_address_cells(const void *fdt, int nodeoffset)
> > > > >  {
> > > > >  	const fdt32_t *ac;
> > > > > @@ -94,3 +97,62 @@ int fdt_size_cells(const void *fdt, int nodeoffset)
> > > > > 
> > > > >  	return val;
> > > > >  }
> > > > > +
> > > > > +static uint64_t _fdt_read_cells(const fdt32_t *cells, int n)
> > > > 
> > > > This is a reasonable helper, but the name is bad.  "read_cells"
> > > > suggests it can read some arbitrary number of cells, but in fact all
> > > > it can do is read a 32-bit int or a 64-bit int.  Plus everything is
> > > > made up of cells, but more specifically what you're doing here is
> > > > interpreting several cells as an integer in the usual encoding.
> > > > 
> > > 
> > > Would "cells_to_integer" be a better name? Or would you recommend something
> > > else for this?
> > 
> > cells_to_integer would be ok.  Or just fdt_read_integer().
> > 
> 
> Thanks for the suggestion. I changed it to fdt_read_integer()
> 
> > > 
> > > > > +{
> > > > > +	int i;
> > > > > +	uint64_t res;
> > > > > +
> > > > > +	/* TODO: Support values larger than 2 cells */
> > > > 
> > > > I don't really see any way you could support >2 cells without
> > > > completely changing the interface.
> > > > 
> > > 
> > > True. I wanted to have the result be a 128 bit integer, but couldn't find a
> > > portable way to do so. Is there a better way to go about this? Or is it fine
> > > to only support at most 2 cells, even though the rest of libfdt supports 4?
> > 
> > I think just supporting 2 cells is ok - it's useful in enough
> > practical cases.  However, I'd suggest thinking of this in a more
> > layered way.
> > 
> > First, create a helper which just locates the relevant pieces of reg
> > entries, returning addresses and sizes as (fdt32_t *).  That's at
> > least somewhat useful for the >2 cells case, even though you have to
> > then parse the address/size yourself.  In the most common case for
> > this, PCI (address-cells == 3), you probably want to do that anyway.
> > This may also be useful for cases which are <= 2 cells, but the
> > encoding of the address is not just a plain integer.
> > 
> > You can then polish up fdt_read_integer() a bit and export it.
> > 
> > Finally you can add another helper which combines these to directly
> > get you the addr+size as u64 for buses where that's suitable.  Make
> > sure to return an error if #a or #s > 2, of course.
> > 
> 
> That seems like a good idea. I implemented this for the next revision.
> 
> > > > > +	if (n > 2)
> > > > > +		return -FDT_ERR_BADNCELLS;
> > > > > +
> > > > > +	res = 0;
> > > > > +	for (i = 0; i < n; i++) {
> > > > > +		res <<= BITS_PER_CELL;
> > > > > +		res |= fdt32_to_cpu(cells[i]);
> > > > > +	}
> > > > > +
> > > > > +	return res;
> > > > > +}
> > > > > +
> > > > > +int fdt_simple_addr_size(const void *fdt, int nodeoffset, int idx,
> > > > > +			 uintptr_t *addr, size_t *size)
> > > > > +{
> > > > > +	int parent;
> > > > > +	int ac, sc, reg_stride;
> > > > > +	int res;
> > > > > +	const fdt32_t *reg;
> > > > > +
> > > > > +	reg = fdt_getprop(fdt, nodeoffset, "reg", &res);
> > > > > +	if (res < 0)
> > > > > +		return res;
> > > > > +
> > > > > +	parent = fdt_parent_offset(fdt, nodeoffset);
> > > > > +	if (parent < 0)
> > > > > +		return res;
> > > > 
> > > > So, fdt_parent_offset() is very expensive, I wouldn't recommend it in
> > > > a function that's likely to be called a lot like this.  Instead I'd
> > > > suggest a function which takes the parent offset as a parameter, and
> > > > optionally a wrapper that uses fdt_parent_offset().
> > > 
> > > Great idea, I'll do this in the next revision once we have a solution for
> > > the rest of the comments.
> > > 
> 
> Would it be OK to pass in the number of address cells and number of size
> cells directly instead of the parent offset?

Yes, that seems reasonable.  Though you might want one function which
does that, then another which will do it for you based on parent
offset.

> I was thinking about this in order to minimize repeated work. Plus, the
> calling function needs to know this information in order to interpret the
> results anyway.

True.

> > > > 
> > > > > +
> > > > > +	ac = fdt_address_cells(fdt, parent);
> > > > > +	if (ac < 0)
> > > > > +		return ac;
> > > > > +
> > > > > +	sc = fdt_size_cells(fdt, parent);
> > > > > +	if (sc < 0)
> > > > > +		return sc;
> > > > > +
> > > > > +	reg_stride = ac + sc;
> > > > > +	/*
> > > > > +	 * res is the number of bytes read and must be an even multiple of the
> > > > > +	 * sum of ac and sc.
> > > > > +	 */
> > > > > +	if ((res % (reg_stride * BYTES_PER_CELL)) != 0)
> > > > > +		return -FDT_ERR_BADVALUE;
> > > > > +
> > > > > +	if (addr)
> > > > > +		*addr = (uintptr_t) _fdt_read_cells(&reg[reg_stride * idx], ac);
> > > > 
> > > > I don't think uintptr_t makes sense here.  The addresses in the device
> > > > tree are in whatever bus they're in, and there are a whole stack of
> > > > reasons that could be unrelated to the pointer size of environment
> > > > libfdt is running in:
> > > >     - The device may be on a subordinate bus whose addresses need
> > > >       to be translated
> > > >     - Even at the top-level, the reg properties represent
> > > >       *physical* addresses, which may not be the same as virtual
> > > >       addresses  in code running on the system
> > > >     - libfdt may be running on a completely different system from the
> > > >       one the device tree in question is aimed at (bootloaders are
> > > >       only one use case for libfdt).
> > > > 
> > > > > +	if (size)
> > > > > +		*size = (size_t) _fdt_read_cells(&reg[ac + reg_stride * idx],
> > > > > +						 sc);
> > > > 
> > > > Likewise size_t isn't necessarily right here, although I suspect it's
> > > > less likely to break in practice.
> > > > 
> > > 
> > > Hmm... Is it fine to use uint64_t for both of these instead then?
> > 
> > Yes, I think that's reasonable.  Or you could even use unsigned long
> > or unsigned long long - as long as you error whenever #cells is too
> > big for the size of that type.  uint64_t is probably clearer and more
> > consistent, though.
> > 
> 
> Thanks, I changed it to use uint64_t
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]