[flashrom] Ideas for implementing support for 2nd status register

[Stefan Tauner]

On Thu, 26 May 2016 07:41:47 +0530
Hatim Kanchwala <hatim at hatimak.me> wrote:

> On Wednesday 25 May 2016 02:26 AM, David Hendricks wrote:
>
> I looked at the datasheets of the 28 SPI chips supported in flashrom 
> that have multiple status registers. 24 of them have 35h opcode for 
> RDSR2. Atmel(1) and Spansion(2) are complicated. And so is GD25Q128C.

There are ALWAYS special cases that make really slick, generic solutions
impossible, sadly. We need to pay attention to them from the beginning.

> > Consequently, I think we need a combination of #2 and #3. For #2, we'll
> > have functions which read the status registers (but must be careful
> > since reading SR2 and SR3 aren't standardized AFAIK). For #3 we can
> > describe the functionality we desire in a reasonably generic way and add
> > chip-specific helper functions to carry out the task regardless of where
> > the bits we are interested in reside. I started going in this direction
> > for ChromiumOS's writeprotect.c, but it's still a work in progress:
> > https://chromium-review.googlesource.com/#/c/208271/
> > https://chromium-review.googlesource.com/#/c/259440/
> > https://chromium-review.googlesource.com/#/c/335822/
> > https://chromium-review.googlesource.com/#/c/335823/
> >  
> So, based on the Chromium OS implementation (along with these patches, 2 
> of which I had to merge locally), this is the revised model. I agree 
> that a combination of #2 and #3 will offer flexibility and can 
> definitely convey more information.
> - enum status_register_bits enumerates all possible bits that we have
> - array of struct status_register_layout as part of struct flashchip
> - each array represents a status register and each element of array 
> represents the bit (using the enum)
> - (*read_status)() within struct flashchip taking as argument which 
> status register to read (SR1 or SR2 or SR3)
> - (*write_status)() within struct flashchip
> In addition to the above basic elements, we can have
> - const char *status_register_bit_name[] that has string-ified 
> representation of corresponding bit from enum
> - array of int status_bit indexed by enum status_register_bits which is 
> populated when corresponding read_status() is called
> 
> To better convey the model, I have implemented some prototype code. 
> Please have a look at the attached file. And have a look here 
> (http://paste.flashrom.org/view.php?id=2918) for the output. Please 
> ignore the violation of 112-character limit in the attached file.
> 
> Please let me know your feedback on the model and on the 
> proof-of-concept implementation. I would also love to hear 
> suggestions/advice on the code style and quality.

This is just a detail, but since the pattern might come up more often
I'll explain it now anyway:
I personally would rather use 0 for RESV (and mark the end by
INVALID=-1, or INVALID=<last> as we do in programmer.h (PROGRAMMER_INVALID)).
In any case, the string array should naturally match the enums, i.e.
the enums need to have a useful value at 0 so that you don't need to
init it as complicated as you do with status_register_bit_name).

I have not looked at chromium's patches but I think we should not
over-engineer it either/be aware of the consequences. Your proposed
approach seems to increase the compiled size of flashrom quite
dramatically due to the grow of the flashchips array. Some growing is
inevitable but storing an integer for every bit in a status register
in every chip... I don't know if that's worth it. AFAICT we could just
as easily provide status register models that are equal to
your .status_register and just use pointers to them like we do with the
pretty print function pointer. If there are not too many different
models, this would reduce the size dramatically without changing the
code much (but still with some added complexity which might be the
opposite of not over-engineering :)

OTOH: floppies are completely dead so why bother with size
restrictions...

> > tl;dr version: Overall I think we should just do the work of
> > representing the status register bits in a generic way, as you describe
> > in #3. It will be tedious at first, but many chips will be able to share
> > the same accessor functions. It's very important to be flexible so that
> > we don't end up with "square peg in a round hole" over-generalizing and
> > relying too heavily on if-statements/switch statements (a mistake I made
> > in the chromiumos sources).
> >  
> I think a solution to making it less tedious would be to write a script 
> to do as much of the modification as possible, and then manually deal 
> with outliers. Based on comments in flashchips.c, we have 28 chips with 
> more than one status register out of a total of 293 SPI chips (revisions 
> that share definition are not considered different).

Coccinelle.

> Intuitively I think it best to roll out this feature in phases such that 
> until the final phase, current (vanilla) flashrom behaviour exists in 
> parallel.

Well, there will two branches in git - one for staging new patches, one
for merging them into a stable line when they are trusted - exactly for
that reason.
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner