diff mbox

[U-Boot,v,1/3] arm: do not force d-cache enable on all boards

Message ID CAB4PhKcbTzL7OOwdx2TB_iCto2h8Es7uQZzozGJHB0kNFTuFkg@mail.gmail.com
State Changes Requested
Headers show

Commit Message

Jason Liu Aug. 1, 2011, 4:33 p.m. UTC 
Hi, Aneesh,

2011/8/1 Aneesh V <aneesh@ti.com>:
> c2dd0d45540397704de9b13287417d21049d34c6 added dcache_enable()
> to board_init_r(). This enables d-cache for all ARM boards.
> As a result some of the arm boards that are not cache-ready
> are broken. Revert this change and allow platform code to
> take the decision on d-cache enabling.
>
> Also add some documentation for cache usage in ARM.
>
> Signed-off-by: Aneesh V <aneesh@ti.com>
> ---
> MAKEALL pending. Will update the results tomorrow.
> ---
>  arch/arm/lib/board.c  |    8 +++-----
>  arch/arm/lib/cache.c  |   12 ++++++++++++
>  doc/README.arm-caches |   40 ++++++++++++++++++++++++++++++++++++++++
>  include/common.h      |    1 +
>  4 files changed, 56 insertions(+), 5 deletions(-)
>  create mode 100644 doc/README.arm-caches
>
> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
> index 90709d0..d093d5b 100644
> --- a/arch/arm/lib/board.c
> +++ b/arch/arm/lib/board.c
> @@ -446,11 +446,9 @@ void board_init_r (gd_t *id, ulong dest_addr)
>        gd->flags |= GD_FLG_RELOC;      /* tell others: relocation done */
>
>        monitor_flash_len = _end_ofs;
> -       /*
> -        * Enable D$:
> -        * I$, if needed, must be already enabled in start.S
> -        */
> -       dcache_enable();
> +
> +       /* Enable caches */
> +       enable_caches();
>
>        debug ("monitor flash len: %08lX\n", monitor_flash_len);
>        board_init();   /* Setup chipselects */
> diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
> index 92b61a2..b545fb7 100644
> --- a/arch/arm/lib/cache.c
> +++ b/arch/arm/lib/cache.c
> @@ -53,3 +53,15 @@ void __flush_dcache_all(void)
>  }
>  void   flush_dcache_all(void)
>        __attribute__((weak, alias("__flush_dcache_all")));
> +
> +
> +/*
> + * Default implementation of enable_caches()
> + * Real implementation should be in platform code
> + */
> +void __enable_caches(void)
> +{
> +       puts("WARNING: Caches not enabled\n");
> +}
> +void enable_caches(void)
> +       __attribute__((weak, alias("__enable_caches")));


What about the following change?

#ifndef CONFIG_SYS_DCACHE_OFF
        dcache_enable();
#else
       puts("WARNING: Caches not enabled\n");
#endif

This can avoid adding the duplicated cache-enable code in every board later.
Just looked at your patches for omap3 and omap4:
 arch/arm/cpu/armv7/omap3/board.c |    8 ++++++++
 arch/arm/cpu/armv7/omap4/board.c |    8 ++++++++
 2 files changed, 16 insertions(+), 0 deletions(-)


Maybe there will be many many duplicated code like this, do you wish that?

Jason

Comments

