mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

PATCH New Marek Vasut 2 participants 21 messages Jun 7, 2018 – Jul 13, 2018

Subsystems (2)

Memory technology devices (MTD) NAND flash

lore

Share & tools

lore

Marek Vasut

author

2018-06-07 20:17 UTC #0 lore raw

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

Cc Masahiro Yamada <[email protected]>

Cc Tom Rini <[email protected]>

---
 drivers/mtd/nand/denali.c | 41 +++++++----------------------------------
 1 file changed, 7 insertions(+), 34 deletions(-)

drivers/mtd/nand/denali.c +7 ~~-34~~

index 089ebce6dd..e5e84a58aa 100644
@@ -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);

Signature · 1 line

2.16.2

Masahiro Yamada

2018-06-19 06:39 UTC #1 lore raw

PATCH 1/1 mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

Hi Marek,

2018-06-08 5:17 GMT+09:00 Marek Vasut <[email protected]>:

Quoted 3 lines

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.

Quoted 99 lines

while common bouncebuf does handle all that.

Signed-off-by Marek Vasut <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Tom Rini <[email protected]>

---
 drivers/mtd/nand/denali.c | 41 +++++++----------------------------------
 1 file changed, 7 insertions(+), 34 deletions(-)

drivers/mtd/nand/denali.c +7 ~~-35~~

index 089ebce6dd..e5e84a58aa 100644
@@ -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

Signature · 2 lines

Best Regards
Masahiro Yamada

Marek Vasut

author

2018-06-20 04:43 UTC #2 lore raw

PATCH 1/1 mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

Quoted 1 line · Masahiro Yamada

Hi Marek,

Hi,

Quoted 8 lines

2018-06-08 5:17 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 3 lines
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).

Quoted 13 lines

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

Quoted 7 lines

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

Signature · 2 lines

Best regards,
Marek Vasut

Masahiro Yamada

2018-06-20 07:14 UTC #3 lore raw

PATCH 1/1 mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

Hi Marek,

2018-06-20 13:43 GMT+09:00 Marek Vasut <[email protected]>:

Quoted 28 lines · Masahiro Yamada

Quoted 1 line
Hi Marek,
Hi,
Quoted 8 lines
2018-06-08 5:17 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 3 lines
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).
Quoted 13 lines
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

Quoted 8 lines

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

Signature · 2 lines

Best Regards
Masahiro Yamada

Marek Vasut

author

2018-06-21 04:37 UTC #4 lore raw

PATCH 1/1 mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

Quoted 37 lines · Masahiro Yamada

Hi Marek,
2018-06-20 13:43 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 34 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi,
Quoted 8 lines
2018-06-08 5:17 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 3 lines
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).
Quoted 19 lines
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,

Quoted 7 lines

Quoted 2 lines
Quoted 1 line
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.

Quoted 2 lines

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.

Signature · 2 lines

Best regards,
Marek Vasut

Marek Vasut

author

2018-07-12 12:51 UTC #5 lore raw

PATCH 1/1 mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

In reply to Masahiro Yamada

Quoted 1 line · Masahiro Yamada

Hi Marek,

Hi!

Quoted 39 lines

2018-06-20 13:43 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 28 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi,
Quoted 8 lines
2018-06-08 5:17 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 3 lines
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).
Quoted 13 lines
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
Quoted 8 lines
Quoted 7 lines
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 ? :)

Quoted 6 lines

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 ?

Signature · 2 lines

Best regards,
Marek Vasut

Masahiro Yamada

2018-07-13 05:13 UTC #6 lore raw

PATCH 1/1 mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

2018-07-12 21:51 GMT+09:00 Marek Vasut <[email protected]>:

Quoted 44 lines · Masahiro Yamada

Quoted 1 line
Hi Marek,
Hi!
Quoted 39 lines
2018-06-20 13:43 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 36 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi,
Quoted 8 lines
2018-06-08 5:17 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 3 lines
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).
Quoted 13 lines
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
Quoted 7 lines
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 ? :)

