[PATCH 0/3] Changes in Marvell NAND controller DT parsing code

[PATCH 0/3] Changes in Marvell NAND controller DT parsing code

[Miquel Raynal]

Hello,

At first I just wanted to send a fix to avoid timeouts at boot time when
using old bindings (harmless):

[    1.367460] marvell-nfc f2720000.nand: Timeout on CMDD (NDSR: 0x00000080)
[    1.474292] marvell-nfc f2720000.nand: Timeout on CMDD (NDSR: 0x00000280)

This is addressed in the first patch and comes from the DT parsing of
the number of Chip Select lines.

While working on it, Boris pointed another issue with the 'num-cs'
property (legacy bindings) in some code that should not exist anyway. I
removed that code in patch 2.

Finally, while writing the second patch, I discovered a better way to handle
the code besides (that handles the CS count for new bindings) by using
of_property_count_elems_of_size(). Patch 3 makes the switch.

Regards,
Miquèl


Miquel Raynal (3):
  mtd: rawnand: marvell: fix the chip-select DT parsing logic
  mtd: rawnand: marvell: fix CS pin count with old bindings
  mtd: rawnand: marvell: use OF helper to read a property element count

 drivers/mtd/nand/raw/marvell_nand.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

-- 
2.14.1

[PATCH 1/3] mtd: rawnand: marvell: fix the chip-select DT parsing logic

[Miquel Raynal]

The block responsible of parsing the DT for the number of chip-select
lines uses an 'if/else if/else if' block. The content of the second and
third 'else if' conditions are:
        1/ the actual condition to enter the sub-block and
        2/ the operation to do in this sub-block.

        [...]
        else if (condition1_to_enter && action1() == failed)
                raise_error();
        else if (condition2_to_enter && action2() == failed)
                raise_error();
        [...]

In case of failure, the sub-block is entered and an error raised.
Otherwise, in case of success, the code would continue erroneously in
the next 'else if' statement because it did not failed (and did not
enter the first 'else if' sub-block).

The first 'else if' refers to legacy bindings while the second 'else if'
refers to new bindings. The second 'else if', which is entered
erroneously, checks for the 'reg' property, which, for old bindings,
does not mean anything because it would not be the number of CS
available, but the regular register map of almost any DT node. This
being said, the content of the 'reg' property being the register map
offset and length, it has '2' values, so the number of CS in this
situation is assumed to be '2'.

When running nand_scan_ident() with 2 CS, the core will check for an
array of chips. It will first issue a RESET and then a READ_ID. Of
course this will trigger two timeouts because there is no chip in front
of the second CS:

[    1.367460] marvell-nfc f2720000.nand: Timeout on CMDD (NDSR: 0x00000080)
[    1.474292] marvell-nfc f2720000.nand: Timeout on CMDD (NDSR: 0x00000280)

Indeed, this is harmless and the core will then assume there is only one
valid CS.

Fix the logic in the whole block by entering each sub-block just on the
'is legacy' condition, doing the action inside the sub-block. This way,
when the action succeeds, the whole block is left.

