| Message ID | 20161121132327.hup6bi7zqu54jsia@pengutronix.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Boris Brezillon |
| Headers | show |
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;
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
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);
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
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
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;