| Message ID | 20171222172853.27710-2-miquel.raynal@free-electrons.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Boris Brezillon |
| Headers | show |
| Series | Migrate the GPMI driver to use NAND core timings | expand |
On Fri, 22 Dec 2017 18:28:52 +0100 Miquel Raynal <miquel.raynal@free-electrons.com> wrote: > Choosing ONFI timings when ->onfi_set/get_features() calls are supported > by the NAND chip is a matter of reading the chip's ONFI parameter page > and telling the chip the chosen mode (between all of the supported ones) > with ->onfi_set_feature(). > > Add a check on whether the chip "acked" the timing mode or not. > > This can be a problem for NAND chips that do not follow entirely the > ONFI specification. These chips actually support other modes than > "mode 0", but do not update the parameter page once a timing mode has > been selected. This issue will be addressed in another patch that will > add the feature to overwrite NAND chips features within the parameter > page, from the NAND chip driver. > > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> > --- > drivers/mtd/nand/nand_base.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index 96c97588e1ba..ea862be6a803 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -1236,6 +1236,18 @@ static int nand_setup_data_interface(struct nand_chip *chip, int chipnr) > tmode_param); > if (ret) > goto err; > + > + ret = chip->onfi_get_features(mtd, chip, > + ONFI_FEATURE_ADDR_TIMING_MODE, > + tmode_param); > + if (ret) > + goto err; > + > + if (tmode_param[0] != chip->onfi_timing_mode_default) { > + pr_warn("timings mode %d not acknowledged by the NAND chip\n", > + chip->onfi_timing_mode_default); > + goto err; > + } Hm, I'm not sure this is safe. The spec says that new ONFI timing mode is applied as soon the CS line is released after a SET_FEATURES(ONFI_FEATURE_ADDR_TIMING_MODE), and since we have no guarantee that the CS will be kept low by the controller after ->onfi_set_features() returns we must assume the new mode has been applied and call ->setup_data_interface() to instruct the controller to apply new timings. If you want to check if the mode has really been applied, you should release the CS (->select_chip(-1)), re-acquire it (->select_chip(X)), and call ->onfi_get_features(ONFI_FEATURE_ADDR_TIMING_MODE). If it appears that the mode has not been applied, you should restore timing mode 0 and issue a RESET. > } > > ret = chip->setup_data_interface(mtd, chipnr, &chip->data_interface);
Hello, > Hm, I'm not sure this is safe. The spec says that new ONFI timing mode > is applied as soon the CS line is released after a > SET_FEATURES(ONFI_FEATURE_ADDR_TIMING_MODE), and since we have no > guarantee that the CS will be kept low by the controller after > ->onfi_set_features() returns we must assume the new mode has been > applied and call ->setup_data_interface() to instruct the controller > to apply new timings. > > If you want to check if the mode has really been applied, you should > release the CS (->select_chip(-1)), re-acquire it > (->select_chip(X)), and call > ->onfi_get_features(ONFI_FEATURE_ADDR_TIMING_MODE). If it appears > that the mode has not been applied, you should restore timing mode 0 > and issue a RESET. Boris, thanks for the comment, I will fix that. Han, could I have your input on this series? Aside Boris' comment of course. Thank you very much, Miquèl
On Fri, 5 Jan 2018 16:42:39 +0100 Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote: > Hello, > > > Hm, I'm not sure this is safe. The spec says that new ONFI timing mode > > is applied as soon the CS line is released after a > > SET_FEATURES(ONFI_FEATURE_ADDR_TIMING_MODE), and since we have no > > guarantee that the CS will be kept low by the controller after > > ->onfi_set_features() returns we must assume the new mode has been > > applied and call ->setup_data_interface() to instruct the controller > > to apply new timings. > > > > If you want to check if the mode has really been applied, you should > > release the CS (->select_chip(-1)), re-acquire it > > (->select_chip(X)), and call > > ->onfi_get_features(ONFI_FEATURE_ADDR_TIMING_MODE). If it appears > > that the mode has not been applied, you should restore timing mode 0 > > and issue a RESET. > > Boris, thanks for the comment, I will fix that. > > Han, could I have your input on this series? Aside Boris' comment of > course. Han, we really need your feedback on this series since you were the one complaining that ONFI mode should be checked back after applying a new mode. Miquel is reworking the framework to mimic what the GPMI driver, but we need to be sure that you'll accept to transition to the generic ->setup_data_interface() solution. Thanks, Boris
Hi Han, On Mon, 8 Jan 2018 14:04:29 +0100 Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Fri, 5 Jan 2018 16:42:39 +0100 > Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote: > > > Hello, > > > > > Hm, I'm not sure this is safe. The spec says that new ONFI timing mode > > > is applied as soon the CS line is released after a > > > SET_FEATURES(ONFI_FEATURE_ADDR_TIMING_MODE), and since we have no > > > guarantee that the CS will be kept low by the controller after > > > ->onfi_set_features() returns we must assume the new mode has been > > > applied and call ->setup_data_interface() to instruct the controller > > > to apply new timings. > > > > > > If you want to check if the mode has really been applied, you should > > > release the CS (->select_chip(-1)), re-acquire it > > > (->select_chip(X)), and call > > > ->onfi_get_features(ONFI_FEATURE_ADDR_TIMING_MODE). If it appears > > > that the mode has not been applied, you should restore timing mode 0 > > > and issue a RESET. > > > > Boris, thanks for the comment, I will fix that. > > > > Han, could I have your input on this series? Aside Boris' comment of > > course. > > Han, we really need your feedback on this series since you were the one > complaining that ONFI mode should be checked back after applying a new > mode. Miquel is reworking the framework to mimic what the GPMI driver, > but we need to be sure that you'll accept to transition to the generic > ->setup_data_interface() solution. Gentle ping.
On Mon, 15 Jan 2018 17:57:08 +0000 Han Xu <han.xu@nxp.com> wrote: > ________________________________________ > From: Boris Brezillon <boris.brezillon@free-electrons.com> > Sent: Monday, January 15, 2018 7:19 AM > To: Han Xu > Cc: Miquel RAYNAL; Richard Weinberger; David Woodhouse; Brian Norris; Marek Vasut; Cyrille Pitchen; linux-mtd@lists.infradead.org > Subject: Re: [PATCH 1/2] mtd: nand: Check ONFI timings have been acked by the chip > > Hi Han, > > On Mon, 8 Jan 2018 14:04:29 +0100 > Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > > > On Fri, 5 Jan 2018 16:42:39 +0100 > > Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote: > > > > > Hello, > > > > > > > Hm, I'm not sure this is safe. The spec says that new ONFI timing mode > > > > is applied as soon the CS line is released after a > > > > SET_FEATURES(ONFI_FEATURE_ADDR_TIMING_MODE), and since we have no > > > > guarantee that the CS will be kept low by the controller after > > > > ->onfi_set_features() returns we must assume the new mode has been > > > > applied and call ->setup_data_interface() to instruct the controller > > > > to apply new timings. > > > > > > > > If you want to check if the mode has really been applied, you should > > > > release the CS (->select_chip(-1)), re-acquire it > > > > (->select_chip(X)), and call > > > > ->onfi_get_features(ONFI_FEATURE_ADDR_TIMING_MODE). If it appears > > > > that the mode has not been applied, you should restore timing mode 0 > > > > and issue a RESET. > > > > > > Boris, thanks for the comment, I will fix that. > > > > > > Han, could I have your input on this series? Aside Boris' comment of > > > course. > > > > Han, we really need your feedback on this series since you were the one > > complaining that ONFI mode should be checked back after applying a new > > mode. Miquel is reworking the framework to mimic what the GPMI driver, > > but we need to be sure that you'll accept to transition to the generic > > ->setup_data_interface() solution. > > Thanks Miquel, I do accept to transit to setup_data_interface solution and actually > I am also working on the similar patches. I reviewed the patch set and don't have > more comments, this one improved more than mine. Glad to hear you're not opposed to the idea.
Hi Han, On Mon, 15 Jan 2018 19:41:08 +0100 Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Mon, 15 Jan 2018 17:57:08 +0000 > Han Xu <han.xu@nxp.com> wrote: > > > ________________________________________ > > From: Boris Brezillon <boris.brezillon@free-electrons.com> > > Sent: Monday, January 15, 2018 7:19 AM > > To: Han Xu > > Cc: Miquel RAYNAL; Richard Weinberger; David Woodhouse; Brian > > Norris; Marek Vasut; Cyrille Pitchen; linux-mtd@lists.infradead.org > > Subject: Re: [PATCH 1/2] mtd: nand: Check ONFI timings have been > > acked by the chip > > > > Hi Han, > > > > On Mon, 8 Jan 2018 14:04:29 +0100 > > Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > > > > > On Fri, 5 Jan 2018 16:42:39 +0100 > > > Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote: > > > > > > > Hello, > > > > > > > > > Hm, I'm not sure this is safe. The spec says that new ONFI > > > > > timing mode is applied as soon the CS line is released after a > > > > > SET_FEATURES(ONFI_FEATURE_ADDR_TIMING_MODE), and since we > > > > > have no guarantee that the CS will be kept low by the > > > > > controller after ->onfi_set_features() returns we must assume > > > > > the new mode has been applied and call > > > > > ->setup_data_interface() to instruct the controller to apply > > > > > new timings. > > > > > > > > > > If you want to check if the mode has really been applied, you > > > > > should release the CS (->select_chip(-1)), re-acquire it > > > > > (->select_chip(X)), and call > > > > > ->onfi_get_features(ONFI_FEATURE_ADDR_TIMING_MODE). If it > > > > > appears that the mode has not been applied, you should > > > > > restore timing mode 0 and issue a RESET. > > > > > > > > Boris, thanks for the comment, I will fix that. > > > > > > > > Han, could I have your input on this series? Aside Boris' > > > > comment of course. > > > > > > Han, we really need your feedback on this series since you were > > > the one complaining that ONFI mode should be checked back after > > > applying a new mode. Miquel is reworking the framework to mimic > > > what the GPMI driver, but we need to be sure that you'll accept > > > to transition to the generic ->setup_data_interface() > > > solution. > > > > Thanks Miquel, I do accept to transit to setup_data_interface > > solution and actually I am also working on the similar patches. I > > reviewed the patch set and don't have more comments, this one > > improved more than mine. > > Glad to hear you're not opposed to the idea. I'm happy to read this, I'll send a v2 tomorrow that addresses Boris' comments. Thanks, Miquèl
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 96c97588e1ba..ea862be6a803 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -1236,6 +1236,18 @@ static int nand_setup_data_interface(struct nand_chip *chip, int chipnr) tmode_param); if (ret) goto err; + + ret = chip->onfi_get_features(mtd, chip, + ONFI_FEATURE_ADDR_TIMING_MODE, + tmode_param); + if (ret) + goto err; + + if (tmode_param[0] != chip->onfi_timing_mode_default) { + pr_warn("timings mode %d not acknowledged by the NAND chip\n", + chip->onfi_timing_mode_default); + goto err; + } } ret = chip->setup_data_interface(mtd, chipnr, &chip->data_interface);
Choosing ONFI timings when ->onfi_set/get_features() calls are supported by the NAND chip is a matter of reading the chip's ONFI parameter page and telling the chip the chosen mode (between all of the supported ones) with ->onfi_set_feature(). Add a check on whether the chip "acked" the timing mode or not. This can be a problem for NAND chips that do not follow entirely the ONFI specification. These chips actually support other modes than "mode 0", but do not update the parameter page once a timing mode has been selected. This issue will be addressed in another patch that will add the feature to overwrite NAND chips features within the parameter page, from the NAND chip driver. Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com> --- drivers/mtd/nand/nand_base.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)