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

[Mike Frysinger]

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
-------------- 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/20120123/15599de1/attachment.pgp>