Skip to contentSkip to conversation

ata: Fixes related to edge trigger latch for ahci_xgene driver.

PATCH Superseded Suman Tripathi 2 participants 8 messages Jan 27, 2016 – Feb 1, 2016 libata

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

v2 cover ata: Fixes related to edge trigger latch for ahci_xgene driver.

This patch set implements a workaround for an errate in the APM X-Gene SATA host controller with edge interrupt. The HOST_IRQ_STAT misses the edge interrupt from the PORT_IRQ_STAT when clearing the HOST_IRQ_STAT and reporting the PORT_IRQ_STAT happens in same clock cycle. It also implements the capability to override generic interrupt handler.

v1 change:
* Implement the capability for LDD to override interrupt handler.

* ahci_xgene driver implements the edge trigger interrupt handler.

v2 change:
* Remove the AHCI_HFLAG_EDGE_IRQ frim libahci.

Suman Tripathi (3):
ata: Remove the AHCI_HFLAG_EDGE_IRQ support from libahci.
libahci: Implement the capability to override the generic ahci
interrupt handler.
ahci_xgene: Implement the workaround to fix the missing of edge
interrupt for HOST_IRQ_STAT.

 drivers/ata/ahci.h       |    5 ++-
 drivers/ata/ahci_xgene.c |   86 +++++++++++++++++++++++++++++++++++++++++++++-
 drivers/ata/libahci.c    |   53 +++++-----------------------
 3 files changed, 98 insertions(+), 46 deletions(-)
#1 lore raw

PATCH v2 1/3 ata: Remove the AHCI_HFLAG_EDGE_IRQ support from libahci.

This patch removes the AHCI_HFLAG_EDGE_IRQ support from libahci. The upcoming patches will have the flexibility to override irq handlers in the LLD's. Controller's implementing a EDGE trigger latch can implement their own interrupt handler inside the driver.

---
 drivers/ata/ahci.h       |    3 +--
 drivers/ata/ahci_xgene.c |    2 +-
 drivers/ata/libahci.c    |   40 ----------------------------------------
 3 files changed, 2 insertions(+), 43 deletions(-)
drivers/ata/ahci.h +1 -2
index a4faa43..e9b57e8 100644
@@ -240,8 +240,7 @@ enum {
 						        error-handling stage) */
 	AHCI_HFLAG_NO_DEVSLP		= (1 << 17), /* no device sleep */
 	AHCI_HFLAG_NO_FBS		= (1 << 18), /* no FBS */
-	AHCI_HFLAG_EDGE_IRQ		= (1 << 19), /* HOST_IRQ_STAT behaves as
-							Edge Triggered */
+
 #ifdef CONFIG_PCI_MSI
 	AHCI_HFLAG_MULTI_MSI		= (1 << 20), /* multiple PCI MSIs */
 	AHCI_HFLAG_MULTI_MSIX		= (1 << 21), /* per-port MSI-X */
drivers/ata/ahci_xgene.c +1 -1
index e2c6d9e..892ec74 100644
@@ -779,7 +779,7 @@ skip_clk_phy:
 		hpriv->flags = AHCI_HFLAG_NO_NCQ;
 		break;
 	case XGENE_AHCI_V2:
-		hpriv->flags |= AHCI_HFLAG_YES_FBS | AHCI_HFLAG_EDGE_IRQ;
+		hpriv->flags |= AHCI_HFLAG_YES_FBS;
 		break;
 	default:
 		break;
drivers/ata/libahci.c +0 -40
index eda3cf2..08be136 100644
@@ -1843,43 +1843,6 @@ static u32 ahci_handle_port_intr(struct ata_host *host, u32 irq_masked)
 	return handled;
 }
 
