Problems with cfi_cmdset_0002.c

Problems with cfi_cmdset_0002.c

[Jonas Holmberg]

Hi,

I'm working on a MTD driver for AMD compatible non-CFI flash chips. Since the command set for those chips is almost the same as the one in cfi_cmdset_0002.c I have been testing that code a lot lately. I have found two "bugs". Firstly the if-statement on line 63 (current in CVS) does not work for my flash chip (AM29LV160DT):

        /* Wheee. Bring me the head of someone at AMD. */
        if (bootloc == 3 && cfi->cfiq->NumEraseRegions > 1) {
                printk("Swapping erase regions for broken CFI table\n");

The expression is false, but the EraseRegionInfo should be swapped. I have no solution to that problem. Can you think of one?

Secondly, the if-statement on line 660:

	if (adr % (1<< cfi->chipshift) == ((regions[i].erasesize * regions[i].numblocks) %( 1<< cfi->chipshift)))

should be:

	if (adr % (1<< cfi->chipshift) == ((regions[i].offset + (regions[i].erasesize * regions[i].numblocks)) %( 1<< cfi->chipshift)))

Best regards
/Jonas Holmberg, Axis Communications


To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Problems with cfi_cmdset_0002.c

[David Woodhouse]

jonas.holmberg@axis.com said:
>      /* Wheee. Bring me the head of someone at AMD. */
>         if (bootloc == 3 && cfi->cfiq->NumEraseRegions > 1) {
>                 printk("Swapping erase regions for broken CFI table\n");

> The expression is false, but the EraseRegionInfo should be swapped. I
> have no solution to that problem. Can you think of one? 

Well, if you can bring me the person at AMD responsible for making the CFI 
table report the erase regions in the wrong order in the first place, I can 
certainly think of a few suggestions. OK, I _know_ the CFI spec doesn't 
explicitly say that the erase regions listed in the CFI tables have to be 
in order - but a modicum of common sense would have helped. 

The 'bootloc == 3' check is supposed to be for devices with the smallest 
erase blocks at the top - "Top Boot" devices. Are your devices "Bottom 
Boot" devices and _still_ have the regions listed backwards? In which case, 
we should probably omit the check for bootloc - they're _all_ backwards. I 
put that in because I assumed they'd at least got it right _somewhere_.

Does anyone else have an AMD bottom-boot device where the CFI table is 
correct and doesn't need swapping? If so, maybe we're going to have to have 
a table listing which device IDs need swapping and which don't. 

Unless someone speaks up fairly quickly, let's assume that _all_ the tables 
are backwards and take out the check for bootloc. If that breaks later, we 
can come up with something better.


jonas.holmberg@axis.com said:
> Secondly, the if-statement on line 660:

Yeah. Please commit. 

--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

RE: Problems with cfi_cmdset_0002.c

[Jonas Holmberg]

> The 'bootloc == 3' check is supposed to be for devices with 
> the smallest 
> erase blocks at the top - "Top Boot" devices. Are your 
> devices "Bottom 
> Boot" devices and _still_ have the regions listed backwards? 
> In which case, 
> we should probably omit the check for bootloc - they're _all_ 
> backwards. I 
> put that in because I assumed they'd at least got it right 
> _somewhere_.
> 
> Does anyone else have an AMD bottom-boot device where the CFI 
> table is 
> correct and doesn't need swapping? If so, maybe we're going 
> to have to have 
> a table listing which device IDs need swapping and which don't. 
> 
> Unless someone speaks up fairly quickly, let's assume that 
> _all_ the tables 
> are backwards and take out the check for bootloc. If that 
> breaks later, we 
> can come up with something better.

I have now tested with a bottom boot chip also and it works (it is not swapped and should not be). So the problem is how to find out that the top boot chip is a top boot chip. A table does not sound like a very nice solution, but I don't know of any other. The datasheet for the flash chip does not mention the address used to read bootloc:

	bootloc = cfi_read_query(map, (adr+15)*ofs_factor);

> 
> 
> jonas.holmberg@axis.com said:
> > Secondly, the if-statement on line 660:
> 
> Yeah. Please commit. 
> 

I haven't got commit access...

/Jonas


To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Problems with cfi_cmdset_0002.c

[David Woodhouse]

jonas.holmberg@axis.com said:
>  I have now tested with a bottom boot chip also and it works (it is
> not swapped and should not be). So the problem is how to find out that
> the top boot chip is a top boot chip. A table does not sound like a
> very nice solution, but I don't know of any other. The datasheet for
> the flash chip does not mention the address used to read bootloc:

> 	bootloc = cfi_read_query(map, (adr+15)*ofs_factor); 

The datasheet for the AM29LV320D lists it - page 23 of AMD publication 
#23579, Rev A; issued Nov 1, 2000.

Personally, my inclination would be to return the poxy things to AMD
complaining that the CFI tables are broken. How many are you buying?

Otherwise, I think the JEDEC ID may be the only option.


jonas.holmberg@axis.com said:
>  I haven't got commit access...

Gimme SSH public key.

--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Problems with cfi_cmdset_0002.c

[Johan Adolfsson]

Wouldn't it be possible to check both the bootloc and the erase regions and
do something like this? :

(Haven't looked in the code how the cfi structure is typed etc. and may be
totally
out when it comes to what the problem is, but you'll probably get the idea)

flip_cfi_table = 0;
if (bootloc == CFI_TOP_BOOT){
  /* We expect the small regions at the top */
  if (EraseRegionInfo[0].size <
       EraseRegionInfo[cfi->cfiq->NumEraseRegions-1].size){
    flip_cfi_table = 1;
 } else if (bootloc == CFI_BOTTOM_BOOT){
  /* We expect the small regions at the bottom */
  if (EraseRegionInfo[0].size >
       EraseRegionInfo[cfi->cfiq->NumEraseRegions-1].size){
    flip_cfi_table = 1;
 }

 if (flip_cfi_table){
   /* Flip the damn thing */
  ...
}

/Johan


----- Original Message -----
From: David Woodhouse <dwmw2@infradead.org>
To: Jonas Holmberg <jonashg@axis.com>
Cc: <mtd@infradead.org>
Sent: Monday, February 12, 2001 17:36
Subject: Re: Problems with cfi_cmdset_0002.c


>
> jonas.holmberg@axis.com said:
> >  I have now tested with a bottom boot chip also and it works (it is
> > not swapped and should not be). So the problem is how to find out that
> > the top boot chip is a top boot chip. A table does not sound like a
> > very nice solution, but I don't know of any other. The datasheet for
> > the flash chip does not mention the address used to read bootloc:
>
> > bootloc = cfi_read_query(map, (adr+15)*ofs_factor);
>
> The datasheet for the AM29LV320D lists it - page 23 of AMD publication
> #23579, Rev A; issued Nov 1, 2000.
>
> Personally, my inclination would be to return the poxy things to AMD
> complaining that the CFI tables are broken. How many are you buying?
>
> Otherwise, I think the JEDEC ID may be the only option.
>
>
> jonas.holmberg@axis.com said:
> >  I haven't got commit access...
>
> Gimme SSH public key.
>
> --
> dwmw2
>
>
>
>
> To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org
>



To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Problems with cfi_cmdset_0002.c

[David Woodhouse]

johan.adolfsson@axis.com said:
> Wouldn't it be possible to check both the bootloc and the erase
> regions and do something like this? :

Would that help? I thought that _all_ the AMD chips had the smallest 
regions listed first - that's the whole problem. But the top-boot chips you 
have don't have bootloc == CFI_BOTTOM_BOOT. Or am I misunderstanding?

If the erase regions are listed the wrong way round _and_ the byte in the 
extended header isn't set appropriately to tell you so, these AMD chips 
really are broken. We need to work round them explicitly with a table of 
JEDEC IDs for the broken chips, or just reject them and buy Intel chips 
instead.
--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Problems with cfi_cmdset_0002.c

[Johan Adolfsson]

----- Original Message -----
From: David Woodhouse <dwmw2@infradead.org>
To: Johan Adolfsson <johana@axis.com>
Cc: Jonas Holmberg <jonashg@axis.com>; <mtd@infradead.org>
Sent: Monday, February 12, 2001 18:24
Subject: Re: Problems with cfi_cmdset_0002.c


>
> johan.adolfsson@axis.com said:
> > Wouldn't it be possible to check both the bootloc and the erase
> > regions and do something like this? :
>
> Would that help? I thought that _all_ the AMD chips had the smallest
> regions listed first - that's the whole problem. But the top-boot chips
you
> have don't have bootloc == CFI_BOTTOM_BOOT. Or am I misunderstanding?

It's probably mee...:-)
Jonas has those flash chips, I dont.
What does the bootloc say? Does it says something other the 2 and 3?

> If the erase regions are listed the wrong way round _and_ the byte in the
> extended header isn't set appropriately to tell you so, these AMD chips
> really are broken. We need to work round them explicitly with a table of
> JEDEC IDs for the broken chips, or just reject them and buy Intel chips
> instead.

We need a table like that for other AMD compatible non CFI chips as well
(e.g. Toshiba).
Jonas has a solution that seems to work here, I guess it should go into the
mtd tree soner or later?
We probably want the check against the list before doing the CFI probe.

> --
> dwmw2

/Johan




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Problems with cfi_cmdset_0002.c

[Johan Adolfsson]

----- Original Message -----
From: Jonas Holmberg <jonashg@axis.com>
To: Johan Adolfsson <johana@axis.com>; David Woodhouse <dwmw2@infradead.org>
Cc: <mtd@infradead.org>
Sent: Tuesday, February 13, 2001 09:31
Subject: RE: Problems with cfi_cmdset_0002.c


> > What does the bootloc say? Does it says something other the 2 and 3?
> >
>
> I have two AM29LV160D one top and one bottom boot and the both say
bootloc==0 :(
> I checked the datasheets and the AM29LV160D does not have 004F (bootloc),
like the AM29LV320D cips do. I'll try to find some other way of determining
the boot location.

How does the erase regions look like for these chips?

/Johan




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

RE: Problems with cfi_cmdset_0002.c

[Jonas Holmberg]

> What does the bootloc say? Does it says something other the 2 and 3?
> 

I have two AM29LV160D one top and one bottom boot and the both say bootloc==0 :(
I checked the datasheets and the AM29LV160D does not have 004F (bootloc), like the AM29LV320D cips do. I'll try to find some other way of determining the boot location.

> > If the erase regions are listed the wrong way round _and_ 
> the byte in the
> > extended header isn't set appropriately to tell you so, 
> these AMD chips
> > really are broken. We need to work round them explicitly 
> with a table of
> > JEDEC IDs for the broken chips, or just reject them and buy 
> Intel chips
> > instead.
> 

We do buy a lot of AMD compatible flash chips, so we have to come up with a solution I'm afraid.

> We need a table like that for other AMD compatible non CFI 
> chips as well
> (e.g. Toshiba).
> Jonas has a solution that seems to work here, I guess it 
> should go into the
> mtd tree soner or later?
> We probably want the check against the list before doing the 
> CFI probe.

I'm also starting to dislike AMD flash chips...

/Jonas


To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

RE: Problems with cfi_cmdset_0002.c

[Jonas Holmberg]

> > I have two AM29LV160D one top and one bottom boot and the both say
> bootloc==0 :(
> > I checked the datasheets and the AM29LV160D does not have 
> 004F (bootloc),
> like the AM29LV320D cips do. I'll try to find some other way 
> of determining
> the boot location.
> 
> How does the erase regions look like for these chips?
> 

When CFI-probed they say that they are identical, but in reality they aren't. That's the problem. The only thing that differs that I have found so far is the device ID. But device IDs aren't the same for different manufacturers...

/Jonas


To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Problems with cfi_cmdset_0002.c

[David Woodhouse]

jonas.holmberg@axis.com said:
> I have two AM29LV160D one top and one bottom boot and the both say
> bootloc==0 :(

One of these days I really am going to painfully slaughter a hardware 
designer. The engineers at an unnamed company who decided to route the 
PCMCIA socket's interrupt line to a general purpose I/O line on the SH3 CPU 
without any possibility of generating an interrupt were prime targets a few 
weeks ago. AMD are in that spot now. Strange - the chip manufacturers are 
generally more clueful - it's the monkeys who glue them together who 
usually screw it up (present company excepted :)

Is there _any_ difference between these chips which is detectable without 
actually trying to erase the damn things and observing how much data gets 
removed?

--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Problems with cfi_cmdset_0002.c

[Johan Adolfsson]

Maybe we need
CONFIG_MTD_IF_ALL_ELSE_FAILS_ASSUME_TOPBOOT
CONFIG_MTD_IF_ALL_ELSE_FAILS_ASSUME_BOTTOMBOOT

:o)

----- Original Message -----
From: David Woodhouse <dwmw2@infradead.org>
To: Jonas Holmberg <jonashg@axis.com>
Cc: Johan Adolfsson <johana@axis.com>; <mtd@infradead.org>
Sent: Tuesday, February 13, 2001 10:17
Subject: Re: Problems with cfi_cmdset_0002.c


>
> jonas.holmberg@axis.com said:
> > I have two AM29LV160D one top and one bottom boot and the both say
> > bootloc==0 :(
>
> One of these days I really am going to painfully slaughter a hardware
> designer. The engineers at an unnamed company who decided to route the
> PCMCIA socket's interrupt line to a general purpose I/O line on the SH3
CPU
> without any possibility of generating an interrupt were prime targets a
few
> weeks ago. AMD are in that spot now. Strange - the chip manufacturers are
> generally more clueful - it's the monkeys who glue them together who
> usually screw it up (present company excepted :)
>
> Is there _any_ difference between these chips which is detectable without
> actually trying to erase the damn things and observing how much data gets
> removed?
>
> --
> dwmw2
>
>



To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

RE: Problems with cfi_cmdset_0002.c

[Jonas Holmberg]

> Is there _any_ difference between these chips which is 
> detectable without 
> actually trying to erase the damn things and observing how 
> much data gets 
> removed?
> 

I may have found something. Take a look at the following table. It includes some AMD, Toshiba and ST flash chips.

Name            dev_id  last_byte_of_dev_id
---------------------------------
AM29F800BT      0x22D6  1101 0110
AM29LV800BT     0x22DA  1101 1010
AM29LV160DT     0x22C4  1100 0100
MBM29LV160TE    0x22C4  1100 0100
M29W800T        0x00D7  1101 0111
M29W160DT       0x22C4  1100 0100
TC58FVT160      0x00C2  1100 0010
                        ^
                        All 1s

M29W160DB       0x2249  0100 1001
MBM29LV160BE    0x2249  0100 1001
AM29LV160DB     0x2249  0100 1001
AM29LV800BB     0x225B  0101 1011
AM29F800BB      0x2258  0101 1000
TC58FVB160      0x0043  0100 0011
                        ^
                        All 0s
				
It looks like the eigth bit in the last byte of the device ID. It is 1 for top boot chips and 0 for bottom boot chips! Does anyone have the device IDs for any other chips? In that case, please confirm this theory.

/Jonas


To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Problems with cfi_cmdset_0002.c

[David Woodhouse]

jonas.holmberg@axis.com said:
>  It looks like the eigth bit in the last byte of the device ID. It is
> 1 for top boot chips and 0 for bottom boot chips! Does anyone have the
> device IDs for any other chips? In that case, please confirm this
> theory. 

Looks like as good a heuristic as any. Just wrap it in an ifdef and we can 
remove it or make it a config option if it causes trouble.

	__u8 maj,min;
	int swap = 0;

	maj = cfi_read_query(map, (adr+3)*ofs_factor);
	min = cfi_read_query(map, (adr+4)*ofs_factor);

	cfi_send_gen_cmd(0xaa, 0x555, base, map, cfi->interleave, cfi->device_type, NULL);
	cfi_send_gen_cmd(0x55, 0x2aa, base, map, cfi->interleave, cfi->device_type, NULL);
	cfi_send_gen_cmd(0x90, 0x555, base, map, cfi->interleave, cfi->device_type, NULL);

       	cfi->mfr = cfi_read_query(map, base);
       	cfi->id = cfi_read_query(map, base + ofs_factor);

	bootloc = if (maj << 8 + min >= 0x0101 /* Is this right? */) {
		if (cfi_read_query(map, (adr+3)*ofs_factor) == 3)
			swap = 1;
	}
#ifdef FTSO_AMD
	else {
		/* Eep. Need to check JEDEC ID */
		if (cfi->id & 0x80) {
			printk(KERN_WARNING "JEDEC Device ID is 0x%02X. Assuming broken CFI table.\n" cfi->id);
			swap = 1;		
		}
	}
#endif
	if (swap) {
		printk(KERN_WARNING "Swapping erase regions for broken CFI table\n");
		/* Do the swap as before ... */
	}

--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

RE: Problems with cfi_cmdset_0002.c

[Jamey Hicks]

According to the spec, you should be able to look at the AMD autoselect
codes.  Autoselect with A1=0, A0=1
	D[15:8] => 0x22
	D[7:0] => 0xC4  for top boot
	D[7:0] => 0x49 for bottom boot

-Jamey

-----Original Message-----
From: David Woodhouse [mailto:dwmw2@infradead.org]
Sent: Tuesday, February 13, 2001 4:17 AM
To: Jonas Holmberg
Cc: Johan Adolfsson; mtd@infradead.org
Subject: Re: Problems with cfi_cmdset_0002.c 



jonas.holmberg@axis.com said:
> I have two AM29LV160D one top and one bottom boot and the both say
> bootloc==0 :(

One of these days I really am going to painfully slaughter a hardware 
designer. The engineers at an unnamed company who decided to route the 
PCMCIA socket's interrupt line to a general purpose I/O line on the SH3 CPU 
without any possibility of generating an interrupt were prime targets a few 
weeks ago. AMD are in that spot now. Strange - the chip manufacturers are 
generally more clueful - it's the monkeys who glue them together who 
usually screw it up (present company excepted :)

Is there _any_ difference between these chips which is detectable without 
actually trying to erase the damn things and observing how much data gets 
removed?

--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org


To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Problems with cfi_cmdset_0002.c

[David Woodhouse]

jamey@crl.dec.com said:
>  According to the spec, you should be able to look at the AMD
> autoselect codes.  Autoselect with A1=0, A0=1
> 	D[15:8] => 0x22
> 	D[7:0] => 0xC4  for top boot
> 	D[7:0] => 0x49 for bottom boot 

Yep. "if (DeviceID | 0x80) it's a top-boot chip" looks like a good heuristic. 

--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

RE: Problems with cfi_cmdset_0002.c

[Jonas Holmberg]

> Looks like as good a heuristic as any. Just wrap it in an 
> ifdef and we can 
> remove it or make it a config option if it causes trouble.
> 
> 	__u8 maj,min;
> 	int swap = 0;
> 
> 	maj = cfi_read_query(map, (adr+3)*ofs_factor);
> 	min = cfi_read_query(map, (adr+4)*ofs_factor);
> 
> 	cfi_send_gen_cmd(0xaa, 0x555, base, map, 
> cfi->interleave, cfi->device_type, NULL);
> 	cfi_send_gen_cmd(0x55, 0x2aa, base, map, 
> cfi->interleave, cfi->device_type, NULL);
> 	cfi_send_gen_cmd(0x90, 0x555, base, map, 
> cfi->interleave, cfi->device_type, NULL);
> 
>        	cfi->mfr = cfi_read_query(map, base);
>        	cfi->id = cfi_read_query(map, base + ofs_factor);
> 
> 	bootloc = if (maj << 8 + min >= 0x0101 /* Is this right? */) {
> 		if (cfi_read_query(map, (adr+3)*ofs_factor) == 3)
> 			swap = 1;
> 	}
> #ifdef FTSO_AMD
> 	else {
> 		/* Eep. Need to check JEDEC ID */
> 		if (cfi->id & 0x80) {
> 			printk(KERN_WARNING "JEDEC Device ID is 
> 0x%02X. Assuming broken CFI table.\n" cfi->id);
> 			swap = 1;		
> 		}
> 	}
> #endif
> 	if (swap) {
> 		printk(KERN_WARNING "Swapping erase regions for 
> broken CFI table\n");
> 		/* Do the swap as before ... */
> 	}
> 
> --
> dwmw2
> 

The following seems to work. Do you still think that the ifdef is necessary? The new code only gets executed if bootloc==0, and then I think it has to be.

/Jonas


+++ ./cfi_cmdset_0002.c	Wed Feb 14 16:29:40 2001
@@ -59,9 +59,25 @@
 
 	bootloc = cfi_read_query(map, (adr+15)*ofs_factor);
 
+	cfi_send_gen_cmd(0xf0, 0x55, 0, map, cfi->interleave, cfi->device_type, NULL);
+
+	cfi_send_gen_cmd(0xaa, 0x555, base, map, cfi->interleave, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0x55, 0x2aa, base, map, cfi->interleave, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0x90, 0x555, base, map, cfi->interleave, cfi->device_type, NULL);
+	cfi->mfr = cfi_read_query(map, base);
+	cfi->id = map->read16(map, base + ofs_factor);
+
 	/* Wheee. Bring me the head of someone at AMD. */
+	if (!bootloc) {
+		if (cfi->id & 0x80) {
+			printk(KERN_WARNING "%s: JEDEC Device ID is 0x%02X. Assuming broken CFI table.\n", map->name, cfi->id);
+			bootloc = 3;	/* top boot */
+		} else {
+			bootloc = 2;	/* bottom boot */
+		}
+	}
 	if (bootloc == 3 && cfi->cfiq->NumEraseRegions > 1) {
-		printk("Swapping erase regions for broken CFI table\n");
+		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;
@@ -88,13 +104,6 @@
 		
 
 	cfi->cmdset_setup = cfi_amdstd_setup;
-	cfi_send_gen_cmd(0xaa, 0x555, base, map, cfi->interleave, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, 0x2aa, base, map, cfi->interleave, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x90, 0x555, base, map, cfi->interleave, cfi->device_type, NULL);
-
-	cfi->mfr = cfi_read_query(map, base);
-	cfi->id = cfi_read_query(map, base + ofs_factor);
-
 	cfi_send_gen_cmd(0xf0, 0x55, 0, map, cfi->interleave, cfi->device_type, NULL);
 	return;
 }


To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Problems with cfi_cmdset_0002.c

[David Woodhouse]

jonas.holmberg@axis.com said:
> +	if (!bootloc) { 
> +		if (cfi->id & 0x80) {


If possible, we should be checking the version number of the query table,
and using bootloc if the version number is equal to or greater than the
version at which the bootloc field was defined. If it's undefined, it
doesn't have to be zero. 

I'd like to keep this kludge wrapped in an ifdef just to make sure it's 
easy to disable it if necessary. It can be left on by default.

--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

RE: Problems with cfi_cmdset_0002.c

[Jonas Holmberg]

> jonas.holmberg@axis.com said:
> > +	if (!bootloc) { 
> > +		if (cfi->id & 0x80) {
> 
> 
> If possible, we should be checking the version number of the 
> query table,
> and using bootloc if the version number is equal to or 
> greater than the
> version at which the bootloc field was defined. If it's undefined, it
> doesn't have to be zero. 
> 

I cannot find any info on relationship between CFI version numbers and bootloc. I looked through the CFI 1.0 and 1.1 spec, but found nothing. Does anybody know? Otherwise my suggestion is still "if (!bootloc)" until proven wrong. I think it's safer than "cfi_version < 1.1". What do you think?

> I'd like to keep this kludge wrapped in an ifdef just to make 
> sure it's 
> easy to disable it if necessary. It can be left on by default.

Okidoki.

/Jonas


To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Problems with cfi_cmdset_0002.c

[David Woodhouse]

jonas.holmberg@axis.com said:
>  I cannot find any info on relationship between CFI version numbers
> and bootloc. I looked through the CFI 1.0 and 1.1 spec, but found
> nothing. Does anybody know? 

Only AMD. My datasheet for the AM29LV320D says the vendor-specific extended
query table version is 1.1 and includes bootloc (AMD pub #23579 Rev A, p23).
If the datasheets for other chips say v1.0 and omit bootloc, then that's
good enough, isn't it?

> Otherwise my suggestion is still "if (!bootloc)" until proven wrong. I
> think it's safer than "cfi_version < 1.1". What do you think?

That means you're depending on the contents of a byte which is not defined. 
If it's off the end of the v1.0 table, it doesn't have to be zero in all 
cases. It's theoretically safer to look for the version number first, and 
only read bootloc if it's at least v1.1.

But I don't care enough to argue :) As long as it works, it's OK by me. 

--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

RE: Problems with cfi_cmdset_0002.c

[Jonas Holmberg]

> But I don't care enough to argue :) As long as it works, it's 
> OK by me. 
> 

You don't have to argue, I'm convinced. Does this look OK (it works for my chips)?

/Jonas

+++ ./cfi_cmdset_0002.c	Thu Feb 15 13:37:41 2001
@@ -46,22 +46,42 @@
 	unsigned char bootloc;
 	int ofs_factor = cfi->interleave * cfi->device_type;
 	int i;
+	__u8 major, minor;
 //	struct cfi_pri_intelext *extp;
 
 	__u16 adr = primary?cfi->cfiq->P_ADR:cfi->cfiq->A_ADR;
 
 	cfi_send_gen_cmd(0x98, 0x55, 0, map, cfi->interleave, cfi->device_type, NULL);
 
-	printk(" Amd/Fujitsu Extended Query Table v%c.%c at 0x%4.4X\n", 
-	       cfi_read_query(map, (adr+3)*ofs_factor),
-	       cfi_read_query(map, (adr+4)*ofs_factor),
-	       adr);
+	major = cfi_read_query(map, (adr+3)*ofs_factor);
+	minor = cfi_read_query(map, (adr+4)*ofs_factor);
 
-	bootloc = cfi_read_query(map, (adr+15)*ofs_factor);
+	printk(" Amd/Fujitsu Extended Query Table v%c.%c at 0x%4.4X\n",
+	       major, minor, adr);
+
+	cfi_send_gen_cmd(0xf0, 0x55, 0, map, cfi->interleave, cfi->device_type, NULL);
+
+	cfi_send_gen_cmd(0xaa, 0x555, base, map, cfi->interleave, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0x55, 0x2aa, base, map, cfi->interleave, cfi->device_type, NULL);
+	cfi_send_gen_cmd(0x90, 0x555, base, map, cfi->interleave, cfi->device_type, NULL);
+	cfi->mfr = cfi_read_query(map, base);
+	cfi->id = map->read16(map, base + ofs_factor);

 	/* Wheee. Bring me the head of someone at AMD. */
+	if (((major << 8) | minor) > 0x3130) {
+		cfi_send_gen_cmd(0x98, 0x55, 0, map, cfi->interleave, cfi->device_type, NULL);
+		bootloc = cfi_read_query(map, (adr+15)*ofs_factor);
+	} else {
+		/* CFI version 1.0 => don't trust bootloc */
+		if (cfi->id & 0x80) {
+			printk(KERN_WARNING "%s: JEDEC Device ID is 0x%02X. Assuming broken CFI table.\n", map->name, cfi->id);
+			bootloc = 3;	/* top boot */
+		} else {
+			bootloc = 2;	/* bottom boot */
+		}
+	}
 	if (bootloc == 3 && cfi->cfiq->NumEraseRegions > 1) {
-		printk("Swapping erase regions for broken CFI table\n");
+		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;
@@ -88,13 +108,6 @@
 		
 
 	cfi->cmdset_setup = cfi_amdstd_setup;
-	cfi_send_gen_cmd(0xaa, 0x555, base, map, cfi->interleave, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x55, 0x2aa, base, map, cfi->interleave, cfi->device_type, NULL);
-	cfi_send_gen_cmd(0x90, 0x555, base, map, cfi->interleave, cfi->device_type, NULL);
-
-	cfi->mfr = cfi_read_query(map, base);
-	cfi->id = cfi_read_query(map, base + ofs_factor);
-
 	cfi_send_gen_cmd(0xf0, 0x55, 0, map, cfi->interleave, cfi->device_type, NULL);
 	return;
 }


To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: Problems with cfi_cmdset_0002.c

[David Woodhouse]

jonas.holmberg@axis.com said:
>  You don't have to argue, I'm convinced. Does this look OK (it works
> for my chips)? 

Looks good, should work on mine too, from memory. Wanna commit it?

--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

RE: Problems with cfi_cmdset_0002.c

[Jonas Holmberg]

> Looks good, should work on mine too, from memory. Wanna commit it?

Done.

/Jonas



To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

RE: Problems with cfi_cmdset_0002.c

[Jonas Holmberg]

I forgot to ifdef the bootloc fix. I have done that now and committed it.

/Jonas


To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org