nand: lpc32xx: add SLC NAND driver

1) Fixed checkpatch script output in legacy code.
A single warning remaining.

The following warning from the legacy code is still present:
lpc32xx_nand.c:195: WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt

From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Albert ARIBAUD
Sent: 17-Jul-15 5:20 PM

Hello Sylvain,

Quoted 6 lines · slemieux.tyco at gmail.com

1) Fixed checkpatch script output in legacy code.
A single warning remaining.

The following warning from the legacy code is still present:
lpc32xx_nand.c:195: WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt

drivers/mtd/nand/lpc32xx_nand_slc.c line 198

Quoted 8 lines

drivers/mtd/nand/lpc32xx_nand_slc.c new file +8 -0

+static u_char lpc32xx_read_byte(struct mtd_info *mtd)
+{
+   struct nand_chip *this = mtd->priv;
+   unsigned long *preg = (unsigned long *)this->IO_ADDR_R;
+   volatile unsigned long tmp32;
+   tmp32 = *preg;
+   return (u_char)tmp32;
+}

The volatile above has no reason to exist; the warning is justified here as we have accessors that guarantee that the access will not be optimized away or reordered, juste like the 'volatile' above tries to do (and yes, these accessors *use* 'volatile'. All the more a reason not to use it again here).

Besides, the code is quite verbose and not precise enough. Yes, 'unsigned long' is 32-bit-ish, but in U-Boot, when something is 32-bit, that is explicit.

All in all, the whole function could be expressed as:

      static u_char lpc32xx_read_byte(struct mtd_info *mtd)
      {
              struct nand_chip *this = mtd->priv;

              return (u_char)readl(this->IO_ADDR_R);
      }

BTW, isn't IO_ADDR_R pointing to the data register, and isn't the data register 16-bits? And if so, then why the 32-bits read?

Also, I did not find where the IO_ADDR_R field is assigned. Did I miss it?

This is just one case, but I suspect in many places, the code is unnecessarily complex. I understand it was ported, not written from scratch, but I think porting code should not prevent us from making it smaller, more efficient and more maintainable.

Amicalement,
--
Albert.

Hi Albert,

Thanks for the feedback.

Quoted 44 lines

From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Albert ARIBAUD
Sent: 17-Jul-15 5:20 PM

Hello Sylvain,

Quoted 7 lines · slemieux.tyco at gmail.com

1) Fixed checkpatch script output in legacy code.
A single warning remaining.

The following warning from the legacy code is still present:
lpc32xx_nand.c:195: WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt

drivers/mtd/nand/lpc32xx_nand_slc.c line 198

Quoted 8 lines

drivers/mtd/nand/lpc32xx_nand_slc.c new file +8 -0

+static u_char lpc32xx_read_byte(struct mtd_info *mtd)
+{
+   struct nand_chip *this = mtd->priv;
+   unsigned long *preg = (unsigned long *)this->IO_ADDR_R;
+   volatile unsigned long tmp32;
+   tmp32 = *preg;
+   return (u_char)tmp32;
+}

The volatile above has no reason to exist; the warning is justified here as we have accessors that guarantee that the access will not be optimized away or reordered, juste like the 'volatile' above tries to do (and yes, these accessors *use* 'volatile'. All the more a reason not to use it again here).

Besides, the code is quite verbose and not precise enough. Yes, 'unsigned long' is 32-bit-ish, but in U-Boot, when something is 32-bit, that is explicit.

All in all, the whole function could be expressed as:

      static u_char lpc32xx_read_byte(struct mtd_info *mtd)
      {
              struct nand_chip *this = mtd->priv;

              return (u_char)readl(this->IO_ADDR_R);
      }

Quoted 3 lines

BTW, isn't IO_ADDR_R pointing to the data register, and isn't the data register 16-bits? And if so, then why the 32-bits read?

The register is 16 bits; this implementation is the porting of the initial code.
I will wait for feedback and see how we want to approach this (add DMA and HW ECC to the NAND SLC driver sent by Vladimir or update the driver as part of the porting effort).

Hi Albert,

Thanks for the feedback.

Quoted 39 lines

From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Albert ARIBAUD
Sent: 17-Jul-15 5:20 PM

Hello Sylvain,

Quoted 6 lines · slemieux.tyco at gmail.com

1) Fixed checkpatch script output in legacy code.
A single warning remaining.

