Flush the date cache before disabling it.

 arch/arm/cpu/arm926ejs/start.S |   10 +++++++---

Quoted 1 line · 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

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 <[email protected]>

Cc Albert Aribaud <[email protected]>

---

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(-)

(quoted diff) +0 -0

b/arch/arm/cpu/arm926ejs/start.S index 6a09c02..6e261c2 100644

arch/arm/cpu/arm926ejs/start.S +6 -3

@@ -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 */

Quoted 3 lines

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?

Quoted 4 lines

Quoted 3 lines

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.

Quoted 1 line

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

Quoted 7 lines · Marek Vasut

Quoted 5 lines

Quoted 3 lines

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?

Quoted 6 lines

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 ?

Quoted 13 lines

Quoted 13 lines · Marek Vasut

Quoted 7 lines

Quoted 4 lines

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.

Quoted 3 lines

Quoted 1 line

-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

Quoted 16 lines · Marek Vasut

Quoted 15 lines

Quoted 14 lines · Marek Vasut

Quoted 7 lines

Quoted 4 lines

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

Quoted 4 lines

Quoted 1 line

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?

Quoted 21 lines

Quoted 20 lines · Marek Vasut

Quoted 8 lines

Quoted 8 lines · Marek Vasut

Quoted 7 lines

Quoted 4 lines

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?

Quoted 9 lines

Quoted 6 lines

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).

Quoted 2 lines

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

Quoted 25 lines · Marek Vasut

Quoted 24 lines

Quoted 14 lines · Marek Vasut

Quoted 13 lines

Quoted 13 lines · Marek Vasut

Quoted 7 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

-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?

Quoted 3 lines

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

Quoted 1 line

-sughosh

Quoted 4 lines

Quoted 3 lines

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.

Quoted 4 lines

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 ?

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 4 lines

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

Quoted 7 lines

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?

Quoted 3 lines

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.

Quoted 5 lines

Quoted 5 lines

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.

Quoted 4 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 3 lines

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

Quoted 6 lines · Marek Vasut

Quoted 5 lines

Quoted 5 lines

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.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

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

xloader with uboot anymore I think.

Hi,

Quoted 6 lines · Marek Vasut

Quoted 6 lines · Marek Vasut

Quoted 5 lines

Quoted 5 lines

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.

Quoted 8 lines

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

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

Quoted 1 line

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

Quoted 3 lines

Hi,

On Wednesday, January 11, 2012, Marek Vasut <[email protected]>

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines · Marek Vasut

Quoted 6 lines

Quoted 6 lines

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

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

Quoted 4 lines

Quoted 1 line

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

Quoted 8 lines

don't need

Quoted 1 line

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

Quoted 1 line

does or how you create it?

So basically, this SPL business can be avoided and this all can be done

standard way?

On Wednesday, January 11, 2012, Marek Vasut <[email protected]> wrote:

Quoted 1 line

Quoted 1 line

RBL executes an AIS script. Sughosh, could you please explain what your

AIS

Quoted 3 lines

Quoted 1 line

does or how you create it?

So basically, this SPL business can be avoided and this all can be done

in a

Quoted 1 line

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

Quoted 1 line · Marek Vasut

<snip>

Quoted 8 lines

Quoted 1 line

Quoted 1 line

RBL executes an AIS script. Sughosh, could you please explain what your

AIS

Quoted 3 lines

Quoted 1 line

does or how you create it?

So basically, this SPL business can be avoided and this all can be done

in a

Quoted 1 line

standard way?

Quoted 3 lines

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.

Le 12/01/2012 07:29, Sughosh Ganu a ?crit :

Quoted 4 lines · Christian Riesch

Quoted 2 lines

On Wednesday, January 11, 2012, Marek Vasut<[email protected]>

Quoted 4 lines

<snip>

Quoted 1 line

Quoted 1 line

Quoted 1 line

RBL executes an AIS script. Sughosh, could you please explain what

Quoted 15 lines

Quoted 14 lines

AIS

Quoted 4 lines

Quoted 2 lines

does or how you create it?

So basically, this SPL business can be avoided and this all can be done

in a

Quoted 2 lines

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

Quoted 3 lines

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

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

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 <[email protected]>

Cc Albert Aribaud <[email protected]>

---

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

Hi Sughosh,

Quoted 8 lines · 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.

Quoted 8 lines

Signed-off-by Sughosh Ganu <[email protected]>

Cc Albert Aribaud <[email protected]>

---

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.

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?

Quoted 65 lines · Christian Riesch

Quoted 8 lines · 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.

Quoted 8 lines

Signed-off-by Sughosh Ganu <[email protected]>

Cc Albert Aribaud <[email protected]>

---

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>

Quoted 13 lines

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

Quoted 7 lines

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 mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Quoted 1 line · Sughosh Ganu

On Thu Jan 12, 2012 at 01:03:05PM +0100, Christian Riesch wrote:

Quoted 7 lines

Quoted 6 lines

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?

Quoted 6 lines

?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.

hi Christian,

Quoted 1 line · Christian Riesch

On Thu, Jan 12, 2012 at 2:53 PM, Sughosh Ganu <[email protected]>

Quoted 1 line

Quoted 1 line · Christian Riesch

<snip>

Quoted 17 lines

Quoted 7 lines

Quoted 6 lines

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?

Quoted 6 lines

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:

Quoted 12 lines

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

Hi Sughosh,

Quoted 25 lines

Quoted 4 lines

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

Quoted 8 lines

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

Quoted 6 lines

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

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.

arch/arm/cpu/arm926ejs/start.S +0 -0

index 6a09c02..6e261c2 100644
@@ -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 */

? ? ? ?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 */

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).

Quoted 7 lines

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.

[...]

Quoted 15 lines

arch/arm/cpu/arm926ejs/start.S +0 -0

index 6a09c02..6e261c2 100644
@@ -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

Quoted 2 lines

+ ? ? ? 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.

Quoted 9 lines

? ? ? ?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.

Quoted 3 lines

? ? ? ?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].

hi Christian,

Quoted 4 lines · 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).

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

Quoted 25 lines · Sughosh Ganu

Quoted 7 lines

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.

[...]

Quoted 15 lines

arch/arm/cpu/arm926ejs/start.S +0 -0

index 6a09c02..6e261c2 100644
@@ -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

Quoted 8 lines

Quoted 2 lines

+ ? ? ? 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.

Quoted 12 lines

Quoted 9 lines

? ? ? ?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.

Quoted 8 lines

Quoted 3 lines

? ? ? ?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.

On Fri, Jan 13, 2012 at 1:26 AM, Sughosh Ganu <[email protected]> wrote:

Quoted 9 lines

Quoted 8 lines

Quoted 3 lines

? ? ? ?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].

Quoted 5 lines

?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.

hi Christian,

Quoted 4 lines · 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).

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

Quoted 4 lines · Sughosh Ganu

Quoted 3 lines

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.

Hello Sugosh,

Quoted 14 lines · Sughosh Ganu

hi Christian,

Quoted 4 lines · 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).

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

Quoted 4 lines · Sughosh Ganu

Quoted 3 lines

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.

hi Heiko,

Quoted 18 lines · Heiko Schocher

Hello Sugosh,

Quoted 14 lines · Sughosh Ganu

hi Christian,

Quoted 4 lines · 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).

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

Quoted 4 lines · Sughosh Ganu

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

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.

On Friday 13 January 2012 11:08 PM, Sughosh Ganu wrote:

Quoted 9 lines

Quoted 4 lines

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?

Quoted 3 lines

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.

hi Heiko,

Quoted 13 lines · Heiko Schocher

Hello Sugosh,

Quoted 10 lines · Sughosh Ganu

hi Christian,

Quoted 4 lines · 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).

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

Quoted 1 line · Sughosh Ganu

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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.

Quoted 3 lines

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.

hi Heiko,

Quoted 18 lines · Heiko Schocher

Hello Sugosh,

Quoted 14 lines · Sughosh Ganu

hi Christian,

Quoted 4 lines · 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).

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

Quoted 4 lines · Sughosh Ganu

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 24 lines · Sughosh Ganu

hi Heiko,

Quoted 13 lines · Heiko Schocher

Hello Sugosh,

Quoted 10 lines · Sughosh Ganu

hi Christian,

Quoted 4 lines · 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).

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

Quoted 1 line · Sughosh Ganu

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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?

Quoted 7 lines

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.

Quoted 27 lines · Sughosh Ganu

hi Heiko,

Quoted 18 lines · Heiko Schocher

Hello Sugosh,

Quoted 14 lines · Sughosh Ganu

hi Christian,

Quoted 4 lines · 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).

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

Quoted 4 lines · Sughosh Ganu

Quoted 3 lines

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.

Quoted 5 lines

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?

Quoted 7 lines

?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.

Quoted 36 lines · Tom Rini

Quoted 27 lines · Sughosh Ganu

hi Heiko,

Quoted 18 lines · Heiko Schocher

Hello Sugosh,

Quoted 14 lines · Sughosh Ganu

hi Christian,

Quoted 4 lines · 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).

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

Quoted 4 lines · Sughosh Ganu

Quoted 3 lines

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.

Quoted 5 lines

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?

Quoted 7 lines

?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.

Quoted 2 lines

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.

Quoted 17 lines · Sughosh Ganu

Quoted 14 lines

Quoted 6 lines

Quoted 5 lines

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?

Quoted 7 lines

?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.

Quoted 2 lines

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

Quoted 7 lines

?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?

Quoted 27 lines · Tom Rini

Quoted 17 lines · Sughosh Ganu

Quoted 14 lines

Quoted 6 lines

Quoted 5 lines

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?

Quoted 7 lines

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.

Quoted 2 lines

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

Quoted 7 lines

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.

Quoted 8 lines

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.

Quoted 3 lines · Sughosh Ganu

? 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.

Hi Aneesh,

Quoted 4 lines · Aneesh V

Quoted 3 lines · Sughosh Ganu

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.

Quoted 4 lines

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

Quoted 11 lines · Christian Riesch

Quoted 9 lines · On Thu, Jan 19, 2012 at 11:17 AM, Aneesh V<[email protected]> ?wrote:

Quoted 3 lines · Sughosh Ganu

? 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.

Quoted 3 lines

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.

Quoted 16 lines · Aneesh V

Quoted 14 lines · Christian Riesch

Quoted 9 lines · Aneesh V

Quoted 3 lines · Sughosh Ganu

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

Quoted 3 lines

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

Sughosh,

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

Hi Aneesh,

Quoted 1 line · Aneesh V

Sughosh,

[...]

Quoted 2 lines

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

Quoted 5 lines · Christian Riesch

Quoted 1 line · On Fri, Jan 20, 2012 at 9:52 AM, Aneesh V<[email protected]> ?wrote:

Sughosh,

[...]

Quoted 1 line

Quoted 7 lines

Quoted 2 lines

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.

Hi Aneesh,

Quoted 19 lines · Aneesh V

Quoted 5 lines · Christian Riesch

Quoted 1 line · Aneesh V

Sughosh,

[...]

Quoted 1 line

Quoted 7 lines

Quoted 2 lines

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:

Quoted 5 lines

        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

Hi Christian,

Quoted 39 lines · Christian Riesch

Hi Aneesh,

Quoted 26 lines · On Fri, Jan 20, 2012 at 1:13 PM, Aneesh V<[email protected]> ?wrote:

Quoted 9 lines · Christian Riesch

Quoted 2 lines · On Fri, Jan 20, 2012 at 9:52 AM, Aneesh V<[email protected]> ? ?wrote:

Sughosh,

[...]

Quoted 2 lines

Quoted 8 lines

Quoted 2 lines

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:

Quoted 5 lines

? ? ? ?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!

Quoted 7 lines

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!

Hi all,

Quoted 11 lines · Tom Rini

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?

Hello Christian,

Quoted 38 lines · Christian Riesch

Hi all,

Quoted 11 lines · Tom Rini

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

Quoted 7 lines

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.

Quoted 8 lines

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 ...

Quoted 9 lines

Quoted 8 lines

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?

Quoted 7 lines

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.

Quoted 7 lines · Christian Riesch

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 ...

Hi all,

Quoted 11 lines · Tom Rini

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

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?

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?

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.

Quoted 4 lines · Christian Riesch

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

Quoted 16 lines · Aneesh V

Quoted 14 lines · Christian Riesch

Quoted 9 lines · On Thu, Jan 19, 2012 at 11:17 AM, Aneesh V<[email protected]> ?wrote:

Quoted 3 lines · Sughosh Ganu

? 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

Quoted 3 lines

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?

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).

Quoted 7 lines · 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.

[...]

Quoted 15 lines

arch/arm/cpu/arm926ejs/start.S +3 -2

index 6a09c02..6e261c2 100644
@@ -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

arch/arm/cpu/arm926ejs/start.S _dynsym_start_ofs: · line 364

Quoted 2 lines

arch/arm/cpu/arm926ejs/start.S +2 -0

+       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.

arch/arm/cpu/arm926ejs/start.S _dynsym_start_ofs: · line 371

