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

[Brian Norris <computersforpeace@gmail.com>]

On Tue, Nov 26, 2013 at 05:21:58PM -0800, Brian Norris wrote:
> + Pekon, Ezequiel

+ Pekon, Ezequiel for real this time! Sorry... Everything else intact
/Brian

> 
> 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