[PATCH] mtd: nand: auto-detection of NAND bus-width from ONFI param or nand_id[]

[PATCH] mtd: nand: auto-detection of NAND bus-width from ONFI param or nand_id[]

[Pekon Gupta]

This patch is alternative implementation for following commit which introduced
NAND_BUSWIDTH_AUTO for detection of bus-width during device probe
   commit 64b37b2a63eb2f80b65c7185f0013f8ffc637ae3
   Author:     Matthieu CASTET <matthieu.castet@parrot.com>
   AuthorDate: 2012-11-06

As NAND device is identified only during nand_scan_ident(), so this patch 
assumes that NAND driver may un-initialized or partially congigured while
calling nand_scan_ident(). Hence, this patch does following:

(1) Temporarily configures 'bus-width=x8' mode before reading ONFI parameters,
 (as required by ONFI spec Refer[*]), and then reverts to original bus-width.
 This allows nand_flash_detect_onfi() to read ONFI paramers page even if
 bus-width was un-initialized or incorrectly configured.

(2) reconfigures driver with correct bus-width determined by:
 - either by reading ONFI param OR
 - as found in nand_flash_id[] table
 So, any driver-specific callback overrides should be done post nand_scan_ident.

This patch removes any dependency on either 'DT binding' or 'platform data' to
for determining NAND device bus-width.

[*] Reference: ONFI spec version 3.1 (section 3.5.3. 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."


Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 drivers/mtd/nand/nand_base.c | 43 ++++++++++++++++++-------------------------
 include/linux/mtd/nand.h     |  7 -------
 2 files changed, 18 insertions(+), 32 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index bd39f7b..3d581a4 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2942,14 +2942,9 @@ 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, so switch to x8 mode */
+	if (chip->options & NAND_BUSWIDTH_16)
+		nand_set_defaults(chip, 0);
 
 	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
 	for (i = 0; i < 3; i++) {
@@ -2962,7 +2957,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 +2975,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 +3028,12 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 	}
 
 	return 1;
+
+return_error:
+	/* revert to original bus-width */
+	nand_set_defaults(chip, chip->options & NAND_BUSWIDTH_16);
+	return 0;
+
 }
 
 /*
@@ -3431,22 +3432,14 @@ ident_done:
 			break;
 	}
 
-	if (chip->options & NAND_BUSWIDTH_AUTO) {
-		WARN_ON(chip->options & NAND_BUSWIDTH_16);
-		chip->options |= busw;
+
+	/* re-configure driver is bus-width was incorrectly configured */
+	if (busw != (chip->options & NAND_BUSWIDTH_16)) {
+		pr_warn("reconfiguring NAND bus width to %d instead %d bit\n",
+			   busw ? 16 : 8,
+			   (chip->options & NAND_BUSWIDTH_16) ? 16 : 8);
+		chip->options = (chip->options & ~NAND_BUSWIDTH_16) | busw;
 		nand_set_defaults(chip, busw);
-	} else if (busw != (chip->options & NAND_BUSWIDTH_16)) {
-		/*
-		 * Check, if buswidth is correct. Hardware drivers should set
-		 * chip correct!
-		 */
-		pr_info("NAND device: Manufacturer ID:"
-			" 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id,
-			*dev_id, nand_manuf_ids[maf_idx].name, mtd->name);
-		pr_warn("NAND bus width %d instead %d bit\n",
-			   (chip->options & NAND_BUSWIDTH_16) ? 16 : 8,
-			   busw ? 16 : 8);
-		return ERR_PTR(-EINVAL);
 	}
 
 	nand_decode_bbm_options(mtd, chip, id_data);
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 9e6c8f9..d5cc642 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -183,13 +183,6 @@ typedef enum {
 #define NAND_OWN_BUFFERS	0x00020000
 /* Chip may not exist, so silence any errors in scan */
 #define NAND_SCAN_SILENT_NODEV	0x00040000
-/*
- * Autodetect nand buswidth with readid/onfi.
- * This suppose the driver will configure the hardware in 8 bits mode
- * when calling nand_scan_ident, and update its configuration
- * before calling nand_scan_tail.
- */
-#define NAND_BUSWIDTH_AUTO      0x00080000
 
 /* Options set by nand scan */
 /* Nand scan has allocated controller struct */
-- 
1.8.1

Re: [PATCH] mtd: nand: auto-detection of NAND bus-width from ONFI param or nand_id[]

[Ezequiel Garcia]

Pekon,

Thanks for taking care of this! :-)

