Re: [PATCH v2] mtd: rawnand: marvell: fix the chip-select DT parsing logic

[Boris Brezillon <boris.brezillon@bootlin.com>]

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

> 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@vger.kernel.org
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Queued to master.

Thanks,

Boris

> ---
> 
> Initially in three different patches, these changes touch the same
> section of code and are linked to each other, so they have been squashed
> for being queued in the fixes branch.
> 
>  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 */