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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.