Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40877 )
Change subject: soc/intel/skl: always enable SataPwrOptEnable ......................................................................
Uploaded patch set 4.
(9 comments)
https://review.coreboot.org/c/coreboot/+/40877/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40877/3//COMMIT_MSG@11 PS3, Line 11: This leads to all sorts of problems and errors, for example: : - links get lost : - only 1.5 or 3 Gbps instead of 6 Gbps : - "Unaligned Write" errors in Linux : - ...
You told me this only happens when you enable power management in SATA (e.g. […]
because it's not salp related as we already discussed on IRC. librem13v2 for example has salp=0 and the same errors show.
https://review.coreboot.org/c/coreboot/+/40877/3//COMMIT_MSG@22 PS3, Line 22: provide an option : for breaking coreboot
That is not its intended purpose, yet your wording makes it sound as it were. I would suggest a less charged wording:
A bit of irony does not hurt ;) and it makes it absolutely clear that it is a really bad idea to set it to 0. But if you insist...
Its actual purpose would be to disable power management stuff for debugging purposes.
Proof?
https://review.coreboot.org/c/coreboot/+/40877/3//COMMIT_MSG@24 PS3, Line 24: : Surprisingly cml and cnl are not affected, even though they share mostly : the same reference code in this regard.
That nobody reported any problems doesn't mean that it's not affected. […]
Ack
https://review.coreboot.org/c/coreboot/+/40877/3//COMMIT_MSG@26 PS3, Line 26: Thus, only skl gets changed.
A more compelling reason to only change SKL for now is because it has been tested and works: as per […]
Done
https://review.coreboot.org/c/coreboot/+/40877/3/src/soc/intel/skylake/chip.... File src/soc/intel/skylake/chip.c:
https://review.coreboot.org/c/coreboot/+/40877/3/src/soc/intel/skylake/chip.... PS3, Line 264: * For unknown reasons FSP skips SATA init steps, resulting in (link) errors, unaligned write
Please fix
you don't need to comment what's already commented...
https://review.coreboot.org/c/coreboot/+/40877/3/src/soc/intel/skylake/chip.... PS3, Line 264: For unknown reasons FSP skips SATA init steps
Does it always skip those SATA init steps? If not, when does it skip such steps? The current wording […]
Done
https://review.coreboot.org/c/coreboot/+/40877/3/src/soc/intel/skylake/chip.... PS3, Line 264: (link)
Why the parentheses?
because there are not only link errors
https://review.coreboot.org/c/coreboot/+/40877/3/src/soc/intel/skylake/chip.... PS3, Line 264: unaligned write
What's an unaligned write error?
This is explained in the commit message and it should be pretty clear what is meant here...
https://review.coreboot.org/c/coreboot/+/40877/3/src/soc/intel/skylake/chip.... PS3, Line 265: the SATA SIR
Only one SATA SIR?
*sigh*