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

[PATCH] dediprog: fix SPI clock setting.

[Stefan Tauner]

On Tue, 07 May 2013 10:37:57 +0200
Nico Huber  wrote:
> > 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).

That's exactly why I started looking at it. I even had a patch that
changed the default frequency in the manpage already :)
No idea how I cam up with that stupid change... thanks for catching
that.

Obviously I got quite confused by the 3 ways of communicating the
requested SPI frequency (the string, the index of the array, the
integer that is stored in the array and gets transferred to the
programmer). I think that is kinda awkward although I see the point of
course.

I will leave it alone anyway since it is there already and we don't
know what Dediprog might bring us in the future ;)
I will however change the default spispeed_idx from 2 to 1 matching the
manpage and the default of Dediprog's CLI tool.