Re: [U-Boot] [PATCH 4/6] sandbox: new SPI flash driver
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 000..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[0x1];
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:
+
Re: [U-Boot] [PATCH 4/6] sandbox: new SPI flash driver
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[0x1];
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 0x1 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
signature.asc
Description: This is a digitally signed message part.
