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

[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.
> };
> +#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?
> @@ -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.
> +/*
> 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.
> .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 [e-mail]\n",
> 	 __func__);

I don't really like the error message, I would prefer "%s called without
register_opaque" or something like that.
> +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.

Otherwise, the patch looks good to me. This is
Acked-By: Michael Karcher

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

Regards,
  Michael Karcher

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

[Carl-Daniel Hailfinger]

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 [e-mail]\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

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

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 [e-mail]\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 [e-mail]\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 [e-mail]\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 [e-mail]\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 [e-mail]\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, ");
    }

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

[Stefan Tauner]

On Wed, 02 Nov 2011 23:20:17 +0100
Carl-Daniel Hailfinger  wrote:
> > > @@ -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?

done

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

[Michael Karcher]

> > > +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 [e-mail]\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" ?

Good idea, go for it.
> Index: flashrom-register_opaque_programmer/opaque.c
>
> --- flashrom-register_opaque_programmer/opaque.c	(Revision 0)
> +++ flashrom-register_opaque_programmer/opaque.c	(Revision 0)

[...]
> +#include <strings.h>

Huh?! What's that strange BSD compatibility header doing here?
> +#include <string.h>
> +#include "flash.h"
> +#include "flashchips.h"

[...]

[in flashchips.c]
> @@ -9005,6 +9027,7 @@
>    	.probe		= probe_spi_rdid,
>    	.write		= NULL,
>    },
> {
>   .vendor		= "Generic",
>   .name		= "unknown SPI chip (REMS)",

This extra line has already be claimed by Stefan Tauner - it's not yours
anymore! ;)
> Signed-off-by: Carl-Daniel Hailfinger

If you remove the <strings.h> include or provide a good reason for it, this is

Acked-by: Michael Karcher

Regards,
  Michael Karcher

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

[Carl-Daniel Hailfinger]

Am 04.11.2011 22:19 schrieb Michael Karcher:
> This extra line has already be claimed by Stefan Tauner - it's not yours
> anymore! ;)

Killed.
> > Signed-off-by: Carl-Daniel Hailfinger
>
> If you remove the <strings.h> include or provide a good reason for it, this is

Killed.
> Acked-by: Michael Karcher

Thanks, committed in r1459.

Regards,
Carl-Daniel