Quoted 8 lines

arch/arm/cpu/arm926ejs/start.S +1 -1

       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.

arch/arm/cpu/arm926ejs/start.S _dynsym_start_ofs: · line 374

Quoted 4 lines

arch/arm/cpu/arm926ejs/start.S +0 -0

       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].

Quoted 10 lines

Quoted 9 lines

       mcr     p15, 0, r0, c8, c7, 0   /* flush v4 TLB */

       /*
        * disable MMU stuff and caches
        */
       mrc     p15, 0, r0, c1, c0, 0

arch/arm/cpu/arm926ejs/start.S +1 -1

-       bic     r0, r0, #0x00002300     /* clear bits 13, 9:8 (--V- --RS) */
+       bic     r0, r0, #0x00000300     /* clear bits 9:8 ( --RS) */

Quoted 1 line

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.

Quoted 5 lines

Quoted 4 lines

       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 */

Quoted 4 lines

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.

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<[email protected]>

---

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(-)

arch/arm/cpu/arm926ejs/start.S +5 -3

index 6a09c02..91a9325 100644
@@ -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
+

Quoted 5 lines

arch/arm/cpu/arm926ejs/start.S +1 -1

 	/*
-	 * disable MMU stuff and caches
+	 * disable MMU and D cache, and enable I cache.
 	 */
 	mrc	p15, 0, r0, c1, c0, 0

arch/arm/cpu/arm926ejs/start.S _dynsym_start_ofs: · line 372

Quoted 2 lines

arch/arm/cpu/arm926ejs/start.S +1 -1

-	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.

hi Albert,

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

<snip>

Quoted 5 lines

Quoted 5 lines

/*

arch/arm/cpu/arm926ejs/start.S +1 -1

-	 * disable MMU stuff and caches
+	 * disable MMU and D cache, and enable I cache.
  	 */
  	mrc	p15, 0, r0, c1, c0, 0

Quoted 8 lines

arch/arm/cpu/arm926ejs/start.S _dynsym_start_ofs: · line 372

Quoted 2 lines

arch/arm/cpu/arm926ejs/start.S +1 -1

-	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

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<[email protected]>

Cc Albert Aribaud<[email protected]>

Cc Tom Rini<[email protected]>

---

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(-)

arch/arm/cpu/arm926ejs/start.S +0 -0

index 6a09c02..d64165a 100644

arch/arm/cpu/arm926ejs/start.S +7 -1

+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) */

Le 14/01/2012 15:02, Sughosh Ganu a ?crit :

Quoted 25 lines

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<[email protected]>

Cc Albert Aribaud<[email protected]>

Cc Tom Rini<[email protected]>

---

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

Review

Quoted 4 lines

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

(quoted diff) +0 -0

Quoted 7 lines

index 6a09c02..d64165a 100644

arch/arm/cpu/arm926ejs/start.S +0 -0

@@ -355,14 +355,18 @@ _dynsym_start_ofs:
  */
 cpu_init_crit:
       /*

arch/arm/cpu/arm926ejs/start.S _dynsym_start_ofs: · line 362

Quoted 6 lines

arch/arm/cpu/arm926ejs/start.S +1 -3

-        * 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:

Quoted 3 lines

arch/arm/cpu/arm926ejs/start.S +3 -0

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

Hi Albert,

Quoted 26 lines · Albert ARIBAUD

Le 14/01/2012 15:02, Sughosh Ganu a ?crit :

Quoted 25 lines

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<[email protected]>

Cc Albert Aribaud<[email protected]>

Cc Tom Rini<[email protected]>

---

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

Quoted 4 lines

Review

Quoted 4 lines

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

(quoted diff) +0 -0

b/arch/arm/cpu/arm926ejs/start.S

Quoted 16 lines

arch/arm/cpu/arm926ejs/start.S _dynsym_start_ofs: · line 362

Quoted 13 lines

index 6a09c02..d64165a 100644

arch/arm/cpu/arm926ejs/start.S +1 -3

@@ -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:

Quoted 3 lines

arch/arm/cpu/arm926ejs/start.S _dynsym_start_ofs: · line 363

Quoted 3 lines

arch/arm/cpu/arm926ejs/start.S +3 -0

+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

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 <[email protected]>

Cc Albert Aribaud <[email protected]>

---

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(-)

arch/arm/cpu/arm926ejs/start.S +6 -3

index 6a09c02..6e261c2 100644
@@ -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