Hi Miquel,
On 26/04/18 02:16, Miquel Raynal 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>
This stops the timeout warnings on my platform. So for that part
Tested-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>