[U-Boot] [PATCH] MTD/SPI/FLASH: add support for Ramtron FRAMs using SPI

[Mike Frysinger <vapier@gentoo.org>]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100825/7378eb1c/attachment.pgp