Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

PATCH New Simon Glass 8 participants 34 messages Aug 3, 2015 – Sep 17, 2015 Library code

lore

Share & tools

lore

Simon Glass

author

2015-08-03 00:13 UTC #0 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

This reverts commit 5b34436035fc862b5e8d0d2c3eab74ba36f1a7f4.

This function has a few problems. It calls fdt_parent_offset() which as mentioned in code review is very slow.

https://patchwork.ozlabs.org/patch/499482/
https://patchwork.ozlabs.org/patch/452604/

It also happens to break SPI flash on Minnowboard max which is how I noticed that this was applied. I can send a patch to tidy that up, but in any case I think we should consider a revert until the function is better implemented.

Signed-off-by Simon Glass <[email protected]>

---

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

lib/fdtdec.c +20 ~~-36~~

index a954051..aac4f8d 100644
@@ -90,45 +90,29 @@ 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 fdt32_t *ptr, *end;
-	int parent, na, ns, len;
-	fdt_addr_t addr;
+	const fdt_addr_t *cell;
+	int len;
 
 	debug("%s: %s: ", __func__, prop_name);
-
-	parent = fdt_parent_offset(blob, node);
-	if (parent < 0) {
-		debug("(no parent 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);
+	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;
 	}
-
-	return addr;
+	debug("(not found)\n");
+	return FDT_ADDR_T_NONE;
 }
 
 fdt_addr_t fdtdec_get_addr(const void *blob, int node,

Signature · 1 line

2.5.0.rc2.392.g76e840b

Stephen Warren

2015-08-03 15:12 UTC #1 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Quoted 11 lines · Simon Glass

This reverts commit 5b34436035fc862b5e8d0d2c3eab74ba36f1a7f4.
This function has a few problems. It calls fdt_parent_offset() which as mentioned in code review is very slow.
https://patchwork.ozlabs.org/patch/499482/
https://patchwork.ozlabs.org/patch/452604/
It also happens to break SPI flash on Minnowboard max which is how I noticed that this was applied. I can send a patch to tidy that up, but in any case I think we should consider a revert until the function is better implemented.

The fact that the function needs to perform a slow operation is not a good reason for a revert. The slowness of the operation is just a matter of fact with DT not having uplinks in its data structure, and U-Boot using those data structures directly.

You'd requested during review that I additionally implement a faster version of the function in the case where the parent node is already known, and said it was fine if that happened in a later patch. I have this on my TODO list, but it's only been a couple of days.

Why not just fix the bug since you said you could? That seems far better than breaking the newly added Tegra210 support.

P.S. What is the issue with SPI flash? The commit description doesn't mention this at all.

Simon Glass

author

2015-08-03 15:52 UTC #2 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Hi Stephen,

Quoted 22 lines · Stephen Warren

Quoted 6 lines · Simon Glass
This reverts commit 5b34436035fc862b5e8d0d2c3eab74ba36f1a7f4.
This function has a few problems. It calls fdt_parent_offset() which as mentioned in code review is very slow.
Quoted 8 lines
https://patchwork.ozlabs.org/patch/499482/
https://patchwork.ozlabs.org/patch/452604/
It also happens to break SPI flash on Minnowboard max which is how I noticed
that this was applied. I can send a patch to tidy that up, but in any case I think we should consider a revert until the function is better implemented.
The fact that the function needs to perform a slow operation is not a good reason for a revert. The slowness of the operation is just a matter of fact with DT not having uplinks in its data structure, and U-Boot using those data structures directly.

Quoted 4 lines

You'd requested during review that I additionally implement a faster version of the function in the case where the parent node is already known, and said it was fine if that happened in a later patch. I have this on my TODO list, but it's only been a couple of days.

I didn't expect this to go to mainline before your new patch.

Quoted 3 lines

Why not just fix the bug since you said you could? That seems far better than breaking the newly added Tegra210 support.

I do have a patch but I'm not 100% comfortable with the approach. I don't see a good reason to move away from the idea of fdt_addr_t and fdt_addr_t being set correctly for the platform. Or maybe I misunderstand the problem the patch was trying to fix. As I said it did not have a commit message, so who knows :-)

Quoted 3 lines

P.S. What is the issue with SPI flash? The commit description doesn't mention this at all.

It calls that function expecting it to pick up an address and size from two consecutive cells. With this patch, that fails (unless the property happens to be "reg").

Actually I do favour a revert as it will allow us to discuss the fix with a clean slate.

Regards,
Simon

Tom Rini

2015-08-03 17:25 UTC #3 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Quoted 46 lines · Simon Glass

Hi Stephen,
Quoted 22 lines · Stephen Warren
Quoted 6 lines · Simon Glass
This reverts commit 5b34436035fc862b5e8d0d2c3eab74ba36f1a7f4.
This function has a few problems. It calls fdt_parent_offset() which as mentioned in code review is very slow.
Quoted 8 lines
https://patchwork.ozlabs.org/patch/499482/
https://patchwork.ozlabs.org/patch/452604/
It also happens to break SPI flash on Minnowboard max which is how I noticed
that this was applied. I can send a patch to tidy that up, but in any case I think we should consider a revert until the function is better implemented.
The fact that the function needs to perform a slow operation is not a good reason for a revert. The slowness of the operation is just a matter of fact with DT not having uplinks in its data structure, and U-Boot using those data structures directly.
Quoted 4 lines
You'd requested during review that I additionally implement a faster version of the function in the case where the parent node is already known, and said it was fine if that happened in a later patch. I have this on my TODO list, but it's only been a couple of days.
I didn't expect this to go to mainline before your new patch.
Quoted 3 lines
Why not just fix the bug since you said you could? That seems far better than breaking the newly added Tegra210 support.
I do have a patch but I'm not 100% comfortable with the approach. I don't see a good reason to move away from the idea of fdt_addr_t and fdt_addr_t being set correctly for the platform. Or maybe I misunderstand the problem the patch was trying to fix. As I said it did not have a commit message, so who knows :-)
Quoted 3 lines
P.S. What is the issue with SPI flash? The commit description doesn't mention this at all.

Quoted 6 lines

It calls that function expecting it to pick up an address and size from two consecutive cells. With this patch, that fails (unless the property happens to be "reg").
Actually I do favour a revert as it will allow us to discuss the fix with a clean slate.

Well, we can unbreak one set of stuff and break another set of stuff, that's not a clean slate for the now broken platforms. If it helps to come up with a better patch we can step one revert and step two better, but I don't want to apply without step two ready.

... speaking as someone who has people poking me about if the SPI flasher they sent me works so I can try U-Boot on x86 things.

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150803/3394b6c4/attachment.sig>

Simon Glass

author

2015-08-03 17:27 UTC #4 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Hi Tom.

Quoted 54 lines · Tom Rini

Quoted 46 lines · Simon Glass
Hi Stephen,
Quoted 22 lines · Stephen Warren
Quoted 6 lines · Simon Glass
This reverts commit 5b34436035fc862b5e8d0d2c3eab74ba36f1a7f4.
This function has a few problems. It calls fdt_parent_offset() which as mentioned in code review is very slow.
Quoted 8 lines
https://patchwork.ozlabs.org/patch/499482/
https://patchwork.ozlabs.org/patch/452604/
It also happens to break SPI flash on Minnowboard max which is how I noticed
that this was applied. I can send a patch to tidy that up, but in any case I think we should consider a revert until the function is better implemented.
The fact that the function needs to perform a slow operation is not a good reason for a revert. The slowness of the operation is just a matter of fact with DT not having uplinks in its data structure, and U-Boot using those data structures directly.
Quoted 4 lines
You'd requested during review that I additionally implement a faster version of the function in the case where the parent node is already known, and said it was fine if that happened in a later patch. I have this on my TODO list, but it's only been a couple of days.
I didn't expect this to go to mainline before your new patch.
Quoted 3 lines
Why not just fix the bug since you said you could? That seems far better than breaking the newly added Tegra210 support.
I do have a patch but I'm not 100% comfortable with the approach. I don't see a good reason to move away from the idea of fdt_addr_t and fdt_addr_t being set correctly for the platform. Or maybe I misunderstand the problem the patch was trying to fix. As I said it did not have a commit message, so who knows :-)
Quoted 3 lines
P.S. What is the issue with SPI flash? The commit description doesn't mention this at all.
Quoted 6 lines
It calls that function expecting it to pick up an address and size from two consecutive cells. With this patch, that fails (unless the property happens to be "reg").
Actually I do favour a revert as it will allow us to discuss the fix with a clean slate.

Quoted 7 lines

Well, we can unbreak one set of stuff and break another set of stuff, that's not a clean slate for the now broken platforms. If it helps to come up with a better patch we can step one revert and step two better, but I don't want to apply without step two ready.
... speaking as someone who has people poking me about if the SPI flasher they sent me works so I can try U-Boot on x86 things.

Fair enough - I don't want to break things either. Perhaps we can apply a revert and the new patch one after the other. But really however we can fix this is fine with me.

Regards,
Simon

Stephen Warren

2015-08-03 18:20 UTC #5 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

In reply to Simon Glass

Quoted 25 lines · Simon Glass

Hi Stephen,
Quoted 22 lines · Stephen Warren
Quoted 6 lines · Simon Glass
This reverts commit 5b34436035fc862b5e8d0d2c3eab74ba36f1a7f4.
This function has a few problems. It calls fdt_parent_offset() which as mentioned in code review is very slow.
Quoted 8 lines
https://patchwork.ozlabs.org/patch/499482/
https://patchwork.ozlabs.org/patch/452604/
It also happens to break SPI flash on Minnowboard max which is how I noticed
that this was applied. I can send a patch to tidy that up, but in any case I think we should consider a revert until the function is better implemented.
The fact that the function needs to perform a slow operation is not a good reason for a revert. The slowness of the operation is just a matter of fact with DT not having uplinks in its data structure, and U-Boot using those data structures directly.

Quoted 6 lines

Quoted 4 lines
You'd requested during review that I additionally implement a faster version of the function in the case where the parent node is already known, and said it was fine if that happened in a later patch. I have this on my TODO list, but it's only been a couple of days.
I didn't expect this to go to mainline before your new patch.

I didn't get that message from the thread; you wrote:

Quoted 11 lines · Simon Glass

Quoted 9 lines · Stephen Warren
Quoted 6 lines · Simon Glass
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?

Quoted 8 lines

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

That sounds to like you'd like a followup patch soon after this patch (my assumption would be within a few days or weeks) to solve your comments, not that you wanted a replacement patch.

Quoted 8 lines

Quoted 2 lines
Why not just fix the bug since you said you could? That seems far better than breaking the newly added Tegra210 support.
I do have a patch but I'm not 100% comfortable with the approach. I don't see a good reason to move away from the idea of fdt_addr_t and fdt_addr_t being set correctly for the platform. Or maybe I misunderstand the problem the patch was trying to fix. As I said it did not have a commit message, so who knows :-)

The size of fdt_addr_t isn't relevant *at all* when parsing DT. The only thing that matters is #address-cells/#size-cells. Those properties are what tell the parsing code how many bytes to read from the reg property. Whether the resultant value fits into the code's internal representation is an internal detail of the code, not part of the semantics of DT itself or how to parse it.

If code assumes that #address-cells == sizeof(fdt_addr_t), it is indeed quite likely that everything will just happen to work most of the time. However, this is purely an accident and not something that anything should rely upon.

(I think Tegra210 support still has CONFIG_PHYS_64BIT undefined which is admittedly a little /unexpected/ for a 64-bit U-Boot build, but in practice is perfectly /legal/ and will work out just fine since, except perhaps for RAM sizes, I don't believe any value in DT will actually require more than 32-bits to represent)

Quoted 6 lines

Quoted 2 lines
P.S. What is the issue with SPI flash? The commit description doesn't mention this at all.
It calls that function expecting it to pick up an address and size from two consecutive cells. With this patch, that fails (unless the property happens to be "reg").

I don't see anything in the new code that interprets the property name in any way, or compares it against "reg". Can you point out where the code is doing something differently based on property name?

Perhaps you mean instead that something is calling fdtdec_get_addr_size() on a property that's not intended to be affected by #address-call/#size-cells? If so, I would assert that's a bug in the calling code; if fdtdec_get_addr_size() is ever intended to parse the reg property, then fdtdec_get_addr_size() must always honor #address-call/#size-cells since that's how the reg property works. If some code wants to parse a property where #address-call/#size-cells shouldn't be honored, but instead some fixed size assumed, then a different function should be called instead (or perhaps some additional function parameters added to specify override values for #address-call/#size-cells).

I think this is all stemming from drivers/mtd/spi/sf_probe.c spi_flash_decode_fdt()'s call to fdtdec_get_addr_size() for property "memory-map"? If so, then looking at arch/x86/dts/minnowmax.dts's /spi node, the code and DT content are clearly inconsistent; For this node #address-cells=<1>, #size-cells=<0> which makes sense given that the address is a chip-select and hence has no size. So, the code should not assume that the memory-map property can be parsed in the same way as a reg property.

I note that the memory-map property doesn't exist in the Linux kernel's DT binding documentation database, or code, hence hasn't been reviewed by the DT binding maintainers.

Simon Glass

author

2015-08-05 04:08 UTC #6 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Hi Stephen,

Quoted 64 lines · Stephen Warren

Quoted 41 lines · Simon Glass
Hi Stephen,
Quoted 28 lines · Stephen Warren
Quoted 10 lines · Simon Glass
This reverts commit 5b34436035fc862b5e8d0d2c3eab74ba36f1a7f4.
This function has a few problems. It calls fdt_parent_offset() which as mentioned in code review is very slow.
https://patchwork.ozlabs.org/patch/499482/
https://patchwork.ozlabs.org/patch/452604/
Quoted 6 lines
It also happens to break SPI flash on Minnowboard max which is how I noticed
that this was applied. I can send a patch to tidy that up, but in any case
I think we should consider a revert until the function is better implemented.
The fact that the function needs to perform a slow operation is not a good
reason for a revert. The slowness of the operation is just a matter of fact
with DT not having uplinks in its data structure, and U-Boot using those data structures directly.
Quoted 7 lines
You'd requested during review that I additionally implement a faster version
of the function in the case where the parent node is already known, and said
it was fine if that happened in a later patch. I have this on my TODO list,
but it's only been a couple of days.
Quoted 1 line
I didn't expect this to go to mainline before your new patch.
I didn't get that message from the thread; you wrote:
Quoted 16 lines · Simon Glass
Quoted 9 lines · Stephen Warren
Quoted 6 lines · Simon Glass
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.

Quoted 7 lines

Quoted 3 lines
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().
That sounds to like you'd like a followup patch soon after this patch (my assumption would be within a few days or weeks) to solve your comments, not that you wanted a replacement patch.

I will take that feedback to be a little more forceful in future, sorry.

Quoted 24 lines

Quoted 10 lines
Quoted 3 lines
Why not just fix the bug since you said you could? That seems far better than breaking the newly added Tegra210 support.
I do have a patch but I'm not 100% comfortable with the approach. I don't see a good reason to move away from the idea of fdt_addr_t and fdt_addr_t being set correctly for the platform. Or maybe I misunderstand the problem the patch was trying to fix. As I said it did not have a commit message, so who knows :-)
The size of fdt_addr_t isn't relevant *at all* when parsing DT. The only thing that matters is #address-cells/#size-cells. Those properties are what tell the parsing code how many bytes to read from the reg property. Whether the resultant value fits into the code's internal representation is an internal detail of the code, not part of the semantics of DT itself or how to parse it.
If code assumes that #address-cells == sizeof(fdt_addr_t), it is indeed quite likely that everything will just happen to work most of the time. However, this is purely an accident and not something that anything should rely upon.

Quoted 5 lines

(I think Tegra210 support still has CONFIG_PHYS_64BIT undefined which is admittedly a little /unexpected/ for a 64-bit U-Boot build, but in practice is perfectly /legal/ and will work out just fine since, except perhaps for RAM sizes, I don't believe any value in DT will actually require more than 32-bits to represent)

I would like to have the types match the platform where possible. At least the types should not be smaller than the platform - e.g. if CONFIG_PHYS_64BIT is not defined we should not support 64-bit address/size in the device tree. This is for efficiency. We don't want to force all the U-Boot code to 64-bit suddenly. This will bloat things for no benefit.

Quoted 33 lines

Quoted 8 lines
Quoted 3 lines
P.S. What is the issue with SPI flash? The commit description doesn't mention this at all.
It calls that function expecting it to pick up an address and size from two consecutive cells. With this patch, that fails (unless the property happens to be "reg").
I don't see anything in the new code that interprets the property name in any way, or compares it against "reg". Can you point out where the code is doing something differently based on property name?
Perhaps you mean instead that something is calling fdtdec_get_addr_size() on a property that's not intended to be affected by #address-call/#size-cells? If so, I would assert that's a bug in the calling code; if fdtdec_get_addr_size() is ever intended to parse the reg property, then fdtdec_get_addr_size() must always honor #address-call/#size-cells since that's how the reg property works. If some code wants to parse a property where #address-call/#size-cells shouldn't be honored, but instead some fixed size assumed, then a different function should be called instead (or perhaps some additional function parameters added to specify override values for #address-call/#size-cells).
I think this is all stemming from drivers/mtd/spi/sf_probe.c spi_flash_decode_fdt()'s call to fdtdec_get_addr_size() for property "memory-map"? If so, then looking at arch/x86/dts/minnowmax.dts's /spi node, the code and DT content are clearly inconsistent; For this node #address-cells=<1>, #size-cells=<0> which makes sense given that the address is a chip-select and hence has no size. So, the code should not assume that the memory-map property can be parsed in the same way as a reg property.

Quoted 3 lines

I note that the memory-map property doesn't exist in the Linux kernel's DT binding documentation database, or code, hence hasn't been reviewed by the DT binding maintainers.

The function comment says:

* Look up an address property in a node and return it as an address.
* The property must hold one address with a length. This is only tested
* on 32-bit machines.

My intention was that this would be an efficient way to decode an address and size from a device tree. To some extent regmap may take over this role (IMO we should turn to drop fdtdec one day one we have more infrastructure). But I'd like it to work efficiently for 32-bit machines. The new function could hardly be less efficient.

I think we should consider the case where the physical address size of U-Boot and the device tree do not match as a corner case. I certainly don't want device tree to add loads of pointless code for 'normal' platforms.

Re memory-map, yes it doesn't seem to be possible to do what it is trying to do (and Thierry says the same below). It is quite weird to have a SPI peripheral which is also memory mapped.

Here's my question - if you fix the CONFIG_PHYS_64BIT problem with 64-bit Tegra, what is actually wrong with the way the function was? This is a boot loader so we should be willing to make some simplifications.

Regards,
Simon

Stephen Warren

2015-08-05 18:22 UTC #7 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Quoted 107 lines · Simon Glass

Hi Stephen,
Quoted 64 lines · Stephen Warren
Quoted 41 lines · Simon Glass
Hi Stephen,
Quoted 28 lines · Stephen Warren
Quoted 10 lines · Simon Glass
This reverts commit 5b34436035fc862b5e8d0d2c3eab74ba36f1a7f4.
This function has a few problems. It calls fdt_parent_offset() which as mentioned in code review is very slow.
https://patchwork.ozlabs.org/patch/499482/
https://patchwork.ozlabs.org/patch/452604/
Quoted 6 lines
It also happens to break SPI flash on Minnowboard max which is how I noticed
that this was applied. I can send a patch to tidy that up, but in any case
I think we should consider a revert until the function is better implemented.
The fact that the function needs to perform a slow operation is not a good
reason for a revert. The slowness of the operation is just a matter of fact
with DT not having uplinks in its data structure, and U-Boot using those data structures directly.
Quoted 7 lines
You'd requested during review that I additionally implement a faster version
of the function in the case where the parent node is already known, and said
it was fine if that happened in a later patch. I have this on my TODO list,
but it's only been a couple of days.
Quoted 1 line
I didn't expect this to go to mainline before your new patch.
I didn't get that message from the thread; you wrote:
Quoted 16 lines · Simon Glass
Quoted 9 lines · Stephen Warren
Quoted 6 lines · Simon Glass
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.
Quoted 7 lines
Quoted 3 lines
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().
That sounds to like you'd like a followup patch soon after this patch (my assumption would be within a few days or weeks) to solve your comments, not that you wanted a replacement patch.
I will take that feedback to be a little more forceful in future, sorry.
Quoted 24 lines
Quoted 10 lines
Quoted 3 lines
Why not just fix the bug since you said you could? That seems far better than breaking the newly added Tegra210 support.
I do have a patch but I'm not 100% comfortable with the approach. I don't see a good reason to move away from the idea of fdt_addr_t and fdt_addr_t being set correctly for the platform. Or maybe I misunderstand the problem the patch was trying to fix. As I said it did not have a commit message, so who knows :-)
The size of fdt_addr_t isn't relevant *at all* when parsing DT. The only thing that matters is #address-cells/#size-cells. Those properties are what tell the parsing code how many bytes to read from the reg property. Whether the resultant value fits into the code's internal representation is an internal detail of the code, not part of the semantics of DT itself or how to parse it.
If code assumes that #address-cells == sizeof(fdt_addr_t), it is indeed quite likely that everything will just happen to work most of the time. However, this is purely an accident and not something that anything should rely upon.
Quoted 5 lines
(I think Tegra210 support still has CONFIG_PHYS_64BIT undefined which is admittedly a little /unexpected/ for a 64-bit U-Boot build, but in practice is perfectly /legal/ and will work out just fine since, except perhaps for RAM sizes, I don't believe any value in DT will actually require more than 32-bits to represent)

Quoted 3 lines

I would like to have the types match the platform where possible. At least the types should not be smaller than the platform - e.g. if CONFIG_PHYS_64BIT is not defined we should not support 64-bit

In general, there's no "should not" here; we "cannot". If there's a 64-bit value in the DT (with bits above bit 31 set), then it can't be stored in a 32-bit variable.

That said, if a DT has #address-cells=<2>, but the actual values stored in all reg values have 0 for the MS word, that'll actually work just fine. Note that it's 100% legal to set #address-cells=<100> and just write a bunch of extra zeros into the property. Silly perhaps, but perfectly legal. Since the function should adapt to whatever #address-cells value is in the DT, supporting that case isn't any more work, so there's no reason we shouldn't.

Quoted 3 lines

address/size in the device tree. This is for efficiency. We don't want to force all the U-Boot code to 64-bit suddenly. This will bloat things for no benefit.

We could and likely should set CONFIG_PHYS_64BIT for Tegra210. However, that's unrelated to using the correct algorithm to parse DT.

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
P.S. What is the issue with SPI flash? The commit description doesn't mention this at all.
It calls that function expecting it to pick up an address and size from two consecutive cells. With this patch, that fails (unless the property happens to be "reg").

...

Quoted 12 lines

Quoted 11 lines
I think this is all stemming from drivers/mtd/spi/sf_probe.c spi_flash_decode_fdt()'s call to fdtdec_get_addr_size() for property "memory-map"? If so, then looking at arch/x86/dts/minnowmax.dts's /spi node, the code and DT content are clearly inconsistent; For this node #address-cells=<1>, #size-cells=<0> which makes sense given that the address is a chip-select and hence has no size. So, the code should not assume that the memory-map property can be parsed in the same way as a reg property.
I note that the memory-map property doesn't exist in the Linux kernel's DT binding documentation database, or code, hence hasn't been reviewed by the DT binding maintainers.

Quoted 5 lines

The function comment says:
* Look up an address property in a node and return it as an address.
* The property must hold one address with a length. This is only tested
* on 32-bit machines.

And Thierry fixed it for systems with #address-cells > 1. Perhaps that part of the function comment should have been removed in the commit.

Quoted 5 lines

My intention was that this would be an efficient way to decode an address and size from a device tree. To some extent regmap may take over this role (IMO we should turn to drop fdtdec one day one we have more infrastructure). But I'd like it to work efficiently for 32-bit machines. The new function could hardly be less efficient.

Again, there's no way in general to make it more efficient. The efficiency issue is directly implied by the DT data structures.

In the special case where the parent node is already known, we can certainly introduce an alternate function that is more efficient. You've already asked for that, and as I said, it's in my TODO list.

Quoted 4 lines

I think we should consider the case where the physical address size of U-Boot and the device tree do not match as a corner case. I certainly don't want device tree to add loads of pointless code for 'normal' platforms.

It's not a corner case. It's a fundamental part of the DT schema. If U-Boot is going to use DT, it should actually use *DT*, not some-very similar-but-not-quite-DT format with all kinds of implicit and unstated exceptions, limitations, and assumptions. This implies fully honoring all aspects of how DT works when parsing it, not picking and choosing features because some are inconvenient or annoying. If U-Boot doesn't want to correctly implement DT support, it should just drop it completely.

As an aside, when instantiating devices, I hope one day that U-Boot will parse DT top-down in a hierarchical way, rather than simply searching for any node anywhere in the DT without regard for whether any parent node is enabled, has a driver, has had the driver initialize, etc. U-Boot ignores this right now, and is only getting away with this accidentally. Without hacky workarounds in drivers, this won't continue to work for all Tegra HW (e.g. host1x graphics/display sub-modules, AHUB's audio-related sub-modules), since parent drivers must initialize before child drivers in order to enable various register buses, including clocks/resets affecting those buses etc.

