[U-Boot] [PATCH RESEND v2] armv7: Disable D cache for goni target (s5p)

[U-Boot] [PATCH RESEND v2] armv7: Disable D cache for goni target (s5p)

[Lukasz Majewski]

Disable D cache for goni target (s5p) - rationale:

1. Before change introduced in commit
SHA1:c2dd0d45540397704de9b13287417d21049d34c6 the D-cache for
GONI target was disabled. This change was supposed to
preserve this state.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Minkyu Kang <mk7.kang@samsung.com>
Cc: Aneesh V <aneesh@ti.com>
Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
---
Changes for v2:
	- Rationale for disabling D cache

 include/configs/s5p_goni.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/include/configs/s5p_goni.h b/include/configs/s5p_goni.h
index d648ce8..010428b 100644
--- a/include/configs/s5p_goni.h
+++ b/include/configs/s5p_goni.h
@@ -74,6 +74,9 @@
 /* It should define before config_cmd_default.h */
 #define CONFIG_SYS_NO_FLASH		1
 
+/* Cache D configuration */
+#define CONFIG_SYS_DCACHE_OFF
+
 /* Command definition */
 #include <config_cmd_default.h>
 
-- 
1.7.2.3

[U-Boot] [PATCH RESEND v2] armv7: Disable D cache for goni target (s5p)

[Linus Walleij]

On Mon, Jul 18, 2011 at 9:23 AM, Lukasz Majewski <l.majewski@samsung.com> wrote:

> Disable D cache for goni target (s5p) - rationale:
>
> 1. Before change introduced in commit
> SHA1:c2dd0d45540397704de9b13287417d21049d34c6 the D-cache for
> GONI target was disabled. This change was supposed to
> preserve this state.

I had exactly the same rationale for disabling cache on the Integrator - it
used to be off now it's default on and doesn't work - but that patch was
NAK:ed by Wolfgang.

BTW: what CPU is there in the S5P?

Linus Walleij

[U-Boot] [PATCH RESEND v2] armv7: Disable D cache for goni target (s5p)

[Minkyu Kang]

Dear Linus Walleij,

On 18 July 2011 19:35, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Jul 18, 2011 at 9:23 AM, Lukasz Majewski <l.majewski@samsung.com> wrote:
>
>> Disable D cache for goni target (s5p) - rationale:
>>
>> 1. Before change introduced in commit
>> SHA1:c2dd0d45540397704de9b13287417d21049d34c6 the D-cache for
>> GONI target was disabled. This change was supposed to
>> preserve this state.
>
> I had exactly the same rationale for disabling cache on the Integrator - it
> used to be off now it's default on and doesn't work - but that patch was
> NAK:ed by Wolfgang.
>

Could you please give me the link of that thread?

Thanks
Minkyu Kang
-- 
from. prom.
www.promsoft.net

[U-Boot] [PATCH RESEND v2] armv7: Disable D cache for goni target (s5p)

[Linus Walleij]

On Thu, Jul 28, 2011 at 7:04 AM, Minkyu Kang <promsoft@gmail.com> wrote:
> Dear Linus Walleij,
>
> On 18 July 2011 19:35, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Mon, Jul 18, 2011 at 9:23 AM, Lukasz Majewski <l.majewski@samsung.com> wrote:
>>
>>> Disable D cache for goni target (s5p) - rationale:
>>>
>>> 1. Before change introduced in commit
>>> SHA1:c2dd0d45540397704de9b13287417d21049d34c6 the D-cache for
>>> GONI target was disabled. This change was supposed to
>>> preserve this state.
>>
>> I had exactly the same rationale for disabling cache on the Integrator - it
>> used to be off now it's default on and doesn't work - but that patch was
>> NAK:ed by Wolfgang.
>>
>
> Could you please give me the link of that thread?

Me:
http://marc.info/?l=u-boot&m=131066804010454&w=2

Wolfgang:
http://marc.info/?l=u-boot&m=131066501332302&w=2

