Patchwork [0/4] Probing rewrite first try

login
register
about
Submitter Carl-Daniel Hailfinger
Date 2014-06-18 02:17:56
Message ID <53A0F6D4.4020003@gmx.net>
Download mbox | patch
Permalink /patch/4200/
State New
Headers show

Comments

Carl-Daniel Hailfinger - 2014-06-18 02:17:56
Am 16.06.2014 02:50 schrieb Stefan Tauner:
> This is based on '[PATCH 0/5] Bunch of stuff on the way to new probing'

I do like your design approach and it looks very sensible for all SPI
chips. You trim quite a lot of fat from various places and group
probe-related data with probe-related functions inside struct flashchip.
This improves readability and is also the right thing to do from a
design POV.
Besides that, draft code is an excellent way to get the discussion
going. Your no-nonsense approach also made me painfully aware of other
issues in struct flashchip which are relics from ancient times and
always worked well enough to ignore their design problems.
Thanks a lot!


First draft of a counter-proposal based on most of the design of your
patches.
Does not compile, only to illustrate what I mean by way of two examples.
Your individual probe function rewrite (except for the data->code moves)
is something I really like from a design POV, thus I didn't replicate it
here.
SPI entries in flashchips.c look exactly the same as in your rewrite
(because I think you designed the struct exceptionally well for SPI
chips), but the non-SPI entries differ (because I extended the struct to
handle the cases I'm concerned about).
I have not yet looked in detail at the probe function prototype for
probe_*, I just used that snippet from your patch as reference for now.

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

Patch

Index: flashrom-proberewrite/flash.h
===================================================================
--- flashrom-proberewrite/flash.h	(Revision 1822)
+++ flashrom-proberewrite/flash.h	(Arbeitskopie)
@@ -141,23 +141,44 @@ 
 #define TEST_BAD_PRE	(struct tested){ .probe = BAD, .read = BAD, .erase = BAD, .write = NT }
 #define TEST_BAD_PREW	(struct tested){ .probe = BAD, .read = BAD, .erase = BAD, .write = BAD }
 
+#define NUM_PROBES 3
+#define NUM_PROBE_BYTES 5 /* Values below 2 will break code. */
+
 struct flashctx;
+struct registered_programmer; /* programmer.h */
+struct probe;
+struct probe_res;
+
+/* fixme  */
+typedef int (probefunc_t)(struct flashctx *flash, struct probe_res *res, unsigned int res_len, const struct probe *p);
 typedef int (erasefunc_t)(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
 
+struct probe_res {
+	probefunc_t *probe_func;
+	unsigned int chip_size; /* in kB */
+	int probe_feature_bits;
+	int chip_relevant_feature_bits;
+	signed int probe_timing;
+	uint8_t len;
+	uint8_t vals[NUM_PROBE_BYTES];
+};
+
+/* Feature types relevant for probing */
+#define PROBE_TIMING	(1 << 0)
+#define PROBE_ADDR	(1 << 1)
+#define PROBE_SIZE	(1 << 2)
+
+struct probe_res_data {
+	uint8_t len;
+	uint8_t vals[NUM_PROBE_BYTES];
+};
+
 struct flashchip {
 	const char *vendor;
 	const char *name;
 
 	enum chipbustype bustype;
 
-	/*
-	 * With 32bit manufacture_id and model_id we can cover IDs up to
-	 * (including) the 4th bank of JEDEC JEP106W Standard Manufacturer's
-	 * Identification code.
-	 */
-	uint32_t manufacture_id;
-	uint32_t model_id;
-
 	/* Total chip size in kilobytes */
 	unsigned int total_size;
 	/* Chip page size in bytes */
@@ -172,13 +193,17 @@ 
 		enum test_state write;
 	} tested;
 
-	int (*probe) (struct flashctx *flash);
-
-	/* Delay after "enter/exit ID mode" commands in microseconds.
-	 * NB: negative values have special meanings, see TIMING_* below.
+	struct prober {
+		probefunc_t *func;
+		struct probe_res_data res_data;
+		int probe_feature_bits;
+		uint32_t extradata;
+		/* extradata can contain the encoded delay after "enter/exit ID mode" commands in microseconds.
+		 * NB: Some values have special meanings, see TIMING_* below.
 	 */
-	signed int probe_timing;
+	} probers[NUM_PROBES];
 
+
 	/*
 	 * Erase blocks and associated erase function. Any chip erase function
 	 * is stored as chip-sized virtual block together with said function.
@@ -208,22 +233,22 @@ 
 };
 
 struct flashctx {
-	struct flashchip *chip;
+	const struct flashchip *chip;
 	chipaddr virtual_memory;
 	/* Some flash devices have an additional register space. */
 	chipaddr virtual_registers;
 	struct registered_programmer *pgm;
 };
 
-/* Timing used in probe routines. ZERO is -2 to differentiate between an unset
- * field and zero delay.
+/* Timing used in probe routines. Add 1 to all values to differentiate between an unset field and zero delay.
  * 
  * SPI devices will always have zero delay and ignore this field.
  */
-#define TIMING_FIXME	-1
+#define TIMING_FIXME	(0) & 0xffff
 /* this is intentionally same value as fixme */
-#define TIMING_IGNORED	-1
-#define TIMING_ZERO	-2
+#define TIMING_IGNORED	(0) & 0xffff
+#define TIMING(x)	(x + 1) & 0xffff
+#define TIMING_MASK	0xffff
 
 extern const struct flashchip flashchips[];
 extern const unsigned int flashchips_size;
@@ -258,7 +283,7 @@ 
 void map_flash_registers(struct flashctx *flash);
 int read_memmapped(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len);
 int erase_flash(struct flashctx *flash);
-int probe_flash(struct registered_programmer *pgm, int startchip, struct flashctx *fill_flash, int force);
+int probe_flash(struct flashctx **flashes, const struct registered_programmer *pgm);
 int read_flash_to_file(struct flashctx *flash, const char *filename);
 char *extract_param(const char *const *haystack, const char *needle, const char *delim);
 int verify_range(struct flashctx *flash, const uint8_t *cmpbuf, unsigned int start, unsigned int len);
@@ -344,5 +369,7 @@ 
 int spi_send_multicommand(struct flashctx *flash, struct spi_command *cmds);
 uint32_t spi_get_valid_read_addr(struct flashctx *flash);
 
+/* programmer.c */
 enum chipbustype get_buses_supported(void);
+
 #endif				/* !__FLASH_H__ */
Index: flashrom-proberewrite/flashchips.c
===================================================================
--- flashrom-proberewrite/flashchips.c	(Revision 1822)
+++ flashrom-proberewrite/flashchips.c	(Arbeitskopie)
@@ -44,6 +44,13 @@ 
 	 * .page_size		= Page or eraseblock(?) size in bytes
 	 * .tested		= Test status
 	 * .probe		= Probe function
+	 * .probers[]		= Array of probe functions
+	 * {
+	 *	.func		= Probe function
+	 *	.res_data	= Expected probe result data
+	 *	.probe_feature_bits = List of feature bit types relevant to this probe
+	 *	.extradata	= Data only relevant for this probe, e.g. timing
+	 * }
 	 * .probe_timing	= Probe function delay
 	 * .block_erasers[]	= Array of erase layouts and erase functions
 	 * {
@@ -61,14 +68,14 @@ 
 		.vendor		= "AMD",
 		.name		= "Am29F010A/B",
 		.bustype	= BUS_PARALLEL,
-		.manufacture_id	= AMD_ID,
-		.model_id	= AMD_AM29F010B,	/* Same as Am29F010A */
 		.total_size	= 128,
 		.page_size	= 16 * 1024,
 		.feature_bits	= FEATURE_ADDR_2AA | FEATURE_EITHER_RESET,
 		.tested		= TEST_OK_PRE,
-		.probe		= probe_jedec,
-		.probe_timing	= TIMING_ZERO,
+		.probers	=
+		{
+			{ probe_jedec, { 2, {  AMIC_ID, AMD_AM29F010B } }, PROBE_ADDR | PROBE_SIZE | PROBE_TIMING, TIMING(0) }, /* Same as Am29F010A */
+		},
 		.block_erasers	=
 		{
 			{
@@ -84,6 +91,7 @@ 
 		.voltage	= {4500, 5500},
 	},
 
+#if 0
 	{
 		.vendor		= "AMD",
 		.name		= "Am29F002(N)BB",
@@ -536,19 +544,21 @@ 
 		.read		= read_memmapped,
 		.voltage	= {3000, 3600}, /* 3.0-3.6V for type -70R, others 2.7-3.6V */
 	},
+#endif
 
 	{
 		.vendor		= "AMIC",
 		.name		= "A25L05PT",
 		.bustype	= BUS_SPI,
-		.manufacture_id	= AMIC_ID,
-		.model_id	= AMIC_A25L05PT,
 		.total_size	= 64,
 		.page_size	= 256,
 		.feature_bits	= FEATURE_WRSR_WREN,
 		.tested		= TEST_UNTESTED,
-		.probe		= probe_spi_rdid4,
-		.probe_timing	= TIMING_ZERO,
+		.probers	=
+		{
+			{ probe_spi_rdid, { 4, { AMIC_ID, AMIC_A25L05PT } } },
+			{ probe_spi_res,  { 1, { 0x05 } } },
+		},
 		.block_erasers	=
 		{
 			{
@@ -571,6 +581,7 @@ 
 		.voltage	= {2700, 3600},
 	},
 
+#if 0
 	{
 		.vendor		= "AMIC",
 		.name		= "A25L05PU",
@@ -13567,6 +13578,7 @@ 
 		.probe		= probe_spi_rems,
 		.write		= NULL,
 	},
+#endif
 
 	{0}
 };