[PATCH] mtd: cfi_cmdset_0001.c: fix resume for LH28F640BF chips

[PATCH] mtd: cfi_cmdset_0001.c: fix resume for LH28F640BF chips

[Andrea Adami]

After '#echo mem > /sys/power/state' some devices can not be properly resumed
because apparently the MTD Partition Configuration Register has been reset
to default thus the rootfs cannot be mounted cleanly on resume.
An example of this can be found in the SA-1100 Developer's Manual at 9.5.3.3
where the second step of the Sleep Shutdown Sequence is described:
"An internal reset is applied to the SA-1100. All units are reset...".

As workaround we refresh the PCR value as done initially on chip setup.

This behavior and the fix are confirmed by our tests done on 2 different Zaurus
collie units with kernel 3.17.

Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
---
 drivers/mtd/chips/cfi_cmdset_0001.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
index a7543ba..59e2355 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -2590,6 +2590,8 @@ static void cfi_intelext_resume(struct mtd_info *mtd)
 
 		/* Go to known state. Chip may have been power cycled */
 		if (chip->state == FL_PM_SUSPENDED) {
+			/* Refresh LH28F640BF Partition Config. Register */
+			fixup_LH28F640BF(mtd);
 			map_write(map, CMD(0xFF), cfi->chips[i].start);
 			chip->oldstate = chip->state = FL_READY;
 			wake_up(&chip->wq);
-- 
1.9.1

Re: [PATCH] mtd: cfi_cmdset_0001.c: fix resume for LH28F640BF chips

[Brian Norris]

On Thu, Oct 23, 2014 at 01:23:01AM +0200, Andrea Adami wrote:
> After '#echo mem > /sys/power/state' some devices can not be properly resumed
> because apparently the MTD Partition Configuration Register has been reset
> to default thus the rootfs cannot be mounted cleanly on resume.
> An example of this can be found in the SA-1100 Developer's Manual at 9.5.3.3
> where the second step of the Sleep Shutdown Sequence is described:
> "An internal reset is applied to the SA-1100. All units are reset...".
> 
> As workaround we refresh the PCR value as done initially on chip setup.
> 
> This behavior and the fix are confirmed by our tests done on 2 different Zaurus
> collie units with kernel 3.17.
> 
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> Signed-off-by: Andrea Adami <andrea.adami@gmail.com>

Who's the author? I have a 'From' header (which becomes the patch
author) of Andrea, but the sign-off is Dmitry. If you add a
'From: xxx <yyy@zzz.tld>' line to the beginning of the email, that can
clarify the author, even when the author is not emailing.

git-send-email will also handle that for you, if the original git commit
has the proper author.

> ---
>  drivers/mtd/chips/cfi_cmdset_0001.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
> index a7543ba..59e2355 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0001.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0001.c
> @@ -2590,6 +2590,8 @@ static void cfi_intelext_resume(struct mtd_info *mtd)
>  
>  		/* Go to known state. Chip may have been power cycled */
>  		if (chip->state == FL_PM_SUSPENDED) {
> +			/* Refresh LH28F640BF Partition Config. Register */
> +			fixup_LH28F640BF(mtd);

OK, that looks fine.

But while we're at it, it seems like any chip which has a boot-time
fixup hook in cfi_fixup_table[] should be run at resume time, since
those chips may have lost power too. For instance, it looks like
fixup_unlock_powerup_lock() would need rerun.

I'm not sure if we should do a more invasive patch like that without any
testing, though.

>  			map_write(map, CMD(0xFF), cfi->chips[i].start);
>  			chip->oldstate = chip->state = FL_READY;
>  			wake_up(&chip->wq);

Brian

Re: [PATCH] mtd: cfi_cmdset_0001.c: fix resume for LH28F640BF chips

[Andrea Adami]

Brian,

thanks for the review.

On Tue, Oct 28, 2014 at 11:25 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
>
> On Thu, Oct 23, 2014 at 01:23:01AM +0200, Andrea Adami wrote:
> > After '#echo mem > /sys/power/state' some devices can not be properly resumed
> > because apparently the MTD Partition Configuration Register has been reset
> > to default thus the rootfs cannot be mounted cleanly on resume.
> > An example of this can be found in the SA-1100 Developer's Manual at 9.5.3.3
> > where the second step of the Sleep Shutdown Sequence is described:
> > "An internal reset is applied to the SA-1100. All units are reset...".
> >
> > As workaround we refresh the PCR value as done initially on chip setup.
> >
> > This behavior and the fix are confirmed by our tests done on 2 different Zaurus
> > collie units with kernel 3.17.
> >
> > Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> > Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
>
> Who's the author? I have a 'From' header (which becomes the patch
> author) of Andrea, but the sign-off is Dmitry. If you add a
> 'From: xxx <yyy@zzz.tld>' line to the beginning of the email, that can
> clarify the author, even when the author is not emailing.
>
> git-send-email will also handle that for you, if the original git commit
> has the proper author.

The author is Dmitry who first noticed the failure on resume and
suggested the fix.
I just verified the PCR was reset [1] and added the comments before
sending it upstream.

>
> > ---
> >  drivers/mtd/chips/cfi_cmdset_0001.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
> > index a7543ba..59e2355 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0001.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0001.c
> > @@ -2590,6 +2590,8 @@ static void cfi_intelext_resume(struct mtd_info *mtd)
> >
> >               /* Go to known state. Chip may have been power cycled */
> >               if (chip->state == FL_PM_SUSPENDED) {
> > +                     /* Refresh LH28F640BF Partition Config. Register */
> > +                     fixup_LH28F640BF(mtd);
>
> OK, that looks fine.
>
> But while we're at it, it seems like any chip which has a boot-time
> fixup hook in cfi_fixup_table[] should be run at resume time, since
> those chips may have lost power too. For instance, it looks like
> fixup_unlock_powerup_lock() would need rerun.
>
> I'm not sure if we should do a more invasive patch like that without any
> testing, though.
>
> >                       map_write(map, CMD(0xFF), cfi->chips[i].start);
> >                       chip->oldstate = chip->state = FL_READY;
> >                       wake_up(&chip->wq);
>
> Brian

For chips supporting 'Instant Lock' the unlock is done few lines later
by cfi_intelext_restore_locks(mtd)

If you don't see more issues I'd resend the patch adding the initial From:.

Thanks

Andrea


[1]
root@collie:~# echo mem > /sys/power/state
PM: Syncing filesystems ... done.
Freezing user space processes ... (elapsed 0.002 seconds) done.
Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
Suspending console(s) (use no_console_suspend to debug)
sd 0:0:0:0: [sda] Stopping disk
PM: suspend of devices complete after 836.004 msecs
PM: late suspend of devices complete after 5.305 msecs
PM: noirq suspend of devices complete after 4.625 msecs
PM: noirq resume of devices complete after 726.661 msecs
PM: early resume of devices complete after 4.004 msecs
sd 0:0:0:0: [sda] Starting disk
Partition Configuration Register was:400   <--------------------this
Reset Partition Config. Register: 1 Partition of 4 planes
cfi_cmdset_0001: Simultaneous Operations disabled
ata1.00: configured for PIO0
PM: resume of devices complete after 276.926 msecs
Restarting tasks ... done.
root@collie:~#

* HEX BIN NR PARTITIONS
* 400 = 100 2   3 planes + 1 plane (boot default)
*
* 000 = 000 1   1 partition of 4 planes, no Dual Work
* 200 = 010 2   2 partitions of 2 planes
* 700 = 111 4   4 partitions of 1 plane

Re: [PATCH] mtd: cfi_cmdset_0001.c: fix resume for LH28F640BF chips

[Brian Norris]

On Thu, Oct 30, 2014 at 12:45:36AM +0100, Andrea Adami wrote:
> If you don't see more issues I'd resend the patch adding the initial From:.

I think everything else looks good, so I'll just modify the 'From'
myself this time.

Brian

Re: [PATCH] mtd: cfi_cmdset_0001.c: fix resume for LH28F640BF chips

[Brian Norris]

On Thu, Oct 23, 2014 at 01:23:01AM +0200, Andrea Adami wrote:
> After '#echo mem > /sys/power/state' some devices can not be properly resumed
> because apparently the MTD Partition Configuration Register has been reset
> to default thus the rootfs cannot be mounted cleanly on resume.
> An example of this can be found in the SA-1100 Developer's Manual at 9.5.3.3
> where the second step of the Sleep Shutdown Sequence is described:
> "An internal reset is applied to the SA-1100. All units are reset...".
> 
> As workaround we refresh the PCR value as done initially on chip setup.
> 
> This behavior and the fix are confirmed by our tests done on 2 different Zaurus
> collie units with kernel 3.17.
> 
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> Signed-off-by: Andrea Adami <andrea.adami@gmail.com>

Pushed to l2-mtd.git/for-3.18. Changed the author to Dmitry and added
the following:

Fixes: 812c5fa82bae: ("mtd: cfi_cmdset_0001.c: add support for Sharp LH28F640BF NOR")
Cc: <stable@vger.kernel.org> # 3.16+

Thanks,
Brian