Quoted 9 lines

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

Signature · 2 lines

Best Regards
Masahiro Yamada

Marek Vasut

author

2018-07-13 07:59 UTC #7 lore raw

PATCH 1/1 mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

Quoted 55 lines · Masahiro Yamada

2018-07-12 21:51 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 44 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi!
Quoted 39 lines
2018-06-20 13:43 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 36 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi,
Quoted 8 lines
2018-06-08 5:17 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 3 lines
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).
Quoted 13 lines
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
Quoted 7 lines
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 ? :)
Quoted 9 lines
Quoted 6 lines
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 ?

Quoted 8 lines

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.

Signature · 2 lines

Best regards,
Marek Vasut

Masahiro Yamada

2018-07-13 08:23 UTC #8 lore raw

PATCH 1/1 mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

Hi Marek,

2018-07-13 16:59 GMT+09:00 Marek Vasut <[email protected]>:

Quoted 59 lines · Masahiro Yamada

Quoted 58 lines
2018-07-12 21:51 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 44 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi!
Quoted 39 lines
2018-06-20 13:43 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 36 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi,
Quoted 8 lines
2018-06-08 5:17 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 3 lines
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).
Quoted 13 lines
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
Quoted 7 lines
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 ? :)
Quoted 9 lines
Quoted 6 lines
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.

Quoted 7 lines

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

drivers/mtd/nand/denali.c +1 -1

index 6266c8a..f391727 100644
@@ -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.

Quoted 1 line

Note that my CPU core is CortexA9, armv7a.

I tested on my Coretex-A9 SoC.

Quoted 8 lines

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

Signature · 2 lines

Best Regards
Masahiro Yamada

Marek Vasut

author

2018-07-13 08:56 UTC #9 lore raw

PATCH 1/1 mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

Quoted 110 lines · Masahiro Yamada

Hi Marek,
2018-07-13 16:59 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 59 lines · Masahiro Yamada
Quoted 58 lines
2018-07-12 21:51 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 44 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi!
Quoted 39 lines
2018-06-20 13:43 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 36 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi,
Quoted 8 lines
2018-06-08 5:17 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 3 lines
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).
Quoted 13 lines
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
Quoted 7 lines
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 ? :)
Quoted 9 lines
Quoted 6 lines
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.
Quoted 7 lines
Quoted 5 lines
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.
drivers/mtd/nand/denali.c +1 -1
index 6266c8a..f391727 100644
@@ -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

Quoted 1 line

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.

Signature · 2 lines

Best regards,
Marek Vasut

Masahiro Yamada

2018-07-13 10:09 UTC #10 lore raw

PATCH 1/1 mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

Hi Marek

2018-07-13 17:56 GMT+09:00 Marek Vasut <[email protected]>:

Quoted 111 lines · Masahiro Yamada

Quoted 110 lines
Hi Marek,
2018-07-13 16:59 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 59 lines · Masahiro Yamada
Quoted 58 lines
2018-07-12 21:51 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 44 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi!
Quoted 39 lines
2018-06-20 13:43 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 36 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi,
Quoted 8 lines
2018-06-08 5:17 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 3 lines
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).
Quoted 13 lines
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
Quoted 7 lines
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 ? :)
Quoted 9 lines
Quoted 6 lines
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.
Quoted 7 lines
Quoted 5 lines
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.
drivers/mtd/nand/denali.c +1 -1
index 6266c8a..f391727 100644
@@ -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

Quoted 3 lines

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

Quoted 2 lines

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.

Signature · 2 lines

Best Regards
Masahiro Yamada

Marek Vasut

author

2018-07-13 10:18 UTC #11 lore raw

PATCH 1/1 mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

Quoted 133 lines · Masahiro Yamada

