diff mbox series

mtd: rawnand: fsmc: Keep bank enable bit set

Message ID 20190109215144.15749-1-linus.walleij@linaro.org
State New
Headers show
Series mtd: rawnand: fsmc: Keep bank enable bit set | expand

Commit Message

Linus Walleij Jan. 9, 2019, 9:51 p.m. UTC 
Hammering the "bank enable" (PBKEN) bit on and off between
every command crashes the Nomadik NHK15 with this message:

Scanning device for bad blocks
Unhandled fault: external abort on non-linefetch (0x008) at 0xcc95e000
pgd = (ptrval)
[cc95e000] *pgd=0b808811, *pte=40000653, *ppte=40000552
Internal error: : 8 [#1] PREEMPT ARM
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 4.20.0-rc2+ #72
Hardware name: Nomadik STn8815
PC is at fsmc_exec_op+0x194/0x204
(...)

After a discussion we (me and Boris Brezillion) start to suspect
that this bit does not immediately control the chip select line
at all, it rather enables access to the bank and the hardware
will drive the CS autonomously. If there is a NAND chip connected,
we should keep this enabled.

As fsmc_nand_setup() sets this bit, we can simply remove the
offending code.

Fixes: 550b9fc4e3af ("mtd: rawnand: fsmc: Stop implementing ->select_chip()")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
 drivers/mtd/nand/raw/fsmc_nand.c | 21 ---------------------
 1 file changed, 21 deletions(-)

-- 
2.19.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Comments

Boris Brezillon Jan. 9, 2019, 10:12 p.m. UTC | #1 
On Wed,  9 Jan 2019 22:51:44 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> Hammering the "bank enable" (PBKEN) bit on and off between

> every command crashes the Nomadik NHK15 with this message:

> 

> Scanning device for bad blocks

> Unhandled fault: external abort on non-linefetch (0x008) at 0xcc95e000

> pgd = (ptrval)

> [cc95e000] *pgd=0b808811, *pte=40000653, *ppte=40000552

> Internal error: : 8 [#1] PREEMPT ARM

> Modules linked in:

> CPU: 0 PID: 1 Comm: swapper Not tainted 4.20.0-rc2+ #72

> Hardware name: Nomadik STn8815

> PC is at fsmc_exec_op+0x194/0x204

> (...)

> 

> After a discussion we (me and Boris Brezillion) start to suspect


				      ^ Brezillon :-)

> that this bit does not immediately control the chip select line

> at all, it rather enables access to the bank and the hardware

> will drive the CS autonomously. If there is a NAND chip connected,

> we should keep this enabled.

> 

> As fsmc_nand_setup() sets this bit, we can simply remove the

> offending code.

> 

> Fixes: 550b9fc4e3af ("mtd: rawnand: fsmc: Stop implementing ->select_chip()")

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>


Reviewed-by: Boris Brezillon <bbrezillon@kernel.org>


Would be great if someone could validate our assumption with a scope.
This being said, given the description of the FSMC logic, I have little
doubt that this bit does not directly controls the CE line, otherwise
concurrent accesses to different memories on the same bus wouldn't work
or would require a lot more synchronization than we currently have in
Linux.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Miquel Raynal Jan. 10, 2019, 8:42 a.m. UTC | #2 
Hi Linus,

Please don't forget to Cc: me when it comes to NAND-related changes :)

Boris Brezillon <bbrezillon@kernel.org> wrote on Wed, 9 Jan 2019
23:12:59 +0100:

> On Wed,  9 Jan 2019 22:51:44 +0100
> Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> > Hammering the "bank enable" (PBKEN) bit on and off between
> > every command crashes the Nomadik NHK15 with this message:
> > 
> > Scanning device for bad blocks
> > Unhandled fault: external abort on non-linefetch (0x008) at 0xcc95e000
> > pgd = (ptrval)
> > [cc95e000] *pgd=0b808811, *pte=40000653, *ppte=40000552
> > Internal error: : 8 [#1] PREEMPT ARM
> > Modules linked in:
> > CPU: 0 PID: 1 Comm: swapper Not tainted 4.20.0-rc2+ #72
> > Hardware name: Nomadik STn8815
> > PC is at fsmc_exec_op+0x194/0x204
> > (...)
> > 
> > After a discussion we (me and Boris Brezillion) start to suspect  
> 
> 				      ^ Brezillon :-)
> 
> > that this bit does not immediately control the chip select line
> > at all, it rather enables access to the bank and the hardware
> > will drive the CS autonomously. If there is a NAND chip connected,
> > we should keep this enabled.
> > 
> > As fsmc_nand_setup() sets this bit, we can simply remove the
> > offending code.
> > 
> > Fixes: 550b9fc4e3af ("mtd: rawnand: fsmc: Stop implementing ->select_chip()")
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>  
> 
> Reviewed-by: Boris Brezillon <bbrezillon@kernel.org>
> 
> Would be great if someone could validate our assumption with a scope.
> This being said, given the description of the FSMC logic, I have little
> doubt that this bit does not directly controls the CE line, otherwise
> concurrent accesses to different memories on the same bus wouldn't work
> or would require a lot more synchronization than we currently have in
> Linux.
> 

Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>


Thanks,
Miquèl
Boris Brezillon Jan. 14, 2019, 8:51 a.m. UTC | #3 
Hi Linus,

On Wed,  9 Jan 2019 22:51:44 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> Hammering the "bank enable" (PBKEN) bit on and off between

> every command crashes the Nomadik NHK15 with this message:

> 

> Scanning device for bad blocks

> Unhandled fault: external abort on non-linefetch (0x008) at 0xcc95e000

> pgd = (ptrval)

> [cc95e000] *pgd=0b808811, *pte=40000653, *ppte=40000552

> Internal error: : 8 [#1] PREEMPT ARM

> Modules linked in:

> CPU: 0 PID: 1 Comm: swapper Not tainted 4.20.0-rc2+ #72

> Hardware name: Nomadik STn8815

> PC is at fsmc_exec_op+0x194/0x204

> (...)

> 

> After a discussion we (me and Boris Brezillion) start to suspect

> that this bit does not immediately control the chip select line

> at all, it rather enables access to the bank and the hardware

> will drive the CS autonomously. If there is a NAND chip connected,

> we should keep this enabled.

> 

> As fsmc_nand_setup() sets this bit, we can simply remove the

> offending code.

> 

> Fixes: 550b9fc4e3af ("mtd: rawnand: fsmc: Stop implementing ->select_chip()")

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

>  drivers/mtd/nand/raw/fsmc_nand.c | 21 ---------------------

>  1 file changed, 21 deletions(-)

> 

> diff --git a/drivers/mtd/nand/raw/fsmc_nand.c b/drivers/mtd/nand/raw/fsmc_nand.c

> index 325b4414dccc..c9149a37f8f0 100644

> --- a/drivers/mtd/nand/raw/fsmc_nand.c

> +++ b/drivers/mtd/nand/raw/fsmc_nand.c

> @@ -593,23 +593,6 @@ static void fsmc_write_buf_dma(struct fsmc_nand_data *host, const u8 *buf,

>  	dma_xfer(host, (void *)buf, len, DMA_TO_DEVICE);

>  }

>  

> -/* fsmc_select_chip - assert or deassert nCE */

> -static void fsmc_ce_ctrl(struct fsmc_nand_data *host, bool assert)

> -{

> -	u32 pc = readl(host->regs_va + FSMC_PC);

> -

> -	if (!assert)

> -		writel_relaxed(pc & ~FSMC_ENABLE, host->regs_va + FSMC_PC);

> -	else

> -		writel_relaxed(pc | FSMC_ENABLE, host->regs_va + FSMC_PC);

> -

> -	/*

> -	 * nCE line changes must be applied before returning from this

> -	 * function.

> -	 */

> -	mb();

> -}

> -

>  /*

>   * fsmc_exec_op - hook called by the core to execute NAND operations

>   *

> @@ -627,8 +610,6 @@ static int fsmc_exec_op(struct nand_chip *chip, const struct nand_operation *op,

>  

>  	pr_debug("Executing operation [%d instructions]:\n", op->ninstrs);

>  

> -	fsmc_ce_ctrl(host, true);

> -

>  	for (op_id = 0; op_id < op->ninstrs; op_id++) {

>  		instr = &op->instrs[op_id];

>  

> @@ -686,8 +667,6 @@ static int fsmc_exec_op(struct nand_chip *chip, const struct nand_operation *op,

>  		}

>  	}

>  

> -	fsmc_ce_ctrl(host, false);

> -

>  	return ret;

>  }

>  


Not related to this patch, but I think we're missing a nand_reset()
call in the ->resume() path. Without it FSMC timings might be wrong
after a suspend if they're not defined in the DT. Would you mind sending
another patch to fix that, and maybe an extra patch to clear
FSMC_ENABLE in the ->remove() path and the ->probe()'s error path as
you suggested?

Thanks,

Boris

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Linus Walleij Jan. 17, 2019, 9:33 p.m. UTC | #4 
On Mon, Jan 14, 2019 at 9:51 AM Boris Brezillon <bbrezillon@kernel.org> wrote:

> Not related to this patch, but I think we're missing a nand_reset()

> call in the ->resume() path. Without it FSMC timings might be wrong

> after a suspend if they're not defined in the DT. Would you mind sending

> another patch to fix that,


I looked into it, It looks like this:

static int fsmc_nand_resume(struct device *dev)
{
    struct fsmc_nand_data *host = dev_get_drvdata(dev);

    if (host) {
        clk_prepare_enable(host->clk);
        if (host->dev_timings)
            fsmc_nand_setup(host, host->dev_timings);
    }

    return 0;
}

fsmc_nand_setup() will set up the timings if we have any.

> and maybe an extra patch to clear

> FSMC_ENABLE in the ->remove() path and the ->probe()'s error path as

> you suggested?


That I can fix. I will resend this patch with that as another patch.

Yours,
Linus Walleij

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Boris Brezillon Jan. 18, 2019, 6:36 a.m. UTC | #5 
On Thu, 17 Jan 2019 22:33:01 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Mon, Jan 14, 2019 at 9:51 AM Boris Brezillon <bbrezillon@kernel.org> wrote:

> 

> > Not related to this patch, but I think we're missing a nand_reset()

> > call in the ->resume() path. Without it FSMC timings might be wrong

> > after a suspend if they're not defined in the DT. Would you mind sending

> > another patch to fix that,  

> 

> I looked into it, It looks like this:

> 

> static int fsmc_nand_resume(struct device *dev)

> {

>     struct fsmc_nand_data *host = dev_get_drvdata(dev);

> 

>     if (host) {

>         clk_prepare_enable(host->clk);

>         if (host->dev_timings)

>             fsmc_nand_setup(host, host->dev_timings);


->dev_timings is only allocated if timings are defined in the DT, but
the driver can also let the framework configure the timings through the
->setup_data_interface() method (approach that should be preferred for
all new boards). In this case ->dev_timings is NULL and we need to
reset the chip to re-apply the timings.

BTW, resetting the NAND chip at resume time is a sane thing to do if
you want to start in known state, so I'd recommend adding

	nand_reset(&host->nand);

here.

>     }

> 

>     return 0;

> }

> 


Regards,

Boris

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
Boris Brezillon Jan. 18, 2019, 11:44 a.m. UTC | #6 
On Wed, 2019-01-09 at 21:51:44 UTC, Linus Walleij wrote:
> Hammering the "bank enable" (PBKEN) bit on and off between

> every command crashes the Nomadik NHK15 with this message:

> 

> Scanning device for bad blocks

> Unhandled fault: external abort on non-linefetch (0x008) at 0xcc95e000

> pgd = (ptrval)

> [cc95e000] *pgd=0b808811, *pte=40000653, *ppte=40000552

> Internal error: : 8 [#1] PREEMPT ARM

> Modules linked in:

> CPU: 0 PID: 1 Comm: swapper Not tainted 4.20.0-rc2+ #72

> Hardware name: Nomadik STn8815

> PC is at fsmc_exec_op+0x194/0x204

> (...)

> 

> After a discussion we (me and Boris Brezillion) start to suspect

> that this bit does not immediately control the chip select line

> at all, it rather enables access to the bank and the hardware

> will drive the CS autonomously. If there is a NAND chip connected,

> we should keep this enabled.

> 

> As fsmc_nand_setup() sets this bit, we can simply remove the

> offending code.

> 

> Fixes: 550b9fc4e3af ("mtd: rawnand: fsmc: Stop implementing ->select_chip()")

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> Reviewed-by: Boris Brezillon <bbrezillon@kernel.org>

> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>


Applied to http://git.infradead.org/linux-mtd.git mtd/fixes, thanks.

Boris

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
diff mbox series

Patch

diff --git a/drivers/mtd/nand/raw/fsmc_nand.c b/drivers/mtd/nand/raw/fsmc_nand.c
index 325b4414dccc..c9149a37f8f0 100644
--- a/drivers/mtd/nand/raw/fsmc_nand.c
+++ b/drivers/mtd/nand/raw/fsmc_nand.c
@@ -593,23 +593,6 @@  static void fsmc_write_buf_dma(struct fsmc_nand_data *host, const u8 *buf,
 	dma_xfer(host, (void *)buf, len, DMA_TO_DEVICE);
 }
 
-/* fsmc_select_chip - assert or deassert nCE */
-static void fsmc_ce_ctrl(struct fsmc_nand_data *host, bool assert)
-{
-	u32 pc = readl(host->regs_va + FSMC_PC);
-
-	if (!assert)
-		writel_relaxed(pc & ~FSMC_ENABLE, host->regs_va + FSMC_PC);
-	else
-		writel_relaxed(pc | FSMC_ENABLE, host->regs_va + FSMC_PC);
-
-	/*
-	 * nCE line changes must be applied before returning from this
-	 * function.
-	 */
-	mb();
-}
-
 /*
  * fsmc_exec_op - hook called by the core to execute NAND operations
  *
@@ -627,8 +610,6 @@  static int fsmc_exec_op(struct nand_chip *chip, const struct nand_operation *op,
 
 	pr_debug("Executing operation [%d instructions]:\n", op->ninstrs);
 
-	fsmc_ce_ctrl(host, true);
-
 	for (op_id = 0; op_id < op->ninstrs; op_id++) {
 		instr = &op->instrs[op_id];
 
@@ -686,8 +667,6 @@  static int fsmc_exec_op(struct nand_chip *chip, const struct nand_operation *op,
 		}
 	}
 
-	fsmc_ce_ctrl(host, false);
-
 	return ret;
 }