Subrata Banik 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:
@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 would just remove the whole driver. Looking at its history:
- It was added in a broken state. The issue it reportedly fixed can only mean that FSP left the AHCI Ports Implemented register in an inconsistent state (or maybe just unitialized, but this driver didn't initialize it either).
[Subrata] Without this driver, patching such FSP issue would have impossible and we need to wait for FSP fix which might be timely affair in some case when it need the most.
- It was most likely added to help with a broken FSP1.1. Which we don't use anymore.
[Subrata] Earlier FSP was not programming those chipset register during POST hence we were seeing those issue. original CL https://review.coreboot.org/c/coreboot/+/17229/ Lately FSP looks like has started taking care of SATA register hence this driver lost its purpose due to RW/O enforcement.
- It was initially written for SKL and KBL U/Y. Still other, incompatible PCI IDs were added which resulted in wild patching. And it still writes a different register for all other platforms than originally intended.
[Subrata] Not sure why you have repeatedly telling about incompatible PCI ID. Since gen5 all SATA chipset register are same except SPD register value been inverted which already taken care.
- It needed a lot of maintenance, probably sucked thousands of dollars out of the coreboot community and still continues to. But it most likely doesn't provide any value since FSP2.0.
- Doing things in FSP and coreboot redundantly can increase complexity and costs by a power of 2.
[Subrata] Agree, but there is no such good document to really callout what is redundant between those 2
Can we take a step approach here. Any suggestion here ?
1. Let this CL fix the SATA driver issue and unblock booting from SATA when FSP missed to program those register and its remain at default state. 2. We shall remove latest SOC SATA ID (TGL and JSL) from this driver to ensure latest IA doesn't use this as FSP already do the needful. I don't want to disturb other SOC by removing entire driver at first place 3. From 1 generation from now (from coreboot next version release) we again evaluate the need of this driver and remove completely?