[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 at 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