[U-Boot] Commit "ARM: CPU: arm926ejs: Consolidate cache routines to common file" breaks u-boot on Dreamplug

[Adam Ford]

On Wed, Feb 27, 2019 at 8:52 AM Leigh Brown <leigh at solinno.co.uk> wrote:
>
> Hi Adam,
>
> Thanks very much for your response.
>
> On 2019-02-27 12:50, Adam Ford wrote:
> > On Wed, Feb 27, 2019 at 5:32 AM Leigh Brown <leigh at solinno.co.uk>
> > wrote:
> >>
> >> Hello,
> >>
> >> Vagrant Cascadian asked for people to test the version of u-boot
> >> packaged
> >> for Debian Buster.  I tested u-boot on my Dreamplug and found it was
> >> not
> >> working correctly.  I raised a bug for Debian[1] but I have also
> >> tested
> >> with the mainline version of u-boot and found the same issues.
> >>
> >> The first issue is that the following commit caused u-boot to no
> >> longer
> >> be able to access usb storage on the Dreamplug:
> >>
> >> commit 93b283d49f933f95f3a6f40762936f454ac655a8
> >> Author: Adam Ford <aford173 at gmail.com>
> >> Date:   Thu Aug 16 13:23:11 2018 -0500
> >>
> >
> > Sorry about that.
> >
> >>      ARM: CPU: arm926ejs: Consolidate cache routines to common file
> >>
> >>      Four different boards had different options for enabling cache
> >>      that were virtually all the same.  This consolidates these
> >>      common functions into arch/arm/cpu/arm926ejs/cache.c
> >>
> >>      This also has the positive side-effect of enabling cache on
> >>      the Davinci (da850) boards.
> >>
> >>      Signed-off-by: Adam Ford <aford173 at gmail.com>
> >>      [trini: Add mach-at91 to the list of consolidations]
> >>      Signed-off-by: Tom Rini <trini at konsulko.com>
> >>
> >> I don't have much knowledge of ARM caching, but the following patch
> >> makes
> >> it work again on my Dreamplug.
> >
> > I am not that familiar with the ARM caching either, I was just hoping
> > to enable it on the da850 board and compared the various code chunks
> > between ARM 926 boards and noticed a common thread.  Tom took it and
> > added some more.
>
> Okay.  Hopefully Tom can comment on my proposed fixes.
>
> >
> >>
> >> diff --git a/arch/arm/mach-kirkwood/cpu.c
> >> b/arch/arm/mach-kirkwood/cpu.c
> >> index d54de53f31..8a065d73ae 100644
> >> --- a/arch/arm/mach-kirkwood/cpu.c
> >> +++ b/arch/arm/mach-kirkwood/cpu.c
> >> @@ -291,7 +291,6 @@ int arch_misc_init(void)
> >>         temp |= (1 << 22);
> >>         writefr_extra_feature_reg(temp);
> >>
> > #ifndef CONFIG_SYS_ICACHE_OFF
> >> -       icache_enable();
> > #endif
> >
> > Instead of commenting out that line, try defining
> > CONFIG_SYS_ICACHE_OFF in your header like you did for the
> > CONFIG_SYS_DCACHE_OFF and encapsulate the function call with ifdef's
> > so any other kirkwood processors can enable/disable it independently.
>
> The reason I removed that line is because icache_enable() is called from
> enable_caches() which is called from initr_caches() which is in the list
> of functions in init_sequence_r[] prior to arch_misc_init().  In other
> words, it will already have been called by then if CONFIG_SYS_ICACHE_OFF
> is not set.  Does that make sense?

That makes a lot of sense to me.

>
> >>         /* Change reset vector to address 0x0 */
> >>         temp = get_cr();
> >>         set_cr(temp & ~CR_V);
> >> diff --git a/include/configs/dreamplug.h b/include/configs/dreamplug.h
> >> index f4d717213c..6348935c68 100644
> >> --- a/include/configs/dreamplug.h
> >> +++ b/include/configs/dreamplug.h
> >> @@ -79,4 +79,6 @@
> >>   #define CONFIG_SYS_ATA_IDE0_OFFSET    MV_SATA_PORT0_OFFSET
> >>   #endif /*CONFIG_MVSATA_IDE*/
> >>
> >> +#define CONFIG_SYS_DCACHE_OFF
> > #define CONFIG_SYS_ICACHE_OFF
>
> The reason I have not done this is because the Kirkwood arch_misc_init()
> function was already unconditionally enabling the instruction cache, so
> we want to retain that behaviour - I think.  I hope that makes sense.

That makes a lot of sense too.

adam

>
> >> +
> >>   #endif /* _CONFIG_DREAMPLUG_H */
> >>
> >> I'd be grateful if someone could take a look.  If you need me to do
> >> any
> >> testing  or provide any more information, please let me know.
> >>
> >> I found another issue (documented in the same bug).  I'll send a
> >> separate
> >> email about that.
> >>
> >> Regards,
> >>
> >> Leigh.
> >>
> >> --
> >> [1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=923379
>
> Regards,
>
> Leigh.