[U-Boot] [PATCH 1/2] Flush the date cache before disabling it.

[U-Boot] [PATCH 1/2] Flush the date cache before disabling it.

[Sughosh Ganu]

The current implementation invalidates the cache instead of flushing
it. This causes problems on platforms where the spl/u-boot is already
loaded to the RAM, with caches enabled by a first stage bootloader.

The V bit of the cp15's control register c1 is set to the value of
VINITHI on reset. Do not clear this bit by default, as there are SOC's
with no valid memory region at 0x0.

Signed-off-by: Sughosh Ganu <urwithsughosh@gmail.com>
Cc: Albert Aribaud <albert.u.boot@aribaud.net>
---
 arch/arm/cpu/arm926ejs/start.S |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S
index 6a09c02..112f708 100644
--- a/arch/arm/cpu/arm926ejs/start.S
+++ b/arch/arm/cpu/arm926ejs/start.S
@@ -355,17 +355,21 @@ _dynsym_start_ofs:
  */
 cpu_init_crit:
 	/*
-	 * flush v4 I/D caches
+	 * flush D cache before disabling it
 	 */
 	mov	r0, #0
-	mcr	p15, 0, r0, c7, c7, 0	/* flush v3/v4 cache */
+flush_dcache:
+	mrc	p15, 0, r15, c7, c10, 3
+	bne	flush_dcache
+
+	mov	r0, #0
 	mcr	p15, 0, r0, c8, c7, 0	/* flush v4 TLB */
 
 	/*
 	 * disable MMU stuff and caches
 	 */
 	mrc	p15, 0, r0, c1, c0, 0
-	bic	r0, r0, #0x00002300	/* clear bits 13, 9:8 (--V- --RS) */
+	bic	r0, r0, #0x00000300	/* clear bits 13, 9:8 ( --RS) */
 	bic	r0, r0, #0x00000087	/* clear bits 7, 2:0 (B--- -CAM) */
 	orr	r0, r0, #0x00000002	/* set bit 2 (A) Align */
 	orr	r0, r0, #0x00001000	/* set bit 12 (I) I-Cache */
-- 
1.7.5.4

[U-Boot] [PATCH 1/2] Flush the date cache before disabling it.

[Mike Frysinger]

On Monday 09 January 2012 13:25:50 Sughosh Ganu wrote:
>  arch/arm/cpu/arm926ejs/start.S |   10 +++++++---

your patch summary should include a relevant prefix.  something like "arm: " or 
"arm926: ".  that way non-arm people can ignore this.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120109/bd49e0a8/attachment.pgp>

[U-Boot] [PATCH 1/2] Flush the date cache before disabling it.

[Sughosh Ganu]

On Mon Jan 09, 2012 at 01:41:58PM -0500, Mike Frysinger wrote:
> On Monday 09 January 2012 13:25:50 Sughosh Ganu wrote:
> >  arch/arm/cpu/arm926ejs/start.S |   10 +++++++---
> 
> your patch summary should include a relevant prefix.  something like "arm: " or 
> "arm926: ".  that way non-arm people can ignore this.
> -mike

  Yep, realised that after i sent the patch. Sorry about that.

-sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Sughosh Ganu]

The current implementation invalidates the cache instead of flushing
it. This causes problems on platforms where the spl/u-boot is already
loaded to the RAM, with caches enabled by a first stage bootloader.

The V bit of the cp15's control register c1 is set to the value of
VINITHI on reset. Do not clear this bit by default, as there are SOC's
with no valid memory region at 0x0.

Signed-off-by: Sughosh Ganu <urwithsughosh@gmail.com>
Cc: Albert Aribaud <albert.u.boot@aribaud.net>
---

Changes since V1
* Added arm926 keyword to the subject line
* Removed the superfluous setting of r0.
* Fixed the comment to reflect the fact that V is not being cleared

 arch/arm/cpu/arm926ejs/start.S |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S
index 6a09c02..6e261c2 100644
--- a/arch/arm/cpu/arm926ejs/start.S
+++ b/arch/arm/cpu/arm926ejs/start.S
@@ -355,17 +355,20 @@ _dynsym_start_ofs:
  */
 cpu_init_crit:
 	/*
-	 * flush v4 I/D caches
+	 * flush D cache before disabling it
 	 */
 	mov	r0, #0
-	mcr	p15, 0, r0, c7, c7, 0	/* flush v3/v4 cache */
+flush_dcache:
+	mrc	p15, 0, r15, c7, c10, 3
+	bne	flush_dcache
+
 	mcr	p15, 0, r0, c8, c7, 0	/* flush v4 TLB */
 
 	/*
 	 * disable MMU stuff and caches
 	 */
 	mrc	p15, 0, r0, c1, c0, 0
-	bic	r0, r0, #0x00002300	/* clear bits 13, 9:8 (--V- --RS) */
+	bic	r0, r0, #0x00000300	/* clear bits 9:8 ( --RS) */
 	bic	r0, r0, #0x00000087	/* clear bits 7, 2:0 (B--- -CAM) */
 	orr	r0, r0, #0x00000002	/* set bit 2 (A) Align */
 	orr	r0, r0, #0x00001000	/* set bit 12 (I) I-Cache */
-- 
1.7.5.4

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Marek Vasut]

> The current implementation invalidates the cache instead of flushing
> it. This causes problems on platforms where the spl/u-boot is already
> loaded to the RAM, with caches enabled by a first stage bootloader.

What platforms are affected?

M
> 
> The V bit of the cp15's control register c1 is set to the value of
> VINITHI on reset. Do not clear this bit by default, as there are SOC's
> with no valid memory region at 0x0.
> 
> Signed-off-by: Sughosh Ganu <urwithsughosh@gmail.com>
> Cc: Albert Aribaud <albert.u.boot@aribaud.net>
> ---
> 
> Changes since V1
> * Added arm926 keyword to the subject line
> * Removed the superfluous setting of r0.
> * Fixed the comment to reflect the fact that V is not being cleared
> 
>  arch/arm/cpu/arm926ejs/start.S |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/cpu/arm926ejs/start.S
> b/arch/arm/cpu/arm926ejs/start.S index 6a09c02..6e261c2 100644
> --- a/arch/arm/cpu/arm926ejs/start.S
> +++ b/arch/arm/cpu/arm926ejs/start.S
> @@ -355,17 +355,20 @@ _dynsym_start_ofs:
>   */
>  cpu_init_crit:
>  	/*
> -	 * flush v4 I/D caches
> +	 * flush D cache before disabling it
>  	 */
>  	mov	r0, #0
> -	mcr	p15, 0, r0, c7, c7, 0	/* flush v3/v4 cache */
> +flush_dcache:
> +	mrc	p15, 0, r15, c7, c10, 3
> +	bne	flush_dcache
> +
>  	mcr	p15, 0, r0, c8, c7, 0	/* flush v4 TLB */
> 
>  	/*
>  	 * disable MMU stuff and caches
>  	 */
>  	mrc	p15, 0, r0, c1, c0, 0
> -	bic	r0, r0, #0x00002300	/* clear bits 13, 9:8 (--V- --RS) */
> +	bic	r0, r0, #0x00000300	/* clear bits 9:8 ( --RS) */
>  	bic	r0, r0, #0x00000087	/* clear bits 7, 2:0 (B--- -CAM) */
>  	orr	r0, r0, #0x00000002	/* set bit 2 (A) Align */
>  	orr	r0, r0, #0x00001000	/* set bit 12 (I) I-Cache */

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Sughosh Ganu]

On Tue Jan 10, 2012 at 09:07:58PM +0100, Marek Vasut wrote:
> > The current implementation invalidates the cache instead of flushing
> > it. This causes problems on platforms where the spl/u-boot is already
> > loaded to the RAM, with caches enabled by a first stage bootloader.
> 
> What platforms are affected?

  It is causing a problem on the hawkboard, where the spl is loaded
  directly to the RAM by a rom bootloader. We did not see this earlier
  since cpu_init_crit was not getting called due to
  CONFIG_SKIP_LOWLEVEL_INIT.

-sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Marek Vasut]

> On Tue Jan 10, 2012 at 09:07:58PM +0100, Marek Vasut wrote:
> > > The current implementation invalidates the cache instead of flushing
> > > it. This causes problems on platforms where the spl/u-boot is already
> > > loaded to the RAM, with caches enabled by a first stage bootloader.
> > 
> > What platforms are affected?
> 
>   It is causing a problem on the hawkboard, where the spl is loaded
>   directly to the RAM by a rom bootloader. We did not see this earlier
>   since cpu_init_crit was not getting called due to
>   CONFIG_SKIP_LOWLEVEL_INIT.
> 
> -sughosh

I see ... why don't you directly load U-Boot to DRAM then ?

M

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Sughosh Ganu]

On Wed Jan 11, 2012 at 11:47:27AM +0100, Marek Vasut wrote:
> > On Tue Jan 10, 2012 at 09:07:58PM +0100, Marek Vasut wrote:
> > > > The current implementation invalidates the cache instead of flushing
> > > > it. This causes problems on platforms where the spl/u-boot is already
> > > > loaded to the RAM, with caches enabled by a first stage bootloader.
> > > 
> > > What platforms are affected?
> > 
> >   It is causing a problem on the hawkboard, where the spl is loaded
> >   directly to the RAM by a rom bootloader. We did not see this earlier
> >   since cpu_init_crit was not getting called due to
> >   CONFIG_SKIP_LOWLEVEL_INIT.
> > 
> > -sughosh
> 
> I see ... why don't you directly load U-Boot to DRAM then ?

  The rom bootloader(rbl) uses a different ecc layout from the one
  used by the davinci nand driver(u-boot and linux). Using rbl to load
  the u-boot to dram would mean that i have to use the TI's external
  flashing utility every time i upgrade u-boot. Also, i would not be
  able to flash u-boot from the kernel.

-sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Marek Vasut]

> On Wed Jan 11, 2012 at 11:47:27AM +0100, Marek Vasut wrote:
> > > On Tue Jan 10, 2012 at 09:07:58PM +0100, Marek Vasut wrote:
> > > > > The current implementation invalidates the cache instead of
> > > > > flushing it. This causes problems on platforms where the
> > > > > spl/u-boot is already loaded to the RAM, with caches enabled by a
> > > > > first stage bootloader.
> > > > 
> > > > What platforms are affected?
> > > > 
> > >   It is causing a problem on the hawkboard, where the spl is loaded
> > >   directly to the RAM by a rom bootloader. We did not see this earlier
> > >   since cpu_init_crit was not getting called due to
> > >   CONFIG_SKIP_LOWLEVEL_INIT.
> > > 
> > > -sughosh
> > 
> > I see ... why don't you directly load U-Boot to DRAM then ?
> 
>   The rom bootloader(rbl) uses a different ecc layout from the one
>   used by the davinci nand driver(u-boot and linux).

Can't you just tell the driver to use the correct ecc layout when flashing then?

>   Using rbl to load
>   the u-boot to dram would mean that i have to use the TI's external
>   flashing utility every time i upgrade u-boot.

Not really, just adjust the NAND driver to handle this special case.

>   Also, i would not be
>   able to flash u-boot from the kernel.

I think it's possible though, right?

M
> 
> -sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Sughosh Ganu]

On Wed Jan 11, 2012 at 01:42:38PM +0100, Marek Vasut wrote:
> > On Wed Jan 11, 2012 at 11:47:27AM +0100, Marek Vasut wrote:
> > > > On Tue Jan 10, 2012 at 09:07:58PM +0100, Marek Vasut wrote:
> > > > > > The current implementation invalidates the cache instead of
> > > > > > flushing it. This causes problems on platforms where the
> > > > > > spl/u-boot is already loaded to the RAM, with caches enabled by a
> > > > > > first stage bootloader.
> > > > > 
> > > > > What platforms are affected?
> > > > > 
> > > >   It is causing a problem on the hawkboard, where the spl is loaded
> > > >   directly to the RAM by a rom bootloader. We did not see this earlier
> > > >   since cpu_init_crit was not getting called due to
> > > >   CONFIG_SKIP_LOWLEVEL_INIT.
> > > > 
> > > > -sughosh
> > > 
> > > I see ... why don't you directly load U-Boot to DRAM then ?
> > 
> >   The rom bootloader(rbl) uses a different ecc layout from the one
> >   used by the davinci nand driver(u-boot and linux).
> 
> Can't you just tell the driver to use the correct ecc layout when
> flashing then?

  Changing the ecc layout for a single board, hmm not sure. Using a
  spl instead does me no harm whatsoever -- I don't need to update the
  spl frequently in any case, and then can use the nand driver as is.

-sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Marek Vasut]

> On Wed Jan 11, 2012 at 01:42:38PM +0100, Marek Vasut wrote:
> > > On Wed Jan 11, 2012 at 11:47:27AM +0100, Marek Vasut wrote:
> > > > > On Tue Jan 10, 2012 at 09:07:58PM +0100, Marek Vasut wrote:
> > > > > > > The current implementation invalidates the cache instead of
> > > > > > > flushing it. This causes problems on platforms where the
> > > > > > > spl/u-boot is already loaded to the RAM, with caches enabled by
> > > > > > > a first stage bootloader.
> > > > > > 
> > > > > > What platforms are affected?
> > > > > > 
> > > > >   It is causing a problem on the hawkboard, where the spl is loaded
> > > > >   directly to the RAM by a rom bootloader. We did not see this
> > > > >   earlier since cpu_init_crit was not getting called due to
> > > > >   CONFIG_SKIP_LOWLEVEL_INIT.
> > > > > 
> > > > > -sughosh
> > > > 
> > > > I see ... why don't you directly load U-Boot to DRAM then ?
> > > > 
> > >   The rom bootloader(rbl) uses a different ecc layout from the one
> > >   used by the davinci nand driver(u-boot and linux).
> > 
> > Can't you just tell the driver to use the correct ecc layout when
> > flashing then?
> 
>   Changing the ecc layout for a single board, hmm not sure. Using a
>   spl instead does me no harm whatsoever -- I don't need to update the
>   spl frequently in any case, and then can use the nand driver as is.

And how do you replace the SPL?

Either way, I think the correct approach is to allow the driver to handle the 
SPL update too.

M
> 
> -sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Marek Vasut]

> > On Wed Jan 11, 2012 at 01:42:38PM +0100, Marek Vasut wrote:
> > > > On Wed Jan 11, 2012 at 11:47:27AM +0100, Marek Vasut wrote:
> > > > > > On Tue Jan 10, 2012 at 09:07:58PM +0100, Marek Vasut wrote:
> > > > > > > > The current implementation invalidates the cache instead of
> > > > > > > > flushing it. This causes problems on platforms where the
> > > > > > > > spl/u-boot is already loaded to the RAM, with caches enabled
> > > > > > > > by a first stage bootloader.
> > > > > > > 
> > > > > > > What platforms are affected?
> > > > > > > 
> > > > > >   It is causing a problem on the hawkboard, where the spl is
> > > > > >   loaded directly to the RAM by a rom bootloader. We did not see
> > > > > >   this earlier since cpu_init_crit was not getting called due to
> > > > > >   CONFIG_SKIP_LOWLEVEL_INIT.
> > > > > > 
> > > > > > -sughosh
> > > > > 
> > > > > I see ... why don't you directly load U-Boot to DRAM then ?
> > > > > 
> > > >   The rom bootloader(rbl) uses a different ecc layout from the one
> > > >   used by the davinci nand driver(u-boot and linux).
> > > 
> > > Can't you just tell the driver to use the correct ecc layout when
> > > flashing then?
> > > 
> >   Changing the ecc layout for a single board, hmm not sure. Using a
> >   spl instead does me no harm whatsoever -- I don't need to update the
> >   spl frequently in any case, and then can use the nand driver as is.
> 
> And how do you replace the SPL?
> 
> Either way, I think the correct approach is to allow the driver to handle
> the SPL update too.

Or rather -- to be able to load u-boot directly. It seems to be more correct 
solution. The SPL you're using is just a workaround and also you need to TI 
flasher to replace it, right ?
> 
> M
> 
> > -sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Sughosh Ganu]

On Wed Jan 11, 2012 at 02:52:44PM +0100, Marek Vasut wrote:

> > >   Changing the ecc layout for a single board, hmm not sure. Using a
> > >   spl instead does me no harm whatsoever -- I don't need to update the
> > >   spl frequently in any case, and then can use the nand driver as is.
> > 
> > And how do you replace the SPL?
> > 
> > Either way, I think the correct approach is to allow the driver to handle
> > the SPL update too.
> 
> Or rather -- to be able to load u-boot directly. It seems to be more correct 
> solution. The SPL you're using is just a workaround and also you need to TI 
> flasher to replace it, right ?

  Yes, using the spl is a workaround, and like i mentioned in my
  previous mail, i thought having the spl was not a problem on the
  hawkboard. I had thought putting a new oob layout in the common
  davinci nand driver for a single board to be not a clean fix
  either. More so, given the fact that we don't have any control over
  rbl -- so if rbl changes it's layout for any subsequent board, we'd
  have to add that as well to the nand driver, and both in u-boot as
  well as the kernel.

  I guess the cleanest solution would have been for the rbl to have
  used the same layout as the one used by u-boot and linux.

  Btw, can you please review this change, that this patch
  fixes. ACK/NAK?

-sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Marek Vasut]

> On Wed Jan 11, 2012 at 02:52:44PM +0100, Marek Vasut wrote:
> > > >   Changing the ecc layout for a single board, hmm not sure. Using a
> > > >   spl instead does me no harm whatsoever -- I don't need to update
> > > >   the spl frequently in any case, and then can use the nand driver
> > > >   as is.
> > > 
> > > And how do you replace the SPL?
> > > 
> > > Either way, I think the correct approach is to allow the driver to
> > > handle the SPL update too.
> > 
> > Or rather -- to be able to load u-boot directly. It seems to be more
> > correct solution. The SPL you're using is just a workaround and also you
> > need to TI flasher to replace it, right ?
> 
>   Yes, using the spl is a workaround, and like i mentioned in my
>   previous mail, i thought having the spl was not a problem on the
>   hawkboard. I had thought putting a new oob layout in the common
>   davinci nand driver for a single board to be not a clean fix
>   either.

Then just allow the driver to be passed different OOB layout and make the 
current one a default

>   More so, given the fact that we don't have any control over
>   rbl -- so if rbl changes it's layout for any subsequent board, we'd
>   have to add that as well to the nand driver, and both in u-boot as
>   well as the kernel.
> 
>   I guess the cleanest solution would have been for the rbl to have
>   used the same layout as the one used by u-boot and linux.

Yep, why not do that then?

> 
>   Btw, can you please review this change, that this patch
>   fixes. ACK/NAK?

I'll think about it. It's ok, but it piles one workaround on top of another one, 
I don't like that approach.

M

> 
> -sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Sughosh Ganu]

On Wed Jan 11, 2012 at 04:01:31PM +0100, Marek Vasut wrote:

