Date:   Wed, 14 Aug 2019 09:09:01 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     Stephen Douthit <stephend@...icom-usa.com>
Cc:     Christoph Hellwig <hch@...radead.org>,
        Jens Axboe <axboe@...nel.dk>,
        "linux-ide@...r.kernel.org" <linux-ide@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID

On Wed, Aug 14, 2019 at 7:34 AM Stephen Douthit
<stephend@...icom-usa.com> wrote:
>
> On 8/13/19 6:07 PM, Dan Williams wrote:
> > On Tue, Aug 13, 2019 at 12:31 AM Christoph Hellwig <hch@...radead.org> 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

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.

Powered by blists - more mailing lists