[PATCH] mtd: rawnand: fsmc: Keep CE enabled fix mb() drain

[PATCH] mtd: rawnand: fsmc: Keep CE enabled fix mb() drain

[Linus Walleij]

Hammering the chip enable 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
(...)

This patch keeps the CE (chip enable, the only chip select)
signal from the FSMC block enabled from the first command
after probe() or resume() until the driver either suspend()
or remove(). Create a state variable to track this.

Runtime PM can possibly be used on top of this approach if
we want to disable the CE line when no flash read/write is
going on for a prolonged time, but for now it stabilizes
the platform by simply keeping CE asserted as it used to
be.

While we're at it also fix the mb() memory barrier drain
order (patch folded in from Boris Brezillion).

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 | 41 ++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/nand/raw/fsmc_nand.c b/drivers/mtd/nand/raw/fsmc_nand.c
index 325b4414dccc..442fda905489 100644
--- a/drivers/mtd/nand/raw/fsmc_nand.c
+++ b/drivers/mtd/nand/raw/fsmc_nand.c
@@ -118,6 +118,7 @@ enum access_mode {
  * @dev:		Parent device
  * @mode:		Access mode
  * @clk:		Clock structure for FSMC.
+ * @ce_asserted:	CE (chip enable) is asserted
  *
  * @read_dma_chan:	DMA channel for read access
  * @write_dma_chan:	DMA channel for write access to NAND
@@ -140,6 +141,7 @@ struct fsmc_nand_data {
 	struct device		*dev;
 	enum access_mode	mode;
 	struct clk		*clk;
+	bool			ce_asserted;
 
 	/* DMA related objects */
 	struct dma_chan		*read_dma_chan;
@@ -598,16 +600,27 @@ static void fsmc_ce_ctrl(struct fsmc_nand_data *host, bool assert)
 {
 	u32 pc = readl(host->regs_va + FSMC_PC);
 
-	if (!assert)
+	/* Shortcut */
+	if (host->ce_asserted)
+		return;
+
+	if (!assert) {
+		/*
+		 * Make sure all previous read/write have been done before
+		 * de-asserting the CE line.
+		 */
+		mb();
 		writel_relaxed(pc & ~FSMC_ENABLE, host->regs_va + FSMC_PC);
-	else
+		host->ce_asserted = false;
+	} else {
 		writel_relaxed(pc | FSMC_ENABLE, host->regs_va + FSMC_PC);
-
-	/*
-	 * nCE line changes must be applied before returning from this
-	 * function.
-	 */
-	mb();
+		/*
+		 * nCE line changes must be applied before returning from this
+		 * function.
+		 */
+		mb();
+		host->ce_asserted = true;
+	}
 }
 
 /*
@@ -627,6 +640,12 @@ static int fsmc_exec_op(struct nand_chip *chip, const struct nand_operation *op,
 
 	pr_debug("Executing operation [%d instructions]:\n", op->ninstrs);
 
+	if (op->cs != 0) {
+		pr_err("illegal chip select\n");
+		fsmc_ce_ctrl(host, false);
+		return -EINVAL;
+	}
+
 	fsmc_ce_ctrl(host, true);
 
 	for (op_id = 0; op_id < op->ninstrs; op_id++) {
@@ -686,8 +705,6 @@ static int fsmc_exec_op(struct nand_chip *chip, const struct nand_operation *op,
 		}
 	}
 
-	fsmc_ce_ctrl(host, false);
-
 	return ret;
 }
 
@@ -1153,6 +1170,8 @@ static int fsmc_nand_remove(struct platform_device *pdev)
 {
 	struct fsmc_nand_data *host = platform_get_drvdata(pdev);
 
+	fsmc_ce_ctrl(host, false);
+
 	if (host) {
 		nand_release(&host->nand);
 
@@ -1171,6 +1190,8 @@ static int fsmc_nand_suspend(struct device *dev)
 {
 	struct fsmc_nand_data *host = dev_get_drvdata(dev);
 
+	fsmc_ce_ctrl(host, false);
+
 	if (host)
 		clk_disable_unprepare(host->clk);
 
-- 
2.19.2

Re: [PATCH] mtd: rawnand: fsmc: Keep CE enabled fix mb() drain

[Boris Brezillon]

On Wed,  9 Jan 2019 21:55:30 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> Hammering the chip enable 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
> (...)
> 
> This patch keeps the CE (chip enable, the only chip select)
> signal from the FSMC block enabled from the first command
> after probe() or resume() until the driver either suspend()
> or remove(). Create a state variable to track this.

I just read the Spear600 reference manual, and I'm not sure the
BANK_ENABLE bit controls the CE line. My understanding is that it just
marks the bank as active and CE line is asserted when you actually
access the AHB mem bank range (probably after making sure the FSMC bus
is idle).

If I'm correct, I'd recommend dropping fsmc_ce_ctrl() and marking the
bank enabled at probe time.

> 
> Runtime PM can possibly be used on top of this approach if
> we want to disable the CE line when no flash read/write is
> going on for a prolonged time, but for now it stabilizes
> the platform by simply keeping CE asserted as it used to
> be.
> 
> While we're at it also fix the mb() memory barrier drain
> order (patch folded in from Boris Brezillion).

Let's move that to a separate patch (but I'm not even sure it will
survive if we go for the "enable bank at probe time" approach).

Re: [PATCH] mtd: rawnand: fsmc: Keep CE enabled fix mb() drain

[Linus Walleij]

On Wed, Jan 9, 2019 at 10:21 PM Boris Brezillon <bbrezillon@kernel.org> wrote:
> On Wed,  9 Jan 2019 21:55:30 +0100
> Linus Walleij <linus.walleij@linaro.org> wrote:

> > This patch keeps the CE (chip enable, the only chip select)
> > signal from the FSMC block enabled from the first command
> > after probe() or resume() until the driver either suspend()
> > or remove(). Create a state variable to track this.
>
> I just read the Spear600 reference manual, and I'm not sure the
> BANK_ENABLE bit controls the CE line. My understanding is that it just
> marks the bank as active and CE line is asserted when you actually
> access the AHB mem bank range (probably after making sure the FSMC bus
> is idle).

The Nomadik STn8815 says (for this bit):

PBKEN PC-card/NAND-Flash chip-select enable.
Enables the corresponding chip-select.
If a disabled chip-select is accessed, an HRESP = ERROR is generated
on the AHB bus.
0: disabled (default after reset)
1: enabled

The same for Nomadik STn8820 and the Ux500 variants.

So "enable" might very well have the meaning you say above,
it's just very unclear and confusing.

> If I'm correct, I'd recommend dropping fsmc_ce_ctrl() and marking the
> bank enabled at probe time.

This already happens in fsmc_nand_setup() so we just
need to delete some code then.

But maybe we should disable it during remove()
at least?

Yours,
Linus Walleij

Re: [PATCH] mtd: rawnand: fsmc: Keep CE enabled fix mb() drain

[Boris Brezillon]

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

> On Wed, Jan 9, 2019 at 10:21 PM Boris Brezillon <bbrezillon@kernel.org> wrote:
> > On Wed,  9 Jan 2019 21:55:30 +0100
> > Linus Walleij <linus.walleij@linaro.org> wrote:  
> 
> > > This patch keeps the CE (chip enable, the only chip select)
> > > signal from the FSMC block enabled from the first command
> > > after probe() or resume() until the driver either suspend()
> > > or remove(). Create a state variable to track this.  
> >
> > I just read the Spear600 reference manual, and I'm not sure the
> > BANK_ENABLE bit controls the CE line. My understanding is that it just
> > marks the bank as active and CE line is asserted when you actually
> > access the AHB mem bank range (probably after making sure the FSMC bus
> > is idle).  
> 
> The Nomadik STn8815 says (for this bit):
> 
> PBKEN PC-card/NAND-Flash chip-select enable.
> Enables the corresponding chip-select.
> If a disabled chip-select is accessed, an HRESP = ERROR is generated
> on the AHB bus.
> 0: disabled (default after reset)
> 1: enabled
> 
> The same for Nomadik STn8820 and the Ux500 variants.
> 
> So "enable" might very well have the meaning you say above,
> it's just very unclear and confusing.
> 
> > If I'm correct, I'd recommend dropping fsmc_ce_ctrl() and marking the
> > bank enabled at probe time.  
> 
> This already happens in fsmc_nand_setup() so we just
> need to delete some code then.
> 
> But maybe we should disable it during remove()
> at least?

Yep, and set it again at resume time, just in case regs content is lost
at suspend time.

Re: [PATCH] mtd: rawnand: fsmc: Keep CE enabled fix mb() drain

[Boris Brezillon]

On Wed, 9 Jan 2019 22:21:25 +0100
Boris Brezillon <bbrezillon@kernel.org> wrote:

> On Wed,  9 Jan 2019 21:55:30 +0100
> Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> > Hammering the chip enable 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
> > (...)
> > 
> > This patch keeps the CE (chip enable, the only chip select)
> > signal from the FSMC block enabled from the first command
> > after probe() or resume() until the driver either suspend()
> > or remove(). Create a state variable to track this.  
> 
> I just read the Spear600 reference manual, and I'm not sure the
> BANK_ENABLE bit controls the CE line. My understanding is that it just
> marks the bank as active and CE line is asserted when you actually
> access the AHB mem bank range (probably after making sure the FSMC bus
> is idle).

My bad, I was looking at the CTRL reg, not CTRL_PC.