Hi Mike, On Sun, Jan 22, 2012 at 10:30 PM, Mike Frysinger <vap...@gentoo.org> wrote: > This adds a SPI flash driver which simulates SPI flash clients. > Currently supports the bare min that U-Boot requires: you can > probe, read, erase, and write. Should be easy to extend to make > it behave more exactly like a real SPI flash, but this is good > enough to merge now. > > Signed-off-by: Mike Frysinger <vap...@gentoo.org> > --- > drivers/mtd/spi/Makefile | 1 + > drivers/mtd/spi/sandbox.c | 318 > +++++++++++++++++++++++++++++++++++++++++++++ > drivers/spi/sandbox_spi.c | 5 + > 3 files changed, 324 insertions(+), 0 deletions(-) > create mode 100644 drivers/mtd/spi/sandbox.c
Again would like a few comments / docs. I'm pleased you have done this - it is much more comprehensive than my noddy implementation. > > diff --git a/drivers/mtd/spi/Makefile b/drivers/mtd/spi/Makefile > index 90f8392..fb37807 100644 > --- a/drivers/mtd/spi/Makefile > +++ b/drivers/mtd/spi/Makefile > @@ -33,6 +33,7 @@ COBJS-$(CONFIG_SPI_FLASH) += spi_flash.o > COBJS-$(CONFIG_SPI_FLASH_ATMEL) += atmel.o > COBJS-$(CONFIG_SPI_FLASH_EON) += eon.o > COBJS-$(CONFIG_SPI_FLASH_MACRONIX) += macronix.o > +COBJS-$(CONFIG_SPI_FLASH_SANDBOX) += sandbox.o > COBJS-$(CONFIG_SPI_FLASH_SPANSION) += spansion.o > COBJS-$(CONFIG_SPI_FLASH_SST) += sst.o > COBJS-$(CONFIG_SPI_FLASH_STMICRO) += stmicro.o > diff --git a/drivers/mtd/spi/sandbox.c b/drivers/mtd/spi/sandbox.c > new file mode 100644 > index 0000000..a1bb641 > --- /dev/null > +++ b/drivers/mtd/spi/sandbox.c > @@ -0,0 +1,318 @@ > +/* > + * Simulate a SPI flash > + * > + * Copyright (c) 2011-2012 The Chromium OS Authors. > + * See file CREDITS for list of people who contributed to this > + * project. > + * > + * Licensed under the GPL-2 or later. > + */ > + > +#include <common.h> > +#include <malloc.h> > +#include <spi.h> > +#include <os.h> > + > +#include <spi_flash.h> > +#include "spi_flash_internal.h" > + > +#include <asm/spi.h> > + These are the different commands, right? > +typedef enum { > + SF_CMD, SF_ID, SF_READ, SF_WRITE, SF_ERASE, SF_READ_STATUS, SF_WREN, > SF_WRDI, > +} sb_sf_state; > + > +#define STAT_WIP (1 << 0) > +#define STAT_WEL (1 << 1) What are these? Status bus? If so, perhaps a comment for each? > + > +struct sb_spi_flash_erase_commands { > + u8 cmd; > + u32 size; > +}; > +#define IDCODE_LEN 5 > +#define MAX_ERASE_CMDS 2 > +struct sb_spi_flash_data { > + const char *name; > + u8 idcode[IDCODE_LEN]; > + u32 size; > + const struct sb_spi_flash_erase_commands erase_cmds[MAX_ERASE_CMDS]; > +}; > + > +struct sb_spi_flash { > + sb_sf_state state; > + uint off; > + const struct sb_spi_flash_data *data; > + int fd; > + u8 status; > +}; All of the above could do with some comments IMO... > + > +static const char *sb_sf_state_name(sb_sf_state state) > +{ > + static const char * const states[] = { > + "CMD", "ID", "READ", "WRITE", "ERASE", "READ_STATUS", "WREN", > "WRDI", > + }; I wonder if it would be better to put this array up next to the enum? > + return states[state]; > +} > + These are the emulated devices I think. > +static const struct sb_spi_flash_data sb_sf_flashes[] = { > + { > + "M25P16", { 0x20, 0x20, 0x15 }, (2 * 1024 * 1024), > + { /* erase commands */ > + { 0xd8, (64 * 1024), }, /* sector */ > + { 0xc7, (2 * 1024 * 1024), }, /* bulk */ Is <<10, <<20 better? Not sure. > + }, > + }, > +}; > + > +static u8 sb_sf_0xff[0x10000]; Perhaps we should add an os_fputc() instead? Or perhaps write in smaller chunks? > + > +static int sb_sf_setup(void **priv, const char *spec) > +{ > + /* spec = idcode:file */ > + struct sb_spi_flash *sbsf; > + const char *file; > + size_t i, len, idname_len; > + const struct sb_spi_flash_data *data; > + > + file = strchr(spec, ':'); > + if (!file) > + goto error; > + idname_len = file - spec; > + ++file; > + > + for (i = 0; i < ARRAY_SIZE(sb_sf_flashes); ++i) { > + data = &sb_sf_flashes[i]; > + len = strlen(data->name); > + if (idname_len != len) > + continue; > + if (!memcmp(spec, data->name, len)) > + break; > + } > + if (i == ARRAY_SIZE(sb_sf_flashes)) { > + printf("sandbox_spi_flash: unknown flash '%*s'\n", > + (int)idname_len, file); > + goto error; > + } > + > + if (sb_sf_0xff[0] == 0x00) > + memset(sb_sf_0xff, 0xff, sizeof(sb_sf_0xff)); > + > + sbsf = malloc(sizeof(*sbsf)); > + if (!sbsf) > + goto error; > + > + sbsf->fd = os_open(file, 02); > + if (sbsf->fd == -1) { > + free(sbsf); > + goto error; > + } > + > + sbsf->state = SF_CMD; > + sbsf->data = data; > + > + *priv = sbsf; > + return 0; > + > + error: > + printf("sandbox_spi_flash: unable to parse client spec\n"); or out of memory or file would not open > + return 1; > +} > + > +static void sb_sf_free(void *priv) > +{ > + struct sb_spi_flash *sbsf = priv; > + > + os_close(sbsf->fd); > + free(sbsf); > +} > + > +static void sb_sf_cs_deactivate(void *priv) > +{ > + struct sb_spi_flash *sbsf = priv; > + > + /* CS is no longer being asserted, so reset state */ > + sbsf->state = SF_CMD; > +} > + > +static int sb_sf_xfer(void *priv, const u8 *tx, u8 *rx, > + uint bytes) > +{ > + struct sb_spi_flash *sbsf = priv; > + uint i, written = 0; > + > + debug("sb_sf: state:%x(%s) bytes:%u\n", sbsf->state, > + sb_sf_state_name(sbsf->state), bytes); > + > + if (sbsf->state == SF_CMD) { I wonder if this if() could be split out as it makes the function very long. Might be hard if too many parameters though. > + sb_sf_state oldstate = sbsf->state; > + > + rx[written++] = 0; > + sb_spi_tristate(rx, 1); > + > + /* > + * XXX: we assume cmds that need addresses get them in one > + * transfer rather than accumulating ... but u-boot > + * doesn't care at all. > + */ > + > + switch (tx[0]) { > + case CMD_READ_ID: > + sbsf->off = 0; > + sbsf->state = SF_ID; > + break; > + case CMD_PAGE_PROGRAM: > + /* XXX: check WEL in status */ > + > + /* > + * XXX: need to handle exotic behavior: > + * - unaligned addresses > + * - too big of an address > + */ > + > + sbsf->off = (tx[1] << 16) | (tx[2] << 8) | tx[3]; > + os_lseek(sbsf->fd, sbsf->off, 0); > + > + written += 3; > + sb_spi_tristate(&rx[1], 3); > + > + sbsf->state = SF_WRITE; > + sbsf->status &= ~STAT_WEL; > + break; > + case CMD_READ_ARRAY_SLOW: > + case CMD_READ_ARRAY_FAST: { > + uint alen = 3; > + > + sbsf->off = (tx[1] << 16) | (tx[2] << 8) | tx[3]; > + os_lseek(sbsf->fd, sbsf->off, 0); > + debug(" read addr: %u\n", sbsf->off); > + > + if (tx[0] == CMD_READ_ARRAY_FAST) > + /* read fast needs a dummy byte */ > + ++alen; > + > + written += alen; > + sb_spi_tristate(&rx[1], alen); > + > + sbsf->state = SF_READ; > + break; > + } Are you allowed to not indent the {} in this case? The } ends up at the same level as the switch(). > + case CMD_WRITE_DISABLE: > + debug(" write disabled\n"); > + sbsf->status &= ~STAT_WEL; > + break; > + case CMD_READ_STATUS: > + sbsf->state = SF_READ_STATUS; > + break; > + case CMD_WRITE_ENABLE: > + debug(" write enabled\n"); > + sbsf->status |= STAT_WEL; > + break; > + default: > + /* handle erase commands first */ > + for (i = 0; i < MAX_ERASE_CMDS; ++i) { > + const struct sb_spi_flash_erase_commands > *erase_cmd = > + &sbsf->data->erase_cmds[i]; > + > + if (erase_cmd->cmd == 0x00) > + continue; > + if (tx[0] != erase_cmd->cmd) > + continue; > + > + /* XXX: check WEL in status */ > + > + /* verify address is aligned */ > + sbsf->off = (tx[1] << 16) | (tx[2] << 8) | > tx[3]; > + if (sbsf->off & ~(erase_cmd->size - 1)) { > + debug(" sector erase: cmd:%#x needs > align:%#x, but we got %#x\n", > + erase_cmd->cmd, > erase_cmd->size, sbsf->off); > + sbsf->status &= ~STAT_WEL; > + return 1; > + } > + > + os_lseek(sbsf->fd, sbsf->off, 0); > + debug(" sector erase addr: %u\n", sbsf->off); > + > + written += 3; > + sb_spi_tristate(&rx[1], 3); > + > + /* XXX: latch WIP in status, and delay before > clearing it ? */ > + os_write(sbsf->fd, sb_sf_0xff, > erase_cmd->size); > + sbsf->status &= ~STAT_WEL; > + goto cmd_done; > + } > + > + debug(" cmd unknown: %#x\n", tx[0]); > + return 1; > + } > + > + cmd_done: > + if (oldstate != sbsf->state) > + debug(" cmd: transition to %s state\n", > + sb_sf_state_name(sbsf->state)); > + } > + > + if (written == bytes) > + return 0; > + > + switch (sbsf->state) { > + case SF_ID: { > + const u8 *idcode = sbsf->data->idcode; > + > + debug(" id: off:%u\n tx:", sbsf->off); > + for (i = sbsf->off; i < IDCODE_LEN; ++i) { > + if (written < bytes) { > + rx[written++] = idcode[i]; > + debug(" %02x", idcode[i]); > + } else > + break; > + } > + if (written < bytes) { > + i = bytes - written; Do you think you should use a different variable than i in this case and below? I doubt it would affect the code output. > + debug(" <memset remaining %u bytes>", i); > + memset(rx + written, 0, i); > + written += i; > + } > + debug("\n"); > + break; > + } > + case SF_READ: > + /* > + * XXX: need to handle exotic behavior: > + * - reading past end of device > + */ > + i = bytes - written; > + debug(" tx: read(%u)\n", i); > + if (i) > + written += os_read(sbsf->fd, rx + written, i); > + break; > + case SF_READ_STATUS: > + debug(" read status: %#x\n", sbsf->status); > + i = bytes - written; > + memset(rx + written, sbsf->status, i); > + written += i; > + break; > + case SF_WRITE: > + /* > + * XXX: need to handle exotic behavior: > + * - more than a page (256) worth of data > + * - reading past end of device > + */ > + i = bytes - written; > + debug(" rx: write(%u)\n", i); > + if (i) > + written += os_write(sbsf->fd, tx + written, i); > + break; > + default: > + debug(" ??? no idea what to do ???\n"); > + break; > + } > + > + return written == bytes ? 0 : 1; > +} > + > +const struct sb_spi_emu_ops sb_sf_ops = { > + .setup = sb_sf_setup, > + .free = sb_sf_free, > + .cs_deactivate = sb_sf_cs_deactivate, > + .xfer = sb_sf_xfer, > +}; > diff --git a/drivers/spi/sandbox_spi.c b/drivers/spi/sandbox_spi.c > index eda0f3e..ae15946 100644 > --- a/drivers/spi/sandbox_spi.c > +++ b/drivers/spi/sandbox_spi.c > @@ -35,10 +35,15 @@ static const char *sb_lookup_arg(unsigned int bus, > unsigned int cs) > return os_getopt(sf_arg, 1); > } > > +extern const struct sb_spi_emu_ops sb_sf_ops; > + > static const struct { > const char *spec; > const struct sb_spi_emu_ops *ops; > } sb_emu_map[] = { > +#ifdef CONFIG_SPI_FLASH_SANDBOX > + { "sf", &sb_sf_ops, }, > +#endif > }; > > static int sb_parse_type(struct sb_spi_slave *sss) > -- > 1.7.8.3 > Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot