[U-Boot,4/6] sandbox: new SPI flash driver

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 <vapier@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

Hi Mike,

On Sun, Jan 22, 2012 at 10:30 PM, Mike Frysinger <vapier@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 <vapier@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

On Monday 23 January 2012 19:41:42 Simon Glass wrote:
> On Sun, Jan 22, 2012 at 10:30 PM, Mike Frysinger wrote:
> > --- /dev/null
> > +++ b/drivers/mtd/spi/sandbox.c
> >
> > +#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?

are you referring to the enum after your quote ?

> > +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?

the STAT defines are for the status register (RDSR).  these low bits are about 
the only ones that SPI flashes have in common.  the others tend to diverge real 
quick (for things like locking different portions of the flash).

> > +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?

my only reason for putting it here was to scope it.  now only this func has 
access to the array and so it's the code enforces that.

> > +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.

personally i find (xxx * 1024 * 1024) easier to understand than bitshifts when 
it comes to sizes.  but that could just be that i'm not used to reading 
things.

> > +static u8 sb_sf_0xff[0x10000];
> 
> Perhaps we should add an os_fputc() instead?

fputc() operates on a FILE*, not a fd.  i don't think we want that.  it's easy 
to emulate putc() with write().

however, we don't want to do that here.  calling putc() 65 thousand times to 
avoid an array of 65 thousand bytes is the wrong trade off imo ;).

> Or perhaps write in smaller chunks?

i picked 0x10000 because it was the largest erase block size and be lazy with 
a single write().  i could shrink it down to 0x1000 and then loop over in 4k 
chunks.  but i'm not too worried about buffers that are 65kb when we talk about 
code that only runs on our 64bit dev platform ;).

> > +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.

yeah, it is a long func.  partly due to it starting small and me slowly 
expanding the state machine, and partly due to the state matching liking to be 
in one func.  i might be able to make it work though ...

> > +               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().

yes, it's ugly, but it's what i've seen in the kernel/u-boot.  i just swallow 
hard and try not to look too hard.

> > +       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.

i'm sure there's no code difference ;).  i used "i" since it's available for 
scratch in this whole func.  should be easy to tweak.
-mike