> >   More so, given the fact that we don't have any control over
> >   rbl -- so if rbl changes it's layout for any subsequent board, we'd
> >   have to add that as well to the nand driver, and both in u-boot as
> >   well as the kernel.
> > 
> >   I guess the cleanest solution would have been for the rbl to have
> >   used the same layout as the one used by u-boot and linux.
> 
> Yep, why not do that then?

  Because rbl is a proprietary bootloader from TI.

> > 
> >   Btw, can you please review this change, that this patch
> >   fixes. ACK/NAK?
> 
> I'll think about it. It's ok, but it piles one workaround on top of another one, 
> I don't like that approach.

  Flushing the data cache before disabling it is not a workaround,
  it's a fix. The current code is invalidating the cache instead of
  flushing it.

-sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Marek Vasut]

> On Wed Jan 11, 2012 at 04:01:31PM +0100, Marek Vasut wrote:
> > >   More so, given the fact that we don't have any control over
> > >   rbl -- so if rbl changes it's layout for any subsequent board, we'd
> > >   have to add that as well to the nand driver, and both in u-boot as
> > >   well as the kernel.
> > >   
> > >   I guess the cleanest solution would have been for the rbl to have
> > >   used the same layout as the one used by u-boot and linux.
> > 
> > Yep, why not do that then?
> 
>   Because rbl is a proprietary bootloader from TI.

Don't we actually have replacement bootloader in uboot already ? You don't need 
xloader with uboot anymore I think.

> 
> > >   Btw, can you please review this change, that this patch
> > >   fixes. ACK/NAK?
> > 
> > I'll think about it. It's ok, but it piles one workaround on top of
> > another one, I don't like that approach.
> 
>   Flushing the data cache before disabling it is not a workaround,
>   it's a fix. The current code is invalidating the cache instead of
>   flushing it.

Ok, but I'd like to see proper solution eventually.
> 
> -sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Christian Riesch]

Hi,

On Wednesday, January 11, 2012, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On Wed Jan 11, 2012 at 04:01:31PM +0100, Marek Vasut wrote:
>> > >   More so, given the fact that we don't have any control over
>> > >   rbl -- so if rbl changes it's layout for any subsequent board, we'd
>> > >   have to add that as well to the nand driver, and both in u-boot as
>> > >   well as the kernel.
>> > >
>> > >   I guess the cleanest solution would have been for the rbl to have
>> > >   used the same layout as the one used by u-boot and linux.
>> >
>> > Yep, why not do that then?
>>
>>   Because rbl is a proprietary bootloader from TI.
>
> Don't we actually have replacement bootloader in uboot already ? You
don't need
> xloader with uboot anymore I think.
>

RBL ist the ROM bootloader in the SoC, it's not only proprietary but also
in ROM and hence cannot be changed.

RBL executes an AIS script. Sughosh, could you please explain what your AIS
does or how you create it?

Regards, Christian

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Marek Vasut]

> Hi,
> 
> On Wednesday, January 11, 2012, Marek Vasut <marek.vasut@gmail.com> wrote:
> >> On Wed Jan 11, 2012 at 04:01:31PM +0100, Marek Vasut wrote:
> >> > >   More so, given the fact that we don't have any control over
> >> > >   rbl -- so if rbl changes it's layout for any subsequent board,
> >> > >   we'd have to add that as well to the nand driver, and both in
> >> > >   u-boot as well as the kernel.
> >> > >   
> >> > >   I guess the cleanest solution would have been for the rbl to have
> >> > >   used the same layout as the one used by u-boot and linux.
> >> > 
> >> > Yep, why not do that then?
> >> > 
> >>   Because rbl is a proprietary bootloader from TI.
> > 
> > Don't we actually have replacement bootloader in uboot already ? You
> 
> don't need
> 
> > xloader with uboot anymore I think.
> 
> RBL ist the ROM bootloader in the SoC, it's not only proprietary but also
> in ROM and hence cannot be changed.
> 
> RBL executes an AIS script. Sughosh, could you please explain what your AIS
> does or how you create it?

So basically, this SPL business can be avoided and this all can be done in a 
standard way?

M
> 
> Regards, Christian

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Christian Riesch]

On Wednesday, January 11, 2012, Marek Vasut <marek.vasut@gmail.com> wrote:
>> Hi,
>>
>> On Wednesday, January 11, 2012, Marek Vasut <marek.vasut@gmail.com>
wrote:
>> >> On Wed Jan 11, 2012 at 04:01:31PM +0100, Marek Vasut wrote:
>> >> > >   More so, given the fact that we don't have any control over
>> >> > >   rbl -- so if rbl changes it's layout for any subsequent board,
>> >> > >   we'd have to add that as well to the nand driver, and both in
>> >> > >   u-boot as well as the kernel.
>> >> > >
>> >> > >   I guess the cleanest solution would have been for the rbl to
have
>> >> > >   used the same layout as the one used by u-boot and linux.
>> >> >
>> >> > Yep, why not do that then?
>> >> >
>> >>   Because rbl is a proprietary bootloader from TI.
>> >
>> > Don't we actually have replacement bootloader in uboot already ? You
>>
>> don't need
>>
>> > xloader with uboot anymore I think.
>>
>> RBL ist the ROM bootloader in the SoC, it's not only proprietary but also
>> in ROM and hence cannot be changed.
>>
>> RBL executes an AIS script. Sughosh, could you please explain what your
AIS
>> does or how you create it?
>
> So basically, this SPL business can be avoided and this all can be done
in a
> standard way?

I don't know, I never had to deal with booting from NAND. I was just
wondering what Sughosh's AIS is doing that he gets these SPL problems.
Christian

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Sughosh Ganu]

On Thu Jan 12, 2012 at 06:56:01AM +0100, Christian Riesch wrote:
> On Wednesday, January 11, 2012, Marek Vasut <marek.vasut@gmail.com> wrote:

<snip>

> >> RBL executes an AIS script. Sughosh, could you please explain what your
> AIS
> >> does or how you create it?
> >
> > So basically, this SPL business can be avoided and this all can be done
> in a
> > standard way?
> 
> I don't know, I never had to deal with booting from NAND. I was just
> wondering what Sughosh's AIS is doing that he gets these SPL problems.
> Christian

  I have checked my ais ini file, and it does the normal pll/ddr
  settings. I think it is the rbl which might be turning the cache
  ON. In any case, this patch holds true irrespective of whether rbl
  boots u-boot or spl.

  As for the question of doing away with spl and booting u-boot
  directly on hawkboard, i would want to stay with spl for now. I
  would check if we can pass the ecc layout to the nand driver in a
  clean elegant way, as Marek has suggested.
  
-sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Christian Riesch]

Hi Sughosh,

On Tue, Jan 10, 2012 at 7:12 PM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> The current implementation invalidates the cache instead of flushing
> it. This causes problems on platforms where the spl/u-boot is already
> loaded to the RAM, with caches enabled by a first stage bootloader.
>
> The V bit of the cp15's control register c1 is set to the value of
> VINITHI on reset. Do not clear this bit by default, as there are SOC's
> with no valid memory region at 0x0.
>
> Signed-off-by: Sughosh Ganu <urwithsughosh@gmail.com>
> Cc: Albert Aribaud <albert.u.boot@aribaud.net>
> ---
>
> Changes since V1
> * Added arm926 keyword to the subject line
> * Removed the superfluous setting of r0.
> * Fixed the comment to reflect the fact that V is not being cleared

I did a few tests of this patch with the da850evm (on an AM1808
experimenter's kit) and the calimain (on our custom board)
configurations.

I used u-boot from git://git.denx.de/u-boot-ti.git master, head is
14023683951b9a2bd277e999b0798b4917aca5d5. On top of this I applied
these three patches:

[U-Boot,1/2,V2] arm926: Flush the data cache before disabling it.
http://patchwork.ozlabs.org/patch/135275/

[U-Boot,2/2,V4] Changes to move hawkboard to the new spl infrastructure
http://patchwork.ozlabs.org/patch/135433/

[U-Boot,v2] arm, davinci: Add support for the Calimain board from
OMICRON electronics
http://patchwork.ozlabs.org/patch/135593/

First I compiled for the da850evm.
make mrproper
make da850evm_config
make -j3 -s u-boot.ais

I flashed the resulting u-boot.ais to the SPI flash of the AM1808
experimenter's kit:
mono dvflashutils/OMAP-L138/GNU/sfh_OMAP-L138.exe -targetType AM1808
-flashType SPI_MEM -p /dev/ttyUSB0 -flash_noubl ../u-boot/u-boot.ais
And tried to boot -> SUCCESS

I also tried the TI way (using the ubl):
mono dvflashutils/OMAP-L138/GNU/sfh_OMAP-L138.exe -targetType AM1808
-flashType SPI_MEM -p /dev/ttyUSB0 -flash
dvflashutils/OMAP-L138/GNU/ubl/ubl_AM1808_SPI_MEM.bin
../u-boot/u-boot.bin
And tried to boot -> SUCCESS

Countercheck: I reverted patch "[U-Boot,1/2,V2] arm926: Flush the data
cache before disabling it." and rebuilt
git revert HEAD~2
make mrproper
make da850evm_config
make -j3 -s u-boot.ais

Again I tried with ubl and without -> both worked -> SUCCESS

Since you state that problems arise with the AIS scripts I also did a
test loading u-boot with an AIS.

This is my da850evm.ini
[General]
busWidth=8
BootMode=SPIMASTER
crcCheckType=NO_CRC
[PLLANDCLOCKCONFIG]
PLL0CFG0 = 0x00180001
PLL0CFG1 = 0x00000205
PERIPHCLKCFG = 0x00000002
[EMIF3DDR]
PLL1CFG0 = 0x15010001
PLL1CFG1 = 0x00000002
DDRPHYC1R = 0x000000C4
SDCR = 0x0A034622
SDTIMR = 0x184929C8
SDTIMR2 = 0xB80FC700
SDRCR = 0x00000406
CLK2XSRC = 0x00000000
[INPUTFILE]
FILENAME=u-boot.bin
LOADADDRESS=0xC1080000
ENTRYPOINTADDRESS=0xC1080000

I run
mono dvflashutils/OMAP-L138/GNU/AISUtils/HexAIS_OMAP-L138.exe -ini
da850evm.ini -o u-boot.ais
program to flash and run -> SUCCESS

I undo the revert
git reset --hard HEAD^
and rebuild
make mrproper
make da850evm_config
make -j3 -s u-boot.ais
program and run -> SUCCESS

Similar tests with the calimain board were also successful.

Since all my tests were successful I wonder what issues did you have
with the cache? Can you describe the problems you had? I think the
difference is that you are booting from NAND and have an OMAP-L138,
whereas I boot from SPI (on the da850evm) or from NOR (on calimain)
and have an AM1808 on both boards, right?

Regards, Christian

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Sughosh Ganu]

hi Christian,

On Thu Jan 12, 2012 at 01:03:05PM +0100, Christian Riesch wrote:
> Hi Sughosh,
> 
> On Tue, Jan 10, 2012 at 7:12 PM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> > The current implementation invalidates the cache instead of flushing
> > it. This causes problems on platforms where the spl/u-boot is already
> > loaded to the RAM, with caches enabled by a first stage bootloader.
> >
> > The V bit of the cp15's control register c1 is set to the value of
> > VINITHI on reset. Do not clear this bit by default, as there are SOC's
> > with no valid memory region at 0x0.
> >
> > Signed-off-by: Sughosh Ganu <urwithsughosh@gmail.com>
> > Cc: Albert Aribaud <albert.u.boot@aribaud.net>
> > ---
> >
> > Changes since V1
> > * Added arm926 keyword to the subject line
> > * Removed the superfluous setting of r0.
> > * Fixed the comment to reflect the fact that V is not being cleared
> 
> I did a few tests of this patch with the da850evm (on an AM1808
> experimenter's kit) and the calimain (on our custom board)
> configurations.
> 
> I used u-boot from git://git.denx.de/u-boot-ti.git master, head is
> 14023683951b9a2bd277e999b0798b4917aca5d5. On top of this I applied
> these three patches:
> 
> [U-Boot,1/2,V2] arm926: Flush the data cache before disabling it.
> http://patchwork.ozlabs.org/patch/135275/
> 
> [U-Boot,2/2,V4] Changes to move hawkboard to the new spl infrastructure
> http://patchwork.ozlabs.org/patch/135433/
> 
> [U-Boot,v2] arm, davinci: Add support for the Calimain board from
> OMICRON electronics
> http://patchwork.ozlabs.org/patch/135593/
> 
> First I compiled for the da850evm.
> make mrproper
> make da850evm_config
> make -j3 -s u-boot.ais
> 
> I flashed the resulting u-boot.ais to the SPI flash of the AM1808
> experimenter's kit:
> mono dvflashutils/OMAP-L138/GNU/sfh_OMAP-L138.exe -targetType AM1808
> -flashType SPI_MEM -p /dev/ttyUSB0 -flash_noubl ../u-boot/u-boot.ais
> And tried to boot -> SUCCESS
> 
> I also tried the TI way (using the ubl):
> mono dvflashutils/OMAP-L138/GNU/sfh_OMAP-L138.exe -targetType AM1808
> -flashType SPI_MEM -p /dev/ttyUSB0 -flash
> dvflashutils/OMAP-L138/GNU/ubl/ubl_AM1808_SPI_MEM.bin
> ../u-boot/u-boot.bin
> And tried to boot -> SUCCESS
> 
> Countercheck: I reverted patch "[U-Boot,1/2,V2] arm926: Flush the data
> cache before disabling it." and rebuilt
> git revert HEAD~2
> make mrproper
> make da850evm_config
> make -j3 -s u-boot.ais
> 
> Again I tried with ubl and without -> both worked -> SUCCESS
> 
> Since you state that problems arise with the AIS scripts I also did a
> test loading u-boot with an AIS.

<snip>

> I run
> mono dvflashutils/OMAP-L138/GNU/AISUtils/HexAIS_OMAP-L138.exe -ini
> da850evm.ini -o u-boot.ais
> program to flash and run -> SUCCESS
> 
> I undo the revert
> git reset --hard HEAD^
> and rebuild
> make mrproper
> make da850evm_config
> make -j3 -s u-boot.ais
> program and run -> SUCCESS
> 
> Similar tests with the calimain board were also successful.
> 
> Since all my tests were successful I wonder what issues did you have
> with the cache? Can you describe the problems you had? I think the
> difference is that you are booting from NAND and have an OMAP-L138,
> whereas I boot from SPI (on the da850evm) or from NOR (on calimain)
> and have an AM1808 on both boards, right?

  Thanks a lot for all the testing. One difference i think we have on
  these boards and hawkboard, is that on hawkboard, the rbl configures
  the memory and loads the target image(spl in this case) directly to
  the ram. Looking at da850evm's lds file, it looks like the spl
  gets loaded to the sram, configures dram and then copies u-boot to
  the target load address.

  I think the rbl is enabling the caches in case of hawkboard, and
  on loading spl, it causes a problem when the cache is invalidated
  instead of being flushed. I'm pretty sure this is the problem, as
  the hawkboard does not boot without this fix. Thanks for the time
  taken for doing such elaborate tests.

-sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Christian Riesch]

On Thu, Jan 12, 2012 at 2:53 PM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> On Thu Jan 12, 2012 at 01:03:05PM +0100, Christian Riesch wrote:
>> On Tue, Jan 10, 2012 at 7:12 PM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
>> > The current implementation invalidates the cache instead of flushing
>> > it. This causes problems on platforms where the spl/u-boot is already
>> > loaded to the RAM, with caches enabled by a first stage bootloader.
>> >
>> > The V bit of the cp15's control register c1 is set to the value of
>> > VINITHI on reset. Do not clear this bit by default, as there are SOC's
>> > with no valid memory region at 0x0.
>> >
>> > Signed-off-by: Sughosh Ganu <urwithsughosh@gmail.com>
>> > Cc: Albert Aribaud <albert.u.boot@aribaud.net>
>> > ---
>> >
>> > Changes since V1
>> > * Added arm926 keyword to the subject line
>> > * Removed the superfluous setting of r0.
>> > * Fixed the comment to reflect the fact that V is not being cleared
>>
>> I did a few tests of this patch with the da850evm (on an AM1808
>> experimenter's kit) and the calimain (on our custom board)
>> configurations.
>>
>> I used u-boot from git://git.denx.de/u-boot-ti.git master, head is
>> 14023683951b9a2bd277e999b0798b4917aca5d5. On top of this I applied
>> these three patches:
>>
>> [U-Boot,1/2,V2] arm926: Flush the data cache before disabling it.
>> http://patchwork.ozlabs.org/patch/135275/
>>
>> [U-Boot,2/2,V4] Changes to move hawkboard to the new spl infrastructure
>> http://patchwork.ozlabs.org/patch/135433/
>>
>> [U-Boot,v2] arm, davinci: Add support for the Calimain board from
>> OMICRON electronics
>> http://patchwork.ozlabs.org/patch/135593/
>>
>> First I compiled for the da850evm.
>> make mrproper
>> make da850evm_config
>> make -j3 -s u-boot.ais
>>
>> I flashed the resulting u-boot.ais to the SPI flash of the AM1808
>> experimenter's kit:
>> mono dvflashutils/OMAP-L138/GNU/sfh_OMAP-L138.exe -targetType AM1808
>> -flashType SPI_MEM -p /dev/ttyUSB0 -flash_noubl ../u-boot/u-boot.ais
>> And tried to boot -> SUCCESS
>>
>> I also tried the TI way (using the ubl):
>> mono dvflashutils/OMAP-L138/GNU/sfh_OMAP-L138.exe -targetType AM1808
>> -flashType SPI_MEM -p /dev/ttyUSB0 -flash
>> dvflashutils/OMAP-L138/GNU/ubl/ubl_AM1808_SPI_MEM.bin
>> ../u-boot/u-boot.bin
>> And tried to boot -> SUCCESS
>>
>> Countercheck: I reverted patch "[U-Boot,1/2,V2] arm926: Flush the data
>> cache before disabling it." and rebuilt
>> git revert HEAD~2
>> make mrproper
>> make da850evm_config
>> make -j3 -s u-boot.ais
>>
>> Again I tried with ubl and without -> both worked -> SUCCESS
>>
>> Since you state that problems arise with the AIS scripts I also did a
>> test loading u-boot with an AIS.
>
> <snip>
>
>> I run
>> mono dvflashutils/OMAP-L138/GNU/AISUtils/HexAIS_OMAP-L138.exe -ini
>> da850evm.ini -o u-boot.ais
>> program to flash and run -> SUCCESS
>>
>> I undo the revert
>> git reset --hard HEAD^
>> and rebuild
>> make mrproper
>> make da850evm_config
>> make -j3 -s u-boot.ais
>> program and run -> SUCCESS
>>
>> Similar tests with the calimain board were also successful.
>>
>> Since all my tests were successful I wonder what issues did you have
>> with the cache? Can you describe the problems you had? I think the
>> difference is that you are booting from NAND and have an OMAP-L138,
>> whereas I boot from SPI (on the da850evm) or from NOR (on calimain)
>> and have an AM1808 on both boards, right?
>
> ?Thanks a lot for all the testing. One difference i think we have on
> ?these boards and hawkboard, is that on hawkboard, the rbl configures
> ?the memory and loads the target image(spl in this case) directly to
> ?the ram. Looking at da850evm's lds file, it looks like the spl
> ?gets loaded to the sram, configures dram and then copies u-boot to
> ?the target load address.

This is only true if the SPL is actually used. Have a closer look at
my test report, I tested three different methods:

1) The first test was done with the SPL and yes, here the RBL loads
the SPL into SRAM, initializes DDR memory and then copies u-boot.bin
to DDR memory.
2) The second test was done with TI's UBL. Here, the RBL loads the UBL
into SRAM, the UBL initializes DDR memory and then copies u-boot.bin
to DDR memory.
3) The third test was done without SPL and without UBL: Here the DDR
memory init is in the AIS, so in fact the RBL does memory
initialization and then RBL loads u-boot.bin to DDR memory. This is
the same case that you have on the hawkboard (only that you have the
OMAP-L138 and NAND flash instead) and it works for me regardless of
your patch.

