Re: [flashrom] [PATCH] dediprog: fix SPI clock setting.

[Nico Huber]

Hello Stefan,

Am 06.05.2013 19:30, schrieb Stefan Tauner:
> Avoid setting SPI speed on firmware versions < 5.0.0 and note this
> limitation in the man page.
> Use the index stored for a given speed in the translation array instead
> of the index of the element in that array.
>
> Signed-off-by: Patrick Georgi 
> Signed-off-by: Stefan Tauner
>
> IMHO the speed setting was broken ever since. Has anyone every checked
> the resulting frequency on the bus?

Frequency wasn't checked but with speed settings below 8M the time to
read a chip increased plausibly.
> I am not entirely sure why the translation table was introduced in the
> first place although dediprog's indices could be "stored" in the array
> offset, but mixing the two approaches is of course not a good idea.

The translation table was used to make it obvious that the speeds are
not ordered (see speed values for 12M and 8M). As the value range is
continuous, we could use the array index as value. But please, comment
on the interchanged speeds, when doing so.

Well, I see one flaw in the current code: The default value for
spispeed_idx in dediprog_init() (2 --> 8M) doesn't match the value
mentioned in the manpage (12M).
> Nico, can you please verify that this patch is what you actually meant to do?

Sorry, it is not. See below.

Regards,
Nico
> dediprog.c |   13 ++++++++-----
>  flashrom.8 |    3 ++-
>  2 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/dediprog.c b/dediprog.c
> index ab5388b..77ca195 100644
> --- a/dediprog.c
> +++ b/dediprog.c
> @@ -184,12 +184,15 @@ static const struct dediprog_spispeeds spispeeds[] = {
>   */
>  static int dediprog_set_spi_speed(unsigned int spispeed_idx)

The spispeed_idx parameter has to be an index into the spispeeds array.
> {
> int ret;
>
> if (dediprog_firmwareversion < FIRMWARE_VERSION(5, 0, 0)) {
> msg_pwarn("Skipping to set SPI speed because firmware is too old.\n");
> return 0;
>
> }
>
> msg_pdbg("SPI speed is %sHz\n", spispeeds[spispeed_idx].name);

This is why I used an index into the array: To print the name of the
selected speed.
> ret = usb_control_msg(dediprog_handle, 0x42, 0x61, spispeeds[spispeed_idx].speed, 0xff,
> 	      NULL, 0x0, DEFAULT_TIMEOUT);
> int ret = usb_control_msg(dediprog_handle, 0x42, 0x61, spispeeds[spispeed_idx].speed, 0xff,

spispeed_idx is always used as an index into the spispeeds array.
> 		  NULL, 0x0, DEFAULT_TIMEOUT);
> if (ret != 0x0) {
> msg_perr("Command Set SPI Speed 0x%x failed!\n", spispeeds[spispeed_idx].speed);

Here also.
> return 1;
>
> @@ -801,12 +804,12 @@ int dediprog_init(void)
>    if (spispeed) {
>    	for (i = 0; spispeeds[i].name; ++i) {
>    		if (!strcasecmp(spispeeds[i].name, spispeed)) {
> 		spispeed_idx = i;
> 		spispeed_idx = spispeeds[i].speed;

The value stored in this spispeed_idx variable is used to call
dediprog_set_spi_speed() later. i is the index into the spispeeds array,
so use that.
> 		break;
> 	}
> }
> if (!spispeeds[i].name) {
> 	msg_perr("Error: Invalid 'spispeed' value.\n");
> 	msg_perr("Error: Invalid spispeed value: '%s'.\n", spispeed);
> free(spispeed);
> return 1;
> }
>
> diff --git a/flashrom.8 b/flashrom.8
> index c7a6c69..4e6ab55 100644
> --- a/flashrom.8
> +++ b/flashrom.8
> @@ -683,7 +683,8 @@ Usage example to select the second device:
>  .sp
>  An optional
>  .B spispeed
> -parameter specifies the frequency of the SPI bus. Syntax is
> +parameter specifies the frequency of the SPI bus. The firmware on the device needs to be 5.0.0 or newer.
> +Syntax is
>  .sp
>  .B "  flashrom -p dediprog:spispeed=frequency"
>  .sp