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.

Yes, no need to argue here. The original driver solved a problem.
But it was only intended for SKL/KBL U/Y. And all later patches
were wrong.

• 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.

You are mixing things up. The register bits written in CB:17229
still exist and are *not* R/WO. Only their location changed.

> * 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.

Because you are wrong. Have you looked into all datasheets for
the given IDs? I have.

Since gen5 all SATA chipset register are same except SPD register value been inverted which already taken care.

That's not true. SPD bits always acted the way they do on current chips.
The D is for disable btw. and these bits were introduced with ICH9. Again
only the register layout changed. Which made this driver write SPD bits
*by accident*.

> * 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

Well, I have approached Intel about this 4 years ago. It doesn't help.

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?

I don't think it's worth to make any plans as long as the developer
behind this driver doesn't read the datasheets correctly.

So, lets get this driver drop now ? we do have any objection from SKL/KBL users still ?

View Change

To view, visit change 41674. To unsubscribe, or for help writing mail filters, visit settings.