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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 8/19/19 10:17 PM, Dan Williams wrote:
> On Mon, Aug 19, 2019 at 9:30 AM Stephen Douthit
> <stephend@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> 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@xxxxxxxxxxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> ---
>   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@xxxxxxxxxxxxxxx>

> +       /*
> +        * 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;
> 





[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux