Re: [flashrom] [PATCH 3/3] serprog: add support for setting the SPI frequency

[Uwe Hermann]

On Thu, Sep 08, 2011 at 12:56:23AM +0200, Stefan Tauner wrote:
> +0x14	Set SPI clock frequency		16-bit requested frequency	ACK + 16-bit set frequency / NAK

^
                 "Set SPI clock frequency (in Hz)"
> 0x14 (S_SPI_SCK):

Please rename to S_SPI_FREQ (also all other places with 'SCK'). SCK is
often used to name a clock pin, this is unnecessarily confusing here.
> Set the SPI clock frequency.

^
                                         in Hz
> f_spi = extract_programmer_param("sck");

sck -> freq
> if (f_spi && strlen(f_spi)) {
> 	uint8_t buf[2];
> 	uint16_t tmp = atoi(f_spi);

Hm, maybe use strtol() later and check for errors, atoi() is not so
good for that.
> 	if (sp_check_commandavail(S_CMD_S_SPI_SCK) == 0)
> 		msg_pdbg(MSGHEADER "Setting SPI clock rate is "
> 			 "not supported!\n");
> 	else if (sp_docommand(S_CMD_S_SPI_SCK, 2, buf, 2, buf)
> 		 == 0) {
> 		tmp = buf[0] | (buf[1]<<8);

^^
                                               Missing spaces.
> 		msg_pdbg(MSGHEADER "Requested to set SPI clock "
> 			 "frequency to %s kHz. It was actually "
> 			 "set to %d kHz\n", f_spi, tmp);

kHz -> Hz

I would highly recommend to use Hz as the unit for setting the frequency
(in the API). The frontend (flashrom) can easily allow freq=1000,
or freq=1khz, or freq=1mhz etc. for user friendlyness, but the API
should maintain max. flexibility IMHO.

Uwe.

[PATCH 3/3] serprog: add support for setting the SPI frequency

[Stefan Tauner]

On Thu, 15 Sep 2011 22:14:11 +0200
Uwe Hermann  wrote:
> On Thu, Sep 08, 2011 at 12:56:23AM +0200, Stefan Tauner wrote:
> > if (f_spi && strlen(f_spi)) {
> > 	uint8_t buf[2];
> > 	uint16_t tmp = atoi(f_spi);
>
> Hm, maybe use strtol() later and check for errors, atoi() is not so
> good for that.

basically i agree, but since it was used all over the place i thought
it makes more sense to make it consistently wrong :)
i would like to postpone this to a later patch or change 2/3
accordingly.
> > 		msg_pdbg(MSGHEADER "Requested to set SPI clock "
> > 			 "frequency to %s kHz. It was actually "
> > 			 "set to %d kHz\n", f_spi, tmp);
>
> kHz -> Hz
>
> I would highly recommend to use Hz as the unit for setting the frequency
> (in the API). The frontend (flashrom) can easily allow freq=1000,
> or freq=1khz, or freq=1mhz etc. for user friendlyness, but the API
> should maintain max. flexibility IMHO.

Hz is an arbitrary unit. Although you get better resolution with
smaller steps, you do not get "maximum flexibility" when using Hz:
there are frequencies less than that (e.g. 1/10 Hz). The question
is what is useful in this use case and i think that is a kHz
resolution. Most programmers will have to round to some frequency due
to fixed PLL ratios etc. anyway, so it does not make much sense to have
such a high resolution. For communication integrity one kHz more or
less does not matter either. The only argument in favor of Hz that i
would acknowledge is that is the most basic unit for humans when they
are thinking of frequencies (although logically there is no such basic
unit as explained above). And that argument is pretty weak because
anyone familiar with Hz is probably able to "think" in kHz too. And for
usability kHz is even better due to less 0s :)

[PATCH] serprog: Add support for setting the SPI frequency.

[Stefan Tauner]

Introduce a new opcode (0x14) that sends the requested frequency as a 32b
long value in Hertz to the programmer and receives the frequency eventually
chosen by the programmer. The user can specify this with the programmer
parameter "spispeed" (named after the similar parameter for the buspirate)
including an optional suffix of 'M' or 'k' for specifying megahertz or kilohertz
respectively (lowercase suffixes are also accepted).

Signed-off-by: Stefan Tauner 
---
 Documentation/serprog-protocol.txt |   10 +++++++
 flashrom.8                         |   12 +++++++-
 serprog.c                          |   53 ++++++++++++++++++++++++++++++++++++
 3 files changed, 74 insertions(+), 1 deletions(-)

Hopefully this "Hz" version will get approval of the community ;)
I have also tweaked the input parsing and edited the manpage.

diff --git a/Documentation/serprog-protocol.txt b/Documentation/serprog-protocol.txt
index a3a4863..74ec36b 100644
--- a/Documentation/serprog-protocol.txt
+++ b/Documentation/serprog-protocol.txt
@@ -33,6 +33,7 @@ COMMAND	Description			Parameters			Return Value
 0x12	Set used bustype		8-bit flags (as with 0x05)	ACK / NAK
 0x13	Perform SPI operation		24-bit slen + 24-bit rlen	ACK + rlen bytes of data / NAK
    				 + slen bytes of data
+0x14	Set SPI clock frequency		32-bit requested frequency	ACK + 32-bit set frequency / NAK
 0x??	unimplemented command - invalid.

@@ -73,6 +74,14 @@ Additional information of the above commands:
    	Maximum slen is Q_WRNMAXLEN in case Q_BUSTYPE returns SPI only or S_BUSTYPE was used
    	to set SPI exclusively before. Same for rlen and Q_RDNMAXLEN.
    	This operation is immediate, meaning it doesnt use the operation buffer.
