[PATCH 0/1] Fix OMAP2 NAND ONFI device detection

[PATCH 0/1] Fix OMAP2 NAND ONFI device detection

[Ezequiel Garcia]

As it was discussed recently in the mailing list, the omap2-nand driver currently
has an issue preventing proper ONFI detection of 16-bit devices (other drivers
may suffer from this same issue).

In an attempt to address such issue, this patch uses NAND_BUSWIDTH_AUTO
(actually discarding any DT property) and leaves the bus width detection
to the NAND core code.

This has been tested in a Beaglebone black (AM335x) board with a 16-bit Micron
NAND device, both ONFI and array-based detections work fine:

[    1.560945] omap-gpmc 50000000.gpmc: GPMC revision 6.0
[    1.569328] NAND device: Manufacturer ID: 0x2c, Chip ID: 0xcc (Micron MT29F4G16ABADAH4)
[    1.577853] NAND device: 512MiB, SLC, page size: 2048, OOB size: 64
[    1.584448] nand: using OMAP_ECC_BCH8_CODE_HW ECC scheme
[    1.591772] 8 ofpart partitions found on MTD device omap2-nand.0
[    1.598171] Creating 8 MTD partitions on "omap2-nand.0":
[    1.603797] 0x000000000000-0x000000020000 : "SPL1"
[    1.612926] 0x000000020000-0x000000040000 : "SPL2"
[    1.620112] 0x000000040000-0x000000060000 : "SPL3"
[    1.627192] 0x000000060000-0x000000080000 : "SPL4"
[    1.634205] 0x000000080000-0x000000260000 : "U-boot"
[    1.642708] 0x000000260000-0x000000280000 : "environment"
[    1.650410] 0x000000280000-0x000000780000 : "Kernel"
[    1.661828] 0x000000780000-0x000010000000 : "File-System"

Also, a quick run of nandtest ends successfully:

# nandtest /dev/mtd6
ECC corrections: 0
ECC failures   : 0
Bad blocks     : 0
BBT blocks     : 0
004e0000: checking...
Finished pass 1 successfully

This time I've decided to submit this patch alone, so we can focus
the discussion on this issue. The other cleanups can wait.

