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

From: Dan Williams <dan.j.williams@intel.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: Stephen Douthit <stephend@silicom-usa.com>,
	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: Mon, 12 Aug 2019 12:31:35 -0700	[thread overview]
Message-ID: <CAA9_cme3saBAJEyob3B1tX=t8keTodWJZMUd1j_v7vPMRU+aXA@mail.gmail.com> (raw)
In-Reply-To: <20190812180613.GA18377@infradead.org>

On Mon, Aug 12, 2019 at 11:08 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Aug 12, 2019 at 05:49:29PM +0000, Stephen Douthit wrote:
> > Does anyone know the background of the original PCS workaround?
>
> Based on a few git-blame iterations on history.git the original PCS
> handling (just when initializing) goes back to this BK commit:
>
> --
> From c0835b838e76c9500facad05dc305170a1a577a8 Mon Sep 17 00:00:00 2001
> From: Jeff Garzik <jgarzik@pobox.com>
> Date: Thu, 14 Oct 2004 16:11:44 -0400
> Subject: [libata ahci] fix several bugs
>
> * PCI IDs from test version didn't make it into mainline... doh
> * do all command setup in ->qc_prep
> * phy_reset routine that does signature check
> * check SATA phy for errors
> * reset hardware from scratch, in case card BIOS didn't run

Ok, that at least matches the expectation that platform firmware
initially enables the ports. However, it still leaves open the
question of whether the PCS bits were actually not configured, or
whether just the controller reset was needed. Certainly there is no
reason to touch that configuration register after every controller
reset (via the HOST_CTL mmio register)

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.