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

[Hans de Goede <hdegoede@xxxxxxxxxx>]

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@xxxxxxxxxxx>
> ---
>   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@xxxxxxxxxx>

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