AFAICS, the remaining questions are:

 1. Does this work in the 8-bit case?
 (I'm not able to test such case for lack of hardware)

 2. Do we want to re-configure the GPMC one the NAND core detects the
    device bus width?

Regarding this last question, and as I've exposed in the discussion with Pekon
about this [1], IMO, doing so is a bad design choice. It's not the NAND
driver's task to fix illed-prepared device-tree's or a badly configured memory
controller (GPMC).

[1] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg97760.html

Ezequiel Garcia (1):
  mtd: nand: omap2: Fix device detection path

 drivers/mtd/nand/omap2.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

-- 
1.8.1.5

[PATCH 1/1] mtd: nand: omap2: Fix device detection path

[Ezequiel Garcia]

Because the device bus can be 8-bit or 16-bit width, yet ONFI detection
cannot work in 16-bit mode, we need to set the NAND_BUSWIDTH_AUTO option
which allows proper initialization configuration.

Once the bus width is detected, nand_scan_ident() updates the nand_chip struct
'option' field to use the appropriate read/write functions and configure
the ECC engine.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/omap2.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index ec40b8d..c71206b 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1662,7 +1662,7 @@ static int omap_nand_probe(struct platform_device *pdev)
 	mtd->owner		= THIS_MODULE;
 	nand_chip		= &info->nand;
 	nand_chip->ecc.priv	= NULL;
-	nand_chip->options	|= NAND_SKIP_BBTSCAN;
+	nand_chip->options	|= NAND_SKIP_BBTSCAN | NAND_BUSWIDTH_AUTO;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (res == NULL) {
@@ -1708,7 +1708,6 @@ static int omap_nand_probe(struct platform_device *pdev)
 	}
 
 	/* scan NAND device connected to chip controller */
-	nand_chip->options |= pdata->devsize & NAND_BUSWIDTH_16;
 	if (nand_scan_ident(mtd, 1, NULL)) {
 		pr_err("nand device scan failed, may be bus-width mismatch\n");
 		err = -ENXIO;
-- 
1.8.1.5

Re: [PATCH 0/1] Fix OMAP2 NAND ONFI device detection

[Brian Norris]

Hi,

On Wed, Oct 30, 2013 at 06:53:07AM -0300, Ezequiel Garcia wrote:
> As it was discussed recently in the mailing list, the omap2-nand driver currently
> has an issue preventing proper ONFI detection of 16-bit devices (other drivers
> may suffer from this same issue).

First of all, thanks for clearly separating this discussion to its own
patch. It is an interesting issue by itself (without tying it up with
other driver-specific issues), and I think it has something useful to
teach other drivers, which may mostly be dodging issues with x16
devices.

> In an attempt to address such issue, this patch uses NAND_BUSWIDTH_AUTO
> (actually discarding any DT property) and leaves the bus width detection
> to the NAND core code.
> 
> This has been tested in a Beaglebone black (AM335x) board with a 16-bit Micron
> NAND device, both ONFI and array-based detections work fine:
> 
> [    1.560945] omap-gpmc 50000000.gpmc: GPMC revision 6.0
> [    1.569328] NAND device: Manufacturer ID: 0x2c, Chip ID: 0xcc (Micron MT29F4G16ABADAH4)
> [    1.577853] NAND device: 512MiB, SLC, page size: 2048, OOB size: 64
> [    1.584448] nand: using OMAP_ECC_BCH8_CODE_HW ECC scheme
> [    1.591772] 8 ofpart partitions found on MTD device omap2-nand.0
> [    1.598171] Creating 8 MTD partitions on "omap2-nand.0":
> [    1.603797] 0x000000000000-0x000000020000 : "SPL1"
> [    1.612926] 0x000000020000-0x000000040000 : "SPL2"
> [    1.620112] 0x000000040000-0x000000060000 : "SPL3"
> [    1.627192] 0x000000060000-0x000000080000 : "SPL4"
> [    1.634205] 0x000000080000-0x000000260000 : "U-boot"
> [    1.642708] 0x000000260000-0x000000280000 : "environment"
> [    1.650410] 0x000000280000-0x000000780000 : "Kernel"
> [    1.661828] 0x000000780000-0x000010000000 : "File-System"
> 
> Also, a quick run of nandtest ends successfully:
> 
> # nandtest /dev/mtd6
> ECC corrections: 0
> ECC failures   : 0
> Bad blocks     : 0
> BBT blocks     : 0
> 004e0000: checking...
> Finished pass 1 successfully

I suspected this was the case (in agreement with your comments on
previous threads): that BUSWIDTH_AUTO will discover the correct buswidth
*AND* will handle initial ONFI requests correctly (i.e., initially in x8
"mode", then switching to x16 callbacks as appropriate).

I do have one curiosity here: omap2.c looks like it's essentially
defaulting to the NAND_OMAP_POLLED callbacks during nand_scan_ident(),
since omap2.c only initializes the read_buf callback after
nand_scan_ident(). Is this ever a problem? Or should omap2.c be
initializing its callbacks both before and after nand_scan_ident()
(similar to how nand_base calls nand_set_defaults() twice for the AUTO
case)?

What value do you use for "ti,nand-xfer-type" in your BeagleBone device
tree? Is the xfer type a hardware requirement, or just a software
configuration? IOW, does the hardware care if I simply use POLLED, even
temporarily? (A potential side issue: does "ti,nand-xfer-type" even
belong in the device tree, if it is not actually a hardware property?)

> This time I've decided to submit this patch alone, so we can focus
> the discussion on this issue. The other cleanups can wait.
> 
> AFAICS, the remaining questions are:
> 
>  1. Does this work in the 8-bit case?
>  (I'm not able to test such case for lack of hardware)

I'm not sure, of course, but I don't see why not. It's more likely to
break for x16 than it is for x8.

>  2. Do we want to re-configure the GPMC one the NAND core detects the
>     device bus width?

I'm not quite sure here, as I don't know if I know enough about the GPMC
to give an educated response here. Additionally, I'm not quite sure how
"re-configurable" GPMC really is. It seems like we would be restricted
physically if GPMC is hooked up as x8 for an x16 NAND (there are not
enough pins connected). So even if we detect the NAND, it cannot
function. I'm not sure about the reverse.

Now, this returns me to my rejection of Pekon's patch here:

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

(This patch addresses the question of checking the GPMC configuration,
not correcting it.)

I'll admit that I was underinformed regarding the need for this type of
patch. Since that time, I am less sure of my criticism. But really, I
just have more questions now.

Does the GPMC intrinsically (physically; according to its pin
configuration) restrict an attached device (e.g., NAND) to use x16 or
x8? Or does it simply specify a maximum data width that is wired up?
(I'm presuming that you could wire an x8 device to an x16 interface and
just leave the upper 8 unconnected...) Or is there some third option?

The DT entry says:

  gpmc,device-width     Total width of device(s) connected to a GPMC
                        chip-select in bytes. The GPMC supports 8-bit
                        and 16-bit devices and so this property must be
                        1 or 2.

So, this *sounds* like it specifies the exact width. But as I read it,
this property could more optimally be specified as the *maximum*
width...

> Regarding this last question, and as I've exposed in the discussion with Pekon
> about this [1], IMO, doing so is a bad design choice. It's not the NAND
> driver's task to fix illed-prepared device-tree's or a badly configured memory
> controller (GPMC).
> 
> [1] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg97760.html

Brian

RE: [PATCH 0/1] Fix OMAP2 NAND ONFI device detection

[Gupta, Pekon]

Hi Brian, Ezequiel Garcia,

Some replies to your queries...

> From: Brian Norris [mailto:computersforpeace@gmail.com]
> > On Wed, Oct 30, 2013 at 06:53:07AM -0300, Ezequiel Garcia wrote:

[...]

> I do have one curiosity here: omap2.c looks like it's essentially
> defaulting to the NAND_OMAP_POLLED callbacks during nand_scan_ident(),
> since omap2.c only initializes the read_buf callback after
> nand_scan_ident(). Is this ever a problem? Or should omap2.c be
> initializing its callbacks both before and after nand_scan_ident()
> (similar to how nand_base calls nand_set_defaults() twice for the AUTO
> case)?
> 
There is no issue if _default_ functions present in nand_base.c
are used for chip->read_byte() or chip->read_buf() while probing the device.
The different callbacks defined in omap2.c like
- omap_read_buf()
- omap_read_buf_pref()
- omap_read_buf_dma_pref()
- omap_read_buf_irq_pref()
Above are alternatives for having better throughput in different use-cases.
These are not tied to hardware. So it's okay if these callbacks are assigned
post nand_scan_ident().


> What value do you use for "ti,nand-xfer-type" in your BeagleBone device
> tree? Is the xfer type a hardware requirement, or just a software
> configuration? IOW, does the hardware care if I simply use POLLED, even
> temporarily? (A potential side issue: does "ti,nand-xfer-type" even
> belong in the device tree, if it is not actually a hardware property?)
> 
DMA and IRQ based callbacks have better throughput numbers than
POLLED ones. Though its not related to hardware, but as it's there in DT
now so we should maintain it. Also considering that:
- some older platforms might not support xx_dma_pref().
- some related pieces of information (like IRQ number, etc) comes from DT.
It was added as part of following patch..
http://www.spinics.net/lists/linux-omap/msg90250.html


> > This time I've decided to submit this patch alone, so we can focus
> > the discussion on this issue. The other cleanups can wait.
> >
> > AFAICS, the remaining questions are:
> >
> >  1. Does this work in the 8-bit case?
> >  (I'm not able to test such case for lack of hardware)
> 
> I'm not sure, of course, but I don't see why not. It's more likely to
> break for x16 than it is for x8.
> 
Another question here is ..
The above patch assumes that user has configured GPMC bus-width
correctly, so if user is already providing GPMC bus-width information
via DT, then do we really need to detect NAND device bus-width again
using NAND_BUSWIDTH_AUTO ?
 

> >  2. Do we want to re-configure the GPMC one the NAND core detects the
> >     device bus width?
> 
> I'm not quite sure here, as I don't know if I know enough about the GPMC
> to give an educated response here. Additionally, I'm not quite sure how
> "re-configurable" GPMC really is. It seems like we would be restricted
> physically if GPMC is hooked up as x8 for an x16 NAND (there are not
> enough pins connected). So even if we detect the NAND, it cannot
> function. I'm not sure about the reverse.
> 
> Now, this returns me to my rejection of Pekon's patch here:
> 
>   http://lists.infradead.org/pipermail/linux-mtd/2013-October/049154.html
> 
> (This patch addresses the question of checking the GPMC configuration,
> not correcting it.)
> 
> I'll admit that I was underinformed regarding the need for this type of
> patch. Since that time, I am less sure of my criticism. But really, I
> just have more questions now.
> 
> Does the GPMC intrinsically (physically; according to its pin
> configuration) restrict an attached device (e.g., NAND) to use x16 or
> x8?
Yes.. 

>  Or does it simply specify a maximum data width that is wired up?
> (I'm presuming that you could wire an x8 device to an x16 interface and
> just leave the upper 8 unconnected...) Or is there some third option?
> 
GPMC engine uses bus-width info to drive its data-lines.
- If GPMC is configured as x8, then it will only perform I/O on D0.. D7,
   even if all D0 .. D15 were wired on board.
- If GPMC is configured as x16, then it will perform I/O on D0.. D15,
   even if only D0 .. D7 were wired on board. There by reading 0x0 or
   garbage on D8 .. D15 lines.

This does not affect the probe, because chip->read_byte() would return
only single byte whether it's a x8 device or x16 device. So READID_CMD
works fine and you can read maf_id and dev_id. And based on that
device parameters can be looked from nand_flash_id[].
However when it comes to reading or writing complete page, then the
mismatch between GPMC configuration and actual NAND device
bus-width is seen, because there chip->read_buf() or chip->write_buf()
is used.


> The DT entry says:
> 
>   gpmc,device-width     Total width of device(s) connected to a GPMC
>                         chip-select in bytes. The GPMC supports 8-bit
>                         and 16-bit devices and so this property must be
>                         1 or 2.
> 
> So, this *sounds* like it specifies the exact width. But as I read it,
> this property could more optimally be specified as the *maximum*
> width...
> 
Yes, its exact width..


> > Regarding this last question, and as I've exposed in the discussion with
> Pekon
> > about this [1], IMO, doing so is a bad design choice. It's not the NAND
> > driver's task to fix illed-prepared device-tree's or a badly configured
> memory
> > controller (GPMC).
> >
> > [1] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg97760.html
> 
> Brian


with regards, pekon

Re: [PATCH 0/1] Fix OMAP2 NAND ONFI device detection

[Ezequiel Garcia]

Pekon,

Let me answer this one alone, given it's an important question.

On Wed, Oct 30, 2013 at 09:18:53PM +0000, Gupta, Pekon wrote:
> > 
> > I'm not sure, of course, but I don't see why not. It's more likely to
> > break for x16 than it is for x8.
> > 
> Another question here is ..
> The above patch assumes that user has configured GPMC bus-width
> correctly, so if user is already providing GPMC bus-width information
> via DT, then do we really need to detect NAND device bus-width again
> using NAND_BUSWIDTH_AUTO ?
>

Hm.. I think you're forgetting the original motivation for this patch,
which was initially explained by you :-) The problem is ONFI doesn't work
in 16-bit mode.

Let me clarify. Since GPMC and NAND widths *must* match (otherwise, it
wouldn't make much sense, right?) we don't really need to autodetect the
NAND width.

However, since ONFI doesn't work in 16-bit mode, we would have to do
something like this (untested pseudo code):

  nand_scan_ident(...);

  if (platform_data->devsize == 16)
  	nand_chip->options |= NAND_BUSWIDTH_16;

And in some cases we might even get away with such solution. However,
we can't guarantee nand_scan_ident() doesn't need to swith to 16-bit mode
inside to perform other commands (maybe some ONFI extended parameter page).

Therefore, and given the width can be autodetected in any case (array
or ONFI based carry such information) it's much cleaner to simply do:

  nand_chip->options |= NAND_BUSWIDTH_AUTOMAGIC;
  nand_scan_ident(...);

See? Much cleaner, no?

And remember: the fact that we must know the device width to configure
the GPMC correctly (which being a hardware parameter belongs to the DT)
is OMAP specific. Other SoCs might not have such memory controller
and thus won't need such knowledge before hand, although that sounds
unlikely to me.

The outcome of this discussion seems to be that no driver should ever
need to specify the 'nand-bus-width' DT property, as Brian
previously suggested, right?

I must admit I'm a bit puzzled by seeing most drivers setting 16-bit
before calling nand_scan_ident(). I guess none of them relies on ONFI?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 0/1] Fix OMAP2 NAND ONFI device detection

[Gupta, Pekon]

> From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
> > On Wed, Oct 30, 2013 at 09:18:53PM +0000, Gupta, Pekon wrote:
> > >
> > > I'm not sure, of course, but I don't see why not. It's more likely to
> > > break for x16 than it is for x8.
> > >
> > Another question here is ..
> > The above patch assumes that user has configured GPMC bus-width
> > correctly, so if user is already providing GPMC bus-width information
> > via DT, then do we really need to detect NAND device bus-width again
> > using NAND_BUSWIDTH_AUTO ?
> >
> 
> Hm.. I think you're forgetting the original motivation for this patch,
> which was initially explained by you :-) The problem is ONFI doesn't work
> in 16-bit mode.
> 
> Let me clarify. Since GPMC and NAND widths *must* match (otherwise, it
> wouldn't make much sense, right?) we don't really need to autodetect the
> NAND width.
> 
> However, since ONFI doesn't work in 16-bit mode, we would have to do
> something like this (untested pseudo code):
> 
>   nand_scan_ident(...);
> 
>   if (platform_data->devsize == 16)
>   	nand_chip->options |= NAND_BUSWIDTH_16;
> 
> And in some cases we might even get away with such solution. However,
> we can't guarantee nand_scan_ident() doesn't need to swith to 16-bit mode
> inside to perform other commands (maybe some ONFI extended parameter
> page).
> 
> Therefore, and given the width can be autodetected in any case (array
> or ONFI based carry such information) it's much cleaner to simply do:
> 
>   nand_chip->options |= NAND_BUSWIDTH_AUTOMAGIC;
>   nand_scan_ident(...);
> 
> See? Much cleaner, no?
> 
but still dependent on DT property for GPMC configurations...
I preferred NAND bus-width detection to be completely independent
of 'any' DT property.


> And remember: the fact that we must know the device width to configure
> the GPMC correctly (which being a hardware parameter belongs to the DT)
> is OMAP specific. Other SoCs might not have such memory controller
> and thus won't need such knowledge before hand, although that sounds
> unlikely to me.
> 
> The outcome of this discussion seems to be that no driver should ever
> need to specify the 'nand-bus-width' DT property, as Brian
> previously suggested, right?
> 
Right.. And this is where I'm suggesting that in-order to completely
eliminate the dependency on 'nand-bus-width' DT property, you need
to also update GPMC driver, so that its registers can be re-configured with
correct NAND bus-width after nand_scan_ident().


> I must admit I'm a bit puzzled by seeing most drivers setting 16-bit
> before calling nand_scan_ident(). I guess none of them relies on ONFI?
> 
May be, but going forward we should make NAND_BUSWIDTH_AUTO
as a mandatory option, because most of the NAND devices have adopted
to ONFI, and remaining legacy (if any) are part of nand_flash_id[] table.
So either way the device would get detected, if you start with x8
(lowest minimum capability bus-width). Thus there is no need for user
to specify NAND device bus-width property via DT.


with regards, pekon

Re: [PATCH 0/1] Fix OMAP2 NAND ONFI device detection

[Ezequiel Garcia]

On Wed, Nov 06, 2013 at 08:54:56PM +0000, Gupta, Pekon wrote:
> > From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
> > > On Wed, Oct 30, 2013 at 09:18:53PM +0000, Gupta, Pekon wrote:
> > > >
> > > > I'm not sure, of course, but I don't see why not. It's more likely to
> > > > break for x16 than it is for x8.
> > > >
> > > Another question here is ..
> > > The above patch assumes that user has configured GPMC bus-width
> > > correctly, so if user is already providing GPMC bus-width information
> > > via DT, then do we really need to detect NAND device bus-width again
> > > using NAND_BUSWIDTH_AUTO ?
> > >
> > 
> > Hm.. I think you're forgetting the original motivation for this patch,
> > which was initially explained by you :-) The problem is ONFI doesn't work
> > in 16-bit mode.
> > 
> > Let me clarify. Since GPMC and NAND widths *must* match (otherwise, it
> > wouldn't make much sense, right?) we don't really need to autodetect the
> > NAND width.
> > 
> > However, since ONFI doesn't work in 16-bit mode, we would have to do
> > something like this (untested pseudo code):
> > 
> >   nand_scan_ident(...);
> > 
> >   if (platform_data->devsize == 16)
> >   	nand_chip->options |= NAND_BUSWIDTH_16;
> > 
> > And in some cases we might even get away with such solution. However,
> > we can't guarantee nand_scan_ident() doesn't need to swith to 16-bit mode
> > inside to perform other commands (maybe some ONFI extended parameter
> > page).
> > 
> > Therefore, and given the width can be autodetected in any case (array
> > or ONFI based carry such information) it's much cleaner to simply do:
> > 
> >   nand_chip->options |= NAND_BUSWIDTH_AUTOMAGIC;
> >   nand_scan_ident(...);
> > 
> > See? Much cleaner, no?
> > 
> but still dependent on DT property for GPMC configurations...
> I preferred NAND bus-width detection to be completely independent
> of 'any' DT property.
> 

I think we're still mixing GPMC and NAND, and I don't think it's sane.

> > And remember: the fact that we must know the device width to configure
> > the GPMC correctly (which being a hardware parameter belongs to the DT)
> > is OMAP specific. Other SoCs might not have such memory controller
> > and thus won't need such knowledge before hand, although that sounds
> > unlikely to me.
> > 
> > The outcome of this discussion seems to be that no driver should ever
> > need to specify the 'nand-bus-width' DT property, as Brian
> > previously suggested, right?
> > 
> Right.. And this is where I'm suggesting that in-order to completely
> eliminate the dependency on 'nand-bus-width' DT property, you need
> to also update GPMC driver, so that its registers can be re-configured with
> correct NAND bus-width after nand_scan_ident().
> 


Well, as I said: I don't think there's any technical reason why you
can't just export a GPMC function to re-configure it from the NAND
driver. I just think it's ugly, and not useful.

So, I think you (or me, or anybody else) can push an RFC/PATCH to see
how ugly would that really be. Maybe it's not so bad.

But: on the other hand, I'd really like you to convince me as to
why is it so bad to require the DTB to have the proper GPMC bus width.

Once again:
1. the NAND devices aren't hot-pluggable
2. the "user" (who is actually an engineer, not some regular dummy user)
knows perfectly well the width of the device.

What's the problem with describing the hardware in the DT and saving us
lots of runtime re-configuration trouble?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 0/1] Fix OMAP2 NAND ONFI device detection

[Gupta, Pekon]

> From: Ezequiel Garcia
[...]

> But: on the other hand, I'd really like you to convince me as to
> why is it so bad to require the DTB to have the proper GPMC bus width.
> 
No its not at all bad, all I want is either of the one way (not mixture of both).
- Either depend on DT completely (which is current case for all drivers)
- OR depend on ONFI and nand_flash_id[] for bus-width detection.


> Once again:
> 1. the NAND devices aren't hot-pluggable
> 2. the "user" (who is actually an engineer, not some regular dummy user)
> knows perfectly well the width of the device.
> 
> What's the problem with describing the hardware in the DT and saving us
> lots of runtime re-configuration trouble?

I agree with both your arguments above.
So shouldn't we kill NAND_BUSWIDTH_AUTO ?
And probably therefore  NAND_BUSWIDTH_AUTO isn't that popular.

Now what remains is ONFI probe, which should always happen in x8 mode.
So for that below patch should be sufficient ..

----------------------
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index ec1db1e..d1220fb 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2942,14 +2942,8 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
                chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I')
                return 0;

-       /*
-        * 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;
-       }
+       /* ONFI must be probed in 8-bit mode only */
+       nand_set_defaults(chip, 0);

        chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
        for (i = 0; i < 3; i++) {
@@ -2962,7 +2956,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,

        if (i == 3) {
                pr_err("Could not find valid ONFI parameter page; aborting\n");
-               return 0;
+               goto return_error;
        }

        /* Check version */
@@ -2980,7 +2974,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,

        if (!chip->onfi_version) {
                pr_info("%s: unsupported ONFI version: %d\n", __func__, val);
-               return 0;
+               goto return_error;
        }

        sanitize_string(p->manufacturer, sizeof(p->manufacturer));
@@ -3033,6 +3027,12 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
        }

        return 1;
+
+return_error:
+       /* revert original bus-width */
+       nand_set_defaults( chip->options & NAND_BUSWIDTH_16);
+       return 0;
+
 }

 /*
------------------------- 


with regards, pekon

Re: [PATCH 0/1] Fix OMAP2 NAND ONFI device detection

[Ezequiel Garcia]

On Wed, Nov 06, 2013 at 10:00:09PM +0000, Gupta, Pekon wrote:
> > From: Ezequiel Garcia
> [...]
> 
> > But: on the other hand, I'd really like you to convince me as to
> > why is it so bad to require the DTB to have the proper GPMC bus width.
> > 
> No its not at all bad, all I want is either of the one way (not mixture of both).
> - Either depend on DT completely (which is current case for all drivers)
> - OR depend on ONFI and nand_flash_id[] for bus-width detection.
> 
> 
> > Once again:
> > 1. the NAND devices aren't hot-pluggable
> > 2. the "user" (who is actually an engineer, not some regular dummy user)
> > knows perfectly well the width of the device.
> > 
> > What's the problem with describing the hardware in the DT and saving us
> > lots of runtime re-configuration trouble?
> 
> I agree with both your arguments above.
> So shouldn't we kill NAND_BUSWIDTH_AUTO ?
> And probably therefore  NAND_BUSWIDTH_AUTO isn't that popular.
> 
> Now what remains is ONFI probe, which should always happen in x8 mode.
> So for that below patch should be sufficient ..
> 

Hm.. that might work. Maybe you should submit this as RFC/PATCH to catch
the attention of some more people. And we can keep discussing on this
new idea...

I should get an 8-bit module for the BBB, so this means I'll be able
to run 8-bit and 16-bit tests.

> ----------------------
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index ec1db1e..d1220fb 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2942,14 +2942,8 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
>                 chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I')
>                 return 0;
> 
> -       /*
> -        * 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;
> -       }
> +       /* ONFI must be probed in 8-bit mode only */
> +       nand_set_defaults(chip, 0);
> 
>         chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
>         for (i = 0; i < 3; i++) {
> @@ -2962,7 +2956,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
> 
>         if (i == 3) {
>                 pr_err("Could not find valid ONFI parameter page; aborting\n");
> -               return 0;
> +               goto return_error;
>         }
> 
>         /* Check version */
> @@ -2980,7 +2974,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
> 
>         if (!chip->onfi_version) {
>                 pr_info("%s: unsupported ONFI version: %d\n", __func__, val);
> -               return 0;
> +               goto return_error;
>         }
> 
>         sanitize_string(p->manufacturer, sizeof(p->manufacturer));
> @@ -3033,6 +3027,12 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
>         }
> 
>         return 1;
> +
> +return_error:
> +       /* revert original bus-width */
> +       nand_set_defaults( chip->options & NAND_BUSWIDTH_16);
> +       return 0;
> +
>  }
> 
>  /*
> ------------------------- 
> 
> 
> with regards, pekon

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/1] Fix OMAP2 NAND ONFI device detection

[Brian Norris]

Hi Ezequiel,

Dragging up an old piece of the conversation, but I think this
highlights some of the difficulty we're still having. Perhaps I should
have headed this off a month ago...

On Wed, Oct 30, 2013 at 08:19:57PM -0300, Ezequiel Garcia wrote:
> On Wed, Oct 30, 2013 at 09:18:53PM +0000, Gupta, Pekon wrote:
> > > 
> > > I'm not sure, of course, but I don't see why not. It's more likely to
> > > break for x16 than it is for x8.
> > > 
> > Another question here is ..
> > The above patch assumes that user has configured GPMC bus-width
> > correctly, so if user is already providing GPMC bus-width information
> > via DT, then do we really need to detect NAND device bus-width again
> > using NAND_BUSWIDTH_AUTO ?
> >
> 
> Hm.. I think you're forgetting the original motivation for this patch,
> which was initially explained by you :-) The problem is ONFI doesn't work
> in 16-bit mode.

So fix our ONFI detection code! Uwe's patch has helped reinforce what I
believe (but can't test yet, as I have no x16 hardware): that we can
*fix* the ONFI code so that it doesn't matter what bus width "mode" we
are in, but we can *always* identify the lower 8 bits of the bus.

> Let me clarify. Since GPMC and NAND widths *must* match (otherwise, it
> wouldn't make much sense, right?) we don't really need to autodetect the
> NAND width.

The rest of your argument should be trimmed here. Your following
comments are seemingly a hack to get around the fact that we don't
support ONFI correctly. But I think we should take a look at Uwe's
approach before going this far on a hack.

> However, since ONFI doesn't work in 16-bit mode, we would have to do
> something like this (untested pseudo code):
> 
>   nand_scan_ident(...);
> 
>   if (platform_data->devsize == 16)
>   	nand_chip->options |= NAND_BUSWIDTH_16;
> 
> And in some cases we might even get away with such solution. However,
> we can't guarantee nand_scan_ident() doesn't need to swith to 16-bit mode
> inside to perform other commands (maybe some ONFI extended parameter page).

(ONFI extended parameter page is only on the lower 8 bits too. So no
"16-bit mode" there.)

> I must admit I'm a bit puzzled by seeing most drivers setting 16-bit
> before calling nand_scan_ident(). I guess none of them relies on ONFI?

I'm fairly confident that most of those drivers have never been used
with a 16-bit bus width since the introduction of ONFI. I believe x16 is
much less common than x8, and there are probably several drivers that
get little or no use anyway. I think the closest we got to testing
results was when a developer complained about this line in the ONFI
code:

	WARN_ON(chip->options & NAND_BUSWIDTH_16);

resulting in commit 0ce82b7f7b7373b16ecf7b5725e21e2975204500. (It's
notable Paul was using similar hardware to you and Pekon...)

Brian