Attention is currently required from: Felix Singer, Nico Huber, Angel Pons.
36 comments:
File Makefile:
ifeq ($(CONFIG_TUXEDO_EC), yes)
UNSUPPORTED_FEATURES += CONFIG_TUXEDO_EC=yes
else
override CONFIG_TUXEDO_EC = no
endif
Any reason why this programmer wouldn't work with libpayload? I'm not asking you to test it, just re […]
Currently there is no coreboot (and libpayload) support for Tuxedo laptops, thus it is disabled here.
Patch Set #1, Line 839: override CONFIG_TUXEDO_EC = no
hmmm, but this programmer does not use libpci?
Probably many other also don't, like MEC1308 from what I can see in the code... Probably NEED_RAW_ACCESS would be enough. What I can do to verify it? Build with TUXEDO_EC enabled only?
Patch Set #1, Line 903: # Disable internal DMI decoder to be able to locate DMI tables on EFI systems and match TUXEDO laptops
Why?
The TUXEDO laptops currenlty come with an UEFI firmware from IBV, no CSM support, so the only way to get the DMI table is to use the external decoder (checking F segment fails).
File flashrom.8.tmpl:
Patch Set #1, Line 1399: autoloading
What's autoloading?
I can only guess. Maybe it indicates whether the EC should automatically load the firmware update from flash after the operation.
Patch Set #1, Line 1411: ite5570
How would the reader of the man page know when to enable IT5570 support?
The code automatically detects ITE5570, however it can be forcefully set as in the reference implementation.
File flashrom.c:
Patch Set #1, Line 2109: #if CONFIG_TUXEDO_EC == 1
I can see why you'd want to do this, but tacking it right here feels odd.
I feel the same when I look at 40 lines back with internal programmer. Maybe we should introduce weak functions like: programmer_before_flash_access and programmer_before_image_write
?
File tuxedo_ec.c:
Patch Set #1, Line 68: typedef struct
Typedefs are frowned upon: https://www.kernel.org/doc/html/latest/process/coding-style. […]
Done
Patch Set #1, Line 184: flashrom@flashrom.org
Personally, I wouldn't know what to do with such a report on the mailing list.
It just happens when you try to flash it two consecutive times in a very short interval. Not aware of any other circumstances when it may happen. We may as well ignore it and remove the report, because simply executing the flashrom again will give the correct result.
msg_pwarn("Querying EC ROM size returned unexpected result.\n"
"Probably the EC has just been flashed and the EC "
"RAM has been reset.\nYou may need to pass the flash"
" size via the programmer parameters or try again in"
" a while.\nIf this warning persists please email a "
"report to flashrom@flashrom.org\n");
Breaking user-visible strings makes it harder to grep for them. How about: […]
Okay. Too much tied to 80 chars per line.
Patch Set #1, Line 205: ec_read_reg
Why `ec_read_reg` and not `tuxedo_ec_read_byte`?
Because reading the AC adapter state was always done with the hardcoded default register pair 0x62/0x66. Although the reference implementation allows to "change" the I/O port pair, it does not apply to all tranfers. That's just how it is...
if (!ec_read_reg(0x10, ®_value)) {
msg_perr("Failed to query first byte of state register.\n");
return false;
}
if (!ec_read_reg(0x10, ®_value)) {
msg_perr("Failed to query AC adapter state.\n");
return false;
}
Do I understand correctly that reading this register results in a side-effect? How does one know whi […]
No idea about side effect. We simply read the register twice, replaying the process from reference implementation. If the read is not done twice, it reports incorrect AC adapter state. First byte of the status register i just a guess. Maybe it is simply a register that needs a read first to fill the value and read again to report the correct status.
Patch Set #1, Line 235: len % BYTES_PER_BLOCK != 0 && len < BYTES_PER_BLOCK
Are you sure this check is supposed to use an AND? Shouldn't it be checking that the length is a mul […]
It should be multiple of BYTES_PER_BLOCK
Patch Set #1, Line 286: return tuxedo_ec_write_byte(ctx_data, data);
`break;` would be equivalent and easier to understand.
Done
Patch Set #1, Line 316: unsigned int
uint8_t ?
Done
Patch Set #1, Line 318: unsigned int
uint8_t ?
Done
Patch Set #1, Line 324: CHUNKS_PER_KBYTE * BYTES_PER_CHUNK
sizeof(ctx_data->first_kbyte)
Done
Patch Set #1, Line 361: CHUNKS_PER_KBYTE * BYTES_PER_CHUNK
sizeof(ctx_data->first_kbyte)
Done
Patch Set #1, Line 384: offset
`start + i`?
Simplified it to something very clear and minimalistic.
Patch Set #1, Line 387: offset - ctx_data->autoload_offset != 1
Can these checks ever be true? The state is reset to `AUTOLOAD_NONE` already.
Simplified it to something very clear and minimalistic.
Patch Set #1, Line 477: internal_sleep(15000 * 64);
No way to poll for completion?
Unfortunately no way that we are aware of.
Patch Set #1, Line 504: ++to_chunk
This would make flashrom erase too much?
The check below should prevent it. Anyway we do not expect to have an erase operation other than 64KiB aligned block erase or multiple 64KiB block erase, so it may be removed as well.
Patch Set #1, Line 534: legnth
typo: length
Done
Patch Set #1, Line 551: ctx_data->rom_size_in_kbytes
This is the only place where `rom_size_in_kbytes` is used. […]
Done
Patch Set #1, Line 555: * Erase operation must be done in one sway.
Doesn't seem to be the case for IT5570? The existing code doesn't allow making use of this, though.
Yes it doesn't seem, but in fact we can't make use of chunk erases to chop the update operation. If we mix read/erase/write operations, the flashing simply fails.
p = extract_programmer_param("type");
if (p && strcmp(p, "ec")) {
msg_pdbg("%s(): tuxedo_ec only supports \"ec\" type devices\n",
__func__);
ret = false;
}
free(p);
Remove this, please. […]
Done
Patch Set #1, Line 713: if (id_length == 4)
If you zero-initialise the array, this "if" can be removed.
Done
Patch Set #1, Line 722: = NULL
Dead assignment
Done
if (!dmi_match("^InfinityBook S 14 Gen6$") &&
!dmi_match("^InfinityBook S 15 Gen6$")) {
To ease adding support for more laptops in the future, put the strings in an array and loop over it?
Done
Patch Set #1, Line 750: extra
extra in which sense?
Removed to avoid confusion. Extra probably meant the programmer state data like autoload, ite5570 support, etc.
if (register_opaque_master(&programmer_opaque_tuxedo_ec, ctx_data))
return 1;
msg_pdbg("%s(): successfully initialized tuxedo_ec\n", __func__);
return 0;
Drop the print and return `register_opaque_master` directly?
Done
tuxedo_ec_init_exit:
tuxedo_ec_shutdown(ctx_data);
return 1;
tuxedo_ec_init_exit_shutdown:
tuxedo_ec_shutdown(ctx_data);
return 1;
Am I missing something, or are these two paths identical?
Indeed.
typo: not
Done
Patch Set #1, Line 826: Ifthe
missing a space
Done
If the current and the file's EC version is identical,
* return two (skip flashing)
Is there any reason to skip flashing if the EC version matches? flashrom already checks if the curre […]
I check the EC version because after the EC restarts, i.e. after `tuxedo_ec_shutdown` is called, the EC seems to reload the firmware from flash and writes a few bytes into it. I consider it pointless to flash e.g. 128K because of a few variable runtime bytes have been written by EC.
That's what I thought initially. But now when I looked at the autoload patching, there was a bug with the offset you pointed out so it incorrectly patched the file and probably resulted in such weird behavior. Removed.
Patch Set #1, Line 832: Contects
typo: Contents
Done
.name = "tuxedo_ec",
.type = OTHER,
.devs.note = "Embedded Controller of TUXEDO laptops.\n",
.init = tuxedo_ec_init,
.map_flash_region = fallback_map,
.unmap_flash_region = fallback_unmap,
.delay = internal_delay,
One tab too many in the indentation?
Done
To view, visit change 55715. To unsubscribe, or for help writing mail filters, visit settings.