[U-Boot] [PATCH] powerpc/85xx: verify the localbus device tree address before booting the OS

[U-Boot] [PATCH] powerpc/85xx: verify the localbus device tree address before booting the OS

[Timur Tabi]

The localbus controller node in the device tree is typically a root node,
even though the controller is part of CCSR.  If we were to put the lbc
node under the SOC node, then the 'ranges' property in the lbc node would
translate through the 'ranges' property of the parent SOC node, and we
don't want that.

Since the lbc is a separate node, it's possible for the 'reg' property to
be wrong.  This happened with the original version of p1022ds.dts, which
used a 32-bit value in the 'reg' address, instead of a 36-bit address.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 arch/powerpc/cpu/mpc85xx/fdt.c |   48 ++++++++++++++++++++++++++++++++-------
 1 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c
index 9d31568..2fcd8c3 100644
--- a/arch/powerpc/cpu/mpc85xx/fdt.c
+++ b/arch/powerpc/cpu/mpc85xx/fdt.c
@@ -677,6 +677,12 @@ void ft_cpu_setup(void *blob, bd_t *bd)
 #define CCSR_VIRT_TO_PHYS(x) \
 	(CONFIG_SYS_CCSRBAR_PHYS + ((x) - CONFIG_SYS_CCSRBAR))
 
+static void msg(const char *name, uint64_t uaddr, uint64_t daddr)
+{
+	printf("Warning: U-Boot configured %s at address %llx,\n"
+	       "but the device tree has it at %llx\n", name, uaddr, daddr);
+}
+
 /*
  * Verify the device tree
  *
@@ -692,33 +698,32 @@ void ft_cpu_setup(void *blob, bd_t *bd)
  */
 int ft_verify_fdt(void *fdt)
 {
-	uint64_t ccsr = 0;
+	uint64_t addr = 0;
 	int aliases;
 	int off;
 
 	/* First check the CCSR base address */
 	off = fdt_node_offset_by_prop_value(fdt, -1, "device_type", "soc", 4);
 	if (off > 0)
-		ccsr = fdt_get_base_address(fdt, off);
+		addr = fdt_get_base_address(fdt, off);
 
-	if (!ccsr) {
+	if (!addr) {
 		printf("Warning: could not determine base CCSR address in "
 		       "device tree\n");
 		/* No point in checking anything else */
 		return 0;
 	}
 
-	if (ccsr != CONFIG_SYS_CCSRBAR_PHYS) {
-		printf("Warning: U-Boot configured CCSR at address %llx,\n"
-		       "but the device tree has it at %llx\n",
-		       (uint64_t) CONFIG_SYS_CCSRBAR_PHYS, ccsr);
+	if (addr != CONFIG_SYS_CCSRBAR_PHYS) {
+		msg("CCSR", CONFIG_SYS_CCSRBAR_PHYS, addr);
 		/* No point in checking anything else */
 		return 0;
 	}
 
 	/*
-	 * Get the 'aliases' node.  If there isn't one, then there's nothing
-	 * left to do.
+	 * Check some nodes via aliases.  We assume that U-Boot and the device
+	 * tree enumerate the devices equally.  E.g. the first serial port in
+	 * U-Boot is the same as "serial0" in the device tree.
 	 */
 	aliases = fdt_path_offset(fdt, "/aliases");
 	if (aliases > 0) {
@@ -735,5 +740,30 @@ int ft_verify_fdt(void *fdt)
 #endif
 	}
 
+	/*
+	 * The localbus node is typically a root node, even though the lbc
+	 * controller is part of CCSR.  If we were to put the lbc node under
+	 * the SOC node, then the 'ranges' property in the lbc node would
+	 * translate through the 'ranges' property of the parent SOC node, and
+	 * we don't want that.  Since it's a separate node, it's possible for
+	 * the 'reg' property to be wrong, so check it here.  For now, we
+	 * only check for "fsl,elbc" nodes.
+	 */
+#ifdef CONFIG_SYS_LBC_ADDR
+	off = fdt_node_offset_by_compatible(fdt, -1, "fsl,elbc");
+	if (off > 0) {
+		const u32 *reg = fdt_getprop(fdt, off, "reg", NULL);
+		if (reg) {
+			uint64_t uaddr = CCSR_VIRT_TO_PHYS(CONFIG_SYS_LBC_ADDR);
+
+			addr = fdt_translate_address(fdt, off, reg);
+			if (uaddr != addr) {
+				msg("the localbus", uaddr, addr);
+				return 0;
+			}
+		}
+	}
+#endif
+
 	return 1;
 }
-- 
1.7.3.4

[U-Boot] [u-boot-release] [PATCH] powerpc/85xx: verify the localbus device tree address before booting the OS

[Swarthout Edward L-SWARTHOU]

> ... it's possible for the 'reg' property to be wrong.

Instead of all this, why doesn't u-boot just fix it?

I have the same annoyance with ccsrbar.

U-boot controls the ccsrbar address via a user configured #define.
So instead of having different dts's based on the u-boot ccsrbar,
why can't u-boot update the device tree?

-Ed

[U-Boot] [u-boot-release] [PATCH] powerpc/85xx: verify the localbus device tree address before booting the OS

[Timur Tabi]

Swarthout Edward L-SWARTHOU wrote:
>> ... it's possible for the 'reg' property to be wrong.
> 
> Instead of all this, why doesn't u-boot just fix it?
> 
> I have the same annoyance with ccsrbar.
> 
> U-boot controls the ccsrbar address via a user configured #define.
> So instead of having different dts's based on the u-boot ccsrbar,
> why can't u-boot update the device tree?

Because we can't just relocate the devices that we know about and ignore the rest.  If CCSR is at the wrong address, then maybe the PCI memory ranges are wrong, too?  Who's to say what else is wrong?

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [u-boot-release] [PATCH] powerpc/85xx: verify the localbus device tree address before booting the OS

[Scott Wood]

On 11/16/2011 01:58 PM, Swarthout Edward L-SWARTHOU wrote:
>> ... it's possible for the 'reg' property to be wrong.
> 
> Instead of all this, why doesn't u-boot just fix it?
> 
> I have the same annoyance with ccsrbar.
> 
> U-boot controls the ccsrbar address via a user configured #define.
> So instead of having different dts's based on the u-boot ccsrbar,
> why can't u-boot update the device tree?

That will require that U-Boot know where every instance of CCSRBAR is
within the tree, which seems fragile.  Do we have any instances yet of
updating unit addresses from U-Boot?  What about aliases that reference
said unit addresses?

-Scott

[U-Boot] [PATCH] powerpc/85xx: verify the localbus device tree address before booting the OS

[Kumar Gala]

On Nov 16, 2011, at 1:28 PM, Timur Tabi wrote:

> The localbus controller node in the device tree is typically a root node,
> even though the controller is part of CCSR.  If we were to put the lbc
> node under the SOC node, then the 'ranges' property in the lbc node would
> translate through the 'ranges' property of the parent SOC node, and we
> don't want that.
> 
> Since the lbc is a separate node, it's possible for the 'reg' property to
> be wrong.  This happened with the original version of p1022ds.dts, which
> used a 32-bit value in the 'reg' address, instead of a 36-bit address.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
> arch/powerpc/cpu/mpc85xx/fdt.c |   48 ++++++++++++++++++++++++++++++++-------
> 1 files changed, 39 insertions(+), 9 deletions(-)

applied to 85xx

- k