Attention is currently required from: Felix Singer, Michał Żygowski, Paul Menzel, Sergii Dmytruk.
30 comments:
Patchset:
Splitting an old comment thread, might make things easier to follow.
Now I probe only for ITE Super I/Os. Just added more model IDs in the switch to catch the supported chips. Redesigning Super I/O code needs more thought and I won't pursue it right now. DMI checks have been replaced with PCI ID and SSID checks to not depend on any dmidecoder (which may fail depending on UEFI vs legacy).
I don't follow, can you elaborate when DMI checks would fail?
File flashrom.8.tmpl:
Patch Set #29, Line 1423: .B " flashrom \-p ite_ecfw:portpair=pairnum"
Isn't this board specific? If so the information should come from
board_enable or similar mechanism.
Patch Set #29, Line 1449: The default value is queried from EC.
When is it needed then?
Patch Set #29, Line 1471: .B " flashrom \-p ite_ecfw:boardmismatch=force"
I don't think flashrom supports such a force yet nor that it should.
However, for the internal programmer we allow to override the matched
board. (Things would be much easier if you'd use the existing infrastructure.)
File it87spi.c:
Patch Set #29, Line 105: case 0x89:
How does this affect the internal programmer? Would be nice to split this
out. It probably will take me hours to review.
File ite_ecfw.c:
Patch Set #29, Line 22: #if defined(__i386__) || defined(__x86_64__)
Please drop, the Makefile should decide when to compile things.
Patch Set #29, Line 94: } __attribute__((packed));
It's one byte AFAICS, so nothing to pack.
Patch Set #29, Line 131: static int force = 0;
No globals, please.
Patch Set #29, Line 133: uint8_t *buf
We usually put output parameters first, like in standard C APIs.
Patch Set #29, Line 173: uint8_t i;
You can just use an `int` or `unsigned int`. Using `uint8_t` probably just
makes the compiler's job a little harder.
Please make sure `ec_project` gets 0 terminated, no matter what the EC replies.
Same as above.
Aside from the command these two functions look the same, any reason to repeat
the code?
Patch Set #29, Line 231: "You may need to pass the flash size via the programmer parameters or simply try again in a while.\n");
Please break this line around 70 chars (of text).
Patch Set #29, Line 247: /* flush the EC registers */
Is that even possible for the status register?
Patch Set #29, Line 268: unsigned int start, unsigned int len)
Please align (I guess the intention was to, as there's already a mix of
tabs and spaces).
Patch Set #29, Line 368: unsigned int block)
Please align.
Patch Set #29, Line 392: CHUNKS_PER_KBYTE * BYTES_PER_CHUNK
Isn't this just `1*KiB`?
No parentheses needed btw.
Missing space.
Patch Set #29, Line 413: memset(ctx_data->first_kbyte, 0, sizeof(ctx_data->first_kbyte));
What for?
Spurious space.
ctx_data->ite_string_offset >= start &&
ctx_data->ite_string_offset <= (start + len - sizeof(struct ite_string))
Shouldn't this check `start == 0` instead? How would ite_ecfw_patch_autoload()
know if there is an offset?
What about ite_ecfw_chunkwise_erase() for ITE5570?
Are the IDs from RDID? I guess we should also check that the database entry
assumes that.
Patch Set #29, Line 717: id_length = 4;
Hmmm, if smaller chips have a smaller id, maybe it's not RDID after all.
Patch Set #29, Line 758: msg_perr
Not an error?
if (superio_count == 0)
return;
Really not necessary, the next line checks about the same.
Patch Set #29, Line 812: Probing
Probing might be the wrong word. AFAICS, the code just starts writing to EC RAM.
Patch Set #29, Line 813: "backlight failure and sudden poweroff.\n"
I assume this is copied from the text for the flash probing on legacy buses?
I think it's much different as that would not write to EC registers. In this
case, I guess, even severe things can happen, e.g. need to reflash the EC
firmware, or even permanent damage.
I don't think we want a force option for this.
Patch Set #29, Line 905: int ite_ecfw_verify_file_project(uint8_t *const newcontents,
Is it still call somewhere?
To view, visit change 55715. To unsubscribe, or for help writing mail filters, visit settings.