Again, if it's simply too inconvenient or bloated to implement DT properly in U-Boot, let's just drop it entirely. A halfway solution is the worst of both worlds. I'm not convinced the full implications of how to (and the need to) correctly and fully support DT have were well thought through before (control) DT support was added to U-Boot.

Quoted 6 lines

Re memory-map, yes it doesn't seem to be possible to do what it is trying to do (and Thierry says the same below). It is quite weird to have a SPI peripheral which is also memory mapped.
Here's my question - if you fix the CONFIG_PHYS_64BIT problem with 64-bit Tegra, what is actually wrong with the way the function was?

With the DT files now checked into U-Boot, I think it would accidentally work, since we just happen to have set #address-cells=<2>, #size-cells=<2>, and that would just happen to match sizeof(fdt_addr_t).

However note this is an accident on a couple of levels:

a) This is because the code assumes that sizeof(fdt_addr_t) == #address-cells * 4. This is an invalid assumption since it does not correctly honor the DT schema. It hard-codes the size of values whereas DT schema says the size is defined by the #xxx-cells properties.

b) The original Tegra210 DTs that TomW posted had #address-cells=<1>, #size-cells=<1>. I asked him to change that to match what I expected to be in the Linux kernel's Tegra210 DTs. However, if he'd rejected my request or I hadn't noticed that, then with CONFIG_PHYS_64BIT set, fdtdec_get_addr_size() would have attempted to read twice as many bytes as it should have from the property. It's entirely plausible that someone could have come along later and realized CONFIG_PHYS_64BIT was set incorrectly and enabled it.

Quoted 2 lines

This is a boot loader so we should be willing to make some simplifications.

When dealing with internal bootloader details, sure assumptions, simplifications, etc. can be made.

However, DT is an externally defined standard. The content of DT must be identical across all OSs (SW stacks, bootloader) and not influenced by requirements/... of any specific individual OS's (SW stack, bootloader) quirks. We can't just pick and choose which parts of it we care about. Well, perhaps if we stop calling it DT we could.

Simon Glass

author

2015-08-05 23:45 UTC #8 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Hi Stephen,

Quoted 276 lines · Stephen Warren

Quoted 136 lines · Simon Glass
Hi Stephen,
Quoted 83 lines · Stephen Warren
Quoted 48 lines · Simon Glass
Hi Stephen,
Quoted 33 lines · Stephen Warren
Quoted 12 lines · Simon Glass
This reverts commit 5b34436035fc862b5e8d0d2c3eab74ba36f1a7f4.
This function has a few problems. It calls fdt_parent_offset() which as
mentioned in code review is very slow.
https://patchwork.ozlabs.org/patch/499482/
https://patchwork.ozlabs.org/patch/452604/
Quoted 6 lines
It also happens to break SPI flash on Minnowboard max which is how I noticed
that this was applied. I can send a patch to tidy that up, but in any case
I think we should consider a revert until the function is better implemented.
The fact that the function needs to perform a slow operation is not a good
reason for a revert. The slowness of the operation is just a matter of fact
with DT not having uplinks in its data structure, and U-Boot using those
data structures directly.
Quoted 7 lines
You'd requested during review that I additionally implement a faster version
of the function in the case where the parent node is already known, and said
it was fine if that happened in a later patch. I have this on my TODO list,
but it's only been a couple of days.
Quoted 1 line
I didn't expect this to go to mainline before your new patch.
I didn't get that message from the thread; you wrote:
Quoted 16 lines · Simon Glass
Quoted 6 lines · Stephen Warren
Quoted 4 lines · Simon Glass
Also, how about (in addition) a
version of this function that works for devices? Like:
Quoted 6 lines
Quoted 3 lines
device_get_addr_size(struct udevice *dev, ...)
so that it can handle this for you.
That sounds like a separate patch?
Quoted 8 lines
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 4 lines
That sounds to like you'd like a followup patch soon after this patch (my assumption would be within a few days or weeks) to solve your comments, not
that you wanted a replacement patch.
I will take that feedback to be a little more forceful in future, sorry.
Quoted 31 lines
Quoted 7 lines
Quoted 4 lines
Why not just fix the bug since you said you could? That seems far better
than breaking the newly added Tegra210 support.
Quoted 5 lines
I do have a patch but I'm not 100% comfortable with the approach. I don't see a good reason to move away from the idea of fdt_addr_t and fdt_addr_t being set correctly for the platform. Or maybe I misunderstand the problem the patch was trying to fix. As I said it did not have a commit message, so who knows :-)
The size of fdt_addr_t isn't relevant *at all* when parsing DT. The only thing that matters is #address-cells/#size-cells. Those properties are what
tell the parsing code how many bytes to read from the reg property. Whether
the resultant value fits into the code's internal representation is an internal detail of the code, not part of the semantics of DT itself or how
to parse it.
If code assumes that #address-cells == sizeof(fdt_addr_t), it is indeed quite likely that everything will just happen to work most of the time. However, this is purely an accident and not something that anything should
rely upon.
Quoted 8 lines
(I think Tegra210 support still has CONFIG_PHYS_64BIT undefined which is admittedly a little /unexpected/ for a 64-bit U-Boot build, but in practice
is perfectly /legal/ and will work out just fine since, except perhaps for
RAM sizes, I don't believe any value in DT will actually require more than
32-bits to represent)
Quoted 3 lines
I would like to have the types match the platform where possible. At least the types should not be smaller than the platform - e.g. if CONFIG_PHYS_64BIT is not defined we should not support 64-bit
In general, there's no "should not" here; we "cannot". If there's a 64-bit value in the DT (with bits above bit 31 set), then it can't be stored in a 32-bit variable.
That said, if a DT has #address-cells=<2>, but the actual values stored in all reg values have 0 for the MS word, that'll actually work just fine. Note that it's 100% legal to set #address-cells=<100> and just write a bunch of extra zeros into the property. Silly perhaps, but perfectly legal. Since the function should adapt to whatever #address-cells value is in the DT, supporting that case isn't any more work, so there's no reason we shouldn't.
Quoted 3 lines
address/size in the device tree. This is for efficiency. We don't want to force all the U-Boot code to 64-bit suddenly. This will bloat things for no benefit.
We could and likely should set CONFIG_PHYS_64BIT for Tegra210. However, that's unrelated to using the correct algorithm to parse DT.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
P.S. What is the issue with SPI flash? The commit description doesn't mention this at all.
It calls that function expecting it to pick up an address and size from two consecutive cells. With this patch, that fails (unless the property happens to be "reg").
...
Quoted 19 lines
Quoted 12 lines
I think this is all stemming from drivers/mtd/spi/sf_probe.c spi_flash_decode_fdt()'s call to fdtdec_get_addr_size() for property "memory-map"? If so, then looking at arch/x86/dts/minnowmax.dts's /spi node,
the code and DT content are clearly inconsistent; For this node #address-cells=<1>, #size-cells=<0> which makes sense given that the address
is a chip-select and hence has no size. So, the code should not assume that
the memory-map property can be parsed in the same way as a reg property.
Quoted 5 lines
I note that the memory-map property doesn't exist in the Linux kernel's DT
binding documentation database, or code, hence hasn't been reviewed by the
DT binding maintainers.
Quoted 5 lines
The function comment says:
* Look up an address property in a node and return it as an address.
* The property must hold one address with a length. This is only tested
* on 32-bit machines.
And Thierry fixed it for systems with #address-cells > 1. Perhaps that part of the function comment should have been removed in the commit.
Quoted 5 lines
My intention was that this would be an efficient way to decode an address and size from a device tree. To some extent regmap may take over this role (IMO we should turn to drop fdtdec one day one we have more infrastructure). But I'd like it to work efficiently for 32-bit machines. The new function could hardly be less efficient.
Again, there's no way in general to make it more efficient. The efficiency issue is directly implied by the DT data structures.
In the special case where the parent node is already known, we can certainly introduce an alternate function that is more efficient. You've already asked for that, and as I said, it's in my TODO list.
Quoted 4 lines
I think we should consider the case where the physical address size of U-Boot and the device tree do not match as a corner case. I certainly don't want device tree to add loads of pointless code for 'normal' platforms.
It's not a corner case. It's a fundamental part of the DT schema. If U-Boot is going to use DT, it should actually use *DT*, not some-very similar-but-not-quite-DT format with all kinds of implicit and unstated exceptions, limitations, and assumptions. This implies fully honoring all aspects of how DT works when parsing it, not picking and choosing features because some are inconvenient or annoying. If U-Boot doesn't want to correctly implement DT support, it should just drop it completely.
As an aside, when instantiating devices, I hope one day that U-Boot will parse DT top-down in a hierarchical way, rather than simply searching for any node anywhere in the DT without regard for whether any parent node is enabled, has a driver, has had the driver initialize, etc. U-Boot ignores this right now, and is only getting away with this accidentally. Without hacky workarounds in drivers, this won't continue to work for all Tegra HW (e.g. host1x graphics/display sub-modules, AHUB's audio-related sub-modules), since parent drivers must initialize before child drivers in order to enable various register buses, including clocks/resets affecting those buses etc.
Again, if it's simply too inconvenient or bloated to implement DT properly in U-Boot, let's just drop it entirely. A halfway solution is the worst of both worlds. I'm not convinced the full implications of how to (and the need to) correctly and fully support DT have were well thought through before (control) DT support was added to U-Boot.
Quoted 6 lines
Re memory-map, yes it doesn't seem to be possible to do what it is trying to do (and Thierry says the same below). It is quite weird to have a SPI peripheral which is also memory mapped.
Here's my question - if you fix the CONFIG_PHYS_64BIT problem with 64-bit Tegra, what is actually wrong with the way the function was?
With the DT files now checked into U-Boot, I think it would accidentally work, since we just happen to have set #address-cells=<2>, #size-cells=<2>, and that would just happen to match sizeof(fdt_addr_t).
However note this is an accident on a couple of levels:
a) This is because the code assumes that sizeof(fdt_addr_t) == #address-cells * 4. This is an invalid assumption since it does not correctly honor the DT schema. It hard-codes the size of values whereas DT schema says the size is defined by the #xxx-cells properties.
b) The original Tegra210 DTs that TomW posted had #address-cells=<1>, #size-cells=<1>. I asked him to change that to match what I expected to be in the Linux kernel's Tegra210 DTs. However, if he'd rejected my request or I hadn't noticed that, then with CONFIG_PHYS_64BIT set, fdtdec_get_addr_size() would have attempted to read twice as many bytes as it should have from the property. It's entirely plausible that someone could have come along later and realized CONFIG_PHYS_64BIT was set incorrectly and enabled it.
Quoted 2 lines
This is a boot loader so we should be willing to make some simplifications.

Quoted 8 lines

When dealing with internal bootloader details, sure assumptions, simplifications, etc. can be made.
However, DT is an externally defined standard. The content of DT must be identical across all OSs (SW stacks, bootloader) and not influenced by requirements/... of any specific individual OS's (SW stack, bootloader) quirks. We can't just pick and choose which parts of it we care about. Well, perhaps if we stop calling it DT we could.

So I think in summary:

- 64-bit machines should have CONFIG_PHYS_64BIT set correctly
- then fdtdec_get_addr_size() would work as expected
- I want to make this cases as efficient as possible since it will be called in SPL
- You are concerned that making assumptions like this violates the DT spec

One option is to split the functions into two - one that works in SPL and makes assumptions, and one that does not and does things the hard way. I suppose we could also add checks/warnings that the DT is 'well-formed' and that the address size matches the machine it is running on.

In any case we do need to get rid of the parent lookup in this function. So can either you or Thierry submit a patch to do this? The parent should instead be a parameter to the function. You may need to create a stub function which looks it up before calling fdtdec_get_addr_size().

I'll see how things look in SPL.

Regards,
Simon

Michal Suchanek

2015-08-06 07:09 UTC #9 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Hello,

Quoted 42 lines · Simon Glass

Hi Stephen,
Quoted 22 lines · Stephen Warren
Quoted 13 lines · Simon Glass
Hi Stephen,
Quoted 9 lines · Stephen Warren
Quoted 7 lines · Simon Glass
Hi Stephen,
Quoted 2 lines · Stephen Warren
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines · Simon Glass
Quoted 2 lines
This is a boot loader so we should be willing to make some simplifications.
Quoted 8 lines
When dealing with internal bootloader details, sure assumptions, simplifications, etc. can be made.
However, DT is an externally defined standard. The content of DT must be identical across all OSs (SW stacks, bootloader) and not influenced by requirements/... of any specific individual OS's (SW stack, bootloader) quirks. We can't just pick and choose which parts of it we care about. Well, perhaps if we stop calling it DT we could.
So I think in summary:
- 64-bit machines should have CONFIG_PHYS_64BIT set correctly
- then fdtdec_get_addr_size() would work as expected
- I want to make this cases as efficient as possible since it will be called in SPL
- You are concerned that making assumptions like this violates the DT spec

Quoted 5 lines

One option is to split the functions into two - one that works in SPL and makes assumptions, and one that does not and does things the hard way. I suppose we could also add checks/warnings that the DT is 'well-formed' and that the address size matches the machine it is running on.

This depends on the way the binding is defined. If the binding description says that the value has as much data as defined in #address-cells and #size-cells then you have to parse those values to interpret the binding correctly. When the binding description says the value is always 32bit or always 64bit or whatever you must always read a value with that many bits.

Prime example of this are MTD partitions defined in dt.

If your flash is small you can get away with 32bit partitions and you would define #address-cells and #size-cells as 1 even on 64bit system. When your flash is large you need 64bits to store the partition size and offset and you will define #address-cells and #size-cells as 2 even on 32bit system because it won't work otherwise.

Thanks

Michal

Stephen Warren

2015-08-06 18:43 UTC #10 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Quoted 58 lines · Michal Suchanek

Hello,
Quoted 42 lines · Simon Glass
Hi Stephen,
Quoted 22 lines · Stephen Warren
Quoted 13 lines · Simon Glass
Hi Stephen,
Quoted 9 lines · Stephen Warren
Quoted 7 lines · Simon Glass
Hi Stephen,
Quoted 2 lines · Stephen Warren
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines · Simon Glass
Quoted 2 lines
This is a boot loader so we should be willing to make some simplifications.
Quoted 8 lines
When dealing with internal bootloader details, sure assumptions, simplifications, etc. can be made.
However, DT is an externally defined standard. The content of DT must be identical across all OSs (SW stacks, bootloader) and not influenced by requirements/... of any specific individual OS's (SW stack, bootloader) quirks. We can't just pick and choose which parts of it we care about. Well, perhaps if we stop calling it DT we could.
So I think in summary:
- 64-bit machines should have CONFIG_PHYS_64BIT set correctly
- then fdtdec_get_addr_size() would work as expected
- I want to make this cases as efficient as possible since it will be called in SPL
- You are concerned that making assumptions like this violates the DT spec
Quoted 5 lines
One option is to split the functions into two - one that works in SPL and makes assumptions, and one that does not and does things the hard way. I suppose we could also add checks/warnings that the DT is 'well-formed' and that the address size matches the machine it is running on.
This depends on the way the binding is defined. If the binding description says that the value has as much data as defined in #address-cells and #size-cells then you have to parse those values to interpret the binding correctly. When the binding description says the value is always 32bit or always 64bit or whatever you must always read a value with that many bits.

Quoted 7 lines

Prime example of this are MTD partitions defined in dt.
If your flash is small you can get away with 32bit partitions and you would define #address-cells and #size-cells as 1 even on 64bit system. When your flash is large you need 64bits to store the partition size and offset and you will define #address-cells and #size-cells as 2 even on 32bit system because it won't work otherwise.

I agree entirely.

FWIW, my part of this discussion was mainly targeted at the "reg" property, where #address-cells/#size-cells are defined as applying as part of the base DT definition. Indeed other bindings can be define this way to (or not) at the whim of the binding author. The bug with Intel flash was triggered by using the same piece of code to parse a property where #address-cells/#size-cells don't apply.

Stephen Warren

2015-08-06 19:03 UTC #11 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

In reply to Simon Glass

Quoted 288 lines · Simon Glass