Jason Liu Aug. 1, 2011, 4:46 p.m. UTC | #1 
2011/8/2 Jason Liu <liu.h.jason@gmail.com>:
> Hi, Aneesh,
>
> 2011/8/1 Aneesh V <aneesh@ti.com>:
>> c2dd0d45540397704de9b13287417d21049d34c6 added dcache_enable()
>> to board_init_r(). This enables d-cache for all ARM boards.
>> As a result some of the arm boards that are not cache-ready
>> are broken. Revert this change and allow platform code to
>> take the decision on d-cache enabling.
>>
>> Also add some documentation for cache usage in ARM.
>>
>> Signed-off-by: Aneesh V <aneesh@ti.com>
>> ---
>> MAKEALL pending. Will update the results tomorrow.
>> ---
>>  arch/arm/lib/board.c  |    8 +++-----
>>  arch/arm/lib/cache.c  |   12 ++++++++++++
>>  doc/README.arm-caches |   40 ++++++++++++++++++++++++++++++++++++++++
>>  include/common.h      |    1 +
>>  4 files changed, 56 insertions(+), 5 deletions(-)
>>  create mode 100644 doc/README.arm-caches
>>
>> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
>> index 90709d0..d093d5b 100644
>> --- a/arch/arm/lib/board.c
>> +++ b/arch/arm/lib/board.c
>> @@ -446,11 +446,9 @@ void board_init_r (gd_t *id, ulong dest_addr)
>>        gd->flags |= GD_FLG_RELOC;      /* tell others: relocation done */
>>
>>        monitor_flash_len = _end_ofs;
>> -       /*
>> -        * Enable D$:
>> -        * I$, if needed, must be already enabled in start.S
>> -        */
>> -       dcache_enable();
>> +
>> +       /* Enable caches */
>> +       enable_caches();
>>
>>        debug ("monitor flash len: %08lX\n", monitor_flash_len);
>>        board_init();   /* Setup chipselects */
>> diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
>> index 92b61a2..b545fb7 100644
>> --- a/arch/arm/lib/cache.c
>> +++ b/arch/arm/lib/cache.c
>> @@ -53,3 +53,15 @@ void __flush_dcache_all(void)
>>  }
>>  void   flush_dcache_all(void)
>>        __attribute__((weak, alias("__flush_dcache_all")));
>> +
>> +
>> +/*
>> + * Default implementation of enable_caches()
>> + * Real implementation should be in platform code
>> + */
>> +void __enable_caches(void)
>> +{
>> +       puts("WARNING: Caches not enabled\n");
>> +}
>> +void enable_caches(void)
>> +       __attribute__((weak, alias("__enable_caches")));
>
>
> What about the following change?
>
> #ifndef CONFIG_SYS_DCACHE_OFF
>        dcache_enable();
> #else
>       puts("WARNING: Caches not enabled\n");
> #endif

Or better:

#ifdef CONFIG_SYS_DCACHE_ON
        dcache_enable();
 #else
       puts("WARNING: Caches not enabled\n");
 #endif

In fact, we can turn on I-cache safely by default, turn-off D-cache by default,
But give big warning to board-maintainer to fix it later if turn off D-cache.

This can keep the most of config file not change and keep using the
only one copy
of common d-cache enable function to avoid code duplication on every board.

Jason
Wolfgang Denk Aug. 1, 2011, 7:45 p.m. UTC | #2 
Dear Jason Liu,

In message <CAB4PhKdj0prR3etkkz5PSqKPgS69CFZepY83Gw-aPQKcFkyKkQ@mail.gmail.com> you wrote:
>
> > What about the following change?
> >
> > #ifndef CONFIG_SYS_DCACHE_OFF
> >        dcache_enable();
> > #else
> >       puts("WARNING: Caches not enabled\n");
> > #endif
> 
> Or better:
> 
> #ifdef CONFIG_SYS_DCACHE_ON
>         dcache_enable();
>  #else
>        puts("WARNING: Caches not enabled\n");
>  #endif
>
> In fact, we can turn on I-cache safely by default, turn-off D-cache by default,
> But give big warning to board-maintainer to fix it later if turn off D-cache.

This is NOT better, it is worse.

The rule is that you don't need to #define a specific option if you
want default behaviour.  Default behaviour should be caches on, so
only "broken" boards where this does not work yet should be able to
opt out by defining some option.


Best regards,

Wolfgang Denk
Albert ARIBAUD Aug. 1, 2011, 7:53 p.m. UTC | #3 
Hi Jason,

Le 01/08/2011 18:33, Jason Liu a écrit :

> Maybe there will be many many duplicated code like this, do you wish that?

I don't think this will or should be duplicated for each ARM board; more 
like suplicated by SoC, or more precisely, by ARM implementation (i.e., 
one cache handling for each of arch/arm/<architecture>/<implementation>) 
-- more or less.

> Jason

Amicalement,
Jason Liu Aug. 2, 2011, 2:35 p.m. UTC | #4 
Hi, Albert,

2011/8/2 Albert ARIBAUD <albert.u.boot@aribaud.net>:
> Hi Jason,
>
> Le 01/08/2011 18:33, Jason Liu a écrit :
>
>> Maybe there will be many many duplicated code like this, do you wish that?
>
> I don't think this will or should be duplicated for each ARM board; more
> like suplicated by SoC, or more precisely, by ARM implementation (i.e., one
> cache handling for each of arch/arm/<architecture>/<implementation>) -- more
> or less.

Yes, not each ARM board, but should be a lot as the followings,

drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 mx31
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 mx35
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 omap24xx
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 s3c64xx
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 tnetv107x
drwxr-xr-x 2 r64343 r64343  4096 2011-07-29 15:21 lpc2292
drwxr-xr-x 2 r64343 r64343  4096 2011-04-13 13:00 s3c4510b
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 a320
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 at91rm9200
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 ep93xx
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 ks8695
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 s3c24x0
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 armada100
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 davinci
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 kirkwood
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 mb86r0x
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 mx25
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 mx27
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 nomadik
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 omap
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 orion5x
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 pantheon
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 spear
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 versatile
drwxr-xr-x 2 r64343 r64343  4096 2011-07-29 19:12 mx5
drwxr-xr-x 2 r64343 r64343  4096 2011-07-29 10:20 omap3
drwxr-xr-x 2 r64343 r64343  4096 2011-07-28 17:23 omap4
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 omap-common
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 10:46 s5pc1xx
drwxr-xr-x 2 r64343 r64343  4096 2011-07-26 17:37 s5pc2xx
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 s5p-common
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 tegra2
drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 u8500
drwxr-xr-x 3 r64343 r64343  4096 2011-07-29 15:21 npe

All these arm/<architecture>/<implementation>s will have the duplicated code.
can we consolidate it?

Jason

> Amicalement,
> --
> Albert.
>
Albert ARIBAUD Aug. 2, 2011, 3:58 p.m. UTC | #5 
Le 02/08/2011 16:35, Jason Liu a écrit :
> Hi, Albert,
>
> 2011/8/2 Albert ARIBAUD<albert.u.boot@aribaud.net>:
>> Hi Jason,
>>
>> Le 01/08/2011 18:33, Jason Liu a écrit :
>>
>>> Maybe there will be many many duplicated code like this, do you wish that?
>>
>> I don't think this will or should be duplicated for each ARM board; more
>> like suplicated by SoC, or more precisely, by ARM implementation (i.e., one
>> cache handling for each of arch/arm/<architecture>/<implementation>) -- more
>> or less.
>
> Yes, not each ARM board, but should be a lot as the followings,
>
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 mx31
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 mx35
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 omap24xx
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 s3c64xx
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 tnetv107x
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-29 15:21 lpc2292
> drwxr-xr-x 2 r64343 r64343  4096 2011-04-13 13:00 s3c4510b
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 a320
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 at91rm9200
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 ep93xx
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 ks8695
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 s3c24x0
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 armada100
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 davinci
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 kirkwood
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 mb86r0x
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 mx25
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 mx27
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 nomadik
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 omap
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 orion5x
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 pantheon
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 spear
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 versatile
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-29 19:12 mx5
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-29 10:20 omap3
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-28 17:23 omap4
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 omap-common
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 10:46 s5pc1xx
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-26 17:37 s5pc2xx
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 s5p-common
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 tegra2
> drwxr-xr-x 2 r64343 r64343  4096 2011-07-27 11:04 u8500
> drwxr-xr-x 3 r64343 r64343  4096 2011-07-29 15:21 npe
>
> All these arm/<architecture>/<implementation>s will have the duplicated code.
> can we consolidate it?

It might be (partially) possible to factorize some of the code from 
implementations of the same <architecture> level (e.g., arm926ejs 
architecture for orion5x, kirkwood, etc).

This will be something that developers (and reviewers) will need to keep 
in mind when submitting patches that enable caches on ARM boards: such 
code should be split across ARM architecture and implementation, so that 
other implementations of the same arch will benefit from the common 
architecture part.

> Jason

Amicalement,
diff mbox

Patch

diff --git a/arch/arm/cpu/armv7/omap3/board.c b/arch/arm/cpu/armv7/omap3/board.c
index 98519a9..de0e90d 100644
--- a/arch/arm/cpu/armv7/omap3/board.c
+++ b/arch/arm/cpu/armv7/omap3/board.c
@@ -390,3 +390,11 @@  void v7_outer_cache_disable(void)
       omap3_update_aux_cr(0, 0x2);
 }
 #endif
+
+#ifndef CONFIG_SYS_DCACHE_OFF
+void enable_caches(void)
+{
+       /* Enable D-cache. I-cache is already enabled in start.S */
+       dcache_enable();
+}
+#endif
diff --git a/arch/arm/cpu/armv7/omap4/board.c b/arch/arm/cpu/armv7/omap4/board.c
index de4cc2a..6ea8a2e 100644
--- a/arch/arm/cpu/armv7/omap4/board.c
+++ b/arch/arm/cpu/armv7/omap4/board.c
@@ -139,3 +139,11 @@  void v7_outer_cache_disable(void)
       set_pl310_ctrl_reg(0);
 }
 #endif
+
+#ifndef CONFIG_SYS_DCACHE_OFF
+void enable_caches(void)
+{
+       /* Enable D-cache. I-cache is already enabled in start.S */
+       dcache_enable();
+}
+#endif