AFAICT the idea is that it should be possible to turn D-cache
off, we have to fix it if it doesn't work. Alternatively I have to
prove that the D-cache on the ARM920T cannot be safely
turned off at all, i.e. if it requires that the CP15 operation be
done from an onchip memory or something like that.

Anyone knows, by the way? I've really tried hard to fix this
and so far gotten nowhere.

Thanks,
Linus Walleij

[U-Boot] [PATCH RESEND v2] armv7: Disable D cache for goni target (s5p)

[Chander Kashyap]

Dear Minkyu,
I am not able to retain environment saved on mmc card using setenv cmd
when dcahe is enabled.
By disabling it it works fine.

On 28 July 2011 10:34, Minkyu Kang <promsoft@gmail.com> wrote:
> Dear Linus Walleij,
>
> On 18 July 2011 19:35, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Mon, Jul 18, 2011 at 9:23 AM, Lukasz Majewski <l.majewski@samsung.com> wrote:
>>
>>> Disable D cache for goni target (s5p) - rationale:
>>>
>>> 1. Before change introduced in commit
>>> SHA1:c2dd0d45540397704de9b13287417d21049d34c6 the D-cache for
>>> GONI target was disabled. This change was supposed to
>>> preserve this state.
>>
>> I had exactly the same rationale for disabling cache on the Integrator - it
>> used to be off now it's default on and doesn't work - but that patch was
>> NAK:ed by Wolfgang.
>>
>
> Could you please give me the link of that thread?
>
> Thanks
> Minkyu Kang
> --
> from. prom.
> www.promsoft.net
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>



-- 
with warm regards,
Chander Kashyap

[U-Boot] [PATCH RESEND v2] armv7: Disable D cache for goni target (s5p)

[Aneesh V]

Hi Linus,

On Friday 29 July 2011 04:04 AM, Linus Walleij wrote:
> On Thu, Jul 28, 2011 at 7:04 AM, Minkyu Kang<promsoft@gmail.com>  wrote:
>> Dear Linus Walleij,
>>
>> On 18 July 2011 19:35, Linus Walleij<linus.walleij@linaro.org>  wrote:
>>> On Mon, Jul 18, 2011 at 9:23 AM, Lukasz Majewski<l.majewski@samsung.com>  wrote:
>>>
>>>> Disable D cache for goni target (s5p) - rationale:
>>>>
>>>> 1. Before change introduced in commit
>>>> SHA1:c2dd0d45540397704de9b13287417d21049d34c6 the D-cache for
>>>> GONI target was disabled. This change was supposed to
>>>> preserve this state.
>>>
>>> I had exactly the same rationale for disabling cache on the Integrator - it
>>> used to be off now it's default on and doesn't work - but that patch was
>>> NAK:ed by Wolfgang.
>>>
>>
>> Could you please give me the link of that thread?
>
> Me:
> http://marc.info/?l=u-boot&m=131066804010454&w=2

I hadn't noticed this thread before. Here are my thoughts on your
problem:

I think the fundamental problem is that caching was enabled for ARM
without putting the cache maintenance functions in place. At a minimum,
we need a few flush and invalidate functions. I worked on this for
armv7, but similar things need to be done for earlier architectures if
you have to enable caching. Once we have these cache maintenance
functions, they need to be used in the following scenarios to avoid
coherency issues.

1. Drivers using DMA:
i. Flush the buffer before initiating a memory to peripheral DMA.
ii. Invalidate the buffer after a peripheral to memory DMA (before
MPU reads it)

2. Cleanup before Linux:
Flush the entire SDRAM and disable the cache.

I think you are hit by not doing 2 properly. If you have dirty entries
in the cache when Linux boots you will have coherency issues. I had
faced similar issues when I had some issues in armv7
cleanup_before_linux(). I had to do an extra invalidate to solve it.
Please see how this has been done for armv7 in arch/arm/cpu/armv7/cpu.c
in function cleanup_before_linux().

In some other cases there are problems due to (1) above. In any case,
the starting point really is to make sure that you have the required
set of maintenance functions for your architecture revision. We may not
need maintenance functions for each architecture revision depending on
the backward compatibility of the successive revisions. I have not
looked into those details though.

