Patchwork ft2232_spi.c: add frequency divider parameter.

login
register
about
Submitter Stefan Tauner
Date 2012-02-28 01:48:01
Message ID <1330393681-12334-1-git-send-email-stefan.tauner@student.tuwien.ac.at>
Download mbox | patch
Permalink /patch/3572/
State Accepted
Commit r1537
Headers show

Comments

Stefan Tauner - 2012-02-28 01:48:01
This adds an extra argument when using the ft2232_spi programmer to set
the frequency divider. The valid values for the divider is any even integer
between 2 and 131072.

Signed-off-by: Samir Ibradžić <sibradzic@gmail.com>
Signed-off-by: Stefan Tauner <stefan.tauner@student.tuwien.ac.at>

---
this was totally broken even without Samir's version of the patch applied.
it might be a better idea to let the user specify the actual frequency instead of
the divisor like some other programmers do (serprog and buspirate).
TODO: man page
---
 ft2232_spi.c |   54 +++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 37 insertions(+), 17 deletions(-)
Samir Ibradžić - 2012-02-28 02:50:37
On 2012年02月28日 10:48, Stefan Tauner wrote:
> This adds an extra argument when using the ft2232_spi programmer to set
> the frequency divider. The valid values for the divider is any even integer
> between 2 and 131072.
>
> Signed-off-by: Samir Ibradžić<sibradzic@gmail.com>
> Signed-off-by: Stefan Tauner<stefan.tauner@student.tuwien.ac.at>
>
> ---
> this was totally broken even without Samir's version of the patch applied.
> it might be a better idea to let the user specify the actual frequency instead of
> the divisor like some other programmers do (serprog and buspirate).

Well, it may be more clear to specify exact frequencies, but we may end 
up with weird set of frequencies for all these large dividers.
Maybe it would be better to support a limited set of "standard" dividers 
and their corresponding frequencies, like:

divider | freq
--------+--------
2       | 30MHz
4       | 15MHz
6       | 10MHz
8       | 7.5MHz
10      | 6MHz
12      | 5MHz
20      | 3MHz
30      | 2MHz
60      | 1MHz
80      | 750KHz
100     | 600KHz
120     | 500KHz
150     | 400KHz
200     | 300KHz
300     | 200KHz
600     | 100KHz
800     | 75KHz
1000    | 60KHz
1200    | 50KHz
1500    | 40KHz
2000    | 30KHz
3000    | 20KHz
6000    | 10KHz
8000    | 7.5KHz
(and so on)

But i vote for supporting specific divider after all, one can never know 
if there is some obscure device out there in the wild that supports only 
6365372Hz.

