Patchwork PONY_SPI, now with Windows support

login
register
about
Submitter Virgil-Adrian Teaca
Date 2012-04-29 16:35:07
Message ID <CAOfppk746ZNEg93b1AbwMKF+zrNDCAoO2ESnpTkpciaw6g-wSg@mail.gmail.com>
Download mbox | patch
Permalink /patch/3613/
State Superseded
Headers show

Comments

Virgil-Adrian Teaca - 2012-04-29 16:35:07
Hello,

This PONY_SPI patch have also the Windows support, but, to note that this
new code is untested.

All the best,
Virgil.
Signed-off-by: Virgil-Adrian Teaca <darkstarlinux@gmail.com>
Michael Karcher - 2012-04-29 21:01:57
On Sun, 2012-04-29 at 18:35 +0200, Virgil-Adrian Teaca wrote:

> This PONY_SPI patch have also the Windows support, but, to note that
> this new code is untested.
Thank you very much for sharing your work!

It might make sense to put the serial port pin manipulation functions
into serial.c instead of pony_spi.c (of course they can't be static
then, should be named sp_xxx instead of pony_xxx and the PIN_xxx
enumeration should be named "SERIAL_PIN_xxx")
Comments from other developers?

I will start with some coding style issues, but please see the code
comments below, too.

I can't provide you any indent command line for flashrom, but we
generally don't use spaces on the inner sider of parentheses. So inside
flashrom:
WRONG: ioctl( sp_fd, TIOCMGET, &ctl );
RIGHT: ioctl(sp_fd, TICMGET, &ctl);
On the other hand, we *do* write a space before the opening parenthesis,
but only after keywords (if, while, for), not after function names, so
for code in flashrom:
WRONG: if( pin == PIN_TXD )
RIGHT: if (pin == PIN_TXD)

Furthermore, I noticed you are using a lot of blank lines. I appreciate
structuring your code by inserting blank lines between logical blocks to
make it more readable, but having every second line blank (like you do
in the programmer detection loop for example) does not really add any
value over not having blank lines. In that specific example, the only
blank line I might leave in is the one before the "if" block.


> @@ -122,6 +122,11 @@
>  else
>  override CONFIG_RAYER_SPI = no
>  endif
> +ifeq ($(CONFIG_PONY_SPI), yes)
> +UNSUPPORTED_FEATURES += CONFIG_PONY_SPI=yes
> +else
> +override CONFIG_PONY_SPI = no
> +endif
>  ifeq ($(CONFIG_NIC3COM), yes)
>  UNSUPPORTED_FEATURES += CONFIG_NIC3COM=yes
>  else
> @@ -246,6 +251,11 @@
>  else
>  override CONFIG_RAYER_SPI = no
>  endif
> +ifeq ($(CONFIG_PONY_SPI), yes)
> +UNSUPPORTED_FEATURES += CONFIG_PONY_SPI=yes
> +else
> +override CONFIG_PONY_SPI = no
> +endif
>  ifeq ($(CONFIG_ATAHPT), yes)
>  UNSUPPORTED_FEATURES += CONFIG_ATAHPT=yes
>  else
Drop these two hunks. They are for programmers that directly access
hardware through processor I/O cycles. You don't do that, as you use the
OS driver for the serial port instead.


> +ifeq ($(CONFIG_PONY_SPI), yes)
> +FEATURE_CFLAGS += -D'CONFIG_PONY_SPI=1'
> +PROGRAMMER_OBJS += pony_spi.o
> +endif
Add a "NEED_SERIAL := yes" in the if block, otherwise serial.c might not
get included.


> +#include <stdlib.h>
> +#include <string.h>

> +#include <termios.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +#include <sys/ioctl.h>
The last six headers are not needed on Windows, and some of them don't
even exist. Wrap these includes by "#ifndef _WIN32"

> +#ifdef _WIN32
> +#include <conio.h>
> +#endif
That include is useless, drop it.

