| Message ID | 1490090645-8576-3-git-send-email-thomas.petazzoni@free-electrons.com |
|---|---|
| State | Superseded |
| Delegated to: | Boris Brezillon |
| Headers | show |
On Tue, 21 Mar 2017 11:03:54 +0100 Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Since a few kernel versions, the NAND subsystem has introduced a > callback called ->setup_data_interface(), which NAND controller drivers > are supposed to use to perform all the NAND data interface. > > This commit therefore converts the fsmc_nand driver to use this callback > instead of the home-grown fsmc_nand_setup() function. > > For now, the NAND timings are not used, and we simply use the timings > provided from the Device Tree, or otherwise the default timings. Support > for the NAND timings is added in a follow-up patch. > > The call to fsmc_nand_setup() in the ->resume() hook is simply replaced > by a call to nand_reset(), which internally will call the > ->setup_data_interface() callback. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > drivers/mtd/nand/fsmc_nand.c | 31 +++++++++++++++++++------------ > 1 file changed, 19 insertions(+), 12 deletions(-) > > diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c > index 66aece9..4a98433 100644 > --- a/drivers/mtd/nand/fsmc_nand.c > +++ b/drivers/mtd/nand/fsmc_nand.c > @@ -370,9 +370,12 @@ static void fsmc_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl) > * This routine initializes timing parameters related to NAND memory access in > * FSMC registers > */ > -static void fsmc_nand_setup(void __iomem *regs, uint32_t bank, > - uint32_t busw, struct fsmc_nand_timings *timings) > +static int fsmc_setup_data_interface(struct mtd_info *mtd, > + const struct nand_data_interface *conf, > + bool check_only) > { > + struct nand_chip *nand = mtd_to_nand(mtd); > + struct fsmc_nand_data *host = nand_get_controller_data(nand); > uint32_t value = FSMC_DEVTYPE_NAND | FSMC_ENABLE | FSMC_WAITON; > uint32_t tclr, tar, thiz, thold, twait, tset; > struct fsmc_nand_timings *tims; > @@ -384,12 +387,18 @@ static void fsmc_nand_setup(void __iomem *regs, uint32_t bank, > .twait = FSMC_TWAIT_6, > .tset = FSMC_TSET_0, > }; > + unsigned int bank = host->bank; > + void __iomem *regs = host->regs_va; > + int ret; > > - if (timings) > - tims = timings; > + if (host->dev_timings) > + tims = host->dev_timings; > else > tims = &default_timings; > > + if (check_only) > + return 0; > + I'm not sure this is such a good idea to move default and DT timings setting in the ->setup_data_interface() hook. ONFI NANDs are changing an internal parameter to switch to a specific timing mode. If you let the core think that you configured the controller to support this timing mode, while you actually configured it with the default or DT timings it might not work as expected. > tclr = (tims->tclr & FSMC_TCLR_MASK) << FSMC_TCLR_SHIFT; > tar = (tims->tar & FSMC_TAR_MASK) << FSMC_TAR_SHIFT; > thiz = (tims->thiz & FSMC_THIZ_MASK) << FSMC_THIZ_SHIFT; > @@ -397,7 +406,7 @@ static void fsmc_nand_setup(void __iomem *regs, uint32_t bank, > twait = (tims->twait & FSMC_TWAIT_MASK) << FSMC_TWAIT_SHIFT; > tset = (tims->tset & FSMC_TSET_MASK) << FSMC_TSET_SHIFT; > > - if (busw) > + if (nand->options & NAND_BUSWIDTH_16) > writel_relaxed(value | FSMC_DEVWID_16, > FSMC_NAND_REG(regs, bank, PC)); > else > @@ -410,6 +419,8 @@ static void fsmc_nand_setup(void __iomem *regs, uint32_t bank, > FSMC_NAND_REG(regs, bank, COMM)); > writel_relaxed(thiz | thold | twait | tset, > FSMC_NAND_REG(regs, bank, ATTRIB)); > + > + return 0; > } > > /* > @@ -991,6 +1002,7 @@ static int __init fsmc_nand_probe(struct platform_device *pdev) > nand->select_chip = fsmc_select_chip; > nand->badblockbits = 7; > nand_set_flash_node(nand, np); > + nand->setup_data_interface = fsmc_setup_data_interface; > > switch (host->mode) { > case USE_DMA_ACCESS: > @@ -1019,10 +1031,6 @@ static int __init fsmc_nand_probe(struct platform_device *pdev) > break; > } > > - fsmc_nand_setup(host->regs_va, host->bank, > - nand->options & NAND_BUSWIDTH_16, > - host->dev_timings); > - > if (AMBA_REV_BITS(host->pid) >= 8) { > nand->ecc.read_page = fsmc_read_page_hwecc; > nand->ecc.calculate = fsmc_read_hwecc_ecc4; > @@ -1121,6 +1129,7 @@ static int __init fsmc_nand_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, host); > dev_info(&pdev->dev, "FSMC NAND driver registration successful\n"); > + > return 0; > > err_probe: > @@ -1172,9 +1181,7 @@ static int fsmc_nand_resume(struct device *dev) > struct fsmc_nand_data *host = dev_get_drvdata(dev); > if (host) { > clk_prepare_enable(host->clk); > - fsmc_nand_setup(host->regs_va, host->bank, > - host->nand.options & NAND_BUSWIDTH_16, > - host->dev_timings); > + nand_reset(&host->nand, 0); > } > return 0; > }
Hello, On Wed, 22 Mar 2017 22:56:17 +0100, Boris Brezillon wrote: > I'm not sure this is such a good idea to move default and DT timings > setting in the ->setup_data_interface() hook. > > ONFI NANDs are changing an internal parameter to switch to a specific > timing mode. If you let the core think that you configured the > controller to support this timing mode, while you actually configured > it with the default or DT timings it might not work as expected. So what do you suggest to keep the compatibility with the existing DT binding for this NAND controller? We also need to take into account that the timings need to be reconfigured upon ->resume(). Thanks! Thomas
On Wed, 22 Mar 2017 23:05:53 +0100 Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Hello, > > On Wed, 22 Mar 2017 22:56:17 +0100, Boris Brezillon wrote: > > > I'm not sure this is such a good idea to move default and DT timings > > setting in the ->setup_data_interface() hook. > > > > ONFI NANDs are changing an internal parameter to switch to a specific > > timing mode. If you let the core think that you configured the > > controller to support this timing mode, while you actually configured > > it with the default or DT timings it might not work as expected. > > So what do you suggest to keep the compatibility with the existing DT > binding for this NAND controller? We have 2 choices here: 1/ drop the old/static timings config and only rely on the dynamic config. This is a bit risky, because we've only tested the new timing conversion logic on one board. 2/ keep both solutions around until all boards have switched to the new solution. In this case, you'll only set the ->setup_data_interface() hook if timings are not defined in the DT > > We also need to take into account that the timings need to be > reconfigured upon ->resume(). Yes, I know, and I also know how painful it is to add extra code for backward compat, but I think abusing ->setup_data_interface() is even worse. To sum-up, if we go for solution #2, in your resume you'll have to conditionally call the legacy timing setup logic (only if timings are defined in the DT) in addition to the nand_reset() call.
Hello, On Wed, 22 Mar 2017 23:23:33 +0100, Boris Brezillon wrote: > > So what do you suggest to keep the compatibility with the existing DT > > binding for this NAND controller? > > We have 2 choices here: > 1/ drop the old/static timings config and only rely on the dynamic > config. This is a bit risky, because we've only tested the new > timing conversion logic on one board. > 2/ keep both solutions around until all boards have switched to the new > solution. In this case, you'll only set the ->setup_data_interface() > hook if timings are not defined in the DT What about the "default" timings that are currently hardcoded in the driver, if no timings are specified in the DT? My patch currently does the following: - Use DT timings if provided - Use NAND timings if provided - Use "default" timing otherwise Should we drop the "default" timing thing? If not, how does it fit in your proposal? Indeed your proposal is simply: if no timings in DT, use the NAND timings. But my patch does more than that. Thanks, Thomas
On Wed, 22 Mar 2017 23:39:15 +0100 Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Hello, > > On Wed, 22 Mar 2017 23:23:33 +0100, Boris Brezillon wrote: > > > > So what do you suggest to keep the compatibility with the existing DT > > > binding for this NAND controller? > > > > We have 2 choices here: > > 1/ drop the old/static timings config and only rely on the dynamic > > config. This is a bit risky, because we've only tested the new > > timing conversion logic on one board. > > 2/ keep both solutions around until all boards have switched to the new > > solution. In this case, you'll only set the ->setup_data_interface() > > hook if timings are not defined in the DT > > What about the "default" timings that are currently hardcoded in the > driver, if no timings are specified in the DT? My patch currently does > the following: > > - Use DT timings if provided > - Use NAND timings if provided > - Use "default" timing otherwise Okay, I didn't notice the 'fallback to "default" timings' in patch 3. This portion of code will actually never be reached, because the core always provide valid SDR timings. If the NAND does not support ONFI timing modes and nothing is specified in the NAND ids table (or the in vendor specific init code), then the lowest timing mode is assumed (timing mode 0), and if you implement ->setup_data_interface(), this is what you will end-up with. > > Should we drop the "default" timing thing? I think so. > If not, how does it fit in > your proposal? Indeed your proposal is simply: if no timings in DT, use > the NAND timings. But my patch does more than that. If we really want to keep "default" timmings around, then we'll have to add an extra Kconfig option, a kernel parameter (or a DT property :-)), but I'm not a big fan of these solutions.
On Tue, Mar 21, 2017 at 11:03 AM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Since a few kernel versions, the NAND subsystem has introduced a > callback called ->setup_data_interface(), which NAND controller drivers > are supposed to use to perform all the NAND data interface. > > This commit therefore converts the fsmc_nand driver to use this callback > instead of the home-grown fsmc_nand_setup() function. > > For now, the NAND timings are not used, and we simply use the timings > provided from the Device Tree, or otherwise the default timings. Support > for the NAND timings is added in a follow-up patch. > > The call to fsmc_nand_setup() in the ->resume() hook is simply replaced > by a call to nand_reset(), which internally will call the > ->setup_data_interface() callback. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Looks good to me, whether you go with Boris' suggestion or not. Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c index 66aece9..4a98433 100644 --- a/drivers/mtd/nand/fsmc_nand.c +++ b/drivers/mtd/nand/fsmc_nand.c @@ -370,9 +370,12 @@ static void fsmc_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl) * This routine initializes timing parameters related to NAND memory access in * FSMC registers */ -static void fsmc_nand_setup(void __iomem *regs, uint32_t bank, - uint32_t busw, struct fsmc_nand_timings *timings) +static int fsmc_setup_data_interface(struct mtd_info *mtd, + const struct nand_data_interface *conf, + bool check_only) { + struct nand_chip *nand = mtd_to_nand(mtd); + struct fsmc_nand_data *host = nand_get_controller_data(nand); uint32_t value = FSMC_DEVTYPE_NAND | FSMC_ENABLE | FSMC_WAITON; uint32_t tclr, tar, thiz, thold, twait, tset; struct fsmc_nand_timings *tims; @@ -384,12 +387,18 @@ static void fsmc_nand_setup(void __iomem *regs, uint32_t bank, .twait = FSMC_TWAIT_6, .tset = FSMC_TSET_0, }; + unsigned int bank = host->bank; + void __iomem *regs = host->regs_va; + int ret; - if (timings) - tims = timings; + if (host->dev_timings) + tims = host->dev_timings; else tims = &default_timings; + if (check_only) + return 0; + tclr = (tims->tclr & FSMC_TCLR_MASK) << FSMC_TCLR_SHIFT; tar = (tims->tar & FSMC_TAR_MASK) << FSMC_TAR_SHIFT; thiz = (tims->thiz & FSMC_THIZ_MASK) << FSMC_THIZ_SHIFT; @@ -397,7 +406,7 @@ static void fsmc_nand_setup(void __iomem *regs, uint32_t bank, twait = (tims->twait & FSMC_TWAIT_MASK) << FSMC_TWAIT_SHIFT; tset = (tims->tset & FSMC_TSET_MASK) << FSMC_TSET_SHIFT; - if (busw) + if (nand->options & NAND_BUSWIDTH_16) writel_relaxed(value | FSMC_DEVWID_16, FSMC_NAND_REG(regs, bank, PC)); else @@ -410,6 +419,8 @@ static void fsmc_nand_setup(void __iomem *regs, uint32_t bank, FSMC_NAND_REG(regs, bank, COMM)); writel_relaxed(thiz | thold | twait | tset, FSMC_NAND_REG(regs, bank, ATTRIB)); + + return 0; } /* @@ -991,6 +1002,7 @@ static int __init fsmc_nand_probe(struct platform_device *pdev) nand->select_chip = fsmc_select_chip; nand->badblockbits = 7; nand_set_flash_node(nand, np); + nand->setup_data_interface = fsmc_setup_data_interface; switch (host->mode) { case USE_DMA_ACCESS: @@ -1019,10 +1031,6 @@ static int __init fsmc_nand_probe(struct platform_device *pdev) break; } - fsmc_nand_setup(host->regs_va, host->bank, - nand->options & NAND_BUSWIDTH_16, - host->dev_timings); - if (AMBA_REV_BITS(host->pid) >= 8) { nand->ecc.read_page = fsmc_read_page_hwecc; nand->ecc.calculate = fsmc_read_hwecc_ecc4; @@ -1121,6 +1129,7 @@ static int __init fsmc_nand_probe(struct platform_device *pdev) platform_set_drvdata(pdev, host); dev_info(&pdev->dev, "FSMC NAND driver registration successful\n"); + return 0; err_probe: @@ -1172,9 +1181,7 @@ static int fsmc_nand_resume(struct device *dev) struct fsmc_nand_data *host = dev_get_drvdata(dev); if (host) { clk_prepare_enable(host->clk); - fsmc_nand_setup(host->regs_va, host->bank, - host->nand.options & NAND_BUSWIDTH_16, - host->dev_timings); + nand_reset(&host->nand, 0); } return 0; }
Since a few kernel versions, the NAND subsystem has introduced a callback called ->setup_data_interface(), which NAND controller drivers are supposed to use to perform all the NAND data interface. This commit therefore converts the fsmc_nand driver to use this callback instead of the home-grown fsmc_nand_setup() function. For now, the NAND timings are not used, and we simply use the timings provided from the Device Tree, or otherwise the default timings. Support for the NAND timings is added in a follow-up patch. The call to fsmc_nand_setup() in the ->resume() hook is simply replaced by a call to nand_reset(), which internally will call the ->setup_data_interface() callback. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- drivers/mtd/nand/fsmc_nand.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-)