RE: [PATCH 3/5] mtd: brcmnand: Optional DT flag to reset IPROC NAND controller

[Anup Patel]

Hi Brian,

> -----Original Message-----
> From: Brian Norris [mailto:computersforpeace@xxxxxxxxx]
> Sent: 13 October 2015 02:58
> To: Anup Patel
> Cc: Florian Fainelli; Scott Branden; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Rob
> Herring; Pawel Moll; Mark Rutland; Ian Campbell; Kumar Gala; Catalin Marinas;
> Will Deacon; David Woodhouse; Ray Jui; Pramod Kumar; Vikram Prakash;
> Sandeep Tripathy; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> linux-mtd@xxxxxxxxxxxxxxxxxxx; bcm-kernel-feedback-list; Rafal Milecki
> Subject: Re: [PATCH 3/5] mtd: brcmnand: Optional DT flag to reset IPROC NAND
> controller
> 
> Hi Anup,
> 
> On Wed, Oct 07, 2015 at 03:33:50AM +0000, Anup Patel wrote:
> > > -----Original Message-----
> > > From: Florian Fainelli [mailto:f.fainelli@xxxxxxxxx]
> > >
> > > On 06/10/15 15:25, Scott Branden wrote:
> > >
> > > Then instead of adding a "reset flag" to Device Tree, another
> > > approach could be to put the desired or currently configured
> > > exhaustive list of NAND timings in Device Tree, and based on that you could
> have this:
> > >
> > > - the NAND controller driver finds that these timings match the
> > > current configuration, you are good to go
> > >
> > > - the NAND controller drivers finds a difference in how current
> > > timings are configured vs. desired timings, and issues a controller
> > > reset, prior to applying new timing configuration
> >
> > To add to this ...
> >
> > The mechanism to reset is BRCM NAND controller is SOC specific so the
> > SoC independent BRCM NAND driver (i.e. brcmnand.c) does not know how
> > to reset the NAND controller.
> >
> > For iProc SoC family, the NAND controller reset is through IDM
> > register space which is only iomap'ed by iproc_nand.c.
> >
> > We might end-up having one more SoC specific callback which will be
> > Provided by iproc_nand.c to brcmnand.c.
> >
> > >
> > > - no timings are configured, reset the controller and use existing
> > > auto-detection capabilities like ONFI modes
> > >
> > > Typically you would put the desired timings instead of the currently
> > > configured timings though..
> >
> > Overall, it would good to support timing parameters through DT or ONFI
> > but for now have we can rely on reset and auto-devid configuration.
> 
> I don't want to support a DT property that is only used as a workaround for the
> right solution. That means the property may quickly become obsolete, yet we
> have to support it forever.
> 
> 
> > > >>     compatible = "brcm,iproc-nand-ns2", ...;
> > > >>
> > > > As described above - the option is not SoC specific.  It is system
> > > > specific.  In some systems we may wish to reset the NAND
> > > > controller in linux.  In some we may wish to rely on
> > > > initialization that has already been done to speed up boot times.
> > >
> > > It seems to me like having this property is fine as long as you are
> > > describing that the controller *needs* a reset to operate properly,
> > > it does not strike me as a particularly well suited property if its
> > > side effect and main usage is to keep or wipe-out existing NAND timings.
> >
> > IMHO, having SoC specific compatible string for NS2 is like saying
> > NAND controller on NS2 is different from other iProc SoCs whereas
> > Having optional DT flags for quirks/work-arounds (e.g. NAND controller
> > reset) is like saying NAND controller on NS2 same as other iProc SoCs
> > but some additional programming is required.
> 
> OK... so what is the reason that you have to reset the controller on NS2 and not
> Cygnus? Is it a SoC difference (i.e., compatible string)?
> Firmware/bootloader difference? So far, all statements have been non-specific,
> AFAICT.
> 

On NS2 SVK, we have 16bit NAND chip whereas on all Cygnus SVKs we mostly
have 8bit NAND chip.

The bootloader on NS2 touches NAND controller and configures it to 16bit mode.
When we reach BRCMNAND driver probing on NS2, the BRCMNAND controller is
already in 16bit mode so NAND READID command does not work. On Cygnus,
we mostly have 8bit NAND chip so BRCMNAND controller is always in 8bit mode
so we don't see any issue with NAND READID command.

We really don't require to reset BRCNNAND controller on NS2 to get NAND
READID command working. Instead, we can simply force 8bit mode before
we do nand_scan_ident() for each CS. This will be a much simpler fix for all
versions of BRCMNAND because NAND READID command will only work
in 8bit mode irrespective to BRCMNAND version (NAND controllers from
other vendors might also have similar issue with NAND READID command).

I will send a revised patchset which will fix brcmnand_init_cs() as-per above.

Best Regards,
Anup
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/