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.
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 :-)
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").
Actually I do favour a revert as it will allow us to discuss the fix with a clean slate.
Regards, Simon