MTD/SPI/FLASH: add support for Ramtron FRAMs using SPI

+/*
+ * Unfortunately, some RAMTRON FRAMs do not have a means of
+ * identifying them. We use an environment variable to select
+ * the device we will handle. The variable "fram_device" should
+ * be set from VPD data.
+ */

+#define DEBUG 1 /* remove this once its been thoroughly tested */

drivers/mtd/spi/ramtron.c new file +13 -0

+static const struct ramtron_spi_fram_params ramtron_spi_fram_table[] = {
+	/* FM25V02: */
+	{.size = 32*1024, .addr_len = 2, .merge_cmd = 0,
+		.id1 = 0x22, .id2 = 0x00, .speed = 40000000, .name = "FM25V02",	},
+	/* FM25VN02: */
+	{.size = 32*1024, .addr_len = 2, .merge_cmd = 0,
+		.id1 = 0x22, .id2 = 0x01, .speed = 40000000, .name = "FM25VN02", },
+	/* FM25V05: */
+	{ .size = 64*1024, .addr_len = 2, .merge_cmd = 0,
+		.id1 = 0x23, .id2 = 0x00, .speed = 40000000, .name = "FM25V05", },
+	/* FM25VN05: */
+	{.size = 64*1024, .addr_len = 2, .merge_cmd = 0,
+		.id1 = 0x23, .id2 = 0x01, .speed = 40000000, .name = "FM25VN05", },

+int ramtron_erase(struct spi_flash *flash, u32 offset, size_t len)

drivers/mtd/spi/ramtron.c new file +6 -0

+	/*
+	 * RAMTRON chips without RDID command support will keep their Q output
+	 * tristated. Depending on MISO termination we will read noise.
+	 * Chips with RDID command support will answer 6*0x7f, 0xc2, id1, id2.
+	 */
+	if (idcode[0]==0x7f && idcode[1]==0x7f && idcode[2]==0x7f &&

idcode[3]==0x7f && idcode[4]==0x7f && idcode[5]==0x7f && idcode[6]==0xc2) {

drivers/mtd/spi/spi_flash.c +6 -0

@@ -116,6 +116,19 @@
        goto err_claim_bus;
 	}
 
+#ifdef CONFIG_SPI_FRAM_RAMTRON
+	/*
+	 * not all RAMTRON FRAMs do have a READ_ID command,
+	 * let the ramtron code figure out details
+	 */
+	flash = spi_fram_probe_ramtron(spi);

i dont have a problem with going through the env as a hook, but it doesnt seem to scale. what if you have 2 fram devices and a winbond spi flash ? perhaps the name spec should be:

	fram_dev.<bus>.<cs>

then you can signal that only certain<bus>.<cs> locations should get special treatment. and it wont inadvertently clobber other devices or detect devices that dont exist without explicit permission.

drivers/mtd/spi/ramtron.c line 119

Quoted 6 lines

drivers/mtd/spi/ramtron.c new file +5 -0

+{
+	struct ramtron_spi_fram *sn = to_ramtron_spi_fram(flash);
+	u8 cmd[4];
+	u8 cmd_len = 1;
+	int ret;

the majority of this function seems to match the write() func ... perhaps the body should be unified in a local static "setup" func and let gcc determine whether to inline its body ?

no space after "getenv". i'd also delay the call to the point where you actually check "device_name" ...

Quoted 12 lines

drivers/mtd/spi/spi_flash.c +6 -0

@@ -116,6 +116,19 @@
         goto err_claim_bus;
  	}
 
+#ifdef CONFIG_SPI_FRAM_RAMTRON
+	/*
+	 * not all RAMTRON FRAMs do have a READ_ID command,
+	 * let the ramtron code figure out details
+	 */
+	flash = spi_fram_probe_ramtron(spi);

drivers/mtd/spi/spi_flash.c line 134

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +7 -0

+	if (flash) {
+		spi_release_bus(spi);
+		return flash;
+	}
+	/* if spi_fram_probe did not find anything, continue normal probe */
+#endif
+
  	/* Read the ID codes */
  	ret = spi_flash_cmd(spi, CMD_READ_ID,&idcode, sizeof(idcode));
  	if (ret)

you hook early to handle extended idcode reads and for devices that dont respect the idcode command.

for the first, we can extend the idcode length yet again, but perhaps this time temper it:
#if defined(CONFIG_SPI_FRAM_RAMTRON)

# define IDCODE_LEN 10
#else
# define IDCODE_LEN 5
#endif

for the second, what do you get back when you issue the idcode ? 0xff ? we already have a fall back case for this with stmicro, so perhaps we should generalize this further too. after the vendor id switch statement, we do:

Dear Mike Frysinger,

Quoted 5 lines

#if defined(CONFIG_SPI_FRAM_RAMTRON)

# define IDCODE_LEN 10
#else
# define IDCODE_LEN 5
#endif

OK, see below. Can't we have it 10 generally? The impact should be negligible?

Quoted 4 lines

for the second, what do you get back when you issue the idcode ? 0xff ? we already have a fall back case for this with stmicro, so perhaps we should

Quoted 1 line

generalize this further too. after the vendor id switch statement, we do:

If MISO has no pull-up the result is indeterminate, the chip simply lets MISO float when it does not honor the read-id command.

I'll add a comment to that file that a pull-up is required for non-standard devices to be detected. Otherwise, depending on random noise, a false detection of a standard device is not entirely impossible.

Quoted 8 lines

Quoted 5 lines

#if defined(CONFIG_SPI_FRAM_RAMTRON)

# define IDCODE_LEN 10
#else
# define IDCODE_LEN 5
#endif

OK, see below. Can't we have it 10 generally? The impact should be negligible?

hrm, i guess ... but i didnt even really like raising it to the 5 for one specific family ...

1. looking at spi_flash.c, shall I add the standard copyright, keeping atmel as author?

2. I can find the following lengths of idcode used:
atmel 2
macronix 3
ramtron 9
spansion 5
sst 3
stmicro 4
winbond 3

Do you really want to flexify this? Even before relocation 5 bytes more on stack would not be an issue...
Of course, for me the 5/10 switch would work as well.

It would involve the

	debug("SF: Got idcode %02x %02x %02x %02x %02x\n", idcode[0],
			idcode[1], idcode[2], idcode[3], idcode[4]);
as well. I can make that into a for() loop
#ifdef DEBUG
	printf("SF: Got idcode");
	for (i=0; i<IDCODE_LEN; i++)
		printf(" %02x", idcode[i]);
	printf("\n");

#endif

1. according to Ramtron, 0x7f is a continuation byte defined in a JEDEC standard (I could not find a spec for that) and shall be ignored until a non-0x7f shows.

That shall be Manufacturer Id (0xc2 for Ramtron), followed by the 2 device id bytes. Following that method, the spi_flash.c should do that and use the first non-0x7f value for the switch statement. The switch would then have a case 0xc2 to call the ramtron-specific code.

about like (rough sketch):
for (index=0; index < IDLENGTH-3 && idcode[index] == 0x7f; index++)

	;

/*
* here we are on the first non-0x7f byte or still on one,
* the switch will sort that out...
*/
switch (idcode[index]) {
case 0xc2: flash = spi_fram_probe_ramtron(spi, idcode+index); /* the function will only access its parameter idcode with index 1 and 2 */ etc...

default: /* covers the 0x7f case as well */

2. the switch case 0xff would occur twice, if someone had defined STMICRO and RAMTRON. I would postulate here that it is not allowed to define both and issue an #error.

3. If someone decided to put the env into FRAM, the env-var describing the nonstandard type would not be there (yet). So this should have another solution. Since because of 2. another non-standard SPI device should not be expected - I would, after accessing the status register to verify something FRAM-like is there, use a CONFIG_FRAM_DEVICE to define the chip to be assumed. That would work for us, since there is only one non-standard FRAM in the list anyway.

interesting, but what if we push it further. something like this (untested):

Dear Mike Frysinger,

Quoted 3 lines · On Thursday, August 26, 2010 16:00:50 Reinhard Meyer wrote:

interesting, but what if we push it further. something like this (untested):

That code does not address the following issues to complete extend:

1. JEDEC conformant that have ID in first byte
2. JEDEC conformant that have ID in later byte

The question that remains is if any ID can be assigned twice in different positions and them meaning different manufacturers?

7f 7f 7f 7f 7f 7f c2 = ramtron

7f 7f 7f c2 = some other manufacturer?

3. non JEDEC conformant or those that do not honor the read-id command and thus present 0xff _if_ the MISO line is pulled up

Review

Quoted 4 lines · On Saturday, August 28, 2010 17:48:39 Mike Frysinger wrote:

(quoted diff) +4 -0

+			/* Make sure the ID was jedec extended */
+			j = flashes[i].shift - 1;
+			while (j>= 0&&  idcode[j] == 0x7f)
+				continue;

thinko ... this loops forever. the tested fix:

	j = flashes[i].shift;
	while (--j>= 0)
		if (idcode[j] != 0x7f)
			break;

this whole change increases code size a bit (20 - 60 bytes on Blackfin depending on how many flashes are supported), but it makes management easier and allows for arbitrarily long manufacturers ids. so i think it's worth it. -mike

Quoted 12 lines · Mike Frysinger

Review

Quoted 4 lines · On Saturday, August 28, 2010 17:48:39 Mike Frysinger wrote:

(quoted diff) +4 -0

+			/* Make sure the ID was jedec extended */
+			j = flashes[i].shift - 1;
+			while (j>= 0&&  idcode[j] == 0x7f)
+				continue;

thinko ... this loops forever. the tested fix:

	j = flashes[i].shift;
	while (--j>= 0)
	
		if (idcode[j] != 0x7f)

Quoted 6 lines

			break;

this whole change increases code size a bit (20 - 60 bytes on Blackfin depending on how many flashes are supported), but it makes management easier and allows for arbitrarily long manufacturers ids. so i think it's worth it.

arbitrarily long ids unfortunately require an arbitrarily long id_buffer :)

In that case I think its easier to right after READ_ID count the 0x7f's and search the table with count and the id:

(n is either a compile time constant or determined by examining the table for largest 'shift'+3)

read-id (n bytes)
shift=count 7f's (max n-3) id=id_buffer[shift]
search table for shift and id, call function. if function returns NULL, continue search in table.

That will allow several shift=0, id=0xff entries in the table, which should be ordered such that least likely to false detect probes come first:
stmicro first since that probe actually has a way to really figure out the device, the FM25H20-ramtron has no other way except for a ram size test which certainly is not a good idea to do here...

It is disputable whether the function gets id_buffer or id_buffer+shift as parameter. I'd prefer the latter.

I am willing to code and test and submit a patch for that method.

if you want to take the patch i posted and extend that by itself with the comments here, and then do the ramtron patch separately on top of that, that'd be great.

Quoted 3 lines · Mike Frysinger

if you want to take the patch i posted and extend that by itself with the comments here, and then do the ramtron patch separately on top of that, that'd be great.

Hehe, you like to impose extra work on poor moi....

the first patch would add the table method and a
#define IDCODE_LEN 5
at the top of file

the ramtron patch would then add (inside the table)

#ifdef CONFIG_SPI_FRAM_RAMTRON
	{6, 0xc2, spi_fram_probe_ramtron},
# if IDCODE_LEN<9
#  undef IDCODE_LEN
#  define IDCODE_LEN 9
# endif
#endif

Not exactly nice, but that way there is only one place where device specific stuff happens, and we will automatically get the largest idbuffer required for all the non-trivial devices included.

Quoted 6 lines · On Saturday, August 28, 2010 21:59:26 Reinhard Meyer wrote:

Quoted 3 lines · Mike Frysinger

if you want to take the patch i posted and extend that by itself with the comments here, and then do the ramtron patch separately on top of that, that'd be great.

Hehe, you like to impose extra work on poor moi....

i can do the pre-ramtron stuff if you like

Quoted 9 lines · Mike Frysinger

Quoted 6 lines · On Saturday, August 28, 2010 21:59:26 Reinhard Meyer wrote:

Quoted 3 lines · Mike Frysinger

if you want to take the patch i posted and extend that by itself with the comments here, and then do the ramtron patch separately on top of that, that'd be great.

Hehe, you like to impose extra work on poor moi....

i can do the pre-ramtron stuff if you like

No, I meant the splitting of the patch into two. As I said, the commit is about 10 back in my working tree, and I see no simple way of splitting the changes...

When I insert the table method at TOT, I can easy squash that back into the old commit, but I still have one patch then. How do I suck the ramtron specifics out of Makefile, spi_flash.c, spi_flash_internal.h without un-editing them, committing, re-adding, committing?

when you're editing a commit in the middle of a rebase, you can add on any number of commits you like on top of it. so usually the way i split commits:
git rebase -i<commit>^
<mark the commit i want to split as "edit">
<exit the rebase-todo window to start the process>
git format-patch -1
<edit the patch and keep all the hunks i want to split out> patch -p1 -R< 0001-*
git commit -a --amend
<edit message to reflect reduced changes>
patch -p1< 0001-*
git commit -a -c HEAD
<edit message to reflect split out changes>
git rebase --continue

maybe someone out there can suggest some shortcuts to my process ...

when you're editing a commit in the middle of a rebase, you can add on any number of commits you like on top of it. so usually the way i split commits:
git rebase -i <commit>^
<mark the commit i want to split as "edit">
<exit the rebase-todo window to start the process>
git format-patch -1
<edit the patch and keep all the hunks i want to split out> patch -p1 -R < 0001-*
git commit -a --amend
<edit message to reflect reduced changes>
patch -p1 < 0001-*
git commit -a -c HEAD
<edit message to reflect split out changes>
git rebase --continue

maybe someone out there can suggest some shortcuts to my process ...

Dear Mike Frysinger,

Quoted 8 lines

i dont have a problem with going through the env as a hook, but it doesnt seem to scale. what if you have 2 fram devices and a winbond spi flash ? perhaps the name spec should be:

	fram_dev.<bus>.<cs>

then you can signal that only certain<bus>.<cs> locations should get special treatment. and it wont inadvertently clobber other devices or detect devices that dont exist without explicit permission.

Makes sense.