[U-Boot] [PATCH 4/6] sandbox: new SPI flash driver
Simon Glass
sjg at chromium.org
Tue Jan 24 01:41:42 CET 2012
Hi Mike,
On Sun, Jan 22, 2012 at 10:30 PM, Mike Frysinger <vapier at 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 at 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
More information about the U-Boot
mailing list