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

[Simon Glass]

Hi Tom.

On 3 August 2015 at 11:25, Tom Rini <trini at konsulko.com> wrote:
> On Mon, Aug 03, 2015 at 09:52:50AM -0600, 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.
>>
>> >
>> > 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.
>
> Well, we can unbreak one set of stuff and break another set of stuff,
> that's not a clean slate for the now broken platforms.  If it helps to
> come up with a better patch we can step one revert and step two better,
> but I don't want to apply without step two ready.
>
> ... speaking as someone who has people poking me about if the SPI
> flasher they sent me works so I can try U-Boot on x86 things.

Fair enough - I don't want to break things either. Perhaps we can
apply a revert and the new patch one after the other. But really
however we can fix this is fine with me.

Regards,
Simon