> +enum pony_pin {
> +       PIN_CD = 0,
> +       PIN_RXD,
> +       PIN_TXD,
> +       PIN_DTR,
> +       PIN_GND,
> +       PIN_DSR,
> +       PIN_RTS,
> +       PIN_CTS,
> +       PIN_RI,
> +};
I wonder whether it makes sense to define all 9 pins of the socket. For
example: What are you intending to use "PIN_GND" for? In the case, you
just define them to have all pins defined, I would suggest to start with
PIN_CD at 1, so the enumerators match the conventional pin numbers.

> +static void pony_set_pin( int pin, int val ) {
Why don't you use "enum pony_pin" as type for the pin?

> +static int pony_get_pin( int pin ) {
dito.

One might even think about using two different enums, one for input
pins, one for output pins.

> +static void pony_bitbang_set_cs( int val )
> +{
> +       if( pony_type == TYPE_SI_PROG )
> +       {
> +               /* CS# is negated by the SI-Prog programmer. */
> +               val =  val ? 0 : 1;
please write this as "val = !val;" or "val ^= 1;"
> +       }
> +
> +       pony_set_pin( PIN_TXD, val );
> +}


> +static int pony_bitbang_get_miso(void)
> +{
> +       int tmp;
> +
> +       tmp = pony_get_pin( PIN_CTS );
> +
> +       if( pony_type == TYPE_SERBANG )
> +       {
> +               /* MISO is negated by the SERBANG programmer. */
> +               tmp = tmp ? 0 : 1;
dito, use "tmp = !tmp;" here, or do it like

	/* MISO is negated by the SERBANG programmer, */
	if (pony_type == TYPE_SERBANG)
		return !tmp;
	else
		return tmp;

> +       /* Register the shutdown callback. */
> +
> +       if ( register_shutdown( serialport_shutdown, NULL ) ) {
> +               return 1;
> +       }

If you would register the shutdown function directly after opening the
port, you would not have to call serialport_shutdown(NULL) yourself in
the error cases before this line.

> +
> +       /* Get the initial value before writing to any line. */
> +
Remove this comment. It doesn't apply.

> +       if ( bitbang_spi_init( &bitbang_spi_master_pony ) ) {
> +               return 1;
> +       }
> +
> +       return 0;
> +}


> diff -urN flashrom.orig/print.c flashrom/print.c
> --- flashrom.orig/print.c       2012-03-13 16:52:50.000000000 +0100
> +++ flashrom/print.c    2012-04-24 00:50:46.329483950 +0200
> @@ -507,6 +507,12 @@
>         /* FIXME */
>         msg_ginfo("RayeR parallel port programmer\n");
>  #endif
> +#if CONFIG_PONY_SPI == 1
> +       msg_ginfo("\nSupported devices for the %s programmer:\n",
> +              programmer_table[PROGRAMMER_PONY_SPI].name);
> +       /* FIXME */
> +       msg_ginfo("SI-Prog serial port programmer\n");
Add serbang here, too.

Regards,
  Michael Karcher

Patch

diff -urN flashrom.orig/Makefile flashrom/Makefile
--- flashrom.orig/Makefile	2012-04-20 12:17:31.000000000 +0200
+++ flashrom/Makefile	2012-04-20 21:46:07.000000000 +0200
@@ -122,6 +122,11 @@ 
 else
 override CONFIG_RAYER_SPI = no
 endif
+ifeq ($(CONFIG_PONY_SPI), yes)
+UNSUPPORTED_FEATURES += CONFIG_PONY_SPI=yes
+else
+override CONFIG_PONY_SPI = no
+endif
 ifeq ($(CONFIG_NIC3COM), yes)
 UNSUPPORTED_FEATURES += CONFIG_NIC3COM=yes
 else
@@ -246,6 +251,11 @@ 
 else
 override CONFIG_RAYER_SPI = no
 endif
+ifeq ($(CONFIG_PONY_SPI), yes)
+UNSUPPORTED_FEATURES += CONFIG_PONY_SPI=yes
+else
+override CONFIG_PONY_SPI = no
+endif
 ifeq ($(CONFIG_ATAHPT), yes)
 UNSUPPORTED_FEATURES += CONFIG_ATAHPT=yes
 else
@@ -292,6 +302,9 @@ 
 # RayeR SPIPGM hardware support
 CONFIG_RAYER_SPI ?= yes
 
+# PonyProg2000 SPI hardware support
+CONFIG_PONY_SPI ?= yes
+
 # Always enable 3Com NICs for now.
 CONFIG_NIC3COM ?= yes
 
@@ -348,6 +361,9 @@ 
 ifeq ($(CONFIG_RAYER_SPI), yes)
 override CONFIG_BITBANG_SPI = yes
 else
+ifeq ($(CONFIG_PONY_SPI), yes)
+override CONFIG_BITBANG_SPI = yes
+else
 ifeq ($(CONFIG_INTERNAL), yes)
 override CONFIG_BITBANG_SPI = yes
 else
@@ -362,6 +378,7 @@ 
 endif
 endif
 endif
+endif
 
 ifeq ($(CONFIG_INTERNAL), yes)
 FEATURE_CFLAGS += -D'CONFIG_INTERNAL=1'
@@ -388,6 +405,11 @@ 
 NEED_PCI := yes
 endif
 
+ifeq ($(CONFIG_PONY_SPI), yes)
+FEATURE_CFLAGS += -D'CONFIG_PONY_SPI=1'
+PROGRAMMER_OBJS += pony_spi.o
+endif
+
 ifeq ($(CONFIG_BITBANG_SPI), yes)
 FEATURE_CFLAGS += -D'CONFIG_BITBANG_SPI=1'
 PROGRAMMER_OBJS += bitbang_spi.o
diff -urN flashrom.orig/flashrom.c flashrom/flashrom.c
--- flashrom.orig/flashrom.c	2012-02-26 11:12:57.000000000 +0100
+++ flashrom/flashrom.c	2012-04-20 18:07:32.000000000 +0200
@@ -201,6 +201,16 @@ 
 	},
 #endif
 
+#if CONFIG_PONY_SPI == 1
+	{
+		.name			= "pony_spi",
+		.init			= pony_spi_init,
+		.map_flash_region	= fallback_map,
+		.unmap_flash_region	= fallback_unmap,
+		.delay			= internal_delay,
+},
+#endif
+
 #if CONFIG_NICINTEL == 1
 	{
 		.name			= "nicintel",
diff -urN flashrom.orig/pony_spi.c flashrom/pony_spi.c
--- flashrom.orig/pony_spi.c	1970-01-01 01:00:00.000000000 +0100
+++ flashrom/pony_spi.c	2012-04-29 18:30:17.325597548 +0200
@@ -0,0 +1,286 @@ 
+/*
+ * This file is part of the flashrom project.
+ *
+ * Copyright (C) 2012 Virgil-Adrian Teaca
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
+ */
+
+/* Driver for the SI-Prog like hardware by Lancos.com.
+ * See http://www.lancos.com/siprogsch.html for schematics and instructions.
+ */
+
+#include <stdlib.h>
+#include <string.h>
+#include <termios.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <sys/ioctl.h>
+
+#ifdef _WIN32
+#include <conio.h>
+#endif
+
+#include "flash.h"
+#include "programmer.h"
+
+/* We have:
+
+ MOSI -----> DTR
+ MISO -----> CTS
+ SCK  --+--> RTS
+        +--> DSR
+ CS#  -----> TXD
+
+*/
+
+enum pony_type {
+	TYPE_SI_PROG,
+	TYPE_SERBANG,
+};
+
+/* Serial port/pin mapping:
+
+  1	CD	<-
+  2	(RXD)	<-
+  3	TXD	->
+  4	DTR	->
+  5	GND     --
+  6	DSR	<-
+  7	RTS	->
+  8	CTS	<-
+  9	RI	<-
+*/
+
+enum pony_pin {
+	PIN_CD = 0,
+	PIN_RXD,
+	PIN_TXD,
+	PIN_DTR,
+	PIN_GND,
+	PIN_DSR,
+	PIN_RTS,
+	PIN_CTS,
+	PIN_RI,
+};
+
+/* The hardware programmer used. */
+static enum pony_type pony_type = TYPE_SI_PROG;
+
+static void pony_set_pin( int pin, int val ) {
+#ifdef _WIN32
+
+	DWORD ctl;
+
+	if( pin == PIN_TXD ) {
+		ctl = val ? SETBREAK: CLRBREAK;
+	}
+	else if( pin == PIN_DTR ) {
+		ctl = val ? SETDTR: CLRDTR;
+	}
+	else {
+		ctl = val ? SETRTS: CLRRTS;
+	}
+
+        EscapeCommFunction( sp_fd, ctl );
+#else
+
+	int ctl, s;
+
+	if( pin == PIN_TXD ) {
+		ioctl( sp_fd, val ? TIOCSBRK : TIOCCBRK, 0 );
+	}
+	else {
+		s = ( pin == PIN_DTR ) ? TIOCM_DTR : TIOCM_RTS;
+
+		ioctl( sp_fd, TIOCMGET, &ctl );
+
+		if ( val ) {
+			ctl |= s;
+		}
+		else {
+			ctl &= ~s;
+		}
+
+		ioctl( sp_fd, TIOCMSET, &ctl );
+	}
+#endif
+}
+
+static int pony_get_pin( int pin ) {
+	int s;
+
+#ifdef _WIN32
+
+	DWORD ctl;
+
+	s = ( pin == PIN_CTS ) ? MS_CTS_ON : MS_DSR_ON;
+
+	GetCommModemStatus( sp_fd, &ctl );
+#else
+
+	int ctl;
+
+        s = ( pin == PIN_CTS ) ? TIOCM_CTS : TIOCM_DSR;
+
+	ioctl( sp_fd, TIOCMGET, &ctl );
+#endif
+
+	return ( ( ctl & s ) ? 1 : 0 );
+
+}
+
+static void pony_bitbang_set_cs( int val )
+{
+	if( pony_type == TYPE_SI_PROG )
+	{
+		/* CS# is negated by the SI-Prog programmer. */
+		val =  val ? 0 : 1;
+	}
+
+	pony_set_pin( PIN_TXD, val );
+}
+
+static void pony_bitbang_set_sck( int val )
+{
+	pony_set_pin( PIN_RTS, val );
+}
+
+static void pony_bitbang_set_mosi( int val )
+{
+	pony_set_pin( PIN_DTR, val );
+}
+
+static int pony_bitbang_get_miso(void)
+{
+	int tmp;
+
+	tmp = pony_get_pin( PIN_CTS );
+
+	if( pony_type == TYPE_SERBANG )
+	{
+		/* MISO is negated by the SERBANG programmer. */
+		tmp = tmp ? 0 : 1;
+	}
+
+	return tmp;
+}
+
+static const struct bitbang_spi_master bitbang_spi_master_pony = {
+	.type = BITBANG_SPI_MASTER_PONY,
+	.set_cs = pony_bitbang_set_cs,
+	.set_sck = pony_bitbang_set_sck,
+	.set_mosi = pony_bitbang_set_mosi,
+	.get_miso = pony_bitbang_get_miso,
+	.half_period = 0,
+};
+
+int pony_spi_init(void)
+{
+	char *arg = NULL;
+
+	int i, data_in, data_out, have_device = 0;
+
+	int have_prog = 1;
+
+	/* The parameter is on format "dev=/dev/device,type=serbang" */
+
+	arg = extract_programmer_param( "dev" );
+
+	if ( arg && strlen( arg ) ) {
+		sp_fd = sp_openserport( arg, 9600 );
+
+		have_device++;
+	}
+
+	free( arg );
+
+	if ( ! have_device ) {
+		msg_perr("Error: No valid device specified.\n"
+			 "Use flashrom -p pony_spi:dev=/dev/device\n");
+
+		return 1;
+	}
+
+	arg = extract_programmer_param( "type" );
+
+	if ( arg ) {
+		if ( ! strcasecmp( arg, "serbang" ) ) {
+			pony_type = TYPE_SERBANG;
+		} else if ( ! strcasecmp( arg, "si_prog" ) ) {
+			pony_type = TYPE_SI_PROG;
+		} else {
+			msg_perr("Error: Invalid programmer type specified.\n");
+
+			free( arg );
+
+			serialport_shutdown( NULL );
+
+			return 1;
+		}
+	}
+
+	free( arg );
+
+	msg_pdbg( "Using the %s programmer.\n", ( ( pony_type == TYPE_SI_PROG ) ? "SI-Prog" : "SERBANG" ) );
+
+	/*
+	 * Detect if there is a SI-Prog compatible programmer connected.
+	 */
+
+	pony_bitbang_set_cs( 1 );
+	pony_bitbang_set_mosi( 1 );
+
+	/* We toggle SCK while we keep MOSI and CS# on. */
+
+	for ( i = 1; i <= 10; ++i ) {
+		data_out = i & 1;
+
+		pony_set_pin( PIN_RTS, data_out );
+
+		programmer_delay( 1000 );
+
+		data_in = pony_get_pin( PIN_DSR );
+
+		if ( data_out != data_in ) {
+			have_prog = 0;
+
+			break;
+		}
+	}
+
+	if ( ! have_prog ) {
+		msg_perr( "No SI-Prog compatible hardware detected.\n" );
+
+		serialport_shutdown( NULL );
+
+		return 1;
+	}
+
+	/* Register the shutdown callback. */
+
+	if ( register_shutdown( serialport_shutdown, NULL ) ) {
+		return 1;
+	}
+
+	/* Get the initial value before writing to any line. */
+
+	if ( bitbang_spi_init( &bitbang_spi_master_pony ) ) {
+		return 1;
+	}
+
+	return 0;
+}
diff -urN flashrom.orig/print.c flashrom/print.c
--- flashrom.orig/print.c	2012-03-13 16:52:50.000000000 +0100
+++ flashrom/print.c	2012-04-24 00:50:46.329483950 +0200
@@ -507,6 +507,12 @@ 
 	/* FIXME */
 	msg_ginfo("RayeR parallel port programmer\n");
 #endif
+#if CONFIG_PONY_SPI == 1
+	msg_ginfo("\nSupported devices for the %s programmer:\n",
+	       programmer_table[PROGRAMMER_PONY_SPI].name);
+	/* FIXME */
+	msg_ginfo("SI-Prog serial port programmer\n");
+#endif
 #if CONFIG_NICINTEL == 1
 	msg_ginfo("\nSupported devices for the %s programmer:\n",
 	       programmer_table[PROGRAMMER_NICINTEL].name);
diff -urN flashrom.orig/programmer.h flashrom/programmer.h
--- flashrom.orig/programmer.h	2012-03-13 16:52:50.000000000 +0100
+++ flashrom/programmer.h	2012-04-20 18:27:12.000000000 +0200
@@ -69,6 +69,9 @@ 
 #if CONFIG_RAYER_SPI == 1
 	PROGRAMMER_RAYER_SPI,
 #endif
+#if CONFIG_PONY_SPI == 1
+	PROGRAMMER_PONY_SPI,
+#endif
 #if CONFIG_NICINTEL == 1
 	PROGRAMMER_NICINTEL,
 #endif
@@ -110,6 +113,9 @@ 
 #if CONFIG_RAYER_SPI == 1
 	BITBANG_SPI_MASTER_RAYER,
 #endif
+#if CONFIG_PONY_SPI == 1
+	BITBANG_SPI_MASTER_PONY,
+#endif
 #if CONFIG_NICINTEL_SPI == 1
 	BITBANG_SPI_MASTER_NICINTEL,
 #endif
@@ -430,6 +436,11 @@ 
 int rayer_spi_init(void);
 #endif
 
+/* pony_spi.c */
+#if CONFIG_PONY_SPI == 1
+int pony_spi_init(void);
+#endif
+
 /* bitbang_spi.c */
 int bitbang_spi_init(const struct bitbang_spi_master *master);