Attention is currently required from: Michał Żygowski, Paul Menzel. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55578 )
Change subject: Add Tiger Lake U Premium support ......................................................................
Patch Set 10: Code-Review+1
(8 comments)
Patchset:
PS10: Thanks for the update and ping. I think this change didn't get the attention it deserves because it's sitting amidst an unrelated patch train with status "Merge Conflict".
It looks almost ready, so I think it's best to rebase it on master directly. One small cosmetic issue (`lp` suffix) and I think adding `BUS_ESPI` is raising unnecessary questions right now, so I'd just put a `0` where used it ;)
File chipset_enable.c:
https://review.coreboot.org/c/flashrom/+/55578/comment/e1b425b6_c00f58d2 PS3, Line 659: { "eSPI", BUS_LPC | BUS_FWH } };
Ping
I still wonder if eSPI is a bus for flashes at all? See comments on the enum change.
Probably easiest to just put a 0 here. The value is only processed by the calling function and there is nothing to be done for eSPI atm.
File chipset_enable.c:
https://review.coreboot.org/c/flashrom/+/55578/comment/a013a974_92bd3938 PS10, Line 657: _lp The `lp` suffix was previously used when there was a low-power version with different behavior. I don't think this is the case with TGP?
File flash.h:
https://review.coreboot.org/c/flashrom/+/55578/comment/dcee4975_fa188165 PS10, Line 70: BUS_ESPI = 1 << 5, Checking how all paths react to this change takes some time and should eventually be reviewed as a separate commit. I had a quick look and it seems BUS_NONSPI (see below) might be the only incon- sistency. I guess if you try to fix that some unit-test will break and fixing that might lead to the question if flashbuses_to_text() should be updated, etc...
So I wonder if it's worth to add this before we actually act on the new value anywhere. Coming back to the question if there are flash chips for eSPI.
https://review.coreboot.org/c/flashrom/+/55578/comment/cffd06c6_8268ae6d PS10, Line 71: BUS_NONSPI = BUS_PARALLEL | BUS_LPC | BUS_FWH, This would definitely need an update. Although, I'm not sure if it is consistently used throughout flashrom code.
File ich_descriptors.c:
https://review.coreboot.org/c/flashrom/+/55578/comment/d83477ee_84f6d4e3 PS5, Line 278: return "reserved";
Kindly asking to check if it does look good now
Ack
Thanks for taking fully care of `freq_read`.
https://review.coreboot.org/c/flashrom/+/55578/comment/eddd80d8_3cfef3a9 PS5, Line 967: return CHIPSET_500_SERIES_TIGER_POINT;
Please check
Done
File ich_descriptors.c:
https://review.coreboot.org/c/flashrom/+/55578/comment/d0469aac_db77eedd PS10, Line 316: msg_pdbg2("Read Clock Frequency: %s\n", "reserved"); Nit, I think we can just skip printing anything here.
-
Nico Huber (Code Review)