Patchwork [U-Boot] mx53: use CONFIG_SYS_L2CACHE_OFF in config file

login
register
mail settings
Submitter Jason Liu
Date July 26, 2011, 8:25 a.m.
Message ID <1311668750-5471-1-git-send-email-jason.hui@linaro.org>
Download mbox | patch
Permalink /patch/106794/
State Changes Requested
Headers show

Comments

Jason Liu - July 26, 2011, 8:25 a.m.
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(-)
Stefano Babic - July 26, 2011, 9:21 a.m.
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
Wolfgang Denk - July 26, 2011, 9:34 a.m.
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
Jason Liu - July 26, 2011, 9:44 a.m.
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
> =====================================================================
>
Jason Liu - July 27, 2011, 2:36 a.m.
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
>
Jason Liu - July 28, 2011, 5:56 a.m.
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
>
Stefano Babic - July 28, 2011, 6:32 a.m.
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
Jason Liu - July 28, 2011, 10:22 a.m.
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
> =====================================================================
>
Stefano Babic - July 28, 2011, 10:40 a.m.
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

Patch

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>