[v10,04/10] mtd: nand: omap: fix device scan: NAND_CMD_READID, NAND_CMD_RESET, CMD_CMD_PARAM use only x8 bus

As per comments below, NAND_CMD_RESET, NAND_CMD_READID, and NAND_CMD_PARAM would
work only in x8 mode.
    commit 64b37b2a63eb2f80b65c7185f0013f8ffc637ae3
    Author:     Matthieu CASTET <matthieu.castet@parrot.com>
    AuthorDate: 2012-11-06
    Note that nand_scan_ident send command (NAND_CMD_RESET, NAND_CMD_READID, NAND_CMD_PARAM), address and read data
    The ONFI specificication is not very clear for x16 device if high byte of address should be driven to 0,
    but according to [1] it should be ok to not drive it during autodetection.

    [1]
    3.3.2. Target Initialization

    [...]
    The Read ID and Read Parameter Page commands only use the lower 8-bits of the data bus.
    The host shall not issue commands that use a word data width on x16 devices until the host
    determines the device supports a 16-bit data bus width in the parameter page.

Thus this patch run nand_scan_ident() with driver configured as x8 device.
Once the NAND device is detected, and its ONFI params are read, the driver
is re-configured based on device-width as passed by DT bindinig 'nand-bus-width'

In-case there is a mis-match between the DT binding 'nand-bus-width' and actual
device-width detected during nand_get_flash_type() then probe returns failure.

All other low-level callback updates happen after the device detection.

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 drivers/mtd/nand/omap2.c | 45 +++++++++++++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 12 deletions(-)

Hi Pekon,

On Sat, Oct 19, 2013 at 02:14:08PM +0530, Pekon Gupta wrote:
> As per comments below, NAND_CMD_RESET, NAND_CMD_READID, and NAND_CMD_PARAM would
> work only in x8 mode.
>     commit 64b37b2a63eb2f80b65c7185f0013f8ffc637ae3
>     Author:     Matthieu CASTET <matthieu.castet@parrot.com>
>     AuthorDate: 2012-11-06
>     Note that nand_scan_ident send command (NAND_CMD_RESET, NAND_CMD_READID, NAND_CMD_PARAM), address and read data
>     The ONFI specificication is not very clear for x16 device if high byte of address should be driven to 0,
>     but according to [1] it should be ok to not drive it during autodetection.
> 
>     [1]
>     3.3.2. Target Initialization
> 
>     [...]
>     The Read ID and Read Parameter Page commands only use the lower 8-bits of the data bus.
>     The host shall not issue commands that use a word data width on x16 devices until the host
>     determines the device supports a 16-bit data bus width in the parameter page.
> 
> Thus this patch run nand_scan_ident() with driver configured as x8 device.

So are you saying that the driver currently doesn't work if you started
in x16 buswidth? Are you having problems with a particular setup? What
sort of devices are you testing?

Running nand_scan_ident() with x8 when the device is actualy x16 will
just cause nand_scan_ident() to abort with an error. It doesn't help you
with the fact that RESET/READID/PARAM need special 8-bit handling on x16
devices, so you're not solving the problem alluded to by Matthieu.

> Once the NAND device is detected, and its ONFI params are read, the driver
> is re-configured based on device-width as passed by DT bindinig 'nand-bus-width'
> 
> In-case there is a mis-match between the DT binding 'nand-bus-width' and actual
> device-width detected during nand_get_flash_type() then probe returns failure.
> 
> All other low-level callback updates happen after the device detection.
> 
> Signed-off-by: Pekon Gupta <pekon@ti.com>

What is your patch trying to solve? It seems like it's just a distortion
of the same code that existed previously. It tries running
nand_scan_ident() in one buswidth configuration, and then it tries the
other if it failed. You still aren't doing either of the options we
talked about previously. I'll repeat them:

(1) You specify the buswidth given by device-tree/platform-data; if this
    is incorrect, you fail

(2) You auto-configure using NAND_BUSWIDTH_AUTO, and you use that to
    validate the platform data; you just warn if the buswidth provided
    by device-tree/platform-data was incorrect

Am I sensing that you are having some implementation problem with one of
these? You really shouldn't need to run nand_scan_ident() twice.

Or perhaps the "solution" in this patch is just that you're moving
around the reassignment of the callback functions? If so, this is not
obvious... see below.

> ---
>  drivers/mtd/nand/omap2.c | 45 +++++++++++++++++++++++++++++++++------------
>  1 file changed, 33 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 5596368..d29edda 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -1856,7 +1856,6 @@ static int omap_nand_probe(struct platform_device *pdev)
>  	mtd->name		= dev_name(&pdev->dev);
>  	mtd->owner		= THIS_MODULE;
>  	nand_chip		= &info->nand;
> -	nand_chip->options	= pdata->devsize;
>  	nand_chip->options	|= NAND_SKIP_BBTSCAN;
>  #ifdef CONFIG_MTD_NAND_OMAP_BCH
>  	info->of_node		= pdata->of_node;
> @@ -1904,6 +1903,39 @@ static int omap_nand_probe(struct platform_device *pdev)
>  		nand_chip->chip_delay = 50;
>  	}
>  
> +	/* scan NAND device connected to chip controller */

Why is this comment separated from the comment below it? Either give a
newline between them (if they're really separate) or make it a single
comment block.

> +	/* configure driver in x8 mode to read ONFI parameter page, as
> +	 * NAND_CMD_READID & NAND_CMD_PARAM may not work in x16 mode */
> +	nand_chip->options &= ~NAND_BUSWIDTH_16;
> +	if (nand_scan_ident(mtd, 1, NULL)) {
> +		/* nand_scan_ident failed */
> +		if (pdata->devsize) {
> +			/* may be because of mis-match of device-width,
> +			 * platform data (DT binding) also says its x16 device
> +			 * So re-scan with proper device-width */
> +			nand_chip->options |= pdata->devsize;
> +			if (nand_scan_ident(mtd, 1, NULL)) {
> +				err = -ENXIO;
> +				goto out_release_mem_region;
> +			}
> +		} else {
> +			/* some genuine failure, because even platform-data
> +			 * (DT binding) says that bus-width is x8 */
> +			err = -ENXIO;
> +			goto out_release_mem_region;
> +		}
> +	} else {
> +		/* nand_scan_ident passed with x8 mode */
> +		if (pdata->devsize) {
> +			/* but platform-data (DT binding) say its x16 device */
> +			pr_err("%s: incorrect bus-width config\n", DRIVER_NAME);

You only print this message in one case (detect x8, but DT said x16) and
not the other (detect x16, but DT said x8). This is a result of your
unnecessarily complicated logic here.

> +			err = -EINVAL;
> +			err = -ENXIO;
> +			goto out_release_mem_region;
> +		}
> +	}
> +
> +	/* re-populate low-level callbacks based on xfer modes */

So am I to understand that the main purpose of this patch is not about
the detection of the buswidth type, but about the (re)assignment of the
callback functions? If so, you aren't making it very clear. (I wasn't
paying too much attention to the fact that this code block is being
moved across all the callback assignments.) The above code is just a
more verbose way of doing the same thing as the code you're replacing,
with an extra check to compare with the device-tree/platform-data.

