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

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

[Hector Palacios]

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.

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;
 	}
 
 	for (mode = best_mode; mode >= 0; mode--) {

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

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

[Miquel Raynal]

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/

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

[Hector Palacios]

Hello Miquel,

On 2/23/23 08:51, Miquel Raynal wrote:
>> 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 thought it made sense to fix it globally given it is so by ONFI 
specification, and this would fix the issue should it appear in other 
NANDs, but I'm okay with fixing it just for Hynix.
Let's hope not other chips have this issue (it took me a while to get to 
the bottom of it).

> 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://linkprotect.cudasvc.com/url?a=https%3a%2f%2felixir.bootlin.com%2flinux%2flatest%2fsource%2fdrivers%2fmtd%2fnand%2fraw%2fnand_macronix.c%23L148&c=E,1,9E7nElZcjSj8kl2R8hkO4e2aulhq2M89qVpO4lQYKq8KnlEKHpy0_uktaTgj5VCh7RF1zieb1j4ajR_HMFNIUrL6i49RS3SU5muHrOpE_P0,&typo=1
> - https://linkprotect.cudasvc.com/url?a=https%3a%2f%2felixir.bootlin.com%2flinux%2flatest%2fsource%2fdrivers%2fmtd%2fnand%2fraw%2fnand_micron.c%23L598&c=E,1,PsXGddQo0M6DxaYiLy9IOETY-Ou7MdqQpmijULAYJX3Uv78VOeH2UsBzGXp5Of2NHphcgoSauNOlsFoCdb768XnR3lypMefCLGPtodD71wPZywdH1HnJT7X4bpw,&typo=1
> 
> 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--) {

Many thanks for the links and hints. That was helpful.
I will send a v2 shortly.
-- 
Héctor Palacios


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