| Message ID | 20081216205650.GA13729@zealous.synapse.com |
|---|---|
| State | New, archived |
| Headers | show |
Hi Marc and the list,
I am afraid to admit that I was the author of the modifications that you
obviously did not like :(
Let me also admit that I am a newbie to MTD, to CFI and to NOR flash
devices so please correct me if I write anything stupid.
You say in the subject "AMD implemtation restore to original version
that has worked fine since 2001".
You seem to imply by this that my modifications broke some previously
working AMD parts.
I apologize if this is the case but I cannot see how.
Which AMD part(s) did I break?
(However I did realize after submitting my patch that it could possibly
break certain Macronix parts using AMD CFI PRI V1.0 that were previously
working more by luck than judgement.)
Before submitting my patch I carefully considered whether to modify the
AMD fixup or to add a Macronix specific one as you did.
It seemed to me that:-
a) In the context of the fixup AMD referred to the "Primary Vendor
Command Set and Control Interface ID Code" used and not to the
manufacturer of the part.
For this Macronix also use the "AMD/Fujitsu Standard Command Set".
b) Although AIUI each manufacturer is free to choose his own Device ID
allocations, in practice most manufacturers use the same Device ID when
they clone another manufacturer's part.
For example IIRC, AMD, Fujitsu, Spansion, EON, ESI and Macronix all use:-
- 0xBA/0x22BA for their 29LV400 B parts
- 0xB9/0x22B9 for their 29LV400 T parts
I therefore concluded that it was legitimate and useful in terms of
avoiding duplicate code to handle Macronix in the AMD fixup routine.
You prefer to separate the fixup routines and I accept your decision (it
was a close call for me).
However I do have one objection:-
Your Macronix version is not equivalent to mine and I believe that my
version handles more parts correctly ;-)
Instead of:-
+ switch (cfi->id) {
+ /* Macronix MX29LV400CT */
+ case 0x00ba:
+ case 0x22ba:
+ extp->TopBottom = 2; /* bottom-boot */
+ break;
+ /* Macronix MX29LV400CB */
+ case 0x00b9:
+ case 0x22b9:
+ extp->TopBottom = 3; /* top-boot */
+ break;
+ default:
+ /* Fall back is to assume we have
+ * bottom-boot. */
+ extp->TopBottom = 2; /* bottom-boot */
+ break;
+ }
I would prefer:-
+ switch (cfi->id) {
+ /* Macronix MX29LV400CB */
+ case 0x00ba:
+ case 0x22ba:
+ extp->TopBottom = 2; /* bottom-boot */
+ break;
+ default:
+ extp->TopBottom = (cfi->id & 0x80)
+ ? 3 /* top-boot */
+ : 2 /* bottom-boot */;
+ break;
+ }
David Woodhouse asked me to add correct handling of a few Macronix parts
which are incorrectly detected by my code and I shall do this once your
patch goes through.
There is one other point that I would like to make (again).
IMHO the patch from Uwe Kleine-Koenig in this thread should be applied:-
http://thread.gmane.org/gmane.linux.drivers.mtd/22267
See also this thread:-
http://thread.gmane.org/gmane.linux.drivers.mtd/22176
Sorry for this long ramble.
Cheers,
Chris
Chris Moore wrote: > Hi Marc and the list, > > I am afraid to admit that I was the author of the modifications that > you obviously did not like :( > Let me also admit that I am a newbie to MTD, to CFI and to NOR flash > devices so please correct me if I write anything stupid. No worries. > > You say in the subject "AMD implemtation restore to original version > that has worked fine since 2001". > You seem to imply by this that my modifications broke some previously > working AMD parts. That isn't what I meant. > I apologize if this is the case but I cannot see how. > Which AMD part(s) did I break? > (However I did realize after submitting my patch that it could > possibly break certain Macronix parts using AMD CFI PRI V1.0 that were > previously working more by luck than judgement.) > > Before submitting my patch I carefully considered whether to modify > the AMD fixup or to add a Macronix specific one as you did. > It seemed to me that:- > a) In the context of the fixup AMD referred to the "Primary Vendor > Command Set and Control Interface ID Code" used and not to the > manufacturer of the part. > For this Macronix also use the "AMD/Fujitsu Standard Command Set". > b) Although AIUI each manufacturer is free to choose his own Device ID > allocations, in practice most manufacturers use the same Device ID > when they clone another manufacturer's part. > For example IIRC, AMD, Fujitsu, Spansion, EON, ESI and Macronix all use:- > - 0xBA/0x22BA for their 29LV400 B parts > - 0xB9/0x22B9 for their 29LV400 T parts > I therefore concluded that it was legitimate and useful in terms of > avoiding duplicate code to handle Macronix in the AMD fixup routine. > You prefer to separate the fixup routines and I accept your decision > (it was a close call for me). The only risky part is that your code depended, implicitly, on the fact that both of the Macronix IDs had the high bit set. IMHO, it is more clear and safer to be explicit in the way that the code will execute. Your wish to avoid duplicate code seems odd in that you check for the macronix ID in the fixup. Someone reading the code is going to have to know that the amd_fixup is called for Macronix parts, but that would be unexpected given the way that the > > However I do have one objection:- > Your Macronix version is not equivalent to mine and I believe that my > version handles more parts correctly ;-) > > Instead of:- > > + switch (cfi->id) { > + /* Macronix MX29LV400CT */ > + case 0x00ba: > + case 0x22ba: > + extp->TopBottom = 2; /* > bottom-boot */ > + break; > + /* Macronix MX29LV400CB */ > + case 0x00b9: > + case 0x22b9: > + extp->TopBottom = 3; /* > top-boot */ > + break; > + default: > + /* Fall back is to assume we have > + * bottom-boot. */ > + extp->TopBottom = 2; /* bottom-boot */ > + break; > + } > > > I would prefer:- > > + switch (cfi->id) { > + /* Macronix MX29LV400CB */ > + case 0x00ba: > + case 0x22ba: > + extp->TopBottom = 2; /* > bottom-boot */ > + break; > + default: > + extp->TopBottom = (cfi->id & 0x80) > + ? 3 /* top-boot */ > + : 2 /* bottom-boot */; > + break; > + } > This is part of the unclear portion of the fixup. Do you know of Macronix parts that can depend on the ID bit 7 selecting top versus bottom boot? > > David Woodhouse asked me to add correct handling of a few Macronix > parts which are incorrectly detected by my code and I shall do this > once your patch goes through. Which parts are those? I can just resubmit with them...or you can submit a patch that takes care of all of this. > > There is one other point that I would like to make (again). > IMHO the patch from Uwe Kleine-Koenig in this thread should be applied:- > http://thread.gmane.org/gmane.linux.drivers.mtd/22267 > See also this thread:- > http://thread.gmane.org/gmane.linux.drivers.mtd/22176 > I'll check on this, but it may be a week before I can do so as I'm travelling soon. > Sorry for this long ramble. > > Cheers, > Chris > Thanks for the response.
Hi Marc, Thank you for your reply. I removed Uwe from the CC list. This may be bad form but this part of the discussion does not concern his patch. Marc Oscar Singer a écrit : > Chris Moore wrote: > >> Before submitting my patch I carefully considered whether to modify >> the AMD fixup or to add a Macronix specific one as you did. >> It seemed to me that:- >> a) In the context of the fixup AMD referred to the "Primary Vendor >> Command Set and Control Interface ID Code" used and not to the >> manufacturer of the part. >> For this Macronix also use the "AMD/Fujitsu Standard Command Set". >> b) Although AIUI each manufacturer is free to choose his own Device ID >> allocations, in practice most manufacturers use the same Device ID >> when they clone another manufacturer's part. >> For example IIRC, AMD, Fujitsu, Spansion, EON, ESI and Macronix all use:- >> - 0xBA/0x22BA for their 29LV400 B parts >> - 0xB9/0x22B9 for their 29LV400 T parts >> I therefore concluded that it was legitimate and useful in terms of >> avoiding duplicate code to handle Macronix in the AMD fixup routine. >> You prefer to separate the fixup routines and I accept your decision >> (it was a close call for me). >> > The only risky part is that your code depended, implicitly, on the fact > that both of the Macronix IDs had the high bit set. > Both ? I count around 23 ;-) In the commit comments for my patch I cited the following Macronix parts with AMD CFI PRI V1.0:- It detects the following parts correctly:- MX28F640C3B T MX29LV002C B MX29LV002NC B MX29LV004C T MX29LV400C T/B MX29LV800C T/B MX29LV160C T/B MX29SL800C T/B MX29SL802C T/B It detects the following uniform part as bottom but it should work correctly:- MX29LV040C It does not detect the following correctly:- MX28F640C3B B MX29LV002C T MX29LV002NC T MX29LV004C B MX29SL400C T/B MX29SL402C T/B > IMHO, it is more clear and safer to be explicit in the way that the code > will execute. > > It seemed clear to me but one's own code always does ... at least at the time of writing it ;-) > Your wish to avoid duplicate code seems odd in that you check for the > macronix ID in the fixup. Someone reading the code > is going to have to know that the amd_fixup is called for Macronix > parts, but that would be unexpected given the way > that the > IMHO the only real problem is that, as you say, the name of the function is not very clear. Maybe I should have changed it to something like fixup_amd_cfi_pri_v1_0_bootblock(). However in my patch I wished to change as little as possible. >> However I do have one objection:- >> Your Macronix version is not equivalent to mine and I believe that my >> version handles more parts correctly ;-) >> >> Instead of:- >> >> + switch (cfi->id) { >> + /* Macronix MX29LV400CT */ >> + case 0x00ba: >> + case 0x22ba: >> + extp->TopBottom = 2; /* >> bottom-boot */ >> + break; >> + /* Macronix MX29LV400CB */ >> + case 0x00b9: >> + case 0x22b9: >> + extp->TopBottom = 3; /* >> top-boot */ >> + break; >> + default: >> + /* Fall back is to assume we have >> + * bottom-boot. */ >> + extp->TopBottom = 2; /* bottom-boot */ >> + break; >> + } >> >> >> I would prefer:- >> >> + switch (cfi->id) { >> + /* Macronix MX29LV400CB */ >> + case 0x00ba: >> + case 0x22ba: >> + extp->TopBottom = 2; /* >> bottom-boot */ >> + break; >> + default: >> + extp->TopBottom = (cfi->id & 0x80) >> + ? 3 /* top-boot */ >> + : 2 /* bottom-boot */; >> + break; >> + } >> >> > This is part of the unclear portion of the fixup. Do you know of > Macronix parts that can depend on the ID bit 7 selecting top versus > bottom boot? > Yes, the following at least and maybe more:- MX29LV800C T/B MX29LV160C T/B MX29SL800C T/B MX29SL802C T/B >> David Woodhouse asked me to add correct handling of a few Macronix >> parts which are incorrectly detected by my code and I shall do this >> once your patch goes through. >> > Which parts are those? I can just resubmit with them...or you can > submit a patch that takes care of all of this. > > MX28F640C3B B MX29LV002C T MX29LV002NC T MX29LV004C B MX29SL400C T/B MX29SL402C T/B I could submit a patch but probably not until after Christmas. I think the best plan is that if you really want to make this change (with which I do not really agree) then:- - Put through your patch, preferably revised to leave the ID bit 7 test. - I shall then submit another patch on top to take care of the remaining cases. But as I wrote above, personally I think that all that is necessary is to give the fixup function a clearer name. > I'll check on this, but it may be a week before I can do so as I'm > travelling soon. > I'll be away next week too. Have a Happy Christmas. Cheers, Chris
IMHO, it would be better to leave the AMD fixup code as it was. It works fine for the AMD parts. My comment about changing what was already working has to do with the fact that editing the AMD fixup code is modifying working code. The number of bytes saved because we don't have to check bit 7 twice in the default is nominal. The fixup table was changed to call the same function for two different manufacturers, so we had to add an explicit check for the manufacturer in the fixup routine. Why bother? The AMD fixup is stable and working. As far as I know, AMD isn't making more NOR flash, so we are in a good position for *not* breaking support for AMD parts. So, instead, lets write a new fixup for the Macronix part. Yes, we have to duplicate the bit 7 test, but the routine, overall, can be a straightforward switch on the exceptional IDs with the default still checking bit 7. So, the principles are 1) leave working code alone. 2) use existing mechanisms as they were intended (i.e. the fixup table that selects by manufacturer) If you agree, then you should be able to make your changes on top of the patch that I already submitted. Cheers.
Marc Oscar Singer a écrit : > IMHO, it would be better to leave the AMD fixup code as it was. It > works fine for the AMD parts. My comment about > changing what was already working has to do with the fact that editing > the AMD fixup code is modifying working code. > The number of bytes saved because we don't have to check bit 7 twice in > the default is nominal. The savings are certainly not enormous but they are greater than you suggest. Your patch also duplicates the fixup function and the CFI PRI V1.0 test. > The fixup table > was changed to call the same function for two different manufacturers, > so we had to add an explicit check for the manufacturer > in the fixup routine. Why bother? The explicit manufacturer check is in fact unnecessary because:- AMD also use the 0xBA/0x22BA device ID for their Am29LV400B part. (As do Fujitsu, Spansion, EON and ESI.for their 29LV400B equivalents.) However AFAIK none of these have CFI. (And no new version is likely to have CFI V1.0.) And, as I said previously, the fact that in practice different manufacturers use the same device ID for their clones was one of my major reasons for choosing a common CFI V1.0 fixup routine. For the record: I chose to add the manufacturer check in order to make it obvious that my patch could not break any AMD part. I am a newbie to MTD but I wanted MX29LV400C B support.to be added quickly as I maintain a NAS which uses this part. To get my patch accepted I figured that I had to make it clear: a) that it supported my part, b) that it broke nothing, c) that I had done my homework (concerning which parts were supported or not). In the end David Woodhouse kindly integrated my patch extremely quickly. (I thought I had missed the merge window but it scraped into 2.6.28-rc1.) > The AMD fixup is stable and > working. As far as I know, AMD isn't making more NOR flash, > so we are in a good position for *not* breaking support for AMD parts. > > I find it ironic that you mention breaking support for parts since: a) we both seem to agree that my patch broke no AMD parts, b) OTOH your patch breaks support for the following Macronix parts: MX28F640C3B T MX29LV004C T MX29LV800C T MX29LV160C T MX29SL800C T MX29SL802C T > So, instead, lets write a new fixup for the Macronix part. Yes, we have > to duplicate the bit 7 test, but the routine, overall, can > be a straightforward switch on the exceptional IDs with the default > still checking bit 7. > > So, the principles are > > 1) leave working code alone. > I agree that working code should not be modified gratuitously However IMHO modification of working code is permissible when the purpose is to add functionality (or even as a clean up) provided it is obvious that nothing is broken. > 2) use existing mechanisms as they were intended (i.e. the fixup table > that selects by manufacturer) > > I do see your point on this one. However, as I said previously, it seemed to me that there were advantages in handling the fixup table by CFI PRI type rather than by the actual manufacturer of the part. I admit that this is dubious with the existing fixup function name and I suggested changing the name. Nevertheless fixup tables by manufacturer are OK with me. > If you agree, then you should be able to make your changes on top of the > patch that I already submitted. > > Yes, of course. Do you want your patch in 2.6.29-rc1? How much time do I have to get my patch on top? Do you think that my patches would be accepted later in the rc cycle? However frankly IMHO your patch needs some changes first. I shall indicate these more clearly in a parallel thread. In particular IMHO the unnecessary case statements will be NACKed in mainline. Do you know who maintains cfi_cmdset_0002.c? The file says it is Thayne Harbaugh but AFAICS he has not posted to the list for four years. Is it David Woodhouse? Is it you? I am afraid that I shall probably not be able even to reply in the next ten days. Cheers, Chris
NACK See also the discussion in a parallel thread. Marc Singer a écrit : > Signed-off-by: Marc Singer <elf@zealous.synapse.com> > --- > drivers/mtd/chips/cfi_cmdset_0002.c | 96 ++++++++++++++++++++++------------- > 1 files changed, 60 insertions(+), 36 deletions(-) > > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c > index d74ec46..4f1b445 100644 > --- a/drivers/mtd/chips/cfi_cmdset_0002.c > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > @@ -136,7 +136,21 @@ static void cfi_tell_features(struct cfi_pri_amdstd *extp) > #endif > > #ifdef AMD_BOOTLOC_BUG > -/* Wheee. Bring me the head of someone at AMD. */ > +/* Wheee. Bring me the head of someone at AMD, > + and another from Macronix...just for good measure. */ > + > +/* Some early AMD NOR flash parts (AM29LV160D, e.g.) and much > + * more recent and inexcusably broken Macronix parts, do not > + * accurately report whether or not the device is top-boot or > + * bottom-boot in the CFI PRI data. This detail is important > + * to correctly sort the erase region information. So, for > + * CFI versions < 1.1 where we do not trust the veracity of > + * the CFI PRI data, we look for explicit manufacturer/device > + * IDs when we know them or for the high bit of the device ID. > + * The latter test has been working reliably since 2001 even > + * though we don't have documentation to support this as a > + * convention. */ > + > static void fixup_amd_bootblock(struct mtd_info *mtd, void* param) > { > struct map_info *map = mtd->priv; > @@ -148,43 +162,53 @@ static void fixup_amd_bootblock(struct mtd_info *mtd, void* param) > if (((major << 8) | minor) < 0x3131) { > /* CFI version 1.0 => don't trust bootloc */ > > + extp->TopBottom = (cfi->id & 0x80) > + ? 3 /* top-boot */ > + : 2 /* bottom-boot */; > + > DEBUG(MTD_DEBUG_LEVEL1, > - "%s: JEDEC Vendor ID is 0x%02X Device ID is 0x%02X\n", > - map->name, cfi->mfr, cfi->id); > + "%s: CFI PRI V%c.%c has no boot block field;" > + " deduced %s from Mfr/Device ID %0x/%0x\n", > + map->name, major, minor, > + extp->TopBottom == 2 ? "bottom" : "top", > + cfi->mfr, cfi->id); > + } > +} > > - /* AFAICS all 29LV400 with a bottom boot block have a device ID > - * of 0x22BA in 16-bit mode and 0xBA in 8-bit mode. > - * These were badly detected as they have the 0x80 bit set > - * so treat them as a special case. > - */ > - if (((cfi->id == 0xBA) || (cfi->id == 0x22BA)) && > - > - /* Macronix added CFI to their 2nd generation > - * MX29LV400C B/T but AFAICS no other 29LV400 (AMD, > - * Fujitsu, Spansion, EON, ESI and older Macronix) > - * has CFI. > - * > - * Therefore also check the manufacturer. > - * This reduces the risk of false detection due to > - * the 8-bit device ID. > - */ > - (cfi->mfr == MANUFACTURER_MACRONIX)) { > - DEBUG(MTD_DEBUG_LEVEL1, > - "%s: Macronix MX29LV400C with bottom boot block" > - " detected\n", map->name); > - extp->TopBottom = 2; /* bottom boot */ > - } else > - if (cfi->id & 0x80) { > - printk(KERN_WARNING "%s: JEDEC Device ID is 0x%02X. Assuming broken CFI table.\n", map->name, cfi->id); > - extp->TopBottom = 3; /* top boot */ > - } else { > - extp->TopBottom = 2; /* bottom boot */ > - } > +static void fixup_macronix_bootblock(struct mtd_info *mtd, void* param) > +{ > + struct map_info *map = mtd->priv; > + struct cfi_private *cfi = map->fldrv_priv; > + struct cfi_pri_amdstd *extp = cfi->cmdset_priv; > + __u8 major = extp->MajorVersion; > + __u8 minor = extp->MinorVersion; > + > + if (((major << 8) | minor) < 0x3131) { > + /* CFI version 1.0 => don't trust bootloc */ > > + switch (cfi->id) { > + /* Macronix MX29LV400CT */ > This comment is wrong and should be: + /* Macronix MX29LV400CB */ > + case 0x00ba: > + case 0x22ba: > + extp->TopBottom = 2; /* bottom-boot */ > + break; > These cases are unnecessary as they are covered by your default case below. > + /* Macronix MX29LV400CB */ > This comment is wrong and should be: + /* Macronix MX29LV400CT */ > + case 0x00b9: > + case 0x22b9: > + extp->TopBottom = 3; /* top-boot */ > + break; > + default: > + /* Fall back is to assume we have > + * bottom-boot. */ > + extp->TopBottom = 2; /* bottom-boot */ > + break; > + } > This switch code is not equivalent to the original and breaks support for the following Macronix parts: MX28F640C3B T MX29LV004C T MX29LV800C T MX29LV160C T MX29SL800C T MX29SL802C T I would prefer something like this (untested):- + switch (cfi->id) { + /* Macronix MX29LV400CB */ + case 0x00ba: + case 0x22ba: + extp->TopBottom = 2; /* bottom-boot */ + break; + default: + extp->TopBottom = (cfi->id & 0x80) + ? 3 /* top-boot */ + : 2 /* bottom-boot */; + break; + } > DEBUG(MTD_DEBUG_LEVEL1, > - "%s: AMD CFI PRI V%c.%c has no boot block field;" > - " deduced %s from Device ID\n", map->name, major, minor, > - extp->TopBottom == 2 ? "bottom" : "top"); > + "%s: CFI PRI V%c.%c has no boot block field;" > + " deduced %s from Mfr/Device ID %0x/%0x\n", > + map->name, major, minor, > + extp->TopBottom == 2 ? "bottom" : "top", > + cfi->mfr, cfi->id); > } > } > #endif > @@ -286,7 +310,7 @@ static struct cfi_fixup cfi_fixup_table[] = { > { CFI_MFR_ATMEL, CFI_ID_ANY, fixup_convert_atmel_pri, NULL }, > #ifdef AMD_BOOTLOC_BUG > { CFI_MFR_AMD, CFI_ID_ANY, fixup_amd_bootblock, NULL }, > - { MANUFACTURER_MACRONIX, CFI_ID_ANY, fixup_amd_bootblock, NULL }, > + { MANUFACTURER_MACRONIX, CFI_ID_ANY, fixup_macronix_bootblock, NULL }, > #endif > { CFI_MFR_AMD, 0x0050, fixup_use_secsi, NULL, }, > { CFI_MFR_AMD, 0x0053, fixup_use_secsi, NULL, }, >
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c index d74ec46..4f1b445 100644 --- a/drivers/mtd/chips/cfi_cmdset_0002.c +++ b/drivers/mtd/chips/cfi_cmdset_0002.c @@ -136,7 +136,21 @@ static void cfi_tell_features(struct cfi_pri_amdstd *extp) #endif #ifdef AMD_BOOTLOC_BUG -/* Wheee. Bring me the head of someone at AMD. */ +/* Wheee. Bring me the head of someone at AMD, + and another from Macronix...just for good measure. */ + +/* Some early AMD NOR flash parts (AM29LV160D, e.g.) and much + * more recent and inexcusably broken Macronix parts, do not + * accurately report whether or not the device is top-boot or + * bottom-boot in the CFI PRI data. This detail is important + * to correctly sort the erase region information. So, for + * CFI versions < 1.1 where we do not trust the veracity of + * the CFI PRI data, we look for explicit manufacturer/device + * IDs when we know them or for the high bit of the device ID. + * The latter test has been working reliably since 2001 even + * though we don't have documentation to support this as a + * convention. */ + static void fixup_amd_bootblock(struct mtd_info *mtd, void* param) { struct map_info *map = mtd->priv; @@ -148,43 +162,53 @@ static void fixup_amd_bootblock(struct mtd_info *mtd, void* param) if (((major << 8) | minor) < 0x3131) { /* CFI version 1.0 => don't trust bootloc */ + extp->TopBottom = (cfi->id & 0x80) + ? 3 /* top-boot */ + : 2 /* bottom-boot */; + DEBUG(MTD_DEBUG_LEVEL1, - "%s: JEDEC Vendor ID is 0x%02X Device ID is 0x%02X\n", - map->name, cfi->mfr, cfi->id); + "%s: CFI PRI V%c.%c has no boot block field;" + " deduced %s from Mfr/Device ID %0x/%0x\n", + map->name, major, minor, + extp->TopBottom == 2 ? "bottom" : "top", + cfi->mfr, cfi->id); + } +} - /* AFAICS all 29LV400 with a bottom boot block have a device ID - * of 0x22BA in 16-bit mode and 0xBA in 8-bit mode. - * These were badly detected as they have the 0x80 bit set - * so treat them as a special case. - */ - if (((cfi->id == 0xBA) || (cfi->id == 0x22BA)) && - - /* Macronix added CFI to their 2nd generation - * MX29LV400C B/T but AFAICS no other 29LV400 (AMD, - * Fujitsu, Spansion, EON, ESI and older Macronix) - * has CFI. - * - * Therefore also check the manufacturer. - * This reduces the risk of false detection due to - * the 8-bit device ID. - */ - (cfi->mfr == MANUFACTURER_MACRONIX)) { - DEBUG(MTD_DEBUG_LEVEL1, - "%s: Macronix MX29LV400C with bottom boot block" - " detected\n", map->name); - extp->TopBottom = 2; /* bottom boot */ - } else - if (cfi->id & 0x80) { - printk(KERN_WARNING "%s: JEDEC Device ID is 0x%02X. Assuming broken CFI table.\n", map->name, cfi->id); - extp->TopBottom = 3; /* top boot */ - } else { - extp->TopBottom = 2; /* bottom boot */ - } +static void fixup_macronix_bootblock(struct mtd_info *mtd, void* param) +{ + struct map_info *map = mtd->priv; + struct cfi_private *cfi = map->fldrv_priv; + struct cfi_pri_amdstd *extp = cfi->cmdset_priv; + __u8 major = extp->MajorVersion; + __u8 minor = extp->MinorVersion; + + if (((major << 8) | minor) < 0x3131) { + /* CFI version 1.0 => don't trust bootloc */ + switch (cfi->id) { + /* Macronix MX29LV400CT */ + case 0x00ba: + case 0x22ba: + extp->TopBottom = 2; /* bottom-boot */ + break; + /* Macronix MX29LV400CB */ + case 0x00b9: + case 0x22b9: + extp->TopBottom = 3; /* top-boot */ + break; + default: + /* Fall back is to assume we have + * bottom-boot. */ + extp->TopBottom = 2; /* bottom-boot */ + break; + } DEBUG(MTD_DEBUG_LEVEL1, - "%s: AMD CFI PRI V%c.%c has no boot block field;" - " deduced %s from Device ID\n", map->name, major, minor, - extp->TopBottom == 2 ? "bottom" : "top"); + "%s: CFI PRI V%c.%c has no boot block field;" + " deduced %s from Mfr/Device ID %0x/%0x\n", + map->name, major, minor, + extp->TopBottom == 2 ? "bottom" : "top", + cfi->mfr, cfi->id); } } #endif @@ -286,7 +310,7 @@ static struct cfi_fixup cfi_fixup_table[] = { { CFI_MFR_ATMEL, CFI_ID_ANY, fixup_convert_atmel_pri, NULL }, #ifdef AMD_BOOTLOC_BUG { CFI_MFR_AMD, CFI_ID_ANY, fixup_amd_bootblock, NULL }, - { MANUFACTURER_MACRONIX, CFI_ID_ANY, fixup_amd_bootblock, NULL }, + { MANUFACTURER_MACRONIX, CFI_ID_ANY, fixup_macronix_bootblock, NULL }, #endif { CFI_MFR_AMD, 0x0050, fixup_use_secsi, NULL, }, { CFI_MFR_AMD, 0x0053, fixup_use_secsi, NULL, },
Signed-off-by: Marc Singer <elf@zealous.synapse.com> --- drivers/mtd/chips/cfi_cmdset_0002.c | 96 ++++++++++++++++++++++------------- 1 files changed, 60 insertions(+), 36 deletions(-)