Re: [PATCH] nand_base: force best_mode to be >= 0

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Hector,

hector.palacios@digi.com wrote on Wed, 22 Feb 2023 16:25:10 +0100:

> According to the ONFI specification, bit 0 of 'SDR timing mode support'
> (bytes 129-130) "shall be 1". That means the NAND supports at least
> timing mode 0.
> 
> NAND chip Hynix H27U4G8F2GDA-BI (at least) is reading a 0 on this field
> which makes best_mode = -1 and the following loop be skipped. An error
> code is returned upstream and the NAND probe fails.
> 
> Given that sdr_timing_modes *must* be 1 by specification, force best_mode
> to be 1 at least, so that this function doesn't return an error on a NAND
> that can work with such timings despite reporting an incorrect ONFI value.

Thanks for the patch!

> 
> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> ---
>  drivers/mtd/nand/raw/nand_base.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index c3cc66039925..474850e4455c 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -947,7 +947,7 @@ int nand_choose_best_sdr_timings(struct nand_chip *chip,
>  		/* Fallback to slower modes */
>  		best_mode = iface->timings.mode;
>  	} else if (chip->parameters.onfi) {
> -		best_mode = fls(chip->parameters.onfi->sdr_timing_modes) - 1;
> +		best_mode = fls(chip->parameters.onfi->sdr_timing_modes | 1) - 1;

I fully understand the problem and how you propose to solve it but I am
a bit hesitating. I would rather prefer a per-chip/per-manufacturer fix
rather than a global solution, just because we don't really know
whether an apparently ONFI compatible device will support that mode (or
we should at least tell the user about it). Maybe reads work but not
writes? (Or not all of them?). And in general, I would prefer to keep
the core "tidy" (well, mtd contains a lot of legacy code, but you see
what I mean) and prevent "undesired" behavior by fixing what's
considered wrong in manufacturer code.

I would rather go for a fix of the parameter page in nand_hynix.c,
either in the .init() hook or in the .fixup_onfi_param_page()
depending of what is most appropriate. You can probably use a
comparison with the model string to target your specific device for
now. Here are two different ways of handling such issues:
- https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_macronix.c#L148
- https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_micron.c#L598

Please don't forget to add a comment explaining why this is a safe
assumption, also please use "BIT(0)" rather than "1" (we don't write an
integer but just add a missing bit, that's the rationale).

>  	}
>  
>  	for (mode = best_mode; mode >= 0; mode--) {


Cheers,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/