Christian

>
> ?I think the rbl is enabling the caches in case of hawkboard, and
> ?on loading spl, it causes a problem when the cache is invalidated
> ?instead of being flushed. I'm pretty sure this is the problem, as
> ?the hawkboard does not boot without this fix. Thanks for the time
> ?taken for doing such elaborate tests.
>
> -sughosh
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Sughosh Ganu]

hi Christian,

On Thu Jan 12, 2012 at 03:04:37PM +0100, Christian Riesch wrote:
> On Thu, Jan 12, 2012 at 2:53 PM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> > On Thu Jan 12, 2012 at 01:03:05PM +0100, Christian Riesch wrote:

<snip>

> >>
> >> Since all my tests were successful I wonder what issues did you have
> >> with the cache? Can you describe the problems you had? I think the
> >> difference is that you are booting from NAND and have an OMAP-L138,
> >> whereas I boot from SPI (on the da850evm) or from NOR (on calimain)
> >> and have an AM1808 on both boards, right?
> >
> > ?Thanks a lot for all the testing. One difference i think we have on
> > ?these boards and hawkboard, is that on hawkboard, the rbl configures
> > ?the memory and loads the target image(spl in this case) directly to
> > ?the ram. Looking at da850evm's lds file, it looks like the spl
> > ?gets loaded to the sram, configures dram and then copies u-boot to
> > ?the target load address.
> 
> This is only true if the SPL is actually used. Have a closer look at
> my test report, I tested three different methods:
> 
> 1) The first test was done with the SPL and yes, here the RBL loads
> the SPL into SRAM, initializes DDR memory and then copies u-boot.bin
> to DDR memory.
> 2) The second test was done with TI's UBL. Here, the RBL loads the UBL
> into SRAM, the UBL initializes DDR memory and then copies u-boot.bin
> to DDR memory.
> 3) The third test was done without SPL and without UBL: Here the DDR
> memory init is in the AIS, so in fact the RBL does memory
> initialization and then RBL loads u-boot.bin to DDR memory. This is
> the same case that you have on the hawkboard (only that you have the
> OMAP-L138 and NAND flash instead) and it works for me regardless of
> your patch.

  Yes, the third case is similar to the one used in hawkboard. I'm not
  sure as to why it causes a problem on my board, though i'm not sure
  if we can compare the two cases, as we have different rbl's. It
  could be that the rbl used on hawkboard initialises the caches, as
  the caches are off by default on reset.

  Here are the values i use in my ini file for ddr init.

[EMIF3DDR]
PLL1CFG0 = 0x15010001
PLL1CFG1 = 0x00000002

DDRPHYC1R = 0x00000043
SDCR = 0x00134632
SDTIMR = 0x26492a09
SDTIMR2 = 0x7d13c722
SDRCR = 0x00000249
CLK2XSRC = 0x00000000

-sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Christian Riesch]

Hi Sughosh,
I had a look at the patch and I tried to understand what's going on
here (I must confess that I didn't know anything about this cache
stuff).

On Tue, Jan 10, 2012 at 7:12 PM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> The current implementation invalidates the cache instead of flushing
> it. This causes problems on platforms where the spl/u-boot is already
> loaded to the RAM, with caches enabled by a first stage bootloader.
>
> The V bit of the cp15's control register c1 is set to the value of
> VINITHI on reset. Do not clear this bit by default, as there are SOC's
> with no valid memory region at 0x0.
[...]
> diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S
> index 6a09c02..6e261c2 100644
> --- a/arch/arm/cpu/arm926ejs/start.S
> +++ b/arch/arm/cpu/arm926ejs/start.S
> @@ -355,17 +355,20 @@ _dynsym_start_ofs:
> ?*/
> ?cpu_init_crit:
> ? ? ? ?/*
> - ? ? ? ?* flush v4 I/D caches
> + ? ? ? ?* flush D cache before disabling it
> ? ? ? ? */
> ? ? ? ?mov ? ? r0, #0
> - ? ? ? mcr ? ? p15, 0, r0, c7, c7, 0 ? /* flush v3/v4 cache */
> +flush_dcache:
> + ? ? ? mrc ? ? p15, 0, r15, c7, c10, 3
> + ? ? ? bne ? ? flush_dcache
> +

Ok, instead of invalidating the D-cache you are cleaning it. From the
ARM926EJ-S Technical Reference Manual [1]: "To guarantee that memory
coherency is maintained, the DCache must be cleaned of dirty data
before it is disabled." So since we are disabling D-Cache a few lines
later (bic r0, r0, #0x00000087), this must be the right way to do it.

> ? ? ? ?mcr ? ? p15, 0, r0, c8, c7, 0 ? /* flush v4 TLB */
>
> ? ? ? ?/*
> ? ? ? ? * disable MMU stuff and caches
> ? ? ? ? */
> ? ? ? ?mrc ? ? p15, 0, r0, c1, c0, 0
> - ? ? ? bic ? ? r0, r0, #0x00002300 ? ? /* clear bits 13, 9:8 (--V- --RS) */
> + ? ? ? bic ? ? r0, r0, #0x00000300 ? ? /* clear bits 9:8 ( --RS) */

Ok, I read your comment above.

> ? ? ? ?bic ? ? r0, r0, #0x00000087 ? ? /* clear bits 7, 2:0 (B--- -CAM) */
> ? ? ? ?orr ? ? r0, r0, #0x00000002 ? ? /* set bit 2 (A) Align */
> ? ? ? ?orr ? ? r0, r0, #0x00001000 ? ? /* set bit 12 (I) I-Cache */

Although this is not changed in your patch, the last line makes me
wonder. The comment says "disable MMU stuff and cached", but actually
the last line sets bit 12 (I), which means that I-Cache gets enabled
according to [1].

Regards, Christian

[1] http://infocenter.arm.com/help/index.jsp

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Sughosh Ganu]

hi Christian,

On Fri Jan 13, 2012 at 09:06:26AM +0100, Christian Riesch wrote:
> Hi Sughosh,
> I had a look at the patch and I tried to understand what's going on
> here (I must confess that I didn't know anything about this cache
> stuff).

  Ok, thanks for taking time off to understand this issue.

> 
> On Tue, Jan 10, 2012 at 7:12 PM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> > The current implementation invalidates the cache instead of flushing
> > it. This causes problems on platforms where the spl/u-boot is already
> > loaded to the RAM, with caches enabled by a first stage bootloader.
> >
> > The V bit of the cp15's control register c1 is set to the value of
> > VINITHI on reset. Do not clear this bit by default, as there are SOC's
> > with no valid memory region at 0x0.
> [...]
> > diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S
> > index 6a09c02..6e261c2 100644
> > --- a/arch/arm/cpu/arm926ejs/start.S
> > +++ b/arch/arm/cpu/arm926ejs/start.S
> > @@ -355,17 +355,20 @@ _dynsym_start_ofs:
> > ?*/
> > ?cpu_init_crit:
> > ? ? ? ?/*
> > - ? ? ? ?* flush v4 I/D caches
> > + ? ? ? ?* flush D cache before disabling it
> > ? ? ? ? */
> > ? ? ? ?mov ? ? r0, #0
> > - ? ? ? mcr ? ? p15, 0, r0, c7, c7, 0 ? /* flush v3/v4 cache */
> > +flush_dcache:
> > + ? ? ? mrc ? ? p15, 0, r15, c7, c10, 3
> > + ? ? ? bne ? ? flush_dcache
> > +
> 
> Ok, instead of invalidating the D-cache you are cleaning it. From the
> ARM926EJ-S Technical Reference Manual [1]: "To guarantee that memory
> coherency is maintained, the DCache must be cleaned of dirty data
> before it is disabled." So since we are disabling D-Cache a few lines
> later (bic r0, r0, #0x00000087), this must be the right way to do it.

  Right, so the problem here is that instead of flushing the cache,
  the code currently invalidates it. This would not be a problem on
  platforms where cache is not turned ON(which would be the majority
  of cases), but on my board, it seems to be turned ON by the rbl,
  due to which it does not boot.

  If you check the manual, the loop that i have introduced is one for
  "Testing and Cleaning", which means that in case the lines are not
  dirty, it won't be flushed. So this should not break any platforms
  where the cache isn't enabled.

> 
> > ? ? ? ?mcr ? ? p15, 0, r0, c8, c7, 0 ? /* flush v4 TLB */
> >
> > ? ? ? ?/*
> > ? ? ? ? * disable MMU stuff and caches
> > ? ? ? ? */
> > ? ? ? ?mrc ? ? p15, 0, r0, c1, c0, 0
> > - ? ? ? bic ? ? r0, r0, #0x00002300 ? ? /* clear bits 13, 9:8 (--V- --RS) */
> > + ? ? ? bic ? ? r0, r0, #0x00000300 ? ? /* clear bits 9:8 ( --RS) */
> 
> Ok, I read your comment above.
> 
> > ? ? ? ?bic ? ? r0, r0, #0x00000087 ? ? /* clear bits 7, 2:0 (B--- -CAM) */
> > ? ? ? ?orr ? ? r0, r0, #0x00000002 ? ? /* set bit 2 (A) Align */
> > ? ? ? ?orr ? ? r0, r0, #0x00001000 ? ? /* set bit 12 (I) I-Cache */
> 
> Although this is not changed in your patch, the last line makes me
> wonder. The comment says "disable MMU stuff and cached", but actually
> the last line sets bit 12 (I), which means that I-Cache gets enabled
> according to [1].

  Yeah, this seems to be copied code, with discrepancies in the code
  and comments. You would see that the line i have removed has a
  comment for flushing the cache, but instead it is invalidating the
  cache. I have just fixed the comments for the lines that i made
  changes to.

-sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Tom Rini]

On Fri, Jan 13, 2012 at 1:26 AM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> hi Christian,
>
> On Fri Jan 13, 2012 at 09:06:26AM +0100, Christian Riesch wrote:
>> Hi Sughosh,
>> I had a look at the patch and I tried to understand what's going on
>> here (I must confess that I didn't know anything about this cache
>> stuff).
>
> ?Ok, thanks for taking time off to understand this issue.
>
>>
>> On Tue, Jan 10, 2012 at 7:12 PM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
>> > The current implementation invalidates the cache instead of flushing
>> > it. This causes problems on platforms where the spl/u-boot is already
>> > loaded to the RAM, with caches enabled by a first stage bootloader.
>> >
>> > The V bit of the cp15's control register c1 is set to the value of
>> > VINITHI on reset. Do not clear this bit by default, as there are SOC's
>> > with no valid memory region at 0x0.
>> [...]
>> > diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S
>> > index 6a09c02..6e261c2 100644
>> > --- a/arch/arm/cpu/arm926ejs/start.S
>> > +++ b/arch/arm/cpu/arm926ejs/start.S
>> > @@ -355,17 +355,20 @@ _dynsym_start_ofs:
>> > ?*/
>> > ?cpu_init_crit:
>> > ? ? ? ?/*
>> > - ? ? ? ?* flush v4 I/D caches
>> > + ? ? ? ?* flush D cache before disabling it
>> > ? ? ? ? */
>> > ? ? ? ?mov ? ? r0, #0
>> > - ? ? ? mcr ? ? p15, 0, r0, c7, c7, 0 ? /* flush v3/v4 cache */
>> > +flush_dcache:
>> > + ? ? ? mrc ? ? p15, 0, r15, c7, c10, 3
>> > + ? ? ? bne ? ? flush_dcache
>> > +
>>
>> Ok, instead of invalidating the D-cache you are cleaning it. From the
>> ARM926EJ-S Technical Reference Manual [1]: "To guarantee that memory
>> coherency is maintained, the DCache must be cleaned of dirty data
>> before it is disabled." So since we are disabling D-Cache a few lines
>> later (bic r0, r0, #0x00000087), this must be the right way to do it.
>
> ?Right, so the problem here is that instead of flushing the cache,
> ?the code currently invalidates it. This would not be a problem on
> ?platforms where cache is not turned ON(which would be the majority
> ?of cases), but on my board, it seems to be turned ON by the rbl,
> ?due to which it does not boot.
>
> ?If you check the manual, the loop that i have introduced is one for
> ?"Testing and Cleaning", which means that in case the lines are not
> ?dirty, it won't be flushed. So this should not break any platforms
> ?where the cache isn't enabled.
>
>>
>> > ? ? ? ?mcr ? ? p15, 0, r0, c8, c7, 0 ? /* flush v4 TLB */
>> >
>> > ? ? ? ?/*
>> > ? ? ? ? * disable MMU stuff and caches
>> > ? ? ? ? */
>> > ? ? ? ?mrc ? ? p15, 0, r0, c1, c0, 0
>> > - ? ? ? bic ? ? r0, r0, #0x00002300 ? ? /* clear bits 13, 9:8 (--V- --RS) */
>> > + ? ? ? bic ? ? r0, r0, #0x00000300 ? ? /* clear bits 9:8 ( --RS) */
>>
>> Ok, I read your comment above.
>>
>> > ? ? ? ?bic ? ? r0, r0, #0x00000087 ? ? /* clear bits 7, 2:0 (B--- -CAM) */
>> > ? ? ? ?orr ? ? r0, r0, #0x00000002 ? ? /* set bit 2 (A) Align */
>> > ? ? ? ?orr ? ? r0, r0, #0x00001000 ? ? /* set bit 12 (I) I-Cache */
>>
>> Although this is not changed in your patch, the last line makes me
>> wonder. The comment says "disable MMU stuff and cached", but actually
>> the last line sets bit 12 (I), which means that I-Cache gets enabled
>> according to [1].
>
> ?Yeah, this seems to be copied code, with discrepancies in the code
> ?and comments. You would see that the line i have removed has a
> ?comment for flushing the cache, but instead it is invalidating the
> ?cache. I have just fixed the comments for the lines that i made
> ?changes to.

I think while we're in here and noticing these things we should fix
the comments at least.

-- 
Tom

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Heiko Schocher]

Hello Christian,

Christian Riesch wrote:
> Hi Sughosh,
> I had a look at the patch and I tried to understand what's going on
> here (I must confess that I didn't know anything about this cache
> stuff).
> 
> On Tue, Jan 10, 2012 at 7:12 PM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
>> The current implementation invalidates the cache instead of flushing
>> it. This causes problems on platforms where the spl/u-boot is already
>> loaded to the RAM, with caches enabled by a first stage bootloader.
>>
>> The V bit of the cp15's control register c1 is set to the value of
>> VINITHI on reset. Do not clear this bit by default, as there are SOC's
>> with no valid memory region at 0x0.
> [...]
>> diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S
>> index 6a09c02..6e261c2 100644
>> --- a/arch/arm/cpu/arm926ejs/start.S
>> +++ b/arch/arm/cpu/arm926ejs/start.S
>> @@ -355,17 +355,20 @@ _dynsym_start_ofs:
>>  */
>>  cpu_init_crit:
>>        /*
>> -        * flush v4 I/D caches
>> +        * flush D cache before disabling it
>>         */
>>        mov     r0, #0
>> -       mcr     p15, 0, r0, c7, c7, 0   /* flush v3/v4 cache */
>> +flush_dcache:
>> +       mrc     p15, 0, r15, c7, c10, 3
>> +       bne     flush_dcache
>> +
> 
> Ok, instead of invalidating the D-cache you are cleaning it. From the
> ARM926EJ-S Technical Reference Manual [1]: "To guarantee that memory
> coherency is maintained, the DCache must be cleaned of dirty data
> before it is disabled." So since we are disabling D-Cache a few lines
> later (bic r0, r0, #0x00000087), this must be the right way to do it.

Yes, thats sounds reasonable to me, Albert?

>>        mcr     p15, 0, r0, c8, c7, 0   /* flush v4 TLB */
>>
>>        /*
>>         * disable MMU stuff and caches
>>         */
>>        mrc     p15, 0, r0, c1, c0, 0
>> -       bic     r0, r0, #0x00002300     /* clear bits 13, 9:8 (--V- --RS) */
>> +       bic     r0, r0, #0x00000300     /* clear bits 9:8 ( --RS) */
> 
> Ok, I read your comment above.

Hmm.. what should we do with the V-Bit? Is it OK not to clear it for all
cases?

Tested this patch on the cam_enc_4xx and enbw_cmc board, works fine
on that boards.

> 
>>        bic     r0, r0, #0x00000087     /* clear bits 7, 2:0 (B--- -CAM) */
>>        orr     r0, r0, #0x00000002     /* set bit 2 (A) Align */
>>        orr     r0, r0, #0x00001000     /* set bit 12 (I) I-Cache */
> 
> Although this is not changed in your patch, the last line makes me
> wonder. The comment says "disable MMU stuff and cached", but actually
> the last line sets bit 12 (I), which means that I-Cache gets enabled
> according to [1].

Yes, the last line enables the I-Cache. So we should at least add a
better comment here.

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Heiko Schocher]

Hello Sugosh,

Sughosh Ganu wrote:
> hi Christian,
> 
> On Fri Jan 13, 2012 at 09:06:26AM +0100, Christian Riesch wrote:
>> Hi Sughosh,
>> I had a look at the patch and I tried to understand what's going on
>> here (I must confess that I didn't know anything about this cache
>> stuff).
> 
>   Ok, thanks for taking time off to understand this issue.
> 
>> On Tue, Jan 10, 2012 at 7:12 PM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
>>> The current implementation invalidates the cache instead of flushing
>>> it. This causes problems on platforms where the spl/u-boot is already
>>> loaded to the RAM, with caches enabled by a first stage bootloader.

Hmm.. how did u-boot work on such boards? How can u-boot work with D-Cache
enabled, if u-boot is not initializing it? (And I think, on davinci SoC
we have a none working uboot ethernet driver if d-cache is enabled too).
There must be a page_table in DRAM for using D-Cache in U-Boot, if u-boot
don't initialize it, it maybe overrides it ... or miss I something?

