[flashrom] [PATCH 1/5] Add opaque programmer registration infrastructure.

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Wed Nov 2 23:20:17 CET 2011


Am 02.11.2011 18:29 schrieb Michael Karcher:
> Am Freitag, den 21.10.2011, 00:39 +0200 schrieb Stefan Tauner:
>> diff --git a/flash.h b/flash.h
>> index 535c1b8..8ad2845 100644
>> --- a/flash.h
>> +++ b/flash.h
>> @@ -62,8 +62,9 @@ enum chipbustype {
>>  	BUS_LPC		= 1 << 1,
>>  	BUS_FWH		= 1 << 2,
>>  	BUS_SPI		= 1 << 3,
>> +	BUS_PROG	= 1 << 4,
>>  	BUS_NONSPI	= BUS_PARALLEL | BUS_LPC | BUS_FWH,
>> -	BUS_UNKNOWN	= BUS_PARALLEL | BUS_LPC | BUS_FWH | BUS_SPI,
>> +	BUS_UNKNOWN	= BUS_PARALLEL | BUS_LPC | BUS_FWH | BUS_SPI | BUS_PROG,
> We don't need BUS_UNKNOWN. It is referenced only in flashbuses_to_text.
> Having a programmer being both opaque and non-opaque seems to be as
> useless as defining that a chip "might also be accessed through an
> opaque interface". In the case of opaque interfaces, we don't care about
> the chip type - so I don't see any use case for BUS_UNKNOWN, even more
> after adding BUS_PROG.

Killed.

 
>>  };
>> +#if defined(CONFIG_INTERNAL) && (defined(__i386__) || defined(__x86_64__))
>> +	{
>> +		.vendor		= "Programmer",
>> +		.name		= "Opaque flash chip",
>> +		.bustype	= BUS_PROG,
>> +		.manufacture_id	= PROGMANUF_ID,
>> +		.model_id	= PROGDEV_ID,
>> +		.total_size	= 0,
>> +		.page_size	= 256,
>> +		/* probe is assumed to work, rest will be filled in by probe */
>> +		.tested		= TEST_OK_PROBE,
>> +		.probe		= probe_opaque,
>> +		/* eraseblock sizes will be set by the probing function */
>> +		.block_erasers	=
>> +		{
>> +			{
>> +				.block_erase = erase_opaque,
>> +			}
>> +		},
>> +		.write		= write_opaque,
>> +		.read		= read_opaque,
>> +	},
>> +#endif // defined(CONFIG_INTERNAL) && (defined(__i386__) || defined(__x86_64__))
> Why do we need any prerequisite for the opaque programmer? It should
> work outside of x86(-64), too, depending on which opaque backend is
> used. I guess this #if is there because at the moment, the only opaque
> programmer is the ICH9 stuff, but wouldn't a define NEED_OPAQUE that is
> set in the include file as soon as any programmer based on the opaque
> programmer is selected be more sensible?

We don't use #ifdef for all other chips, so the sane solution is to
avoid the #ifdef completely.


