* [PATCH v4 2/2] mtd: cfi_cmdset_0002: do not fail on no extended query table as they are both optional
2010-04-23 13:33 [PATCH v4 1/2] [MTD] remove bogus warning about missing boot bank location Guillaume LECERF
@ 2010-04-23 13:33 ` Guillaume LECERF
2010-04-24 5:13 ` [PATCH v4 1/2] [MTD] remove bogus warning about missing boot bank location Chris Moore
2010-04-24 9:35 ` Wolfram Sang
2 siblings, 0 replies; 4+ messages in thread
From: Guillaume LECERF @ 2010-04-23 13:33 UTC (permalink / raw)
To: linux-mtd; +Cc: Wolfram Sang, David Woodhouse, Chris Moore, Artem Bityutskiy
After looking at AMD's CFI specification [1], both of the extended query
tables are optional. Thus, it looks like relying that at least one of
those tables exist is a bug in cfi_cmdset_0002.
This patch inverts the logic and checks for unlock function pointers before
exiting on error. This approach leaves place to add a call to a fixup
function to try to handle chips compatible with the early AMD specification
from 1995 [2].
[1] http://www.amd.com/us-en/assets/content_type/DownloadableAssets/cfi_r20.pdf
[2] http://noel.feld.cvut.cz/hw/amd/20158a.pdf
Signed-off-by: Guillaume LECERF <glecerf@gmail.com>
---
drivers/mtd/chips/cfi_cmdset_0002.c | 79 ++++++++++++++++++-----------------
1 files changed, 40 insertions(+), 39 deletions(-)
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index 8da8655..23114ab 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -353,60 +353,61 @@ struct mtd_info *cfi_cmdset_0002(struct map_info *map, int primary)
if (cfi->cfi_mode==CFI_MODE_CFI){
unsigned char bootloc;
- /*
- * It's a real CFI chip, not one for which the probe
- * routine faked a CFI structure. So we read the feature
- * table from it.
- */
__u16 adr = primary?cfi->cfiq->P_ADR:cfi->cfiq->A_ADR;
struct cfi_pri_amdstd *extp;
extp = (struct cfi_pri_amdstd*)cfi_read_pri(map, adr, sizeof(*extp), "Amd/Fujitsu");
- if (!extp) {
- kfree(mtd);
- return NULL;
- }
-
- cfi_fixup_major_minor(cfi, extp);
-
- if (extp->MajorVersion != '1' ||
- (extp->MinorVersion < '0' || extp->MinorVersion > '4')) {
- printk(KERN_ERR " Unknown Amd/Fujitsu Extended Query "
- "version %c.%c.\n", extp->MajorVersion,
- extp->MinorVersion);
- kfree(extp);
- kfree(mtd);
- return NULL;
- }
+ if (extp) {
+ /*
+ * It's a real CFI chip, not one for which the probe
+ * routine faked a CFI structure.
+ */
+ cfi_fixup_major_minor(cfi, extp);
+
+ if (extp->MajorVersion != '1' ||
+ (extp->MinorVersion < '0' || extp->MinorVersion > '4')) {
+ printk(KERN_ERR " Unknown Amd/Fujitsu Extended Query "
+ "version %c.%c.\n", extp->MajorVersion,
+ extp->MinorVersion);
+ kfree(extp);
+ kfree(mtd);
+ return NULL;
+ }
- /* Install our own private info structure */
- cfi->cmdset_priv = extp;
+ /* Install our own private info structure */
+ cfi->cmdset_priv = extp;
- /* Apply cfi device specific fixups */
- cfi_fixup(mtd, cfi_fixup_table);
+ /* Apply cfi device specific fixups */
+ cfi_fixup(mtd, cfi_fixup_table);
#ifdef DEBUG_CFI_FEATURES
- /* Tell the user about it in lots of lovely detail */
- cfi_tell_features(extp);
+ /* Tell the user about it in lots of lovely detail */
+ cfi_tell_features(extp);
#endif
- bootloc = extp->TopBottom;
+ bootloc = extp->TopBottom;
- if (bootloc == 3 && cfi->cfiq->NumEraseRegions > 1) {
- printk(KERN_WARNING "%s: Swapping erase regions for broken CFI table.\n", map->name);
+ if (bootloc == 3 && cfi->cfiq->NumEraseRegions > 1) {
+ printk(KERN_WARNING "%s: Swapping erase regions for broken CFI table.\n", map->name);
- for (i=0; i<cfi->cfiq->NumEraseRegions / 2; i++) {
- int j = (cfi->cfiq->NumEraseRegions-1)-i;
- __u32 swap;
+ for (i=0; i<cfi->cfiq->NumEraseRegions / 2; i++) {
+ int j = (cfi->cfiq->NumEraseRegions-1)-i;
+ __u32 swap;
- swap = cfi->cfiq->EraseRegionInfo[i];
- cfi->cfiq->EraseRegionInfo[i] = cfi->cfiq->EraseRegionInfo[j];
- cfi->cfiq->EraseRegionInfo[j] = swap;
+ swap = cfi->cfiq->EraseRegionInfo[i];
+ cfi->cfiq->EraseRegionInfo[i] = cfi->cfiq->EraseRegionInfo[j];
+ cfi->cfiq->EraseRegionInfo[j] = swap;
+ }
}
+ /* Set the default CFI lock/unlock addresses */
+ cfi->addr_unlock1 = 0x555;
+ cfi->addr_unlock2 = 0x2aa;
+ }
+
+ if (!cfi->addr_unlock1 || !cfi->addr_unlock2) {
+ kfree(mtd);
+ return NULL;
}
- /* Set the default CFI lock/unlock addresses */
- cfi->addr_unlock1 = 0x555;
- cfi->addr_unlock2 = 0x2aa;
} /* CFI mode */
else if (cfi->cfi_mode == CFI_MODE_JEDEC) {
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v4 1/2] [MTD] remove bogus warning about missing boot bank location
2010-04-23 13:33 [PATCH v4 1/2] [MTD] remove bogus warning about missing boot bank location Guillaume LECERF
2010-04-23 13:33 ` [PATCH v4 2/2] mtd: cfi_cmdset_0002: do not fail on no extended query table as they are both optional Guillaume LECERF
@ 2010-04-24 5:13 ` Chris Moore
2010-04-24 9:35 ` Wolfram Sang
2 siblings, 0 replies; 4+ messages in thread
From: Chris Moore @ 2010-04-24 5:13 UTC (permalink / raw)
To: Guillaume LECERF, David Woodhouse
Cc: linux-mtd, Wolfram Sang, Artem Bityutskiy
Le 23/04/2010 15:33, Guillaume LECERF a écrit :
> From: Uwe Kleine-König<Uwe.Kleine-Koenigi@digi.com>
>
> 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 ea2a7f6..8da8655 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -391,11 +391,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);
>
>
FWIW:-
Acked-by Christopher Moore <moore@free.fr>
^ permalink raw reply [flat|nested] 4+ messages in thread