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

Wolfgang Denk
2010-11-10T19:51:13Z

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