Hi Stephen,
On 3 August 2015 at 12:20, Stephen Warren swarren@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@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