Patchwork PONY_SPI, now with Windows support

login
register
about
Submitter Virgil-Adrian Teaca
Date 2012-04-29 21:58:49
Message ID <CAOfppk4c8DUiOKS6GfXiCd3CzbkYX7we35XgiuU_D=_oo-Vctg@mail.gmail.com>
Download mbox | patch
Permalink /patch/3614/
State Superseded
Headers show

Comments

Virgil-Adrian Teaca - 2012-04-29 21:58:49
First of all, thank you for your kindly review!

Then, there is attached a new patch, where code is cleaned and some main
bitbanging functions are moved to serial.c, to simplify the pony_spi.c and
to make them available to all possible serial drivers which want
bitbanging...

All the best,
Virgil.
Signed-off-by: Virgil-Adrian Teaca <darkstarlinux@gmail.com>
Michael Karcher - 2012-04-29 22:36:56
On Sun, 2012-04-29 at 23:58 +0200, Virgil-Adrian Teaca wrote:

> Then, there is attached a new patch, where code is cleaned and some
> main bitbanging functions are moved to serial.c, to simplify the
> pony_spi.c and to make them available to all possible serial drivers
> which want bitbanging...
Great! The code is now styled and designed as I expect it for flashrom
code, just some minor nitpicks and one detail I missed to mention in the
last review, and is a problem we can't expect you to know. Let's start
with that one:

In programmer.h, we have:

#if CONFIG_OGP_SPI == 1 || CONFIG_NICINTEL_SPI == 1 || CONFIG_RAYER_SPI == 1 || CONFIG_PONY_SPI == 1 || (CONFIG_INTERNAL == 1 && (defined(__i386__) || defined(__x86_64__)))
        SPI_CONTROLLER_BITBANG,
#endif

If the only bitbang programmer enabled is your programmer,
SPI_CONTROLLER_BITBANG doesn't get defined. I hit that problem when I
tried to cross-compile for Windows, as on Windows, we don't support the
internal programmer or PCI-based programmers (which also means direct
port I/O programmers like rayer for now), so your programmer would be
the only one enabled, but is not in the list of programmers tested.

In my oppion, the whole test should be replaced by
	#if CONFIG_BITBANG_SPI == 1
because the Makefile already handles defining CONFIG_BITBANG_SPI the
right way.

[Makefile]
>  LIBFLASHROM_OBJS = $(CHIP_OBJS) $(PROGRAMMER_OBJS) $(LIB_OBJS)
> -OBJS = $(CLI_OBJS) $(LIBFLASHROM_OBJS) 
> +OBJS = $(CLI_OBJS) $(LIBFLASHROM_OBJS)
This change is an white-space change (removing a space at end of line.
It should not have been there in the first place, though) which is
unrelated to your patch. If it doesn't take too much hassle for you,
please remove this change from your patch.

[pony_spi.c]

> +static void pony_bitbang_set_cs(int val)
> +{
> +       if(pony_type == TYPE_SI_PROG)
Please add a space after "if"
> +       if( pony_type == TYPE_SERBANG )
Please add a space after "if" and remove the inner spaces
> +       /* The parameter is on format "dev=/dev/device,type=serbang" */
> +       arg = extract_programmer_param( "dev" );
No spaces inside the parens, please.
> +               } else {
> +                       msg_perr("Error: Invalid programmer type specified.\n");
> +                       free( arg );
> +                       return 1;
> +               }
> +       }
> +       free( arg );
> +
> +       msg_pdbg("Using the %s programmer.\n", ((pony_type == TYPE_SI_PROG ) ? "SI-Prog" : "SERBANG"));
In the "free" calls, remove space, too.
Generally, remove the space after "!" for negation.

[programmer.h]
> +enum sp_pin {
> +       PIN_CD = 1,
> +       PIN_RXD,
> +       PIN_TXD,
> +       PIN_DTR,
> +       PIN_GND,
> +       PIN_DSR,
> +       PIN_RTS,
> +       PIN_CTS,
> +       PIN_RI,
> +};
Can you rename them to SP_PIN_...? This enum is defined in programmer.h
now, which is included nearly everywhere in flashrom, not only on serial
port related stuff, so limiting to names starting with serialport or SP_
seems appropriate.

[serial.c]
> +               ioctl( sp_fd, val ? TIOCSBRK : TIOCCBRK, 0 );
spacing inside parentheses.
> +               ioctl( sp_fd, TIOCMGET, &ctl );
dito.
> +               if ( val ) {
dito.
> +       GetCommModemStatus( sp_fd, &ctl );
dito.

> @@ -212,7 +266,7 @@
>                 }
>                 if (!tmp)
>                         msg_pdbg("Empty write\n");
> -               writecnt -= tmp; 
> +               writecnt -= tmp;
>                 buf += tmp;
>         }
unrelated whitespace change.

Regards,
  Michael Karcher
Stefan Tauner - 2012-04-29 23:11:51
On Mon, 30 Apr 2012 00:36:56 +0200
Michael Karcher <flashrom@mkarcher.dialup.fu-berlin.de> wrote:

> [Makefile]
> >  LIBFLASHROM_OBJS = $(CHIP_OBJS) $(PROGRAMMER_OBJS) $(LIB_OBJS)
> > -OBJS = $(CLI_OBJS) $(LIBFLASHROM_OBJS) 
> > +OBJS = $(CLI_OBJS) $(LIBFLASHROM_OBJS)  
> This change is an white-space change (removing a space at end of line.
> It should not have been there in the first place, though) which is
> unrelated to your patch. If it doesn't take too much hassle for you,
> please remove this change from your patch.

added that hunk to my tested_stuff branch.

Patch

diff -urN flashrom.orig/Makefile flashrom/Makefile
--- flashrom.orig/Makefile	2012-04-20 12:17:31.000000000 +0200
+++ flashrom/Makefile	2012-04-29 23:24:35.351875636 +0200
@@ -292,6 +292,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 +351,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 +368,7 @@ 
 endif
 endif
 endif
+endif
 
 ifeq ($(CONFIG_INTERNAL), yes)
 FEATURE_CFLAGS += -D'CONFIG_INTERNAL=1'
@@ -388,6 +395,12 @@ 
 NEED_PCI := yes
 endif
 
+ifeq ($(CONFIG_PONY_SPI), yes)
+FEATURE_CFLAGS += -D'CONFIG_PONY_SPI=1'
+PROGRAMMER_OBJS += pony_spi.o
+NEED_SERIAL := yes
+endif
+
 ifeq ($(CONFIG_BITBANG_SPI), yes)
 FEATURE_CFLAGS += -D'CONFIG_BITBANG_SPI=1'
 PROGRAMMER_OBJS += bitbang_spi.o
@@ -534,7 +547,7 @@ 
 FEATURE_LIBS += $(shell LC_ALL=C grep -q "NEEDLIBZ := yes" .libdeps && printf "%s" "-lz")
 
 LIBFLASHROM_OBJS = $(CHIP_OBJS) $(PROGRAMMER_OBJS) $(LIB_OBJS)
-OBJS = $(CLI_OBJS) $(LIBFLASHROM_OBJS) 
+OBJS = $(CLI_OBJS) $(LIBFLASHROM_OBJS)
 
 $(PROGRAM)$(EXEC_SUFFIX): $(OBJS)
 	$(CC) $(LDFLAGS) -o $(PROGRAM)$(EXEC_SUFFIX) $(OBJS) $(FEATURE_LIBS) $(LIBS)
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 23:49:14.422355934 +0200
@@ -0,0 +1,159 @@ 
+/*
+ * 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 "flash.h"
+#include "programmer.h"
+
+/* We have:
+
+ MOSI -----> DTR
+ MISO -----> CTS
+ SCK  --+--> RTS
+        +--> DSR
+ CS#  -----> TXD
+
+*/
+
+enum pony_type {
+	TYPE_SI_PROG,
+	TYPE_SERBANG,
+};
+
+/* The hardware programmer used. */
+static enum pony_type pony_type = TYPE_SI_PROG;
+
+static void pony_bitbang_set_cs(int val)
+{
+	if(pony_type == TYPE_SI_PROG)
+	{
+		/* CS# is negated by the SI-Prog programmer. */
+		val ^=  1;
+	}
+	sp_set_pin(PIN_TXD, val);
+}
+
+static void pony_bitbang_set_sck(int val)
+{
+	sp_set_pin(PIN_RTS, val);
+}
+
+static void pony_bitbang_set_mosi(int val)
+{
+	sp_set_pin(PIN_DTR, val);
+}
+
+static int pony_bitbang_get_miso(void)
+{
+	int tmp;
+	tmp = sp_get_pin(PIN_CTS);
+
+	if( pony_type == TYPE_SERBANG )
+	{
+		/* MISO is negated by the SERBANG programmer. */
+		tmp ^= 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;
+	}
+	/* Register the shutdown callback. */
+	if (register_shutdown(serialport_shutdown, NULL)) {
+		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 );
+			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;
+		sp_set_pin(PIN_RTS, data_out);
+		programmer_delay( 1000 );
+		data_in = sp_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" );
+		return 1;
+	}
+
+	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-29 23:34:07.795932226 +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 and SERBANG 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-29 23:44:46.573435372 +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);
 
@@ -634,4 +645,31 @@ 
 int serialport_write(unsigned char *buf, unsigned int writecnt);
 int serialport_read(unsigned char *buf, unsigned int readcnt);
 
+/* Serial port/pin mapping:
+
+  1	CD	<-
+  2	(RXD)	<-
+  3	TXD	->
+  4	DTR	->
+  5	GND     --
+  6	DSR	<-
+  7	RTS	->
+  8	CTS	<-
+  9	RI	<-
+*/
+enum sp_pin {
+	PIN_CD = 1,
+	PIN_RXD,
+	PIN_TXD,
+	PIN_DTR,
+	PIN_GND,
+	PIN_DSR,
+	PIN_RTS,
+	PIN_CTS,
+	PIN_RI,
+};
+
+void sp_set_pin(enum sp_pin pin, int val);
+int sp_get_pin(enum sp_pin pin);
+
 #endif				/* !__PROGRAMMER_H__ */
diff -urN flashrom.orig/serial.c flashrom/serial.c
--- flashrom.orig/serial.c	2011-10-22 21:55:16.000000000 +0200
+++ flashrom/serial.c	2012-04-29 23:47:28.493733237 +0200
@@ -32,6 +32,9 @@ 
 #include <conio.h>
 #else
 #include <termios.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/ioctl.h>
 #endif
 #include "flash.h"
 #include "programmer.h"
@@ -172,6 +175,57 @@ 
 #endif
 }
 
+void sp_set_pin(enum sp_pin 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
+}
+
+int sp_get_pin(enum sp_pin 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);
+
+}
+
 void sp_flush_incoming(void)
 {
 #ifdef _WIN32
@@ -212,7 +266,7 @@ 
 		}
 		if (!tmp)
 			msg_pdbg("Empty write\n");
-		writecnt -= tmp; 
+		writecnt -= tmp;
 		buf += tmp;
 	}