[PATCH v5 0/7] Armada8k enable per-port SATA interrupts and drop a hack in the IRQ subsystem

[PATCH v5 0/7] Armada8k enable per-port SATA interrupts and drop a hack in the IRQ subsystem

[sven.auhagen]

From: Sven Auhagen <sven.auhagen@voleatech.de>

Hello,

there were already 4 versions of this series from Miquèl.
I talked to Miquèl and I fixed up the last comments from v4.
I am looking for feedback if this patch series is now ready to be merged
and what should be further changed.

Here is the original cover letter:

Some time ago, when the initial support for Armada CP110 was
contributed, the SATA core was not able to handle per-port
interrupts. Despite the hardware reality, the device tree only
represents one main interrupt for the two ports. Having both SATA
ports enabled at the same time has been achieved by a hack in the ICU
driver(1) that faked the use of the two interrupts, no matter which
SATA port was in use.

Now that the SATA core is ready to handle more than one interrupt,
this series adds support for it in the libahci_platform code. The
CP110 device tree must be updated to reflect the two SATA ports
available and their respective interrupts. To do not break DT backward
compatibility, the ahci_platform driver now embeds a special quirk
which checks if the DT is valid (only for A8k compatible) and, if
needed, creates the two missing sub-nodes, and assign them the
relevant "reg" and "interrupts" properties, before removing the main
SATA node "interrupts" one.

(1) The ICU is an irqchip aggregating the CP110 (south-bridge)
interrupts into MSIs for the AP806 (north-bridge).

Best
Sven

Changes in v5
=============
* Rebase on top of v5.8-rc3
* Move code to ahci mvebu and use custom functions

Changes in v4
=============
* Rebase on top of v5.2-rc1
* s/ARM64/arm64/ in the DT change commit title.

Changes in v3
=============
* Removed useless locking when acking the SATA host IRQ_STAT bit.
* As spotted by Hans, do not enable the AHCI_HFLAG_MULTI_MSI flag only
  if more than one port is enabled, DTs might declare only one
  interrupt and have multiple ports. Added Hans' Reviewed-by tag.
* Added Rob's Reviewed-by tags.
* Added explicit references to the 'Device Tree' being the culprit
  for the hacks which are being treated in patch "ata: ahci: mvebu:
  Add support for A8k legacy bindings" as suggested by Marc.
* Modified all DTs to avoid enabling/disabling the SATA ports
  independently, which does not work. Either both ports are enabled, or
  none. Tested on MacchiatoBin that all three availabe ports on the
  board are functional.

Changes in v2
=============
* In the AHCI world, the 'irq' is now an '*irqs' array, I ensured it
  is allocated even when not using *_platform drivers.
* Moved the whole logic from the generic ahci_platform.c driver to the
  Marvell's ahci_mvebu.c driver.
* Dropped the whole DT manipulation quirk.
* Instead used a hack to configure both interrupts when using the
  deprecated bindings, this hack is a8k specific but there is a flag
  that is passed to the core during the ahci_platform_get_resources()
  to indicate that the number of ports must be forced to 2 no matter
  the number of child nodes.
