Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID
From: Dan Williams
Date: Wed Aug 14 2019 - 12:09:15 EST
On Wed, Aug 14, 2019 at 7:34 AM Stephen Douthit
> 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
Ugh, ok, thanks for clarifying and my mistake for not realizing
Denverton already violates that heuristic.
So now I'm trying to grok Christoph's suggestion. Are you saying that
all existing board-ids would assume old PCS format? That would mean
that Denverton gets the wrong format via board_ahci via the class code
matching? Maybe I'm not understanding the suggestion...
Another option might be that controllers >= 1.3.1 will stop using the
PCS twiddling, and then we go add a new board id where / if it happens
to matter in practice.