[02/13] mtd: nand: fsmc: rework fsmc_nand_setup() to use ->setup_data_interface()
diff mbox

Message ID 1490090645-8576-3-git-send-email-thomas.petazzoni@free-electrons.com
State Superseded
Delegated to: Boris Brezillon
Headers show

Commit Message

Thomas Petazzoni March 21, 2017, 10:03 a.m. UTC
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(-)

Comments

Boris Brezillon March 22, 2017, 9:56 p.m. UTC | #1
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;
>  }
Thomas Petazzoni March 22, 2017, 10:05 p.m. UTC | #2
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
Boris Brezillon March 22, 2017, 10:23 p.m. UTC | #3
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.
Thomas Petazzoni March 22, 2017, 10:39 p.m. UTC | #4
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
Boris Brezillon March 22, 2017, 10:53 p.m. UTC | #5
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.
Linus Walleij March 23, 2017, 9:57 a.m. UTC | #6
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

Patch
diff mbox

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;
 }