ONFI timing mode with onfi_set_features unsupported

ONFI timing mode with onfi_set_features unsupported

[Sascha Hauer]

Hi all,

I have a i.MX6ul here with a Cypress S34ML04G2 connected. This Nand chip
has ONFI support, but onfi_set_features is unsupported (opt_cmd is
0x3b). Currently the gpmi Nand driver calls nand_onfi_set_features(),
gets -EINVAL as error and continues with a very slow default timing.

I assume the nand_onfi_set_features() call is just unnecessary for this
chip, if I skip it, the chip works with the fast timing.

Any idea how to cope with this situation? I attached the most obvious
patch, but it looks a bit hackish. Any suggestions or is the patch fine
as is?

Sascha


---------------------------------8<----------------------------

>From 1d76ea80915b298b99def706cac0e20b65bd7ef4 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Mon, 21 Nov 2016 14:18:27 +0100
Subject: [PATCH] mtd: nand: gpmi: Fix timing on ONFI chips with
 onfi_set_features unsupported

Some chips like for example the Cypress S34ML04G2 are ONFI compliant,
but do not support the get/set_feature commands. Do not call this
command when it's not supported to allow for a fast timing on these
chips.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
index 141bd70..7b2862c 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
@@ -930,23 +930,26 @@ static int enable_edo_mode(struct gpmi_nand_data *this, int mode)
 	if (!feature)
 		return -ENOMEM;
 
