Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID

[Dan Williams]

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.