[PATCH v4 1/2] mtd: rawnand: marvell: document a bit more the driver

[PATCH v4 1/2] mtd: rawnand: marvell: document a bit more the driver

[Miquel Raynal]

A stale document about the old pxa3cc_nand.c driver is available in
Documentation/mtd/nand/. Rewrite the parts that explain the IP itself
and some non-trivial choices made in the driver directly in
marvell_nand.c to then be able to remove this file.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---

Changes since v2:
=================
* Addressed Boris comments, fixed (new) typos, explained what spare
  bytes and naked operations are.

Changes since v2:
=================
* Addressed Boris comments, mostly typos and rewritting as suggested.

Changes since v1:
=================
* Corrected mistakes pointed by Boris.
* Enlarged the documentation as suggested by Thomas to mention the
  layouts as seen per the controller.
* Added a mention about the maximum ECC size which is 2kiB.


 drivers/mtd/nand/raw/marvell_nand.c | 67 +++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
index 218e09431d3d..53ebd5cb3e6e 100644
--- a/drivers/mtd/nand/raw/marvell_nand.c
+++ b/drivers/mtd/nand/raw/marvell_nand.c
@@ -5,6 +5,73 @@
  * Copyright (C) 2017 Marvell
  * Author: Miquel RAYNAL <miquel.raynal@free-electrons.com>
  *
+ *
+ * This NAND controller driver handles two versions of the hardware,
+ * one is called NFCv1 and is available on PXA SoCs and the other is
+ * called NFCv2 and is available on Armada SoCs.
+ *
+ * The main visible difference is that NFCv1 only has Hamming ECC
+ * capabilities, while NFCv2 also embeds a BCH ECC engine. Also, DMA
+ * is not used with NFCv2.
+ *
+ * The ECC layouts are depicted in details in Marvell AN-379, but here
+ * is a brief description.
+ *
+ * When using Hamming, the data is split in 512B chunks (either 1, 2
+ * or 4) and each chunk will have its own ECC "digest" of 6B at the
+ * beginning of the OOB area and eventually the remaining free OOB
+ * bytes (also called "spare" bytes in the driver). This engine
+ * corrects up to 1 bit per chunk and detects reliably an error if
+ * there are at most 2 bitflips. Here is the page layout used by the
+ * controller when Hamming is chosen:
+ *
+ * +-------------------------------------------------------------+
+ * | Data 1 | ... | Data N | ECC 1 | ... | ECCN | Free OOB bytes |
+ * +-------------------------------------------------------------+
+ *
+ * When using the BCH engine, there are N identical (data + free OOB +
+ * ECC) sections and potentially an extra one to deal with
+ * configurations where the chosen (data + free OOB + ECC) sizes do
+ * not align with the page (data + OOB) size. ECC bytes are always
+ * 30B per ECC chunk. Here is the page layout used by the controller
+ * when BCH is chosen:
+ *
+ * +-----------------------------------------
+ * | Data 1 | Free OOB bytes 1 | ECC 1 | ...
+ * +-----------------------------------------
+ *
+ *      -------------------------------------------
+ *       ... | Data N | Free OOB bytes N | ECC N |
+ *      -------------------------------------------
+ *
+ *           --------------------------------------------+
+ *            Last Data | Last Free OOB bytes | Last ECC |
+ *           --------------------------------------------+
+ *
+ * In both cases, the layout seen by the user is always: all data
+ * first, then all free OOB bytes and finally all ECC bytes. With BCH,
+ * ECC bytes are 30B long and are padded with 0xFF to align on 32
+ * bytes.
+ *
+ * The controller has certain limitations that are handled by the
+ * driver:
+ *   - It can only read 2k at a time. To overcome this limitation, the
+ *     driver issues data cycles on the bus, without issuing new
+ *     CMD + ADDR cycles. The Marvell term is "naked" operations.
+ *   - The ECC strength in BCH mode cannot be tuned. It is fixed 16
+ *     bits. What can be tuned is the ECC block size as long as it
+ *     stays between 512B and 2kiB. It's usually chosen based on the
+ *     chip ECC requirements. For instance, using 2kiB ECC chunks
+ *     provides 4b/512B correctability.
+ *   - The controller will always treat data bytes, free OOB bytes
+ *     and ECC bytes in that order, no matter what the real layout is
+ *     (which is usually all data then all OOB bytes). The
+ *     marvell_nfc_layouts array below contains the currently
+ *     supported layouts.
+ *   - Because of these weird layouts, the Bad Block Markers can be
+ *     located in data section. In this case, the NAND_BBT_NO_OOB_BBM
+ *     option must be set to prevent scanning/writing bad block
+ *     markers.
  */
 
 #include <linux/module.h>