>> @@ -9005,6 +9027,7 @@ const struct flashchip flashchips[] = {
>>  		.probe		= probe_spi_rdid,
>>  		.write		= NULL,
>>  	},
>> +
>>  	{
>>  		.vendor		= "Generic",
>>  		.name		= "unknown SPI chip (REMS)",
> Unrelated change - you might have it in accidentally.

Right. Stefan, do you plan to move this to your tested_stuff branch?

 
>> +/*
>> + * Contains the opaque programmer framework.
>> + * An opaque programmer is a programmer which does not provide direct access
>> + * to the flash chip and which abstracts all flash chip properties into a
>> + * programmer specific interface.
>> + */
>> +
>> +#include <strings.h>
>> +#include <string.h>
>> +#include "flash.h"
>> +#include "flashchips.h"
>> +#include "chipdrivers.h"
>> +#include "programmer.h"
>> +
>> +const struct opaque_programmer opaque_programmer_none = {
>> +	.max_data_read = MAX_DATA_UNSPECIFIED,
>> +	.max_data_write = MAX_DATA_UNSPECIFIED,
>> +	.probe = NULL,
>> +	.read = NULL,
> You are missing ".erase = NULL" here.

Fixed.

 
>> +	.write = NULL,
>> +};
>> +
>> +const struct opaque_programmer *opaque_programmer = &opaque_programmer_none;
>> +
>> +int probe_opaque(struct flashchip *flash)
>> +{
>> +	if (!opaque_programmer->probe) {
>> +		msg_perr("%s called, but this is not an opaque programmer. "
>> +			 "Please report a bug at flashrom at flashrom.org\n",
>> +			 __func__);
> I don't really like the error message, I would prefer "%s called without
> register_opaque" or something like that.

What about "%s called before register_opaque_programmer" ?


>> +void register_opaque_programmer(const struct opaque_programmer *pgm)
>> +{
>> +	opaque_programmer = pgm;
>> +	buses_supported |= BUS_PROG;
>> +}
> You might want to check that the function pointers in pgm are not NULL,
> to make the suggested error message "without register_opaque" more apt.

Fixed.

 
> Otherwise, the patch looks good to me. This is
> Acked-By: Michael Karcher <flashrom at mkarcher.dialup.fu-berlin.de>

Thanks for the review!

 
> If you feel confident you didn't mess up anything, feel free to re-use
> the ack after minor edits.

While I tested compilation, I'd still appreciate a second look at the
result.


An opaque programmer does not allow direct flash access and only offers
abstract probe/read/erase/write methods.
Due to that, opaque programmers need their own infrastructure and
registration framework.

Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

Index: flashrom-register_opaque_programmer/flash.h
===================================================================
--- flashrom-register_opaque_programmer/flash.h	(Revision 1458)
+++ flashrom-register_opaque_programmer/flash.h	(Arbeitskopie)
@@ -62,8 +62,8 @@
 	BUS_LPC		= 1 << 1,
 	BUS_FWH		= 1 << 2,
 	BUS_SPI		= 1 << 3,
+	BUS_PROG	= 1 << 4,
 	BUS_NONSPI	= BUS_PARALLEL | BUS_LPC | BUS_FWH,
-	BUS_UNKNOWN	= BUS_PARALLEL | BUS_LPC | BUS_FWH | BUS_SPI,
 };
 
 /*
Index: flashrom-register_opaque_programmer/Makefile
===================================================================
--- flashrom-register_opaque_programmer/Makefile	(Revision 1458)
+++ flashrom-register_opaque_programmer/Makefile	(Arbeitskopie)
@@ -242,7 +242,7 @@
 CHIP_OBJS = jedec.o stm50flw0x0x.o w39.o w29ee011.o \
 	sst28sf040.o m29f400bt.o 82802ab.o pm49fl00x.o \
 	sst49lfxxxc.o sst_fwhub.o flashchips.o spi.o spi25.o sharplhf00l04.o \
-	a25.o at25.o
+	a25.o at25.o opaque.o
 
 LIB_OBJS = layout.o
 
Index: flashrom-register_opaque_programmer/opaque.c
===================================================================
--- flashrom-register_opaque_programmer/opaque.c	(Revision 0)
+++ flashrom-register_opaque_programmer/opaque.c	(Revision 0)
@@ -0,0 +1,100 @@
+/*
+ * This file is part of the flashrom project.
+ *
+ * Copyright (C) 2011 Carl-Daniel Hailfinger
+ *
+ * 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
+ */
+
+/*
+ * Contains the opaque programmer framework.
+ * An opaque programmer is a programmer which does not provide direct access
+ * to the flash chip and which abstracts all flash chip properties into a
+ * programmer specific interface.
+ */
+
+#include <strings.h>
+#include <string.h>
+#include "flash.h"
+#include "flashchips.h"
+#include "chipdrivers.h"
+#include "programmer.h"
+
+const struct opaque_programmer opaque_programmer_none = {
+	.max_data_read = MAX_DATA_UNSPECIFIED,
+	.max_data_write = MAX_DATA_UNSPECIFIED,
+	.probe = NULL,
+	.read = NULL,
+	.write = NULL,
+	.erase = NULL,
+};
+
+const struct opaque_programmer *opaque_programmer = &opaque_programmer_none;
+
+int probe_opaque(struct flashchip *flash)
+{
+	if (!opaque_programmer->probe) {
+		msg_perr("%s called before register_opaque_programmer. "
+			 "Please report a bug at flashrom at flashrom.org\n",
+			 __func__);
+		return 0;
+	}
+
+	return opaque_programmer->probe(flash);
+}
+
+int read_opaque(struct flashchip *flash, uint8_t *buf, int start, int len)
+{
+	if (!opaque_programmer->read) {
+		msg_perr("%s called before register_opaque_programmer. "
+			 "Please report a bug at flashrom at flashrom.org\n",
+			 __func__);
+		return 1;
+	}
+	return opaque_programmer->read(flash, buf, start, len);
+}
+
+int write_opaque(struct flashchip *flash, uint8_t *buf, int start, int len)
+{
+	if (!opaque_programmer->write) {
+		msg_perr("%s called before register_opaque_programmer. "
+			 "Please report a bug at flashrom at flashrom.org\n",
+			 __func__);
+		return 1;
+	}
+	return opaque_programmer->write(flash, buf, start, len);
+}
+
+int erase_opaque(struct flashchip *flash, unsigned int blockaddr, unsigned int blocklen)
+{
+	if (!opaque_programmer->erase) {
+		msg_perr("%s called before register_opaque_programmer. "
+			 "Please report a bug at flashrom at flashrom.org\n",
+			 __func__);
+		return 1;
+	}
+	return opaque_programmer->erase(flash, blockaddr, blocklen);
+}
+
+void register_opaque_programmer(const struct opaque_programmer *pgm)
+{
+	if (!pgm->probe || !pgm->read || !pgm->write || !pgm->erase) {
+		msg_perr("%s called with one of probe/read/write/erase being "
+			 "NULL. Please report a bug at flashrom at flashrom.org\n",
+			 __func__);
+		return;
+	}
+	opaque_programmer = pgm;
+	buses_supported |= BUS_PROG;
+}
Index: flashrom-register_opaque_programmer/flashchips.c
===================================================================
--- flashrom-register_opaque_programmer/flashchips.c	(Revision 1458)
+++ flashrom-register_opaque_programmer/flashchips.c	(Arbeitskopie)
@@ -8874,6 +8874,28 @@
 	},
 
 	{
+		.vendor		= "Programmer",
+		.name		= "Opaque flash chip",
+		.bustype	= BUS_PROG,
+		.manufacture_id	= PROGMANUF_ID,
+		.model_id	= PROGDEV_ID,
+		.total_size	= 0,
+		.page_size	= 256,
+		/* probe is assumed to work, rest will be filled in by probe */
+		.tested		= TEST_OK_PROBE,
+		.probe		= probe_opaque,
+		/* eraseblock sizes will be set by the probing function */
+		.block_erasers	=
+		{
+			{
+				.block_erase = erase_opaque,
+			}
+		},
+		.write		= write_opaque,
+		.read		= read_opaque,
+	},
+
+	{
 		.vendor		= "AMIC",
 		.name		= "unknown AMIC SPI chip",
 		.bustype	= BUS_SPI,
@@ -9005,6 +9027,7 @@
 		.probe		= probe_spi_rdid,
 		.write		= NULL,
 	},
+
 	{
 		.vendor		= "Generic",
 		.name		= "unknown SPI chip (REMS)",
Index: flashrom-register_opaque_programmer/flashchips.h
===================================================================
--- flashrom-register_opaque_programmer/flashchips.h	(Revision 1458)
+++ flashrom-register_opaque_programmer/flashchips.h	(Arbeitskopie)
@@ -646,4 +646,7 @@
 #define WINBOND_W49V002A	0xB0
 #define WINBOND_W49V002FA	0x32
 
+#define PROGMANUF_ID		0xFFFE	/* dummy ID for opaque chips behind a programmer */
+#define PROGDEV_ID		0x01	/* dummy ID for opaque chips behind a programmer */
+
 #endif /* !FLASHCHIPS_H */
Index: flashrom-register_opaque_programmer/programmer.h
===================================================================
--- flashrom-register_opaque_programmer/programmer.h	(Revision 1458)
+++ flashrom-register_opaque_programmer/programmer.h	(Arbeitskopie)
@@ -601,6 +601,19 @@
 int wbsio_check_for_spi(void);
 #endif
 
+/* opaque.c */
+struct opaque_programmer {
+	int max_data_read;
+	int max_data_write;
+	/* Specific functions for this programmer */
+	int (*probe) (struct flashchip *flash);
+	int (*read) (struct flashchip *flash, uint8_t *buf, int start, int len);
+	int (*write) (struct flashchip *flash, uint8_t *buf, int start, int len);
+	int (*erase) (struct flashchip *flash, unsigned int blockaddr, unsigned int blocklen);
+};
+extern const struct opaque_programmer *opaque_programmer;
+void register_opaque_programmer(const struct opaque_programmer *pgm);
+
 /* serprog.c */
 #if CONFIG_SERPROG == 1
 int serprog_init(void);
Index: flashrom-register_opaque_programmer/chipdrivers.h
===================================================================
--- flashrom-register_opaque_programmer/chipdrivers.h	(Revision 1458)
+++ flashrom-register_opaque_programmer/chipdrivers.h	(Arbeitskopie)
@@ -58,6 +58,12 @@
 int spi_write_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize);
 int spi_aai_write(struct flashchip *flash, uint8_t *buf, int start, int len);
 
+/* opaque.c */
+int probe_opaque(struct flashchip *flash);
+int read_opaque(struct flashchip *flash, uint8_t *buf, int start, int len);
+int write_opaque(struct flashchip *flash, uint8_t *buf, int start, int len);
+int erase_opaque(struct flashchip *flash, unsigned int blockaddr, unsigned int blocklen);
+
 /* a25.c */
 int spi_prettyprint_status_register_amic_a25l05p(struct flashchip *flash);
 int spi_prettyprint_status_register_amic_a25l40p(struct flashchip *flash);
Index: flashrom-register_opaque_programmer/print.c
===================================================================
--- flashrom-register_opaque_programmer/print.c	(Revision 1458)
+++ flashrom-register_opaque_programmer/print.c	(Arbeitskopie)
@@ -32,13 +32,11 @@
 char *flashbuses_to_text(enum chipbustype bustype)
 {
 	char *ret = calloc(1, 1);
-	if (bustype == BUS_UNKNOWN) {
-		ret = strcat_realloc(ret, "Unknown, ");
 	/*
 	 * FIXME: Once all chipsets and flash chips have been updated, NONSPI
 	 * will cease to exist and should be eliminated here as well.
 	 */
-	} else if (bustype == BUS_NONSPI) {
+	if (bustype == BUS_NONSPI) {
 		ret = strcat_realloc(ret, "Non-SPI, ");
 	} else {
 		if (bustype & BUS_PARALLEL)
@@ -49,6 +47,8 @@
 			ret = strcat_realloc(ret, "FWH, ");
 		if (bustype & BUS_SPI)
 			ret = strcat_realloc(ret, "SPI, ");
+		if (bustype & BUS_PROG)
+			ret = strcat_realloc(ret, "Programmer-specific, ");
 		if (bustype == BUS_NONE)
 			ret = strcat_realloc(ret, "None, ");
 	}


-- 
http://www.hailfinger.org/





More information about the flashrom mailing list