Are you sure, the RBL enables the D-Cache on your board? Nevertheless,
I think, we must disable the D-Cache here with "cleaning" it (as your
patch did) instead only invalidating it, as current code did.

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Sughosh Ganu]

hi Heiko,

On Fri Jan 13, 2012 at 04:06:22PM +0100, Heiko Schocher wrote:

<snip>

> 
> >>        mcr     p15, 0, r0, c8, c7, 0   /* flush v4 TLB */
> >>
> >>        /*
> >>         * disable MMU stuff and caches
> >>         */
> >>        mrc     p15, 0, r0, c1, c0, 0
> >> -       bic     r0, r0, #0x00002300     /* clear bits 13, 9:8 (--V- --RS) */
> >> +       bic     r0, r0, #0x00000300     /* clear bits 9:8 ( --RS) */
> > 
> > Ok, I read your comment above.
> 
> Hmm.. what should we do with the V-Bit? Is it OK not to clear it for all
> cases?

  The V bit gets set to the value of VINITHI input pin on reset, which
  is the default value. This setting clears the V bit by default,
  which shifts the vector table to 0x0. Certain SoC's(e.g omap-l138)
  don't have any valid memory at 0x0. Hence i think this setting
  should not be made here, but on a SOC level.

> 
> Tested this patch on the cam_enc_4xx and enbw_cmc board, works fine
> on that boards.
> 
> > 
> >>        bic     r0, r0, #0x00000087     /* clear bits 7, 2:0 (B--- -CAM) */
> >>        orr     r0, r0, #0x00000002     /* set bit 2 (A) Align */
> >>        orr     r0, r0, #0x00001000     /* set bit 12 (I) I-Cache */
> > 
> > Although this is not changed in your patch, the last line makes me
> > wonder. The comment says "disable MMU stuff and cached", but actually
> > the last line sets bit 12 (I), which means that I-Cache gets enabled
> > according to [1].
> 
> Yes, the last line enables the I-Cache. So we should at least add a
> better comment here.

  I will fix the other comments too in the next version.

-sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Sughosh Ganu]

On Fri Jan 13, 2012 at 07:41:37AM -0700, Tom Rini wrote:
> On Fri, Jan 13, 2012 at 1:26 AM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:

<snip>

> >> > ? ? ? ?bic ? ? r0, r0, #0x00000087 ? ? /* clear bits 7, 2:0 (B--- -CAM) */
> >> > ? ? ? ?orr ? ? r0, r0, #0x00000002 ? ? /* set bit 2 (A) Align */
> >> > ? ? ? ?orr ? ? r0, r0, #0x00001000 ? ? /* set bit 12 (I) I-Cache */
> >>
> >> Although this is not changed in your patch, the last line makes me
> >> wonder. The comment says "disable MMU stuff and cached", but actually
> >> the last line sets bit 12 (I), which means that I-Cache gets enabled
> >> according to [1].
> >
> > ?Yeah, this seems to be copied code, with discrepancies in the code
> > ?and comments. You would see that the line i have removed has a
> > ?comment for flushing the cache, but instead it is invalidating the
> > ?cache. I have just fixed the comments for the lines that i made
> > ?changes to.
> 
> I think while we're in here and noticing these things we should fix
> the comments at least.

  Will fix them in the next version.

-sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Sughosh Ganu]

hi Heiko,

On Fri Jan 13, 2012 at 04:29:29PM +0100, Heiko Schocher wrote:
> Hello Sugosh,
> 
> Sughosh Ganu wrote:
> > hi Christian,
> > 
> > On Fri Jan 13, 2012 at 09:06:26AM +0100, Christian Riesch wrote:
> >> Hi Sughosh,
> >> I had a look at the patch and I tried to understand what's going on
> >> here (I must confess that I didn't know anything about this cache
> >> stuff).
> > 
> >   Ok, thanks for taking time off to understand this issue.
> > 
> >> On Tue, Jan 10, 2012 at 7:12 PM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> >>> The current implementation invalidates the cache instead of flushing
> >>> it. This causes problems on platforms where the spl/u-boot is already
> >>> loaded to the RAM, with caches enabled by a first stage bootloader.
> 
> Hmm.. how did u-boot work on such boards? How can u-boot work with D-Cache
> enabled, if u-boot is not initializing it? (And I think, on davinci SoC
> we have a none working uboot ethernet driver if d-cache is enabled too).
> There must be a page_table in DRAM for using D-Cache in U-Boot, if u-boot
> don't initialize it, it maybe overrides it ... or miss I something?

  Well, there is some data in the cache, which if not flushed creates
  problems on my board. I get the board to boot just by commenting out
  cpu_init_crit call. My hypothesis that the D-cache is enabled is
  simply because cache invalidation followed by cache disabling breaks
  the board, while flushing it prior to disabling gets it to boot
  fine. This(invalidation) would not have been a problem if the cache
  was in the disabled state.

> 
> Are you sure, the RBL enables the D-Cache on your board? Nevertheless,
> I think, we must disable the D-Cache here with "cleaning" it (as your
> patch did) instead only invalidating it, as current code did.

  I am not sure, this is just my guess. By default, the caches are
  disabled on reset, so the only possible source is the rbl. But I
  don't have access to the rbl code to say for sure. Unfortunately i
  also don't have JTAG or any other debugger to dig more into
  this. But yes, like you mention, we must be cleaning the cache
  before disabling it, so this fix is correct.

-sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Aneesh V]

On Friday 13 January 2012 11:08 PM, Sughosh Ganu wrote:
> hi Heiko,
>
> On Fri Jan 13, 2012 at 04:29:29PM +0100, Heiko Schocher wrote:
>> Hello Sugosh,
>>
>> Sughosh Ganu wrote:
>>> hi Christian,
>>>
>>> On Fri Jan 13, 2012 at 09:06:26AM +0100, Christian Riesch wrote:
>>>> Hi Sughosh,
>>>> I had a look at the patch and I tried to understand what's going on
>>>> here (I must confess that I didn't know anything about this cache
>>>> stuff).
>>>
>>>    Ok, thanks for taking time off to understand this issue.
>>>
>>>> On Tue, Jan 10, 2012 at 7:12 PM, Sughosh Ganu<urwithsughosh@gmail.com>  wrote:
>>>>> The current implementation invalidates the cache instead of flushing
>>>>> it. This causes problems on platforms where the spl/u-boot is already
>>>>> loaded to the RAM, with caches enabled by a first stage bootloader.
>>
>> Hmm.. how did u-boot work on such boards? How can u-boot work with D-Cache
>> enabled, if u-boot is not initializing it? (And I think, on davinci SoC
>> we have a none working uboot ethernet driver if d-cache is enabled too).
>> There must be a page_table in DRAM for using D-Cache in U-Boot, if u-boot
>> don't initialize it, it maybe overrides it ... or miss I something?
>
>    Well, there is some data in the cache, which if not flushed creates
>    problems on my board. I get the board to boot just by commenting out
>    cpu_init_crit call. My hypothesis that the D-cache is enabled is
>    simply because cache invalidation followed by cache disabling breaks
>    the board, while flushing it prior to disabling gets it to boot
>    fine. This(invalidation) would not have been a problem if the cache
>    was in the disabled state.

It would have affected if the state of dirty bits and valid bits are
not guaranteed at reset. But looks like cache entries are guaranteed to
be invalidated on reset in ARM926ejs per the spec (which is not the
case in ARMv7). In this case, I agree that flush shouldn't harm
anything ideally.

>
>>
>> Are you sure, the RBL enables the D-Cache on your board? Nevertheless,
>> I think, we must disable the D-Cache here with "cleaning" it (as your
>> patch did) instead only invalidating it, as current code did.
>
>    I am not sure, this is just my guess. By default, the caches are
>    disabled on reset, so the only possible source is the rbl. But I
>    don't have access to the rbl code to say for sure. Unfortunately i
>    also don't have JTAG or any other debugger to dig more into

I will be surprised too if D-cache is enabled by ROM code. But I agree
that your problem and the solution seems to indicate something like
that. To be sure can you check the value of CP15 System control
register on SPL entry? You are already reading it. Can you save it
somewhere and spit it out later?

>    this. But yes, like you mention, we must be cleaning the cache
>    before disabling it, so this fix is correct.

I think it will be good to add an invalidate of I-cache too. You have
replaced an invalidate of I & D cache with a flush of only D-cache.

br,
Aneesh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Sughosh Ganu]

On Fri Jan 13, 2012 at 11:49:57PM +0530, Aneesh V wrote:
> On Friday 13 January 2012 11:08 PM, Sughosh Ganu wrote:

<snip>

> >>
> >>Are you sure, the RBL enables the D-Cache on your board? Nevertheless,
> >>I think, we must disable the D-Cache here with "cleaning" it (as your
> >>patch did) instead only invalidating it, as current code did.
> >
> >   I am not sure, this is just my guess. By default, the caches are
> >   disabled on reset, so the only possible source is the rbl. But I
> >   don't have access to the rbl code to say for sure. Unfortunately i
> >   also don't have JTAG or any other debugger to dig more into
> 
> I will be surprised too if D-cache is enabled by ROM code. But I agree
> that your problem and the solution seems to indicate something like
> that. To be sure can you check the value of CP15 System control
> register on SPL entry? You are already reading it. Can you save it
> somewhere and spit it out later?

  Yes, i can try this out. Spitting it out as is won't be
  straightforward given the limited resources we have with spl(no
  printf). I'll see if i can pass the value to board_init_f, where i
  can check for it, and output some debug message.

> 
> >   this. But yes, like you mention, we must be cleaning the cache
> >   before disabling it, so this fix is correct.
> 
> I think it will be good to add an invalidate of I-cache too. You have
> replaced an invalidate of I & D cache with a flush of only D-cache.

  Ok, will add invalidation of the I cache.

-sughosh

[U-Boot] [PATCH 1/2 V3] arm926: Flush the data cache before disabling it

[Sughosh Ganu]

The current implementation invalidates the cache instead of flushing
it. This causes problems on platforms where the spl/u-boot is already
loaded to the RAM, with caches enabled by a first stage bootloader.

The V bit of the cp15's control register c1 is set to the value of
VINITHI on reset. Do not clear this bit by default, as there are SOC's
with no valid memory region at 0x0.

Also fix the comments to match code.

Signed-off-by: Sughosh Ganu <urwithsughosh@gmail.com>
---

Changes since V2
* Added code to invalidate I cache, based on review comment by Aneesh.
* Fixed comments to match the code.

Changes since V1
* Added arm926 keyword to the subject line
* Removed the superfluous setting of r0.
* Fixed the comment to reflect the fact that V is not being cleared

 arch/arm/cpu/arm926ejs/start.S |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S
index 6a09c02..91a9325 100644
--- a/arch/arm/cpu/arm926ejs/start.S
+++ b/arch/arm/cpu/arm926ejs/start.S
@@ -355,17 +355,21 @@ _dynsym_start_ofs:
  */
 cpu_init_crit:
 	/*
-	 * flush v4 I/D caches
+	 * flush D cache before disabling it
 	 */
 	mov	r0, #0
-	mcr	p15, 0, r0, c7, c7, 0	/* flush v3/v4 cache */
-	mcr	p15, 0, r0, c8, c7, 0	/* flush v4 TLB */
+flush_dcache:
+	mrc	p15, 0, r15, c7, c10, 3
+	bne	flush_dcache
+
+	mcr	p15, 0, r0, c8, c7, 0	/* invalidate TLB */
+	mcr	p15, 0, r0, c7, c5, 0	/* invalidate I Cache */
 
 	/*
-	 * disable MMU stuff and caches
+	 * disable MMU and D cache, and enable I cache.
 	 */
 	mrc	p15, 0, r0, c1, c0, 0
-	bic	r0, r0, #0x00002300	/* clear bits 13, 9:8 (--V- --RS) */
+	bic	r0, r0, #0x00000300	/* clear bits 9:8 ( --RS) */
 	bic	r0, r0, #0x00000087	/* clear bits 7, 2:0 (B--- -CAM) */
 	orr	r0, r0, #0x00000002	/* set bit 2 (A) Align */
 	orr	r0, r0, #0x00001000	/* set bit 12 (I) I-Cache */
-- 
1.7.5.4

[U-Boot] [PATCH 1/2 V3] arm926: Flush the data cache before disabling it

[Albert ARIBAUD]

Hi Sughosh,

Le 14/01/2012 08:49, Sughosh Ganu a ?crit :
> The current implementation invalidates the cache instead of flushing
> it. This causes problems on platforms where the spl/u-boot is already
> loaded to the RAM, with caches enabled by a first stage bootloader.
>
> The V bit of the cp15's control register c1 is set to the value of
> VINITHI on reset. Do not clear this bit by default, as there are SOC's
> with no valid memory region at 0x0.
>
> Also fix the comments to match code.
>
> Signed-off-by: Sughosh Ganu<urwithsughosh@gmail.com>
> ---
>
> Changes since V2
> * Added code to invalidate I cache, based on review comment by Aneesh.
> * Fixed comments to match the code.
>
> Changes since V1
> * Added arm926 keyword to the subject line
> * Removed the superfluous setting of r0.
> * Fixed the comment to reflect the fact that V is not being cleared
>
>   arch/arm/cpu/arm926ejs/start.S |   14 +++++++++-----
>   1 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S
> index 6a09c02..91a9325 100644
> --- a/arch/arm/cpu/arm926ejs/start.S
> +++ b/arch/arm/cpu/arm926ejs/start.S
> @@ -355,17 +355,21 @@ _dynsym_start_ofs:
>    */
>   cpu_init_crit:
>   	/*
> -	 * flush v4 I/D caches
> +	 * flush D cache before disabling it
>   	 */
>   	mov	r0, #0
> -	mcr	p15, 0, r0, c7, c7, 0	/* flush v3/v4 cache */
> -	mcr	p15, 0, r0, c8, c7, 0	/* flush v4 TLB */
> +flush_dcache:
> +	mrc	p15, 0, r15, c7, c10, 3
> +	bne	flush_dcache
> +
> +	mcr	p15, 0, r0, c8, c7, 0	/* invalidate TLB */
> +	mcr	p15, 0, r0, c7, c5, 0	/* invalidate I Cache */
>
>   	/*
> -	 * disable MMU stuff and caches
> +	 * disable MMU and D cache, and enable I cache.
>   	 */
>   	mrc	p15, 0, r0, c1, c0, 0
> -	bic	r0, r0, #0x00002300	/* clear bits 13, 9:8 (--V- --RS) */
> +	bic	r0, r0, #0x00000300	/* clear bits 9:8 ( --RS) */

NAK--this alters the functioning of U-Boot for many boards in ways 
unpredictable. If you want to get this specific V change into ARM, then 
please also add code to set V in all relevant SoCs, or (better yet IMO) 
make "do not set V in cpu_init_crit" a config option and set it in the 
relevant SoCs or boards.

>   	bic	r0, r0, #0x00000087	/* clear bits 7, 2:0 (B--- -CAM) */
>   	orr	r0, r0, #0x00000002	/* set bit 2 (A) Align */
>   	orr	r0, r0, #0x00001000	/* set bit 12 (I) I-Cache */

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Albert ARIBAUD]

Le 12/01/2012 07:29, Sughosh Ganu a ?crit :
> On Thu Jan 12, 2012 at 06:56:01AM +0100, Christian Riesch wrote:
>> On Wednesday, January 11, 2012, Marek Vasut<marek.vasut@gmail.com>  wrote:
>
> <snip>
>
>>>> RBL executes an AIS script. Sughosh, could you please explain what your
>> AIS
>>>> does or how you create it?
>>>
>>> So basically, this SPL business can be avoided and this all can be done
>> in a
>>> standard way?
>>
>> I don't know, I never had to deal with booting from NAND. I was just
>> wondering what Sughosh's AIS is doing that he gets these SPL problems.
>> Christian
>
>    I have checked my ais ini file, and it does the normal pll/ddr
>    settings. I think it is the rbl which might be turning the cache
>    ON.

I do understand it is ROM code so no change can be done to it, but that 
a bootloader pass control to its payload with the cache still enabled 
and, worse yet, dirty, is Bad(tm).

Can the AIS not be augmented with instructions to flush and disable the 
cache?

Note: I do NOT intend to reject the U-Boot patch if the AIS can indeed 
be modified; I am just trying to apply the Postel principe fully, and 
while the patch would make U-Boot be (more) liberal in what it receives 
from the ROM bootloader, I would like the bootloader to be (more) 
conservative in what it gives U-Boot, i.e. give it a clean system with 
as little assumptions to make or constraints to respect.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/2 V3] arm926: Flush the data cache before disabling it

[Sughosh Ganu]

hi Albert,

On Sat Jan 14, 2012 at 10:02:16AM +0100, Albert ARIBAUD wrote:

<snip>

> >  	/*
> >-	 * disable MMU stuff and caches
> >+	 * disable MMU and D cache, and enable I cache.
> >  	 */
> >  	mrc	p15, 0, r0, c1, c0, 0
> >-	bic	r0, r0, #0x00002300	/* clear bits 13, 9:8 (--V- --RS) */
> >+	bic	r0, r0, #0x00000300	/* clear bits 9:8 ( --RS) */
> 
> NAK--this alters the functioning of U-Boot for many boards in ways
> unpredictable. If you want to get this specific V change into ARM,
> then please also add code to set V in all relevant SoCs, or (better
> yet IMO) make "do not set V in cpu_init_crit" a config option and
> set it in the relevant SoCs or boards.

  Ok, but the problem i have is that i don't have visibility into all
  the SoC's out there -- don't know what maps where. So i think it
  should be done by people using those specific SOC's. I can add a
  config option, and introduce it for my board/SOC. Will that be fine.

  In any case, i will split this patch into two, with the cache
  flushing part kept separate, as it fixes a real issue on my
  board. Will work out the setting of the V bit in a separate patch.

-sughosh

[U-Boot] [PATCH 1/2 V3] arm926: Flush the data cache before disabling it

[Albert ARIBAUD]

Hi Sughosh

Le 14/01/2012 10:21, Sughosh Ganu a ?crit :
> hi Albert,
>
> On Sat Jan 14, 2012 at 10:02:16AM +0100, Albert ARIBAUD wrote:
>
> <snip>
>
>>>   	/*
>>> -	 * disable MMU stuff and caches
>>> +	 * disable MMU and D cache, and enable I cache.
>>>   	 */
>>>   	mrc	p15, 0, r0, c1, c0, 0
>>> -	bic	r0, r0, #0x00002300	/* clear bits 13, 9:8 (--V- --RS) */
>>> +	bic	r0, r0, #0x00000300	/* clear bits 9:8 ( --RS) */
>>
>> NAK--this alters the functioning of U-Boot for many boards in ways
>> unpredictable. If you want to get this specific V change into ARM,
>> then please also add code to set V in all relevant SoCs, or (better
>> yet IMO) make "do not set V in cpu_init_crit" a config option and
>> set it in the relevant SoCs or boards.
>
>    Ok, but the problem i have is that i don't have visibility into all
>    the SoC's out there -- don't know what maps where. So i think it
>    should be done by people using those specific SOC's. I can add a
>    config option, and introduce it for my board/SOC. Will that be fine.

