On Wed, 25 Apr 2018 16:16:32 +0200
Miquel Raynal <miquel.raynal@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
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 */