| Message ID | 20190109205530.5158-1-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | mtd: rawnand: fsmc: Keep CE enabled fix mb() drain | expand |
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). ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
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. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
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. ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
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);
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(-) -- 2.19.2 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/