[flashrom] [PATCH 0/4] Probing rewrite first try
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Wed Jun 18 04:17:56 CEST 2014
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 at gmx.net>
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}
};
--
http://www.hailfinger.org/
More information about the flashrom
mailing list