Re: [PATCH] mtd: gpmi-nand: do not fail setting ONFI timing mode if available

[Miquel RAYNAL <miquel.raynal@free-electrons.com>]

Hi Han and Boris,

On Fri, 14 Jul 2017 19:31:43 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> Hi Han,
> 
> Le Fri, 14 Jul 2017 14:53:39 +0000,
> Han Xu <han.xu@nxp.com> a écrit :
> 
> > On 07/13/2017 03:15 PM, Boris Brezillon wrote:  
> > > Hi Miquel,
> > >
> > > Le Thu, 13 Jul 2017 21:20:30 +0200,
> > > Miquel Raynal <miquel.raynal@free-electrons.com> a écrit :
> > >    
> > >> GPMI NFC driver fails to apply timing mode if the
> > >> ->onfi_get_features() does not return the mode that was
> > >> previously applied.
> > >>
> > >> We can assume that a nand chip supports a timing as long as it is
> > >> read from the ONFI parameter page. Reading back a different mode
> > >> than the one previously applied does not mean the mode is
> > >> unsupported but that the nand chip does not implement the ONFI
> > >> feature because it probably does not need to.
> > >>
> > >> The output of ->onfi_get_feature() is irrelevant so delete
> > >> it.    
> > > Having the NAND part that is not supporting the
> > > get/set(timing_mode) feature explicitly mentioned in the commit
> > > message would help reviewers understand why this patch is needed.
> > >
> > > Also mention that, even though the SET/GET_FEATURES(timing_mode)
> > > is marked as required in the ONFI spec, this Macronix chip does
> > > not support it which could be considered as a bug.
> > >
> > > Regards,
> > >
> > > Boris    
> > 
> > Yes, this is a Macronix chip bug and I have reproduced on my side, 
> > ignoring the GET_FEATURE checking is a workaround and the chip will 
> > still works in EDO mode 5, but I don't accept to remove the
> > reasonable checking code for a chip bug.  
> 
> I understand why you're reluctant to remove this check just to make
> one particular chip work correctly, but, on the other hand, if we were
> only supporting non-broken NAND chip in mainline, plenty of boards
> wouldn't be supported. Flash vendors tend to take liberties with
> standards, that's a fact, and once the chip is out there's nothing we
> can do about it, except add a workaround to support it.
> 
> So let's try to find a solution that makes everyone happy: now that we
> have nand_manufacturer_ops, we can easily let manufacturer code flag
> specific chip features as broken and let the core or drivers test for
> it before using the feature.
> This way, the gpmi-nand driver could check this flag before trying to
> call ->onfi_set/get_features(TIMING).
> Would that work for you?
> 
> BTW, that'd be great to have this driver converted to the
> ->setup_data_interface() approach at some point.  

I do agree with both of you.

If sent this patch without asking myself more questions because:
not checking if the timings have been properly set by a call to
->onfi_get_features() is what the nand core does.

http://elixir.free-electrons.com/linux/v4.12/source/drivers/mtd/nand/nand_base.c#L1110

Of course it would be better to use the ->setup_data_interface()
but this is much bigger effort.


Regards,
Miquèl

> 
> Regards,
> 
> Boris
> 
> >   
> > >> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > >> ---
> > >>   drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 7 -------
> > >>   1 file changed, 7 deletions(-)
> > >>
> > >> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> > >> b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c index
> > >> 141bd70a49c2..4d137145439c 100644 ---
> > >> a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c +++
> > >> b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c @@ -939,13 +939,6 @@
> > >> static int enable_edo_mode(struct gpmi_nand_data *this, int
> > >> mode) if (ret) goto err_out;
> > >>   
> > >> -	/* [2] send GET FEATURE command to double-check the
> > >> timing mode */
> > >> -	memset(feature, 0, ONFI_SUBFEATURE_PARAM_LEN);
> > >> -	ret = nand->onfi_get_features(mtd, nand,
> > >> -				ONFI_FEATURE_ADDR_TIMING_MODE,
> > >> feature);
> > >> -	if (ret || feature[0] != mode)
> > >> -		goto err_out;
> > >> -
> > >>   	nand->select_chip(mtd, -1);
> > >>   
> > >>   	/* [3] set the main IO clock, 100MHz for mode 5, 80MHz
> > >> for mode 4. */    
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/



-- 
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com