[PATCH v2 00/10] Enable per-port SATA interrupts and drop an hack in the IRQ subsystem

[PATCH v2 00/10] 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).

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


Miquel Raynal (9):
  ata: libahci: Ensure the host interrupt status bits are cleared
  ata: ahci: Support per-port interrupts
  dt-bindings: ata: Update ahci bindings with possible per-port
    interrupts
  ata: ahci: mvebu: Rename a platform data flag
  ata: ahci: mvebu: Add a parameter to a platform data callback
  dt-bindings: ata: Update ahci_mvebu bindings
  ata: ahci: mvebu: Support A8k compatible
  ata: ahci: mvebu: Add support for A8k legacy bindings
  irqchip/irq-mvebu-icu: Remove the double SATA ports interrupt hack

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

 .../devicetree/bindings/ata/ahci-platform.txt |  7 ++
 .../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                            |  8 ++-
 drivers/ata/ahci.h                            |  3 +-
 drivers/ata/ahci_mvebu.c                      | 61 +++++++++++++---
 drivers/ata/libahci.c                         |  9 ++-
 drivers/ata/libahci_platform.c                | 70 ++++++++++++++++---
 drivers/ata/sata_highbank.c                   |  2 +-
 drivers/irqchip/irq-mvebu-icu.c               | 18 -----
 include/linux/ahci_platform.h                 |  1 +
 14 files changed, 174 insertions(+), 49 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 v2 01/10] 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 v2 01/10] 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


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

Re: [PATCH v2 01/10] ata: libahci: Ensure the host interrupt status bits are cleared

[Marc Zyngier]

On 06/03/2019 10:21, Miquel Raynal wrote:
> 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);

What's not clear here is under which circumstances this is required.
This write should be atomic (if it isn't, you have bigger problems), and
it is at best unclear what you're avoiding by taking the host lock.

Thanks,

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

Re: [PATCH v2 01/10] ata: libahci: Ensure the host interrupt status bits are cleared

[Marc Zyngier]

On 06/03/2019 10:21, Miquel Raynal wrote:
> 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);

What's not clear here is under which circumstances this is required.
This write should be atomic (if it isn't, you have bigger problems), and
it is at best unclear what you're avoiding by taking the host lock.

Thanks,

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

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

Re: [PATCH v2 01/10] ata: libahci: Ensure the host interrupt status bits are cleared

[Miquel Raynal]

Hi Marc,

Marc Zyngier <marc.zyngier@arm.com> wrote on Thu, 7 Mar 2019 16:25:02
+0000:

> On 06/03/2019 10:21, Miquel Raynal wrote:
> > 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);  
> 
> What's not clear here is under which circumstances this is required.
> This write should be atomic (if it isn't, you have bigger problems), and
> it is at best unclear what you're avoiding by taking the host lock.

You're right, taking the lock is not needed. It is a relict of a
previous implementation anyway.


Thanks,
Miquèl

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

[PATCH v2 02/10] ata: ahci: Support per-port interrupts

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             |  8 +++-
 drivers/ata/ahci.h             |  3 +-
 drivers/ata/libahci.c          |  2 +-
 drivers/ata/libahci_platform.c | 67 ++++++++++++++++++++++++++++------
 drivers/ata/sata_highbank.c    |  2 +-
 6 files changed, 68 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..bc37a34fa043 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1817,7 +1817,13 @@ 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 = devm_kzalloc(dev, sizeof(*hpriv->irqs) * n_ports,
+				   GFP_KERNEL);
+	if (!hpriv->irqs)
+		return -ENOMEM;
+
+	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 8810475f307a..f569f6a0d9f5 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -363,7 +363,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
@@ -434,6 +434,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 81b1a3332ed6..673d355a59ab 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"
@@ -95,6 +96,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
@@ -385,6 +394,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
@@ -396,7 +406,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))
@@ -489,10 +499,30 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 		goto err_out;
 	}
 