> TODO: man page
> ---
>   ft2232_spi.c |   54 +++++++++++++++++++++++++++++++++++++-----------------
>   1 files changed, 37 insertions(+), 17 deletions(-)
>
> diff --git a/ft2232_spi.c b/ft2232_spi.c
> index 122866f..7d598b1 100644
> --- a/ft2232_spi.c
> +++ b/ft2232_spi.c
> @@ -64,17 +64,8 @@ const struct usbdev_status devs_ft2232spi[] = {
>   	{},
>   };
>
> -/*
> - * The 'H' chips can run internally at either 12MHz or 60MHz.
> - * The non-H chips can only run at 12MHz.
> - */
> -static uint8_t clock_5x = 1;
>
> -/*
> - * In either case, the divisor is a simple integer clock divider.
> - * If clock_5x is set, this divisor divides 30MHz, else it divides 6MHz.
> - */
> -#define DIVIDE_BY 3  /* e.g. '3' will give either 10MHz or 2MHz SPI clock. */
> +#define DEFAULT_DIVISOR 2
>
>   #define BITMODE_BITBANG_NORMAL	1
>   #define BITMODE_BITBANG_SPI	2
> @@ -162,12 +153,28 @@ static const struct spi_programmer spi_programmer_ft2232 = {
>   /* Returns 0 upon success, a negative number upon errors. */
>   int ft2232_spi_init(void)
>   {
> -	int f, ret = 0;
> +	int ret = 0;
>   	struct ftdi_context *ftdic =&ftdic_context;
>   	unsigned char buf[512];
>   	int ft2232_vid = FTDI_VID;
>   	int ft2232_type = FTDI_FT4232H_PID;
>   	enum ftdi_interface ft2232_interface = INTERFACE_B;
> +	/*
> +	 * The 'H' chips can run with an internal clock of either 12 MHz or 60 MHz,
> +	 * but the non-H chips can only run at 12 MHz. We enable the divide-by-5
> +	 * prescaler on the former to run on the same speed.
> +	 */
> +	uint8_t clock_5x = 1;
> +	/* In addition to the prescaler mentioned above there is also another
> +	 * configurable one on all versions of the chips. Its divisor div can be
> +	 * set by a 16 bit value x according to the following formula:
> +	 * div = (1 + x) * 2<->  x = div / 2 - 1
> +	 * Hence the expressible divisors are all even numbers between 2 and
> +	 * 2^17 (=131072) resulting in SCK frequencies of 2 MHz down to about
> +	 * 92 Hz for 12 MHz inputs.
> +	 */
> +	uint32_t divisor = DEFAULT_DIVISOR;
> +	int f;
>   	char *arg;
>   	double mpsse_clk;
>
> @@ -243,6 +250,21 @@ int ft2232_spi_init(void)
>   		}
>   	}
>   	free(arg);
> +	arg = extract_programmer_param("divisor");
> +	if (arg&&  strlen(arg)) {
> +		unsigned int temp = 0;
> +		char *endptr;
> +		temp = strtoul(arg,&endptr, 10);
> +		if (*endptr || temp<  2 || temp>  131072 || temp&  0x1) {
> +			msg_perr("Error: Invalid SPI frequency divisor specified: \"%s\".\n"
> +				 "Valid are even values between 2 and 131072.\n", arg);
> +			free(arg);
> +			return -2;
> +		} else {
> +			divisor = (uint32_t)temp;
> +		}
> +	}
> +	free(arg);
>   	msg_pdbg("Using device type %s %s ",
>   		 get_ft2232_vendorname(ft2232_vid, ft2232_type),
>   		 get_ft2232_devicename(ft2232_vid, ft2232_type));
> @@ -303,17 +325,15 @@ int ft2232_spi_init(void)
>
>   	msg_pdbg("Set clock divisor\n");
>   	buf[0] = 0x86;		/* command "set divisor" */
> -	/* valueL/valueH are (desired_divisor - 1) */
> -	buf[1] = (DIVIDE_BY - 1)&  0xff;
> -	buf[2] = ((DIVIDE_BY - 1)>>  8)&  0xff;
>   	if (send_buf(ftdic, buf, 3)) {
>   		ret = -6;
>   		goto ftdi_err;
>   	}
> +	buf[1] = (divisor / 2 - 1)&  0xff;
> +	buf[2] = ((divisor / 2 - 1)>>  8)&  0xff;
>
> -	msg_pdbg("MPSSE clock: %f MHz divisor: %d "
> -		 "SPI clock: %f MHz\n", mpsse_clk, DIVIDE_BY,
> -		 (double)(mpsse_clk / (((DIVIDE_BY - 1) + 1) * 2)));
> +	msg_pdbg("MPSSE clock: %f MHz, divisor: %u, SPI clock: %f MHz\n",
> +		 mpsse_clk, divisor, (double)(mpsse_clk / divisor));
>
>   	/* Disconnect TDI/DO to TDO/DI for loopback. */
>   	msg_pdbg("No loopback of TDI/DO TDO/DI\n");
Stefan Tauner - 2012-03-06 15:25:40
On Tue, 28 Feb 2012 11:50:37 +0900
Samir Ibradžić <sibradzic@gmail.com> wrote:

> > it might be a better idea to let the user specify the actual frequency instead of
> > the divisor like some other programmers do (serprog and buspirate).  
> 
> Well, it may be more clear to specify exact frequencies, but we may end 
> up with weird set of frequencies for all these large dividers.
> Maybe it would be better to support a limited set of "standard" dividers 
> and their corresponding frequencies, like:
> 
> divider | freq
> --------+--------
> 2       | 30MHz
> 4       | 15MHz
> 6       | 10MHz
> […]
> (and so on)
> 
> But i vote for supporting specific divider after all, one can never know 
> if there is some obscure device out there in the wild that supports only 
> 6365372Hz.

well we could support both actually... selecting the divider as in our
patch, or alternatively the frequency (if it is not possible print the
next lower available one)...

anyway this can be done later and i'd like to get this into svn
now-ish rather than wait for the perfect patch. you do not happen to
own a logic analyzer that could confirm that the frequency now is
really what we think it is, do you?
in any case could you please test the patch i have sent so that we know
it does not break the programmer completely?

