Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752907AbdI0Pcf (ORCPT ); Wed, 27 Sep 2017 11:32:35 -0400 Received: from mail-qk0-f172.google.com ([209.85.220.172]:50355 "EHLO mail-qk0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752414AbdI0Pcb (ORCPT ); Wed, 27 Sep 2017 11:32:31 -0400 X-Google-Smtp-Source: AOwi7QCU750XPNP+UztucKF0g0HTw/NbXD4TdDdONOCDkAUwN+9uZjC6Sfg1u5o67Or/b4mxssYnoONMTKy0dowA6gU= MIME-Version: 1.0 In-Reply-To: <20170927170811.57830c1f@bbrezillon> References: <20170927083555.16580-1-romain.izard.pro@gmail.com> <20170927083555.16580-5-romain.izard.pro@gmail.com> <20170927170811.57830c1f@bbrezillon> From: Romain Izard Date: Wed, 27 Sep 2017 17:32:10 +0200 X-Google-Sender-Auth: pReTrqq9fvUFtxS8Rke_Vhngehg Message-ID: Subject: Re: [PATCH v3 4/8] mtd: nand: atmel: Avoid ECC errors when leaving backup mode To: Boris Brezillon Cc: Michael Turquette , Stephen Boyd , Lee Jones , Wenyou Yang , Josh Wu , Richard Weinberger , David Woodhouse , Brian Norris , Marek Vasut , Cyrille Pitchen , Thierry Reding , Richard Genoud , Greg Kroah-Hartman , Jiri Slaby , Alan Stern , Ludovic Desroches , Nicolas Ferre , Alexandre Belloni , linux-pwm@vger.kernel.org, linux-usb@vger.kernel.org, LKML , linux-mtd , linux-serial@vger.kernel.org, linux-clk@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2937 Lines: 73 2017-09-27 17:08 GMT+02:00 Boris Brezillon : > On Wed, 27 Sep 2017 10:35:51 +0200 > Romain Izard 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 >> --- >> 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... -- Romain Izard