[PATCH] do something useful with the dediprog LEDs

[Stefan Reinauer]

This patch makes use of the dediprog LEDs.

It would be nice to have some "more global" infrastructure for device
dependent status reporting. Right now some scenarios can not be
reported as an error, though the user would think of them as one. For
example, if the chip is IDed successfully but not supported by
flashrom, thus it can not be flashed. At best, the dediprog driver
sees a successful RDID command, but has no knowledge about the failure
to read/write the chip. Therefore it can not report it.

However, the attached patch improves my work flow here quite a bit, so
I thought it should be shared.

[Mathias Krause]

On 21.01.2011 20:09, Stefan Reinauer wrote:
> This patch makes use of the dediprog LEDs.

Nice!
> It would be nice to have some "more global" infrastructure for device
> dependent status reporting. Right now some scenarios can not be
> reported as an error, though the user would think of them as one. For
> example, if the chip is IDed successfully but not supported by
> flashrom, thus it can not be flashed. At best, the dediprog driver
> sees a successful RDID command, but has no knowledge about the failure
> to read/write the chip. Therefore it can not report it.
>
> However, the attached patch improves my work flow here quite a bit, so
> I thought it should be shared.

Thanks for the patch. Albeit it would be good to have a "noleds" option
to disable the functionality because we don't know if it breaks older
firmware versions.
> Implement proper LED handling for dediprog.
>
> ... so you don't have to stare at the screen to see when the write is 
> finished and whether it worked.
>
> Signed-off-by: Stefan Reinauer
> Index: dediprog.c
>
> --- dediprog.c	(revision 1254)
> +++ dediprog.c	(working copy)
> @@ -58,6 +58,33 @@
>
> //int usb_control_msg(usb_dev_handle *dev, int requesttype, int request, int value, int index, char *bytes, int size, int timeout);
>
> +/* Set/clear LEDs on dediprog. */
> +#define PASS_ON		(0 << 0)
> +#define PASS_OFF	(1 << 0)
> +#define BUSY_ON		(0 << 1)
> +#define BUSY_OFF	(1 << 1)
> +#define ERROR_ON	(0 << 2)
> +#define ERROR_OFF	(1 << 2)
> +static int current_led_status = 0;

