[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Marek Vasut]

Replace the ad-hoc DMA cache management functions with common bouncebuf,
since those functions are not handling cases where unaligned buffer is
passed in, while common bouncebuf does handle all that.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Tom Rini <trini@konsulko.com>
---
 drivers/mtd/nand/denali.c | 41 +++++++----------------------------------
 1 file changed, 7 insertions(+), 34 deletions(-)

diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 089ebce6dd..e5e84a58aa 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2009-2010, Intel Corporation and its suppliers.
  */
 
+#include <bouncebuf.h>
 #include <dm.h>
 #include <nand.h>
 #include <linux/bitfield.h>
@@ -16,31 +17,6 @@
 
 #include "denali.h"
 
-static dma_addr_t dma_map_single(void *dev, void *ptr, size_t size,
-				 enum dma_data_direction dir)
-{
-	unsigned long addr = (unsigned long)ptr;
-
-	if (dir == DMA_FROM_DEVICE)
-		invalidate_dcache_range(addr, addr + size);
-	else
-		flush_dcache_range(addr, addr + size);
-
-	return addr;
-}
-
-static void dma_unmap_single(void *dev, dma_addr_t addr, size_t size,
-			     enum dma_data_direction dir)
-{
-	if (dir != DMA_TO_DEVICE)
-		invalidate_dcache_range(addr, addr + size);
-}
-
-static int dma_mapping_error(void *dev, dma_addr_t addr)
-{
-	return 0;
-}
-
 #define DENALI_NAND_NAME    "denali-nand"
 
 /* for Indexed Addressing */
@@ -563,16 +539,12 @@ static int denali_pio_xfer(struct denali_nand_info *denali, void *buf,
 static int denali_dma_xfer(struct denali_nand_info *denali, void *buf,
 			   size_t size, int page, int raw, int write)
 {
-	dma_addr_t dma_addr;
 	uint32_t irq_mask, irq_status, ecc_err_mask;
-	enum dma_data_direction dir = write ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
+	unsigned int bbflags = write ? GEN_BB_READ : GEN_BB_WRITE;
+	struct bounce_buffer bbstate;
 	int ret = 0;
 
-	dma_addr = dma_map_single(denali->dev, buf, size, dir);
-	if (dma_mapping_error(denali->dev, dma_addr)) {
-		dev_dbg(denali->dev, "Failed to DMA-map buffer. Trying PIO.\n");
-		return denali_pio_xfer(denali, buf, size, page, raw, write);
-	}
+	bounce_buffer_start(&bbstate, buf, size, bbflags);
 
 	if (write) {
 		/*
@@ -593,7 +565,8 @@ static int denali_dma_xfer(struct denali_nand_info *denali, void *buf,
 	iowrite32(DMA_ENABLE__FLAG, denali->reg + DMA_ENABLE);
 
 	denali_reset_irq(denali);
-	denali->setup_dma(denali, dma_addr, page, write);
+	denali->setup_dma(denali, virt_to_phys(bbstate.bounce_buffer),
+			  page, write);
 
 	irq_status = denali_wait_for_irq(denali, irq_mask);
 	if (!(irq_status & INTR__DMA_CMD_COMP))
@@ -603,7 +576,7 @@ static int denali_dma_xfer(struct denali_nand_info *denali, void *buf,
 
 	iowrite32(0, denali->reg + DMA_ENABLE);
 
-	dma_unmap_single(denali->dev, dma_addr, size, dir);
+	bounce_buffer_stop(&bbstate);
 
 	if (irq_status & INTR__ERASED_PAGE)
 		memset(buf, 0xff, size);
-- 
2.16.2

[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Masahiro Yamada]

Hi Marek,


2018-06-08 5:17 GMT+09:00 Marek Vasut <marex@denx.de>:
> Replace the ad-hoc DMA cache management functions with common bouncebuf,
> since those functions are not handling cases where unaligned buffer is
> passed in,


Were you hit by a problem,
or just-in-case replacement?


I thought I took care of the buffer alignment.

The bounce buffer is allocated by kmalloc():
https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1348

According to the lib/linux_compat.c implementation,
it returns memory aligned with ARCH_DMA_MINALIGN.


If the buffer is passed from the upper MTD layer,
the NAND core also makes sure the enough alignment:
https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1273

This is how this driver works in Linux.

I'd rather want to keep the current code
unless this is a real problem,


One possible clean-up is to move dma_(un)map_single to a common place.


> while common bouncebuf does handle all that.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Tom Rini <trini@konsulko.com>
> ---
>  drivers/mtd/nand/denali.c | 41 +++++++----------------------------------
>  1 file changed, 7 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index 089ebce6dd..e5e84a58aa 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -5,6 +5,7 @@
>   * Copyright (C) 2009-2010, Intel Corporation and its suppliers.
>   */
>
> +#include <bouncebuf.h>
>  #include <dm.h>
>  #include <nand.h>
>  #include <linux/bitfield.h>
> @@ -16,31 +17,6 @@
>
>  #include "denali.h"
>
> -static dma_addr_t dma_map_single(void *dev, void *ptr, size_t size,
> -                                enum dma_data_direction dir)
> -{
> -       unsigned long addr = (unsigned long)ptr;
> -
> -       if (dir == DMA_FROM_DEVICE)
> -               invalidate_dcache_range(addr, addr + size);
> -       else
> -               flush_dcache_range(addr, addr + size);
> -
> -       return addr;
> -}
> -
> -static void dma_unmap_single(void *dev, dma_addr_t addr, size_t size,
> -                            enum dma_data_direction dir)
> -{
> -       if (dir != DMA_TO_DEVICE)
> -               invalidate_dcache_range(addr, addr + size);
> -}
> -
> -static int dma_mapping_error(void *dev, dma_addr_t addr)
> -{
> -       return 0;
> -}
> -
>  #define DENALI_NAND_NAME    "denali-nand"
>
>  /* for Indexed Addressing */
> @@ -563,16 +539,12 @@ static int denali_pio_xfer(struct denali_nand_info *denali, void *buf,
>  static int denali_dma_xfer(struct denali_nand_info *denali, void *buf,
>                            size_t size, int page, int raw, int write)
>  {
> -       dma_addr_t dma_addr;
>         uint32_t irq_mask, irq_status, ecc_err_mask;
> -       enum dma_data_direction dir = write ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
> +       unsigned int bbflags = write ? GEN_BB_READ : GEN_BB_WRITE;
> +       struct bounce_buffer bbstate;
>         int ret = 0;
>
> -       dma_addr = dma_map_single(denali->dev, buf, size, dir);
> -       if (dma_mapping_error(denali->dev, dma_addr)) {
> -               dev_dbg(denali->dev, "Failed to DMA-map buffer. Trying PIO.\n");
> -               return denali_pio_xfer(denali, buf, size, page, raw, write);
> -       }
> +       bounce_buffer_start(&bbstate, buf, size, bbflags);
>
>         if (write) {
>                 /*
> @@ -593,7 +565,8 @@ static int denali_dma_xfer(struct denali_nand_info *denali, void *buf,
>         iowrite32(DMA_ENABLE__FLAG, denali->reg + DMA_ENABLE);
>
>         denali_reset_irq(denali);
> -       denali->setup_dma(denali, dma_addr, page, write);
> +       denali->setup_dma(denali, virt_to_phys(bbstate.bounce_buffer),
> +                         page, write);
>
>         irq_status = denali_wait_for_irq(denali, irq_mask);
>         if (!(irq_status & INTR__DMA_CMD_COMP))
> @@ -603,7 +576,7 @@ static int denali_dma_xfer(struct denali_nand_info *denali, void *buf,
>
>         iowrite32(0, denali->reg + DMA_ENABLE);
>
> -       dma_unmap_single(denali->dev, dma_addr, size, dir);
> +       bounce_buffer_stop(&bbstate);
>
>         if (irq_status & INTR__ERASED_PAGE)
>                 memset(buf, 0xff, size);
> --
> 2.16.2
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot



-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Marek Vasut]

On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
> Hi Marek,

Hi,

> 2018-06-08 5:17 GMT+09:00 Marek Vasut <marex@denx.de>:
>> Replace the ad-hoc DMA cache management functions with common bouncebuf,
>> since those functions are not handling cases where unaligned buffer is
>> passed in,
> 
> 
> Were you hit by a problem,
> or just-in-case replacement?

Yes, UBI triggers unaligned cache operations on my system (SoCFPGA).
> I thought I took care of the buffer alignment.
> 
> The bounce buffer is allocated by kmalloc():
> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1348
> 
> According to the lib/linux_compat.c implementation,
> it returns memory aligned with ARCH_DMA_MINALIGN.
> 
> 
> If the buffer is passed from the upper MTD layer,
> the NAND core also makes sure the enough alignment:
> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1273
> 
> This is how this driver works in Linux.
> 
> I'd rather want to keep the current code
> unless this is a real problem,
> 
> 
> One possible clean-up is to move dma_(un)map_single to a common place.
Is there any chance you can try UBI on the denali nand on uniphier ? :)

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Masahiro Yamada]

Hi Marek,

2018-06-20 13:43 GMT+09:00 Marek Vasut <marex@denx.de>:
> On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
>> Hi Marek,
>
> Hi,
>
>> 2018-06-08 5:17 GMT+09:00 Marek Vasut <marex@denx.de>:
>>> Replace the ad-hoc DMA cache management functions with common bouncebuf,
>>> since those functions are not handling cases where unaligned buffer is
>>> passed in,
>>
>>
>> Were you hit by a problem,
>> or just-in-case replacement?
>
> Yes, UBI triggers unaligned cache operations on my system (SoCFPGA).
>> I thought I took care of the buffer alignment.
>>
>> The bounce buffer is allocated by kmalloc():
>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1348
>>
>> According to the lib/linux_compat.c implementation,
>> it returns memory aligned with ARCH_DMA_MINALIGN.
>>
>>
>> If the buffer is passed from the upper MTD layer,
>> the NAND core also makes sure the enough alignment:
>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1273
>>
>> This is how this driver works in Linux.
>>
>> I'd rather want to keep the current code
>> unless this is a real problem,
>>
>>
>> One possible clean-up is to move dma_(un)map_single to a common place.
> Is there any chance you can try UBI on the denali nand on uniphier ? :)


I tested the driver only for raw block devices.

OK, I will test UBI on my platform.

BTW, do you see the problem only in U-Boot?
Is the denali driver in Linux working fine?


-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Marek Vasut]

On 06/20/2018 09:14 AM, Masahiro Yamada wrote:
> Hi Marek,
> 
> 2018-06-20 13:43 GMT+09:00 Marek Vasut <marex@denx.de>:
>> On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> 2018-06-08 5:17 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>> Replace the ad-hoc DMA cache management functions with common bouncebuf,
>>>> since those functions are not handling cases where unaligned buffer is
>>>> passed in,
>>>
>>>
>>> Were you hit by a problem,
>>> or just-in-case replacement?
>>
>> Yes, UBI triggers unaligned cache operations on my system (SoCFPGA).
>>> I thought I took care of the buffer alignment.
>>>
>>> The bounce buffer is allocated by kmalloc():
>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1348
>>>
>>> According to the lib/linux_compat.c implementation,
>>> it returns memory aligned with ARCH_DMA_MINALIGN.
>>>
>>>
>>> If the buffer is passed from the upper MTD layer,
>>> the NAND core also makes sure the enough alignment:
>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1273
>>>
>>> This is how this driver works in Linux.
>>>
>>> I'd rather want to keep the current code
>>> unless this is a real problem,
>>>
>>>
>>> One possible clean-up is to move dma_(un)map_single to a common place.
>> Is there any chance you can try UBI on the denali nand on uniphier ? :)
> 
> 
> I tested the driver only for raw block devices.
> 
> OK, I will test UBI on my platform.

Please do. With plain block access it works fine.

> BTW, do you see the problem only in U-Boot?
> Is the denali driver in Linux working fine?

Yes, I only see it in U-Boot.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Marek Vasut]

On 06/20/2018 09:14 AM, Masahiro Yamada wrote:
> Hi Marek,

Hi!

> 2018-06-20 13:43 GMT+09:00 Marek Vasut <marex@denx.de>:
>> On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> 2018-06-08 5:17 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>> Replace the ad-hoc DMA cache management functions with common bouncebuf,
>>>> since those functions are not handling cases where unaligned buffer is
>>>> passed in,
>>>
>>>
>>> Were you hit by a problem,
>>> or just-in-case replacement?
>>
>> Yes, UBI triggers unaligned cache operations on my system (SoCFPGA).
>>> I thought I took care of the buffer alignment.
>>>
>>> The bounce buffer is allocated by kmalloc():
>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1348
>>>
>>> According to the lib/linux_compat.c implementation,
>>> it returns memory aligned with ARCH_DMA_MINALIGN.
>>>
>>>
>>> If the buffer is passed from the upper MTD layer,
>>> the NAND core also makes sure the enough alignment:
>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1273
>>>
>>> This is how this driver works in Linux.
>>>
>>> I'd rather want to keep the current code
>>> unless this is a real problem,
>>>
>>>
>>> One possible clean-up is to move dma_(un)map_single to a common place.
>> Is there any chance you can try UBI on the denali nand on uniphier ? :)
> 
> 
> I tested the driver only for raw block devices.
> 
> OK, I will test UBI on my platform.
> 
> BTW, do you see the problem only in U-Boot?
> Is the denali driver in Linux working fine?

Bump on this one ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Masahiro Yamada]

2018-07-12 21:51 GMT+09:00 Marek Vasut <marex@denx.de>:
> On 06/20/2018 09:14 AM, Masahiro Yamada wrote:
>> Hi Marek,
>
> Hi!
>
>> 2018-06-20 13:43 GMT+09:00 Marek Vasut <marex@denx.de>:
>>> On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
>>>> Hi Marek,
>>>
>>> Hi,
>>>
>>>> 2018-06-08 5:17 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>> Replace the ad-hoc DMA cache management functions with common bouncebuf,
>>>>> since those functions are not handling cases where unaligned buffer is
>>>>> passed in,
>>>>
>>>>
>>>> Were you hit by a problem,
>>>> or just-in-case replacement?
>>>
>>> Yes, UBI triggers unaligned cache operations on my system (SoCFPGA).
>>>> I thought I took care of the buffer alignment.
>>>>
>>>> The bounce buffer is allocated by kmalloc():
>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1348
>>>>
>>>> According to the lib/linux_compat.c implementation,
>>>> it returns memory aligned with ARCH_DMA_MINALIGN.
>>>>
>>>>
>>>> If the buffer is passed from the upper MTD layer,
>>>> the NAND core also makes sure the enough alignment:
>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1273
>>>>
>>>> This is how this driver works in Linux.
>>>>
>>>> I'd rather want to keep the current code
>>>> unless this is a real problem,
>>>>
>>>>
>>>> One possible clean-up is to move dma_(un)map_single to a common place.
>>> Is there any chance you can try UBI on the denali nand on uniphier ? :)
>>
>>
>> I tested the driver only for raw block devices.
>>
>> OK, I will test UBI on my platform.
>>
>> BTW, do you see the problem only in U-Boot?
>> Is the denali driver in Linux working fine?
>
> Bump on this one ?
>

Sorry for delay.


UBI is working for me without your patch.

Not sure what is the difference.

I will dig into it a little more, though.




-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Marek Vasut]

On 07/13/2018 07:13 AM, Masahiro Yamada wrote:
> 2018-07-12 21:51 GMT+09:00 Marek Vasut <marex@denx.de>:
>> On 06/20/2018 09:14 AM, Masahiro Yamada wrote:
>>> Hi Marek,
>>
>> Hi!
>>
>>> 2018-06-20 13:43 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>> On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
>>>>> Hi Marek,
>>>>
>>>> Hi,
>>>>
>>>>> 2018-06-08 5:17 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>> Replace the ad-hoc DMA cache management functions with common bouncebuf,
>>>>>> since those functions are not handling cases where unaligned buffer is
>>>>>> passed in,
>>>>>
>>>>>
>>>>> Were you hit by a problem,
>>>>> or just-in-case replacement?
>>>>
>>>> Yes, UBI triggers unaligned cache operations on my system (SoCFPGA).
>>>>> I thought I took care of the buffer alignment.
>>>>>
>>>>> The bounce buffer is allocated by kmalloc():
>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1348
>>>>>
>>>>> According to the lib/linux_compat.c implementation,
>>>>> it returns memory aligned with ARCH_DMA_MINALIGN.
>>>>>
>>>>>
>>>>> If the buffer is passed from the upper MTD layer,
>>>>> the NAND core also makes sure the enough alignment:
>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1273
>>>>>
>>>>> This is how this driver works in Linux.
>>>>>
>>>>> I'd rather want to keep the current code
>>>>> unless this is a real problem,
>>>>>
>>>>>
>>>>> One possible clean-up is to move dma_(un)map_single to a common place.
>>>> Is there any chance you can try UBI on the denali nand on uniphier ? :)
>>>
>>>
>>> I tested the driver only for raw block devices.
>>>
>>> OK, I will test UBI on my platform.
>>>
>>> BTW, do you see the problem only in U-Boot?
>>> Is the denali driver in Linux working fine?
>>
>> Bump on this one ?
>>
> 
> Sorry for delay.
> 
> 
> UBI is working for me without your patch.
> 
> Not sure what is the difference.
> 
> I will dig into it a little more, though.

Verify that you're not seeing any unaligned cache flushes. I do.
Note that my CPU core is CortexA9, armv7a.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Masahiro Yamada]

Hi Marek,

2018-07-13 16:59 GMT+09:00 Marek Vasut <marex@denx.de>:
> On 07/13/2018 07:13 AM, Masahiro Yamada wrote:
>> 2018-07-12 21:51 GMT+09:00 Marek Vasut <marex@denx.de>:
>>> On 06/20/2018 09:14 AM, Masahiro Yamada wrote:
>>>> Hi Marek,
>>>
>>> Hi!
>>>
>>>> 2018-06-20 13:43 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>> On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
>>>>>> Hi Marek,
>>>>>
>>>>> Hi,
>>>>>
>>>>>> 2018-06-08 5:17 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>> Replace the ad-hoc DMA cache management functions with common bouncebuf,
>>>>>>> since those functions are not handling cases where unaligned buffer is
>>>>>>> passed in,
>>>>>>
>>>>>>
>>>>>> Were you hit by a problem,
>>>>>> or just-in-case replacement?
>>>>>
>>>>> Yes, UBI triggers unaligned cache operations on my system (SoCFPGA).
>>>>>> I thought I took care of the buffer alignment.
>>>>>>
>>>>>> The bounce buffer is allocated by kmalloc():
>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1348
>>>>>>
>>>>>> According to the lib/linux_compat.c implementation,
>>>>>> it returns memory aligned with ARCH_DMA_MINALIGN.
>>>>>>
>>>>>>
>>>>>> If the buffer is passed from the upper MTD layer,
>>>>>> the NAND core also makes sure the enough alignment:
>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1273
>>>>>>
>>>>>> This is how this driver works in Linux.
>>>>>>
>>>>>> I'd rather want to keep the current code
>>>>>> unless this is a real problem,
>>>>>>
>>>>>>
>>>>>> One possible clean-up is to move dma_(un)map_single to a common place.
>>>>> Is there any chance you can try UBI on the denali nand on uniphier ? :)
>>>>
>>>>
>>>> I tested the driver only for raw block devices.
>>>>
>>>> OK, I will test UBI on my platform.
>>>>
>>>> BTW, do you see the problem only in U-Boot?
>>>> Is the denali driver in Linux working fine?
>>>
>>> Bump on this one ?
>>>
>>
>> Sorry for delay.
>>
>>
>> UBI is working for me without your patch.
>>
>> Not sure what is the difference.
>>
>> I will dig into it a little more, though.
>
> Verify that you're not seeing any unaligned cache flushes. I do.


Yeah, I am testing it now,
but I never see 'Misaligned operation at range' when using UBI.

However, I found a simple way to trigger the warning
in raw device access.



=> nand  read  81000010   0  1000

NAND read: device 0 offset 0x0, size 0x1000
CACHE: Misaligned operation at range [81000010, 81001010]
CACHE: Misaligned operation at range [81000010, 81001010]
CACHE: Misaligned operation at range [81000010, 81001010]
CACHE: Misaligned operation at range [81000010, 81001010]
 4096 bytes read: OK




I can fix it with one liner.



diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 6266c8a..f391727 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
                denali->dma_avail = 1;

        if (denali->dma_avail) {
-               chip->buf_align = 16;
+               chip->buf_align = ARCH_DMA_MINALIGN;
                if (denali->caps & DENALI_CAP_DMA_64BIT)
                        denali->setup_dma = denali_setup_dma64;
                else


I guess this will work for you too.





> Note that my CPU core is CortexA9, armv7a.


I tested on my Coretex-A9 SoC.



>
> --
> Best regards,
> Marek Vasut
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot



-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Marek Vasut]

On 07/13/2018 10:23 AM, Masahiro Yamada wrote:
> Hi Marek,
> 
> 2018-07-13 16:59 GMT+09:00 Marek Vasut <marex@denx.de>:
>> On 07/13/2018 07:13 AM, Masahiro Yamada wrote:
>>> 2018-07-12 21:51 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>> On 06/20/2018 09:14 AM, Masahiro Yamada wrote:
>>>>> Hi Marek,
>>>>
>>>> Hi!
>>>>
>>>>> 2018-06-20 13:43 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>> On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
>>>>>>> Hi Marek,
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>>> 2018-06-08 5:17 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>> Replace the ad-hoc DMA cache management functions with common bouncebuf,
>>>>>>>> since those functions are not handling cases where unaligned buffer is
>>>>>>>> passed in,
>>>>>>>
>>>>>>>
>>>>>>> Were you hit by a problem,
>>>>>>> or just-in-case replacement?
>>>>>>
>>>>>> Yes, UBI triggers unaligned cache operations on my system (SoCFPGA).
>>>>>>> I thought I took care of the buffer alignment.
>>>>>>>
>>>>>>> The bounce buffer is allocated by kmalloc():
>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1348
>>>>>>>
>>>>>>> According to the lib/linux_compat.c implementation,
>>>>>>> it returns memory aligned with ARCH_DMA_MINALIGN.
>>>>>>>
>>>>>>>
>>>>>>> If the buffer is passed from the upper MTD layer,
>>>>>>> the NAND core also makes sure the enough alignment:
>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1273
>>>>>>>
>>>>>>> This is how this driver works in Linux.
>>>>>>>
>>>>>>> I'd rather want to keep the current code
>>>>>>> unless this is a real problem,
>>>>>>>
>>>>>>>
>>>>>>> One possible clean-up is to move dma_(un)map_single to a common place.
>>>>>> Is there any chance you can try UBI on the denali nand on uniphier ? :)
>>>>>
>>>>>
>>>>> I tested the driver only for raw block devices.
>>>>>
>>>>> OK, I will test UBI on my platform.
>>>>>
>>>>> BTW, do you see the problem only in U-Boot?
>>>>> Is the denali driver in Linux working fine?
>>>>
>>>> Bump on this one ?
>>>>
>>>
>>> Sorry for delay.
>>>
>>>
>>> UBI is working for me without your patch.
>>>
>>> Not sure what is the difference.
>>>
>>> I will dig into it a little more, though.
>>
>> Verify that you're not seeing any unaligned cache flushes. I do.
> 
> 
> Yeah, I am testing it now,
> but I never see 'Misaligned operation at range' when using UBI.
> 
> However, I found a simple way to trigger the warning
> in raw device access.
> 
> 
> 
> => nand  read  81000010   0  1000
> 
> NAND read: device 0 offset 0x0, size 0x1000
> CACHE: Misaligned operation at range [81000010, 81001010]
> CACHE: Misaligned operation at range [81000010, 81001010]
> CACHE: Misaligned operation at range [81000010, 81001010]
> CACHE: Misaligned operation at range [81000010, 81001010]
>  4096 bytes read: OK
> 
> 
> 
> 
> I can fix it with one liner.
> 
> 
> 
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index 6266c8a..f391727 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>                 denali->dma_avail = 1;
> 
>         if (denali->dma_avail) {
> -               chip->buf_align = 16;
> +               chip->buf_align = ARCH_DMA_MINALIGN;
>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>                         denali->setup_dma = denali_setup_dma64;
>                 else
> 
> 
> I guess this will work for you too.

Doesn't that only apply if DMA is available ?

And anyway, I'd rather use common U-Boot code than have a custom
implementation in a driver which we need to maintain and fix separately.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Masahiro Yamada]

Hi Marek

2018-07-13 17:56 GMT+09:00 Marek Vasut <marex@denx.de>:
> On 07/13/2018 10:23 AM, Masahiro Yamada wrote:
>> Hi Marek,
>>
>> 2018-07-13 16:59 GMT+09:00 Marek Vasut <marex@denx.de>:
>>> On 07/13/2018 07:13 AM, Masahiro Yamada wrote:
>>>> 2018-07-12 21:51 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>> On 06/20/2018 09:14 AM, Masahiro Yamada wrote:
>>>>>> Hi Marek,
>>>>>
>>>>> Hi!
>>>>>
>>>>>> 2018-06-20 13:43 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>> On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
>>>>>>>> Hi Marek,
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>>> 2018-06-08 5:17 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>> Replace the ad-hoc DMA cache management functions with common bouncebuf,
>>>>>>>>> since those functions are not handling cases where unaligned buffer is
>>>>>>>>> passed in,
>>>>>>>>
>>>>>>>>
>>>>>>>> Were you hit by a problem,
>>>>>>>> or just-in-case replacement?
>>>>>>>
>>>>>>> Yes, UBI triggers unaligned cache operations on my system (SoCFPGA).
>>>>>>>> I thought I took care of the buffer alignment.
>>>>>>>>
>>>>>>>> The bounce buffer is allocated by kmalloc():
>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1348
>>>>>>>>
>>>>>>>> According to the lib/linux_compat.c implementation,
>>>>>>>> it returns memory aligned with ARCH_DMA_MINALIGN.
>>>>>>>>
>>>>>>>>
>>>>>>>> If the buffer is passed from the upper MTD layer,
>>>>>>>> the NAND core also makes sure the enough alignment:
>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1273
>>>>>>>>
>>>>>>>> This is how this driver works in Linux.
>>>>>>>>
>>>>>>>> I'd rather want to keep the current code
>>>>>>>> unless this is a real problem,
>>>>>>>>
>>>>>>>>
>>>>>>>> One possible clean-up is to move dma_(un)map_single to a common place.
>>>>>>> Is there any chance you can try UBI on the denali nand on uniphier ? :)
>>>>>>
>>>>>>
>>>>>> I tested the driver only for raw block devices.
>>>>>>
>>>>>> OK, I will test UBI on my platform.
>>>>>>
>>>>>> BTW, do you see the problem only in U-Boot?
>>>>>> Is the denali driver in Linux working fine?
>>>>>
>>>>> Bump on this one ?
>>>>>
>>>>
>>>> Sorry for delay.
>>>>
>>>>
>>>> UBI is working for me without your patch.
>>>>
>>>> Not sure what is the difference.
>>>>
>>>> I will dig into it a little more, though.
>>>
>>> Verify that you're not seeing any unaligned cache flushes. I do.
>>
>>
>> Yeah, I am testing it now,
>> but I never see 'Misaligned operation at range' when using UBI.
>>
>> However, I found a simple way to trigger the warning
>> in raw device access.
>>
>>
>>
>> => nand  read  81000010   0  1000
>>
>> NAND read: device 0 offset 0x0, size 0x1000
>> CACHE: Misaligned operation at range [81000010, 81001010]
>> CACHE: Misaligned operation at range [81000010, 81001010]
>> CACHE: Misaligned operation at range [81000010, 81001010]
>> CACHE: Misaligned operation at range [81000010, 81001010]
>>  4096 bytes read: OK
>>
>>
>>
>>
>> I can fix it with one liner.
>>
>>
>>
>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>> index 6266c8a..f391727 100644
>> --- a/drivers/mtd/nand/denali.c
>> +++ b/drivers/mtd/nand/denali.c
>> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>>                 denali->dma_avail = 1;
>>
>>         if (denali->dma_avail) {
>> -               chip->buf_align = 16;
>> +               chip->buf_align = ARCH_DMA_MINALIGN;
>>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>>                         denali->setup_dma = denali_setup_dma64;
>>                 else
>>
>>
>> I guess this will work for you too.
>
> Doesn't that only apply if DMA is available ?

Of course.
If you use PIO instead of DMA,
you do not need to perform cache operation, right?



> And anyway, I'd rather use common U-Boot code than have a custom
> implementation in a driver which we need to maintain and fix separately.


bounce_buffer_{start,stop} does
malloc() and free() every time.
This is not efficient.


struct nand_chip already contains page buffers,
which guarantee alignment for DMA.

https://github.com/u-boot/u-boot/blob/v2018.07/include/linux/mtd/rawnand.h#L640


We can rely on the NAND framework
for handling DMA-capable alignment.





-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Marek Vasut]

On 07/13/2018 12:09 PM, Masahiro Yamada wrote:
> Hi Marek
> 
> 2018-07-13 17:56 GMT+09:00 Marek Vasut <marex@denx.de>:
>> On 07/13/2018 10:23 AM, Masahiro Yamada wrote:
>>> Hi Marek,
>>>
>>> 2018-07-13 16:59 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>> On 07/13/2018 07:13 AM, Masahiro Yamada wrote:
>>>>> 2018-07-12 21:51 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>> On 06/20/2018 09:14 AM, Masahiro Yamada wrote:
>>>>>>> Hi Marek,
>>>>>>
>>>>>> Hi!
>>>>>>
>>>>>>> 2018-06-20 13:43 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>> On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
>>>>>>>>> Hi Marek,
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>>> 2018-06-08 5:17 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>> Replace the ad-hoc DMA cache management functions with common bouncebuf,
>>>>>>>>>> since those functions are not handling cases where unaligned buffer is
>>>>>>>>>> passed in,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Were you hit by a problem,
>>>>>>>>> or just-in-case replacement?
>>>>>>>>
>>>>>>>> Yes, UBI triggers unaligned cache operations on my system (SoCFPGA).
>>>>>>>>> I thought I took care of the buffer alignment.
>>>>>>>>>
>>>>>>>>> The bounce buffer is allocated by kmalloc():
>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1348
>>>>>>>>>
>>>>>>>>> According to the lib/linux_compat.c implementation,
>>>>>>>>> it returns memory aligned with ARCH_DMA_MINALIGN.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> If the buffer is passed from the upper MTD layer,
>>>>>>>>> the NAND core also makes sure the enough alignment:
>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1273
>>>>>>>>>
>>>>>>>>> This is how this driver works in Linux.
>>>>>>>>>
>>>>>>>>> I'd rather want to keep the current code
>>>>>>>>> unless this is a real problem,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> One possible clean-up is to move dma_(un)map_single to a common place.
>>>>>>>> Is there any chance you can try UBI on the denali nand on uniphier ? :)
>>>>>>>
>>>>>>>
>>>>>>> I tested the driver only for raw block devices.
>>>>>>>
>>>>>>> OK, I will test UBI on my platform.
>>>>>>>
>>>>>>> BTW, do you see the problem only in U-Boot?
>>>>>>> Is the denali driver in Linux working fine?
>>>>>>
>>>>>> Bump on this one ?
>>>>>>
>>>>>
>>>>> Sorry for delay.
>>>>>
>>>>>
>>>>> UBI is working for me without your patch.
>>>>>
>>>>> Not sure what is the difference.
>>>>>
>>>>> I will dig into it a little more, though.
>>>>
>>>> Verify that you're not seeing any unaligned cache flushes. I do.
>>>
>>>
>>> Yeah, I am testing it now,
>>> but I never see 'Misaligned operation at range' when using UBI.
>>>
>>> However, I found a simple way to trigger the warning
>>> in raw device access.
>>>
>>>
>>>
>>> => nand  read  81000010   0  1000
>>>
>>> NAND read: device 0 offset 0x0, size 0x1000
>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>  4096 bytes read: OK
>>>
>>>
>>>
>>>
>>> I can fix it with one liner.
>>>
>>>
>>>
>>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>>> index 6266c8a..f391727 100644
>>> --- a/drivers/mtd/nand/denali.c
>>> +++ b/drivers/mtd/nand/denali.c
>>> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>>>                 denali->dma_avail = 1;
>>>
>>>         if (denali->dma_avail) {
>>> -               chip->buf_align = 16;
>>> +               chip->buf_align = ARCH_DMA_MINALIGN;
>>>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>>>                         denali->setup_dma = denali_setup_dma64;
>>>                 else
>>>
>>>
>>> I guess this will work for you too.
>>
>> Doesn't that only apply if DMA is available ?
> 
> Of course.
> If you use PIO instead of DMA,
> you do not need to perform cache operation, right?
> 
> 
> 
>> And anyway, I'd rather use common U-Boot code than have a custom
>> implementation in a driver which we need to maintain and fix separately.
> 
> 
> bounce_buffer_{start,stop} does
> malloc() and free() every time.
> This is not efficient.
> 
> 
> struct nand_chip already contains page buffers,
> which guarantee alignment for DMA.
> 
> https://github.com/u-boot/u-boot/blob/v2018.07/include/linux/mtd/rawnand.h#L640
> 
> 
> We can rely on the NAND framework
> for handling DMA-capable alignment.

Clearly that doesn't work, otherwise I won't need this bounce buffer patch ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Masahiro Yamada]

2018-07-13 19:18 GMT+09:00 Marek Vasut <marex@denx.de>:
> On 07/13/2018 12:09 PM, Masahiro Yamada wrote:
>> Hi Marek
>>
>> 2018-07-13 17:56 GMT+09:00 Marek Vasut <marex@denx.de>:
>>> On 07/13/2018 10:23 AM, Masahiro Yamada wrote:
>>>> Hi Marek,
>>>>
>>>> 2018-07-13 16:59 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>> On 07/13/2018 07:13 AM, Masahiro Yamada wrote:
>>>>>> 2018-07-12 21:51 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>> On 06/20/2018 09:14 AM, Masahiro Yamada wrote:
>>>>>>>> Hi Marek,
>>>>>>>
>>>>>>> Hi!
>>>>>>>
>>>>>>>> 2018-06-20 13:43 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>> On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
>>>>>>>>>> Hi Marek,
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>>> 2018-06-08 5:17 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>> Replace the ad-hoc DMA cache management functions with common bouncebuf,
>>>>>>>>>>> since those functions are not handling cases where unaligned buffer is
>>>>>>>>>>> passed in,
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Were you hit by a problem,
>>>>>>>>>> or just-in-case replacement?
>>>>>>>>>
>>>>>>>>> Yes, UBI triggers unaligned cache operations on my system (SoCFPGA).
>>>>>>>>>> I thought I took care of the buffer alignment.
>>>>>>>>>>
>>>>>>>>>> The bounce buffer is allocated by kmalloc():
>>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1348
>>>>>>>>>>
>>>>>>>>>> According to the lib/linux_compat.c implementation,
>>>>>>>>>> it returns memory aligned with ARCH_DMA_MINALIGN.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> If the buffer is passed from the upper MTD layer,
>>>>>>>>>> the NAND core also makes sure the enough alignment:
>>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1273
>>>>>>>>>>
>>>>>>>>>> This is how this driver works in Linux.
>>>>>>>>>>
>>>>>>>>>> I'd rather want to keep the current code
>>>>>>>>>> unless this is a real problem,
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> One possible clean-up is to move dma_(un)map_single to a common place.
>>>>>>>>> Is there any chance you can try UBI on the denali nand on uniphier ? :)
>>>>>>>>
>>>>>>>>
>>>>>>>> I tested the driver only for raw block devices.
>>>>>>>>
>>>>>>>> OK, I will test UBI on my platform.
>>>>>>>>
>>>>>>>> BTW, do you see the problem only in U-Boot?
>>>>>>>> Is the denali driver in Linux working fine?
>>>>>>>
>>>>>>> Bump on this one ?
>>>>>>>
>>>>>>
>>>>>> Sorry for delay.
>>>>>>
>>>>>>
>>>>>> UBI is working for me without your patch.
>>>>>>
>>>>>> Not sure what is the difference.
>>>>>>
>>>>>> I will dig into it a little more, though.
>>>>>
>>>>> Verify that you're not seeing any unaligned cache flushes. I do.
>>>>
>>>>
>>>> Yeah, I am testing it now,
>>>> but I never see 'Misaligned operation at range' when using UBI.
>>>>
>>>> However, I found a simple way to trigger the warning
>>>> in raw device access.
>>>>
>>>>
>>>>
>>>> => nand  read  81000010   0  1000
>>>>
>>>> NAND read: device 0 offset 0x0, size 0x1000
>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>  4096 bytes read: OK
>>>>
>>>>
>>>>
>>>>
>>>> I can fix it with one liner.
>>>>
>>>>
>>>>
>>>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>>>> index 6266c8a..f391727 100644
>>>> --- a/drivers/mtd/nand/denali.c
>>>> +++ b/drivers/mtd/nand/denali.c
>>>> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>>>>                 denali->dma_avail = 1;
>>>>
>>>>         if (denali->dma_avail) {
>>>> -               chip->buf_align = 16;
>>>> +               chip->buf_align = ARCH_DMA_MINALIGN;
>>>>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>>>>                         denali->setup_dma = denali_setup_dma64;
>>>>                 else
>>>>
>>>>
>>>> I guess this will work for you too.
>>>
>>> Doesn't that only apply if DMA is available ?
>>
>> Of course.
>> If you use PIO instead of DMA,
>> you do not need to perform cache operation, right?
>>
>>
>>
>>> And anyway, I'd rather use common U-Boot code than have a custom
>>> implementation in a driver which we need to maintain and fix separately.
>>
>>
>> bounce_buffer_{start,stop} does
>> malloc() and free() every time.
>> This is not efficient.
>>
>>
>> struct nand_chip already contains page buffers,
>> which guarantee alignment for DMA.
>>
>> https://github.com/u-boot/u-boot/blob/v2018.07/include/linux/mtd/rawnand.h#L640
>>
>>
>> We can rely on the NAND framework
>> for handling DMA-capable alignment.
>
> Clearly that doesn't work, otherwise I won't need this bounce buffer patch ?
>




diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 6266c8a..f391727 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
                denali->dma_avail = 1;

        if (denali->dma_avail) {
-               chip->buf_align = 16;
+               chip->buf_align = ARCH_DMA_MINALIGN;
                if (denali->caps & DENALI_CAP_DMA_64BIT)
                        denali->setup_dma = denali_setup_dma64;
                else


Did you try this?
I do not see unaligned cache operation.


-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Marek Vasut]

On 07/13/2018 12:22 PM, Masahiro Yamada wrote:
> 2018-07-13 19:18 GMT+09:00 Marek Vasut <marex@denx.de>:
>> On 07/13/2018 12:09 PM, Masahiro Yamada wrote:
>>> Hi Marek
>>>
>>> 2018-07-13 17:56 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>> On 07/13/2018 10:23 AM, Masahiro Yamada wrote:
>>>>> Hi Marek,
>>>>>
>>>>> 2018-07-13 16:59 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>> On 07/13/2018 07:13 AM, Masahiro Yamada wrote:
>>>>>>> 2018-07-12 21:51 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>> On 06/20/2018 09:14 AM, Masahiro Yamada wrote:
>>>>>>>>> Hi Marek,
>>>>>>>>
>>>>>>>> Hi!
>>>>>>>>
>>>>>>>>> 2018-06-20 13:43 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>> On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
>>>>>>>>>>> Hi Marek,
>>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>>> 2018-06-08 5:17 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>>> Replace the ad-hoc DMA cache management functions with common bouncebuf,
>>>>>>>>>>>> since those functions are not handling cases where unaligned buffer is
>>>>>>>>>>>> passed in,
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Were you hit by a problem,
>>>>>>>>>>> or just-in-case replacement?
>>>>>>>>>>
>>>>>>>>>> Yes, UBI triggers unaligned cache operations on my system (SoCFPGA).
>>>>>>>>>>> I thought I took care of the buffer alignment.
>>>>>>>>>>>
>>>>>>>>>>> The bounce buffer is allocated by kmalloc():
>>>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1348
>>>>>>>>>>>
>>>>>>>>>>> According to the lib/linux_compat.c implementation,
>>>>>>>>>>> it returns memory aligned with ARCH_DMA_MINALIGN.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> If the buffer is passed from the upper MTD layer,
>>>>>>>>>>> the NAND core also makes sure the enough alignment:
>>>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1273
>>>>>>>>>>>
>>>>>>>>>>> This is how this driver works in Linux.
>>>>>>>>>>>
>>>>>>>>>>> I'd rather want to keep the current code
>>>>>>>>>>> unless this is a real problem,
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> One possible clean-up is to move dma_(un)map_single to a common place.
>>>>>>>>>> Is there any chance you can try UBI on the denali nand on uniphier ? :)
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I tested the driver only for raw block devices.
>>>>>>>>>
>>>>>>>>> OK, I will test UBI on my platform.
>>>>>>>>>
>>>>>>>>> BTW, do you see the problem only in U-Boot?
>>>>>>>>> Is the denali driver in Linux working fine?
>>>>>>>>
>>>>>>>> Bump on this one ?
>>>>>>>>
>>>>>>>
>>>>>>> Sorry for delay.
>>>>>>>
>>>>>>>
>>>>>>> UBI is working for me without your patch.
>>>>>>>
>>>>>>> Not sure what is the difference.
>>>>>>>
>>>>>>> I will dig into it a little more, though.
>>>>>>
>>>>>> Verify that you're not seeing any unaligned cache flushes. I do.
>>>>>
>>>>>
>>>>> Yeah, I am testing it now,
>>>>> but I never see 'Misaligned operation at range' when using UBI.
>>>>>
>>>>> However, I found a simple way to trigger the warning
>>>>> in raw device access.
>>>>>
>>>>>
>>>>>
>>>>> => nand  read  81000010   0  1000
>>>>>
>>>>> NAND read: device 0 offset 0x0, size 0x1000
>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>  4096 bytes read: OK
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> I can fix it with one liner.
>>>>>
>>>>>
>>>>>
>>>>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>>>>> index 6266c8a..f391727 100644
>>>>> --- a/drivers/mtd/nand/denali.c
>>>>> +++ b/drivers/mtd/nand/denali.c
>>>>> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>>>>>                 denali->dma_avail = 1;
>>>>>
>>>>>         if (denali->dma_avail) {
>>>>> -               chip->buf_align = 16;
>>>>> +               chip->buf_align = ARCH_DMA_MINALIGN;
>>>>>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>>>>>                         denali->setup_dma = denali_setup_dma64;
>>>>>                 else
>>>>>
>>>>>
>>>>> I guess this will work for you too.
>>>>
>>>> Doesn't that only apply if DMA is available ?
>>>
>>> Of course.
>>> If you use PIO instead of DMA,
>>> you do not need to perform cache operation, right?
>>>
>>>
>>>
>>>> And anyway, I'd rather use common U-Boot code than have a custom
>>>> implementation in a driver which we need to maintain and fix separately.
>>>
>>>
>>> bounce_buffer_{start,stop} does
>>> malloc() and free() every time.
>>> This is not efficient.
>>>
>>>
>>> struct nand_chip already contains page buffers,
>>> which guarantee alignment for DMA.
>>>
>>> https://github.com/u-boot/u-boot/blob/v2018.07/include/linux/mtd/rawnand.h#L640
>>>
>>>
>>> We can rely on the NAND framework
>>> for handling DMA-capable alignment.
>>
>> Clearly that doesn't work, otherwise I won't need this bounce buffer patch ?
>>
> 
> 
> 
> 
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index 6266c8a..f391727 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>                 denali->dma_avail = 1;
> 
>         if (denali->dma_avail) {
> -               chip->buf_align = 16;
> +               chip->buf_align = ARCH_DMA_MINALIGN;
>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>                         denali->setup_dma = denali_setup_dma64;
>                 else
> 
> 
> Did you try this?
> I do not see unaligned cache operation.

Nope, I'll have to assemble the hardware.
But this only works if dma_avail, right ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Masahiro Yamada]

2018-07-13 19:35 GMT+09:00 Marek Vasut <marex@denx.de>:
> On 07/13/2018 12:22 PM, Masahiro Yamada wrote:
>> 2018-07-13 19:18 GMT+09:00 Marek Vasut <marex@denx.de>:
>>> On 07/13/2018 12:09 PM, Masahiro Yamada wrote:
>>>> Hi Marek
>>>>
>>>> 2018-07-13 17:56 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>> On 07/13/2018 10:23 AM, Masahiro Yamada wrote:
>>>>>> Hi Marek,
>>>>>>
>>>>>> 2018-07-13 16:59 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>> On 07/13/2018 07:13 AM, Masahiro Yamada wrote:
>>>>>>>> 2018-07-12 21:51 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>> On 06/20/2018 09:14 AM, Masahiro Yamada wrote:
>>>>>>>>>> Hi Marek,
>>>>>>>>>
>>>>>>>>> Hi!
>>>>>>>>>
>>>>>>>>>> 2018-06-20 13:43 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>> On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
>>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>>> 2018-06-08 5:17 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>>>> Replace the ad-hoc DMA cache management functions with common bouncebuf,
>>>>>>>>>>>>> since those functions are not handling cases where unaligned buffer is
>>>>>>>>>>>>> passed in,
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Were you hit by a problem,
>>>>>>>>>>>> or just-in-case replacement?
>>>>>>>>>>>
>>>>>>>>>>> Yes, UBI triggers unaligned cache operations on my system (SoCFPGA).
>>>>>>>>>>>> I thought I took care of the buffer alignment.
>>>>>>>>>>>>
>>>>>>>>>>>> The bounce buffer is allocated by kmalloc():
>>>>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1348
>>>>>>>>>>>>
>>>>>>>>>>>> According to the lib/linux_compat.c implementation,
>>>>>>>>>>>> it returns memory aligned with ARCH_DMA_MINALIGN.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> If the buffer is passed from the upper MTD layer,
>>>>>>>>>>>> the NAND core also makes sure the enough alignment:
>>>>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1273
>>>>>>>>>>>>
>>>>>>>>>>>> This is how this driver works in Linux.
>>>>>>>>>>>>
>>>>>>>>>>>> I'd rather want to keep the current code
>>>>>>>>>>>> unless this is a real problem,
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> One possible clean-up is to move dma_(un)map_single to a common place.
>>>>>>>>>>> Is there any chance you can try UBI on the denali nand on uniphier ? :)
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I tested the driver only for raw block devices.
>>>>>>>>>>
>>>>>>>>>> OK, I will test UBI on my platform.
>>>>>>>>>>
>>>>>>>>>> BTW, do you see the problem only in U-Boot?
>>>>>>>>>> Is the denali driver in Linux working fine?
>>>>>>>>>
>>>>>>>>> Bump on this one ?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Sorry for delay.
>>>>>>>>
>>>>>>>>
>>>>>>>> UBI is working for me without your patch.
>>>>>>>>
>>>>>>>> Not sure what is the difference.
>>>>>>>>
>>>>>>>> I will dig into it a little more, though.
>>>>>>>
>>>>>>> Verify that you're not seeing any unaligned cache flushes. I do.
>>>>>>
>>>>>>
>>>>>> Yeah, I am testing it now,
>>>>>> but I never see 'Misaligned operation at range' when using UBI.
>>>>>>
>>>>>> However, I found a simple way to trigger the warning
>>>>>> in raw device access.
>>>>>>
>>>>>>
>>>>>>
>>>>>> => nand  read  81000010   0  1000
>>>>>>
>>>>>> NAND read: device 0 offset 0x0, size 0x1000
>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>  4096 bytes read: OK
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> I can fix it with one liner.
>>>>>>
>>>>>>
>>>>>>
>>>>>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>>>>>> index 6266c8a..f391727 100644
>>>>>> --- a/drivers/mtd/nand/denali.c
>>>>>> +++ b/drivers/mtd/nand/denali.c
>>>>>> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>>>>>>                 denali->dma_avail = 1;
>>>>>>
>>>>>>         if (denali->dma_avail) {
>>>>>> -               chip->buf_align = 16;
>>>>>> +               chip->buf_align = ARCH_DMA_MINALIGN;
>>>>>>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>>>>>>                         denali->setup_dma = denali_setup_dma64;
>>>>>>                 else
>>>>>>
>>>>>>
>>>>>> I guess this will work for you too.
>>>>>
>>>>> Doesn't that only apply if DMA is available ?
>>>>
>>>> Of course.
>>>> If you use PIO instead of DMA,
>>>> you do not need to perform cache operation, right?
>>>>
>>>>
>>>>
>>>>> And anyway, I'd rather use common U-Boot code than have a custom
>>>>> implementation in a driver which we need to maintain and fix separately.
>>>>
>>>>
>>>> bounce_buffer_{start,stop} does
>>>> malloc() and free() every time.
>>>> This is not efficient.
>>>>
>>>>
>>>> struct nand_chip already contains page buffers,
>>>> which guarantee alignment for DMA.
>>>>
>>>> https://github.com/u-boot/u-boot/blob/v2018.07/include/linux/mtd/rawnand.h#L640
>>>>
>>>>
>>>> We can rely on the NAND framework
>>>> for handling DMA-capable alignment.
>>>
>>> Clearly that doesn't work, otherwise I won't need this bounce buffer patch ?
>>>
>>
>>
>>
>>
>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>> index 6266c8a..f391727 100644
>> --- a/drivers/mtd/nand/denali.c
>> +++ b/drivers/mtd/nand/denali.c
>> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>>                 denali->dma_avail = 1;
>>
>>         if (denali->dma_avail) {
>> -               chip->buf_align = 16;
>> +               chip->buf_align = ARCH_DMA_MINALIGN;
>>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>>                         denali->setup_dma = denali_setup_dma64;
>>                 else
>>
>>
>> Did you try this?
>> I do not see unaligned cache operation.
>
> Nope, I'll have to assemble the hardware.
> But this only works if dma_avail, right ?
>


So, what are you worried about?



-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Marek Vasut]

On 07/13/2018 12:52 PM, Masahiro Yamada wrote:
> 2018-07-13 19:35 GMT+09:00 Marek Vasut <marex@denx.de>:
>> On 07/13/2018 12:22 PM, Masahiro Yamada wrote:
>>> 2018-07-13 19:18 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>> On 07/13/2018 12:09 PM, Masahiro Yamada wrote:
>>>>> Hi Marek
>>>>>
>>>>> 2018-07-13 17:56 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>> On 07/13/2018 10:23 AM, Masahiro Yamada wrote:
>>>>>>> Hi Marek,
>>>>>>>
>>>>>>> 2018-07-13 16:59 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>> On 07/13/2018 07:13 AM, Masahiro Yamada wrote:
>>>>>>>>> 2018-07-12 21:51 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>> On 06/20/2018 09:14 AM, Masahiro Yamada wrote:
>>>>>>>>>>> Hi Marek,
>>>>>>>>>>
>>>>>>>>>> Hi!
>>>>>>>>>>
>>>>>>>>>>> 2018-06-20 13:43 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>>> On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
>>>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>>
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>>> 2018-06-08 5:17 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>>>>> Replace the ad-hoc DMA cache management functions with common bouncebuf,
>>>>>>>>>>>>>> since those functions are not handling cases where unaligned buffer is
>>>>>>>>>>>>>> passed in,
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Were you hit by a problem,
>>>>>>>>>>>>> or just-in-case replacement?
>>>>>>>>>>>>
>>>>>>>>>>>> Yes, UBI triggers unaligned cache operations on my system (SoCFPGA).
>>>>>>>>>>>>> I thought I took care of the buffer alignment.
>>>>>>>>>>>>>
>>>>>>>>>>>>> The bounce buffer is allocated by kmalloc():
>>>>>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1348
>>>>>>>>>>>>>
>>>>>>>>>>>>> According to the lib/linux_compat.c implementation,
>>>>>>>>>>>>> it returns memory aligned with ARCH_DMA_MINALIGN.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> If the buffer is passed from the upper MTD layer,
>>>>>>>>>>>>> the NAND core also makes sure the enough alignment:
>>>>>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1273
>>>>>>>>>>>>>
>>>>>>>>>>>>> This is how this driver works in Linux.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I'd rather want to keep the current code
>>>>>>>>>>>>> unless this is a real problem,
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> One possible clean-up is to move dma_(un)map_single to a common place.
>>>>>>>>>>>> Is there any chance you can try UBI on the denali nand on uniphier ? :)
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I tested the driver only for raw block devices.
>>>>>>>>>>>
>>>>>>>>>>> OK, I will test UBI on my platform.
>>>>>>>>>>>
>>>>>>>>>>> BTW, do you see the problem only in U-Boot?
>>>>>>>>>>> Is the denali driver in Linux working fine?
>>>>>>>>>>
>>>>>>>>>> Bump on this one ?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Sorry for delay.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> UBI is working for me without your patch.
>>>>>>>>>
>>>>>>>>> Not sure what is the difference.
>>>>>>>>>
>>>>>>>>> I will dig into it a little more, though.
>>>>>>>>
>>>>>>>> Verify that you're not seeing any unaligned cache flushes. I do.
>>>>>>>
>>>>>>>
>>>>>>> Yeah, I am testing it now,
>>>>>>> but I never see 'Misaligned operation at range' when using UBI.
>>>>>>>
>>>>>>> However, I found a simple way to trigger the warning
>>>>>>> in raw device access.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> => nand  read  81000010   0  1000
>>>>>>>
>>>>>>> NAND read: device 0 offset 0x0, size 0x1000
>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>>  4096 bytes read: OK
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I can fix it with one liner.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>>>>>>> index 6266c8a..f391727 100644
>>>>>>> --- a/drivers/mtd/nand/denali.c
>>>>>>> +++ b/drivers/mtd/nand/denali.c
>>>>>>> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>>>>>>>                 denali->dma_avail = 1;
>>>>>>>
>>>>>>>         if (denali->dma_avail) {
>>>>>>> -               chip->buf_align = 16;
>>>>>>> +               chip->buf_align = ARCH_DMA_MINALIGN;
>>>>>>>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>>>>>>>                         denali->setup_dma = denali_setup_dma64;
>>>>>>>                 else
>>>>>>>
>>>>>>>
>>>>>>> I guess this will work for you too.
>>>>>>
>>>>>> Doesn't that only apply if DMA is available ?
>>>>>
>>>>> Of course.
>>>>> If you use PIO instead of DMA,
>>>>> you do not need to perform cache operation, right?
>>>>>
>>>>>
>>>>>
>>>>>> And anyway, I'd rather use common U-Boot code than have a custom
>>>>>> implementation in a driver which we need to maintain and fix separately.
>>>>>
>>>>>
>>>>> bounce_buffer_{start,stop} does
>>>>> malloc() and free() every time.
>>>>> This is not efficient.
>>>>>
>>>>>
>>>>> struct nand_chip already contains page buffers,
>>>>> which guarantee alignment for DMA.
>>>>>
>>>>> https://github.com/u-boot/u-boot/blob/v2018.07/include/linux/mtd/rawnand.h#L640
>>>>>
>>>>>
>>>>> We can rely on the NAND framework
>>>>> for handling DMA-capable alignment.
>>>>
>>>> Clearly that doesn't work, otherwise I won't need this bounce buffer patch ?
>>>>
>>>
>>>
>>>
>>>
>>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>>> index 6266c8a..f391727 100644
>>> --- a/drivers/mtd/nand/denali.c
>>> +++ b/drivers/mtd/nand/denali.c
>>> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>>>                 denali->dma_avail = 1;
>>>
>>>         if (denali->dma_avail) {
>>> -               chip->buf_align = 16;
>>> +               chip->buf_align = ARCH_DMA_MINALIGN;
>>>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>>>                         denali->setup_dma = denali_setup_dma64;
>>>                 else
>>>
>>>
>>> Did you try this?
>>> I do not see unaligned cache operation.
>>
>> Nope, I'll have to assemble the hardware.
>> But this only works if dma_avail, right ?
>>
> 
> 
> So, what are you worried about?

That the denali NAND is broken in mainline on socfpga.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Masahiro Yamada]

2018-07-13 19:58 GMT+09:00 Marek Vasut <marex@denx.de>:
> On 07/13/2018 12:52 PM, Masahiro Yamada wrote:
>> 2018-07-13 19:35 GMT+09:00 Marek Vasut <marex@denx.de>:
>>> On 07/13/2018 12:22 PM, Masahiro Yamada wrote:
>>>> 2018-07-13 19:18 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>> On 07/13/2018 12:09 PM, Masahiro Yamada wrote:
>>>>>> Hi Marek
>>>>>>
>>>>>> 2018-07-13 17:56 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>> On 07/13/2018 10:23 AM, Masahiro Yamada wrote:
>>>>>>>> Hi Marek,
>>>>>>>>
>>>>>>>> 2018-07-13 16:59 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>> On 07/13/2018 07:13 AM, Masahiro Yamada wrote:
>>>>>>>>>> 2018-07-12 21:51 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>> On 06/20/2018 09:14 AM, Masahiro Yamada wrote:
>>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>
>>>>>>>>>>> Hi!
>>>>>>>>>>>
>>>>>>>>>>>> 2018-06-20 13:43 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>>>> On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
>>>>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>
>>>>>>>>>>>>>> 2018-06-08 5:17 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>>>>>> Replace the ad-hoc DMA cache management functions with common bouncebuf,
>>>>>>>>>>>>>>> since those functions are not handling cases where unaligned buffer is
>>>>>>>>>>>>>>> passed in,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Were you hit by a problem,
>>>>>>>>>>>>>> or just-in-case replacement?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yes, UBI triggers unaligned cache operations on my system (SoCFPGA).
>>>>>>>>>>>>>> I thought I took care of the buffer alignment.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The bounce buffer is allocated by kmalloc():
>>>>>>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1348
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> According to the lib/linux_compat.c implementation,
>>>>>>>>>>>>>> it returns memory aligned with ARCH_DMA_MINALIGN.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> If the buffer is passed from the upper MTD layer,
>>>>>>>>>>>>>> the NAND core also makes sure the enough alignment:
>>>>>>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1273
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This is how this driver works in Linux.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I'd rather want to keep the current code
>>>>>>>>>>>>>> unless this is a real problem,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> One possible clean-up is to move dma_(un)map_single to a common place.
>>>>>>>>>>>>> Is there any chance you can try UBI on the denali nand on uniphier ? :)
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> I tested the driver only for raw block devices.
>>>>>>>>>>>>
>>>>>>>>>>>> OK, I will test UBI on my platform.
>>>>>>>>>>>>
>>>>>>>>>>>> BTW, do you see the problem only in U-Boot?
>>>>>>>>>>>> Is the denali driver in Linux working fine?
>>>>>>>>>>>
>>>>>>>>>>> Bump on this one ?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Sorry for delay.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> UBI is working for me without your patch.
>>>>>>>>>>
>>>>>>>>>> Not sure what is the difference.
>>>>>>>>>>
>>>>>>>>>> I will dig into it a little more, though.
>>>>>>>>>
>>>>>>>>> Verify that you're not seeing any unaligned cache flushes. I do.
>>>>>>>>
>>>>>>>>
>>>>>>>> Yeah, I am testing it now,
>>>>>>>> but I never see 'Misaligned operation at range' when using UBI.
>>>>>>>>
>>>>>>>> However, I found a simple way to trigger the warning
>>>>>>>> in raw device access.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> => nand  read  81000010   0  1000
>>>>>>>>
>>>>>>>> NAND read: device 0 offset 0x0, size 0x1000
>>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>>>  4096 bytes read: OK
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> I can fix it with one liner.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>>>>>>>> index 6266c8a..f391727 100644
>>>>>>>> --- a/drivers/mtd/nand/denali.c
>>>>>>>> +++ b/drivers/mtd/nand/denali.c
>>>>>>>> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>>>>>>>>                 denali->dma_avail = 1;
>>>>>>>>
>>>>>>>>         if (denali->dma_avail) {
>>>>>>>> -               chip->buf_align = 16;
>>>>>>>> +               chip->buf_align = ARCH_DMA_MINALIGN;
>>>>>>>>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>>>>>>>>                         denali->setup_dma = denali_setup_dma64;
>>>>>>>>                 else
>>>>>>>>
>>>>>>>>
>>>>>>>> I guess this will work for you too.
>>>>>>>
>>>>>>> Doesn't that only apply if DMA is available ?
>>>>>>
>>>>>> Of course.
>>>>>> If you use PIO instead of DMA,
>>>>>> you do not need to perform cache operation, right?
>>>>>>
>>>>>>
>>>>>>
>>>>>>> And anyway, I'd rather use common U-Boot code than have a custom
>>>>>>> implementation in a driver which we need to maintain and fix separately.
>>>>>>
>>>>>>
>>>>>> bounce_buffer_{start,stop} does
>>>>>> malloc() and free() every time.
>>>>>> This is not efficient.
>>>>>>
>>>>>>
>>>>>> struct nand_chip already contains page buffers,
>>>>>> which guarantee alignment for DMA.
>>>>>>
>>>>>> https://github.com/u-boot/u-boot/blob/v2018.07/include/linux/mtd/rawnand.h#L640
>>>>>>
>>>>>>
>>>>>> We can rely on the NAND framework
>>>>>> for handling DMA-capable alignment.
>>>>>
>>>>> Clearly that doesn't work, otherwise I won't need this bounce buffer patch ?
>>>>>
>>>>
>>>>
>>>>
>>>>
>>>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>>>> index 6266c8a..f391727 100644
>>>> --- a/drivers/mtd/nand/denali.c
>>>> +++ b/drivers/mtd/nand/denali.c
>>>> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>>>>                 denali->dma_avail = 1;
>>>>
>>>>         if (denali->dma_avail) {
>>>> -               chip->buf_align = 16;
>>>> +               chip->buf_align = ARCH_DMA_MINALIGN;
>>>>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>>>>                         denali->setup_dma = denali_setup_dma64;
>>>>                 else
>>>>
>>>>
>>>> Did you try this?
>>>> I do not see unaligned cache operation.
>>>
>>> Nope, I'll have to assemble the hardware.
>>> But this only works if dma_avail, right ?
>>>
>>
>>
>> So, what are you worried about?
>
> That the denali NAND is broken in mainline on socfpga.

I suggested more efficient fix.


I am asking about your
"But this only works if dma_avail, right ?"

Do you see any problem in 'dma_avail == false' case?



-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Marek Vasut]

On 07/13/2018 01:05 PM, Masahiro Yamada wrote:
> 2018-07-13 19:58 GMT+09:00 Marek Vasut <marex@denx.de>:
>> On 07/13/2018 12:52 PM, Masahiro Yamada wrote:
>>> 2018-07-13 19:35 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>> On 07/13/2018 12:22 PM, Masahiro Yamada wrote:
>>>>> 2018-07-13 19:18 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>> On 07/13/2018 12:09 PM, Masahiro Yamada wrote:
>>>>>>> Hi Marek
>>>>>>>
>>>>>>> 2018-07-13 17:56 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>> On 07/13/2018 10:23 AM, Masahiro Yamada wrote:
>>>>>>>>> Hi Marek,
>>>>>>>>>
>>>>>>>>> 2018-07-13 16:59 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>> On 07/13/2018 07:13 AM, Masahiro Yamada wrote:
>>>>>>>>>>> 2018-07-12 21:51 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>>> On 06/20/2018 09:14 AM, Masahiro Yamada wrote:
>>>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>>
>>>>>>>>>>>> Hi!
>>>>>>>>>>>>
>>>>>>>>>>>>> 2018-06-20 13:43 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>>>>> On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
>>>>>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 2018-06-08 5:17 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>>>>>>> Replace the ad-hoc DMA cache management functions with common bouncebuf,
>>>>>>>>>>>>>>>> since those functions are not handling cases where unaligned buffer is
>>>>>>>>>>>>>>>> passed in,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Were you hit by a problem,
>>>>>>>>>>>>>>> or just-in-case replacement?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Yes, UBI triggers unaligned cache operations on my system (SoCFPGA).
>>>>>>>>>>>>>>> I thought I took care of the buffer alignment.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> The bounce buffer is allocated by kmalloc():
>>>>>>>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1348
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> According to the lib/linux_compat.c implementation,
>>>>>>>>>>>>>>> it returns memory aligned with ARCH_DMA_MINALIGN.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> If the buffer is passed from the upper MTD layer,
>>>>>>>>>>>>>>> the NAND core also makes sure the enough alignment:
>>>>>>>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1273
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> This is how this driver works in Linux.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I'd rather want to keep the current code
>>>>>>>>>>>>>>> unless this is a real problem,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> One possible clean-up is to move dma_(un)map_single to a common place.
>>>>>>>>>>>>>> Is there any chance you can try UBI on the denali nand on uniphier ? :)
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> I tested the driver only for raw block devices.
>>>>>>>>>>>>>
>>>>>>>>>>>>> OK, I will test UBI on my platform.
>>>>>>>>>>>>>
>>>>>>>>>>>>> BTW, do you see the problem only in U-Boot?
>>>>>>>>>>>>> Is the denali driver in Linux working fine?
>>>>>>>>>>>>
>>>>>>>>>>>> Bump on this one ?
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Sorry for delay.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> UBI is working for me without your patch.
>>>>>>>>>>>
>>>>>>>>>>> Not sure what is the difference.
>>>>>>>>>>>
>>>>>>>>>>> I will dig into it a little more, though.
>>>>>>>>>>
>>>>>>>>>> Verify that you're not seeing any unaligned cache flushes. I do.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Yeah, I am testing it now,
>>>>>>>>> but I never see 'Misaligned operation at range' when using UBI.
>>>>>>>>>
>>>>>>>>> However, I found a simple way to trigger the warning
>>>>>>>>> in raw device access.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> => nand  read  81000010   0  1000
>>>>>>>>>
>>>>>>>>> NAND read: device 0 offset 0x0, size 0x1000
>>>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>>>>  4096 bytes read: OK
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I can fix it with one liner.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>>>>>>>>> index 6266c8a..f391727 100644
>>>>>>>>> --- a/drivers/mtd/nand/denali.c
>>>>>>>>> +++ b/drivers/mtd/nand/denali.c
>>>>>>>>> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>>>>>>>>>                 denali->dma_avail = 1;
>>>>>>>>>
>>>>>>>>>         if (denali->dma_avail) {
>>>>>>>>> -               chip->buf_align = 16;
>>>>>>>>> +               chip->buf_align = ARCH_DMA_MINALIGN;
>>>>>>>>>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>>>>>>>>>                         denali->setup_dma = denali_setup_dma64;
>>>>>>>>>                 else
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I guess this will work for you too.
>>>>>>>>
>>>>>>>> Doesn't that only apply if DMA is available ?
>>>>>>>
>>>>>>> Of course.
>>>>>>> If you use PIO instead of DMA,
>>>>>>> you do not need to perform cache operation, right?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> And anyway, I'd rather use common U-Boot code than have a custom
>>>>>>>> implementation in a driver which we need to maintain and fix separately.
>>>>>>>
>>>>>>>
>>>>>>> bounce_buffer_{start,stop} does
>>>>>>> malloc() and free() every time.
>>>>>>> This is not efficient.
>>>>>>>
>>>>>>>
>>>>>>> struct nand_chip already contains page buffers,
>>>>>>> which guarantee alignment for DMA.
>>>>>>>
>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.07/include/linux/mtd/rawnand.h#L640
>>>>>>>
>>>>>>>
>>>>>>> We can rely on the NAND framework
>>>>>>> for handling DMA-capable alignment.
>>>>>>
>>>>>> Clearly that doesn't work, otherwise I won't need this bounce buffer patch ?
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>>>>> index 6266c8a..f391727 100644
>>>>> --- a/drivers/mtd/nand/denali.c
>>>>> +++ b/drivers/mtd/nand/denali.c
>>>>> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>>>>>                 denali->dma_avail = 1;
>>>>>
>>>>>         if (denali->dma_avail) {
>>>>> -               chip->buf_align = 16;
>>>>> +               chip->buf_align = ARCH_DMA_MINALIGN;
>>>>>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>>>>>                         denali->setup_dma = denali_setup_dma64;
>>>>>                 else
>>>>>
>>>>>
>>>>> Did you try this?
>>>>> I do not see unaligned cache operation.
>>>>
>>>> Nope, I'll have to assemble the hardware.
>>>> But this only works if dma_avail, right ?
>>>>
>>>
>>>
>>> So, what are you worried about?
>>
>> That the denali NAND is broken in mainline on socfpga.
> 
> I suggested more efficient fix.
> 
> 
> I am asking about your
> "But this only works if dma_avail, right ?"
> 
> Do you see any problem in 'dma_avail == false' case?

I cannot test this now.

But I am still not very happy about keeping two copies of code doing the
same.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Masahiro Yamada]

2018-07-13 23:51 GMT+09:00 Marek Vasut <marex@denx.de>:
> On 07/13/2018 01:05 PM, Masahiro Yamada wrote:
>> 2018-07-13 19:58 GMT+09:00 Marek Vasut <marex@denx.de>:
>>> On 07/13/2018 12:52 PM, Masahiro Yamada wrote:
>>>> 2018-07-13 19:35 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>> On 07/13/2018 12:22 PM, Masahiro Yamada wrote:
>>>>>> 2018-07-13 19:18 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>> On 07/13/2018 12:09 PM, Masahiro Yamada wrote:
>>>>>>>> Hi Marek
>>>>>>>>
>>>>>>>> 2018-07-13 17:56 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>> On 07/13/2018 10:23 AM, Masahiro Yamada wrote:
>>>>>>>>>> Hi Marek,
>>>>>>>>>>
>>>>>>>>>> 2018-07-13 16:59 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>> On 07/13/2018 07:13 AM, Masahiro Yamada wrote:
>>>>>>>>>>>> 2018-07-12 21:51 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>>>> On 06/20/2018 09:14 AM, Masahiro Yamada wrote:
>>>>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi!
>>>>>>>>>>>>>
>>>>>>>>>>>>>> 2018-06-20 13:43 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>>>>>> On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
>>>>>>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 2018-06-08 5:17 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>>>>>>>> Replace the ad-hoc DMA cache management functions with common bouncebuf,
>>>>>>>>>>>>>>>>> since those functions are not handling cases where unaligned buffer is
>>>>>>>>>>>>>>>>> passed in,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Were you hit by a problem,
>>>>>>>>>>>>>>>> or just-in-case replacement?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Yes, UBI triggers unaligned cache operations on my system (SoCFPGA).
>>>>>>>>>>>>>>>> I thought I took care of the buffer alignment.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> The bounce buffer is allocated by kmalloc():
>>>>>>>>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1348
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> According to the lib/linux_compat.c implementation,
>>>>>>>>>>>>>>>> it returns memory aligned with ARCH_DMA_MINALIGN.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> If the buffer is passed from the upper MTD layer,
>>>>>>>>>>>>>>>> the NAND core also makes sure the enough alignment:
>>>>>>>>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1273
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> This is how this driver works in Linux.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I'd rather want to keep the current code
>>>>>>>>>>>>>>>> unless this is a real problem,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> One possible clean-up is to move dma_(un)map_single to a common place.
>>>>>>>>>>>>>>> Is there any chance you can try UBI on the denali nand on uniphier ? :)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I tested the driver only for raw block devices.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> OK, I will test UBI on my platform.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> BTW, do you see the problem only in U-Boot?
>>>>>>>>>>>>>> Is the denali driver in Linux working fine?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Bump on this one ?
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Sorry for delay.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> UBI is working for me without your patch.
>>>>>>>>>>>>
>>>>>>>>>>>> Not sure what is the difference.
>>>>>>>>>>>>
>>>>>>>>>>>> I will dig into it a little more, though.
>>>>>>>>>>>
>>>>>>>>>>> Verify that you're not seeing any unaligned cache flushes. I do.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Yeah, I am testing it now,
>>>>>>>>>> but I never see 'Misaligned operation at range' when using UBI.
>>>>>>>>>>
>>>>>>>>>> However, I found a simple way to trigger the warning
>>>>>>>>>> in raw device access.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> => nand  read  81000010   0  1000
>>>>>>>>>>
>>>>>>>>>> NAND read: device 0 offset 0x0, size 0x1000
>>>>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>>>>>  4096 bytes read: OK
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I can fix it with one liner.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>>>>>>>>>> index 6266c8a..f391727 100644
>>>>>>>>>> --- a/drivers/mtd/nand/denali.c
>>>>>>>>>> +++ b/drivers/mtd/nand/denali.c
>>>>>>>>>> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>>>>>>>>>>                 denali->dma_avail = 1;
>>>>>>>>>>
>>>>>>>>>>         if (denali->dma_avail) {
>>>>>>>>>> -               chip->buf_align = 16;
>>>>>>>>>> +               chip->buf_align = ARCH_DMA_MINALIGN;
>>>>>>>>>>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>>>>>>>>>>                         denali->setup_dma = denali_setup_dma64;
>>>>>>>>>>                 else
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I guess this will work for you too.
>>>>>>>>>
>>>>>>>>> Doesn't that only apply if DMA is available ?
>>>>>>>>
>>>>>>>> Of course.
>>>>>>>> If you use PIO instead of DMA,
>>>>>>>> you do not need to perform cache operation, right?
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> And anyway, I'd rather use common U-Boot code than have a custom
>>>>>>>>> implementation in a driver which we need to maintain and fix separately.
>>>>>>>>
>>>>>>>>
>>>>>>>> bounce_buffer_{start,stop} does
>>>>>>>> malloc() and free() every time.
>>>>>>>> This is not efficient.
>>>>>>>>
>>>>>>>>
>>>>>>>> struct nand_chip already contains page buffers,
>>>>>>>> which guarantee alignment for DMA.
>>>>>>>>
>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.07/include/linux/mtd/rawnand.h#L640
>>>>>>>>
>>>>>>>>
>>>>>>>> We can rely on the NAND framework
>>>>>>>> for handling DMA-capable alignment.
>>>>>>>
>>>>>>> Clearly that doesn't work, otherwise I won't need this bounce buffer patch ?
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>>>>>> index 6266c8a..f391727 100644
>>>>>> --- a/drivers/mtd/nand/denali.c
>>>>>> +++ b/drivers/mtd/nand/denali.c
>>>>>> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>>>>>>                 denali->dma_avail = 1;
>>>>>>
>>>>>>         if (denali->dma_avail) {
>>>>>> -               chip->buf_align = 16;
>>>>>> +               chip->buf_align = ARCH_DMA_MINALIGN;
>>>>>>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>>>>>>                         denali->setup_dma = denali_setup_dma64;
>>>>>>                 else
>>>>>>
>>>>>>
>>>>>> Did you try this?
>>>>>> I do not see unaligned cache operation.
>>>>>
>>>>> Nope, I'll have to assemble the hardware.
>>>>> But this only works if dma_avail, right ?
>>>>>
>>>>
>>>>
>>>> So, what are you worried about?
>>>
>>> That the denali NAND is broken in mainline on socfpga.
>>
>> I suggested more efficient fix.
>>
>>
>> I am asking about your
>> "But this only works if dma_avail, right ?"
>>
>> Do you see any problem in 'dma_avail == false' case?
>
> I cannot test this now.


denali_dma_xfer() is only called when dma_avail == true.

You do not need to worry about the cache-op
if dma_avail == false.  Believe me.

https://github.com/u-boot/u-boot/blob/v2018.07/drivers/mtd/nand/denali.c#L621




> But I am still not very happy about keeping two copies of code doing the
> same.


What do you mean by 'two copies of code' ?



> --
> Best regards,
> Marek Vasut
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot



-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Marek Vasut]

On 07/13/2018 05:09 PM, Masahiro Yamada wrote:
> 2018-07-13 23:51 GMT+09:00 Marek Vasut <marex@denx.de>:
>> On 07/13/2018 01:05 PM, Masahiro Yamada wrote:
>>> 2018-07-13 19:58 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>> On 07/13/2018 12:52 PM, Masahiro Yamada wrote:
>>>>> 2018-07-13 19:35 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>> On 07/13/2018 12:22 PM, Masahiro Yamada wrote:
>>>>>>> 2018-07-13 19:18 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>> On 07/13/2018 12:09 PM, Masahiro Yamada wrote:
>>>>>>>>> Hi Marek
>>>>>>>>>
>>>>>>>>> 2018-07-13 17:56 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>> On 07/13/2018 10:23 AM, Masahiro Yamada wrote:
>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>
>>>>>>>>>>> 2018-07-13 16:59 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>>> On 07/13/2018 07:13 AM, Masahiro Yamada wrote:
>>>>>>>>>>>>> 2018-07-12 21:51 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>>>>> On 06/20/2018 09:14 AM, Masahiro Yamada wrote:
>>>>>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi!
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 2018-06-20 13:43 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>>>>>>> On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
>>>>>>>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> 2018-06-08 5:17 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>>>>>>>>> Replace the ad-hoc DMA cache management functions with common bouncebuf,
>>>>>>>>>>>>>>>>>> since those functions are not handling cases where unaligned buffer is
>>>>>>>>>>>>>>>>>> passed in,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Were you hit by a problem,
>>>>>>>>>>>>>>>>> or just-in-case replacement?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Yes, UBI triggers unaligned cache operations on my system (SoCFPGA).
>>>>>>>>>>>>>>>>> I thought I took care of the buffer alignment.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> The bounce buffer is allocated by kmalloc():
>>>>>>>>>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1348
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> According to the lib/linux_compat.c implementation,
>>>>>>>>>>>>>>>>> it returns memory aligned with ARCH_DMA_MINALIGN.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> If the buffer is passed from the upper MTD layer,
>>>>>>>>>>>>>>>>> the NAND core also makes sure the enough alignment:
>>>>>>>>>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1273
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> This is how this driver works in Linux.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I'd rather want to keep the current code
>>>>>>>>>>>>>>>>> unless this is a real problem,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> One possible clean-up is to move dma_(un)map_single to a common place.
>>>>>>>>>>>>>>>> Is there any chance you can try UBI on the denali nand on uniphier ? :)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I tested the driver only for raw block devices.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> OK, I will test UBI on my platform.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> BTW, do you see the problem only in U-Boot?
>>>>>>>>>>>>>>> Is the denali driver in Linux working fine?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Bump on this one ?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Sorry for delay.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> UBI is working for me without your patch.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Not sure what is the difference.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I will dig into it a little more, though.
>>>>>>>>>>>>
>>>>>>>>>>>> Verify that you're not seeing any unaligned cache flushes. I do.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Yeah, I am testing it now,
>>>>>>>>>>> but I never see 'Misaligned operation at range' when using UBI.
>>>>>>>>>>>
>>>>>>>>>>> However, I found a simple way to trigger the warning
>>>>>>>>>>> in raw device access.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> => nand  read  81000010   0  1000
>>>>>>>>>>>
>>>>>>>>>>> NAND read: device 0 offset 0x0, size 0x1000
>>>>>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>>>>>>  4096 bytes read: OK
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I can fix it with one liner.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>>>>>>>>>>> index 6266c8a..f391727 100644
>>>>>>>>>>> --- a/drivers/mtd/nand/denali.c
>>>>>>>>>>> +++ b/drivers/mtd/nand/denali.c
>>>>>>>>>>> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>>>>>>>>>>>                 denali->dma_avail = 1;
>>>>>>>>>>>
>>>>>>>>>>>         if (denali->dma_avail) {
>>>>>>>>>>> -               chip->buf_align = 16;
>>>>>>>>>>> +               chip->buf_align = ARCH_DMA_MINALIGN;
>>>>>>>>>>>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>>>>>>>>>>>                         denali->setup_dma = denali_setup_dma64;
>>>>>>>>>>>                 else
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I guess this will work for you too.
>>>>>>>>>>
>>>>>>>>>> Doesn't that only apply if DMA is available ?
>>>>>>>>>
>>>>>>>>> Of course.
>>>>>>>>> If you use PIO instead of DMA,
>>>>>>>>> you do not need to perform cache operation, right?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> And anyway, I'd rather use common U-Boot code than have a custom
>>>>>>>>>> implementation in a driver which we need to maintain and fix separately.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> bounce_buffer_{start,stop} does
>>>>>>>>> malloc() and free() every time.
>>>>>>>>> This is not efficient.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> struct nand_chip already contains page buffers,
>>>>>>>>> which guarantee alignment for DMA.
>>>>>>>>>
>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.07/include/linux/mtd/rawnand.h#L640
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> We can rely on the NAND framework
>>>>>>>>> for handling DMA-capable alignment.
>>>>>>>>
>>>>>>>> Clearly that doesn't work, otherwise I won't need this bounce buffer patch ?
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>>>>>>> index 6266c8a..f391727 100644
>>>>>>> --- a/drivers/mtd/nand/denali.c
>>>>>>> +++ b/drivers/mtd/nand/denali.c
>>>>>>> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>>>>>>>                 denali->dma_avail = 1;
>>>>>>>
>>>>>>>         if (denali->dma_avail) {
>>>>>>> -               chip->buf_align = 16;
>>>>>>> +               chip->buf_align = ARCH_DMA_MINALIGN;
>>>>>>>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>>>>>>>                         denali->setup_dma = denali_setup_dma64;
>>>>>>>                 else
>>>>>>>
>>>>>>>
>>>>>>> Did you try this?
>>>>>>> I do not see unaligned cache operation.
>>>>>>
>>>>>> Nope, I'll have to assemble the hardware.
>>>>>> But this only works if dma_avail, right ?
>>>>>>
>>>>>
>>>>>
>>>>> So, what are you worried about?
>>>>
>>>> That the denali NAND is broken in mainline on socfpga.
>>>
>>> I suggested more efficient fix.
>>>
>>>
>>> I am asking about your
>>> "But this only works if dma_avail, right ?"
>>>
>>> Do you see any problem in 'dma_avail == false' case?
>>
>> I cannot test this now.
> 
> 
> denali_dma_xfer() is only called when dma_avail == true.
> 
> You do not need to worry about the cache-op
> if dma_avail == false.  Believe me.
> 
> https://github.com/u-boot/u-boot/blob/v2018.07/drivers/mtd/nand/denali.c#L621
> 
> 
> 
> 
>> But I am still not very happy about keeping two copies of code doing the
>> same.
> 
> 
> What do you mean by 'two copies of code' ?

We now have two copies of bounce buffer code, one ad-hoc variant in the
denali driver and one generic variant.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

[Masahiro Yamada]

2018-07-14 3:15 GMT+09:00 Marek Vasut <marex@denx.de>:
> On 07/13/2018 05:09 PM, Masahiro Yamada wrote:
>> 2018-07-13 23:51 GMT+09:00 Marek Vasut <marex@denx.de>:
>>> On 07/13/2018 01:05 PM, Masahiro Yamada wrote:
>>>> 2018-07-13 19:58 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>> On 07/13/2018 12:52 PM, Masahiro Yamada wrote:
>>>>>> 2018-07-13 19:35 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>> On 07/13/2018 12:22 PM, Masahiro Yamada wrote:
>>>>>>>> 2018-07-13 19:18 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>> On 07/13/2018 12:09 PM, Masahiro Yamada wrote:
>>>>>>>>>> Hi Marek
>>>>>>>>>>
>>>>>>>>>> 2018-07-13 17:56 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>> On 07/13/2018 10:23 AM, Masahiro Yamada wrote:
>>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>>
>>>>>>>>>>>> 2018-07-13 16:59 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>>>> On 07/13/2018 07:13 AM, Masahiro Yamada wrote:
>>>>>>>>>>>>>> 2018-07-12 21:51 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>>>>>> On 06/20/2018 09:14 AM, Masahiro Yamada wrote:
>>>>>>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi!
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 2018-06-20 13:43 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>>>>>>>> On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
>>>>>>>>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> 2018-06-08 5:17 GMT+09:00 Marek Vasut <marex@denx.de>:
>>>>>>>>>>>>>>>>>>> Replace the ad-hoc DMA cache management functions with common bouncebuf,
>>>>>>>>>>>>>>>>>>> since those functions are not handling cases where unaligned buffer is
>>>>>>>>>>>>>>>>>>> passed in,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Were you hit by a problem,
>>>>>>>>>>>>>>>>>> or just-in-case replacement?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Yes, UBI triggers unaligned cache operations on my system (SoCFPGA).
>>>>>>>>>>>>>>>>>> I thought I took care of the buffer alignment.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> The bounce buffer is allocated by kmalloc():
>>>>>>>>>>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1348
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> According to the lib/linux_compat.c implementation,
>>>>>>>>>>>>>>>>>> it returns memory aligned with ARCH_DMA_MINALIGN.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> If the buffer is passed from the upper MTD layer,
>>>>>>>>>>>>>>>>>> the NAND core also makes sure the enough alignment:
>>>>>>>>>>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.05/drivers/mtd/nand/denali.c#L1273
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> This is how this driver works in Linux.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I'd rather want to keep the current code
>>>>>>>>>>>>>>>>>> unless this is a real problem,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> One possible clean-up is to move dma_(un)map_single to a common place.
>>>>>>>>>>>>>>>>> Is there any chance you can try UBI on the denali nand on uniphier ? :)
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I tested the driver only for raw block devices.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> OK, I will test UBI on my platform.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> BTW, do you see the problem only in U-Boot?
>>>>>>>>>>>>>>>> Is the denali driver in Linux working fine?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Bump on this one ?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Sorry for delay.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> UBI is working for me without your patch.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Not sure what is the difference.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I will dig into it a little more, though.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Verify that you're not seeing any unaligned cache flushes. I do.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Yeah, I am testing it now,
>>>>>>>>>>>> but I never see 'Misaligned operation at range' when using UBI.
>>>>>>>>>>>>
>>>>>>>>>>>> However, I found a simple way to trigger the warning
>>>>>>>>>>>> in raw device access.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> => nand  read  81000010   0  1000
>>>>>>>>>>>>
>>>>>>>>>>>> NAND read: device 0 offset 0x0, size 0x1000
>>>>>>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>>>>>>> CACHE: Misaligned operation at range [81000010, 81001010]
>>>>>>>>>>>>  4096 bytes read: OK
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> I can fix it with one liner.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>>>>>>>>>>>> index 6266c8a..f391727 100644
>>>>>>>>>>>> --- a/drivers/mtd/nand/denali.c
>>>>>>>>>>>> +++ b/drivers/mtd/nand/denali.c
>>>>>>>>>>>> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>>>>>>>>>>>>                 denali->dma_avail = 1;
>>>>>>>>>>>>
>>>>>>>>>>>>         if (denali->dma_avail) {
>>>>>>>>>>>> -               chip->buf_align = 16;
>>>>>>>>>>>> +               chip->buf_align = ARCH_DMA_MINALIGN;
>>>>>>>>>>>>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>>>>>>>>>>>>                         denali->setup_dma = denali_setup_dma64;
>>>>>>>>>>>>                 else
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> I guess this will work for you too.
>>>>>>>>>>>
>>>>>>>>>>> Doesn't that only apply if DMA is available ?
>>>>>>>>>>
>>>>>>>>>> Of course.
>>>>>>>>>> If you use PIO instead of DMA,
>>>>>>>>>> you do not need to perform cache operation, right?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> And anyway, I'd rather use common U-Boot code than have a custom
>>>>>>>>>>> implementation in a driver which we need to maintain and fix separately.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> bounce_buffer_{start,stop} does
>>>>>>>>>> malloc() and free() every time.
>>>>>>>>>> This is not efficient.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> struct nand_chip already contains page buffers,
>>>>>>>>>> which guarantee alignment for DMA.
>>>>>>>>>>
>>>>>>>>>> https://github.com/u-boot/u-boot/blob/v2018.07/include/linux/mtd/rawnand.h#L640
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> We can rely on the NAND framework
>>>>>>>>>> for handling DMA-capable alignment.
>>>>>>>>>
>>>>>>>>> Clearly that doesn't work, otherwise I won't need this bounce buffer patch ?
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
>>>>>>>> index 6266c8a..f391727 100644
>>>>>>>> --- a/drivers/mtd/nand/denali.c
>>>>>>>> +++ b/drivers/mtd/nand/denali.c
>>>>>>>> @@ -1270,7 +1270,7 @@ int denali_init(struct denali_nand_info *denali)
>>>>>>>>                 denali->dma_avail = 1;
>>>>>>>>
>>>>>>>>         if (denali->dma_avail) {
>>>>>>>> -               chip->buf_align = 16;
>>>>>>>> +               chip->buf_align = ARCH_DMA_MINALIGN;
>>>>>>>>                 if (denali->caps & DENALI_CAP_DMA_64BIT)
>>>>>>>>                         denali->setup_dma = denali_setup_dma64;
>>>>>>>>                 else
>>>>>>>>
>>>>>>>>
>>>>>>>> Did you try this?
>>>>>>>> I do not see unaligned cache operation.
>>>>>>>
>>>>>>> Nope, I'll have to assemble the hardware.
>>>>>>> But this only works if dma_avail, right ?
>>>>>>>
>>>>>>
>>>>>>
>>>>>> So, what are you worried about?
>>>>>
>>>>> That the denali NAND is broken in mainline on socfpga.
>>>>
>>>> I suggested more efficient fix.
>>>>
>>>>
>>>> I am asking about your
>>>> "But this only works if dma_avail, right ?"
>>>>
>>>> Do you see any problem in 'dma_avail == false' case?
>>>
>>> I cannot test this now.
>>
>>
>> denali_dma_xfer() is only called when dma_avail == true.
>>
>> You do not need to worry about the cache-op
>> if dma_avail == false.  Believe me.
>>
>> https://github.com/u-boot/u-boot/blob/v2018.07/drivers/mtd/nand/denali.c#L621
>>
>>
>>
>>
>>> But I am still not very happy about keeping two copies of code doing the
>>> same.
>>
>>
>> What do you mean by 'two copies of code' ?
>
> We now have two copies of bounce buffer code, one ad-hoc variant in the
> denali driver and one generic variant.
>


The denali driver has no bounce buffer in it.

It is *you* who is trying to add a bounce buffer.




If you are referring the following code,
the purpose of denali->buf is _not_
to guarantee the DMA-safe alignment.

     /*
      * This buffer is DMA-mapped by denali_{read,write}_page_raw.  Do not
      * use devm_kmalloc() because the memory allocated by devm_ does not
      * guarantee DMA-safe alignment.
      */
     denali->buf = kmalloc(mtd->writesize + mtd->oobsize, GFP_KERNEL);
     if (!denali->buf)
             return -ENOMEM;


denali->buf is used to cope with the syndrome ECC layout.

If you do not know why this is needed,
check the commit log of
26d266e10e5eb59cfbcc47922655dc3149e1bd59
in Linux.



-- 
Best Regards
Masahiro Yamada