Fixes: 02f26ecf8c772 ("mtd: nand: add reworked Marvell NAND controller driver")
Cc: stable@vger.kernel.org
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/marvell_nand.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
index 10e953218948..2a467c72bbfb 100644
--- a/drivers/mtd/nand/raw/marvell_nand.c
+++ b/drivers/mtd/nand/raw/marvell_nand.c
@@ -2308,13 +2308,16 @@ static int marvell_nand_chip_init(struct device *dev, struct marvell_nfc *nfc,
 	 */
 	if (pdata) {
 		nsels = 1;
-	} else if (nfc->caps->legacy_of_bindings &&
-		   !of_get_property(np, "num-cs", &nsels)) {
-		dev_err(dev, "missing num-cs property\n");
-		return -EINVAL;
-	} else if (!of_get_property(np, "reg", &nsels)) {
-		dev_err(dev, "missing reg property\n");
-		return -EINVAL;
+	} else if (nfc->caps->legacy_of_bindings) {
+		if (!of_get_property(np, "num-cs", &nsels)) {
+			dev_err(dev, "missing num-cs property\n");
+			return -EINVAL;
+		}
+	} else {
+		if (!of_get_property(np, "reg", &nsels)) {
+			dev_err(dev, "missing reg property\n");
+			return -EINVAL;
+		}
 	}
 
 	if (!pdata)
-- 
2.14.1

[PATCH 2/3] mtd: rawnand: marvell: fix CS pin count with old bindings

[Miquel Raynal]

For both the old bindings and the new bindings the same logic was
applied to retrieve the number of CS lines: using of_get_property() to
get a size in bytes, converted in the actual number of lines by dividing
it per sizeof(u32) (4 bytes).

This is fine for the 'reg' property which is a list of the CS IDs but
not for the 'num-cs' property which is directly the value of the number
of CS.

Anyway, no existing DT uses another value than 'num-cs = <1>' and no
other value has ever been supported by the old driver (pxa3xx_nand.c).
Remove this condition and apply a number of 1 CS anyway, as already
described in the bindings.

Fixes: 02f26ecf8c772 ("mtd: nand: add reworked Marvell NAND controller driver")
Cc: stable@vger.kernel.org
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/marvell_nand.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
index 2a467c72bbfb..c33ebc2cc024 100644
--- a/drivers/mtd/nand/raw/marvell_nand.c
+++ b/drivers/mtd/nand/raw/marvell_nand.c
@@ -2299,29 +2299,23 @@ static int marvell_nand_chip_init(struct device *dev, struct marvell_nfc *nfc,
 	/*
 	 * The legacy "num-cs" property indicates the number of CS on the only
 	 * chip connected to the controller (legacy bindings does not support
-	 * more than one chip). CS are only incremented one by one while the RB
-	 * pin is always the #0.
+	 * more than one chip). The CS and RB pins are always the #0.
 	 *
 	 * When not using legacy bindings, a couple of "reg" and "nand-rb"
 	 * properties must be filled. For each chip, expressed as a subnode,
 	 * "reg" points to the CS lines and "nand-rb" to the RB line.
 	 */
-	if (pdata) {
+	if (pdata || nfc->caps->legacy_of_bindings) {
 		nsels = 1;
-	} else if (nfc->caps->legacy_of_bindings) {
-		if (!of_get_property(np, "num-cs", &nsels)) {
-			dev_err(dev, "missing num-cs property\n");
-			return -EINVAL;
-		}
 	} else {
 		if (!of_get_property(np, "reg", &nsels)) {
 			dev_err(dev, "missing reg property\n");
 			return -EINVAL;
 		}
-	}
 
-	if (!pdata)
 		nsels /= sizeof(u32);
+	}
+
 	if (!nsels) {
 		dev_err(dev, "invalid reg property size\n");
 		return -EINVAL;
-- 
2.14.1

[PATCH 3/3] mtd: rawnand: marvell: use OF helper to read a property element count

[Miquel Raynal]

The 'reg' property of a 'nand' node (with the new bindings) gives the
IDs of each CS line in use. marvell_nand.c driver first look at the
number of CS lines that are present in this property.

Better use of_property_count_elems_of_size() than dividing by 4 the size
of the number of bytes returned by of_get_property().

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/marvell_nand.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
index c33ebc2cc024..1d779a35ac8e 100644
--- a/drivers/mtd/nand/raw/marvell_nand.c
+++ b/drivers/mtd/nand/raw/marvell_nand.c
@@ -2308,17 +2308,11 @@ static int marvell_nand_chip_init(struct device *dev, struct marvell_nfc *nfc,
 	if (pdata || nfc->caps->legacy_of_bindings) {
 		nsels = 1;
 	} else {
-		if (!of_get_property(np, "reg", &nsels)) {
-			dev_err(dev, "missing reg property\n");
+		nsels = of_property_count_elems_of_size(np, "reg", sizeof(u32));
+		if (nsels <= 0) {
+			dev_err(dev, "missing/invalid reg property\n");
 			return -EINVAL;
 		}
-
-		nsels /= sizeof(u32);
-	}
-
-	if (!nsels) {
-		dev_err(dev, "invalid reg property size\n");
-		return -EINVAL;
 	}
 
 	/* Alloc the nand chip structure */
-- 
2.14.1

Re: [PATCH 0/3] Changes in Marvell NAND controller DT parsing code

[Boris Brezillon]

Hi Miquel,

On Wed, 25 Apr 2018 14:53:28 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hello,
> 
> At first I just wanted to send a fix to avoid timeouts at boot time when
> using old bindings (harmless):
> 
> [    1.367460] marvell-nfc f2720000.nand: Timeout on CMDD (NDSR: 0x00000080)
> [    1.474292] marvell-nfc f2720000.nand: Timeout on CMDD (NDSR: 0x00000280)
> 
> This is addressed in the first patch and comes from the DT parsing of
> the number of Chip Select lines.
> 
> While working on it, Boris pointed another issue with the 'num-cs'
> property (legacy bindings) in some code that should not exist anyway. I
> removed that code in patch 2.
> 
> Finally, while writing the second patch, I discovered a better way to handle
> the code besides (that handles the CS count for new bindings) by using
> of_property_count_elems_of_size(). Patch 3 makes the switch.
> 
> Regards,
> Miquèl
> 
> 
> Miquel Raynal (3):
>   mtd: rawnand: marvell: fix the chip-select DT parsing logic
>   mtd: rawnand: marvell: fix CS pin count with old bindings
>   mtd: rawnand: marvell: use OF helper to read a property element count

I usually ask people to properly split their changes, but this time I'd
prefer to have everything in a single patch so that I can queue it to
the fixes branch.

Regards,

Boris

Re: [PATCH 0/3] Changes in Marvell NAND controller DT parsing code

[Miquel Raynal]

Hi Boris,

On Wed, 25 Apr 2018 15:20:07 +0200, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:

> Hi Miquel,
> 
> On Wed, 25 Apr 2018 14:53:28 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hello,
> > 
> > At first I just wanted to send a fix to avoid timeouts at boot time when
> > using old bindings (harmless):
> > 
> > [    1.367460] marvell-nfc f2720000.nand: Timeout on CMDD (NDSR: 0x00000080)
> > [    1.474292] marvell-nfc f2720000.nand: Timeout on CMDD (NDSR: 0x00000280)
> > 
> > This is addressed in the first patch and comes from the DT parsing of
> > the number of Chip Select lines.
> > 
> > While working on it, Boris pointed another issue with the 'num-cs'
> > property (legacy bindings) in some code that should not exist anyway. I
> > removed that code in patch 2.
> > 
> > Finally, while writing the second patch, I discovered a better way to handle
> > the code besides (that handles the CS count for new bindings) by using
> > of_property_count_elems_of_size(). Patch 3 makes the switch.
> > 
> > Regards,
> > Miquèl
> > 
> > 
> > Miquel Raynal (3):
> >   mtd: rawnand: marvell: fix the chip-select DT parsing logic
> >   mtd: rawnand: marvell: fix CS pin count with old bindings
> >   mtd: rawnand: marvell: use OF helper to read a property element count  
> 
> I usually ask people to properly split their changes, but this time I'd
> prefer to have everything in a single patch so that I can queue it to
> the fixes branch.

That's sad :) but sure, I'll squash all of that.

Thanks,
Miquèl

> 
> Regards,
> 
> Boris
> 



-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com