+	hpriv->irqs = devm_kzalloc(dev, 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;
@@ -521,6 +551,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;
@@ -548,6 +590,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;
@@ -584,16 +638,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 v2 02/10] ata: ahci: 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             |  8 +++-
 drivers/ata/ahci.h             |  3 +-
 drivers/ata/libahci.c          |  2 +-
 drivers/ata/libahci_platform.c | 67 ++++++++++++++++++++++++++++------
 drivers/ata/sata_highbank.c    |  2 +-
 6 files changed, 68 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..bc37a34fa043 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1817,7 +1817,13 @@ 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 = devm_kzalloc(dev, sizeof(*hpriv->irqs) * n_ports,
+				   GFP_KERNEL);
+	if (!hpriv->irqs)
+		return -ENOMEM;
+
+	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 8810475f307a..f569f6a0d9f5 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -363,7 +363,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
@@ -434,6 +434,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 81b1a3332ed6..673d355a59ab 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"
@@ -95,6 +96,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
@@ -385,6 +394,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
@@ -396,7 +406,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))
@@ -489,10 +499,30 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
 		goto err_out;
 	}
 
+	hpriv->irqs = devm_kzalloc(dev, 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;
@@ -521,6 +551,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;
@@ -548,6 +590,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;
@@ -584,16 +638,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


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

Re: [PATCH v2 02/10] ata: ahci: Support per-port interrupts

[Hans de Goede]

Hi,

On 06-03-19 11:21, 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             |  8 +++-
>   drivers/ata/ahci.h             |  3 +-
>   drivers/ata/libahci.c          |  2 +-
>   drivers/ata/libahci_platform.c | 67 ++++++++++++++++++++++++++++------
>   drivers/ata/sata_highbank.c    |  2 +-
>   6 files changed, 68 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..bc37a34fa043 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1817,7 +1817,13 @@ 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 = devm_kzalloc(dev, sizeof(*hpriv->irqs) * n_ports,
> +				   GFP_KERNEL);
> +	if (!hpriv->irqs)
> +		return -ENOMEM;
> +
> +	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 8810475f307a..f569f6a0d9f5 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -363,7 +363,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
> @@ -434,6 +434,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 81b1a3332ed6..673d355a59ab 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"
> @@ -95,6 +96,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
> @@ -385,6 +394,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
> @@ -396,7 +406,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))
> @@ -489,10 +499,30 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>   		goto err_out;
>   	}
>   
> +	hpriv->irqs = devm_kzalloc(dev, 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;
> @@ -521,6 +551,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;
> @@ -548,6 +590,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;
> +	}
> +

I believe that the "if (enabled_ports > 1)" here needs to be:

	if (!ctrl_irq && enabled_ports > 1) {

Otherwise existing boards which use a single irq defined at the
main of_node level for the device and have defined more then 1
child-node in their DTB will now get AHCI_HFLAG_MULTI_MSI set,
but they will only have hpriv->irqs[0] set to the ctrl_irq
leading to returning of 0 as irq from ahci_get_per_port_irq_vector
for the other ports, which is wrong.

Otherwise this patch looks good to me, so with this fixed
(assuming you agree) you may add my:

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

to the next version of this patch.

The rest of the patches in this series are outside my area
of expertise.

Regards.

Hans




>   	pm_runtime_enable(dev);
>   	pm_runtime_get_sync(dev);
>   	hpriv->got_runtime_pm = true;
> @@ -584,16 +638,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 v2 02/10] ata: ahci: Support per-port interrupts

[Hans de Goede]

Hi,

On 06-03-19 11:21, 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             |  8 +++-
>   drivers/ata/ahci.h             |  3 +-
>   drivers/ata/libahci.c          |  2 +-
>   drivers/ata/libahci_platform.c | 67 ++++++++++++++++++++++++++++------
>   drivers/ata/sata_highbank.c    |  2 +-
>   6 files changed, 68 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..bc37a34fa043 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1817,7 +1817,13 @@ 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 = devm_kzalloc(dev, sizeof(*hpriv->irqs) * n_ports,
> +				   GFP_KERNEL);
> +	if (!hpriv->irqs)
> +		return -ENOMEM;
> +
> +	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 8810475f307a..f569f6a0d9f5 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -363,7 +363,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
> @@ -434,6 +434,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 81b1a3332ed6..673d355a59ab 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"
> @@ -95,6 +96,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
> @@ -385,6 +394,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
> @@ -396,7 +406,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))
> @@ -489,10 +499,30 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
>   		goto err_out;
>   	}
>   
> +	hpriv->irqs = devm_kzalloc(dev, 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;
> @@ -521,6 +551,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;
> @@ -548,6 +590,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;
> +	}
> +

I believe that the "if (enabled_ports > 1)" here needs to be:

	if (!ctrl_irq && enabled_ports > 1) {

Otherwise existing boards which use a single irq defined at the
main of_node level for the device and have defined more then 1
child-node in their DTB will now get AHCI_HFLAG_MULTI_MSI set,
but they will only have hpriv->irqs[0] set to the ctrl_irq
leading to returning of 0 as irq from ahci_get_per_port_irq_vector
for the other ports, which is wrong.

Otherwise this patch looks good to me, so with this fixed
(assuming you agree) you may add my:

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

to the next version of this patch.

The rest of the patches in this series are outside my area
of expertise.

Regards.

Hans




>   	pm_runtime_enable(dev);
>   	pm_runtime_get_sync(dev);
>   	hpriv->got_runtime_pm = true;
> @@ -584,16 +638,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));
> 

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

Re: [PATCH v2 02/10] ata: ahci: Support per-port interrupts

[Miquel Raynal]

Hi Hans,

Hans de Goede <hdegoede@redhat.com> wrote on Wed, 6 Mar 2019 16:01:16
+0100:

> >   /**
> >    * ahci_platform_enable_clks - Enable platform clocks
> >    * @hpriv: host private area to store config values
> > @@ -385,6 +394,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
> > @@ -396,7 +406,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))  
> > @@ -489,10 +499,30 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev,
> >   		goto err_out;
> >   	}  
> >   > +	hpriv->irqs = devm_kzalloc(dev, 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;
> > @@ -521,6 +551,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;
> > @@ -548,6 +590,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;
> > +	}
> > +  
> 
> I believe that the "if (enabled_ports > 1)" here needs to be:
> 
> 	if (!ctrl_irq && enabled_ports > 1) {
> 
> Otherwise existing boards which use a single irq defined at the
> main of_node level for the device and have defined more then 1
> child-node in their DTB will now get AHCI_HFLAG_MULTI_MSI set,
> but they will only have hpriv->irqs[0] set to the ctrl_irq
> leading to returning of 0 as irq from ahci_get_per_port_irq_vector
> for the other ports, which is wrong.
> 
> Otherwise this patch looks good to me, so with this fixed
> (assuming you agree) you may add my:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> 
> to the next version of this patch.

You are completely right, this is a mistake, I will fix it in v3. I
will just wait for the review of the other ahci_mvebu patches.


Thanks,
Miquèl

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

[PATCH v2 03/10] dt-bindings: ata: Update ahci bindings with possible per-port interrupts

[Miquel Raynal]

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>
---
 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 e30fd106df4f..80261e2845b0 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -53,6 +53,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.19.1

[PATCH v2 03/10] dt-bindings: ata: Update ahci bindings with possible per-port interrupts

[Miquel Raynal]

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>
---
 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 e30fd106df4f..80261e2845b0 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -53,6 +53,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.19.1


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

Re: [PATCH v2 03/10] dt-bindings: ata: Update ahci bindings with possible per-port interrupts

[Rob Herring]

On Wed,  6 Mar 2019 11:21:39 +0100, Miquel Raynal wrote:
> 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>
> ---
>  Documentation/devicetree/bindings/ata/ahci-platform.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 03/10] dt-bindings: ata: Update ahci bindings with possible per-port interrupts

[Rob Herring]

On Wed,  6 Mar 2019 11:21:39 +0100, Miquel Raynal wrote:
> 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>
> ---
>  Documentation/devicetree/bindings/ata/ahci-platform.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

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

[PATCH v2 04/10] ata: ahci: mvebu: Rename a platform data flag

[Miquel Raynal]

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

[PATCH v2 04/10] ata: ahci: mvebu: Rename a platform data flag

[Miquel Raynal]

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


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

[PATCH v2 05/10] ata: ahci: mvebu: Add a parameter to a platform data callback

[Miquel Raynal]

Before using the ahci_mvebu.c driver with Armada 8k hardware (right
now it is using the ahci_platform.c generic driver), let's add a
'struct device' pointer to the argument list of the
->plat_config() callback. This parameter will be used by the A8k's
callback.

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

diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
index 43bb2db59698..507ee7c5437c 100644
--- a/drivers/ata/ahci_mvebu.c
+++ b/drivers/ata/ahci_mvebu.c
@@ -29,7 +29,7 @@
 #define AHCI_WINDOW_SIZE(win)	(0x68 + ((win) << 4))
 
 struct ahci_mvebu_plat_data {
-	int (*plat_config)(struct ahci_host_priv *hpriv);
+	int (*plat_config)(struct ahci_host_priv *hpriv, struct device *dev);
 	unsigned int host_flags;
 };
 
@@ -67,7 +67,8 @@ static void ahci_mvebu_regret_option(struct ahci_host_priv *hpriv)
 	writel(0x80, hpriv->mmio + AHCI_VENDOR_SPECIFIC_0_DATA);
 }
 
-static int ahci_mvebu_armada_380_config(struct ahci_host_priv *hpriv)
+static int ahci_mvebu_armada_380_config(struct ahci_host_priv *hpriv,
+					struct device *dev)
 {
 	const struct mbus_dram_target_info *dram;
 	int rc = 0;
@@ -83,7 +84,8 @@ static int ahci_mvebu_armada_380_config(struct ahci_host_priv *hpriv)
 	return rc;
 }
 
-static int ahci_mvebu_armada_3700_config(struct ahci_host_priv *hpriv)
+static int ahci_mvebu_armada_3700_config(struct ahci_host_priv *hpriv,
+					 struct device *dev)
 {
 	u32 reg;
 
@@ -162,7 +164,7 @@ static int ahci_mvebu_resume(struct platform_device *pdev)
 	struct ahci_host_priv *hpriv = host->private_data;
 	const struct ahci_mvebu_plat_data *pdata = hpriv->plat_data;
 
-	pdata->plat_config(hpriv);
+	pdata->plat_config(hpriv, &pdev->dev);
 
 	return ahci_platform_resume_host(&pdev->dev);
 }
@@ -205,7 +207,7 @@ static int ahci_mvebu_probe(struct platform_device *pdev)
 
 	hpriv->stop_engine = ahci_mvebu_stop_engine;
 
-	rc = pdata->plat_config(hpriv);
+	rc = pdata->plat_config(hpriv, &pdev->dev);
 	if (rc)
 		goto disable_resources;
 
-- 
2.19.1

[PATCH v2 05/10] ata: ahci: mvebu: Add a parameter to a platform data callback

[Miquel Raynal]

Before using the ahci_mvebu.c driver with Armada 8k hardware (right
now it is using the ahci_platform.c generic driver), let's add a
'struct device' pointer to the argument list of the
->plat_config() callback. This parameter will be used by the A8k's
callback.

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

diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
index 43bb2db59698..507ee7c5437c 100644
--- a/drivers/ata/ahci_mvebu.c
+++ b/drivers/ata/ahci_mvebu.c
@@ -29,7 +29,7 @@
 #define AHCI_WINDOW_SIZE(win)	(0x68 + ((win) << 4))
 
 struct ahci_mvebu_plat_data {
-	int (*plat_config)(struct ahci_host_priv *hpriv);
+	int (*plat_config)(struct ahci_host_priv *hpriv, struct device *dev);
 	unsigned int host_flags;
 };
 
@@ -67,7 +67,8 @@ static void ahci_mvebu_regret_option(struct ahci_host_priv *hpriv)
 	writel(0x80, hpriv->mmio + AHCI_VENDOR_SPECIFIC_0_DATA);
 }
 
-static int ahci_mvebu_armada_380_config(struct ahci_host_priv *hpriv)
+static int ahci_mvebu_armada_380_config(struct ahci_host_priv *hpriv,
+					struct device *dev)
 {
 	const struct mbus_dram_target_info *dram;
 	int rc = 0;
@@ -83,7 +84,8 @@ static int ahci_mvebu_armada_380_config(struct ahci_host_priv *hpriv)
 	return rc;
 }
 
-static int ahci_mvebu_armada_3700_config(struct ahci_host_priv *hpriv)
+static int ahci_mvebu_armada_3700_config(struct ahci_host_priv *hpriv,
+					 struct device *dev)
 {
 	u32 reg;
 
@@ -162,7 +164,7 @@ static int ahci_mvebu_resume(struct platform_device *pdev)
 	struct ahci_host_priv *hpriv = host->private_data;
 	const struct ahci_mvebu_plat_data *pdata = hpriv->plat_data;
 
-	pdata->plat_config(hpriv);
+	pdata->plat_config(hpriv, &pdev->dev);
 
 	return ahci_platform_resume_host(&pdev->dev);
 }
@@ -205,7 +207,7 @@ static int ahci_mvebu_probe(struct platform_device *pdev)
 
 	hpriv->stop_engine = ahci_mvebu_stop_engine;
 
-	rc = pdata->plat_config(hpriv);
+	rc = pdata->plat_config(hpriv, &pdev->dev);
 	if (rc)
 		goto disable_resources;
 
-- 
2.19.1


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

[PATCH v2 06/10] dt-bindings: ata: Update ahci_mvebu bindings

[Miquel Raynal]

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

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 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 80261e2845b0..a1e6a3a27ee3 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -17,6 +17,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.19.1

[PATCH v2 06/10] dt-bindings: ata: Update ahci_mvebu bindings

[Miquel Raynal]

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

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 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 80261e2845b0..a1e6a3a27ee3 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -17,6 +17,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.19.1


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

Re: [PATCH v2 06/10] dt-bindings: ata: Update ahci_mvebu bindings

[Rob Herring]

On Wed,  6 Mar 2019 11:21:42 +0100, Miquel Raynal wrote:
> Update bindings with the already in use Armada 8k compatible.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  Documentation/devicetree/bindings/ata/ahci-platform.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 06/10] dt-bindings: ata: Update ahci_mvebu bindings

[Rob Herring]

On Wed,  6 Mar 2019 11:21:42 +0100, Miquel Raynal wrote:
> Update bindings with the already in use Armada 8k compatible.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  Documentation/devicetree/bindings/ata/ahci-platform.txt | 1 +
>  1 file changed, 1 insertion(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

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

[PATCH v2 07/10] ata: ahci: mvebu: Support A8k compatible

[Miquel Raynal]

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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
index 507ee7c5437c..8671aa8179fa 100644
--- a/drivers/ata/ahci_mvebu.c
+++ b/drivers/ata/ahci_mvebu.c
@@ -98,6 +98,12 @@ 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,
+				       struct device *dev)
+{
+	return 0;
+}
+
 /**
  * ahci_mvebu_stop_engine
  *
@@ -232,6 +238,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",
@@ -241,6 +251,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.19.1

[PATCH v2 07/10] ata: ahci: mvebu: Support A8k compatible

[Miquel Raynal]

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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
index 507ee7c5437c..8671aa8179fa 100644
--- a/drivers/ata/ahci_mvebu.c
+++ b/drivers/ata/ahci_mvebu.c
@@ -98,6 +98,12 @@ 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,
+				       struct device *dev)
+{
+	return 0;
+}
+
 /**
  * ahci_mvebu_stop_engine
  *
@@ -232,6 +238,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",
@@ -241,6 +251,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.19.1


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

[PATCH v2 08/10] ata: ahci: mvebu: Add support for A8k legacy bindings

[Miquel Raynal]

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 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: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/ata/ahci_mvebu.c       | 29 ++++++++++++++++++++++++++++-
 drivers/ata/libahci_platform.c |  3 +++
 include/linux/ahci_platform.h  |  1 +
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
index 8671aa8179fa..8c981b8d6e4d 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,13 @@
 #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, struct device *dev);
 	unsigned int host_flags;
+	unsigned int resource_flags;
 };
 
 static void ahci_mvebu_mbus_config(struct ahci_host_priv *hpriv,
@@ -101,6 +106,27 @@ static int ahci_mvebu_armada_3700_config(struct ahci_host_priv *hpriv,
 static int ahci_mvebu_armada_8k_config(struct ahci_host_priv *hpriv,
 				       struct device *dev)
 {
+	struct device_node *np = of_irq_find_parent(dev->of_node);
+	struct irq_data *irqd = irq_get_irq_data(hpriv->irqs[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;
+
+	hpriv->irqs[1] = irq_create_fwspec_mapping(&fwspec);
+	if (hpriv->irqs[1]) {
+		hpriv->flags |= AHCI_HFLAG_MULTI_MSI;
+		hpriv->get_irq_vector = ahci_get_per_port_irq_vector;
+		hpriv->mask_port_map = GENMASK(1, 0);
+	}
+
 	return 0;
 }
 
@@ -200,7 +226,7 @@ static int ahci_mvebu_probe(struct platform_device *pdev)
 	if (!pdata)
 		return -EINVAL;
 
-	hpriv = ahci_platform_get_resources(pdev, 0);
+	hpriv = ahci_platform_get_resources(pdev, pdata->resource_flags);
 	if (IS_ERR(hpriv))
 		return PTR_ERR(hpriv);
 
@@ -240,6 +266,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_platform.c b/drivers/ata/libahci_platform.c
index 673d355a59ab..7fb7bd590b2b 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -484,6 +484,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 eaedca5fe6fc..57465ba6bb15 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -44,5 +44,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.19.1

[PATCH v2 08/10] ata: ahci: mvebu: Add support for A8k legacy bindings

[Miquel Raynal]

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 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: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/ata/ahci_mvebu.c       | 29 ++++++++++++++++++++++++++++-
 drivers/ata/libahci_platform.c |  3 +++
 include/linux/ahci_platform.h  |  1 +
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/ahci_mvebu.c b/drivers/ata/ahci_mvebu.c
index 8671aa8179fa..8c981b8d6e4d 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,13 @@
 #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, struct device *dev);
 	unsigned int host_flags;
+	unsigned int resource_flags;
 };
 
 static void ahci_mvebu_mbus_config(struct ahci_host_priv *hpriv,
@@ -101,6 +106,27 @@ static int ahci_mvebu_armada_3700_config(struct ahci_host_priv *hpriv,
 static int ahci_mvebu_armada_8k_config(struct ahci_host_priv *hpriv,
 				       struct device *dev)
 {
+	struct device_node *np = of_irq_find_parent(dev->of_node);
+	struct irq_data *irqd = irq_get_irq_data(hpriv->irqs[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;
+
+	hpriv->irqs[1] = irq_create_fwspec_mapping(&fwspec);
+	if (hpriv->irqs[1]) {
+		hpriv->flags |= AHCI_HFLAG_MULTI_MSI;
+		hpriv->get_irq_vector = ahci_get_per_port_irq_vector;
+		hpriv->mask_port_map = GENMASK(1, 0);
+	}
+
 	return 0;
 }
 
@@ -200,7 +226,7 @@ static int ahci_mvebu_probe(struct platform_device *pdev)
 	if (!pdata)
 		return -EINVAL;
 
-	hpriv = ahci_platform_get_resources(pdev, 0);
+	hpriv = ahci_platform_get_resources(pdev, pdata->resource_flags);
 	if (IS_ERR(hpriv))
 		return PTR_ERR(hpriv);
 
@@ -240,6 +266,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_platform.c b/drivers/ata/libahci_platform.c
index 673d355a59ab..7fb7bd590b2b 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -484,6 +484,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 eaedca5fe6fc..57465ba6bb15 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -44,5 +44,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.19.1


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

Re: [PATCH v2 08/10] ata: ahci: mvebu: Add support for A8k legacy bindings

[Marc Zyngier]

On 06/03/2019 10:21, Miquel Raynal wrote:
> 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 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: Miquel Raynal <miquel.raynal@bootlin.com>

It'd be good to add that all these hacks only exist for the purpose of
DT. The same HW booting with ACPI doesn't require any of this because
the firmware abstracts stuff that the kernel shouldn't be concerned with
the first place.

Thanks,

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

Re: [PATCH v2 08/10] ata: ahci: mvebu: Add support for A8k legacy bindings

[Marc Zyngier]

On 06/03/2019 10:21, Miquel Raynal wrote:
> 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 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: Miquel Raynal <miquel.raynal@bootlin.com>

It'd be good to add that all these hacks only exist for the purpose of
DT. The same HW booting with ACPI doesn't require any of this because
the firmware abstracts stuff that the kernel shouldn't be concerned with
the first place.

Thanks,

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

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

[PATCH v2 09/10] irqchip/irq-mvebu-icu: Remove 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 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>
---
 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 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 v2 09/10] irqchip/irq-mvebu-icu: Remove 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 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>
---
 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 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


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

[PATCH v2 10/10] 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 ++++++-
 .../dts/marvell/armada-8040-clearfog-gt-8k.dts   |  5 ++++-
 arch/arm64/boot/dts/marvell/armada-8040-db.dts   | 14 ++++++++++++--
 arch/arm64/boot/dts/marvell/armada-cp110.dtsi    | 16 ++++++++++++++--
 4 files changed, 36 insertions(+), 6 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-clearfog-gt-8k.dts b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
index 5b4a9609e31f..16fc76ef7804 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
@@ -335,7 +335,10 @@
 
 &cp1_sata0 {
 	pinctrl-0 = <&cp0_pci1_reset_pins>;
-	status = "okay";
+
+	sata-port@1 {
+		status = "okay";
+	};
 };
 
 &cp1_mdio {
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

[PATCH v2 10/10] 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 ++++++-
 .../dts/marvell/armada-8040-clearfog-gt-8k.dts   |  5 ++++-
 arch/arm64/boot/dts/marvell/armada-8040-db.dts   | 14 ++++++++++++--
 arch/arm64/boot/dts/marvell/armada-cp110.dtsi    | 16 ++++++++++++++--
 4 files changed, 36 insertions(+), 6 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-clearfog-gt-8k.dts b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
index 5b4a9609e31f..16fc76ef7804 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
@@ -335,7 +335,10 @@
 
 &cp1_sata0 {
 	pinctrl-0 = <&cp0_pci1_reset_pins>;
-	status = "okay";
+
+	sata-port@1 {
+		status = "okay";
+	};
 };
 
 &cp1_mdio {
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

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

[Baruch Siach]

Hi Miquel,

On Wed, Mar 06, 2019 at 11:21:46AM +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 ++++++-
>  .../dts/marvell/armada-8040-clearfog-gt-8k.dts   |  5 ++++-
>  arch/arm64/boot/dts/marvell/armada-8040-db.dts   | 14 ++++++++++++--
>  arch/arm64/boot/dts/marvell/armada-cp110.dtsi    | 16 ++++++++++++++--

What about armada-8040-mcbin.dtsi?

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 v2 10/10] arm64: dts: marvell: armada-cp110: Switch to per-port SATA interrupts

[Baruch Siach]

Hi Miquel,

On Wed, Mar 06, 2019 at 11:21:46AM +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 ++++++-
>  .../dts/marvell/armada-8040-clearfog-gt-8k.dts   |  5 ++++-
>  arch/arm64/boot/dts/marvell/armada-8040-db.dts   | 14 ++++++++++++--
>  arch/arm64/boot/dts/marvell/armada-cp110.dtsi    | 16 ++++++++++++++--

What about armada-8040-mcbin.dtsi?

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 -

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

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

[Miquel Raynal]

Hi Baruch,

Baruch Siach <baruch@tkos.co.il> wrote on Wed, 6 Mar 2019 12:30:39
+0200:

> Hi Miquel,
> 
> On Wed, Mar 06, 2019 at 11:21:46AM +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 ++++++-
> >  .../dts/marvell/armada-8040-clearfog-gt-8k.dts   |  5 ++++-
> >  arch/arm64/boot/dts/marvell/armada-8040-db.dts   | 14 ++++++++++++--
> >  arch/arm64/boot/dts/marvell/armada-cp110.dtsi    | 16 ++++++++++++++--  
> 
> What about armada-8040-mcbin.dtsi?
> 

I only checked final .dts files but indeed the SATA node is enabled in
the mcbin .dtsi. I will wait a bit for other reviews, if there is no
need to send a v3 for the SATA patches I will just resend the DT
patches for Gregory in a v3.


Thanks,
Miquèl

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