From: Robert Richter <[email protected]>
This patch adds generic support for MSI-X interrupts to the SATA PCI
driver. Only single interrupt support is implemented. Thus, per-port
interrupts can not yet be enabled.
The driver now checks the device for the existence of MSI-X and tries
to enable the interrupt. Otherwise, if a device is not MSI-X capable,
the initialization is skipped and MSI or intx interrupts are
configured.
This patch also enables AHCI for Cavium Thunder SoCs that uses MSI-X.
v2:
* determine irq vector from pci_dev->msi_list
Based on a patch from Sunil Goutham <[email protected]>.
Signed-off-by: Robert Richter <[email protected]>
---
arch/arm64/Kconfig | 1 +
drivers/ata/ahci.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++----
2 files changed, 75 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index b1f9a20a3677..2d95bbc0557e 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -157,6 +157,7 @@ config ARCH_THUNDER
bool "Cavium Inc. Thunder SoC Family"
help
This enables support for Cavium's Thunder Family of SoCs.
+ select SATA_AHCI
config ARCH_VEXPRESS
bool "ARMv8 software model (Versatile Express)"
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 49f1e6890587..36c88f6d8490 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -42,6 +42,7 @@
#include <linux/device.h>
#include <linux/dmi.h>
#include <linux/gfp.h>
+#include <linux/msi.h>
#include <scsi/scsi_host.h>
#include <scsi/scsi_cmnd.h>
#include <linux/libata.h>
@@ -52,6 +53,7 @@
enum {
AHCI_PCI_BAR_STA2X11 = 0,
+ AHCI_PCI_BAR_CAVIUM = 0,
AHCI_PCI_BAR_ENMOTUS = 2,
AHCI_PCI_BAR_STANDARD = 5,
};
@@ -500,6 +502,9 @@ static const struct pci_device_id ahci_pci_tbl[] = {
/* Enmotus */
{ PCI_DEVICE(0x1c44, 0x8000), board_ahci },
+ /* Cavium */
+ { PCI_DEVICE(0x177d, 0xa01c), .driver_data = board_ahci },
+
/* Generic, PCI class code for AHCI */
{ PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
PCI_CLASS_STORAGE_SATA_AHCI, 0xffffff, board_ahci },
@@ -1202,11 +1207,41 @@ static inline void ahci_gtf_filter_workaround(struct ata_host *host)
{}
#endif
-static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
- struct ahci_host_priv *hpriv)
+static int ahci_init_msix(struct pci_dev *pdev, unsigned int n_ports,
+ struct ahci_host_priv *hpriv)
+{
+ int rc, nvec;
+ struct msix_entry entry = {};
+
+ /* check if msix is supported */
+ nvec = pci_msix_vec_count(pdev);
+ if (nvec <= 0)
+ return 0;
+
+ /* per-port msix interrupts are not supported */
+ if (n_ports > 1 && nvec >= n_ports)
+ return -ENOSYS;
+
+ /* only enable the first entry (entry.entry = 0) */
+ rc = pci_enable_msix_exact(pdev, &entry, 1);
+ if (rc < 0)
+ return rc;
+
+ return 1;
+}
+
+static int __ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
+ struct ahci_host_priv *hpriv)
{
int rc, nvec;
+ nvec = ahci_init_msix(pdev, n_ports, hpriv);
+ if (nvec > 0)
+ return nvec;
+
+ if (nvec && nvec != -ENOSYS)
+ dev_err(&pdev->dev, "failed to enable MSI-X: %d", nvec);
+
if (hpriv->flags & AHCI_HFLAG_NO_MSI)
goto intx;
@@ -1250,6 +1285,35 @@ static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
return 0;
}
+static struct msi_desc *msix_get_desc(struct pci_dev *dev, u16 entry)
+{
+ struct msi_desc *desc;
+
+ list_for_each_entry(desc, &dev->msi_list, list) {
+ if (desc->msi_attrib.entry_nr == entry)
+ return desc;
+ }
+
+ return NULL;
+}
+
+static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
+ struct ahci_host_priv *hpriv)
+{
+ struct msi_desc *desc;
+
+ __ahci_init_interrupts(pdev, n_ports, hpriv);
+
+ if (!pdev->msix_enabled)
+ return pdev->irq;
+
+ desc = msix_get_desc(pdev, 0); /* first entry */
+ if (!desc)
+ return -ENODEV;
+
+ return desc->irq;
+}
+
static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
{
unsigned int board_id = ent->driver_data;
@@ -1260,6 +1324,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
struct ata_host *host;
int n_ports, i, rc;
int ahci_pci_bar = AHCI_PCI_BAR_STANDARD;
+ int irq;
VPRINTK("ENTER\n");
@@ -1285,11 +1350,13 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
dev_info(&pdev->dev,
"PDC42819 can only drive SATA devices with this driver\n");
- /* Both Connext and Enmotus devices use non-standard BARs */
+ /* Some devices use non-standard BARs */
if (pdev->vendor == PCI_VENDOR_ID_STMICRO && pdev->device == 0xCC06)
ahci_pci_bar = AHCI_PCI_BAR_STA2X11;
else if (pdev->vendor == 0x1c44 && pdev->device == 0x8000)
ahci_pci_bar = AHCI_PCI_BAR_ENMOTUS;
+ else if (pdev->vendor == 0x177d && pdev->device == 0xa01c)
+ ahci_pci_bar = AHCI_PCI_BAR_CAVIUM;
/*
* The JMicron chip 361/363 contains one SATA controller and one
@@ -1411,7 +1478,9 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
*/
n_ports = max(ahci_nr_ports(hpriv->cap), fls(hpriv->port_map));
- ahci_init_interrupts(pdev, n_ports, hpriv);
+ irq = ahci_init_interrupts(pdev, n_ports, hpriv);
+ if (irq < 0)
+ return irq;
host = ata_host_alloc_pinfo(&pdev->dev, ppi, n_ports);
if (!host)
@@ -1463,7 +1532,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
pci_set_master(pdev);
- return ahci_host_activate(host, pdev->irq, &ahci_sht);
+ return ahci_host_activate(host, irq, &ahci_sht);
}
module_pci_driver(ahci_pci_driver);
--
2.1.1
Hello, Robert. (cc'ing Alexander for ahci msi)
On Mon, May 04, 2015 at 09:45:37AM +0200, Robert Richter wrote:
> From: Robert Richter <[email protected]>
>
> This patch adds generic support for MSI-X interrupts to the SATA PCI
> driver. Only single interrupt support is implemented. Thus, per-port
> interrupts can not yet be enabled.
>
> The driver now checks the device for the existence of MSI-X and tries
> to enable the interrupt. Otherwise, if a device is not MSI-X capable,
> the initialization is skipped and MSI or intx interrupts are
> configured.
>
> This patch also enables AHCI for Cavium Thunder SoCs that uses MSI-X.
Please don't mix these two changes in the same patch.
> @@ -1202,11 +1207,41 @@ static inline void ahci_gtf_filter_workaround(struct ata_host *host)
> {}
> #endif
>
> -static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
> - struct ahci_host_priv *hpriv)
> +static int ahci_init_msix(struct pci_dev *pdev, unsigned int n_ports,
> + struct ahci_host_priv *hpriv)
> +{
> + int rc, nvec;
> + struct msix_entry entry = {};
> +
> + /* check if msix is supported */
> + nvec = pci_msix_vec_count(pdev);
> + if (nvec <= 0)
> + return 0;
> +
> + /* per-port msix interrupts are not supported */
> + if (n_ports > 1 && nvec >= n_ports)
> + return -ENOSYS;
Hmm... can you please elaborate why the condition isn't nvec > 1?
Also, shouldn't we be printing a warning message here explaining why
probing is failing?
> +
> + /* only enable the first entry (entry.entry = 0) */
> + rc = pci_enable_msix_exact(pdev, &entry, 1);
So, enabling the first msix works if nvec > 1 && nvec < n_ports but
not if nvec >= n_ports?
> + if (rc < 0)
> + return rc;
> +
> + return 1;
> +}
> +
> +static int __ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
> + struct ahci_host_priv *hpriv)
> {
> int rc, nvec;
>
> + nvec = ahci_init_msix(pdev, n_ports, hpriv);
> + if (nvec > 0)
> + return nvec;
> +
> + if (nvec && nvec != -ENOSYS)
> + dev_err(&pdev->dev, "failed to enable MSI-X: %d", nvec);
> +
> if (hpriv->flags & AHCI_HFLAG_NO_MSI)
> goto intx;
>
> @@ -1250,6 +1285,35 @@ static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
> return 0;
> }
>
> +static struct msi_desc *msix_get_desc(struct pci_dev *dev, u16 entry)
> +{
> + struct msi_desc *desc;
> +
> + list_for_each_entry(desc, &dev->msi_list, list) {
> + if (desc->msi_attrib.entry_nr == entry)
> + return desc;
> + }
> +
> + return NULL;
> +}
> +
> +static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
> + struct ahci_host_priv *hpriv)
> +{
> + struct msi_desc *desc;
> +
> + __ahci_init_interrupts(pdev, n_ports, hpriv);
> +
> + if (!pdev->msix_enabled)
> + return pdev->irq;
> +
> + desc = msix_get_desc(pdev, 0); /* first entry */
> + if (!desc)
> + return -ENODEV;
> +
> + return desc->irq;
> +}
Can we please do this properly? We should be able to move port priv
allocation to host allocaotion time and add and use pp->irq instead,
right?
Thanks.
--
tejun
Tejun,
thanks for your review and answer.
On 04.05.15 12:06:52, Tejun Heo wrote:
> > This patch also enables AHCI for Cavium Thunder SoCs that uses MSI-X.
>
> Please don't mix these two changes in the same patch.
I will split the patch.
> > + /* per-port msix interrupts are not supported */
> > + if (n_ports > 1 && nvec >= n_ports)
> > + return -ENOSYS;
>
> Hmm... can you please elaborate why the condition isn't nvec > 1?
I slightly changed the check and added a comment that explains that's
going on in the function. This is the new version:
static int ahci_init_msix(struct pci_dev *pdev, unsigned int n_ports,
struct ahci_host_priv *hpriv)
{
int rc, nvec;
struct msix_entry entry = {};
/* check if msix is supported */
nvec = pci_msix_vec_count(pdev);
if (nvec <= 0)
return 0;
/*
* Per-port msix interrupts are not supported. Assume single
* port interrupts for:
*
* n_ports == 1, or
* nvec < n_ports.
*
* We also need to check for n_ports != 0 which is implicitly
* covered here since nvec > 0.
*/
if (n_ports != 1 && nvec >= n_ports)
return -ENOSYS;
/*
* There can exist more than one vector (e.g. for error
* detection or hdd hotplug). Then the first vector is used,
* all others are ignored. Only enable the first entry here
* (entry.entry = 0).
*/
rc = pci_enable_msix_exact(pdev, &entry, 1);
if (rc < 0)
return rc;
return 1;
}
Note that the check changed to n_ports != 1 to also cover the case
n_ports == 0 which should return -ENOSYS.
> Also, shouldn't we be printing a warning message here explaining why
> probing is failing?
I didn't want to print a warning in case -ENOSYS for backward
compatability. Only if msi-x code fails there is a message, see
__ahci_init_interrupts(). In any other case the behaviour is as
before, thus no message is printed.
> > +
> > + /* only enable the first entry (entry.entry = 0) */
> > + rc = pci_enable_msix_exact(pdev, &entry, 1);
>
> So, enabling the first msix works if nvec > 1 && nvec < n_ports but
> not if nvec >= n_ports?
For n_ports > 1 && nvec >= n_ports we need to assume per-port
interrupts. There are enough vectors for all ports then.
> > + if (rc < 0)
> > + return rc;
> > +
> > + return 1;
> > +}
> > +
> > +static int __ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
> > + struct ahci_host_priv *hpriv)
> > {
> > int rc, nvec;
> >
> > + nvec = ahci_init_msix(pdev, n_ports, hpriv);
> > + if (nvec > 0)
> > + return nvec;
> > +
> > + if (nvec && nvec != -ENOSYS)
> > + dev_err(&pdev->dev, "failed to enable MSI-X: %d", nvec);
> > +
> > if (hpriv->flags & AHCI_HFLAG_NO_MSI)
> > goto intx;
> >
> > @@ -1250,6 +1285,35 @@ static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
> > return 0;
> > }
> >
> > +static struct msi_desc *msix_get_desc(struct pci_dev *dev, u16 entry)
> > +{
> > + struct msi_desc *desc;
> > +
> > + list_for_each_entry(desc, &dev->msi_list, list) {
> > + if (desc->msi_attrib.entry_nr == entry)
> > + return desc;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
> > + struct ahci_host_priv *hpriv)
> > +{
> > + struct msi_desc *desc;
> > +
> > + __ahci_init_interrupts(pdev, n_ports, hpriv);
> > +
> > + if (!pdev->msix_enabled)
> > + return pdev->irq;
> > +
> > + desc = msix_get_desc(pdev, 0); /* first entry */
> > + if (!desc)
> > + return -ENODEV;
> > +
> > + return desc->irq;
> > +}
>
> Can we please do this properly? We should be able to move port priv
> allocation to host allocaotion time and add and use pp->irq instead,
> right?
I started working implementing this.
Will send an updated patch set once finished.
Thanks,
-Robert
Tejun,
On 11.05.15 19:18:10, Robert Richter wrote:
> > > +static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
> > > + struct ahci_host_priv *hpriv)
> > > +{
> > > + struct msi_desc *desc;
> > > +
> > > + __ahci_init_interrupts(pdev, n_ports, hpriv);
> > > +
> > > + if (!pdev->msix_enabled)
> > > + return pdev->irq;
> > > +
> > > + desc = msix_get_desc(pdev, 0); /* first entry */
> > > + if (!desc)
> > > + return -ENODEV;
> > > +
> > > + return desc->irq;
> > > +}
> >
> > Can we please do this properly? We should be able to move port priv
> > allocation to host allocaotion time and add and use pp->irq instead,
> > right?
>
> I started working implementing this.
>
> Will send an updated patch set once finished.
A couple of questions here.
If we store the irq for each port separate in host->ports[i]->irq,
then there are duplicates for !AHCI_HFLAG_MULTI_MSI. We can not
iterate over all ports then to initialize the interrupt. Instead we
need to check for !AHCI_HFLAG_MULTI_MSI and store in that case the irq
in host->irq. This would avoid initializing duplicates and makes
host->ports[i]->irq only useful for multi-msi. Right now multi-msi
uses a base irq + index to register all irqs. This makes
host->ports[i]->irq obsolete at all.
Now, adding host->irq would change the function interface of
ahci_host_activate() to:
int ahci_host_activate(struct ata_host *host, struct scsi_host_template *sht);
I don't think this is worth the effort as all internal and external
drivers need to be changed basically from:
ahci_host_activate(host, irq, &ahci_sht);
to:
host->irq = irq;
ahci_host_activate(host, &ahci_sht);
This looks not very useful to do. Since irq is used only a single
time, there is no reason to store it in the host's data structure. It
also makes the interface more error prone since host->irq might not be
setup. Apart from that there is an abi change.
I agree that we will need the implemention of host->ports[i]->irq for
the case there irqs are no longer in sequential order as this might be
the case for per-port msi-x interrupts. But this is not the focus of
my implementation and as long there is no hardware for this available,
it wouldn't make sense to implement this at all.
So how to proceed? I could send you patches that implement host->irq
for a single per-host interrupt, and also one that reworks multi-port
interrupts to use host->ports[i]->irq. But I don't see any benefit
here. That said, I would better keep my patch here as it is. That do
you think?
Thanks,
-Robert
Hello, Robert.
On Mon, May 11, 2015 at 07:18:10PM +0200, Robert Richter wrote:
> static int ahci_init_msix(struct pci_dev *pdev, unsigned int n_ports,
> struct ahci_host_priv *hpriv)
> {
> int rc, nvec;
> struct msix_entry entry = {};
>
> /* check if msix is supported */
> nvec = pci_msix_vec_count(pdev);
> if (nvec <= 0)
> return 0;
>
> /*
> * Per-port msix interrupts are not supported. Assume single
> * port interrupts for:
> *
> * n_ports == 1, or
> * nvec < n_ports.
> *
> * We also need to check for n_ports != 0 which is implicitly
> * covered here since nvec > 0.
> */
> if (n_ports != 1 && nvec >= n_ports)
> return -ENOSYS;
Why are failing the whole thing when nvec >= n_ports? Can't we just
print some warning and configure it for single interrupt mode?
> > Also, shouldn't we be printing a warning message here explaining why
> > probing is failing?
>
> I didn't want to print a warning in case -ENOSYS for backward
> compatability. Only if msi-x code fails there is a message, see
> __ahci_init_interrupts(). In any other case the behaviour is as
> before, thus no message is printed.
I'm confused here. Why are we implementing msix support at all if it
only support single interrupt mode? I kinda assumed that that was
because you're trying to support a controller which does only msix,
no? At any rate, I don't think it's wrong to print an informational /
warning message when a controller declares msix support but has wacko
parameters.
> > > +
> > > + /* only enable the first entry (entry.entry = 0) */
> > > + rc = pci_enable_msix_exact(pdev, &entry, 1);
> >
> > So, enabling the first msix works if nvec > 1 && nvec < n_ports but
> > not if nvec >= n_ports?
>
> For n_ports > 1 && nvec >= n_ports we need to assume per-port
> interrupts. There are enough vectors for all ports then.
Again, and we fail irq init in that case?
Thanks.
--
tejun
Hello,
On Tue, May 12, 2015 at 01:46:47PM +0200, Robert Richter wrote:
> I don't think this is worth the effort as all internal and external
> drivers need to be changed basically from:
>
> ahci_host_activate(host, irq, &ahci_sht);
>
> to:
>
> host->irq = irq;
> ahci_host_activate(host, &ahci_sht);
>
> This looks not very useful to do. Since irq is used only a single
> time, there is no reason to store it in the host's data structure. It
Doesn't really matter tho.
> also makes the interface more error prone since host->irq might not be
> setup. Apart from that there is an abi change.
But large part of @host needs to be initialized before activation. I
don't think moving irq to that pool changes much if anything.
> I agree that we will need the implemention of host->ports[i]->irq for
> the case there irqs are no longer in sequential order as this might be
> the case for per-port msi-x interrupts. But this is not the focus of
> my implementation and as long there is no hardware for this available,
> it wouldn't make sense to implement this at all.
Why are we doing msix at all? I don't get it.
> So how to proceed? I could send you patches that implement host->irq
> for a single per-host interrupt, and also one that reworks multi-port
> interrupts to use host->ports[i]->irq. But I don't see any benefit
> here. That said, I would better keep my patch here as it is. That do
> you think?
Let's start with why we're doing this in the first place.
Thanks.
--
tejun
Tejun,
On 13.05.15 10:39:06, Tejun Heo wrote:
> On Tue, May 12, 2015 at 01:46:47PM +0200, Robert Richter wrote:
> > I don't think this is worth the effort as all internal and external
> > drivers need to be changed basically from:
> >
> > ahci_host_activate(host, irq, &ahci_sht);
> >
> > to:
> >
> > host->irq = irq;
> > ahci_host_activate(host, &ahci_sht);
> >
> > This looks not very useful to do. Since irq is used only a single
> > time, there is no reason to store it in the host's data structure. It
>
> Doesn't really matter tho.
Since ahci_host_activate() is EXPORT_SYMBOL_GPL I really have concerns
changing the i/f. But I will send you a patch for this.
> > also makes the interface more error prone since host->irq might not be
> > setup. Apart from that there is an abi change.
>
> But large part of @host needs to be initialized before activation. I
> don't think moving irq to that pool changes much if anything.
>
> > I agree that we will need the implemention of host->ports[i]->irq for
> > the case there irqs are no longer in sequential order as this might be
> > the case for per-port msi-x interrupts. But this is not the focus of
> > my implementation and as long there is no hardware for this available,
> > it wouldn't make sense to implement this at all.
>
> Why are we doing msix at all? I don't get it.
See below.
> > So how to proceed? I could send you patches that implement host->irq
> > for a single per-host interrupt, and also one that reworks multi-port
> > interrupts to use host->ports[i]->irq. But I don't see any benefit
> > here. That said, I would better keep my patch here as it is. That do
> > you think?
>
> Let's start with why we're doing this in the first place.
Right, the sata controller is connected to a pci ecam controller, both
are on an SoC together with the processor. There are no external pci
ports for the connection of external devices. Since all pci devices on
the chip support msi-x, the controller is only capable to handle this
and not INTx nor MSI. So for enabling of the sata hc we need msix
support.
-Robert
Hello, Robert.
On Wed, May 13, 2015 at 07:28:28PM +0200, Robert Richter wrote:
> > > This looks not very useful to do. Since irq is used only a single
> > > time, there is no reason to store it in the host's data structure. It
> >
> > Doesn't really matter tho.
>
> Since ahci_host_activate() is EXPORT_SYMBOL_GPL I really have concerns
> changing the i/f. But I will send you a patch for this.
It doesn't matter. Please go ahead and change it.
> > Let's start with why we're doing this in the first place.
>
> Right, the sata controller is connected to a pci ecam controller, both
> are on an SoC together with the processor. There are no external pci
> ports for the connection of external devices. Since all pci devices on
> the chip support msi-x, the controller is only capable to handle this
> and not INTx nor MSI. So for enabling of the sata hc we need msix
> support.
I see. If you can get hold of an ahci controller which actually can
do multi-irq msix, it'd be the best. If not, let's make it super
clear that this is a special case and use it as the last resort (which
also clers up the warning issue).
Thanks.
--
tejun
On 13.05.15 13:46:40, Tejun Heo wrote:
> Hello, Robert.
>
> On Wed, May 13, 2015 at 07:28:28PM +0200, Robert Richter wrote:
> > > > This looks not very useful to do. Since irq is used only a single
> > > > time, there is no reason to store it in the host's data structure. It
> > >
> > > Doesn't really matter tho.
> >
> > Since ahci_host_activate() is EXPORT_SYMBOL_GPL I really have concerns
> > changing the i/f. But I will send you a patch for this.
>
> It doesn't matter. Please go ahead and change it.
Ok, np.
> > > Let's start with why we're doing this in the first place.
> >
> > Right, the sata controller is connected to a pci ecam controller, both
> > are on an SoC together with the processor. There are no external pci
> > ports for the connection of external devices. Since all pci devices on
> > the chip support msi-x, the controller is only capable to handle this
> > and not INTx nor MSI. So for enabling of the sata hc we need msix
> > support.
>
> I see. If you can get hold of an ahci controller which actually can
> do multi-irq msix, it'd be the best. If not, let's make it super
> clear that this is a special case and use it as the last resort (which
> also clers up the warning issue).
This sounds good.
The device actually supports multi-irq msix, a single mode interrupt +
additional interrupts for error or hotplug handling. But I don't have
hardware for a per-port msix host controller.
So I will move msix after msi then and mark it as a special case if
msi is not supported. I don't want to move it after intx since this is
the fallback if nothing else works, there is no explicit check for
intx, I even don't know if that is possible at all.
Thanks,
-Robert
Hello,
On Wed, May 13, 2015 at 08:07:05PM +0200, Robert Richter wrote:
> So I will move msix after msi then and mark it as a special case if
> msi is not supported. I don't want to move it after intx since this is
> the fallback if nothing else works, there is no explicit check for
> intx, I even don't know if that is possible at all.
Sure, that sounds good to me.
Thanks.
--
tejun
On Mon, May 04, 2015 at 09:45:37AM +0200, Robert Richter wrote:
> From: Robert Richter <[email protected]>
>
> This patch adds generic support for MSI-X interrupts to the SATA PCI
> driver. Only single interrupt support is implemented. Thus, per-port
> interrupts can not yet be enabled.
>
> The driver now checks the device for the existence of MSI-X and tries
> to enable the interrupt. Otherwise, if a device is not MSI-X capable,
> the initialization is skipped and MSI or intx interrupts are
> configured.
Hi Robert,
Sorry for jumping in late.
I have took a cursory look at your code and have a major concern.
You enable MSI-X for a single chip, but introduce a change to the
generic AHCI code.
In general MSI-X case, what makes you believe that IRQ vectors are
assigned continuously?
(Interface ahci_host_activate() kinda expects a contiguous vector
range, but MSI-X does not guarantee that at all).
Thanks!
> This patch also enables AHCI for Cavium Thunder SoCs that uses MSI-X.
>
> v2:
> * determine irq vector from pci_dev->msi_list
>
> Based on a patch from Sunil Goutham <[email protected]>.
>
> Signed-off-by: Robert Richter <[email protected]>
> ---
> arch/arm64/Kconfig | 1 +
> drivers/ata/ahci.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++----
> 2 files changed, 75 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index b1f9a20a3677..2d95bbc0557e 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -157,6 +157,7 @@ config ARCH_THUNDER
> bool "Cavium Inc. Thunder SoC Family"
> help
> This enables support for Cavium's Thunder Family of SoCs.
> + select SATA_AHCI
>
> config ARCH_VEXPRESS
> bool "ARMv8 software model (Versatile Express)"
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 49f1e6890587..36c88f6d8490 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -42,6 +42,7 @@
> #include <linux/device.h>
> #include <linux/dmi.h>
> #include <linux/gfp.h>
> +#include <linux/msi.h>
> #include <scsi/scsi_host.h>
> #include <scsi/scsi_cmnd.h>
> #include <linux/libata.h>
> @@ -52,6 +53,7 @@
>
> enum {
> AHCI_PCI_BAR_STA2X11 = 0,
> + AHCI_PCI_BAR_CAVIUM = 0,
> AHCI_PCI_BAR_ENMOTUS = 2,
> AHCI_PCI_BAR_STANDARD = 5,
> };
> @@ -500,6 +502,9 @@ static const struct pci_device_id ahci_pci_tbl[] = {
> /* Enmotus */
> { PCI_DEVICE(0x1c44, 0x8000), board_ahci },
>
> + /* Cavium */
> + { PCI_DEVICE(0x177d, 0xa01c), .driver_data = board_ahci },
> +
> /* Generic, PCI class code for AHCI */
> { PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
> PCI_CLASS_STORAGE_SATA_AHCI, 0xffffff, board_ahci },
> @@ -1202,11 +1207,41 @@ static inline void ahci_gtf_filter_workaround(struct ata_host *host)
> {}
> #endif
>
> -static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
> - struct ahci_host_priv *hpriv)
> +static int ahci_init_msix(struct pci_dev *pdev, unsigned int n_ports,
> + struct ahci_host_priv *hpriv)
> +{
> + int rc, nvec;
> + struct msix_entry entry = {};
> +
> + /* check if msix is supported */
> + nvec = pci_msix_vec_count(pdev);
> + if (nvec <= 0)
> + return 0;
> +
> + /* per-port msix interrupts are not supported */
> + if (n_ports > 1 && nvec >= n_ports)
> + return -ENOSYS;
> +
> + /* only enable the first entry (entry.entry = 0) */
> + rc = pci_enable_msix_exact(pdev, &entry, 1);
> + if (rc < 0)
> + return rc;
> +
> + return 1;
> +}
> +
> +static int __ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
> + struct ahci_host_priv *hpriv)
> {
> int rc, nvec;
>
> + nvec = ahci_init_msix(pdev, n_ports, hpriv);
> + if (nvec > 0)
> + return nvec;
> +
> + if (nvec && nvec != -ENOSYS)
> + dev_err(&pdev->dev, "failed to enable MSI-X: %d", nvec);
> +
> if (hpriv->flags & AHCI_HFLAG_NO_MSI)
> goto intx;
>
> @@ -1250,6 +1285,35 @@ static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
> return 0;
> }
>
> +static struct msi_desc *msix_get_desc(struct pci_dev *dev, u16 entry)
> +{
> + struct msi_desc *desc;
> +
> + list_for_each_entry(desc, &dev->msi_list, list) {
> + if (desc->msi_attrib.entry_nr == entry)
> + return desc;
> + }
> +
> + return NULL;
> +}
> +
> +static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
> + struct ahci_host_priv *hpriv)
> +{
> + struct msi_desc *desc;
> +
> + __ahci_init_interrupts(pdev, n_ports, hpriv);
> +
> + if (!pdev->msix_enabled)
> + return pdev->irq;
> +
> + desc = msix_get_desc(pdev, 0); /* first entry */
> + if (!desc)
> + return -ENODEV;
> +
> + return desc->irq;
> +}
> +
> static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
> unsigned int board_id = ent->driver_data;
> @@ -1260,6 +1324,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> struct ata_host *host;
> int n_ports, i, rc;
> int ahci_pci_bar = AHCI_PCI_BAR_STANDARD;
> + int irq;
>
> VPRINTK("ENTER\n");
>
> @@ -1285,11 +1350,13 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> dev_info(&pdev->dev,
> "PDC42819 can only drive SATA devices with this driver\n");
>
> - /* Both Connext and Enmotus devices use non-standard BARs */
> + /* Some devices use non-standard BARs */
> if (pdev->vendor == PCI_VENDOR_ID_STMICRO && pdev->device == 0xCC06)
> ahci_pci_bar = AHCI_PCI_BAR_STA2X11;
> else if (pdev->vendor == 0x1c44 && pdev->device == 0x8000)
> ahci_pci_bar = AHCI_PCI_BAR_ENMOTUS;
> + else if (pdev->vendor == 0x177d && pdev->device == 0xa01c)
> + ahci_pci_bar = AHCI_PCI_BAR_CAVIUM;
>
> /*
> * The JMicron chip 361/363 contains one SATA controller and one
> @@ -1411,7 +1478,9 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> */
> n_ports = max(ahci_nr_ports(hpriv->cap), fls(hpriv->port_map));
>
> - ahci_init_interrupts(pdev, n_ports, hpriv);
> + irq = ahci_init_interrupts(pdev, n_ports, hpriv);
> + if (irq < 0)
> + return irq;
>
> host = ata_host_alloc_pinfo(&pdev->dev, ppi, n_ports);
> if (!host)
> @@ -1463,7 +1532,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>
> pci_set_master(pdev);
>
> - return ahci_host_activate(host, pdev->irq, &ahci_sht);
> + return ahci_host_activate(host, irq, &ahci_sht);
> }
>
> module_pci_driver(ahci_pci_driver);
> --
> 2.1.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Regards,
Alexander Gordeev
[email protected]
On 17.05.15 08:33:32, Alexander Gordeev wrote:
> You enable MSI-X for a single chip, but introduce a change to the
> generic AHCI code.
>
> In general MSI-X case, what makes you believe that IRQ vectors are
> assigned continuously?
>
> (Interface ahci_host_activate() kinda expects a contiguous vector
> range, but MSI-X does not guarantee that at all).
That's the reason why I only enable single interrupt mode which our
hardware supports.
To not break other chips by this generic code change, there are the
following precautions:
* Interrupt ranges are not enabled at all.
* Only single interrupt mode is enabled for msix cap devices. These
devices require a single port only or a total number of int entries
less than the total number of ports. In this case only one
interrupt will be enabled.
* During the discussion with Tejun we agreed to change the init
sequence from msix-msi-intx to msi-msix-intx. Thus, if a device
offers msi and init does not fail, the msix init code will not be
executed. This is equivalent to current code.
With this, the code only setups single mode msix as a last resort if
msi fails. No interrupt range is enabled at all. Only one interrupt
will be enabled. Considering all this I think your concerns are
addressed.
Also, the code can be easily extended for other devices and thus
should be generic from the beginning.
-Robert