On Mon, Nov 25, 2013 at 06:02:08PM +0530, Pekon Gupta wrote:
> This patch is alternative implementation for following commit which introduced
> NAND_BUSWIDTH_AUTO for detection of bus-width during device probe
>    commit 64b37b2a63eb2f80b65c7185f0013f8ffc637ae3
>    Author:     Matthieu CASTET <matthieu.castet@parrot.com>
>    AuthorDate: 2012-11-06
> 
> As NAND device is identified only during nand_scan_ident(), so this patch 
> assumes that NAND driver may un-initialized or partially congigured while
> calling nand_scan_ident(). Hence, this patch does following:
> 
> (1) Temporarily configures 'bus-width=x8' mode before reading ONFI parameters,
>  (as required by ONFI spec Refer[*]), and then reverts to original bus-width.
>  This allows nand_flash_detect_onfi() to read ONFI paramers page even if
>  bus-width was un-initialized or incorrectly configured.
> 
> (2) reconfigures driver with correct bus-width determined by:
>  - either by reading ONFI param OR
>  - as found in nand_flash_id[] table
>  So, any driver-specific callback overrides should be done post nand_scan_ident.
> 
> This patch removes any dependency on either 'DT binding' or 'platform data' to
> for determining NAND device bus-width.
> 
> [*] Reference: ONFI spec version 3.1 (section 3.5.3. 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."
> 
> 
> Signed-off-by: Pekon Gupta <pekon@ti.com>
> ---
>  drivers/mtd/nand/nand_base.c | 43 ++++++++++++++++++-------------------------
>  include/linux/mtd/nand.h     |  7 -------
>  2 files changed, 18 insertions(+), 32 deletions(-)
> 
[..]
> +
> +	/* re-configure driver is bus-width was incorrectly configured */
> +	if (busw != (chip->options & NAND_BUSWIDTH_16)) {
> +		pr_warn("reconfiguring NAND bus width to %d instead %d bit\n",
> +			   busw ? 16 : 8,
> +			   (chip->options & NAND_BUSWIDTH_16) ? 16 : 8);
> +		chip->options = (chip->options & ~NAND_BUSWIDTH_16) | busw;

Looking at this makes me wonder why are we *re* configuring, instead of
just configuring. I mean, why do we keep the NAND_BUSWIDTH_16 setting?

What use case might need the user to set it, before hand?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

RE: [PATCH] mtd: nand: auto-detection of NAND bus-width from ONFI param or nand_id[]

[Gupta, Pekon]

Hi Ezequiel,


> From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
> Thanks for taking care of this! :-)
> 
Yes, I was waiting for -rc1 to be TI-GPMC driver which still gets configured
from DT independently. However that’s a separate discussion already
going in your earlier thread.


> > On Mon, Nov 25, 2013 at 06:02:08PM +0530, Pekon Gupta wrote:
[...]
> > +
> > +	/* re-configure driver is bus-width was incorrectly configured */
> > +	if (busw != (chip->options & NAND_BUSWIDTH_16)) {
> > +		pr_warn("reconfiguring NAND bus width to %d instead %d
> bit\n",
> > +			   busw ? 16 : 8,
> > +			   (chip->options & NAND_BUSWIDTH_16) ? 16 : 8);
> > +		chip->options = (chip->options & ~NAND_BUSWIDTH_16) |
> busw;
> 
> Looking at this makes me wonder why are we *re* configuring, instead of
> just configuring. I mean, why do we keep the NAND_BUSWIDTH_16 setting?
> 
> What use case might need the user to set it, before hand?
> 
Nothing.. I just said reconfiguring, bcoz some driver already configure
'chip->options & NAND_BUSWIDTH_16' pre-hand before calling
nand_scan_ident(). So, I wanted to convey that this patch should not
affect any of their functionality. And no change is should be required.

Need this to get tested with -ve testing on different boards.. 
(like setting in-correct DT binding nand-bus-width and driver should still
be able to detect and probe ONFI params) Then only it proves that this
patch is actually auto-detecting bus-width under all cases for all controllers.


with regards, pekon

RE: [PATCH] mtd: nand: auto-detection of NAND bus-width from ONFI param or nand_id[]

[Gupta, Pekon]

> From: Gupta, Pekon
> > > From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
> > Thanks for taking care of this! :-)
> >
> Yes, I was waiting for -rc1 to be TI-GPMC driver which still gets configured
> from DT independently. However that’s a separate discussion already
> going in your earlier thread.
> 
Weird typo sorry (re-writing again) ..
Yes, I was waiting for -rc1 to be tagged.
However TI-GPMC driver still needs patch to re-configure its registers,
as it gets configured from DT independently. However that’s a separate
discussion already going in different mail thread.


with regards, pekon

Re: [PATCH] mtd: nand: auto-detection of NAND bus-width from ONFI param or nand_id[]

[Ezequiel Garcia]

On Mon, Nov 25, 2013 at 01:26:11PM +0000, Gupta, Pekon wrote:
> Hi Ezequiel,
> 
> 
> > From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
> > Thanks for taking care of this! :-)
> > 
> Yes, I was waiting for -rc1 to be TI-GPMC driver which still gets configured
> from DT independently. However that’s a separate discussion already
> going in your earlier thread.
> 
> 
> > > On Mon, Nov 25, 2013 at 06:02:08PM +0530, Pekon Gupta wrote:
> [...]
> > > +
> > > +	/* re-configure driver is bus-width was incorrectly configured */
> > > +	if (busw != (chip->options & NAND_BUSWIDTH_16)) {
> > > +		pr_warn("reconfiguring NAND bus width to %d instead %d
> > bit\n",
> > > +			   busw ? 16 : 8,
> > > +			   (chip->options & NAND_BUSWIDTH_16) ? 16 : 8);
> > > +		chip->options = (chip->options & ~NAND_BUSWIDTH_16) |
> > busw;
> > 
> > Looking at this makes me wonder why are we *re* configuring, instead of
> > just configuring. I mean, why do we keep the NAND_BUSWIDTH_16 setting?
> > 
> > What use case might need the user to set it, before hand?
> > 
> Nothing.. I just said reconfiguring, bcoz some driver already configure
> 'chip->options & NAND_BUSWIDTH_16' pre-hand before calling
> nand_scan_ident(). So, I wanted to convey that this patch should not
> affect any of their functionality. And no change is should be required.
> 
> Need this to get tested with -ve testing on different boards.. 
> (like setting in-correct DT binding nand-bus-width and driver should still
> be able to detect and probe ONFI params) Then only it proves that this
> patch is actually auto-detecting bus-width under all cases for all controllers.
> 

You seem to keep insisting with the kernel auto-fixing after wrong DT
configuration. I don't think that should matter.

My point is: why don't we *remove* the devicetree property nand-bus-width and the
NAND_BUSWIDTH_16 entirely, together with this patch?

Sounds like the user shouldn't need to mess with any of these, since we
are able to auto-configure things for him.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH] mtd: nand: auto-detection of NAND bus-width from ONFI param or nand_id[]

[Huang Shijie]

于 2013年11月25日 20:32, Pekon Gupta 写道:
> This patch is alternative implementation for following commit which introduced
> NAND_BUSWIDTH_AUTO for detection of bus-width during device probe
>     commit 64b37b2a63eb2f80b65c7185f0013f8ffc637ae3
>     Author:     Matthieu CASTET<matthieu.castet@parrot.com>
>     AuthorDate: 2012-11-06
>
> As NAND device is identified only during nand_scan_ident(), so this patch
> assumes that NAND driver may un-initialized or partially congigured while
> calling nand_scan_ident(). Hence, this patch does following:
>
> (1) Temporarily configures 'bus-width=x8' mode before reading ONFI parameters,
>   (as required by ONFI spec Refer[*]), and then reverts to original bus-width.
>   This allows nand_flash_detect_onfi() to read ONFI paramers page even if
>   bus-width was un-initialized or incorrectly configured.
>
> (2) reconfigures driver with correct bus-width determined by:
>   - either by reading ONFI param OR
>   - as found in nand_flash_id[] table
>   So, any driver-specific callback overrides should be done post nand_scan_ident.
>
> This patch removes any dependency on either 'DT binding' or 'platform data' to
> for determining NAND device bus-width.
>
> [*] Reference: ONFI spec version 3.1 (section 3.5.3. 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."
>
>
> Signed-off-by: Pekon Gupta<pekon@ti.com>
> ---
>   drivers/mtd/nand/nand_base.c | 43 ++++++++++++++++++-------------------------
>   include/linux/mtd/nand.h     |  7 -------
>   2 files changed, 18 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index bd39f7b..3d581a4 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2942,14 +2942,9 @@ 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, so switch to x8 mode */
> +	if (chip->options&  NAND_BUSWIDTH_16)
> +		nand_set_defaults(chip, 0);
>
>   	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
>   	for (i = 0; i<  3; i++) {
> @@ -2962,7 +2957,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 +2975,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 +3028,12 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
>   	}
If it is 16bit bus, could we reconfigure the bus width in here?
it's better to limit the influence only in the ONFI code.

thanks
Huang Shijie


>
>   	return 1;

RE: [PATCH] mtd: nand: auto-detection of NAND bus-width from ONFI param or nand_id[]

[Gupta, Pekon]

> From: Huang Shijie [mailto:b32955@freescale.com]
[...]
> > @@ -3033,6 +3028,12 @@ static int nand_flash_detect_onfi(struct
> mtd_info *mtd, struct nand_chip *chip,
> >   	}
> If it is 16bit bus, could we reconfigure the bus width in here?
> it's better to limit the influence only in the ONFI code.
> 

Yes I too wanted same, but then once a device is found in
nand_flash_id[] table, ONFI probe is not done ..
Refer below code in nand_base.c
	@@ nand_get_flash_type()
	if (!type->name || !type->pagesize) {
			/* Check is chip is ONFI compliant */
			if (nand_flash_detect_onfi(mtd, chip, &busw))

Thus, if we want to truly auto-detection, then we need to
set bus-width in both cases
(1) when NAND device is detected based on nand_flash_id[] table
(2) when NAND device is detected based on ONFI probe

Again need more thoughts.. ?

with regards, pekon

RE: [PATCH] mtd: nand: auto-detection of NAND bus-width from ONFI param or nand_id[]

[Gupta, Pekon]

> From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
[...]
> My point is: why don't we *remove* the devicetree property nand-bus-
> width and 
> 
We cannot remove DT property like this, we can only deprecate its use,
while not breaking the functionality, which is what I'm trying here..
To completely remove a DT binding it needs approval and Ack by DT
maintainers. And over that DT binding has too still remain deprecated for
sometime till all users have migrated to newer version of kernel.
(hope you remember reviews of my earlier omap-nand patches which
updated few DT bindings :-) )...

However there should be some lifetime associated with DT bindings,
I heard David W. participated in some DT binding related discussion in
some linux conference. May be he can throw more light on "how to 
phase out unused DT bindings?"


> the NAND_BUSWIDTH_16 entirely, together with this patch?
> 
We need this Macros, as may driver use this after nand_scan_ident()
to configure their custom callbacks like:
	if (chip->options & NAND_BUSWIDTH_16)
		chip->read_buf = omap_read_buf16;
	else 
		chip->read_buf = omap_read_buf8;


> Sounds like the user shouldn't need to mess with any of these, since we
> are able to auto-configure things for him.
> 
Yes, end-user is free.. But driver still needs to x16 and x8 information
to do some controller level optimizations..


with regards, pekon

Re: [PATCH] mtd: nand: auto-detection of NAND bus-width from ONFI param or nand_id[]

[Huang Shijie]

于 2013年11月26日 15:49, Gupta, Pekon 写道:
>> From: Huang Shijie [mailto:b32955@freescale.com]
> [...]
>>> @@ -3033,6 +3028,12 @@ static int nand_flash_detect_onfi(struct
>> mtd_info *mtd, struct nand_chip *chip,
>>>    	}
>> If it is 16bit bus, could we reconfigure the bus width in here?
>> it's better to limit the influence only in the ONFI code.
>>
> Yes I too wanted same, but then once a device is found in
> nand_flash_id[] table, ONFI probe is not done ..
> Refer below code in nand_base.c
> 	@@ nand_get_flash_type()
> 	if (!type->name || !type->pagesize) {
> 			/* Check is chip is ONFI compliant */
> 			if (nand_flash_detect_onfi(mtd, chip,&busw))
>
> Thus, if we want to truly auto-detection, then we need to
> set bus-width in both cases
> (1) when NAND device is detected based on nand_flash_id[] table
If the bus-width is not the same. we'd better return with -EINVAL to tell
the user that he has a wrong configuration.

so i do not suggest we remove the -EINVAL code of nand_get_flash_type().
(I am not sure whether we should remove the NAND_BUSWIDTH_AUTO, let 
Brian judge it)


> (2) when NAND device is detected based on ONFI probe
>
Just as suspend/resume, if we do the same thing in the ONFI probe,

we do not need to remove the -EINVAL code of the nand_get_flash_type().


thanks
Huang Shijie

Re: [PATCH] mtd: nand: auto-detection of NAND bus-width from ONFI param or nand_id[]

[Ezequiel Garcia]

Hi Pekon,

On Tue, Nov 26, 2013 at 07:59:07AM +0000, Gupta, Pekon wrote:
> 
> > From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
> [...]
> > My point is: why don't we *remove* the devicetree property nand-bus-
> > width and 
> > 
> We cannot remove DT property like this, we can only deprecate its use,
> while not breaking the functionality, which is what I'm trying here..

Of course, by "remove" the property I meant make it meaningless in the
driver.

> > the NAND_BUSWIDTH_16 entirely, together with this patch?
> > 
> We need this Macros, as may driver use this after nand_scan_ident()
> to configure their custom callbacks like:
> 	if (chip->options & NAND_BUSWIDTH_16)
> 		chip->read_buf = omap_read_buf16;
> 	else 
> 		chip->read_buf = omap_read_buf8;
> 
> 
> > Sounds like the user shouldn't need to mess with any of these, since we
> > are able to auto-configure things for him.
> > 
> Yes, end-user is free.. But driver still needs to x16 and x8 information
> to do some controller level optimizations..
> 

Right. So, we should keep it but remove the effect on the initial
configuration. AFAICS, the bus width should be *reported* by the NAND
core and then each driver will have access to that information to set
any additional configuration.

On the other side, no driver should *advise* the bus width.

Right?
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH] mtd: nand: auto-detection of NAND bus-width from ONFI param or nand_id[]

[Ezequiel Garcia]

On Tue, Nov 26, 2013 at 05:22:03PM +0800, Huang Shijie wrote:
> (I am not sure whether we should remove the NAND_BUSWIDTH_AUTO, let 
> Brian judge it)
> 

Have you followed the past discussion around 16-bit ONFI detection
(currently broken, IIRC)?

We concluded that the auto-discovery (aka NAND_BUSWIDTH_AUTO) should be the default
behavior, since there's no other way to handle ONFI devices. Hence,
there's little point in keeping the macro.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

RE: [PATCH] mtd: nand: auto-detection of NAND bus-width from ONFI param or nand_id[]

[Gupta, Pekon]

> From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
> > On Tue, Nov 26, 2013 at 07:59:07AM +0000, Gupta, Pekon wrote:
> > > From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
[...]
> > > the NAND_BUSWIDTH_16 entirely, together with this patch?
> > >
> > We need this Macros, as may driver use this after nand_scan_ident()
> > to configure their custom callbacks like:
> > 	if (chip->options & NAND_BUSWIDTH_16)
> > 		chip->read_buf = omap_read_buf16;
> > 	else
> > 		chip->read_buf = omap_read_buf8;
> >
> >
> > > Sounds like the user shouldn't need to mess with any of these, since we
> > > are able to auto-configure things for him.
> > >
> > Yes, end-user is free.. But driver still needs to x16 and x8 information
> > to do some controller level optimizations..
> >
> 
> Right. So, we should keep it but remove the effect on the initial
> configuration. AFAICS, the bus width should be *reported* by the NAND
> core and then each driver will have access to that information to set
> any additional configuration.
> 
> On the other side, no driver should *advise* the bus width.
> 
> Right?

Right .. but still most drivers are using conventional approach and had 
not migrated to NAND_BUSWIDTH_AUTO, so they still set 
chip->options & NAND_BUSWIDTH_16 before calling nand_scan_ident().
So, we should simply ignore that, and not return error.


With regards, pekon

Re: [PATCH] mtd: nand: auto-detection of NAND bus-width from ONFI param or nand_id[]

[Ezequiel Garcia]

Hi Pekon,

On Mon, Nov 25, 2013 at 06:02:08PM +0530, Pekon Gupta wrote:
> This patch is alternative implementation for following commit which introduced
> NAND_BUSWIDTH_AUTO for detection of bus-width during device probe
>    commit 64b37b2a63eb2f80b65c7185f0013f8ffc637ae3
>    Author:     Matthieu CASTET <matthieu.castet@parrot.com>
>    AuthorDate: 2012-11-06
> 
> As NAND device is identified only during nand_scan_ident(), so this patch 
> assumes that NAND driver may un-initialized or partially congigured while
> calling nand_scan_ident(). Hence, this patch does following:
> 
> (1) Temporarily configures 'bus-width=x8' mode before reading ONFI parameters,
>  (as required by ONFI spec Refer[*]), and then reverts to original bus-width.
>  This allows nand_flash_detect_onfi() to read ONFI paramers page even if
>  bus-width was un-initialized or incorrectly configured.
> 
> (2) reconfigures driver with correct bus-width determined by:
>  - either by reading ONFI param OR
>  - as found in nand_flash_id[] table
>  So, any driver-specific callback overrides should be done post nand_scan_ident.
> 
> This patch removes any dependency on either 'DT binding' or 'platform data' to
> for determining NAND device bus-width.
> 
> [*] Reference: ONFI spec version 3.1 (section 3.5.3. 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."
> 
> 
> Signed-off-by: Pekon Gupta <pekon@ti.com>
> ---
>  drivers/mtd/nand/nand_base.c | 43 ++++++++++++++++++-------------------------
>  include/linux/mtd/nand.h     |  7 -------
>  2 files changed, 18 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index bd39f7b..3d581a4 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2942,14 +2942,9 @@ 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, so switch to x8 mode */
> +	if (chip->options & NAND_BUSWIDTH_16)
> +		nand_set_defaults(chip, 0);
>  
>  	chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
>  	for (i = 0; i < 3; i++) {
> @@ -2962,7 +2957,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 +2975,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 +3028,12 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
>  	}
>  
>  	return 1;
> +
> +return_error:
> +	/* revert to original bus-width */
> +	nand_set_defaults(chip, chip->options & NAND_BUSWIDTH_16);
> +	return 0;
> +
>  }
>  
>  /*
> @@ -3431,22 +3432,14 @@ ident_done:
>  			break;
>  	}
>  
> -	if (chip->options & NAND_BUSWIDTH_AUTO) {
> -		WARN_ON(chip->options & NAND_BUSWIDTH_16);
> -		chip->options |= busw;
> +
> +	/* re-configure driver is bus-width was incorrectly configured */
> +	if (busw != (chip->options & NAND_BUSWIDTH_16)) {
> +		pr_warn("reconfiguring NAND bus width to %d instead %d bit\n",
> +			   busw ? 16 : 8,
> +			   (chip->options & NAND_BUSWIDTH_16) ? 16 : 8);
> +		chip->options = (chip->options & ~NAND_BUSWIDTH_16) | busw;
>  		nand_set_defaults(chip, busw);
> -	} else if (busw != (chip->options & NAND_BUSWIDTH_16)) {
> -		/*
> -		 * Check, if buswidth is correct. Hardware drivers should set
> -		 * chip correct!
> -		 */
> -		pr_info("NAND device: Manufacturer ID:"
> -			" 0x%02x, Chip ID: 0x%02x (%s %s)\n", *maf_id,
> -			*dev_id, nand_manuf_ids[maf_idx].name, mtd->name);
> -		pr_warn("NAND bus width %d instead %d bit\n",
> -			   (chip->options & NAND_BUSWIDTH_16) ? 16 : 8,
> -			   busw ? 16 : 8);
> -		return ERR_PTR(-EINVAL);
>  	}
>  
>  	nand_decode_bbm_options(mtd, chip, id_data);
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 9e6c8f9..d5cc642 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -183,13 +183,6 @@ typedef enum {
>  #define NAND_OWN_BUFFERS	0x00020000
>  /* Chip may not exist, so silence any errors in scan */
>  #define NAND_SCAN_SILENT_NODEV	0x00040000
> -/*
> - * Autodetect nand buswidth with readid/onfi.
> - * This suppose the driver will configure the hardware in 8 bits mode
> - * when calling nand_scan_ident, and update its configuration
> - * before calling nand_scan_tail.
> - */
> -#define NAND_BUSWIDTH_AUTO      0x00080000
>  
>  /* Options set by nand scan */
>  /* Nand scan has allocated controller struct */
> -- 
> 1.8.1
> 

I'm not really convinced about the path you're taking here, as I think the
solution should be a bit simpler.

I've prepared an RFC with my proposal, based on this same patch. Let me
finish some testings and post it (I took the liberty of adding your SOB
to it).
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

RE: [PATCH] mtd: nand: auto-detection of NAND bus-width from ONFI param or nand_id[]

[Gupta, Pekon]

>From: Ezequiel Garcia [mailto:ezequiel.garcia@free-electrons.com]
>
>I'm not really convinced about the path you're taking here, as I think the
>solution should be a bit simpler.
>
>I've prepared an RFC with my proposal, based on this same patch. Let me
>finish some testings and post it (I took the liberty of adding your SOB
>to it).
> 
Sure .. plz keep me in CC when you send your patch..
I believe that having good and efficient code is more important than
having ownership/authorship of patches. So plz feel free to extend or
modify this current patch (and submit with your authorship) if you feel
it's partly useful.

with regards, pekon