For me it is fine: this way, you won't affect code behavior for existing 
boards.

>    In any case, i will split this patch into two, with the cache
>    flushing part kept separate, as it fixes a real issue on my
>    board. Will work out the setting of the V bit in a separate patch.

This is a good idea too.

> -sughosh

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/2 V4] arm926: Flush the data cache before disabling it

[Sughosh Ganu]

The current implementation invalidates the cache instead of flushing
it. This causes problems on platforms where the spl/u-boot is already
loaded to the RAM, with caches enabled by a first stage bootloader.

Also fix the comments to match code.

Signed-off-by: Sughosh Ganu <urwithsughosh@gmail.com>
Cc: Albert Aribaud <albert.u.boot@aribaud.net>
Cc: Tom Rini <trini@ti.com>
---

Changes since V3
* Removed tampering of the V bit setting. Would be done in a separate
  patch on the lines of review comments by Albert.

Changes since V2
* Added code to invalidate I cache, based on review comment by Aneesh.
* Fixed comments to match the code.

Changes since V1
* Added arm926 keyword to the subject line
* Removed the superfluous setting of r0.
* Fixed the comment to reflect the fact that V is not being cleared

 arch/arm/cpu/arm926ejs/start.S |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S
index 6a09c02..d64165a 100644
--- a/arch/arm/cpu/arm926ejs/start.S
+++ b/arch/arm/cpu/arm926ejs/start.S
@@ -355,14 +355,18 @@ _dynsym_start_ofs:
  */
 cpu_init_crit:
 	/*
-	 * flush v4 I/D caches
+	 * flush D cache before disabling it
 	 */
 	mov	r0, #0
-	mcr	p15, 0, r0, c7, c7, 0	/* flush v3/v4 cache */
-	mcr	p15, 0, r0, c8, c7, 0	/* flush v4 TLB */
+flush_dcache:
+	mrc	p15, 0, r15, c7, c10, 3
+	bne	flush_dcache
+
+	mcr	p15, 0, r0, c8, c7, 0	/* invalidate TLB */
+	mcr	p15, 0, r0, c7, c5, 0	/* invalidate I Cache */
 
 	/*
-	 * disable MMU stuff and caches
+	 * disable MMU and D cache, and enable I cache.
 	 */
 	mrc	p15, 0, r0, c1, c0, 0
 	bic	r0, r0, #0x00002300	/* clear bits 13, 9:8 (--V- --RS) */
-- 
1.7.5.4

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Christian Riesch]

Hi Albert,

On Saturday, January 14, 2012, Albert ARIBAUD <albert.u.boot@aribaud.net>
wrote:
> Le 12/01/2012 07:29, Sughosh Ganu a ?crit :
>>
>> On Thu Jan 12, 2012 at 06:56:01AM +0100, Christian Riesch wrote:
>>>
>>> On Wednesday, January 11, 2012, Marek Vasut<marek.vasut@gmail.com>
 wrote:
>>
>> <snip>
>>
>>>>> RBL executes an AIS script. Sughosh, could you please explain what
your
>>>
>>> AIS
>>>>>
>>>>> does or how you create it?
>>>>
>>>> So basically, this SPL business can be avoided and this all can be done
>>>
>>> in a
>>>>
>>>> standard way?
>>>
>>> I don't know, I never had to deal with booting from NAND. I was just
>>> wondering what Sughosh's AIS is doing that he gets these SPL problems.
>>> Christian
>>
>>   I have checked my ais ini file, and it does the normal pll/ddr
>>   settings. I think it is the rbl which might be turning the cache
>>   ON.
>
> I do understand it is ROM code so no change can be done to it, but that a
bootloader pass control to its payload with the cache still enabled and,
worse yet, dirty, is Bad(tm).
>
> Can the AIS not be augmented with instructions to flush and disable the
cache?

I checked the AIS documentation and I don't think this can be done... AIS
has a command to write arbitrary memory regions, but not registers.
Christian

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Christian Riesch]

Hi Sughosh,

On Thursday, January 12, 2012, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> hi Christian,
>
> On Thu Jan 12, 2012 at 03:04:37PM +0100, Christian Riesch wrote:
>> On Thu, Jan 12, 2012 at 2:53 PM, Sughosh Ganu <urwithsughosh@gmail.com>
wrote:
>> > On Thu Jan 12, 2012 at 01:03:05PM +0100, Christian Riesch wrote:
>
> <snip>
>
>> >>
>> >> Since all my tests were successful I wonder what issues did you have
>> >> with the cache? Can you describe the problems you had? I think the
>> >> difference is that you are booting from NAND and have an OMAP-L138,
>> >> whereas I boot from SPI (on the da850evm) or from NOR (on calimain)
>> >> and have an AM1808 on both boards, right?
>> >
>> >  Thanks a lot for all the testing. One difference i think we have on
>> >  these boards and hawkboard, is that on hawkboard, the rbl configures
>> >  the memory and loads the target image(spl in this case) directly to
>> >  the ram. Looking at da850evm's lds file, it looks like the spl
>> >  gets loaded to the sram, configures dram and then copies u-boot to
>> >  the target load address.
>>
>> This is only true if the SPL is actually used. Have a closer look at
>> my test report, I tested three different methods:
>>
>> 1) The first test was done with the SPL and yes, here the RBL loads
>> the SPL into SRAM, initializes DDR memory and then copies u-boot.bin
>> to DDR memory.
>> 2) The second test was done with TI's UBL. Here, the RBL loads the UBL
>> into SRAM, the UBL initializes DDR memory and then copies u-boot.bin
>> to DDR memory.
>> 3) The third test was done without SPL and without UBL: Here the DDR
>> memory init is in the AIS, so in fact the RBL does memory
>> initialization and then RBL loads u-boot.bin to DDR memory. This is
>> the same case that you have on the hawkboard (only that you have the
>> OMAP-L138 and NAND flash instead) and it works for me regardless of
>> your patch.
>
>  Yes, the third case is similar to the one used in hawkboard. I'm not
>  sure as to why it causes a problem on my board, though i'm not sure
>  if we can compare the two cases, as we have different rbl's. It
>  could be that the rbl used on hawkboard initialises the caches, as
>  the caches are off by default on reset.
>
>  Here are the values i use in my ini file for ddr init.
>
> [EMIF3DDR]
> PLL1CFG0 = 0x15010001
> PLL1CFG1 = 0x00000002
>
> DDRPHYC1R = 0x00000043
> SDCR = 0x00134632
> SDTIMR = 0x26492a09
> SDTIMR2 = 0x7d13c722
> SDRCR = 0x00000249
> CLK2XSRC = 0x00000000
>

Just for curiosity, could you please send the full ini file?
Thanks, Christian

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Sughosh Ganu]

hi Christian,

On Sat Jan 14, 2012 at 06:20:06PM +0100, Christian Riesch wrote:
> Hi Sughosh,

<snip>

> On Thursday, January 12, 2012, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> >> 1) The first test was done with the SPL and yes, here the RBL loads
> >> the SPL into SRAM, initializes DDR memory and then copies u-boot.bin
> >> to DDR memory.
> >> 2) The second test was done with TI's UBL. Here, the RBL loads the UBL
> >> into SRAM, the UBL initializes DDR memory and then copies u-boot.bin
> >> to DDR memory.
> >> 3) The third test was done without SPL and without UBL: Here the DDR
> >> memory init is in the AIS, so in fact the RBL does memory
> >> initialization and then RBL loads u-boot.bin to DDR memory. This is
> >> the same case that you have on the hawkboard (only that you have the
> >> OMAP-L138 and NAND flash instead) and it works for me regardless of
> >> your patch.
> >
> >  Yes, the third case is similar to the one used in hawkboard. I'm not
> >  sure as to why it causes a problem on my board, though i'm not sure
> >  if we can compare the two cases, as we have different rbl's. It
> >  could be that the rbl used on hawkboard initialises the caches, as
> >  the caches are off by default on reset.
> >
> >  Here are the values i use in my ini file for ddr init.
> >
> > [EMIF3DDR]
> > PLL1CFG0 = 0x15010001
> > PLL1CFG1 = 0x00000002
> >
> > DDRPHYC1R = 0x00000043
> > SDCR = 0x00134632
> > SDTIMR = 0x26492a09
> > SDTIMR2 = 0x7d13c722
> > SDRCR = 0x00000249
> > CLK2XSRC = 0x00000000

Here it is.
[General]
busWidth=8

BootMode=NAND

crcCheckType=NO_CRC

[PLL0CONFIG]
PLL0CFG0 = 0x00180001
PLL0CFG1 = 0x00000205

[EMIF3DDR]
PLL1CFG0 = 0x15010001
PLL1CFG1 = 0x00000002
DDRPHYC1R = 0x00000043
SDCR = 0x00134632
SDTIMR = 0x26492a09
SDTIMR2 = 0x7d13c722
SDRCR = 0x00000249
CLK2XSRC = 0x00000000
[ARM_EMIF3DDR_PATCHFXN]

-sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Heiko Schocher]

Hello Sughosh,

Sughosh Ganu wrote:
> hi Heiko,
> 
> On Fri Jan 13, 2012 at 04:29:29PM +0100, Heiko Schocher wrote:
>> Hello Sugosh,
>>
>> Sughosh Ganu wrote:
>>> hi Christian,
>>>
>>> On Fri Jan 13, 2012 at 09:06:26AM +0100, Christian Riesch wrote:
>>>> Hi Sughosh,
>>>> I had a look at the patch and I tried to understand what's going on
>>>> here (I must confess that I didn't know anything about this cache
>>>> stuff).
>>>   Ok, thanks for taking time off to understand this issue.
>>>
>>>> On Tue, Jan 10, 2012 at 7:12 PM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
>>>>> The current implementation invalidates the cache instead of flushing
>>>>> it. This causes problems on platforms where the spl/u-boot is already
>>>>> loaded to the RAM, with caches enabled by a first stage bootloader.
>> Hmm.. how did u-boot work on such boards? How can u-boot work with D-Cache
>> enabled, if u-boot is not initializing it? (And I think, on davinci SoC
>> we have a none working uboot ethernet driver if d-cache is enabled too).
>> There must be a page_table in DRAM for using D-Cache in U-Boot, if u-boot
>> don't initialize it, it maybe overrides it ... or miss I something?
> 
>   Well, there is some data in the cache, which if not flushed creates
>   problems on my board. I get the board to boot just by commenting out
>   cpu_init_crit call. My hypothesis that the D-cache is enabled is

That is what I not understand, because, if the RBL really enables
the D-Cache, how could u-boot work, if it not disables it!

>   simply because cache invalidation followed by cache disabling breaks
>   the board, while flushing it prior to disabling gets it to boot
>   fine. This(invalidation) would not have been a problem if the cache
>   was in the disabled state.
> 
>> Are you sure, the RBL enables the D-Cache on your board? Nevertheless,
>> I think, we must disable the D-Cache here with "cleaning" it (as your
>> patch did) instead only invalidating it, as current code did.
> 
>   I am not sure, this is just my guess. By default, the caches are
>   disabled on reset, so the only possible source is the rbl. But I
>   don't have access to the rbl code to say for sure. Unfortunately i
>   also don't have JTAG or any other debugger to dig more into

:-( You maybe could read the interesting values, and store them some-
where, and print it later?

>   this. But yes, like you mention, we must be cleaning the cache
>   before disabling it, so this fix is correct.

Yes, the fix is correct, but we should try to understand, what is
really the problem on your hw!

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Tom Rini]

On Fri, Jan 13, 2012 at 10:38 AM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> hi Heiko,
>
> On Fri Jan 13, 2012 at 04:29:29PM +0100, Heiko Schocher wrote:
>> Hello Sugosh,
>>
>> Sughosh Ganu wrote:
>> > hi Christian,
>> >
>> > On Fri Jan 13, 2012 at 09:06:26AM +0100, Christian Riesch wrote:
>> >> Hi Sughosh,
>> >> I had a look at the patch and I tried to understand what's going on
>> >> here (I must confess that I didn't know anything about this cache
>> >> stuff).
>> >
>> > ? Ok, thanks for taking time off to understand this issue.
>> >
>> >> On Tue, Jan 10, 2012 at 7:12 PM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
>> >>> The current implementation invalidates the cache instead of flushing
>> >>> it. This causes problems on platforms where the spl/u-boot is already
>> >>> loaded to the RAM, with caches enabled by a first stage bootloader.
>>
>> Hmm.. how did u-boot work on such boards? How can u-boot work with D-Cache
>> enabled, if u-boot is not initializing it? (And I think, on davinci SoC
>> we have a none working uboot ethernet driver if d-cache is enabled too).
>> There must be a page_table in DRAM for using D-Cache in U-Boot, if u-boot
>> don't initialize it, it maybe overrides it ... or miss I something?
>
> ?Well, there is some data in the cache, which if not flushed creates
> ?problems on my board. I get the board to boot just by commenting out
> ?cpu_init_crit call. My hypothesis that the D-cache is enabled is
> ?simply because cache invalidation followed by cache disabling breaks
> ?the board, while flushing it prior to disabling gets it to boot
> ?fine. This(invalidation) would not have been a problem if the cache
> ?was in the disabled state.

Putting my TI hat on, I've confirmed with the RBL folks that they
aren't turning on ICACHE/DCACHE.

-- 
Tom

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Heiko Schocher]

Hello Tom,

Tom Rini wrote:
> On Fri, Jan 13, 2012 at 10:38 AM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
>> hi Heiko,
>>
>> On Fri Jan 13, 2012 at 04:29:29PM +0100, Heiko Schocher wrote:
>>> Hello Sugosh,
>>>
>>> Sughosh Ganu wrote:
>>>> hi Christian,
>>>>
>>>> On Fri Jan 13, 2012 at 09:06:26AM +0100, Christian Riesch wrote:
>>>>> Hi Sughosh,
>>>>> I had a look at the patch and I tried to understand what's going on
>>>>> here (I must confess that I didn't know anything about this cache
>>>>> stuff).
>>>>   Ok, thanks for taking time off to understand this issue.
>>>>
>>>>> On Tue, Jan 10, 2012 at 7:12 PM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
>>>>>> The current implementation invalidates the cache instead of flushing
>>>>>> it. This causes problems on platforms where the spl/u-boot is already
>>>>>> loaded to the RAM, with caches enabled by a first stage bootloader.
>>> Hmm.. how did u-boot work on such boards? How can u-boot work with D-Cache
>>> enabled, if u-boot is not initializing it? (And I think, on davinci SoC
>>> we have a none working uboot ethernet driver if d-cache is enabled too).
>>> There must be a page_table in DRAM for using D-Cache in U-Boot, if u-boot
>>> don't initialize it, it maybe overrides it ... or miss I something?
>>  Well, there is some data in the cache, which if not flushed creates
>>  problems on my board. I get the board to boot just by commenting out
>>  cpu_init_crit call. My hypothesis that the D-cache is enabled is
>>  simply because cache invalidation followed by cache disabling breaks
>>  the board, while flushing it prior to disabling gets it to boot
>>  fine. This(invalidation) would not have been a problem if the cache
>>  was in the disabled state.
> 
> Putting my TI hat on, I've confirmed with the RBL folks that they
> aren't turning on ICACHE/DCACHE.

That was my thought too, thanks for the clarification!

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Sughosh Ganu]

On Mon Jan 16, 2012 at 10:57:05AM -0700, Tom Rini wrote:
> On Fri, Jan 13, 2012 at 10:38 AM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> > hi Heiko,
> >
> > On Fri Jan 13, 2012 at 04:29:29PM +0100, Heiko Schocher wrote:
> >> Hello Sugosh,
> >>
> >> Sughosh Ganu wrote:
> >> > hi Christian,
> >> >
> >> > On Fri Jan 13, 2012 at 09:06:26AM +0100, Christian Riesch wrote:
> >> >> Hi Sughosh,
> >> >> I had a look at the patch and I tried to understand what's going on
> >> >> here (I must confess that I didn't know anything about this cache
> >> >> stuff).
> >> >
> >> > ? Ok, thanks for taking time off to understand this issue.
> >> >
> >> >> On Tue, Jan 10, 2012 at 7:12 PM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> >> >>> The current implementation invalidates the cache instead of flushing
> >> >>> it. This causes problems on platforms where the spl/u-boot is already
> >> >>> loaded to the RAM, with caches enabled by a first stage bootloader.
> >>
> >> Hmm.. how did u-boot work on such boards? How can u-boot work with D-Cache
> >> enabled, if u-boot is not initializing it? (And I think, on davinci SoC
> >> we have a none working uboot ethernet driver if d-cache is enabled too).
> >> There must be a page_table in DRAM for using D-Cache in U-Boot, if u-boot
> >> don't initialize it, it maybe overrides it ... or miss I something?
> >
> > ?Well, there is some data in the cache, which if not flushed creates
> > ?problems on my board. I get the board to boot just by commenting out
> > ?cpu_init_crit call. My hypothesis that the D-cache is enabled is
> > ?simply because cache invalidation followed by cache disabling breaks
> > ?the board, while flushing it prior to disabling gets it to boot
> > ?fine. This(invalidation) would not have been a problem if the cache
> > ?was in the disabled state.
> 
> Putting my TI hat on, I've confirmed with the RBL folks that they
> aren't turning on ICACHE/DCACHE.

  Well, i'm not sure then why does the cache invalidation cause a
  problem on my platform. Btw, will this patch be
  accepted. Irrespective of the cache state on my board, it is not a
  wrong fix, and moreover results in the booting of my board.

  I haven't yet tried out reading the cache bit state in the spl. Will
  try out this experiment in a day or two, and share the results.

-sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Tom Rini]

