Re: [U-Boot] [PATCH] mtd: nand: mxs: fix cache alignment for cache lines >32

[Hannes Schmelzer]

On 08/03/2016 08:09 PM, Tom Rini wrote:

On Wed, Aug 03, 2016 at 02:39:47PM -0300, Fabio Estevam wrote:

On Wed, Aug 3, 2016 at 1:13 PM, Stefan Agner  wrote:


As you noted, this particular case is due to cache flush of the page
table and should be fixed with:
arm: cache: always flush cache line size for page table

Yes, just noticed that on a imx7d-sdb I still get these cache warnings
even with "arm: cache: always flush cache line size for page table"
applied:


Filename 'zImage'.
Load address: 0x8080
Loading: #
  #
  #
  #
  #
  #
  #
  #
  9.1 MiB/s
done
Bytes transferred = 7334512 (6fea70 hex)
CACHE: Misaligned operation at range [8080, 80efea70]

I feel like we must have done something wrong of late, can you bisect
when these came in?  Thanks!
The mistake(s) must have been long time ago, but since commit 
bcc53bf095893fbdae531a9a7b5d4ef4a125a7fc it comes out visible.


I think we have to check every those warning for correct alignment.

cheers,
Hannes

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] mtd: nand: mxs: fix cache alignment for cache lines >32

[Stefan Agner]

On 2016-08-03 12:08, Simon Glass wrote:
> Hi Fabio,
> 
> On 3 August 2016 at 12:44, Fabio Estevam  wrote:
>> Hi Simon,
>>
>> On Wed, Aug 3, 2016 at 3:35 PM, Simon Glass  wrote:
>>
>>> Actually I think these are bugs and should be fixed. In this case,
>>> from what I can tell netboot_common() should cache-align the size in
>>> the call to:
>>>
>>> /* flush cache */
>>> flush_cache(load_addr, size);
>>
>> Do you mean like this?
>>
>> --- a/cmd/net.c
>> +++ b/cmd/net.c
>> @@ -244,6 +244,8 @@ static int netboot_common(enum proto_t proto, cmd_tbl_t 
>> *cmd
>> }
>>
>> /* flush cache */
>> +   load_addr &= ~(CONFIG_SYS_CACHELINE_SIZE - 1);
>> +   size = ALIGN(size, CONFIG_SYS_CACHELINE_SIZE);
>> flush_cache(load_addr, size);
>>
>> bootstage_mark(BOOTSTAGE_ID_NET_LOADED);
>>
>> This makes the net warnings go away.
>>
>> There is still this one that I am seeing:
>>
>> Kernel image @ 0x8080 [ 0x00 - 0x6fea70 ]
>> ## Flattened Device Tree blob at 8300
>>Booting using the fdt blob at 0x8300
>>Using Device Tree in place at 8300, end 83009c5d
>>
>> Starting kernel ...
>>
>> CACHE: Misaligned operation at range [0090, 00900529]
>> [0.00] Booting Linux on physical CPU 0x0
>>
>> Any ideas where it may come from?
> 
> Not really...maybe boot_ramdisk_high()?
> 
> It's clearly pretty late in the process.

That looks like a SRAM address, and it is used for the secure stuff for
instance (CONFIG_ARMV7_SECURE_BASE).

I bet its from relocate_secure_section in arch/arm/cpu/armv7/virt-v7.c.

Will send out a patch for that.

--
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] mtd: nand: mxs: fix cache alignment for cache lines >32

[Stefan Agner]

On 2016-08-03 11:44, Fabio Estevam wrote:
> Hi Simon,
> 
> On Wed, Aug 3, 2016 at 3:35 PM, Simon Glass  wrote:
> 
>> Actually I think these are bugs and should be fixed. In this case,

I am completely with you Simon!

check_cache_range return 0, which actually leads to the cache operation
not being executed at all! Depending on the situation this can be quite
catastrophic...


>> from what I can tell netboot_common() should cache-align the size in
>> the call to:
>>
>> /* flush cache */
>> flush_cache(load_addr, size);
> 
> Do you mean like this?
> 
> --- a/cmd/net.c
> +++ b/cmd/net.c
> @@ -244,6 +244,8 @@ static int netboot_common(enum proto_t proto, cmd_tbl_t 
> *cmd
> }
> 
> /* flush cache */
> +   load_addr &= ~(CONFIG_SYS_CACHELINE_SIZE - 1);
> +   size = ALIGN(size, CONFIG_SYS_CACHELINE_SIZE);
> flush_cache(load_addr, size);
> 
> bootstage_mark(BOOTSTAGE_ID_NET_LOADED);
> 
> This makes the net warnings go away.