-	nand->select_chip(mtd, 0);
+	if ((le16_to_cpu(nand->onfi_params.opt_cmd)
+	      & ONFI_OPT_CMD_SET_GET_FEATURES)) {
+		nand->select_chip(mtd, 0);
 
-	/* [1] send SET FEATURE commond to NAND */
-	feature[0] = mode;
-	ret = nand->onfi_set_features(mtd, nand,
+		/* [1] send SET FEATURE commond to NAND */
+		feature[0] = mode;
+		ret = nand->onfi_set_features(mtd, nand,
 				ONFI_FEATURE_ADDR_TIMING_MODE, feature);
-	if (ret)
-		goto err_out;
+		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,
+		/* [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;
+		if (ret || feature[0] != mode)
+			goto err_out;
 
-	nand->select_chip(mtd, -1);
+		nand->select_chip(mtd, -1);
+	}
 
 	/* [3] set the main IO clock, 100MHz for mode 5, 80MHz for mode 4. */
 	rate = (mode == 5) ? 100000000 : 80000000;
-- 
2.10.2

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: ONFI timing mode with onfi_set_features unsupported

[Boris Brezillon]

Hi Sascha,

On Mon, 21 Nov 2016 14:23:27 +0100
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> Hi all,
> 
> I have a i.MX6ul here with a Cypress S34ML04G2 connected. This Nand chip
> has ONFI support, but onfi_set_features is unsupported (opt_cmd is
> 0x3b). Currently the gpmi Nand driver calls nand_onfi_set_features(),
> gets -EINVAL as error and continues with a very slow default timing.
> 
> I assume the nand_onfi_set_features() call is just unnecessary for this
> chip, if I skip it, the chip works with the fast timing.
> 
> Any idea how to cope with this situation? I attached the most obvious
> patch, but it looks a bit hackish. Any suggestions or is the patch fine
> as is?

It looks good to me. Why do you find this code hackish?
Of course, it would be even better to implement the
->setup_data_interface() method.

BTW, can you patch the core to only send the ->set_feature() command
(to change the timings mode) when the chip supports it?

Thanks,

Boris

> 
> Sascha
> 
> 
> ---------------------------------8<----------------------------
> 
> From 1d76ea80915b298b99def706cac0e20b65bd7ef4 Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Mon, 21 Nov 2016 14:18:27 +0100
> Subject: [PATCH] mtd: nand: gpmi: Fix timing on ONFI chips with
>  onfi_set_features unsupported
> 
> Some chips like for example the Cypress S34ML04G2 are ONFI compliant,
> but do not support the get/set_feature commands. Do not call this
> command when it's not supported to allow for a fast timing on these
> chips.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> index 141bd70..7b2862c 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> @@ -930,23 +930,26 @@ static int enable_edo_mode(struct gpmi_nand_data *this, int mode)
>  	if (!feature)
>  		return -ENOMEM;
>  
> -	nand->select_chip(mtd, 0);
> +	if ((le16_to_cpu(nand->onfi_params.opt_cmd)
> +	      & ONFI_OPT_CMD_SET_GET_FEATURES)) {
> +		nand->select_chip(mtd, 0);
>  
> -	/* [1] send SET FEATURE commond to NAND */
> -	feature[0] = mode;
> -	ret = nand->onfi_set_features(mtd, nand,
> +		/* [1] send SET FEATURE commond to NAND */
> +		feature[0] = mode;
> +		ret = nand->onfi_set_features(mtd, nand,
>  				ONFI_FEATURE_ADDR_TIMING_MODE, feature);
> -	if (ret)
> -		goto err_out;
> +		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,
> +		/* [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;
> +		if (ret || feature[0] != mode)
> +			goto err_out;
>  
> -	nand->select_chip(mtd, -1);
> +		nand->select_chip(mtd, -1);
> +	}
>  
>  	/* [3] set the main IO clock, 100MHz for mode 5, 80MHz for mode 4. */
>  	rate = (mode == 5) ? 100000000 : 80000000;

Re: ONFI timing mode with onfi_set_features unsupported

[Sascha Hauer]

On Mon, Nov 21, 2016 at 02:51:04PM +0100, Boris Brezillon wrote:
> Hi Sascha,
> 
> On Mon, 21 Nov 2016 14:23:27 +0100
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > Hi all,
> > 
> > I have a i.MX6ul here with a Cypress S34ML04G2 connected. This Nand chip
> > has ONFI support, but onfi_set_features is unsupported (opt_cmd is
> > 0x3b). Currently the gpmi Nand driver calls nand_onfi_set_features(),
> > gets -EINVAL as error and continues with a very slow default timing.
> > 
> > I assume the nand_onfi_set_features() call is just unnecessary for this
> > chip, if I skip it, the chip works with the fast timing.
> > 
> > Any idea how to cope with this situation? I attached the most obvious
> > patch, but it looks a bit hackish. Any suggestions or is the patch fine
> > as is?
> 
> It looks good to me. Why do you find this code hackish?
> Of course, it would be even better to implement the
> ->setup_data_interface() method.

Indeed, but my current project doesn't allow to spend that much time at
the moment.

> 
> BTW, can you patch the core to only send the ->set_feature() command
> (to change the timings mode) when the chip supports it?

With hackish I mean that I think the problem should be solved in the
core. How about returning -EOPNOTSUPP from onfi_set_features() when the
operation is not supported? The caller could then decide what to do
without testing for bits in the onfi param page.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: ONFI timing mode with onfi_set_features unsupported

[Boris Brezillon]

On Tue, 22 Nov 2016 12:02:27 +0100
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Mon, Nov 21, 2016 at 02:51:04PM +0100, Boris Brezillon wrote:
> > Hi Sascha,
> > 
> > On Mon, 21 Nov 2016 14:23:27 +0100
> > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >   
> > > Hi all,
> > > 
> > > I have a i.MX6ul here with a Cypress S34ML04G2 connected. This Nand chip
> > > has ONFI support, but onfi_set_features is unsupported (opt_cmd is
> > > 0x3b). Currently the gpmi Nand driver calls nand_onfi_set_features(),
> > > gets -EINVAL as error and continues with a very slow default timing.
> > > 
> > > I assume the nand_onfi_set_features() call is just unnecessary for this
> > > chip, if I skip it, the chip works with the fast timing.
> > > 
> > > Any idea how to cope with this situation? I attached the most obvious
> > > patch, but it looks a bit hackish. Any suggestions or is the patch fine
> > > as is?  
> > 
> > It looks good to me. Why do you find this code hackish?
> > Of course, it would be even better to implement the  
> > ->setup_data_interface() method.  
> 
> Indeed, but my current project doesn't allow to spend that much time at
> the moment.

Understood. Let's wait for Han's review.

> 
> > 
> > BTW, can you patch the core to only send the ->set_feature() command
> > (to change the timings mode) when the chip supports it?  
> 
> With hackish I mean that I think the problem should be solved in the
> core. How about returning -EOPNOTSUPP from onfi_set_features() when the
> operation is not supported? The caller could then decide what to do
> without testing for bits in the onfi param page.

The problem is, ->onfi_set_feature() is a method that can be overloaded
by NAND controller drivers. Of course, we could add a wrapper around
->onfi_set_feature() (nand_set_feature()?), but then, the meaning of
-ENOTSUPP is not clear. It could be returned if the
->onfi_set_feature() is NULL or if the requested feature is not
supported.

Another solution would be to add an extra helper to check if a specific
feature is supported:

bool nand_feature_supported(nand, feature_id);

Re: ONFI timing mode with onfi_set_features unsupported

[Sascha Hauer]

On Tue, Nov 22, 2016 at 12:10:47PM +0100, Boris Brezillon wrote:
> On Tue, 22 Nov 2016 12:02:27 +0100
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > On Mon, Nov 21, 2016 at 02:51:04PM +0100, Boris Brezillon wrote:
> > > Hi Sascha,
> > > 
> > > On Mon, 21 Nov 2016 14:23:27 +0100
> > > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > >   
> > > > Hi all,
> > > > 
> > > > I have a i.MX6ul here with a Cypress S34ML04G2 connected. This Nand chip
> > > > has ONFI support, but onfi_set_features is unsupported (opt_cmd is
> > > > 0x3b). Currently the gpmi Nand driver calls nand_onfi_set_features(),
> > > > gets -EINVAL as error and continues with a very slow default timing.
> > > > 
> > > > I assume the nand_onfi_set_features() call is just unnecessary for this
> > > > chip, if I skip it, the chip works with the fast timing.
> > > > 
> > > > Any idea how to cope with this situation? I attached the most obvious
> > > > patch, but it looks a bit hackish. Any suggestions or is the patch fine
> > > > as is?  
> > > 
> > > It looks good to me. Why do you find this code hackish?
> > > Of course, it would be even better to implement the  
> > > ->setup_data_interface() method.  
> > 
> > Indeed, but my current project doesn't allow to spend that much time at
> > the moment.
> 
> Understood. Let's wait for Han's review.
> 
> > 
> > > 
> > > BTW, can you patch the core to only send the ->set_feature() command
> > > (to change the timings mode) when the chip supports it?  
> > 
> > With hackish I mean that I think the problem should be solved in the
> > core. How about returning -EOPNOTSUPP from onfi_set_features() when the
> > operation is not supported? The caller could then decide what to do
> > without testing for bits in the onfi param page.
> 
> The problem is, ->onfi_set_feature() is a method that can be overloaded
> by NAND controller drivers. Of course, we could add a wrapper around
> ->onfi_set_feature() (nand_set_feature()?),

Such a wrapper would have the advantage that we wouldn't have to repeat
the

	if (!chip->onfi_version ||
		    !(le16_to_cpu(chip->onfi_params.opt_cmd)
		    	      & ONFI_OPT_CMD_SET_GET_FEATURES))

test in each driver with a special onfi_get_features() implementation.

> but then, the meaning of
> -ENOTSUPP is not clear. It could be returned if the
> ->onfi_set_feature() is NULL or if the requested feature is not
> supported

->onfi_set_feature() will never be NULL as it's set to nand_onfi_set_features
if it's NULL in during registration. Also I would suggest -ENOSYS if the
driver is lacking support for an operation.

> 
> Another solution would be to add an extra helper to check if a specific
> feature is supported:
> 
> bool nand_feature_supported(nand, feature_id);

Yes, that would work aswell. Up to you to decide ;)

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: ONFI timing mode with onfi_set_features unsupported

[Boris Brezillon]

On Tue, 22 Nov 2016 12:18:54 +0100
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Tue, Nov 22, 2016 at 12:10:47PM +0100, Boris Brezillon wrote:
> > On Tue, 22 Nov 2016 12:02:27 +0100
> > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >   
> > > On Mon, Nov 21, 2016 at 02:51:04PM +0100, Boris Brezillon wrote:  
> > > > Hi Sascha,
> > > > 
> > > > On Mon, 21 Nov 2016 14:23:27 +0100
> > > > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > > >     
> > > > > Hi all,
> > > > > 
> > > > > I have a i.MX6ul here with a Cypress S34ML04G2 connected. This Nand chip
> > > > > has ONFI support, but onfi_set_features is unsupported (opt_cmd is
> > > > > 0x3b). Currently the gpmi Nand driver calls nand_onfi_set_features(),
> > > > > gets -EINVAL as error and continues with a very slow default timing.
> > > > > 
> > > > > I assume the nand_onfi_set_features() call is just unnecessary for this
> > > > > chip, if I skip it, the chip works with the fast timing.
> > > > > 
> > > > > Any idea how to cope with this situation? I attached the most obvious
> > > > > patch, but it looks a bit hackish. Any suggestions or is the patch fine
> > > > > as is?    
> > > > 
> > > > It looks good to me. Why do you find this code hackish?
> > > > Of course, it would be even better to implement the    
> > > > ->setup_data_interface() method.    
> > > 
> > > Indeed, but my current project doesn't allow to spend that much time at
> > > the moment.  
> > 
> > Understood. Let's wait for Han's review.
> >   
> > >   
> > > > 
> > > > BTW, can you patch the core to only send the ->set_feature() command
> > > > (to change the timings mode) when the chip supports it?    
> > > 
> > > With hackish I mean that I think the problem should be solved in the
> > > core. How about returning -EOPNOTSUPP from onfi_set_features() when the
> > > operation is not supported? The caller could then decide what to do
> > > without testing for bits in the onfi param page.  
> > 
> > The problem is, ->onfi_set_feature() is a method that can be overloaded
> > by NAND controller drivers. Of course, we could add a wrapper around  
> > ->onfi_set_feature() (nand_set_feature()?),  
> 
> Such a wrapper would have the advantage that we wouldn't have to repeat
> the
> 
> 	if (!chip->onfi_version ||
> 		    !(le16_to_cpu(chip->onfi_params.opt_cmd)
> 		    	      & ONFI_OPT_CMD_SET_GET_FEATURES))
> 
> test in each driver with a special onfi_get_features() implementation.

You're right.

> 
> > but then, the meaning of
> > -ENOTSUPP is not clear. It could be returned if the  
> > ->onfi_set_feature() is NULL or if the requested feature is not  
> > supported  
> 
> ->onfi_set_feature() will never be NULL as it's set to nand_onfi_set_features  
> if it's NULL in during registration.

You're right, that's actually one of the problem with calling
->onfi_set_feature() without having any guarantee that the underlying
->cmdfunc() will implement it properly. But that's another story.

> Also I would suggest -ENOSYS if the
> driver is lacking support for an operation.

ENOSYS is documented as "Invalid system call number". Not sure it is
appropriate to describe a missing operation.

> 
> > 
> > Another solution would be to add an extra helper to check if a specific
> > feature is supported:
> > 
> > bool nand_feature_supported(nand, feature_id);  
> 
> Yes, that would work aswell. Up to you to decide ;)

Let's go for the first proposal: nand_{get,set}_feature() wrappers
returning -ENOTSUP if ONFI_OPT_CMD_SET_GET_FEATURES is not set.
Note that even non-ONFI NANDs support the SET/GET_FEATURE commands, so
the test you suggested above is incorrect.

Ideally, we should have an extra bitmap marking features as supported or
not, and let NAND id detection code set these flags. But in the
meantime, we can just use your test with a comment saying that this
should be reworked if we want to support SET/GET_FEATURE on non-ONFI
NANDs.

Regards,

Boris