On 8/13/19 6:07 PM, Dan Williams wrote:
> On Tue, Aug 13, 2019 at 12:31 AM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
>>
>> On Mon, Aug 12, 2019 at 12:31:35PM -0700, Dan Williams wrote:
>>> It seems platforms / controllers that fail to run the option-rom
>>> should be quirked by device-id, but the PCS register twiddling be
>>> removed for everyone else. "Card BIOS" to me implies devices with an
>>> Option-ROM BAR which I don't think modern devices have, so that might
>>> be a simple way to try to phase out this quirk going forward without
>>> regressing working setups that might be relying on this.
>>>
>>> Then again the driver is already depending on the number of enabled
>>> ports to be reliable before PCS is written, and the current driver
>>> does not attempt to enable ports that were not enabled previously.
>>> That tells me that if the PCS quirk ever mattered it would have
>>> already regressed when the driver switched from blindly writing 0xf to
>>> only setting the bits that were already set in ->port_map.
>>
>> But how do we find that out?
>
> We can layer another assumption on top of Tejun's assumptions from
> commit 49f290903935 "ahci: update PCS programming". The kernel
> community has not received any regression reports from that change
> which says:
>
> "
> port_map is determined from PORTS_IMPL PCI register which is
> implemented as write or write-once register. If the register isn't
> programmed, ahci automatically generates it from number of ports,
> which is good enough for PCS programming. ICH6/7M are probably the
> only ones where non-contiguous enable bits are necessary && PORTS_IMPL
> isn't programmed properly but they're proven to work reliably with 0xf
> anyway.
> "
>
> So the potential options I see are:
>
> 1/ Keep the current scheme, but limit it to cases where PORTS_IMPL is
> less than 8 and assume this need to set the bits is unnecessary legacy
> to carry forward
>
> 2/ Option1 + additionally use PORTS_IMPL as a gate to guess when the
> PCS format might be different for values >= 8.
>
> I think the driver does not need to consider Option2 unless / until it
> encounters a platform where firmware does not "do the right thing",
> and given Denverton has been in the wild with the wrong PCS twiddling
> it seems to suggest nothing needs to be done there.
>
>> A compromise to me seems that we just do the PCS quirk for all Intel
>> devices explicitly listed in the PCI Ids based on new board_* values
>> as long as they have the old PCS location, and assume anything new
>> enough to have the new location won't need to quirk, given that it
>> never properly worked. This might miss some intel devices that were
>> supported with the class based catchall, though.
>
> I'd be more comfortable with PORT_IMPL as the deciding factor.
Unfortunately we can't use CAP.NP or PORTS_IMPL for this heuristic.
The problem is that BIOS should be setting the PORTS_IMPL bitmask to
match which lanes have actually been connected on the board. So
PORTS_IMPL can be < 8 even if the controller is new enough to
potentially support > 8 and has the new config space layout.
As proof here's the relevant ahci_print_info() output booting on a
Denverton based box with 5 ports implemented:
ahci 0000:00:14.0: AHCI 0001.0301 32 slots 5 ports 6 Gbps 0x7a impl SATA mode
| \-PORTS_IMPL
\-CAP.NP
