| Message ID | 1311668750-5471-1-git-send-email-jason.hui@linaro.org |
|---|---|
| State | Changes Requested |
| Headers | show |
On 07/26/2011 10:25 AM, Jason Liu wrote: > CONFIG_L2_OFF is obsolete after the following commit: > > e47f2db5371047eb9bcd115fee084e6a8a92a239 > armv7: rename cache related CONFIG flags > Replace the cache related CONFIG flags with more meaningful > names. Following are the changes: > CONFIG_L2_OFF -> CONFIG_SYS_L2CACHE_OFF > > But the above commit does not clean up all the mx53 board config file. > This patch does it by using CONFIG_SYS_L2CACHE_OFF instead of _L2_OFF > Hi Jason, I agree that CONFIG_L2_OFF is obsolete and must be removed. However, I do not see any application of CONFIG_SYS_L2CACHE_OFF. If I grep into u-boot code, this config is not used at all for i.MX. Indeed, I can find application for OMAP boards: arch/arm/cpu/armv7/omap3/board.c:#ifndef CONFIG_SYS_L2CACHE_OFF arch/arm/cpu/armv7/omap3/board.c:#ifndef CONFIG_SYS_L2CACHE_OFF arch/arm/cpu/armv7/s5pc1xx/cache.S:#ifndef CONFIG_SYS_L2CACHE_OFF arch/arm/cpu/armv7/omap4/board.c:#ifndef CONFIG_SYS_L2CACHE_OFF So this setup is usefulness, I think. By the way (and I know it is not the same topic), do you mean maybe CONFIG_SYS_DCACHE_OFF ? Have you read the thread related to cache problems on MX51 ? Maybe we have to disable D_CACHE until we find a fix for the drivers. What do you mean ? Best regards, Stefano Babic
Dear Jason Liu, In message <1311668750-5471-1-git-send-email-jason.hui@linaro.org> you wrote: > CONFIG_L2_OFF is obsolete after the following commit: > > e47f2db5371047eb9bcd115fee084e6a8a92a239 > armv7: rename cache related CONFIG flags > Replace the cache related CONFIG flags with more meaningful > names. Following are the changes: > CONFIG_L2_OFF -> CONFIG_SYS_L2CACHE_OFF > > But the above commit does not clean up all the mx53 board config file. > This patch does it by using CONFIG_SYS_L2CACHE_OFF instead of _L2_OFF > > Signed-off-by: Jason Liu <jason.hui@linaro.org> > Cc:Stefano Babic <sbabic@denx.de> why are you posting a second version of this patch just a few minutes after the first one, and witrhout any indication ifd there are any differences between the two versions, and what these differences might be? Please STOP doing this! Wolfgang Denk
Hi, Stefano, On Tue, Jul 26, 2011 at 5:21 PM, Stefano Babic <sbabic@denx.de> wrote: > On 07/26/2011 10:25 AM, Jason Liu wrote: >> CONFIG_L2_OFF is obsolete after the following commit: >> >> e47f2db5371047eb9bcd115fee084e6a8a92a239 >> armv7: rename cache related CONFIG flags >> Replace the cache related CONFIG flags with more meaningful >> names. Following are the changes: >> CONFIG_L2_OFF -> CONFIG_SYS_L2CACHE_OFF >> >> But the above commit does not clean up all the mx53 board config file. >> This patch does it by using CONFIG_SYS_L2CACHE_OFF instead of _L2_OFF >> > > Hi Jason, > > I agree that CONFIG_L2_OFF is obsolete and must be removed. However, I > do not see any application of CONFIG_SYS_L2CACHE_OFF. > > If I grep into u-boot code, this config is not used at all for i.MX. > Indeed, I can find application for OMAP boards: > > arch/arm/cpu/armv7/omap3/board.c:#ifndef CONFIG_SYS_L2CACHE_OFF > arch/arm/cpu/armv7/omap3/board.c:#ifndef CONFIG_SYS_L2CACHE_OFF > arch/arm/cpu/armv7/s5pc1xx/cache.S:#ifndef CONFIG_SYS_L2CACHE_OFF > arch/arm/cpu/armv7/omap4/board.c:#ifndef CONFIG_SYS_L2CACHE_OFF > > So this setup is usefulness, I think. It's used before, if you checkout tag: v2011.06 #ifndef CONFIG_L2_OFF /* turn off L2 cache */ l2_cache_disable(); /* invalidate L2 cache also */ invalidate_dcache(get_device_type()); #endif i = 0; /* mem barrier to sync up things */ asm("mcr p15, 0, %0, c7, c10, 4": :"r"(i)); #ifndef CONFIG_L2_OFF l2_cache_enable(); #endif I still think we need explicitly disable L2 cache first. If it only omap related, I don't think we need CONFIG_SYS_L2CACHE_OFF for the global u-boot. If you grep the CONFIG_SYS_L2CACHE_OFF under include/configs, you will see a lot of define other than omap platform. > > By the way (and I know it is not the same topic), do you mean maybe > CONFIG_SYS_DCACHE_OFF ? Have you read the thread related to cache > problems on MX51 ? Maybe we have to disable D_CACHE until we find a fix > for the drivers. What do you mean ? Yes, the fec is not working after commit: c2dd0d45540397704de9b13287417d21049d34c6 armv7: integrate cache maintenance support It's due to it enable dcache by default if not define CONFIG_SYS_DCACHE_OFF explicitly. mxc_fec driver need be fixed or re-write to consider cache safe. I agree to disable d-cache first. Jason > > Best regards, > Stefano Babic > > -- > ===================================================================== > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de > ===================================================================== >
Hi, Wolfgang, 2011/7/26 Wolfgang Denk <wd@denx.de>: > Dear Jason Liu, > > In message <1311668750-5471-1-git-send-email-jason.hui@linaro.org> you wrote: >> CONFIG_L2_OFF is obsolete after the following commit: >> >> e47f2db5371047eb9bcd115fee084e6a8a92a239 >> armv7: rename cache related CONFIG flags >> Replace the cache related CONFIG flags with more meaningful >> names. Following are the changes: >> CONFIG_L2_OFF -> CONFIG_SYS_L2CACHE_OFF >> >> But the above commit does not clean up all the mx53 board config file. >> This patch does it by using CONFIG_SYS_L2CACHE_OFF instead of _L2_OFF >> >> Signed-off-by: Jason Liu <jason.hui@linaro.org> >> Cc:Stefano Babic <sbabic@denx.de> > > why are you posting a second version of this patch just a few minutes > after the first one, and witrhout any indication ifd there are any > differences between the two versions, and what these differences might > be? > > Please STOP doing this! It's my fault to send out the second email when I thought the previous email was not show up in mail-list. will take care to avoid it later. Sorry for the noise. Jason > > > Wolfgang Denk > > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de > The faster I go, the behinder I get. -- Lewis Carroll > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
Hi, Stefano, 2011/7/26 Jason Hui <jason.hui@linaro.org>: > Hi, Stefano, > > On Tue, Jul 26, 2011 at 5:21 PM, Stefano Babic <sbabic@denx.de> wrote: >> On 07/26/2011 10:25 AM, Jason Liu wrote: >>> CONFIG_L2_OFF is obsolete after the following commit: >>> >>> e47f2db5371047eb9bcd115fee084e6a8a92a239 >>> armv7: rename cache related CONFIG flags >>> Replace the cache related CONFIG flags with more meaningful >>> names. Following are the changes: >>> CONFIG_L2_OFF -> CONFIG_SYS_L2CACHE_OFF >>> >>> But the above commit does not clean up all the mx53 board config file. >>> This patch does it by using CONFIG_SYS_L2CACHE_OFF instead of _L2_OFF >>> >> >> Hi Jason, >> >> I agree that CONFIG_L2_OFF is obsolete and must be removed. However, I >> do not see any application of CONFIG_SYS_L2CACHE_OFF. >> >> If I grep into u-boot code, this config is not used at all for i.MX. >> Indeed, I can find application for OMAP boards: >> >> arch/arm/cpu/armv7/omap3/board.c:#ifndef CONFIG_SYS_L2CACHE_OFF >> arch/arm/cpu/armv7/omap3/board.c:#ifndef CONFIG_SYS_L2CACHE_OFF >> arch/arm/cpu/armv7/s5pc1xx/cache.S:#ifndef CONFIG_SYS_L2CACHE_OFF >> arch/arm/cpu/armv7/omap4/board.c:#ifndef CONFIG_SYS_L2CACHE_OFF >> >> So this setup is usefulness, I think. > > It's used before, if you checkout tag: v2011.06 > > #ifndef CONFIG_L2_OFF > /* turn off L2 cache */ > l2_cache_disable(); > /* invalidate L2 cache also */ > invalidate_dcache(get_device_type()); > #endif > i = 0; > /* mem barrier to sync up things */ > asm("mcr p15, 0, %0, c7, c10, 4": :"r"(i)); > > #ifndef CONFIG_L2_OFF > l2_cache_enable(); > #endif > > > I still think we need explicitly disable L2 cache first. If it only > omap related, I don't think we need > CONFIG_SYS_L2CACHE_OFF for the global u-boot. If you grep the > CONFIG_SYS_L2CACHE_OFF > under include/configs, you will see a lot of define other than omap platform. > >> >> By the way (and I know it is not the same topic), do you mean maybe >> CONFIG_SYS_DCACHE_OFF ? Have you read the thread related to cache >> problems on MX51 ? Maybe we have to disable D_CACHE until we find a fix >> for the drivers. What do you mean ? > > Yes, the fec is not working after commit: > c2dd0d45540397704de9b13287417d21049d34c6 > armv7: integrate cache maintenance support > > It's due to it enable dcache by default if not define > CONFIG_SYS_DCACHE_OFF explicitly. > mxc_fec driver need be fixed or re-write to consider cache safe. I > agree to disable d-cache first. Do I need submit patch to disable D-CACHE first? And another issue for imx51 is that mmc command does not work correctly sometimes such as saveenv. Do you notice that? Jason > > Jason > >> >> Best regards, >> Stefano Babic >> >> -- >> ===================================================================== >> DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel >> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany >> Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de >> ===================================================================== >> > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
On 07/28/2011 07:56 AM, Jason Liu wrote: > Hi, Stefano, Hi Jason, >> >> It's used before, if you checkout tag: v2011.06 >> >> #ifndef CONFIG_L2_OFF >> /* turn off L2 cache */ >> l2_cache_disable(); >> /* invalidate L2 cache also */ >> invalidate_dcache(get_device_type()); >> #endif >> i = 0; >> /* mem barrier to sync up things */ >> asm("mcr p15, 0, %0, c7, c10, 4": :"r"(i)); >> >> #ifndef CONFIG_L2_OFF >> l2_cache_enable(); >> #endif However, as far as I can understand, L2 cache is disabled until explicitely enabled. >> >> >> I still think we need explicitly disable L2 cache first I am still checking this point. My concern is to understand if in the current code we need to do something or not. If there is no code to enable L2 cache (I have not found), there should be no need to disable it. After a reset, L2 cache is disabled, am I right ? Why do you think we have to explicitely disable it ? Am I missing something ? >>. If it only >> omap related, I don't think we need >> CONFIG_SYS_L2CACHE_OFF for the global u-boot. If you grep the >> CONFIG_SYS_L2CACHE_OFF >> under include/configs, you will see a lot of define other than omap platform. Nevertheless we have to decide if we need it for the MX5. If we manage enable/disable L2 Cache for the MX5, we need it, else not. As I can see in the actual code, we do not manage L2 Cache. >> It's due to it enable dcache by default if not define >> CONFIG_SYS_DCACHE_OFF explicitly. >> mxc_fec driver need be fixed or re-write to consider cache safe. I >> agree to disable d-cache first. > > Do I need submit patch to disable D-CACHE first? let's see if we agree on the following points: - CONFIG_L2_OFF is obsolete, as you pointed out, and must be removed - CONFIG_SYS_L2CACHE_OFF is needed for MX5 if we have code to enable / disable it. Let me know if you agree that L2 cache is disabled after a reset. Then we do not need this define until we explicitely enable it as default, as now for D cache. - because of D Cache issues in the driver, it is better to disable D Cache for MX5 processors. I think, if we agree on these point, we can manage the changes in a single patch (changes are made on the same files, that is the configuration file for the boards), and it is enough to add a useful comment to explain what we do. > And another issue > for imx51 is that > mmc command does not work correctly sometimes such as saveenv. Do you > notice that? I admit I have not tested recently and I have not noted this issue. Last time was after some changes in MMC code. I am sure we get the same issues for D Cache as for the FEC. In fact, for powerpc the fsl_esdhc.c enables cache snooping and does not need to invalidate buffers. We have no counterpart for MX5. Best regards, Stefano Babic
Hi, stefano, 2011/7/28 Stefano Babic <sbabic@denx.de>: > On 07/28/2011 07:56 AM, Jason Liu wrote: >> Hi, Stefano, > > Hi Jason, > >>> >>> It's used before, if you checkout tag: v2011.06 >>> >>> #ifndef CONFIG_L2_OFF >>> /* turn off L2 cache */ >>> l2_cache_disable(); >>> /* invalidate L2 cache also */ >>> invalidate_dcache(get_device_type()); >>> #endif >>> i = 0; >>> /* mem barrier to sync up things */ >>> asm("mcr p15, 0, %0, c7, c10, 4": :"r"(i)); >>> >>> #ifndef CONFIG_L2_OFF >>> l2_cache_enable(); >>> #endif > > However, as far as I can understand, L2 cache is disabled until > explicitely enabled. Look at the meaning from the name X_OFF, if we define it, it tells people that we want to disable it explicitly. If not define it, it underlying tells that we want to enable it, right? So, if we remove this define L2_OFF from board config file, it may cause confuse since we don't want to enable it now. > >>> >>> >>> I still think we need explicitly disable L2 cache first > > I am still checking this point. My concern is to understand if in the > current code we need to do something or not. If there is no code to > enable L2 cache (I have not found), there should be no need to disable > it. After a reset, L2 cache is disabled, am I right ? > Why do you think we have to explicitely disable it ? Am I missing > something ? Currently, MX5 will disable L2 after reset. some points as above. > >>>. If it only >>> omap related, I don't think we need >>> CONFIG_SYS_L2CACHE_OFF for the global u-boot. If you grep the >>> CONFIG_SYS_L2CACHE_OFF >>> under include/configs, you will see a lot of define other than omap platform. > > Nevertheless we have to decide if we need it for the MX5. If we manage > enable/disable L2 Cache for the MX5, we need it, else not. As I can see > in the actual code, we do not manage L2 Cache. Yes, currently, we don't manage L2. But, I don't know whether the common code will be changed in the future, if we don't disable L2 explicitly, it will enable L2 by default just like d-cache. > >>> It's due to it enable dcache by default if not define >>> CONFIG_SYS_DCACHE_OFF explicitly. >>> mxc_fec driver need be fixed or re-write to consider cache safe. I >>> agree to disable d-cache first. >> >> Do I need submit patch to disable D-CACHE first? > > let's see if we agree on the following points: > > - CONFIG_L2_OFF is obsolete, as you pointed out, and must be removed > > - CONFIG_SYS_L2CACHE_OFF is needed for MX5 if we have code to enable / > disable it. Let me know if you agree that L2 cache is disabled after a > reset. Then we do not need this define until we explicitely enable it as > default, as now for D cache. > > - because of D Cache issues in the driver, it is better to disable D > Cache for MX5 processors. > > I think, if we agree on these point, we can manage the changes in a > single patch (changes are made on the same files, that is the > configuration file for the boards), and it is enough to add a useful > comment to explain what we do. OK, after get agreement, I will do it soon with single patch. > >> And another issue >> for imx51 is that >> mmc command does not work correctly sometimes such as saveenv. Do you >> notice that? > > I admit I have not tested recently and I have not noted this issue. Last > time was after some changes in MMC code. > > I am sure we get the same issues for D Cache as for the FEC. In fact, > for powerpc the fsl_esdhc.c enables cache snooping and does not need to > invalidate buffers. We have no counterpart for MX5. Yes, correct. Jason > > Best regards, > Stefano Babic > > -- > ===================================================================== > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de > ===================================================================== >
On 07/28/2011 12:22 PM, Jason Liu wrote: > Hi, stefano, Hi Jason, > Look at the meaning from the name X_OFF, if we define it, it tells > people that we want to disable it explicitly. Yes, but is only dead code for MX5. It has sense only if we add l2_cache_enable() and disable functions. > If not define it, it underlying > tells that we want to enable it, right? No, it says nothing. A lot of boards has not set this CONFIG, but this does not mean that L2 cache is enabled. > So, if we remove this define L2_OFF > from board config file, it may cause confuse since we don't want to > enable it now. IMHO it is confusing if we add it. If we set them into the config file, it means that the L2 Cache is enable simply dropping it, and this is not true, as there is not yet support. Better to add it when we have really support for it. >> I am still checking this point. My concern is to understand if in the >> current code we need to do something or not. If there is no code to >> enable L2 cache (I have not found), there should be no need to disable >> it. After a reset, L2 cache is disabled, am I right ? >> Why do you think we have to explicitely disable it ? Am I missing >> something ? > > Currently, MX5 will disable L2 after reset. some points as above. Ok, we agree > Yes, currently, we don't manage L2. > But, I don't know whether the common code will be changed in the future, > if we don't disable L2 explicitly, it will enable L2 by default just > like d-cache. Well, we will check this issue when it will be needed. We cannot know the future, it coul also be there is support for L2 Cache at that moment for the i.MX5. >> I think, if we agree on these point, we can manage the changes in a >> single patch (changes are made on the same files, that is the >> configuration file for the boards), and it is enough to add a useful >> comment to explain what we do. > > OK, after get agreement, I will do it soon with single patch. Ok, I think we agree how to proceed ;-) >> I am sure we get the same issues for D Cache as for the FEC. In fact, >> for powerpc the fsl_esdhc.c enables cache snooping and does not need to >> invalidate buffers. We have no counterpart for MX5. > > Yes, correct. Best regards, Stefano Babic
diff --git a/include/configs/mx53ard.h b/include/configs/mx53ard.h index c872510..0e323e6 100644 --- a/include/configs/mx53ard.h +++ b/include/configs/mx53ard.h @@ -29,7 +29,7 @@ #define CONFIG_DISPLAY_CPUINFO #define CONFIG_DISPLAY_BOARDINFO -#define CONFIG_L2_OFF +#define CONFIG_SYS_L2CACHE_OFF #include <asm/arch/imx-regs.h> diff --git a/include/configs/mx53loco.h b/include/configs/mx53loco.h index 544e3fb..bc1b401 100644 --- a/include/configs/mx53loco.h +++ b/include/configs/mx53loco.h @@ -30,7 +30,7 @@ #define CONFIG_DISPLAY_CPUINFO #define CONFIG_DISPLAY_BOARDINFO -#define CONFIG_L2_OFF +#define CONFIG_SYS_L2CACHE_OFF #include <asm/arch/imx-regs.h> diff --git a/include/configs/mx53smd.h b/include/configs/mx53smd.h index 65d5e05..194047b 100644 --- a/include/configs/mx53smd.h +++ b/include/configs/mx53smd.h @@ -29,7 +29,7 @@ #define CONFIG_DISPLAY_CPUINFO #define CONFIG_DISPLAY_BOARDINFO -#define CONFIG_L2_OFF +#define CONFIG_SYS_L2CACHE_OFF #include <asm/arch/imx-regs.h>
CONFIG_L2_OFF is obsolete after the following commit: e47f2db5371047eb9bcd115fee084e6a8a92a239 armv7: rename cache related CONFIG flags Replace the cache related CONFIG flags with more meaningful names. Following are the changes: CONFIG_L2_OFF -> CONFIG_SYS_L2CACHE_OFF But the above commit does not clean up all the mx53 board config file. This patch does it by using CONFIG_SYS_L2CACHE_OFF instead of _L2_OFF Signed-off-by: Jason Liu <jason.hui@linaro.org> Cc:Stefano Babic <sbabic@denx.de> --- include/configs/mx53ard.h | 2 +- include/configs/mx53loco.h | 2 +- include/configs/mx53smd.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)