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

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

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

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 <stefan.agner@toradex.com>
---
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
 #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_SIZE		32
+#else
+#define	MXS_NAND_COMMAND_BUFFER_SIZE		CONFIG_SYS_CACHELINE_SIZE
+#endif
 
 #define	MXS_NAND_BCH_TIMEOUT			10000
 
-- 
2.9.0

[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 <stefan.agner@toradex.com>
> 
> 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 <stefan.agner@toradex.com>
> ---
> 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_SIZE		32
> +#else
> +#define	MXS_NAND_COMMAND_BUFFER_SIZE		CONFIG_SYS_CACHELINE_SIZE
> +#endif
>  
>  #define	MXS_NAND_BCH_TIMEOUT			10000
>  
> 

Reviewed-by: Stefano Babic <sbabic@denx.de>

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: sbabic at denx.de
=====================================================================

[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 <stefan.agner@toradex.com>
> 
> 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 <stefan.agner@toradex.com>
> ---


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: sbabic at denx.de
=====================================================================

[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 <stefan@agner.ch> wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
>
> 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 <stefan.agner@toradex.com>
> ---
> 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 [8fff0000, 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] [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 <stefan@agner.ch> wrote:
>> From: Stefan Agner <stefan.agner@toradex.com>
>>
>> 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 <stefan.agner@toradex.com>
>> ---
>> 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 [8fff0000, 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] [PATCH] mtd: nand: mxs: fix cache alignment for cache lines >32

[Fabio Estevam]

On Wed, Aug 3, 2016 at 1:13 PM, Stefan Agner <stefan@agner.ch> 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: 0x80800000
Loading: #################################################################
         #################################################################
         #################################################################
         #################################################################
         #################################################################
         #################################################################
         #################################################################
         #############################################
         9.1 MiB/s
done
Bytes transferred = 7334512 (6fea70 hex)
CACHE: Misaligned operation at range [80800000, 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: 0x83000000
Loading: ##
         376 KiB/s
done
Bytes transferred = 27742 (6c5e hex)
CACHE: Misaligned operation at range [83000000, 83006c5e]
Kernel image @ 0x80800000 [ 0x000000 - 0x6fea70 ]
## Flattened Device Tree blob at 83000000
   Booting using the fdt blob at 0x83000000
   Using Device Tree in place at 83000000, end 83009c5d

Starting kernel ...

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

[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 <stefan@agner.ch> 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: 0x80800000
> Loading: #################################################################
>          #################################################################
>          #################################################################
>          #################################################################
>          #################################################################
>          #################################################################
>          #################################################################
>          #############################################
>          9.1 MiB/s
> done
> Bytes transferred = 7334512 (6fea70 hex)
> CACHE: Misaligned operation at range [80800000, 80efea70]

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160803/1ae80ae7/attachment.sig>

[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 <trini@konsulko.com> 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 <sjg@chromium.org>
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 <sjg@chromium.org>

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] [PATCH] mtd: nand: mxs: fix cache alignment for cache lines >32

[Simon Glass]

Hi,

On 3 August 2016 at 12:29, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Tom,
>
> On Wed, Aug 3, 2016 at 3:09 PM, Tom Rini <trini@konsulko.com> 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 <sjg@chromium.org>
> 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 <sjg@chromium.org>
>
> 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] [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 <sjg@chromium.org> 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 @ 0x80800000 [ 0x000000 - 0x6fea70 ]
## Flattened Device Tree blob at 83000000
   Booting using the fdt blob at 0x83000000
   Using Device Tree in place at 83000000, end 83009c5d

Starting kernel ...

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

Any ideas where it may come from?

Thanks

[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 <festevam@gmail.com> wrote:
> Hi Simon,
>
> On Wed, Aug 3, 2016 at 3:35 PM, Simon Glass <sjg@chromium.org> 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 @ 0x80800000 [ 0x000000 - 0x6fea70 ]
> ## Flattened Device Tree blob at 83000000
>    Booting using the fdt blob at 0x83000000
>    Using Device Tree in place at 83000000, end 83009c5d
>
> Starting kernel ...
>
> CACHE: Misaligned operation at range [00900000, 00900529]
> [    0.000000] 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] [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 <sjg@chromium.org> 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 @ 0x80800000 [ 0x000000 - 0x6fea70 ]
> ## Flattened Device Tree blob at 83000000
>    Booting using the fdt blob at 0x83000000
>    Using Device Tree in place at 83000000, end 83009c5d
> 
> Starting kernel ...
> 
> CACHE: Misaligned operation at range [00900000, 00900529]
> [    0.000000] 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] [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 <festevam@gmail.com> wrote:
>> Hi Simon,
>>
>> On Wed, Aug 3, 2016 at 3:35 PM, Simon Glass <sjg@chromium.org> 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 @ 0x80800000 [ 0x000000 - 0x6fea70 ]
>> ## Flattened Device Tree blob at 83000000
>>    Booting using the fdt blob at 0x83000000
>>    Using Device Tree in place at 83000000, end 83009c5d
>>
>> Starting kernel ...
>>
>> CACHE: Misaligned operation at range [00900000, 00900529]
>> [    0.000000] 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] [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 <stefan@agner.ch> 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: 0x80800000
>> Loading: #################################################################
>>           #################################################################
>>           #################################################################
>>           #################################################################
>>           #################################################################
>>           #################################################################
>>           #################################################################
>>           #############################################
>>           9.1 MiB/s
>> done
>> Bytes transferred = 7334512 (6fea70 hex)
>> CACHE: Misaligned operation at range [80800000, 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