[flashrom] [PATCH] Add Linux SPI support

[Uwe Hermann]

On Wed, Mar 02, 2011 at 11:56:43PM +0100, Carl-Daniel Hailfinger wrote:
> > Signed-off-by: Sven Schnelle <svens at stackframe.org>

Thanks, committed as r1427 with a few changes as proposed below, and
with more changes due to forward-porting to current flashrom trunk.


> Thanks for your patch! Review follows.

I addressed some of the issues (but not all, yet). Shouldn't be a big
deal though, as the driver is disabled by default. The rest of the
changes can be in another patch.

Compile-tested, but not tested on hardware by me.

 
> Cosmetic:
> linux_spi_init() has trailing whitespace in some lines, and spaces
> instead of tabs in some lines.

Fixed.


> Having a man page entry for this would be awesome, but that can be
> postponed until the main patch is in.

Yup, can be extra patch, didn't add something for now.

 
> > +CONFIG_LINUX_SPI ?= yes
> 
> Either set it to no by default, or make the yes conditional on detecting
> a Linux target (not host, because DOS binaries are generated on a Linux
> host). I'd postpone the Linux detection to a later patch.
>
> Adding a short comment why it defaults to no or a comment under which
> circumstances it defaults to yes would be appreciated.
 
Yup, set to no for now, added comment.


> > +int linux_spi_init(void)
> > +{
> > +	char *p, *endp, *dev;
> > +	int speed = 0;
> >   
> 
> int speed, but later you use strtoul which fills an unsigned long.

TODO
 
 
> > +        dev = extract_programmer_param("dev");
> > +        if (!dev || !strlen(dev)) {
> >   
> 
> strlen(dev) can happen if the user specified no device name:
> -p linux_spi:dev=
> Please handle that case separately (error message, abort).

TODO
 
 
> > +                msg_perr("No spi device given. Use flashrom -p "
> > +			 "linux_spi:dev=/dev/spidevX.Y\n");
> > +                return 1;
> > +        }   
> >   
> 
> Maybe add a msg_dbg or msg_spew about the device name specified by the user.

TODO
 
 
> > +        p = extract_programmer_param("speed");
> > +        if (p && strlen(p)) {
> >   
> 
> 
> Please handle the same problem for speed=
 
TODO

 
> > +		speed = strtoul(p, &endp, 10) * 1024;
> > +		if (p == endp) {
> > +			msg_perr("%s: invalid clock: %s\n", __func__, p);
> >   
> 
> Maybe "%s: invalid clock: %s kHz\n" instead?
 
Done.


> > +			return 1;
> > +		}
> >   
> 
> Print the chosen clock frequency here? Check if the supplied clock
> frequency is nonzero and nonnegative?
 
TODO

 
> > +        }   
> > +
> > +	if ((fd = open(dev, O_RDWR)) == -1) {
> > +		msg_perr("%s: failed to open %s: %s\n", __func__,
> > +			 dev, strerror(errno));
> > +		return 1;
> > +	}
> > +
> > +	if (speed > 0 && ioctl(fd, SPI_IOC_WR_MAX_SPEED_HZ, &speed) == -1) {
> >   
> 
> Are you 100% sure that this ioctl wants a signed int?
> http://www.kernel.org/doc/Documentation/spi/spidev says something about
> an u32.
 
TODO

 
> > +		msg_perr("%s: failed to set speed %dHz: %s\n",
> > +			 __func__, speed, strerror(errno));
> > +		close(fd);
> > +		return 1;
> > +	}
> >   
> 
> Not sure if Linux SPI masters use the right CPOL/CPHA and bit ordering
> and bits per word by default, so setting SPI_IOC_WR_MODE and
> SPI_IOC_WR_LSB_FIRST and SPI_IOC_WR_BITS_PER_WORD would probably be a
> very good idea.
 
TODO

 
> > +
> > +	buses_supported = CHIP_BUSTYPE_SPI;
> > +	spi_controller = SPI_CONTROLLER_LINUX;

I changed this, as we now use register_shutdown() in trunk.


I also moved this:

  +#if CONFIG_LINUX_SPI == 1
  +       { /* SPI_CONTROLLER_LINUX */
  +               .command = linux_spi_send_command,
  +               .multicommand = default_spi_send_multicommand,
  +               .read = linux_spi_read,
  +               .write_256 = linux_spi_write_256,
  +       },
  +#endif

to linux_spi.c and used

  static const struct spi_programmer spi_programmer_linux = { ... }
  ...
  register_spi_programmer(&spi_programmer_linux);

as the other SPI programmers do these days.


I also added

        .max_data_read  = MAX_DATA_UNSPECIFIED, /* TODO? */
        .max_data_write = MAX_DATA_UNSPECIFIED, /* TODO? */

in that struct, not sure what the correct values are.


> > +int linux_spi_send_command(unsigned int writecnt, unsigned int readcnt,
> > +		const unsigned char *txbuf, unsigned char *rxbuf)
> > +{
> > +	struct spi_ioc_transfer msg[2] = {
> > +		{ .tx_buf = (unsigned long)txbuf, .len = writecnt },
> > +		{ .rx_buf = (unsigned long)rxbuf, .len = readcnt }
> >   
> 
> Really unsigned long casts for txbuf and rxbuf?

Required it seems, as otherwise:

  linux_spi.c:111:4: error: initialization makes integer from pointer without a cast [-Werror]
  linux_spi.c:111:4: error: (near initialization for ‘msg[0].tx_buf’) [-Werror]
  etc.

This is due to .tx_buf/.rx_buf being defined as __u64 in linux/spi/spidev.h:

  struct spi_ioc_transfer {
          __u64           tx_buf;
          __u64           rx_buf;
  	...
  }

I changed "unsigned long" to "uint64_t", though.
 
 
> > +int linux_spi_read(struct flashchip *flash, uint8_t *buf, int start, int len)
> > +{
> > +	return spi_read_chunked(flash, buf, start, len, 12);
> >   
> 
> Should not be 12, but rather getpagesize()

Fixed.


> and a comment that max transfer size is one page.

?


> > +int linux_spi_write_256(struct flashchip *flash, uint8_t *buf, int start, int len)
> > +{
> > +	return spi_write_chunked(flash, buf, start, len, 12);
> 
> Should not be 12, but rather getpagesize()-4 with a similar comment as
> above. We lose 4 bytes at the beginning because we have to write
> command+addr to the SPI line.

Fixed, but no idea what a good comment would look like (same as above).


Sven, can you please check if trunk works for you, and/or fix it if I
broke stuff? Also, do you plan to work on addressing the other TODOs?
I guess I have some board where I can test this driver too, but it will
take a while to setup etc.


Thanks, Uwe.
-- 
http://hermann-uwe.de     | http://sigrok.org
http://randomprojects.org | http://unmaintained-free-software.org