The following warning from the legacy code is still present:
lpc32xx_nand.c:195: WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt

drivers/mtd/nand/lpc32xx_nand_slc.c line 198

Quoted 8 lines

drivers/mtd/nand/lpc32xx_nand_slc.c new file +8 -0

+static u_char lpc32xx_read_byte(struct mtd_info *mtd)
+{
+   struct nand_chip *this = mtd->priv;
+   unsigned long *preg = (unsigned long *)this->IO_ADDR_R;
+   volatile unsigned long tmp32;
+   tmp32 = *preg;
+   return (u_char)tmp32;
+}

The volatile above has no reason to exist; the warning is justified here as we have accessors that guarantee that the access will not be optimized away or reordered, juste like the 'volatile' above tries to do (and yes, these accessors *use* 'volatile'. All the more a reason not to use it again here).

Besides, the code is quite verbose and not precise enough. Yes, 'unsigned long' is 32-bit-ish, but in U-Boot, when something is 32-bit, that is explicit.

All in all, the whole function could be expressed as:

      static u_char lpc32xx_read_byte(struct mtd_info *mtd)
      {
              struct nand_chip *this = mtd->priv;

Quoted 6 lines

              return (u_char)readl(this->IO_ADDR_R);
      }

BTW, isn't IO_ADDR_R pointing to the data register, and isn't the data register 16-bits? And if so, then why the 32-bits read?

The register is 16 bits; this implementation is the porting of the initial code.

I will wait for feedback and see how we want to approach this (add DMA and HW ECC to the NAND SLC driver sent by Vladimir or update the driver as part of the porting effort).

Quoted 2 lines

Also, I did not find where the IO_ADDR_R field is assigned. Did I miss it?

"CONFIG_SYS_NAND_SELF_INIT" is not define; the legacy BSP driver is using the default initialization done within "nand_init_chip()" function inside "drivers/mtd/nand/nand.c".

Quoted 10 lines

This is just one case, but I suspect in many places, the code is unnecessarily complex. I understand it was ported, not written from scratch, but I think porting code should not prevent us from making it smaller, more efficient and more maintainable.

Amicalement,
--
Albert.

Sylvain

From: Vladimir Zapolskiy [mailto:vz at mleia.com]
Sent: 17-Jul-15 7:10 PM

Hi Sylvain, Albert,

Quoted 43 lines · SYLVAIN

Hi Albert,

Thanks for the feedback.

Quoted 39 lines

From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Albert ARIBAUD
Sent: 17-Jul-15 5:20 PM

Hello Sylvain,

Quoted 6 lines · slemieux.tyco at gmail.com

1) Fixed checkpatch script output in legacy code.
A single warning remaining.

The following warning from the legacy code is still present:
lpc32xx_nand.c:195: WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt

drivers/mtd/nand/lpc32xx_nand_slc.c line 198

Quoted 8 lines

drivers/mtd/nand/lpc32xx_nand_slc.c new file +8 -0

+static u_char lpc32xx_read_byte(struct mtd_info *mtd)
+{
+   struct nand_chip *this = mtd->priv;
+   unsigned long *preg = (unsigned long *)this->IO_ADDR_R;
+   volatile unsigned long tmp32;
+   tmp32 = *preg;
+   return (u_char)tmp32;
+}

The volatile above has no reason to exist; the warning is justified here as we have accessors that guarantee that the access will not be optimized away or reordered, juste like the 'volatile' above tries to do (and yes, these accessors *use* 'volatile'. All the more a reason not to use it again here).

Besides, the code is quite verbose and not precise enough. Yes, 'unsigned long' is 32-bit-ish, but in U-Boot, when something is 32-bit, that is explicit.

All in all, the whole function could be expressed as:

      static u_char lpc32xx_read_byte(struct mtd_info *mtd)
      {
              struct nand_chip *this = mtd->priv;

Quoted 8 lines

Quoted 6 lines

              return (u_char)readl(this->IO_ADDR_R);
      }

BTW, isn't IO_ADDR_R pointing to the data register, and isn't the data register 16-bits? And if so, then why the 32-bits read?

The register is 16 bits; this implementation is the porting of the initial code.

hmm, you remember it was discussed yesterday that the data register is 32-bit...

----8<----
5.2 Data FIFO

There is only one Data FIFO. The Data FIFO is configured either in Read or in Write mode.

1. When the Data FIFO is configured in Read mode, the sequencer reads data from the NAND flash, and stores the data in the Data FIFO. The FIFO is then emptied by 32-bit reads on the AHB bus from either the ARM or the DMA.

2. When the Data FIFO is configured in Write mode, the ARM or the DMA writes data to the FIFO with 32-bit AHB bus writes. The sequencer then takes data out of the FIFO 8 bits at a time, and writes data to the NAND flash.

----8<----

Quoted 3 lines

I will wait for feedback and see how we want to approach this (add DMA and HW ECC to the NAND SLC driver sent by Vladimir or update the driver as part of the porting effort).

now when I see the code I still haven't changed my opinion, I would propose to add HW ECC processing on top of my trivial change.

Some general reasons:

* I agree with Albert that the code is a bit overcomplicated and can be improved, basic functions like read_byte(), cmd_ctrl() etc are better in my version IMHO --- for example just compare Kevin's monstrous lpc32xx_nand_hwcontrol() and my lpc32xx_nand_cmd_ctrl() --- so if my version is reviewed and accepted, then there is no need to fix all the same issues in this legacy forward-ported code,

* this change strictly depends on DMA driver (the driver simply does not work, if DMA is disabled), this means that DMA driver must be 1/3 and SLC NAND should go as 2/3, this implies that DMA driver is reviewed and accepted by maintainers firstly,

* I don't see any users of this new code, this addresses Albert's notice about adding dead code ---
http://lists.denx.de/pipermail/u-boot/2015-July/219124.html

* What about functional support in SPL? Is it correct, that if I want to have this code in SPL, then I have to pull in DMA driver as a mandatory dependency to tiny SPL?

Quoted 12 lines

Quoted 2 lines

Also, I did not find where the IO_ADDR_R field is assigned. Did I miss it?

"CONFIG_SYS_NAND_SELF_INIT" is not define; the legacy BSP driver is using the default initialization done within "nand_init_chip()" function inside "drivers/mtd/nand/nand.c".

Quoted 6 lines

This is just one case, but I suspect in many places, the code is unnecessarily complex. I understand it was ported, not written from scratch, but I think porting code should not prevent us from making it smaller, more efficient and more maintainable.

Quoted 7 lines

Quoted 4 lines

Amicalement,
--
Albert.

Sylvain

Hi Sylvain, Albert,

Quoted 43 lines · SYLVAIN

Hi Albert,

Thanks for the feedback.

Quoted 39 lines

From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Albert ARIBAUD
Sent: 17-Jul-15 5:20 PM

Hello Sylvain,

Quoted 6 lines · slemieux.tyco at gmail.com

1) Fixed checkpatch script output in legacy code.
A single warning remaining.

The following warning from the legacy code is still present:
lpc32xx_nand.c:195: WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt

drivers/mtd/nand/lpc32xx_nand_slc.c line 198

Quoted 8 lines

drivers/mtd/nand/lpc32xx_nand_slc.c new file +8 -0

+static u_char lpc32xx_read_byte(struct mtd_info *mtd)
+{
+   struct nand_chip *this = mtd->priv;
+   unsigned long *preg = (unsigned long *)this->IO_ADDR_R;
+   volatile unsigned long tmp32;
+   tmp32 = *preg;
+   return (u_char)tmp32;
+}

The volatile above has no reason to exist; the warning is justified here as we have accessors that guarantee that the access will not be optimized away or reordered, juste like the 'volatile' above tries to do (and yes, these accessors *use* 'volatile'. All the more a reason not to use it again here).

Besides, the code is quite verbose and not precise enough. Yes, 'unsigned long' is 32-bit-ish, but in U-Boot, when something is 32-bit, that is explicit.

All in all, the whole function could be expressed as:

      static u_char lpc32xx_read_byte(struct mtd_info *mtd)
      {
              struct nand_chip *this = mtd->priv;

Quoted 8 lines

Quoted 6 lines

              return (u_char)readl(this->IO_ADDR_R);
      }

BTW, isn't IO_ADDR_R pointing to the data register, and isn't the data register 16-bits? And if so, then why the 32-bits read?

The register is 16 bits; this implementation is the porting of the initial code.

hmm, you remember it was discussed yesterday that the data register is 32-bit...

----8<----
5.2 Data FIFO

