[PATCH 0/5] Enable per-port SATA interrupts and drop an hack in the IRQ subsystem

[PATCH 0/5] Enable per-port SATA interrupts and drop an hack in the IRQ subsystem

[Miquel Raynal]

Hello,

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.


Thanks,
Miquèl

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


Miquel Raynal (4):
  ata: libahci: Ensure the host interrupt status bits are cleared
  ata: libahci_platform: Support per-port interrupts
  irqchip/irq-mvebu-icu: Move the double SATA ports interrupt hack
  arm64: dts: marvell: armada-8040-clearfog: Drop non-existent SATA port

Thomas Petazzoni (1):
  arm64: dts: marvell: armada-cp110: Switch to per-port SATA interrupts

 .../arm64/boot/dts/marvell/armada-7040-db.dts |   7 +-
 .../marvell/armada-8040-clearfog-gt-8k.dts    |   5 -
 .../arm64/boot/dts/marvell/armada-8040-db.dts |  14 +-
 arch/arm64/boot/dts/marvell/armada-cp110.dtsi |  16 +-
 drivers/ata/acard-ahci.c                      |   2 +-
 drivers/ata/ahci.c                            |   2 +-
 drivers/ata/ahci.h                            |   3 +-
 drivers/ata/ahci_platform.c                   | 174 ++++++++++++++++++
 drivers/ata/libahci.c                         |   9 +-
 drivers/ata/libahci_platform.c                |  66 +++++--
 drivers/ata/sata_highbank.c                   |   2 +-
 drivers/irqchip/irq-mvebu-icu.c               |  18 --
 12 files changed, 274 insertions(+), 44 deletions(-)

-- 
2.19.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/5] ata: libahci: Ensure the host interrupt status bits are cleared

[Miquel Raynal]