Hi Stephen,
Quoted 276 lines · Stephen Warren
Quoted 136 lines · Simon Glass
Hi Stephen,
Quoted 83 lines · Stephen Warren
Quoted 48 lines · Simon Glass
Hi Stephen,
Quoted 33 lines · Stephen Warren
Quoted 12 lines · Simon Glass
This reverts commit 5b34436035fc862b5e8d0d2c3eab74ba36f1a7f4.
This function has a few problems. It calls fdt_parent_offset() which as
mentioned in code review is very slow.
https://patchwork.ozlabs.org/patch/499482/
https://patchwork.ozlabs.org/patch/452604/
Quoted 6 lines
It also happens to break SPI flash on Minnowboard max which is how I noticed
that this was applied. I can send a patch to tidy that up, but in any case
I think we should consider a revert until the function is better implemented.
The fact that the function needs to perform a slow operation is not a good
reason for a revert. The slowness of the operation is just a matter of fact
with DT not having uplinks in its data structure, and U-Boot using those
data structures directly.
Quoted 7 lines
You'd requested during review that I additionally implement a faster version
of the function in the case where the parent node is already known, and said
it was fine if that happened in a later patch. I have this on my TODO list,
but it's only been a couple of days.
Quoted 1 line
I didn't expect this to go to mainline before your new patch.
I didn't get that message from the thread; you wrote:
Quoted 16 lines · Simon Glass
Quoted 6 lines · Stephen Warren
Quoted 4 lines · Simon Glass
Also, how about (in addition) a
version of this function that works for devices? Like:
Quoted 6 lines
Quoted 3 lines
device_get_addr_size(struct udevice *dev, ...)
so that it can handle this for you.
That sounds like a separate patch?
Quoted 8 lines
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 4 lines
That sounds to like you'd like a followup patch soon after this patch (my assumption would be within a few days or weeks) to solve your comments, not
that you wanted a replacement patch.
I will take that feedback to be a little more forceful in future, sorry.
Quoted 31 lines
Quoted 7 lines
Quoted 4 lines
Why not just fix the bug since you said you could? That seems far better
than breaking the newly added Tegra210 support.
Quoted 5 lines
I do have a patch but I'm not 100% comfortable with the approach. I don't see a good reason to move away from the idea of fdt_addr_t and fdt_addr_t being set correctly for the platform. Or maybe I misunderstand the problem the patch was trying to fix. As I said it did not have a commit message, so who knows :-)
The size of fdt_addr_t isn't relevant *at all* when parsing DT. The only thing that matters is #address-cells/#size-cells. Those properties are what
tell the parsing code how many bytes to read from the reg property. Whether
the resultant value fits into the code's internal representation is an internal detail of the code, not part of the semantics of DT itself or how
to parse it.
If code assumes that #address-cells == sizeof(fdt_addr_t), it is indeed quite likely that everything will just happen to work most of the time. However, this is purely an accident and not something that anything should
rely upon.
Quoted 8 lines
(I think Tegra210 support still has CONFIG_PHYS_64BIT undefined which is admittedly a little /unexpected/ for a 64-bit U-Boot build, but in practice
is perfectly /legal/ and will work out just fine since, except perhaps for
RAM sizes, I don't believe any value in DT will actually require more than
32-bits to represent)
Quoted 3 lines
I would like to have the types match the platform where possible. At least the types should not be smaller than the platform - e.g. if CONFIG_PHYS_64BIT is not defined we should not support 64-bit
In general, there's no "should not" here; we "cannot". If there's a 64-bit value in the DT (with bits above bit 31 set), then it can't be stored in a 32-bit variable.
That said, if a DT has #address-cells=<2>, but the actual values stored in all reg values have 0 for the MS word, that'll actually work just fine. Note that it's 100% legal to set #address-cells=<100> and just write a bunch of extra zeros into the property. Silly perhaps, but perfectly legal. Since the function should adapt to whatever #address-cells value is in the DT, supporting that case isn't any more work, so there's no reason we shouldn't.
Quoted 3 lines
address/size in the device tree. This is for efficiency. We don't want to force all the U-Boot code to 64-bit suddenly. This will bloat things for no benefit.
We could and likely should set CONFIG_PHYS_64BIT for Tegra210. However, that's unrelated to using the correct algorithm to parse DT.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
P.S. What is the issue with SPI flash? The commit description doesn't mention this at all.
It calls that function expecting it to pick up an address and size from two consecutive cells. With this patch, that fails (unless the property happens to be "reg").
...
Quoted 19 lines
Quoted 12 lines
I think this is all stemming from drivers/mtd/spi/sf_probe.c spi_flash_decode_fdt()'s call to fdtdec_get_addr_size() for property "memory-map"? If so, then looking at arch/x86/dts/minnowmax.dts's /spi node,
the code and DT content are clearly inconsistent; For this node #address-cells=<1>, #size-cells=<0> which makes sense given that the address
is a chip-select and hence has no size. So, the code should not assume that
the memory-map property can be parsed in the same way as a reg property.
Quoted 5 lines
I note that the memory-map property doesn't exist in the Linux kernel's DT
binding documentation database, or code, hence hasn't been reviewed by the
DT binding maintainers.
Quoted 5 lines
The function comment says:
* Look up an address property in a node and return it as an address.
* The property must hold one address with a length. This is only tested
* on 32-bit machines.
And Thierry fixed it for systems with #address-cells > 1. Perhaps that part of the function comment should have been removed in the commit.
Quoted 5 lines
My intention was that this would be an efficient way to decode an address and size from a device tree. To some extent regmap may take over this role (IMO we should turn to drop fdtdec one day one we have more infrastructure). But I'd like it to work efficiently for 32-bit machines. The new function could hardly be less efficient.
Again, there's no way in general to make it more efficient. The efficiency issue is directly implied by the DT data structures.
In the special case where the parent node is already known, we can certainly introduce an alternate function that is more efficient. You've already asked for that, and as I said, it's in my TODO list.
Quoted 4 lines
I think we should consider the case where the physical address size of U-Boot and the device tree do not match as a corner case. I certainly don't want device tree to add loads of pointless code for 'normal' platforms.
It's not a corner case. It's a fundamental part of the DT schema. If U-Boot is going to use DT, it should actually use *DT*, not some-very similar-but-not-quite-DT format with all kinds of implicit and unstated exceptions, limitations, and assumptions. This implies fully honoring all aspects of how DT works when parsing it, not picking and choosing features because some are inconvenient or annoying. If U-Boot doesn't want to correctly implement DT support, it should just drop it completely.
As an aside, when instantiating devices, I hope one day that U-Boot will parse DT top-down in a hierarchical way, rather than simply searching for any node anywhere in the DT without regard for whether any parent node is enabled, has a driver, has had the driver initialize, etc. U-Boot ignores this right now, and is only getting away with this accidentally. Without hacky workarounds in drivers, this won't continue to work for all Tegra HW (e.g. host1x graphics/display sub-modules, AHUB's audio-related sub-modules), since parent drivers must initialize before child drivers in order to enable various register buses, including clocks/resets affecting those buses etc.
Again, if it's simply too inconvenient or bloated to implement DT properly in U-Boot, let's just drop it entirely. A halfway solution is the worst of both worlds. I'm not convinced the full implications of how to (and the need to) correctly and fully support DT have were well thought through before (control) DT support was added to U-Boot.
Quoted 6 lines
Re memory-map, yes it doesn't seem to be possible to do what it is trying to do (and Thierry says the same below). It is quite weird to have a SPI peripheral which is also memory mapped.
Here's my question - if you fix the CONFIG_PHYS_64BIT problem with 64-bit Tegra, what is actually wrong with the way the function was?
With the DT files now checked into U-Boot, I think it would accidentally work, since we just happen to have set #address-cells=<2>, #size-cells=<2>, and that would just happen to match sizeof(fdt_addr_t).
However note this is an accident on a couple of levels:
a) This is because the code assumes that sizeof(fdt_addr_t) == #address-cells * 4. This is an invalid assumption since it does not correctly honor the DT schema. It hard-codes the size of values whereas DT schema says the size is defined by the #xxx-cells properties.
b) The original Tegra210 DTs that TomW posted had #address-cells=<1>, #size-cells=<1>. I asked him to change that to match what I expected to be in the Linux kernel's Tegra210 DTs. However, if he'd rejected my request or I hadn't noticed that, then with CONFIG_PHYS_64BIT set, fdtdec_get_addr_size() would have attempted to read twice as many bytes as it should have from the property. It's entirely plausible that someone could have come along later and realized CONFIG_PHYS_64BIT was set incorrectly and enabled it.
Quoted 2 lines
This is a boot loader so we should be willing to make some simplifications.
Quoted 8 lines
When dealing with internal bootloader details, sure assumptions, simplifications, etc. can be made.
However, DT is an externally defined standard. The content of DT must be identical across all OSs (SW stacks, bootloader) and not influenced by requirements/... of any specific individual OS's (SW stack, bootloader) quirks. We can't just pick and choose which parts of it we care about. Well, perhaps if we stop calling it DT we could.

Quoted 3 lines

So I think in summary:
- 64-bit machines should have CONFIG_PHYS_64BIT set correctly

It turns out that arch/arm/include/asm/config.h already enables this for all ARM64 platforms.

As such, we can in fact go ahead with reverting this patch, and U-Boot will still function on Tegra210 boards.

In the short term, I think that means TomR should just apply this revert patch, and we don't need to send any additional patches. In the slightly longer term, we should add some comments to fdtdec_get_addr_size() describing its problems, and slightly longer term, add back Thierry's patch, but in a way that lets callers specify whether #address-cells/#size-cells should be used, or whether caller-supplied hard-coded values should be used.

I apologize for not noticing this earlier; I'd assumed that since none of the Tegra-specific files in include/configs/ set this flag, nor any of the Tegra-specific Kconfig files, it wasn't set, and hence a revert of the patch would break Tegra210 support.

Quoted 1 line

- then fdtdec_get_addr_size() would work as expected

I take issue with *works* as expected", since I would expect the function to implement DT parsing rules completely.

However, it's certainly true to say that it will generate the desired results, even if it doesn't /work/ (isn't implemented) as expected.

(I suppose this depends on whether you're talking about "works" w.r.t to correctness of the returned results or side-effects, or w.r.t. how the internal implementation works.)

Quoted 7 lines

- I want to make this cases as efficient as possible since it will be called in SPL
- You are concerned that making assumptions like this violates the DT spec
One option is to split the functions into two - one that works in SPL and makes assumptions, and one that does not and does things the hard way.

Why should SPL be any different? U-Boot should either parse DT correctly or not at all. SPL shouldn't be a special case. Admittedly SPL on Tegra does very little (and isn't even present on Tegra210), but in general, can't someone use storage drivers, filesystems, etc. in SPL to choose the next stage to load, read GPIOs or other data sources to select between different boot paths, perhaps even interact with a user? If so, then assuming that SPL can somehow implements a reduced set of features, and hence can make assumptions that non-SPL can't, seems quite dangerous. We could only do that if we put some active checks in the U-Boot makefiles to ensure that nobody enabled anything in the SPL besides a set of strictly audited features.

Quoted 3 lines

I suppose we could also add checks/warnings that the DT is 'well-formed' and that the address size matches the machine it is running on.

Yes, we certainly should do that.

Quoted 5 lines

In any case we do need to get rid of the parent lookup in this function. So can either you or Thierry submit a patch to do this? The parent should instead be a parameter to the function. You may need to create a stub function which looks it up before calling fdtdec_get_addr_size().

Yes, we can't remove the parent lookup in all cases. However, we can avoid it in the cases where the caller can supply it. I think that's most, but not quite all.

Quoted 5 lines

I'll see how things look in SPL.
Regards,
Simon

Simon Glass

author

2015-08-09 15:08 UTC #12 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Hi Stephen,

Quoted 367 lines · Stephen Warren

Quoted 355 lines · Simon Glass
Hi Stephen,
Quoted 343 lines · Stephen Warren
Quoted 161 lines · Simon Glass
Hi Stephen,
Quoted 100 lines · Stephen Warren
Quoted 57 lines · Simon Glass
Hi Stephen,
Quoted 38 lines · Stephen Warren
Quoted 13 lines · Simon Glass
This reverts commit 5b34436035fc862b5e8d0d2c3eab74ba36f1a7f4.
This function has a few problems. It calls fdt_parent_offset() which as
mentioned in code review is very slow.
https://patchwork.ozlabs.org/patch/499482/
https://patchwork.ozlabs.org/patch/452604/
Quoted 7 lines
It also happens to break SPI flash on Minnowboard max which is how I noticed
that this was applied. I can send a patch to tidy that up, but in any
case
I think we should consider a revert until the function is better implemented.
The fact that the function needs to perform a slow operation is not a good
reason for a revert. The slowness of the operation is just a matter of
fact
with DT not having uplinks in its data structure, and U-Boot using those
data structures directly.
Quoted 8 lines
You'd requested during review that I additionally implement a faster version
of the function in the case where the parent node is already known, and
said
it was fine if that happened in a later patch. I have this on my TODO list,
but it's only been a couple of days.
Quoted 1 line
I didn't expect this to go to mainline before your new patch.
I didn't get that message from the thread; you wrote:
Quoted 21 lines · Simon Glass
Quoted 8 lines · Stephen Warren
Quoted 5 lines · Simon Glass
Also, how about (in addition) a
version of this function that works for devices? Like:
Quoted 7 lines
Quoted 3 lines
device_get_addr_size(struct udevice *dev, ...)
so that it can handle this for you.
That sounds like a separate patch?
Quoted 8 lines
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 5 lines
That sounds to like you'd like a followup patch soon after this patch (my
assumption would be within a few days or weeks) to solve your comments, not
that you wanted a replacement patch.
I will take that feedback to be a little more forceful in future, sorry.
Quoted 34 lines
Quoted 8 lines
Quoted 4 lines
Why not just fix the bug since you said you could? That seems far better
than breaking the newly added Tegra210 support.
Quoted 5 lines
I do have a patch but I'm not 100% comfortable with the approach. I don't see a good reason to move away from the idea of fdt_addr_t and fdt_addr_t being set correctly for the platform. Or maybe I misunderstand the problem the patch was trying to fix. As I said it did not have a commit message, so who knows :-)
The size of fdt_addr_t isn't relevant *at all* when parsing DT. The only
thing that matters is #address-cells/#size-cells. Those properties are what
tell the parsing code how many bytes to read from the reg property. Whether
the resultant value fits into the code's internal representation is an internal detail of the code, not part of the semantics of DT itself or how
to parse it.
If code assumes that #address-cells == sizeof(fdt_addr_t), it is indeed quite likely that everything will just happen to work most of the time. However, this is purely an accident and not something that anything should
rely upon.
Quoted 9 lines
(I think Tegra210 support still has CONFIG_PHYS_64BIT undefined which is
admittedly a little /unexpected/ for a 64-bit U-Boot build, but in practice
is perfectly /legal/ and will work out just fine since, except perhaps for
RAM sizes, I don't believe any value in DT will actually require more than
32-bits to represent)
Quoted 3 lines
I would like to have the types match the platform where possible. At least the types should not be smaller than the platform - e.g. if CONFIG_PHYS_64BIT is not defined we should not support 64-bit
In general, there's no "should not" here; we "cannot". If there's a 64-bit
value in the DT (with bits above bit 31 set), then it can't be stored in a
32-bit variable.
That said, if a DT has #address-cells=<2>, but the actual values stored in
all reg values have 0 for the MS word, that'll actually work just fine. Note
that it's 100% legal to set #address-cells=<100> and just write a bunch of
extra zeros into the property. Silly perhaps, but perfectly legal. Since the
function should adapt to whatever #address-cells value is in the DT, supporting that case isn't any more work, so there's no reason we shouldn't.
Quoted 3 lines
address/size in the device tree. This is for efficiency. We don't want to force all the U-Boot code to 64-bit suddenly. This will bloat things for no benefit.
We could and likely should set CONFIG_PHYS_64BIT for Tegra210. However, that's unrelated to using the correct algorithm to parse DT.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
P.S. What is the issue with SPI flash? The commit description doesn't mention this at all.
It calls that function expecting it to pick up an address and size from two consecutive cells. With this patch, that fails (unless the property happens to be "reg").
...
Quoted 22 lines
Quoted 14 lines
I think this is all stemming from drivers/mtd/spi/sf_probe.c spi_flash_decode_fdt()'s call to fdtdec_get_addr_size() for property "memory-map"? If so, then looking at arch/x86/dts/minnowmax.dts's /spi node,
the code and DT content are clearly inconsistent; For this node #address-cells=<1>, #size-cells=<0> which makes sense given that the address
is a chip-select and hence has no size. So, the code should not assume that
the memory-map property can be parsed in the same way as a reg property.
Quoted 5 lines
I note that the memory-map property doesn't exist in the Linux kernel's DT
binding documentation database, or code, hence hasn't been reviewed by the
DT binding maintainers.
Quoted 6 lines
The function comment says:
* Look up an address property in a node and return it as an address.
* The property must hold one address with a length. This is only
tested
* on 32-bit machines.
And Thierry fixed it for systems with #address-cells > 1. Perhaps that part
of the function comment should have been removed in the commit.
Quoted 5 lines
My intention was that this would be an efficient way to decode an address and size from a device tree. To some extent regmap may take over this role (IMO we should turn to drop fdtdec one day one we have more infrastructure). But I'd like it to work efficiently for 32-bit machines. The new function could hardly be less efficient.
Again, there's no way in general to make it more efficient. The efficiency
issue is directly implied by the DT data structures.
In the special case where the parent node is already known, we can certainly
introduce an alternate function that is more efficient. You've already asked
for that, and as I said, it's in my TODO list.
Quoted 4 lines
I think we should consider the case where the physical address size of U-Boot and the device tree do not match as a corner case. I certainly don't want device tree to add loads of pointless code for 'normal' platforms.
It's not a corner case. It's a fundamental part of the DT schema. If U-Boot
is going to use DT, it should actually use *DT*, not some-very similar-but-not-quite-DT format with all kinds of implicit and unstated exceptions, limitations, and assumptions. This implies fully honoring all aspects of how DT works when parsing it, not picking and choosing features
because some are inconvenient or annoying. If U-Boot doesn't want to correctly implement DT support, it should just drop it completely.
As an aside, when instantiating devices, I hope one day that U-Boot will parse DT top-down in a hierarchical way, rather than simply searching for any node anywhere in the DT without regard for whether any parent node is enabled, has a driver, has had the driver initialize, etc. U-Boot ignores this right now, and is only getting away with this accidentally. Without hacky workarounds in drivers, this won't continue to work for all Tegra HW
(e.g. host1x graphics/display sub-modules, AHUB's audio-related sub-modules), since parent drivers must initialize before child drivers in
order to enable various register buses, including clocks/resets affecting those buses etc.
Again, if it's simply too inconvenient or bloated to implement DT properly
in U-Boot, let's just drop it entirely. A halfway solution is the worst of
both worlds. I'm not convinced the full implications of how to (and the need
to) correctly and fully support DT have were well thought through before (control) DT support was added to U-Boot.
Quoted 6 lines
Re memory-map, yes it doesn't seem to be possible to do what it is trying to do (and Thierry says the same below). It is quite weird to have a SPI peripheral which is also memory mapped.
Here's my question - if you fix the CONFIG_PHYS_64BIT problem with 64-bit Tegra, what is actually wrong with the way the function was?
With the DT files now checked into U-Boot, I think it would accidentally work, since we just happen to have set #address-cells=<2>, #size-cells=<2>,
and that would just happen to match sizeof(fdt_addr_t).
However note this is an accident on a couple of levels:
a) This is because the code assumes that sizeof(fdt_addr_t) == #address-cells * 4. This is an invalid assumption since it does not correctly honor the DT schema. It hard-codes the size of values whereas DT
schema says the size is defined by the #xxx-cells properties.
b) The original Tegra210 DTs that TomW posted had #address-cells=<1>, #size-cells=<1>. I asked him to change that to match what I expected to be
in the Linux kernel's Tegra210 DTs. However, if he'd rejected my request or
I hadn't noticed that, then with CONFIG_PHYS_64BIT set, fdtdec_get_addr_size() would have attempted to read twice as many bytes as
it should have from the property. It's entirely plausible that someone could
have come along later and realized CONFIG_PHYS_64BIT was set incorrectly and
enabled it.
Quoted 2 lines
This is a boot loader so we should be willing to make some simplifications.
When dealing with internal bootloader details, sure assumptions, simplifications, etc. can be made.
Quoted 6 lines
However, DT is an externally defined standard. The content of DT must be identical across all OSs (SW stacks, bootloader) and not influenced by requirements/... of any specific individual OS's (SW stack, bootloader) quirks. We can't just pick and choose which parts of it we care about. Well,
perhaps if we stop calling it DT we could.
Quoted 3 lines
So I think in summary:
- 64-bit machines should have CONFIG_PHYS_64BIT set correctly
It turns out that arch/arm/include/asm/config.h already enables this for all ARM64 platforms.
As such, we can in fact go ahead with reverting this patch, and U-Boot will still function on Tegra210 boards.

Quoted 6 lines

In the short term, I think that means TomR should just apply this revert patch, and we don't need to send any additional patches. In the slightly longer term, we should add some comments to fdtdec_get_addr_size() describing its problems, and slightly longer term, add back Thierry's patch, but in a way that lets callers specify whether #address-cells/#size-cells should be used, or whether caller-supplied hard-coded values should be used.

OK great. But I see your new patch so I think we can apply both at the same time.

Quoted 5 lines

I apologize for not noticing this earlier; I'd assumed that since none of the Tegra-specific files in include/configs/ set this flag, nor any of the Tegra-specific Kconfig files, it wasn't set, and hence a revert of the patch would break Tegra210 support.

No problem - I assumed it would also.

Quoted 23 lines

