* [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID @ 2019-08-08 20:24 Stephen Douthit 2019-08-09 3:46 ` Jens Axboe 2019-08-10 7:43 ` Christoph Hellwig 0 siblings, 2 replies; 22+ messages in thread From: Stephen Douthit @ 2019-08-08 20:24 UTC (permalink / raw) To: Jens Axboe; +Cc: Stephen Douthit, linux-ide, linux-kernel Intel moved the PCS register from 0x92 to 0x94 on Denverton for some reason, so now we get to check the device ID before poking it on reset. Signed-off-by: Stephen Douthit <stephend@silicom-usa.com> --- drivers/ata/ahci.c | 42 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c index f7652baa6337..7090c7754fc2 100644 --- a/drivers/ata/ahci.c +++ b/drivers/ata/ahci.c @@ -623,6 +623,41 @@ static void ahci_pci_save_initial_config(struct pci_dev *pdev, ahci_save_initial_config(&pdev->dev, hpriv); } +/* + * Intel moved the PCS register on the Denverton AHCI controller, see which + * offset this controller is using + */ +static int ahci_pcs_offset(struct ata_host *host) +{ + struct pci_dev *pdev = to_pci_dev(host->dev); + + switch (pdev->device) { + case 0x19b0: + case 0x19b1: + case 0x19b2: + case 0x19b3: + case 0x19b4: + case 0x19b5: + case 0x19b6: + case 0x19b7: + case 0x19bE: + case 0x19bF: + case 0x19c0: + case 0x19c1: + case 0x19c2: + case 0x19c3: + case 0x19c4: + case 0x19c5: + case 0x19c6: + case 0x19c7: + case 0x19cE: + case 0x19cF: + return 0x94; + } + + return 0x92; +} + static int ahci_pci_reset_controller(struct ata_host *host) { struct pci_dev *pdev = to_pci_dev(host->dev); @@ -634,13 +669,14 @@ static int ahci_pci_reset_controller(struct ata_host *host) if (pdev->vendor == PCI_VENDOR_ID_INTEL) { struct ahci_host_priv *hpriv = host->private_data; + int pcs = ahci_pcs_offset(host); u16 tmp16; /* configure PCS */ - pci_read_config_word(pdev, 0x92, &tmp16); + pci_read_config_word(pdev, pcs, &tmp16); if ((tmp16 & hpriv->port_map) != hpriv->port_map) { - tmp16 |= hpriv->port_map; - pci_write_config_word(pdev, 0x92, tmp16); + tmp16 |= hpriv->port_map & 0xff; + pci_write_config_word(pdev, pcs, tmp16); } } -- 2.21.0 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID 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-10 7:43 ` Christoph Hellwig 1 sibling, 1 reply; 22+ messages in thread From: Jens Axboe @ 2019-08-09 3:46 UTC (permalink / raw) To: Stephen Douthit; +Cc: linux-ide, linux-kernel On 8/8/19 1:24 PM, Stephen Douthit wrote: > Intel moved the PCS register from 0x92 to 0x94 on Denverton for some > reason, so now we get to check the device ID before poking it on reset. > > Signed-off-by: Stephen Douthit <stephend@silicom-usa.com> > --- > drivers/ata/ahci.c | 42 +++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 39 insertions(+), 3 deletions(-) > > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c > index f7652baa6337..7090c7754fc2 100644 > --- a/drivers/ata/ahci.c > +++ b/drivers/ata/ahci.c > @@ -623,6 +623,41 @@ static void ahci_pci_save_initial_config(struct pci_dev *pdev, > ahci_save_initial_config(&pdev->dev, hpriv); > } > > +/* > + * Intel moved the PCS register on the Denverton AHCI controller, see which > + * offset this controller is using > + */ > +static int ahci_pcs_offset(struct ata_host *host) > +{ > + struct pci_dev *pdev = to_pci_dev(host->dev); > + > + switch (pdev->device) { > + case 0x19b0: > + case 0x19b1: > + case 0x19b2: > + case 0x19b3: > + case 0x19b4: > + case 0x19b5: > + case 0x19b6: > + case 0x19b7: > + case 0x19bE: > + case 0x19bF: > + case 0x19c0: > + case 0x19c1: > + case 0x19c2: > + case 0x19c3: > + case 0x19c4: > + case 0x19c5: > + case 0x19c6: > + case 0x19c7: > + case 0x19cE: > + case 0x19cF: Any particular reason why you made some of hex alphas upper case? -- Jens Axboe ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID 2019-08-09 3:46 ` Jens Axboe @ 2019-08-09 14:13 ` Stephen Douthit 2019-08-09 14:16 ` Jens Axboe 0 siblings, 1 reply; 22+ messages in thread From: Stephen Douthit @ 2019-08-09 14:13 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-ide, linux-kernel On 8/8/19 11:46 PM, Jens Axboe wrote: > On 8/8/19 1:24 PM, Stephen Douthit wrote: >> Intel moved the PCS register from 0x92 to 0x94 on Denverton for some >> reason, so now we get to check the device ID before poking it on reset. >> >> Signed-off-by: Stephen Douthit <stephend@silicom-usa.com> >> --- >> drivers/ata/ahci.c | 42 +++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 39 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c >> index f7652baa6337..7090c7754fc2 100644 >> --- a/drivers/ata/ahci.c >> +++ b/drivers/ata/ahci.c >> @@ -623,6 +623,41 @@ static void ahci_pci_save_initial_config(struct pci_dev *pdev, >> ahci_save_initial_config(&pdev->dev, hpriv); >> } >> >> +/* >> + * Intel moved the PCS register on the Denverton AHCI controller, see which >> + * offset this controller is using >> + */ >> +static int ahci_pcs_offset(struct ata_host *host) >> +{ >> + struct pci_dev *pdev = to_pci_dev(host->dev); >> + >> + switch (pdev->device) { >> + case 0x19b0: >> + case 0x19b1: >> + case 0x19b2: >> + case 0x19b3: >> + case 0x19b4: >> + case 0x19b5: >> + case 0x19b6: >> + case 0x19b7: >> + case 0x19bE: >> + case 0x19bF: >> + case 0x19c0: >> + case 0x19c1: >> + case 0x19c2: >> + case 0x19c3: >> + case 0x19c4: >> + case 0x19c5: >> + case 0x19c6: >> + case 0x19c7: >> + case 0x19cE: >> + case 0x19cF: > > Any particular reason why you made some of hex alphas upper case? No good reason. These were copied from the ahci_pci_tbl above and I didn't notice the mixed case. I'll resend. Would you like a separate cleanup patch for ahci_pci_tbl as well? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID 2019-08-09 14:13 ` Stephen Douthit @ 2019-08-09 14:16 ` Jens Axboe 0 siblings, 0 replies; 22+ messages in thread From: Jens Axboe @ 2019-08-09 14:16 UTC (permalink / raw) To: Stephen Douthit; +Cc: linux-ide, linux-kernel On 8/9/19 7:13 AM, Stephen Douthit wrote: > On 8/8/19 11:46 PM, Jens Axboe wrote: >> On 8/8/19 1:24 PM, Stephen Douthit wrote: >>> Intel moved the PCS register from 0x92 to 0x94 on Denverton for some >>> reason, so now we get to check the device ID before poking it on reset. >>> >>> Signed-off-by: Stephen Douthit <stephend@silicom-usa.com> >>> --- >>> drivers/ata/ahci.c | 42 +++++++++++++++++++++++++++++++++++++++--- >>> 1 file changed, 39 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c >>> index f7652baa6337..7090c7754fc2 100644 >>> --- a/drivers/ata/ahci.c >>> +++ b/drivers/ata/ahci.c >>> @@ -623,6 +623,41 @@ static void ahci_pci_save_initial_config(struct pci_dev *pdev, >>> ahci_save_initial_config(&pdev->dev, hpriv); >>> } >>> >>> +/* >>> + * Intel moved the PCS register on the Denverton AHCI controller, see which >>> + * offset this controller is using >>> + */ >>> +static int ahci_pcs_offset(struct ata_host *host) >>> +{ >>> + struct pci_dev *pdev = to_pci_dev(host->dev); >>> + >>> + switch (pdev->device) { >>> + case 0x19b0: >>> + case 0x19b1: >>> + case 0x19b2: >>> + case 0x19b3: >>> + case 0x19b4: >>> + case 0x19b5: >>> + case 0x19b6: >>> + case 0x19b7: >>> + case 0x19bE: >>> + case 0x19bF: >>> + case 0x19c0: >>> + case 0x19c1: >>> + case 0x19c2: >>> + case 0x19c3: >>> + case 0x19c4: >>> + case 0x19c5: >>> + case 0x19c6: >>> + case 0x19c7: >>> + case 0x19cE: >>> + case 0x19cF: >> >> Any particular reason why you made some of hex alphas upper case? > > No good reason. These were copied from the ahci_pci_tbl above and I > didn't notice the mixed case. Ah I see. > I'll resend. > > Would you like a separate cleanup patch for ahci_pci_tbl as well? Yes please, I have a hard time reading the weird mixed case. -- Jens Axboe ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID 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-10 7:43 ` Christoph Hellwig 2019-08-10 20:22 ` Dan Williams 2019-08-12 13:02 ` Stephen Douthit 1 sibling, 2 replies; 22+ messages in thread From: Christoph Hellwig @ 2019-08-10 7:43 UTC (permalink / raw) To: Stephen Douthit; +Cc: Jens Axboe, linux-ide, linux-kernel, Dan Williams On Thu, Aug 08, 2019 at 08:24:31PM +0000, Stephen Douthit wrote: > Intel moved the PCS register from 0x92 to 0x94 on Denverton for some > reason, so now we get to check the device ID before poking it on reset. And now you just match on the new IDs, which means we'll perpetually catch up on any new device. Dan, can you reach out inside Intel to figure out if there is a way to find out the PCS register location without the PCI ID check? > static int ahci_pci_reset_controller(struct ata_host *host) > { > struct pci_dev *pdev = to_pci_dev(host->dev); > @@ -634,13 +669,14 @@ static int ahci_pci_reset_controller(struct ata_host *host) > > if (pdev->vendor == PCI_VENDOR_ID_INTEL) { > struct ahci_host_priv *hpriv = host->private_data; > + int pcs = ahci_pcs_offset(host); > u16 tmp16; > > /* configure PCS */ > - pci_read_config_word(pdev, 0x92, &tmp16); > + pci_read_config_word(pdev, pcs, &tmp16); > if ((tmp16 & hpriv->port_map) != hpriv->port_map) { > - tmp16 |= hpriv->port_map; > - pci_write_config_word(pdev, 0x92, tmp16); > + tmp16 |= hpriv->port_map & 0xff; > + pci_write_config_word(pdev, pcs, tmp16); > } > } And Stephen, while you are at it, can you split this Intel-specific quirk into a separate helper? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID 2019-08-10 7:43 ` Christoph Hellwig @ 2019-08-10 20:22 ` Dan Williams 2019-08-12 13:02 ` Stephen Douthit 1 sibling, 0 replies; 22+ messages in thread From: Dan Williams @ 2019-08-10 20:22 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Stephen Douthit, Jens Axboe, linux-ide, linux-kernel On Sat, Aug 10, 2019 at 12:43 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Thu, Aug 08, 2019 at 08:24:31PM +0000, Stephen Douthit wrote: > > Intel moved the PCS register from 0x92 to 0x94 on Denverton for some > > reason, so now we get to check the device ID before poking it on reset. > > And now you just match on the new IDs, which means we'll perpetually > catch up on any new device. Dan, can you reach out inside Intel to > figure out if there is a way to find out the PCS register location > without the PCI ID check? I'll ask. One guess for now is that num_ports >= 8 indicates the new layout since the old layout ran out of space, but that might fall over if the SOC uses the new layout, but implements fewer ports. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID 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 1 sibling, 2 replies; 22+ messages in thread From: Stephen Douthit @ 2019-08-12 13:02 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Jens Axboe, linux-ide, linux-kernel, Dan Williams On 8/10/19 3:43 AM, Christoph Hellwig wrote: > On Thu, Aug 08, 2019 at 08:24:31PM +0000, Stephen Douthit wrote: >> Intel moved the PCS register from 0x92 to 0x94 on Denverton for some >> reason, so now we get to check the device ID before poking it on reset. > > And now you just match on the new IDs, which means we'll perpetually > catch up on any new device. Dan, can you reach out inside Intel to > figure out if there is a way to find out the PCS register location > without the PCI ID check? > > >> static int ahci_pci_reset_controller(struct ata_host *host) >> { >> struct pci_dev *pdev = to_pci_dev(host->dev); >> @@ -634,13 +669,14 @@ static int ahci_pci_reset_controller(struct ata_host *host) >> >> if (pdev->vendor == PCI_VENDOR_ID_INTEL) { >> struct ahci_host_priv *hpriv = host->private_data; >> + int pcs = ahci_pcs_offset(host); >> u16 tmp16; >> >> /* configure PCS */ >> - pci_read_config_word(pdev, 0x92, &tmp16); >> + pci_read_config_word(pdev, pcs, &tmp16); >> if ((tmp16 & hpriv->port_map) != hpriv->port_map) { >> - tmp16 |= hpriv->port_map; >> - pci_write_config_word(pdev, 0x92, tmp16); >> + tmp16 |= hpriv->port_map & 0xff; >> + pci_write_config_word(pdev, pcs, tmp16); >> } >> } > > And Stephen, while you are at it, can you split this Intel-specific > quirk into a separate helper? I can do that. I'll wait until we hear back from Dan if there's a better scheme than a device ID lookup. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID 2019-08-12 13:02 ` Stephen Douthit @ 2019-08-12 14:11 ` Jens Axboe 2019-08-12 16:29 ` Dan Williams 1 sibling, 0 replies; 22+ messages in thread From: Jens Axboe @ 2019-08-12 14:11 UTC (permalink / raw) To: Stephen Douthit, Christoph Hellwig; +Cc: linux-ide, linux-kernel, Dan Williams On 8/12/19 6:02 AM, Stephen Douthit wrote: > On 8/10/19 3:43 AM, Christoph Hellwig wrote: >> On Thu, Aug 08, 2019 at 08:24:31PM +0000, Stephen Douthit wrote: >>> Intel moved the PCS register from 0x92 to 0x94 on Denverton for some >>> reason, so now we get to check the device ID before poking it on reset. >> >> And now you just match on the new IDs, which means we'll perpetually >> catch up on any new device. Dan, can you reach out inside Intel to >> figure out if there is a way to find out the PCS register location >> without the PCI ID check? >> >> >>> static int ahci_pci_reset_controller(struct ata_host *host) >>> { >>> struct pci_dev *pdev = to_pci_dev(host->dev); >>> @@ -634,13 +669,14 @@ static int ahci_pci_reset_controller(struct ata_host *host) >>> >>> if (pdev->vendor == PCI_VENDOR_ID_INTEL) { >>> struct ahci_host_priv *hpriv = host->private_data; >>> + int pcs = ahci_pcs_offset(host); >>> u16 tmp16; >>> >>> /* configure PCS */ >>> - pci_read_config_word(pdev, 0x92, &tmp16); >>> + pci_read_config_word(pdev, pcs, &tmp16); >>> if ((tmp16 & hpriv->port_map) != hpriv->port_map) { >>> - tmp16 |= hpriv->port_map; >>> - pci_write_config_word(pdev, 0x92, tmp16); >>> + tmp16 |= hpriv->port_map & 0xff; >>> + pci_write_config_word(pdev, pcs, tmp16); >>> } >>> } >> >> And Stephen, while you are at it, can you split this Intel-specific >> quirk into a separate helper? > > I can do that. I'll wait until we hear back from Dan if there's a > better scheme than a device ID lookup. I'll drop the previous series for now. -- Jens Axboe ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID 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 1 sibling, 1 reply; 22+ messages in thread From: Dan Williams @ 2019-08-12 16:29 UTC (permalink / raw) To: Stephen Douthit; +Cc: Christoph Hellwig, Jens Axboe, linux-ide, linux-kernel On Mon, Aug 12, 2019 at 6:03 AM Stephen Douthit <stephend@silicom-usa.com> wrote: > > On 8/10/19 3:43 AM, Christoph Hellwig wrote: > > On Thu, Aug 08, 2019 at 08:24:31PM +0000, Stephen Douthit wrote: > >> Intel moved the PCS register from 0x92 to 0x94 on Denverton for some > >> reason, so now we get to check the device ID before poking it on reset. > > > > And now you just match on the new IDs, which means we'll perpetually > > catch up on any new device. Dan, can you reach out inside Intel to > > figure out if there is a way to find out the PCS register location > > without the PCI ID check? > > > > > >> static int ahci_pci_reset_controller(struct ata_host *host) > >> { > >> struct pci_dev *pdev = to_pci_dev(host->dev); > >> @@ -634,13 +669,14 @@ static int ahci_pci_reset_controller(struct ata_host *host) > >> > >> if (pdev->vendor == PCI_VENDOR_ID_INTEL) { > >> struct ahci_host_priv *hpriv = host->private_data; > >> + int pcs = ahci_pcs_offset(host); > >> u16 tmp16; > >> > >> /* configure PCS */ > >> - pci_read_config_word(pdev, 0x92, &tmp16); > >> + pci_read_config_word(pdev, pcs, &tmp16); > >> if ((tmp16 & hpriv->port_map) != hpriv->port_map) { > >> - tmp16 |= hpriv->port_map; > >> - pci_write_config_word(pdev, 0x92, tmp16); > >> + tmp16 |= hpriv->port_map & 0xff; > >> + pci_write_config_word(pdev, pcs, tmp16); > >> } > >> } > > > > And Stephen, while you are at it, can you split this Intel-specific > > quirk into a separate helper? > > I can do that. I'll wait until we hear back from Dan if there's a > better scheme than a device ID lookup. Do you see any behavior change in practice with this patch? It's purportedly to re-enable the ports after a reset, but that would only be needed if the entire pci device reset. In this path the reset is being performed via the host control register. That is only meant to touch mmio registers, not config registers. So, as far as I can see this register bit twiddling after reset has never been necessary. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID 2019-08-12 16:29 ` Dan Williams @ 2019-08-12 17:49 ` Stephen Douthit 2019-08-12 18:06 ` Christoph Hellwig 0 siblings, 1 reply; 22+ messages in thread From: Stephen Douthit @ 2019-08-12 17:49 UTC (permalink / raw) To: Dan Williams; +Cc: Christoph Hellwig, Jens Axboe, linux-ide, linux-kernel On 8/12/19 12:29 PM, Dan Williams wrote: > On Mon, Aug 12, 2019 at 6:03 AM Stephen Douthit > <stephend@silicom-usa.com> wrote: >> >> On 8/10/19 3:43 AM, Christoph Hellwig wrote: >>> On Thu, Aug 08, 2019 at 08:24:31PM +0000, Stephen Douthit wrote: >>>> Intel moved the PCS register from 0x92 to 0x94 on Denverton for some >>>> reason, so now we get to check the device ID before poking it on reset. >>> >>> And now you just match on the new IDs, which means we'll perpetually >>> catch up on any new device. Dan, can you reach out inside Intel to >>> figure out if there is a way to find out the PCS register location >>> without the PCI ID check? >>> >>> >>>> static int ahci_pci_reset_controller(struct ata_host *host) >>>> { >>>> struct pci_dev *pdev = to_pci_dev(host->dev); >>>> @@ -634,13 +669,14 @@ static int ahci_pci_reset_controller(struct ata_host *host) >>>> >>>> if (pdev->vendor == PCI_VENDOR_ID_INTEL) { >>>> struct ahci_host_priv *hpriv = host->private_data; >>>> + int pcs = ahci_pcs_offset(host); >>>> u16 tmp16; >>>> >>>> /* configure PCS */ >>>> - pci_read_config_word(pdev, 0x92, &tmp16); >>>> + pci_read_config_word(pdev, pcs, &tmp16); >>>> if ((tmp16 & hpriv->port_map) != hpriv->port_map) { >>>> - tmp16 |= hpriv->port_map; >>>> - pci_write_config_word(pdev, 0x92, tmp16); >>>> + tmp16 |= hpriv->port_map & 0xff; >>>> + pci_write_config_word(pdev, pcs, tmp16); >>>> } >>>> } >>> >>> And Stephen, while you are at it, can you split this Intel-specific >>> quirk into a separate helper? >> >> I can do that. I'll wait until we hear back from Dan if there's a >> better scheme than a device ID lookup. > > Do you see any behavior change in practice with this patch? It's > purportedly to re-enable the ports after a reset, but that would only > be needed if the entire pci device reset. In this path the reset is > being performed via the host control register. That is only meant to > touch mmio registers, not config registers. So, as far as I can see > this register bit twiddling after reset has never been necessary. Not on Denverton. I have seen AHCI reset issues on Avoton/Rangeley, but I'd have to go digging at this point to know for sure if they were fixed solely by the ahci_avn_hardreset() workaround, or that in combination with the existing PCS workaround. I found this not because of failure I saw in Linux, but because I was using the Linux driver as reference while debugging the u-boot AHCI driver. When I couldn't find config space offset 0x92 defined in the Denverton EDS I went digging, and that's where the patch comes from. I wasn't quickly able to find what chipset the PCS workaround was added for. If it's for an obsolete chipset then dropping this entirely would be cleaner. Does anyone know the background of the original PCS workaround? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID 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 0 siblings, 2 replies; 22+ messages in thread From: Christoph Hellwig @ 2019-08-12 18:06 UTC (permalink / raw) To: Stephen Douthit Cc: Dan Williams, Christoph Hellwig, Jens Axboe, linux-ide, linux-kernel 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@pobox.com> Date: Thu, 14 Oct 2004 16:11:44 -0400 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 --- drivers/scsi/ahci.c | 150 ++++++++++++++++++++++++++++++++------------ 1 file changed, 109 insertions(+), 41 deletions(-) diff --git a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c index e2fdeb8b857e..408ecc527b59 100644 --- a/drivers/scsi/ahci.c +++ b/drivers/scsi/ahci.c @@ -38,7 +38,7 @@ #include <asm/io.h> #define DRV_NAME "ahci" -#define DRV_VERSION "0.10" +#define DRV_VERSION "0.11" enum { @@ -52,6 +52,7 @@ enum { AHCI_PORT_PRIV_DMA_SZ = AHCI_CMD_SLOT_SZ + AHCI_CMD_TBL_SZ + AHCI_RX_FIS_SZ, AHCI_IRQ_ON_SG = (1 << 31), + AHCI_CMD_ATAPI = (1 << 5), AHCI_CMD_WRITE = (1 << 6), RX_FIS_D2H_REG = 0x40, /* offset of D2H Register FIS data */ @@ -82,8 +83,10 @@ enum { PORT_IRQ_MASK = 0x14, /* interrupt enable/disable mask */ PORT_CMD = 0x18, /* port command */ PORT_TFDATA = 0x20, /* taskfile data */ + PORT_SIG = 0x24, /* device TF signature */ PORT_CMD_ISSUE = 0x38, /* command issue */ PORT_SCR = 0x28, /* SATA phy register block */ + PORT_SCR_STAT = 0x28, /* SATA phy register: SStatus */ PORT_SCR_CTL = 0x2c, /* SATA phy register: SControl */ PORT_SCR_ERR = 0x30, /* SATA phy register: SError */ @@ -161,9 +164,9 @@ struct ahci_port_priv { static u32 ahci_scr_read (struct ata_port *ap, unsigned int sc_reg); static void ahci_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val); static int ahci_init_one (struct pci_dev *pdev, const struct pci_device_id *ent); -static void ahci_dma_setup(struct ata_queued_cmd *qc); -static void ahci_dma_start(struct ata_queued_cmd *qc); +static int ahci_qc_issue(struct ata_queued_cmd *qc); static irqreturn_t ahci_interrupt (int irq, void *dev_instance, struct pt_regs *regs); +static void ahci_phy_reset(struct ata_port *ap); static void ahci_irq_clear(struct ata_port *ap); static void ahci_eng_timeout(struct ata_port *ap); static int ahci_port_start(struct ata_port *ap); @@ -202,13 +205,12 @@ static struct ata_port_operations ahci_ops = { .tf_read = ahci_tf_read, .check_status = ahci_check_status, .exec_command = ahci_exec_command, + .dev_select = ata_noop_dev_select, - .phy_reset = sata_phy_reset, + .phy_reset = ahci_phy_reset, - .bmdma_setup = ahci_dma_setup, - .bmdma_start = ahci_dma_start, .qc_prep = ahci_qc_prep, - .qc_issue = ata_qc_issue_prot, + .qc_issue = ahci_qc_issue, .eng_timeout = ahci_eng_timeout, @@ -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, board_ahci }, { } /* terminate list */ }; @@ -292,6 +296,7 @@ static int ahci_port_start(struct ata_port *ap) rc = -ENOMEM; goto err_out_kfree; } + memset(mem, 0, AHCI_PORT_PRIV_DMA_SZ); pp->rx_fis = mem; pp->rx_fis_dma = mem_dma; @@ -302,7 +307,7 @@ static int ahci_port_start(struct ata_port *ap) pp->cmd_tbl = mem; pp->cmd_tbl_dma = mem_dma; - mem2 = mem + 128; + mem2 = mem + 0x80; pp->cmd_tbl_sg = mem2; mem += AHCI_CMD_TBL_SZ; @@ -398,6 +403,29 @@ static void ahci_scr_write (struct ata_port *ap, unsigned int sc_reg_in, writel(val, (void *) ap->ioaddr.scr_addr + (sc_reg * 4)); } +static void ahci_phy_reset(struct ata_port *ap) +{ + void __iomem *port_mmio = (void __iomem *) ap->ioaddr.cmd_addr; + struct ata_taskfile tf; + struct ata_device *dev = &ap->device[0]; + u32 tmp; + + __sata_phy_reset(ap); + + if (ap->flags & ATA_FLAG_PORT_DISABLED) + return; + + tmp = readl(port_mmio + PORT_SIG); + tf.lbah = (tmp >> 24) & 0xff; + tf.lbam = (tmp >> 16) & 0xff; + tf.lbal = (tmp >> 8) & 0xff; + tf.nsect = (tmp) & 0xff; + + dev->class = ata_dev_classify(&tf); + if (!ata_dev_present(dev)) + ata_port_disable(ap); +} + static u8 ahci_check_status(struct ata_port *ap) { void *mmio = (void *) ap->ioaddr.cmd_addr; @@ -424,7 +452,7 @@ static void ahci_fill_sg(struct ata_queued_cmd *qc) pp->cmd_tbl_sg[i].addr = cpu_to_le32(addr & 0xffffffff); pp->cmd_tbl_sg[i].addr_hi = cpu_to_le32((addr >> 16) >> 16); - pp->cmd_tbl_sg[i].flags_size = cpu_to_le32(sg_len); + pp->cmd_tbl_sg[i].flags_size = cpu_to_le32(sg_len - 1); } } @@ -442,6 +470,19 @@ static void ahci_qc_prep(struct ata_queued_cmd *qc) opts = (qc->n_elem << 16) | cmd_fis_len; if (qc->tf.flags & ATA_TFLAG_WRITE) opts |= AHCI_CMD_WRITE; + + switch (qc->tf.protocol) { + case ATA_PROT_ATAPI: + case ATA_PROT_ATAPI_NODATA: + case ATA_PROT_ATAPI_DMA: + opts |= AHCI_CMD_ATAPI; + break; + + default: + /* do nothing */ + break; + } + pp->cmd_slot->opts = cpu_to_le32(opts); pp->cmd_slot->status = 0; pp->cmd_slot->tbl_addr = cpu_to_le32(pp->cmd_tbl_dma & 0xffffffff); @@ -547,11 +588,12 @@ static inline int ahci_host_intr(struct ata_port *ap, struct ata_queued_cmd *qc) { void *mmio = ap->host_set->mmio_base; void *port_mmio = ahci_port_base(mmio, ap->port_no); - u32 status, ci; + u32 status, serr, ci; + + serr = readl(port_mmio + PORT_SCR_ERR); + writel(serr, port_mmio + PORT_SCR_ERR); status = readl(port_mmio + PORT_IRQ_STAT); - if (!status) - return 0; writel(status, port_mmio + PORT_IRQ_STAT); ci = readl(port_mmio + PORT_CMD_ISSUE); @@ -624,18 +666,15 @@ static irqreturn_t ahci_interrupt (int irq, void *dev_instance, struct pt_regs * return IRQ_RETVAL(handled); } -static void ahci_dma_setup(struct ata_queued_cmd *qc) -{ - /* do nothing... for now */ -} - -static void ahci_dma_start(struct ata_queued_cmd *qc) +static int ahci_qc_issue(struct ata_queued_cmd *qc) { struct ata_port *ap = qc->ap; void *mmio = (void *) ap->ioaddr.cmd_addr; writel(1, mmio + PORT_CMD_ISSUE); readl(mmio + PORT_CMD_ISSUE); /* flush */ + + return 0; } static void ahci_tf_read(struct ata_port *ap, struct ata_taskfile *tf) @@ -663,20 +702,30 @@ static void ahci_exec_command(struct ata_port *ap, struct ata_taskfile *tf) static void ahci_setup_port(struct ata_ioports *port, unsigned long base, unsigned int port_idx) { + VPRINTK("ENTER, base==0x%lx, port_idx %u\n", base, port_idx); base = ahci_port_base_ul(base, port_idx); + VPRINTK("base now==0x%lx\n", base); port->cmd_addr = base; port->scr_addr = base + PORT_SCR; + + VPRINTK("EXIT\n"); } 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); hpriv->cap = readl(mmio + HOST_CAP); hpriv->port_map = readl(mmio + HOST_PORTS_IMPL); - probe_ent->n_ports = hpriv->cap & 0x1f; + probe_ent->n_ports = (hpriv->cap & 0x1f) + 1; + + VPRINTK("cap 0x%x port_map 0x%x n_ports %d\n", + hpriv->cap, hpriv->port_map, probe_ent->n_ports); using_dac = hpriv->cap & HOST_CAP_64; if (using_dac && @@ -746,18 +794,18 @@ static int ahci_host_init(struct ata_probe_ent *probe_ent) } for (i = 0; i < probe_ent->n_ports; i++) { - void *port_mmio; - if (!(hpriv->port_map & (1 << i))) continue; port_mmio = ahci_port_base(mmio, i); + VPRINTK("mmio %p port_mmio %p\n", mmio, port_mmio); ahci_setup_port(&probe_ent->port[i], (unsigned long) mmio, i); /* make sure port is not active */ tmp = readl(port_mmio + PORT_CMD); + VPRINTK("PORT_CMD 0x%x\n", tmp); if (tmp & (PORT_CMD_LIST_ON | PORT_CMD_FIS_ON | PORT_CMD_FIS_RX | PORT_CMD_START)) { tmp &= ~(PORT_CMD_LIST_ON | PORT_CMD_FIS_ON | @@ -772,14 +820,27 @@ static int ahci_host_init(struct ata_probe_ent *probe_ent) schedule_timeout((HZ / 2) + 1); } + writel(PORT_CMD_SPIN_UP, port_mmio + PORT_CMD); + + j = 0; + while (j < 100) { + msleep(10); + tmp = readl(port_mmio + PORT_SCR_STAT); + if ((tmp & 0xf) == 0x3) + break; + j++; + } + + tmp = readl(port_mmio + PORT_SCR_ERR); + VPRINTK("PORT_SCR_ERR 0x%x\n", tmp); + writel(tmp, port_mmio + PORT_SCR_ERR); + /* ack any pending irq events for this port */ tmp = readl(port_mmio + PORT_IRQ_STAT); + VPRINTK("PORT_IRQ_STAT 0x%x\n", tmp); if (tmp) writel(tmp, port_mmio + PORT_IRQ_STAT); - tmp = readl(port_mmio + PORT_SCR_ERR); - writel(tmp, port_mmio + PORT_SCR_ERR); - writel(1 << i, mmio + HOST_IRQ_STAT); /* set irq mask (enables interrupts) */ @@ -787,7 +848,10 @@ static int ahci_host_init(struct ata_probe_ent *probe_ent) } tmp = readl(mmio + HOST_CTL); + VPRINTK("HOST_CTL 0x%x\n", tmp); writel(tmp | HOST_IRQ_EN, mmio + HOST_CTL); + tmp = readl(mmio + HOST_CTL); + VPRINTK("HOST_CTL 0x%x\n", tmp); pci_set_master(pdev); @@ -830,10 +894,12 @@ static void ahci_print_info(struct ata_probe_ent *probe_ent) "%u slots %u ports %s Gbps 0x%x impl\n" , pci_name(pdev), + (vers >> 24) & 0xff, (vers >> 16) & 0xff, (vers >> 8) & 0xff, vers & 0xff, + ((cap >> 8) & 0x1f) + 1, (cap & 0x1f) + 1, speed_s, @@ -872,6 +938,8 @@ static int ahci_init_one (struct pci_dev *pdev, const struct pci_device_id *ent) unsigned int board_idx = (unsigned int) ent->driver_data; int rc; + VPRINTK("ENTER\n"); + if (!printed_version++) printk(KERN_DEBUG DRV_NAME " version " DRV_VERSION "\n"); -- 2.20.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID 2019-08-12 18:06 ` Christoph Hellwig @ 2019-08-12 19:12 ` Stephen Douthit 2019-08-12 19:31 ` Dan Williams 1 sibling, 0 replies; 22+ messages in thread From: Stephen Douthit @ 2019-08-12 19:12 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Dan Williams, Jens Axboe, linux-ide, linux-kernel 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@pobox.com> > 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID 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 1 sibling, 1 reply; 22+ messages in thread From: Dan Williams @ 2019-08-12 19:31 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Stephen Douthit, Jens Axboe, linux-ide, linux-kernel On Mon, Aug 12, 2019 at 11:08 AM Christoph Hellwig <hch@infradead.org> 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@pobox.com> > Date: Thu, 14 Oct 2004 16:11:44 -0400 > 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 Ok, that at least matches the expectation that platform firmware initially enables the ports. However, it still leaves open the question of whether the PCS bits were actually not configured, or whether just the controller reset was needed. Certainly there is no reason to touch that configuration register after every controller reset (via the HOST_CTL mmio register) It seems platforms / controllers that fail to run the option-rom should be quirked by device-id, but the PCS register twiddling be removed for everyone else. "Card BIOS" to me implies devices with an Option-ROM BAR which I don't think modern devices have, so that might be a simple way to try to phase out this quirk going forward without regressing working setups that might be relying on this. Then again the driver is already depending on the number of enabled ports to be reliable before PCS is written, and the current driver does not attempt to enable ports that were not enabled previously. That tells me that if the PCS quirk ever mattered it would have already regressed when the driver switched from blindly writing 0xf to only setting the bits that were already set in ->port_map. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID 2019-08-12 19:31 ` Dan Williams @ 2019-08-13 7:29 ` Christoph Hellwig 2019-08-13 22:07 ` Dan Williams 0 siblings, 1 reply; 22+ messages in thread From: Christoph Hellwig @ 2019-08-13 7:29 UTC (permalink / raw) To: Dan Williams Cc: Christoph Hellwig, Stephen Douthit, Jens Axboe, linux-ide, linux-kernel On Mon, Aug 12, 2019 at 12:31:35PM -0700, Dan Williams wrote: > It seems platforms / controllers that fail to run the option-rom > should be quirked by device-id, but the PCS register twiddling be > removed for everyone else. "Card BIOS" to me implies devices with an > Option-ROM BAR which I don't think modern devices have, so that might > be a simple way to try to phase out this quirk going forward without > regressing working setups that might be relying on this. > > Then again the driver is already depending on the number of enabled > ports to be reliable before PCS is written, and the current driver > does not attempt to enable ports that were not enabled previously. > That tells me that if the PCS quirk ever mattered it would have > already regressed when the driver switched from blindly writing 0xf to > only setting the bits that were already set in ->port_map. But how do we find that out? A compromise to me seems that we just do the PCS quirk for all Intel devices explicitly listed in the PCI Ids based on new board_* values as long as they have the old PCS location, and assume anything new enough to have the new location won't need to quirk, given that it never properly worked. This might miss some intel devices that were supported with the class based catchall, though. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID 2019-08-13 7:29 ` Christoph Hellwig @ 2019-08-13 22:07 ` Dan Williams 2019-08-14 14:34 ` Stephen Douthit 0 siblings, 1 reply; 22+ messages in thread From: Dan Williams @ 2019-08-13 22:07 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Stephen Douthit, Jens Axboe, linux-ide, linux-kernel On Tue, Aug 13, 2019 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote: > > On Mon, Aug 12, 2019 at 12:31:35PM -0700, Dan Williams wrote: > > It seems platforms / controllers that fail to run the option-rom > > should be quirked by device-id, but the PCS register twiddling be > > removed for everyone else. "Card BIOS" to me implies devices with an > > Option-ROM BAR which I don't think modern devices have, so that might > > be a simple way to try to phase out this quirk going forward without > > regressing working setups that might be relying on this. > > > > Then again the driver is already depending on the number of enabled > > ports to be reliable before PCS is written, and the current driver > > does not attempt to enable ports that were not enabled previously. > > That tells me that if the PCS quirk ever mattered it would have > > already regressed when the driver switched from blindly writing 0xf to > > only setting the bits that were already set in ->port_map. > > But how do we find that out? We can layer another assumption on top of Tejun's assumptions from commit 49f290903935 "ahci: update PCS programming". The kernel community has not received any regression reports from that change which says: " 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. ICH6/7M are probably the only ones where non-contiguous enable bits are necessary && PORTS_IMPL isn't programmed properly but they're proven to work reliably with 0xf anyway. " So the potential options I see are: 1/ Keep the current scheme, but limit it to cases where PORTS_IMPL is less than 8 and assume this need to set the bits is unnecessary legacy to carry forward 2/ Option1 + additionally use PORTS_IMPL as a gate to guess when the PCS format might be different for values >= 8. I think the driver does not need to consider Option2 unless / until it encounters a platform where firmware does not "do the right thing", and given Denverton has been in the wild with the wrong PCS twiddling it seems to suggest nothing needs to be done there. > A compromise to me seems that we just do the PCS quirk for all Intel > devices explicitly listed in the PCI Ids based on new board_* values > as long as they have the old PCS location, and assume anything new > enough to have the new location won't need to quirk, given that it > never properly worked. This might miss some intel devices that were > supported with the class based catchall, though. I'd be more comfortable with PORT_IMPL as the deciding factor. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID 2019-08-13 22:07 ` Dan Williams @ 2019-08-14 14:34 ` Stephen Douthit 2019-08-14 16:09 ` Dan Williams 0 siblings, 1 reply; 22+ messages in thread From: Stephen Douthit @ 2019-08-14 14:34 UTC (permalink / raw) To: Dan Williams, Christoph Hellwig; +Cc: Jens Axboe, linux-ide, linux-kernel On 8/13/19 6:07 PM, Dan Williams wrote: > On Tue, Aug 13, 2019 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote: >> >> On Mon, Aug 12, 2019 at 12:31:35PM -0700, Dan Williams wrote: >>> It seems platforms / controllers that fail to run the option-rom >>> should be quirked by device-id, but the PCS register twiddling be >>> removed for everyone else. "Card BIOS" to me implies devices with an >>> Option-ROM BAR which I don't think modern devices have, so that might >>> be a simple way to try to phase out this quirk going forward without >>> regressing working setups that might be relying on this. >>> >>> Then again the driver is already depending on the number of enabled >>> ports to be reliable before PCS is written, and the current driver >>> does not attempt to enable ports that were not enabled previously. >>> That tells me that if the PCS quirk ever mattered it would have >>> already regressed when the driver switched from blindly writing 0xf to >>> only setting the bits that were already set in ->port_map. >> >> But how do we find that out? > > We can layer another assumption on top of Tejun's assumptions from > commit 49f290903935 "ahci: update PCS programming". The kernel > community has not received any regression reports from that change > which says: > > " > 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. ICH6/7M are probably the > only ones where non-contiguous enable bits are necessary && PORTS_IMPL > isn't programmed properly but they're proven to work reliably with 0xf > anyway. > " > > So the potential options I see are: > > 1/ Keep the current scheme, but limit it to cases where PORTS_IMPL is > less than 8 and assume this need to set the bits is unnecessary legacy > to carry forward > > 2/ Option1 + additionally use PORTS_IMPL as a gate to guess when the > PCS format might be different for values >= 8. > > I think the driver does not need to consider Option2 unless / until it > encounters a platform where firmware does not "do the right thing", > and given Denverton has been in the wild with the wrong PCS twiddling > it seems to suggest nothing needs to be done there. > >> A compromise to me seems that we just do the PCS quirk for all Intel >> devices explicitly listed in the PCI Ids based on new board_* values >> as long as they have the old PCS location, and assume anything new >> enough to have the new location won't need to quirk, given that it >> never properly worked. This might miss some intel devices that were >> supported with the class based catchall, though. > > I'd be more comfortable with PORT_IMPL as the deciding factor. Unfortunately we can't use CAP.NP or PORTS_IMPL for this heuristic. The problem is that BIOS should be setting the PORTS_IMPL bitmask to match which lanes have actually been connected on the board. So PORTS_IMPL can be < 8 even if the controller is new enough to potentially support > 8 and has the new config space layout. As proof here's the relevant ahci_print_info() output booting on a Denverton based box with 5 ports implemented: ahci 0000:00:14.0: AHCI 0001.0301 32 slots 5 ports 6 Gbps 0x7a impl SATA mode | \-PORTS_IMPL \-CAP.NP ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID 2019-08-14 14:34 ` Stephen Douthit @ 2019-08-14 16:09 ` Dan Williams 2019-08-14 16:54 ` Stephen Douthit 0 siblings, 1 reply; 22+ messages in thread From: Dan Williams @ 2019-08-14 16:09 UTC (permalink / raw) To: Stephen Douthit; +Cc: Christoph Hellwig, Jens Axboe, linux-ide, linux-kernel On Wed, Aug 14, 2019 at 7:34 AM Stephen Douthit <stephend@silicom-usa.com> wrote: > > On 8/13/19 6:07 PM, Dan Williams wrote: > > On Tue, Aug 13, 2019 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote: > >> > >> On Mon, Aug 12, 2019 at 12:31:35PM -0700, Dan Williams wrote: > >>> It seems platforms / controllers that fail to run the option-rom > >>> should be quirked by device-id, but the PCS register twiddling be > >>> removed for everyone else. "Card BIOS" to me implies devices with an > >>> Option-ROM BAR which I don't think modern devices have, so that might > >>> be a simple way to try to phase out this quirk going forward without > >>> regressing working setups that might be relying on this. > >>> > >>> Then again the driver is already depending on the number of enabled > >>> ports to be reliable before PCS is written, and the current driver > >>> does not attempt to enable ports that were not enabled previously. > >>> That tells me that if the PCS quirk ever mattered it would have > >>> already regressed when the driver switched from blindly writing 0xf to > >>> only setting the bits that were already set in ->port_map. > >> > >> But how do we find that out? > > > > We can layer another assumption on top of Tejun's assumptions from > > commit 49f290903935 "ahci: update PCS programming". The kernel > > community has not received any regression reports from that change > > which says: > > > > " > > 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. ICH6/7M are probably the > > only ones where non-contiguous enable bits are necessary && PORTS_IMPL > > isn't programmed properly but they're proven to work reliably with 0xf > > anyway. > > " > > > > So the potential options I see are: > > > > 1/ Keep the current scheme, but limit it to cases where PORTS_IMPL is > > less than 8 and assume this need to set the bits is unnecessary legacy > > to carry forward > > > > 2/ Option1 + additionally use PORTS_IMPL as a gate to guess when the > > PCS format might be different for values >= 8. > > > > I think the driver does not need to consider Option2 unless / until it > > encounters a platform where firmware does not "do the right thing", > > and given Denverton has been in the wild with the wrong PCS twiddling > > it seems to suggest nothing needs to be done there. > > > >> A compromise to me seems that we just do the PCS quirk for all Intel > >> devices explicitly listed in the PCI Ids based on new board_* values > >> as long as they have the old PCS location, and assume anything new > >> enough to have the new location won't need to quirk, given that it > >> never properly worked. This might miss some intel devices that were > >> supported with the class based catchall, though. > > > > I'd be more comfortable with PORT_IMPL as the deciding factor. > > Unfortunately we can't use CAP.NP or PORTS_IMPL for this heuristic. > > The problem is that BIOS should be setting the PORTS_IMPL bitmask to > match which lanes have actually been connected on the board. So > PORTS_IMPL can be < 8 even if the controller is new enough to > potentially support > 8 and has the new config space layout. > > As proof here's the relevant ahci_print_info() output booting on a > Denverton based box with 5 ports implemented: > > ahci 0000:00:14.0: AHCI 0001.0301 32 slots 5 ports 6 Gbps 0x7a impl SATA mode > | \-PORTS_IMPL > \-CAP.NP Ugh, ok, thanks for clarifying and my mistake for not realizing Denverton already violates that heuristic. So now I'm trying to grok Christoph's suggestion. Are you saying that all existing board-ids would assume old PCS format? That would mean that Denverton gets the wrong format via board_ahci via the class code matching? Maybe I'm not understanding the suggestion... Another option might be that controllers >= 1.3.1 will stop using the PCS twiddling, and then we go add a new board id where / if it happens to matter in practice. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID 2019-08-14 16:09 ` Dan Williams @ 2019-08-14 16:54 ` Stephen Douthit 2019-08-14 17:17 ` Dan Williams 0 siblings, 1 reply; 22+ messages in thread From: Stephen Douthit @ 2019-08-14 16:54 UTC (permalink / raw) To: Dan Williams; +Cc: Christoph Hellwig, Jens Axboe, linux-ide, linux-kernel On 8/14/19 12:09 PM, Dan Williams wrote: > On Wed, Aug 14, 2019 at 7:34 AM Stephen Douthit > <stephend@silicom-usa.com> wrote: >> >> On 8/13/19 6:07 PM, Dan Williams wrote: >>> On Tue, Aug 13, 2019 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote: >>>> >>>> On Mon, Aug 12, 2019 at 12:31:35PM -0700, Dan Williams wrote: >>>>> It seems platforms / controllers that fail to run the option-rom >>>>> should be quirked by device-id, but the PCS register twiddling be >>>>> removed for everyone else. "Card BIOS" to me implies devices with an >>>>> Option-ROM BAR which I don't think modern devices have, so that might >>>>> be a simple way to try to phase out this quirk going forward without >>>>> regressing working setups that might be relying on this. >>>>> >>>>> Then again the driver is already depending on the number of enabled >>>>> ports to be reliable before PCS is written, and the current driver >>>>> does not attempt to enable ports that were not enabled previously. >>>>> That tells me that if the PCS quirk ever mattered it would have >>>>> already regressed when the driver switched from blindly writing 0xf to >>>>> only setting the bits that were already set in ->port_map. >>>> >>>> But how do we find that out? >>> >>> We can layer another assumption on top of Tejun's assumptions from >>> commit 49f290903935 "ahci: update PCS programming". The kernel >>> community has not received any regression reports from that change >>> which says: >>> >>> " >>> 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. ICH6/7M are probably the >>> only ones where non-contiguous enable bits are necessary && PORTS_IMPL >>> isn't programmed properly but they're proven to work reliably with 0xf >>> anyway. >>> " >>> >>> So the potential options I see are: >>> >>> 1/ Keep the current scheme, but limit it to cases where PORTS_IMPL is >>> less than 8 and assume this need to set the bits is unnecessary legacy >>> to carry forward >>> >>> 2/ Option1 + additionally use PORTS_IMPL as a gate to guess when the >>> PCS format might be different for values >= 8. >>> >>> I think the driver does not need to consider Option2 unless / until it >>> encounters a platform where firmware does not "do the right thing", >>> and given Denverton has been in the wild with the wrong PCS twiddling >>> it seems to suggest nothing needs to be done there. >>> >>>> A compromise to me seems that we just do the PCS quirk for all Intel >>>> devices explicitly listed in the PCI Ids based on new board_* values >>>> as long as they have the old PCS location, and assume anything new >>>> enough to have the new location won't need to quirk, given that it >>>> never properly worked. This might miss some intel devices that were >>>> supported with the class based catchall, though. >>> >>> I'd be more comfortable with PORT_IMPL as the deciding factor. >> >> Unfortunately we can't use CAP.NP or PORTS_IMPL for this heuristic. >> >> The problem is that BIOS should be setting the PORTS_IMPL bitmask to >> match which lanes have actually been connected on the board. So >> PORTS_IMPL can be < 8 even if the controller is new enough to >> potentially support > 8 and has the new config space layout. >> >> As proof here's the relevant ahci_print_info() output booting on a >> Denverton based box with 5 ports implemented: >> >> ahci 0000:00:14.0: AHCI 0001.0301 32 slots 5 ports 6 Gbps 0x7a impl SATA mode >> | \-PORTS_IMPL >> \-CAP.NP > > Ugh, ok, thanks for clarifying and my mistake for not realizing > Denverton already violates that heuristic. > > So now I'm trying to grok Christoph's suggestion. Are you saying that > all existing board-ids would assume old PCS format? That would mean > that Denverton gets the wrong format via board_ahci via the class code > matching? Maybe I'm not understanding the suggestion... My understanding of Christoph's suggestion was that we create a new board_ahci_* entry (e.g. board_ahci_intel_legacy) to match in ahci_pci_tbl against those devices using the original config space layout where PCS is @ 0x92. Once we have that we can conditionally run the PCS poke code in ahci_pci_reset_controller() only for devices with that new board_id. We assume that all newer devices added to the table will not use the legacy board_id and not need to do PCS pokes. Right now there are entries for devices with both the new and old config space layout in ahci_pci_tbl. That means someone at Intel would need to go through each entry and mark it as new or old. Your point about devices matching solely on the class code still applies. > Another option might be that controllers >= 1.3.1 will stop using the > PCS twiddling, and then we go add a new board id where / if it happens > to matter in practice. 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID 2019-08-14 16:54 ` Stephen Douthit @ 2019-08-14 17:17 ` Dan Williams 2019-08-19 16:30 ` Stephen Douthit 0 siblings, 1 reply; 22+ messages in thread From: Dan Williams @ 2019-08-14 17:17 UTC (permalink / raw) To: Stephen Douthit; +Cc: Christoph Hellwig, Jens Axboe, linux-ide, linux-kernel On Wed, Aug 14, 2019 at 9:54 AM Stephen Douthit <stephend@silicom-usa.com> wrote: > > On 8/14/19 12:09 PM, Dan Williams wrote: > > On Wed, Aug 14, 2019 at 7:34 AM Stephen Douthit > > <stephend@silicom-usa.com> wrote: > >> > >> On 8/13/19 6:07 PM, Dan Williams wrote: > >>> On Tue, Aug 13, 2019 at 12:31 AM Christoph Hellwig <hch@infradead.org> wrote: > >>>> > >>>> On Mon, Aug 12, 2019 at 12:31:35PM -0700, Dan Williams wrote: > >>>>> It seems platforms / controllers that fail to run the option-rom > >>>>> should be quirked by device-id, but the PCS register twiddling be > >>>>> removed for everyone else. "Card BIOS" to me implies devices with an > >>>>> Option-ROM BAR which I don't think modern devices have, so that might > >>>>> be a simple way to try to phase out this quirk going forward without > >>>>> regressing working setups that might be relying on this. > >>>>> > >>>>> Then again the driver is already depending on the number of enabled > >>>>> ports to be reliable before PCS is written, and the current driver > >>>>> does not attempt to enable ports that were not enabled previously. > >>>>> That tells me that if the PCS quirk ever mattered it would have > >>>>> already regressed when the driver switched from blindly writing 0xf to > >>>>> only setting the bits that were already set in ->port_map. > >>>> > >>>> But how do we find that out? > >>> > >>> We can layer another assumption on top of Tejun's assumptions from > >>> commit 49f290903935 "ahci: update PCS programming". The kernel > >>> community has not received any regression reports from that change > >>> which says: > >>> > >>> " > >>> 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. ICH6/7M are probably the > >>> only ones where non-contiguous enable bits are necessary && PORTS_IMPL > >>> isn't programmed properly but they're proven to work reliably with 0xf > >>> anyway. > >>> " > >>> > >>> So the potential options I see are: > >>> > >>> 1/ Keep the current scheme, but limit it to cases where PORTS_IMPL is > >>> less than 8 and assume this need to set the bits is unnecessary legacy > >>> to carry forward > >>> > >>> 2/ Option1 + additionally use PORTS_IMPL as a gate to guess when the > >>> PCS format might be different for values >= 8. > >>> > >>> I think the driver does not need to consider Option2 unless / until it > >>> encounters a platform where firmware does not "do the right thing", > >>> and given Denverton has been in the wild with the wrong PCS twiddling > >>> it seems to suggest nothing needs to be done there. > >>> > >>>> A compromise to me seems that we just do the PCS quirk for all Intel > >>>> devices explicitly listed in the PCI Ids based on new board_* values > >>>> as long as they have the old PCS location, and assume anything new > >>>> enough to have the new location won't need to quirk, given that it > >>>> never properly worked. This might miss some intel devices that were > >>>> supported with the class based catchall, though. > >>> > >>> I'd be more comfortable with PORT_IMPL as the deciding factor. > >> > >> Unfortunately we can't use CAP.NP or PORTS_IMPL for this heuristic. > >> > >> The problem is that BIOS should be setting the PORTS_IMPL bitmask to > >> match which lanes have actually been connected on the board. So > >> PORTS_IMPL can be < 8 even if the controller is new enough to > >> potentially support > 8 and has the new config space layout. > >> > >> As proof here's the relevant ahci_print_info() output booting on a > >> Denverton based box with 5 ports implemented: > >> > >> ahci 0000:00:14.0: AHCI 0001.0301 32 slots 5 ports 6 Gbps 0x7a impl SATA mode > >> | \-PORTS_IMPL > >> \-CAP.NP > > > > Ugh, ok, thanks for clarifying and my mistake for not realizing > > Denverton already violates that heuristic. > > > > So now I'm trying to grok Christoph's suggestion. Are you saying that > > all existing board-ids would assume old PCS format? That would mean > > that Denverton gets the wrong format via board_ahci via the class code > > matching? Maybe I'm not understanding the suggestion... > > My understanding of Christoph's suggestion was that we create a new > board_ahci_* entry (e.g. board_ahci_intel_legacy) to match in > ahci_pci_tbl against those devices using the original config space > layout where PCS is @ 0x92. > > Once we have that we can conditionally run the PCS poke code in > ahci_pci_reset_controller() only for devices with that new board_id. We > assume that all newer devices added to the table will not use the legacy > board_id and not need to do PCS pokes. > > Right now there are entries for devices with both the new and old config > space layout in ahci_pci_tbl. That means someone at Intel would need > to go through each entry and mark it as new or old. > > Your point about devices matching solely on the class code still > applies. > > > Another option might be that controllers >= 1.3.1 will stop using the > > PCS twiddling, and then we go add a new board id where / if it happens > > to matter in practice. > > 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. 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. 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID 2019-08-14 17:17 ` Dan Williams @ 2019-08-19 16:30 ` Stephen Douthit 2019-08-20 2:17 ` Dan Williams 0 siblings, 1 reply; 22+ messages in thread From: Stephen Douthit @ 2019-08-19 16:30 UTC (permalink / raw) To: Dan Williams; +Cc: Christoph Hellwig, Jens Axboe, linux-ide, linux-kernel 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. > 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. 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID 2019-08-19 16:30 ` Stephen Douthit @ 2019-08-20 2:17 ` Dan Williams 2019-08-20 13:59 ` Stephen Douthit 0 siblings, 1 reply; 22+ messages in thread From: Dan Williams @ 2019-08-20 2:17 UTC (permalink / raw) To: Stephen Douthit; +Cc: Christoph Hellwig, Jens Axboe, linux-ide, linux-kernel 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. > > 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 */ + + /* + * 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; + + /* + * 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); + } +} + 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; -- 2.20.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] ata: ahci: Lookup PCS register offset based on PCI device ID 2019-08-20 2:17 ` Dan Williams @ 2019-08-20 13:59 ` Stephen Douthit 0 siblings, 0 replies; 22+ messages in thread From: Stephen Douthit @ 2019-08-20 13:59 UTC (permalink / raw) To: Dan Williams; +Cc: Christoph Hellwig, Jens Axboe, linux-ide, linux-kernel 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; > ^ permalink raw reply related [flat|nested] 22+ messages in thread
end of thread, other threads:[~2019-08-20 14:00 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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).