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

[PATCH 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>
---
 drivers/mtd/nand/raw/marvell_nand.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
index ba6889bbe802..a50ea47baa4f 100644
--- a/drivers/mtd/nand/raw/marvell_nand.c
+++ b/drivers/mtd/nand/raw/marvell_nand.c
@@ -5,6 +5,37 @@
  * 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 almost all the Armada SoCs.
+ *
+ * The main differences are that the NFCv1 has DMA support and only
+ * has Hamming ECC capabilities, while NFCv2 does not support DMA but
+ * has hardware BCH support.
+ *
+ * The internal ECC operations are depicted in details in Marvell
+ * AN-379.
+ *
+ * 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 makes use of 'naked' operations.
+ *   - The ECC strength in BCH mode cannot be tuned easily. It is a
+ *     fixed 16 bytes. What can be tuned is the area on which this
+ *     correction occurs. Hence, using 2048B ECC chunks makes the
+ *     strength to be 4b/512B.
+ *   - The controller will always treat data bytes, spare bytes and
+ *     ECC bytes in that order, no matter the real factory layout
+ *     (which is usually all data then all OOB bytes). But depending
+ *     on the chosen layout, the areas of each section may vary or be
+ *     absent. The same data/spare/ecc layout is repeated until the
+ *     next chunk were each section may be different again. 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. In this case, the NAND_BBT_NO_OOB_BBM option
+ *     must be set.
  */
 
 #include <linux/module.h>
-- 
2.14.1

[PATCH 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>
---
 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 2/2] Documentation: mtd: remove stale pxa3xx NAND controller documentation

[Thomas Petazzoni]

Hello,

On Sat, 14 Jul 2018 15:54:28 +0200, Miquel Raynal 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>

I agree with moving the documentation to the driver itself, but the
documentation you've added in PATCH 1/2 into the driver is much, much
more limited than the documentation present in this file.

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Miquel Raynal]

Hi Thomas,

Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote on Sat, 14 Jul
2018 22:46:12 +0200:

> Hello,
> 
> On Sat, 14 Jul 2018 15:54:28 +0200, Miquel Raynal 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>  
> 
> I agree with moving the documentation to the driver itself, but the
> documentation you've added in PATCH 1/2 into the driver is much, much
> more limited than the documentation present in this file.

I think you are right on the fact that showing how data/spare/ecc bytes
might be exposed by the controller is something useful.

I will add the example given in the doc.

Thanks,
Miquèl

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

[Thomas Petazzoni]

Hello,

On Sat, 14 Jul 2018 15:54:27 +0200, Miquel Raynal wrote:

> + * The internal ECC operations are depicted in details in Marvell
> + * AN-379.

AN-379 is as far as I know not publicly available, so I'm not sure if it
makes a lot of sense to mention it here.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Miquel Raynal]

Hi Thomas,

Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote on Sat, 14 Jul
2018 22:46:48 +0200:

> Hello,
> 
> On Sat, 14 Jul 2018 15:54:27 +0200, Miquel Raynal wrote:
> 
> > + * The internal ECC operations are depicted in details in Marvell
> > + * AN-379.  
> 
> AN-379 is as far as I know not publicly available, so I'm not sure if it
> makes a lot of sense to mention it here.

No it's not, but I think it's not the first time this AN is quoted in
the ML and it might be useful to know about it for people having access
to Marvell specifications. At least in this file all layouts are
explained in details.

Thanks,
Miquèl

Re: [PATCH v4 12/14] arm64: dts: marvell: add AP806 SEI subnode

[Miquel Raynal]

Hi Rob,

Rob Herring <robh@kernel.org> wrote on Mon, 16 Jul 2018 09:31:58 -0600:

> On Thu, Jul 05, 2018 at 02:40:09PM +0200, Miquel Raynal wrote:
> > Add the System Error Interrupt node, representing an IRQ chip which is
> > part of the GIC. The SEI node has two subnodes, one for each interrupt
> > domain: wired (from the AP) and not-wired (MSIs from the CPs).  
> 
> Where are the 2 sub-nodes?

Indeed I did not update the commit log.

[...]

> > +			sei: interrupt-controller@3f0200 {
> > +				compatible = "marvell,armada-8k-sei";
> > +				reg = <0x3f0200 0x40>;
> > +				interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
> > +				marvell,sei-ap-ranges = <0 21>;
> > +				marvell,sei-cp-ranges = <21 43>;

After more discussion, these ranges seems to be only related to the
IP internal organization itself and should not appear in the device
tree at all. Instead, I could probably have a more meaningful
compatible string, like "marvell,ap806-sei". Next time an AP has a
different internal distribution, we'll just add a different compatible
and handle the differences in the driver directly.

This does not have a big impact on the rest of the driver, I should
probably let Marc review this version first.

Thanks,
Miquèl

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

[Boris Brezillon]

On Sat, 14 Jul 2018 15:54:27 +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>
> ---
>  drivers/mtd/nand/raw/marvell_nand.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
> index ba6889bbe802..a50ea47baa4f 100644
> --- a/drivers/mtd/nand/raw/marvell_nand.c
> +++ b/drivers/mtd/nand/raw/marvell_nand.c
> @@ -5,6 +5,37 @@
>   * 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 almost all the Armada SoCs.
> + *
> + * The main differences are that the NFCv1 has DMA support and only
> + * has Hamming ECC capabilities, while NFCv2 does not support DMA but
> + * has hardware BCH support.
> + *
> + * The internal ECC operations are depicted in details in Marvell
> + * AN-379.
> + *
> + * 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 makes use of 'naked' operations.
> + *   - The ECC strength in BCH mode cannot be tuned easily. It is a
> + *     fixed 16 bytes. What can be tuned is the area on which this

		   ^ bits

> + *     correction occurs. Hence, using 2048B ECC chunks makes the
> + *     strength to be 4b/512B.

I'd mention that max ECC step size is 2k here. So you can actually
choose something between 512 and 2k based on the chip requirements.

> + *   - The controller will always treat data bytes, spare bytes and

Some people call OOB bytes, spare bytes, what you refer to here are
free OOB bytes. I think it's worth clarifying that somewhere.

> + *     ECC bytes in that order, no matter the real factory layout
> + *     (which is usually all data then all OOB bytes). But depending
> + *     on the chosen layout, the areas of each section may vary or be
> + *     absent. The same data/spare/ecc layout is repeated until the
> + *     next chunk were each section may be different again. 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. In this case, the NAND_BBT_NO_OOB_BBM option
> + *     must be set.
>   */
>  
>  #include <linux/module.h>