[U-Boot] [PATCH] [v2] powerpc/85xx: introduce 'fdt verify' command

[U-Boot] [PATCH] [v2] powerpc/85xx: introduce 'fdt verify' command

[Timur Tabi]

Introduce the 'fdt verify' command, which verifies some of the physical
addresses in the device tree.

U-Boot uses macros to determine where devices should be located in physical
memory, and Linux uses the device tree to determine where the devices are
actually located.  However, U-Boot does not update the device tree with those
addresses when it boots the operating system.  This means that it's possible
to use a device tree with the wrong addresses, and U-Boot won't complain.
This frequently happens when U-Boot is compiled for 32-bit physical addresses,
but the device tree uses 36-bit addresses.

It's not safe or practical to have U-Boot update the addresses in the device
tree, mostly because U-Boot is not aware of all the devices.  We can, however,
spot-check a few common devices to see if the addresses match, and then display
a warning if they do not.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 arch/powerpc/cpu/mpc85xx/fdt.c |  168 ++++++++++++++++++++++++++++++++++++++++
 common/cmd_fdt.c               |   19 +++++
 common/fdt_support.c           |  165 +++++++++++++++++++++++++++++++++++++++
 include/fdt_support.h          |   28 +++++++
 4 files changed, 380 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c
index 53e0596..91ffbc3 100644
--- a/arch/powerpc/cpu/mpc85xx/fdt.c
+++ b/arch/powerpc/cpu/mpc85xx/fdt.c
@@ -393,6 +393,174 @@ static void ft_fixup_qe_snum(void *blob)
 }
 #endif
 
