[PATCH v2 0/5] Assorted OMAP2 NAND clean-ups

[Gupta, Pekon]

Hi Ezequiel Garcia,

Sorry I'm bit out of my place.. so not able to sync often with mails..
However plz see my replies below..

[...]

> >
> > > Pekon, Brian: Do you think this solution might work for 8-bit and 16-bit
> > > devices?
> > >
> > I think NAND_BUSWIDTH_AUTO (without GPMC changes) would fail in
> > following scenarios..
> >
> > Case-1: configuring gpmc,device-width=1 from DT when using x16 device.
> 
> ... which is wrong. That's why we have a DT property to configure that.
> The GPMC *must* be properly configured.
> 
No, I think the main intention of using NAND_BUSWIDTH_AUTO is that
your controller should not depend on DT or any other user-input to detect
NAND bus-width, (whether its gpmc,device-width or anything else).


> > As your NAND driver is using NAND_BUSWIDTH_AUTO, it should
> > ignore this DT config, and based on ONFI params it should work as x16
> >
> 
> Hm.. I don't think so. The auto-stuff is just for the NAND driver, not
> for the memory controller. I don't know much about hardware, but in my
> mind
> I imagine them as different controllers.
> 
TI's GPMC hardware engine can do much more than supporting only
NAND devices, therefore there is an independent GPMC driver, and
dependent NAND driver. 
So when a NAND device is connected all GPMC driver does is initializes
GPMC hardware based on static DT bindings, and pass the control to
NAND driver. But there should be a mechanism to override these
static DT configurations done in GPMC driver by NAND driver.
(it is this piece which is missing today).


> > Case-2: configuring gpmc,device-width=2 from DT when using x8 device.
> 
> ... which is also wrong.
> 
> Once again, you're mis-configuring the GPMC. We cannot expect the NAND
> driver to work properly if the GPMC is not properly initialized, don't
> you think?
> 
> > Actually having NAND_BUSWIDTH_AUTO would require change in GPMC
> > driver also.. please refer my comments in..
> > http://lists.infradead.org/pipermail/linux-mtd/2013-October/049284.html
> >
> 
> Well, I think the approach should be different and much simpler: GPMC
> *must* be properly configured and then NAND can do ONFI detection
> starting in 8-bit and then switching to 16-bit if needed.
> 
> This is what this patch is doing: it _fixes_ the NAND driver ONFI detection,
> _provided_ the GPMC is well-prepared.
> 
> You seem to think that GPMC + NAND should be able to automagically detect
> the device and work, but I don't think that's even physically possible, for
> the reasons you have just exposed.
> 
> I think this fix is simple enough.
> 
No, I still think there should be a way to tell the user that there is a
mis-match between bus-width configured in DT, and that detected
by ONFI probe. If it was straight forward, this would have been already
accepted earlier :-)

http://lists.infradead.org/pipermail/linux-mtd/2013-October/049154.html

(read the comments from Brian by following above patch thread)

Problem is if there is a mis-match in your bus-width due to incorrect
GPMC DT binding configurations, still NAND probe and everything will
pass correctly, because nand_scan_ident() uses read_byte() which
returns same for both x8 and x16 devices.
You will find issues when you actually do page reads and writes. And
this is where you would see corrupted half-page data. And user
would be clueless on why he is seeing such erratic behavior.

This is why I added checks for mismatch of DT binding right during
nand probe(), in above mentioned patch. I could just remove calling
nand_scan_ident() second-time and it becomes your patch, but then
it would be just validating the DT setting, not pure auto-detection.


> 
> BTW: The GPMC code ignores the DT value in 'gpmc,device-width' and uses
> 'nand-bus-width' instead, but that's a different bug and a different fix :)
> 
I think you mean the opposie.. GPMC driver uses gpmc,device-width..
Refer: $KERNEL/arch/arm/mach-omap2/gpmc.c
@@gpmc_read_settings_dt(...)
	of_property_read_u32(np, "gpmc,device-width", &p->device_width);

'nand-bus-width' is not used anywhere instead..


with regards, pekon