[U-Boot] [PATCH] powerpc/85xx: compare actual device addresses with the device tree

[U-Boot] [PATCH] powerpc/85xx: compare actual device addresses with the device tree

[Timur Tabi]

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 |  351 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 351 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c
index 53e0596..7bd9423 100644
--- a/arch/powerpc/cpu/mpc85xx/fdt.c
+++ b/arch/powerpc/cpu/mpc85xx/fdt.c
@@ -393,6 +393,354 @@ 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
+
+/*
+ * 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 unsigned int fdt_get_address_cells(void *blob, int node)
+{
+	const u32 *prop = fdt_getprop(blob, node, "#address-cells", NULL);
+
+	return prop ? *prop : 2;
+}
+
+#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.
+ */
+static 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);
+}
+
+/*
+ * Returns the base address of an SOC or PCI node
+ */
+static 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;
+}
+
+static int fdt_next_pci_node(void *blob, int off)
+{
+	return fdt_node_offset_by_prop_value(blob, off,
+					     "device_type", "pci", 4);
+}
+
+/*
+ * 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 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.
+ */
+static 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);
+}
+
+/*
+ * 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.
+ */
+static 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;
@@ -478,4 +826,7 @@ void ft_cpu_setup(void *blob, bd_t *bd)
 
 	fdt_fixup_qportals(blob);
 #endif
+
+	/* Now that we've done all the fixups, check the device tree */
+	fdt_verify_addresses(blob);
 }
-- 
1.7.2.3

[U-Boot] [PATCH] powerpc/85xx: compare actual device addresses with the device tree

[Wolfgang Denk]

Dear Timur Tabi,

In message <1289343709-11793-1-git-send-email-timur@freescale.com> you wrote:
> 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.

I understand that you can reasonably check these addresses only in
this specific case (i. e. a 36 bit DT and a 32 bit U-Boot) ?

What is the actualk woth of such a check when it covers only one out
of 4 possible combinations?

And should that not be made optional?  There are enough 85xx boards
which don;t even have 36 bit configurations in Linux, so why waste
memory and runtime on these?

> +	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);

Braces needed for multiline statements. Please fix globally.