* The A8k based Clearfog-GT actually uses the SATA IP (Baruch's info)
  so do not remove the SATA node from the DT. Instead, change the DTS
  to fit the new bindings (the board only uses the second port at
  offset 1).
* Added bindings documentation about the A8k AHCI compatible (existing
  in DTs, missing in the doc).
* SATA Sub-nodes representing ports already are documented, I just
  added a mention that they can also have an interrupts property which
  is mutually exclusive with the root SATA node.

Miquel Raynal (5):
  ata: ahci: mvebu: Rename a platform data flag
  ata: ahci: mvebu: Support A8k compatible
  irqchip/irq-mvebu-icu: Remove the double SATA ports interrupt hack
  dt-bindings: ata: Update ahci bindings with possible per-port
    interrupts
  dt-bindings: ata: Update ahci_mvebu bindings

Sven Auhagen (2):
  ata: ahci: mvebu: Add support for A8k legacy DT bindings
  arm64: dts: marvell: armada-cp110: Switch to per-port SATA interrupts

 .../devicetree/bindings/ata/ahci-platform.txt |   7 +
 arch/arm64/boot/dts/marvell/armada-cp11x.dtsi |   6 +-
 drivers/ata/ahci.h                            |   3 +
 drivers/ata/ahci_mvebu.c                      | 254 +++++++++++++++++-
 drivers/ata/libahci.c                         |   3 +-
 drivers/ata/libahci_platform.c                |   3 +
 drivers/irqchip/irq-mvebu-icu.c               |  18 --
 include/linux/ahci_platform.h                 |   1 +
 8 files changed, 265 insertions(+), 30 deletions(-)

-- 
2.24.3 (Apple Git-128)

[PATCH v5 1/7] ata: ahci: mvebu: Rename a platform data flag

[sven.auhagen]

From: Miquel Raynal <miquel.raynal@bootlin.com>

Before adding more entries in the platform data structure, rename the
flags entry to be more precise and name it host_flags.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/ata/ahci_mvebu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
index d4bba3ace45d..43bb2db59698 100644
--- a/drivers/ata/ahci_mvebu.c
+++ b/drivers/ata/ahci_mvebu.c
@@ -30,7 +30,7 @@
 
 struct ahci_mvebu_plat_data {
 	int (*plat_config)(struct ahci_host_priv *hpriv);
-	unsigned int flags;
+	unsigned int host_flags;
 };
 
 static void ahci_mvebu_mbus_config(struct ahci_host_priv *hpriv,
@@ -196,7 +196,7 @@ static int ahci_mvebu_probe(struct platform_device *pdev)
 	if (IS_ERR(hpriv))
 		return PTR_ERR(hpriv);
 
-	hpriv->flags |= pdata->flags;
+	hpriv->flags |= pdata->host_flags;
 	hpriv->plat_data = (void *)pdata;
 
 	rc = ahci_platform_enable_resources(hpriv);
@@ -227,7 +227,7 @@ static const struct ahci_mvebu_plat_data ahci_mvebu_armada_380_plat_data = {
 
 static const struct ahci_mvebu_plat_data ahci_mvebu_armada_3700_plat_data = {
 	.plat_config = ahci_mvebu_armada_3700_config,
-	.flags = AHCI_HFLAG_SUSPEND_PHYS,
+	.host_flags = AHCI_HFLAG_SUSPEND_PHYS,
 };
 
 static const struct of_device_id ahci_mvebu_of_match[] = {
-- 
2.24.3 (Apple Git-128)

[PATCH v5 2/7] ata: ahci: mvebu: Support A8k compatible

[sven.auhagen]

From: Miquel Raynal <miquel.raynal@bootlin.com>

The ahci_platform.c driver was historically the one bound to the A8k
AHCI compatible string, but before adding a quirk for this compatible,
it is probably cleaner to put all Marvell EBU code in one place: the
ahci_mvebu.c driver.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/ata/ahci_mvebu.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
index 43bb2db59698..1cdc126882eb 100644
--- a/drivers/ata/ahci_mvebu.c
+++ b/drivers/ata/ahci_mvebu.c
@@ -96,6 +96,11 @@ static int ahci_mvebu_armada_3700_config(struct ahci_host_priv *hpriv)
 	return 0;
 }
 
+static int ahci_mvebu_armada_8k_config(struct ahci_host_priv *hpriv)
+{
+	return 0;
+}
+
 /**
  * ahci_mvebu_stop_engine
  *
@@ -230,6 +235,10 @@ static const struct ahci_mvebu_plat_data ahci_mvebu_armada_3700_plat_data = {
 	.host_flags = AHCI_HFLAG_SUSPEND_PHYS,
 };
 
+static const struct ahci_mvebu_plat_data ahci_mvebu_armada_8k_plat_data = {
+	.plat_config = ahci_mvebu_armada_8k_config,
+};
+
 static const struct of_device_id ahci_mvebu_of_match[] = {
 	{
 		.compatible = "marvell,armada-380-ahci",
@@ -239,6 +248,10 @@ static const struct of_device_id ahci_mvebu_of_match[] = {
 		.compatible = "marvell,armada-3700-ahci",
 		.data = &ahci_mvebu_armada_3700_plat_data,
 	},
+	{
+		.compatible = "marvell,armada-8k-ahci",
+		.data = &ahci_mvebu_armada_8k_plat_data,
+	},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, ahci_mvebu_of_match);
-- 
2.24.3 (Apple Git-128)

[PATCH v5 3/7] ata: ahci: mvebu: Add support for A8k legacy DT bindings

[sven.auhagen]

From: Sven Auhagen <sven.auhagen@voleatech.de>

The CP110 SATA unit has 2 ports, and a dedicated ICU entry per
port. In the past, the AHCI SATA driver only supported one interrupt
per SATA unit. To solve this conflict, the 2 SATA wired interrupts in
the South-Bridge got configured as 1 GIC interrupt in the
North-Bridge, regardless of the number of SATA ports actually
enabled/in use, and the DT bindings only referenced the interrupt of
one port.

Since then, this limitation has been addressed and this patch ensures
backward compatibility with old DTs not describing SATA ports
correctly directly from the AHCI MVEBU driver. This way, we will be
able to drop the hack from the ICU driver. IOW, when the A8k
compatible string is used and there is no sub-nodes in the DT, we
fake the creation and mapping of the second (missing) interrupt.

Signed-off-by: Sven Auhagen <sven.auhagen@voleatech.de>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/ata/ahci.h             |   3 +
 drivers/ata/ahci_mvebu.c       | 235 ++++++++++++++++++++++++++++++++-
 drivers/ata/libahci.c          |   3 +-
 drivers/ata/libahci_platform.c |   3 +
 include/linux/ahci_platform.h  |   1 +
 5 files changed, 239 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index d991dd46e89c..8cb256eec86b 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -407,6 +407,9 @@ int ahci_do_softreset(struct ata_link *link, unsigned int *class,
 int ahci_do_hardreset(struct ata_link *link, unsigned int *class,
 		      unsigned long deadline, bool *online);
 
+void ahci_handle_port_interrupt(struct ata_port *ap,
+				       void __iomem *port_mmio, u32 status);
+
 unsigned int ahci_qc_issue(struct ata_queued_cmd *qc);
 int ahci_stop_engine(struct ata_port *ap);
 void ahci_start_fis_rx(struct ata_port *ap);
diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
index 1cdc126882eb..62e9b94e2bba 100644
--- a/drivers/ata/ahci_mvebu.c
+++ b/drivers/ata/ahci_mvebu.c
@@ -16,6 +16,7 @@
 #include <linux/mbus.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
+#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include "ahci.h"
 
@@ -28,9 +29,14 @@
 #define AHCI_WINDOW_BASE(win)	(0x64 + ((win) << 4))
 #define AHCI_WINDOW_SIZE(win)	(0x68 + ((win) << 4))
 
+#define ICU_SATA0_ICU_ID 109
+#define ICU_SATA1_ICU_ID 107
+
 struct ahci_mvebu_plat_data {
 	int (*plat_config)(struct ahci_host_priv *hpriv);
 	unsigned int host_flags;
+	unsigned int resource_flags;
+	unsigned int port_irq[2];
 };
 
 static void ahci_mvebu_mbus_config(struct ahci_host_priv *hpriv,
@@ -96,6 +102,213 @@ static int ahci_mvebu_armada_3700_config(struct ahci_host_priv *hpriv)
 	return 0;
 }
 
+static int ahci_get_per_port_irq_armada8k(struct ata_host *host, int port)
+{
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct ahci_mvebu_plat_data *pdata = hpriv->plat_data;
+
+	return pdata->port_irq[port];
+}
+
+static irqreturn_t ahci_multi_irqs_intr_hard_armada8k(int irq, void *dev_instance)
+{
+	struct ata_port *ap = dev_instance;
+	struct ata_host *host = ap->host;
+	struct ahci_host_priv *hpriv = host->private_data;
+	void __iomem *port_mmio = ahci_port_base(ap);
+	void __iomem *mmio = hpriv->mmio;
+	u32 status;
+
+	VPRINTK("ENTER\n");
+
+	status = readl(port_mmio + PORT_IRQ_STAT);
+	writel(status, port_mmio + PORT_IRQ_STAT);
+
+	spin_lock(ap->lock);
+	ahci_handle_port_interrupt(ap, port_mmio, status);
+	spin_unlock(ap->lock);
+
+	writel(BIT(ap->port_no), mmio + HOST_IRQ_STAT);
+
+	VPRINTK("EXIT\n");
+
+	return IRQ_HANDLED;
+}
+
+static int ahci_host_activate_multi_irqs_armada8k(struct ata_host *host,
+					 struct scsi_host_template *sht)
+{
+	struct ahci_host_priv *hpriv = host->private_data;
+	int i, rc;
+
+	rc = ata_host_start(host);
+	if (rc)
+		return rc;
+	/*
+	 * Requests IRQs one per port
+	 */
+	for (i = 0; i < host->n_ports; i++) {
+		struct ahci_port_priv *pp = host->ports[i]->private_data;
+		int irq = hpriv->get_irq_vector(host, i);
+
+		/* Do not receive interrupts sent by dummy ports */
+		if (!pp) {
+			disable_irq(irq);
+			continue;
+		}
+
+		rc = devm_request_irq(host->dev, irq, ahci_multi_irqs_intr_hard_armada8k,
+				0, pp->irq_desc, host->ports[i]);
+
+		if (rc)
+			return rc;
+		ata_port_desc(host->ports[i], "irq %d", irq);
+	}
+
+	return ata_host_register(host, sht);
+}
+
+static int ahci_mvebu_armada_8k_irq_backwards(struct ahci_host_priv *hpriv,
+				       struct device *dev)
+{
+	struct device_node *np = of_irq_find_parent(dev->of_node);
+	struct ahci_mvebu_plat_data *pdata = hpriv->plat_data;
+	struct irq_data *irqd = irq_get_irq_data(pdata->port_irq[0]);
+	int host_irq = irqd ? irqd_to_hwirq(irqd) : 0;
+	int missing_irq = (host_irq == ICU_SATA1_ICU_ID) ?
+		ICU_SATA0_ICU_ID : ICU_SATA1_ICU_ID;
+	struct irq_fwspec fwspec = {
+		.fwnode = of_node_to_fwnode(np),
+		.param_count = 2,
+		.param = {missing_irq, IRQ_TYPE_LEVEL_HIGH},
+	};
+
+	if (of_get_child_count(dev->of_node))
+		return 0;
+
+	pdata->port_irq[1] = irq_create_fwspec_mapping(&fwspec);
+	if (pdata->port_irq[1])
+		hpriv->mask_port_map = GENMASK(1, 0);
+
+	return 0;
+}
+
+static int ahci_platform_init_host_armada8k(struct platform_device *pdev,
+			    struct ahci_host_priv *hpriv,
+			    const struct ata_port_info *pi_template,
+			    struct scsi_host_template *sht)
+{
+	struct ahci_mvebu_plat_data *pdata = hpriv->plat_data;
+	struct device *dev = &pdev->dev;
+	struct ata_port_info pi = *pi_template;
+	const struct ata_port_info *ppi[] = { &pi, NULL };
+	struct device_node *child;
+	struct ata_host *host;
+	int i, port_irq, n_ports, rc, child_nodes, port = 0;
+
+	/* Get IRQs per port */
+	child_nodes = of_get_child_count(dev->of_node);
+	if (child_nodes) {
+		for_each_child_of_node(dev->of_node, child) {
+
+			port_irq = of_irq_get(child, 0);
+			if (!port_irq)
+				port_irq = -EINVAL;
+			if (port_irq < 0) {
+				rc = port_irq;
+				return rc;
+			}
+
+			pdata->port_irq[port] = port_irq;
+			port++;
+		}
+	} else {
+		/* Backwards Compatibility Check */
+		port_irq = platform_get_irq(pdev, 0);
+		if (port_irq > 0) {
+			pdata->port_irq[0] = port_irq;
+			ahci_mvebu_armada_8k_irq_backwards(hpriv, dev);
+		} else {
+			dev_err(dev, "no irq\n");
+			return -EINVAL;
+		}
+	}
+
+	hpriv->get_irq_vector = ahci_get_per_port_irq_armada8k;
+
+	/* prepare host */
+	pi.private_data = (void *)(unsigned long)hpriv->flags;
+
+	ahci_save_initial_config(dev, hpriv);
+
+	if (hpriv->cap & HOST_CAP_NCQ)
+		pi.flags |= ATA_FLAG_NCQ;
+
+	if (hpriv->cap & HOST_CAP_PMP)
+		pi.flags |= ATA_FLAG_PMP;
+
+	ahci_set_em_messages(hpriv, &pi);
+
+	/* CAP.NP sometimes indicate the index of the last enabled
+	 * port, at other times, that of the last possible port, so
+	 * determining the maximum port number requires looking at
+	 * both CAP.NP and port_map.
+	 */
+	n_ports = max(ahci_nr_ports(hpriv->cap), fls(hpriv->port_map));
+
+	host = ata_host_alloc_pinfo(dev, ppi, n_ports);
+	if (!host)
+		return -ENOMEM;
+
+	host->private_data = hpriv;
+
+	if (!(hpriv->cap & HOST_CAP_SSS) || ahci_ignore_sss)
+		host->flags |= ATA_HOST_PARALLEL_SCAN;
+	else
+		dev_info(dev, "SSS flag set, parallel bus scan disabled\n");
+
+	if (pi.flags & ATA_FLAG_EM)
+		ahci_reset_em(host);
+
+	for (i = 0; i < host->n_ports; i++) {
+		struct ata_port *ap = host->ports[i];
+
+		ata_port_desc(ap, "mmio %pR",
+			      platform_get_resource(pdev, IORESOURCE_MEM, 0));
+		ata_port_desc(ap, "port 0x%x", 0x100 + ap->port_no * 0x80);
+
+		/* set enclosure management message type */
+		if (ap->flags & ATA_FLAG_EM)
+			ap->em_message_type = hpriv->em_msg_type;
+
+		/* disabled/not-implemented port */
+		if (!(hpriv->port_map & (1 << i)))
+			ap->ops = &ata_dummy_port_ops;
+	}
+
+	if (hpriv->cap & HOST_CAP_64) {
+		rc = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64));
+		if (rc) {
+			rc = dma_coerce_mask_and_coherent(dev,
+							  DMA_BIT_MASK(32));
+			if (rc) {
+				dev_err(dev, "Failed to enable 64-bit DMA.\n");
+				return rc;
+			}
+			dev_warn(dev, "Enable 32-bit DMA instead of 64-bit.\n");
+		}
+	}
+
+	rc = ahci_reset_controller(host);
+	if (rc)
+		return rc;
+
+	ahci_init_controller(host);
+	ahci_print_info(host, "platform");
+
+	return ahci_host_activate_multi_irqs_armada8k(host, sht);
+}
+
 static int ahci_mvebu_armada_8k_config(struct ahci_host_priv *hpriv)
 {
 	return 0;
@@ -189,15 +402,22 @@ static struct scsi_host_template ahci_platform_sht = {
 
 static int ahci_mvebu_probe(struct platform_device *pdev)
 {
-	const struct ahci_mvebu_plat_data *pdata;
+	const struct ahci_mvebu_plat_data *pdata_plat;
+	struct ahci_mvebu_plat_data *pdata;
 	struct ahci_host_priv *hpriv;
 	int rc;
 
-	pdata = of_device_get_match_data(&pdev->dev);
-	if (!pdata)
+	pdata_plat = of_device_get_match_data(&pdev->dev);
+	if (!pdata_plat)
 		return -EINVAL;
 
-	hpriv = ahci_platform_get_resources(pdev, 0);
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	memcpy(pdata, pdata_plat, sizeof(*pdata));
+
+	hpriv = ahci_platform_get_resources(pdev, pdata->resource_flags);
 	if (IS_ERR(hpriv))
 		return PTR_ERR(hpriv);
 
@@ -214,7 +434,11 @@ static int ahci_mvebu_probe(struct platform_device *pdev)
 	if (rc)
 		goto disable_resources;
 
-	rc = ahci_platform_init_host(pdev, hpriv, &ahci_mvebu_port_info,
+	if (pdata->resource_flags & AHCI_PLATFORM_A8K_QUIRK)
+		rc = ahci_platform_init_host_armada8k(pdev, hpriv, &ahci_mvebu_port_info,
+				     &ahci_platform_sht);
+	else
+		rc = ahci_platform_init_host(pdev, hpriv, &ahci_mvebu_port_info,
 				     &ahci_platform_sht);
 	if (rc)
 		goto disable_resources;
@@ -237,6 +461,7 @@ static const struct ahci_mvebu_plat_data ahci_mvebu_armada_3700_plat_data = {
 
 static const struct ahci_mvebu_plat_data ahci_mvebu_armada_8k_plat_data = {
 	.plat_config = ahci_mvebu_armada_8k_config,
+	.resource_flags = AHCI_PLATFORM_A8K_QUIRK,
 };
 
 static const struct of_device_id ahci_mvebu_of_match[] = {
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index ea5bf5f4cbed..0e6eaa2e03a9 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1799,7 +1799,7 @@ static void ahci_error_intr(struct ata_port *ap, u32 irq_stat)
 		ata_port_abort(ap);
 }
 
-static void ahci_handle_port_interrupt(struct ata_port *ap,
+void ahci_handle_port_interrupt(struct ata_port *ap,
 				       void __iomem *port_mmio, u32 status)
 {
 	struct ata_eh_info *ehi = &ap->link.eh_info;
@@ -1882,6 +1882,7 @@ static void ahci_handle_port_interrupt(struct ata_port *ap,
 		ata_port_freeze(ap);
 	}
 }
+EXPORT_SYMBOL_GPL(ahci_handle_port_interrupt);
 
 static void ahci_port_intr(struct ata_port *ap)
 {
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 129556fcf6be..21332a33f766 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -464,6 +464,9 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 	if (!child_nodes)
 		hpriv->nports = 1;
 
+	if (!child_nodes && flags & AHCI_PLATFORM_A8K_QUIRK)
+		hpriv->nports = 2;
+
 	hpriv->phys = devm_kcalloc(dev, hpriv->nports, sizeof(*hpriv->phys), GFP_KERNEL);
 	if (!hpriv->phys) {
 		rc = -ENOMEM;
diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index 49e5383d4222..b31972e0bfbf 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -42,5 +42,6 @@ int ahci_platform_suspend(struct device *dev);
 int ahci_platform_resume(struct device *dev);
 
 #define AHCI_PLATFORM_GET_RESETS	0x01
+#define AHCI_PLATFORM_A8K_QUIRK		0x02
 
 #endif /* _AHCI_PLATFORM_H */
-- 
2.24.3 (Apple Git-128)

[PATCH v5 4/7] irqchip/irq-mvebu-icu: Remove the double SATA ports interrupt hack

[sven.auhagen]

From: Miquel Raynal <miquel.raynal@bootlin.com>

When writing the driver, a hack was introduced to configure both SATA
interrupts regardless of the port in use to overcome a limitation in
the SATA core. Now that this limitation has been addressed and the
hack moved in the (historically) responsible SATA driver,
ahci_{platform,mvebu}.c, let's clean this driver section.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/irq-mvebu-icu.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c
index 91adf771f185..3e29f8d5b33b 100644
--- a/drivers/irqchip/irq-mvebu-icu.c
+++ b/drivers/irqchip/irq-mvebu-icu.c
@@ -38,8 +38,6 @@
 
 /* ICU definitions */
 #define ICU_MAX_IRQS		207
-#define ICU_SATA0_ICU_ID	109
-#define ICU_SATA1_ICU_ID	107
 
 struct mvebu_icu_subset_data {
 	unsigned int icu_group;
@@ -111,22 +109,6 @@ static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg)
 	}
 
 	writel_relaxed(icu_int, icu->base + ICU_INT_CFG(d->hwirq));
-
-	/*
-	 * The SATA unit has 2 ports, and a dedicated ICU entry per
-	 * port. The ahci sata driver supports only one irq interrupt
-	 * per SATA unit. To solve this conflict, we configure the 2
-	 * SATA wired interrupts in the south bridge into 1 GIC
-	 * interrupt in the north bridge. Even if only a single port
-	 * is enabled, if sata node is enabled, both interrupts are
-	 * configured (regardless of which port is actually in use).
-	 */
-	if (d->hwirq == ICU_SATA0_ICU_ID || d->hwirq == ICU_SATA1_ICU_ID) {
-		writel_relaxed(icu_int,
-			       icu->base + ICU_INT_CFG(ICU_SATA0_ICU_ID));
-		writel_relaxed(icu_int,
-			       icu->base + ICU_INT_CFG(ICU_SATA1_ICU_ID));
-	}
 }
 
 static struct irq_chip mvebu_icu_nsr_chip = {
-- 
2.24.3 (Apple Git-128)

[PATCH v5 5/7] dt-bindings: ata: Update ahci bindings with possible per-port interrupts

[sven.auhagen]

From: Miquel Raynal <miquel.raynal@bootlin.com>

Update bindings to reflect the fact that a SATA IP can either have:
- only one interrupt: in this case an 'interrupts' property is
  declared at the root of the node;
or
- each SATA port can have their own interrupt: in this case there is
  one 'interrupts' property per port/sub-node and none at the root.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/ata/ahci-platform.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index 77091a277642..83d715cbcecd 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -49,6 +49,12 @@ And at least one of the following properties:
 - phys		    : reference to the SATA PHY node
 - target-supply     : regulator for SATA target power
 
+Sub-nodes optional properties:
+- interrupts        : <interrupt mapping for SATA ports IRQ>, please
+                      note that either the root SATA node has the
+                      interrupts property, or there is one per SATA
+                      port, but not both at the same time.
+
 Examples:
         sata@ffe08000 {
 		compatible = "snps,spear-ahci";
-- 
2.24.3 (Apple Git-128)

[PATCH v5 6/7] dt-bindings: ata: Update ahci_mvebu bindings

[sven.auhagen]

From: Miquel Raynal <miquel.raynal@bootlin.com>

Update bindings with the already in use Armada 8k compatible.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/ata/ahci-platform.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index 83d715cbcecd..78d9d413a5c6 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -15,6 +15,7 @@ Required properties:
   - "ibm,476gtr-ahci"
   - "marvell,armada-380-ahci"
   - "marvell,armada-3700-ahci"
+  - "marvell,armada-8k-ahci"
   - "snps,dwc-ahci"
   - "snps,spear-ahci"
   - "generic-ahci"
-- 
2.24.3 (Apple Git-128)

[PATCH v5 7/7] arm64: dts: marvell: armada-cp110: Switch to per-port SATA interrupts

[sven.auhagen]

From: Sven Auhagen <sven.auhagen@voleatech.de>

There are two SATA ports per CP110. Each of them has a dedicated
interrupt. Describe the real hardware by adding two SATA ports to the
CP110 SATA node.

Signed-off-by: Sven Auhagen <sven.auhagen@voleatech.de>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 arch/arm64/boot/dts/marvell/armada-cp11x.dtsi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
index 9dcf16beabf5..ec27294f097b 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
@@ -300,11 +300,9 @@ CP11X_LABEL(usb3_1): usb3@510000 {
 		};
 
 		CP11X_LABEL(sata0): sata@540000 {
-			compatible = "marvell,armada-8k-ahci",
-			"generic-ahci";
+			compatible = "marvell,armada-8k-ahci";
 			reg = <0x540000 0x30000>;
 			dma-coherent;
-			interrupts = <107 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&CP11X_LABEL(clk) 1 15>,
 				 <&CP11X_LABEL(clk) 1 16>;
 			#address-cells = <1>;
@@ -312,10 +310,12 @@ CP11X_LABEL(sata0): sata@540000 {
 			status = "disabled";
 
 			sata-port@0 {
+				interrupts = <109 IRQ_TYPE_LEVEL_HIGH>;
 				reg = <0>;
 			};
 
 			sata-port@1 {
+				interrupts = <107 IRQ_TYPE_LEVEL_HIGH>;
 				reg = <1>;
 			};
 		};
-- 
2.24.3 (Apple Git-128)

Re: [PATCH v5 0/7] Armada8k enable per-port SATA interrupts and drop a hack in the IRQ subsystem

[Sven Auhagen]

Hi,

does anyone have any feedback on this?
It has been over a month since I submitted this patch series.

Best
Sven

On Sun, Jul 19, 2020 at 08:59:35AM +0200, sven.auhagen@voleatech.de wrote:
> From: Sven Auhagen <sven.auhagen@voleatech.de>
> 
> Hello,
> 
> there were already 4 versions of this series from Miquèl.
> I talked to Miquèl and I fixed up the last comments from v4.
> I am looking for feedback if this patch series is now ready to be merged
> and what should be further changed.
> 
> Here is the original cover letter:
> 
> Some time ago, when the initial support for Armada CP110 was
> contributed, the SATA core was not able to handle per-port
> interrupts. Despite the hardware reality, the device tree only
> represents one main interrupt for the two ports. Having both SATA
> ports enabled at the same time has been achieved by a hack in the ICU
> driver(1) that faked the use of the two interrupts, no matter which
> SATA port was in use.
> 
> Now that the SATA core is ready to handle more than one interrupt,
> this series adds support for it in the libahci_platform code. The
> CP110 device tree must be updated to reflect the two SATA ports
> available and their respective interrupts. To do not break DT backward
> compatibility, the ahci_platform driver now embeds a special quirk
> which checks if the DT is valid (only for A8k compatible) and, if
> needed, creates the two missing sub-nodes, and assign them the
> relevant "reg" and "interrupts" properties, before removing the main
> SATA node "interrupts" one.
> 
> (1) The ICU is an irqchip aggregating the CP110 (south-bridge)
> interrupts into MSIs for the AP806 (north-bridge).
> 
> Best
> Sven
> 
> Changes in v5
> =============
> * Rebase on top of v5.8-rc3
> * Move code to ahci mvebu and use custom functions
> 
> Changes in v4
> =============
> * Rebase on top of v5.2-rc1
> * s/ARM64/arm64/ in the DT change commit title.
> 
> Changes in v3
> =============
> * Removed useless locking when acking the SATA host IRQ_STAT bit.
> * As spotted by Hans, do not enable the AHCI_HFLAG_MULTI_MSI flag only
>   if more than one port is enabled, DTs might declare only one
>   interrupt and have multiple ports. Added Hans' Reviewed-by tag.
> * Added Rob's Reviewed-by tags.
> * Added explicit references to the 'Device Tree' being the culprit
>   for the hacks which are being treated in patch "ata: ahci: mvebu:
>   Add support for A8k legacy bindings" as suggested by Marc.
> * Modified all DTs to avoid enabling/disabling the SATA ports
>   independently, which does not work. Either both ports are enabled, or
>   none. Tested on MacchiatoBin that all three availabe ports on the
>   board are functional.
> 
> Changes in v2
> =============
> * In the AHCI world, the 'irq' is now an '*irqs' array, I ensured it
>   is allocated even when not using *_platform drivers.
> * Moved the whole logic from the generic ahci_platform.c driver to the
>   Marvell's ahci_mvebu.c driver.
> * Dropped the whole DT manipulation quirk.
> * Instead used a hack to configure both interrupts when using the
>   deprecated bindings, this hack is a8k specific but there is a flag
>   that is passed to the core during the ahci_platform_get_resources()
>   to indicate that the number of ports must be forced to 2 no matter
>   the number of child nodes.
> * The A8k based Clearfog-GT actually uses the SATA IP (Baruch's info)
>   so do not remove the SATA node from the DT. Instead, change the DTS
>   to fit the new bindings (the board only uses the second port at
>   offset 1).
> * Added bindings documentation about the A8k AHCI compatible (existing
>   in DTs, missing in the doc).
> * SATA Sub-nodes representing ports already are documented, I just
>   added a mention that they can also have an interrupts property which
>   is mutually exclusive with the root SATA node.
> 
> Miquel Raynal (5):
>   ata: ahci: mvebu: Rename a platform data flag
>   ata: ahci: mvebu: Support A8k compatible
>   irqchip/irq-mvebu-icu: Remove the double SATA ports interrupt hack
>   dt-bindings: ata: Update ahci bindings with possible per-port
>     interrupts
>   dt-bindings: ata: Update ahci_mvebu bindings
> 
> Sven Auhagen (2):
>   ata: ahci: mvebu: Add support for A8k legacy DT bindings
>   arm64: dts: marvell: armada-cp110: Switch to per-port SATA interrupts
> 
>  .../devicetree/bindings/ata/ahci-platform.txt |   7 +
>  arch/arm64/boot/dts/marvell/armada-cp11x.dtsi |   6 +-
>  drivers/ata/ahci.h                            |   3 +
>  drivers/ata/ahci_mvebu.c                      | 254 +++++++++++++++++-
>  drivers/ata/libahci.c                         |   3 +-
>  drivers/ata/libahci_platform.c                |   3 +
>  drivers/irqchip/irq-mvebu-icu.c               |  18 --
>  include/linux/ahci_platform.h                 |   1 +
>  8 files changed, 265 insertions(+), 30 deletions(-)
> 
> -- 
> 2.24.3 (Apple Git-128)
>