Yes, see my patch too.
https://patchwork.ozlabs.org/patch/654585/

So up until now, that stuff did not get flushed whenever the file size
was not cache line aligned, and nobody noticed... Is that cache flush
necessary at all?


> 
> There is still this one that I am seeing:
> 
> Kernel image @ 0x8080 [ 0x00 - 0x6fea70 ]
> ## Flattened Device Tree blob at 8300
>Booting using the fdt blob at 0x8300
>Using Device Tree in place at 8300, end 83009c5d
> 
> Starting kernel ...
> 
> CACHE: Misaligned operation at range [0090, 00900529]
> [0.00] Booting Linux on physical CPU 0x0
> 
> Any ideas where it may come from?

I did not had that one on i.MX 7

--
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] mtd: nand: mxs: fix cache alignment for cache lines >32

[Simon Glass]

Hi Fabio,

On 3 August 2016 at 12:44, Fabio Estevam  wrote:
> Hi Simon,
>
> On Wed, Aug 3, 2016 at 3:35 PM, Simon Glass  wrote:
>
>> Actually I think these are bugs and should be fixed. In this case,
>> from what I can tell netboot_common() should cache-align the size in
>> the call to:
>>
>> /* flush cache */
>> flush_cache(load_addr, size);
>
> Do you mean like this?
>
> --- a/cmd/net.c
> +++ b/cmd/net.c
> @@ -244,6 +244,8 @@ static int netboot_common(enum proto_t proto, cmd_tbl_t 
> *cmd
> }
>
> /* flush cache */
> +   load_addr &= ~(CONFIG_SYS_CACHELINE_SIZE - 1);
> +   size = ALIGN(size, CONFIG_SYS_CACHELINE_SIZE);
> flush_cache(load_addr, size);
>
> bootstage_mark(BOOTSTAGE_ID_NET_LOADED);
>
> This makes the net warnings go away.
>
> There is still this one that I am seeing:
>
> Kernel image @ 0x8080 [ 0x00 - 0x6fea70 ]
> ## Flattened Device Tree blob at 8300
>Booting using the fdt blob at 0x8300
>Using Device Tree in place at 8300, end 83009c5d
>
> Starting kernel ...
>
> CACHE: Misaligned operation at range [0090, 00900529]
> [0.00] Booting Linux on physical CPU 0x0
>
> Any ideas where it may come from?

Not really...maybe boot_ramdisk_high()?

It's clearly pretty late in the process.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] mtd: nand: mxs: fix cache alignment for cache lines >32

[Fabio Estevam]

Hi Simon,

On Wed, Aug 3, 2016 at 3:35 PM, Simon Glass  wrote:

> Actually I think these are bugs and should be fixed. In this case,
> from what I can tell netboot_common() should cache-align the size in
> the call to:
>
> /* flush cache */
> flush_cache(load_addr, size);

Do you mean like this?

--- a/cmd/net.c
+++ b/cmd/net.c
@@ -244,6 +244,8 @@ static int netboot_common(enum proto_t proto, cmd_tbl_t *cmd
}

/* flush cache */
+   load_addr &= ~(CONFIG_SYS_CACHELINE_SIZE - 1);
+   size = ALIGN(size, CONFIG_SYS_CACHELINE_SIZE);
flush_cache(load_addr, size);

bootstage_mark(BOOTSTAGE_ID_NET_LOADED);

This makes the net warnings go away.

There is still this one that I am seeing:

Kernel image @ 0x8080 [ 0x00 - 0x6fea70 ]
## Flattened Device Tree blob at 8300
   Booting using the fdt blob at 0x8300
   Using Device Tree in place at 8300, end 83009c5d

Starting kernel ...

CACHE: Misaligned operation at range [0090, 00900529]
[0.00] Booting Linux on physical CPU 0x0

Any ideas where it may come from?

Thanks
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] mtd: nand: mxs: fix cache alignment for cache lines >32

[Simon Glass]

Hi,

