Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41674 )
Change subject: soc/intel/common/block/sata: Fix SATA detection issue between Ports 3-7 ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4: -Code-Review
Patch Set 4:
> If you want to keep and fix the driver, I suggest to first > test at what stage (silicon init / notification phase?) FSP > configures things. coreboot's .final seems to run after the > PCI Enum Complete phase, but before the last two phases.
FSP does this in NotifyPhaseApi() - Begin [Phase: 00000020] Coreboot does it in Finalize devices... PCI: 00:17.0 final
This is odd, if FSP runs first and should write a write-once register, coreboot should not have any effect, right? Maybe there is a bug in your FSP?
Yes, looks like in my case FSP doesn't even programming those register in notify phase and coreboot is doing wrong 0-2 bit enabling results into SATA device not detected over Port 4. Hence I don't want to much dependent on FSP for such programming at first place and make sure coreboot doing things right if such condition do arise in future.
I don't want to rely on FSP either. But it's there. And Nate once commented that the point of using Intel's blobs is to have a single upstream where bug fixes are gathered so every Intel customer can get them. If we cover FSP bugs up, we defeat its purpose. Don't worry, I don't mind much. If I'd ever do a port for a new Intel platform, I would not use FSP-S anyway. It causes too much trouble.
If different FSP binaries do different things with these registers and then we have to do different things in coreboot, maybe we should make this platform-specific.
AFAICT cnl and kbl don't differ in this regard and they both do the "right thing" except that disabling ports that are non-hot-plug and have no device present is not very nice behaviour...
It might be a bug inside FSP which will fix that not the concern here, but my concern is if FSP misses such that doesn't mean coreboot will perform wrong programming hence the invention was to make common code right to handle PCH-H SATA controller as well. This driver is doing thing right as per register it just because those registers are RW/O hence 2nd time write doesn't take any effect.
I don't see a real bug in FSP - this seems to be wanted behaviour from the devs...
@Nico, what should be our direction here, fix this register programming issue or remove those finalize code from here. I'm with first one.
I guess we can't do anything about that bad FSP behaviour. A workaround would be setting hotplug for all ports that have no device present - then FSP won't disabled them - but I don't really like that tbh :S