Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/55651 )
Change subject: ich_descriptors: Normalize chipset detection ......................................................................
Patch Set 5:
(3 comments)
Patchset:
PS5:
This patch added an undocumented assumption that Ibex Peak will have the most straps in the `ICCRIBA == 0x00` case, which we now know is not true. I've fixed that assumption in CB:57633.
No, not accurately. It makes this assumption, but only for the platforms that are already supported.
The other change in this patch looks suspicious as well. Perhaps we should just revert this whole patch?
The purpose of this patch was to make it easier to add new platforms without much hassle whilst keeping the exact behavior (return-value-wise) for all already known platforms. If you don't want that, please try to go ahead.
File ich_descriptors.c:
https://review.coreboot.org/c/flashrom/+/55651/comment/6a3211df_c5ddcfd4 PS5, Line 936: if (content->ISL <= 16) : return CHIPSET_5_SERIES_IBEX_PEAK;
Removing this adds an implicit assumption that chipsets added later on will not have more straps tha […]
Please give some number example, I don't follow.
https://review.coreboot.org/c/flashrom/+/55651/comment/b3910e08_3ce338a0 PS5, Line 953: return CHIPSET_8_SERIES_LYNX_POINT; : warn_peculiar_desc(true, "Wildcat Point"); : return CHIPSET_9_SERIES_WILDCAT_POINT;
The commit message says "Always end with the newest, assumed compatible chipset. […]
We don't know any difference in the descriptor between these two. So they should be 100% compatible. We could also just always return 9 series. This is what the last point of the commit message is about.
-
Nico Huber (Code Review)