[PATCH] Enable spi clock setting in dediprog driver

[Nico Huber]

This adds a programmer parameter 'speed' in the dediprog driver to
controll the transfer rate on the spi bus. The following rates are
available (all in kHz):
  24000, 12000, 8000, 3000, 2180, 1500, 750, 375

Signed-off-by: Nico Huber

Index: dediprog.c
===================================================================
--- dediprog.c	(Revision 1541)
+++ dediprog.c	(Arbeitskopie)
@@ -135,7 +144,6 @@
    return 0;
 }

-#if 0
 /* After dediprog_set_spi_speed, the original app always calls
  * dediprog_set_spi_voltage(0) and then
  * dediprog_check_devicestring() four times in a row.
@@ -149,43 +157,43 @@
  * This looks suspiciously like the microprocessor in the SF100 has to be
  * restarted/reinitialized in case the speed changes.
  */
-static int dediprog_set_spi_speed(uint16_t speed)
+static int dediprog_set_spi_speed(unsigned int khz)
 {
    int ret;
-	unsigned int khz;
+	uint16_t speed;

/* Case 1 and 2 are in weird order. Probably an organically "grown"
     * interface.
     * Base frequency is 24000 kHz, divisors are (in order)
     * 1, 3, 2, 8, 11, 16, 32, 64.
     */
-	switch (speed) {
-	case 0x0:
-		khz = 24000;
+	switch (khz) {
+	case 24000:
+		speed = 0;
    	break;
-	case 0x1:
-		khz = 8000;
+	case 8000:
+		khz = 1;
    	break;
-	case 0x2:
-		khz = 12000;
+	case 12000:
+		khz = 2;
    	break;
-	case 0x3:
-		khz = 3000;
+	case 3000:
+		khz = 3;
    	break;
-	case 0x4:
-		khz = 2180;
+	case 2180:
+		khz = 4;
    	break;
-	case 0x5:
-		khz = 1500;
+	case 1500:
+		khz = 5;
    	break;
-	case 0x6:
-		khz = 750;
+	case 750:
+		khz = 6;
    	break;
-	case 0x7:
-		khz = 375;
+	case 375:
+		khz = 7;
    	break;
    default:
-		msg_perr("Unknown frequency selector 0x%x! Aborting.\n", speed);
+		msg_perr("Unsupported frequency %d kHz! Aborting.\n", khz);
    	return 1;
    }
    msg_pdbg("Setting SPI speed to %u kHz\n", khz);
@@ -198,7 +206,6 @@
    }
    return 0;
 }
-#endif

