diff mbox

mtd: gpmi-nand: do not fail setting ONFI timing mode if available

Message ID 20170713192030.22177-1-miquel.raynal@free-electrons.com
State Changes Requested
Delegated to: Boris Brezillon
Headers show

Commit Message

Miquel Raynal July 13, 2017, 7:20 p.m. UTC 
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.

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 7 -------
 1 file changed, 7 deletions(-)

Comments

Boris Brezillon July 13, 2017, 8:15 p.m. UTC | #1 
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

> 
> 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. */
Han Xu July 14, 2017, 2:53 p.m. UTC | #2 
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.

>> 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. */
Boris Brezillon July 14, 2017, 5:31 p.m. UTC | #3 
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.

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. */
Miquel Raynal July 15, 2017, 10:52 a.m. UTC | #4 
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/
Boris Brezillon July 15, 2017, 12:47 p.m. UTC | #5 
Le Sat, 15 Jul 2017 12:52:00 +0200,
Miquel RAYNAL <miquel.raynal@free-electrons.com> a écrit :

> 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

Probably something we should fix in nand_setup_data_interface().
Checking if a parameter has been properly set by reading it back sounds
like a good practice.

Note that, based on the tests Sascha and I did back when he implemented
the ->setup_data_interface() infrastructure, I doubt setting timing
mode on an SDR NAND has any effect.
This is the very reason I initially suggested you to drop the extra
check in the GPMI driver: if the NAND properly implements
SET/GET_FEATURES(timing), then SET_FEATURES(timing, X) should work just
fine, and if it does not but still advertise that it support modes 0 to
X, that means SET_FEATURES(timing, X) is useless and we shouldn't care
if GET_FEATURES(timing) returns a wrong value.

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

Yes, I was doing this suggestion to know if Han (or someone else) had
planned to support this feature. And yes, the initial effort is not
comparable to the 7-lines patch you submitted, but having NAND timings
selection logic in a single place is easier to maintain.

Note that I'm not requiring this rework, just gently suggesting to
think about it ;-).
diff mbox

Patch

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. */