[U-Boot] [PATCH] Remove extra fdt_fixup_ethernet() call

[Chen-Yu Tsai]

On Mon, Apr 3, 2017 at 3:03 PM, Joakim Tjernlund
<Joakim.Tjernlund at infinera.com> wrote:
> On Fri, 2017-03-31 at 22:21 -0600, Simon Glass wrote:
>> Hi Joakim,
>>
>> On 23 March 2017 at 11:02, Joakim Tjernlund
>> <joakim.tjernlund at infinera.com> wrote:
>> > ft_cpu_setup() already calls fdt_fixup_ethernet(), calling it
>> > in image_setup_libfdt() is both redundant and breaks any modifications
>> > done by ft_board_setup(). Restore the old behavior by removing
>> > the call in image_setup_libfdt()
>>
>> Which old behaviour? Can you please add a Fixes: tag with the details?
>
> The feature of having the possibility to rewrite the dev trees MAC addresses in
> ft_board_setup(). Currently such rewrites are overwritten by the call to dt_fixup_ethernet(blob)
> in image_setup_libfdt().
> Looking into the history I see that commit 13d06981(by you as it happens :) is the one adding
> the extra call to fdt_fixup_ethernet()
>>
>> Also, which ft_cpu_setup()?
> # > git grep fdt_fixup_ethernet
> arch/arm/cpu/armv7/ls102xa/fdt.c:       fdt_fixup_ethernet(blob);
> arch/mips/lib/bootm.c:  fdt_fixup_ethernet(images->ft_addr);
> arch/nios2/cpu/fdt.c:   fdt_fixup_ethernet(blob);
> arch/powerpc/cpu/mpc512x/cpu.c: fdt_fixup_ethernet(blob);
> arch/powerpc/cpu/mpc8260/cpu.c: fdt_fixup_ethernet(blob);
> arch/powerpc/cpu/mpc83xx/fdt.c: fdt_fixup_ethernet(blob);
> arch/powerpc/cpu/mpc85xx/fdt.c: fdt_fixup_ethernet(blob);
> arch/powerpc/cpu/mpc86xx/fdt.c: fdt_fixup_ethernet(blob);
> arch/powerpc/cpu/mpc8xx/fdt.c:  fdt_fixup_ethernet(blob);
> arch/powerpc/cpu/ppc4xx/fdt.c:  fdt_fixup_ethernet(blob);
>
> Seems like it is mostly PPC which has fdt_fixup_ethernet() call though.

I think that's because everyone else is depending on the call in
image_setup_libfdt... So far RPi and sunxi are broken. I'm sure
there are others.

IMHO, the commit message offers little context on the attempts
to check existing users would break or not. Just because someone
is not explicitly calling it does not mean they aren't expecting
that behavior from the system. There's also no context on what
_your_ platform is, how it's affected and and why actually needs
the explicit call.

Yes the 2 redundant calls are confusing, and one overwrites the
other. However it was introduced in 2007, and device tree usage
has increased a lot, particularly for ARM, since then. For us,
the "new" behavior was standard from day 1.

Regards
ChenYu

>
>  Jocke
>>
>> >
>> > Signed-off-by: Joakim Tjernlund <joakim.tjernlund at infinera.com>
>> > ---
>> >  common/image-fdt.c | 1 -
>> >  1 file changed, 1 deletion(-)
>> >
>> > diff --git a/common/image-fdt.c b/common/image-fdt.c
>> > index 80e3e63..b8f5654 100644
>> > --- a/common/image-fdt.c
>> > +++ b/common/image-fdt.c
>> > @@ -498,7 +498,6 @@ int image_setup_libfdt(bootm_headers_t *images, void *blob,
>> >                         goto err;
>> >                 }
>> >         }
>> > -       fdt_fixup_ethernet(blob);
>> >
>> >         /* Delete the old LMB reservation */
>> >         lmb_free(lmb, (phys_addr_t)(u32)(uintptr_t)blob,
>> > --
>> > 2.10.2
>> >
>>
>> Regards,
>> Simon
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot