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
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} };