| Message ID | 1310466535-15287-5-git-send-email-leiwen@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 12, 2011 at 12:28 PM, Lei Wen <leiwen@marvell.com> wrote: > This patch add protection on the suspend&resume path to prevent > some unexpected behavior, like interrupt occur at the very second > of resume back and it don't follow normal command path, which lead > to bug. > > Signed-off-by: Lei Wen <leiwen@marvell.com> > --- > drivers/mtd/nand/pxa3xx_nand.c | 28 ++++++++++++++++++++++++++++ > 1 files changed, 28 insertions(+), 0 deletions(-) > [...] > @@ -1267,6 +1283,18 @@ static int pxa3xx_nand_resume(struct platform_device *pdev) > info->cs = 0xff; > clk_enable(info->clk); > > + /* > + * As the spec, the NDSR would be updated to 0x1800 when > + * do the nand_clk disable/enable. > + * To prevent it damage state machine of the driver, clear > + * all status before resume > + */ > + nand_writel(nand, NDSR, NDSR_MASK); This doesn't build: CC drivers/mtd/nand/pxa3xx_nand.o drivers/mtd/nand/pxa3xx_nand.c: In function 'pxa3xx_nand_resume': drivers/mtd/nand/pxa3xx_nand.c:1292: error: 'nand' undeclared (first use in this function) drivers/mtd/nand/pxa3xx_nand.c:1292: error: (Each undeclared identifier is reported only once drivers/mtd/nand/pxa3xx_nand.c:1292: error: for each function it appears in.) drivers/mtd/nand/pxa3xx_nand.c:1294: error: 'mtd' undeclared (first use in this function) make[3]: *** [drivers/mtd/nand/pxa3xx_nand.o] Error 1 I guess this was not even compile tested? Anyway, I did a trivial fix-up and will test. Daniel
On Tue, Jul 12, 2011 at 1:39 PM, Daniel Mack <zonque@gmail.com> wrote: > On Tue, Jul 12, 2011 at 12:28 PM, Lei Wen <leiwen@marvell.com> wrote: >> This patch add protection on the suspend&resume path to prevent >> some unexpected behavior, like interrupt occur at the very second >> of resume back and it don't follow normal command path, which lead >> to bug. >> >> Signed-off-by: Lei Wen <leiwen@marvell.com> >> --- >> drivers/mtd/nand/pxa3xx_nand.c | 28 ++++++++++++++++++++++++++++ >> 1 files changed, 28 insertions(+), 0 deletions(-) >> > > [...] > >> @@ -1267,6 +1283,18 @@ static int pxa3xx_nand_resume(struct platform_device *pdev) >> info->cs = 0xff; >> clk_enable(info->clk); >> >> + /* >> + * As the spec, the NDSR would be updated to 0x1800 when >> + * do the nand_clk disable/enable. >> + * To prevent it damage state machine of the driver, clear >> + * all status before resume >> + */ >> + nand_writel(nand, NDSR, NDSR_MASK); > > This doesn't build: > > CC drivers/mtd/nand/pxa3xx_nand.o > drivers/mtd/nand/pxa3xx_nand.c: In function 'pxa3xx_nand_resume': > drivers/mtd/nand/pxa3xx_nand.c:1292: error: 'nand' undeclared (first > use in this function) > drivers/mtd/nand/pxa3xx_nand.c:1292: error: (Each undeclared > identifier is reported only once > drivers/mtd/nand/pxa3xx_nand.c:1292: error: for each function it appears in.) > drivers/mtd/nand/pxa3xx_nand.c:1294: error: 'mtd' undeclared (first > use in this function) > make[3]: *** [drivers/mtd/nand/pxa3xx_nand.o] Error 1 > > I guess this was not even compile tested? Anyway, I did a trivial > fix-up and will test. Also, with this (fixed) patch applied, the system doesn't resume at all. No messages, it simply doesn't come back. Daniel
On 07/12/11 15:02, Daniel Mack wrote: > On Tue, Jul 12, 2011 at 1:39 PM, Daniel Mack <zonque@gmail.com> wrote: >> On Tue, Jul 12, 2011 at 12:28 PM, Lei Wen <leiwen@marvell.com> wrote: >>> This patch add protection on the suspend&resume path to prevent >>> some unexpected behavior, like interrupt occur at the very second >>> of resume back and it don't follow normal command path, which lead >>> to bug. >>> >>> Signed-off-by: Lei Wen <leiwen@marvell.com> >>> --- >>> drivers/mtd/nand/pxa3xx_nand.c | 28 ++++++++++++++++++++++++++++ >>> 1 files changed, 28 insertions(+), 0 deletions(-) >>> >> [...] >> >>> @@ -1267,6 +1283,18 @@ static int pxa3xx_nand_resume(struct platform_device *pdev) >>> info->cs = 0xff; >>> clk_enable(info->clk); >>> >>> + /* >>> + * As the spec, the NDSR would be updated to 0x1800 when >>> + * do the nand_clk disable/enable. >>> + * To prevent it damage state machine of the driver, clear >>> + * all status before resume >>> + */ >>> + nand_writel(nand, NDSR, NDSR_MASK); >> This doesn't build: >> >> CC drivers/mtd/nand/pxa3xx_nand.o >> drivers/mtd/nand/pxa3xx_nand.c: In function 'pxa3xx_nand_resume': >> drivers/mtd/nand/pxa3xx_nand.c:1292: error: 'nand' undeclared (first >> use in this function) >> drivers/mtd/nand/pxa3xx_nand.c:1292: error: (Each undeclared >> identifier is reported only once >> drivers/mtd/nand/pxa3xx_nand.c:1292: error: for each function it appears in.) >> drivers/mtd/nand/pxa3xx_nand.c:1294: error: 'mtd' undeclared (first >> use in this function) >> make[3]: *** [drivers/mtd/nand/pxa3xx_nand.o] Error 1 >> >> I guess this was not even compile tested? Anyway, I did a trivial >> fix-up and will test. > Also, with this (fixed) patch applied, the system doesn't resume at > all. No messages, it simply doesn't come back. I was skeptic about the clock being disabled in Lei's patch, as I observed system hangs if that clock was disabled back then in 2.6.31, but wanted to give it a try, because things has changed since then. Now I see, that Lei already sent v7 without clock toggling...
On Tue, Jul 12, 2011 at 5:56 PM, Igor Grinberg <grinberg@compulab.co.il> wrote: > On 07/12/11 15:02, Daniel Mack wrote: > >> On Tue, Jul 12, 2011 at 1:39 PM, Daniel Mack <zonque@gmail.com> wrote: >>> On Tue, Jul 12, 2011 at 12:28 PM, Lei Wen <leiwen@marvell.com> wrote: >>>> This patch add protection on the suspend&resume path to prevent >>>> some unexpected behavior, like interrupt occur at the very second >>>> of resume back and it don't follow normal command path, which lead >>>> to bug. >>>> >>>> Signed-off-by: Lei Wen <leiwen@marvell.com> >>>> --- >>>> drivers/mtd/nand/pxa3xx_nand.c | 28 ++++++++++++++++++++++++++++ >>>> 1 files changed, 28 insertions(+), 0 deletions(-) >>>> >>> [...] >>> >>>> @@ -1267,6 +1283,18 @@ static int pxa3xx_nand_resume(struct platform_device *pdev) >>>> info->cs = 0xff; >>>> clk_enable(info->clk); >>>> >>>> + /* >>>> + * As the spec, the NDSR would be updated to 0x1800 when >>>> + * do the nand_clk disable/enable. >>>> + * To prevent it damage state machine of the driver, clear >>>> + * all status before resume >>>> + */ >>>> + nand_writel(nand, NDSR, NDSR_MASK); >>> This doesn't build: >>> >>> CC drivers/mtd/nand/pxa3xx_nand.o >>> drivers/mtd/nand/pxa3xx_nand.c: In function 'pxa3xx_nand_resume': >>> drivers/mtd/nand/pxa3xx_nand.c:1292: error: 'nand' undeclared (first >>> use in this function) >>> drivers/mtd/nand/pxa3xx_nand.c:1292: error: (Each undeclared >>> identifier is reported only once >>> drivers/mtd/nand/pxa3xx_nand.c:1292: error: for each function it appears in.) >>> drivers/mtd/nand/pxa3xx_nand.c:1294: error: 'mtd' undeclared (first >>> use in this function) >>> make[3]: *** [drivers/mtd/nand/pxa3xx_nand.o] Error 1 >>> >>> I guess this was not even compile tested? Anyway, I did a trivial >>> fix-up and will test. >> Also, with this (fixed) patch applied, the system doesn't resume at >> all. No messages, it simply doesn't come back. > > I was skeptic about the clock being disabled in Lei's patch, > as I observed system hangs if that clock was disabled back then in 2.6.31, > but wanted to give it a try, because things has changed since then. > > Now I see, that Lei already sent v7 without clock toggling... Yes, we debugged this quickly via Jabber, and without the clock disable, things work fine for me again. Daniel
diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c index 3756d54..35149fe 100644 --- a/drivers/mtd/nand/pxa3xx_nand.c +++ b/drivers/mtd/nand/pxa3xx_nand.c @@ -1246,18 +1246,34 @@ static int pxa3xx_nand_probe(struct platform_device *pdev) static int pxa3xx_nand_suspend(struct platform_device *pdev, pm_message_t state) { struct pxa3xx_nand_info *info = platform_get_drvdata(pdev); + struct pxa3xx_nand_platform_data *pdata; + struct mtd_info *mtd; + int cs; + pdata = pdev->dev.platform_data; if (info->state) { dev_err(&pdev->dev, "driver busy, state = %d\n", info->state); return -EAGAIN; } + for (cs = 0; cs < pdata->num_cs; cs ++) { + mtd = info->host[cs]->mtd; + mtd->suspend(mtd); + } + + clk_disable(info->clk); return 0; } static int pxa3xx_nand_resume(struct platform_device *pdev) { struct pxa3xx_nand_info *info = platform_get_drvdata(pdev); + struct pxa3xx_nand_platform_data *pdata; + int cs; + + pdata = pdev->dev.platform_data; + /* We don't want to handle interrupt without calling mtd routine */ + disable_int(info, NDCR_INT_MASK); /* * Directly set the chip select to a invalid value, @@ -1267,6 +1283,18 @@ static int pxa3xx_nand_resume(struct platform_device *pdev) info->cs = 0xff; clk_enable(info->clk); + /* + * As the spec, the NDSR would be updated to 0x1800 when + * do the nand_clk disable/enable. + * To prevent it damage state machine of the driver, clear + * all status before resume + */ + nand_writel(nand, NDSR, NDSR_MASK); + for (cs = 0; cs < pdata->num_cs; cs ++) { + mtd = info->host[cs]->mtd; + mtd->resume(mtd); + } + return 0; } #else
This patch add protection on the suspend&resume path to prevent some unexpected behavior, like interrupt occur at the very second of resume back and it don't follow normal command path, which lead to bug. Signed-off-by: Lei Wen <leiwen@marvell.com> --- drivers/mtd/nand/pxa3xx_nand.c | 28 ++++++++++++++++++++++++++++ 1 files changed, 28 insertions(+), 0 deletions(-)