best regards,
Aneesh

[U-Boot] [PATCH RESEND v2] armv7: Disable D cache for goni target (s5p)

[Chander Kashyap]

Hi Aneesh,

On 29 July 2011 15:43, Aneesh V <aneesh@ti.com> wrote:
> Hi Linus,
>
> On Friday 29 July 2011 04:04 AM, Linus Walleij wrote:
>> On Thu, Jul 28, 2011 at 7:04 AM, Minkyu Kang<promsoft@gmail.com> ?wrote:
>>> Dear Linus Walleij,
>>>
>>> On 18 July 2011 19:35, Linus Walleij<linus.walleij@linaro.org> ?wrote:
>>>> On Mon, Jul 18, 2011 at 9:23 AM, Lukasz Majewski<l.majewski@samsung.com> ?wrote:
>>>>
>>>>> Disable D cache for goni target (s5p) - rationale:
>>>>>
>>>>> 1. Before change introduced in commit
>>>>> SHA1:c2dd0d45540397704de9b13287417d21049d34c6 the D-cache for
>>>>> GONI target was disabled. This change was supposed to
>>>>> preserve this state.
>>>>
>>>> I had exactly the same rationale for disabling cache on the Integrator - it
>>>> used to be off now it's default on and doesn't work - but that patch was
>>>> NAK:ed by Wolfgang.
>>>>
>>>
>>> Could you please give me the link of that thread?
>>
>> Me:
>> http://marc.info/?l=u-boot&m=131066804010454&w=2
>
> I hadn't noticed this thread before. Here are my thoughts on your
> problem:
>
> I think the fundamental problem is that caching was enabled for ARM
> without putting the cache maintenance functions in place. At a minimum,
> we need a few flush and invalidate functions. I worked on this for
> armv7,
In my case i am using armv7 based board.
Still flushing is not happening. If i am resetting the board after
issuing saveenv command, environment is not retained.
IMHO in armv7 still more handling need to be done. I am using mmc as boot media.
>but similar things need to be done for earlier architectures if
> you have to enable caching. Once we have these cache maintenance
> functions, they need to be used in the following scenarios to avoid
> coherency issues.
>
> 1. Drivers using DMA:
> i. Flush the buffer before initiating a memory to peripheral DMA.
> ii. Invalidate the buffer after a peripheral to memory DMA (before
> MPU reads it)
>
> 2. Cleanup before Linux:
> Flush the entire SDRAM and disable the cache.
>
> I think you are hit by not doing 2 properly. If you have dirty entries
> in the cache when Linux boots you will have coherency issues. I had
> faced similar issues when I had some issues in armv7
> cleanup_before_linux(). I had to do an extra invalidate to solve it.
> Please see how this has been done for armv7 in arch/arm/cpu/armv7/cpu.c
> in function cleanup_before_linux().
>
> In some other cases there are problems due to (1) above. In any case,
> the starting point really is to make sure that you have the required
> set of maintenance functions for your architecture revision. We may not
> need maintenance functions for each architecture revision depending on
> the backward compatibility of the successive revisions. I have not
> looked into those details though.
>
> best regards,
> Aneesh
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>



-- 
with warm regards,
Chander Kashyap

[U-Boot] [PATCH RESEND v2] armv7: Disable D cache for goni target (s5p)

[Aneesh V]

Hi Chander,

