[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