mtd: rawnand: marvell: fix the chip-select DT parsing logic

[Linux-MTD Mailing List]

Gitweb:     http://git.infradead.org/?p=mtd-2.6.git;a=commit;h=f6997bec6af43396ff530caee79e178d32774a49
Commit:     f6997bec6af43396ff530caee79e178d32774a49
Parent:     7b70eb14392a7cf505f9b358d06c33b5af73d1e7
Author:     Miquel Raynal <miquel.raynal at bootlin.com>
AuthorDate: Wed Apr 25 16:16:32 2018 +0200
Committer:  Boris Brezillon <boris.brezillon at bootlin.com>
CommitDate: Thu Apr 26 19:06:42 2018 +0200

    mtd: rawnand: marvell: fix the chip-select DT parsing logic
    
    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.
    
    Furthermore, 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.
    
    Finally, 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().
    
    Fixes: 02f26ecf8c772 ("mtd: nand: add reworked Marvell NAND controller driver")
    Cc: stable at vger.kernel.org
    Signed-off-by: Miquel Raynal <miquel.raynal at bootlin.com>
    Tested-by: Chris Packham <chris.packham at alliedtelesis.co.nz>
    Signed-off-by: Boris Brezillon <boris.brezillon at bootlin.com>
---
 drivers/mtd/nand/raw/marvell_nand.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c
index 10e953218948..1d779a35ac8e 100644
--- a/drivers/mtd/nand/raw/marvell_nand.c
+++ b/drivers/mtd/nand/raw/marvell_nand.c
@@ -2299,29 +2299,20 @@ 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 &&
-		   !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;
+	} else {
+		nsels = of_property_count_elems_of_size(np, "reg", sizeof(u32));
+		if (nsels <= 0) {
+			dev_err(dev, "missing/invalid reg property\n");
+			return -EINVAL;
+		}
 	}
 
 	/* Alloc the nand chip structure */