From: Stephen Douthit <stephend@silicom-usa.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: Christoph Hellwig <hch@infradead.org>,
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: Tue, 20 Aug 2019 13:59:53 +0000 [thread overview]
Message-ID: <a04c0ae7-ef4d-4275-de05-b74beaeef86c@silicom-usa.com> (raw)
In-Reply-To: <CAPcyv4giQDk3ZMzCUM5dv_QLk_NDVCeAgcTi-Mk-YVENq_xpjg@mail.gmail.com>
On 8/19/19 10:17 PM, Dan Williams wrote:
> On Mon, Aug 19, 2019 at 9:30 AM Stephen Douthit
> <stephend@silicom-usa.com> wrote:
>>
>> On 8/14/19 1:17 PM, Dan Williams wrote:
>>>> Can you get someone from the controller design team to give us a clear
>>>> answer on a revision where this PCS change happened?
>>>>
>>>> It would be nice if we could just check PCI_REVISION_ID or something
>>>> similar.
>>>
>>> I don't think such a reliable association with rev-id exists, the> intent was that the OS need never consider PCS.
>>
>> Can you please ask to confirm? It would be a much simpler check if it
>> is possible.
>
> No. Even if it were accidentally the case today the Linux driver can't
> trust that rev-id across the different implementations will be
> maintained for this purpose because the OS driver is not meant to
> touch this register. Just look at a sampling of rev-id from a few
> different systems, and note that rev-id applies to the chipset not
> just the ahci controller.
>
> rev 08
> rev 11
> rev 31
>
> ...which one of those is Denverton?
>
> The intent is that PCS is a platform-firmware concern and that any
> software that cares about PCS is caring about it by explicit
> identification.
Understood.
My hope was that there was a guaranteed relation, but no such luck.
>>> The way Linux is using
>>> it is already broken with the assumption that it is performed after
>>> every HOST_CTL based reset which only resets mmio space. At most it
>>> should only be required at initial PCI discovery iff platform firmware
>>> failed to run.
>>
>> This is a separate issue.
>>
>> It's broken in the sense that the code is called more often that it
>> needs to be, but reset isn't a hot path, and there are no side effects
>> to doing this multiple times that I can see.
>
> The problem is that there is no known need to do it for Denverton, and
> likely more platform implementations.
>
>> And as you point out, no
>> bug reports, so pretty benign all things considered.
>>
>> We could move the PCS quirk code to ahci_init_one() to address this
>> concern once there's agreement on what the criteria is to run/not-run
>> this code.
>>
>>> There are no bug reports with the current
>>> implementation that only attempts to enable bits based on PORTS_IMPL,
>>> so I think we are firmer ground trying to draw a line where the driver
>>> just stops worrying about PCS rather than try to detect the layout.
>>
>> Someone at Intel is going to need to decide where/how to draw this line.
>
> This is a case of Linux touching a "BIOS only" register and assuming
> that the quirk is widely applicable. I think a reasonable fix is to
> just whitelist all the known Intel ids, apply the PCS fixup assuming
> the legacy configuration register location, and stop applying the
> quirk by default.
>
> Here is a proposed patch along these lines. I can send a
> non-whitespace damaged version if this approach looks acceptable:
>
> ---
>
> From f40a7f287c97cfba71393ccb592ba521e43d807b Mon Sep 17 00:00:00 2001
> From: Dan Williams <dan.j.williams@intel.com>
> Date: Mon, 19 Aug 2019 11:30:37 -0700
> Subject: [PATCH] libata/ahci: Drop PCS quirk for Denverton and beyond
>
> 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 to a whitelist
> when / if problematic platforms are found in the future. The list in
> ahci_intel_pcs_quirk() is populated with all the current explicit
> device-ids with the expectation that class-code based detection need not
> apply the quirk.
>
> Reported-by: Stephen Douthit <stephend@silicom-usa.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/ata/ahci.c | 211 +++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 184 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index f7652baa6337..ceb38d205e1b 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -623,30 +623,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 +825,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 +860,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 +1595,181 @@ 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)
> +{
> + static const struct pci_device_id ahci_pcs_ids[] = {
> + /* Historical ids, ambiguous if the PCS quirk is needed... */
> + { PCI_VDEVICE(INTEL, 0x2652), }, /* ICH6 */
> + { PCI_VDEVICE(INTEL, 0x2653), }, /* ICH6M */
> + { PCI_VDEVICE(INTEL, 0x27c1), }, /* ICH7 */
> + { PCI_VDEVICE(INTEL, 0x27c5), }, /* ICH7M */
> + { PCI_VDEVICE(INTEL, 0x27c3), }, /* ICH7R */
> + { PCI_VDEVICE(INTEL, 0x2681), }, /* ESB2 */
> + { PCI_VDEVICE(INTEL, 0x2682), }, /* ESB2 */
> + { PCI_VDEVICE(INTEL, 0x2683), }, /* ESB2 */
> + { PCI_VDEVICE(INTEL, 0x27c6), }, /* ICH7-M DH */
> + { PCI_VDEVICE(INTEL, 0x2821), }, /* ICH8 */
> + { PCI_VDEVICE(INTEL, 0x2822), }, /* ICH8 */
> + { PCI_VDEVICE(INTEL, 0x2824), }, /* ICH8 */
> + { PCI_VDEVICE(INTEL, 0x2829), }, /* ICH8M */
> + { PCI_VDEVICE(INTEL, 0x282a), }, /* ICH8M */
> + { PCI_VDEVICE(INTEL, 0x2922), }, /* ICH9 */
> + { PCI_VDEVICE(INTEL, 0x2923), }, /* ICH9 */
> + { PCI_VDEVICE(INTEL, 0x2924), }, /* ICH9 */
> + { PCI_VDEVICE(INTEL, 0x2925), }, /* ICH9 */
> + { PCI_VDEVICE(INTEL, 0x2927), }, /* ICH9 */
> + { PCI_VDEVICE(INTEL, 0x2929), }, /* ICH9M */
> + { PCI_VDEVICE(INTEL, 0x292a), }, /* ICH9M */
> + { PCI_VDEVICE(INTEL, 0x292b), }, /* ICH9M */
> + { PCI_VDEVICE(INTEL, 0x292c), }, /* ICH9M */
> + { PCI_VDEVICE(INTEL, 0x292f), }, /* ICH9M */
> + { PCI_VDEVICE(INTEL, 0x294d), }, /* ICH9 */
> + { PCI_VDEVICE(INTEL, 0x294e), }, /* ICH9M */
> + { PCI_VDEVICE(INTEL, 0x502a), }, /* Tolapai */
> + { PCI_VDEVICE(INTEL, 0x502b), }, /* Tolapai */
> + { PCI_VDEVICE(INTEL, 0x3a05), }, /* ICH10 */
> + { PCI_VDEVICE(INTEL, 0x3a22), }, /* ICH10 */
> + { PCI_VDEVICE(INTEL, 0x3a25), }, /* ICH10 */
> + { PCI_VDEVICE(INTEL, 0x3b22), }, /* PCH AHCI */
> + { PCI_VDEVICE(INTEL, 0x3b23), }, /* PCH AHCI */
> + { PCI_VDEVICE(INTEL, 0x3b24), }, /* PCH RAID */
> + { PCI_VDEVICE(INTEL, 0x3b25), }, /* PCH RAID */
> + { PCI_VDEVICE(INTEL, 0x3b29), }, /* PCH M AHCI */
> + { PCI_VDEVICE(INTEL, 0x3b2b), }, /* PCH RAID */
> + { PCI_VDEVICE(INTEL, 0x3b2c), }, /* PCH M RAID */
> + { PCI_VDEVICE(INTEL, 0x3b2f), }, /* PCH AHCI */
> + { PCI_VDEVICE(INTEL, 0x1c02), }, /* CPT AHCI */
> + { PCI_VDEVICE(INTEL, 0x1c03), }, /* CPT M AHCI */
> + { PCI_VDEVICE(INTEL, 0x1c04), }, /* CPT RAID */
> + { PCI_VDEVICE(INTEL, 0x1c05), }, /* CPT M RAID */
> + { PCI_VDEVICE(INTEL, 0x1c06), }, /* CPT RAID */
> + { PCI_VDEVICE(INTEL, 0x1c07), }, /* CPT RAID */
> + { PCI_VDEVICE(INTEL, 0x1d02), }, /* PBG AHCI */
> + { PCI_VDEVICE(INTEL, 0x1d04), }, /* PBG RAID */
> + { PCI_VDEVICE(INTEL, 0x1d06), }, /* PBG RAID */
> + { PCI_VDEVICE(INTEL, 0x2826), }, /* PBG RAID */
> + { PCI_VDEVICE(INTEL, 0x2323), }, /* DH89xxCC AHCI */
> + { PCI_VDEVICE(INTEL, 0x1e02), }, /* Panther Point AHCI */
> + { PCI_VDEVICE(INTEL, 0x1e03), }, /* Panther M AHCI */
> + { PCI_VDEVICE(INTEL, 0x1e04), }, /* Panther Point RAID */
> + { PCI_VDEVICE(INTEL, 0x1e05), }, /* Panther Point RAID */
> + { PCI_VDEVICE(INTEL, 0x1e06), }, /* Panther Point RAID */
> + { PCI_VDEVICE(INTEL, 0x1e07), }, /* Panther M RAID */
> + { PCI_VDEVICE(INTEL, 0x1e0e), }, /* Panther Point RAID */
> + { PCI_VDEVICE(INTEL, 0x8c02), }, /* Lynx Point AHCI */
> + { PCI_VDEVICE(INTEL, 0x8c03), }, /* Lynx M AHCI */
> + { PCI_VDEVICE(INTEL, 0x8c04), }, /* Lynx Point RAID */
> + { PCI_VDEVICE(INTEL, 0x8c05), }, /* Lynx M RAID */
> + { PCI_VDEVICE(INTEL, 0x8c06), }, /* Lynx Point RAID */
> + { PCI_VDEVICE(INTEL, 0x8c07), }, /* Lynx M RAID */
> + { PCI_VDEVICE(INTEL, 0x8c0e), }, /* Lynx Point RAID */
> + { PCI_VDEVICE(INTEL, 0x8c0f), }, /* Lynx M RAID */
> + { PCI_VDEVICE(INTEL, 0x9c02), }, /* Lynx LP AHCI */
> + { PCI_VDEVICE(INTEL, 0x9c03), }, /* Lynx LP AHCI */
> + { PCI_VDEVICE(INTEL, 0x9c04), }, /* Lynx LP RAID */
> + { PCI_VDEVICE(INTEL, 0x9c05), }, /* Lynx LP RAID */
> + { PCI_VDEVICE(INTEL, 0x9c06), }, /* Lynx LP RAID */
> + { PCI_VDEVICE(INTEL, 0x9c07), }, /* Lynx LP RAID */
> + { PCI_VDEVICE(INTEL, 0x9c0e), }, /* Lynx LP RAID */
> + { PCI_VDEVICE(INTEL, 0x9c0f), }, /* Lynx LP RAID */
> + { PCI_VDEVICE(INTEL, 0x9dd3), }, /* Cannon Lake PCH-LP AHCI */
> + { PCI_VDEVICE(INTEL, 0x1f22), }, /* Avoton AHCI */
> + { PCI_VDEVICE(INTEL, 0x1f23), }, /* Avoton AHCI */
> + { PCI_VDEVICE(INTEL, 0x1f24), }, /* Avoton RAID */
> + { PCI_VDEVICE(INTEL, 0x1f25), }, /* Avoton RAID */
> + { PCI_VDEVICE(INTEL, 0x1f26), }, /* Avoton RAID */
> + { PCI_VDEVICE(INTEL, 0x1f27), }, /* Avoton RAID */
> + { PCI_VDEVICE(INTEL, 0x1f2e), }, /* Avoton RAID */
> + { PCI_VDEVICE(INTEL, 0x1f2f), }, /* Avoton RAID */
> + { PCI_VDEVICE(INTEL, 0x1f32), }, /* Avoton AHCI */
> + { PCI_VDEVICE(INTEL, 0x1f33), }, /* Avoton AHCI */
> + { PCI_VDEVICE(INTEL, 0x1f34), }, /* Avoton RAID */
> + { PCI_VDEVICE(INTEL, 0x1f35), }, /* Avoton RAID */
> + { PCI_VDEVICE(INTEL, 0x1f36), }, /* Avoton RAID */
> + { PCI_VDEVICE(INTEL, 0x1f37), }, /* Avoton RAID */
> + { PCI_VDEVICE(INTEL, 0x1f3e), }, /* Avoton RAID */
> + { PCI_VDEVICE(INTEL, 0x1f3f), }, /* Avoton RAID */
> + { PCI_VDEVICE(INTEL, 0x2823), }, /* Wellsburg RAID */
> + { PCI_VDEVICE(INTEL, 0x2827), }, /* Wellsburg RAID */
> + { PCI_VDEVICE(INTEL, 0x8d02), }, /* Wellsburg AHCI */
> + { PCI_VDEVICE(INTEL, 0x8d04), }, /* Wellsburg RAID */
> + { PCI_VDEVICE(INTEL, 0x8d06), }, /* Wellsburg RAID */
> + { PCI_VDEVICE(INTEL, 0x8d0e), }, /* Wellsburg RAID */
> + { PCI_VDEVICE(INTEL, 0x8d62), }, /* Wellsburg AHCI */
> + { PCI_VDEVICE(INTEL, 0x8d64), }, /* Wellsburg RAID */
> + { PCI_VDEVICE(INTEL, 0x8d66), }, /* Wellsburg RAID */
> + { PCI_VDEVICE(INTEL, 0x8d6e), }, /* Wellsburg RAID */
> + { PCI_VDEVICE(INTEL, 0x23a3), }, /* Coleto Creek AHCI */
> + { PCI_VDEVICE(INTEL, 0x9c83), }, /* Wildcat LP AHCI */
> + { PCI_VDEVICE(INTEL, 0x9c85), }, /* Wildcat LP RAID */
> + { PCI_VDEVICE(INTEL, 0x9c87), }, /* Wildcat LP RAID */
> + { PCI_VDEVICE(INTEL, 0x9c8f), }, /* Wildcat LP RAID */
> + { PCI_VDEVICE(INTEL, 0x8c82), }, /* 9 Series AHCI */
> + { PCI_VDEVICE(INTEL, 0x8c83), }, /* 9 Series M AHCI */
> + { PCI_VDEVICE(INTEL, 0x8c84), }, /* 9 Series RAID */
> + { PCI_VDEVICE(INTEL, 0x8c85), }, /* 9 Series M RAID */
> + { PCI_VDEVICE(INTEL, 0x8c86), }, /* 9 Series RAID */
> + { PCI_VDEVICE(INTEL, 0x8c87), }, /* 9 Series M RAID */
> + { PCI_VDEVICE(INTEL, 0x8c8e), }, /* 9 Series RAID */
> + { PCI_VDEVICE(INTEL, 0x8c8f), }, /* 9 Series M RAID */
> + { PCI_VDEVICE(INTEL, 0x9d03), }, /* Sunrise LP AHCI */
> + { PCI_VDEVICE(INTEL, 0x9d05), }, /* Sunrise LP RAID */
> + { PCI_VDEVICE(INTEL, 0x9d07), }, /* Sunrise LP RAID */
> + { PCI_VDEVICE(INTEL, 0xa102), }, /* Sunrise Point-H AHCI */
> + { PCI_VDEVICE(INTEL, 0xa103), }, /* Sunrise M AHCI */
> + { PCI_VDEVICE(INTEL, 0xa105), }, /* Sunrise Point-H RAID */
> + { PCI_VDEVICE(INTEL, 0xa106), }, /* Sunrise Point-H RAID */
> + { PCI_VDEVICE(INTEL, 0xa107), }, /* Sunrise M RAID */
> + { PCI_VDEVICE(INTEL, 0xa10f), }, /* Sunrise Point-H RAID */
> + { PCI_VDEVICE(INTEL, 0x2822), }, /* Lewisburg RAID*/
> + { PCI_VDEVICE(INTEL, 0x2823), }, /* Lewisburg AHCI*/
> + { PCI_VDEVICE(INTEL, 0x2826), }, /* Lewisburg RAID*/
> + { PCI_VDEVICE(INTEL, 0x2827), }, /* Lewisburg RAID*/
> + { PCI_VDEVICE(INTEL, 0xa182), }, /* Lewisburg AHCI*/
> + { PCI_VDEVICE(INTEL, 0xa186), }, /* Lewisburg RAID*/
> + { PCI_VDEVICE(INTEL, 0xa1d2), }, /* Lewisburg RAID*/
> + { PCI_VDEVICE(INTEL, 0xa1d6), }, /* Lewisburg RAID*/
> + { PCI_VDEVICE(INTEL, 0xa202), }, /* Lewisburg AHCI*/
> + { PCI_VDEVICE(INTEL, 0xa206), }, /* Lewisburg RAID*/
> + { PCI_VDEVICE(INTEL, 0xa252), }, /* Lewisburg RAID*/
> + { PCI_VDEVICE(INTEL, 0xa256), }, /* Lewisburg RAID*/
> + { PCI_VDEVICE(INTEL, 0xa356), }, /* Cannon Lake PCH-H RAID */
> + { PCI_VDEVICE(INTEL, 0x0f22), }, /* Bay Trail AHCI */
> + { PCI_VDEVICE(INTEL, 0x0f23), }, /* Bay Trail AHCI */
> + { PCI_VDEVICE(INTEL, 0x22a3), }, /* Cherry Tr. AHCI */
> + { PCI_VDEVICE(INTEL, 0x5ae3), }, /* ApolloLake AHCI */
> + { PCI_VDEVICE(INTEL, 0x34d3), }, /* Ice Lake LP AHCI */
Instead of reproducing a chunk of ahci_pci_tbl here could we add a board
id/host flag for this quirk?
Something like:
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -53,6 +53,7 @@ enum board_ids {
board_ahci_nomsi,
board_ahci_noncq,
board_ahci_nosntf,
+ board_ahci_pcs,
board_ahci_yes_fbs,
/* board IDs for specific chipsets in alphabetical order */
@@ -153,6 +154,13 @@ static const struct ata_port_info ahci_port_info[] = {
.udma_mask = ATA_UDMA6,
.port_ops = &ahci_ops,
},
+ [board_ahci_pcs] = {
+ AHCI_HFLAGS (AHCI_HFLAG_PCS),
+ .flags = AHCI_FLAG_COMMON,
+ .pio_mask = ATA_PIO4,
+ .udma_mask = ATA_UDMA6,
+ .port_ops = &ahci_ops,
+ },
[board_ahci_yes_fbs] = {
AHCI_HFLAGS (AHCI_HFLAG_YES_FBS),
.flags = AHCI_FLAG_COMMON,
@@ -162,6 +170,7 @@ static const struct ata_port_info ahci_port_info[] = {
},
/* by chipsets */
[board_ahci_avn] = {
+ AHCI_HFLAGS (AHCI_HFLAG_PCS),
.flags = AHCI_FLAG_COMMON,
.pio_mask = ATA_PIO4,
.udma_mask = ATA_UDMA6,
@@ -224,9 +233,9 @@ static const struct ata_port_info ahci_port_info[] = {
static const struct pci_device_id ahci_pci_tbl[] = {
/* Intel */
- { PCI_VDEVICE(INTEL, 0x2652), board_ahci }, /* ICH6 */
- { PCI_VDEVICE(INTEL, 0x2653), board_ahci }, /* ICH6M */
- { PCI_VDEVICE(INTEL, 0x27c1), board_ahci }, /* ICH7 */
+ { PCI_VDEVICE(INTEL, 0x2652), board_ahci_pcs }, /* ICH6 */
+ { PCI_VDEVICE(INTEL, 0x2653), board_ahci_pcs }, /* ICH6M */
+ { PCI_VDEVICE(INTEL, 0x27c1), board_ahci_pcs }, /* ICH7 */
[snip]
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 0570629d719d..ff9c41bc8d8d 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -240,6 +240,7 @@ enum {
as default lpm_policy */
AHCI_HFLAG_SUSPEND_PHYS = (1 << 26), /* handle PHYs during
suspend/resume */
+ AHCI_HFLAG_PCS = (1 << 27), /* apply Intel PCS quirk */
/* ap->flags bits */
> + /*
> + * Known ids where PCS fixup is needed, be careful to
> + * check the format and location of PCS when adding ids
> + * to this list. For example Denverton supports more
> + * than 6 ports and changes the format of PCS, but
> + * deployed platforms are not known to require a PCS
> + * fixup.
> + */
> + { },
> + };
> + u16 tmp16;
> +
> + if (!pci_match_id(ahci_pcs_ids, pdev))
> + return;
Then here we can just check if the AHCI_HFLAG_PCS flag is set.
Either way works for me, so:
Reviewed-by: Stephen Douthit <stephend@silicom-usa.com>
> + /*
> + * 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, 0x92, &tmp16);
> + if ((tmp16 & hpriv->port_map) != hpriv->port_map) {
> + tmp16 |= hpriv->port_map;
> + pci_write_config_word(pdev, 0x92, tmp16);
This is a nit, but since it came up earlier do we want to name the PCS
offset here instead of using a magic number?
> + }
> +}
> +
> static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
> unsigned int board_id = ent->driver_data;
> @@ -1731,6 +1882,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 +1997,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;
>
prev parent reply other threads:[~2019-08-20 14:00 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
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 [this message]
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=a04c0ae7-ef4d-4275-de05-b74beaeef86c@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).