Hi Marek
2018-07-13 17:56 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 111 lines · Masahiro Yamada
Quoted 110 lines
Hi Marek,
2018-07-13 16:59 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 59 lines · Masahiro Yamada
Quoted 58 lines
2018-07-12 21:51 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 44 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi!
Quoted 39 lines
2018-06-20 13:43 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 36 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi,
Quoted 8 lines
2018-06-08 5:17 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 3 lines
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).
Quoted 13 lines
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
Quoted 7 lines
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 ? :)
Quoted 9 lines
Quoted 6 lines
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.
Quoted 7 lines
Quoted 5 lines
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.
drivers/mtd/nand/denali.c +1 -1
index 6266c8a..f391727 100644
@@ -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
Quoted 3 lines
Quoted 1 line
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?
Quoted 2 lines
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.

Quoted 8 lines

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 ?

Signature · 2 lines

Best regards,
Marek Vasut

Masahiro Yamada

2018-07-13 10:22 UTC #12 lore raw

PATCH 1/1 mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

2018-07-13 19:18 GMT+09:00 Marek Vasut <[email protected]>:

Quoted 137 lines · Masahiro Yamada

Quoted 136 lines
Hi Marek
2018-07-13 17:56 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 111 lines · Masahiro Yamada
Quoted 110 lines
Hi Marek,
2018-07-13 16:59 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 59 lines · Masahiro Yamada
Quoted 58 lines
2018-07-12 21:51 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 44 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi!
Quoted 39 lines
2018-06-20 13:43 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 36 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi,
Quoted 8 lines
2018-06-08 5:17 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 3 lines
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).
Quoted 13 lines
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
Quoted 7 lines
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 ? :)
Quoted 9 lines
Quoted 6 lines
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.
Quoted 7 lines
Quoted 5 lines
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.
drivers/mtd/nand/denali.c +1 -1
index 6266c8a..f391727 100644
@@ -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
Quoted 3 lines
Quoted 1 line
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?
Quoted 2 lines
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.

Quoted 8 lines

Quoted 5 lines
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 ?

drivers/mtd/nand/denali.c +1 -1

index 6266c8a..f391727 100644
@@ -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.

Signature · 2 lines

Best Regards
Masahiro Yamada

Marek Vasut

author

2018-07-13 10:35 UTC #13 lore raw

PATCH 1/1 mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

Quoted 165 lines · Masahiro Yamada

2018-07-13 19:18 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 145 lines · Masahiro Yamada
Quoted 133 lines
Hi Marek
2018-07-13 17:56 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 111 lines · Masahiro Yamada
Quoted 110 lines
Hi Marek,
2018-07-13 16:59 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 59 lines · Masahiro Yamada
Quoted 58 lines
2018-07-12 21:51 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 44 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi!
Quoted 39 lines
2018-06-20 13:43 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 36 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi,
Quoted 8 lines
2018-06-08 5:17 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 3 lines
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).
Quoted 13 lines
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
Quoted 7 lines
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 ? :)
Quoted 9 lines
Quoted 6 lines
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.
Quoted 7 lines
Quoted 5 lines
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.
drivers/mtd/nand/denali.c +1 -1
index 6266c8a..f391727 100644
@@ -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
Quoted 3 lines
Quoted 1 line
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?
Quoted 2 lines
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.
Quoted 8 lines
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 ?
drivers/mtd/nand/denali.c +1 -1
index 6266c8a..f391727 100644
@@ -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

Quoted 2 lines

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 ?

Signature · 2 lines

Best regards,
Marek Vasut

Masahiro Yamada

2018-07-13 10:52 UTC #14 lore raw

PATCH 1/1 mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

2018-07-13 19:35 GMT+09:00 Marek Vasut <[email protected]>:

Quoted 166 lines · Masahiro Yamada

Quoted 165 lines
2018-07-13 19:18 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 145 lines · Masahiro Yamada
Quoted 133 lines
Hi Marek
2018-07-13 17:56 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 111 lines · Masahiro Yamada
Quoted 110 lines
Hi Marek,
2018-07-13 16:59 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 59 lines · Masahiro Yamada
Quoted 58 lines
2018-07-12 21:51 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 44 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi!
Quoted 39 lines
2018-06-20 13:43 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 36 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi,
Quoted 8 lines
2018-06-08 5:17 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 3 lines
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).
Quoted 13 lines
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
Quoted 7 lines
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 ? :)
Quoted 9 lines
Quoted 6 lines
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.
Quoted 7 lines
Quoted 5 lines
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.
drivers/mtd/nand/denali.c +1 -1
index 6266c8a..f391727 100644
@@ -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
Quoted 3 lines
Quoted 1 line
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?
Quoted 2 lines
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.
Quoted 8 lines
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 ?
drivers/mtd/nand/denali.c +1 -1
index 6266c8a..f391727 100644
@@ -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

Quoted 6 lines

Quoted 2 lines
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?

Signature · 2 lines

Best Regards
Masahiro Yamada

Marek Vasut

author

2018-07-13 10:58 UTC #15 lore raw

PATCH 1/1 mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

Quoted 175 lines · Masahiro Yamada

2018-07-13 19:35 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 166 lines · Masahiro Yamada
Quoted 165 lines
2018-07-13 19:18 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 145 lines · Masahiro Yamada
Quoted 133 lines
Hi Marek
2018-07-13 17:56 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 111 lines · Masahiro Yamada
Quoted 110 lines
Hi Marek,
2018-07-13 16:59 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 59 lines · Masahiro Yamada
Quoted 58 lines
2018-07-12 21:51 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 44 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi!
Quoted 39 lines
2018-06-20 13:43 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 36 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi,
Quoted 8 lines
2018-06-08 5:17 GMT+09:00 Marek Vasut <[email protected]>:
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).
Quoted 20 lines
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 ? :)
Quoted 9 lines
Quoted 6 lines
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.
Quoted 7 lines
Quoted 5 lines
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.
drivers/mtd/nand/denali.c +1 -1
index 6266c8a..f391727 100644
@@ -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
Quoted 3 lines
Quoted 1 line
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?
Quoted 2 lines
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.
Quoted 8 lines
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 ?
drivers/mtd/nand/denali.c +1 -1
index 6266c8a..f391727 100644
@@ -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
Quoted 6 lines
Quoted 2 lines
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 ?

Quoted 1 line

So, what are you worried about?

That the denali NAND is broken in mainline on socfpga.

Signature · 2 lines

Best regards,
Marek Vasut

Masahiro Yamada

2018-07-13 11:05 UTC #16 lore raw

PATCH 1/1 mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

2018-07-13 19:58 GMT+09:00 Marek Vasut <[email protected]>:

Quoted 176 lines · Masahiro Yamada

Quoted 175 lines
2018-07-13 19:35 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 172 lines · Masahiro Yamada
Quoted 165 lines
2018-07-13 19:18 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 145 lines · Masahiro Yamada
Quoted 133 lines
Hi Marek
2018-07-13 17:56 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 111 lines · Masahiro Yamada
Quoted 110 lines
Hi Marek,
2018-07-13 16:59 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 59 lines · Masahiro Yamada
Quoted 58 lines
2018-07-12 21:51 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 44 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi!
Quoted 39 lines
2018-06-20 13:43 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 36 lines · Masahiro Yamada
Hi Marek,
Hi,
2018-06-08 5:17 GMT+09:00 Marek Vasut <[email protected]>:
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 ? :)
Quoted 9 lines
Quoted 6 lines
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.
Quoted 7 lines
Quoted 5 lines
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.
drivers/mtd/nand/denali.c +1 -1
index 6266c8a..f391727 100644
@@ -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
Quoted 3 lines
Quoted 1 line
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?
Quoted 2 lines
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.
Quoted 8 lines
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 ?
drivers/mtd/nand/denali.c +1 -1
index 6266c8a..f391727 100644
@@ -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
Quoted 2 lines
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 ?

Quoted 3 lines

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

Signature · 2 lines

Best Regards
Masahiro Yamada

Marek Vasut

author

2018-07-13 14:51 UTC #17 lore raw

PATCH 1/1 mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

Quoted 181 lines · Masahiro Yamada

2018-07-13 19:58 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 176 lines · Masahiro Yamada
Quoted 175 lines
2018-07-13 19:35 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 172 lines · Masahiro Yamada
Quoted 165 lines
2018-07-13 19:18 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 145 lines · Masahiro Yamada
Quoted 133 lines
Hi Marek
2018-07-13 17:56 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 111 lines · Masahiro Yamada
Quoted 110 lines
Hi Marek,
2018-07-13 16:59 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 59 lines · Masahiro Yamada
Quoted 58 lines
2018-07-12 21:51 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 44 lines · Masahiro Yamada
Quoted 1 line
Hi Marek,
Hi!
Quoted 39 lines
2018-06-20 13:43 GMT+09:00 Marek Vasut <[email protected]>:
On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
Hi Marek,
Hi,
2018-06-08 5:17 GMT+09:00 Marek Vasut <[email protected]>:
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 ? :)
Quoted 9 lines
Quoted 6 lines
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.
Quoted 7 lines
Quoted 5 lines
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.
drivers/mtd/nand/denali.c +1 -1
index 6266c8a..f391727 100644
@@ -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
Quoted 3 lines
Quoted 1 line
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?
Quoted 2 lines
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.
Quoted 8 lines
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 ?
drivers/mtd/nand/denali.c +1 -1
index 6266c8a..f391727 100644
@@ -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
Quoted 2 lines
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 ?
Quoted 3 lines
Quoted 1 line
So, what are you worried about?
That the denali NAND is broken in mainline on socfpga.

Quoted 7 lines

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.

Signature · 2 lines

Best regards,
Marek Vasut

Masahiro Yamada

2018-07-13 15:09 UTC #18 lore raw

PATCH 1/1 mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

2018-07-13 23:51 GMT+09:00 Marek Vasut <[email protected]>:

Quoted 185 lines · Masahiro Yamada

Quoted 184 lines
2018-07-13 19:58 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 176 lines · Masahiro Yamada
Quoted 175 lines
2018-07-13 19:35 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 172 lines · Masahiro Yamada
Quoted 165 lines
2018-07-13 19:18 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 145 lines · Masahiro Yamada
Quoted 133 lines
Hi Marek
2018-07-13 17:56 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 111 lines · Masahiro Yamada
Quoted 110 lines
Hi Marek,
2018-07-13 16:59 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 59 lines · Masahiro Yamada
Quoted 58 lines
2018-07-12 21:51 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 53 lines · Masahiro Yamada
Hi Marek,
Hi!
2018-06-20 13:43 GMT+09:00 Marek Vasut <[email protected]>:
On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
Hi Marek,
Hi,
2018-06-08 5:17 GMT+09:00 Marek Vasut <[email protected]>:
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.
Quoted 7 lines
Quoted 5 lines
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.
drivers/mtd/nand/denali.c +1 -1
index 6266c8a..f391727 100644
@@ -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
Quoted 3 lines
Quoted 1 line
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?
Quoted 2 lines
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.
Quoted 8 lines
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 ?
drivers/mtd/nand/denali.c +1 -1
index 6266c8a..f391727 100644
@@ -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
Quoted 2 lines
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 ?
Quoted 3 lines
Quoted 1 line
So, what are you worried about?
That the denali NAND is broken in mainline on socfpga.
I suggested more efficient fix.

Quoted 6 lines

Quoted 4 lines
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

Quoted 2 lines

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

Quoted 7 lines

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

Signature · 2 lines

Best Regards
Masahiro Yamada

Marek Vasut

author

2018-07-13 18:15 UTC #19 lore raw

PATCH 1/1 mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

Quoted 204 lines · Masahiro Yamada

