Patchwork [MTD] physmap: remove physmap_flash_shutdown() method.

login
register
mail settings
Submitter Vladimir Zapolskiy
Date March 12, 2010, 1:26 p.m.
Message ID <1268400366-14802-1-git-send-email-vzapolskiy@gmail.com>
Download mbox | patch
Permalink /patch/47674/
State Rejected
Headers show

Comments

Vladimir Zapolskiy - March 12, 2010, 1:26 p.m.
physmap driver operates with cfi, jedec, qinfo and map_rom compliant
chips. None of interface drivers for all these chips uses actual
suspend and resume chip operations, but correspondent two methods only
change flash chip state in drivers' internal state machines, and for
Intel chips save/restore block locks is done.

To operate with boards with Intel flash chips where RP# pin is not
asserted on reset there is an already implemented mechanism of reboot
notifiers, and on reboot the flash moves into defined "read array"
state.

If CONFIG_PM kernel option is enabled, then physmap_flash_shutdown()
method is called after reboot notifier work, and save/restore block
locks operations disturb the chip from default "read array" state,
thus if the flash is a bootable one, board cann't be booted from it
after reset.

Signed-off-by: Vladimir Zapolskiy <vzapolskiy@gmail.com>
Cc: David Woodhouse <David.Woodhouse@intel.com>
Cc: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
Cc: Mike Frysinger <vapier.adi@gmail.com>
Cc: Lennert Buytenhek <buytenh@wantstofly.org>
---
 drivers/mtd/maps/physmap.c |   16 ----------------
 1 files changed, 0 insertions(+), 16 deletions(-)
Mike Frysinger - March 12, 2010, 1:27 p.m.
On Fri, Mar 12, 2010 at 08:26, Vladimir Zapolskiy <vzapolskiy@gmail.com> wrote:
> thus if the flash is a bootable one, board cann't be booted from it

"cann't" -> "can't"
-mike
Vladimir Zapolskiy - March 25, 2010, 8:03 a.m.
Hello David,

please could you be so kind to share your objections about the proposed
change?

Thank you in advance.

Vladimir Zapolskiy <vzapolskiy@gmail.com> writes:

> physmap driver operates with cfi, jedec, qinfo and map_rom compliant
> chips. None of interface drivers for all these chips uses actual
> suspend and resume chip operations, but correspondent two methods only
> change flash chip state in drivers' internal state machines, and for
> Intel chips save/restore block locks is done.
>
> To operate with boards with Intel flash chips where RP# pin is not
> asserted on reset there is an already implemented mechanism of reboot
> notifiers, and on reboot the flash moves into defined "read array"
> state.
>
> If CONFIG_PM kernel option is enabled, then physmap_flash_shutdown()
> method is called after reboot notifier work, and save/restore block
> locks operations disturb the chip from default "read array" state,
> thus if the flash is a bootable one, board cann't be booted from it
> after reset.
>

With best wishes,
Vladimir
Artem Bityutskiy - April 6, 2010, 8:58 a.m.
On Fri, 2010-03-12 at 16:26 +0300, Vladimir Zapolskiy wrote:
> physmap driver operates with cfi, jedec, qinfo and map_rom compliant
> chips. None of interface drivers for all these chips uses actual
> suspend and resume chip operations, but correspondent two methods only
> change flash chip state in drivers' internal state machines, and for
> Intel chips save/restore block locks is done.
> 
> To operate with boards with Intel flash chips where RP# pin is not
> asserted on reset there is an already implemented mechanism of reboot
> notifiers, and on reboot the flash moves into defined "read array"
> state.
> 
> If CONFIG_PM kernel option is enabled, then physmap_flash_shutdown()
> method is called after reboot notifier work, and save/restore block
> locks operations disturb the chip from default "read array" state,
> thus if the flash is a bootable one, board cann't be booted from it
> after reset.
> 
> Signed-off-by: Vladimir Zapolskiy <vzapolskiy@gmail.com>
> Cc: David Woodhouse <David.Woodhouse@intel.com>
> Cc: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> Cc: Mike Frysinger <vapier.adi@gmail.com>
> Cc: Lennert Buytenhek <buytenh@wantstofly.org>
> ---
>  drivers/mtd/maps/physmap.c |   16 ----------------
>  1 files changed, 0 insertions(+), 16 deletions(-)

Taken to my l2-mtd-2.6.git / dunno, thanks.
Vladimir Zapolskiy - April 6, 2010, 10:11 a.m.
Hello Artem,

>
> Taken to my l2-mtd-2.6.git / dunno, thanks.
>

thank you, but please consider this change as a bugfix.

With best wishes,
Vladimir
Artem Bityutskiy - April 6, 2010, 10:25 a.m.
On Tue, 2010-04-06 at 14:11 +0400, Vladimir Zapolskiy wrote:
> Hello Artem,
> 
> >
> > Taken to my l2-mtd-2.6.git / dunno, thanks.
> >
> 
> thank you, but please consider this change as a bugfix.

Should be done by the maintainer, I can't do that.
David Woodhouse - May 13, 2010, 11:36 p.m.
On Fri, 2010-03-12 at 16:26 +0300, Vladimir Zapolskiy wrote:
> physmap driver operates with cfi, jedec, qinfo and map_rom compliant
> chips. None of interface drivers for all these chips uses actual
> suspend and resume chip operations, but correspondent two methods only
> change flash chip state in drivers' internal state machines, and for
> Intel chips save/restore block locks is done.
> 
> To operate with boards with Intel flash chips where RP# pin is not
> asserted on reset there is an already implemented mechanism of reboot
> notifiers, and on reboot the flash moves into defined "read array"
> state.
> 
> If CONFIG_PM kernel option is enabled, then physmap_flash_shutdown()
> method is called after reboot notifier work, and save/restore block
> locks operations disturb the chip from default "read array" state,
> thus if the flash is a bootable one, board cann't be booted from it
> after reset. 

I think this is the wrong approach. We should instead remove the reboot
notifier, and keep the shutdown() method called through the map driver,
which is in the right place in the power management tree.

I'd much rather see a patch which does that.

Patch

diff --git a/drivers/mtd/maps/physmap.c b/drivers/mtd/maps/physmap.c
index d9603f7..dea81d7 100644
--- a/drivers/mtd/maps/physmap.c
+++ b/drivers/mtd/maps/physmap.c
@@ -193,25 +193,9 @@  err_out:
 	return err;
 }
 
-#ifdef CONFIG_PM
-static void physmap_flash_shutdown(struct platform_device *dev)
-{
-	struct physmap_flash_info *info = platform_get_drvdata(dev);
-	int i;
-
-	for (i = 0; i < MAX_RESOURCES && info->mtd[i]; i++)
-		if (info->mtd[i]->suspend && info->mtd[i]->resume)
-			if (info->mtd[i]->suspend(info->mtd[i]) == 0)
-				info->mtd[i]->resume(info->mtd[i]);
-}
-#else
-#define physmap_flash_shutdown NULL
-#endif
-
 static struct platform_driver physmap_flash_driver = {
 	.probe		= physmap_flash_probe,
 	.remove		= physmap_flash_remove,
-	.shutdown	= physmap_flash_shutdown,
 	.driver		= {
 		.name	= "physmap-flash",
 		.owner	= THIS_MODULE,