-static irqreturn_t ahci_single_edge_irq_intr(int irq, void *dev_instance)
-{
-	struct ata_host *host = dev_instance;
-	struct ahci_host_priv *hpriv;
-	unsigned int rc = 0;
-	void __iomem *mmio;
-	u32 irq_stat, irq_masked;
-
-	VPRINTK("ENTER\n");
-
-	hpriv = host->private_data;
-	mmio = hpriv->mmio;
-
-	/* sigh.  0xffffffff is a valid return from h/w */
-	irq_stat = readl(mmio + HOST_IRQ_STAT);
-	if (!irq_stat)
-		return IRQ_NONE;
-
-	irq_masked = irq_stat & hpriv->port_map;
-
-	spin_lock(&host->lock);
-
-	/*
-	 * HOST_IRQ_STAT behaves as edge triggered latch meaning that
-	 * it should be cleared before all the port events are cleared.
-	 */
-	writel(irq_stat, mmio + HOST_IRQ_STAT);
-
-	rc = ahci_handle_port_intr(host, irq_masked);
-
-	spin_unlock(&host->lock);
-
-	VPRINTK("EXIT\n");
-
-	return IRQ_RETVAL(rc);
-}
-
 static irqreturn_t ahci_single_level_irq_intr(int irq, void *dev_instance)
 {
 	struct ata_host *host = dev_instance;
@@ -2507,9 +2470,6 @@ int ahci_host_activate(struct ata_host *host, struct scsi_host_template *sht)
 
 	if (hpriv->flags & (AHCI_HFLAG_MULTI_MSI | AHCI_HFLAG_MULTI_MSIX))
 		rc = ahci_host_activate_multi_irqs(host, sht);
-	else if (hpriv->flags & AHCI_HFLAG_EDGE_IRQ)
-		rc = ata_host_activate(host, irq, ahci_single_edge_irq_intr,
-				       IRQF_SHARED, sht);
 	else
 		rc = ata_host_activate(host, irq, ahci_single_level_irq_intr,
 				       IRQF_SHARED, sht);

--
1.7.1

#2 lore raw

PATCH v2 1/3 ata: Remove the AHCI_HFLAG_EDGE_IRQ support from libahci.

Quoted 6 lines · Suman Tripathi

This patch removes the AHCI_HFLAG_EDGE_IRQ support from libahci. The upcoming patches will have the flexibility to override irq handlers in the LLD's. Controller's implementing a EDGE trigger latch can implement their own interrupt handler inside the driver.

Shouldn't this patch come after the irq override is in place?

1. introduce irq override.

2. remove AHCI_HFLAG_EDGE_IRQ by moving the edge triggered handler to ahci_xgene.

3. Update xgene irq handler to address the erratum.

Thanks.

Signature · 1 line
tejun
#3 lore raw

PATCH v2 1/3 ata: Remove the AHCI_HFLAG_EDGE_IRQ support from libahci.

Quoted 8 lines · Tejun Heo
Quoted 6 lines · Suman Tripathi

This patch removes the AHCI_HFLAG_EDGE_IRQ support from libahci. The upcoming patches will have the flexibility to override irq handlers in the LLD's. Controller's implementing a EDGE trigger latch can implement their own interrupt handler inside the driver.

Quoted 8 lines

Shouldn't this patch come after the irq override is in place?

1. introduce irq override.

2. remove AHCI_HFLAG_EDGE_IRQ by moving the edge triggered handler to ahci_xgene.

3. Update xgene irq handler to address the erratum.

Thought of remove the AHCI_HFLAG_EDGE entirely first because we don't require it . Then implement the irq override capability.

Quoted 5 lines

Thanks.

--
tejun

Signature · 3 lines
Thanks,
with regards,
Suman Tripathi
#4 lore raw

PATCH v2 2/3 libahci: Implement the capability to override the generic ahci interrupt handler.

In reply to Suman Tripathi

This patch implements the capability to override the generic ahci interrupt handler so that LDD drivers can implement there own interrupt handler routines.

---
 drivers/ata/ahci.h    |    2 ++
 drivers/ata/libahci.c |   13 ++++++++++---
 2 files changed, 12 insertions(+), 3 deletions(-)
drivers/ata/ahci.h +2 -0
index e9b57e8..284ce3a 100644
@@ -359,6 +359,7 @@ struct ahci_host_priv {
 	 * be overridden anytime before the host is activated.
 	 */
 	void			(*start_engine)(struct ata_port *ap);
+	irqreturn_t 		(*ahci_irq_intr)(int irq, void *dev_instance);
 };
 
 #ifdef CONFIG_PCI_MSI
@@ -422,6 +423,7 @@ int ahci_reset_em(struct ata_host *host);
 void ahci_print_info(struct ata_host *host, const char *scc_s);
 int ahci_host_activate(struct ata_host *host, struct scsi_host_template *sht);
 void ahci_error_handler(struct ata_port *ap);
+u32 ahci_handle_port_intr(struct ata_host *host, u32 irq_masked);
 
 static inline void __iomem *__ahci_port_base(struct ata_host *host,
 					     unsigned int port_no)
drivers/ata/libahci.c +10 -3
index 08be136..fa098c6 100644
@@ -1816,7 +1816,7 @@ static irqreturn_t ahci_multi_irqs_intr_hard(int irq, void *dev_instance)
 	return IRQ_HANDLED;
 }
 
-static u32 ahci_handle_port_intr(struct ata_host *host, u32 irq_masked)
+u32 ahci_handle_port_intr(struct ata_host *host, u32 irq_masked)
 {
 	unsigned int i, handled = 0;
 
@@ -1842,6 +1842,7 @@ static u32 ahci_handle_port_intr(struct ata_host *host, u32 irq_masked)
 
 	return handled;
 }
