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

[Stephen Douthit]

On 8/12/19 2:06 PM, Christoph Hellwig 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@xxxxxxxxx>
> Date: Thu, 14 Oct 2004 16:11:44 -0400

Thanks for digging, this is definitely further back than I looked.

> 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
> * implement staggered spinup by default
> * adding additional debugging output

[snip]

> @@ -236,7 +238,9 @@ static struct ata_port_info ahci_port_info[] = {
>   };
>   
>   static struct pci_device_id ahci_pci_tbl[] = {
> -	{ PCI_VENDOR_ID_INTEL, 0x0000, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> +	{ PCI_VENDOR_ID_INTEL, 0x2652, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> +	  board_ahci },
> +	{ PCI_VENDOR_ID_INTEL, 0x2653, PCI_ANY_ID, PCI_ANY_ID, 0, 0,

These look to be for the ICH6, and at these early days Intel looks to be
the only vendor in the list.  That would explain the lack of a vendor
guard on the PCS poke below.

>   	  board_ahci },
>   	{ }	/* terminate list */
>   };

[snip]

>   static int ahci_host_init(struct ata_probe_ent *probe_ent)
>   {
>   	struct ahci_host_priv *hpriv = probe_ent->private_data;
>   	struct pci_dev *pdev = probe_ent->pdev;
> -	void *mmio = probe_ent->mmio_base;
> -	u32 tmp;
> -	unsigned int i, using_dac;
> +	void __iomem *mmio = probe_ent->mmio_base;
> +	u32 tmp, cap_save;
> +	u16 tmp16;
> +	unsigned int i, j, using_dac;
>   	int rc;
> +	void __iomem *port_mmio;
> +
> +	cap_save = readl(mmio + HOST_CAP);
> +	cap_save &= ( (1<<28) | (1<<17) );
> +	cap_save |= (1 << 27);
>   
>   	/* global controller reset */
>   	tmp = readl(mmio + HOST_CTL);
> @@ -698,23 +747,22 @@ static int ahci_host_init(struct ata_probe_ent *probe_ent)
>   		return -EIO;
>   	}
>   
> -	/* make sure we're in AHCI mode, with irq disabled */
> -	if ((tmp & (HOST_AHCI_EN | HOST_IRQ_EN)) != HOST_AHCI_EN) {
> -		tmp |= HOST_AHCI_EN;
> -		tmp &= ~HOST_IRQ_EN;
> -		writel(tmp, mmio + HOST_CTL);
> -		readl(mmio + HOST_CTL); /* flush */
> -	}
> -	tmp = readl(mmio + HOST_CTL);
> -	if ((tmp & (HOST_AHCI_EN | HOST_IRQ_EN)) != HOST_AHCI_EN) {
> -		printk(KERN_ERR DRV_NAME "(%s): AHCI enable failed (0x%x)\n",
> -			pci_name(pdev), tmp);
> -		return -EIO;
> -	}
> +	writel(HOST_AHCI_EN, mmio + HOST_CTL);
> +	(void) readl(mmio + HOST_CTL);	/* flush */
> +	writel(cap_save, mmio + HOST_CAP);
> +	writel(0xf, mmio + HOST_PORTS_IMPL);
> +	(void) readl(mmio + HOST_PORTS_IMPL);	/* flush */
> +
> +	pci_read_config_word(pdev, 0x92, &tmp16);
> +	tmp16 |= 0xf;
> +	pci_write_config_word(pdev, 0x92, tmp16);

Not much to explain why this is here unfortunately...

Maybe this isn't so much a workaround as an implementation specific
quirk.  If we can't rely on 100% of BIOS implementations to have set PCS
to match Ports Implemented (probably a safe bet) then we still need the
PCS poke code.

In that case we're back to Christoph's question of if there's a better
way of figuring out where PCS lives than checking device IDs.

In the meantime on Denverton 0x92 contains the Port Disable bits of the
MAP register.  Since these are write-once BIOS should have locked them
already so we probably won't break anything poking them until a solution
is identified.  I don't think we want to rely on that assumption in the
long term though.