There is only one Data FIFO. The Data FIFO is configured either in Read or in Write mode.

1. When the Data FIFO is configured in Read mode, the sequencer reads data from the NAND flash, and stores the data in the Data FIFO. The FIFO is then emptied by 32-bit reads on the AHB bus from either the ARM or the DMA.

2. When the Data FIFO is configured in Write mode, the ARM or the DMA writes data to the FIFO with 32-bit AHB bus writes. The sequencer then takes data out of the FIFO 8 bits at a time, and writes data to the NAND flash.

----8<----

Quoted 3 lines

I will wait for feedback and see how we want to approach this (add DMA and HW ECC to the NAND SLC driver sent by Vladimir or update the driver as part of the porting effort).

now when I see the code I still haven't changed my opinion, I would propose to add HW ECC processing on top of my trivial change.

Some general reasons:

* I agree with Albert that the code is a bit overcomplicated and can be improved, basic functions like read_byte(), cmd_ctrl() etc are better in my version IMHO --- for example just compare Kevin's monstrous lpc32xx_nand_hwcontrol() and my lpc32xx_nand_cmd_ctrl() --- so if my version is reviewed and accepted, then there is no need to fix all the same issues in this legacy forward-ported code,

* this change strictly depends on DMA driver (the driver simply does not work, if DMA is disabled), this means that DMA driver must be 1/3 and SLC NAND should go as 2/3, this implies that DMA driver is reviewed and accepted by maintainers firstly,

* I don't see any users of this new code, this addresses Albert's notice about adding dead code ---
http://lists.denx.de/pipermail/u-boot/2015-July/219124.html

* What about functional support in SPL? Is it correct, that if I want to have this code in SPL, then I have to pull in DMA driver as a mandatory dependency to tiny SPL?

From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net]
Sent: 18-Jul-15 1:50 AM

Hello Vladimir,

Quoted 16 lines · Vladimir Zapolskiy

Hi Sylvain, Albert,

Quoted 13 lines · SYLVAIN

Hi Albert,

Thanks for the feedback.

Quoted 9 lines

From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Albert ARIBAUD
Sent: 17-Jul-15 5:20 PM

Hello Sylvain,

Quoted 1 line · slemieux.tyco at gmail.com

Quoted 4 lines

* What about functional support in SPL? Is it correct, that if I want to have this code in SPL, then I have to pull in DMA driver as a mandatory dependency to tiny SPL?

You have to ensure DMA, but whether you pull the whole BSP driver or hard-code the necessary parts into a single self-contained SLC NAND + DMA file is a design decision. That said, I would personally go for pulling the driver *and* adding preprocessing out any part of it not necessary for SPL -- not so much for size (the compiler and linker should optimize useless parts away on their own anyway) than for clarity and maintenance (readers of the driver code would know which part is needed for SPL and which is not).

Amicalement,
--
Albert.

Hi Vladimir and Albert,

See comment, questions and test results below;

Quoted 23 lines

From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net]
Sent: 18-Jul-15 1:50 AM

Hello Vladimir,

Quoted 16 lines · Vladimir Zapolskiy

Hi Sylvain, Albert,

Quoted 13 lines · SYLVAIN

Hi Albert,

Thanks for the feedback.

Quoted 9 lines

From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Albert ARIBAUD
Sent: 17-Jul-15 5:20 PM

Hello Sylvain,

Quoted 1 line · slemieux.tyco at gmail.com

....

Quoted 5 lines

Quoted 4 lines

* What about functional support in SPL? Is it correct, that if I want to have this code in SPL, then I have to pull in DMA driver as a mandatory dependency to tiny SPL?

Quoted 9 lines

You have to ensure DMA, but whether you pull the whole BSP driver or hard-code the necessary parts into a single self-contained SLC NAND + DMA file is a design decision. That said, I would personally go for pulling the driver *and* adding preprocessing out any part of it not necessary for SPL -- not so much for size (the compiler and linker should optimize useless parts away on their own anyway) than for clarity and maintenance (readers of the driver code would know which part is needed for SPL and which is not).

I am in the process of putting a patch together to add the hardware ECC support on top of the SLC NAND driver patch
(https://patchwork.ozlabs.org/patch/497308/).

A have a few questions:
1) The LPC32xx SLC NAND driver is using a custom NAND ECC Layout for small page; You can refer to the Kernel driver:
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/drivers/mtd/nand/lpc32xx_slc.c?id=refs/tags/v4.1.3

