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

[Stephen Douthit]

On 8/12/19 12:29 PM, Dan Williams wrote:
> On Mon, Aug 12, 2019 at 6:03 AM Stephen Douthit
> <stephend@xxxxxxxxxxxxxxx> wrote:
>>
>> On 8/10/19 3:43 AM, Christoph Hellwig wrote:
>>> On Thu, Aug 08, 2019 at 08:24:31PM +0000, Stephen Douthit wrote:
>>>> Intel moved the PCS register from 0x92 to 0x94 on Denverton for some
>>>> reason, so now we get to check the device ID before poking it on reset.
>>>
>>> And now you just match on the new IDs, which means we'll perpetually
>>> catch up on any new device.  Dan, can you reach out inside Intel to
>>> figure out if there is a way to find out the PCS register location
>>> without the PCI ID check?
>>>
>>>
>>>>    static int ahci_pci_reset_controller(struct ata_host *host)
>>>>    {
>>>>       struct pci_dev *pdev = to_pci_dev(host->dev);
>>>> @@ -634,13 +669,14 @@ static int ahci_pci_reset_controller(struct ata_host *host)
>>>>
>>>>       if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
>>>>               struct ahci_host_priv *hpriv = host->private_data;
>>>> +            int pcs = ahci_pcs_offset(host);
>>>>               u16 tmp16;
>>>>
>>>>               /* configure PCS */
>>>> -            pci_read_config_word(pdev, 0x92, &tmp16);
>>>> +            pci_read_config_word(pdev, pcs, &tmp16);
>>>>               if ((tmp16 & hpriv->port_map) != hpriv->port_map) {
>>>> -                    tmp16 |= hpriv->port_map;
>>>> -                    pci_write_config_word(pdev, 0x92, tmp16);
>>>> +                    tmp16 |= hpriv->port_map & 0xff;
>>>> +                    pci_write_config_word(pdev, pcs, tmp16);
>>>>               }
>>>>       }
>>>
>>> And Stephen, while you are at it, can you split this Intel-specific
>>> quirk into a separate helper?
>>
>> I can do that.  I'll wait until we hear back from Dan if there's a
>> better scheme than a device ID lookup.
> 
> Do you see any behavior change in practice with this patch? It's
> purportedly to re-enable the ports after a reset, but that would only
> be needed if the entire pci device reset. In this path the reset is
> being performed via the host control register. That is only meant to
> touch mmio registers, not config registers. So, as far as I can see
> this register bit twiddling after reset has never been necessary.

Not on Denverton.  I have seen AHCI reset issues on Avoton/Rangeley, but
I'd have to go digging at this point to know for sure if they were fixed
solely by the ahci_avn_hardreset() workaround, or that in combination
with the existing PCS workaround.

I found this not because of failure I saw in Linux, but because I was
using the Linux driver as reference while debugging the u-boot AHCI
driver.  When I couldn't find config space offset 0x92 defined in the
Denverton EDS I went digging, and that's where the patch comes from.

I wasn't quickly able to find what chipset the PCS workaround was added
for.  If it's for an obsolete chipset then dropping this entirely would
be cleaner.

Does anyone know the background of the original PCS workaround?