[RFC,1/2] mtd: rawnand: handle ONFI revision number field being 0
diff mbox series

Message ID 20180618045255.8015-2-chris.packham@alliedtelesis.co.nz
State Changes Requested
Delegated to: Miquel Raynal
Headers show
Series
  • mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F
Related show

Commit Message

Chris Packham June 18, 2018, 4:52 a.m. UTC
Some Micron NAND chips (MT29F1G08ABAFAWP-ITE:F) report 00 00 for the
revision number field of the ONFI parameter page. Rather than rejecting
these outright assume ONFI version 1.0 if the revision number is 00 00.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
At the moment I haven't qualified this check on anything, I should
probably at least include vendor == MICRON.

As far as I can tell revision number == 0 is not permitted by the ONFI
spec but this wouldn't be the first time a vendor has ignored a spec. On
the other hand maybe I'm reading the spec wrong and someone here will
say "oh 0 means ...".

 drivers/mtd/nand/raw/nand_base.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Miquel Raynal June 18, 2018, 1:05 p.m. UTC | #1
Hi Chris,

On Mon, 18 Jun 2018 16:52:54 +1200, Chris Packham
<chris.packham@alliedtelesis.co.nz> wrote:

> Some Micron NAND chips (MT29F1G08ABAFAWP-ITE:F) report 00 00 for the
> revision number field of the ONFI parameter page. Rather than rejecting
> these outright assume ONFI version 1.0 if the revision number is 00 00.
> 

Thanks for getting your hands into this.

> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> At the moment I haven't qualified this check on anything, I should
> probably at least include vendor == MICRON.

The more I think about it the more I convince myself that this is not
needed. If the 4 first bytes are "ONFI", then the chip is ONFI...

Then what you do below is simple and readable and (sadly) probably
right.

> 
> As far as I can tell revision number == 0 is not permitted by the ONFI
> spec but this wouldn't be the first time a vendor has ignored a spec. On
> the other hand maybe I'm reading the spec wrong and someone here will
> say "oh 0 means ...".
> 
>  drivers/mtd/nand/raw/nand_base.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 0cd3e216b95c..1691c7005ae4 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -5184,6 +5184,8 @@ static int nand_flash_detect_onfi(struct nand_chip *chip)
>  		chip->parameters.onfi.version = 20;
>  	else if (val & (1 << 1))
>  		chip->parameters.onfi.version = 10;
> +	else if (val == 0)
> +		chip->parameters.onfi.version = 10;
>  
>  	if (!chip->parameters.onfi.version) {
>  		pr_info("unsupported ONFI version: %d\n", val);

Regards,
Miquèl
Boris Brezillon June 18, 2018, 1:17 p.m. UTC | #2
On Mon, 18 Jun 2018 15:05:21 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Chris,
> 
> On Mon, 18 Jun 2018 16:52:54 +1200, Chris Packham
> <chris.packham@alliedtelesis.co.nz> wrote:
> 
> > Some Micron NAND chips (MT29F1G08ABAFAWP-ITE:F) report 00 00 for the
> > revision number field of the ONFI parameter page. Rather than rejecting
> > these outright assume ONFI version 1.0 if the revision number is 00 00.
> >   
> 
> Thanks for getting your hands into this.
> 
> > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > ---
> > At the moment I haven't qualified this check on anything, I should
> > probably at least include vendor == MICRON.  
> 
> The more I think about it the more I convince myself that this is not
> needed. If the 4 first bytes are "ONFI", then the chip is ONFI...
> 
> Then what you do below is simple and readable and (sadly) probably
> right.

Hm, I'm not entirely convinced considering version = 0 <=> version = 1
is the right thing to do. Clearly, we don't know if other chips are
also broken WRT version field but the actual version might differ.

I'd recommend letting the manufacturer driver fix the param page
instead of guessing what has to been done in the core. That means
adding a new hook to nand_manufacturer_ops (->fixup_onfi_param_page()?)
and calling it just after the CRC has been checked [1].

[1]https://elixir.bootlin.com/linux/v4.18-rc1/source/drivers/mtd/nand/raw/nand_base.c#L5170

Patch
diff mbox series

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 0cd3e216b95c..1691c7005ae4 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -5184,6 +5184,8 @@  static int nand_flash_detect_onfi(struct nand_chip *chip)
 		chip->parameters.onfi.version = 20;
 	else if (val & (1 << 1))
 		chip->parameters.onfi.version = 10;
+	else if (val == 0)
+		chip->parameters.onfi.version = 10;
 
 	if (!chip->parameters.onfi.version) {
 		pr_info("unsupported ONFI version: %d\n", val);