Are you planning to update your driver with the change or should I submit a separate patch for it before adding the support for DMA and hardware ECC?

2) As suggested by Albert, I will add a conditional compile option to ensure the original version of the driver (no DMA) can be used for SPL.

I was planning to do it using the configuration option use to select the LPC32xx DMA driver (CONFIG_DMA_LPC32XX). Are you OK with this approach?

3) Should I separate the NAND SLC /DMA & the USB into 2 separate patch?

Test result (full command log below):

Clock configuration:
CPU clock: 266MHz / AHB bus clock: 133MHz / Peripheral clock: 13MHz

1) NAND SLC driver: https://patchwork.ozlabs.org/patch/497308/
NAND read: 51904512 bytes
read.raw: 1.444 MiB per second / read: 0.625 MiB per second

2) NAND SLC driver: https://patchwork.ozlabs.org/patch/497308/
& Legacy BSP porting (hardware ECC & DMA)
NAND read: 51904512 bytes
read.raw: 2.272 MiB per second / read: 2.139 MiB per second

------------------
Full log:

NAND SLC driver: https://patchwork.ozlabs.org/patch/497308/

==> gettime; nand read.raw 0x80000000 0xd00000 0x6000; gettime Timer val: 259607
Seconds : 259
Remainder : 607
sys_hz = 1000

NAND read: 51904512 bytes read: OK
Timer val: 293885
Seconds : 293
Remainder : 885
sys_hz = 1000
--> 1.444 MiB per second

==> gettime; nand read.e 0x80000000 0xd00000 0x3180000; gettime Timer val: 11304
Seconds : 11
Remainder : 304
sys_hz = 1000

NAND read: device 0 offset 0xd00000, size 0x3180000
51904512 bytes read: OK
Timer val: 90495
Seconds : 90
Remainder : 495
sys_hz = 1000
--> 0.625 MiB per second

NAND SLC driver: https://patchwork.ozlabs.org/patch/497308/
& Legacy BSP porting (hardware ECC & DMA)

==> gettime; nand read.raw 0x80000000 0xd00000 0x6000; gettime Timer val: 49705
Seconds : 49
Remainder : 705
sys_hz = 1000

NAND read: 51904512 bytes read: OK
Timer val: 71483
Seconds : 71
Remainder : 483
sys_hz = 1000
--> 2.272 MiB per second

Timer val: 280282
Seconds : 280
Remainder : 282
sys_hz = 1000

NAND read: device 0 offset 0xd00000, size 0x3180000
51904512 bytes read: OK
Timer val: 303423
Seconds : 303
Remainder : 423
sys_hz = 1000
--> 2.139 MiB per second

From: Vladimir Zapolskiy [mailto:vz at mleia.com]
Sent: 27-Jul-15 8:22 PM

Hi Sylvain,

Quoted 54 lines · SYLVAIN

Hi Vladimir and Albert,

See comment, questions and test results below;

Quoted 23 lines

From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net]
Sent: 18-Jul-15 1:50 AM

Hello Vladimir,

Quoted 16 lines · Vladimir Zapolskiy

Hi Sylvain, Albert,

Quoted 13 lines · SYLVAIN

Hi Albert,

Thanks for the feedback.

Quoted 9 lines

From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Albert ARIBAUD
Sent: 17-Jul-15 5:20 PM

Hello Sylvain,

Quoted 1 line · slemieux.tyco at gmail.com

....

Quoted 5 lines

Quoted 4 lines

* What about functional support in SPL? Is it correct, that if I want to have this code in SPL, then I have to pull in DMA driver as a mandatory dependency to tiny SPL?

Quoted 9 lines

You have to ensure DMA, but whether you pull the whole BSP driver or hard-code the necessary parts into a single self-contained SLC NAND + DMA file is a design decision. That said, I would personally go for pulling the driver *and* adding preprocessing out any part of it not necessary for SPL -- not so much for size (the compiler and linker should optimize useless parts away on their own anyway) than for clarity and maintenance (readers of the driver code would know which part is needed for SPL and which is not).

