Re: [PATCH v3] libata/ahci: Drop PCS quirk for Denverton and beyond

From: Stephen Douthit
Date: Fri Aug 30 2019 - 11:47:39 EST

On 8/29/19 7:30 PM, Dan Williams wrote:
> The Linux ahci driver has historically implemented a configuration fixup
> for platforms / platform-firmware that fails to enable the ports prior
> to OS hand-off at boot. The fixup was originally implemented way back
> before ahci moved from drivers/scsi/ to drivers/ata/, and was updated in
> 2007 via commit 49f290903935 "ahci: update PCS programming". The quirk
> sets a port-enable bitmap in the PCS register at offset 0x92.
>
> This quirk could be applied generically up until the arrival of the
> Denverton (DNV) platform. The DNV AHCI controller architecture supports
> more than 6 ports and along with that the PCS register location and
> format were updated to allow for more possible ports in the bitmap. DNV
> AHCI expands the register to 32-bits and moves it to offset 0x94.
>
> As it stands there are no known problem reports with existing Linux
> trying to set bits at offset 0x92 which indicates that the quirk is not
> applicable. Likely it is not applicable on a wider range of platforms,
> but it is difficult to discern which platforms if any still depend on
> the quirk.
>
> Rather than try to fix the PCS quirk to consider the DNV register layout
> instead require explicit opt-in. The assumption is that the OS driver
> need not touch this register, and platforms can be added with a new
> boad_ahci_pcs7 board-id when / if problematic platforms are found in the
> future. The logic in ahci_intel_pcs_quirk() looks for all Intel AHCI
> instances with "legacy" board-ids and otherwise skips the quirk if the
> board was matched by class-code.
>
> Reported-by: Stephen Douthit <stephend@xxxxxxxxxxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> ---
> Changes since v2:
> - Use board_ahci_pcs7 to opt Denverton out of the PCS quirk.
>
> drivers/ata/ahci.c | 116 +++++++++++++++++++++++++++++++---------------------
> drivers/ata/ahci.h | 2 +
> 2 files changed, 71 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index f7652baa6337..3e63294304c7 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -65,6 +65,12 @@ enum board_ids {
> board_ahci_sb700, /* for SB700 and SB800 */
> board_ahci_vt8251,
>
> + /*
> + * board IDs for Intel chipsets that support more than 6 ports
> + * *and* end up needing the PCS quirk.
> + */
> + board_ahci_pcs7,
> +
> /* aliases */
> board_ahci_mcp_linux = board_ahci_mcp65,
> board_ahci_mcp67 = board_ahci_mcp65,
> @@ -220,6 +226,12 @@ static const struct ata_port_info ahci_port_info[] = {
> .udma_mask = ATA_UDMA6,
> .port_ops = &ahci_vt8251_ops,
> },
> + [board_ahci_pcs7] = {
> + .flags = AHCI_FLAG_COMMON,
> + .pio_mask = ATA_PIO4,
> + .udma_mask = ATA_UDMA6,
> + .port_ops = &ahci_ops,
> + },
> };
>
> static const struct pci_device_id ahci_pci_tbl[] = {
> @@ -264,26 +276,26 @@ static const struct pci_device_id ahci_pci_tbl[] = {
> { PCI_VDEVICE(INTEL, 0x3b2b), board_ahci }, /* PCH RAID */
> { PCI_VDEVICE(INTEL, 0x3b2c), board_ahci_mobile }, /* PCH M RAID */
> { PCI_VDEVICE(INTEL, 0x3b2f), board_ahci }, /* PCH AHCI */
> - { PCI_VDEVICE(INTEL, 0x19b0), board_ahci }, /* DNV AHCI */
> - { PCI_VDEVICE(INTEL, 0x19b1), board_ahci }, /* DNV AHCI */
> - { PCI_VDEVICE(INTEL, 0x19b2), board_ahci }, /* DNV AHCI */
> - { PCI_VDEVICE(INTEL, 0x19b3), board_ahci }, /* DNV AHCI */
> - { PCI_VDEVICE(INTEL, 0x19b4), board_ahci }, /* DNV AHCI */
> - { PCI_VDEVICE(INTEL, 0x19b5), board_ahci }, /* DNV AHCI */
> - { PCI_VDEVICE(INTEL, 0x19b6), board_ahci }, /* DNV AHCI */
> - { PCI_VDEVICE(INTEL, 0x19b7), board_ahci }, /* DNV AHCI */
> - { PCI_VDEVICE(INTEL, 0x19bE), board_ahci }, /* DNV AHCI */
> - { PCI_VDEVICE(INTEL, 0x19bF), board_ahci }, /* DNV AHCI */
> - { PCI_VDEVICE(INTEL, 0x19c0), board_ahci }, /* DNV AHCI */
> - { PCI_VDEVICE(INTEL, 0x19c1), board_ahci }, /* DNV AHCI */
> - { PCI_VDEVICE(INTEL, 0x19c2), board_ahci }, /* DNV AHCI */
> - { PCI_VDEVICE(INTEL, 0x19c3), board_ahci }, /* DNV AHCI */
> - { PCI_VDEVICE(INTEL, 0x19c4), board_ahci }, /* DNV AHCI */
> - { PCI_VDEVICE(INTEL, 0x19c5), board_ahci }, /* DNV AHCI */
> - { PCI_VDEVICE(INTEL, 0x19c6), board_ahci }, /* DNV AHCI */
> - { PCI_VDEVICE(INTEL, 0x19c7), board_ahci }, /* DNV AHCI */
> - { PCI_VDEVICE(INTEL, 0x19cE), board_ahci }, /* DNV AHCI */
> - { PCI_VDEVICE(INTEL, 0x19cF), board_ahci }, /* DNV AHCI */
> + { PCI_VDEVICE(INTEL, 0x19b0), board_ahci_pcs7 }, /* DNV AHCI */
> + { PCI_VDEVICE(INTEL, 0x19b1), board_ahci_pcs7 }, /* DNV AHCI */
> + { PCI_VDEVICE(INTEL, 0x19b2), board_ahci_pcs7 }, /* DNV AHCI */
> + { PCI_VDEVICE(INTEL, 0x19b3), board_ahci_pcs7 }, /* DNV AHCI */
> + { PCI_VDEVICE(INTEL, 0x19b4), board_ahci_pcs7 }, /* DNV AHCI */
> + { PCI_VDEVICE(INTEL, 0x19b5), board_ahci_pcs7 }, /* DNV AHCI */
> + { PCI_VDEVICE(INTEL, 0x19b6), board_ahci_pcs7 }, /* DNV AHCI */
> + { PCI_VDEVICE(INTEL, 0x19b7), board_ahci_pcs7 }, /* DNV AHCI */
> + { PCI_VDEVICE(INTEL, 0x19bE), board_ahci_pcs7 }, /* DNV AHCI */
> + { PCI_VDEVICE(INTEL, 0x19bF), board_ahci_pcs7 }, /* DNV AHCI */
> + { PCI_VDEVICE(INTEL, 0x19c0), board_ahci_pcs7 }, /* DNV AHCI */
> + { PCI_VDEVICE(INTEL, 0x19c1), board_ahci_pcs7 }, /* DNV AHCI */
> + { PCI_VDEVICE(INTEL, 0x19c2), board_ahci_pcs7 }, /* DNV AHCI */
> + { PCI_VDEVICE(INTEL, 0x19c3), board_ahci_pcs7 }, /* DNV AHCI */
> + { PCI_VDEVICE(INTEL, 0x19c4), board_ahci_pcs7 }, /* DNV AHCI */
> + { PCI_VDEVICE(INTEL, 0x19c5), board_ahci_pcs7 }, /* DNV AHCI */
> + { PCI_VDEVICE(INTEL, 0x19c6), board_ahci_pcs7 }, /* DNV AHCI */
> + { PCI_VDEVICE(INTEL, 0x19c7), board_ahci_pcs7 }, /* DNV AHCI */
> + { PCI_VDEVICE(INTEL, 0x19cE), board_ahci_pcs7 }, /* DNV AHCI */
> + { PCI_VDEVICE(INTEL, 0x19cF), board_ahci_pcs7 }, /* DNV AHCI */

I think you want to invert this scheme and mark the old controllers as
board_ahci_pcs6 and leave Denverton/newer controllers as board_ahci.

Otherwise the quirk below runs for any Intel controllers that matched
based on the AHCI class code, since their board_id will be board_ahci
which is < board_ahci_pcs7.

> { PCI_VDEVICE(INTEL, 0x1c02), board_ahci }, /* CPT AHCI */
> { PCI_VDEVICE(INTEL, 0x1c03), board_ahci_mobile }, /* CPT M AHCI */
> { PCI_VDEVICE(INTEL, 0x1c04), board_ahci }, /* CPT RAID */
> @@ -623,30 +635,6 @@ static void ahci_pci_save_initial_config(struct pci_dev *pdev,
> ahci_save_initial_config(&pdev->dev, hpriv);
> }
>
> -static int ahci_pci_reset_controller(struct ata_host *host)
> -{
> - struct pci_dev *pdev = to_pci_dev(host->dev);
> - int rc;
> -
> - rc = ahci_reset_controller(host);
> - if (rc)
> - return rc;
> -
> - if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
> - struct ahci_host_priv *hpriv = host->private_data;
> - u16 tmp16;
> -
> - /* configure PCS */
> - pci_read_config_word(pdev, 0x92, &tmp16);
> - if ((tmp16 & hpriv->port_map) != hpriv->port_map) {
> - tmp16 |= hpriv->port_map;
> - pci_write_config_word(pdev, 0x92, tmp16);
> - }
> - }
> -
> - return 0;
> -}
> -
> static void ahci_pci_init_controller(struct ata_host *host)
> {
> struct ahci_host_priv *hpriv = host->private_data;
> @@ -849,7 +837,7 @@ static int ahci_pci_device_runtime_resume(struct device *dev)
> struct ata_host *host = pci_get_drvdata(pdev);
> int rc;
>
> - rc = ahci_pci_reset_controller(host);
> + rc = ahci_reset_controller(host);
> if (rc)
> return rc;
> ahci_pci_init_controller(host);
> @@ -884,7 +872,7 @@ static int ahci_pci_device_resume(struct device *dev)
> ahci_mcp89_apple_enable(pdev);
>
> if (pdev->dev.power.power_state.event == PM_EVENT_SUSPEND) {
> - rc = ahci_pci_reset_controller(host);
> + rc = ahci_reset_controller(host);
> if (rc)
> return rc;
>
> @@ -1619,6 +1607,34 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap,
> ap->target_lpm_policy = policy;
> }
>
> +static void ahci_intel_pcs_quirk(struct pci_dev *pdev, struct ahci_host_priv *hpriv)
> +{
> + const struct pci_device_id *id = pci_match_id(ahci_pci_tbl, pdev);
> + u16 tmp16;
> +
> + /*
> + * Only apply the 6-port PCS quirk for known legacy platforms.
> + */
> + if (!id || id->vendor != PCI_VENDOR_ID_INTEL)
> + return;
> + if (((enum board_ids) id->driver_data) < board_ahci_pcs7)
> + return;
> +
> + /*
> + * 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. It is
> + * otherwise expected that platform firmware enables the ports
> + * before the OS boots.
> + */
> + pci_read_config_word(pdev, PCS_6, &tmp16);
> + if ((tmp16 & hpriv->port_map) != hpriv->port_map) {
> + tmp16 |= hpriv->port_map;
> + pci_write_config_word(pdev, PCS_6, tmp16);
> + }
> +}
> +
> static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
> unsigned int board_id = ent->driver_data;
> @@ -1731,6 +1747,12 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> /* save initial config */
> ahci_pci_save_initial_config(pdev, hpriv);
>
> + /*
> + * If platform firmware failed to enable ports, try to enable
> + * them here.
> + */
> + ahci_intel_pcs_quirk(pdev, hpriv);
> +
> /* prepare host */
> if (hpriv->cap & HOST_CAP_NCQ) {
> pi.flags |= ATA_FLAG_NCQ;
> @@ -1840,7 +1862,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> if (rc)
> return rc;
>
> - rc = ahci_pci_reset_controller(host);
> + rc = ahci_reset_controller(host);
> if (rc)
> return rc;
>
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index 0570629d719d..3dbf398c92ea 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -247,6 +247,8 @@ enum {
> ATA_FLAG_ACPI_SATA | ATA_FLAG_AN,
>
> ICH_MAP = 0x90, /* ICH MAP register */
> + PCS_6 = 0x92, /* 6 port PCS */
> + PCS_7 = 0x94, /* 7+ port PCS (Denverton) */
>
> /* em constants */
> EM_MAX_SLOTS = 8,
>