On Wednesday 25 May 2016 02:26 AM, David Hendricks wrote:
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.
Thanks for the explanation. Highlighting use cases was helpful in understanding. I shall try to formulate future models based on clear use cases to convey more effectively.
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.
Agreed. This was what I had in mind when I mentioned it.
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.
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.
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.
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).
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.
On Tue, May 24, 2016 at 2:07 AM, Stefan Tauner <stefan.tauner@alumni.tuwien.ac.at mailto:stefan.tauner@alumni.tuwien.ac.at> wrote:
On Tue, 24 May 2016 12:49:51 +0530 Hatim Kanchwala <hatim@hatimak.me <mailto:hatim@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@flashrom.org <mailto:flashrom@flashrom.org> https://www.flashrom.org/mailman/listinfo/flashrom-- David Hendricks (dhendrix) Systems Software Engineer, Google Inc.
Thanks for your time.
Bye, Hatim
On Thu, 26 May 2016 07:41:47 +0530 Hatim Kanchwala hatim@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.
On Thursday 26 May 2016 02:09 PM, Stefan Tauner wrote:
On Thu, 26 May 2016 07:41:47 +0530 Hatim Kanchwala hatim@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
On Fri, 27 May 2016 03:12:22 +0530 Hatim Kanchwala hatim@hatimak.me wrote:
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.
It might make sense to have all the status register-related function pointers in the model structs as well. If the status registers are alike then the functions are probably as well.
One disadvantage of your current approach might be that the status register pretty prints will be less expressive (in case we use your print_status_register function exclusively and get rid of all the various customized functions). Some of them are pretty complicated to be as generic as possible and convey way more information than a simple "is (not) set" semantic, e.g. spi_prettyprint_status_register_at45db. However, combining your approach with the existing expressiveness may lead to something quite complicated... I'll let you try to come up with something useful :)
Hi Hatim, Interesting approach. It seems to work well for pretty printing, though I am curious how this will translate into ranges. Do you have an example for translating status_register_layout structs to a range of bytes protected, for example 0x000000-0x1fffff?
-
David Hendricks -
Hatim Kanchwala -
Stefan Tauner