>  	switch (pdata->xfer_type) {
>  	case NAND_OMAP_PREFETCH_POLLED:
>  		nand_chip->read_buf   = omap_read_buf_pref;
> @@ -2011,17 +2043,6 @@ static int omap_nand_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	/* DIP switches on some boards change between 8 and 16 bit
> -	 * bus widths for flash.  Try the other width if the first try fails.
> -	 */
> -	if (nand_scan_ident(mtd, 1, NULL)) {
> -		nand_chip->options ^= NAND_BUSWIDTH_16;
> -		if (nand_scan_ident(mtd, 1, NULL)) {
> -			err = -ENXIO;
> -			goto out_release_mem_region;
> -		}
> -	}
> -
>  	/* rom code layout */
>  	if (pdata->ecc_opt == OMAP_ECC_HAM1_CODE_HW) {
>  

Anyway, I'm just going to have to flat out NAK this patch for now.
Please rework the series without this patch and we can continue
discussion of this patch independently (for one, this thread does not
need to CC all of the device-tree folks).

Brian

Hi Brian,

> From: Brian Norris [mailto:computersforpeace@gmail.com]
> On Sat, Oct 19, 2013 at 02:14:08PM +0530, Pekon Gupta wrote:
[...]

> >
> > Thus this patch run nand_scan_ident() with driver configured as x8 device.
> 
> So are you saying that the driver currently doesn't work if you started
> in x16 buswidth? Are you having problems with a particular setup? What
> sort of devices are you testing?
> 
No, I'm saying that you cannot read ONFI params in x16 mode.
And, that is what was pointed out in following commit log also ..
(Reference to 3.3.2. Target Initialization: given above)
So, if I run nand_scan_ident() in x16 mode, my ONFI detection would 
fail for sure ..


> Running nand_scan_ident() with x8 when the device is actualy x16 will
> just cause nand_scan_ident() to abort with an error. It doesn't help you
> with the fact that RESET/READID/PARAM need special 8-bit handling on x16
> devices, so you're not solving the problem alluded to by Matthieu.
> 
Yes, absolutely agree.. 
The original code was calling nand_scan_ident() twice, without taking
into consideration whether the first nand_scan_ident() failed because
of bus-width mismatch or something else.

I'm also calling nand_scan_ident() twice, but only when the failure may
be due to bus-width mis-match. I'm just avoiding an extra call to
nand_scan_ident() if the failure was genuine.

If the device actually was x8 then nand_scan_ident() should not fail
for the first-time (in my patch), but if it still fails, then it's a genuine failure
_not_ because of bus-width mismatch.
I'm avoiding the call to nand_scan_ident() again .. 
(same is in my comments below..)

[...]

> What is your patch trying to solve? It seems like it's just a distortion
> of the same code that existed previously. It tries running
> nand_scan_ident() in one buswidth configuration, and then it tries the
> other if it failed. You still aren't doing either of the options we
> talked about previously. I'll repeat them:
> 
Absolutely.. probably you missed my replies in [PATCH v9 4/9]...


> (1) You specify the buswidth given by device-tree/platform-data; if this
>     is incorrect, you fail
> 
Absolutely this is what I'm doing.
But tell me how would you know the actual device-width if
nand_scan_ident()  fails 
(a) to probe ONFI params and 
- your device is  not in nand_ids[]
So get the actual device width, I call the first nand_scan_ident() in x8 mode.
so that ONFI params are read.
Now, if it nand_scan_ident() fails then it means actual device *may* be x16
So, I re-invoke nand_scan_ident() with chip->option |= pdata->devsize;


> (2) You auto-configure using NAND_BUSWIDTH_AUTO, and you use that to
>     validate the platform data; you just warn if the buswidth provided
>     by device-tree/platform-data was incorrect
> 
I have removed NAND_BUSWIDTH_AUTO implementation, as per feedbacks
This is <new> patch. May be you are confusing it with earlier version...
*please re-review*


> Am I sensing that you are having some implementation problem with one of
> these? You really shouldn't need to run nand_scan_ident() twice.
> 
> Or perhaps the "solution" in this patch is just that you're moving
> around the reassignment of the callback functions? If so, this is not
> obvious... see below.
> 
I'm repost my replies from [PATCH v9 4/9] in-case you missed...

------------------------------------
The GPMC driver will be touched by NAND_BUSWIDTH_AUTO change.
There are two set of configurations in GPMC controller..
(a) device based configurations:
  These are static configurations derived on DT bindings for each
  chip-select (like NAND signal timings, etc). These done on GPMC probe.
(b) ecc-scheme based configurations:
  These are dynamic configurations done in NAND driver in
  chip->ecc.hwctl() and are refreshed at each NAND accesss.

However, 'nand-bus-width' configuration is part of both (1) and (2).
Thus, both 'OMAP NAND driver' and 'GPMC driver' need to be
consistent in programming  'nand-bus-width' otherwise ecc-engine
would not work.

Thus, I'm proceeding with (1) DT based parsing of 'nand-bus-width'.
I have re-posted [PATCH v10 4/10] with this updates. However, please
take into account that some limitation of dual programming require
such probe. New patch also call nand_scan_ident() twice, but only
on certain pre-determined conditions, not in all failure.
Once I convert to NAND_BUSWIDTH_AUTO these should get clean,
 as I would update both GPMC and NAND driver for this.
(Till then this was most optimal solution I could think of)..
------------------------------------





> > ---
> >  drivers/mtd/nand/omap2.c | 45
> +++++++++++++++++++++++++++++++++------------
> >  1 file changed, 33 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> > index 5596368..d29edda 100644
> > --- a/drivers/mtd/nand/omap2.c
> > +++ b/drivers/mtd/nand/omap2.c
> > @@ -1856,7 +1856,6 @@ static int omap_nand_probe(struct
> platform_device *pdev)
> >  	mtd->name		= dev_name(&pdev->dev);
> >  	mtd->owner		= THIS_MODULE;
> >  	nand_chip		= &info->nand;
> > -	nand_chip->options	= pdata->devsize;
> >  	nand_chip->options	|= NAND_SKIP_BBTSCAN;

See there is no NAND_BUSWIDTH_AUTO here ....



> > +	/* re-populate low-level callbacks based on xfer modes */
> >  	switch (pdata->xfer_type) {
> >  	case NAND_OMAP_PREFETCH_POLLED:
> >  		nand_chip->read_buf   = omap_read_buf_pref;
> 
> So am I to understand that the main purpose of this patch is not about
> the detection of the buswidth type, but about the (re)assignment of the
> callback functions? If so, you aren't making it very clear. (I wasn't
> paying too much attention to the fact that this code block is being
> moved across all the callback assignments.) The above code is just a
> more verbose way of doing the same thing as the code you're replacing,
> with an extra check to compare with the device-tree/platform-data.
> 
Nope, this is just the comment clean-up.. Please drop it if it confuses you.
One main request please review this patch by locally including it.
May be then you can understand that it has *nothing* to do with 
(re)assignment of callbacks.. rather there is no re-assignment at all
in this patch..
See there is no change in the lines below this comment change
Probably this comment clean-up confused you..


> > @@ -2011,17 +2043,6 @@ static int omap_nand_probe(struct
> platform_device *pdev)
> >  		}
> >  	}
> >
> > -	/* DIP switches on some boards change between 8 and 16 bit
> > -	 * bus widths for flash.  Try the other width if the first try fails.
> > -	 */
> > -	if (nand_scan_ident(mtd, 1, NULL)) {
> > -		nand_chip->options ^= NAND_BUSWIDTH_16;
> > -		if (nand_scan_ident(mtd, 1, NULL)) {
> > -			err = -ENXIO;
> > -			goto out_release_mem_region;
> > -		}
> > -	}
> > -
> >  	/* rom code layout */
> >  	if (pdata->ecc_opt == OMAP_ECC_HAM1_CODE_HW) {
> >
> 
> Anyway, I'm just going to have to flat out NAK this patch for now.
> Please rework the series without this patch and we can continue
> discussion of this patch independently (for one, this thread does not
> need to CC all of the device-tree folks).
> 

I think there was some confusion here..
So, I hv explained my patch above. Request you to please re-review this. 

I'll take NAND_BUSWIDTH_AUTO implementation as a separate patch,
because it would require changes in GPMC driver as stated above.
And so it would be all together independent patch-set.

I would wait for your reply.. 


with regards, pekon

On 10/22/2013 10:07 PM, Gupta, Pekon wrote:
>> From: Brian Norris [mailto:computersforpeace@gmail.com]
>> On Sat, Oct 19, 2013 at 02:14:08PM +0530, Pekon Gupta wrote:
> [...]
> 
>>>
>>> Thus this patch run nand_scan_ident() with driver configured as x8 device.
>>
>> So are you saying that the driver currently doesn't work if you started
>> in x16 buswidth? Are you having problems with a particular setup? What
>> sort of devices are you testing?
>>
> No, I'm saying that you cannot read ONFI params in x16 mode.
> And, that is what was pointed out in following commit log also ..
> (Reference to 3.3.2. Target Initialization: given above)
> So, if I run nand_scan_ident() in x16 mode, my ONFI detection would 
> fail for sure ..

But you cannot just run nand_scan_ident() with !(chip->options &
NAND_BUSWIDTH_16) when your devices is x16. You need to solve the ONFI
detection problem while correctly specifying NAND_BUSWIDTH_16.

Since you didn't answer the other 2 questions there: are you testing any
x16 devices?

>> Running nand_scan_ident() with x8 when the device is actualy x16 will
>> just cause nand_scan_ident() to abort with an error. It doesn't help you
>> with the fact that RESET/READID/PARAM need special 8-bit handling on x16
>> devices, so you're not solving the problem alluded to by Matthieu.
>>
> Yes, absolutely agree.. 
> The original code was calling nand_scan_ident() twice, without taking
> into consideration whether the first nand_scan_ident() failed because
> of bus-width mismatch or something else.
> 
> I'm also calling nand_scan_ident() twice, but only when the failure may
> be due to bus-width mis-match. I'm just avoiding an extra call to
> nand_scan_ident() if the failure was genuine.

You NEVER need to call nand_scan_ident() twice for the same chip.
Period. I will reject any patch that retains this pattern. It is wrong,
and I seriously doubt the code does what you think it does when you do this.

I think nand_scan_ident() may have a weak point where it won't support
ONFI properly for x16 devices. I believe NAND_BUSWIDTH_AUTO was added to
help with this fact. (I don't have any x16 devices to test it.) But if
this is a problem for you, fix it. Don't work around it.

>> What is your patch trying to solve? It seems like it's just a distortion
>> of the same code that existed previously. It tries running
>> nand_scan_ident() in one buswidth configuration, and then it tries the
>> other if it failed. You still aren't doing either of the options we
>> talked about previously. I'll repeat them:
>>
> Absolutely.. probably you missed my replies in [PATCH v9 4/9]...

No, I did not. I just don't see how you think that your code matches the
options (1) or (2) that I described. Perhaps it's a failure in
communication. I will try to be absolutely clear.

>> (1) You specify the buswidth given by device-tree/platform-data; if this
>>     is incorrect, you fail
>>
> Absolutely this is what I'm doing.

No it isn't. You are ignoring the provided buswidth information and
UNCONDITIONALLY trying x8. If/when that fails, you then error out or
retry in x16 (depending on the DT/platform-data).

This is plain wrong.

nand_base is designed (and it's documented in the comments) that the
driver must set the buswidth correctly BEFORE calling nand_scan_ident().
You may not use nand_scan_ident() as a trial-and-error identification
function.

So, to properly do (1), you should only have something like this, just
like all the other NAND drivers:

  nand_chip->options = pdata->devsize & NAND_BUSWIDTH_16;

  ret = nand_scan_ident(...);
  if (ret) {
      // exit with error code...
  }

If there is a problem with this, then you have to fix your driver or
nand_scan_ident(). Perhaps you have to adjust your readbuf() or
cmdfunc() callbacks to do this. But do not add complicated workaround
logic in your driver.

> But tell me how would you know the actual device-width if
> nand_scan_ident()  fails 

If you are not using NAND_BUSWIDTH_AUTO, then you MUST know the correct
buswidth before calling nand_scan_ident(). If your
device-tree/platform-data is wrong, then fix that.

> (a) to probe ONFI params and 
> - your device is  not in nand_ids[]
> So get the actual device width, I call the first nand_scan_ident() in x8 mode.
> so that ONFI params are read.

You don't call nand_scan_ident() with !(chip->options &
NAND_BUSWIDTH_16) when you have an x16 device.

Now, if this causes NAND_CMD_PARAM to fail, then you have a *different*
problem to solve. But you are not solving this problem.

[snipping the rest]

I read your patch, and I gave you my review. I will not accept this
patch, nor any patch that works around nand_scan_ident() by calling it
twice. Fix the framework if the framework is giving you problems.

I believe that this patch is not integral to the rest of the series, so
I will repeat: separate this patch out so I can take the rest of this
series without it.

Brian

Hi Brian,

> From: Brian Norris [mailto:computersforpeace@gmail.com]
> > On 10/22/2013 10:07 PM, Gupta, Pekon wrote:
> >> From: Brian Norris [mailto:computersforpeace@gmail.com]
> >> On Sat, Oct 19, 2013 at 02:14:08PM +0530, Pekon Gupta wrote:

[...]

>> So are you saying that the driver currently doesn't work if you started
>> in x16 buswidth? Are you having problems with a particular setup? What
>> sort of devices are you testing?
>>
...
> Since you didn't answer the other 2 questions there: are you testing any
> x16 devices?
> 

Ans-1: Yes, I'm testing on following boards:
  (a) AM335x-EVM which has x8 Micron device.
  http://www.ti.com/tool/tmdxevm3358
  (b) beaglebone with 'NAND cape', which has x16 Micron device.
  http://beagleboardtoys.info/index.php?title=BeagleBone_4Gb_16-Bit_NAND_Module
  (c) AM437x board (which has 4K page NAND from Macronix).
  (d) Also would be testing on DRA7xx again having Micron Device.

Ans-2:
Its not the setup but rather constrain in nand_base.c which prevents
reading of ONFI params in x16 mode. Please read below..

Ans-3:
Mostly are either x8 or x16 SLC NAND device.


[...]

> You NEVER need to call nand_scan_ident() twice for the same chip.
> Period. I will reject any patch that retains this pattern. It is wrong,
> and I seriously doubt the code does what you think it does when you do this.
> 
> I think nand_scan_ident() may have a weak point where it won't support
> ONFI properly for x16 devices. I believe NAND_BUSWIDTH_AUTO was added
> to
> help with this fact. (I don't have any x16 devices to test it.) But if
> this is a problem for you, fix it. Don't work around it.
> 
So, here comes the conflict.. 
If I'm _not_ using NAND_BUSWIDTH_AUTO, how should I read the ONFI 
params on x16 device ? I don't think there is any way because of following
code in generic nand_base.c
@@ nand_flash_onfi_detect()
/*
 * ONFI must be probed in 8-bit mode or with NAND_BUSWIDTH_AUTO, not
 * with NAND_BUSWIDTH_16
 */
if (chip->options & NAND_BUSWIDTH_16) {
	pr_err("ONFI cannot be probed in 16-bit mode; aborting\n");
	return 0;
}

Introduced in commit..
commit 0ce82b7f7b7373b16ecf7b5725e21e2975204500
Author:     Matthieu CASTET <matthieu.castet@parrot.com>
AuthorDate: 2013-01-16

And, I think this commit has implicitly made NAND_BUSWIDTH_AUTO
*mandatory* to be supported by every driver for interfacing with
x16 NAND devices. Isn't it ?
(unless you add every x16 device present in market to nand_flash_id[])

 [...]

> nand_base is designed (and it's documented in the comments) that the
> driver must set the buswidth correctly BEFORE calling nand_scan_ident().
> You may not use nand_scan_ident() as a trial-and-error identification
> function.
> 
> So, to properly do (1), you should only have something like this, just
> like all the other NAND drivers:
> 
>   nand_chip->options = pdata->devsize & NAND_BUSWIDTH_16;
> 
>   ret = nand_scan_ident(...);
>   if (ret) {
>       // exit with error code...
>   }
> 
For a x16 device.. This would *always* fail for x16 device, unless device
is listed in nand_flash_id[]. isn't  it ?
because you cannot read ONFI params in x16 mode, and your device is
not listed in nand_flash_ids[], so your device would not get identified.

And I sincerely don't know how other NAND drivers are probing
x16 NAND devices _without_ enabling NAND_BUSWIDTH_AUTO.
(may be by adding every supported NAND device to nand_flash_id[]
look-up table, which is not recommended).


> If there is a problem with this, then you have to fix your driver or
> nand_scan_ident(). Perhaps you have to adjust your readbuf() or
> cmdfunc() callbacks to do this. But do not add complicated workaround
> logic in your driver.
> 
chip->cmdfunc() and chip->readbufs() are all fine. Rather I let the
generic driver set them for nand_scan_ident().
So, all this calling nand_scan_ident() twice was done because of
previously mentioned reasons..


> [snipping the rest]
> 
> I read your patch, and I gave you my review. I will not accept this
> patch, nor any patch that works around nand_scan_ident() by calling it
> twice. Fix the framework if the framework is giving you problems.
> 
It's a chicken and egg problem, 
I have no solution but all I can say is the above commit, which converted
WARNING into ERROR, until all drivers adapt to NAND_BUSWIDTH_AUTO.

Anyway I have to do nand_scan_ident() somewhere before populating
the chip->ecc.layout and other fields.. so can't drop the patch..
But I'll put your suggestion, instead of my mine.

> I believe that this patch is not integral to the rest of the series, so
> I will repeat: separate this patch out so I can take the rest of this
> series without it.
> 
I'll replace the patch with your suggestions,
So, that you have both versions. Please pick whichever you like :-)


with regards, pekon

Brian,

On Tue, Oct 22, 2013 at 11:13:32PM -0700, Brian Norris wrote:
> On 10/22/2013 10:07 PM, Gupta, Pekon wrote:
> >> From: Brian Norris [mailto:computersforpeace@gmail.com]
> >> On Sat, Oct 19, 2013 at 02:14:08PM +0530, Pekon Gupta wrote:
> > [...]
> > 
> >>>
> >>> Thus this patch run nand_scan_ident() with driver configured as x8 device.
> >>
> >> So are you saying that the driver currently doesn't work if you started
> >> in x16 buswidth? Are you having problems with a particular setup? What
> >> sort of devices are you testing?
> >>
> > No, I'm saying that you cannot read ONFI params in x16 mode.
> > And, that is what was pointed out in following commit log also ..
> > (Reference to 3.3.2. Target Initialization: given above)
> > So, if I run nand_scan_ident() in x16 mode, my ONFI detection would 
> > fail for sure ..
> 
> But you cannot just run nand_scan_ident() with !(chip->options &
> NAND_BUSWIDTH_16) when your devices is x16. You need to solve the ONFI
> detection problem while correctly specifying NAND_BUSWIDTH_16.
> 
> Since you didn't answer the other 2 questions there: are you testing any
> x16 devices?
> 

I'm jumping on this thread without having read all the discussion, sorry
about that.

FWIW, I have a Beaglebone with a 16-bit bus NAND attached to it.

Coincidentally, yesterday I was doing some tests as I'm ramping up the
NAND and I found that weird double nand_scan_ident() call.
The whole thing looks buggy to me, so I'm happy to help, review, test
and patches to take care of this.

I'm using some TI SDK with some ancient v3.2.x (with no git history!),
but from this discussion it seems the issue is still present in
mainline.

Hi,

> From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
[...]
> FWIW, I have a Beaglebone with a 16-bit bus NAND attached to it.
> 
> Coincidentally, yesterday I was doing some tests as I'm ramping up the
> NAND and I found that weird double nand_scan_ident() call.
> The whole thing looks buggy to me, so I'm happy to help, review, test
> and patches to take care of this.
> 
Yes, thanks .. that would be of great help.. 
And may be your experience of Atmel drivers would help me here..

*Correct, should not be double calls to nand_scan_ident()..*
But there is a constrain in nand_base.c, that it does not allow ONFI
page reading in x16 mode.. So how to overcome that..

I see the similar implementation in your ATMEL driver, it does not use
NAND_BUSWIDTH_AUTO so how do you perform ONFI read
for x16 devices ?
drivers/mtd/nand/atmel_nand.c @@atmel_nand_probe()
/* here you move to x16 mode based on your DT or platform data */
	if (host->board.bus_width_16)	/* 16-bit bus width */
		nand_chip->options |= NAND_BUSWIDTH_16;
/* And then you call nand_scan_ident */
/* first scan to find the device and get the page size */
	if (nand_scan_ident(mtd, 1, NULL)) {
		res = -ENXIO;
		goto err_scan_ident;
	}

Wouldn't this fail, _unless_ your device is listed in nand_flash_id[] ?
because it would not be able to read ONFI params.. 
Refer below commit.. 
commit 0ce82b7f7b7373b16ecf7b5725e21e2975204500
Author:     Matthieu CASTET <matthieu.castet@parrot.com>
AuthorDate: 2013-01-16

Thanks for pitching in, this would help me to understand this better.



> I'm using some TI SDK with some ancient v3.2.x (with no git history!),
> but from this discussion it seems the issue is still present in
> mainline.
> 
Aah sorry, then you might have some problem here in rebasing the
patches. But still if you can, thanks much ..


with regards, pekon

Hi Gupta,

On Wed, Oct 23, 2013 at 01:15:20PM +0000, Gupta, Pekon wrote:
> Hi,
> 
> > From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
> [...]
> > FWIW, I have a Beaglebone with a 16-bit bus NAND attached to it.
> > 
> > Coincidentally, yesterday I was doing some tests as I'm ramping up the
> > NAND and I found that weird double nand_scan_ident() call.
> > The whole thing looks buggy to me, so I'm happy to help, review, test
> > and patches to take care of this.
> > 
> Yes, thanks .. that would be of great help.. 
> And may be your experience of Atmel drivers would help me here..
> 

It's not Atmel, but Marvell :-)

> *Correct, should not be double calls to nand_scan_ident()..*
> But there is a constrain in nand_base.c, that it does not allow ONFI
> page reading in x16 mode.. So how to overcome that..
> 
> I see the similar implementation in your ATMEL driver, it does not use
> NAND_BUSWIDTH_AUTO so how do you perform ONFI read
> for x16 devices ?
> drivers/mtd/nand/atmel_nand.c @@atmel_nand_probe()
> /* here you move to x16 mode based on your DT or platform data */
> 	if (host->board.bus_width_16)	/* 16-bit bus width */
> 		nand_chip->options |= NAND_BUSWIDTH_16;
> /* And then you call nand_scan_ident */
> /* first scan to find the device and get the page size */
> 	if (nand_scan_ident(mtd, 1, NULL)) {
> 		res = -ENXIO;
> 		goto err_scan_ident;
> 	}
> 
> Wouldn't this fail, _unless_ your device is listed in nand_flash_id[] ?
> because it would not be able to read ONFI params.. 
> Refer below commit.. 
> commit 0ce82b7f7b7373b16ecf7b5725e21e2975204500
> Author:     Matthieu CASTET <matthieu.castet@parrot.com>
> AuthorDate: 2013-01-16
> 
> 

Not my driver, but I'm taking a look at it now. Not sure if I'll get
into something here.

> 
> 
> > I'm using some TI SDK with some ancient v3.2.x (with no git history!),
> > but from this discussion it seems the issue is still present in
> > mainline.
> > 
> Aah sorry, then you might have some problem here in rebasing the
> patches. But still if you can, thanks much ..
> 

I'm currently trying mainline (just for this issue not for my product).
I just need some time to prepare the bootargs and write a DT node for
the NAND cape.

Again, not sure if I'll make some progress, but I'll give it a shot :-)

Pekon,

On Wed, Oct 23, 2013 at 10:24:57AM -0300, Ezequiel Garcia wrote:
[..]
> 
> I'm currently trying mainline (just for this issue not for my product).
> I just need some time to prepare the bootargs and write a DT node for
> the NAND cape.
> 
> Again, not sure if I'll make some progress, but I'll give it a shot :-)

I won't be able to make too much progress without some help or without
squeezing my brains out :P

Care to push some git branch on some random repo with DT support for
the NAND cape in the Beaglebone?

Please base it in either some recent l2-mtd or some tag from Linus.

Thanks!

Hi Ezequiel,

> From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]

> I won't be able to make too much progress without some help or without

> squeezing my brains out :P

> 

> Care to push some git branch on some random repo with DT support for

> the NAND cape in the Beaglebone?

> 

Apologies for delay, I was testing and preparing a newer version of patch-set..

Please find the beagle-bone DTS attached. However, consider this as RFC
only, as I'm waiting for current series which has some DT-binding updates
to get accepted first.

Commit log in the patch would also guide you for some board tweaks
for enabling NAND cape on beagle-bone (LT/white)..
(I have recently sent v11 of the patch series incorporating last few
 comments from Brian about nand_scan_ident().)


Thanks.. 
with regards, pekon

Pekon,

On Thu, Oct 24, 2013 at 12:59:26PM +0000, Gupta, Pekon wrote:
> Hi Ezequiel,
> 
> > From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
> > I won't be able to make too much progress without some help or without
> > squeezing my brains out :P
> > 
> > Care to push some git branch on some random repo with DT support for
> > the NAND cape in the Beaglebone?
> > 
> Apologies for delay, I was testing and preparing a newer version of patch-set..
> 
> Please find the beagle-bone DTS attached. However, consider this as RFC
> only, as I'm waiting for current series which has some DT-binding updates
> to get accepted first.
> 
> Commit log in the patch would also guide you for some board tweaks
> for enabling NAND cape on beagle-bone (LT/white)..
> (I have recently sent v11 of the patch series incorporating last few
>  comments from Brian about nand_scan_ident().)
> 

Thanks! I'll give this a try.

Anyway, for future reference, it's really easier for testers and
reviewers to just push the code in some public git repo (github? gitorious?).

This way I don't have to apply the patches from my mailbox,
but just checkout a branch...