[U-Boot] [PATCH] MTD/SPI/FLASH: add support for Ramtron FRAMs using SPI
From: Reinhard Meyer info@emk-elektronik.de
Signed-off-by: Reinhard Meyer u-boot@emk-elektronik.de --- drivers/mtd/spi/Makefile | 1 + drivers/mtd/spi/ramtron.c | 297 ++++++++++++++++++++++++++++++++++ drivers/mtd/spi/spi_flash.c | 13 ++ drivers/mtd/spi/spi_flash_internal.h | 2 + 4 files changed, 313 insertions(+), 0 deletions(-) create mode 100644 drivers/mtd/spi/ramtron.c
diff --git a/drivers/mtd/spi/Makefile b/drivers/mtd/spi/Makefile index 4f11b36..a082ca7 100644 --- a/drivers/mtd/spi/Makefile +++ b/drivers/mtd/spi/Makefile @@ -32,6 +32,7 @@ COBJS-$(CONFIG_SPI_FLASH_SPANSION) += spansion.o COBJS-$(CONFIG_SPI_FLASH_SST) += sst.o COBJS-$(CONFIG_SPI_FLASH_STMICRO) += stmicro.o COBJS-$(CONFIG_SPI_FLASH_WINBOND) += winbond.o +COBJS-$(CONFIG_SPI_FRAM_RAMTRON) += ramtron.o COBJS-$(CONFIG_SPI_M95XXX) += eeprom_m95xxx.o
COBJS := $(COBJS-y) diff --git a/drivers/mtd/spi/ramtron.c b/drivers/mtd/spi/ramtron.c new file mode 100644 index 0000000..9271e5b --- /dev/null +++ b/drivers/mtd/spi/ramtron.c @@ -0,0 +1,297 @@ +/* + * (C) Copyright 2010 + * Reinhard Meyer, EMK Elektronik, reinhard.meyer@emk-elektronik.de + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +/* + * Note: RAMTRON SPI FRAMs are ferroelectric, nonvolatile RAMs + * with an interface identical to SPI flash devices. + * However since they behave like RAM there are no delays or + * busy polls required. They can sustain read or write at the + * allowed SPI bus speed, which can be 40 MHz for some devices. + * + * 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 */ + +#include <common.h> +#include <malloc.h> +#include <spi_flash.h> +#include "spi_flash_internal.h" + +/* RAMTRON commands common to all devices */ +#define CMD_RAMTRON_WREN 0x06 /* Write Enable */ +#define CMD_RAMTRON_WRDI 0x04 /* Write Disable */ +#define CMD_RAMTRON_RDSR 0x05 /* Read Status Register */ +#define CMD_RAMTRON_WRSR 0x01 /* Write Status Register */ +#define CMD_RAMTRON_READ 0x03 /* Read Data Bytes */ +#define CMD_RAMTRON_WRITE 0x02 /* Write Data Bytes */ +/* not all have those: */ +#define CMD_RAMTRON_FSTRD 0x0b /* Fast Read (for compatibility - not used here) */ +#define CMD_RAMTRON_SLEEP 0xb9 /* Enter Sleep Mode */ +#define CMD_RAMTRON_RDID 0x9f /* Read ID */ +#define CMD_RAMTRON_SNR 0xc3 /* Read Serial Number */ + +#define ENV_VARIABLE "fram_device" + +/* + * Properties of supported FRAMs + * Note: speed is currently not used because we have no method to deliver that + * value to the upper layers + */ +struct ramtron_spi_fram_params { + u32 size; /* size in bytes */ + u8 addr_len; /* number of address bytes */ + u8 merge_cmd; /* some address bits are in the command byte */ + u8 id1; /* device ID 1 (family, density) */ + u8 id2; /* device ID 2 (sub, rev, rsvd) */ + u32 speed; /* max. SPI clock in Hz */ + const char *name; /* name for display and/or matching */ +}; + +struct ramtron_spi_fram { + struct spi_flash flash; + const struct ramtron_spi_fram_params *params; +}; + +static inline struct ramtron_spi_fram *to_ramtron_spi_fram(struct spi_flash + *flash) +{ + return container_of(flash, struct ramtron_spi_fram, flash); +} + +/* + * table describing supported FRAM chips: + * chips without RDID command must have the values 0xff for id1 and id2 + */ +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", }, + /* FM25V10: */ + {.size = 128*1024, .addr_len = 3, .merge_cmd = 0, + .id1 = 0x24, .id2 = 0x00, .speed = 40000000, .name = "FM25V10", }, + /* FM25VN10: */ + {.size = 128*1024, .addr_len = 3, .merge_cmd = 0, + .id1 = 0x24, .id2 = 0x01, .speed = 40000000, .name = "FM25VN10", }, + /* FM25H20: no identification */ + {.size = 256*1024, .addr_len = 3, .merge_cmd = 0, + .id1 = 0xff, .id2 = 0xff, .speed = 40000000, .name = "FM25H20", }, +}; + +static int ramtron_read(struct spi_flash *flash, + u32 offset, size_t len, void *buf) +{ + struct ramtron_spi_fram *sn = to_ramtron_spi_fram(flash); + u8 cmd[4]; + u8 cmd_len = 1; + int ret; + + if (sn->params->addr_len == 3 && sn->params->merge_cmd == 0) { + cmd[0] = CMD_RAMTRON_READ; + cmd[1] = offset >> 16; + cmd[2] = offset >> 8; + cmd[3] = offset; + cmd_len = 4; + } + else if (sn->params->addr_len == 2 && sn->params->merge_cmd == 0) { + cmd[0] = CMD_RAMTRON_READ; + cmd[1] = offset >> 8; + cmd[2] = offset; + cmd_len = 3; + } + else { + printf("SF: unsupported addr_len or merge_cmd\n"); + return -1; + } + + debug("READ: 0x%x => cmd = { 0x%02x 0x%02x%02x%02x } len = 0x%x\n", + offset, cmd[0], cmd[1], cmd[2], cmd[3], len); + + /* claim the bus */ + ret = spi_claim_bus(flash->spi); + if (ret) { + debug("SF: Unable to claim SPI bus\n"); + return ret; + } + + /* read the data */ + ret = spi_flash_read_common(flash, cmd, cmd_len, buf, len); + if (ret < 0) + debug("SF: Read failed\n"); + + /* release the bus */ + spi_release_bus(flash->spi); + return ret; +} + +static int ramtron_write(struct spi_flash *flash, + u32 offset, size_t len, const void *buf) +{ + struct ramtron_spi_fram *sn = to_ramtron_spi_fram(flash); + u8 cmd[4]; + u8 cmd_len = 1; + int ret; + + if (sn->params->addr_len == 3 && sn->params->merge_cmd == 0) { + cmd[0] = CMD_RAMTRON_WRITE; + cmd[1] = offset >> 16; + cmd[2] = offset >> 8; + cmd[3] = offset; + cmd_len = 4; + } + else if (sn->params->addr_len == 2 && sn->params->merge_cmd == 0) { + cmd[0] = CMD_RAMTRON_WRITE; + cmd[1] = offset >> 8; + cmd[2] = offset; + cmd_len = 3; + } + else { + printf("SF: unsupported addr_len or merge_cmd\n"); + return -1; + } + + debug("WRITE: 0x%x => cmd = { 0x%02x 0x%02x%02x%02x } len = 0x%x\n", + offset, cmd[0], cmd[1], cmd[2], cmd[3], len); + + /* claim the bus */ + ret = spi_claim_bus(flash->spi); + if (ret) { + debug("SF: Unable to claim SPI bus\n"); + return ret; + } + + /* send WREN */ + ret = spi_flash_cmd(flash->spi, CMD_RAMTRON_WREN, NULL, 0); + if (ret < 0) { + debug("SF: Enabling Write failed\n"); + goto releasebus; + } + + /* write the data */ + ret = spi_flash_cmd_write(flash->spi, cmd, cmd_len, buf, len); + if (ret < 0) + debug("SF: Write failed\n"); + +releasebus: + /* release the bus */ + spi_release_bus(flash->spi); + return ret; +} + +int ramtron_erase(struct spi_flash *flash, u32 offset, size_t len) +{ + debug("SF: Erase of RAMTRON FRAMs is pointless\n"); + return -1; +} + +struct spi_flash *spi_fram_probe_ramtron(struct spi_slave *spi) +{ + const struct ramtron_spi_fram_params *params; + struct ramtron_spi_fram *sn; + unsigned int i; + u8 idcode[10]; + int ret; + const char *device_name = getenv (ENV_VARIABLE); + + /* NOTE: the bus has been claimed before this function is called! */ + + /* try to get an ID first */ + ret = spi_flash_cmd(spi, CMD_RAMTRON_RDID, &idcode, sizeof(idcode)); + if (ret) { + debug("SF: Read ID command failed\n"); + return NULL; + } + + debug("SF: Got idcode %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x\n", + idcode[0], idcode[1], idcode[2], idcode[3], idcode[4], + idcode[5], idcode[6], idcode[7], idcode[8], idcode[9]); + + /* + * 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) { + for (i = 0; i < ARRAY_SIZE(ramtron_spi_fram_table); i++) { + params = &ramtron_spi_fram_table[i]; + if (idcode[7]==params->id1 && idcode[8]==params->id2) + goto found; + } + debug("SF: Unsupported RAMTRON device id1=%02x id2=%02x\n", + idcode[7], idcode[8]); + return NULL; + } + + /* + * If no ID was found, we will try the environment variable, + * but we will only match chips that do not support RDID command + */ + if (device_name) { + for (i = 0; i < ARRAY_SIZE(ramtron_spi_fram_table); i++) { + params = &ramtron_spi_fram_table[i]; + if (params->id1==0xff && params->id2==0xff && + !strcmp(params->name, device_name)) + goto found; + } + debug("SF: Unsupported RAMTRON device %s\n", device_name); + return NULL; + } + + /* no success with either method... */ + return NULL; + +found: + sn = malloc(sizeof(struct ramtron_spi_fram)); + if (!sn) { + debug("SF: Failed to allocate memory\n"); + return NULL; + } + + sn->params = params; + sn->flash.spi = spi; + sn->flash.name = params->name; + + sn->flash.write = ramtron_write; + sn->flash.read = ramtron_read; + sn->flash.erase = ramtron_erase; + sn->flash.size = params->size; + + printf("SF: Detected %s with size ", params->name); + print_size(sn->flash.size, "\n"); + + return &sn->flash; +} + diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c index ea875dc..328366b 100644 --- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -116,6 +116,19 @@ struct spi_flash *spi_flash_probe(unsigned int bus, unsigned int cs, 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); + 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) diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h index 08546fb..0f1b8da 100644 --- a/drivers/mtd/spi/spi_flash_internal.h +++ b/drivers/mtd/spi/spi_flash_internal.h @@ -50,3 +50,5 @@ struct spi_flash *spi_flash_probe_macronix(struct spi_slave *spi, u8 *idcode); struct spi_flash *spi_flash_probe_sst(struct spi_slave *spi, u8 *idcode); struct spi_flash *spi_flash_probe_stmicro(struct spi_slave *spi, u8 *idcode); struct spi_flash *spi_flash_probe_winbond(struct spi_slave *spi, u8 *idcode); +struct spi_flash *spi_fram_probe_ramtron(struct spi_slave *spi); +
On Wednesday, August 25, 2010 08:47:19 Reinhard Meyer wrote:
+/*
- 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.
- */
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.
+#define DEBUG 1 /* remove this once its been thoroughly tested */
mmm, more like submitted ... i dont think drivers should be merged with this
+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", },- /* FM25V10: */
- {.size = 128*1024, .addr_len = 3, .merge_cmd = 0,
.id1 = 0x24, .id2 = 0x00, .speed = 40000000, .name = "FM25V10", },- /* FM25VN10: */
- {.size = 128*1024, .addr_len = 3, .merge_cmd = 0,
.id1 = 0x24, .id2 = 0x01, .speed = 40000000, .name = "FM25VN10", },- /* FM25H20: no identification */
- {.size = 256*1024, .addr_len = 3, .merge_cmd = 0,
.id1 = 0xff, .id2 = 0xff, .speed = 40000000, .name = "FM25H20", },+};
please adopt the expanded struct style used in all the other spi flash drivers
+static int ramtron_read(struct spi_flash *flash,
u32 offset, size_t len, void *buf)
you try in a bunch of places to align the indentation ... but it seems to fail more often than not ...
+{
- 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 ?
also, you set cmd_len to 1, but then you change it to 3 or 4 below (only other case is to error out) ...
- if (sn->params->addr_len == 3 && sn->params->merge_cmd == 0) {
cmd[0] = CMD_RAMTRON_READ;cmd[1] = offset >> 16;cmd[2] = offset >> 8;cmd[3] = offset;cmd_len = 4;- }
- else if (sn->params->addr_len == 2 && sn->params->merge_cmd == 0) {
incorrect style ... the "else" should be cuddled with the "}"
+int ramtron_erase(struct spi_flash *flash, u32 offset, size_t len)
static
- const char *device_name = getenv (ENV_VARIABLE);
no space after "getenv". i'd also delay the call to the point where you actually check "device_name" ...
- /*
* 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) {
you need spaces around those "=="
perhaps it'd be quicker to memcmp() against a local buffer on the stack set to { 0x7f, 0x7f, ... } ...
if (idcode[7]==params->id1 && idcode[8]==params->id2)if (params->id1==0xff && params->id2==0xff &&
spaces around the "=="
+found:
- sn = malloc(sizeof(struct ramtron_spi_fram));
sizeof(*sn)
--- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -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);
- 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: + /* Handle non-standard flashes */ +#ifdef CONFIG_SPI_FRAM_RAMTRON + if (!flash) + flash = spi_fram_probe_ramtron(spi, idcode); +#endif +#ifdef CONFIG_SPI_FLASH_STMICRO + if (!flash) + flash = spi_flash_probe_stmicro(spi, idcode); +#endif
if (!flash) goto .... -mike
Dear Mike Frysinger,
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.
+{
- 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 ?
OK
no space after "getenv". i'd also delay the call to the point where you actually check "device_name" ...
Right.
--- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -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);
- 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
OK, see below. Can't we have it 10 generally? The impact should be negligible?
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:
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.
On a side note: its beyond my comprehension why ramtron's newest and densest variant FM25H20 has no read-id command (while all devices before that except for the really old and small ones do have it)....
Reinhard
On Thursday, August 26, 2010 01:57:23 Reinhard Meyer wrote:
Dear Mike Frysinger,
#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 ...
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:
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.
that sounds reasonable to me -mike
Dear Mike Frysinger,
#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 ...
Once I am editing the file, I can make that a 3-way #if.
Reinhard
Dear Mike,
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
Reinhard
On Thursday, August 26, 2010 04:27:36 Reinhard Meyer wrote:
- looking at spi_flash.c, shall I add the standard copyright,
keeping atmel as author?
doesnt matter to me
- 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.
i would keep the 5/10 split
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
use print_buffer() -mike
Dear Mike,
I have been looking at the possible solutions quite some time now, the issue is rather complex:
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.
Your thoughts?
Best Regards Reinhard
On Thursday, August 26, 2010 16:00:50 Reinhard Meyer wrote:
- 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.
yep http://www.jedec.org/standards-documents/results/taxonomy%3A3127
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 */
interesting, but what if we push it further. something like this (untested):
--- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -96,6 +96,31 @@ return ret; }
+const struct { + const u8 shift; + const u8 idcode; + struct spi_flash *(*probe) (struct spi_slave *spi, u8 *idcode); +} flashes[] = { +#ifdef CONFIG_SPI_FLASH_SPANSION + { 0, 0x01, spi_flash_probe_spansion, }, +#endif +#ifdef CONFIG_SPI_FLASH_ATMEL + { 0, 0x1F, spi_flash_probe_atmel, }, +#endif +#ifdef CONFIG_SPI_FLASH_MACRONIX + { 0, 0xc2, spi_flash_probe_macronix, }, +#endif +#ifdef CONFIG_SPI_FLASH_WINBOND + { 0, 0xef, spi_flash_probe_winbond, }, +#endif +#ifdef CONFIG_SPI_FLASH_STMICRO + { 0, 0x20, spi_flash_probe_stmicro, }, +#endif +#ifdef CONFIG_SPI_FLASH_SST + { 0, 0xBF, spi_flash_probe_sst, }, +#endif +}; + struct spi_flash *spi_flash_probe(unsigned int bus, unsigned int cs, unsigned int max_hz, unsigned int spi_mode) { @@ -124,46 +149,34 @@ struct spi_flash *spi_flash_probe(unsigned int bus, unsigned int cs, debug("SF: Got idcode %02x %02x %02x %02x %02x\n", idcode[0], idcode[1], idcode[2], idcode[3], idcode[4]);
- switch (idcode[0]) { -#ifdef CONFIG_SPI_FLASH_SPANSION - case 0x01: - flash = spi_flash_probe_spansion(spi, idcode); - break; -#endif -#ifdef CONFIG_SPI_FLASH_ATMEL - case 0x1F: - flash = spi_flash_probe_atmel(spi, idcode); - break; -#endif -#ifdef CONFIG_SPI_FLASH_MACRONIX - case 0xc2: - flash = spi_flash_probe_macronix(spi, idcode); - break; -#endif -#ifdef CONFIG_SPI_FLASH_WINBOND - case 0xef: - flash = spi_flash_probe_winbond(spi, idcode); - break; -#endif -#ifdef CONFIG_SPI_FLASH_STMICRO - case 0x20: - case 0xff: /* Let the stmicro func handle non-JEDEC ids */ - flash = spi_flash_probe_stmicro(spi, idcode); - break; -#endif -#ifdef CONFIG_SPI_FLASH_SST - case 0xBF: - flash = spi_flash_probe_sst(spi, idcode); - break; -#endif - default: - printf("SF: Unsupported manufacturer %02X\n", idcode[0]); - flash = NULL; - break; + flash = NULL; + if (idcode[0] == 0xff) { + /* handle non-JEDEC flashes */ +#ifdef CONFIG_SPI_FLASH_STMICRO + flash = spi_flash_probe_stmicro(spi, idcode); +#endif + } else { + int i, j; + for (i = 0; i < ARRAY_SIZE(flashes); ++i) { + /* See if we have any known manufacturers */ + if (idcode[flashes[i].shift] != flashes[i].idcode) + continue; + + /* Make sure the ID was jedec extended */ + j = flashes[i].shift - 1; + while (j >= 0 && idcode[j] == 0x7f) + continue; + if (j == -1) { + flash = flashes[i].probe(spi, idcode); + break; + } + } }
- if (!flash) + if (!flash) { + printf("SF: Unsupported manufacturer %02X\n", idcode[0]); goto err_manufacturer_probe; + }
spi_release_bus(spi);
you should now be able to add an entry to the table like: { 6, 0xc2, spi_fram_probe_ramtron, },
- 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.
right
- 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.
this is what the default env is for ...
you hit a similar problem with CONFIG_SYS_CONSOLE_IS_IN_ENV -mike
Dear Mike Frysinger,
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 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
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?
Anyway my patch V2 provides a solution with minimum changes to the original driver.
If more devices with the 7f code are going to show up, the same method as with 0xff can be applied.
Reinhard
On Saturday, August 28, 2010 18:15:43 Reinhard Meyer wrote:
Dear Mike Frysinger,
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:
- JEDEC conformant that have ID in first byte
- JEDEC conformant that have ID in later byte
why doesnt it ? look at the structure i created a bit closer as well as the code that parses it.
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
{ 6, 0xc2, ramtron_probe, },
7f 7f 7f c2 = some other manufacturer?
{ 3, 0xc2, some_other_manu_probe, },
- non JEDEC conformant or those that do not honor the read-id command
and thus present 0xff _if_ the MISO line is pulled up
that isnt really changed at all by my patch. there is a simple idcode[0] == 0xff branch for people to call probe functions which are prepared to handle non-jedec compatible probes. -mike
On Saturday, August 28, 2010 17:48:39 Mike Frysinger wrote:
/* 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
On 29.08.2010 01:17, Mike Frysinger wrote:
On Saturday, August 28, 2010 17:48:39 Mike Frysinger wrote:
/* 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
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.
Reinhard
On Saturday, August 28, 2010 19:45:57 Reinhard Meyer wrote:
On 29.08.2010 01:17, Mike Frysinger wrote:
On Saturday, August 28, 2010 17:48:39 Mike Frysinger wrote:
/* 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.
arbitrarily long ids unfortunately require an arbitrarily long id_buffer :)
well right, but now the problem has been reduced to simply changing the len of the idcode 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:
hmm, that's true. no point in rescanning the idcode multiple times.
(n is either a compile time constant or determined by examining the table for largest 'shift'+3)
i'd prefer to stick to compile constant for now so we dont have to force the hardware to keep recomputing a number that gcc isnt capable of optimizing into a constant for us.
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...
hmm, pushing the non-jedec probes into the const table is attractive indeed
It is disputable whether the function gets id_buffer or id_buffer+shift as parameter. I'd prefer the latter.
since we already need to calculate the value of id_buffer+shift to check the one byte, i think passing that down is OK for now. i would just make a note in the code about this behavior.
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. -mike
On 29.08.2010 02:14, Mike Frysinger wrote:
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.
Reinhard
On Saturday, August 28, 2010 21:59:26 Reinhard Meyer wrote:
On 29.08.2010 02:14, Mike Frysinger wrote:
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
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.
looks fine to me -mike
On 29.08.2010 04:26, Mike Frysinger wrote:
On Saturday, August 28, 2010 21:59:26 Reinhard Meyer wrote:
On 29.08.2010 02:14, Mike Frysinger wrote:
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?
Reinhard
On Saturday, August 28, 2010 23:35:08 Reinhard Meyer wrote:
On 29.08.2010 04:26, Mike Frysinger wrote:
On Saturday, August 28, 2010 21:59:26 Reinhard Meyer wrote:
On 29.08.2010 02:14, Mike Frysinger wrote:
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 ... -mike
On 29.08.2010 06:34, Mike Frysinger wrote:
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 ...
That's slightly shorter than my method. I'll try it. And I think it can't be shortened any further...
Reinhard
Hi Mike,
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 ...
You could save a step and using 'git add -p' instead of the patch creation/edit/remove: git rebase -i <commit>^ <mark the commit i want to split as "edit"> <exit the rebase-todo window to start the process> git reset HEAD~1 git add -p <go through changes, including, ignoring, splitting as needed> git commit <edit message to reflect reduced changes> git commit -a <edit message to reflect split out changes> git rebase --continue
This has the advantage that you can split up an individual hunk which would be very difficult to do when editing a patch file. The downside is that 'git reset HEAD~1' makes it so that the original commit message isn't preserved, so you have to re-enter or copy it.
Peter
Dear Reinhard Meyer,
In message 4C760243.6030102@emk-elektronik.de you wrote:
Dear Mike Frysinger,
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.
Actually we have a standard feature for issues like this (until we have device tree support in U-Boot :-) - use hwconfig (CONFIG_HWCONFIG).
Best regards,
Wolfgang Denk
participants (5)
-
Mike Frysinger -
Peter Tyser -
Reinhard Meyer -
Reinhard Meyer -
Wolfgang Denk