ahci_multi_irqs_intr_hard() is going to be used as interrupt handler
to support SATA per-port interrupts. The current logic is to check and
clear the SATA port interrupt status register only. To avoid spurious
IRQs and interrupt storms, it will be needed to clear the port
interrupt bit in the host interrupt status register as well.

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

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index b5f57c69c487..66d4906a5013 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1912,7 +1912,10 @@ static void ahci_port_intr(struct ata_port *ap)
 static irqreturn_t ahci_multi_irqs_intr_hard(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");
@@ -1924,6 +1927,10 @@ static irqreturn_t ahci_multi_irqs_intr_hard(int irq, void *dev_instance)
 	ahci_handle_port_interrupt(ap, port_mmio, status);
 	spin_unlock(ap->lock);
 
+	spin_lock(&host->lock);
+	writel(BIT(ap->port_no), mmio + HOST_IRQ_STAT);
+	spin_unlock(&host->lock);
+
 	VPRINTK("EXIT\n");
 
 	return IRQ_HANDLED;
-- 
2.19.1

[PATCH 2/5] ata: libahci_platform: Support per-port interrupts

[Miquel Raynal]

Right now the ATA core only allows IPs to use a single interrupt. Some
of them (for instance the Armada-CP110 one) actually has one interrupt
per port. Add some logic to support such situation.

We consider that either there is one single interrupt declared in the
main IP node, or there are per-port interrupts, each of them being
declared in the port sub-nodes.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/ata/acard-ahci.c       |  2 +-
 drivers/ata/ahci.c             |  2 +-
 drivers/ata/ahci.h             |  3 +-
 drivers/ata/libahci.c          |  2 +-
 drivers/ata/libahci_platform.c | 66 ++++++++++++++++++++++++++++------
 drivers/ata/sata_highbank.c    |  2 +-
 6 files changed, 61 insertions(+), 16 deletions(-)

diff --git a/drivers/ata/acard-ahci.c b/drivers/ata/acard-ahci.c
index 583e366be7e2..9414b81e994c 100644
--- a/drivers/ata/acard-ahci.c
+++ b/drivers/ata/acard-ahci.c
@@ -434,7 +434,7 @@ static int acard_ahci_init_one(struct pci_dev *pdev, const struct pci_device_id
 	if (!hpriv)
 		return -ENOMEM;
 
-	hpriv->irq = pdev->irq;
+	hpriv->irqs[0] = pdev->irq;
 	hpriv->flags |= (unsigned long)pi.private_data;
 
 	if (!(hpriv->flags & AHCI_HFLAG_NO_MSI))
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 021ce46e2e57..18bce556d85f 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1817,7 +1817,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		/* legacy intx interrupts */
 		pci_intx(pdev, 1);
 	}
-	hpriv->irq = pci_irq_vector(pdev, 0);
+	hpriv->irqs[0] = pci_irq_vector(pdev, 0);
 
 	if (!(hpriv->cap & HOST_CAP_SSS) || ahci_ignore_sss)
 		host->flags |= ATA_HOST_PARALLEL_SCAN;
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index ef356e70e6de..1f1c00be5b2e 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -361,7 +361,7 @@ struct ahci_host_priv {
 	struct phy		**phys;
 	unsigned		nports;		/* Number of ports */
 	void			*plat_data;	/* Other platform data */
-	unsigned int		irq;		/* interrupt line */
+	unsigned int		*irqs;		/* interrupt line(s) */
 	/*
 	 * Optional ahci_start_engine override, if not set this gets set to the
 	 * default ahci_start_engine during ahci_save_initial_config, this can
@@ -432,6 +432,7 @@ 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);
+int ahci_get_per_port_irq_vector(struct ata_host *host, int port);
 
 static inline void __iomem *__ahci_port_base(struct ata_host *host,
 					     unsigned int port_no)
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 66d4906a5013..25970138a65a 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2602,7 +2602,7 @@ static int ahci_host_activate_multi_irqs(struct ata_host *host,
 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;
+	int irq = hpriv->irqs[0];
 	int rc;
 
 	if (hpriv->flags & AHCI_HFLAG_MULTI_MSI) {
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 4b900fc659f7..af3d65c09087 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -24,6 +24,7 @@
 #include <linux/ahci_platform.h>
 #include <linux/phy/phy.h>
 #include <linux/pm_runtime.h>
+#include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/reset.h>
 #include "ahci.h"
@@ -89,6 +90,14 @@ static void ahci_platform_disable_phys(struct ahci_host_priv *hpriv)
 	}
 }
 
+int ahci_get_per_port_irq_vector(struct ata_host *host, int port)
+{
+	struct ahci_host_priv *hpriv = host->private_data;
+
+	return hpriv->irqs[port];
+}
+EXPORT_SYMBOL_GPL(ahci_get_per_port_irq_vector);
+
 /**
  * ahci_platform_enable_clks - Enable platform clocks
  * @hpriv: host private area to store config values
@@ -379,6 +388,7 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
  *    or for non devicetree enabled platforms a single clock
  * 4) resets, if flags has AHCI_PLATFORM_GET_RESETS (optional)
  * 5) phys (optional)
+ * 6) interrupt(s)
  *
  * RETURNS:
  * The allocated ahci_host_priv on success, otherwise an ERR_PTR value
@@ -390,7 +400,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 	struct ahci_host_priv *hpriv;
 	struct clk *clk;
 	struct device_node *child;
-	int i, enabled_ports = 0, rc = -ENOMEM, child_nodes;
+	int i, enabled_ports = 0, rc = -ENOMEM, child_nodes, ctrl_irq;
 	u32 mask_port_map = 0;
 
 	if (!devres_open_group(dev, NULL, GFP_KERNEL))
@@ -483,10 +493,29 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 		goto err_out;
 	}
 
+	hpriv->irqs = kzalloc(sizeof(*hpriv->irqs) * hpriv->nports, GFP_KERNEL);
+	if (!hpriv->irqs) {
+		rc = -ENOMEM;
+		goto err_out;
+	}
+
+	ctrl_irq = platform_get_irq(pdev, 0);
+	if (ctrl_irq < 0) {
+		if (ctrl_irq == -EPROBE_DEFER) {
+			rc = ctrl_irq;
+			goto err_out;
+		}
+		ctrl_irq = 0;
+	}
+
+	if (ctrl_irq > 0)
+		hpriv->irqs[0] = ctrl_irq;
+
 	if (child_nodes) {
 		for_each_child_of_node(dev->of_node, child) {
 			u32 port;
 			struct platform_device *port_dev __maybe_unused;
+			int port_irq;
 
 			if (!of_device_is_available(child))
 				continue;
@@ -515,6 +544,18 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 			}
 #endif
 
+			if (!ctrl_irq) {
+				port_irq = of_irq_get(child, 0);
+				if (!port_irq)
+					port_irq = -EINVAL;
+				if (port_irq < 0) {
+					rc = port_irq;
+					goto err_out;
+				}
+
+				hpriv->irqs[port] = port_irq;
+			}
+
 			rc = ahci_platform_get_phy(hpriv, port, dev, child);
 			if (rc)
 				goto err_out;
@@ -542,6 +583,18 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 		if (rc == -EPROBE_DEFER)
 			goto err_out;
 	}
+
+	if (!ctrl_irq && !enabled_ports) {
+		dev_err(&pdev->dev, "No IRQ defined\n");
+		rc = -ENODEV;
+		goto err_out;
+	}
+
+	if (enabled_ports > 1) {
+		hpriv->flags |= AHCI_HFLAG_MULTI_MSI;
+		hpriv->get_irq_vector = ahci_get_per_port_irq_vector;
+	}
+
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 	hpriv->got_runtime_pm = true;
@@ -578,16 +631,7 @@ int ahci_platform_init_host(struct platform_device *pdev,
 	struct ata_port_info pi = *pi_template;
 	const struct ata_port_info *ppi[] = { &pi, NULL };
 	struct ata_host *host;
-	int i, irq, n_ports, rc;
-
-	irq = platform_get_irq(pdev, 0);
-	if (irq <= 0) {
-		if (irq != -EPROBE_DEFER)
-			dev_err(dev, "no irq\n");
-		return irq;
-	}
-
-	hpriv->irq = irq;
+	int i, n_ports, rc;
 
 	/* prepare host */
 	pi.private_data = (void *)(unsigned long)hpriv->flags;
diff --git a/drivers/ata/sata_highbank.c b/drivers/ata/sata_highbank.c
index c8fc9280d6e4..dcfdab20021b 100644
--- a/drivers/ata/sata_highbank.c
+++ b/drivers/ata/sata_highbank.c
@@ -496,7 +496,7 @@ static int ahci_highbank_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	hpriv->irq = irq;
+	hpriv->irqs[0] = irq;
 	hpriv->flags |= (unsigned long)pi.private_data;
 
 	hpriv->mmio = devm_ioremap(dev, mem->start, resource_size(mem));
-- 
2.19.1

[PATCH 3/5] irqchip/irq-mvebu-icu: Move the double SATA ports interrupt hack

[Miquel Raynal]

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, let's
clean this driver section and move the hack into the (historically)
responsible SATA driver: ahci_platform.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/ata/ahci_platform.c     | 174 ++++++++++++++++++++++++++++++++
 drivers/irqchip/irq-mvebu-icu.c |  18 ----
 2 files changed, 174 insertions(+), 18 deletions(-)

diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index cf1e0e18a7a9..4401a137e6d5 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -17,6 +17,7 @@
 #include <linux/pm.h>
 #include <linux/device.h>
 #include <linux/of_device.h>
+#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/libata.h>
 #include <linux/ahci_platform.h>
@@ -26,6 +27,9 @@
 
 #define DRV_NAME "ahci"
 
+#define ICU_SATA0_ICU_ID 109
+#define ICU_SATA1_ICU_ID 107
+
 static const struct ata_port_info ahci_port_info = {
 	.flags		= AHCI_FLAG_COMMON,
 	.pio_mask	= ATA_PIO4,
@@ -44,6 +48,170 @@ static struct scsi_host_template ahci_platform_sht = {
 	AHCI_SHT(DRV_NAME),
 };
 
+/*
+ * The Armada CP110 SATA unit (on A7k/A8k SoCs) 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, 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.
+ *
+ * Since then, this limitation has been addressed and the hack described
+ * above has been removed from the ICU driver. However, for compatibility
+ * it is needed to support DT with just one interrupt and no SATA ports.
+ * Instead of hacking everywhere in the ahci/libahci code, this quirk has been
+ * written to manually create the SATA port sub-nodes if they are missing,
+ * assign them the right port numbers through the "reg" properties and their
+ * respective "interrupts".
+ */
+static int ahci_armada_8k_quirk(struct device *dev)
+{
+	struct device_node *dn = dev->of_node;
+	struct property *interrupt = of_find_property(dn, "interrupts", NULL);
+	struct device_node *ports;
+	struct property *regs, *ints;
+	int rc, i;
+
+	if (!(of_device_is_compatible(dn, "marvell,armada-8k-ahci") &&
+	      !of_get_child_count(dn)))
+		return 0;
+
+	if (!of_node_get(dn))
+		return -EINVAL;
+
+	/* Verify the main SATA node "interrupts" property validity */
+	if (!interrupt) {
+		rc = -EINVAL;
+		goto put_dn;
+	}
+
+	/* Create the two sub-nodes */
+	ports = kzalloc(2 * sizeof(*ports), GFP_KERNEL);
+	if (!ports) {
+		rc = -ENOMEM;
+		goto put_dn;
+	}
+
+	for (i = 0; i < 2; i++) {
+		of_node_set_flag(&ports[i], OF_DYNAMIC);
+		of_node_init(&ports[i]);
+		ports[i].parent = dn;
+	}
+
+	ports[0].full_name = kstrdup("sata-port@0", GFP_KERNEL);
+	ports[1].full_name = kstrdup("sata-port@1", GFP_KERNEL);
+	if (!ports[0].full_name || !ports[1].full_name) {
+		rc = -ENOMEM;
+		goto free_ports_names;
+	}
+
+	/* Create the two "reg" and "interrupts" property for the sub-nodes */
+	regs = kzalloc(2 * sizeof(*regs), GFP_KERNEL);
+	ints = kzalloc(2 * sizeof(*ints), GFP_KERNEL);
+	if (!regs || !ints) {
+		rc = -ENOMEM;
+		goto free_props;
+	}
+
+	for (i = 0; i < 2; i++) {
+		regs[i].name = kstrdup("reg", GFP_KERNEL);
+		regs[i].length = sizeof(u32);
+		regs[i].value = kmalloc(sizeof(regs[i].length), GFP_KERNEL);
+
+		ints[i].name = kstrdup("interrupts", GFP_KERNEL);
+		ints[i].length = interrupt->length;
+		ints[i].value = kmemdup(interrupt->value, interrupt->length,
+					GFP_KERNEL);
+
+		if (!regs[i].name || !regs[i].value ||
+		    !ints[i].name || !ints[i].value) {
+			rc = -ENOMEM;
+			goto free_props_allocs;
+		}
+	}
+
+	/* Force the values of the ICU interrupts for both ports.
+	 * In the past, "interrupts" properties had three values:
+	 * 1/ ICU interrupt type, 2/ interrupt ID, 3/ interrupt type
+	 * Now there are only two:
+	 * 1/ interrupt ID, 2/ interrupt type
+	 * In both case we want to access the penultimate.
+	 */
+	for (i = 0; i < 2; i++) {
+		u32 *reg, *icu_int;
+		u8 *value;
+
+		reg = regs[i].value;
+		*reg = cpu_to_be32(i);
+
+		value = ints[i].value;
+		value = &value[ints[i].length - (2 * sizeof(u32))];
+		icu_int = (u32 *)value;
+		if (!i)
+			*icu_int = cpu_to_be32(ICU_SATA0_ICU_ID);
+		else
+			*icu_int = cpu_to_be32(ICU_SATA1_ICU_ID);
+	}
+
+	for (i = 0; i < 2; i++) {
+		ports[i].properties = &regs[i];
+		regs[i].next = &ints[i];
+		ints[i].next = NULL;
+	}
+
+	/* Create the two sub-nodes by attaching them */
+	rc = of_attach_node(&ports[0]);
+	if (rc) {
+		dev_err(dev, "Compat: cannot attach port0 (err %d)\n", rc);
+		goto free_props_allocs;
+	}
+
+	rc = of_attach_node(&ports[1]);
+	if (rc) {
+		dev_err(dev, "Compat: cannot attach port1 (err %d)\n", rc);
+		goto detach_port0;
+	}
+
+	/* Delete the "interrupts" property from the SATA host node */
+	rc = of_remove_property(dn, interrupt);
+	if (rc) {
+		dev_err(dev, "Compat: cannot delete SATA host interrupt\n");
+		goto detach_ports;
+	}
+
+	of_node_put(dn);
+
+	return 0;
+
+detach_ports:
+	of_detach_node(&ports[1]);
+
+detach_port0:
+	of_detach_node(&ports[0]);
+
+free_props_allocs:
+	for (i = 0; i < 2; i++) {
+		kfree(regs[i].name);
+		kfree(regs[i].value);
+		kfree(ints[i].name);
+		kfree(ints[i].value);
+	}
+
+free_props:
+	kfree(regs);
+	kfree(ints);
+
+free_ports_names:
+	for (i = 0; i < 2; i++)
+		kfree(ports[i].full_name);
+
+	kfree(ports);
+
+put_dn:
+	of_node_put(dn);
+
+	return rc;
+	}
+
 static int ahci_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -51,6 +219,12 @@ static int ahci_probe(struct platform_device *pdev)
 	const struct ata_port_info *port;
 	int rc;
 
+	rc = ahci_armada_8k_quirk(dev);
+	if (rc) {
+		dev_err(dev, "Problem with CP110 backward compatibility\n");
+		return rc;
+	}
+
 	hpriv = ahci_platform_get_resources(pdev,
 					    AHCI_PLATFORM_GET_RESETS);
 	if (IS_ERR(hpriv))
diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c
index 547045d89c4b..f3b43f63fe2e 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.19.1

[PATCH 4/5] arm64: dts: marvell: armada-8040-clearfog: Drop non-existent SATA port

[Miquel Raynal]

There is no CP110 SATA port available on the 8040 Clearfog A8k, SATA
may be used thanks to a mPCIe -> SATA extension board only. Hence, the
cp1_sata0 node must be removed from the device tree.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
index 5b4a9609e31f..caabbd3a85a8 100644
--- a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
+++ b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
@@ -333,11 +333,6 @@
 	};
 };
 
-&cp1_sata0 {
-	pinctrl-0 = <&cp0_pci1_reset_pins>;
-	status = "okay";
-};
-
 &cp1_mdio {
 	pinctrl-names = "default";
 	pinctrl-0 = <&cp1_ge_mdio_pins>;
-- 
2.19.1

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

[Miquel Raynal]

From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

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 and enabling them in all the DTs including it
(7040-db/8040-db/8040-clearfog).

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 arch/arm64/boot/dts/marvell/armada-7040-db.dts |  7 ++++++-
 arch/arm64/boot/dts/marvell/armada-8040-db.dts | 14 ++++++++++++--
 arch/arm64/boot/dts/marvell/armada-cp110.dtsi  | 16 ++++++++++++++--
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-7040-db.dts b/arch/arm64/boot/dts/marvell/armada-7040-db.dts
index 412efdb46e7c..54c1c0ddc813 100644
--- a/arch/arm64/boot/dts/marvell/armada-7040-db.dts
+++ b/arch/arm64/boot/dts/marvell/armada-7040-db.dts
@@ -194,7 +194,12 @@
 };
 
 &cp0_sata0 {
-	status = "okay";
+	sata-port@0 {
+		status = "okay";
+	};
+	sata-port@1 {
+		status = "okay";
+	};
 };
 
 &cp0_usb3_0 {
diff --git a/arch/arm64/boot/dts/marvell/armada-8040-db.dts b/arch/arm64/boot/dts/marvell/armada-8040-db.dts
index 1bac437369a1..988cc7dc15d9 100644
--- a/arch/arm64/boot/dts/marvell/armada-8040-db.dts
+++ b/arch/arm64/boot/dts/marvell/armada-8040-db.dts
@@ -147,7 +147,12 @@
 
 /* CON4 on CP0 expansion */
 &cp0_sata0 {
-	status = "okay";
+	sata-port@0 {
+		status = "okay";
+	};
+	sata-port@1 {
+		status = "okay";
+	};
 };
 
 /* CON9 on CP0 expansion */
@@ -279,7 +284,12 @@
 
 /* CON4 on CP1 expansion */
 &cp1_sata0 {
-	status = "okay";
+	sata-port@0 {
+		status = "okay";
+	};
+	sata-port@1 {
+		status = "okay";
+	};
 };
 
 /* CON9 on CP1 expansion */
diff --git a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi
index b9d9f31e3ba1..f27edddcacd1 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi
@@ -292,10 +292,22 @@
 			"generic-ahci";
 			reg = <0x540000 0x30000>;
 			dma-coherent;
-			interrupts = <107 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&CP110_LABEL(clk) 1 15>,
 				 <&CP110_LABEL(clk) 1 16>;
-			status = "disabled";
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			sata-port@0 {
+				reg = <0>;
+				interrupts = <109 IRQ_TYPE_LEVEL_HIGH>;
+				status = "disabled";
+			};
+
+			sata-port@1 {
+				reg = <1>;
+				interrupts = <107 IRQ_TYPE_LEVEL_HIGH>;
+				status = "disabled";
+			};
 		};
 
 		CP110_LABEL(xor0): xor@6a0000 {
-- 
2.19.1

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

[Russell King - ARM Linux admin]

On Fri, Feb 22, 2019 at 03:53:56PM +0100, Miquel Raynal wrote:
> From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> 
> 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 and enabling them in all the DTs including it
> (7040-db/8040-db/8040-clearfog).
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  arch/arm64/boot/dts/marvell/armada-7040-db.dts |  7 ++++++-
>  arch/arm64/boot/dts/marvell/armada-8040-db.dts | 14 ++++++++++++--
>  arch/arm64/boot/dts/marvell/armada-cp110.dtsi  | 16 ++++++++++++++--
>  3 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/marvell/armada-7040-db.dts b/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> index 412efdb46e7c..54c1c0ddc813 100644
> --- a/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> @@ -194,7 +194,12 @@
>  };
>  
>  &cp0_sata0 {
> -	status = "okay";
> +	sata-port@0 {

Don't these all need a reg = <N> to correspond with @N in the node name?

> +		status = "okay";
> +	};
> +	sata-port@1 {
> +		status = "okay";
> +	};
>  };
>  
>  &cp0_usb3_0 {
> diff --git a/arch/arm64/boot/dts/marvell/armada-8040-db.dts b/arch/arm64/boot/dts/marvell/armada-8040-db.dts
> index 1bac437369a1..988cc7dc15d9 100644
> --- a/arch/arm64/boot/dts/marvell/armada-8040-db.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-8040-db.dts
> @@ -147,7 +147,12 @@
>  
>  /* CON4 on CP0 expansion */
>  &cp0_sata0 {
> -	status = "okay";
> +	sata-port@0 {
> +		status = "okay";
> +	};
> +	sata-port@1 {
> +		status = "okay";
> +	};
>  };
>  
>  /* CON9 on CP0 expansion */
> @@ -279,7 +284,12 @@
>  
>  /* CON4 on CP1 expansion */
>  &cp1_sata0 {
> -	status = "okay";
> +	sata-port@0 {
> +		status = "okay";
> +	};
> +	sata-port@1 {
> +		status = "okay";
> +	};
>  };
>  
>  /* CON9 on CP1 expansion */
> diff --git a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi
> index b9d9f31e3ba1..f27edddcacd1 100644
> --- a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi
> @@ -292,10 +292,22 @@
>  			"generic-ahci";
>  			reg = <0x540000 0x30000>;
>  			dma-coherent;
> -			interrupts = <107 IRQ_TYPE_LEVEL_HIGH>;
>  			clocks = <&CP110_LABEL(clk) 1 15>,
>  				 <&CP110_LABEL(clk) 1 16>;
> -			status = "disabled";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			sata-port@0 {
> +				reg = <0>;
> +				interrupts = <109 IRQ_TYPE_LEVEL_HIGH>;
> +				status = "disabled";
> +			};
> +
> +			sata-port@1 {
> +				reg = <1>;
> +				interrupts = <107 IRQ_TYPE_LEVEL_HIGH>;
> +				status = "disabled";
> +			};
>  		};
>  
>  		CP110_LABEL(xor0): xor@6a0000 {
> -- 
> 2.19.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH 2/5] ata: libahci_platform: Support per-port interrupts

[Hans de Goede]

Hi,

On 2/22/19 3:53 PM, Miquel Raynal wrote:
> Right now the ATA core only allows IPs to use a single interrupt. Some
> of them (for instance the Armada-CP110 one) actually has one interrupt
> per port. Add some logic to support such situation.
> 
> We consider that either there is one single interrupt declared in the
> main IP node, or there are per-port interrupts, each of them being
> declared in the port sub-nodes.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>   drivers/ata/acard-ahci.c       |  2 +-
>   drivers/ata/ahci.c             |  2 +-
>   drivers/ata/ahci.h             |  3 +-
>   drivers/ata/libahci.c          |  2 +-
>   drivers/ata/libahci_platform.c | 66 ++++++++++++++++++++++++++++------
>   drivers/ata/sata_highbank.c    |  2 +-
>   6 files changed, 61 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/ata/acard-ahci.c b/drivers/ata/acard-ahci.c
> index 583e366be7e2..9414b81e994c 100644
> --- a/drivers/ata/acard-ahci.c
> +++ b/drivers/ata/acard-ahci.c
> @@ -434,7 +434,7 @@ static int acard_ahci_init_one(struct pci_dev *pdev, const struct pci_device_id
>   	if (!hpriv)
>   		return -ENOMEM;
>   
> -	hpriv->irq = pdev->irq;
> +	hpriv->irqs[0] = pdev->irq;
>   	hpriv->flags |= (unsigned long)pi.private_data;
>   

What code-path is going to alloc hpriv->irqs for drivers using this code-path
which are not using libahci_platform .c ?


Regards,

Hans





>   	if (!(hpriv->flags & AHCI_HFLAG_NO_MSI))
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 021ce46e2e57..18bce556d85f 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1817,7 +1817,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>   		/* legacy intx interrupts */
>   		pci_intx(pdev, 1);
>   	}
> -	hpriv->irq = pci_irq_vector(pdev, 0);
> +	hpriv->irqs[0] = pci_irq_vector(pdev, 0);
>   
>   	if (!(hpriv->cap & HOST_CAP_SSS) || ahci_ignore_sss)
>   		host->flags |= ATA_HOST_PARALLEL_SCAN;
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index ef356e70e6de..1f1c00be5b2e 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -361,7 +361,7 @@ struct ahci_host_priv {
>   	struct phy		**phys;
>   	unsigned		nports;		/* Number of ports */
>   	void			*plat_data;	/* Other platform data */
> -	unsigned int		irq;		/* interrupt line */
> +	unsigned int		*irqs;		/* interrupt line(s) */
>   	/*
>   	 * Optional ahci_start_engine override, if not set this gets set to the
>   	 * default ahci_start_engine during ahci_save_initial_config, this can
> @@ -432,6 +432,7 @@ 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);
> +int ahci_get_per_port_irq_vector(struct ata_host *host, int port);
>   
>   static inline void __iomem *__ahci_port_base(struct ata_host *host,
>   					     unsigned int port_no)
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 66d4906a5013..25970138a65a 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -2602,7 +2602,7 @@ static int ahci_host_activate_multi_irqs(struct ata_host *host,
>   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;
> +	int irq = hpriv->irqs[0];
>   	int rc;
>   
>   	if (hpriv->flags & AHCI_HFLAG_MULTI_MSI) {
> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
> index 4b900fc659f7..af3d65c09087 100644
> --- a/drivers/ata/libahci_platform.c
> +++ b/drivers/ata/libahci_platform.c
> @@ -24,6 +24,7 @@
>   #include <linux/ahci_platform.h>
>   #include <linux/phy/phy.h>
>   #include <linux/pm_runtime.h>
> +#include <linux/of_irq.h>
>   #include <linux/of_platform.h>
>   #include <linux/reset.h>
>   #include "ahci.h"
> @@ -89,6 +90,14 @@ static void ahci_platform_disable_phys(struct ahci_host_priv *hpriv)
>   	}
>   }
>   
> +int ahci_get_per_port_irq_vector(struct ata_host *host, int port)
> +{
> +	struct ahci_host_priv *hpriv = host->private_data;
> +
> +	return hpriv->irqs[port];
> +}
> +EXPORT_SYMBOL_GPL(ahci_get_per_port_irq_vector);
> +
>   /**
>    * ahci_platform_enable_clks - Enable platform clocks
>    * @hpriv: host private area to store config values
> @@ -379,6 +388,7 @@ static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
>    *    or for non devicetree enabled platforms a single clock
>    * 4) resets, if flags has AHCI_PLATFORM_GET_RESETS (optional)
>    * 5) phys (optional)
> + * 6) interrupt(s)
>    *
>    * RETURNS:
>    * The allocated ahci_host_priv on success, otherwise an ERR_PTR value
> @@ -390,7 +400,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>   	struct ahci_host_priv *hpriv;
>   	struct clk *clk;
>   	struct device_node *child;
> -	int i, enabled_ports = 0, rc = -ENOMEM, child_nodes;
> +	int i, enabled_ports = 0, rc = -ENOMEM, child_nodes, ctrl_irq;
>   	u32 mask_port_map = 0;
>   
>   	if (!devres_open_group(dev, NULL, GFP_KERNEL))
> @@ -483,10 +493,29 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>   		goto err_out;
>   	}
>   
> +	hpriv->irqs = kzalloc(sizeof(*hpriv->irqs) * hpriv->nports, GFP_KERNEL);
> +	if (!hpriv->irqs) {
> +		rc = -ENOMEM;
> +		goto err_out;
> +	}
> +
> +	ctrl_irq = platform_get_irq(pdev, 0);
> +	if (ctrl_irq < 0) {
> +		if (ctrl_irq == -EPROBE_DEFER) {
> +			rc = ctrl_irq;
> +			goto err_out;
> +		}
> +		ctrl_irq = 0;
> +	}
> +
> +	if (ctrl_irq > 0)
> +		hpriv->irqs[0] = ctrl_irq;
> +
>   	if (child_nodes) {
>   		for_each_child_of_node(dev->of_node, child) {
>   			u32 port;
>   			struct platform_device *port_dev __maybe_unused;
> +			int port_irq;
>   
>   			if (!of_device_is_available(child))
>   				continue;
> @@ -515,6 +544,18 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>   			}
>   #endif
>   
> +			if (!ctrl_irq) {
> +				port_irq = of_irq_get(child, 0);
> +				if (!port_irq)
> +					port_irq = -EINVAL;
> +				if (port_irq < 0) {
> +					rc = port_irq;
> +					goto err_out;
> +				}
> +
> +				hpriv->irqs[port] = port_irq;
> +			}
> +
>   			rc = ahci_platform_get_phy(hpriv, port, dev, child);
>   			if (rc)
>   				goto err_out;
> @@ -542,6 +583,18 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>   		if (rc == -EPROBE_DEFER)
>   			goto err_out;
>   	}
> +
> +	if (!ctrl_irq && !enabled_ports) {
> +		dev_err(&pdev->dev, "No IRQ defined\n");
> +		rc = -ENODEV;
> +		goto err_out;
> +	}
> +
> +	if (enabled_ports > 1) {
> +		hpriv->flags |= AHCI_HFLAG_MULTI_MSI;
> +		hpriv->get_irq_vector = ahci_get_per_port_irq_vector;
> +	}
> +
>   	pm_runtime_enable(dev);
>   	pm_runtime_get_sync(dev);
>   	hpriv->got_runtime_pm = true;
> @@ -578,16 +631,7 @@ int ahci_platform_init_host(struct platform_device *pdev,
>   	struct ata_port_info pi = *pi_template;
>   	const struct ata_port_info *ppi[] = { &pi, NULL };
>   	struct ata_host *host;
> -	int i, irq, n_ports, rc;
> -
> -	irq = platform_get_irq(pdev, 0);
> -	if (irq <= 0) {
> -		if (irq != -EPROBE_DEFER)
> -			dev_err(dev, "no irq\n");
> -		return irq;
> -	}
> -
> -	hpriv->irq = irq;
> +	int i, n_ports, rc;
>   
>   	/* prepare host */
>   	pi.private_data = (void *)(unsigned long)hpriv->flags;
> diff --git a/drivers/ata/sata_highbank.c b/drivers/ata/sata_highbank.c
> index c8fc9280d6e4..dcfdab20021b 100644
> --- a/drivers/ata/sata_highbank.c
> +++ b/drivers/ata/sata_highbank.c
> @@ -496,7 +496,7 @@ static int ahci_highbank_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>   	}
>   
> -	hpriv->irq = irq;
> +	hpriv->irqs[0] = irq;
>   	hpriv->flags |= (unsigned long)pi.private_data;
>   
>   	hpriv->mmio = devm_ioremap(dev, mem->start, resource_size(mem));
> 

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

[Miquel Raynal]

Hi Russell,

Thanks for the review,

Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote on Fri, 22
Feb 2019 15:13:35 +0000:

> On Fri, Feb 22, 2019 at 03:53:56PM +0100, Miquel Raynal wrote:
> > From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> > 
> > 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 and enabling them in all the DTs including it
> > (7040-db/8040-db/8040-clearfog).
> > 
> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  arch/arm64/boot/dts/marvell/armada-7040-db.dts |  7 ++++++-
> >  arch/arm64/boot/dts/marvell/armada-8040-db.dts | 14 ++++++++++++--
> >  arch/arm64/boot/dts/marvell/armada-cp110.dtsi  | 16 ++++++++++++++--
> >  3 files changed, 32 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/marvell/armada-7040-db.dts b/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> > index 412efdb46e7c..54c1c0ddc813 100644
> > --- a/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> > +++ b/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> > @@ -194,7 +194,12 @@
> >  };
> >  
> >  &cp0_sata0 {
> > -	status = "okay";
> > +	sata-port@0 {  
> 
> Don't these all need a reg = <N> to correspond with @N in the node name?

Yes a reg property is added to the sata-port(s) sub-nodes when they are
defined (here they are just enabled, see below).

> 
> > +		status = "okay";
> > +	};
> > +	sata-port@1 {
> > +		status = "okay";
> > +	};
> >  };
> >  
> >  &cp0_usb3_0 {
> > diff --git a/arch/arm64/boot/dts/marvell/armada-8040-db.dts b/arch/arm64/boot/dts/marvell/armada-8040-db.dts
> > index 1bac437369a1..988cc7dc15d9 100644
> > --- a/arch/arm64/boot/dts/marvell/armada-8040-db.dts
> > +++ b/arch/arm64/boot/dts/marvell/armada-8040-db.dts
> > @@ -147,7 +147,12 @@
> >  
> >  /* CON4 on CP0 expansion */
> >  &cp0_sata0 {
> > -	status = "okay";
> > +	sata-port@0 {
> > +		status = "okay";
> > +	};
> > +	sata-port@1 {
> > +		status = "okay";
> > +	};
> >  };
> >  
> >  /* CON9 on CP0 expansion */
> > @@ -279,7 +284,12 @@
> >  
> >  /* CON4 on CP1 expansion */
> >  &cp1_sata0 {
> > -	status = "okay";
> > +	sata-port@0 {
> > +		status = "okay";
> > +	};
> > +	sata-port@1 {
> > +		status = "okay";
> > +	};
> >  };
> >  
> >  /* CON9 on CP1 expansion */
> > diff --git a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi
> > index b9d9f31e3ba1..f27edddcacd1 100644
> > --- a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi
> > +++ b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi
> > @@ -292,10 +292,22 @@
> >  			"generic-ahci";
> >  			reg = <0x540000 0x30000>;
> >  			dma-coherent;
> > -			interrupts = <107 IRQ_TYPE_LEVEL_HIGH>;
> >  			clocks = <&CP110_LABEL(clk) 1 15>,
> >  				 <&CP110_LABEL(clk) 1 16>;
> > -			status = "disabled";
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +
> > +			sata-port@0 {
> > +				reg = <0>;

Here                            ^

> > +				interrupts = <109 IRQ_TYPE_LEVEL_HIGH>;
> > +				status = "disabled";
> > +			};
> > +
> > +			sata-port@1 {
> > +				reg = <1>;
> > +				interrupts = <107 IRQ_TYPE_LEVEL_HIGH>;
> > +				status = "disabled";
> > +			};
> >  		};
> >  
> >  		CP110_LABEL(xor0): xor@6a0000 {
> > -- 
> > 2.19.1
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >   
> 


Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] ata: libahci_platform: Support per-port interrupts

