[U-Boot] [PATCH] Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

[Stephen Warren]

On 08/03/2015 09:52 AM, Simon Glass wrote:
> Hi Stephen,
>
> On 3 August 2015 at 09:12, Stephen Warren <swarren at wwwdotorg.org> wrote:
>> On 08/02/2015 06:13 PM, Simon Glass wrote:
>>>
>>> This reverts commit 5b34436035fc862b5e8d0d2c3eab74ba36f1a7f4.
>>>
>>> This function has a few problems. It calls fdt_parent_offset() which as
>>> mentioned in code review is very slow.
>>>
>>> https://patchwork.ozlabs.org/patch/499482/
>>> https://patchwork.ozlabs.org/patch/452604/
>>>
>>> It also happens to break SPI flash on Minnowboard max which is how I
>>> noticed
>>> that this was applied. I can send a patch to tidy that up, but in any case
>>> I think we should consider a revert until the function is better
>>> implemented.
>>
>>
>> The fact that the function needs to perform a slow operation is not a good
>> reason for a revert. The slowness of the operation is just a matter of fact
>> with DT not having uplinks in its data structure, and U-Boot using those
>> data structures directly.
>>
>> You'd requested during review that I additionally implement a faster version
>> of the function in the case where the parent node is already known, and said
>> it was fine if that happened in a later patch. I have this on my TODO list,
>> but it's only been a couple of days.
>
> I didn't expect this to go to mainline before your new patch.

I didn't get that message from the thread; you wrote:

Simon Glass wrote:
 > Stephen Warren wrote:
 >> Simon Glass wrote:
 >>> Also, how about (in addition) a
 >>> version of this function that works for devices? Like:
 >>>
 >>> device_get_addr_size(struct udevice *dev, ...)
 >>>
 >>> so that it can handle this for you.
 >>
 >> That sounds like a separate patch?
 >
 > Yes, but I think we should get it in there so that people don't start
 > using this (wildly inefficient) function when they don't need to. I
 > think by passing in the parent node we force people to think about the
 > cost.
 >
 > Yes the driver model function can be a separate patch, but let's get
 > it in at about the same time. We have dev_get_addr() so could have
 > dev_get_addr_size().

That sounds to like you'd like a followup patch soon after this patch 
(my assumption would be within a few days or weeks) to solve your 
comments, not that you wanted a replacement patch.

>> Why not just fix the bug since you said you could? That seems far better
>> than breaking the newly added Tegra210 support.
>
> I do have a patch but I'm not 100% comfortable with the approach. I
> don't see a good reason to move away from the idea of fdt_addr_t and
> fdt_addr_t being set correctly for the platform. Or maybe I
> misunderstand the problem the patch was trying to fix. As I said it
> did not have a commit message, so who knows :-)

The size of fdt_addr_t isn't relevant *at all* when parsing DT. The only 
thing that matters is #address-cells/#size-cells. Those properties are 
what tell the parsing code how many bytes to read from the reg property. 
Whether the resultant value fits into the code's internal representation 
is an internal detail of the code, not part of the semantics of DT 
itself or how to parse it.

If code assumes that #address-cells == sizeof(fdt_addr_t), it is indeed 
quite likely that everything will just happen to work most of the time. 
However, this is purely an accident and not something that anything 
should rely upon.

(I think Tegra210 support still has CONFIG_PHYS_64BIT undefined which is 
admittedly a little /unexpected/ for a 64-bit U-Boot build, but in 
practice is perfectly /legal/ and will work out just fine since, except 
perhaps for RAM sizes, I don't believe any value in DT will actually 
require more than 32-bits to represent)

>> P.S. What is the issue with SPI flash? The commit description doesn't
>> mention this at all.
>
> It calls that function expecting it to pick up an address and size
> from two consecutive cells. With this patch, that fails (unless the
> property happens to be "reg").

I don't see anything in the new code that interprets the property name 
in any way, or compares it against "reg". Can you point out where the 
code is doing something differently based on property name?

Perhaps you mean instead that something is calling 
fdtdec_get_addr_size() on a property that's not intended to be affected 
by #address-call/#size-cells? If so, I would assert that's a bug in the 
calling code; if fdtdec_get_addr_size() is ever intended to parse the 
reg property, then fdtdec_get_addr_size() must always honor 
#address-call/#size-cells since that's how the reg property works. If 
some code wants to parse a property where #address-call/#size-cells 
shouldn't be honored, but instead some fixed size assumed, then a 
different function should be called instead (or perhaps some additional 
function parameters added to specify override values for 
#address-call/#size-cells).

I think this is all stemming from drivers/mtd/spi/sf_probe.c 
spi_flash_decode_fdt()'s call to fdtdec_get_addr_size() for property
"memory-map"? If so, then looking at arch/x86/dts/minnowmax.dts's /spi 
node, the code and DT content are clearly inconsistent; For this node 
#address-cells=<1>, #size-cells=<0> which makes sense given that the 
address is a chip-select and hence has no size. So, the code should not 
assume that the memory-map property can be parsed in the same way as a 
reg property.

I note that the memory-map property doesn't exist in the Linux kernel's 
DT binding documentation database, or code, hence hasn't been reviewed 
by the DT binding maintainers.