+/*
+ * For some CCSR devices, we only have the virtual address, not the physical
+ * address.  This is because we map CCSR as a whole, so we typically don't need
+ * a macro for the physical address of any device within CCSR.  In this case,
+ * we calculate the physical address of that device using it's the difference
+ * between the virtual address of the device and the virtual address of the
+ * beginning of CCSR.
+ */
+#define CCSR_VIRT_TO_PHYS(x) \
+	(CONFIG_SYS_CCSRBAR_PHYS + ((x) - CONFIG_SYS_CCSRBAR))
+
+#ifdef CONFIG_PCI
+
+/*
+ * Verify the addresses for all of the PCI controllers
+ *
+ * PCI is complicated because there is no correlation between the numbering
+ * of the controllers by U-Boot and the numbering the device tree.  So we need
+ * to search all of the aliases until we find a patch
+ */
+static void fdt_verify_pci_aliases(void *blob)
+{
+	int off;
+
+	for (off = fdt_next_pci_node(blob, -1); off != -FDT_ERR_NOTFOUND;
+	     off = fdt_next_pci_node(blob, off)) {
+		const u32 *reg;
+		u64 addr;
+
+		reg = fdt_getprop(blob, off, "reg", NULL);
+		if (!reg || !*reg)
+			continue;
+
+		addr = fdt_translate_address(blob, off, reg);
+		if (!addr)
+			/* We can't determine the base address, so skip it */
+			continue;
+
+#ifdef CONFIG_SYS_PCI1_MEM_PHYS
+		if (addr == CCSR_VIRT_TO_PHYS(CONFIG_SYS_PCI1_ADDR)) {
+			fdt_check_pci_addresses(blob, off,
+				CONFIG_SYS_PCI1_MEM_PHYS,
+				CONFIG_SYS_PCI1_MEM_SIZE, 0);
+			fdt_check_pci_addresses(blob, off,
+				CONFIG_SYS_PCI1_IO_PHYS,
+				CONFIG_SYS_PCI1_IO_SIZE, 1);
+			continue;
+		}
+#endif
+#ifdef CONFIG_SYS_PCI2_MEM_PHYS
+		if (addr == CCSR_VIRT_TO_PHYS(CONFIG_SYS_PCI2_ADDR)) {
+			fdt_check_pci_addresses(blob, off,
+				CONFIG_SYS_PCI2_MEM_PHYS,
+				CONFIG_SYS_PCI2_MEM_SIZE, 0);
+			fdt_check_pci_addresses(blob, off,
+				CONFIG_SYS_PCI2_IO_PHYS,
+				CONFIG_SYS_PCI2_IO_SIZE, 1);
+			continue;
+		}
+#endif
+#ifdef CONFIG_SYS_PCIE1_MEM_PHYS
+		if (addr == CCSR_VIRT_TO_PHYS(CONFIG_SYS_PCIE1_ADDR)) {
+			fdt_check_pci_addresses(blob, off,
+				CONFIG_SYS_PCIE1_MEM_PHYS,
+				CONFIG_SYS_PCIE1_MEM_SIZE, 0);
+			fdt_check_pci_addresses(blob, off,
+				CONFIG_SYS_PCIE1_IO_PHYS,
+				CONFIG_SYS_PCIE1_IO_SIZE, 1);
+			continue;
+		}
+#endif
+#ifdef CONFIG_SYS_PCIE2_MEM_PHYS
+		if (addr == CCSR_VIRT_TO_PHYS(CONFIG_SYS_PCIE2_ADDR)) {
+			fdt_check_pci_addresses(blob, off,
+				CONFIG_SYS_PCIE2_MEM_PHYS,
+				CONFIG_SYS_PCIE2_MEM_SIZE, 0);
+			fdt_check_pci_addresses(blob, off,
+				CONFIG_SYS_PCIE2_IO_PHYS,
+				CONFIG_SYS_PCIE2_IO_SIZE, 1);
+			continue;
+		}
+#endif
+#ifdef CONFIG_SYS_PCIE3_MEM_PHYS
+		if (addr == CCSR_VIRT_TO_PHYS(CONFIG_SYS_PCIE3_ADDR)) {
+			fdt_check_pci_addresses(blob, off,
+				CONFIG_SYS_PCIE3_MEM_PHYS,
+				CONFIG_SYS_PCIE3_MEM_SIZE, 0);
+			fdt_check_pci_addresses(blob, off,
+				CONFIG_SYS_PCIE3_IO_PHYS,
+				CONFIG_SYS_PCIE3_IO_SIZE, 1);
+			continue;
+		}
+#endif
+#ifdef CONFIG_SYS_PCIE4_MEM_PHYS
+		if (addr == CCSR_VIRT_TO_PHYS(CONFIG_SYS_PCIE4_ADDR)) {
+			fdt_check_pci_addresses(blob, off,
+				CONFIG_SYS_PCIE4_MEM_PHYS,
+				CONFIG_SYS_PCIE4_MEM_SIZE, 0);
+			fdt_check_pci_addresses(blob, off,
+				CONFIG_SYS_PCIE4_IO_PHYS,
+				CONFIG_SYS_PCIE4_IO_SIZE, 1);
+			continue;
+		}
+#endif
+
+		printf("Warning: node %s is set at address %llx,\n"
+		       "but U-Boot has not configured any PCI(e) device at "
+		       "that address.\n", fdt_get_name(blob, off, NULL), addr);
+	}
+}
+
+#endif /* #ifdef CONFIG_PCI */
+
+/*
+ * Verify the device addresses in the device tree
+ *
+ * This function compares several CONFIG_xxx macros that contain physical
+ * addresses with the corresponding nodes in the device tree, to see if the
+ * physical addresses are all correct.  For example, if CONFIG_SYS_NS16550_COM1
+ * is defined, then it contains the virtual address of the first UART.  We
+ * convert this to a physical address and compare that with the physical
+ * address of the first ns16550-compatible node in the device tree.  If they
+ * don't match, then we display a warning.
+ */
+void fdt_verify_addresses(void *blob)
+{
+	uint64_t ccsr = 0;
+	int aliases;
+	int off;
+
+	/* First check the CCSR base address */
+	off = fdt_node_offset_by_prop_value(blob, -1, "device_type", "soc", 4);
+	if (off > 0)
+		ccsr = fdt_get_base_address(blob, off);
+
+	if (!ccsr) {
+		printf("Warning: could not determine base CCSR address in "
+		       "device tree\n");
+		return;
+	}
+
+	if (ccsr != CONFIG_SYS_CCSRBAR_PHYS)
+		printf("Warning: U-Boot configured CCSR at address %llx, " \
+		       "but the device tree has it at %llx\n",
+		       (uint64_t) CONFIG_SYS_CCSRBAR_PHYS, ccsr);
+
+	/*
+	 * Get the 'aliases' node.  If there isn't one, then there's nothing
+	 * left to do.
+	 */
+	aliases = fdt_path_offset(blob, "/aliases");
+	if (aliases > 0) {
+#ifdef CONFIG_SYS_NS16550_COM1
+		fdt_verify_alias_address(blob, aliases, "serial0",
+			CCSR_VIRT_TO_PHYS(CONFIG_SYS_NS16550_COM1));
+#endif
+
+#ifdef CONFIG_SYS_NS16550_COM2
+		fdt_verify_alias_address(blob, aliases, "serial1",
+			CCSR_VIRT_TO_PHYS(CONFIG_SYS_NS16550_COM2));
+#endif
+	}
+
+#ifdef CONFIG_PCI
+	fdt_verify_pci_aliases(blob);
+#endif
+}
+
 void ft_cpu_setup(void *blob, bd_t *bd)
 {
 	int off;
diff --git a/common/cmd_fdt.c b/common/cmd_fdt.c
index 3d0c2b7..5e77e7c 100644
--- a/common/cmd_fdt.c
+++ b/common/cmd_fdt.c
@@ -46,6 +46,20 @@ static int fdt_parse_prop(char *const*newval, int count, char *data, int *len);
 static int fdt_print(const char *pathp, char *prop, int depth);
 
 /*
+ * Verify the physical addresses in the device tree
+ *
+ * This CPU- or board-specific function compares the physical addresses of
+ * various devices in the device tree with the physical addresses as defined
+ * in the board header file.
+ */
+void __fdt_verify_addresses(void *blob)
+{
+}
+
+void fdt_verify_addresses(void *blob)
+	__attribute__((weak, alias("__fdt_verify_addresses")));
+
+/*
  * The working_fdt points to our working flattened device tree.
  */
 struct fdt_header *working_fdt;
@@ -436,6 +450,10 @@ int do_fdt (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 	else if (strncmp(argv[1], "re", 2) == 0) {
 		fdt_resize(working_fdt);
 	}
+	/* verify the addresses in the fdt */
+	else if (argv[1][0] == 'v') {
+		fdt_verify_addresses(working_fdt);
+	}
 	else {
 		/* Unrecognized command */
 		return cmd_usage(cmdtp);
@@ -824,6 +842,7 @@ U_BOOT_CMD(
 	"fdt rsvmem delete <index>           - Delete a mem reserves\n"
 	"fdt chosen [<start> <end>]          - Add/update the /chosen branch in the tree\n"
 	"                                        <start>/<end> - initrd start/end addr\n"
+	"fdt verify                          - Verify the addresses in the device tree\n"
 	"NOTE: Dereference aliases by omiting the leading '/', "
 		"e.g. fdt print ethernet0."
 );
diff --git a/common/fdt_support.c b/common/fdt_support.c
index 5829afd..1c2af5d 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -1223,3 +1223,168 @@ err_size:
 	return ret;
 }
 #endif
+
+/*
+ * Verify the physical address of device tree node for a given alias
+ *
+ * This function locates the device tree node of a given alias, and then
+ * verifies that the physical address of that device matches the given
+ * parameter.  It displays a message if there is a mismatch.
+ */
+void fdt_verify_alias_address(void *blob, int anode, const char *alias,
+			      phys_addr_t addr)
+{
+	const char *path;
+	const u32 *reg;
+	int node, len;
+	u64 dt_addr;
+
+	path = fdt_getprop(blob, anode, alias, NULL);
+	if (!path)
+		return;
+
+	node = fdt_path_offset(blob, path);
+	if (node < 0)
+		return;
+
+	reg = fdt_getprop(blob, node, "reg", &len);
+	if (!reg)
+		return;
+
+	dt_addr = fdt_translate_address(blob, node, reg);
+	if (addr != dt_addr)
+		printf("Warning: U-Boot configured device %s@address %llx,\n"
+		       "but the device tree has it address %llx.\n",
+		       alias, (u64)addr, dt_addr);
+}
+
+/*
+ * Returns the base address of an SOC or PCI node
+ */
+u64 fdt_get_base_address(void *blob, int node)
+{
+	int size;
+	u32 naddr;
+	const u32 *prop;
+
+	prop = fdt_getprop(blob, node, "#address-cells", &size);
+	if (prop && size == 4)
+		naddr = *prop;
+	else
+		naddr = 2;
+
+	prop = fdt_getprop(blob, node, "ranges", &size);
+
+	return prop ? fdt_translate_address(blob, node, prop + naddr) : 0;
+}
+
+#ifdef CONFIG_PCI
+
+#define PCI_CELL0_SS_MASK	0x3000000
+#define PCI_CELL0_SS_IO		0x1000000
+
+/*
+ * Verify the memory and I/O addresses in a PCI node
+ *
+ * This function scans the 'ranges' property of a PCI node and looks for a
+ * match to the given parameters.  If there's an error or a mismatch, a message
+ * is displayed.
+ */
+void fdt_check_pci_addresses(void *blob, int node, phys_addr_t addr,
+			     phys_addr_t size, int is_io)
+{
+	unsigned int address_cells, parent_address_cells, size_cells;
+	u64 dt_addr, dt_size;
+	u32 attr;	/* The first cell is the PCI attributes */
+	const u32 *ranges;
+	const u32 *prop;
+	unsigned int i, count;
+	int len, row_size;
+
+	/* Get the address and size cells that we need */
+	address_cells = fdt_get_address_cells(blob, node);
+	parent_address_cells =
+		fdt_get_address_cells(blob, fdt_parent_offset(blob, node));
+
+	prop = fdt_getprop(blob, node, "#size-cells", NULL);
+	size_cells = prop ? *prop : 1;
+
+	ranges = fdt_getprop(blob, node, "ranges", &len);
+	if (!ranges) {
+		printf("Warning: node %s is missing 'ranges' property\n",
+		       fdt_get_name(blob, node, NULL));
+		return;
+	}
+
+	/* Make sure the 'ranges' property is the right size*/
+	row_size = 4 * (address_cells + parent_address_cells + size_cells);
+	if (len % row_size) {
+		printf("Warning: 'ranges' property of node %s is malformed\n",
+		       fdt_get_name(blob, node, NULL));
+		return;
+	}
+
+	count = len / row_size;
+
+	for (i = 0; i < count; i++) {
+		/* Parse one line of the 'ranges' property */
+		attr = *ranges;
+		if (parent_address_cells == 1)
+			dt_addr = be32_to_cpup(ranges + address_cells);
+		else
+			/* parent_address_cells == 2 */
+			dt_addr = be64_to_cpup(ranges + address_cells);
+		if (size_cells == 1)
+			dt_size = be32_to_cpup(ranges + address_cells +
+					       parent_address_cells);
+		else
+			/* size_cells == 2 */
+			dt_size = be64_to_cpup(ranges + address_cells +
+					      parent_address_cells);
+
+		/*
+		 * Check for matches.  If the address matches but is the wrong
+		 * type or wrong size, then return an error.
+		 */
+		if ((attr & PCI_CELL0_SS_MASK) == PCI_CELL0_SS_IO) {
+			if (is_io && (dt_addr == addr)) {
+				if (dt_size == size)
+					return;
+				else
+					goto wrong_size;
+			}
+			if (!is_io && (dt_addr == addr))
+				goto wrong_type;
+		} else {
+			if (!is_io && (dt_addr == addr)) {
+				if (dt_size == size)
+					return;
+				else
+					goto wrong_size;
+			}
+			if (is_io && (dt_addr == addr))
+				goto wrong_type;
+		}
+		ranges += address_cells + parent_address_cells + size_cells;
+	}
+
+	printf("Warning: node %s has a missing or incorrect %s region at\n"
+	       "address=%llx size=%llx\n",
+	       fdt_get_name(blob, node, NULL), is_io ? "an I/O" : "a memory",
+	       (u64)addr, (u64)size);
+	return;
+
+wrong_type:
+	printf("Warning: node %s has 'ranges' property for address %llx and\n"
+	       "size %llx, but of the wrong type\n",
+	       fdt_get_name(blob, node, NULL), (u64)dt_addr, (u64)dt_size);
+	return;
+
+wrong_size:
+	printf("Warning: node %s has 'ranges' property for address %llx but\n"
+	       "the wrong size (%llx, but U-Boot has %llx)\n",
+	       fdt_get_name(blob, node, NULL),
+	       (u64)dt_addr, (u64)dt_size, (u64)size);
+}
+
+#endif /* #ifdef CONFIG_PCI */
diff --git a/include/fdt_support.h b/include/fdt_support.h
index ce6817b..7ef73d5 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -90,5 +90,33 @@ int fdt_node_offset_by_compat_reg(void *blob, const char *compat,
 int fdt_alloc_phandle(void *blob);
 int fdt_add_edid(void *blob, const char *compat, unsigned char *buf);
 
+void fdt_verify_alias_address(void *blob, int anode, const char *alias,
+			      phys_addr_t addr);
+void fdt_check_pci_addresses(void *blob, int node, phys_addr_t addr,
+			     phys_addr_t size, int is_io);
+u64 fdt_get_base_address(void *blob, int node);
+
+/*
+ * Returns the next PCI node in the device tree
+ */
+static inline int fdt_next_pci_node(void *blob, int off)
+{
+	return fdt_node_offset_by_prop_value(blob, off,
+					     "device_type", "pci", 4);
+}
+
+/*
+ * Returns the value of the address cells for the given node
+ *
+ * If a given node does not have an #address-cells property, then the default
+ * value of '2' is returned.
+ */
+static inline unsigned int fdt_get_address_cells(void *blob, int node)
+{
+	const u32 *prop = fdt_getprop(blob, node, "#address-cells", NULL);
+
+	return prop ? *prop : 2;
+}
+
 #endif /* ifdef CONFIG_OF_LIBFDT */
 #endif /* ifndef __FDT_SUPPORT_H */
-- 
1.7.2.3

[U-Boot] [PATCH] [v2] powerpc/85xx: introduce 'fdt verify' command

[Wolfgang Denk]

Dear Timur Tabi,

In message <1290015805-18791-1-git-send-email-timur@freescale.com> you wrote:
> Introduce the 'fdt verify' command, which verifies some of the physical
> addresses in the device tree.

> +		addr = fdt_translate_address(blob, off, reg);
> +		if (!addr)
> +			/* We can't determine the base address, so skip it */
> +			continue;

Braces needed for multiline statements.

> +/*
> + * Verify the device addresses in the device tree
> + *
> + * This function compares several CONFIG_xxx macros that contain physical
> + * addresses with the corresponding nodes in the device tree, to see if the
> + * physical addresses are all correct.  For example, if CONFIG_SYS_NS16550_COM1
> + * is defined, then it contains the virtual address of the first UART.  We
> + * convert this to a physical address and compare that with the physical
> + * address of the first ns16550-compatible node in the device tree.  If they
> + * don't match, then we display a warning.
> + */

LIne toolong. Please check and fix globally.

> +	if (ccsr != CONFIG_SYS_CCSRBAR_PHYS)
> +		printf("Warning: U-Boot configured CCSR at address %llx, " \
> +		       "but the device tree has it at %llx\n",
> +		       (uint64_t) CONFIG_SYS_CCSRBAR_PHYS, ccsr);

Braces needed for multiline statements. Please fix globally.

No backslash needed at end of line, please remove.

>  struct fdt_header *working_fdt;
> @@ -436,6 +450,10 @@ int do_fdt (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
>  	else if (strncmp(argv[1], "re", 2) == 0) {
>  		fdt_resize(working_fdt);
>  	}
> +	/* verify the addresses in the fdt */
> +	else if (argv[1][0] == 'v') {
> +		fdt_verify_addresses(working_fdt);
> +	}
>  	else {

2x incorrect coding style - the "else" goes on the saame line with the
'}' and the '{'

> +	if (addr != dt_addr)
> +		printf("Warning: U-Boot configured device %s at address %llx,\n"
> +		       "but the device tree has it address %llx.\n",
> +		       alias, (u64)addr, dt_addr);

Braces needed.

> +		if (parent_address_cells == 1)
> +			dt_addr = be32_to_cpup(ranges + address_cells);
> +		else
> +			/* parent_address_cells == 2 */
> +			dt_addr = be64_to_cpup(ranges + address_cells);

and again.

> +		if (size_cells == 1)
> +			dt_size = be32_to_cpup(ranges + address_cells +
> +					       parent_address_cells);

and again.

> +		else
> +			/* size_cells == 2 */
> +			dt_size = be64_to_cpup(ranges + address_cells +
> +					      parent_address_cells);

and again. etc. etc.

> +
> +		/*
> +		 * Check for matches.  If the address matches but is the wrong
> +		 * type or wrong size, then return an error.
> +		 */
> +		if ((attr & PCI_CELL0_SS_MASK) == PCI_CELL0_SS_IO) {
> +			if (is_io && (dt_addr == addr)) {
> +				if (dt_size == size)
> +					return;
> +				else
> +					goto wrong_size;
> +			}
> +			if (!is_io && (dt_addr == addr))
> +				goto wrong_type;
> +		} else {
> +			if (!is_io && (dt_addr == addr)) {
> +				if (dt_size == size)
> +					return;
> +				else
> +					goto wrong_size;
> +			}
> +			if (is_io && (dt_addr == addr))
> +				goto wrong_type;
> +		}

Factor out the "(dt_addr == addr)" from all 4 "if"s.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Human beings were created by water to transport it uphill.

[U-Boot] [PATCH] [v2] powerpc/85xx: introduce 'fdt verify' command

[Timur Tabi]

Wolfgang Denk wrote:
>> >  struct fdt_header *working_fdt;
>> > @@ -436,6 +450,10 @@ int do_fdt (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
>> >  	else if (strncmp(argv[1], "re", 2) == 0) {
>> >  		fdt_resize(working_fdt);
>> >  	}
>> > +	/* verify the addresses in the fdt */
>> > +	else if (argv[1][0] == 'v') {
>> > +		fdt_verify_addresses(working_fdt);
>> > +	}
>> >  	else {
> 2x incorrect coding style - the "else" goes on the saame line with the
> '}' and the '{'
> 

Ah, the bottom half of the switch statement uses this style:

}
else if


But the top half uses this style:

} else if

And the top half is correct.

-- 
Timur Tabi
Linux kernel developer at Freescale