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

Hatim Kanchwala hatim at hatimak.me
Thu May 26 23:42:22 CEST 2016


On Thursday 26 May 2016 02:09 PM, Stefan Tauner wrote:
> 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...
>
So I incorporated the changes you suggested. Please have a look at the 
updated attached file (output is here 
http://paste.flashrom.org/view.php?id=2919). IMO, the flashchip struct 
abstracts the characteristics or behaviour of the flashchip and not its 
state (like status register bits). For that reason, I have removed the 
array of int. The struct not has pointers to struct 
status_register_layout, like you pointed out. This is indeed a much more 
elegant solution as many chips will have a lot in common. It was stupid 
of me to go with the previous model. The two function pointers, 
read_status and write_status, will also be modeled like the existing 
printlock/prettyprint functions. A good change in the implementation due 
to these changes was that I no longer required that INVALID bit.

If most of you agree with this design, I would like to bring it out of 
prototype stage and start developing it.

>>> 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.
>
I have started learning this. Thanks for pointing out :)

>> 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.
>

Thanks for your time.

Bye,
Hatim
-------------- next part --------------
A non-text attachment was scrubbed...
Name: array_of_struct.c
Type: text/x-csrc
Size: 3179 bytes
Desc: not available
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20160527/095af128/attachment.c>


More information about the flashrom mailing list