On Mon, Jan 16, 2012 at 11:46 PM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
> On Mon Jan 16, 2012 at 10:57:05AM -0700, Tom Rini wrote:
>> On Fri, Jan 13, 2012 at 10:38 AM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
>> > hi Heiko,
>> >
>> > On Fri Jan 13, 2012 at 04:29:29PM +0100, Heiko Schocher wrote:
>> >> Hello Sugosh,
>> >>
>> >> Sughosh Ganu wrote:
>> >> > hi Christian,
>> >> >
>> >> > On Fri Jan 13, 2012 at 09:06:26AM +0100, Christian Riesch wrote:
>> >> >> Hi Sughosh,
>> >> >> I had a look at the patch and I tried to understand what's going on
>> >> >> here (I must confess that I didn't know anything about this cache
>> >> >> stuff).
>> >> >
>> >> > ? Ok, thanks for taking time off to understand this issue.
>> >> >
>> >> >> On Tue, Jan 10, 2012 at 7:12 PM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:
>> >> >>> The current implementation invalidates the cache instead of flushing
>> >> >>> it. This causes problems on platforms where the spl/u-boot is already
>> >> >>> loaded to the RAM, with caches enabled by a first stage bootloader.
>> >>
>> >> Hmm.. how did u-boot work on such boards? How can u-boot work with D-Cache
>> >> enabled, if u-boot is not initializing it? (And I think, on davinci SoC
>> >> we have a none working uboot ethernet driver if d-cache is enabled too).
>> >> There must be a page_table in DRAM for using D-Cache in U-Boot, if u-boot
>> >> don't initialize it, it maybe overrides it ... or miss I something?
>> >
>> > ?Well, there is some data in the cache, which if not flushed creates
>> > ?problems on my board. I get the board to boot just by commenting out
>> > ?cpu_init_crit call. My hypothesis that the D-cache is enabled is
>> > ?simply because cache invalidation followed by cache disabling breaks
>> > ?the board, while flushing it prior to disabling gets it to boot
>> > ?fine. This(invalidation) would not have been a problem if the cache
>> > ?was in the disabled state.
>>
>> Putting my TI hat on, I've confirmed with the RBL folks that they
>> aren't turning on ICACHE/DCACHE.
>
> ?Well, i'm not sure then why does the cache invalidation cause a
> ?problem on my platform. Btw, will this patch be
> ?accepted. Irrespective of the cache state on my board, it is not a
> ?wrong fix, and moreover results in the booting of my board.
>
> ?I haven't yet tried out reading the cache bit state in the spl. Will
> ?try out this experiment in a day or two, and share the results.

I think someone needs to look over this init code very carefully.  If
I can summarize from memory quickly, when we enable the
CP_CRITICAL_INITS code for everyone that exposed a problem on the
hawkboard that _looks_like_ DCACHE is enabled by RBL as changing the
code from doing an invalidate to a flush+invalidate fixes a problem.
But the manual says we should be doing flush+invalidate anyhow and the
RBL code is not turning on DCACHE.  Maybe we've got something else
being done not as the manual says and that's tickling another issue?

-- 
Tom

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Sughosh Ganu]

On Tue Jan 17, 2012 at 08:27:58AM -0700, Tom Rini wrote:
> On Mon, Jan 16, 2012 at 11:46 PM, Sughosh Ganu <urwithsughosh@gmail.com> wrote:

> >> >> Hmm.. how did u-boot work on such boards? How can u-boot work with D-Cache
> >> >> enabled, if u-boot is not initializing it? (And I think, on davinci SoC
> >> >> we have a none working uboot ethernet driver if d-cache is enabled too).
> >> >> There must be a page_table in DRAM for using D-Cache in U-Boot, if u-boot
> >> >> don't initialize it, it maybe overrides it ... or miss I something?
> >> >
> >> > ?Well, there is some data in the cache, which if not flushed creates
> >> > ?problems on my board. I get the board to boot just by commenting out
> >> > ?cpu_init_crit call. My hypothesis that the D-cache is enabled is
> >> > ?simply because cache invalidation followed by cache disabling breaks
> >> > ?the board, while flushing it prior to disabling gets it to boot
> >> > ?fine. This(invalidation) would not have been a problem if the cache
> >> > ?was in the disabled state.
> >>
> >> Putting my TI hat on, I've confirmed with the RBL folks that they
> >> aren't turning on ICACHE/DCACHE.
> >
> > ?Well, i'm not sure then why does the cache invalidation cause a
> > ?problem on my platform. Btw, will this patch be
> > ?accepted. Irrespective of the cache state on my board, it is not a
> > ?wrong fix, and moreover results in the booting of my board.
> >
> > ?I haven't yet tried out reading the cache bit state in the spl. Will
> > ?try out this experiment in a day or two, and share the results.
> 
> I think someone needs to look over this init code very carefully.  If
> I can summarize from memory quickly, when we enable the
> CP_CRITICAL_INITS code for everyone that exposed a problem on the
> hawkboard that _looks_like_ DCACHE is enabled by RBL as changing the
> code from doing an invalidate to a flush+invalidate fixes a problem.
> But the manual says we should be doing flush+invalidate anyhow and the
> RBL code is not turning on DCACHE.  Maybe we've got something else
> being done not as the manual says and that's tickling another issue?
  
  Tried a few things on my end.
  * Read the D-cache value in the spl, and confirmed that the data
    cache is indeed not enabled.
    
  * Enabled the data cache explicitly in cpu_init_crit, and booted
    u-boot. u-boot comes up fine, and trying a ping switches off the
    data cache with the warning message.

  So it definitely looks like the cache is not enabled. But still not
  able to figure out as to why does the flushing operation
  help. Really need to get a debugger :)

-sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Aneesh V]

Hi Sughosh,

On Thursday 19 January 2012 12:23 PM, Sughosh Ganu wrote:
> On Tue Jan 17, 2012 at 08:27:58AM -0700, Tom Rini wrote:
>> On Mon, Jan 16, 2012 at 11:46 PM, Sughosh Ganu<urwithsughosh@gmail.com>  wrote:
>
>>>>>> Hmm.. how did u-boot work on such boards? How can u-boot work with D-Cache
>>>>>> enabled, if u-boot is not initializing it? (And I think, on davinci SoC
>>>>>> we have a none working uboot ethernet driver if d-cache is enabled too).
>>>>>> There must be a page_table in DRAM for using D-Cache in U-Boot, if u-boot
>>>>>> don't initialize it, it maybe overrides it ... or miss I something?
>>>>>
>>>>>   Well, there is some data in the cache, which if not flushed creates
>>>>>   problems on my board. I get the board to boot just by commenting out
>>>>>   cpu_init_crit call. My hypothesis that the D-cache is enabled is
>>>>>   simply because cache invalidation followed by cache disabling breaks
>>>>>   the board, while flushing it prior to disabling gets it to boot
>>>>>   fine. This(invalidation) would not have been a problem if the cache
>>>>>   was in the disabled state.
>>>>
>>>> Putting my TI hat on, I've confirmed with the RBL folks that they
>>>> aren't turning on ICACHE/DCACHE.
>>>
>>>   Well, i'm not sure then why does the cache invalidation cause a
>>>   problem on my platform. Btw, will this patch be
>>>   accepted. Irrespective of the cache state on my board, it is not a
>>>   wrong fix, and moreover results in the booting of my board.
>>>
>>>   I haven't yet tried out reading the cache bit state in the spl. Will
>>>   try out this experiment in a day or two, and share the results.
>>
>> I think someone needs to look over this init code very carefully.  If
>> I can summarize from memory quickly, when we enable the
>> CP_CRITICAL_INITS code for everyone that exposed a problem on the
>> hawkboard that _looks_like_ DCACHE is enabled by RBL as changing the
>> code from doing an invalidate to a flush+invalidate fixes a problem.
>> But the manual says we should be doing flush+invalidate anyhow and the
>> RBL code is not turning on DCACHE.  Maybe we've got something else
>> being done not as the manual says and that's tickling another issue?
>
>    Tried a few things on my end.
>    * Read the D-cache value in the spl, and confirmed that the data
>      cache is indeed not enabled.

What is the value of the B bit in CP15 SCR register? I wonder if RBL is
doing all the IMB operations required after copying the SPL image and
before executing it. IMB is required for consistency between data and
instruction sides. For instance after copying the SPL, the memory and
instruction cache could be incoherent. So, instruction cache needs to
be invalidated. In fact more needs to be done and there is an entire
chapter in the ARM926EJS TRM that discusses this (Chapter 9 -
Instruction Memory Barrier). Here is the key excerpt:

9.2 IMB operation
To ensure consistency between data and instruction sides, you must take
the following
steps:
1. Clean the DCache
2. Drain the write buffer
3. Synchronize data and instruction streams in level two AHB subsystems
4. Invalidate the ICache on page 9-4
5. Flush the prefetch buffer on page 9-4.

Ideally RBL should be doing all this before jumping to SPL. But, I
wonder if anything is missing in RBL and was getting done as a
side-effect of your flush.

Just a thought. Do you care to try 2-5 in SPL and see if it makes any
difference?(and avoid 1. in fact 1 seems to be the least useful thing
in our case!)

>
>    * Enabled the data cache explicitly in cpu_init_crit, and booted
>      u-boot. u-boot comes up fine, and trying a ping switches off the
>      data cache with the warning message.

How are you enabling D-cache. Please note that just setting C bit
doesn't enable D-cache. MMU needs to be enabled (M bit) for D-cache to
be enabled. MMU in turn needs page-table. I wonder if you are doing all
this in cpu_init_crit()?

br,
Aneesh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Christian Riesch]

Hi Aneesh,

On Thu, Jan 19, 2012 at 11:17 AM, Aneesh V <aneesh@ti.com> wrote:
> On Thursday 19 January 2012 12:23 PM, Sughosh Ganu wrote:
>> ? Tried a few things on my end.
>> ? * Read the D-cache value in the spl, and confirmed that the data
>> ? ? cache is indeed not enabled.
> What is the value of the B bit in CP15 SCR register? I wonder if RBL is
> doing all the IMB operations required after copying the SPL image and
> before executing it. IMB is required for consistency between data and
> instruction sides.

Only if caches are used, right? Or also without caches?
Tom wrote that RBL does not turn on cache.
Regards, Christian

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Aneesh V]

On Thursday 19 January 2012 05:00 PM, Christian Riesch wrote:
> Hi Aneesh,
>
> On Thu, Jan 19, 2012 at 11:17 AM, Aneesh V<aneesh@ti.com>  wrote:
>> On Thursday 19 January 2012 12:23 PM, Sughosh Ganu wrote:
>>>    Tried a few things on my end.
>>>    * Read the D-cache value in the spl, and confirmed that the data
>>>      cache is indeed not enabled.
>> What is the value of the B bit in CP15 SCR register? I wonder if RBL is
>> doing all the IMB operations required after copying the SPL image and
>> before executing it. IMB is required for consistency between data and
>> instruction sides.
>
> Only if caches are used, right? Or also without caches?
> Tom wrote that RBL does not turn on cache.
> Regards, Christian

Only D-cache seems to be disabled in this case. I-cache and Write
buffer are likely to be enabled. If so, all the IMB operations except
the data-cache flushing are still relevant.

br,
Aneesh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Christian Riesch]

On Thu, Jan 19, 2012 at 12:54 PM, Aneesh V <aneesh@ti.com> wrote:
> On Thursday 19 January 2012 05:00 PM, Christian Riesch wrote:
>> On Thu, Jan 19, 2012 at 11:17 AM, Aneesh V<aneesh@ti.com> ?wrote:
>>> On Thursday 19 January 2012 12:23 PM, Sughosh Ganu wrote:
>>>> ? Tried a few things on my end.
>>>> ? * Read the D-cache value in the spl, and confirmed that the data
>>>> ? ? cache is indeed not enabled.
>>>
>>> What is the value of the B bit in CP15 SCR register? I wonder if RBL is
>>> doing all the IMB operations required after copying the SPL image and
>>> before executing it. IMB is required for consistency between data and
>>> instruction sides.
>>
>> Only if caches are used, right? Or also without caches?
>> Tom wrote that RBL does not turn on cache.
>> Regards, Christian
>
> Only D-cache seems to be disabled in this case. I-cache and Write
> buffer are likely to be enabled. If so, all the IMB operations except
> the data-cache flushing are still relevant.

Tom, when you wrote that RBL does not turn on caches, did you mean it
never turns it on or it turns some of them on and turns them off
before exit?
Christian

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Aneesh V]

Sughosh,

On Friday 20 January 2012 12:58 PM, Christian Riesch wrote:
> On Thu, Jan 19, 2012 at 12:54 PM, Aneesh V<aneesh@ti.com>  wrote:
>> On Thursday 19 January 2012 05:00 PM, Christian Riesch wrote:
>>> On Thu, Jan 19, 2012 at 11:17 AM, Aneesh V<aneesh@ti.com>    wrote:
>>>> On Thursday 19 January 2012 12:23 PM, Sughosh Ganu wrote:
>>>>>    Tried a few things on my end.
>>>>>    * Read the D-cache value in the spl, and confirmed that the data
>>>>>      cache is indeed not enabled.
>>>>
>>>> What is the value of the B bit in CP15 SCR register? I wonder if RBL is
>>>> doing all the IMB operations required after copying the SPL image and
>>>> before executing it. IMB is required for consistency between data and
>>>> instruction sides.
>>>
>>> Only if caches are used, right? Or also without caches?
>>> Tom wrote that RBL does not turn on cache.
>>> Regards, Christian
>>
>> Only D-cache seems to be disabled in this case. I-cache and Write
>> buffer are likely to be enabled. If so, all the IMB operations except
>> the data-cache flushing are still relevant.
>
> Tom, when you wrote that RBL does not turn on caches, did you mean it
> never turns it on or it turns some of them on and turns them off
> before exit?
> Christian

Can you send the value of SCR you found at SPL entry? This will clarify
what's enabled and what's not.

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Christian Riesch]

Hi Aneesh,

On Fri, Jan 20, 2012 at 9:52 AM, Aneesh V <aneesh@ti.com> wrote:
> Sughosh,
[...]
> Can you send the value of SCR you found at SPL entry? This will clarify
> what's enabled and what's not.

I would like to try that on my board as well for comparison. Could you
please tell me how this register can be read? In the ARM manuals SCR
seems to have several meanings... Thank you!
Regards, Christian

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[James W.]

so sorry to you,
i think it's difference between DISABLE and Flush.

be careful.


On Wed, Jan 11, 2012 at 2:12 AM, Sughosh Ganu <urwithsughosh@gmail.com>wrote:

> The current implementation invalidates the cache instead of flushing
> it. This causes problems on platforms where the spl/u-boot is already
> loaded to the RAM, with caches enabled by a first stage bootloader.
>
> The V bit of the cp15's control register c1 is set to the value of
> VINITHI on reset. Do not clear this bit by default, as there are SOC's
> with no valid memory region at 0x0.
>
> Signed-off-by: Sughosh Ganu <urwithsughosh@gmail.com>
> Cc: Albert Aribaud <albert.u.boot@aribaud.net>
> ---
>
> Changes since V1
> * Added arm926 keyword to the subject line
> * Removed the superfluous setting of r0.
> * Fixed the comment to reflect the fact that V is not being cleared
>
>  arch/arm/cpu/arm926ejs/start.S |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/cpu/arm926ejs/start.S
> b/arch/arm/cpu/arm926ejs/start.S
> index 6a09c02..6e261c2 100644
> --- a/arch/arm/cpu/arm926ejs/start.S
> +++ b/arch/arm/cpu/arm926ejs/start.S
> @@ -355,17 +355,20 @@ _dynsym_start_ofs:
>  */
>  cpu_init_crit:
>        /*
> -        * flush v4 I/D caches
> +        * flush D cache before disabling it
>         */
>        mov     r0, #0
> -       mcr     p15, 0, r0, c7, c7, 0   /* flush v3/v4 cache */
> +flush_dcache:
> +       mrc     p15, 0, r15, c7, c10, 3
> +       bne     flush_dcache
> +
>        mcr     p15, 0, r0, c8, c7, 0   /* flush v4 TLB */
>
>        /*
>         * disable MMU stuff and caches
>         */
>        mrc     p15, 0, r0, c1, c0, 0
> -       bic     r0, r0, #0x00002300     /* clear bits 13, 9:8 (--V- --RS)
> */
> +       bic     r0, r0, #0x00000300     /* clear bits 9:8 ( --RS) */
>        bic     r0, r0, #0x00000087     /* clear bits 7, 2:0 (B--- -CAM) */
>        orr     r0, r0, #0x00000002     /* set bit 2 (A) Align */
>        orr     r0, r0, #0x00001000     /* set bit 12 (I) I-Cache */
> --
> 1.7.5.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Tom Rini]

On Fri, Jan 20, 2012 at 12:28 AM, Christian Riesch
<christian.riesch@omicron.at> wrote:
> On Thu, Jan 19, 2012 at 12:54 PM, Aneesh V <aneesh@ti.com> wrote:
>> On Thursday 19 January 2012 05:00 PM, Christian Riesch wrote:
>>> On Thu, Jan 19, 2012 at 11:17 AM, Aneesh V<aneesh@ti.com> ?wrote:
>>>> On Thursday 19 January 2012 12:23 PM, Sughosh Ganu wrote:
>>>>> ? Tried a few things on my end.
>>>>> ? * Read the D-cache value in the spl, and confirmed that the data
>>>>> ? ? cache is indeed not enabled.
>>>>
>>>> What is the value of the B bit in CP15 SCR register? I wonder if RBL is
>>>> doing all the IMB operations required after copying the SPL image and
>>>> before executing it. IMB is required for consistency between data and
>>>> instruction sides.
>>>
>>> Only if caches are used, right? Or also without caches?
>>> Tom wrote that RBL does not turn on cache.
>>> Regards, Christian
>>
>> Only D-cache seems to be disabled in this case. I-cache and Write
>> buffer are likely to be enabled. If so, all the IMB operations except
>> the data-cache flushing are still relevant.
>
> Tom, when you wrote that RBL does not turn on caches, did you mean it
> never turns it on or it turns some of them on and turns them off
> before exit?

I'm away from the code atm (and when I get back I can point Aneesh at
it as well), but DCACHE is never enabled and I'm thinking ICACHE too.

-- 
Tom

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Aneesh V]

On Friday 20 January 2012 02:51 PM, Christian Riesch wrote:
> Hi Aneesh,
>
> On Fri, Jan 20, 2012 at 9:52 AM, Aneesh V<aneesh@ti.com>  wrote:
>> Sughosh,
> [...]
>> Can you send the value of SCR you found at SPL entry? This will clarify
>> what's enabled and what's not.
>
> I would like to try that on my board as well for comparison. Could you
> please tell me how this register can be read? In the ARM manuals SCR
> seems to have several meanings... Thank you!
> Regards, Christian

If you have a JTAG based debugger that has the capability of displaying
CP15 registers, look for "CP15 System Control Register". Otherwise you
will have to read it using an assembly instruction like below:

	mrc	p15, 0, r0, c1, c0, 0

After this instruction r0 will contain the SCR value. arm926ejs/start.S
has this instruction at line #367. You can put a breakpoint after this
and look at r0.

br,
Aneesh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Christian Riesch]

Hi Aneesh,

On Fri, Jan 20, 2012 at 1:13 PM, Aneesh V <aneesh@ti.com> wrote:
> On Friday 20 January 2012 02:51 PM, Christian Riesch wrote:
>> On Fri, Jan 20, 2012 at 9:52 AM, Aneesh V<aneesh@ti.com> ?wrote:
>>> Sughosh,
>>
>> [...]
>>>
>>> Can you send the value of SCR you found at SPL entry? This will clarify
>>> what's enabled and what's not.
>>
>> I would like to try that on my board as well for comparison. Could you
>> please tell me how this register can be read? In the ARM manuals SCR
>> seems to have several meanings... Thank you!
>> Regards, Christian
>
> If you have a JTAG based debugger that has the capability of displaying
> CP15 registers, look for "CP15 System Control Register". Otherwise you
> will have to read it using an assembly instruction like below:
>
>
> ? ? ? ?mrc ? ? p15, 0, r0, c1, c0, 0
>
> After this instruction r0 will contain the SCR value. arm926ejs/start.S
> has this instruction at line #367. You can put a breakpoint after this
> and look at r0.

Thank you!

I don't have a JTAG debugger so I stored it in a register, pushed it
later to the stack and then read it with md.l from the memory. I tried
it on my custom board (AM1808 SoC, direct boot from NOR flash) and on
both the da850evm (with AM1808 SoC, AIS boot from SPI flash). The
result was the same for both cases, 0x00052078. So DCache and ICache
are disabled after the RBL.
Regards, Christian

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Aneesh V]

Hi Christian,

On Friday 20 January 2012 06:18 PM, Christian Riesch wrote:
> Hi Aneesh,
>
> On Fri, Jan 20, 2012 at 1:13 PM, Aneesh V<aneesh@ti.com>  wrote:
>> On Friday 20 January 2012 02:51 PM, Christian Riesch wrote:
>>> On Fri, Jan 20, 2012 at 9:52 AM, Aneesh V<aneesh@ti.com>    wrote:
>>>> Sughosh,
>>>
>>> [...]
>>>>
>>>> Can you send the value of SCR you found at SPL entry? This will clarify
>>>> what's enabled and what's not.
>>>
>>> I would like to try that on my board as well for comparison. Could you
>>> please tell me how this register can be read? In the ARM manuals SCR
>>> seems to have several meanings... Thank you!
>>> Regards, Christian
>>
>> If you have a JTAG based debugger that has the capability of displaying
>> CP15 registers, look for "CP15 System Control Register". Otherwise you
>> will have to read it using an assembly instruction like below:
>>
>>
>>         mrc     p15, 0, r0, c1, c0, 0
>>
>> After this instruction r0 will contain the SCR value. arm926ejs/start.S
>> has this instruction at line #367. You can put a breakpoint after this
>> and look at r0.
>
> Thank you!
>
> I don't have a JTAG debugger so I stored it in a register, pushed it
> later to the stack and then read it with md.l from the memory. I tried
> it on my custom board (AM1808 SoC, direct boot from NOR flash) and on
> both the da850evm (with AM1808 SoC, AIS boot from SPI flash). The
> result was the same for both cases, 0x00052078. So DCache and ICache
> are disabled after the RBL.
> Regards, Christian

Hmm.. That's different from the OMAP processors I have seen. At least
OMAP4, that I verified again now, leaves the I-cache enabled
(0x00C51878)

So, Sughosh's problem still remains a mystery:)

br,
Aneesh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Tom Rini]

On Fri, Jan 20, 2012 at 6:06 AM, Aneesh V <aneesh@ti.com> wrote:
> Hi Christian,
>
>
> On Friday 20 January 2012 06:18 PM, Christian Riesch wrote:
>>
>> Hi Aneesh,
>>
>> On Fri, Jan 20, 2012 at 1:13 PM, Aneesh V<aneesh@ti.com> ?wrote:
>>>
>>> On Friday 20 January 2012 02:51 PM, Christian Riesch wrote:
>>>>
>>>> On Fri, Jan 20, 2012 at 9:52 AM, Aneesh V<aneesh@ti.com> ? ?wrote:
>>>>>
>>>>> Sughosh,
>>>>
>>>>
>>>> [...]
>>>>>
>>>>>
>>>>> Can you send the value of SCR you found at SPL entry? This will clarify
>>>>> what's enabled and what's not.
>>>>
>>>>
>>>> I would like to try that on my board as well for comparison. Could you
>>>> please tell me how this register can be read? In the ARM manuals SCR
>>>> seems to have several meanings... Thank you!
>>>> Regards, Christian
>>>
>>>
>>> If you have a JTAG based debugger that has the capability of displaying
>>> CP15 registers, look for "CP15 System Control Register". Otherwise you
>>> will have to read it using an assembly instruction like below:
>>>
>>>
>>> ? ? ? ?mrc ? ? p15, 0, r0, c1, c0, 0
>>>
>>> After this instruction r0 will contain the SCR value. arm926ejs/start.S
>>> has this instruction at line #367. You can put a breakpoint after this
>>> and look at r0.
>>
>>
>> Thank you!
>>
>> I don't have a JTAG debugger so I stored it in a register, pushed it
>> later to the stack and then read it with md.l from the memory. I tried
>> it on my custom board (AM1808 SoC, direct boot from NOR flash) and on
>> both the da850evm (with AM1808 SoC, AIS boot from SPI flash). The
>> result was the same for both cases, 0x00052078. So DCache and ICache
>> are disabled after the RBL.
>> Regards, Christian
>
>
> Hmm.. That's different from the OMAP processors I have seen. At least
> OMAP4, that I verified again now, leaves the I-cache enabled
> (0x00C51878)
>
> So, Sughosh's problem still remains a mystery:)

So, what do we want to do here?  We really want to get this fix in so
we can get the hawkboard SPL changes in, and the other platforms /
fixups that are gated by that.

If I can sum it up, in the relevant section of code we have incorrect
comments and the init code is not following what the manual says the
correct sequence is.  However, given the (potentially wide) impact the
changes would have, Albert had previously requested making the change
opt-in (but I believe this request came before the "the manual says we
must do ...").  If this is still the case?  If so, can we get an
updated patch?  Thanks!

-- 
Tom

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Christian Riesch]

Hi all,

On Fri, Jan 27, 2012 at 7:33 PM, Tom Rini <tom.rini@gmail.com> wrote:
> So, what do we want to do here? ?We really want to get this fix in so
> we can get the hawkboard SPL changes in, and the other platforms /
> fixups that are gated by that.
>
> If I can sum it up, in the relevant section of code we have incorrect
> comments and the init code is not following what the manual says the
> correct sequence is. ?However, given the (potentially wide) impact the
> changes would have, Albert had previously requested making the change
> opt-in (but I believe this request came before the "the manual says we
> must do ..."). ?If this is still the case? ?If so, can we get an
> updated patch? ?Thanks!

A few thoughts:

1) Before commit ca4b55800ed74207c35271bf7335a092d4955416 the low
level initialization code that we are discussing here was only
executed if CONFIG_SKIP_LOWLEVEL_INIT is not defined. For ARM926EJS
the relevant section in start.S looked like this

#ifndef CONFIG_SKIP_LOWLEVEL_INIT
flush caches
turn off dcache, do some other configurations of the CPU, enable icache
call the SoC specific lowlevel_init
#endif

For DA850 SoCs we had no lowlevel_init routine and therefore
CONFIG_SKIP_LOWLEVEL_INIT defined in all board configurations. The
lowlevel initialization was done by TI's UBL boot loader. Now we have
boards that don't use UBL (e.g. enbw_cmc, calimain, da850evm when used
with the SPL), therefore we need some lowlevel init. Most important
configuration is enabling icache, otherwise u-boot startup gets really
slow.

Since commit ca4b55800ed74207c35271bf7335a092d4955416 it is

flush caches
turn off dcache, do some other configurations of the CPU, enable icache
#ifndef CONFIG_SKIP_LOWLEVEL_INIT
call the SoC specific lowlevel_init
#endif

So the change that has a big impact is already done and in mainline.

Perhaps we should revert that change and instead remove
CONFIG_SKIP_LOWLEVEL_INIT from the da850 board config files. But since
we don't need the lowlevel_init function for DA850 SoCs we must either
add a dummy function or add some additional defines that allow
removing the CONFIG_SKIP_LOWLEVEL_INIT define without calling
lowlevel_init. Any suggestions how such defines should be called or
how the logic behind them should be so it doesn't cause breakage of
existing boards?

What do you think? Or is reverting to dangerous since we would change
the behavior of start.S twice if we do so?

2) The current version of Sughosh's patch does not change the logic
behind the LOWLEVEL_INIT defines but just fixes the code to agree with
ARM's manual. Instead of invalidating the cache it now is flushed. I
think we should therefore merge his patch (@Tom: Yes, the manual says
we must do.). The big change that Albert fears was already done
earlier in commit ca4b55800ed74207c35271bf7335a092d4955416. And while
we are doing this we also get the comments right.

3) As Sughosh pointed out, the current code changes the V bit
(location of exceptions). Sughosh's patch removes this code that does
this change.  I'm not sure if this is correct or not, so maybe you,
Tom, could put your TI hat on again and clarify this?

4) The current code turns on icache. This is great since my board
executes u-boot directly from flash until it relocates itself and thus
the startup time is greatly reduced by using icache. However, there is
this CONFIG_SYS_ICACHE_OFF define. Should the code be changed to
respect this define?

Regards, Christian

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Heiko Schocher]

Hello Christian,

Christian Riesch wrote:
> Hi all,
> 
> On Fri, Jan 27, 2012 at 7:33 PM, Tom Rini <tom.rini@gmail.com> wrote:
>> So, what do we want to do here?  We really want to get this fix in so
>> we can get the hawkboard SPL changes in, and the other platforms /
>> fixups that are gated by that.
>>
>> If I can sum it up, in the relevant section of code we have incorrect
>> comments and the init code is not following what the manual says the
>> correct sequence is.  However, given the (potentially wide) impact the
>> changes would have, Albert had previously requested making the change
>> opt-in (but I believe this request came before the "the manual says we
>> must do ...").  If this is still the case?  If so, can we get an
>> updated patch?  Thanks!
> 
> A few thoughts:
> 
> 1) Before commit ca4b55800ed74207c35271bf7335a092d4955416 the low
> level initialization code that we are discussing here was only
> executed if CONFIG_SKIP_LOWLEVEL_INIT is not defined. For ARM926EJS
> the relevant section in start.S looked like this
> 
> #ifndef CONFIG_SKIP_LOWLEVEL_INIT
> flush caches
> turn off dcache, do some other configurations of the CPU, enable icache
> call the SoC specific lowlevel_init
> #endif
> 
> For DA850 SoCs we had no lowlevel_init routine and therefore
> CONFIG_SKIP_LOWLEVEL_INIT defined in all board configurations. The
> lowlevel initialization was done by TI's UBL boot loader. Now we have
> boards that don't use UBL (e.g. enbw_cmc, calimain, da850evm when used
> with the SPL), therefore we need some lowlevel init. Most important
> configuration is enabling icache, otherwise u-boot startup gets really
> slow.
> 
> Since commit ca4b55800ed74207c35271bf7335a092d4955416 it is
> 
> flush caches
> turn off dcache, do some other configurations of the CPU, enable icache
> #ifndef CONFIG_SKIP_LOWLEVEL_INIT
> call the SoC specific lowlevel_init
> #endif
> 
> So the change that has a big impact is already done and in mainline.

Yep.

> Perhaps we should revert that change and instead remove
> CONFIG_SKIP_LOWLEVEL_INIT from the da850 board config files. But since
> we don't need the lowlevel_init function for DA850 SoCs we must either
> add a dummy function or add some additional defines that allow
> removing the CONFIG_SKIP_LOWLEVEL_INIT define without calling
> lowlevel_init. Any suggestions how such defines should be called or
> how the logic behind them should be so it doesn't cause breakage of
> existing boards?

or we should introduce a "CONFIG_SKIP_CPU_CRIT_INIT" define, which
if defined, prevent the jump to cpu_init_crit ... so we have the same
behaviour as before commit "ca4b55800ed74207c35271bf7335a092d4955416"

Boards which have problems with cpu_crit_init, should define
this new define ... but it would be better to find out, what
is really going on here ...

> What do you think? Or is reverting to dangerous since we would change
> the behavior of start.S twice if we do so?

See comment above. I can send such a patch for this, if we decide to go
this direction ... as I need the cpu_init_crit part for the enbw_cmc
board, but do not need the branch to "lowlevel_init" ...

> 2) The current version of Sughosh's patch does not change the logic
> behind the LOWLEVEL_INIT defines but just fixes the code to agree with
> ARM's manual. Instead of invalidating the cache it now is flushed. I
> think we should therefore merge his patch (@Tom: Yes, the manual says
> we must do.). The big change that Albert fears was already done
> earlier in commit ca4b55800ed74207c35271bf7335a092d4955416. And while
> we are doing this we also get the comments right.

What do you mean with commit "ca4b55800ed74207c35271bf7335a092d4955416"?
I could not find it in mainline ...

> 3) As Sughosh pointed out, the current code changes the V bit
> (location of exceptions). Sughosh's patch removes this code that does
> this change.  I'm not sure if this is correct or not, so maybe you,
> Tom, could put your TI hat on again and clarify this?

Yes, I am also not sure, what to do with this V Bit ...

> 4) The current code turns on icache. This is great since my board
> executes u-boot directly from flash until it relocates itself and thus
> the startup time is greatly reduced by using icache. However, there is
> this CONFIG_SYS_ICACHE_OFF define. Should the code be changed to
> respect this define?

Yep, that should be done. Default should be icache on, so we do
not change exisiting behaviour.

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Sughosh Ganu]

hi Christian,

On Sun Jan 29, 2012 at 02:36:39PM +0100, Christian Riesch wrote:
> Hi all,
> 
> On Fri, Jan 27, 2012 at 7:33 PM, Tom Rini <tom.rini@gmail.com> wrote:
> > So, what do we want to do here? ?We really want to get this fix in so
> > we can get the hawkboard SPL changes in, and the other platforms /
> > fixups that are gated by that.
> >
> > If I can sum it up, in the relevant section of code we have incorrect
> > comments and the init code is not following what the manual says the
> > correct sequence is. ?However, given the (potentially wide) impact the
> > changes would have, Albert had previously requested making the change
> > opt-in (but I believe this request came before the "the manual says we
> > must do ..."). ?If this is still the case? ?If so, can we get an
> > updated patch? ?Thanks!
> 
> A few thoughts:
> 
> 1) Before commit ca4b55800ed74207c35271bf7335a092d4955416 the low
> level initialization code that we are discussing here was only
> executed if CONFIG_SKIP_LOWLEVEL_INIT is not defined. For ARM926EJS
> the relevant section in start.S looked like this
> 
> #ifndef CONFIG_SKIP_LOWLEVEL_INIT
> flush caches
> turn off dcache, do some other configurations of the CPU, enable icache
> call the SoC specific lowlevel_init
> #endif

  This is correct, the only differece being, 

  flush caches, is what the comment said, but it was actually doing a
  invalidate caches.

<snip> 


> So the change that has a big impact is already done and in mainline.
> 
> Perhaps we should revert that change and instead remove
> CONFIG_SKIP_LOWLEVEL_INIT from the da850 board config files. But since
> we don't need the lowlevel_init function for DA850 SoCs we must either
> add a dummy function or add some additional defines that allow
> removing the CONFIG_SKIP_LOWLEVEL_INIT define without calling
> lowlevel_init. Any suggestions how such defines should be called or
> how the logic behind them should be so it doesn't cause breakage of
> existing boards?

  Actually, even i can do with the enabling of the icache :). The only
  change i made was that instead of invalidating the data cache, i do
  doing a "test and flush".

> What do you think? Or is reverting to dangerous since we would change
> the behavior of start.S twice if we do so?
> 
> 2) The current version of Sughosh's patch does not change the logic
> behind the LOWLEVEL_INIT defines but just fixes the code to agree with
> ARM's manual. Instead of invalidating the cache it now is flushed. I
> think we should therefore merge his patch (@Tom: Yes, the manual says
> we must do.). The big change that Albert fears was already done
> earlier in commit ca4b55800ed74207c35271bf7335a092d4955416. And while
> we are doing this we also get the comments right.

  Actually, the part of the code that Albert NAK'ed was changing the
  setting of the V bit. To this he had proposed to make this a config
  option.

  The current behaviour clears the V bit by default in the
  cpu_init_crit function, which maps the exception vectors at 0x0. But
  some SoC's don't have anything mapped at 0x0(eg. omapl-138), so i
  had proposed not to clear this bit by default.

  http://www.mail-archive.com/u-boot at lists.denx.de/msg75271.html

> 
> 3) As Sughosh pointed out, the current code changes the V bit
> (location of exceptions). Sughosh's patch removes this code that does
> this change.  I'm not sure if this is correct or not, so maybe you,
> Tom, could put your TI hat on again and clarify this?

  Please check the link i have provided above.

-sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Christian Riesch]

Hi,

On Monday, January 30, 2012, Heiko Schocher <hs@denx.de> wrote:
> Hello Christian,
>
> Christian Riesch wrote:
>> Hi all,
>>
>> On Fri, Jan 27, 2012 at 7:33 PM, Tom Rini <tom.rini@gmail.com> wrote:
>>> So, what do we want to do here?  We really want to get this fix in so
>>> we can get the hawkboard SPL changes in, and the other platforms /
>>> fixups that are gated by that.
>>>
>>> If I can sum it up, in the relevant section of code we have incorrect
>>> comments and the init code is not following what the manual says the
>>> correct sequence is.  However, given the (potentially wide) impact the
>>> changes would have, Albert had previously requested making the change
>>> opt-in (but I believe this request came before the "the manual says we
>>> must do ...").  If this is still the case?  If so, can we get an
>>> updated patch?  Thanks!
>>
>> A few thoughts:
>>
>> 1) Before commit ca4b55800ed74207c35271bf7335a092d4955416 the low
>> level initialization code that we are discussing here was only
>> executed if CONFIG_SKIP_LOWLEVEL_INIT is not defined. For ARM926EJS
>> the relevant section in start.S looked like this
>>
>> #ifndef CONFIG_SKIP_LOWLEVEL_INIT
>> flush caches
>> turn off dcache, do some other configurations of the CPU, enable icache
>> call the SoC specific lowlevel_init
>> #endif
>>
>> For DA850 SoCs we had no lowlevel_init routine and therefore
>> CONFIG_SKIP_LOWLEVEL_INIT defined in all board configurations. The
>> lowlevel initialization was done by TI's UBL boot loader. Now we have
>> boards that don't use UBL (e.g. enbw_cmc, calimain, da850evm when used
>> with the SPL), therefore we need some lowlevel init. Most important
>> configuration is enabling icache, otherwise u-boot startup gets really
>> slow.
>>
>> Since commit ca4b55800ed74207c35271bf7335a092d4955416 it is
>>
>> flush caches
>> turn off dcache, do some other configurations of the CPU, enable icache
>> #ifndef CONFIG_SKIP_LOWLEVEL_INIT
>> call the SoC specific lowlevel_init
>> #endif
>>
>> So the change that has a big impact is already done and in mainline.
>
> Yep.
>
>> Perhaps we should revert that change and instead remove
>> CONFIG_SKIP_LOWLEVEL_INIT from the da850 board config files. But since
>> we don't need the lowlevel_init function for DA850 SoCs we must either
>> add a dummy function or add some additional defines that allow
>> removing the CONFIG_SKIP_LOWLEVEL_INIT define without calling
>> lowlevel_init. Any suggestions how such defines should be called or
>> how the logic behind them should be so it doesn't cause breakage of
>> existing boards?
>
> or we should introduce a "CONFIG_SKIP_CPU_CRIT_INIT" define, which
> if defined, prevent the jump to cpu_init_crit ... so we have the same
> behaviour as before commit "ca4b55800ed74207c35271bf7335a092d4955416"
>
> Boards which have problems with cpu_crit_init, should define
> this new define ... but it would be better to find out, what
> is really going on here ...