Should be initialized to -1, see below why.
> +static int dediprog_set_leds(int leds)
> +{
> int ret;
> if (leds < 0 || leds > 7)
> leds = 0; // Bogus value, enable all LEDs

We can skip sending the USB command when the LED already has the value
we want it to. Something like:

if (leds == current_led_status)
    return 0;
> ret = usb_control_msg(dediprog_handle, 0x42, 0x07, 0x09, leds, NULL, 0x0, DEFAULT_TIMEOUT);
> if (ret != 0x0) {
> msg_perr("Command Set LED 0x%x failed (%s)!\n", leds, usb_strerror());
> return 1;
> }
> current_led_status = leds;
> return 0;
>
> +}
>
> static int dediprog_set_spi_voltage(int millivolt)
>  {
>    int ret;
> @@ -211,20 +238,26 @@
>    int residue = start % chunksize ? chunksize - start % chunksize : 0;
>    int bulklen;
> dediprog_set_leds(PASS_OFF|BUSY_ON|ERROR_OFF);
> if (residue) {
>   msg_pdbg("Slow read for partial block from 0x%x, length 0x%x\n",
>   	 start, residue);
>   ret = spi_read_chunked(flash, buf, start, residue, 16);
> if (ret)
> if (ret) {
> 	dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> return ret;
> }
>
> }
>
> /* Round down. */
> bulklen = (len - residue) / chunksize * chunksize;
> ret = dediprog_spi_bulk_read(flash, buf + residue, start + residue,
> 		     bulklen);
> if (ret)
>
> if (ret) {
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
>
> return ret;
>
> }
>
> len -= residue + bulklen;
> if (len) {
>
> @@ -232,17 +265,29 @@
>    		 start, len);
>    	ret = spi_read_chunked(flash, buf + residue + bulklen,
>    			       start + residue + bulklen, len, 16);
> if (ret)
> if (ret) {
> 	dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> return ret;
> }
>
> }
>
> dediprog_set_leds(PASS_ON|BUSY_OFF|ERROR_OFF);
> return 0;
>
> }
>
> int dediprog_spi_write_256(struct flashchip *flash, uint8_t *buf, int start, int len)
>  {
> int ret;
> dediprog_set_leds(PASS_OFF|BUSY_ON|ERROR_OFF);
> /* No idea about the real limit. Maybe 12, maybe more, maybe less. */
> return spi_write_chunked(flash, buf, start, len, 12);
> ret = spi_write_chunked(flash, buf, start, len, 12);
> if (ret)
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> else
> dediprog_set_leds(PASS_ON|BUSY_OFF|ERROR_OFF);

Missing 'return ret;'
> }
>
> int dediprog_spi_send_command(unsigned int writecnt, unsigned int readcnt,
> @@ -410,81 +455,8 @@
>    }
>    return 0;
>  }
>
> -/* Start/stop blinking?
> Present in eng_detect_blink.log with firmware 3.1.8
> Preceded by Command J
> */
>
> -static int dediprog_command_g(void)
> -{
> int ret;
> ret = usb_control_msg(dediprog_handle, 0x42, 0x07, 0x09, 0x03, NULL, 0x0, DEFAULT_TIMEOUT);
> if (ret != 0x0) {
> msg_perr("Command G failed (%s)!\n", usb_strerror());
> return 1;
> }
> return 0;
>
> -}
>
> -/* Something.
> Present in all logs with firmware 5.1.5
> Always preceded by Command Receive Device String
> Always followed by Command Set SPI Voltage nonzero
> */
>
> -static int dediprog_command_h(void)
> -{
> int ret;
> ret = usb_control_msg(dediprog_handle, 0x42, 0x07, 0x09, 0x05, NULL, 0x0, DEFAULT_TIMEOUT);
> if (ret != 0x0) {
> msg_perr("Command H failed (%s)!\n", usb_strerror());
> return 1;
> }
> return 0;
>
> -}
>  #endif
>
> -/* Shutdown for firmware 5.x?
> This command swithces the "Pass" LED on.
> Present in all logs with firmware 5.1.5
> Often preceded by a SPI operation (Command Read SPI Bulk or Receive SPI)
> Always followed by Command Set SPI Voltage 0x0000
> */
>
> -static int dediprog_command_i(void)
> -{
> int ret;
> ret = usb_control_msg(dediprog_handle, 0x42, 0x07, 0x09, 0x06, NULL, 0x0, DEFAULT_TIMEOUT);
> if (ret != 0x0) {
> msg_perr("Command I failed (%s)!\n", usb_strerror());
> return 1;
> }
> return 0;
>
> -}
>
> -#if 0
> -/* Start/stop blinking?
> Present in all logs with firmware 5.1.5
> Always preceded by Command Receive Device String on 5.1.5
> Always followed by Command Set SPI Voltage nonzero on 5.1.5
> Present in eng_detect_blink.log with firmware 3.1.8
> Preceded by Command B in eng_detect_blink.log
> Followed by Command G in eng_detect_blink.log
> */
>
> -static int dediprog_command_j(void)
> -{
> int ret;
> ret = usb_control_msg(dediprog_handle, 0x42, 0x07, 0x09, 0x07, NULL, 0x0, DEFAULT_TIMEOUT);
> if (ret != 0x0) {
> msg_perr("Command J failed (%s)!\n", usb_strerror());
> return 1;
> }
> return 0;
>
> -}
> -#endif
>
> static int parse_voltage(char *voltage)
>  {
>    char *tmp = NULL;
> @@ -584,22 +556,35 @@
>    	return 1;
>    }
>    dediprog_endpoint = 2;
> dediprog_set_leds(PASS_ON|BUSY_ON|ERROR_ON);
> /* URB 6. Command A. */
> if (dediprog_command_a())
> if (dediprog_command_a()) {
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> return 1;
> }
> /* URB 7. Command A. */
> if (dediprog_command_a())
> if (dediprog_command_a()) {
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> return 1;
> }
> /* URB 8. Command Prepare Receive Device String. */
> /* URB 9. Command Receive Device String. */
> if (dediprog_check_devicestring())
> if (dediprog_check_devicestring()) {
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> return 1;
> }
> /* URB 10. Command C. */
> if (dediprog_command_c())
> if (dediprog_command_c()) {
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> return 1;
> }
> /* URB 11. Command Set SPI Voltage. */
> if (dediprog_set_spi_voltage(millivolt))
>
> if (dediprog_set_spi_voltage(millivolt)) {
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
>
> return 1;
>
> }
>
> buses_supported = CHIP_BUSTYPE_SPI;
> spi_controller = SPI_CONTROLLER_DEDIPROG;
>
> @@ -610,6 +595,8 @@
>    dediprog_do_stuff();
>  #endif
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_OFF);
> return 0;
>
> }
>
> @@ -670,10 +657,6 @@
>  {
>    msg_pspew("%s\n", __func__);
> /* Shutdown on firmware 5.x */
> if (dediprog_firmwareversion == 5)
> if (dediprog_command_i())
> 	return 1;
> /* URB 28. Command Set SPI Voltage to 0. */
> if (dediprog_set_spi_voltage(0x0))
> return 1;

Otherwise looks good to me.

Tested-by: Mathias Krause

With the above mentioned changes:

Acked-by: Mathias Krause

[Stefan Reinauer]

* Mathias Krause  [110124 11:14]:
> On 21.01.2011 20:09, Stefan Reinauer wrote:
> > This patch makes use of the dediprog LEDs.
>
> Nice!
> > It would be nice to have some "more global" infrastructure for device
> > dependent status reporting. Right now some scenarios can not be
> > reported as an error, though the user would think of them as one. For
> > example, if the chip is IDed successfully but not supported by
> > flashrom, thus it can not be flashed. At best, the dediprog driver
> > sees a successful RDID command, but has no knowledge about the failure
> > to read/write the chip. Therefore it can not report it.
> >
> > However, the attached patch improves my work flow here quite a bit, so
> > I thought it should be shared.
>
> Thanks for the patch. Albeit it would be good to have a "noleds" option
> to disable the functionality because we don't know if it breaks older
> firmware versions.

Ok, I'll hold it off until we get a test report from an older firmware
version. I'd like to avoid adding the noleds option if possible, and
make the code contain the knowledge about the capabilities of the
firmware version rather than the user.

Thanks for the review. I fixed your findings.

Stefan

[Stefan Reinauer]

On Mon, Jan 24, 2011 at 2:14 AM, Mathias Krause
 wrote:
> On 21.01.2011 20:09, Stefan Reinauer wrote:
> > This patch makes use of the dediprog LEDs.
>
> Nice!
> > It would be nice to have some "more global" infrastructure for device
> > dependent status reporting. Right now some scenarios can not be
> > reported as an error, though the user would think of them as one. For
> > example, if the chip is IDed successfully but not supported by
> > flashrom, thus it can not be flashed. At best, the dediprog driver
> > sees a successful RDID command, but has no knowledge about the failure
> > to read/write the chip. Therefore it can not report it.
> >
> > However, the attached patch improves my work flow here quite a bit, so
> > I thought it should be shared.
>
> Thanks for the patch. Albeit it would be good to have a "noleds" option
> to disable the functionality because we don't know if it breaks older
> firmware versions.

All changes added, except this one. Instead the updated version
explicitly support older firmware versions.

[Mathias Krause]

On 27.01.2011 22:02, Stefan Reinauer wrote:
> On Mon, Jan 24, 2011 at 2:14 AM, Mathias Krause
>  wrote:
> > On 21.01.2011 20:09, Stefan Reinauer wrote:
> > > This patch makes use of the dediprog LEDs.
> >
> > Nice!
> > > It would be nice to have some "more global" infrastructure for device
> > > dependent status reporting. Right now some scenarios can not be
> > > reported as an error, though the user would think of them as one. For
> > > example, if the chip is IDed successfully but not supported by
> > > flashrom, thus it can not be flashed. At best, the dediprog driver
> > > sees a successful RDID command, but has no knowledge about the failure
> > > to read/write the chip. Therefore it can not report it.
> > >
> > > However, the attached patch improves my work flow here quite a bit, so
> > > I thought it should be shared.
> >
> > Thanks for the patch. Albeit it would be good to have a "noleds" option
> > to disable the functionality because we don't know if it breaks older
> > firmware versions.
>
> All changes added, except this one. Instead the updated version
> explicitly support older firmware versions.

Thanks! Looks like a better solution to the problem. Albeit I found two
(read: one and a half) more hunks to complain about ;)
> Support Dediprog LEDs on devices with 2 and 3 LEDs.
>
> Signed-off-by: Stefan Reinauer
> Index: dediprog.c
>
> --- dediprog.c	(revision 1256)
> +++ dediprog.c	(working copy)
> @@ -25,6 +25,7 @@
>  #include "programmer.h"
>  #include "spi.h"
>
> +#define FIRMWARE_VERSION(x,y,z) ((x << 16) | (y << 8) | z)
>  #define DEFAULT_TIMEOUT 3000
>  static usb_dev_handle *dediprog_handle;
>  static int dediprog_firmwareversion;
> @@ -58,6 +59,50 @@
>
> //int usb_control_msg(usb_dev_handle *dev, int requesttype, int request, int value, int index, char *bytes, int size, int timeout);
>
> +/* Set/clear LEDs on dediprog */
> +#define PASS_ON		(0 << 0)
> +#define PASS_OFF	(1 << 0)
> +#define BUSY_ON		(0 << 1)
> +#define BUSY_OFF	(1 << 1)
> +#define ERROR_ON	(0 << 2)
> +#define ERROR_OFF	(1 << 2)
> +static int current_led_status = -1;
>
> +static int dediprog_set_leds(int leds)
> +{
> int ret, target_leds;
> if (leds == current_led_status)
> return 0;

Little nitpicky, but how about putting that test below the 'bogus value'
test? That way it'll handle all bogus values the same without sending
the same USB command again and again.
> if (leds < 0 || leds > 7)
> leds = 0; // Bogus value, enable all LEDs
> /* Older Dediprogs with 2.x.x and 3.x.x firmware only had
> * two LEDs, and they were reversed. So map them around if 
> * we have an old device. On those devices the LEDs map as
> * follows:
> *   bit 2 == 0: green light is on.
> *   bit 0 == 0: red light is on. 
> */
> if (dediprog_firmwareversion < FIRMWARE_VERSION(5,0,0)) {
> target_leds = ((leds & ERROR_OFF) >> 2) | 
> 	((leds & PASS_OFF) << 2);
> } else {
> target_leds = leds;
> }
> ret = usb_control_msg(dediprog_handle, 0x42, 0x07, 0x09, target_leds, NULL, 0x0, DEFAULT_TIMEOUT);
> if (ret != 0x0) {
> msg_perr("Command Set LED 0x%x failed (%s)!\n", leds, usb_strerror());
> return 1;
> }
> current_led_status = leds;
> return 0;
>
> +}
>
> static int dediprog_set_spi_voltage(int millivolt)
>  {
>    int ret;
> @@ -211,20 +256,26 @@
>    int residue = start % chunksize ? chunksize - start % chunksize : 0;
>    int bulklen;
> dediprog_set_leds(PASS_OFF|BUSY_ON|ERROR_OFF);
> if (residue) {
>   msg_pdbg("Slow read for partial block from 0x%x, length 0x%x\n",
>   	 start, residue);
>   ret = spi_read_chunked(flash, buf, start, residue, 16);
> if (ret)
> if (ret) {
> 	dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> return ret;
> }
>
> }
>
> /* Round down. */
> bulklen = (len - residue) / chunksize * chunksize;
> ret = dediprog_spi_bulk_read(flash, buf + residue, start + residue,
> 		     bulklen);
> if (ret)
>
> if (ret) {
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
>
> return ret;
>
> }
>
> len -= residue + bulklen;
> if (len) {
>
> @@ -232,17 +283,31 @@
>    		 start, len);
>    	ret = spi_read_chunked(flash, buf + residue + bulklen,
>    			       start + residue + bulklen, len, 16);
> if (ret)
> if (ret) {
> 	dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> return ret;
> }
>
> }
>
> dediprog_set_leds(PASS_ON|BUSY_OFF|ERROR_OFF);
> return 0;
>
> }
>
> int dediprog_spi_write_256(struct flashchip *flash, uint8_t *buf, int start, int len)
>  {
> int ret;
> dediprog_set_leds(PASS_OFF|BUSY_ON|ERROR_OFF);
> /* No idea about the real limit. Maybe 12, maybe more, maybe less. */
> return spi_write_chunked(flash, buf, start, len, 12);
> ret = spi_write_chunked(flash, buf, start, len, 12);
> if (ret)
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> else
> dediprog_set_leds(PASS_ON|BUSY_OFF|ERROR_OFF);
> return ret;
>
> }
>
> int dediprog_spi_send_command(unsigned int writecnt, unsigned int readcnt,
> @@ -318,7 +383,7 @@
>    		 fw[1], fw[2]);
>    	return 1;
>    }
> dediprog_firmwareversion = fw[0];
> dediprog_firmwareversion = (fw[0] << 16) | (fw[1] << 8) | fw[2];