On 3 August 2016 at 12:29, Fabio Estevam  wrote:
> Hi Tom,
>
> On Wed, Aug 3, 2016 at 3:09 PM, Tom Rini  wrote:
>
>> I feel like we must have done something wrong of late, can you bisect
>> when these came in?  Thanks!
>
> This cache warnings start to appear since commit:
>
> commit bcc53bf095893fbdae531a9a7b5d4ef4a125a7fc
> Author: Simon Glass 
> Date:   Sun Jun 19 19:43:05 2016 -0600
>
> arm: Show cache warnings in U-Boot proper only
>
> Avoid bloating the SPL image size.
>
> Signed-off-by: Simon Glass 
>
> Prior to this commit the cache warnings would be printed only with
> DEBUG enabled. Now they are always enabled when we build a non-spl
> target.
>
> We could restore the original behavior and also keep Simon's intention
> of not bloating the SPL image size with the following change:
>
> --- a/arch/arm/lib/cache.c
> +++ b/arch/arm/lib/cache.c
> @@ -60,10 +60,11 @@ int check_cache_range(unsigned long start, unsigned long 
> sto
> if (stop & (CONFIG_SYS_CACHELINE_SIZE - 1))
> ok = 0;
>
> -   if (!ok) {
> -   warn_non_spl("CACHE: Misaligned operation at range [%08lx, 
> %08lx
> -start, stop);
> -   }
> +#ifndef CONFIG_SPL_BUILD
> +   if (!ok)
> +   debug("CACHE: Misaligned operation at range [%08lx, %08lx]\n",
> + start, stop);
> +#endif
>
> return ok;
>  }
>
> If this looks OK, I can submit a formal patch.

Actually I think these are bugs and should be fixed. In this case,
from what I can tell netboot_common() should cache-align the size in
the call to:

/* flush cache */
flush_cache(load_addr, size);

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] mtd: nand: mxs: fix cache alignment for cache lines >32

[Fabio Estevam]

Hi Tom,

On Wed, Aug 3, 2016 at 3:09 PM, Tom Rini  wrote:

> I feel like we must have done something wrong of late, can you bisect
> when these came in?  Thanks!

This cache warnings start to appear since commit:

commit bcc53bf095893fbdae531a9a7b5d4ef4a125a7fc
Author: Simon Glass 
Date:   Sun Jun 19 19:43:05 2016 -0600

arm: Show cache warnings in U-Boot proper only

Avoid bloating the SPL image size.

Signed-off-by: Simon Glass 

Prior to this commit the cache warnings would be printed only with
DEBUG enabled. Now they are always enabled when we build a non-spl
target.

We could restore the original behavior and also keep Simon's intention
of not bloating the SPL image size with the following change:

--- a/arch/arm/lib/cache.c
+++ b/arch/arm/lib/cache.c
@@ -60,10 +60,11 @@ int check_cache_range(unsigned long start, unsigned long sto
if (stop & (CONFIG_SYS_CACHELINE_SIZE - 1))
ok = 0;

-   if (!ok) {
-   warn_non_spl("CACHE: Misaligned operation at range [%08lx, %08lx
-start, stop);
-   }
+#ifndef CONFIG_SPL_BUILD
+   if (!ok)
+   debug("CACHE: Misaligned operation at range [%08lx, %08lx]\n",
+ start, stop);
+#endif

return ok;
 }

If this looks OK, I can submit a formal patch.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] mtd: nand: mxs: fix cache alignment for cache lines >32

[Tom Rini]

On Wed, Aug 03, 2016 at 02:39:47PM -0300, Fabio Estevam wrote:
> On Wed, Aug 3, 2016 at 1:13 PM, Stefan Agner  wrote:
> 
> > As you noted, this particular case is due to cache flush of the page
> > table and should be fixed with:
> > arm: cache: always flush cache line size for page table
> 
> Yes, just noticed that on a imx7d-sdb I still get these cache warnings
> even with "arm: cache: always flush cache line size for page table"
> applied:
> 
> 
> Filename 'zImage'.
> Load address: 0x8080
> Loading: #
>  #
>  #
>  #
>  #
>  #
>  #
>  #
>  9.1 MiB/s
> done
> Bytes transferred = 7334512 (6fea70 hex)
> CACHE: Misaligned operation at range [8080, 80efea70]

I feel like we must have done something wrong of late, can you bisect
when these came in?  Thanks!

-- 
Tom


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] mtd: nand: mxs: fix cache alignment for cache lines >32

[Fabio Estevam]

On Wed, Aug 3, 2016 at 1:13 PM, Stefan Agner  wrote:

> As you noted, this particular case is due to cache flush of the page
> table and should be fixed with:
> arm: cache: always flush cache line size for page table