Quoted 2 lines
- then fdtdec_get_addr_size() would work as expected
I take issue with *works* as expected", since I would expect the function to implement DT parsing rules completely.
However, it's certainly true to say that it will generate the desired results, even if it doesn't /work/ (isn't implemented) as expected.
(I suppose this depends on whether you're talking about "works" w.r.t to correctness of the returned results or side-effects, or w.r.t. how the internal implementation works.)
Quoted 7 lines
- I want to make this cases as efficient as possible since it will be called in SPL
- You are concerned that making assumptions like this violates the DT spec
One option is to split the functions into two - one that works in SPL and makes assumptions, and one that does not and does things the hard way.

Quoted 10 lines

Why should SPL be any different? U-Boot should either parse DT correctly or not at all. SPL shouldn't be a special case. Admittedly SPL on Tegra does very little (and isn't even present on Tegra210), but in general, can't someone use storage drivers, filesystems, etc. in SPL to choose the next stage to load, read GPIOs or other data sources to select between different boot paths, perhaps even interact with a user? If so, then assuming that SPL can somehow implements a reduced set of features, and hence can make assumptions that non-SPL can't, seems quite dangerous. We could only do that if we put some active checks in the U-Boot makefiles to ensure that nobody enabled anything in the SPL besides a set of strictly audited features.

Yes we could do that and it would avoid pain later. I suppose SPL on Tegra is a bit of a special case since there is really no size limit. For some chips the limits are pretty severe so I am quite sensitive to code size even at the expense of extra debug time when something breaks.

Quoted 16 lines

Quoted 5 lines
I suppose we could also add checks/warnings that the DT is
'well-formed' and that the address size matches the machine it is running on.
Yes, we certainly should do that.
Quoted 5 lines
In any case we do need to get rid of the parent lookup in this function. So can either you or Thierry submit a patch to do this? The parent should instead be a parameter to the function. You may need to create a stub function which looks it up before calling fdtdec_get_addr_size().

Quoted 3 lines

Yes, we can't remove the parent lookup in all cases. However, we can avoid it in the cases where the caller can supply it. I think that's most, but not quite all.

My main concern is dev_get_addr() since that is the official way of reading a device address now. See also regmap_init_mem() which does things its own way.

Quoted 8 lines

Quoted 7 lines
I'll see how things look in SPL.
Regards,
Simon

Regards,
Simon

Bin Meng

2015-08-14 08:10 UTC #13 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Hi,

Quoted 444 lines · Simon Glass

Hi Stephen,
Quoted 367 lines · Stephen Warren
Quoted 355 lines · Simon Glass
Hi Stephen,
Quoted 343 lines · Stephen Warren
Quoted 161 lines · Simon Glass
Hi Stephen,
Quoted 100 lines · Stephen Warren
Quoted 57 lines · Simon Glass
Hi Stephen,
Quoted 38 lines · Stephen Warren
Quoted 13 lines · Simon Glass
This reverts commit 5b34436035fc862b5e8d0d2c3eab74ba36f1a7f4.
This function has a few problems. It calls fdt_parent_offset() which as
mentioned in code review is very slow.
https://patchwork.ozlabs.org/patch/499482/
https://patchwork.ozlabs.org/patch/452604/
Quoted 7 lines
It also happens to break SPI flash on Minnowboard max which is how I noticed
that this was applied. I can send a patch to tidy that up, but in any
case
I think we should consider a revert until the function is better implemented.
The fact that the function needs to perform a slow operation is not a good
reason for a revert. The slowness of the operation is just a matter of
fact
with DT not having uplinks in its data structure, and U-Boot using those
data structures directly.
Quoted 8 lines
You'd requested during review that I additionally implement a faster version
of the function in the case where the parent node is already known, and
said
it was fine if that happened in a later patch. I have this on my TODO list,
but it's only been a couple of days.
Quoted 1 line
I didn't expect this to go to mainline before your new patch.
I didn't get that message from the thread; you wrote:
Quoted 21 lines · Simon Glass
Quoted 8 lines · Stephen Warren
Quoted 5 lines · Simon Glass
Also, how about (in addition) a
version of this function that works for devices? Like:
Quoted 7 lines
Quoted 3 lines
device_get_addr_size(struct udevice *dev, ...)
so that it can handle this for you.
That sounds like a separate patch?
Quoted 8 lines
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 5 lines
That sounds to like you'd like a followup patch soon after this patch (my
assumption would be within a few days or weeks) to solve your comments, not
that you wanted a replacement patch.
I will take that feedback to be a little more forceful in future, sorry.
Quoted 34 lines
Quoted 8 lines
Quoted 4 lines
Why not just fix the bug since you said you could? That seems far better
than breaking the newly added Tegra210 support.
Quoted 5 lines
I do have a patch but I'm not 100% comfortable with the approach. I don't see a good reason to move away from the idea of fdt_addr_t and fdt_addr_t being set correctly for the platform. Or maybe I misunderstand the problem the patch was trying to fix. As I said it did not have a commit message, so who knows :-)
The size of fdt_addr_t isn't relevant *at all* when parsing DT. The only
thing that matters is #address-cells/#size-cells. Those properties are what
tell the parsing code how many bytes to read from the reg property. Whether
the resultant value fits into the code's internal representation is an internal detail of the code, not part of the semantics of DT itself or how
to parse it.
If code assumes that #address-cells == sizeof(fdt_addr_t), it is indeed quite likely that everything will just happen to work most of the time. However, this is purely an accident and not something that anything should
rely upon.
Quoted 9 lines
(I think Tegra210 support still has CONFIG_PHYS_64BIT undefined which is
admittedly a little /unexpected/ for a 64-bit U-Boot build, but in practice
is perfectly /legal/ and will work out just fine since, except perhaps for
RAM sizes, I don't believe any value in DT will actually require more than
32-bits to represent)
Quoted 3 lines
I would like to have the types match the platform where possible. At least the types should not be smaller than the platform - e.g. if CONFIG_PHYS_64BIT is not defined we should not support 64-bit
In general, there's no "should not" here; we "cannot". If there's a 64-bit
value in the DT (with bits above bit 31 set), then it can't be stored in a
32-bit variable.
That said, if a DT has #address-cells=<2>, but the actual values stored in
all reg values have 0 for the MS word, that'll actually work just fine. Note
that it's 100% legal to set #address-cells=<100> and just write a bunch of
extra zeros into the property. Silly perhaps, but perfectly legal. Since the
function should adapt to whatever #address-cells value is in the DT, supporting that case isn't any more work, so there's no reason we shouldn't.
Quoted 3 lines
address/size in the device tree. This is for efficiency. We don't want to force all the U-Boot code to 64-bit suddenly. This will bloat things for no benefit.
We could and likely should set CONFIG_PHYS_64BIT for Tegra210. However, that's unrelated to using the correct algorithm to parse DT.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
P.S. What is the issue with SPI flash? The commit description doesn't mention this at all.
It calls that function expecting it to pick up an address and size from two consecutive cells. With this patch, that fails (unless the property happens to be "reg").
...
Quoted 22 lines
Quoted 14 lines
I think this is all stemming from drivers/mtd/spi/sf_probe.c spi_flash_decode_fdt()'s call to fdtdec_get_addr_size() for property "memory-map"? If so, then looking at arch/x86/dts/minnowmax.dts's /spi node,
the code and DT content are clearly inconsistent; For this node #address-cells=<1>, #size-cells=<0> which makes sense given that the address
is a chip-select and hence has no size. So, the code should not assume that
the memory-map property can be parsed in the same way as a reg property.
Quoted 5 lines
I note that the memory-map property doesn't exist in the Linux kernel's DT
binding documentation database, or code, hence hasn't been reviewed by the
DT binding maintainers.
Quoted 6 lines
The function comment says:
* Look up an address property in a node and return it as an address.
* The property must hold one address with a length. This is only
tested
* on 32-bit machines.
And Thierry fixed it for systems with #address-cells > 1. Perhaps that part
of the function comment should have been removed in the commit.
Quoted 5 lines
My intention was that this would be an efficient way to decode an address and size from a device tree. To some extent regmap may take over this role (IMO we should turn to drop fdtdec one day one we have more infrastructure). But I'd like it to work efficiently for 32-bit machines. The new function could hardly be less efficient.
Again, there's no way in general to make it more efficient. The efficiency
issue is directly implied by the DT data structures.
In the special case where the parent node is already known, we can certainly
introduce an alternate function that is more efficient. You've already asked
for that, and as I said, it's in my TODO list.
Quoted 4 lines
I think we should consider the case where the physical address size of U-Boot and the device tree do not match as a corner case. I certainly don't want device tree to add loads of pointless code for 'normal' platforms.
It's not a corner case. It's a fundamental part of the DT schema. If U-Boot
is going to use DT, it should actually use *DT*, not some-very similar-but-not-quite-DT format with all kinds of implicit and unstated exceptions, limitations, and assumptions. This implies fully honoring all aspects of how DT works when parsing it, not picking and choosing features
because some are inconvenient or annoying. If U-Boot doesn't want to correctly implement DT support, it should just drop it completely.
As an aside, when instantiating devices, I hope one day that U-Boot will parse DT top-down in a hierarchical way, rather than simply searching for any node anywhere in the DT without regard for whether any parent node is enabled, has a driver, has had the driver initialize, etc. U-Boot ignores this right now, and is only getting away with this accidentally. Without hacky workarounds in drivers, this won't continue to work for all Tegra HW
(e.g. host1x graphics/display sub-modules, AHUB's audio-related sub-modules), since parent drivers must initialize before child drivers in
order to enable various register buses, including clocks/resets affecting those buses etc.
Again, if it's simply too inconvenient or bloated to implement DT properly
in U-Boot, let's just drop it entirely. A halfway solution is the worst of
both worlds. I'm not convinced the full implications of how to (and the need
to) correctly and fully support DT have were well thought through before (control) DT support was added to U-Boot.
Quoted 6 lines
Re memory-map, yes it doesn't seem to be possible to do what it is trying to do (and Thierry says the same below). It is quite weird to have a SPI peripheral which is also memory mapped.
Here's my question - if you fix the CONFIG_PHYS_64BIT problem with 64-bit Tegra, what is actually wrong with the way the function was?
With the DT files now checked into U-Boot, I think it would accidentally work, since we just happen to have set #address-cells=<2>, #size-cells=<2>,
and that would just happen to match sizeof(fdt_addr_t).
However note this is an accident on a couple of levels:
a) This is because the code assumes that sizeof(fdt_addr_t) == #address-cells * 4. This is an invalid assumption since it does not correctly honor the DT schema. It hard-codes the size of values whereas DT
schema says the size is defined by the #xxx-cells properties.
b) The original Tegra210 DTs that TomW posted had #address-cells=<1>, #size-cells=<1>. I asked him to change that to match what I expected to be
in the Linux kernel's Tegra210 DTs. However, if he'd rejected my request or
I hadn't noticed that, then with CONFIG_PHYS_64BIT set, fdtdec_get_addr_size() would have attempted to read twice as many bytes as
it should have from the property. It's entirely plausible that someone could
have come along later and realized CONFIG_PHYS_64BIT was set incorrectly and
enabled it.
Quoted 2 lines
This is a boot loader so we should be willing to make some simplifications.
When dealing with internal bootloader details, sure assumptions, simplifications, etc. can be made.
Quoted 6 lines
However, DT is an externally defined standard. The content of DT must be identical across all OSs (SW stacks, bootloader) and not influenced by requirements/... of any specific individual OS's (SW stack, bootloader) quirks. We can't just pick and choose which parts of it we care about. Well,
perhaps if we stop calling it DT we could.
Quoted 3 lines
So I think in summary:
- 64-bit machines should have CONFIG_PHYS_64BIT set correctly
It turns out that arch/arm/include/asm/config.h already enables this for all ARM64 platforms.
As such, we can in fact go ahead with reverting this patch, and U-Boot will still function on Tegra210 boards.
Quoted 6 lines
In the short term, I think that means TomR should just apply this revert patch, and we don't need to send any additional patches. In the slightly longer term, we should add some comments to fdtdec_get_addr_size() describing its problems, and slightly longer term, add back Thierry's patch, but in a way that lets callers specify whether #address-cells/#size-cells should be used, or whether caller-supplied hard-coded values should be used.
OK great. But I see your new patch so I think we can apply both at the same time.
Quoted 5 lines
I apologize for not noticing this earlier; I'd assumed that since none of the Tegra-specific files in include/configs/ set this flag, nor any of the Tegra-specific Kconfig files, it wasn't set, and hence a revert of the patch would break Tegra210 support.
No problem - I assumed it would also.
Quoted 23 lines
Quoted 2 lines
- then fdtdec_get_addr_size() would work as expected
I take issue with *works* as expected", since I would expect the function to implement DT parsing rules completely.
However, it's certainly true to say that it will generate the desired results, even if it doesn't /work/ (isn't implemented) as expected.
(I suppose this depends on whether you're talking about "works" w.r.t to correctness of the returned results or side-effects, or w.r.t. how the internal implementation works.)
Quoted 7 lines
- I want to make this cases as efficient as possible since it will be called in SPL
- You are concerned that making assumptions like this violates the DT spec
One option is to split the functions into two - one that works in SPL and makes assumptions, and one that does not and does things the hard way.
Quoted 10 lines
Why should SPL be any different? U-Boot should either parse DT correctly or not at all. SPL shouldn't be a special case. Admittedly SPL on Tegra does very little (and isn't even present on Tegra210), but in general, can't someone use storage drivers, filesystems, etc. in SPL to choose the next stage to load, read GPIOs or other data sources to select between different boot paths, perhaps even interact with a user? If so, then assuming that SPL can somehow implements a reduced set of features, and hence can make assumptions that non-SPL can't, seems quite dangerous. We could only do that if we put some active checks in the U-Boot makefiles to ensure that nobody enabled anything in the SPL besides a set of strictly audited features.
Yes we could do that and it would avoid pain later. I suppose SPL on Tegra is a bit of a special case since there is really no size limit. For some chips the limits are pretty severe so I am quite sensitive to code size even at the expense of extra debug time when something breaks.
Quoted 16 lines
Quoted 5 lines
I suppose we could also add checks/warnings that the DT is
'well-formed' and that the address size matches the machine it is running on.
Yes, we certainly should do that.
Quoted 5 lines
In any case we do need to get rid of the parent lookup in this function. So can either you or Thierry submit a patch to do this? The parent should instead be a parameter to the function. You may need to create a stub function which looks it up before calling fdtdec_get_addr_size().

Quoted 17 lines

Quoted 3 lines
Yes, we can't remove the parent lookup in all cases. However, we can avoid it in the cases where the caller can supply it. I think that's most, but not quite all.
My main concern is dev_get_addr() since that is the official way of reading a device address now. See also regmap_init_mem() which does things its own way.
Quoted 8 lines
Quoted 7 lines
I'll see how things look in SPL.
Regards,
Simon

Do we have any conclusion about commit 5b34436? Today I started to check the pre-relocatoin DM PCI UART issue, but found it is now broken due to this commit. The broken part is at ns16550_serial_ofdata_to_platdata() in drivers/serial/ns16550.c, in which it has:

    addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
#ifdef CONFIG_PCI
    if (addr == FDT_ADDR_T_NONE) {

...

Before commit 5b34436, the old behavior is that the call to fdtdec_get_addr() returns FDT_ADDR_T_NONE so that we can trap into the PCI logic. But with commit 5b34436, addr is now zero which just bypass this logic.

As for why addr is now zero, this is because fdtdec_get_number() can only handle a 64-bit number at most. However for PCI reg, it has 3 cells. So if I have the following encoding:

reg = <0x00025100 0x0 0x0 0x0 0x0

           0x01025110 0x0 0x0 0x0 0x0>;

The addr will be assigned to zero after two rounds of left shift by 32-bit.

I can certainly change ns16550 driver to test the return value against 0 now, but I think this fdtdec_get_addr() does not cover all cases. Please advise.

Regards,
Bin

Thierry Reding

2015-08-14 08:32 UTC #14 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Quoted 485 lines · Bin Meng

Hi,
Quoted 444 lines · Simon Glass
Hi Stephen,
Quoted 367 lines · Stephen Warren
Quoted 355 lines · Simon Glass
Hi Stephen,
Quoted 343 lines · Stephen Warren
Quoted 161 lines · Simon Glass
Hi Stephen,
Quoted 100 lines · Stephen Warren
Quoted 57 lines · Simon Glass
Hi Stephen,
Quoted 38 lines · Stephen Warren
Quoted 13 lines · Simon Glass
This reverts commit 5b34436035fc862b5e8d0d2c3eab74ba36f1a7f4.
This function has a few problems. It calls fdt_parent_offset() which as
mentioned in code review is very slow.
https://patchwork.ozlabs.org/patch/499482/
https://patchwork.ozlabs.org/patch/452604/
Quoted 7 lines
It also happens to break SPI flash on Minnowboard max which is how I noticed
that this was applied. I can send a patch to tidy that up, but in any
case
I think we should consider a revert until the function is better implemented.
The fact that the function needs to perform a slow operation is not a good
reason for a revert. The slowness of the operation is just a matter of
fact
with DT not having uplinks in its data structure, and U-Boot using those
data structures directly.
Quoted 8 lines
You'd requested during review that I additionally implement a faster version
of the function in the case where the parent node is already known, and
said
it was fine if that happened in a later patch. I have this on my TODO list,
but it's only been a couple of days.
Quoted 1 line
I didn't expect this to go to mainline before your new patch.
I didn't get that message from the thread; you wrote:
Quoted 21 lines · Simon Glass
Quoted 8 lines · Stephen Warren
Quoted 5 lines · Simon Glass
Also, how about (in addition) a
version of this function that works for devices? Like:
Quoted 7 lines
Quoted 3 lines
device_get_addr_size(struct udevice *dev, ...)
so that it can handle this for you.
That sounds like a separate patch?
Quoted 8 lines
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 5 lines
That sounds to like you'd like a followup patch soon after this patch (my
assumption would be within a few days or weeks) to solve your comments, not
that you wanted a replacement patch.
I will take that feedback to be a little more forceful in future, sorry.
Quoted 34 lines
Quoted 8 lines
Quoted 4 lines
Why not just fix the bug since you said you could? That seems far better
than breaking the newly added Tegra210 support.
Quoted 5 lines
I do have a patch but I'm not 100% comfortable with the approach. I don't see a good reason to move away from the idea of fdt_addr_t and fdt_addr_t being set correctly for the platform. Or maybe I misunderstand the problem the patch was trying to fix. As I said it did not have a commit message, so who knows :-)
The size of fdt_addr_t isn't relevant *at all* when parsing DT. The only
thing that matters is #address-cells/#size-cells. Those properties are what
tell the parsing code how many bytes to read from the reg property. Whether
the resultant value fits into the code's internal representation is an internal detail of the code, not part of the semantics of DT itself or how
to parse it.
If code assumes that #address-cells == sizeof(fdt_addr_t), it is indeed quite likely that everything will just happen to work most of the time. However, this is purely an accident and not something that anything should
rely upon.
Quoted 9 lines
(I think Tegra210 support still has CONFIG_PHYS_64BIT undefined which is
admittedly a little /unexpected/ for a 64-bit U-Boot build, but in practice
is perfectly /legal/ and will work out just fine since, except perhaps for
RAM sizes, I don't believe any value in DT will actually require more than
32-bits to represent)
Quoted 3 lines
I would like to have the types match the platform where possible. At least the types should not be smaller than the platform - e.g. if CONFIG_PHYS_64BIT is not defined we should not support 64-bit
In general, there's no "should not" here; we "cannot". If there's a 64-bit
value in the DT (with bits above bit 31 set), then it can't be stored in a
32-bit variable.
That said, if a DT has #address-cells=<2>, but the actual values stored in
all reg values have 0 for the MS word, that'll actually work just fine. Note
that it's 100% legal to set #address-cells=<100> and just write a bunch of
extra zeros into the property. Silly perhaps, but perfectly legal. Since the
function should adapt to whatever #address-cells value is in the DT, supporting that case isn't any more work, so there's no reason we shouldn't.
Quoted 3 lines
address/size in the device tree. This is for efficiency. We don't want to force all the U-Boot code to 64-bit suddenly. This will bloat things for no benefit.
We could and likely should set CONFIG_PHYS_64BIT for Tegra210. However, that's unrelated to using the correct algorithm to parse DT.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
P.S. What is the issue with SPI flash? The commit description doesn't mention this at all.
It calls that function expecting it to pick up an address and size from two consecutive cells. With this patch, that fails (unless the property happens to be "reg").
...
Quoted 22 lines
Quoted 14 lines
I think this is all stemming from drivers/mtd/spi/sf_probe.c spi_flash_decode_fdt()'s call to fdtdec_get_addr_size() for property "memory-map"? If so, then looking at arch/x86/dts/minnowmax.dts's /spi node,
the code and DT content are clearly inconsistent; For this node #address-cells=<1>, #size-cells=<0> which makes sense given that the address
is a chip-select and hence has no size. So, the code should not assume that
the memory-map property can be parsed in the same way as a reg property.
Quoted 5 lines
I note that the memory-map property doesn't exist in the Linux kernel's DT
binding documentation database, or code, hence hasn't been reviewed by the
DT binding maintainers.
Quoted 6 lines
The function comment says:
* Look up an address property in a node and return it as an address.
* The property must hold one address with a length. This is only
tested
* on 32-bit machines.
And Thierry fixed it for systems with #address-cells > 1. Perhaps that part
of the function comment should have been removed in the commit.
Quoted 5 lines
My intention was that this would be an efficient way to decode an address and size from a device tree. To some extent regmap may take over this role (IMO we should turn to drop fdtdec one day one we have more infrastructure). But I'd like it to work efficiently for 32-bit machines. The new function could hardly be less efficient.
Again, there's no way in general to make it more efficient. The efficiency
issue is directly implied by the DT data structures.
In the special case where the parent node is already known, we can certainly
introduce an alternate function that is more efficient. You've already asked
for that, and as I said, it's in my TODO list.
Quoted 4 lines
I think we should consider the case where the physical address size of U-Boot and the device tree do not match as a corner case. I certainly don't want device tree to add loads of pointless code for 'normal' platforms.
It's not a corner case. It's a fundamental part of the DT schema. If U-Boot
is going to use DT, it should actually use *DT*, not some-very similar-but-not-quite-DT format with all kinds of implicit and unstated exceptions, limitations, and assumptions. This implies fully honoring all aspects of how DT works when parsing it, not picking and choosing features
because some are inconvenient or annoying. If U-Boot doesn't want to correctly implement DT support, it should just drop it completely.
As an aside, when instantiating devices, I hope one day that U-Boot will parse DT top-down in a hierarchical way, rather than simply searching for any node anywhere in the DT without regard for whether any parent node is enabled, has a driver, has had the driver initialize, etc. U-Boot ignores this right now, and is only getting away with this accidentally. Without hacky workarounds in drivers, this won't continue to work for all Tegra HW
(e.g. host1x graphics/display sub-modules, AHUB's audio-related sub-modules), since parent drivers must initialize before child drivers in
order to enable various register buses, including clocks/resets affecting those buses etc.
Again, if it's simply too inconvenient or bloated to implement DT properly
in U-Boot, let's just drop it entirely. A halfway solution is the worst of
both worlds. I'm not convinced the full implications of how to (and the need
to) correctly and fully support DT have were well thought through before (control) DT support was added to U-Boot.
Quoted 6 lines
Re memory-map, yes it doesn't seem to be possible to do what it is trying to do (and Thierry says the same below). It is quite weird to have a SPI peripheral which is also memory mapped.
Here's my question - if you fix the CONFIG_PHYS_64BIT problem with 64-bit Tegra, what is actually wrong with the way the function was?
With the DT files now checked into U-Boot, I think it would accidentally work, since we just happen to have set #address-cells=<2>, #size-cells=<2>,
and that would just happen to match sizeof(fdt_addr_t).
However note this is an accident on a couple of levels:
a) This is because the code assumes that sizeof(fdt_addr_t) == #address-cells * 4. This is an invalid assumption since it does not correctly honor the DT schema. It hard-codes the size of values whereas DT
schema says the size is defined by the #xxx-cells properties.
b) The original Tegra210 DTs that TomW posted had #address-cells=<1>, #size-cells=<1>. I asked him to change that to match what I expected to be
in the Linux kernel's Tegra210 DTs. However, if he'd rejected my request or
I hadn't noticed that, then with CONFIG_PHYS_64BIT set, fdtdec_get_addr_size() would have attempted to read twice as many bytes as
it should have from the property. It's entirely plausible that someone could
have come along later and realized CONFIG_PHYS_64BIT was set incorrectly and
enabled it.
Quoted 2 lines
This is a boot loader so we should be willing to make some simplifications.
When dealing with internal bootloader details, sure assumptions, simplifications, etc. can be made.
Quoted 6 lines
However, DT is an externally defined standard. The content of DT must be identical across all OSs (SW stacks, bootloader) and not influenced by requirements/... of any specific individual OS's (SW stack, bootloader) quirks. We can't just pick and choose which parts of it we care about. Well,
perhaps if we stop calling it DT we could.
Quoted 3 lines
So I think in summary:
- 64-bit machines should have CONFIG_PHYS_64BIT set correctly
It turns out that arch/arm/include/asm/config.h already enables this for all ARM64 platforms.
As such, we can in fact go ahead with reverting this patch, and U-Boot will still function on Tegra210 boards.
Quoted 6 lines
In the short term, I think that means TomR should just apply this revert patch, and we don't need to send any additional patches. In the slightly longer term, we should add some comments to fdtdec_get_addr_size() describing its problems, and slightly longer term, add back Thierry's patch, but in a way that lets callers specify whether #address-cells/#size-cells should be used, or whether caller-supplied hard-coded values should be used.
OK great. But I see your new patch so I think we can apply both at the same time.
Quoted 5 lines
I apologize for not noticing this earlier; I'd assumed that since none of the Tegra-specific files in include/configs/ set this flag, nor any of the Tegra-specific Kconfig files, it wasn't set, and hence a revert of the patch would break Tegra210 support.
No problem - I assumed it would also.
Quoted 23 lines
Quoted 2 lines
- then fdtdec_get_addr_size() would work as expected
I take issue with *works* as expected", since I would expect the function to implement DT parsing rules completely.
However, it's certainly true to say that it will generate the desired results, even if it doesn't /work/ (isn't implemented) as expected.
(I suppose this depends on whether you're talking about "works" w.r.t to correctness of the returned results or side-effects, or w.r.t. how the internal implementation works.)
Quoted 7 lines
- I want to make this cases as efficient as possible since it will be called in SPL
- You are concerned that making assumptions like this violates the DT spec
One option is to split the functions into two - one that works in SPL and makes assumptions, and one that does not and does things the hard way.
Quoted 10 lines
Why should SPL be any different? U-Boot should either parse DT correctly or not at all. SPL shouldn't be a special case. Admittedly SPL on Tegra does very little (and isn't even present on Tegra210), but in general, can't someone use storage drivers, filesystems, etc. in SPL to choose the next stage to load, read GPIOs or other data sources to select between different boot paths, perhaps even interact with a user? If so, then assuming that SPL can somehow implements a reduced set of features, and hence can make assumptions that non-SPL can't, seems quite dangerous. We could only do that if we put some active checks in the U-Boot makefiles to ensure that nobody enabled anything in the SPL besides a set of strictly audited features.
Yes we could do that and it would avoid pain later. I suppose SPL on Tegra is a bit of a special case since there is really no size limit. For some chips the limits are pretty severe so I am quite sensitive to code size even at the expense of extra debug time when something breaks.
Quoted 16 lines
Quoted 5 lines
I suppose we could also add checks/warnings that the DT is
'well-formed' and that the address size matches the machine it is running on.
Yes, we certainly should do that.
Quoted 5 lines
In any case we do need to get rid of the parent lookup in this function. So can either you or Thierry submit a patch to do this? The parent should instead be a parameter to the function. You may need to create a stub function which looks it up before calling fdtdec_get_addr_size().
Quoted 17 lines
Quoted 3 lines
Yes, we can't remove the parent lookup in all cases. However, we can avoid it in the cases where the caller can supply it. I think that's most, but not quite all.
My main concern is dev_get_addr() since that is the official way of reading a device address now. See also regmap_init_mem() which does things its own way.
Quoted 8 lines
Quoted 7 lines
I'll see how things look in SPL.
Regards,
Simon
Do we have any conclusion about commit 5b34436? Today I started to check the pre-relocatoin DM PCI UART issue, but found it is now broken due to this commit. The broken part is at ns16550_serial_ofdata_to_platdata() in drivers/serial/ns16550.c, in which it has:
    addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
#ifdef CONFIG_PCI
    if (addr == FDT_ADDR_T_NONE) {
...
Before commit 5b34436, the old behavior is that the call to fdtdec_get_addr() returns FDT_ADDR_T_NONE so that we can trap into the PCI logic. But with commit 5b34436, addr is now zero which just bypass this logic.
As for why addr is now zero, this is because fdtdec_get_number() can only handle a 64-bit number at most. However for PCI reg, it has 3 cells. So if I have the following encoding:

Quoted 8 lines

reg = <0x00025100 0x0 0x0 0x0 0x0
           0x01025110 0x0 0x0 0x0 0x0>;
The addr will be assigned to zero after two rounds of left shift by 32-bit.
I can certainly change ns16550 driver to test the return value against 0 now, but I think this fdtdec_get_addr() does not cover all cases. Please advise.

This type of simple address parsing breaks down in the face of PCI. PCI requires 3 address cells because it has different types of address spaces. fdtdec_get_address() actually does the right thing here. It returns the "address" associated with the entry. The address is the 64-bit value you get by concatenating cells 0 and 1. Cell 2 contains additional meta data such as the PCI address and the type of memory that you are dealing with (configuration space, I/O, memory-mapped).

I'd suggest that we add code to properly check whether this is a PCI device.

Also, why isn't this code obtaining the memory address from the base address registers?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150814/edb27db6/attachment.sig>

Bin Meng

2015-08-14 08:44 UTC #15 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Hi Thierry,

Quoted 495 lines · Thierry Reding

Quoted 485 lines · Bin Meng
Hi,
Quoted 444 lines · Simon Glass
Hi Stephen,
Quoted 367 lines · Stephen Warren
Quoted 355 lines · Simon Glass
Hi Stephen,
Quoted 343 lines · Stephen Warren
Quoted 161 lines · Simon Glass
Hi Stephen,
Quoted 100 lines · Stephen Warren
Quoted 57 lines · Simon Glass
Hi Stephen,
Quoted 38 lines · Stephen Warren
Quoted 13 lines · Simon Glass
This reverts commit 5b34436035fc862b5e8d0d2c3eab74ba36f1a7f4.
This function has a few problems. It calls fdt_parent_offset() which as
mentioned in code review is very slow.
https://patchwork.ozlabs.org/patch/499482/
https://patchwork.ozlabs.org/patch/452604/
Quoted 7 lines
It also happens to break SPI flash on Minnowboard max which is how I noticed
that this was applied. I can send a patch to tidy that up, but in any
case
I think we should consider a revert until the function is better implemented.
The fact that the function needs to perform a slow operation is not a good
reason for a revert. The slowness of the operation is just a matter of
fact
with DT not having uplinks in its data structure, and U-Boot using those
data structures directly.
Quoted 8 lines
You'd requested during review that I additionally implement a faster version
of the function in the case where the parent node is already known, and
said
it was fine if that happened in a later patch. I have this on my TODO list,
but it's only been a couple of days.
Quoted 1 line
I didn't expect this to go to mainline before your new patch.
I didn't get that message from the thread; you wrote:
Quoted 21 lines · Simon Glass
Quoted 8 lines · Stephen Warren
Quoted 5 lines · Simon Glass
Also, how about (in addition) a
version of this function that works for devices? Like:
Quoted 7 lines
Quoted 3 lines
device_get_addr_size(struct udevice *dev, ...)
so that it can handle this for you.
That sounds like a separate patch?
Quoted 8 lines
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 5 lines
That sounds to like you'd like a followup patch soon after this patch (my
assumption would be within a few days or weeks) to solve your comments, not
that you wanted a replacement patch.
I will take that feedback to be a little more forceful in future, sorry.
Quoted 34 lines
Quoted 8 lines
Quoted 4 lines
Why not just fix the bug since you said you could? That seems far better
than breaking the newly added Tegra210 support.
Quoted 5 lines
I do have a patch but I'm not 100% comfortable with the approach. I don't see a good reason to move away from the idea of fdt_addr_t and fdt_addr_t being set correctly for the platform. Or maybe I misunderstand the problem the patch was trying to fix. As I said it did not have a commit message, so who knows :-)
The size of fdt_addr_t isn't relevant *at all* when parsing DT. The only
thing that matters is #address-cells/#size-cells. Those properties are what
tell the parsing code how many bytes to read from the reg property. Whether
the resultant value fits into the code's internal representation is an internal detail of the code, not part of the semantics of DT itself or how
to parse it.
If code assumes that #address-cells == sizeof(fdt_addr_t), it is indeed quite likely that everything will just happen to work most of the time. However, this is purely an accident and not something that anything should
rely upon.
Quoted 9 lines
(I think Tegra210 support still has CONFIG_PHYS_64BIT undefined which is
admittedly a little /unexpected/ for a 64-bit U-Boot build, but in practice
is perfectly /legal/ and will work out just fine since, except perhaps for
RAM sizes, I don't believe any value in DT will actually require more than
32-bits to represent)
Quoted 3 lines
I would like to have the types match the platform where possible. At least the types should not be smaller than the platform - e.g. if CONFIG_PHYS_64BIT is not defined we should not support 64-bit
In general, there's no "should not" here; we "cannot". If there's a 64-bit
value in the DT (with bits above bit 31 set), then it can't be stored in a
32-bit variable.
That said, if a DT has #address-cells=<2>, but the actual values stored in
all reg values have 0 for the MS word, that'll actually work just fine. Note
that it's 100% legal to set #address-cells=<100> and just write a bunch of
extra zeros into the property. Silly perhaps, but perfectly legal. Since the
function should adapt to whatever #address-cells value is in the DT, supporting that case isn't any more work, so there's no reason we shouldn't.
Quoted 3 lines
address/size in the device tree. This is for efficiency. We don't want to force all the U-Boot code to 64-bit suddenly. This will bloat things for no benefit.
We could and likely should set CONFIG_PHYS_64BIT for Tegra210. However, that's unrelated to using the correct algorithm to parse DT.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
P.S. What is the issue with SPI flash? The commit description doesn't mention this at all.
It calls that function expecting it to pick up an address and size from two consecutive cells. With this patch, that fails (unless the property happens to be "reg").
...
Quoted 22 lines
Quoted 14 lines
I think this is all stemming from drivers/mtd/spi/sf_probe.c spi_flash_decode_fdt()'s call to fdtdec_get_addr_size() for property "memory-map"? If so, then looking at arch/x86/dts/minnowmax.dts's /spi node,
the code and DT content are clearly inconsistent; For this node #address-cells=<1>, #size-cells=<0> which makes sense given that the address
is a chip-select and hence has no size. So, the code should not assume that
the memory-map property can be parsed in the same way as a reg property.
Quoted 5 lines
I note that the memory-map property doesn't exist in the Linux kernel's DT
binding documentation database, or code, hence hasn't been reviewed by the
DT binding maintainers.
Quoted 6 lines
The function comment says:
* Look up an address property in a node and return it as an address.
* The property must hold one address with a length. This is only
tested
* on 32-bit machines.
And Thierry fixed it for systems with #address-cells > 1. Perhaps that part
of the function comment should have been removed in the commit.
Quoted 5 lines
My intention was that this would be an efficient way to decode an address and size from a device tree. To some extent regmap may take over this role (IMO we should turn to drop fdtdec one day one we have more infrastructure). But I'd like it to work efficiently for 32-bit machines. The new function could hardly be less efficient.
Again, there's no way in general to make it more efficient. The efficiency
issue is directly implied by the DT data structures.
In the special case where the parent node is already known, we can certainly
introduce an alternate function that is more efficient. You've already asked
for that, and as I said, it's in my TODO list.
Quoted 4 lines
I think we should consider the case where the physical address size of U-Boot and the device tree do not match as a corner case. I certainly don't want device tree to add loads of pointless code for 'normal' platforms.
It's not a corner case. It's a fundamental part of the DT schema. If U-Boot
is going to use DT, it should actually use *DT*, not some-very similar-but-not-quite-DT format with all kinds of implicit and unstated exceptions, limitations, and assumptions. This implies fully honoring all aspects of how DT works when parsing it, not picking and choosing features
because some are inconvenient or annoying. If U-Boot doesn't want to correctly implement DT support, it should just drop it completely.
As an aside, when instantiating devices, I hope one day that U-Boot will parse DT top-down in a hierarchical way, rather than simply searching for any node anywhere in the DT without regard for whether any parent node is enabled, has a driver, has had the driver initialize, etc. U-Boot ignores this right now, and is only getting away with this accidentally. Without hacky workarounds in drivers, this won't continue to work for all Tegra HW
(e.g. host1x graphics/display sub-modules, AHUB's audio-related sub-modules), since parent drivers must initialize before child drivers in
order to enable various register buses, including clocks/resets affecting those buses etc.
Again, if it's simply too inconvenient or bloated to implement DT properly
in U-Boot, let's just drop it entirely. A halfway solution is the worst of
both worlds. I'm not convinced the full implications of how to (and the need
to) correctly and fully support DT have were well thought through before (control) DT support was added to U-Boot.
Quoted 6 lines
Re memory-map, yes it doesn't seem to be possible to do what it is trying to do (and Thierry says the same below). It is quite weird to have a SPI peripheral which is also memory mapped.
Here's my question - if you fix the CONFIG_PHYS_64BIT problem with 64-bit Tegra, what is actually wrong with the way the function was?
With the DT files now checked into U-Boot, I think it would accidentally work, since we just happen to have set #address-cells=<2>, #size-cells=<2>,
and that would just happen to match sizeof(fdt_addr_t).
However note this is an accident on a couple of levels:
a) This is because the code assumes that sizeof(fdt_addr_t) == #address-cells * 4. This is an invalid assumption since it does not correctly honor the DT schema. It hard-codes the size of values whereas DT
schema says the size is defined by the #xxx-cells properties.
b) The original Tegra210 DTs that TomW posted had #address-cells=<1>, #size-cells=<1>. I asked him to change that to match what I expected to be
in the Linux kernel's Tegra210 DTs. However, if he'd rejected my request or
I hadn't noticed that, then with CONFIG_PHYS_64BIT set, fdtdec_get_addr_size() would have attempted to read twice as many bytes as
it should have from the property. It's entirely plausible that someone could
have come along later and realized CONFIG_PHYS_64BIT was set incorrectly and
enabled it.
Quoted 2 lines
This is a boot loader so we should be willing to make some simplifications.
When dealing with internal bootloader details, sure assumptions, simplifications, etc. can be made.
Quoted 6 lines
However, DT is an externally defined standard. The content of DT must be identical across all OSs (SW stacks, bootloader) and not influenced by requirements/... of any specific individual OS's (SW stack, bootloader) quirks. We can't just pick and choose which parts of it we care about. Well,
perhaps if we stop calling it DT we could.
Quoted 3 lines
So I think in summary:
- 64-bit machines should have CONFIG_PHYS_64BIT set correctly
It turns out that arch/arm/include/asm/config.h already enables this for all ARM64 platforms.
As such, we can in fact go ahead with reverting this patch, and U-Boot will still function on Tegra210 boards.
Quoted 6 lines
In the short term, I think that means TomR should just apply this revert patch, and we don't need to send any additional patches. In the slightly longer term, we should add some comments to fdtdec_get_addr_size() describing its problems, and slightly longer term, add back Thierry's patch, but in a way that lets callers specify whether #address-cells/#size-cells should be used, or whether caller-supplied hard-coded values should be used.
OK great. But I see your new patch so I think we can apply both at the same time.
Quoted 5 lines
I apologize for not noticing this earlier; I'd assumed that since none of the Tegra-specific files in include/configs/ set this flag, nor any of the Tegra-specific Kconfig files, it wasn't set, and hence a revert of the patch would break Tegra210 support.
No problem - I assumed it would also.
Quoted 23 lines
Quoted 2 lines
- then fdtdec_get_addr_size() would work as expected
I take issue with *works* as expected", since I would expect the function to implement DT parsing rules completely.
However, it's certainly true to say that it will generate the desired results, even if it doesn't /work/ (isn't implemented) as expected.
(I suppose this depends on whether you're talking about "works" w.r.t to correctness of the returned results or side-effects, or w.r.t. how the internal implementation works.)
Quoted 7 lines
- I want to make this cases as efficient as possible since it will be called in SPL
- You are concerned that making assumptions like this violates the DT spec
One option is to split the functions into two - one that works in SPL and makes assumptions, and one that does not and does things the hard way.
Quoted 10 lines
Why should SPL be any different? U-Boot should either parse DT correctly or not at all. SPL shouldn't be a special case. Admittedly SPL on Tegra does very little (and isn't even present on Tegra210), but in general, can't someone use storage drivers, filesystems, etc. in SPL to choose the next stage to load, read GPIOs or other data sources to select between different boot paths, perhaps even interact with a user? If so, then assuming that SPL can somehow implements a reduced set of features, and hence can make assumptions that non-SPL can't, seems quite dangerous. We could only do that if we put some active checks in the U-Boot makefiles to ensure that nobody enabled anything in the SPL besides a set of strictly audited features.
Yes we could do that and it would avoid pain later. I suppose SPL on Tegra is a bit of a special case since there is really no size limit. For some chips the limits are pretty severe so I am quite sensitive to code size even at the expense of extra debug time when something breaks.
Quoted 16 lines
Quoted 5 lines
I suppose we could also add checks/warnings that the DT is
'well-formed' and that the address size matches the machine it is running on.
Yes, we certainly should do that.
Quoted 5 lines
In any case we do need to get rid of the parent lookup in this function. So can either you or Thierry submit a patch to do this? The parent should instead be a parameter to the function. You may need to create a stub function which looks it up before calling fdtdec_get_addr_size().
Quoted 17 lines
Quoted 3 lines
Yes, we can't remove the parent lookup in all cases. However, we can avoid it in the cases where the caller can supply it. I think that's most, but not quite all.
My main concern is dev_get_addr() since that is the official way of reading a device address now. See also regmap_init_mem() which does things its own way.
Quoted 8 lines
Quoted 7 lines
I'll see how things look in SPL.
Regards,
Simon
Do we have any conclusion about commit 5b34436? Today I started to check the pre-relocatoin DM PCI UART issue, but found it is now broken due to this commit. The broken part is at ns16550_serial_ofdata_to_platdata() in drivers/serial/ns16550.c, in which it has:
    addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
#ifdef CONFIG_PCI
    if (addr == FDT_ADDR_T_NONE) {
...
Before commit 5b34436, the old behavior is that the call to fdtdec_get_addr() returns FDT_ADDR_T_NONE so that we can trap into the PCI logic. But with commit 5b34436, addr is now zero which just bypass this logic.
As for why addr is now zero, this is because fdtdec_get_number() can only handle a 64-bit number at most. However for PCI reg, it has 3 cells. So if I have the following encoding:
Quoted 8 lines
reg = <0x00025100 0x0 0x0 0x0 0x0
           0x01025110 0x0 0x0 0x0 0x0>;
The addr will be assigned to zero after two rounds of left shift by 32-bit.
I can certainly change ns16550 driver to test the return value against 0 now, but I think this fdtdec_get_addr() does not cover all cases. Please advise.

Quoted 7 lines

This type of simple address parsing breaks down in the face of PCI. PCI requires 3 address cells because it has different types of address spaces. fdtdec_get_address() actually does the right thing here. It returns the "address" associated with the entry. The address is the 64-bit value you get by concatenating cells 0 and 1. Cell 2 contains additional meta data such as the PCI address and the type of memory that you are dealing with (configuration space, I/O, memory-mapped).

I think you wanted to say fdtdec_get_address() returns cell 1 and 2 and cell 0 contains the meta data.

Quoted 3 lines

I'd suggest that we add code to properly check whether this is a PCI device.

fdtdec_get_number() will only get the last 2 cells into the returned addr. If we have more than 2 cells (in the PCI case), we end up get cell 1 and 2 which is zero. And if we have 4 cells, we will get cell 2 and 3.

Quoted 4 lines

Also, why isn't this code obtaining the memory address from the base address registers?

I think we discussed this before. For simplification, we use device tree to pass the BAR number to the driver, otherwise we will end up dealing with a large code block of PCI vendor ID and device ID switch/case to determine which BAR we should read.

To fix this, I think I can create a dedicated PCI UART driver and move the PCI logic codes from ns16550_serial_ofdata_to_platdata() to the new driver.

Regards,
Bin

Thierry Reding

2015-08-14 14:06 UTC #16 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Quoted 506 lines · Bin Meng

Hi Thierry,
Quoted 495 lines · Thierry Reding
Quoted 485 lines · Bin Meng
Hi,
Quoted 444 lines · Simon Glass
Hi Stephen,
Quoted 367 lines · Stephen Warren
Quoted 355 lines · Simon Glass
Hi Stephen,
Quoted 343 lines · Stephen Warren
Quoted 161 lines · Simon Glass
Hi Stephen,
Quoted 100 lines · Stephen Warren
Quoted 57 lines · Simon Glass
Hi Stephen,
Quoted 38 lines · Stephen Warren
Quoted 13 lines · Simon Glass
This reverts commit 5b34436035fc862b5e8d0d2c3eab74ba36f1a7f4.
This function has a few problems. It calls fdt_parent_offset() which as
mentioned in code review is very slow.
https://patchwork.ozlabs.org/patch/499482/
https://patchwork.ozlabs.org/patch/452604/
Quoted 7 lines
It also happens to break SPI flash on Minnowboard max which is how I noticed
that this was applied. I can send a patch to tidy that up, but in any
case
I think we should consider a revert until the function is better implemented.
The fact that the function needs to perform a slow operation is not a good
reason for a revert. The slowness of the operation is just a matter of
fact
with DT not having uplinks in its data structure, and U-Boot using those
data structures directly.
Quoted 8 lines
You'd requested during review that I additionally implement a faster version
of the function in the case where the parent node is already known, and
said
it was fine if that happened in a later patch. I have this on my TODO list,
but it's only been a couple of days.
Quoted 1 line
I didn't expect this to go to mainline before your new patch.
I didn't get that message from the thread; you wrote:
Quoted 21 lines · Simon Glass
Quoted 8 lines · Stephen Warren
Quoted 5 lines · Simon Glass
Also, how about (in addition) a
version of this function that works for devices? Like:
Quoted 7 lines
Quoted 3 lines
device_get_addr_size(struct udevice *dev, ...)
so that it can handle this for you.
That sounds like a separate patch?
Quoted 8 lines
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 5 lines
That sounds to like you'd like a followup patch soon after this patch (my
assumption would be within a few days or weeks) to solve your comments, not
that you wanted a replacement patch.
I will take that feedback to be a little more forceful in future, sorry.
Quoted 34 lines
Quoted 8 lines
Quoted 4 lines
Why not just fix the bug since you said you could? That seems far better
than breaking the newly added Tegra210 support.
Quoted 5 lines
I do have a patch but I'm not 100% comfortable with the approach. I don't see a good reason to move away from the idea of fdt_addr_t and fdt_addr_t being set correctly for the platform. Or maybe I misunderstand the problem the patch was trying to fix. As I said it did not have a commit message, so who knows :-)
The size of fdt_addr_t isn't relevant *at all* when parsing DT. The only
thing that matters is #address-cells/#size-cells. Those properties are what
tell the parsing code how many bytes to read from the reg property. Whether
the resultant value fits into the code's internal representation is an internal detail of the code, not part of the semantics of DT itself or how
to parse it.
If code assumes that #address-cells == sizeof(fdt_addr_t), it is indeed quite likely that everything will just happen to work most of the time. However, this is purely an accident and not something that anything should
rely upon.
Quoted 9 lines
(I think Tegra210 support still has CONFIG_PHYS_64BIT undefined which is
admittedly a little /unexpected/ for a 64-bit U-Boot build, but in practice
is perfectly /legal/ and will work out just fine since, except perhaps for
RAM sizes, I don't believe any value in DT will actually require more than
32-bits to represent)
Quoted 3 lines
I would like to have the types match the platform where possible. At least the types should not be smaller than the platform - e.g. if CONFIG_PHYS_64BIT is not defined we should not support 64-bit
In general, there's no "should not" here; we "cannot". If there's a 64-bit
value in the DT (with bits above bit 31 set), then it can't be stored in a
32-bit variable.
That said, if a DT has #address-cells=<2>, but the actual values stored in
all reg values have 0 for the MS word, that'll actually work just fine. Note
that it's 100% legal to set #address-cells=<100> and just write a bunch of
extra zeros into the property. Silly perhaps, but perfectly legal. Since the
function should adapt to whatever #address-cells value is in the DT, supporting that case isn't any more work, so there's no reason we shouldn't.
Quoted 3 lines
address/size in the device tree. This is for efficiency. We don't want to force all the U-Boot code to 64-bit suddenly. This will bloat things for no benefit.
We could and likely should set CONFIG_PHYS_64BIT for Tegra210. However, that's unrelated to using the correct algorithm to parse DT.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
P.S. What is the issue with SPI flash? The commit description doesn't mention this at all.
It calls that function expecting it to pick up an address and size from two consecutive cells. With this patch, that fails (unless the property happens to be "reg").
...
Quoted 22 lines
Quoted 14 lines
I think this is all stemming from drivers/mtd/spi/sf_probe.c spi_flash_decode_fdt()'s call to fdtdec_get_addr_size() for property "memory-map"? If so, then looking at arch/x86/dts/minnowmax.dts's /spi node,
the code and DT content are clearly inconsistent; For this node #address-cells=<1>, #size-cells=<0> which makes sense given that the address
is a chip-select and hence has no size. So, the code should not assume that
the memory-map property can be parsed in the same way as a reg property.
Quoted 5 lines
I note that the memory-map property doesn't exist in the Linux kernel's DT
binding documentation database, or code, hence hasn't been reviewed by the
DT binding maintainers.
Quoted 6 lines
The function comment says:
* Look up an address property in a node and return it as an address.
* The property must hold one address with a length. This is only
tested
* on 32-bit machines.
And Thierry fixed it for systems with #address-cells > 1. Perhaps that part
of the function comment should have been removed in the commit.
Quoted 5 lines
My intention was that this would be an efficient way to decode an address and size from a device tree. To some extent regmap may take over this role (IMO we should turn to drop fdtdec one day one we have more infrastructure). But I'd like it to work efficiently for 32-bit machines. The new function could hardly be less efficient.
Again, there's no way in general to make it more efficient. The efficiency
issue is directly implied by the DT data structures.
In the special case where the parent node is already known, we can certainly
introduce an alternate function that is more efficient. You've already asked
for that, and as I said, it's in my TODO list.
Quoted 4 lines
I think we should consider the case where the physical address size of U-Boot and the device tree do not match as a corner case. I certainly don't want device tree to add loads of pointless code for 'normal' platforms.
It's not a corner case. It's a fundamental part of the DT schema. If U-Boot
is going to use DT, it should actually use *DT*, not some-very similar-but-not-quite-DT format with all kinds of implicit and unstated exceptions, limitations, and assumptions. This implies fully honoring all aspects of how DT works when parsing it, not picking and choosing features
because some are inconvenient or annoying. If U-Boot doesn't want to correctly implement DT support, it should just drop it completely.
As an aside, when instantiating devices, I hope one day that U-Boot will parse DT top-down in a hierarchical way, rather than simply searching for any node anywhere in the DT without regard for whether any parent node is enabled, has a driver, has had the driver initialize, etc. U-Boot ignores this right now, and is only getting away with this accidentally. Without hacky workarounds in drivers, this won't continue to work for all Tegra HW
(e.g. host1x graphics/display sub-modules, AHUB's audio-related sub-modules), since parent drivers must initialize before child drivers in
order to enable various register buses, including clocks/resets affecting those buses etc.
Again, if it's simply too inconvenient or bloated to implement DT properly
in U-Boot, let's just drop it entirely. A halfway solution is the worst of
both worlds. I'm not convinced the full implications of how to (and the need
to) correctly and fully support DT have were well thought through before (control) DT support was added to U-Boot.
Quoted 6 lines
Re memory-map, yes it doesn't seem to be possible to do what it is trying to do (and Thierry says the same below). It is quite weird to have a SPI peripheral which is also memory mapped.
Here's my question - if you fix the CONFIG_PHYS_64BIT problem with 64-bit Tegra, what is actually wrong with the way the function was?
With the DT files now checked into U-Boot, I think it would accidentally work, since we just happen to have set #address-cells=<2>, #size-cells=<2>,
and that would just happen to match sizeof(fdt_addr_t).
However note this is an accident on a couple of levels:
a) This is because the code assumes that sizeof(fdt_addr_t) == #address-cells * 4. This is an invalid assumption since it does not correctly honor the DT schema. It hard-codes the size of values whereas DT
schema says the size is defined by the #xxx-cells properties.
b) The original Tegra210 DTs that TomW posted had #address-cells=<1>, #size-cells=<1>. I asked him to change that to match what I expected to be
in the Linux kernel's Tegra210 DTs. However, if he'd rejected my request or
I hadn't noticed that, then with CONFIG_PHYS_64BIT set, fdtdec_get_addr_size() would have attempted to read twice as many bytes as
it should have from the property. It's entirely plausible that someone could
have come along later and realized CONFIG_PHYS_64BIT was set incorrectly and
enabled it.
Quoted 2 lines
This is a boot loader so we should be willing to make some simplifications.
When dealing with internal bootloader details, sure assumptions, simplifications, etc. can be made.
Quoted 6 lines
However, DT is an externally defined standard. The content of DT must be identical across all OSs (SW stacks, bootloader) and not influenced by requirements/... of any specific individual OS's (SW stack, bootloader) quirks. We can't just pick and choose which parts of it we care about. Well,
perhaps if we stop calling it DT we could.
Quoted 3 lines
So I think in summary:
- 64-bit machines should have CONFIG_PHYS_64BIT set correctly
It turns out that arch/arm/include/asm/config.h already enables this for all ARM64 platforms.
As such, we can in fact go ahead with reverting this patch, and U-Boot will still function on Tegra210 boards.
Quoted 6 lines
In the short term, I think that means TomR should just apply this revert patch, and we don't need to send any additional patches. In the slightly longer term, we should add some comments to fdtdec_get_addr_size() describing its problems, and slightly longer term, add back Thierry's patch, but in a way that lets callers specify whether #address-cells/#size-cells should be used, or whether caller-supplied hard-coded values should be used.
OK great. But I see your new patch so I think we can apply both at the same time.
Quoted 5 lines
I apologize for not noticing this earlier; I'd assumed that since none of the Tegra-specific files in include/configs/ set this flag, nor any of the Tegra-specific Kconfig files, it wasn't set, and hence a revert of the patch would break Tegra210 support.
No problem - I assumed it would also.
Quoted 23 lines
Quoted 2 lines
- then fdtdec_get_addr_size() would work as expected
I take issue with *works* as expected", since I would expect the function to implement DT parsing rules completely.
However, it's certainly true to say that it will generate the desired results, even if it doesn't /work/ (isn't implemented) as expected.
(I suppose this depends on whether you're talking about "works" w.r.t to correctness of the returned results or side-effects, or w.r.t. how the internal implementation works.)
Quoted 7 lines
- I want to make this cases as efficient as possible since it will be called in SPL
- You are concerned that making assumptions like this violates the DT spec
One option is to split the functions into two - one that works in SPL and makes assumptions, and one that does not and does things the hard way.
Quoted 10 lines
Why should SPL be any different? U-Boot should either parse DT correctly or not at all. SPL shouldn't be a special case. Admittedly SPL on Tegra does very little (and isn't even present on Tegra210), but in general, can't someone use storage drivers, filesystems, etc. in SPL to choose the next stage to load, read GPIOs or other data sources to select between different boot paths, perhaps even interact with a user? If so, then assuming that SPL can somehow implements a reduced set of features, and hence can make assumptions that non-SPL can't, seems quite dangerous. We could only do that if we put some active checks in the U-Boot makefiles to ensure that nobody enabled anything in the SPL besides a set of strictly audited features.
Yes we could do that and it would avoid pain later. I suppose SPL on Tegra is a bit of a special case since there is really no size limit. For some chips the limits are pretty severe so I am quite sensitive to code size even at the expense of extra debug time when something breaks.
Quoted 16 lines
Quoted 5 lines
I suppose we could also add checks/warnings that the DT is
'well-formed' and that the address size matches the machine it is running on.
Yes, we certainly should do that.
Quoted 5 lines
In any case we do need to get rid of the parent lookup in this function. So can either you or Thierry submit a patch to do this? The parent should instead be a parameter to the function. You may need to create a stub function which looks it up before calling fdtdec_get_addr_size().
Quoted 17 lines
Quoted 3 lines
Yes, we can't remove the parent lookup in all cases. However, we can avoid it in the cases where the caller can supply it. I think that's most, but not quite all.
My main concern is dev_get_addr() since that is the official way of reading a device address now. See also regmap_init_mem() which does things its own way.
Quoted 8 lines
Quoted 7 lines
I'll see how things look in SPL.
Regards,
Simon
Do we have any conclusion about commit 5b34436? Today I started to check the pre-relocatoin DM PCI UART issue, but found it is now broken due to this commit. The broken part is at ns16550_serial_ofdata_to_platdata() in drivers/serial/ns16550.c, in which it has:
    addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
#ifdef CONFIG_PCI
    if (addr == FDT_ADDR_T_NONE) {
...
Before commit 5b34436, the old behavior is that the call to fdtdec_get_addr() returns FDT_ADDR_T_NONE so that we can trap into the PCI logic. But with commit 5b34436, addr is now zero which just bypass this logic.
As for why addr is now zero, this is because fdtdec_get_number() can only handle a 64-bit number at most. However for PCI reg, it has 3 cells. So if I have the following encoding:
Quoted 8 lines
reg = <0x00025100 0x0 0x0 0x0 0x0
           0x01025110 0x0 0x0 0x0 0x0>;
The addr will be assigned to zero after two rounds of left shift by 32-bit.
I can certainly change ns16550 driver to test the return value against 0 now, but I think this fdtdec_get_addr() does not cover all cases. Please advise.
Quoted 7 lines
This type of simple address parsing breaks down in the face of PCI. PCI requires 3 address cells because it has different types of address spaces. fdtdec_get_address() actually does the right thing here. It returns the "address" associated with the entry. The address is the 64-bit value you get by concatenating cells 0 and 1. Cell 2 contains additional meta data such as the PCI address and the type of memory that you are dealing with (configuration space, I/O, memory-mapped).

Quoted 2 lines

I think you wanted to say fdtdec_get_address() returns cell 1 and 2 and cell 0 contains the meta data.

That depends on how you look at it. But yes, I think we're talking about the same thing.

Quoted 7 lines

Quoted 2 lines
I'd suggest that we add code to properly check whether this is a PCI device.
fdtdec_get_number() will only get the last 2 cells into the returned addr. If we have more than 2 cells (in the PCI case), we end up get cell 1 and 2 which is zero. And if we have 4 cells, we will get cell 2 and 3.

Yes. All the more reason to special-case here. fdtdec_get_address() here will inevitably lead to pain for anything that exceeds 64-bit (2 cells).

Quoted 8 lines

Quoted 3 lines
Also, why isn't this code obtaining the memory address from the base address registers?
I think we discussed this before. For simplification, we use device tree to pass the BAR number to the driver, otherwise we will end up dealing with a large code block of PCI vendor ID and device ID switch/case to determine which BAR we should read.

That's really abusing DT, but like you said, I think we discussed this before and disagreed the same way before. =)

Bin Meng

2015-08-14 14:29 UTC #17 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Hi Thierry,

Quoted 528 lines · Thierry Reding

Quoted 506 lines · Bin Meng
Hi Thierry,
Quoted 495 lines · Thierry Reding
Quoted 485 lines · Bin Meng
Hi,
Quoted 444 lines · Simon Glass
Hi Stephen,
Quoted 367 lines · Stephen Warren
Quoted 355 lines · Simon Glass
Hi Stephen,
Quoted 343 lines · Stephen Warren
Quoted 161 lines · Simon Glass
Hi Stephen,
Quoted 100 lines · Stephen Warren
Quoted 57 lines · Simon Glass
Hi Stephen,
Quoted 38 lines · Stephen Warren
Quoted 20 lines · Simon Glass
This reverts commit 5b34436035fc862b5e8d0d2c3eab74ba36f1a7f4.
This function has a few problems. It calls fdt_parent_offset() which
as
mentioned in code review is very slow.
https://patchwork.ozlabs.org/patch/499482/
https://patchwork.ozlabs.org/patch/452604/
It also happens to break SPI flash on Minnowboard max which is how I
noticed
that this was applied. I can send a patch to tidy that up, but in
any
case
I think we should consider a revert until the function is better
implemented.
The fact that the function needs to perform a slow operation is not a good
reason for a revert. The slowness of the operation is just a matter of
fact
with DT not having uplinks in its data structure, and U-Boot using those
data structures directly.
Quoted 8 lines
You'd requested during review that I additionally implement a faster version
of the function in the case where the parent node is already known, and
said
it was fine if that happened in a later patch. I have this on my TODO list,
but it's only been a couple of days.
Quoted 1 line
I didn't expect this to go to mainline before your new patch.
I didn't get that message from the thread; you wrote:
Quoted 21 lines · Simon Glass
Quoted 8 lines · Stephen Warren
Quoted 5 lines · Simon Glass
Also, how about (in addition) a
version of this function that works for devices? Like:
Quoted 7 lines
Quoted 3 lines
device_get_addr_size(struct udevice *dev, ...)
so that it can handle this for you.
That sounds like a separate patch?
Quoted 8 lines
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 5 lines
That sounds to like you'd like a followup patch soon after this patch (my
assumption would be within a few days or weeks) to solve your comments, not
that you wanted a replacement patch.
I will take that feedback to be a little more forceful in future, sorry.
Quoted 34 lines
Quoted 8 lines
Quoted 4 lines
Why not just fix the bug since you said you could? That seems far better
than breaking the newly added Tegra210 support.
Quoted 5 lines
I do have a patch but I'm not 100% comfortable with the approach. I don't see a good reason to move away from the idea of fdt_addr_t and fdt_addr_t being set correctly for the platform. Or maybe I misunderstand the problem the patch was trying to fix. As I said it did not have a commit message, so who knows :-)
The size of fdt_addr_t isn't relevant *at all* when parsing DT. The only
thing that matters is #address-cells/#size-cells. Those properties are what
tell the parsing code how many bytes to read from the reg property. Whether
the resultant value fits into the code's internal representation is an internal detail of the code, not part of the semantics of DT itself or how
to parse it.
If code assumes that #address-cells == sizeof(fdt_addr_t), it is indeed quite likely that everything will just happen to work most of the time. However, this is purely an accident and not something that anything should
rely upon.
Quoted 9 lines
(I think Tegra210 support still has CONFIG_PHYS_64BIT undefined which is
admittedly a little /unexpected/ for a 64-bit U-Boot build, but in practice
is perfectly /legal/ and will work out just fine since, except perhaps for
RAM sizes, I don't believe any value in DT will actually require more than
32-bits to represent)
Quoted 3 lines
I would like to have the types match the platform where possible. At least the types should not be smaller than the platform - e.g. if CONFIG_PHYS_64BIT is not defined we should not support 64-bit
In general, there's no "should not" here; we "cannot". If there's a 64-bit
value in the DT (with bits above bit 31 set), then it can't be stored in a
32-bit variable.
That said, if a DT has #address-cells=<2>, but the actual values stored in
all reg values have 0 for the MS word, that'll actually work just fine. Note
that it's 100% legal to set #address-cells=<100> and just write a bunch of
extra zeros into the property. Silly perhaps, but perfectly legal. Since the
function should adapt to whatever #address-cells value is in the DT, supporting that case isn't any more work, so there's no reason we shouldn't.
Quoted 3 lines
address/size in the device tree. This is for efficiency. We don't want to force all the U-Boot code to 64-bit suddenly. This will bloat things for no benefit.
We could and likely should set CONFIG_PHYS_64BIT for Tegra210. However, that's unrelated to using the correct algorithm to parse DT.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
P.S. What is the issue with SPI flash? The commit description doesn't mention this at all.
It calls that function expecting it to pick up an address and size from two consecutive cells. With this patch, that fails (unless the property happens to be "reg").
...
Quoted 22 lines
Quoted 14 lines
I think this is all stemming from drivers/mtd/spi/sf_probe.c spi_flash_decode_fdt()'s call to fdtdec_get_addr_size() for property "memory-map"? If so, then looking at arch/x86/dts/minnowmax.dts's /spi node,
the code and DT content are clearly inconsistent; For this node #address-cells=<1>, #size-cells=<0> which makes sense given that the address
is a chip-select and hence has no size. So, the code should not assume that
the memory-map property can be parsed in the same way as a reg property.
Quoted 5 lines
I note that the memory-map property doesn't exist in the Linux kernel's DT
binding documentation database, or code, hence hasn't been reviewed by the
DT binding maintainers.
Quoted 6 lines
The function comment says:
* Look up an address property in a node and return it as an address.
* The property must hold one address with a length. This is only
tested
* on 32-bit machines.
And Thierry fixed it for systems with #address-cells > 1. Perhaps that part
of the function comment should have been removed in the commit.
Quoted 5 lines
My intention was that this would be an efficient way to decode an address and size from a device tree. To some extent regmap may take over this role (IMO we should turn to drop fdtdec one day one we have more infrastructure). But I'd like it to work efficiently for 32-bit machines. The new function could hardly be less efficient.
Again, there's no way in general to make it more efficient. The efficiency
issue is directly implied by the DT data structures.
In the special case where the parent node is already known, we can certainly
introduce an alternate function that is more efficient. You've already asked
for that, and as I said, it's in my TODO list.
Quoted 4 lines
I think we should consider the case where the physical address size of U-Boot and the device tree do not match as a corner case. I certainly don't want device tree to add loads of pointless code for 'normal' platforms.
It's not a corner case. It's a fundamental part of the DT schema. If U-Boot
is going to use DT, it should actually use *DT*, not some-very similar-but-not-quite-DT format with all kinds of implicit and unstated exceptions, limitations, and assumptions. This implies fully honoring all aspects of how DT works when parsing it, not picking and choosing features
because some are inconvenient or annoying. If U-Boot doesn't want to correctly implement DT support, it should just drop it completely.
As an aside, when instantiating devices, I hope one day that U-Boot will parse DT top-down in a hierarchical way, rather than simply searching for any node anywhere in the DT without regard for whether any parent node is enabled, has a driver, has had the driver initialize, etc. U-Boot ignores this right now, and is only getting away with this accidentally. Without hacky workarounds in drivers, this won't continue to work for all Tegra HW
(e.g. host1x graphics/display sub-modules, AHUB's audio-related sub-modules), since parent drivers must initialize before child drivers in
order to enable various register buses, including clocks/resets affecting those buses etc.
Again, if it's simply too inconvenient or bloated to implement DT properly
in U-Boot, let's just drop it entirely. A halfway solution is the worst of
both worlds. I'm not convinced the full implications of how to (and the need
to) correctly and fully support DT have were well thought through before (control) DT support was added to U-Boot.
Quoted 6 lines
Re memory-map, yes it doesn't seem to be possible to do what it is trying to do (and Thierry says the same below). It is quite weird to have a SPI peripheral which is also memory mapped.
Here's my question - if you fix the CONFIG_PHYS_64BIT problem with 64-bit Tegra, what is actually wrong with the way the function was?
With the DT files now checked into U-Boot, I think it would accidentally work, since we just happen to have set #address-cells=<2>, #size-cells=<2>,
and that would just happen to match sizeof(fdt_addr_t).
However note this is an accident on a couple of levels:
a) This is because the code assumes that sizeof(fdt_addr_t) == #address-cells * 4. This is an invalid assumption since it does not correctly honor the DT schema. It hard-codes the size of values whereas DT
schema says the size is defined by the #xxx-cells properties.
b) The original Tegra210 DTs that TomW posted had #address-cells=<1>, #size-cells=<1>. I asked him to change that to match what I expected to be
in the Linux kernel's Tegra210 DTs. However, if he'd rejected my request or
I hadn't noticed that, then with CONFIG_PHYS_64BIT set, fdtdec_get_addr_size() would have attempted to read twice as many bytes as
it should have from the property. It's entirely plausible that someone could
have come along later and realized CONFIG_PHYS_64BIT was set incorrectly and
enabled it.
Quoted 2 lines
This is a boot loader so we should be willing to make some simplifications.
When dealing with internal bootloader details, sure assumptions, simplifications, etc. can be made.
Quoted 6 lines
However, DT is an externally defined standard. The content of DT must be identical across all OSs (SW stacks, bootloader) and not influenced by requirements/... of any specific individual OS's (SW stack, bootloader) quirks. We can't just pick and choose which parts of it we care about. Well,
perhaps if we stop calling it DT we could.
Quoted 3 lines
So I think in summary:
- 64-bit machines should have CONFIG_PHYS_64BIT set correctly
It turns out that arch/arm/include/asm/config.h already enables this for all ARM64 platforms.
As such, we can in fact go ahead with reverting this patch, and U-Boot will still function on Tegra210 boards.
Quoted 6 lines
In the short term, I think that means TomR should just apply this revert patch, and we don't need to send any additional patches. In the slightly longer term, we should add some comments to fdtdec_get_addr_size() describing its problems, and slightly longer term, add back Thierry's patch, but in a way that lets callers specify whether #address-cells/#size-cells should be used, or whether caller-supplied hard-coded values should be used.
OK great. But I see your new patch so I think we can apply both at the same time.
Quoted 5 lines
I apologize for not noticing this earlier; I'd assumed that since none of the Tegra-specific files in include/configs/ set this flag, nor any of the Tegra-specific Kconfig files, it wasn't set, and hence a revert of the patch would break Tegra210 support.
No problem - I assumed it would also.
Quoted 23 lines
Quoted 2 lines
- then fdtdec_get_addr_size() would work as expected
I take issue with *works* as expected", since I would expect the function to implement DT parsing rules completely.
However, it's certainly true to say that it will generate the desired results, even if it doesn't /work/ (isn't implemented) as expected.
(I suppose this depends on whether you're talking about "works" w.r.t to correctness of the returned results or side-effects, or w.r.t. how the internal implementation works.)
Quoted 7 lines
- I want to make this cases as efficient as possible since it will be called in SPL
- You are concerned that making assumptions like this violates the DT spec
One option is to split the functions into two - one that works in SPL and makes assumptions, and one that does not and does things the hard way.
Quoted 10 lines
Why should SPL be any different? U-Boot should either parse DT correctly or not at all. SPL shouldn't be a special case. Admittedly SPL on Tegra does very little (and isn't even present on Tegra210), but in general, can't someone use storage drivers, filesystems, etc. in SPL to choose the next stage to load, read GPIOs or other data sources to select between different boot paths, perhaps even interact with a user? If so, then assuming that SPL can somehow implements a reduced set of features, and hence can make assumptions that non-SPL can't, seems quite dangerous. We could only do that if we put some active checks in the U-Boot makefiles to ensure that nobody enabled anything in the SPL besides a set of strictly audited features.
Yes we could do that and it would avoid pain later. I suppose SPL on Tegra is a bit of a special case since there is really no size limit. For some chips the limits are pretty severe so I am quite sensitive to code size even at the expense of extra debug time when something breaks.
Quoted 16 lines
Quoted 5 lines
I suppose we could also add checks/warnings that the DT is
'well-formed' and that the address size matches the machine it is running on.
Yes, we certainly should do that.
Quoted 5 lines
In any case we do need to get rid of the parent lookup in this function. So can either you or Thierry submit a patch to do this? The parent should instead be a parameter to the function. You may need to create a stub function which looks it up before calling fdtdec_get_addr_size().
Quoted 17 lines
Quoted 3 lines
Yes, we can't remove the parent lookup in all cases. However, we can avoid it in the cases where the caller can supply it. I think that's most, but not quite all.
My main concern is dev_get_addr() since that is the official way of reading a device address now. See also regmap_init_mem() which does things its own way.
Quoted 8 lines
Quoted 7 lines
I'll see how things look in SPL.
Regards,
Simon
Do we have any conclusion about commit 5b34436? Today I started to check the pre-relocatoin DM PCI UART issue, but found it is now broken due to this commit. The broken part is at ns16550_serial_ofdata_to_platdata() in drivers/serial/ns16550.c, in which it has:
    addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
#ifdef CONFIG_PCI
    if (addr == FDT_ADDR_T_NONE) {
...
Before commit 5b34436, the old behavior is that the call to fdtdec_get_addr() returns FDT_ADDR_T_NONE so that we can trap into the PCI logic. But with commit 5b34436, addr is now zero which just bypass this logic.
As for why addr is now zero, this is because fdtdec_get_number() can only handle a 64-bit number at most. However for PCI reg, it has 3 cells. So if I have the following encoding:
Quoted 8 lines
reg = <0x00025100 0x0 0x0 0x0 0x0
           0x01025110 0x0 0x0 0x0 0x0>;
The addr will be assigned to zero after two rounds of left shift by 32-bit.
I can certainly change ns16550 driver to test the return value against 0 now, but I think this fdtdec_get_addr() does not cover all cases. Please advise.
Quoted 7 lines
This type of simple address parsing breaks down in the face of PCI. PCI requires 3 address cells because it has different types of address spaces. fdtdec_get_address() actually does the right thing here. It returns the "address" associated with the entry. The address is the 64-bit value you get by concatenating cells 0 and 1. Cell 2 contains additional meta data such as the PCI address and the type of memory that you are dealing with (configuration space, I/O, memory-mapped).
Quoted 2 lines
I think you wanted to say fdtdec_get_address() returns cell 1 and 2 and cell 0 contains the meta data.
That depends on how you look at it. But yes, I think we're talking about the same thing.
Quoted 7 lines
Quoted 2 lines
I'd suggest that we add code to properly check whether this is a PCI device.
fdtdec_get_number() will only get the last 2 cells into the returned addr. If we have more than 2 cells (in the PCI case), we end up get cell 1 and 2 which is zero. And if we have 4 cells, we will get cell 2 and 3.
Yes. All the more reason to special-case here. fdtdec_get_address() here will inevitably lead to pain for anything that exceeds 64-bit (2 cells).
Quoted 4 lines
Quoted 3 lines
Also, why isn't this code obtaining the memory address from the base address registers?

Quoted 8 lines

Quoted 4 lines
I think we discussed this before. For simplification, we use device tree to pass the BAR number to the driver, otherwise we will end up dealing with a large code block of PCI vendor ID and device ID switch/case to determine which BAR we should read.
That's really abusing DT, but like you said, I think we discussed this before and disagreed the same way before. =)

Well, I am open for discussions. I don't think current implementation is abusing DT. IMHO, DT is only suitable for describing devices that are not probable. For PCI devices (not the PCI host controller) since they have a unique vendor ID & device ID pair, we can hardcoded all those stuff in the driver itself. We don't need specify anything in the device tree (like BAR number, b.d.f etc). Current PCI <reg> bindings comes from Open Firmware spec. I believe the original intent was to have bootloader fill in the <reg> property with bootloader configured values and pass it to the OS so that OS does not need re-configure the bus. Well, I would call abusing DT that we describe a PCI device in the device tree. We shouldn't do that.

Anyway, given we have driver model PCI support now, I think I can create a new PCI ns16550 driver using driver model. In the new driver, I can continue using current implementation to get the BAR, but if anyone thinks that we should never using device tree to describe a PCI device, I can do a vendor ID & device ID match game :-) Be warned that the PCI ns16550 driver may get bigger and bigger (see Linux driver 8250_pci.c and you know what I am talking about)

Regards,
Bin

Michal Suchanek

2015-08-14 09:01 UTC #18 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

In reply to Bin Meng

Quoted 20 lines · Bin Meng

Do we have any conclusion about commit 5b34436? Today I started to check the pre-relocatoin DM PCI UART issue, but found it is now broken due to this commit. The broken part is at ns16550_serial_ofdata_to_platdata() in drivers/serial/ns16550.c, in which it has:
    addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
#ifdef CONFIG_PCI
    if (addr == FDT_ADDR_T_NONE) {
...
Before commit 5b34436, the old behavior is that the call to fdtdec_get_addr() returns FDT_ADDR_T_NONE so that we can trap into the PCI logic. But with commit 5b34436, addr is now zero which just bypass this logic.
As for why addr is now zero, this is because fdtdec_get_number() can only handle a 64-bit number at most. However for PCI reg, it has 3 cells. So if I have the following encoding:

Quoted 9 lines

reg = <0x00025100 0x0 0x0 0x0 0x0
           0x01025110 0x0 0x0 0x0 0x0>;
The addr will be assigned to zero after two rounds of left shift by 32-bit.
I can certainly change ns16550 driver to test the return value against 0 now, but I think this fdtdec_get_addr() does not cover all cases. Please advise.

Hello,

What do you expect the fdtdec_get_addr to do?

Any 32bit (or 64bit number on 64bit archs) is valid return value so there is no possibility to return an error. This is probably a problem with the interface. If there is more than can fit or there is an error you will never know.

eg. Linux has this code for decoding numbers which can have 1-2 cells:

                reg = of_get_property(pp, "reg", &len);
                if (!reg) {

blah
}

a_cells = of_n_addr_cells(pp); /* scan parents for
#address-cells */

                s_cells = of_n_size_cells(pp);
                (*pparts)[i].offset = of_read_number(reg, a_cells);
                (*pparts)[i].size = of_read_number(reg + a_cells, s_cells);

If you read an address that can be 3 cells then you need a function that returns cell array rather than a single integer. Or you can check the address length and decide if you can fit that into an integer on your platform.

Thanks

Michal

Bin Meng

2015-08-14 09:08 UTC #19 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Hi Michal,

Quoted 32 lines · Michal Suchanek

Quoted 20 lines · Bin Meng
Do we have any conclusion about commit 5b34436? Today I started to check the pre-relocatoin DM PCI UART issue, but found it is now broken due to this commit. The broken part is at ns16550_serial_ofdata_to_platdata() in drivers/serial/ns16550.c, in which it has:
    addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
#ifdef CONFIG_PCI
    if (addr == FDT_ADDR_T_NONE) {
...
Before commit 5b34436, the old behavior is that the call to fdtdec_get_addr() returns FDT_ADDR_T_NONE so that we can trap into the PCI logic. But with commit 5b34436, addr is now zero which just bypass this logic.
As for why addr is now zero, this is because fdtdec_get_number() can only handle a 64-bit number at most. However for PCI reg, it has 3 cells. So if I have the following encoding:
Quoted 9 lines
reg = <0x00025100 0x0 0x0 0x0 0x0
           0x01025110 0x0 0x0 0x0 0x0>;
The addr will be assigned to zero after two rounds of left shift by 32-bit.
I can certainly change ns16550 driver to test the return value against 0 now, but I think this fdtdec_get_addr() does not cover all cases. Please advise.

Quoted 8 lines

Hello,
What do you expect the fdtdec_get_addr to do?
Any 32bit (or 64bit number on 64bit archs) is valid return value so there is no possibility to return an error. This is probably a problem with the interface. If there is more than can fit or there is an error you will never know.

Agreed.

Quoted 15 lines

eg. Linux has this code for decoding numbers which can have 1-2 cells:

                reg = of_get_property(pp, "reg", &len);
                if (!reg) {

blah
}

a_cells = of_n_addr_cells(pp); /* scan parents for
#address-cells */

                s_cells = of_n_size_cells(pp);
                (*pparts)[i].offset = of_read_number(reg, a_cells);
                (*pparts)[i].size = of_read_number(reg + a_cells, s_cells);

Quoted 5 lines

If you read an address that can be 3 cells then you need a function that returns cell array rather than a single integer. Or you can check the address length and decide if you can fit that into an integer on your platform.

Yep, we have an API fdtdec_get_pci_addr() to parse a PCI reg. I was raising this because I see this broke the existing codes and there was discussion on reverting this commit.

Regards,
Bin

Simon Glass

author

2015-08-14 10:04 UTC #20 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Hi,

Quoted 67 lines · Bin Meng

Hi Michal,
Quoted 32 lines · Michal Suchanek
Quoted 20 lines · Bin Meng
Do we have any conclusion about commit 5b34436? Today I started to check the pre-relocatoin DM PCI UART issue, but found it is now broken due to this commit. The broken part is at ns16550_serial_ofdata_to_platdata() in drivers/serial/ns16550.c, in which it has:
    addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
#ifdef CONFIG_PCI
    if (addr == FDT_ADDR_T_NONE) {
...
Before commit 5b34436, the old behavior is that the call to fdtdec_get_addr() returns FDT_ADDR_T_NONE so that we can trap into the PCI logic. But with commit 5b34436, addr is now zero which just bypass this logic.
As for why addr is now zero, this is because fdtdec_get_number() can only handle a 64-bit number at most. However for PCI reg, it has 3 cells. So if I have the following encoding:
Quoted 9 lines
reg = <0x00025100 0x0 0x0 0x0 0x0
           0x01025110 0x0 0x0 0x0 0x0>;
The addr will be assigned to zero after two rounds of left shift by 32-bit.
I can certainly change ns16550 driver to test the return value against 0 now, but I think this fdtdec_get_addr() does not cover all cases. Please advise.
Quoted 8 lines
Hello,
What do you expect the fdtdec_get_addr to do?
Any 32bit (or 64bit number on 64bit archs) is valid return value so there is no possibility to return an error. This is probably a problem with the interface. If there is more than can fit or there is an error you will never know.
Agreed.
Quoted 15 lines
eg. Linux has this code for decoding numbers which can have 1-2 cells:
                reg = of_get_property(pp, "reg", &len);
                if (!reg) {
blah
}
a_cells = of_n_addr_cells(pp); /* scan parents for
#address-cells */
                s_cells = of_n_size_cells(pp);
                (*pparts)[i].offset = of_read_number(reg, a_cells);
                (*pparts)[i].size = of_read_number(reg + a_cells, s_cells);
Quoted 5 lines
If you read an address that can be 3 cells then you need a function that returns cell array rather than a single integer. Or you can check the address length and decide if you can fit that into an integer on your platform.

Quoted 3 lines

Yep, we have an API fdtdec_get_pci_addr() to parse a PCI reg. I was raising this because I see this broke the existing codes and there was discussion on reverting this commit.

I plan to apply this revert to u-boot-x86 (where SPI is currently broken) and (once it has a bit more testing) also this patch which I think makes the change in a safer way:

https://patchwork.ozlabs.org/patch/504918/

At present that patch breaks at least one x86 board and I have not dug into it yet.

The revert should not break tegra, according to Stephen.

Regards,
Simon

Tom Warren

2015-09-02 16:52 UTC #21 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Simon, et al,

Quoted 11 lines

-----Original Message-----
From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
Sent: Friday, August 14, 2015 3:05 AM
To: Bin Meng
Cc: Michal Suchanek; Tom Rini; Stephen Warren; U-Boot Mailing List; Tom Warren; Thierry Reding
Subject: Re: [U-Boot] [PATCH] Revert "fdt: Fix fdtdec_get_addr_size() for 64- bit"
Hi,

[snip]

Quoted 11 lines

I plan to apply this revert to u-boot-x86 (where SPI is currently broken) and (once it has a bit more testing) also this patch which I think makes the change in a safer way:
https://patchwork.ozlabs.org/patch/504918/
At present that patch breaks at least one x86 board and I have not dug into it yet.
The revert should not break tegra, according to Stephen.

Unfortunately, my testing on P2571 with TOT u-boot-tegra (rebased against TOT u-boot/master this morning) shows that that is not true.

The revert of the disputed 'fdtdec_get_addr_size' patch _does_ break Tegra 64-bit (P2571, at least). Nyan-big is OK. With Simon's revert in place, my board just loops on SPL signon, so I assume it's faulting, etc. in CPU init. Note that this is the current state of TOT u-boot/master.

If I 'revert' Simon's revert, I can boot my P2571 to the command prompt again. If I leave his revert of fdtdec_get_addr_size in, but use Stephen's rewrite patch (as listed by Simon above), I can again boot to the command prompt on my P2571 (haven't tested my Nyan-big yet).

I've got some patches pending in u-boot-tegra/master that I wanted to send a PR for, but I can't until I can assure that at least a small sample of boards (T124 and T210, in this case) still boot.

Since Stephen's patch isn't a Tegra patch, I don't think I should apply it to u-boot-tegra for the PR. I think it's best handled by TomR in u-boot/master, especially considering the controversy surrounding the previous attempt, and Simon's assertion that it still needs work for at least on x86 board.

Please put your heads together and solve this, since we're now broken on 64-bit T210 Tegra (Stephen - I know you are OOTO and may not be able to test this on your T210 boards, but I'd like someone else to verify my findings ASAP if possible).

Tom
--
nvpublic

Quoted 3 lines

Regards,
Simon

Simon Glass

author

2015-09-02 16:58 UTC #22 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Hi Tom,

Quoted 33 lines · Tom Warren

Simon, et al,
Quoted 11 lines
-----Original Message-----
From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
Sent: Friday, August 14, 2015 3:05 AM
To: Bin Meng
Cc: Michal Suchanek; Tom Rini; Stephen Warren; U-Boot Mailing List; Tom Warren; Thierry Reding
Subject: Re: [U-Boot] [PATCH] Revert "fdt: Fix fdtdec_get_addr_size() for 64- bit"
Hi,
[snip]
Quoted 11 lines
I plan to apply this revert to u-boot-x86 (where SPI is currently broken) and (once it has a bit more testing) also this patch which I think makes the change in a safer way:
https://patchwork.ozlabs.org/patch/504918/
At present that patch breaks at least one x86 board and I have not dug into it yet.
The revert should not break tegra, according to Stephen.
Unfortunately, my testing on P2571 with TOT u-boot-tegra (rebased against TOT u-boot/master this morning) shows that that is not true.
The revert of the disputed 'fdtdec_get_addr_size' patch _does_ break Tegra 64-bit (P2571, at least). Nyan-big is OK. With Simon's revert in place, my board just loops on SPL signon, so I assume it's faulting, etc. in CPU init. Note that this is the current state of TOT u-boot/master.

Quoted 7 lines

If I 'revert' Simon's revert, I can boot my P2571 to the command prompt again. If I leave his revert of fdtdec_get_addr_size in, but use Stephen's rewrite patch (as listed by Simon above), I can again boot to the command prompt on my P2571 (haven't tested my Nyan-big yet).
I've got some patches pending in u-boot-tegra/master that I wanted to send a PR for, but I can't until I can assure that at least a small sample of boards (T124 and T210, in this case) still boot.
Since Stephen's patch isn't a Tegra patch, I don't think I should apply it to u-boot-tegra for the PR. I think it's best handled by TomR in u-boot/master, especially considering the controversy surrounding the previous attempt, and Simon's assertion that it still needs work for at least on x86 board.
Please put your heads together and solve this, since we're now broken on 64-bit T210 Tegra (Stephen - I know you are OOTO and may not be able to test this on your T210 boards, but I'd like someone else to verify my findings ASAP if possible).

Oh dear.

I think the solution is to apply Stephen's patch. However that currently breaks other boards. I'm planning to figure out why but have not got to it yet. I'll make sure it gets done by early next week.

Regards,
Simon

Stephen Warren

2015-09-02 20:04 UTC #23 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

In reply to Tom Warren

Quoted 9 lines · Tom Warren

Simon, et al,
Quoted 7 lines
Simon Glass wrote at Friday, August 14, 2015 3:05 AM:
I plan to apply this revert to u-boot-x86 (where SPI is currently broken) and (once it has a bit more testing) also this patch which I think makes the change in a safer way:
https://patchwork.ozlabs.org/patch/504918/

Quoted 8 lines

Quoted 4 lines
At present that patch breaks at least one x86 board and I have not dug into it yet.
The revert should not break tegra, according to Stephen.
Unfortunately, my testing on P2571 with TOT u-boot-tegra (rebased against TOT u-boot/master this morning) shows that that is not true.
The revert of the disputed 'fdtdec_get_addr_size' patch _does_ break Tegra 64-bit (P2571, at least). Nyan-big is OK. With Simon's revert in place, my board just loops on SPL signon, so I assume it's faulting, etc. in CPU init. Note that this is the current state of TOT u-boot/master.

I'm a bit confused. So far, we don't support SPL on T210 since we assume some other bootloader runs on the boot CPU and starts just the main U-Boot on the main CPU. It sounds like you're testing some local-only SPL support?

Tom Warren

2015-09-02 20:39 UTC #24 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Quoted 28 lines

-----Original Message-----
From: Stephen Warren
Sent: Wednesday, September 02, 2015 1:05 PM
To: Tom Warren; Simon Glass
Cc: Bin Meng; Thierry Reding; Tom Rini; U-Boot Mailing List
Subject: Re: [U-Boot] [PATCH] Revert "fdt: Fix fdtdec_get_addr_size() for 64- bit"
Quoted 7 lines · Tom Warren
Simon, et al,
Quoted 5 lines
Simon Glass wrote at Friday, August 14, 2015 3:05 AM:
I plan to apply this revert to u-boot-x86 (where SPI is currently broken) and (once it has a bit more testing) also this patch which I think makes the change in a safer way:
Quoted 8 lines
Quoted 6 lines
https://patchwork.ozlabs.org/patch/504918/
At present that patch breaks at least one x86 board and I have not dug into it yet.
The revert should not break tegra, according to Stephen.
Unfortunately, my testing on P2571 with TOT u-boot-tegra (rebased against
TOT u-boot/master this morning) shows that that is not true.
Quoted 1 line

Quoted 8 lines

Quoted 1 line
The revert of the disputed 'fdtdec_get_addr_size' patch _does_ break Tegra
64-bit (P2571, at least). Nyan-big is OK. With Simon's revert in place, my board just loops on SPL signon, so I assume it's faulting, etc. in CPU init. Note that this is the current state of TOT u-boot/master.
I'm a bit confused. So far, we don't support SPL on T210 since we assume some other bootloader runs on the boot CPU and starts just the main U-Boot on the main CPU. It sounds like you're testing some local-only SPL support?

Currently there are a couple of ways to get T210 64-bit U-Boot loaded/executed. The first is the way I've always done it (during development and today) - use a 32-bit SPL that I built when I first ported 32-bit U-Boot to T210. I've saved away the SPL portion as a binary, and combine it with the current 64-bit T210 U-Boot proper when building my image. It's always worked up to now. What I'm seeing is a failure in the 64-bit CPU U-Boot portion. I just mentioned the looping SPL signon symptom because that's what I see as the indicator of a broken 64-bit image.

The other way is to use your proprietary NV bootloader for the 32-bit portion (this will become the de facto standard when we release said NV bootloader code as open-source, or a binary first-stage loader 'tool'). I haven't tried that, since my way works and is an easy part of my workflow.

If you can point me to your boot CPU loader internally, I can try your method and see if it makes a difference, but I doubt it will.

Tom
--
nvpublic

Stephen Warren

2015-09-02 20:54 UTC #25 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Quoted 33 lines · Tom Warren

Quoted 33 lines
-----Original Message-----
From: Stephen Warren
Sent: Wednesday, September 02, 2015 1:05 PM
To: Tom Warren; Simon Glass
Cc: Bin Meng; Thierry Reding; Tom Rini; U-Boot Mailing List
Subject: Re: [U-Boot] [PATCH] Revert "fdt: Fix fdtdec_get_addr_size() for 64- bit"
Quoted 7 lines · Tom Warren
Simon, et al,
Quoted 5 lines
Simon Glass wrote at Friday, August 14, 2015 3:05 AM:
I plan to apply this revert to u-boot-x86 (where SPI is currently broken) and (once it has a bit more testing) also this patch which I think makes the change in a safer way:
Quoted 8 lines
Quoted 6 lines
https://patchwork.ozlabs.org/patch/504918/
At present that patch breaks at least one x86 board and I have not dug into it yet.
The revert should not break tegra, according to Stephen.
Unfortunately, my testing on P2571 with TOT u-boot-tegra (rebased against
TOT u-boot/master this morning) shows that that is not true.
Quoted 2 lines
The revert of the disputed 'fdtdec_get_addr_size' patch _does_ break Tegra
64-bit (P2571, at least). Nyan-big is OK. With Simon's revert in place, my board just loops on SPL signon, so I assume it's faulting, etc. in CPU init. Note that this is the current state of TOT u-boot/master.

Quoted 5 lines

Quoted 3 lines
I'm a bit confused. So far, we don't support SPL on T210 since we assume some other bootloader runs on the boot CPU and starts just the main U-Boot on the main CPU. It sounds like you're testing some local-only SPL support?
Currently there are a couple of ways to get T210 64-bit U-Boot loaded/executed. The first is the way I've always done it (during development and today) - use a 32-bit SPL that I built when I first ported 32-bit U-Boot to T210. I've saved away the SPL portion as a binary, and combine it with the current 64-bit T210 U-Boot proper when building my image. It's always worked up to now. What I'm seeing is a failure in the 64-bit CPU U-Boot portion. I just mentioned the looping SPL signon symptom because that's what I see as the indicator of a broken 64-bit image.

Oh I see; the SPL is only looping because the main U-Boot binary crashes/... and resets the CPU, hence re-executing the SPL. I thought you were referring to a loop purely within SPL.

Now it makes more sense.

Quoted 3 lines

The other way is to use your proprietary NV bootloader for the 32-bit portion (this will become the de facto standard when we release said NV bootloader code as open-source, or a binary first-stage loader 'tool'). I haven't tried that, since my way works and is an easy part of my workflow.
If you can point me to your boot CPU loader internally, I can try your method and see if it makes a difference, but I doubt it will.

I sent you an internal email message.

Perhaps you could also try my upstream U-Boot dev branch at:

repo git://github.com/swarren/u-boot.git branch tegra_dev

That has the revert of the original patch in, but also has the following replacement which you'd want to revert (or perhaps best: try with and without it):

c1fd5e1d5586 fdt: add new fdt address parsing functions

I'm sure I tested Simon's revert at the time I said it was OK. I wonder if the revert used to work fine, but something since then fails if the revert is in place? I would try testing this now, but I'm travelling so it's a bit more painful.

Stephen Warren

2015-09-02 23:43 UTC #26 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Quoted 60 lines · Stephen Warren

Quoted 33 lines · Tom Warren
Quoted 33 lines
-----Original Message-----
From: Stephen Warren
Sent: Wednesday, September 02, 2015 1:05 PM
To: Tom Warren; Simon Glass
Cc: Bin Meng; Thierry Reding; Tom Rini; U-Boot Mailing List
Subject: Re: [U-Boot] [PATCH] Revert "fdt: Fix fdtdec_get_addr_size() for 64- bit"
Quoted 7 lines · Tom Warren
Simon, et al,
Quoted 5 lines
Simon Glass wrote at Friday, August 14, 2015 3:05 AM:
I plan to apply this revert to u-boot-x86 (where SPI is currently broken) and (once it has a bit more testing) also this patch which I think makes the change in a safer way:
Quoted 8 lines
Quoted 6 lines
https://patchwork.ozlabs.org/patch/504918/
At present that patch breaks at least one x86 board and I have not dug into it yet.
The revert should not break tegra, according to Stephen.
Unfortunately, my testing on P2571 with TOT u-boot-tegra (rebased against
TOT u-boot/master this morning) shows that that is not true.
Quoted 2 lines
The revert of the disputed 'fdtdec_get_addr_size' patch _does_ break Tegra
64-bit (P2571, at least). Nyan-big is OK. With Simon's revert in place, my board just loops on SPL signon, so I assume it's faulting, etc. in CPU init. Note that this is the current state of TOT u-boot/master.
Quoted 5 lines
Quoted 3 lines
I'm a bit confused. So far, we don't support SPL on T210 since we assume some other bootloader runs on the boot CPU and starts just the main U-Boot on the main CPU. It sounds like you're testing some local-only SPL support?
Currently there are a couple of ways to get T210 64-bit U-Boot loaded/executed. The first is the way I've always done it (during development and today) - use a 32-bit SPL that I built when I first ported 32-bit U-Boot to T210. I've saved away the SPL portion as a binary, and combine it with the current 64-bit T210 U-Boot proper when building my image. It's always worked up to now. What I'm seeing is a failure in the 64-bit CPU U-Boot portion. I just mentioned the looping SPL signon symptom because that's what I see as the indicator of a broken 64-bit image.
Oh I see; the SPL is only looping because the main U-Boot binary crashes/... and resets the CPU, hence re-executing the SPL. I thought you were referring to a loop purely within SPL.
Now it makes more sense.
Quoted 3 lines
The other way is to use your proprietary NV bootloader for the 32-bit portion (this will become the de facto standard when we release said NV bootloader code as open-source, or a binary first-stage loader 'tool'). I haven't tried that, since my way works and is an easy part of my workflow.
If you can point me to your boot CPU loader internally, I can try your method and see if it makes a difference, but I doubt it will.
I sent you an internal email message.
Perhaps you could also try my upstream U-Boot dev branch at:
repo git://github.com/swarren/u-boot.git branch tegra_dev
That has the revert of the original patch in, but also has the following replacement which you'd want to revert (or perhaps best: try with and without it):

Quoted 6 lines

c1fd5e1d5586 fdt: add new fdt address parsing functions
I'm sure I tested Simon's revert at the time I said it was OK. I wonder if the revert used to work fine, but something since then fails if the revert is in place? I would try testing this now, but I'm travelling so it's a bit more painful.

I worked out how to remote control my device, and tested the current u-boot-tegra/master (which contains the patch revert this email thread is about) with and without "fdt: add new fdt address parsing functions" removed, and it works fine for me.

When you're concatenating SPL+U-Boot+DTB, are you using the DTB from the same source tree as the main U-Boot (likely by getting U-Boot+DTB from u-boot-dtb.bin in that source tree)?

Tom Warren

2015-09-03 02:02 UTC #27 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Quoted 90 lines

-----Original Message-----
From: Stephen Warren [mailto:swarren at wwwdotorg.org]
Sent: Wednesday, September 02, 2015 4:44 PM
To: Tom Warren; Simon Glass
Cc: U-Boot Mailing List; Thierry Reding; Tom Rini
Subject: Re: [U-Boot] [PATCH] Revert "fdt: Fix fdtdec_get_addr_size() for 64- bit"
Quoted 32 lines · Stephen Warren
Quoted 31 lines · Tom Warren
Quoted 31 lines
-----Original Message-----
From: Stephen Warren
Sent: Wednesday, September 02, 2015 1:05 PM
To: Tom Warren; Simon Glass
Cc: Bin Meng; Thierry Reding; Tom Rini; U-Boot Mailing List
Subject: Re: [U-Boot] [PATCH] Revert "fdt: Fix fdtdec_get_addr_size() for 64- bit"
Quoted 9 lines · Tom Warren
Simon, et al,
Quoted 7 lines
Simon Glass wrote at Friday, August 14, 2015 3:05 AM:
I plan to apply this revert to u-boot-x86 (where SPI is currently broken) and (once it has a bit more testing) also this patch which I think makes the change in a safer way:
https://patchwork.ozlabs.org/patch/504918/
Quoted 7 lines
Quoted 4 lines
At present that patch breaks at least one x86 board and I have not dug into it yet.
The revert should not break tegra, according to Stephen.
Unfortunately, my testing on P2571 with TOT u-boot-tegra (rebased against
TOT u-boot/master this morning) shows that that is not true.
Quoted 3 lines
The revert of the disputed 'fdtdec_get_addr_size' patch _does_ break Tegra
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
64-bit (P2571, at least). Nyan-big is OK. With Simon's revert in place, my board just loops on SPL signon, so I assume it's faulting, etc. in CPU init. Note that this is the current state of TOT u-boot/master.
I'm a bit confused. So far, we don't support SPL on T210 since we assume some other bootloader runs on the boot CPU and starts just the main U-Boot on the main CPU. It sounds like you're testing some local-
only SPL support?
Quoted 2 lines
Quoted 2 lines
Currently there are a couple of ways to get T210 64-bit U-Boot
loaded/executed. The first is the way I've always done it (during development and today) - use a 32-bit SPL that I built when I first ported 32-bit U-Boot to T210. I've saved away the SPL portion as a binary, and combine it with the current 64-bit T210 U-Boot proper when building my image. It's always worked up to now. What I'm seeing is a failure in the 64-bit CPU U-Boot portion. I just mentioned the looping SPL signon symptom because that's what I see as the indicator of a broken 64-bit image.
Quoted 8 lines
Oh I see; the SPL is only looping because the main U-Boot binary crashes/... and resets the CPU, hence re-executing the SPL. I thought you were referring to a loop purely within SPL.
Now it makes more sense.
Quoted 1 line
The other way is to use your proprietary NV bootloader for the 32-bit
portion (this will become the de facto standard when we release said NV bootloader code as open-source, or a binary first-stage loader 'tool'). I haven't tried that, since my way works and is an easy part of my workflow.
Quoted 2 lines
Quoted 2 lines
If you can point me to your boot CPU loader internally, I can try your
method and see if it makes a difference, but I doubt it will.
Quoted 11 lines
I sent you an internal email message.
Perhaps you could also try my upstream U-Boot dev branch at:
repo git://github.com/swarren/u-boot.git branch tegra_dev
That has the revert of the original patch in, but also has the following replacement which you'd want to revert (or perhaps best: try with and without it):
Quoted 6 lines
c1fd5e1d5586 fdt: add new fdt address parsing functions
I'm sure I tested Simon's revert at the time I said it was OK. I wonder if the revert used to work fine, but something since then fails if the revert is in place? I would try testing this now, but I'm travelling so it's a bit more painful.

Quoted 8 lines

I worked out how to remote control my device, and tested the current u-boot- tegra/master (which contains the patch revert this email thread is about) with and without "fdt: add new fdt address parsing functions"
removed, and it works fine for me.
When you're concatenating SPL+U-Boot+DTB, are you using the DTB from the same source tree as the main U-Boot (likely by getting U-Boot+DTB from u- boot-dtb.bin in that source tree)?

Yes

--
nvpublic

Tom Warren

2015-09-16 21:46 UTC #28 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

In reply to Stephen Warren

Simon,

Quoted 108 lines

-----Original Message-----
From: Tom Warren
Sent: Wednesday, September 02, 2015 7:02 PM
To: 'Stephen Warren'; Simon Glass
Cc: U-Boot Mailing List; Thierry Reding; Tom Rini
Subject: RE: [U-Boot] [PATCH] Revert "fdt: Fix fdtdec_get_addr_size() for 64- bit"
Quoted 8 lines
-----Original Message-----
From: Stephen Warren [mailto:swarren at wwwdotorg.org]
Sent: Wednesday, September 02, 2015 4:44 PM
To: Tom Warren; Simon Glass
Cc: U-Boot Mailing List; Thierry Reding; Tom Rini
Subject: Re: [U-Boot] [PATCH] Revert "fdt: Fix fdtdec_get_addr_size() for 64- bit"
Quoted 37 lines
Quoted 36 lines · Stephen Warren
Quoted 35 lines · Tom Warren
Quoted 35 lines
-----Original Message-----
From: Stephen Warren
Sent: Wednesday, September 02, 2015 1:05 PM
To: Tom Warren; Simon Glass
Cc: Bin Meng; Thierry Reding; Tom Rini; U-Boot Mailing List
Subject: Re: [U-Boot] [PATCH] Revert "fdt: Fix fdtdec_get_addr_size() for 64- bit"
Quoted 10 lines · Tom Warren
Simon, et al,
Quoted 8 lines
Simon Glass wrote at Friday, August 14, 2015 3:05 AM:
I plan to apply this revert to u-boot-x86 (where SPI is currently
broken) and (once it has a bit more testing) also this patch which I think makes the change in a safer way:
https://patchwork.ozlabs.org/patch/504918/
Quoted 7 lines
Quoted 4 lines
At present that patch breaks at least one x86 board and I have not dug into it yet.
The revert should not break tegra, according to Stephen.
Unfortunately, my testing on P2571 with TOT u-boot-tegra (rebased against
TOT u-boot/master this morning) shows that that is not true.
Quoted 3 lines
The revert of the disputed 'fdtdec_get_addr_size' patch _does_ break Tegra
64-bit (P2571, at least). Nyan-big is OK. With Simon's revert in place, my board just loops on SPL signon, so I assume it's faulting, etc. in CPU init. Note that this is the current state of TOT u-
boot/master.
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
I'm a bit confused. So far, we don't support SPL on T210 since we assume some other bootloader runs on the boot CPU and starts just the main U-Boot on the main CPU. It sounds like you're testing some local-
only SPL support?
Quoted 1 line
Quoted 1 line
Quoted 8 lines
Quoted 1 line
Quoted 1 line
Currently there are a couple of ways to get T210 64-bit U-Boot
loaded/executed. The first is the way I've always done it (during development and today) - use a 32-bit SPL that I built when I first ported 32-bit U-Boot to T210. I've saved away the SPL portion as a binary, and combine it with the current 64-bit T210 U-Boot proper when building my image. It's always worked up to now. What I'm seeing is a failure in the 64-bit CPU U-Boot portion. I just mentioned the looping SPL signon symptom because that's what I see as the indicator of a
broken 64-bit image.
Quoted 5 lines
Quoted 5 lines
Oh I see; the SPL is only looping because the main U-Boot binary crashes/... and resets the CPU, hence re-executing the SPL. I thought you were referring to a loop purely within SPL.
Quoted 7 lines
Quoted 4 lines
Now it makes more sense.
Quoted 2 lines
The other way is to use your proprietary NV bootloader for the 32-bit
portion (this will become the de facto standard when we release said NV bootloader code as open-source, or a binary first-stage loader 'tool'). I haven't tried that, since my way works and is an easy part of my
workflow.
Quoted 17 lines
Quoted 3 lines
Quoted 3 lines
If you can point me to your boot CPU loader internally, I can try your
method and see if it makes a difference, but I doubt it will.
Quoted 13 lines
I sent you an internal email message.
Perhaps you could also try my upstream U-Boot dev branch at:
repo git://github.com/swarren/u-boot.git branch tegra_dev
That has the revert of the original patch in, but also has the following replacement which you'd want to revert (or perhaps best:
try with and without it):
c1fd5e1d5586 fdt: add new fdt address parsing functions
Quoted 8 lines
Quoted 4 lines
I'm sure I tested Simon's revert at the time I said it was OK. I wonder if the revert used to work fine, but something since then fails if the revert is in place? I would try testing this now, but I'm travelling so it's a bit more painful.
I worked out how to remote control my device, and tested the current u-boot- tegra/master (which contains the patch revert this email thread is about) with and without "fdt: add new fdt address parsing

Quoted 8 lines

functions"
Quoted 5 lines
removed, and it works fine for me.
When you're concatenating SPL+U-Boot+DTB, are you using the DTB from the same source tree as the main U-Boot (likely by getting U-Boot+DTB from u- boot-dtb.bin in that source tree)?
Yes

I'm not sure if this was the last thread on this (I was on vacation for a few days), but have you resolved the problem you had with Stephen's new 'fdt: Add new fdt address parsing functions" patch? I'd really like to get this resolved so I can send a PR to TomR.

Thanks,

Tom
--
nvpublic

Simon Glass

author

2015-09-17 01:10 UTC #29 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Hi Tom,

Quoted 113 lines · Tom Warren

Simon,
Quoted 111 lines
-----Original Message-----
From: Tom Warren
Sent: Wednesday, September 02, 2015 7:02 PM
To: 'Stephen Warren'; Simon Glass
Cc: U-Boot Mailing List; Thierry Reding; Tom Rini
Subject: RE: [U-Boot] [PATCH] Revert "fdt: Fix fdtdec_get_addr_size() for 64- bit"
Quoted 8 lines
-----Original Message-----
From: Stephen Warren [mailto:swarren at wwwdotorg.org]
Sent: Wednesday, September 02, 2015 4:44 PM
To: Tom Warren; Simon Glass
Cc: U-Boot Mailing List; Thierry Reding; Tom Rini
Subject: Re: [U-Boot] [PATCH] Revert "fdt: Fix fdtdec_get_addr_size() for 64- bit"
Quoted 37 lines
Quoted 36 lines · Stephen Warren
Quoted 35 lines · Tom Warren
Quoted 35 lines
-----Original Message-----
From: Stephen Warren
Sent: Wednesday, September 02, 2015 1:05 PM
To: Tom Warren; Simon Glass
Cc: Bin Meng; Thierry Reding; Tom Rini; U-Boot Mailing List
Subject: Re: [U-Boot] [PATCH] Revert "fdt: Fix fdtdec_get_addr_size() for 64- bit"
Quoted 10 lines · Tom Warren
Simon, et al,
Quoted 8 lines
Simon Glass wrote at Friday, August 14, 2015 3:05 AM:
I plan to apply this revert to u-boot-x86 (where SPI is currently
broken) and (once it has a bit more testing) also this patch which I think makes the change in a safer way:
https://patchwork.ozlabs.org/patch/504918/
Quoted 7 lines
Quoted 4 lines
At present that patch breaks at least one x86 board and I have not dug into it yet.
The revert should not break tegra, according to Stephen.
Unfortunately, my testing on P2571 with TOT u-boot-tegra (rebased against
TOT u-boot/master this morning) shows that that is not true.
Quoted 3 lines
The revert of the disputed 'fdtdec_get_addr_size' patch _does_ break Tegra
64-bit (P2571, at least). Nyan-big is OK. With Simon's revert in place, my board just loops on SPL signon, so I assume it's faulting, etc. in CPU init. Note that this is the current state of TOT u-
boot/master.
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
I'm a bit confused. So far, we don't support SPL on T210 since we assume some other bootloader runs on the boot CPU and starts just the main U-Boot on the main CPU. It sounds like you're testing some local-
only SPL support?
Quoted 1 line
Quoted 1 line
Quoted 8 lines
Quoted 1 line
Quoted 1 line
Currently there are a couple of ways to get T210 64-bit U-Boot
loaded/executed. The first is the way I've always done it (during development and today) - use a 32-bit SPL that I built when I first ported 32-bit U-Boot to T210. I've saved away the SPL portion as a binary, and combine it with the current 64-bit T210 U-Boot proper when building my image. It's always worked up to now. What I'm seeing is a failure in the 64-bit CPU U-Boot portion. I just mentioned the looping SPL signon symptom because that's what I see as the indicator of a
broken 64-bit image.
Quoted 5 lines
Quoted 5 lines
Oh I see; the SPL is only looping because the main U-Boot binary crashes/... and resets the CPU, hence re-executing the SPL. I thought you were referring to a loop purely within SPL.
Quoted 7 lines
Quoted 4 lines
Now it makes more sense.
Quoted 2 lines
The other way is to use your proprietary NV bootloader for the 32-bit
portion (this will become the de facto standard when we release said NV bootloader code as open-source, or a binary first-stage loader 'tool'). I haven't tried that, since my way works and is an easy part of my
workflow.
Quoted 17 lines
Quoted 3 lines
Quoted 3 lines
If you can point me to your boot CPU loader internally, I can try your
method and see if it makes a difference, but I doubt it will.
Quoted 13 lines
I sent you an internal email message.
Perhaps you could also try my upstream U-Boot dev branch at:
repo git://github.com/swarren/u-boot.git branch tegra_dev
That has the revert of the original patch in, but also has the following replacement which you'd want to revert (or perhaps best:
try with and without it):
c1fd5e1d5586 fdt: add new fdt address parsing functions
Quoted 8 lines
Quoted 4 lines
I'm sure I tested Simon's revert at the time I said it was OK. I wonder if the revert used to work fine, but something since then fails if the revert is in place? I would try testing this now, but I'm travelling so it's a bit more painful.
I worked out how to remote control my device, and tested the current u-boot- tegra/master (which contains the patch revert this email thread is about) with and without "fdt: add new fdt address parsing
functions"
Quoted 2 lines
removed, and it works fine for me.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
When you're concatenating SPL+U-Boot+DTB, are you using the DTB from the same source tree as the main U-Boot (likely by getting U-Boot+DTB from u- boot-dtb.bin in that source tree)?
Yes
I'm not sure if this was the last thread on this (I was on vacation for a few days), but have you resolved the problem you had with Stephen's new 'fdt: Add new fdt address parsing functions" patch? I'd really like to get this resolved so I can send a PR to TomR.

Yes I believe the issues are resolved (not with the patch BTW - the only issue with the patch was the #define DEBUG which I removed).

The patch has just been applied to mainline.

Regards,
Simon

Tom Warren

2015-09-17 01:58 UTC #30 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

Simon,

Quoted 10 lines

-----Original Message-----
From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
Sent: Wednesday, September 16, 2015 6:11 PM
To: Tom Warren
Cc: U-Boot Mailing List; Thierry Reding; Tom Rini; Stephen Warren
Subject: Re: [U-Boot] [PATCH] Revert "fdt: Fix fdtdec_get_addr_size() for 64- bit"
Hi Tom,

<snip>

Quoted 9 lines

Quoted 1 line
I'm not sure if this was the last thread on this (I was on vacation for a few
days), but have you resolved the problem you had with Stephen's new 'fdt:
Add new fdt address parsing functions" patch? I'd really like to get this resolved so I can send a PR to TomR.
Yes I believe the issues are resolved (not with the patch BTW - the only issue with the patch was the #define DEBUG which I removed).
The patch has just been applied to mainline.

Saw that, and sent a PR to TomR for the pending Tegra stuff. Thanks

Quoted 3 lines

Regards,
Simon

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Simon Glass

author

2015-08-14 16:50 UTC #31 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

In reply to Stephen Warren

On 6 August 2015 at 13:03, Stephen Warren <[email protected]> wrote:
[snip]

Quoted 5 lines

It turns out that arch/arm/include/asm/config.h already enables this for all ARM64 platforms.
As such, we can in fact go ahead with reverting this patch, and U-Boot will still function on Tegra210 boards.

Applied to u-boot-x86.

Bin Meng

2015-08-03 15:40 UTC #32 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

In reply to Simon Glass

Hi Simon,

Quoted 5 lines · Simon Glass

This reverts commit 5b34436035fc862b5e8d0d2c3eab74ba36f1a7f4.
This function has a few problems. It calls fdt_parent_offset() which as mentioned in code review is very slow.

Quoted 7 lines

https://patchwork.ozlabs.org/patch/499482/
https://patchwork.ozlabs.org/patch/452604/
It also happens to break SPI flash on Minnowboard max which is how I noticed that this was applied. I can send a patch to tidy that up, but in any case I think we should consider a revert until the function is better implemented.

Ah, today I rebased my working branch on top of u-boot-x86/master and also noticed that SPI flash on Intel Bayley Bay is no longer working. I haven't got time today to trace the broken commit, but looks that you have caught it.

I will try this revert commit tomorrow.

Quoted 67 lines

Signed-off-by Simon Glass <[email protected]>

---

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

lib/fdtdec.c +17 ~~-34~~

index a954051..aac4f8d 100644
@@ -90,45 +90,29 @@ 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 fdt32_t *ptr, *end;
-       int parent, na, ns, len;
-       fdt_addr_t addr;
+       const fdt_addr_t *cell;
+       int len;
 
        debug("%s: %s: ", __func__, prop_name);
-
-       parent = fdt_parent_offset(blob, node);
-       if (parent < 0) {
-               debug("(no parent 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);
+       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);
+               }

lib/fdtdec.c const char *fdtdec_get_compatible(enum fdt_compat_id id)

Quoted 10 lines

lib/fdtdec.c +3 -3

+               return addr;
        }
-
-       return addr;
+       debug("(not found)\n");
+       return FDT_ADDR_T_NONE;
 }
 
 fdt_addr_t fdtdec_get_addr(const void *blob, int node,
--

Regards,
Bin

Thierry Reding

2015-08-04 15:27 UTC #33 lore raw

PATCH 1/1 Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

In reply to Simon Glass

Quoted 11 lines · Simon Glass

This reverts commit 5b34436035fc862b5e8d0d2c3eab74ba36f1a7f4.
This function has a few problems. It calls fdt_parent_offset() which as mentioned in code review is very slow.
https://patchwork.ozlabs.org/patch/499482/
https://patchwork.ozlabs.org/patch/452604/
It also happens to break SPI flash on Minnowboard max which is how I noticed that this was applied. I can send a patch to tidy that up, but in any case I think we should consider a revert until the function is better implemented.

I suspect that what breaks is when you try to parse the "memory-map" property from this (taken from arch/x86/dts/minnowmax.dts):

	spi {
		#address-cells = <1>;
		#size-cells = <0>;
		compatible = "intel,ich-spi";
		spi-flash at 0 {
			reg = <0>;
			compatible = "stmicro,n25q064a", "spi-flash";
			memory-map = <0xff800000 0x00800000>;
		};
	};

That's obviously not going to work because as Stephen and I have pointed out it specifies an address range in a different address space than that of the SPI flash. The spi parent node defines #address-cells and #size-cells, both of which define the layout of the "reg" property (and some others like "ranges"). I don't see a way around this other than to read two individual cells from the "memory-map" property. I don't know of a concept in DT that would allow you to translate from memory-map in the spi-flash at 0 node to the address space in the parent node of the spi node (the root node).