From: Stephen Douthit <stephend@silicom-usa.com>
To: Dan Williams <dan.j.williams@intel.com>,
Christoph Hellwig <hch@infradead.org>
Cc: Jens Axboe <axboe@kernel.dk>,
"linux-ide@vger.kernel.org" <linux-ide@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID
Date: Wed, 14 Aug 2019 14:34:07 +0000 [thread overview]
Message-ID: <c023a18c-8b70-dc59-3db8-51d3a6b23d3c@silicom-usa.com> (raw)
In-Reply-To: <CAPcyv4h5kCKVyCjomBUY27MJwheDZ8v87+a9K-2YCgyqRWR7eQ@mail.gmail.com>
On 8/13/19 6:07 PM, Dan Williams wrote:
> On Tue, Aug 13, 2019 at 12:31 AM Christoph Hellwig <hch@infradead.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
next prev parent reply other threads:[~2019-08-14 14:34 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-08 20:24 [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID Stephen Douthit
2019-08-09 3:46 ` Jens Axboe
2019-08-09 14:13 ` Stephen Douthit
2019-08-09 14:16 ` Jens Axboe
2019-08-10 7:43 ` Christoph Hellwig
2019-08-10 20:22 ` Dan Williams
2019-08-12 13:02 ` Stephen Douthit
2019-08-12 14:11 ` Jens Axboe
2019-08-12 16:29 ` Dan Williams
2019-08-12 17:49 ` Stephen Douthit
2019-08-12 18:06 ` Christoph Hellwig
2019-08-12 19:12 ` Stephen Douthit
2019-08-12 19:31 ` Dan Williams
2019-08-13 7:29 ` Christoph Hellwig
2019-08-13 22:07 ` Dan Williams
2019-08-14 14:34 ` Stephen Douthit [this message]
2019-08-14 16:09 ` Dan Williams
2019-08-14 16:54 ` Stephen Douthit
2019-08-14 17:17 ` Dan Williams
2019-08-19 16:30 ` Stephen Douthit
2019-08-20 2:17 ` Dan Williams
2019-08-20 13:59 ` Stephen Douthit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c023a18c-8b70-dc59-3db8-51d3a6b23d3c@silicom-usa.com \
--to=stephend@silicom-usa.com \
--cc=axboe@kernel.dk \
--cc=dan.j.williams@intel.com \
--cc=hch@infradead.org \
--cc=linux-ide@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).