+	0x14 (S_SPI_FREQ):
+		Set the SPI clock frequency. The 32-bit value indicates the
+		requested frequency in Hertz. Value 0 is reserved and should
+		be NAKed by the programmer. The requested frequency should be
+		mapped by the programmer software to a supported frequency
+		lower than the one requested. If there is no lower frequency
+		available the lowest possible should be used. The value
+		chosen is sent back in the reply with an ACK.
    About mandatory commands:
    	The only truly mandatory commands for any device are 0x00, 0x01, 0x02 and 0x10,
    	but one can't really do anything with these commands.
@@ -107,3 +116,4 @@ This define listing should help C coders - (it's here to be the single source fo
 #define S_CMD_Q_RDNMAXLEN	0x11		/* Query read-n maximum length			*/
 #define S_CMD_S_BUSTYPE		0x12		/* Set used bustype(s).				*/
 #define S_CMD_O_SPIOP		0x13		/* Perform SPI operation.			*/
+#define S_CMD_S_SPI_FREQ	0x14		/* Set SPI clock frequency */
diff --git a/flashrom.8 b/flashrom.8
index 6b25b43..eabc3ea 100644
--- a/flashrom.8
+++ b/flashrom.8
@@ -477,7 +477,17 @@ syntax and for IP, you have to use
 .sp
 .B "  flashrom -p serprog:ip=ipaddr:port"
 .sp
-instead. More information about serprog is available in
+instead. In case the device supports it, you can set the SPI clock frequency
+with the optional
+.B spispeed
+parameter. The frequency is parsed as Hertz, unless an
+.BR M ", or " k
+suffix is given, then megahertz or kilohertz are used respectively.
+Example that sets the frequency to 2 MHz:
+.sp
+.B "flashrom -p serprog:dev=/dev/device:baud,spispeed=2M"
+.sp
+More information about serprog is available in
 .B serprog-protocol.txt
 in the source distribution.
 .TP
diff --git a/serprog.c b/serprog.c
index 65539a1..d783e51 100644
--- a/serprog.c
+++ b/serprog.c
@@ -69,6 +69,7 @@ static int serprog_shutdown(void *data);
 #define S_CMD_Q_RDNMAXLEN	0x11	/* Query read-n maximum length			*/
 #define S_CMD_S_BUSTYPE		0x12	/* Set used bustype(s).				*/
 #define S_CMD_O_SPIOP		0x13	/* Perform SPI operation.			*/
+#define S_CMD_S_SPI_FREQ	0x14	/* Set SPI clock frequency			*/

static uint16_t sp_device_serbuf_size = 16;
 static uint16_t sp_device_opbuf_size = 300;
@@ -460,6 +461,7 @@ int serprog_init(void)
    /* Check for the minimum operational set of commands. */
    if (serprog_buses_supported & BUS_SPI) {
    	uint8_t bt = BUS_SPI;
+		char *spispeed;
    	if (sp_check_commandavail(S_CMD_O_SPIOP) == 0) {
    		msg_perr("Error: SPI operation not supported while the "
    			 "bustype is SPI\n");
@@ -490,6 +492,57 @@ int serprog_init(void)
    		spi_programmer_serprog.max_data_read = v;
    		msg_pdbg(MSGHEADER "Maximum read-n length is %d\n", v);
    	}
+		spispeed = extract_programmer_param("spispeed");
+		if (spispeed && strlen(spispeed)) {
+			uint32_t f_spi_req, f_spi;
+			uint8_t buf[4];
+			char *f_spi_suffix;
+
+			errno = 0;
+			f_spi_req = strtol(spispeed, &f_spi_suffix, 0);
+			if (errno != 0 || spispeed == f_spi_suffix) {
+				msg_perr("Error: Could not convert "
+					 "'spispeed'.\n");
+				return 1;
+			}
+			if (strlen(f_spi_suffix) == 1) {
+				if (!strcasecmp(f_spi_suffix, "M"))
+					f_spi_req *= 1000000;
+				else if (!strcasecmp(f_spi_suffix, "k"))
+					f_spi_req *= 1000;
+				else {
+					msg_perr("Error: Garbage following "
+						 "'spispeed' value.\n");
+					return 1;
+				}
+			} else if (strlen(f_spi_suffix) > 1) {
+				msg_perr("Error: Garbage following "
+					 "'spispeed' value.\n");
+				return 1;
+			}
+
+			buf[0] = (f_spi_req >> (0 * 8)) & 0xFF;
+			buf[1] = (f_spi_req >> (1 * 8)) & 0xFF;
+			buf[2] = (f_spi_req >> (2 * 8)) & 0xFF;
+			buf[3] = (f_spi_req >> (3 * 8)) & 0xFF;
+
+			if (sp_check_commandavail(S_CMD_S_SPI_FREQ) == 0)
+				msg_pdbg(MSGHEADER "Setting the SPI clock rate "
+					 "is not supported!\n");
+			else if (sp_docommand(S_CMD_S_SPI_FREQ, 4, buf, 4, buf)
+				 == 0) {
+				f_spi = buf[0];
+				f_spi |= buf[1] << (1 * 8);
+				f_spi |= buf[2] << (2 * 8);
+				f_spi |= buf[3] << (3 * 8);
+				msg_pdbg(MSGHEADER "Requested to set SPI clock "
+					 "frequency to %u Hz. It was actually "
+					 "set to %u Hz\n", f_spi_req, f_spi);
+			} else
+				msg_pdbg(MSGHEADER "Setting SPI clock rate to "
+					 "%u Hz failed!\n", f_spi_req);
+		}
+		free(spispeed);
    	bt = serprog_buses_supported;
    	sp_docommand(S_CMD_S_BUSTYPE, 1, &bt, 0, NULL);
    }