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

[Stefan Tauner]

On Fri, 31 Aug 2012 02:18:33 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> 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?
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner