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

[David Hendricks]

As Stefan points out, some decisions were made in ChromiumOS to avoid
modifying generic code and (in theory) make upstreaming easier. The
writeprotect stuff is the main example. We can re-visit some of those
decisions and put some code in generic locations if it makes sense to do so.

I don't like the idea of using a FEATURE_SR2 bit - It doesn't describe the
functionality it represents in a useful manner IMO. As you point out,
reading SR2 needlessly can slow things down, especially for the common use
case where we only really care about reading the busy bit. For the write
protection use case, the exact layout of the status registers needs to be
taken into account, not just whether SR2 is present or not since SR2 is not
as well standardized. To make matters more complicated, Gigadevice GD25Q128
even has a 3rd status register which has a bit that defines the entire
write-protection scheme. Also, some chips such as Spansion S25FS and S25FL
series don't use status registers in the same manner as other chips and
instead define a set of control registers with special opcodes to
read/write them.

Also, as Stefan points out, the status register bits are relevant to 4B
addressing and quad I/O in addition to write protection. The implementation
details are non-uniform.

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/

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

On Tue, May 24, 2016 at 2:07 AM, Stefan Tauner <
stefan.tauner at alumni.tuwien.ac.at> wrote:

> On Tue, 24 May 2016 12:49:51 +0530
> Hatim Kanchwala <hatim at hatimak.me> wrote:
>
> > Hi,
> >
> > The GSoC coding period has started and I have been working on my first
> > sub-project, adding support for 2nd status register. Most supported
> > chips that have multiple registers (around 80%) have opcode 0x35 for
> > reading SR2 and, opcode 0x01 for writing SR2. I have developed 3 models
> > and would like to have some feedback on them.
> >
> > 1. Integrate read/write SR2 with existing functions
> > PROPOSAL -
> > - Define a feature bit FEATURE_SR2
> > - spi_read_status_register(), defined in spi25_statusreg.c, additionally
> > reads second status register depending on FEATURE_SR2 bit for that flash
> > - Change return to uint16_t for spi_read_status_register()
> > - spi_write_status_register_flag(), defined in spi25_statusreg.c,
> > considers higher byte of status to write to SR2, depending on
> > FEATURE_SR2 bit for that flash.
> > MERITS -
> > - Fits in elegantly with existing implementation (IMO). Most code in
> > flashrom stores status reads/writes in int, which can easily accommodate
> > the 16 bits
> > - Very little hassle when editing struct flashchip in flashchips.c
> >
> > DEMERITS -
> > - RDSR will take more time - will read SR2 even if not needed
> >
> > 2. Define separate functions to read/write SR2
> > PROPOSAL -
> > - Define spi_read_status_register2() to read only SR2
> > - Define spi_write_status_register2_flag() and
> > spi_write_status_register2() to write to SR2. This will read SR1 first,
> > then (SR2 << 8 | SR1) will be written using WRSR
> >
> > MERITS -
> > - Flexibility (as compared to 1.)
> >
> > DEMERITS -
> > - Need to write more lines (compared to 1.) when dealing with struct
> > flashchip
> >
> > 3. struct status_register_layout
> > The ChromiumOS fork defines a struct status_register_layout in
> > writeprotect.c, which contains only BP and SRP bits' information.
> > PROPOSAL -
> > Do a similar flashrom-wide definition, which contains all status
> > register bits' information.
> >
> > MERITS -
> > - Very detailed representation of information
> >
> > DEMERITS -
> > - Too much work when adding support for new chips
> > - Overhaul of existing infrastructure
> >
> > IMHO, the first model is the best among these 3. I would like to know
> > what you guys think about these, whether you have some new idea. Looking
> > forward to your feedback. Thanks for your time.
>
> Hi,
>
> thanks Hatim for this summary. It may make sense if David could sum up
> the rationales behind the chromiumos implementation. AFAIK much of that
> is driven by the desire to not touch flashchips.c to ease later
> upstreaming. A more detailed explanation might help Hatim.
>
> What I miss from Hatim's proposals is how use cases affect the various
> implementations. The main question that needs to be addressed before
> implementing any kind of infrastructure like this is: what do we want
> to do with it once it is there.
>
> Till now (vanilla) flashrom did only access the status registers for
> gathering (and printing) the status, especially the write protection
> bits, and for unlocking said bits.
> In the future we want to at least be able to set various protections
> additionally. What else do we need? Some bits are relevant to 4B
> addressing and Quad I/O too for example...
>
> --
> Kind regards/Mit freundlichen Grüßen, Stefan Tauner
>
> _______________________________________________
> flashrom mailing list
> flashrom at flashrom.org
> https://www.flashrom.org/mailman/listinfo/flashrom
>



-- 
David Hendricks (dhendrix)
Systems Software Engineer, Google Inc.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20160524/7a4516c2/attachment.html>