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

[Simon Glass]

Hi Stephen,

On 3 August 2015 at 12:20, Stephen Warren <swarren at wwwdotorg.org> wrote:
> 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.

I will take that feedback to be a little more forceful in future, sorry.

>
>>> 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)

I would like to have the types match the platform where possible. At
least the types should not be smaller than the platform - e.g. if
CONFIG_PHYS_64BIT is not defined we should not support 64-bit
address/size in the device tree. This is for efficiency. We don't want
to force all the U-Boot code to 64-bit suddenly. This will bloat
things for no benefit.

>
>>> 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.

The function comment says:

 * Look up an address property in a node and return it as an address.
 * The property must hold one address with a length. This is only tested
 * on 32-bit machines.

My intention was that this would be an efficient way to decode an
address and size from a device tree. To some extent regmap may take
over this role (IMO we should turn to drop fdtdec one day one we have
more infrastructure). But I'd like it to work efficiently for 32-bit
machines. The new function could hardly be less efficient.

I think we should consider the case where the physical address size of
U-Boot and the device tree do not match as a corner case. I certainly
don't want device tree to add loads of pointless code for 'normal'
platforms.

Re memory-map, yes it doesn't seem to be possible to do what it is
trying to do (and Thierry says the same below). It is quite weird to
have a SPI peripheral which is also memory mapped.

Here's my question - if you fix the CONFIG_PHYS_64BIT problem with
64-bit Tegra, what is actually wrong with the way the function was?
This is a boot loader so we should be willing to make some
simplifications.

Regards,
Simon