Yes, that would be good of course.

Another idea: Actually the init code that we are discussing here is
executed twice. First in the SPL and then in the full-blown u-boot.
Sughosh, are you sure it is the execution of the code in SPL that causes
the trouble? I'm asking since we don't do any memory barrier related stuff
in the code that loads u-boot from flash in SPL. Of course, dcache is off
but icache is on.

Regards, Christian

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Sughosh Ganu]

hi Christian,

On Mon Jan 30, 2012 at 09:10:46AM +0100, Christian Riesch wrote:

<snip>

> >> Perhaps we should revert that change and instead remove
> >> CONFIG_SKIP_LOWLEVEL_INIT from the da850 board config files. But since
> >> we don't need the lowlevel_init function for DA850 SoCs we must either
> >> add a dummy function or add some additional defines that allow
> >> removing the CONFIG_SKIP_LOWLEVEL_INIT define without calling
> >> lowlevel_init. Any suggestions how such defines should be called or
> >> how the logic behind them should be so it doesn't cause breakage of
> >> existing boards?
> >
> > or we should introduce a "CONFIG_SKIP_CPU_CRIT_INIT" define, which
> > if defined, prevent the jump to cpu_init_crit ... so we have the same
> > behaviour as before commit "ca4b55800ed74207c35271bf7335a092d4955416"
> >
> > Boards which have problems with cpu_crit_init, should define
> > this new define ... but it would be better to find out, what
> > is really going on here ...
> 
> Yes, that would be good of course.
> 
> Another idea: Actually the init code that we are discussing here is
> executed twice. First in the SPL and then in the full-blown u-boot.
> Sughosh, are you sure it is the execution of the code in SPL that causes
> the trouble? I'm asking since we don't do any memory barrier related stuff
> in the code that loads u-boot from flash in SPL. Of course, dcache is off
> but icache is on.

  Yes, it is the spl that is executing the code. There is a spl
  specific string that gets displayed on the console, before spl goes
  on to load the u-boot image and jump to it, and i don't see that
  string on the console.

-sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Christian Riesch]

Hello Heiko,

On Mon, Jan 30, 2012 at 7:39 AM, Heiko Schocher <hs@denx.de> wrote:
> Christian Riesch wrote:
>> 2) The current version of Sughosh's patch does not change the logic
>> behind the LOWLEVEL_INIT defines but just fixes the code to agree with
>> ARM's manual. Instead of invalidating the cache it now is flushed. I
>> think we should therefore merge his patch (@Tom: Yes, the manual says
>> we must do.). The big change that Albert fears was already done
>> earlier in commit ca4b55800ed74207c35271bf7335a092d4955416. And while
>> we are doing this we also get the comments right.
>
> What do you mean with commit "ca4b55800ed74207c35271bf7335a092d4955416"?
> I could not find it in mainline ...

commit ca4b55800ed74207c35271bf7335a092d4955416
Author: Heiko Schocher <hs@denx.de>
Date:   Wed Nov 9 20:06:23 2011 +0000

arm, arm926ejs: always do cpu critical inits

Regards, Christian

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Tom Rini]

On Sun, Jan 29, 2012 at 6:36 AM, Christian Riesch
<christian.riesch@omicron.at> wrote:

> 3) As Sughosh pointed out, the current code changes the V bit
> (location of exceptions). Sughosh's patch removes this code that does
> this change. ?I'm not sure if this is correct or not, so maybe you,
> Tom, could put your TI hat on again and clarify this?

OK, I've asked Christian for questions I can pass along, and here's
what's I've learned:

Q1) Currently, the low level initialization code for ARM926EJS CPUs in
the u-boot bootloader clears the V-bit of the cp15 control register
c1. By default, this bit is set on AM1808 and OMAP-L138 before u-boot
ist started. Sughosh Ganu now submitted a patch to remove the code
that clears the V bit because he states that these SoCs have no valid
memory region at 0x0. I had a look at the memory map of the AM1808 and
yes, there is no valid memory at 0x0, so the V bit should be set and
not cleared. My question is: Since the AM1808 has no valid memory at
0x0, does clearing the V bit have any effect on the operation of the
processor? Or is is just a don't care bit since it doesn't make any
sense to clear it?

A1) This may be a design question, but I can say that the default
value of the V-bit is set to 1 because of tie-offs to the core, but
I'm not sure if the functionality of the V-bit is still active.  I
would guess that it is because I see no reason we would have mucked
around in the ARM core design to remove that functionality, so unless
there is another tie-off to the core that disables the V-bit
functionality entirely, I would guess clearing the V-bit is not a good
idea.  In fact, I don't see why the u-boot init code needs to mess
with the default value of that bit ever, for any device or arch.

Q2) RBL: In an earlier post to the u-boot mailing list Tom Rini wrote
that the RBLs of AM1808 and OMAP-L138 do not turn on caches. Does this
mean that caches (dcache and/or icache) are *never* enabled or does it
mean that they get enabled and then disabled before RBL exits. In the
latter case, does RBL do everything that is necessary to ensure
consistency between data and instruction streams (as described in the
ARM926EJ-S Technical Reference Manual,
http://infocenter.arm.com/help/index.jsp, Section "Instruction Memory
Barrier.1. About the instruction memory barrier operation")? Are there
maybe earlier versions of RBL in earlier versions of the SoC that have
a bug here?

A2) The RBL NEVER enables the caches.  Unfortunately, this does slow
the operation of the ROM boot loader, but it is what is

-- 
Tom

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Sughosh Ganu]

On Mon Jan 30, 2012 at 10:03:40AM -0700, Tom Rini wrote:

> Q1) Currently, the low level initialization code for ARM926EJS CPUs in
> the u-boot bootloader clears the V-bit of the cp15 control register
> c1. By default, this bit is set on AM1808 and OMAP-L138 before u-boot
> ist started. Sughosh Ganu now submitted a patch to remove the code
> that clears the V bit because he states that these SoCs have no valid
> memory region at 0x0. I had a look at the memory map of the AM1808 and
> yes, there is no valid memory at 0x0, so the V bit should be set and
> not cleared. My question is: Since the AM1808 has no valid memory at
> 0x0, does clearing the V bit have any effect on the operation of the
> processor? Or is is just a don't care bit since it doesn't make any
> sense to clear it?
> 
> A1) This may be a design question, but I can say that the default
> value of the V-bit is set to 1 because of tie-offs to the core, but
> I'm not sure if the functionality of the V-bit is still active.  I
> would guess that it is because I see no reason we would have mucked
> around in the ARM core design to remove that functionality, so unless
> there is another tie-off to the core that disables the V-bit
> functionality entirely, I would guess clearing the V-bit is not a good
> idea.  In fact, I don't see why the u-boot init code needs to mess
> with the default value of that bit ever, for any device or arch.

  I'm not sure about whether the V-bit functionality is disabled by
  some setting, but the omapl-138 ref manual states this(section 2.4
  "Exceptions and Exception Vectors").

" NOTE: The VINTH signal is configurable by way of the register
setting in CP15. However, it is not recommended to set VINTH = 0, as
the device has no physical memory in the 0000 0000h address region." 

-sughosh

[U-Boot] [PATCH 1/2 V2] arm926: Flush the data cache before disabling it.

[Christian Riesch]

Hi Tom,

On Mon, Jan 30, 2012 at 6:03 PM, Tom Rini <tom.rini@gmail.com> wrote:
> On Sun, Jan 29, 2012 at 6:36 AM, Christian Riesch
> <christian.riesch@omicron.at> wrote:
>
>> 3) As Sughosh pointed out, the current code changes the V bit
>> (location of exceptions). Sughosh's patch removes this code that does
>> this change. ?I'm not sure if this is correct or not, so maybe you,
>> Tom, could put your TI hat on again and clarify this?
>
> OK, I've asked Christian for questions I can pass along, and here's
> what's I've learned:

Thanks a lot!

> Q1) Currently, the low level initialization code for ARM926EJS CPUs in
> the u-boot bootloader clears the V-bit of the cp15 control register
> c1. By default, this bit is set on AM1808 and OMAP-L138 before u-boot
> ist started. Sughosh Ganu now submitted a patch to remove the code
> that clears the V bit because he states that these SoCs have no valid
> memory region at 0x0. I had a look at the memory map of the AM1808 and
> yes, there is no valid memory at 0x0, so the V bit should be set and
> not cleared. My question is: Since the AM1808 has no valid memory at
> 0x0, does clearing the V bit have any effect on the operation of the
> processor? Or is is just a don't care bit since it doesn't make any
> sense to clear it?
>
> A1) This may be a design question, but I can say that the default
> value of the V-bit is set to 1 because of tie-offs to the core, but
> I'm not sure if the functionality of the V-bit is still active. ?I
> would guess that it is because I see no reason we would have mucked
> around in the ARM core design to remove that functionality, so unless
> there is another tie-off to the core that disables the V-bit
> functionality entirely, I would guess clearing the V-bit is not a good
> idea. ?In fact, I don't see why the u-boot init code needs to mess
> with the default value of that bit ever, for any device or arch.

Ok. So this should not be cleared, at least not for davinci.

> Q2) RBL: In an earlier post to the u-boot mailing list Tom Rini wrote
> that the RBLs of AM1808 and OMAP-L138 do not turn on caches. Does this
> mean that caches (dcache and/or icache) are *never* enabled or does it
> mean that they get enabled and then disabled before RBL exits. In the
> latter case, does RBL do everything that is necessary to ensure
> consistency between data and instruction streams (as described in the
> ARM926EJ-S Technical Reference Manual,
> http://infocenter.arm.com/help/index.jsp, Section "Instruction Memory
> Barrier.1. About the instruction memory barrier operation")? Are there
> maybe earlier versions of RBL in earlier versions of the SoC that have
> a bug here?
>
> A2) The RBL NEVER enables the caches. ?Unfortunately, this does slow
> the operation of the ROM boot loader, but it is what is

I think this ends our speculations about RBL causing the hawkboard
problems. We will need someone with a hawkboard and a JTAG debugger to
find out more...

For now I think we should make these changes:
1) correct the comments
2) replace invalidating the cache with flushing the cache since this
is the way to do it according to the ARM manual (Sughosh's patch)
3) do not clear the V bit on DA850 SoCs
4) revert Heiko's patch that changes the behavior of
CONFIG_SKIP_LOWLEVEL_INIT and add a possibility to remove this option
on boards that need lowlevel config
2) respect CONFIG_SYS_ICACHE_OFF

I prepared a patchset that also includes Sughosh's patches and have
just submitted it (The people on Cc received it twice, sorry for
this...).

Regards, Christian

[U-Boot] [PATCH 1/2 V4] arm926: Flush the data cache before disabling it

[Albert ARIBAUD]

Hi Sughosh,

Le 14/01/2012 15:02, Sughosh Ganu a ?crit :
> The current implementation invalidates the cache instead of flushing
> it. This causes problems on platforms where the spl/u-boot is already
> loaded to the RAM, with caches enabled by a first stage bootloader.
>
> Also fix the comments to match code.
>
> Signed-off-by: Sughosh Ganu<urwithsughosh@gmail.com>
> Cc: Albert Aribaud<albert.u.boot@aribaud.net>
> Cc: Tom Rini<trini@ti.com>
> ---
>
> Changes since V3
> * Removed tampering of the V bit setting. Would be done in a separate
>    patch on the lines of review comments by Albert.
>
> Changes since V2
> * Added code to invalidate I cache, based on review comment by Aneesh.
> * Fixed comments to match the code.
>
> Changes since V1
> * Added arm926 keyword to the subject line
> * Removed the superfluous setting of r0.
> * Fixed the comment to reflect the fact that V is not being cleared
>
>   arch/arm/cpu/arm926ejs/start.S |   12 ++++++++----
>   1 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/cpu/arm926ejs/start.S b/arch/arm/cpu/arm926ejs/start.S
> index 6a09c02..d64165a 100644
> --- a/arch/arm/cpu/arm926ejs/start.S
> +++ b/arch/arm/cpu/arm926ejs/start.S
> @@ -355,14 +355,18 @@ _dynsym_start_ofs:
>    */
>   cpu_init_crit:
>   	/*
> -	 * flush v4 I/D caches
> +	 * flush D cache before disabling it
>   	 */
>   	mov	r0, #0
> -	mcr	p15, 0, r0, c7, c7, 0	/* flush v3/v4 cache */
> -	mcr	p15, 0, r0, c8, c7, 0	/* flush v4 TLB */

Please add a comment explaining what the loop is waiting for exactly:

> +flush_dcache:
> +	mrc	p15, 0, r15, c7, c10, 3
> +	bne	flush_dcache

> +
> +	mcr	p15, 0, r0, c8, c7, 0	/* invalidate TLB */
> +	mcr	p15, 0, r0, c7, c5, 0	/* invalidate I Cache */
>
>   	/*
> -	 * disable MMU stuff and caches
> +	 * disable MMU and D cache, and enable I cache.
>   	 */
>   	mrc	p15, 0, r0, c1, c0, 0
>   	bic	r0, r0, #0x00002300	/* clear bits 13, 9:8 (--V- --RS) */

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/2 V3] arm926: Flush the data cache before disabling it

[Christian Riesch]

Hi Albert,

On Saturday, February 18, 2012, Albert ARIBAUD <albert.u.boot@aribaud.net>
wrote:
> Le 14/01/2012 15:02, Sughosh Ganu a ?crit :
>>
>> The current implementation invalidates the cache instead of flushing
>> it. This causes problems on platforms where the spl/u-boot is already
>> loaded to the RAM, with caches enabled by a first stage bootloader.
>>
>> Also fix the comments to match code.
>>
>> Signed-off-by: Sughosh Ganu<urwithsughosh@gmail.com>
>> Cc: Albert Aribaud<albert.u.boot@aribaud.net>
>> Cc: Tom Rini<trini@ti.com>
>> ---
>>
>> Changes since V3
>> * Removed tampering of the V bit setting. Would be done in a separate
>>   patch on the lines of review comments by Albert.
>>
>> Changes since V2
>> * Added code to invalidate I cache, based on review comment by Aneesh.
>> * Fixed comments to match the code.
>>
>> Changes since V1
>> * Added arm926 keyword to the subject line
>> * Removed the superfluous setting of r0.
>> * Fixed the comment to reflect the fact that V is not being cleared
>>
>>  arch/arm/cpu/arm926ejs/start.S |   12 ++++++++----
>>  1 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/cpu/arm926ejs/start.S
b/arch/arm/cpu/arm926ejs/start.S
>> index 6a09c02..d64165a 100644
>> --- a/arch/arm/cpu/arm926ejs/start.S
>> +++ b/arch/arm/cpu/arm926ejs/start.S
>> @@ -355,14 +355,18 @@ _dynsym_start_ofs:
>>   */
>>  cpu_init_crit:
>>        /*
>> -        * flush v4 I/D caches
>> +        * flush D cache before disabling it
>>         */
>>        mov     r0, #0
>> -       mcr     p15, 0, r0, c7, c7, 0   /* flush v3/v4 cache */
>> -       mcr     p15, 0, r0, c8, c7, 0   /* flush v4 TLB */
>
> Please add a comment explaining what the loop is waiting for exactly:
>
>> +flush_dcache:
>> +       mrc     p15, 0, r15, c7, c10, 3
>> +       bne     flush_dcache

That's the flush dcache code that is given in ARM's TRM for the 926ejs.
Anyway, that's already in mainline. Shall we prepare a patch that adds a
comment?

Regards, Christian

[U-Boot] [PATCH 1/2 V3] arm926: Flush the data cache before disabling it

[Albert ARIBAUD]

Hi Christian,

Le 18/02/2012 19:51, Christian Riesch a ?crit :
> Hi Albert,
>
> On Saturday, February 18, 2012, Albert ARIBAUD<albert.u.boot@aribaud.net>
> wrote:
>> Le 14/01/2012 15:02, Sughosh Ganu a ?crit :
>>>
>>> The current implementation invalidates the cache instead of flushing
>>> it. This causes problems on platforms where the spl/u-boot is already
>>> loaded to the RAM, with caches enabled by a first stage bootloader.
>>>
>>> Also fix the comments to match code.
>>>
>>> Signed-off-by: Sughosh Ganu<urwithsughosh@gmail.com>
>>> Cc: Albert Aribaud<albert.u.boot@aribaud.net>
>>> Cc: Tom Rini<trini@ti.com>
>>> ---
>>>
>>> Changes since V3
>>> * Removed tampering of the V bit setting. Would be done in a separate
>>>    patch on the lines of review comments by Albert.
>>>
>>> Changes since V2
>>> * Added code to invalidate I cache, based on review comment by Aneesh.
>>> * Fixed comments to match the code.
>>>
>>> Changes since V1
>>> * Added arm926 keyword to the subject line
>>> * Removed the superfluous setting of r0.
>>> * Fixed the comment to reflect the fact that V is not being cleared
>>>
>>>   arch/arm/cpu/arm926ejs/start.S |   12 ++++++++----
>>>   1 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm/cpu/arm926ejs/start.S
> b/arch/arm/cpu/arm926ejs/start.S
>>> index 6a09c02..d64165a 100644
>>> --- a/arch/arm/cpu/arm926ejs/start.S
>>> +++ b/arch/arm/cpu/arm926ejs/start.S
>>> @@ -355,14 +355,18 @@ _dynsym_start_ofs:
>>>    */
>>>   cpu_init_crit:
>>>         /*
>>> -        * flush v4 I/D caches
>>> +        * flush D cache before disabling it
>>>          */
>>>         mov     r0, #0
>>> -       mcr     p15, 0, r0, c7, c7, 0   /* flush v3/v4 cache */
>>> -       mcr     p15, 0, r0, c8, c7, 0   /* flush v4 TLB */
>>
>> Please add a comment explaining what the loop is waiting for exactly:
>>
>>> +flush_dcache:
>>> +       mrc     p15, 0, r15, c7, c10, 3
>>> +       bne     flush_dcache
>
> That's the flush dcache code that is given in ARM's TRM for the 926ejs.

Just because it comes from official ARM documentation does not make it 
understandable to new readers of the code. :)

> Anyway, that's already in mainline. Shall we prepare a patch that adds a
> comment?

Sorry, I missed the fact it'd gone in already. No, that's ok, I'll just 
complain again, and earlier, when the file is next touched.

> Regards, Christian

Amicalement,
-- 
Albert.