How about using the FIRMWARE_VERSION() macro here, too?
> return 0;
>  }
>
> @@ -410,81 +475,8 @@
>    }
>    return 0;
>  }
>
> -/* Start/stop blinking?
> Present in eng_detect_blink.log with firmware 3.1.8
> Preceded by Command J
> */
>
> -static int dediprog_command_g(void)
> -{
> int ret;
> ret = usb_control_msg(dediprog_handle, 0x42, 0x07, 0x09, 0x03, NULL, 0x0, DEFAULT_TIMEOUT);
> if (ret != 0x0) {
> msg_perr("Command G failed (%s)!\n", usb_strerror());
> return 1;
> }
> return 0;
>
> -}
>
> -/* Something.
> Present in all logs with firmware 5.1.5
> Always preceded by Command Receive Device String
> Always followed by Command Set SPI Voltage nonzero
> */
>
> -static int dediprog_command_h(void)
> -{
> int ret;
> ret = usb_control_msg(dediprog_handle, 0x42, 0x07, 0x09, 0x05, NULL, 0x0, DEFAULT_TIMEOUT);
> if (ret != 0x0) {
> msg_perr("Command H failed (%s)!\n", usb_strerror());
> return 1;
> }
> return 0;
>
> -}
>  #endif
>
> -/* Shutdown for firmware 5.x?
> This command swithces the "Pass" LED on.
> Present in all logs with firmware 5.1.5
> Often preceded by a SPI operation (Command Read SPI Bulk or Receive SPI)
> Always followed by Command Set SPI Voltage 0x0000
> */
>
> -static int dediprog_command_i(void)
> -{
> int ret;
> ret = usb_control_msg(dediprog_handle, 0x42, 0x07, 0x09, 0x06, NULL, 0x0, DEFAULT_TIMEOUT);
> if (ret != 0x0) {
> msg_perr("Command I failed (%s)!\n", usb_strerror());
> return 1;
> }
> return 0;
>
> -}
>
> -#if 0
> -/* Start/stop blinking?
> Present in all logs with firmware 5.1.5
> Always preceded by Command Receive Device String on 5.1.5
> Always followed by Command Set SPI Voltage nonzero on 5.1.5
> Present in eng_detect_blink.log with firmware 3.1.8
> Preceded by Command B in eng_detect_blink.log
> Followed by Command G in eng_detect_blink.log
> */
>
> -static int dediprog_command_j(void)
> -{
> int ret;
> ret = usb_control_msg(dediprog_handle, 0x42, 0x07, 0x09, 0x07, NULL, 0x0, DEFAULT_TIMEOUT);
> if (ret != 0x0) {
> msg_perr("Command J failed (%s)!\n", usb_strerror());
> return 1;
> }
> return 0;
>
> -}
> -#endif
>
> static int parse_voltage(char *voltage)
>  {
>    char *tmp = NULL;
> @@ -584,22 +576,35 @@
>    	return 1;
>    }
>    dediprog_endpoint = 2;
> dediprog_set_leds(PASS_ON|BUSY_ON|ERROR_ON);
> /* URB 6. Command A. */
> if (dediprog_command_a())
> if (dediprog_command_a()) {
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> return 1;
> }
> /* URB 7. Command A. */
> if (dediprog_command_a())
> if (dediprog_command_a()) {
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> return 1;
> }
> /* URB 8. Command Prepare Receive Device String. */
> /* URB 9. Command Receive Device String. */
> if (dediprog_check_devicestring())
> if (dediprog_check_devicestring()) {
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> return 1;
> }
> /* URB 10. Command C. */
> if (dediprog_command_c())
> if (dediprog_command_c()) {
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> return 1;
> }
> /* URB 11. Command Set SPI Voltage. */
> if (dediprog_set_spi_voltage(millivolt))
>
> if (dediprog_set_spi_voltage(millivolt)) {
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
>
> return 1;
>
> }
>
> buses_supported = CHIP_BUSTYPE_SPI;
> spi_controller = SPI_CONTROLLER_DEDIPROG;
>
> @@ -610,6 +615,8 @@
>    dediprog_do_stuff();
>  #endif
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_OFF);
> return 0;
>
> }
>
> @@ -670,10 +677,6 @@
>  {
>    msg_pspew("%s\n", __func__);
> /* Shutdown on firmware 5.x */
> if (dediprog_firmwareversion == 5)
> if (dediprog_command_i())
> 	return 1;
> /* URB 28. Command Set SPI Voltage to 0. */
> if (dediprog_set_spi_voltage(0x0))
> return 1;

With the above changes:

Acked-by: Mathias Krause

[Stefan Reinauer]

* Mathias Krause  [110128 09:31]:
> > > Thanks for the patch. Albeit it would be good to have a "noleds" option
> > > to disable the functionality because we don't know if it breaks older
> > > firmware versions.
> >
> > All changes added, except this one. Instead the updated version
> > explicitly support older firmware versions.
>
> Thanks! Looks like a better solution to the problem. Albeit I found two
> (read: one and a half) more hunks to complain about ;)

Nice :-) Thanks for the review.
> Acked-by: Mathias Krause

Went in as r1257.