| Message ID | 20170927083555.16580-5-romain.izard.pro@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, 27 Sep 2017 10:35:51 +0200 Romain Izard <romain.izard.pro@gmail.com> wrote: > During backup mode, the contents of all registers will be cleared as the > SoC will be completely powered down. For a product that boots on NAND > Flash memory, the bootloader will obviously use the related controller > to read the Flash and correct any detected error in the memory, before > handling back control to the kernel's resuming entry point. > > But it does not clean the NAND controller registers after use and on its > side the kernel driver expects the error locator to be powered down and > in a clean state. Add a resume hook for the PMECC error locator, and > reset its registers. > > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com> > --- > Change in v3: > * keep the PMECC disabled when not in use, and use atmel_pmecc_resume to > reset the controller after the bootloader has left it enabled. > > drivers/mtd/nand/atmel/nand-controller.c | 3 +++ > drivers/mtd/nand/atmel/pmecc.c | 22 ++++++++++++++-------- > drivers/mtd/nand/atmel/pmecc.h | 1 + > 3 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/drivers/mtd/nand/atmel/nand-controller.c b/drivers/mtd/nand/atmel/nand-controller.c > index f25eca79f4e5..86c2199380c2 100644 > --- a/drivers/mtd/nand/atmel/nand-controller.c > +++ b/drivers/mtd/nand/atmel/nand-controller.c > @@ -2530,6 +2530,9 @@ static __maybe_unused int atmel_nand_controller_resume(struct device *dev) > struct atmel_nand_controller *nc = dev_get_drvdata(dev); > struct atmel_nand *nand; > > + if (nand->pmecc) > + atmel_pmecc_resume(nand->pmecc); > + nand is used uninitialized here, and atmel_pmecc_resume() should be passed a atmel_pmecc object not a atmel_pmecc_user. if (nc->pmecc) atmel_pmecc_resume(nc->pmecc); > list_for_each_entry(nand, &nc->chips, node) { > int i; > > diff --git a/drivers/mtd/nand/atmel/pmecc.c b/drivers/mtd/nand/atmel/pmecc.c > index 146af8218314..ff09c0f25dd4 100644 > --- a/drivers/mtd/nand/atmel/pmecc.c > +++ b/drivers/mtd/nand/atmel/pmecc.c > @@ -765,6 +765,12 @@ void atmel_pmecc_get_generated_eccbytes(struct atmel_pmecc_user *user, > } > EXPORT_SYMBOL_GPL(atmel_pmecc_get_generated_eccbytes); > > +void atmel_pmecc_reset(struct atmel_pmecc *pmecc) > +{ > + writel(PMECC_CTRL_RST, pmecc->regs.base + ATMEL_PMECC_CTRL); > + writel(PMECC_CTRL_DISABLE, pmecc->regs.base + ATMEL_PMECC_CTRL); > +} It's not used outside of this file, so it should have a static specifier. Anyway, I wonder why you don't expose atmel_pmecc_reset() directly instead of creating this atmel_pmecc_resume() wrapper. -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2017-09-27 17:08 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > On Wed, 27 Sep 2017 10:35:51 +0200 > Romain Izard <romain.izard.pro@gmail.com> wrote: > >> During backup mode, the contents of all registers will be cleared as the >> SoC will be completely powered down. For a product that boots on NAND >> Flash memory, the bootloader will obviously use the related controller >> to read the Flash and correct any detected error in the memory, before >> handling back control to the kernel's resuming entry point. >> >> But it does not clean the NAND controller registers after use and on its >> side the kernel driver expects the error locator to be powered down and >> in a clean state. Add a resume hook for the PMECC error locator, and >> reset its registers. >> >> Signed-off-by: Romain Izard <romain.izard.pro@gmail.com> >> --- >> Change in v3: >> * keep the PMECC disabled when not in use, and use atmel_pmecc_resume to >> reset the controller after the bootloader has left it enabled. >> >> drivers/mtd/nand/atmel/nand-controller.c | 3 +++ >> drivers/mtd/nand/atmel/pmecc.c | 22 ++++++++++++++-------- >> drivers/mtd/nand/atmel/pmecc.h | 1 + >> 3 files changed, 18 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/mtd/nand/atmel/nand-controller.c b/drivers/mtd/nand/atmel/nand-controller.c >> index f25eca79f4e5..86c2199380c2 100644 >> --- a/drivers/mtd/nand/atmel/nand-controller.c >> +++ b/drivers/mtd/nand/atmel/nand-controller.c >> @@ -2530,6 +2530,9 @@ static __maybe_unused int atmel_nand_controller_resume(struct device *dev) >> struct atmel_nand_controller *nc = dev_get_drvdata(dev); >> struct atmel_nand *nand; >> >> + if (nand->pmecc) >> + atmel_pmecc_resume(nand->pmecc); >> + > > nand is used uninitialized here, and atmel_pmecc_resume() should be > passed a atmel_pmecc object not a atmel_pmecc_user. > > if (nc->pmecc) > atmel_pmecc_resume(nc->pmecc); > And yet I thought I correctly tested this code... :( >> list_for_each_entry(nand, &nc->chips, node) { >> int i; >> >> diff --git a/drivers/mtd/nand/atmel/pmecc.c b/drivers/mtd/nand/atmel/pmecc.c >> index 146af8218314..ff09c0f25dd4 100644 >> --- a/drivers/mtd/nand/atmel/pmecc.c >> +++ b/drivers/mtd/nand/atmel/pmecc.c >> @@ -765,6 +765,12 @@ void atmel_pmecc_get_generated_eccbytes(struct atmel_pmecc_user *user, >> } >> EXPORT_SYMBOL_GPL(atmel_pmecc_get_generated_eccbytes); >> >> +void atmel_pmecc_reset(struct atmel_pmecc *pmecc) >> +{ >> + writel(PMECC_CTRL_RST, pmecc->regs.base + ATMEL_PMECC_CTRL); >> + writel(PMECC_CTRL_DISABLE, pmecc->regs.base + ATMEL_PMECC_CTRL); >> +} > > It's not used outside of this file, so it should have a static > specifier. Anyway, I wonder why you don't expose atmel_pmecc_reset() > directly instead of creating this atmel_pmecc_resume() wrapper. I will fix this...
diff --git a/drivers/mtd/nand/atmel/nand-controller.c b/drivers/mtd/nand/atmel/nand-controller.c index f25eca79f4e5..86c2199380c2 100644 --- a/drivers/mtd/nand/atmel/nand-controller.c +++ b/drivers/mtd/nand/atmel/nand-controller.c @@ -2530,6 +2530,9 @@ static __maybe_unused int atmel_nand_controller_resume(struct device *dev) struct atmel_nand_controller *nc = dev_get_drvdata(dev); struct atmel_nand *nand; + if (nand->pmecc) + atmel_pmecc_resume(nand->pmecc); + list_for_each_entry(nand, &nc->chips, node) { int i; diff --git a/drivers/mtd/nand/atmel/pmecc.c b/drivers/mtd/nand/atmel/pmecc.c index 146af8218314..ff09c0f25dd4 100644 --- a/drivers/mtd/nand/atmel/pmecc.c +++ b/drivers/mtd/nand/atmel/pmecc.c @@ -765,6 +765,12 @@ void atmel_pmecc_get_generated_eccbytes(struct atmel_pmecc_user *user, } EXPORT_SYMBOL_GPL(atmel_pmecc_get_generated_eccbytes); +void atmel_pmecc_reset(struct atmel_pmecc *pmecc) +{ + writel(PMECC_CTRL_RST, pmecc->regs.base + ATMEL_PMECC_CTRL); + writel(PMECC_CTRL_DISABLE, pmecc->regs.base + ATMEL_PMECC_CTRL); +} + int atmel_pmecc_enable(struct atmel_pmecc_user *user, int op) { struct atmel_pmecc *pmecc = user->pmecc; @@ -797,14 +803,17 @@ EXPORT_SYMBOL_GPL(atmel_pmecc_enable); void atmel_pmecc_disable(struct atmel_pmecc_user *user) { - struct atmel_pmecc *pmecc = user->pmecc; - - writel(PMECC_CTRL_RST, pmecc->regs.base + ATMEL_PMECC_CTRL); - writel(PMECC_CTRL_DISABLE, pmecc->regs.base + ATMEL_PMECC_CTRL); + atmel_pmecc_reset(user->pmecc); mutex_unlock(&user->pmecc->lock); } EXPORT_SYMBOL_GPL(atmel_pmecc_disable); +void atmel_pmecc_resume(struct atmel_pmecc_user *user) +{ + atmel_pmecc_reset(user->pmecc); +} +EXPORT_SYMBOL_GPL(atmel_pmecc_resume); + int atmel_pmecc_wait_rdy(struct atmel_pmecc_user *user) { struct atmel_pmecc *pmecc = user->pmecc; @@ -855,10 +864,7 @@ static struct atmel_pmecc *atmel_pmecc_create(struct platform_device *pdev, /* Disable all interrupts before registering the PMECC handler. */ writel(0xffffffff, pmecc->regs.base + ATMEL_PMECC_IDR); - - /* Reset the ECC engine */ - writel(PMECC_CTRL_RST, pmecc->regs.base + ATMEL_PMECC_CTRL); - writel(PMECC_CTRL_DISABLE, pmecc->regs.base + ATMEL_PMECC_CTRL); + atmel_pmecc_reset(pmecc); return pmecc; } diff --git a/drivers/mtd/nand/atmel/pmecc.h b/drivers/mtd/nand/atmel/pmecc.h index a8ddbfca2ea5..488a90f1965d 100644 --- a/drivers/mtd/nand/atmel/pmecc.h +++ b/drivers/mtd/nand/atmel/pmecc.h @@ -63,6 +63,7 @@ void atmel_pmecc_destroy_user(struct atmel_pmecc_user *user); int atmel_pmecc_enable(struct atmel_pmecc_user *user, int op); void atmel_pmecc_disable(struct atmel_pmecc_user *user); +void atmel_pmecc_resume(struct atmel_pmecc_user *user); int atmel_pmecc_wait_rdy(struct atmel_pmecc_user *user); int atmel_pmecc_correct_sector(struct atmel_pmecc_user *user, int sector, void *data, void *ecc);
During backup mode, the contents of all registers will be cleared as the SoC will be completely powered down. For a product that boots on NAND Flash memory, the bootloader will obviously use the related controller to read the Flash and correct any detected error in the memory, before handling back control to the kernel's resuming entry point. But it does not clean the NAND controller registers after use and on its side the kernel driver expects the error locator to be powered down and in a clean state. Add a resume hook for the PMECC error locator, and reset its registers. Signed-off-by: Romain Izard <romain.izard.pro@gmail.com> --- Change in v3: * keep the PMECC disabled when not in use, and use atmel_pmecc_resume to reset the controller after the bootloader has left it enabled. drivers/mtd/nand/atmel/nand-controller.c | 3 +++ drivers/mtd/nand/atmel/pmecc.c | 22 ++++++++++++++-------- drivers/mtd/nand/atmel/pmecc.h | 1 + 3 files changed, 18 insertions(+), 8 deletions(-)