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