Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45794 )
Change subject: soc/intel/common/block/pmc: Add PMC API for low power programming ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45794/6/src/soc/intel/common/block/... File src/soc/intel/common/block/pmc/Kconfig:
https://review.coreboot.org/c/coreboot/+/45794/6/src/soc/intel/common/block/... PS6, Line 36: Enable this for PMC devices to perform registers programming : to ensure low power in active idle scenario. :
if FSP fails to do so, coreboot still has means to handle it. i meant for early SoC platform with newer FSP
Sorry to jump in here so rudely, but I have to say this:
Expecting FSP to fail is no excuse to make a mess in coreboot.
If you want a library of things coreboot could do "for early SoC platform with newer FSP", please do it outside of the production code. It's ok to have that in the coreboot tree, IMO, but why clutter all of coreboot just to be prepared for a new, faulty, FSP? It probably costs the coreboot community hundreds of thousands of dollars (plus the same in spare time of volunteers) to maintain this mess.
And the XTAL topic is a huge mess: The commit that introduced it is not clear about the effects of the change (e.g. it tested things after the commit, but doesn't mention what was before). We have in the past weeks spend days(!), accumulated, to figure out the details just to eventually find out that the bit is always set by FSP unconditionally. Such things need to be documented!!!! please start with the explanation why the bit (XTALSDQDIS) doesn't default to 1 after reset. Why does it exist? What are the potential downside of setting it?
Also, why does the bit need options in coreboot if it can be set uncon- ditionally?
If Intel doesn't have the resources for proper documentation, please keep coreboot as small and dull as possible! No unnecessary options, please, no unnecessary `if`, especially no `#if`.