fdt: Fix fdtdec_get_addr_size() for 64-bit

Quoted 1 line

+ parent = fdt_parent_offset(blob, node);

This function is very slow as it must scan the whole tree. Can we instead pass in the parent node?

Also, how about (in addition) a
version of this function that works for devices? Like:

device_get_addr_size(struct udevice *dev, ...)

so that it can handle this for you.

From: Thierry Reding <[email protected]>

Signed-off-by Thierry Reding <[email protected]>

Signed-off-by Tom Warren <[email protected]>

Signed-off-by Stephen Warren <[email protected]>

---
Simon,

When Thierry first posted this patch, you responded:

Quoted 4 lines

Quoted 1 line

+ parent = fdt_parent_offset(blob, node);

This function is very slow as it must scan the whole tree. Can we instead pass in the parent node?

I don't think that's possible in general. This function is called from fdtdec_get_addr(), and it's easy to find call sites of that function that don't have the parent node available. IIRC, the first couple of example I found scan the DT for a node with a certain compatible value, or look up nodes via aliases, rather than being called while parsing the DT in a top-down tree-like fashion, where the parent node is easily available. I didn't do an exhaustive search after I found a few problematic cases.

Quoted 6 lines

Also, how about (in addition) a
version of this function that works for devices? Like:

device_get_addr_size(struct udevice *dev, ...)

so that it can handle this for you.

That sounds like a separate patch?

Equally, I see that struct udevice contains an of_offset field, but no parent_of_offset or similar. There is a struct udevice *parent though; is the struct udevice hierarchy guaranteed to 100% match the DT hierarchy? I know this isn't necessarily guaranteed in Linux's device model for example.

As such, this patch seems OK to me as-is.

---
 lib/fdtdec.c | 56 ++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 20 deletions(-)

lib/fdtdec.c +36 -21