I am in the process of putting a patch together to add the hardware ECC support on top of the SLC NAND driver patch
(https://patchwork.ozlabs.org/patch/497308/).

A have a few questions:
1) The LPC32xx SLC NAND driver is using a custom NAND ECC Layout for small page; You can refer to the Kernel driver:
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/drivers/mtd/nand/lpc32xx_slc.c?id=refs/tags/v4.1.3

Quoted 2 lines

Are you planning to update your driver with the change or should I submit a separate patch for it before adding the support for DMA and hardware ECC?

if you don't object to rebase your changes on top of mine (see my closing comment), I'm fine, if custom NAND ECC Layout for small page chip is added in a separate patch. For your information on my side I won't be able to test it though.

Quoted 3 lines

2) As suggested by Albert, I will add a conditional compile option to ensure the original version of the driver (no DMA) can be used for SPL.

Great, thank you.

Quoted 2 lines

I was planning to do it using the configuration option use to select the LPC32xx DMA driver (CONFIG_DMA_LPC32XX). Are you OK with this approach?

I'm not sure, if DMA is really needed in SPL binary, but if you want to add this option, I don't object. To separate code for SPL and normal image please use CONFIG_SPL_BUILD macro.

Quoted 1 line

3) Should I separate the NAND SLC /DMA & the USB into 2 separate patch?

I would say DMA driver as a prerequisite should go first, then in arbitrary order NAND SLC updates specific to available/compiled DMA and USB changes.

Quoted 68 lines

Test result (full command log below):

Clock configuration:
CPU clock: 266MHz / AHB bus clock: 133MHz / Peripheral clock: 13MHz

1) NAND SLC driver: https://patchwork.ozlabs.org/patch/497308/
NAND read: 51904512 bytes
read.raw: 1.444 MiB per second / read: 0.625 MiB per second

2) NAND SLC driver: https://patchwork.ozlabs.org/patch/497308/
& Legacy BSP porting (hardware ECC & DMA)
NAND read: 51904512 bytes
read.raw: 2.272 MiB per second / read: 2.139 MiB per second

------------------
Full log:

NAND SLC driver: https://patchwork.ozlabs.org/patch/497308/

==> gettime; nand read.raw 0x80000000 0xd00000 0x6000; gettime Timer val: 259607
Seconds : 259
Remainder : 607
sys_hz = 1000

NAND read: 51904512 bytes read: OK
Timer val: 293885
Seconds : 293
Remainder : 885
sys_hz = 1000
--> 1.444 MiB per second

==> gettime; nand read.e 0x80000000 0xd00000 0x3180000; gettime Timer val: 11304
Seconds : 11
Remainder : 304
sys_hz = 1000

NAND read: device 0 offset 0xd00000, size 0x3180000
51904512 bytes read: OK
Timer val: 90495
Seconds : 90
Remainder : 495
sys_hz = 1000
--> 0.625 MiB per second

NAND SLC driver: https://patchwork.ozlabs.org/patch/497308/
& Legacy BSP porting (hardware ECC & DMA)

==> gettime; nand read.raw 0x80000000 0xd00000 0x6000; gettime Timer val: 49705
Seconds : 49
Remainder : 705
sys_hz = 1000

NAND read: 51904512 bytes read: OK
Timer val: 71483
Seconds : 71
Remainder : 483
sys_hz = 1000
--> 2.272 MiB per second

Timer val: 280282
Seconds : 280
Remainder : 282
sys_hz = 1000

Quoted 8 lines

NAND read: device 0 offset 0xd00000, size 0x3180000
51904512 bytes read: OK
Timer val: 303423
Seconds : 303
Remainder : 423
sys_hz = 1000
--> 2.139 MiB per second

Thank you for tests. So, as expected if DMA and hardware ECC is enabled, then the performance is 1.5 (raw read) or 3.5 times (read & ecc) better.

I do recognize and appreciate this work, advocating my change I can repeat my last arguments:
* it has a ready valid user in U-boot, my change is not a dead code,
* it is more functional -- read from NAND in SPL is supported,
* it is extremely simple (180 LoC vs. 500 LoC + ~200 LoC of DMA driver),
* it has been reviewed by Scott, all review comments are fixed and published in v4, and it is ready for inclusion to the next release IMHO.

If there is no more review comments to my version of the driver, I would kindly ask you to rebase legacy SLC NAND driver on top of my version of the driver, it is a doable and simple task in my opinion, performance optimization with the associated code complexity may be added later on.

--
With best wishes,
Vladimir