Attention is currently required from: Nico Huber, Subrata Banik, Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63584 )
Change subject: ichspi: Add support for Ice Lake ......................................................................
Patch Set 1:
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/63584/comment/8b531d74_23c13e66 PS1, Line 7: ichspi: Add support for Ice Lake
As discussed during review of the first of these patches, adding all […]
Not sure what you mean by "undone" as those add legitimate new chipset support, seems like hyperbole. I believe you mean to say there should be some refactors happening to help consolidate so many enum branches. As you pointed out Nico, you tried a number of times in the past and didn't manage to trivially do it.
In any case, I actually did upload a start towards making progress in that direction in CB:63321 but it has been sitting unreviewed for a few weeks at the moment. I believe the theme of that patch can be applied to a few other places towards a direction of having most of the chipset static details in a lookup table and a switch.
More careful review of SPI Programmer Guides between generations is required to find better common patterns.
File chipset_enable.c:
https://review.coreboot.org/c/flashrom/+/63584/comment/c687b8e2_19acc666 PS1, Line 998: if (strcmp(name, "Ice Lake") == 0) : return enable_flash_pch100_or_c620(dev, name, 0x1f, 5, CHIPSET_400_SERIES_ICE_POINT); : else : return enable_flash_pch100_or_c620(dev, name, 0x1f, 5, CHIPSET_400_SERIES_COMET_POINT); This feels wrong to have a strcmp(). I do not have a copy of IceLake/IcePoint SPI Programming Guide to figure out if there is any difference, if you have a copy Subrata can we look together and figure out a strategy?
If this patch is right to say there is no difference then can we rename `CHIPSET_400_SERIES_COMET_POINT` to `CHIPSET_400_SERIES_COMET_ICE_POINT` Maybe?
https://review.coreboot.org/c/flashrom/+/63584/comment/68caca04_44159b7d PS1, Line 2149: 0x34a4
But can it be hidden? Maybe not everybody have updated their code […]
Subrata, flashrom can be used in "odd" situations when folks are porting coreboot to boards in the community. I think this is what Nico is alluding to here as to make flashrom as robust as possible to allow for flashing.
Should DID's in the table used for detection of hidden situations perhaps have " (hidden)" suffix and a more detailed code comment explaining so we can avoid confusion? Feel like this is it's own patch.