On Friday 29 July 2011 04:01 PM, Chander Kashyap wrote:
> Hi Aneesh,
>
> On 29 July 2011 15:43, Aneesh V<aneesh@ti.com>  wrote:
>> Hi Linus,
>>
>> On Friday 29 July 2011 04:04 AM, Linus Walleij wrote:
>>> On Thu, Jul 28, 2011 at 7:04 AM, Minkyu Kang<promsoft@gmail.com>    wrote:
>>>> Dear Linus Walleij,
>>>>
>>>> On 18 July 2011 19:35, Linus Walleij<linus.walleij@linaro.org>    wrote:
>>>>> On Mon, Jul 18, 2011 at 9:23 AM, Lukasz Majewski<l.majewski@samsung.com>    wrote:
>>>>>
>>>>>> Disable D cache for goni target (s5p) - rationale:
>>>>>>
>>>>>> 1. Before change introduced in commit
>>>>>> SHA1:c2dd0d45540397704de9b13287417d21049d34c6 the D-cache for
>>>>>> GONI target was disabled. This change was supposed to
>>>>>> preserve this state.
>>>>>
>>>>> I had exactly the same rationale for disabling cache on the Integrator - it
>>>>> used to be off now it's default on and doesn't work - but that patch was
>>>>> NAK:ed by Wolfgang.
>>>>>
>>>>
>>>> Could you please give me the link of that thread?
>>>
>>> Me:
>>> http://marc.info/?l=u-boot&m=131066804010454&w=2
>>
>> I hadn't noticed this thread before. Here are my thoughts on your
>> problem:
>>
>> I think the fundamental problem is that caching was enabled for ARM
>> without putting the cache maintenance functions in place. At a minimum,
>> we need a few flush and invalidate functions. I worked on this for
>> armv7,
> In my case i am using armv7 based board.
> Still flushing is not happening. If i am resetting the board after
> issuing saveenv command, environment is not retained.
> IMHO in armv7 still more handling need to be done. I am using mmc as boot media.

Please note that what I have provided is a means to do the flushing,
the APIs, and also I have taken care of the cleanup_beofre_linux() for
armv7. You still need to do some work to fix the issues in category (1)
below for your platform. For instance, does your MMC driver use DMA? If
yes, you may have to look into it.

best regards,
Aneesh

[U-Boot] [PATCH RESEND v2] armv7: Disable D cache for goni target (s5p)

[Linus Walleij]

Hi Aneesh, thanks for taking time for this!

On Fri, Jul 29, 2011 at 12:13 PM, Aneesh V <aneesh@ti.com> wrote:

> 1. Drivers using DMA:
> i. Flush the buffer before initiating a memory to peripheral DMA.
> ii. Invalidate the buffer after a peripheral to memory DMA (before
> MPU reads it)

No problems with this. I only use the serial port and it's working
flawlessly downloading a uImage to target with caches enabled.

> 2. Cleanup before Linux:
> Flush the entire SDRAM and disable the cache.
>
> I think you are hit by not doing 2 properly. If you have dirty entries
> in the cache when Linux boots you will have coherency issues. I had
> faced similar issues when I had some issues in armv7
> cleanup_before_linux(). I had to do an extra invalidate to solve it.
> Please see how this has been done for armv7 in arch/arm/cpu/armv7/cpu.c
> in function cleanup_before_linux().

Yes, this is the problem. The problem is further that I've tried
several variants of code and of course following the ARM920T manual
on how to do this.

Actually I think the problem is really not with the cache *at all* but
with the MMU. The current ARM CP15 MMU code sets up an
identity mapping and it is always turned on/off in conjunction with
the cache.

The moste pedantic and careful code I have does this:

+#ifdef CONFIG_ARM920T
+       /* Flush all cache */
+       debug("FLUSH I-cache\n");
+       asm volatile("mcr p15, 0, %0, c7, c5, 0" : : "r" (0));
+       isb();
+       debug("FLUSH D-cache\n");
+       asm volatile("mcr p15, 0, %0, c7, c6, 0" : : "r" (0));
+       isb();
+       debug("FLUSH all cache\n");
+       asm volatile("mcr p15, 0, %0, c7, c7, 0" : : "r" (0));
+       isb();
+       /* Flush I+D TLB */
+       debug("FLUSH I+D TLB\n");
+       asm volatile("mcr p15, 0, %0, c8, c7, 0" : : "r" (0));
+       isb();
+       /* Drain write buffer */
+       debug("Drain write buffer\n");
+       asm volatile("mcr p15, 0, %0, c7, c10, 4" : : "r" (0));
+       isb();
+#endif

This is called in arch/arm/lib/cache.c generic CP15 code as it
calls __flush_cache before it turns off the MMU.

At this point the program counter goes astray and the machine
hangs :-(

It's something like removing the identity map is not good for the
CPU, it gets lost. I think. (Working hopothesis.)

I think actually all ARM920T machines are broken, they have just
not been tested yet.

> In some other cases there are problems due to (1) above. In any case,
> the starting point really is to make sure that you have the required
> set of maintenance functions for your architecture revision.

For the old CP15 caches it's quite simple, as can be seen from
Linux MM code you just invalidate all the cache:
asm volatile("mcr p15, 0, %0, c7, c7, 0" : : "r" (0));

Yours,
Linus Walleij

[U-Boot] [PATCH RESEND v2] armv7: Disable D cache for goni target (s5p)

[Aneesh V]

Hi Linus,

On Sunday 31 July 2011 01:48 PM, Linus Walleij wrote:
> Hi Aneesh, thanks for taking time for this!
>
> On Fri, Jul 29, 2011 at 12:13 PM, Aneesh V<aneesh@ti.com>  wrote:
>
>> 1. Drivers using DMA:
>> i. Flush the buffer before initiating a memory to peripheral DMA.
>> ii. Invalidate the buffer after a peripheral to memory DMA (before
>> MPU reads it)
>
> No problems with this. I only use the serial port and it's working
> flawlessly downloading a uImage to target with caches enabled.
>
>> 2. Cleanup before Linux:
>> Flush the entire SDRAM and disable the cache.
>>
>> I think you are hit by not doing 2 properly. If you have dirty entries
>> in the cache when Linux boots you will have coherency issues. I had
>> faced similar issues when I had some issues in armv7
>> cleanup_before_linux(). I had to do an extra invalidate to solve it.
>> Please see how this has been done for armv7 in arch/arm/cpu/armv7/cpu.c
>> in function cleanup_before_linux().
>
> Yes, this is the problem. The problem is further that I've tried
> several variants of code and of course following the ARM920T manual
> on how to do this.
>
> Actually I think the problem is really not with the cache *at all* but
> with the MMU. The current ARM CP15 MMU code sets up an
> identity mapping and it is always turned on/off in conjunction with
> the cache.
>
> The moste pedantic and careful code I have does this:
>
> +#ifdef CONFIG_ARM920T
> +       /* Flush all cache */
> +       debug("FLUSH I-cache\n");
> +       asm volatile("mcr p15, 0, %0, c7, c5, 0" : : "r" (0));
> +       isb();
> +       debug("FLUSH D-cache\n");
> +       asm volatile("mcr p15, 0, %0, c7, c6, 0" : : "r" (0));
> +       isb();
> +       debug("FLUSH all cache\n");
> +       asm volatile("mcr p15, 0, %0, c7, c7, 0" : : "r" (0));
> +       isb();
> +       /* Flush I+D TLB */
> +       debug("FLUSH I+D TLB\n");
> +       asm volatile("mcr p15, 0, %0, c8, c7, 0" : : "r" (0));
> +       isb();
> +       /* Drain write buffer */
> +       debug("Drain write buffer\n");
> +       asm volatile("mcr p15, 0, %0, c7, c10, 4" : : "r" (0));
> +       isb();
> +#endif
>
> This is called in arch/arm/lib/cache.c generic CP15 code as it
> calls __flush_cache before it turns off the MMU.
>
> At this point the program counter goes astray and the machine
> hangs :-(

Is it hanging when you return from the function? What is the order in
which you are disabling the cache, flushing it etc. This order is
important. I had faced some issues initially when I developed the code
for armv7. Please see a discussion about those issues in this thread:

http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/91332/focus=91965

>
> It's something like removing the identity map is not good for the
> CPU, it gets lost. I think. (Working hopothesis.)

I am not sure if identity mapping is the issue for you. At least, it's
not causing any trouble on armv7.

PC going astray may be due to restoring wrong r14, which in turn, could
be due to a cache-coherency problem.

best regards,
Aneesh