Re: [PATCH v5 1/3] mtd: nand: gpio: Add DT property to automatically determine bus width

[Brian Norris <computersforpeace@gmail.com>]

+ Pekon, Ezequiel

Hi Alexander,

We're dealing with a similar issue in other drivers currently, and I
think it's worth straightening out the issue for all systems.

On Wed, Nov 13, 2013 at 03:58:02PM +0400, Alexander Shiyan wrote:
> This patch adds a property to automatically determine the NAND
> bus width by CFI/ONFI information from chip. This property works
> if the bus width is not specified explicitly.

This issue brings up a few questions in my mind, which are relevant to
device tree in general.

First of all, do we need a device tree property at all, for something
that is auto-detectable?

Related: is a device tree consumer (like Linux) supposed to be a
validator, or simply a best-effort? I'm considering the following case:
if Linux is allowed to auto-detect some property which also has a device
tree binding (e.g., "nand-bus-width", in
Documentation/devicetree/bindings/mtd/nand.txt), what should happen if
the binding happens to be incorrect? IOW, what if the device tree
specifies buswidth is x16, but Linux correctly detects it as x8?
Shouldn't we make the best effort to bring the hardware up, regardless
of what the device tree says?

So for something like this GPIO driver, I'm thinking that Linux should
just use NAND_BUSWIDTH_AUTO all the time [*]. But (as I'm hinting above)
that would allow DTB firmware implementors to be lazier and have a
technically-incorrect "nand-bus-width" or "bank-width" binding, since
they know it can reliably be detected and overridden by Linux.

[*] Except where resource_size(res) < 2, as Alexander already has in
this patch.

> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> ---
>  .../devicetree/bindings/mtd/gpio-control-nand.txt        |  3 +++
>  drivers/mtd/nand/gpio.c                                  | 16 ++++++++++++----
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
> index 36ef07d..fe4e960 100644
> --- a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt
> @@ -19,6 +19,9 @@ Optional properties:
>    defaults to 1 byte.
>  - chip-delay : chip dependent delay for transferring data from array to
>    read registers (tR).  If not present then a default of 20us is used.
> +- gpio-control-nand,bank-width-auto : Device bus width is determined
> +  automatically by CFI/ONFI information from chip if "bank-width"
> +  parameter is omitted (Boolean).

If we do resort to a new binding for auto-buswidth, it should be a
generic one that all NAND drivers can use. Maybe a separate boolean
"nand-bus-width-auto" and if it is present, then it overrules the
presence (or lack) of the "nand-bus-width" boolean property?
Or is it possible to extend "nand-bus-width" to allow the value of 0 to
mean automatic?

You may want to modify the of_get_nand_bus_width() helper (or add a new
one, of_nand_bus_width_auto()?) to drivers/of/of_mtd.c to assist with
this.

...BTW, it looks like we have a duplicate binding here: GPIO NAND
defines "bank-width" where generic NAND defines "nand-bus-width". Aren't
these essentially duplications? Can we support the generic binding in
gpio.c and discourage "bank-width"? Or is that just unnecessary effort?

>  - gpio-control-nand,io-sync-reg : A 64-bit physical address for a read
>    location used to guard against bus reordering with regards to accesses to
>    the GPIO's and the NAND flash data bus.  If present, then after changing

[...]

Brian