if you like to work on a followup patch please use "spispeed" as the
parameter for the frequency because that is the one used in serprog and
the buspirate already.
Samir Ibradžić - 2012-03-30 15:52:14
On 2012年03月07日 00:25, Stefan Tauner wrote:
> On Tue, 28 Feb 2012 11:50:37 +0900
> Samir Ibradžić<sibradzic@gmail.com>  wrote:
>
>>> it might be a better idea to let the user specify the actual frequency instead of
>>> the divisor like some other programmers do (serprog and buspirate).
>>
>> Well, it may be more clear to specify exact frequencies, but we may end
>> up with weird set of frequencies for all these large dividers.
>> Maybe it would be better to support a limited set of "standard" dividers
>> and their corresponding frequencies, like:
>>
>> divider | freq
>> --------+--------
>> 2       | 30MHz
>> 4       | 15MHz
>> 6       | 10MHz
>> […]
>> (and so on)
>>
>> But i vote for supporting specific divider after all, one can never know
>> if there is some obscure device out there in the wild that supports only
>> 6365372Hz.
>
> well we could support both actually... selecting the divider as in our
> patch, or alternatively the frequency (if it is not possible print the
> next lower available one)...
>
> anyway this can be done later and i'd like to get this into svn
> now-ish rather than wait for the perfect patch. you do not happen to
> own a logic analyzer that could confirm that the frequency now is
> really what we think it is, do you?

Sorry, no analyzer available here. I can confirm though, that when using 
bigger dividers, data transfer speeds achieved matches the calculated 
frequencies, at least on the device i have here (PicoTAP).

> in any case could you please test the patch i have sent so that we know
> it does not break the programmer completely?

Already tested, seems ok. Anyway, we just need to be careful that the 
patch has zero consequences when "spispeed" option is not used at all.

>
> if you like to work on a followup patch please use "spispeed" as the
> parameter for the frequency because that is the one used in serprog and
> the buspirate already.
>

Do you have any updates, or i can re-base on top of your previous patch?

R,
S
Stefan Tauner - 2012-03-30 22:05:19
On Sat, 31 Mar 2012 00:52:14 +0900
Samir Ibradžić <sibradzic@gmail.com> wrote:

> On 2012年03月07日 00:25, Stefan Tauner wrote:
> > On Tue, 28 Feb 2012 11:50:37 +0900
> > Samir Ibradžić<sibradzic@gmail.com>  wrote:

> >> But i vote for supporting specific divider after all, one can never know
> >> if there is some obscure device out there in the wild that supports only
> >> 6365372Hz.
> >
> > well we could support both actually... selecting the divider as in our
> > patch, or alternatively the frequency (if it is not possible print the
> > next lower available one)...
> >
> > anyway this can be done later and i'd like to get this into svn
> > now-ish rather than wait for the perfect patch. you do not happen to
> > own a logic analyzer that could confirm that the frequency now is
> > really what we think it is, do you?
> 
> Sorry, no analyzer available here. I can confirm though, that when using 
> bigger dividers, data transfer speeds achieved matches the calculated 
> frequencies, at least on the device i have here (PicoTAP).

by "matches" you mean that e.g. doubling the divisor approximately
doubles the time needed for an read/erase/write, right?

> > in any case could you please test the patch i have sent so that we know
> > it does not break the programmer completely?
> 
> Already tested, seems ok. Anyway, we just need to be careful that the 
> patch has zero consequences when "spispeed" option is not used at all.

thanks for testing.
yes (though i think you meant divisor not spispeed?), altough zero
consequences need to be specified more precisely:
the previous code set divisor 2 by sending 0 to the device. our new
code does the same by setting DEFAULT_DIVISOR to 2 and doing the math
right. so from the programmers point of view the communication should
be equal.

> >
> > if you like to work on a followup patch please use "spispeed" as the
> > parameter for the frequency because that is the one used in serprog and
> > the buspirate already.
> >
> 
> Do you have any updates, or i can re-base on top of your previous patch?

no updates yet, but i will change this bit because it does seem like a
typo:
 	 * Hence the expressible divisors are all even numbers between 2 and
-	 * 2^17 (=131072) resulting in SCK frequencies of 2 MHz down to about
+	 * 2^17 (=131072) resulting in SCK frequencies of 6 MHz down to about
 	 * 92 Hz for 12 MHz inputs.
 	 */

it would be nice if someone could really verify the correct
frequencies on the output pins, but if no one will do that in the next
days/weeks, i will commit it anyway.
Stefan Tauner - 2012-05-15 22:59:44
On Sat, 31 Mar 2012 00:05:19 +0200
Stefan Tauner <stefan.tauner@student.tuwien.ac.at> wrote:

> […]
>
> it would be nice if someone could really verify the correct
> frequencies on the output pins, but if no one will do that in the next
> days/weeks, i will commit it anyway.

committed in r1537 after adding a section to the manpage explaining the
new parameter.

Patch

diff --git a/ft2232_spi.c b/ft2232_spi.c
index 122866f..7d598b1 100644
--- a/ft2232_spi.c
+++ b/ft2232_spi.c
@@ -64,17 +64,8 @@  const struct usbdev_status devs_ft2232spi[] = {
 	{},
 };
 
-/*
- * The 'H' chips can run internally at either 12MHz or 60MHz.
- * The non-H chips can only run at 12MHz.
- */
-static uint8_t clock_5x = 1;
 
-/*
- * In either case, the divisor is a simple integer clock divider.
- * If clock_5x is set, this divisor divides 30MHz, else it divides 6MHz.
- */
-#define DIVIDE_BY 3  /* e.g. '3' will give either 10MHz or 2MHz SPI clock. */
+#define DEFAULT_DIVISOR 2
 
 #define BITMODE_BITBANG_NORMAL	1
 #define BITMODE_BITBANG_SPI	2
@@ -162,12 +153,28 @@  static const struct spi_programmer spi_programmer_ft2232 = {
 /* Returns 0 upon success, a negative number upon errors. */
 int ft2232_spi_init(void)
 {
-	int f, ret = 0;
+	int ret = 0;
 	struct ftdi_context *ftdic = &ftdic_context;
 	unsigned char buf[512];
 	int ft2232_vid = FTDI_VID;
 	int ft2232_type = FTDI_FT4232H_PID;
 	enum ftdi_interface ft2232_interface = INTERFACE_B;
+	/*
+	 * The 'H' chips can run with an internal clock of either 12 MHz or 60 MHz,
+	 * but the non-H chips can only run at 12 MHz. We enable the divide-by-5
+	 * prescaler on the former to run on the same speed.
+	 */
+	uint8_t clock_5x = 1;
+	/* In addition to the prescaler mentioned above there is also another
+	 * configurable one on all versions of the chips. Its divisor div can be
+	 * set by a 16 bit value x according to the following formula:
+	 * div = (1 + x) * 2 <-> x = div / 2 - 1
+	 * Hence the expressible divisors are all even numbers between 2 and
+	 * 2^17 (=131072) resulting in SCK frequencies of 2 MHz down to about
+	 * 92 Hz for 12 MHz inputs.
+	 */
+	uint32_t divisor = DEFAULT_DIVISOR;
+	int f;
 	char *arg;
 	double mpsse_clk;
 
@@ -243,6 +250,21 @@  int ft2232_spi_init(void)
 		}
 	}
 	free(arg);
+	arg = extract_programmer_param("divisor");
+	if (arg && strlen(arg)) {
+		unsigned int temp = 0;
+		char *endptr;
+		temp = strtoul(arg, &endptr, 10);
+		if (*endptr || temp < 2 || temp > 131072 || temp & 0x1) {
+			msg_perr("Error: Invalid SPI frequency divisor specified: \"%s\".\n"
+				 "Valid are even values between 2 and 131072.\n", arg);
+			free(arg);
+			return -2;
+		} else {
+			divisor = (uint32_t)temp;
+		}
+	}
+	free(arg);
 	msg_pdbg("Using device type %s %s ",
 		 get_ft2232_vendorname(ft2232_vid, ft2232_type),
 		 get_ft2232_devicename(ft2232_vid, ft2232_type));
@@ -303,17 +325,15 @@  int ft2232_spi_init(void)
 
 	msg_pdbg("Set clock divisor\n");
 	buf[0] = 0x86;		/* command "set divisor" */
-	/* valueL/valueH are (desired_divisor - 1) */
-	buf[1] = (DIVIDE_BY - 1) & 0xff;
-	buf[2] = ((DIVIDE_BY - 1) >> 8) & 0xff;
 	if (send_buf(ftdic, buf, 3)) {
 		ret = -6;
 		goto ftdi_err;
 	}
+	buf[1] = (divisor / 2 - 1) & 0xff;
+	buf[2] = ((divisor / 2 - 1) >> 8) & 0xff;
 
-	msg_pdbg("MPSSE clock: %f MHz divisor: %d "
-		 "SPI clock: %f MHz\n", mpsse_clk, DIVIDE_BY,
-		 (double)(mpsse_clk / (((DIVIDE_BY - 1) + 1) * 2)));
+	msg_pdbg("MPSSE clock: %f MHz, divisor: %u, SPI clock: %f MHz\n",
+		 mpsse_clk, divisor, (double)(mpsse_clk / divisor));
 
 	/* Disconnect TDI/DO to TDO/DI for loopback. */
 	msg_pdbg("No loopback of TDI/DO TDO/DI\n");