index 9c6b3619da24..56e72eafaade 100644
@@ -88,29 +88,45 @@ const char *fdtdec_get_compatible(enum fdt_compat_id id)
 fdt_addr_t fdtdec_get_addr_size(const void *blob, int node,
                const char *prop_name, fdt_size_t *sizep)
 {
-       const fdt_addr_t *cell;
-       int len;
+       const fdt32_t *ptr, *end;
+       int parent, na, ns, len;
+       fdt_addr_t addr;
 
        debug("%s: %s: ", __func__, prop_name);
-       cell = fdt_getprop(blob, node, prop_name, &len);
-       if (cell && ((!sizep && len == sizeof(fdt_addr_t)) ||
-                    len == sizeof(fdt_addr_t) * 2)) {
-               fdt_addr_t addr = fdt_addr_to_cpu(*cell);
-               if (sizep) {
-                       const fdt_size_t *size;
-
-                       size = (fdt_size_t *)((char *)cell +
-                                       sizeof(fdt_addr_t));
-                       *sizep = fdt_size_to_cpu(*size);
-                       debug("addr=%08lx, size=%llx\n",
-                             (ulong)addr, (u64)*sizep);
-               } else {
-                       debug("%08lx\n", (ulong)addr);
-               }
-               return addr;
+
+       parent = fdt_parent_offset(blob, node);
+       if (parent < 0) {
+               debug("(no parent found)\n");
+               return FDT_ADDR_T_NONE;
        }
-       debug("(not found)\n");
-       return FDT_ADDR_T_NONE;
+
+       na = fdt_address_cells(blob, parent);
+       ns = fdt_size_cells(blob, parent);
+
+       ptr = fdt_getprop(blob, node, prop_name, &len);
+       if (!ptr) {
+               debug("(not found)\n");
+               return FDT_ADDR_T_NONE;
+       }
+
+       end = ptr + len / sizeof(*ptr);
+
+       if (ptr + na + ns > end) {
+               debug("(not enough data: expected %d bytes, got %d bytes)\n",
+                     (na + ns) * 4, len);
+               return FDT_ADDR_T_NONE;
+       }
+
+       addr = fdtdec_get_number(ptr, na);
+
+       if (sizep) {
+               *sizep = fdtdec_get_number(ptr + na, ns);
+               debug("addr=%pa, size=%pa\n", &addr, sizep);
+       } else {
+               debug("%pa\n", &addr);
+       }
+
+       return addr;
 }
 
 fdt_addr_t fdtdec_get_addr(const void *blob, int node,
--

1.9.1

Hi,

Quoted 23 lines · Stephen Warren

From: Thierry Reding <[email protected]>

Signed-off-by Thierry Reding <[email protected]>

Signed-off-by Tom Warren <[email protected]>

Signed-off-by Stephen Warren <[email protected]>

---
Simon,

When Thierry first posted this patch, you responded:

Quoted 4 lines

Quoted 1 line

+ parent = fdt_parent_offset(blob, node);

This function is very slow as it must scan the whole tree. Can we instead pass in the parent node?

I don't think that's possible in general. This function is called from fdtdec_get_addr(), and it's easy to find call sites of that function that don't have the parent node available. IIRC, the first couple of example I found scan the DT for a node with a certain compatible value, or look up nodes via aliases, rather than being called while parsing the DT in a top-down tree-like fashion, where the parent node is easily available. I didn't do an exhaustive search after I found a few problematic cases.

Quoted 8 lines

Quoted 6 lines

Also, how about (in addition) a
version of this function that works for devices? Like:

device_get_addr_size(struct udevice *dev, ...)

so that it can handle this for you.

That sounds like a separate patch?

Yes, but I think we should get it in there so that people don't start using this (wildly inefficient) function when they don't need to. I think by passing in the parent node we force people to think about the cost.

Yes the driver model function can be a separate patch, but let's get it in at about the same time. We have dev_get_addr() so could have dev_get_addr_size().

Quoted 1 line

Quoted 5 lines

Equally, I see that struct udevice contains an of_offset field, but no parent_of_offset or similar. There is a struct udevice *parent though; is the struct udevice hierarchy guaranteed to 100% match the DT hierarchy? I know this isn't necessarily guaranteed in Linux's device model for example.

Yes it is 100% guaranteed, so dev->parent->of_offset will do the right thing.

Quoted 6 lines

As such, this patch seems OK to me as-is.

---
 lib/fdtdec.c | 56 ++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 20 deletions(-)

Quoted 69 lines

lib/fdtdec.c +36 -20

index 9c6b3619da24..56e72eafaade 100644
@@ -88,29 +88,45 @@ const char *fdtdec_get_compatible(enum fdt_compat_id id)
 fdt_addr_t fdtdec_get_addr_size(const void *blob, int node,
                const char *prop_name, fdt_size_t *sizep)
 {
-       const fdt_addr_t *cell;
-       int len;
+       const fdt32_t *ptr, *end;
+       int parent, na, ns, len;
+       fdt_addr_t addr;
 
        debug("%s: %s: ", __func__, prop_name);
-       cell = fdt_getprop(blob, node, prop_name, &len);
-       if (cell && ((!sizep && len == sizeof(fdt_addr_t)) ||
-                    len == sizeof(fdt_addr_t) * 2)) {
-               fdt_addr_t addr = fdt_addr_to_cpu(*cell);
-               if (sizep) {
-                       const fdt_size_t *size;
-
-                       size = (fdt_size_t *)((char *)cell +
-                                       sizeof(fdt_addr_t));
-                       *sizep = fdt_size_to_cpu(*size);
-                       debug("addr=%08lx, size=%llx\n",
-                             (ulong)addr, (u64)*sizep);
-               } else {
-                       debug("%08lx\n", (ulong)addr);
-               }
-               return addr;
+
+       parent = fdt_parent_offset(blob, node);
+       if (parent < 0) {
+               debug("(no parent found)\n");
+               return FDT_ADDR_T_NONE;
        }
-       debug("(not found)\n");
-       return FDT_ADDR_T_NONE;
+
+       na = fdt_address_cells(blob, parent);
+       ns = fdt_size_cells(blob, parent);
+
+       ptr = fdt_getprop(blob, node, prop_name, &len);
+       if (!ptr) {
+               debug("(not found)\n");
+               return FDT_ADDR_T_NONE;
+       }
+
+       end = ptr + len / sizeof(*ptr);
+
+       if (ptr + na + ns > end) {
+               debug("(not enough data: expected %d bytes, got %d bytes)\n",
+                     (na + ns) * 4, len);
+               return FDT_ADDR_T_NONE;
+       }
+
+       addr = fdtdec_get_number(ptr, na);
+
+       if (sizep) {
+               *sizep = fdtdec_get_number(ptr + na, ns);
+               debug("addr=%pa, size=%pa\n", &addr, sizep);
+       } else {
+               debug("%pa\n", &addr);
+       }
+
+       return addr;
 }
 

Quoted 4 lines

lib/fdtdec.c +0 -1

 fdt_addr_t fdtdec_get_addr(const void *blob, int node,
--

1.9.1

Hi,

Quoted 45 lines · Simon Glass

Hi,

Quoted 23 lines · Stephen Warren

From: Thierry Reding <[email protected]>

Signed-off-by Thierry Reding <[email protected]>

Signed-off-by Tom Warren <[email protected]>

Signed-off-by Stephen Warren <[email protected]>

---
Simon,

When Thierry first posted this patch, you responded:

Quoted 4 lines

Quoted 1 line

+ parent = fdt_parent_offset(blob, node);

This function is very slow as it must scan the whole tree. Can we instead pass in the parent node?

I don't think that's possible in general. This function is called from fdtdec_get_addr(), and it's easy to find call sites of that function that don't have the parent node available. IIRC, the first couple of example I found scan the DT for a node with a certain compatible value, or look up nodes via aliases, rather than being called while parsing the DT in a top-down tree-like fashion, where the parent node is easily available. I didn't do an exhaustive search after I found a few problematic cases.

Quoted 8 lines

Quoted 6 lines

Also, how about (in addition) a
version of this function that works for devices? Like:

device_get_addr_size(struct udevice *dev, ...)

so that it can handle this for you.

That sounds like a separate patch?

Yes, but I think we should get it in there so that people don't start using this (wildly inefficient) function when they don't need to. I think by passing in the parent node we force people to think about the cost.

Yes the driver model function can be a separate patch, but let's get it in at about the same time. We have dev_get_addr() so could have dev_get_addr_size().

Quoted 1 line

Quoted 14 lines

Quoted 5 lines

Equally, I see that struct udevice contains an of_offset field, but no parent_of_offset or similar. There is a struct udevice *parent though; is the struct udevice hierarchy guaranteed to 100% match the DT hierarchy? I know this isn't necessarily guaranteed in Linux's device model for example.

Yes it is 100% guaranteed, so dev->parent->of_offset will do the right thing.

Quoted 6 lines

As such, this patch seems OK to me as-is.

---
 lib/fdtdec.c | 56 ++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 20 deletions(-)

This patch has been applied. I'm going to post a revert of this patch. Please can you take a look at the comments above? In particular this function is called from dev_get_addr() which is a core driver model function. It needs to be fast - and in fact dev_get_addr() already has access to the parent node.

Also looking a bit closer this patch does a lot more than 'fix it for 64-bit'. A commit message would be useful to explain what problems it is fixing, etc.

Another point is that fdt_addr_t and fdt_size_t are supposed to match the address size used in the device tree. Is that not the case with Tegra210?

So what this does is really fix parsing of address and size cells in the general case, though it would still fail for values of #address-cells or #size-cells bigger than 2 (because we don't have a datatype that would be able to contain such large values).

Note that there's also still a corner case that this doesn't handle. The DT specification states, if I remember correctly, that #address-cells and #size-cells are inherited. That means with the current code we will wrongly parse something like this:

	/ {
		...
		#address-cells = <1>;
		#size-cells = <1>;
		...
		bus at XXXXXXXX {
			...
			device at XXXXXXXX {
				...
				reg = <0xXXXXXXXX 0x1000>;
				...
			};
			...
		};
		...
	};

According to the DT specification the bus at XXXXXXXX node would inherit #address-cells = <1> and #size-cells = <1> from the root node. However with libfdt what really happens is that since bus at XXXXXXXX does not have either property it will default to 2 in both cases. I'm not sure if this really is a problem. Typically nodes are not nested that deeply, or if they are then, typically, they explicitly contain #address-cells and #size-cells properties.

On 08/04/2015 08:26 AM, Thierry Reding wrote:
... [ discussion of new fdtdec_get_addr_size() implementation]

Quoted 27 lines

So what this does is really fix parsing of address and size cells in the general case, though it would still fail for values of #address-cells or #size-cells bigger than 2 (because we don't have a datatype that would be able to contain such large values).

Note that there's also still a corner case that this doesn't handle. The DT specification states, if I remember correctly, that #address-cells and #size-cells are inherited. That means with the current code we will wrongly parse something like this:

	/ {
		...
		#address-cells = <1>;
		#size-cells = <1>;
		...
		bus at XXXXXXXX {
			...
			device at XXXXXXXX {
				...
				reg = <0xXXXXXXXX 0x1000>;
				...
			};
			...
		};
		...
	};

Quoted 7 lines

According to the DT specification the bus at XXXXXXXX node would inherit #address-cells = <1> and #size-cells = <1> from the root node. However with libfdt what really happens is that since bus at XXXXXXXX does not have either property it will default to 2 in both cases. I'm not sure if this really is a problem. Typically nodes are not nested that deeply, or if they are then, typically, they explicitly contain #address-cells and #size-cells properties.

I don't think #address-cells/#size-cells do actually get inherited. Admittedly some other properties (e.g. interrupt-parent) do, but according to:

https://lists.ozlabs.org/pipermail/linuxppc-dev/2008-January/049113.html
[PATCH] powerpc: #address-cells & #size-cells properties not inherited

... and my vague memory, these two don't.

You can search Google for e.g. "#address-cells inherited" and find a number of similar assertions.