Re: [flashrom] [PATCH] Enable spi clock setting in dediprog driver

[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