2018-07-13 23:51 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 185 lines · Masahiro Yamada
Quoted 184 lines
2018-07-13 19:58 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 176 lines · Masahiro Yamada
Quoted 175 lines
2018-07-13 19:35 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 172 lines · Masahiro Yamada
Quoted 165 lines
2018-07-13 19:18 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 145 lines · Masahiro Yamada
Quoted 133 lines
Hi Marek
2018-07-13 17:56 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 111 lines · Masahiro Yamada
Quoted 110 lines
Hi Marek,
2018-07-13 16:59 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 59 lines · Masahiro Yamada
Quoted 58 lines
2018-07-12 21:51 GMT+09:00 Marek Vasut <[email protected]>:
On 06/20/2018 09:14 AM, Masahiro Yamada wrote:
Hi Marek,
Hi!
2018-06-20 13:43 GMT+09:00 Marek Vasut <[email protected]>:
On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
Hi Marek,
Hi,
2018-06-08 5:17 GMT+09:00 Marek Vasut <[email protected]>:
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.
Quoted 7 lines
Quoted 5 lines
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.
drivers/mtd/nand/denali.c +1 -1
index 6266c8a..f391727 100644
@@ -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
Quoted 3 lines
Quoted 1 line
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?
Quoted 2 lines
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.
Quoted 8 lines
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 ?
drivers/mtd/nand/denali.c +1 -1
index 6266c8a..f391727 100644
@@ -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
Quoted 2 lines
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 ?
Quoted 3 lines
Quoted 1 line
So, what are you worried about?
That the denali NAND is broken in mainline on socfpga.
I suggested more efficient fix.
Quoted 6 lines
Quoted 4 lines
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

Quoted 5 lines

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

Signature · 2 lines

Best regards,
Marek Vasut

Masahiro Yamada

2018-07-13 23:16 UTC #20 lore raw

PATCH 1/1 mtd: nand: denali: Replace the ad-hoc cache management with bouncebuf

2018-07-14 3:15 GMT+09:00 Marek Vasut <[email protected]>:

Quoted 205 lines · Masahiro Yamada

Quoted 204 lines
2018-07-13 23:51 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 185 lines · Masahiro Yamada
Quoted 184 lines
2018-07-13 19:58 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 176 lines · Masahiro Yamada
Quoted 175 lines
2018-07-13 19:35 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 172 lines · Masahiro Yamada
Quoted 165 lines
2018-07-13 19:18 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 145 lines · Masahiro Yamada
Quoted 133 lines
Hi Marek
2018-07-13 17:56 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 111 lines · Masahiro Yamada
Quoted 110 lines
Hi Marek,
2018-07-13 16:59 GMT+09:00 Marek Vasut <[email protected]>:
Quoted 66 lines · Masahiro Yamada
2018-07-12 21:51 GMT+09:00 Marek Vasut <[email protected]>:
On 06/20/2018 09:14 AM, Masahiro Yamada wrote:
Hi Marek,
Hi!
2018-06-20 13:43 GMT+09:00 Marek Vasut <[email protected]>:
On 06/19/2018 08:39 AM, Masahiro Yamada wrote:
Hi Marek,
Hi,
2018-06-08 5:17 GMT+09:00 Marek Vasut <[email protected]>:
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.
drivers/mtd/nand/denali.c +1 -1
index 6266c8a..f391727 100644
@@ -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
Quoted 3 lines
Quoted 1 line
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?
Quoted 2 lines
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.
Quoted 8 lines
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 ?
drivers/mtd/nand/denali.c +1 -1
index 6266c8a..f391727 100644
@@ -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
Quoted 2 lines
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 ?
Quoted 3 lines
Quoted 1 line
So, what are you worried about?
That the denali NAND is broken in mainline on socfpga.
I suggested more efficient fix.
Quoted 6 lines
Quoted 4 lines
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

Quoted 9 lines

Quoted 5 lines
Quoted 2 lines
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.

Signature · 2 lines

Best Regards
Masahiro Yamada