[Miquel Raynal]

Hi Hans,

Hans de Goede <hdegoede@redhat.com> wrote on Fri, 22 Feb 2019 16:26:01
+0100:

> Hi,
> 
> On 2/22/19 3:53 PM, Miquel Raynal wrote:
> > Right now the ATA core only allows IPs to use a single interrupt. Some
> > of them (for instance the Armada-CP110 one) actually has one interrupt
> > per port. Add some logic to support such situation.
> > 
> > We consider that either there is one single interrupt declared in the
> > main IP node, or there are per-port interrupts, each of them being
> > declared in the port sub-nodes.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >   drivers/ata/acard-ahci.c       |  2 +-
> >   drivers/ata/ahci.c             |  2 +-
> >   drivers/ata/ahci.h             |  3 +-
> >   drivers/ata/libahci.c          |  2 +-
> >   drivers/ata/libahci_platform.c | 66 ++++++++++++++++++++++++++++------
> >   drivers/ata/sata_highbank.c    |  2 +-
> >   6 files changed, 61 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/ata/acard-ahci.c b/drivers/ata/acard-ahci.c
> > index 583e366be7e2..9414b81e994c 100644
> > --- a/drivers/ata/acard-ahci.c
> > +++ b/drivers/ata/acard-ahci.c
> > @@ -434,7 +434,7 @@ static int acard_ahci_init_one(struct pci_dev *pdev, const struct pci_device_id
> >   	if (!hpriv)
> >   		return -ENOMEM;  
> >   > -	hpriv->irq = pdev->irq;  
> > +	hpriv->irqs[0] = pdev->irq;
> >   	hpriv->flags |= (unsigned long)pi.private_data;
> >     
> What code-path is going to alloc hpriv->irqs for drivers using this code-path
> which are not using libahci_platform .c ?

I don't understand the question (or the remark behind the question),
can you explain a little bit more what you have in mind?


Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] ata: libahci_platform: Support per-port interrupts