Yes, just noticed that on a imx7d-sdb I still get these cache warnings
even with "arm: cache: always flush cache line size for page table"
applied:


Filename 'zImage'.
Load address: 0x8080
Loading: #
 #
 #
 #
 #
 #
 #
 #
 9.1 MiB/s
done
Bytes transferred = 7334512 (6fea70 hex)
CACHE: Misaligned operation at range [8080, 80efea70]
BOOTP broadcast 1
*** Unhandled DHCP Option in OFFER/ACK: 44
*** Unhandled DHCP Option in OFFER/ACK: 46
*** Unhandled DHCP Option in OFFER/ACK: 44
*** Unhandled DHCP Option in OFFER/ACK: 46
DHCP client bound to address 10.29.244.54 (356 ms)
Using FEC0 device
TFTP from server 10.29.244.110; our IP address is 10.29.244.54
Filename 'imx7d-sdb.dtb'.
Load address: 0x8300
Loading: ##
 376 KiB/s
done
Bytes transferred = 27742 (6c5e hex)
CACHE: Misaligned operation at range [8300, 83006c5e]
Kernel image @ 0x8080 [ 0x00 - 0x6fea70 ]
## Flattened Device Tree blob at 8300
   Booting using the fdt blob at 0x8300
   Using Device Tree in place at 8300, end 83009c5d

Starting kernel ...

CACHE: Misaligned operation at range [0090, 00900529]
[0.00] Booting Linux on physical CPU 0x0
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] mtd: nand: mxs: fix cache alignment for cache lines >32

[Stefan Agner]

On 2016-08-03 06:51, Fabio Estevam wrote:
> On Tue, Aug 2, 2016 at 3:55 AM, Stefan Agner  wrote:
>> From: Stefan Agner 
>>
>> Currently the command buffer gets allocated with a size of 32 bytes.
>> This causes warning messages on systems with cache lines bigger than
>> 32 bytes:
>> CACHE: Misaligned operation at range [9df17a00, 9df17a20]
>>
>> Define command buffer to be at least 32 bytes, but more if cache
>> line is bigger.
>>
>> Signed-off-by: Stefan Agner 
>> ---
>> This appeared after Simon enable the message in check_cache_range
>> by default...
> 
> On mx6ul pico I also get similar warnings even though NAND is not used
> on this board:
> 
> U-Boot 2016.09-rc1-00245-gad6a303 (Aug 03 2016 - 10:31:52 -0300)
> 
> CPU:   Freescale i.MX6UL rev1.0 at 396 MHz
> Reset cause: WDOG
> Board: PICO-IMX6UL-EMMC
> I2C:   ready
> DRAM:  256 MiB
> CACHE: Misaligned operation at range [8fff, 8fff0004]
> CACHE: Misaligned operation at range [8fff0024, 8fff0028]
> PMIC: PFUZE3000 DEV_ID=0x30 REV_ID=0x11
> MMC:   FSL_SDHC: 0
> *** Warning - bad CRC, using default environment
> 
> In:serial
> Out:   serial
> Err:   serial
> Net:   FEC
> Hit any key to stop autoboot:  0
> 
> Looks like we need a more generic fix?

As you noted, this particular case is due to cache flush of the page
table and should be fixed with:
arm: cache: always flush cache line size for page table

Afaik, there is no such thing as a generic fix for cache line alignment
issues... Every call site need to make sure to keep data cache line
aligned, especially in case a external device (DMA) are modifying the
same data...

--
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] mtd: nand: mxs: fix cache alignment for cache lines >32

[Fabio Estevam]

On Tue, Aug 2, 2016 at 3:55 AM, Stefan Agner  wrote:
> From: Stefan Agner 
>
> Currently the command buffer gets allocated with a size of 32 bytes.
> This causes warning messages on systems with cache lines bigger than
> 32 bytes:
> CACHE: Misaligned operation at range [9df17a00, 9df17a20]
>
> Define command buffer to be at least 32 bytes, but more if cache
> line is bigger.
>
> Signed-off-by: Stefan Agner 
> ---
> This appeared after Simon enable the message in check_cache_range
> by default...

On mx6ul pico I also get similar warnings even though NAND is not used
on this board:

U-Boot 2016.09-rc1-00245-gad6a303 (Aug 03 2016 - 10:31:52 -0300)

CPU:   Freescale i.MX6UL rev1.0 at 396 MHz
Reset cause: WDOG
Board: PICO-IMX6UL-EMMC
I2C:   ready
DRAM:  256 MiB
CACHE: Misaligned operation at range [8fff, 8fff0004]
CACHE: Misaligned operation at range [8fff0024, 8fff0028]
PMIC: PFUZE3000 DEV_ID=0x30 REV_ID=0x11
MMC:   FSL_SDHC: 0
*** Warning - bad CRC, using default environment

In:serial
Out:   serial
Err:   serial
Net:   FEC
Hit any key to stop autoboot:  0

Looks like we need a more generic fix?

Thanks
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] mtd: nand: mxs: fix cache alignment for cache lines >32

[Stefano Babic]

On 02/08/2016 08:55, Stefan Agner wrote:
> From: Stefan Agner 
> 
> Currently the command buffer gets allocated with a size of 32 bytes.
> This causes warning messages on systems with cache lines bigger than
> 32 bytes:
> CACHE: Misaligned operation at range [9df17a00, 9df17a20]
> 
> Define command buffer to be at least 32 bytes, but more if cache
> line is bigger.
> 
> Signed-off-by: Stefan Agner 
> ---


Applied to -u-boot-imx, thanks !

Best regards,
Stefano Babic


-- 
=
DENX Software Engineering GmbH,  Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de
=
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] mtd: nand: mxs: fix cache alignment for cache lines >32

[Stefano Babic]

Hi Stefan,

On 02/08/2016 08:55, Stefan Agner wrote:
> From: Stefan Agner 
> 
> Currently the command buffer gets allocated with a size of 32 bytes.
> This causes warning messages on systems with cache lines bigger than
> 32 bytes:
> CACHE: Misaligned operation at range [9df17a00, 9df17a20]
> 

I've never seen this on NAND

> Define command buffer to be at least 32 bytes, but more if cache
> line is bigger.
> 
> Signed-off-by: Stefan Agner 
> ---
> This appeared after Simon enable the message in check_cache_range
> by default...

ok, this explains why !

> 
>  drivers/mtd/nand/mxs_nand.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c
> index 94fc5c1..4bf564e 100644
> --- a/drivers/mtd/nand/mxs_nand.c
> +++ b/drivers/mtd/nand/mxs_nand.c
> @@ -37,7 +37,12 @@
>  #endif
>  #define  MXS_NAND_METADATA_SIZE  10
>  #define  MXS_NAND_BITS_PER_ECC_LEVEL 13
> +
> +#if !defined(CONFIG_SYS_CACHELINE_SIZE) || CONFIG_SYS_CACHELINE_SIZE < 32
>  #define  MXS_NAND_COMMAND_BUFFER_SIZE32
> +#else
> +#define  MXS_NAND_COMMAND_BUFFER_SIZE
> CONFIG_SYS_CACHELINE_SIZE
> +#endif
>  
>  #define  MXS_NAND_BCH_TIMEOUT1
>  
> 

Reviewed-by: Stefano Babic 

Best regards,
Stefano Babic

-- 
=
DENX Software Engineering GmbH,  Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de
=
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] mtd: nand: mxs: fix cache alignment for cache lines >32

[Stefan Agner]

From: Stefan Agner 

Currently the command buffer gets allocated with a size of 32 bytes.
This causes warning messages on systems with cache lines bigger than
32 bytes:
CACHE: Misaligned operation at range [9df17a00, 9df17a20]

Define command buffer to be at least 32 bytes, but more if cache
line is bigger.

Signed-off-by: Stefan Agner 
---
This appeared after Simon enable the message in check_cache_range
by default...

 drivers/mtd/nand/mxs_nand.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/mtd/nand/mxs_nand.c b/drivers/mtd/nand/mxs_nand.c
index 94fc5c1..4bf564e 100644
--- a/drivers/mtd/nand/mxs_nand.c
+++ b/drivers/mtd/nand/mxs_nand.c
@@ -37,7 +37,12 @@
 #endif
 #defineMXS_NAND_METADATA_SIZE  10
 #defineMXS_NAND_BITS_PER_ECC_LEVEL 13
+
+#if !defined(CONFIG_SYS_CACHELINE_SIZE) || CONFIG_SYS_CACHELINE_SIZE < 32
 #defineMXS_NAND_COMMAND_BUFFER_SIZE32
+#else
+#defineMXS_NAND_COMMAND_BUFFER_SIZE
CONFIG_SYS_CACHELINE_SIZE
+#endif
 
 #defineMXS_NAND_BCH_TIMEOUT1
 
-- 
2.9.0

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot