Message ID | CAB4PhKcbTzL7OOwdx2TB_iCto2h8Es7uQZzozGJHB0kNFTuFkg@mail.gmail.com |
---|---|
State | Changes Requested |
Headers | show |
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
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
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,
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. >
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 --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