[Hans de Goede]

Hi,

On 2/22/19 4:31 PM, Miquel Raynal wrote:
> Hi Hans,
> 
> Hans de Goede <hdegoede@redhat.com> wrote on Fri, 22 Feb 2019 16:26:01
> +0100:
> 
>> Hi,
>>
>> On 2/22/19 3:53 PM, Miquel Raynal wrote:
>>> Right now the ATA core only allows IPs to use a single interrupt. Some
>>> of them (for instance the Armada-CP110 one) actually has one interrupt
>>> per port. Add some logic to support such situation.
>>>
>>> We consider that either there is one single interrupt declared in the
>>> main IP node, or there are per-port interrupts, each of them being
>>> declared in the port sub-nodes.
>>>
>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>> ---
>>>    drivers/ata/acard-ahci.c       |  2 +-
>>>    drivers/ata/ahci.c             |  2 +-
>>>    drivers/ata/ahci.h             |  3 +-
>>>    drivers/ata/libahci.c          |  2 +-
>>>    drivers/ata/libahci_platform.c | 66 ++++++++++++++++++++++++++++------
>>>    drivers/ata/sata_highbank.c    |  2 +-
>>>    6 files changed, 61 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/ata/acard-ahci.c b/drivers/ata/acard-ahci.c
>>> index 583e366be7e2..9414b81e994c 100644
>>> --- a/drivers/ata/acard-ahci.c
>>> +++ b/drivers/ata/acard-ahci.c
>>> @@ -434,7 +434,7 @@ static int acard_ahci_init_one(struct pci_dev *pdev, const struct pci_device_id
>>>    	if (!hpriv)
>>>    		return -ENOMEM;
>>>    > -	hpriv->irq = pdev->irq;
>>> +	hpriv->irqs[0] = pdev->irq;
>>>    	hpriv->flags |= (unsigned long)pi.private_data;
>>>      
>> What code-path is going to alloc hpriv->irqs for drivers using this code-path
>> which are not using libahci_platform .c ?
> 
> I don't understand the question (or the remark behind the question),
> can you explain a little bit more what you have in mind?

Sorry I got the code context wrong I meant to put that comment below this chunk:

 > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
 > index 021ce46e2e57..18bce556d85f 100644
 > --- a/drivers/ata/ahci.c
 > +++ b/drivers/ata/ahci.c
 > @@ -1817,7 +1817,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 >   		/* legacy intx interrupts */
 >   		pci_intx(pdev, 1);
 >   	}
 > -	hpriv->irq = pci_irq_vector(pdev, 0);
 > +	hpriv->irqs[0] = pci_irq_vector(pdev, 0);
 >
 >   	if (!(hpriv->cap & HOST_CAP_SSS) || ahci_ignore_sss)
 >   		host->flags |= ATA_HOST_PARALLEL_SCAN;


Which AFAIK is a common code-path also used by ahci drivers not using
libahci_platform, and in that case hpriv->irqs will be NULL as nothing
initializes it.

Regards,

Hans

Re: [PATCH 2/5] ata: libahci_platform: Support per-port interrupts

[Miquel Raynal]

Hi Hans,

Hans de Goede <hdegoede@redhat.com> wrote on Fri, 22 Feb 2019 16:52:55
+0100:

> Hi,
> 
> On 2/22/19 4:31 PM, Miquel Raynal wrote:
> > Hi Hans,
> > 
> > Hans de Goede <hdegoede@redhat.com> wrote on Fri, 22 Feb 2019 16:26:01
> > +0100:
> >   
> >> Hi,
> >>
> >> On 2/22/19 3:53 PM, Miquel Raynal wrote:  
> >>> Right now the ATA core only allows IPs to use a single interrupt. Some
> >>> of them (for instance the Armada-CP110 one) actually has one interrupt
> >>> per port. Add some logic to support such situation.
> >>>
> >>> We consider that either there is one single interrupt declared in the
> >>> main IP node, or there are per-port interrupts, each of them being
> >>> declared in the port sub-nodes.
> >>>
> >>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> >>> ---
> >>>    drivers/ata/acard-ahci.c       |  2 +-
> >>>    drivers/ata/ahci.c             |  2 +-
> >>>    drivers/ata/ahci.h             |  3 +-
> >>>    drivers/ata/libahci.c          |  2 +-
> >>>    drivers/ata/libahci_platform.c | 66 ++++++++++++++++++++++++++++------
> >>>    drivers/ata/sata_highbank.c    |  2 +-
> >>>    6 files changed, 61 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/drivers/ata/acard-ahci.c b/drivers/ata/acard-ahci.c
> >>> index 583e366be7e2..9414b81e994c 100644
> >>> --- a/drivers/ata/acard-ahci.c
> >>> +++ b/drivers/ata/acard-ahci.c
> >>> @@ -434,7 +434,7 @@ static int acard_ahci_init_one(struct pci_dev *pdev, const struct pci_device_id
> >>>    	if (!hpriv)
> >>>    		return -ENOMEM;  
> >>>    > -	hpriv->irq = pdev->irq;  
> >>> +	hpriv->irqs[0] = pdev->irq;
> >>>    	hpriv->flags |= (unsigned long)pi.private_data;  
> >>>      >> What code-path is going to alloc hpriv->irqs for drivers using this code-path  
> >> which are not using libahci_platform .c ?  
> > 
> > I don't understand the question (or the remark behind the question),
> > can you explain a little bit more what you have in mind?  
> 
> Sorry I got the code context wrong I meant to put that comment below this chunk:
> 
>  > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>  > index 021ce46e2e57..18bce556d85f 100644
>  > --- a/drivers/ata/ahci.c
>  > +++ b/drivers/ata/ahci.c
>  > @@ -1817,7 +1817,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  >   		/* legacy intx interrupts */
>  >   		pci_intx(pdev, 1);
>  >   	}
>  > -	hpriv->irq = pci_irq_vector(pdev, 0);
>  > +	hpriv->irqs[0] = pci_irq_vector(pdev, 0);
>  >
>  >   	if (!(hpriv->cap & HOST_CAP_SSS) || ahci_ignore_sss)
>  >   		host->flags |= ATA_HOST_PARALLEL_SCAN;  
> 
> 
> Which AFAIK is a common code-path also used by ahci drivers not using
> libahci_platform, and in that case hpriv->irqs will be NULL as nothing
> initializes it.

Oh I see. What do you prefer:
1/
* I add "irqs" besides "irq" in the structure
* copy the value of irq in irqs[0]
* use irqs instead of irq in the libahci_platform ?
or
2/
* Allocated one irq there if there is none ?


Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] ata: libahci_platform: Support per-port interrupts

[Hans de Goede]

Hi,

On 2/22/19 5:03 PM, Miquel Raynal wrote:
> Hi Hans,
> 
> Hans de Goede <hdegoede@redhat.com> wrote on Fri, 22 Feb 2019 16:52:55
> +0100:
> 
>> Hi,
>>
>> On 2/22/19 4:31 PM, Miquel Raynal wrote:
>>> Hi Hans,
>>>
>>> Hans de Goede <hdegoede@redhat.com> wrote on Fri, 22 Feb 2019 16:26:01
>>> +0100:
>>>    
>>>> Hi,
>>>>
>>>> On 2/22/19 3:53 PM, Miquel Raynal wrote:
>>>>> Right now the ATA core only allows IPs to use a single interrupt. Some
>>>>> of them (for instance the Armada-CP110 one) actually has one interrupt
>>>>> per port. Add some logic to support such situation.
>>>>>
>>>>> We consider that either there is one single interrupt declared in the
>>>>> main IP node, or there are per-port interrupts, each of them being
>>>>> declared in the port sub-nodes.
>>>>>
>>>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>>>> ---
>>>>>     drivers/ata/acard-ahci.c       |  2 +-
>>>>>     drivers/ata/ahci.c             |  2 +-
>>>>>     drivers/ata/ahci.h             |  3 +-
>>>>>     drivers/ata/libahci.c          |  2 +-
>>>>>     drivers/ata/libahci_platform.c | 66 ++++++++++++++++++++++++++++------
>>>>>     drivers/ata/sata_highbank.c    |  2 +-
>>>>>     6 files changed, 61 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/drivers/ata/acard-ahci.c b/drivers/ata/acard-ahci.c
>>>>> index 583e366be7e2..9414b81e994c 100644
>>>>> --- a/drivers/ata/acard-ahci.c
>>>>> +++ b/drivers/ata/acard-ahci.c
>>>>> @@ -434,7 +434,7 @@ static int acard_ahci_init_one(struct pci_dev *pdev, const struct pci_device_id
>>>>>     	if (!hpriv)
>>>>>     		return -ENOMEM;
>>>>>     > -	hpriv->irq = pdev->irq;
>>>>> +	hpriv->irqs[0] = pdev->irq;
>>>>>     	hpriv->flags |= (unsigned long)pi.private_data;
>>>>>       >> What code-path is going to alloc hpriv->irqs for drivers using this code-path
>>>> which are not using libahci_platform .c ?
>>>
>>> I don't understand the question (or the remark behind the question),
>>> can you explain a little bit more what you have in mind?
>>
>> Sorry I got the code context wrong I meant to put that comment below this chunk:
>>
>>   > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>>   > index 021ce46e2e57..18bce556d85f 100644
>>   > --- a/drivers/ata/ahci.c
>>   > +++ b/drivers/ata/ahci.c
>>   > @@ -1817,7 +1817,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>>   >   		/* legacy intx interrupts */
>>   >   		pci_intx(pdev, 1);
>>   >   	}
>>   > -	hpriv->irq = pci_irq_vector(pdev, 0);
>>   > +	hpriv->irqs[0] = pci_irq_vector(pdev, 0);
>>   >
>>   >   	if (!(hpriv->cap & HOST_CAP_SSS) || ahci_ignore_sss)
>>   >   		host->flags |= ATA_HOST_PARALLEL_SCAN;
>>
>>
>> Which AFAIK is a common code-path also used by ahci drivers not using
>> libahci_platform, and in that case hpriv->irqs will be NULL as nothing
>> initializes it.
> 
> Oh I see. What do you prefer:
> 1/
> * I add "irqs" besides "irq" in the structure
> * copy the value of irq in irqs[0]
> * use irqs instead of irq in the libahci_platform ?
> or
> 2/
> * Allocated one irq there if there is none ?

I don't really have a preference, Jens what is your take on this?

Regards,

Hans

Re: [PATCH 2/5] ata: libahci_platform: Support per-port interrupts

[Marc Zyngier]

On Fri, 22 Feb 2019 16:03:48 +0000,
Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> Hi Hans,
> 
> Hans de Goede <hdegoede@redhat.com> wrote on Fri, 22 Feb 2019 16:52:55
> +0100:
> 
> > Hi,
> > 
> > On 2/22/19 4:31 PM, Miquel Raynal wrote:
> > > Hi Hans,
> > > 
> > > Hans de Goede <hdegoede@redhat.com> wrote on Fri, 22 Feb 2019 16:26:01
> > > +0100:
> > >   
> > >> Hi,
> > >>
> > >> On 2/22/19 3:53 PM, Miquel Raynal wrote:  
> > >>> Right now the ATA core only allows IPs to use a single interrupt. Some
> > >>> of them (for instance the Armada-CP110 one) actually has one interrupt
> > >>> per port. Add some logic to support such situation.
> > >>>
> > >>> We consider that either there is one single interrupt declared in the
> > >>> main IP node, or there are per-port interrupts, each of them being
> > >>> declared in the port sub-nodes.
> > >>>
> > >>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > >>> ---
> > >>>    drivers/ata/acard-ahci.c       |  2 +-
> > >>>    drivers/ata/ahci.c             |  2 +-
> > >>>    drivers/ata/ahci.h             |  3 +-
> > >>>    drivers/ata/libahci.c          |  2 +-
> > >>>    drivers/ata/libahci_platform.c | 66 ++++++++++++++++++++++++++++------
> > >>>    drivers/ata/sata_highbank.c    |  2 +-
> > >>>    6 files changed, 61 insertions(+), 16 deletions(-)
> > >>>
> > >>> diff --git a/drivers/ata/acard-ahci.c b/drivers/ata/acard-ahci.c
> > >>> index 583e366be7e2..9414b81e994c 100644
> > >>> --- a/drivers/ata/acard-ahci.c
> > >>> +++ b/drivers/ata/acard-ahci.c
> > >>> @@ -434,7 +434,7 @@ static int acard_ahci_init_one(struct pci_dev *pdev, const struct pci_device_id
> > >>>    	if (!hpriv)
> > >>>    		return -ENOMEM;  
> > >>>    > -	hpriv->irq = pdev->irq;  
> > >>> +	hpriv->irqs[0] = pdev->irq;
> > >>>    	hpriv->flags |= (unsigned long)pi.private_data;  
> > >>>      >> What code-path is going to alloc hpriv->irqs for drivers using this code-path  
> > >> which are not using libahci_platform .c ?  
> > > 
> > > I don't understand the question (or the remark behind the question),
> > > can you explain a little bit more what you have in mind?  
> > 
> > Sorry I got the code context wrong I meant to put that comment below this chunk:
> > 
> >  > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> >  > index 021ce46e2e57..18bce556d85f 100644
> >  > --- a/drivers/ata/ahci.c
> >  > +++ b/drivers/ata/ahci.c
> >  > @@ -1817,7 +1817,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> >  >   		/* legacy intx interrupts */
> >  >   		pci_intx(pdev, 1);
> >  >   	}
> >  > -	hpriv->irq = pci_irq_vector(pdev, 0);
> >  > +	hpriv->irqs[0] = pci_irq_vector(pdev, 0);
> >  >
> >  >   	if (!(hpriv->cap & HOST_CAP_SSS) || ahci_ignore_sss)
> >  >   		host->flags |= ATA_HOST_PARALLEL_SCAN;  
> > 
> > 
> > Which AFAIK is a common code-path also used by ahci drivers not using
> > libahci_platform, and in that case hpriv->irqs will be NULL as nothing
> > initializes it.
> 
> Oh I see. What do you prefer:
> 1/
> * I add "irqs" besides "irq" in the structure
> * copy the value of irq in irqs[0]
> * use irqs instead of irq in the libahci_platform ?
> or
> 2/
> * Allocated one irq there if there is none ?

3) you make it a union, and only use it as a pointer when some flag
somewhere says that you have multiple interrupts.

Yes, this is terrible, but it would limit the changes to the one
affected platform instead of inflicting the changes on all SATA users.

	M.

-- 
Jazz is not dead, it just smell funny.

Re: [PATCH 3/5] irqchip/irq-mvebu-icu: Move the double SATA ports interrupt hack

[Marc Zyngier]

On Fri, 22 Feb 2019 14:53:54 +0000,
Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> 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, let's
> clean this driver section and move the hack into the (historically)
> responsible SATA driver: ahci_platform.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/ata/ahci_platform.c     | 174 ++++++++++++++++++++++++++++++++
>  drivers/irqchip/irq-mvebu-icu.c |  18 ----
>  2 files changed, 174 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
> index cf1e0e18a7a9..4401a137e6d5 100644
> --- a/drivers/ata/ahci_platform.c
> +++ b/drivers/ata/ahci_platform.c
> @@ -17,6 +17,7 @@
>  #include <linux/pm.h>
>  #include <linux/device.h>
>  #include <linux/of_device.h>
> +#include <linux/of_irq.h>
>  #include <linux/platform_device.h>
>  #include <linux/libata.h>
>  #include <linux/ahci_platform.h>
> @@ -26,6 +27,9 @@
>  
>  #define DRV_NAME "ahci"
>  
> +#define ICU_SATA0_ICU_ID 109
> +#define ICU_SATA1_ICU_ID 107
> +
>  static const struct ata_port_info ahci_port_info = {
>  	.flags		= AHCI_FLAG_COMMON,
>  	.pio_mask	= ATA_PIO4,
> @@ -44,6 +48,170 @@ static struct scsi_host_template ahci_platform_sht = {
>  	AHCI_SHT(DRV_NAME),
>  };
>  
> +/*
> + * The Armada CP110 SATA unit (on A7k/A8k SoCs) 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, 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.
> + *
> + * Since then, this limitation has been addressed and the hack described
> + * above has been removed from the ICU driver. However, for compatibility
> + * it is needed to support DT with just one interrupt and no SATA ports.
> + * Instead of hacking everywhere in the ahci/libahci code, this quirk has been
> + * written to manually create the SATA port sub-nodes if they are missing,
> + * assign them the right port numbers through the "reg" properties and their
> + * respective "interrupts".

I don't think this write-up, however interesting, is useful here. That's the
kind of war story that makes perfect sense in a commit log, and not much in
the actual code. I'd suggest you keep the story short and only explain the
problem the workaround tries to paper over.

> + */
> +static int ahci_armada_8k_quirk(struct device *dev)
> +{
> +	struct device_node *dn = dev->of_node;
> +	struct property *interrupt = of_find_property(dn, "interrupts", NULL);

It'd make more sense if you checked the interrupt property once you actually
need it.

> +	struct device_node *ports;
> +	struct property *regs, *ints;
> +	int rc, i;
> +
> +	if (!(of_device_is_compatible(dn, "marvell,armada-8k-ahci") &&

Where is this compat string documented?

> +	      !of_get_child_count(dn)))
> +		return 0;
> +
> +	if (!of_node_get(dn))
> +		return -EINVAL;

Shouldn't you do that first, before having started to mess with the node?

> +
> +	/* Verify the main SATA node "interrupts" property validity */
> +	if (!interrupt) {
> +		rc = -EINVAL;
> +		goto put_dn;
> +	}
> +
> +	/* Create the two sub-nodes */
> +	ports = kzalloc(2 * sizeof(*ports), GFP_KERNEL);
> +	if (!ports) {
> +		rc = -ENOMEM;
> +		goto put_dn;
> +	}
> +
> +	for (i = 0; i < 2; i++) {
> +		of_node_set_flag(&ports[i], OF_DYNAMIC);

Do you really expect all users to now have to select OF_DYNAMIC to be able
to use their system? I'm not sure that's the right thing to do.

I'd rather see this workaround be handled at probe time, generating the
right data structures right away, rather than patching the DT from inside
the kernel to eventually generate these structures. Also, how does this work
for non-DT?

> +		of_node_init(&ports[i]);
> +		ports[i].parent = dn;
> +	}
> +
> +	ports[0].full_name = kstrdup("sata-port@0", GFP_KERNEL);
> +	ports[1].full_name = kstrdup("sata-port@1", GFP_KERNEL);
> +	if (!ports[0].full_name || !ports[1].full_name) {
> +		rc = -ENOMEM;
> +		goto free_ports_names;
> +	}
> +
> +	/* Create the two "reg" and "interrupts" property for the sub-nodes */
> +	regs = kzalloc(2 * sizeof(*regs), GFP_KERNEL);
> +	ints = kzalloc(2 * sizeof(*ints), GFP_KERNEL);
> +	if (!regs || !ints) {
> +		rc = -ENOMEM;
> +		goto free_props;
> +	}
> +
> +	for (i = 0; i < 2; i++) {
> +		regs[i].name = kstrdup("reg", GFP_KERNEL);
> +		regs[i].length = sizeof(u32);
> +		regs[i].value = kmalloc(sizeof(regs[i].length), GFP_KERNEL);
> +
> +		ints[i].name = kstrdup("interrupts", GFP_KERNEL);
> +		ints[i].length = interrupt->length;
> +		ints[i].value = kmemdup(interrupt->value, interrupt->length,
> +					GFP_KERNEL);
> +
> +		if (!regs[i].name || !regs[i].value ||
> +		    !ints[i].name || !ints[i].value) {
> +			rc = -ENOMEM;
> +			goto free_props_allocs;
> +		}
> +	}
> +
> +	/* Force the values of the ICU interrupts for both ports.

Comment formatting.

> +	 * In the past, "interrupts" properties had three values:
> +	 * 1/ ICU interrupt type, 2/ interrupt ID, 3/ interrupt type

I don't understand this comment. From what I can see in the last patch, the
interrupts property always had 2 cells.

> +	 * Now there are only two:
> +	 * 1/ interrupt ID, 2/ interrupt type
> +	 * In both case we want to access the penultimate.
> +	 */
> +	for (i = 0; i < 2; i++) {
> +		u32 *reg, *icu_int;
> +		u8 *value;
> +
> +		reg = regs[i].value;
> +		*reg = cpu_to_be32(i);
> +
> +		value = ints[i].value;
> +		value = &value[ints[i].length - (2 * sizeof(u32))];
> +		icu_int = (u32 *)value;
> +		if (!i)
> +			*icu_int = cpu_to_be32(ICU_SATA0_ICU_ID);
> +		else
> +			*icu_int = cpu_to_be32(ICU_SATA1_ICU_ID);
> +	}
> +
> +	for (i = 0; i < 2; i++) {
> +		ports[i].properties = &regs[i];
> +		regs[i].next = &ints[i];
> +		ints[i].next = NULL;
> +	}
> +
> +	/* Create the two sub-nodes by attaching them */
> +	rc = of_attach_node(&ports[0]);
> +	if (rc) {
> +		dev_err(dev, "Compat: cannot attach port0 (err %d)\n", rc);
> +		goto free_props_allocs;
> +	}
> +
> +	rc = of_attach_node(&ports[1]);
> +	if (rc) {
> +		dev_err(dev, "Compat: cannot attach port1 (err %d)\n", rc);
> +		goto detach_port0;
> +	}
> +
> +	/* Delete the "interrupts" property from the SATA host node */
> +	rc = of_remove_property(dn, interrupt);
> +	if (rc) {
> +		dev_err(dev, "Compat: cannot delete SATA host interrupt\n");
> +		goto detach_ports;
> +	}
> +
> +	of_node_put(dn);
> +
> +	return 0;
> +
> +detach_ports:
> +	of_detach_node(&ports[1]);
> +
> +detach_port0:
> +	of_detach_node(&ports[0]);
> +
> +free_props_allocs:
> +	for (i = 0; i < 2; i++) {
> +		kfree(regs[i].name);
> +		kfree(regs[i].value);
> +		kfree(ints[i].name);
> +		kfree(ints[i].value);
> +	}
> +
> +free_props:
> +	kfree(regs);
> +	kfree(ints);
> +
> +free_ports_names:
> +	for (i = 0; i < 2; i++)
> +		kfree(ports[i].full_name);
> +
> +	kfree(ports);

How about using devm allocations instead, since you're going to fail the
probe anyway?

> +
> +put_dn:
> +	of_node_put(dn);
> +
> +	return rc;
> +	}
> +
>  static int ahci_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -51,6 +219,12 @@ static int ahci_probe(struct platform_device *pdev)
>  	const struct ata_port_info *port;
>  	int rc;
>  
> +	rc = ahci_armada_8k_quirk(dev);
> +	if (rc) {
> +		dev_err(dev, "Problem with CP110 backward compatibility\n");
> +		return rc;
> +	}
> +
>  	hpriv = ahci_platform_get_resources(pdev,
>  					    AHCI_PLATFORM_GET_RESETS);
>  	if (IS_ERR(hpriv))
> diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c
> index 547045d89c4b..f3b43f63fe2e 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 = {

Clearly, this is not much of an irqchip patch. I'd expect you either extract
the irqchip bit in a separate patch.

Overall, I'm not sure this patch can fly as is. The dependency on OF_DYNAMIC
is not exactly great, and the whole thing is likely to fail anyway, as you
don't even bother selecting that new dependency. I strongly suggest you
implement the quirk by generating the right structures at the right time.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

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

[Marc Zyngier]

On Fri, 22 Feb 2019 14:53:56 +0000,
Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> 
> 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 and enabling them in all the DTs including it
> (7040-db/8040-db/8040-clearfog).
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  arch/arm64/boot/dts/marvell/armada-7040-db.dts |  7 ++++++-
>  arch/arm64/boot/dts/marvell/armada-8040-db.dts | 14 ++++++++++++--
>  arch/arm64/boot/dts/marvell/armada-cp110.dtsi  | 16 ++++++++++++++--
>  3 files changed, 32 insertions(+), 5 deletions(-)
> 

[...]

> diff --git a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi
> index b9d9f31e3ba1..f27edddcacd1 100644
> --- a/arch/arm64/boot/dts/marvell/armada-cp110.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-cp110.dtsi
> @@ -292,10 +292,22 @@
>  			"generic-ahci";
>  			reg = <0x540000 0x30000>;
>  			dma-coherent;
> -			interrupts = <107 IRQ_TYPE_LEVEL_HIGH>;
>  			clocks = <&CP110_LABEL(clk) 1 15>,
>  				 <&CP110_LABEL(clk) 1 16>;
> -			status = "disabled";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			sata-port@0 {
> +				reg = <0>;
> +				interrupts = <109 IRQ_TYPE_LEVEL_HIGH>;

Where has this change to the binding been documented?

> +				status = "disabled";
> +			};
> +
> +			sata-port@1 {
> +				reg = <1>;
> +				interrupts = <107 IRQ_TYPE_LEVEL_HIGH>;
> +				status = "disabled";
> +			};
>  		};
>  
>  		CP110_LABEL(xor0): xor@6a0000 {
> -- 
> 2.19.1
> 

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

Re: [PATCH 4/5] arm64: dts: marvell: armada-8040-clearfog: Drop non-existent SATA port

[Baruch Siach]

Hi Miquel,

On Fri, Feb 22 2019, Miquel Raynal wrote:

> There is no CP110 SATA port available on the 8040 Clearfog A8k, SATA
> may be used thanks to a mPCIe -> SATA extension board only. Hence, the
> cp1_sata0 node must be removed from the device tree.

Not true. You can use the mini PCIe serdes as SATA directly if you
configure it as such. You only need to invert the serdes Rx pair
polarity. This is the default configuration for the Clearfog GT-8K CON3
mini-PCIe slot (CP1, lane #0) in current mainline U-Boot. I verified
that this setup works on Clearfog GT-8K.

This patch would break mini PCIe direct SATA.

baruch

>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
> index 5b4a9609e31f..caabbd3a85a8 100644
> --- a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
> @@ -333,11 +333,6 @@
>  	};
>  };
>
> -&cp1_sata0 {
> -	pinctrl-0 = <&cp0_pci1_reset_pins>;
> -	status = "okay";
> -};
> -
>  &cp1_mdio {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&cp1_ge_mdio_pins>;


--
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH 4/5] arm64: dts: marvell: armada-8040-clearfog: Drop non-existent SATA port

[Miquel Raynal]

Hi Baruch,

Baruch Siach <baruch@tkos.co.il> wrote on Sun, 24 Feb 2019 07:29:09
+0200:

> Hi Miquel,
> 
> On Fri, Feb 22 2019, Miquel Raynal wrote:
> 
> > There is no CP110 SATA port available on the 8040 Clearfog A8k, SATA
> > may be used thanks to a mPCIe -> SATA extension board only. Hence, the
> > cp1_sata0 node must be removed from the device tree.  
> 
> Not true. You can use the mini PCIe serdes as SATA directly if you
> configure it as such. You only need to invert the serdes Rx pair
> polarity. This is the default configuration for the Clearfog GT-8K CON3
> mini-PCIe slot (CP1, lane #0) in current mainline U-Boot. I verified
> that this setup works on Clearfog GT-8K.
> 
> This patch would break mini PCIe direct SATA.

Thanks for explaining, I am a little bit surprised that it actually
uses the SATA host IP on CP110 but fine. So can you tell me which SATA
port is used in this case? Because I will have to update the DT
representation along with the CP110 changes.

Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] arm64: dts: marvell: armada-8040-clearfog: Drop non-existent SATA port

[Baruch Siach]

Hi Miquel,

On Mon, Feb 25, 2019 at 11:58:26AM +0100, Miquel Raynal wrote:
> Baruch Siach <baruch@tkos.co.il> wrote on Sun, 24 Feb 2019 07:29:09
> +0200:
> 
> > On Fri, Feb 22 2019, Miquel Raynal wrote:
> > > There is no CP110 SATA port available on the 8040 Clearfog A8k, SATA
> > > may be used thanks to a mPCIe -> SATA extension board only. Hence, the
> > > cp1_sata0 node must be removed from the device tree.  
> > 
> > Not true. You can use the mini PCIe serdes as SATA directly if you
> > configure it as such. You only need to invert the serdes Rx pair
> > polarity. This is the default configuration for the Clearfog GT-8K CON3
> > mini-PCIe slot (CP1, lane #0) in current mainline U-Boot. I verified
> > that this setup works on Clearfog GT-8K.
> > 
> > This patch would break mini PCIe direct SATA.
> 
> Thanks for explaining, I am a little bit surprised that it actually
> uses the SATA host IP on CP110 but fine. So can you tell me which SATA
> port is used in this case? Because I will have to update the DT
> representation along with the CP110 changes.

According to the cp110_comphy_phy_mux_data[] array in U-Boot 
drivers/phy/marvell/comphy_cp110.c, serdes 0 of CP110 can only be SATA1 (i.e. 
the second port; first is SATA0).

Please Cc me on your next submission.

Thanks,
baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH 4/5] arm64: dts: marvell: armada-8040-clearfog: Drop non-existent SATA port

[Russell King - ARM Linux admin]

On Mon, Feb 25, 2019 at 02:15:19PM +0200, Baruch Siach wrote:
> Hi Miquel,
> 
> On Mon, Feb 25, 2019 at 11:58:26AM +0100, Miquel Raynal wrote:
> > Baruch Siach <baruch@tkos.co.il> wrote on Sun, 24 Feb 2019 07:29:09
> > +0200:
> > 
> > > On Fri, Feb 22 2019, Miquel Raynal wrote:
> > > > There is no CP110 SATA port available on the 8040 Clearfog A8k, SATA
> > > > may be used thanks to a mPCIe -> SATA extension board only. Hence, the
> > > > cp1_sata0 node must be removed from the device tree.  
> > > 
> > > Not true. You can use the mini PCIe serdes as SATA directly if you
> > > configure it as such. You only need to invert the serdes Rx pair
> > > polarity. This is the default configuration for the Clearfog GT-8K CON3
> > > mini-PCIe slot (CP1, lane #0) in current mainline U-Boot. I verified
> > > that this setup works on Clearfog GT-8K.
> > > 
> > > This patch would break mini PCIe direct SATA.
> > 
> > Thanks for explaining, I am a little bit surprised that it actually
> > uses the SATA host IP on CP110 but fine. So can you tell me which SATA
> > port is used in this case? Because I will have to update the DT
> > representation along with the CP110 changes.
> 
> According to the cp110_comphy_phy_mux_data[] array in U-Boot 
> drivers/phy/marvell/comphy_cp110.c, serdes 0 of CP110 can only be SATA1 (i.e. 
> the second port; first is SATA0).

Adding folk from SolidRun...

Why are the mPCIe connectors configured by default for mSATA cards?
This sounds like it's going to cause confusion.  The published
specification for the board at:

https://developer.solid-run.com/products/clearfog-gt-8k/

states that the board has "3 x mPCIe (USB 2.0 + PCIe)" and makes no
mention of mSATA.

mSATA is not compatible with mPCIe - mSATA cards expect the serdes
lanes to be connected to a SATA interface, mPCIe cards expect a PCIe
controller at the other end of the serdes lanes.

Given that there is a lot of confusion about mSATA vs mPCIe out there,
(caused by both being the same physical form factor and fitting into
the same socket, yet being electrically different) I think it's
important to have a coherent story on these connectors everywhere.

Maybe we need a way to have these connectors configurable by the end
user?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH 3/5] irqchip/irq-mvebu-icu: Move the double SATA ports interrupt hack

[Miquel Raynal]

Hi Marc,

Marc Zyngier <marc.zyngier@arm.com> wrote on Sat, 23 Feb 2019 19:19:27
+0000:

> On Fri, 22 Feb 2019 14:53:54 +0000,
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > 
> > 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, let's
> > clean this driver section and move the hack into the (historically)
> > responsible SATA driver: ahci_platform.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/ata/ahci_platform.c     | 174 ++++++++++++++++++++++++++++++++
> >  drivers/irqchip/irq-mvebu-icu.c |  18 ----
> >  2 files changed, 174 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
> > index cf1e0e18a7a9..4401a137e6d5 100644
> > --- a/drivers/ata/ahci_platform.c
> > +++ b/drivers/ata/ahci_platform.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/pm.h>
> >  #include <linux/device.h>
> >  #include <linux/of_device.h>
> > +#include <linux/of_irq.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/libata.h>
> >  #include <linux/ahci_platform.h>
> > @@ -26,6 +27,9 @@
> >  
> >  #define DRV_NAME "ahci"
> >  
> > +#define ICU_SATA0_ICU_ID 109
> > +#define ICU_SATA1_ICU_ID 107
> > +
> >  static const struct ata_port_info ahci_port_info = {
> >  	.flags		= AHCI_FLAG_COMMON,
> >  	.pio_mask	= ATA_PIO4,
> > @@ -44,6 +48,170 @@ static struct scsi_host_template ahci_platform_sht = {
> >  	AHCI_SHT(DRV_NAME),
> >  };
> >  
> > +/*
> > + * The Armada CP110 SATA unit (on A7k/A8k SoCs) 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, 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.
> > + *
> > + * Since then, this limitation has been addressed and the hack described
> > + * above has been removed from the ICU driver. However, for compatibility
> > + * it is needed to support DT with just one interrupt and no SATA ports.
> > + * Instead of hacking everywhere in the ahci/libahci code, this quirk has been
> > + * written to manually create the SATA port sub-nodes if they are missing,
> > + * assign them the right port numbers through the "reg" properties and their
> > + * respective "interrupts".  
> 
> I don't think this write-up, however interesting, is useful here. That's the
> kind of war story that makes perfect sense in a commit log, and not much in
> the actual code. I'd suggest you keep the story short and only explain the
> problem the workaround tries to paper over.
> 

Sure.

> > + */
> > +static int ahci_armada_8k_quirk(struct device *dev)
> > +{
> > +	struct device_node *dn = dev->of_node;
> > +	struct property *interrupt = of_find_property(dn, "interrupts", NULL);  
> 
> It'd make more sense if you checked the interrupt property once you actually
> need it.

Ok.

> 
> > +	struct device_node *ports;
> > +	struct property *regs, *ints;
> > +	int rc, i;
> > +
> > +	if (!(of_device_is_compatible(dn, "marvell,armada-8k-ahci") &&  
> 
> Where is this compat string documented?

This string already exists, but you are right that it should have been
documented in Documentation/devicetree/bindings/ata/ahci-platform.txt.
I will write a patch for that.

> 
> > +	      !of_get_child_count(dn)))
> > +		return 0;
> > +
> > +	if (!of_node_get(dn))
> > +		return -EINVAL;  
> 
> Shouldn't you do that first, before having started to mess with the node?

Ok.

> 
> > +
> > +	/* Verify the main SATA node "interrupts" property validity */
> > +	if (!interrupt) {
> > +		rc = -EINVAL;
> > +		goto put_dn;
> > +	}
> > +
> > +	/* Create the two sub-nodes */
> > +	ports = kzalloc(2 * sizeof(*ports), GFP_KERNEL);
> > +	if (!ports) {
> > +		rc = -ENOMEM;
> > +		goto put_dn;
> > +	}
> > +
> > +	for (i = 0; i < 2; i++) {
> > +		of_node_set_flag(&ports[i], OF_DYNAMIC);  
> 
> Do you really expect all users to now have to select OF_DYNAMIC to be able
> to use their system? I'm not sure that's the right thing to do.

I overlooked that point.

However, I just ran a "make ARCH=arm64 defconfig" and it enables
OF_DYNAMIC, so shall we still consider it as a problem?

Would a "select OF_DYNAMIC if ARM64" under "config MVEBU_AHCI" be
appropriate?

> I'd rather see this workaround be handled at probe time, generating the
> right data structures right away, rather than patching the DT from inside
> the kernel to eventually generate these structures. Also, how does this work
> for non-DT?

My first intention was to allocate a second interrupt at probe time,
but this would have not worked because there is a lot of core code
relying on the number of SATA ports (it has an impact on how the core
deals with interrupts).

Doing what you propose is IMHO too hackish and invasive, it would
impact a lot of core code out of the ahci-platform driver because it is
there that the DT is parsed and the structures are allocated.

Also for non-DT situation, I wonder if that is even possible due to the
fact that most of the libahci_platform code relies on of_ functions,
of_ structures, the number of child nodes, etc.

> 
> > +		of_node_init(&ports[i]);
> > +		ports[i].parent = dn;
> > +	}
> > +
> > +	ports[0].full_name = kstrdup("sata-port@0", GFP_KERNEL);
> > +	ports[1].full_name = kstrdup("sata-port@1", GFP_KERNEL);
> > +	if (!ports[0].full_name || !ports[1].full_name) {
> > +		rc = -ENOMEM;
> > +		goto free_ports_names;
> > +	}
> > +
> > +	/* Create the two "reg" and "interrupts" property for the sub-nodes */
> > +	regs = kzalloc(2 * sizeof(*regs), GFP_KERNEL);
> > +	ints = kzalloc(2 * sizeof(*ints), GFP_KERNEL);
> > +	if (!regs || !ints) {
> > +		rc = -ENOMEM;
> > +		goto free_props;
> > +	}
> > +
> > +	for (i = 0; i < 2; i++) {
> > +		regs[i].name = kstrdup("reg", GFP_KERNEL);
> > +		regs[i].length = sizeof(u32);
> > +		regs[i].value = kmalloc(sizeof(regs[i].length), GFP_KERNEL);
> > +
> > +		ints[i].name = kstrdup("interrupts", GFP_KERNEL);
> > +		ints[i].length = interrupt->length;
> > +		ints[i].value = kmemdup(interrupt->value, interrupt->length,
> > +					GFP_KERNEL);
> > +
> > +		if (!regs[i].name || !regs[i].value ||
> > +		    !ints[i].name || !ints[i].value) {
> > +			rc = -ENOMEM;
> > +			goto free_props_allocs;
> > +		}
> > +	}
> > +
> > +	/* Force the values of the ICU interrupts for both ports.  
> 
> Comment formatting.

Missed that, sorry.

> 
> > +	 * In the past, "interrupts" properties had three values:
> > +	 * 1/ ICU interrupt type, 2/ interrupt ID, 3/ interrupt type  
> 
> I don't understand this comment. From what I can see in the last patch, the
> interrupts property always had 2 cells.

Yes, but the bindings for ICU interrupts have changed in a near past
from 3 to 2 cells, we removed the ICU_NSR entry because now there is a
"parent" (GICP, SEI, REI) which will help knowing the interrupt
"type".

It is backward-backward-compatiblity :-/

> 
> > +	 * Now there are only two:
> > +	 * 1/ interrupt ID, 2/ interrupt type
> > +	 * In both case we want to access the penultimate.
> > +	 */
> > +	for (i = 0; i < 2; i++) {
> > +		u32 *reg, *icu_int;
> > +		u8 *value;
> > +
> > +		reg = regs[i].value;
> > +		*reg = cpu_to_be32(i);
> > +
> > +		value = ints[i].value;
> > +		value = &value[ints[i].length - (2 * sizeof(u32))];
> > +		icu_int = (u32 *)value;
> > +		if (!i)
> > +			*icu_int = cpu_to_be32(ICU_SATA0_ICU_ID);
> > +		else
> > +			*icu_int = cpu_to_be32(ICU_SATA1_ICU_ID);
> > +	}
> > +
> > +	for (i = 0; i < 2; i++) {
> > +		ports[i].properties = &regs[i];
> > +		regs[i].next = &ints[i];
> > +		ints[i].next = NULL;
> > +	}
> > +
> > +	/* Create the two sub-nodes by attaching them */
> > +	rc = of_attach_node(&ports[0]);
> > +	if (rc) {
> > +		dev_err(dev, "Compat: cannot attach port0 (err %d)\n", rc);
> > +		goto free_props_allocs;
> > +	}
> > +
> > +	rc = of_attach_node(&ports[1]);
> > +	if (rc) {
> > +		dev_err(dev, "Compat: cannot attach port1 (err %d)\n", rc);
> > +		goto detach_port0;
> > +	}
> > +
> > +	/* Delete the "interrupts" property from the SATA host node */
> > +	rc = of_remove_property(dn, interrupt);
> > +	if (rc) {
> > +		dev_err(dev, "Compat: cannot delete SATA host interrupt\n");
> > +		goto detach_ports;
> > +	}
> > +
> > +	of_node_put(dn);
> > +
> > +	return 0;
> > +
> > +detach_ports:
> > +	of_detach_node(&ports[1]);
> > +
> > +detach_port0:
> > +	of_detach_node(&ports[0]);
> > +
> > +free_props_allocs:
> > +	for (i = 0; i < 2; i++) {
> > +		kfree(regs[i].name);
> > +		kfree(regs[i].value);
> > +		kfree(ints[i].name);
> > +		kfree(ints[i].value);
> > +	}
> > +
> > +free_props:
> > +	kfree(regs);
> > +	kfree(ints);
> > +
> > +free_ports_names:
> > +	for (i = 0; i < 2; i++)
> > +		kfree(ports[i].full_name);
> > +
> > +	kfree(ports);  
> 
> How about using devm allocations instead, since you're going to fail the
> probe anyway?

Yes, will be cleaner.

> 
> > +
> > +put_dn:
> > +	of_node_put(dn);
> > +
> > +	return rc;
> > +	}
> > +
> >  static int ahci_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > @@ -51,6 +219,12 @@ static int ahci_probe(struct platform_device *pdev)
> >  	const struct ata_port_info *port;
> >  	int rc;
> >  
> > +	rc = ahci_armada_8k_quirk(dev);
> > +	if (rc) {
> > +		dev_err(dev, "Problem with CP110 backward compatibility\n");
> > +		return rc;
> > +	}
> > +
> >  	hpriv = ahci_platform_get_resources(pdev,
> >  					    AHCI_PLATFORM_GET_RESETS);
> >  	if (IS_ERR(hpriv))
> > diff --git a/drivers/irqchip/irq-mvebu-icu.c b/drivers/irqchip/irq-mvebu-icu.c
> > index 547045d89c4b..f3b43f63fe2e 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 = {  
> 
> Clearly, this is not much of an irqchip patch. I'd expect you either extract
> the irqchip bit in a separate patch.

I am not sure this is valid as it would break bisectability. I have not
tested if it actually fails though (will do and split the patch
depending on the outcome).

> 
> Overall, I'm not sure this patch can fly as is. The dependency on OF_DYNAMIC
> is not exactly great, and the whole thing is likely to fail anyway, as you
> don't even bother selecting that new dependency. I strongly suggest you
> implement the quirk by generating the right structures at the right time.
> 
> Thanks,
> 
> 	M.
> 


Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] ata: libahci_platform: Support per-port interrupts

[Jens Axboe]

On 2/22/19 9:10 AM, Hans de Goede wrote:
> Hi,
> 
> On 2/22/19 5:03 PM, Miquel Raynal wrote:
>> Hi Hans,
>>
>> Hans de Goede <hdegoede@redhat.com> wrote on Fri, 22 Feb 2019 16:52:55
>> +0100:
>>
>>> Hi,
>>>
>>> On 2/22/19 4:31 PM, Miquel Raynal wrote:
>>>> Hi Hans,
>>>>
>>>> Hans de Goede <hdegoede@redhat.com> wrote on Fri, 22 Feb 2019 16:26:01
>>>> +0100:
>>>>    
>>>>> Hi,
>>>>>
>>>>> On 2/22/19 3:53 PM, Miquel Raynal wrote:
>>>>>> Right now the ATA core only allows IPs to use a single interrupt. Some
>>>>>> of them (for instance the Armada-CP110 one) actually has one interrupt
>>>>>> per port. Add some logic to support such situation.
>>>>>>
>>>>>> We consider that either there is one single interrupt declared in the
>>>>>> main IP node, or there are per-port interrupts, each of them being
>>>>>> declared in the port sub-nodes.
>>>>>>
>>>>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>>>>> ---
>>>>>>     drivers/ata/acard-ahci.c       |  2 +-
>>>>>>     drivers/ata/ahci.c             |  2 +-
>>>>>>     drivers/ata/ahci.h             |  3 +-
>>>>>>     drivers/ata/libahci.c          |  2 +-
>>>>>>     drivers/ata/libahci_platform.c | 66 ++++++++++++++++++++++++++++------
>>>>>>     drivers/ata/sata_highbank.c    |  2 +-
>>>>>>     6 files changed, 61 insertions(+), 16 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/ata/acard-ahci.c b/drivers/ata/acard-ahci.c
>>>>>> index 583e366be7e2..9414b81e994c 100644
>>>>>> --- a/drivers/ata/acard-ahci.c
>>>>>> +++ b/drivers/ata/acard-ahci.c
>>>>>> @@ -434,7 +434,7 @@ static int acard_ahci_init_one(struct pci_dev *pdev, const struct pci_device_id
>>>>>>     	if (!hpriv)
>>>>>>     		return -ENOMEM;
>>>>>>     > -	hpriv->irq = pdev->irq;
>>>>>> +	hpriv->irqs[0] = pdev->irq;
>>>>>>     	hpriv->flags |= (unsigned long)pi.private_data;
>>>>>>       >> What code-path is going to alloc hpriv->irqs for drivers using this code-path
>>>>> which are not using libahci_platform .c ?
>>>>
>>>> I don't understand the question (or the remark behind the question),
>>>> can you explain a little bit more what you have in mind?
>>>
>>> Sorry I got the code context wrong I meant to put that comment below this chunk:
>>>
>>>   > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>>>   > index 021ce46e2e57..18bce556d85f 100644
>>>   > --- a/drivers/ata/ahci.c
>>>   > +++ b/drivers/ata/ahci.c
>>>   > @@ -1817,7 +1817,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>   >   		/* legacy intx interrupts */
>>>   >   		pci_intx(pdev, 1);
>>>   >   	}
>>>   > -	hpriv->irq = pci_irq_vector(pdev, 0);
>>>   > +	hpriv->irqs[0] = pci_irq_vector(pdev, 0);
>>>   >
>>>   >   	if (!(hpriv->cap & HOST_CAP_SSS) || ahci_ignore_sss)
>>>   >   		host->flags |= ATA_HOST_PARALLEL_SCAN;
>>>
>>>
>>> Which AFAIK is a common code-path also used by ahci drivers not using
>>> libahci_platform, and in that case hpriv->irqs will be NULL as nothing
>>> initializes it.
>>
>> Oh I see. What do you prefer:
>> 1/
>> * I add "irqs" besides "irq" in the structure
>> * copy the value of irq in irqs[0]
>> * use irqs instead of irq in the libahci_platform ?
>> or
>> 2/
>> * Allocated one irq there if there is none ?
> 
> I don't really have a preference, Jens what is your take on this?

Single array would be the cleanest, don't add an irqs[] beside the
irq.

-- 
Jens Axboe

Re: [PATCH 4/5] arm64: dts: marvell: armada-8040-clearfog: Drop non-existent SATA port

[Baruch Siach]

Hi Russell,

On Mon, Feb 25 2019, Russell King wrote:
> On Mon, Feb 25, 2019 at 02:15:19PM +0200, Baruch Siach wrote:
>> On Mon, Feb 25, 2019 at 11:58:26AM +0100, Miquel Raynal wrote:
>> > Baruch Siach <baruch@tkos.co.il> wrote on Sun, 24 Feb 2019 07:29:09
>> > +0200:
>> >
>> > > On Fri, Feb 22 2019, Miquel Raynal wrote:
>> > > > There is no CP110 SATA port available on the 8040 Clearfog A8k, SATA
>> > > > may be used thanks to a mPCIe -> SATA extension board only. Hence, the
>> > > > cp1_sata0 node must be removed from the device tree.
>> > >
>> > > Not true. You can use the mini PCIe serdes as SATA directly if you
>> > > configure it as such. You only need to invert the serdes Rx pair
>> > > polarity. This is the default configuration for the Clearfog GT-8K CON3
>> > > mini-PCIe slot (CP1, lane #0) in current mainline U-Boot. I verified
>> > > that this setup works on Clearfog GT-8K.
>> > >
>> > > This patch would break mini PCIe direct SATA.
>> >
>> > Thanks for explaining, I am a little bit surprised that it actually
>> > uses the SATA host IP on CP110 but fine. So can you tell me which SATA
>> > port is used in this case? Because I will have to update the DT
>> > representation along with the CP110 changes.
>>
>> According to the cp110_comphy_phy_mux_data[] array in U-Boot
>> drivers/phy/marvell/comphy_cp110.c, serdes 0 of CP110 can only be SATA1 (i.e.
>> the second port; first is SATA0).
>
> Adding folk from SolidRun...
>
> Why are the mPCIe connectors configured by default for mSATA cards?

This is sort of capability demonstration.

> This sounds like it's going to cause confusion.  The published
> specification for the board at:
>
> https://developer.solid-run.com/products/clearfog-gt-8k/
>
> states that the board has "3 x mPCIe (USB 2.0 + PCIe)" and makes no
> mention of mSATA.
>
> mSATA is not compatible with mPCIe - mSATA cards expect the serdes
> lanes to be connected to a SATA interface, mPCIe cards expect a PCIe
> controller at the other end of the serdes lanes.
>
> Given that there is a lot of confusion about mSATA vs mPCIe out there,
> (caused by both being the same physical form factor and fitting into
> the same socket, yet being electrically different) I think it's
> important to have a coherent story on these connectors everywhere.
>
> Maybe we need a way to have these connectors configurable by the end
> user?

The user can set the PCIe/SATA serdes configuration in U-Boot CP110
comphy nodes.

baruch

--
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -