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

[PATCH 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,

I am resubmitting this patch series since I did not get a feedback
the past 8 weeks.

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

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.20.1

[PATCH 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.20.1

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

[Marc Zyngier]

On 2020-10-07 16:45, sven.auhagen@voleatech.de wrote:
> 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>

You are missing your own sign-off, as you are posting someone
else's patch.

         M.
-- 
Jazz is not dead. It just smells funny...

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

[Sven Auhagen]

On Wed, Oct 07, 2020 at 05:01:15PM +0100, Marc Zyngier wrote:
> On 2020-10-07 16:45, sven.auhagen@voleatech.de wrote:
> > 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>
> 
> You are missing your own sign-off, as you are posting someone
> else's patch.

Ah, I did not know that it was necessary.
I used the patch without any changes.

I will add it for the next version, thanks.

Best
Sven

> 
>         M.
> -- 
> Jazz is not dead. It just smells funny...

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

[Hans de Goede]

Hi,

On 10/7/20 5:45 PM, sven.auhagen@voleatech.de wrote:
> 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>

Patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  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[] = {
> 

[PATCH 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.20.1

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

[Hans de Goede]

Hi,

On 10/7/20 5:45 PM, sven.auhagen@voleatech.de wrote:
> 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>

Patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> ---
>  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);
> 

[PATCH 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.20.1

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

[Hans de Goede]

Hi,

On 10/7/20 5:45 PM, sven.auhagen@voleatech.de wrote:
> 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>

See my comments inline.

> ---
>  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);

This bit + the but making the function non static and exporting
it needs to go into its own separate patch.

> 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);
> +}

This is more or less a 1:1 copy of ahci_host_activate_multi_irqs(). Please don't go
around and make copies of functions (also see below).

The only difference seems to be the use of ahci_multi_irqs_intr_hard_armada8k
where as ahci_host_activate_multi_irqs() uses ahci_multi_irqs_intr_hard as IRQ handler.

And ahci_multi_irqs_intr_hard_armada8k itself is a copy of
ahci_multi_irqs_intr_hard() with a small change

No, just no all this function copying is not ok, hard NACK!

Instead I suggest that you add an "multi_irq_host_ack" callback to some
shared data struct, which will then get called by ahci_multi_irqs_intr_hard()
(when defined) in the place where you have added the following line:

	writel(BIT(ap->port_no), mmio + HOST_IRQ_STAT);

To your private copy of ahci_multi_irqs_intr_hard(), then you can drop
both the ahci_multi_irqs_intr_hard() and the ahci_host_activate_multi_irqs()
copies and reduce the size of this patch by a lot of lines, again also see
below for dropping even more lines!

> +
> +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);
> +}
> +

So this is basically a copy of ahci_platform_init_host() with:

1. This bit:

        irq = platform_get_irq(pdev, 0);
        if (irq <= 0) {
                if (irq != -EPROBE_DEFER)
                        dev_err(dev, "no irq\n");
                return irq;
        }

        hpriv->irq = irq;

Replaced with:

> +	/* 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;

You can just do this new bit, before calling ahci_platform_init_host()
and then disable the bit of ahci_platform_init_host() which you do not
want by adding an if around it like this:

	if (!(hpriv->flags & AHCI_HFLAG_MULTI_MSI)) {
	        irq = platform_get_irq(pdev, 0);
        	if (irq <= 0) {
                	if (irq != -EPROBE_DEFER)
                        	dev_err(dev, "no irq\n");
	                return irq;
        	}
	        hpriv->irq = irq;
	}

Note please add this new if() check in a separate patch.

The only other change to ahci_platform_init_host() here is
the function ending with:

	return ahci_host_activate_multi_irqs_armada8k(host, sht);

Instead of with:

       return ahci_host_activate(host, sht);

As mentioned above you really do NOT need that function, which
is yet another copy. Instead make the changes which I suggested
above and set the AHCI_HFLAG_MULTI_MSI flag to make
ahci_host_activate() do what you want.

After this you can drop all 3 private functions of pre-existing
ahci functions AFAICT.

>  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);

And this change can go away then too...

>  	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)
>  {

As already mentioned this needs to go to a separate patch.

> 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 */
> 

These last 2 changes need to go into a separate patch and please rename the quirk to
AHCI_PLATFORM_ARMADA8K_QUIRK to make it clear that it is for armada SoCs.

Regards,

Hans

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

[Sven Auhagen]

On Tue, Oct 27, 2020 at 03:22:15PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 10/7/20 5:45 PM, sven.auhagen@voleatech.de wrote:
> > 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>
> 
> See my comments inline.

Hi Hans,

