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

[Stephen Warren]

On 08/04/2015 10:08 PM, Simon Glass wrote:
> 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

In general, there's no "should not" here; we "cannot". If there's a 
64-bit value in the DT (with bits above bit 31 set), then it can't be 
stored in a 32-bit variable.

That said, if a DT has #address-cells=<2>, but the actual values stored 
in all reg values have 0 for the MS word, that'll actually work just 
fine. Note that it's 100% legal to set #address-cells=<100> and just 
write a bunch of extra zeros into the property. Silly perhaps, but 
perfectly legal. Since the function should adapt to whatever 
#address-cells value is in the DT, supporting that case isn't any more 
work, so there's no reason we shouldn't.

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

We could and likely should set CONFIG_PHYS_64BIT for Tegra210. However, 
that's unrelated to using the correct algorithm to parse DT.

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

And Thierry fixed it for systems with #address-cells > 1. Perhaps that 
part of the function comment should have been removed in the commit.

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

Again, there's no way in general to make it more efficient. The 
efficiency issue is directly implied by the DT data structures.

In the special case where the parent node is already known, we can 
certainly introduce an alternate function that is more efficient. You've 
already asked for that, and as I said, it's in my TODO list.

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

It's not a corner case. It's a fundamental part of the DT schema. If 
U-Boot is going to use DT, it should actually use *DT*, not some-very 
similar-but-not-quite-DT format with all kinds of implicit and unstated 
exceptions, limitations, and assumptions. This implies fully honoring 
all aspects of how DT works when parsing it, not picking and choosing 
features because some are inconvenient or annoying. If U-Boot doesn't 
want to correctly implement DT support, it should just drop it completely.

As an aside, when instantiating devices, I hope one day that U-Boot will 
parse DT top-down in a hierarchical way, rather than simply searching 
for any node anywhere in the DT without regard for whether any parent 
node is enabled, has a driver, has had the driver initialize, etc. 
U-Boot ignores this right now, and is only getting away with this 
accidentally. Without hacky workarounds in drivers, this won't continue 
to work for all Tegra HW (e.g. host1x graphics/display sub-modules, 
AHUB's audio-related sub-modules), since parent drivers must initialize 
before child drivers in order to enable various register buses, 
including clocks/resets affecting those buses etc.

Again, if it's simply too inconvenient or bloated to implement DT 
properly in U-Boot, let's just drop it entirely. A halfway solution is 
the worst of both worlds. I'm not convinced the full implications of how 
to (and the need to) correctly and fully support DT have were well 
thought through before (control) DT support was added to U-Boot.

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

With the DT files now checked into U-Boot, I think it would accidentally 
work, since we just happen to have set #address-cells=<2>, 
#size-cells=<2>, and that would just happen to match sizeof(fdt_addr_t).

However note this is an accident on a couple of levels:

a) This is because the code assumes that sizeof(fdt_addr_t) == 
#address-cells * 4. This is an invalid assumption since it does not 
correctly honor the DT schema. It hard-codes the size of values whereas 
DT schema says the size is defined by the #xxx-cells properties.

b) The original Tegra210 DTs that TomW posted had #address-cells=<1>, 
#size-cells=<1>. I asked him to change that to match what I expected to 
be in the Linux kernel's Tegra210 DTs. However, if he'd rejected my 
request or I hadn't noticed that, then with CONFIG_PHYS_64BIT set, 
fdtdec_get_addr_size() would have attempted to read twice as many bytes 
as it should have from the property. It's entirely plausible that 
someone could have come along later and realized CONFIG_PHYS_64BIT was 
set incorrectly and enabled it.

> This is a boot loader so we should be willing to make some
> simplifications.

When dealing with internal bootloader details, sure assumptions, 
simplifications, etc. can be made.

However, DT is an externally defined standard. The content of DT must be 
identical across all OSs (SW stacks, bootloader) and not influenced by 
requirements/... of any specific individual OS's (SW stack, bootloader) 
quirks. We can't just pick and choose which parts of it we care about. 
Well, perhaps if we stop calling it DT we could.