[U-Boot] [PATCH] mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf
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);
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#L13...
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#L12...
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);elseflush_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@lists.denx.de https://lists.denx.de/listinfo/u-boot
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#L13...
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#L12...
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 ? :)
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#L13...
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#L12...
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?
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#L13...
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#L12...
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.
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#L13...
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#L12...
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 ?
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#L13...
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#L12...
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.
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#L13...
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#L12...
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.
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#L13...
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#L12...
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@lists.denx.de https://lists.denx.de/listinfo/u-boot
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#L13... > > 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#L12... > > 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; elseI 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.
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#L13... >> >> 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#L12... >> >> 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; elseI 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#L...
We can rely on the NAND framework for handling DMA-capable alignment.
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#L13... >>> >>> 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#L12... >>> >>> 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; elseI 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#L...
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 ?
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#L13... >>>> >>>> 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#L12... >>>> >>>> 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; elseI 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#L...
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.
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#L13... >>>>> >>>>> 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#L12... >>>>> >>>>> 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; elseI 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#L...
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; elseDid 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 ?
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#L13... >>>>>> >>>>>> 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#L12... >>>>>> >>>>>> 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; elseI 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#L...
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; elseDid 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?
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#L13... >>>>>>> >>>>>>> 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#L12... >>>>>>> >>>>>>> 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#L...
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; elseDid 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.
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#L13... >>>>>>>> >>>>>>>> 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#L12... >>>>>>>> >>>>>>>> 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#L...
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; elseDid 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?
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#L13... >>>>>>>>> >>>>>>>>> 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#L12... >>>>>>>>> >>>>>>>>> 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#L... > > > 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; elseDid 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.
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#L13... >>>>>>>>>> >>>>>>>>>> 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#L12... >>>>>>>>>> >>>>>>>>>> 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#L... >> >> >> 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; elseDid 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#L62...
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@lists.denx.de https://lists.denx.de/listinfo/u-boot
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#L13... >>>>>>>>>>> >>>>>>>>>>> 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#L12... >>>>>>>>>>> >>>>>>>>>>> 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#L... >>> >>> >>> 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#L62...
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.
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#L13... >>>>>>>>>>>> >>>>>>>>>>>> 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#L12... >>>>>>>>>>>> >>>>>>>>>>>> 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#L... >>>> >>>> >>>> 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#L62...
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.
participants (2)
-
Marek Vasut -
Masahiro Yamada