On Thursday 26 May 2016 02:09 PM, Stefan Tauner wrote:
On Thu, 26 May 2016 07:41:47 +0530
Hatim Kanchwala <[email protected]> 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
#include <stdio.h>
#include <stdint.h>
enum status_register_bit {
RESV, WIP, WEL, SRP0, SRP1, BP0, BP1, BP2, BP3, BP4, QE,
TB, CMP, LB3, LB2, LB1, WPS, SUS1, SUS2, DRV0, DRV1, RST
};
const char *status_register_bit_name[] = {
"RESV", "WIP", "WEL", "SRP0", "SRP1", "BP0", "BP1", "BP2", "BP3", "BP4", "QE",
"TB", "CMP", "LB3", "LB2", "LB1", "WPS", "SUS1", "SUS2", "DRV0", "DRV1", "RST"
};
enum status_register_num { SR1, SR2, SR3 };
struct status_register_layout {
enum status_register_bit bits[8];
};
struct flashchip {
const char *name;
const struct status_register_layout *status_register[4];
uint8_t (*read_status)(struct flashchip *flash, enum status_register_num);
int (*write_status)(struct flashchip *flash, int status);
};
int num_status_register(struct flashchip *flash) {
int num = 0;
struct status_register_layout **status = flash->status_register;
while (*(status + num++) != NULL)
;
return --num;
}
int print_status_register(struct flashchip *flash, enum status_register_num sr) {
if (flash->status_register[sr] == NULL) {
printf("%s does not have SR%d\n", flash->name, sr + 1);
return -1;
}
uint8_t status_sr = flash->read_status(flash, sr);
printf("SR%d: \n", sr + 1);
for (int j = 0; j < 8; j++) {
printf("%s is %sset\n", \
status_register_bit_name[flash->status_register[sr]->bits[j]], \
(status_sr & (1 << j)) ? "" : "not ");
}
return 0;
}
int bp_bitfield(struct flashchip *flash) {
int mask = 0, i = 0, tmp;
while (flash->status_register[SR1]->bits[i] != BP0)
i++;
mask = 1 << i;
tmp = flash->status_register[SR1]->bits[++i];
while (tmp > BP0 & tmp <= BP4) {
mask = mask | (1 << (i));
tmp = flash->status_register[SR1]->bits[++i];
}
return mask;
}
uint8_t read_status_dummy(struct flashchip *flash, enum status_register_num sr) {
return 0xA8;
}
int main(int argc, const char **argv) {
/* Common SR1 layouts */
struct status_register_layout sr1_bp4 = { WIP, WEL, BP0, BP1, BP2, BP3, BP4, SRP0 };
struct status_register_layout sr1_bp3 = { WIP, WEL, BP0, BP1, BP2, BP3, RESV, SRP0 };
struct status_register_layout sr1_bp2 = { WIP, WEL, BP0, BP1, BP2, RESV, RESV, SRP0 };
/* Common SR2 layouts */
struct status_register_layout sr2_qe_lb3 = (struct status_register_layout){ SRP1, QE, SUS2, LB1, LB2, LB3, CMP, SUS1 };
/* Common SR3 layouts */
struct status_register_layout sr3_wps = (struct status_register_layout){ RESV, RESV, WPS, RESV, RESV, DRV0, DRV1, RST };
struct flashchip flash_1 = {
.name = "SPI25_1",
.status_register = {
[SR1] = &sr1_bp4,
[SR2] = &sr2_qe_lb3,
[SR3] = &sr3_wps,
},
.read_status = read_status_dummy,
};
struct flashchip flash_2 = {
.name = "SPI25_2",
.status_register = { &sr1_bp3, &sr2_qe_lb3 },
.read_status = read_status_dummy,
};
printf("num_status_register(flash_1) = %d\n", num_status_register(&flash_1));
printf("num_status_register(flash_2) = %d\n", num_status_register(&flash_2));
print_status_register(&flash_1, SR1);
print_status_register(&flash_1, SR2);
print_status_register(&flash_2, SR3);
printf("BP mask = 0x%02x\n", bp_bitfield(&flash_1));
printf("BP mask = 0x%02x\n", bp_bitfield(&flash_2));
return 0;
}
_______________________________________________
flashrom mailing list
[email protected]
https://www.flashrom.org/mailman/listinfo/flashrom