/* Bulk read interface, will read multiple 512 byte chunks aligned to 512 bytes.
  * @start	start address
@@ -701,6 +708,28 @@
    return millivolt;
 }

+static int dediprog_setup(void)
+{
+	/* URB 6. Command A. */
+	if (dediprog_command_a()) {
+		return 1;
+	}
+	/* URB 7. Command A. */
+	if (dediprog_command_a()) {
+		return 1;
+	}
+	/* URB 8. Command Prepare Receive Device String. */
+	/* URB 9. Command Receive Device String. */
+	if (dediprog_check_devicestring()) {
+		return 1;
+	}
+	/* URB 10. Command C. */
+	if (dediprog_command_c()) {
+		return 1;
+	}
+	return 0;
+}
+
 static const struct spi_programmer spi_programmer_dediprog = {
    .type		= SPI_CONTROLLER_DEDIPROG,
    .max_data_read	= MAX_DATA_UNSPECIFIED,
@@ -741,12 +770,18 @@
 int dediprog_init(void)
 {
    struct usb_device *dev;
-	char *voltage;
-	int millivolt = 3500;
+	char *voltage, *speed;
+	int khz = 0, millivolt = 3500;
    int ret;

msg_pspew("%s\n", __func__);

+	speed = extract_programmer_param("speed");
+	if (speed) {
+		khz = strtol(speed, NULL, 0);
+		free(speed);
+		msg_pinfo("Setting speed to %i kHz\n", khz);
+	}
    voltage = extract_programmer_param("voltage");
    if (voltage) {
    	millivolt = parse_voltage(voltage);
@@ -791,27 +843,25 @@

dediprog_set_leds(PASS_ON|BUSY_ON|ERROR_ON);

-	/* URB 6. Command A. */
-	if (dediprog_command_a()) {
+	/* Perform basic setup. */
+	if (dediprog_setup()) {
    	dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
    	return 1;
    }
-	/* URB 7. Command A. */
-	if (dediprog_command_a()) {
-		dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
-		return 1;
+
+	/*
+	 * Set speed if requested. Set voltage to zero beforehand and setup
+	 * again afterwards. Maybe we can skip the first setup.
+	 */
+	if (khz) {
+		if (dediprog_set_spi_voltage(0) ||
+				dediprog_set_spi_speed(khz) ||
+				dediprog_setup()) {
+			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()) {
-		dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
-		return 1;
-	}
-	/* URB 10. 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)) {
    	dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);

[Carl-Daniel Hailfinger]

Hi Nico,

thanks for your patch. Review follows.

Am 13.06.2012 11:01 schrieb Nico Huber:
> This adds a programmer parameter 'speed' in the dediprog driver to
> controll the transfer rate on the spi bus. The following rates are
> available (all in kHz):
>   24000, 12000, 8000, 3000, 2180, 1500, 750, 375
>
> Signed-off-by: Nico Huber

Sorry, this patch does not work as intended.

Short comment about coding style in advance: We recently switched to a
112 column limit (instead of 80 columns), and if you're changing
multiline command anyway, it would be nice if you could reformat it to
use 112 columns. Thanks! (I could do the reformatting on the fly before
commit, but that breaks patch tracking.)
> Index: dediprog.c
>
> --- dediprog.c	(Revision 1541)
> +++ dediprog.c	(Arbeitskopie)
> @@ -135,7 +144,6 @@
>    return 0;
>  }
>
> -#if 0
>  /* After dediprog_set_spi_speed, the original app always calls
> dediprog_set_spi_voltage(0) and then
> dediprog_check_devicestring() four times in a row.
>
> @@ -149,43 +157,43 @@
> This looks suspiciously like the microprocessor in the SF100 has to be
> restarted/reinitialized in case the speed changes.
>
> */
> -static int dediprog_set_spi_speed(uint16_t speed)
> +static int dediprog_set_spi_speed(unsigned int khz)
>  {
>    int ret;
> unsigned int khz;
>
> uint16_t speed;
>
> /* Case 1 and 2 are in weird order. Probably an organically "grown"
> interface.
> Base frequency is 24000 kHz, divisors are (in order)
> 1, 3, 2, 8, 11, 16, 32, 64.
>
> */
> switch (speed) {
> case 0x0:
> khz = 24000;
> switch (khz) {
> case 24000:
> speed = 0;
> break;
> case 0x1:
> khz = 8000;
> case 8000:
> khz = 1;

This looks like an incomplete search/replace operation. I think you
wanted to set speed instead of khz in all cases below.
speed = 1;
> break;
> case 0x2:
> khz = 12000;
> case 12000:
> khz = 2;

dito.
> break;
> case 0x3:
> khz = 3000;
> case 3000:
> khz = 3;
> break;
> case 0x4:
> khz = 2180;
> case 2180:
> khz = 4;
> break;
> case 0x5:
> khz = 1500;
> case 1500:
> khz = 5;
> break;
> case 0x6:
> khz = 750;
> case 750:
> khz = 6;
> break;
> case 0x7:
> khz = 375;
> case 375:
> khz = 7;
> break;
> default:
> msg_perr("Unknown frequency selector 0x%x! Aborting.\n", speed);
> msg_perr("Unsupported frequency %d kHz! Aborting.\n", khz);
> return 1;
> }
> msg_pdbg("Setting SPI speed to %u kHz\n", khz);
>
> @@ -198,7 +206,6 @@
>    }
>    return 0;
>  }
> -#endif
>
> /* Bulk read interface, will read multiple 512 byte chunks aligned to 512 bytes.
> @start	start address
>
> @@ -701,6 +708,28 @@
>    return millivolt;
>  }
>
> +static int dediprog_setup(void)
> +{
> /* URB 6. Command A. */
> if (dediprog_command_a()) {
> return 1;
> }
> /* URB 7. Command A. */
> if (dediprog_command_a()) {
> return 1;
> }
> /* URB 8. Command Prepare Receive Device String. */
> /* URB 9. Command Receive Device String. */
> if (dediprog_check_devicestring()) {
> return 1;
> }
> /* URB 10. Command C. */
> if (dediprog_command_c()) {
> return 1;
> }
> return 0;
>
> +}
>
> static const struct spi_programmer spi_programmer_dediprog = {
>    .type		= SPI_CONTROLLER_DEDIPROG,
>    .max_data_read	= MAX_DATA_UNSPECIFIED,
> @@ -741,12 +770,18 @@
>  int dediprog_init(void)
>  {
>    struct usb_device *dev;
> char *voltage;
> int millivolt = 3500;
> char *voltage, *speed;
> int khz = 0, millivolt = 3500;

dediprog_set_spi_speed() expects an unsigned int, please use that for
khz here as well.
> int ret;
>
> msg_pspew("%s\n", __func__);
> speed = extract_programmer_param("speed");
> if (speed) {
> khz = strtol(speed, NULL, 0);

We want a sensible base, and I can't envision anybody wanting base 8 or
base 16 here.
strtoul(speed, NULL, 10);
That said, the Bus Pirate driver offers SPI speed selection as well, but
the interface differs a bit ("spispeed" instead of "speed", specifying
frequency e.g as "8M" instead of "8000"). It would be nice if we had a
consistent interface for this feature.
> free(speed);
> msg_pinfo("Setting speed to %i kHz\n", khz);
> }
> voltage = extract_programmer_param("voltage");
> if (voltage) {
> millivolt = parse_voltage(voltage);
>
> @@ -791,27 +843,25 @@
>
> dediprog_set_leds(PASS_ON|BUSY_ON|ERROR_ON);
> /* URB 6. Command A. */
> if (dediprog_command_a()) {
> /* Perform basic setup. */
> if (dediprog_setup()) {
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> return 1;
> }
> /* URB 7. Command A. */
> if (dediprog_command_a()) {
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> return 1;
> /*
> * Set speed if requested. Set voltage to zero beforehand and setup
> * again afterwards. Maybe we can skip the first setup.
> */

This code change was not obvious from the patch description. I don't
object to the change itself, but mentioning the possible double init in
the commit message would be nice.
Another question is whether selecting the speed in one flashrom run will
have any impact on subsequent flashrom runs. If yes, we should probably
always set the SPI frequency.
> if (khz) {
> if (dediprog_set_spi_voltage(0) ||
> 		dediprog_set_spi_speed(khz) ||
> 		dediprog_setup()) {
> 	dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> 	return 1;

The whole init sequence would probably benefit from a common failure
path to which you'd jump with goto after setting the return code. That's
not your fault, but your change makes this even more painfully visible.
I can fix this in a followup after your patches are in (unless you want
to do that restructuring yourself, in which case it would be nice to
have that as a self-contained patch.)
> }
> }
> /* URB 8. Command Prepare Receive Device String. */
> /* URB 9. Command Receive Device String. */
> if (dediprog_check_devicestring()) {
> dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);
> return 1;
> }
> /* URB 10. 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)) {
>   dediprog_set_leds(PASS_OFF|BUSY_OFF|ERROR_ON);

New programmer parameters need a man page entry. You could simply clone
the spispeed entry of the Bus Pirate section.

Regards,
Carl-Daniel

[Nico Huber]

Hello Carl-Daniel,

thank you for your review.

Am 13.06.2012 23:22, schrieb Carl-Daniel Hailfinger:
> Hi Nico,
>
> thanks for your patch. Review follows.
>
> Am 13.06.2012 11:01 schrieb Nico Huber:
> > This adds a programmer parameter 'speed' in the dediprog driver to
> > controll the transfer rate on the spi bus. The following rates are
> > available (all in kHz):
> >   24000, 12000, 8000, 3000, 2180, 1500, 750, 375
> >
> > Signed-off-by: Nico Huber
>
> Sorry, this patch does not work as intended.

You are right. I can't believe I sent this.
> > /* Case 1 and 2 are in weird order. Probably an organically "grown"
> >     * interface.
> >     * Base frequency is 24000 kHz, divisors are (in order)
> >     * 1, 3, 2, 8, 11, 16, 32, 64.
> >     */
> > switch (speed) {
> > case 0x0:
> > khz = 24000;
> > switch (khz) {
> > case 24000:
> > speed = 0;
> > break;
> > case 0x1:
> > khz = 8000;
> > case 8000:
> > khz = 1;
>
> This looks like an incomplete search/replace operation. I think you
> wanted to set speed instead of khz in all cases below.
> speed = 1;

Admitted. Thank you for pointing that out.
> > speed = extract_programmer_param("speed");
> > if (speed) {
> > khz = strtol(speed, NULL, 0);
>
> We want a sensible base, and I can't envision anybody wanting base 8 or
> base 16 here.
> strtoul(speed, NULL, 10);
> That said, the Bus Pirate driver offers SPI speed selection as well, but
> the interface differs a bit ("spispeed" instead of "speed", specifying
> frequency e.g as "8M" instead of "8000"). It would be nice if we had a
> consistent interface for this feature.

I hadn't looked at the other programmer drivers. I'll unify this.
How about if I also extract some common code? like:

struct param_value_lut {
    const char *const	name;
    const int		value;
};
int lookup_programmer_param_value(const char *const param_name,
    			  const struct param_value_lut *const lut);

which looks up an parameter value for a string (like 8M => 0x7 in
buspirate).
> Another question is whether selecting the speed in one flashrom run will
> have any impact on subsequent flashrom runs. If yes, we should probably
> always set the SPI frequency.

Yes it will affect subsequent runs. I thought it would be better to make
this optional, as I can only test it with one device and can't assure
that it works with other hardware revisions/firmware versions.

Best,
Nico