> +		/*
> +		 * 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_type is referenced only once, so please move the code and get
rid of the "goto".


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
Perl already has an IDE.  It's called Unix.
                      -- Tom Christiansen in 375bd509 at cs.colorado.edu

[U-Boot] [PATCH] powerpc/85xx: compare actual device addresses with the device tree

[Timur Tabi]

Wolfgang Denk wrote:

> I understand that you can reasonably check these addresses only in
> this specific case (i. e. a 36 bit DT and a 32 bit U-Boot) ?

That was just an example, albeit a very common one.  This code can check for any
mismatch in CCSR device physical addresses between the U-Boot configuration and
the device tree.

> What is the actualk woth of such a check when it covers only one out
> of 4 possible combinations?

It's more than that.  Any mismatch in the CCSR base address, serial device
offsets, or PCI addresses will be caught.  For instance, if you put the PCIE1
memory range at ff800000 in the device tree, but ff6000000 in U-Boot, it will
catch that.

> And should that not be made optional?  There are enough 85xx boards
> which don;t even have 36 bit configurations in Linux, so why waste
> memory and runtime on these?

Because the problem shows up when you least expect it.  It's designed to
eliminate debug problems.  If we make this enabled only when people define a
macro that isn't normally defined, then people won't know about it.  We have so
many problems with customers and other developers getting the device tree wrong,
and immediately contacting us for help without doing even the slightest
debugging.  I'm sure you're familiar with that.

I can add a macro that needs to be defined in order to *disable* it, so that on
finalized systems where the device tree is known to be correct, this check can
be skipped.  But I really would prefer to have this feature enabled by default.

>> +	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);
> 
> Braces needed for multiline statements. Please fix globally.

So you're saying you want to see this:

	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);
	}

> wrong_type is referenced only once, so please move the code and get
> rid of the "goto".

Ok.


-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] powerpc/85xx: compare actual device addresses with the device tree

[Wolfgang Denk]

Dear Timur Tabi,

In message <4CDAFC37.40309@freescale.com> you wrote:
> 
> It's more than that.  Any mismatch in the CCSR base address, serial device
> offsets, or PCI addresses will be caught.  For instance, if you put the PCIE1
> memory range at ff800000 in the device tree, but ff6000000 in U-Boot, it will
> catch that.

Would that hurt? I though Linux does it's own initialization of the
PCI system anyway, so does it matter what we did in U-Boot?

> Because the problem shows up when you least expect it.  It's designed to
> eliminate debug problems.  If we make this enabled only when people define a
> macro that isn't normally defined, then people won't know about it.  We have so
> many problems with customers and other developers getting the device tree wrong,
> and immediately contacting us for help without doing even the slightest
> debugging.  I'm sure you're familiar with that.

Hey, you are not supposed to bring us out of work. Rather help to make
the code complex and difficult to understand ;-)

> I can add a macro that needs to be defined in order to *disable* it, so that on
> finalized systems where the device tree is known to be correct, this check can
> be skipped.  But I really would prefer to have this feature enabled by default.

I understand what you want to do, and why you want to do it, but then
I also feel thatit is inherently wrong. It cannot be U-Boot's taks to
validate the correctness of the device tree on every boot.

If we agree that this is adebug help, then please provide a separate
command to perform this operation. Make this command optional (feel
free to add it to the default list, but it must be possible to disable
it if wanted). Then users who want this feature can add it to their
boot command sequence, and others, who are interested in fast boot
times can omit it.


> So you're saying you want to see this:
> 
> 	if (parent_address_cells == 1)
> 		dt_addr = be32_to_cpup(ranges + address_cells);
> 	else {

Yes, at least. Actually I prefer to use braces for the "then" branch
as well if the "else" branch needs them.

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
:       I've tried (in vi) "g/[a-z]\n[a-z]/s//_/"...but that doesn't
: cut it.  Any ideas?  (I take it that it may be a two-pass sort of solution).
In the first pass, install perl. :-) Larry Wall <6849@jpl-devvax.JPL.NASA.GOV>

[U-Boot] [PATCH] powerpc/85xx: compare actual device addresses with the device tree

[Timur Tabi]

Wolfgang Denk wrote:
> Dear Timur Tabi,
> 
> In message <4CDAFC37.40309@freescale.com> you wrote:
>>
>> It's more than that.  Any mismatch in the CCSR base address, serial device
>> offsets, or PCI addresses will be caught.  For instance, if you put the PCIE1
>> memory range at ff800000 in the device tree, but ff6000000 in U-Boot, it will
>> catch that.
> 
> Would that hurt? I though Linux does it's own initialization of the
> PCI system anyway, so does it matter what we did in U-Boot?

Well, it probably isn't an *error* technically, but it might be unexpected.  But
I will consider removing that code, if there's some kind of consensus that it's
not worthwhile.

>> Because the problem shows up when you least expect it.  It's designed to
>> eliminate debug problems.  If we make this enabled only when people define a
>> macro that isn't normally defined, then people won't know about it.  We have so
>> many problems with customers and other developers getting the device tree wrong,
>> and immediately contacting us for help without doing even the slightest
>> debugging.  I'm sure you're familiar with that.
> 
> Hey, you are not supposed to bring us out of work. Rather help to make
> the code complex and difficult to understand ;-)

Heh.

>> I can add a macro that needs to be defined in order to *disable* it, so that on
>> finalized systems where the device tree is known to be correct, this check can
>> be skipped.  But I really would prefer to have this feature enabled by default.
> 
> I understand what you want to do, and why you want to do it, but then
> I also feel thatit is inherently wrong. It cannot be U-Boot's taks to
> validate the correctness of the device tree on every boot.

Well, I really do think it should be done at every boot by default, but I would
be willing to consider an "fdt verify" command.  Would you be willing to buy me
a beer every time someone forgets to run "fdt verify" and emails me instead?

> If we agree that this is adebug help, then please provide a separate
> command to perform this operation. Make this command optional (feel
> free to add it to the default list, but it must be possible to disable
> it if wanted). Then users who want this feature can add it to their
> boot command sequence, and others, who are interested in fast boot
> times can omit it.

Would "fdt verify" be a good place?

>> So you're saying you want to see this:
>>
>> 	if (parent_address_cells == 1)
>> 		dt_addr = be32_to_cpup(ranges + address_cells);
>> 	else {
> 
> Yes, at least. Actually I prefer to use braces for the "then" branch
> as well if the "else" branch needs them.

Ok.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] powerpc/85xx: compare actual device addresses with the device tree

[Scott Wood]

On Wed, 10 Nov 2010 21:34:51 +0100
Wolfgang Denk <wd@denx.de> wrote:

> Dear Timur Tabi,
> 
> In message <4CDAFC37.40309@freescale.com> you wrote:
> > 
> > It's more than that.  Any mismatch in the CCSR base address, serial device
> > offsets, or PCI addresses will be caught.  For instance, if you put the PCIE1
> > memory range at ff800000 in the device tree, but ff6000000 in U-Boot, it will
> > catch that.
> 
> Would that hurt? I though Linux does it's own initialization of the
> PCI system anyway, so does it matter what we did in U-Boot?

It's caused us problems in the past on 83xx.  I think it still relies
on U-Boot's config.

-Scott

[U-Boot] [PATCH] powerpc/85xx: compare actual device addresses with the device tree

[Wolfgang Denk]

Dear Timur Tabi,

In message <4CDB0686.300@freescale.com> you wrote:
>
> Well, I really do think it should be done at every boot by default, but I would
> be willing to consider an "fdt verify" command.  Would you be willing to buy me
> a beer every time someone forgets to run "fdt verify" and emails me instead?

Definitely not.  I'm afraid you'd never be sober again, and while this
might deem a desirable state to you I would not want to see you
submitting U-Boot patches then :-)

> > If we agree that this is adebug help, then please provide a separate
> > command to perform this operation. Make this command optional (feel
> > free to add it to the default list, but it must be possible to disable
> > it if wanted). Then users who want this feature can add it to their
> > boot command sequence, and others, who are interested in fast boot
> > times can omit it.
> 
> Would "fdt verify" be a good place?

Yes, sounds good to me.


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
"If it ain't broke, don't fix it."                       - Bert Lantz

[U-Boot] [PATCH] powerpc/85xx: compare actual device addresses with the device tree

[Timur Tabi]

Wolfgang Denk wrote:
> Definitely not.  I'm afraid you'd never be sober again, and while this
> might deem a desirable state to you I would not want to see you
> submitting U-Boot patches then :-)

I guess you don't know about the U-Boot patch submission drinking game?

:-)

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] powerpc/85xx: compare actual device addresses with the device tree

[Wolfgang Denk]

Dear Scott Wood,

In message <20101110151241.49d687e8@udp111988uds.am.freescale.net> you wrote:
> 
> > Would that hurt? I though Linux does it's own initialization of the
> > PCI system anyway, so does it matter what we did in U-Boot?
> 
> It's caused us problems in the past on 83xx.  I think it still relies
> on U-Boot's config.

I tend to consider this a bug in the Linux code, then.

Linux should be mostly independent from the boot loader.

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
How many Unix hacks does it take to change a light bulb?  Let's  see,
   can you use a shell script for that or does it need a C program?

[U-Boot] [PATCH] powerpc/85xx: compare actual device addresses with the device tree

[Wolfgang Denk]

Dear Timur Tabi,

In message <4CDB0B65.8090908@freescale.com> you wrote:
>
> I guess you don't know about the U-Boot patch submission drinking game?
> 
> :-)

May I consider this an invitation?

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
Men will always be men -- no matter where they are.
	-- Harry Mudd, "Mudd's Women", stardate 1329.8

[U-Boot] [PATCH] powerpc/85xx: compare actual device addresses with the device tree

[Scott Wood]

On Wed, 10 Nov 2010 22:30:07 +0100
Wolfgang Denk <wd@denx.de> wrote:

> Dear Scott Wood,
> 
> In message <20101110151241.49d687e8@udp111988uds.am.freescale.net> you wrote:
> > 
> > > Would that hurt? I though Linux does it's own initialization of the
> > > PCI system anyway, so does it matter what we did in U-Boot?
> > 
> > It's caused us problems in the past on 83xx.  I think it still relies
> > on U-Boot's config.
> 
> I tend to consider this a bug in the Linux code, then.

Possibly -- and certainly it doesn't seem like something that should be
one way on 83xx and another way on 85xx.  There are some cooperative AMP
cases where it's easier to have Linux just not touch anything, though
that needn't affect what happens in the normal case.  I do recall the
server-side powerpc people liking to keep all PCI device assignments as
the firmware left them, as they tend to have early consoles on PCI and
don't want to disrupt them by moving BARs around (esp. if debug output
is enabled in the PCI code).

There could be some weirdness if the mismatch causes there to be
overlap with something else that U-Boot has set up -- possibly just
temporarily, such as if U-Boot assigns address X to PCI0 and Y to
PCI1, and Linux assigns address X to PCI1 and Y to PCI0.  Linux might
set PCI0 to the new address and try to initialize it before it gets to
looking at PCI1.

In general, it seems like something that we'd like to know about if
they don't match, even if we fix the 83xx dependency.

Also, while 85xx sets the ATMU registers, I don't see where Linux sets
the LAWs.

-Scott

[U-Boot] [PATCH] powerpc/85xx: compare actual device addresses with the device tree

[Timur Tabi]

Wolfgang Denk wrote:
> 
>> > If we agree that this is adebug help, then please provide a separate
>> > command to perform this operation. Make this command optional (feel
>> > free to add it to the default list, but it must be possible to disable
>> > it if wanted). Then users who want this feature can add it to their
>> > boot command sequence, and others, who are interested in fast boot
>> > times can omit it.
>>
>> Would "fdt verify" be a good place?
> 
> Yes, sounds good to me.

There is one problem -- a lot of the code is 85xx-specific.  I'm not sure how to
reasonably make this feature available to all fdt-capable architectures.  Would
you be okay with I enclosed it in an #ifdef CONFIG_MPC85xx, like this:

U_BOOT_CMD(
	fdt,	255,	0,	do_fdt,
	"flattened device tree utility commands",
	    "addr   <addr> [<length>]        - Set the fdt location to <addr>\n"
#ifdef CONFIG_OF_BOARD_SETUP
	"fdt boardsetup                      - Do board-specific set up\n"
#endif
	"fdt move   <fdt> <newaddr> <length> - Copy the fdt to <addr> and make it active\n"
...
#ifdef CONFIG_MPC85xx
	"fdt verify                          - Verify the addresses in the device tree\n"
#endif
	"NOTE: Dereference aliases by omiting the leading '/', "
		"e.g. fdt print ethernet0."
);

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] powerpc/85xx: compare actual device addresses with the device tree

[Wolfgang Denk]

Dear Timur Tabi,

In message <4CE302A5.8070608@freescale.com> you wrote:
>
> >> Would "fdt verify" be a good place?
> > 
> > Yes, sounds good to me.
> 
> There is one problem -- a lot of the code is 85xx-specific.  I'm not sure how to
> reasonably make this feature available to all fdt-capable architectures.  Would
> you be okay with I enclosed it in an #ifdef CONFIG_MPC85xx, like this:

No, I don't like this.  The parts that are not specific to 85xx should
be available to all, and CPU specific (and eventually board specific?)
parts should be possible by calling into respective functions (which
might, for example, have weak dummy implementations).

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
I am pleased to see that we have differences.  May we together become
greater than the sum of both of us.
	-- Surak of Vulcan, "The Savage Curtain", stardate 5906.4

[U-Boot] [PATCH] powerpc/85xx: compare actual device addresses with the device tree

[Timur Tabi]

Wolfgang Denk wrote:
> No, I don't like this.  The parts that are not specific to 85xx should
> be available to all, and CPU specific (and eventually board specific?)
> parts should be possible by calling into respective functions (which
> might, for example, have weak dummy implementations).

Ok, I can do it this way, but 99% of the code will be CPU-specific.  The whole
point behind the function is to compare the device tree numbers with the
physical addresses of the actual devices in hardware.

Do you want the default weak functions to say something like "not implemented",
or just do nothing at all?

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] powerpc/85xx: compare actual device addresses with the device tree

[Wolfgang Denk]

Dear Timur Tabi,

In message <4CE30FE1.2010806@freescale.com> you wrote:
>
> Ok, I can do it this way, but 99% of the code will be CPU-specific.  The whole
> point behind the function is to compare the device tree numbers with the
> physical addresses of the actual devices in hardware.

Maybe this can be implemented in a largely CPU independent way? Try to
make it data-driven, so you can then for example just provide a
CPU-specific data sructure (array of names / addresses or whatever you
are checking).
 
> Do you want the default weak functions to say something like "not implemented",
> or just do nothing at all?

No news is good news.  Make them silent, please.

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
Why can you only have two doors on a chicken coop? If it had four  it
would be a chicken sedan.

[U-Boot] [PATCH] powerpc/85xx: compare actual device addresses with the device tree

[Tabi Timur-B04825]

Wolfgang Denk wrote:
> Maybe this can be implemented in a largely CPU independent way? Try to
> make it data-driven, so you can then for example just provide a
> CPU-specific data sructure (array of names / addresses or whatever you
> are checking).

I'm not sure this will work out that well, but I'll try.

>> >  Do you want the default weak functions to say something like "not implemented",
>> >  or just do nothing at all?
> No news is good news.  Make them silent, please.

My only concern with being silent is that if the user types in "fdt verify"
and gets no output, he'll think that his device tree has been verified and
passes all tests.

-- 
Timur Tabi
Linux kernel developer