-- 
2.14.1

[PATCH v4 2/2] Documentation: mtd: remove stale pxa3xx NAND controller documentation

[Miquel Raynal]

It is preferred to have the documentation about the drivers directly
embedded in the driver itself. Remove this file now that the most
important information from this file have been re-written in
marvell_nand.c.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
---

Changes since v1/v2/v3:
=======================
* None.

 Documentation/mtd/nand/pxa3xx-nand.txt | 113 ---------------------------------
 1 file changed, 113 deletions(-)
 delete mode 100644 Documentation/mtd/nand/pxa3xx-nand.txt

diff --git a/Documentation/mtd/nand/pxa3xx-nand.txt b/Documentation/mtd/nand/pxa3xx-nand.txt
deleted file mode 100644
index 1074cbc67ec6..000000000000
--- a/Documentation/mtd/nand/pxa3xx-nand.txt
+++ /dev/null
@@ -1,113 +0,0 @@
-
-About this document
-===================
-
-Some notes about Marvell's NAND controller available in PXA and Armada 370/XP
-SoC (aka NFCv1 and NFCv2), with an emphasis on the latter.
-
-NFCv2 controller background
-===========================
-
-The controller has a 2176 bytes FIFO buffer. Therefore, in order to support
-larger pages, I/O operations on 4 KiB and 8 KiB pages is done with a set of
-chunked transfers.
-
-For instance, if we choose a 2048 data chunk and set "BCH" ECC (see below)
-we'll have this layout in the pages:
-
-  ------------------------------------------------------------------------------
-  | 2048B data | 32B spare | 30B ECC || 2048B data | 32B spare | 30B ECC | ... |
-  ------------------------------------------------------------------------------
-
-The driver reads the data and spare portions independently and builds an internal
-buffer with this layout (in the 4 KiB page case):
-
-  ------------------------------------------
-  |     4096B data     |     64B spare     |
-  ------------------------------------------
-
-Also, for the READOOB command the driver disables the ECC and reads a 'spare + ECC'
-OOB, one per chunk read.
-
-  -------------------------------------------------------------------
-  |     4096B data     |  32B spare | 30B ECC | 32B spare | 30B ECC |
-  -------------------------------------------------------------------
-
-So, in order to achieve reading (for instance), we issue several READ0 commands
-(with some additional controller-specific magic) and read two chunks of 2080B
-(2048 data + 32 spare) each.
-The driver accommodates this data to expose the NAND core a contiguous buffer
-(4096 data + spare) or (4096 + spare + ECC + spare + ECC).
-
-ECC
-===
-
-The controller has built-in hardware ECC capabilities. In addition it is
-configurable between two modes: 1) Hamming, 2) BCH.
-
-Note that the actual BCH mode: BCH-4 or BCH-8 will depend on the way
-the controller is configured to transfer the data.
-
-In the BCH mode the ECC code will be calculated for each transferred chunk
-and expected to be located (when reading/programming) right after the spare
-bytes as the figure above shows.
-
-So, repeating the above scheme, a 2048B data chunk will be followed by 32B
-spare, and then the ECC controller will read/write the ECC code (30B in
-this case):
-
-  ------------------------------------
-  | 2048B data | 32B spare | 30B ECC |
-  ------------------------------------
-
-If the ECC mode is 'BCH' then the ECC is *always* 30 bytes long.
-If the ECC mode is 'Hamming' the ECC is 6 bytes long, for each 512B block.
-So in Hamming mode, a 2048B page will have a 24B ECC.
-
-Despite all of the above, the controller requires the driver to only read or
-write in multiples of 8-bytes, because the data buffer is 64-bits.
-
-OOB
-===
-
-Because of the above scheme, and because the "spare" OOB is really located in
-the middle of a page, spare OOB cannot be read or write independently of the
-data area. In other words, in order to read the OOB (aka READOOB), the entire
-page (aka READ0) has to be read.
-
-In the same sense, in order to write to the spare OOB the driver has to write
-an *entire* page.
-
-Factory bad blocks handling
-===========================
-
-Given the ECC BCH requires to layout the device's pages in a split
-data/OOB/data/OOB way, the controller has a view of the flash page that's
-different from the specified (aka the manufacturer's) view. In other words,
-
-Factory view:
-
-  -----------------------------------------------
-  |                    Data           |x  OOB   |
-  -----------------------------------------------
-
-Driver's view:
-
-  -----------------------------------------------
-  |      Data      | OOB |      Data   x  | OOB |
-  -----------------------------------------------
-
-It can be seen from the above, that the factory bad block marker must be
-searched within the 'data' region, and not in the usual OOB region.
-
-In addition, this means under regular usage the driver will write such
-position (since it belongs to the data region) and every used block is
-likely to be marked as bad.
-
-For this reason, marking the block as bad in the OOB is explicitly
-disabled by using the NAND_BBT_NO_OOB_BBM option in the driver. The rationale
-for this is that there's no point in marking a block as bad, because good
-blocks are also 'marked as bad' (in the OOB BBM sense) under normal usage.
-
-Instead, the driver relies on the bad block table alone, and should only perform
-the bad block scan on the very first time (when the device hasn't been used).
-- 
2.14.1

Re: [PATCH v4 2/2] Documentation: mtd: remove stale pxa3xx NAND controller documentation

[Robert Jarzmik]

Miquel Raynal <miquel.raynal@bootlin.com> writes:

> It is preferred to have the documentation about the drivers directly
> embedded in the driver itself.
Could you tell me where this statement comes from ?

Because so far I've always been told the opposite.

Cheers.

--
Robert

Re: [PATCH v4 2/2] Documentation: mtd: remove stale pxa3xx NAND controller documentation

[Boris Brezillon]

Hi Robert,

On Sun, 05 Aug 2018 21:06:48 +0200
Robert Jarzmik <robert.jarzmik@free.fr> wrote:

> Miquel Raynal <miquel.raynal@bootlin.com> writes:
> 
> > It is preferred to have the documentation about the drivers directly
> > embedded in the driver itself.  
> Could you tell me where this statement comes from ?
> 
> Because so far I've always been told the opposite.

I complained about that when I inadvertently discovered there was a doc
explaining how the marvell IP works in Documentation/mtd/nand/
(which happened after someone posted a patch series adding a new file
there). That's really something I'd expect to be described inside the
driver directly. I'm perfectly fine having framework documentation or
even SoC documentation placed in Documentation, but when it comes to
driver/IP documentation it's IMO better placed directly in the driver
source code, just because people usually don't look for driver-specific
doc in Documentation/.

A compromise would be to have everything documented in the driver
using overview doc comments [1], and then let sphynx parse this doc
header and generate the proper doc entry, but that's IMHO less important
to have such IP-specific information embedded in the kernel doc.

Regards,

Boris

[1]https://www.kernel.org/doc/html/v4.11/doc-guide/kernel-doc.html#overview-documentation-comments

Re: [PATCH v4 2/2] Documentation: mtd: remove stale pxa3xx NAND controller documentation

[Ezequiel Garcia]

On 5 August 2018 at 11:52, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> It is preferred to have the documentation about the drivers directly
> embedded in the driver itself. Remove this file now that the most
> important information from this file have been re-written in
> marvell_nand.c.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>

Makes sense.

Acked-by: Ezequiel Garcia <ezequiel@collabora.com>

Thanks,
Eze

> ---
>
> Changes since v1/v2/v3:
> =======================
> * None.
>
>  Documentation/mtd/nand/pxa3xx-nand.txt | 113 ---------------------------------
>  1 file changed, 113 deletions(-)
>  delete mode 100644 Documentation/mtd/nand/pxa3xx-nand.txt
>
> diff --git a/Documentation/mtd/nand/pxa3xx-nand.txt b/Documentation/mtd/nand/pxa3xx-nand.txt
> deleted file mode 100644
> index 1074cbc67ec6..000000000000
> --- a/Documentation/mtd/nand/pxa3xx-nand.txt
> +++ /dev/null
> @@ -1,113 +0,0 @@
> -
> -About this document
> -===================
> -
> -Some notes about Marvell's NAND controller available in PXA and Armada 370/XP
> -SoC (aka NFCv1 and NFCv2), with an emphasis on the latter.
> -
> -NFCv2 controller background
> -===========================
> -
> -The controller has a 2176 bytes FIFO buffer. Therefore, in order to support
> -larger pages, I/O operations on 4 KiB and 8 KiB pages is done with a set of
> -chunked transfers.
> -
> -For instance, if we choose a 2048 data chunk and set "BCH" ECC (see below)
> -we'll have this layout in the pages:
> -
> -  ------------------------------------------------------------------------------
> -  | 2048B data | 32B spare | 30B ECC || 2048B data | 32B spare | 30B ECC | ... |
> -  ------------------------------------------------------------------------------
> -
> -The driver reads the data and spare portions independently and builds an internal
> -buffer with this layout (in the 4 KiB page case):
> -
> -  ------------------------------------------
> -  |     4096B data     |     64B spare     |
> -  ------------------------------------------
> -
> -Also, for the READOOB command the driver disables the ECC and reads a 'spare + ECC'
> -OOB, one per chunk read.
> -
> -  -------------------------------------------------------------------
> -  |     4096B data     |  32B spare | 30B ECC | 32B spare | 30B ECC |
> -  -------------------------------------------------------------------
> -
> -So, in order to achieve reading (for instance), we issue several READ0 commands
> -(with some additional controller-specific magic) and read two chunks of 2080B
> -(2048 data + 32 spare) each.
> -The driver accommodates this data to expose the NAND core a contiguous buffer
> -(4096 data + spare) or (4096 + spare + ECC + spare + ECC).
> -
> -ECC
> -===
> -
> -The controller has built-in hardware ECC capabilities. In addition it is
> -configurable between two modes: 1) Hamming, 2) BCH.
> -
> -Note that the actual BCH mode: BCH-4 or BCH-8 will depend on the way
> -the controller is configured to transfer the data.
> -
> -In the BCH mode the ECC code will be calculated for each transferred chunk
> -and expected to be located (when reading/programming) right after the spare
> -bytes as the figure above shows.
> -
> -So, repeating the above scheme, a 2048B data chunk will be followed by 32B
> -spare, and then the ECC controller will read/write the ECC code (30B in
> -this case):
> -
> -  ------------------------------------
> -  | 2048B data | 32B spare | 30B ECC |
> -  ------------------------------------
> -
> -If the ECC mode is 'BCH' then the ECC is *always* 30 bytes long.
> -If the ECC mode is 'Hamming' the ECC is 6 bytes long, for each 512B block.
> -So in Hamming mode, a 2048B page will have a 24B ECC.
> -
> -Despite all of the above, the controller requires the driver to only read or
> -write in multiples of 8-bytes, because the data buffer is 64-bits.
> -
> -OOB
> -===
> -
> -Because of the above scheme, and because the "spare" OOB is really located in
> -the middle of a page, spare OOB cannot be read or write independently of the
> -data area. In other words, in order to read the OOB (aka READOOB), the entire
> -page (aka READ0) has to be read.
> -
> -In the same sense, in order to write to the spare OOB the driver has to write
> -an *entire* page.
> -
> -Factory bad blocks handling
> -===========================
> -
> -Given the ECC BCH requires to layout the device's pages in a split
> -data/OOB/data/OOB way, the controller has a view of the flash page that's
> -different from the specified (aka the manufacturer's) view. In other words,
> -
> -Factory view:
> -
> -  -----------------------------------------------
> -  |                    Data           |x  OOB   |
> -  -----------------------------------------------
> -
> -Driver's view:
> -
> -  -----------------------------------------------
> -  |      Data      | OOB |      Data   x  | OOB |
> -  -----------------------------------------------
> -
> -It can be seen from the above, that the factory bad block marker must be
> -searched within the 'data' region, and not in the usual OOB region.
> -
> -In addition, this means under regular usage the driver will write such
> -position (since it belongs to the data region) and every used block is
> -likely to be marked as bad.
> -
> -For this reason, marking the block as bad in the OOB is explicitly
> -disabled by using the NAND_BBT_NO_OOB_BBM option in the driver. The rationale
> -for this is that there's no point in marking a block as bad, because good
> -blocks are also 'marked as bad' (in the OOB BBM sense) under normal usage.
> -
> -Instead, the driver relies on the bad block table alone, and should only perform
> -the bad block scan on the very first time (when the device hasn't been used).
> --
> 2.14.1
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/



-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH v4 1/2] mtd: rawnand: marvell: document a bit more the driver

[Boris Brezillon]

On Sun,  5 Aug 2018 16:52:56 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> A stale document about the old pxa3cc_nand.c driver is available in
> Documentation/mtd/nand/. Rewrite the parts that explain the IP itself
> and some non-trivial choices made in the driver directly in
> marvell_nand.c to then be able to remove this file.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>

> ---
> 
> Changes since v2:
> =================
> * Addressed Boris comments, fixed (new) typos, explained what spare
>   bytes and naked operations are.
> 
> Changes since v2:
> =================
> * Addressed Boris comments, mostly typos and rewritting as suggested.
> 
> Changes since v1:
> =================
> * Corrected mistakes pointed by Boris.
> * Enlarged the documentation as suggested by Thomas to mention the
>   layouts as seen per the controller.
> * Added a mention about the maximum ECC size which is 2kiB.
> 
> 
>  drivers/mtd/nand/raw/marvell_nand.c | 67 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
> index 218e09431d3d..53ebd5cb3e6e 100644
> --- a/drivers/mtd/nand/raw/marvell_nand.c
> +++ b/drivers/mtd/nand/raw/marvell_nand.c
> @@ -5,6 +5,73 @@
>   * Copyright (C) 2017 Marvell
>   * Author: Miquel RAYNAL <miquel.raynal@free-electrons.com>
>   *
> + *
> + * This NAND controller driver handles two versions of the hardware,
> + * one is called NFCv1 and is available on PXA SoCs and the other is
> + * called NFCv2 and is available on Armada SoCs.
> + *
> + * The main visible difference is that NFCv1 only has Hamming ECC
> + * capabilities, while NFCv2 also embeds a BCH ECC engine. Also, DMA
> + * is not used with NFCv2.
> + *
> + * The ECC layouts are depicted in details in Marvell AN-379, but here
> + * is a brief description.
> + *
> + * When using Hamming, the data is split in 512B chunks (either 1, 2
> + * or 4) and each chunk will have its own ECC "digest" of 6B at the
> + * beginning of the OOB area and eventually the remaining free OOB
> + * bytes (also called "spare" bytes in the driver). This engine
> + * corrects up to 1 bit per chunk and detects reliably an error if
> + * there are at most 2 bitflips. Here is the page layout used by the
> + * controller when Hamming is chosen:
> + *
> + * +-------------------------------------------------------------+
> + * | Data 1 | ... | Data N | ECC 1 | ... | ECCN | Free OOB bytes |
> + * +-------------------------------------------------------------+
> + *
> + * When using the BCH engine, there are N identical (data + free OOB +
> + * ECC) sections and potentially an extra one to deal with
> + * configurations where the chosen (data + free OOB + ECC) sizes do
> + * not align with the page (data + OOB) size. ECC bytes are always
> + * 30B per ECC chunk. Here is the page layout used by the controller
> + * when BCH is chosen:
> + *
> + * +-----------------------------------------
> + * | Data 1 | Free OOB bytes 1 | ECC 1 | ...
> + * +-----------------------------------------
> + *
> + *      -------------------------------------------
> + *       ... | Data N | Free OOB bytes N | ECC N |
> + *      -------------------------------------------
> + *
> + *           --------------------------------------------+
> + *            Last Data | Last Free OOB bytes | Last ECC |
> + *           --------------------------------------------+
> + *
> + * In both cases, the layout seen by the user is always: all data
> + * first, then all free OOB bytes and finally all ECC bytes. With BCH,
> + * ECC bytes are 30B long and are padded with 0xFF to align on 32
> + * bytes.
> + *
> + * The controller has certain limitations that are handled by the
> + * driver:
> + *   - It can only read 2k at a time. To overcome this limitation, the
> + *     driver issues data cycles on the bus, without issuing new
> + *     CMD + ADDR cycles. The Marvell term is "naked" operations.
> + *   - The ECC strength in BCH mode cannot be tuned. It is fixed 16
> + *     bits. What can be tuned is the ECC block size as long as it
> + *     stays between 512B and 2kiB. It's usually chosen based on the
> + *     chip ECC requirements. For instance, using 2kiB ECC chunks
> + *     provides 4b/512B correctability.
> + *   - The controller will always treat data bytes, free OOB bytes
> + *     and ECC bytes in that order, no matter what the real layout is
> + *     (which is usually all data then all OOB bytes). The
> + *     marvell_nfc_layouts array below contains the currently
> + *     supported layouts.
> + *   - Because of these weird layouts, the Bad Block Markers can be
> + *     located in data section. In this case, the NAND_BBT_NO_OOB_BBM
> + *     option must be set to prevent scanning/writing bad block
> + *     markers.
>   */
>  
>  #include <linux/module.h>

Re: [PATCH v4 1/2] mtd: rawnand: marvell: document a bit more the driver

[Miquel Raynal]

Hello,

Miquel Raynal <miquel.raynal@bootlin.com> wrote on Sun,  5 Aug 2018
16:52:56 +0200:

> A stale document about the old pxa3cc_nand.c driver is available in
> Documentation/mtd/nand/. Rewrite the parts that explain the IP itself
> and some non-trivial choices made in the driver directly in
> marvell_nand.c to then be able to remove this file.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---

Series applied to nand/next.

Miquèl