thank you for your feedback, that is very helpful.
I will get on the changes and resubmit a new patch series.

Best
Sven

> 
> > ---
> >  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);
> 
> This bit + the but making the function non static and exporting
> it needs to go into its own separate patch.
> 
> > 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);
> > +}
> 
> This is more or less a 1:1 copy of ahci_host_activate_multi_irqs(). Please don't go
> around and make copies of functions (also see below).
> 
> The only difference seems to be the use of ahci_multi_irqs_intr_hard_armada8k
> where as ahci_host_activate_multi_irqs() uses ahci_multi_irqs_intr_hard as IRQ handler.
> 
> And ahci_multi_irqs_intr_hard_armada8k itself is a copy of
> ahci_multi_irqs_intr_hard() with a small change
> 
> No, just no all this function copying is not ok, hard NACK!
> 
> Instead I suggest that you add an "multi_irq_host_ack" callback to some
> shared data struct, which will then get called by ahci_multi_irqs_intr_hard()
> (when defined) in the place where you have added the following line:
> 
> 	writel(BIT(ap->port_no), mmio + HOST_IRQ_STAT);
> 
> To your private copy of ahci_multi_irqs_intr_hard(), then you can drop
> both the ahci_multi_irqs_intr_hard() and the ahci_host_activate_multi_irqs()
> copies and reduce the size of this patch by a lot of lines, again also see
> below for dropping even more lines!
> 
> > +
> > +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);
> > +}
> > +
> 
> So this is basically a copy of ahci_platform_init_host() with:
> 
> 1. This bit:
> 
>         irq = platform_get_irq(pdev, 0);
>         if (irq <= 0) {
>                 if (irq != -EPROBE_DEFER)
>                         dev_err(dev, "no irq\n");
>                 return irq;
>         }
> 
>         hpriv->irq = irq;
> 
> Replaced with:
> 
> > +	/* 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;
> 
> You can just do this new bit, before calling ahci_platform_init_host()
> and then disable the bit of ahci_platform_init_host() which you do not
> want by adding an if around it like this:
> 
> 	if (!(hpriv->flags & AHCI_HFLAG_MULTI_MSI)) {
> 	        irq = platform_get_irq(pdev, 0);
>         	if (irq <= 0) {
>                 	if (irq != -EPROBE_DEFER)
>                         	dev_err(dev, "no irq\n");
> 	                return irq;
>         	}
> 	        hpriv->irq = irq;
> 	}
> 
> Note please add this new if() check in a separate patch.
> 
> The only other change to ahci_platform_init_host() here is
> the function ending with:
> 
> 	return ahci_host_activate_multi_irqs_armada8k(host, sht);
> 
> Instead of with:
> 
>        return ahci_host_activate(host, sht);
> 
> As mentioned above you really do NOT need that function, which
> is yet another copy. Instead make the changes which I suggested
> above and set the AHCI_HFLAG_MULTI_MSI flag to make
> ahci_host_activate() do what you want.
> 
> After this you can drop all 3 private functions of pre-existing
> ahci functions AFAICT.
> 
> >  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);
> 
> And this change can go away then too...
> 
> >  	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)
> >  {
> 
> As already mentioned this needs to go to a separate patch.
> 
> > 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 */
> > 
> 
> These last 2 changes need to go into a separate patch and please rename the quirk to
> AHCI_PLATFORM_ARMADA8K_QUIRK to make it clear that it is for armada SoCs.
> 
> Regards,
> 
> Hans
> 

[PATCH 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.20.1

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

[Hans de Goede]

Hi,

On 10/7/20 5:45 PM, sven.auhagen@voleatech.de wrote:
> 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>

Patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> ---
>  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 = {
> 

[PATCH 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.20.1

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

[Hans de Goede]

Hi,

On 10/7/20 5:45 PM, sven.auhagen@voleatech.de wrote:
> 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>

Patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> ---
>  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";
> 

[PATCH 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.20.1

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

[Hans de Goede]

Hi,

On 10/7/20 5:45 PM, sven.auhagen@voleatech.de wrote:
> 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>

Patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> ---
>  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"
> 

[PATCH 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(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 @@
 			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.20.1

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

[Hans de Goede]

Hi,

On 10/7/20 5:45 PM, sven.auhagen@voleatech.de wrote:
> 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>

Patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> ---
>  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(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 @@
>  			status = "disabled";
>  
>  			sata-port@0 {
> +				interrupts = <109 IRQ_TYPE_LEVEL_HIGH>;
>  				reg = <0>;
>  			};
>  
>  			sata-port@1 {
> +				interrupts = <107 IRQ_TYPE_LEVEL_HIGH>;
>  				reg = <1>;
>  			};
>  		};
>