[PATCH 0/4] some mtd fixes

[PATCH 0/4] some mtd fixes

[Uwe Kleine-König]

Hello,

In the past I send a few problem reports and patches but only got a
single response that one of the patches was already submitted without
making it into upstream, though.

So I'm resending in the hope that continuous reminding will help in the
end :-)

Shortlog and overall diffstat can be found below.

You can also pull or fetch these patches from

	git://www.modarm9.com/gitsrc/pub/people/ukleinek/linux-2.6.git mtd-fixes

and look at them at

	http://www.modarm9.com/git/people/ukleinek/linux-2.6.git?a=shortlog;h=refs/heads/mtd-fixes

Best regards and thanks for your attention,
Uwe

Robert Jarzmik (1):
      [MTD] physmap: Fix suspend/resume/shutdown bugs.

Uwe Kleine-König (3):
      [MTD] remove bogus warning about missing boot bank location
      [RFC] [MTD] cfi_probe: remove Intel chip workaround
      [MTD] physmap: resume already suspended chips on failure to suspend


 drivers/mtd/chips/cfi_cmdset_0002.c |    5 -----
 drivers/mtd/chips/cfi_probe.c       |    8 --------
 drivers/mtd/maps/physmap.c          |   22 ++++++++++++++++++----
 3 files changed, 18 insertions(+), 17 deletions(-)

[PATCH 1/4] [MTD] remove bogus warning about missing boot bank location

[Uwe Kleine-König]

