[PATCH] Add Linux SPI support

[Sven Schnelle]

Hi List,

this patch adds support for the Linux SPI subsystem. See 
http://www.kernel.org/doc/Documentation/spi/spidev for a 
short introduction.

Usage is as follows:

flashrom -p linux_spi:dev=/dev/spidevX.Y

where X is the bus number, and Y device. It accepts an optional
parameter 'speed' which allows to set the SPI CLK speed in KHz.

I'm using a AVR32 Board
(http://www.atmel.com/dyn/products/tools_card.asp?tool_id=4102) to
program my ThinkPad X60, but it should work on every Linux system.

Signed-off-by: Sven Schnelle

[Carl-Daniel Hailfinger]

Auf 25.02.2011 23:31, Sven Schnelle schrieb:
> Hi List,
>
> this patch adds support for the Linux SPI subsystem. See 
> http://www.kernel.org/doc/Documentation/spi/spidev for a 
> short introduction.
>
> Usage is as follows:
>
> flashrom -p linux_spi:dev=/dev/spidevX.Y
>
> where X is the bus number, and Y device. It accepts an optional
> parameter 'speed' which allows to set the SPI CLK speed in KHz.
>
> I'm using a AVR32 Board
> (http://www.atmel.com/dyn/products/tools_card.asp?tool_id=4102) to
> program my ThinkPad X60, but it should work on every Linux system.
>
> Signed-off-by: Sven Schnelle

Thanks for your patch! Review follows.

Cosmetic:
linux_spi_init() has trailing whitespace in some lines, and spaces
instead of tabs in some lines.
Having a man page entry for this would be awesome, but that can be
postponed until the main patch is in.
> Index: Makefile
>
> --- Makefile	(revision 1261)
> +++ Makefile	(working copy)
> @@ -152,6 +152,8 @@
>  # Always enable Bus Pirate SPI for now.
>  CONFIG_BUSPIRATE_SPI ?= yes
>
> +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.
> # Disable Dediprog SF100 until support is complete and tested.
>  CONFIG_DEDIPROG ?= no
> Index: linux_spi.c
>
> --- linux_spi.c	(revision 0)
> +++ linux_spi.c	(revision 0)
> @@ -0,0 +1,111 @@
> +int linux_spi_init(void)
> +{
> char *p, *endp, *dev;
> int speed = 0;

int speed, but later you use strtoul which fills an unsigned long.
>    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).
>            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.
>    p = extract_programmer_param("speed");
>    if (p && strlen(p)) {

Please handle the same problem for speed=
> 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?
> 	return 1;
> }

Print the chosen clock frequency here? Check if the supplied clock
frequency is nonzero and nonnegative?
>    }   
> 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.
> 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.
> buses_supported = CHIP_BUSTYPE_SPI;
> spi_controller = SPI_CONTROLLER_LINUX;
> return 0;
>
> +}
>
> +int linux_spi_shutdown(void)
> +{
> if (fd != -1) {
> close(fd);
> fd = -1;
> }
> return 0;
>
> +}
>
> +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?
> };
> if (fd == -1)
> return -1;
> if (ioctl(fd, SPI_IOC_MESSAGE(2), msg) == -1) {
> msg_cerr("%s: ioctl: %s\n", __func__, strerror(errno));
> return -1;
> }
> return 0;
>
> +}
>
> +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() 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.
> +}

I think the next iteration will definitely be ackable.

Regards,
Carl-Daniel

[Uwe Hermann]

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

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.

[Sven Schnelle]

Uwe Hermann  writes:
> On Wed, Mar 02, 2011 at 11:56:43PM +0100, Carl-Daniel Hailfinger wrote:
> > > Signed-off-by: Sven Schnelle
>
> Thanks, committed as r1427 with a few changes as proposed below, and
> with more changes due to forward-porting to current flashrom trunk.

Thanks! I must admit that i actually forgot about that patch (but using
it for about half a year now ;-)
> > > +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.

That breaks compilation on 32bit for me (AVR32 is 32bit):

linux_spi.c:110: warning: cast from pointer to integer of different size
linux_spi.c:114: warning: cast from pointer to integer of different size

I've changed the code to:

Index: linux_spi.c
===================================================================
--- linux_spi.c	(revision 1427)
+++ linux_spi.c	(working copy)
@@ -107,11 +107,11 @@
 {
    struct spi_ioc_transfer msg[2] = {
    	{
-			.tx_buf = (uint64_t)txbuf,
+			.tx_buf = (uint64_t)(ptrdiff_t)txbuf,
    		.len = writecnt,
    	},
    	{
-			.rx_buf = (uint64_t)rxbuf,
+			.rx_buf = (uint64_t)(ptrdiff_t)rxbuf,
    		.len = readcnt,
    	},
    };
> 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.

I've did a short test with the patch above, and flashrom works fine on
linux-2.6.38. Will respond to the other TODOs in a seperate Mail.

Sven.

[Uwe Hermann]

On Mon, Sep 05, 2011 at 10:09:48PM +0200, Sven Schnelle wrote:
> That breaks compilation on 32bit for me (AVR32 is 32bit):

Yeah, my bad, sorry. Fixed in r1428.

Uwe.

[Sven Schnelle]

Hi Uwe,

Uwe Hermann  writes:
> > > +int linux_spi_init(void)
> > > +{
> > > char *p, *endp, *dev;
> > > int speed = 0;
> >
> > int speed, but later you use strtoul which fills an unsigned long.
>
> Fixed in the attached patch.
> > >    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).

How can that happen? The NULL pointer and zero length cases are handled
correctly. With the current trunk i get:

# ./flashrom -r test.bin -p linux_spi:dev=
flashrom v0.9.4-r1427 on Linux 2.6.38+ (avr32), built with GCC
4.2.4-atmel.1.1.3.avr32linux.1, big endian
flashrom is free software, get the source code at
http://www.flashrom.org

Calibrating delay loop... OK.
No SPI device given. Use flashrom -p linux_spi:dev=/dev/spidevX.Y
Error: Programmer initialization failed.

Which looks ok for me.
> > >            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.
>
> Done.
> > >    p = extract_programmer_param("speed");
> > >    if (p && strlen(p)) {
> >
> > Please handle the same problem for speed=
>
> Same question here :)
> > >    }   
> > > 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.
>
> Fixed.
> > 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.

The spi_board_info structure in the board specific linux kernel code
specify the default values. Such a structure has to exist for every
single SPI device that sits on the Board, so we have correct default
values set in the Kernel. As a feature we might still make that
configurable later.

Signed-off-by: Sven Schnelle

Index: linux_spi.c
===================================================================
--- linux_spi.c	(revision 1427)
+++ linux_spi.c	(working copy)
@@ -54,7 +54,7 @@
 int linux_spi_init(void)
 {
    char *p, *endp, *dev;
-	int speed = 0;
+	uint32_t speed = 0;

dev = extract_programmer_param("dev");
    if (!dev || !strlen(dev)) {
@@ -65,31 +65,35 @@

p = extract_programmer_param("speed");
    if (p && strlen(p)) {
-		speed = strtoul(p, &endp, 10) * 1024;
+		speed = (uint32_t)strtoul(p, &endp, 10) * 1024;
    	if (p == endp) {
    		msg_perr("%s: invalid clock: %s kHz\n", __func__, p);
    		return 1;
    	}
    }

+	msg_pdbg("Using device %s\n", dev);
    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) {
-		msg_perr("%s: failed to set speed %dHz: %s\n",
-			 __func__, speed, strerror(errno));
-		close(fd);
-		return 1;
-	}
+	if (speed > 0) {
+		if (ioctl(fd, SPI_IOC_WR_MAX_SPEED_HZ, &speed) == -1) {
+			msg_perr("%s: failed to set speed %dHz: %s\n",
+				 __func__, speed, strerror(errno));
+			close(fd);
+			return 1;
+		}

+		msg_pdbg("Using %d KHz clock\n", speed);
+        }
+
    if (register_shutdown(linux_spi_shutdown, NULL))
    	return 1;

register_spi_programmer(&spi_programmer_linux);
-
    return 0;
 }

[Uwe Hermann]

On Mon, Sep 05, 2011 at 10:34:37PM +0200, Sven Schnelle wrote:
> Signed-off-by: Sven Schnelle

Thanks, r1432.

Uwe.