+EXPORT_SYMBOL_GPL(ahci_handle_port_intr);
 
 static irqreturn_t ahci_single_level_irq_intr(int irq, void *dev_instance)
 {
@@ -2466,13 +2467,19 @@ int ahci_host_activate(struct ata_host *host, struct scsi_host_template *sht)
 {
 	struct ahci_host_priv *hpriv = host->private_data;
 	int irq = hpriv->irq;
+	irqreturn_t (*ahci_irq_handler)(int irq, void *dev_instance);
 	int rc;
 
 	if (hpriv->flags & (AHCI_HFLAG_MULTI_MSI | AHCI_HFLAG_MULTI_MSIX))
 		rc = ahci_host_activate_multi_irqs(host, sht);
-	else
-		rc = ata_host_activate(host, irq, ahci_single_level_irq_intr,
+	else {
+		ahci_irq_handler = hpriv->ahci_irq_intr ? hpriv->ahci_irq_intr :
+				   ahci_single_level_irq_intr;
+
+		rc = ata_host_activate(host, irq, ahci_irq_handler,
 				       IRQF_SHARED, sht);
+	}
+
 	return rc;
 }
 EXPORT_SYMBOL_GPL(ahci_host_activate);

--
1.7.1

#5 lore raw

PATCH v2 2/3 libahci: Implement the capability to override the generic ahci interrupt handler.

drivers/ata/libahci.c int ahci_host_activate(struct ata_host *host, struct scsi_host_template *sht) · line 2474

Quoted 9 lines · Suman Tripathi
drivers/ata/libahci.c +1 -0
@@ -2466,13 +2467,19 @@ int ahci_host_activate(struct ata_host *host, struct scsi_host_template *sht)
 {
 	struct ahci_host_priv *hpriv = host->private_data;
 	int irq = hpriv->irq;
+	irqreturn_t (*ahci_irq_handler)(int irq, void *dev_instance);
 	int rc;
 
 	if (hpriv->flags & (AHCI_HFLAG_MULTI_MSI | AHCI_HFLAG_MULTI_MSIX))
 		rc = ahci_host_activate_multi_irqs(host, sht);

You need to add { } to the if body too. Please read CodingStyle.

drivers/ata/libahci.c int ahci_host_activate(struct ata_host *host, struct scsi_host_template *sht) · line 2481

Quoted 9 lines
drivers/ata/libahci.c +6 -2
-	else
-		rc = ata_host_activate(host, irq, ahci_single_level_irq_intr,
+	else {
+		ahci_irq_handler = hpriv->ahci_irq_intr ? hpriv->ahci_irq_intr :
+				   ahci_single_level_irq_intr;
+
+		rc = ata_host_activate(host, irq, ahci_irq_handler,
 				       IRQF_SHARED, sht);
+	}

Can you please make this handled the same way as ->start_engine? Also, maybe print out a warning message if a driver specifies both AHCI_HFLAG_MULTI_MSI[X] and the irq handler override?

Thanks.

Signature · 1 line
tejun
#6 lore raw

PATCH v2 2/3 libahci: Implement the capability to override the generic ahci interrupt handler.

Quoted 23 lines · Tejun Heo

drivers/ata/libahci.c int ahci_host_activate(struct ata_host *host, struct scsi_host_template *sht) · line 2474

Quoted 9 lines · Suman Tripathi
drivers/ata/libahci.c +1 -0
@@ -2466,13 +2467,19 @@ int ahci_host_activate(struct ata_host *host, struct scsi_host_template *sht)
 {
      struct ahci_host_priv *hpriv = host->private_data;
      int irq = hpriv->irq;
+     irqreturn_t (*ahci_irq_handler)(int irq, void *dev_instance);
      int rc;
 
      if (hpriv->flags & (AHCI_HFLAG_MULTI_MSI | AHCI_HFLAG_MULTI_MSIX))
              rc = ahci_host_activate_multi_irqs(host, sht);

You need to add { } to the if body too. Please read CodingStyle.

drivers/ata/libahci.c int ahci_host_activate(struct ata_host *host, struct scsi_host_template *sht) · line 2481

Quoted 9 lines
drivers/ata/libahci.c +6 -2
-     else
-             rc = ata_host_activate(host, irq, ahci_single_level_irq_intr,
+     else {
+             ahci_irq_handler = hpriv->ahci_irq_intr ? hpriv->ahci_irq_intr :
+                                ahci_single_level_irq_intr;
+
+             rc = ata_host_activate(host, irq, ahci_irq_handler,
                                     IRQF_SHARED, sht);
+     }
Quoted 3 lines

Can you please make this handled the same way as ->start_engine? Also, maybe print out a warning message if a driver specifies both AHCI_HFLAG_MULTI_MSI[X] and the irq handler override?

Ok Sure

Quoted 5 lines

Thanks.

--
tejun

Signature · 3 lines
Thanks,
with regards,
Suman Tripathi
#7 lore raw

PATCH v2 3/3 ahci_xgene: Implement the workaround to fix the missing of edge interrupt for HOST_IRQ_STAT.

In reply to Suman Tripathi

Due to H/W errata, the HOST_IRQ_STAT register misses the edge interrupt when clearing the HOST_IRQ_STAT register and hardware reporting the PORT_IRQ_STAT register at the same clock cycle.

---
 drivers/ata/ahci_xgene.c |   84 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 84 insertions(+), 0 deletions(-)
drivers/ata/ahci_xgene.c +84 -0
index 892ec74..60076a3 100644
@@ -548,6 +548,89 @@ softreset_retry:
 	return rc;
 }
 
+/**
+ * xgene_ahci_handle_broken_edge_irq - Handle the broken irq.
+ * @ata_host: Host that recieved the irq
+ * @irq_masked: HOST_IRQ_STAT value
+ *
+ * For hardware with broken edge trigger latch
+ * the HOST_IRQ_STAT register misses the edge interrupt
+ * when clearing of HOST_IRQ_STAT register and hardware
+ * reporting the PORT_IRQ_STAT register at the
+ * same clock cycle.
+ * As such, the algorithm below outlines the workaround.
+ *
+ * 1. Read HOST_IRQ_STAT register and save the state.
+ * 2. Clear the HOST_IRQ_STAT register.
+ * 3. Read back the HOST_IRQ_STAT register.
+ * 4. If HOST_IRQ_STAT register equals to zero, then
+ *    traverse the rest of port's PORT_IRQ_STAT register
+ *    to check if an interrupt is triggered at that point else
+ *    go to step 6.
+ * 5. If PORT_IRQ_STAT register of rest ports is not equal to zero
+ *    then update the state of HOST_IRQ_STAT saved in step 1.
+ * 6. Handle port interrupts.
+ * 7. Exit
+ */
+static int xgene_ahci_handle_broken_edge_irq(struct ata_host *host,
+					     u32 irq_masked)
+{
+	struct ahci_host_priv *hpriv = host->private_data;
+	void __iomem *port_mmio;
+	int i;
+
+	if (!readl(hpriv->mmio + HOST_IRQ_STAT)) {
+		for (i = 0; i < host->n_ports; i++) {
+			if (irq_masked & (1 << i))
+				continue;
+
+			port_mmio = ahci_port_base(host->ports[i]);
+			if (readl(port_mmio + PORT_IRQ_STAT))
+				irq_masked |= (1 << i);
+		}
+	}
+
+	return ahci_handle_port_intr(host, irq_masked);
+}
+
+static irqreturn_t xgene_ahci_irq_intr(int irq, void *dev_instance)
+{
+	struct ata_host *host = dev_instance;
+	struct ahci_host_priv *hpriv;
+	unsigned int rc = 0;
+	void __iomem *mmio;
+	u32 irq_stat, irq_masked;
+
+	VPRINTK("ENTER\n");
+
+	hpriv = host->private_data;
+	mmio = hpriv->mmio;
+
+	/* sigh.  0xffffffff is a valid return from h/w */
+	irq_stat = readl(mmio + HOST_IRQ_STAT);
+	if (!irq_stat)
+		return IRQ_NONE;
+
+	irq_masked = irq_stat & hpriv->port_map;
+
+	spin_lock(&host->lock);
+
+	/*
+	 * HOST_IRQ_STAT behaves as edge triggered latch meaning that
+	 * it should be cleared before all the port events are cleared.
+	 */
+	writel(irq_stat, mmio + HOST_IRQ_STAT);
+
+	rc = xgene_ahci_handle_broken_edge_irq(host, irq_masked);
+
+	spin_unlock(&host->lock);
+
+	VPRINTK("EXIT\n");
+
+	return IRQ_RETVAL(rc);
+
+}
+
 static struct ata_port_operations xgene_ahci_v1_ops = {
 	.inherits = &ahci_ops,
 	.host_stop = xgene_ahci_host_stop,
@@ -780,6 +863,7 @@ skip_clk_phy:
 		break;
 	case XGENE_AHCI_V2:
 		hpriv->flags |= AHCI_HFLAG_YES_FBS;
+		hpriv->ahci_irq_intr = xgene_ahci_irq_intr;
 		break;
 	default:
 		break;

--
1.7.1