After the deleted block bootloc is only used once as follows:

	if (bootloc == 3 && something_else) {
		...

So setting bootloc = 2 doesn't change anything.  Taking that the warning is
wrong and missleading.

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
 drivers/mtd/chips/cfi_cmdset_0002.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index f7fcc63..8b5b554 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -344,11 +344,6 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
 #endif
 
 		bootloc = extp->TopBottom;
-		if ((bootloc != 2) && (bootloc != 3)) {
-			printk(KERN_WARNING "%s: CFI does not contain boot "
-			       "bank location. Assuming top.\n", map->name);
-			bootloc = 2;
-		}
 
 		if (bootloc == 3 && cfi->cfiq->NumEraseRegions > 1) {
 			printk(KERN_WARNING "%s: Swapping erase regions for broken CFI table.\n", map->name);
-- 
1.5.6.3

[PATCH 2/4] [RFC] [MTD] cfi_probe: remove Intel chip workaround

[Uwe Kleine-König]

For me this workaround breaks detection of a M29W128GL flash because it
doesn't switch to CFI Query mode after writing 0xff.

This workaround was introduced in 2004 between v2.6.8-rc1 and
v2.6.8-rc2 in CVS revision 1.77 of cfi_probe.c.  ("Send 0xFF to switch Intel
chips back to read mode too. Some of them ignore 0xF0")

Alternatively writing 0xff *before* 0xf0 works with the M29W128GL.

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
 drivers/mtd/chips/cfi_probe.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/chips/cfi_probe.c b/drivers/mtd/chips/cfi_probe.c
index a4463a9..6b7ee5a 100644
--- a/drivers/mtd/chips/cfi_probe.c
+++ b/drivers/mtd/chips/cfi_probe.c
@@ -46,7 +46,6 @@ do { \
 #define xip_enable(base, map, cfi) \
 do { \
 	cfi_send_gen_cmd(0xF0, 0, base, map, cfi, cfi->device_type, NULL); \
-	cfi_send_gen_cmd(0xFF, 0, base, map, cfi, cfi->device_type, NULL); \
 	xip_allowed(base, map); \
 } while (0)
 
@@ -54,7 +53,6 @@ do { \
 do { \
 	xip_disable(); \
 	cfi_send_gen_cmd(0xF0, 0, base, map, cfi, cfi->device_type, NULL); \
-	cfi_send_gen_cmd(0xFF, 0, base, map, cfi, cfi->device_type, NULL); \
 	cfi_send_gen_cmd(0x98, 0x55, base, map, cfi, cfi->device_type, NULL); \
 } while (0)
 
@@ -118,7 +116,6 @@ static int __xipram cfi_probe_chip(struct map_info *map, __u32 base,
 
 	xip_disable();
 	cfi_send_gen_cmd(0xF0, 0, base, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0xFF, 0, base, map, cfi, cfi->device_type, NULL);
 	cfi_send_gen_cmd(0x98, 0x55, base, map, cfi, cfi->device_type, NULL);
 
 	if (!qry_present(map,base,cfi)) {
@@ -146,7 +143,6 @@ static int __xipram cfi_probe_chip(struct map_info *map, __u32 base,
 			/* Eep. This chip also had the QRY marker.
 			 * Is it an alias for the new one? */
 			cfi_send_gen_cmd(0xF0, 0, start, map, cfi, cfi->device_type, NULL);
-			cfi_send_gen_cmd(0xFF, 0, start, map, cfi, cfi->device_type, NULL);
 
 			/* If the QRY marker goes away, it's an alias */
 			if (!qry_present(map, start, cfi)) {
@@ -160,7 +156,6 @@ static int __xipram cfi_probe_chip(struct map_info *map, __u32 base,
 			 * too and if it's the same, assume it's an alias. */
 			/* FIXME: Use other modes to do a proper check */
 			cfi_send_gen_cmd(0xF0, 0, base, map, cfi, cfi->device_type, NULL);
-			cfi_send_gen_cmd(0xFF, 0, start, map, cfi, cfi->device_type, NULL);
 
 			if (qry_present(map, base, cfi)) {
 				xip_allowed(base, map);
@@ -178,7 +173,6 @@ static int __xipram cfi_probe_chip(struct map_info *map, __u32 base,
 
 	/* Put it back into Read Mode */
 	cfi_send_gen_cmd(0xF0, 0, base, map, cfi, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0xFF, 0, base, map, cfi, cfi->device_type, NULL);
 	xip_allowed(base, map);
 
 	printk(KERN_INFO "%s: Found %d x%d devices at 0x%x in %d-bit bank\n",
@@ -239,8 +233,6 @@ static int __xipram cfi_chip_setup(struct map_info *map,
 
 	/* Put it back into Read Mode */
 	cfi_send_gen_cmd(0xF0, 0, base, map, cfi, cfi->device_type, NULL);
-	/* ... even if it's an Intel chip */
-	cfi_send_gen_cmd(0xFF, 0, base, map, cfi, cfi->device_type, NULL);
 	xip_allowed(base, map);
 
 	/* Do any necessary byteswapping */
-- 
1.5.6.3

[PATCH 3/4] [MTD] physmap: Fix suspend/resume/shutdown bugs.

[Uwe Kleine-König]

From: Robert Jarzmik <rjarzmik@free.fr>

Don't call suspend/resume functions if they have not been
defined.

Signed-off-by: Robert Jarzmik <rjarzmik@free.fr>
Acked-By: Joern Engel <joern@logfs.org>
Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
 drivers/mtd/maps/physmap.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/maps/physmap.c b/drivers/mtd/maps/physmap.c
index 183255f..73c770b 100644
--- a/drivers/mtd/maps/physmap.c
+++ b/drivers/mtd/maps/physmap.c
@@ -203,7 +203,8 @@ static int physmap_flash_suspend(struct platform_device *dev, pm_message_t state
 	int i;
 
 	for (i = 0; i < MAX_RESOURCES && info->mtd[i]; i++)
-		ret |= info->mtd[i]->suspend(info->mtd[i]);
+		if (info->mtd[i]->suspend)
+			ret |= info->mtd[i]->suspend(info->mtd[i]);
 
 	return ret;
 }
@@ -214,7 +215,8 @@ static int physmap_flash_resume(struct platform_device *dev)
 	int i;
 
 	for (i = 0; i < MAX_RESOURCES && info->mtd[i]; i++)
-		info->mtd[i]->resume(info->mtd[i]);
+		if (info->mtd[i]->resume)
+			info->mtd[i]->resume(info->mtd[i]);
 
 	return 0;
 }
@@ -225,8 +227,9 @@ static void physmap_flash_shutdown(struct platform_device *dev)
 	int i;
 
 	for (i = 0; i < MAX_RESOURCES && info->mtd[i]; i++)
-		if (info->mtd[i]->suspend(info->mtd[i]) == 0)
-			info->mtd[i]->resume(info->mtd[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_suspend NULL
-- 
1.5.6.3

[PATCH 4/4] [MTD] physmap: resume already suspended chips on failure to suspend

[Uwe Kleine-König]

A nice side effect of this patch is that the return value of
physmap_flash_suspend in the error path is the value of the first failing
suspend callback and not the bitwise OR of all of them.

Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com>
---
 drivers/mtd/maps/physmap.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/maps/physmap.c b/drivers/mtd/maps/physmap.c
index 73c770b..3e60e87 100644
--- a/drivers/mtd/maps/physmap.c
+++ b/drivers/mtd/maps/physmap.c
@@ -203,8 +203,19 @@ static int physmap_flash_suspend(struct platform_device *dev, pm_message_t state
 	int i;
 
 	for (i = 0; i < MAX_RESOURCES && info->mtd[i]; i++)
-		if (info->mtd[i]->suspend)
-			ret |= info->mtd[i]->suspend(info->mtd[i]);
+		if (info->mtd[i]->suspend) {
+			ret = info->mtd[i]->suspend(info->mtd[i]);
+			if (ret)
+				goto fail;
+		}
+
+	return 0;
+fail:
+	for (--i; i >= 0; --i)
+		if (info->mtd[i]->suspend) {
+			BUG_ON(!info->mtd[i]->resume);
+			info->mtd[i]->resume(info->mtd[i]);
+		}
 
 	return ret;
 }
-- 
1.5.6.3

Re: [PATCH 2/4] [RFC] [MTD] cfi_probe: remove Intel chip workaround

[David Woodhouse]

On Tue, 2008-07-22 at 09:38 +0200, Uwe Kleine-König wrote:
> For me this workaround breaks detection of a M29W128GL flash because it
> doesn't switch to CFI Query mode after writing 0xff.
> 
> This workaround was introduced in 2004 between v2.6.8-rc1 and
> v2.6.8-rc2 in CVS revision 1.77 of cfi_probe.c.  ("Send 0xFF to switch Intel
> chips back to read mode too. Some of them ignore 0xF0")
> 
> Alternatively writing 0xff *before* 0xf0 works with the M29W128GL.

That's probably what you should do then; your patch as it stands would
break Intel chips, wouldn't it?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH 2/4] [RFC] [MTD] cfi_probe: remove Intel chip workaround

[Uwe Kleine-König]

Hi David,

David Woodhouse wrote:
> On Tue, 2008-07-22 at 09:38 +0200, Uwe Kleine-König wrote:
> > For me this workaround breaks detection of a M29W128GL flash because it
> > doesn't switch to CFI Query mode after writing 0xff.
> >
> > This workaround was introduced in 2004 between v2.6.8-rc1 and
> > v2.6.8-rc2 in CVS revision 1.77 of cfi_probe.c.  ("Send 0xFF to switch Intel
> > chips back to read mode too. Some of them ignore 0xF0")
> >
> > Alternatively writing 0xff *before* 0xf0 works with the M29W128GL.
> 
> That's probably what you should do then; your patch as it stands would
> break Intel chips, wouldn't it?
IMHO it depends on how common these chips are today.  If they still are,
I'd like to test exchanging the two commands with them before apply it.
After all it might not work for them?!  And if there were only a few and
they don't exist anymore today I would like to remove the workaround for
them.  That's why I marked my patch as RFC.

As you committed that workaround you might know which chips are/were
affected.  And you might even be able to test it and/or judge if the
workaround is still needed today.

Best regards,
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: [PATCH 2/4] [RFC] [MTD] cfi_probe: remove Intel chip workaround

[Alexander Belyakov]

On Tue, Jul 22, 2008 at 11:38 AM, Uwe Kleine-König
<Uwe.Kleine-Koenig@digi.com> wrote:
> For me this workaround breaks detection of a M29W128GL flash because it
> doesn't switch to CFI Query mode after writing 0xff.
>
> This workaround was introduced in 2004 between v2.6.8-rc1 and
> v2.6.8-rc2 in CVS revision 1.77 of cfi_probe.c.  ("Send 0xFF to switch Intel
> chips back to read mode too. Some of them ignore 0xF0")
>
> Alternatively writing 0xff *before* 0xf0 works with the M29W128GL.
>

As far as I know M29W128GL is only chip having troubles with 0xFF.
Even M29DW128GL works fine with that.

Writing 0xFF before 0xF0 works fine both M29W128GL, M29DW128GL and
Intel's flash (we have tried with M18). Not sure though if it would
work for *all* flash parts.

Thank you,
Alexander

Re: [PATCH 2/4] [RFC] [MTD] cfi_probe: remove Intel chip workaround

[Alexey Korolev]

Hi All,

> > For me this workaround breaks detection of a M29W128GL flash because it
> > doesn't switch to CFI Query mode after writing 0xff.
> >
> > This workaround was introduced in 2004 between v2.6.8-rc1 and
> > v2.6.8-rc2 in CVS revision 1.77 of cfi_probe.c.  ("Send 0xFF to switch Intel
> > chips back to read mode too. Some of them ignore 0xF0")
> >
> > Alternatively writing 0xff *before* 0xf0 works with the M29W128GL.
> >
> 
> As far as I know M29W128GL is only chip having troubles with 0xFF.
> Even M29DW128GL works fine with that.
> 
> Writing 0xFF before 0xF0 works fine both M29W128GL, M29DW128GL and
> Intel's flash (we have tried with M18). Not sure though if it would
> work for *all* flash parts.

Several days ago I have asked our specialists about what chips require
0xFF just after 0xF0. Probably it was some early revisions of L30.
Unfortunately the fixup description do not tell us what it was exactly. It was long time ago and it is
not so simple to find out what chips require this. Please stand by for
2-3 days I still believe I could be able to find what it was the chip
exactly. If it was some beta revisions - then it make sence to kill 0xFF
lines. If not it might be necessary to use another fix. 

Thanks,
Alexey

Re: [PATCH 2/4] [RFC] [MTD] cfi_probe: remove Intel chip workaround

[Alexey Korolev]

Hi,
> 
> > > For me this workaround breaks detection of a M29W128GL flash because it
> > > doesn't switch to CFI Query mode after writing 0xff.
> > >
> > > This workaround was introduced in 2004 between v2.6.8-rc1 and
> > > v2.6.8-rc2 in CVS revision 1.77 of cfi_probe.c.  ("Send 0xFF to switch Intel
> > > chips back to read mode too. Some of them ignore 0xF0")
> > >
> > > Alternatively writing 0xff *before* 0xf0 works with the M29W128GL.
> > >
> > 
> > As far as I know M29W128GL is only chip having troubles with 0xFF.
> > Even M29DW128GL works fine with that.
> > 
> > Writing 0xFF before 0xF0 works fine both M29W128GL, M29DW128GL and
> > Intel's flash (we have tried with M18). Not sure though if it would
> > work for *all* flash parts.
> 
> Several days ago I have asked our specialists about what chips require
> 0xFF just after 0xF0. Probably it was some early revisions of L30.
> Unfortunately the fixup description do not tell us what it was exactly. It was long time ago and it is
> not so simple to find out what chips require this. Please stand by for
> 2-3 days I still believe I could be able to find what it was the chip
> exactly. If it was some beta revisions - then it make sence to kill 0xFF
> lines. If not it might be necessary to use another fix. 
>

I've lost any hope to reach info about what chips are require sending
0xFF just after reset. About 10 different persons were asked - nobody
knows. 
So we don't know exactly what chips require 0xFF. So it might be risky to kick off
these 0xFF lines since it may affect users. 
May be it would be better to make a kind of cfi_fixup routine to provide
more or less generic QRY procedure for "odd" chips at the stage of
probing?
It make sence at least because there is one more chip requires special
routine for QRY. ST M29DW should send 0x98 command not at 0x55 offset
but at 0x555 offset.
So using existing routines for ST M29DW causes failure. 

I've made patch and verified it on ST M29W, ST M29DW, M18. Please find
the patch in next messsage. Hope it will help.

Thanks,
Alexey
 

Re: [PATCH 1/4] [MTD] remove bogus warning about missing boot bank location

[Christopher Moore]

Uwe Kleine-König <Uwe.Kleine-Koenig <at> digi.com> writes:

> 
> After the deleted block bootloc is only used once as follows:
> 
> 	if (bootloc == 3 && something_else) {
> 		...
> 
> So setting bootloc = 2 doesn't change anything.  Taking that the warning is
> wrong and missleading.
> 
> Signed-off-by: Uwe Kleine-König <Uwe.Kleine-Koenig <at> digi.com>


Nice catch, Uwe.

IMHO the worst thing is that the warning is completely bogus.
It says "Assuming top" but the next line sets bootloc to bottom (2) :(

See also this thread: http://thread.gmane.org/gmane.linux.drivers.mtd/22176

Last time I looked this patch had not been applied so I am pinging it.

Therefore a bit late and FWIW:-
Acked-by: Christopher Moore <moore@free.fr>