Re: [flashrom] [PATCH] Add support for generic SPI RES matching.

[Carl-Daniel Hailfinger]

Am 30.08.2012 23:55 schrieb Stefan Tauner:
> Support generic 1-byte SPI RES matching (low match quality) and enable
> read support by guessing the chip size from the ID.
> Support generic 2-byte SPI RES matching (high match quality).
>
> Signed-off-by: Carl-Daniel Hailfinger 
> Signed-off-by: Stefan Tauner
>
> chipdrivers.h |    1 +
>  flashchips.c  |   31 +++++++++++++++++++++++++++++
>  flashrom.c    |    4 +++-
>  spi25.c       |   61 +++++++++++++++++++++++++++++++++++++++++++++------------
>  4 files changed, 84 insertions(+), 13 deletions(-)
>
> diff --git a/chipdrivers.h b/chipdrivers.h
> index b43e16e..63c5d37 100644
> --- a/chipdrivers.h
> +++ b/chipdrivers.h
> @@ -33,6 +33,7 @@ int probe_spi_rdid4(struct flashctx *flash);
>  int probe_spi_rems(struct flashctx *flash);
>  int probe_spi_res1(struct flashctx *flash);
>  int probe_spi_res2(struct flashctx *flash);
> +int probe_spi_automagic_res1(struct flashctx *flash);
>  int spi_write_enable(struct flashctx *flash);
>  int spi_write_disable(struct flashctx *flash);
>  int spi_block_erase_20(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
> diff --git a/flashchips.c b/flashchips.c
> index 8fe50bd..c175108 100644
> --- a/flashchips.c
> +++ b/flashchips.c
> @@ -9662,6 +9662,24 @@ const struct flashchip flashchips[] = {
>
> {
>    	.vendor		= "Generic",
> .name		= "unknown SPI chip (RES1)",
> .bustype	= BUS_SPI,
> .manufacture_id	= GENERIC_MANUF_ID,
> .model_id	= GENERIC_DEVICE_ID,
> .total_size	= 0,
> .page_size	= 256,
> .tested		= TEST_BAD_PREW,
> .probe		= probe_spi_automagic_res1,
> .probe_timing	= TIMING_ZERO,
> .block_erasers	= {},
> .unlock		= spi_disable_blockprotect,

Please kill the unlock here. AFAICS we have no way to determine how to
unlock for a REMS1 chip.
> .write		= NULL,
> .read		= spi_chip_read,
> .voltage	= {},
> },

The RES1 chip should be below the RES2 chip to make sure the RES2 chip
matches first (better match quality) and prevents RES1 matching if RES2
matches.
> {
> .vendor		= "Generic",
> .name		= "unknown SPI chip (REMS)",
> .bustype	= BUS_SPI,
> .manufacture_id	= GENERIC_MANUF_ID,
>
> @@ -9673,5 +9691,18 @@ const struct flashchip flashchips[] = {
>    	.write		= NULL,
>    },
> {
> .vendor		= "Generic",
> .name		= "unknown SPI chip (RES2)",
> .bustype	= BUS_SPI,
> .manufacture_id	= GENERIC_MANUF_ID,
> .model_id	= GENERIC_DEVICE_ID,
> .total_size	= 0,
> .page_size	= 256,
> .tested		= TEST_BAD_PREW,
> .probe		= probe_spi_res2,
> .write		= NULL,
> },
> { NULL 	}
>
> };
> diff --git a/flashrom.c b/flashrom.c
> index fdc5412..148f969 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -1028,7 +1028,9 @@ int probe_flash(struct registered_programmer *pgm, int startchip, struct flashct
>    	if (startchip == 0)
>    		break;
>    	/* Not the first flash chip detected on this bus, but not a generic match either. */
> if ((flash->chip->model_id != GENERIC_DEVICE_ID) && (flash->chip->model_id != SFDP_DEVICE_ID))
> if ((flash->chip->manufacture_id != GENERIC_MANUF_ID) &&
>     (flash->chip->model_id != GENERIC_DEVICE_ID) &&
>     (flash->chip->model_id != SFDP_DEVICE_ID))
> break;
> /* Not the first flash chip detected on this bus, and it's just a generic match. Ignore it. */
>
> notfound:
> diff --git a/spi25.c b/spi25.c
> index 914b821..7d9e948 100644
> --- a/spi25.c
> +++ b/spi25.c
> @@ -232,6 +232,7 @@ int probe_spi_rems(struct flashctx *flash)
>
> int probe_spi_res1(struct flashctx *flash)
>  {
> const struct flashchip *chip = flash->chip;
> static const unsigned char allff[] = {0xff, 0xff, 0xff};
> static const unsigned char all00[] = {0x00, 0x00, 0x00};
> unsigned char readarr[3];
>
> @@ -265,18 +266,23 @@ int probe_spi_res1(struct flashctx *flash)
>
> msg_cdbg("%s: id 0x%x\n", __func__, id2);
> if (id2 != flash->chip->model_id)
> return 0;
> if (id2 == flash->chip->model_id) {

my bad, this could have been simplified to use chip->model_id instead of
flash->chip->model_id
> /* Print the status register to tell the user about possible write protection. */
> spi_prettyprint_status_register(flash);
>
> /* Print the status register to tell the
> * user about possible write protection.
> */
> spi_prettyprint_status_register(flash);
> return 1;
> return 1;
> }
> /* Test if there is any device ID. */
> if (GENERIC_MANUF_ID == chip->manufacture_id && id2 == chip->model_id)
> return 1;
> return 0;
>
> }
>
> int probe_spi_res2(struct flashctx *flash)
>  {
> const struct flashchip *chip = flash->chip;
> unsigned char readarr[2];
> uint32_t id1, id2;
>
> @@ -289,13 +295,44 @@ int probe_spi_res2(struct flashctx *flash)
>
> msg_cdbg("%s: id1 0x%x, id2 0x%x\n", __func__, id1, id2);
> if (id1 != flash->chip->manufacture_id || id2 != flash->chip->model_id)
> if (id1 == chip->manufacture_id && id2 == chip->model_id) {
> /* Print the status register to tell the
>  * user about possible write protection.
>  */
> spi_prettyprint_status_register(flash);
> return 1;
> }
> /* Test if this is a pure vendor match. */
> if (id1 == chip->manufacture_id && GENERIC_DEVICE_ID == chip->model_id)
> return 1;
> /* Test if there is any vendor ID and if this is indeed a two-byte RES. */
> if (GENERIC_MANUF_ID == chip->manufacture_id && id1 != 0xff && id1 != id2)
> return 1;
> return 0;
>
> +}
>
> +int probe_spi_automagic_res1(struct flashctx *flash)
> +{
> unsigned char readarr[1];
> unsigned int total_size;
> uint32_t id1;

IMHO we should check here that neither RDID nor REMS nor RES2 work
before proceeding with RES1.
> if (spi_res(flash, readarr, 1))
> return 0;
> /* Print the status register to tell the
> * user about possible write protection.
> */
> spi_prettyprint_status_register(flash);
> id1 = readarr[0];
> msg_cdbg("%s: id1 0x%02x\n", __func__, id1);
> if (id1 < 0x10 || id1 > 0x17) {
> msg_cdbg("RES ID out of bounds!\n");
> return 0;
> }
> total_size = (2 << id1) / 1024;
> flash->chip->total_size = total_size;
> flash->chip->tested = TEST_OK_PR | TEST_BAD_ERASE | TEST_BAD_WRITE;
> return 1;
>
> }

I might have missed something, but right now I hope the review is complete.

Regards,
Carl-Daniel

[PATCH] Add support for generic SPI RES matching.

[Stefan Tauner]

On Fri, 31 Aug 2012 02:18:33 +0200
Carl-Daniel Hailfinger  wrote:
> I might have missed something, but right now I hope the review is complete.

you missed nothing, but the point ;)

we need to decide what we want to achieve with this patch. atm it is
neither fish nor fowl. we need to decide if we want to add erase (and
write) support in general «A» (for example by adding rather safe, common
chip-wide erasers), and by letting the user define them «B». If we want
to require any brick-like parameter before attempting any action (or
just erases/writes) «C».

i think «A» would be a good idea and justified if we include a force
parameter and appropriate warning. i do understand your concerns
regarding «B» (that if at all it would make more sense to add a way to
completely specify a chip without recompilation, and that this could
have rather unpleasant consequences (e.g wrong definitions floating
around, or less contribution towards upstream)) and agree that we
should not add such a thing to probe_spi_automagic_res1.
i can not remember if you ever answered «C» in our chats yet. imho it
makes sense for erase/write, but spi reads shouldn't be harmful... OTOH
there are the AT45DB chips that interpret a block eraser opcode as read
status register... maybe there are some that interpret 0x03 as chip
erase... who knows :)

apart from this major decisions there are also some other points, which
you have partially addressed in the review:

- .tested status field: it is probably a good idea to add our own
   warning like we do in the SFDP code, i.e. we should set it to
   TESTED_OK_PREW and think about the message content (which relies on
   the outcome of the other discussion points).

- order of flashchips.c entries: while it is true that the RES2 and
   REMS probing is of high quality regarding unique identification, the
   question is what is more beneficial to the user? i argue that the
   user has little benefit from knowing the exact IDs in comparison to
   be able to at least read it. hence i would rather move the automagic
   chip on top of the three (RES1, RES2, REMS). why not before all RDID
   chips? if RDID works, but the chip is not there yet, it should be.
   this is not true with the other methods with our current
   infrastructure.

- testing for RES2, REMS and RDID inside the automagic probe function:
   this practice is in general awful and stems from the inferior
   probing mechanism that is in place atm. due to this it makes some*
   sense in the current implementation of probe_spi_res1. it does not
   in the automagic method: either we want it to match even if the
   other generic methods (of higher quality but less use) match as
   stated above, or it does not get called at all because there was a
   match before anyway.

* actually i think it does only make sense if there are no non-generic
  chips in flashchips.c that have multiple entries to use RDID (or REMS
  or RES2) and RES1. currently there are 3 such chips of the ST M25P*
  family. btw did we ever try to contact ST/Numonyx/Micro/whoever to
  make sure the M25P05 and M25P10 really have the same RES ID?