[PATCH 0/4] Use new MSI infrastructure on Marvell EBU

[PATCH 0/4] Use new MSI infrastructure on Marvell EBU

[Thomas Petazzoni]

Hello,

IMPORTANT: do not apply the following set of patches now. They depend
on two irqdomain fixes from Marc Zyngier, which he hasn't submitted
yet. So this patch series is currently for review.

(In fact patches 1 and 4 and be applied now, but patches 2 and 3 will
have to wait until Marc submits his patches.)

For those interested, the two fixes needed are available at
https://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/irqdomain-fixes.

This patch series converts the Armada 370/XP irqchip driver (also used
on Armada 375, 38x and 39x) to the new MSI infrastructure put in place
by Marc. It doesn't bring any functional difference other than using
the right, modern, MSI mechanism, which might ultimately allow Marc to
remove the old way of implementing MSI support.

It has been tested on Armada 38x and Armada XP with an Intel e1000e
NIC which uses MSI.

Thanks,

Thomas

Thomas Petazzoni (4):
  irqchip: irq-armada-370-xp: add Kconfig option for the driver
  irqchip: irq-armada-370-xp: use the generic MSI infrastructure
  irqchip: irq-armada-370-xp: use shorter names for irq_chip
  ARM: mvebu: use the ARMADA_370_XP_IRQ option

 arch/arm/mach-mvebu/Kconfig         |   6 +-
 drivers/irqchip/Kconfig             |   6 ++
 drivers/irqchip/Makefile            |   2 +-
 drivers/irqchip/irq-armada-370-xp.c | 153 +++++++++++++++---------------------
 4 files changed, 77 insertions(+), 90 deletions(-)

-- 
2.6.4

[PATCH 1/4] irqchip: irq-armada-370-xp: add Kconfig option for the driver

[Thomas Petazzoni]

Instead of building the irq-armada-370-xp driver directly when
CONFIG_ARCH_MVEBU is enabled, this commit introduces an intermediate
CONFIG_ARMADA_370_XP_IRQ hidden Kconfig option.

This allows this option to select other interrupt-related Kconfig
options (which will be needed in follow-up commits) rather than having
such selects done from arch/arm/mach-<foo>/.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/irqchip/Kconfig  | 5 +++++
 drivers/irqchip/Makefile | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 4d7294e..ab672b0 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -47,6 +47,11 @@ config ARM_VIC_NR
 	  The maximum number of VICs available in the system, for
 	  power management.
 
+config ARMADA_370_XP_IRQ
+	bool
+	default y if ARCH_MVEBU
+	select GENERIC_IRQ_CHIP
+
 config ATMEL_AIC_IRQ
 	bool
 	select GENERIC_IRQ_CHIP
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 177f78f..4d740d8 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -5,7 +5,6 @@ obj-$(CONFIG_ARCH_BCM2835)		+= irq-bcm2836.o
 obj-$(CONFIG_ARCH_EXYNOS)		+= exynos-combiner.o
 obj-$(CONFIG_ARCH_HIP04)		+= irq-hip04.o
 obj-$(CONFIG_ARCH_MMP)			+= irq-mmp.o
-obj-$(CONFIG_ARCH_MVEBU)		+= irq-armada-370-xp.o
 obj-$(CONFIG_IRQ_MXS)			+= irq-mxs.o
 obj-$(CONFIG_ARCH_TEGRA)		+= irq-tegra.o
 obj-$(CONFIG_ARCH_S3C24XX)		+= irq-s3c24xx.o
@@ -26,6 +25,7 @@ obj-$(CONFIG_ARM_GIC_V3)		+= irq-gic-v3.o irq-gic-common.o
 obj-$(CONFIG_ARM_GIC_V3_ITS)		+= irq-gic-v3-its.o irq-gic-v3-its-pci-msi.o irq-gic-v3-its-platform-msi.o
 obj-$(CONFIG_ARM_NVIC)			+= irq-nvic.o
 obj-$(CONFIG_ARM_VIC)			+= irq-vic.o
+obj-$(CONFIG_ARMADA_370_XP_IRQ)		+= irq-armada-370-xp.o
 obj-$(CONFIG_ATMEL_AIC_IRQ)		+= irq-atmel-aic-common.o irq-atmel-aic.o
 obj-$(CONFIG_ATMEL_AIC5_IRQ)	+= irq-atmel-aic-common.o irq-atmel-aic5.o
 obj-$(CONFIG_I8259)			+= irq-i8259.o
-- 
2.6.4

[PATCH 1/4] irqchip: irq-armada-370-xp: add Kconfig option for the driver

[Gregory CLEMENT]

Hi Thomas,
 
 On lun., d?c. 21 2015, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

> Instead of building the irq-armada-370-xp driver directly when
> CONFIG_ARCH_MVEBU is enabled, this commit introduces an intermediate
> CONFIG_ARMADA_370_XP_IRQ hidden Kconfig option.
>
> This allows this option to select other interrupt-related Kconfig
> options (which will be needed in follow-up commits) rather than having
> such selects done from arch/arm/mach-<foo>/.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Thanks,

Gregory

> ---
>  drivers/irqchip/Kconfig  | 5 +++++
>  drivers/irqchip/Makefile | 2 +-
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 4d7294e..ab672b0 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -47,6 +47,11 @@ config ARM_VIC_NR
>  	  The maximum number of VICs available in the system, for
>  	  power management.
>  
> +config ARMADA_370_XP_IRQ
> +	bool
> +	default y if ARCH_MVEBU
> +	select GENERIC_IRQ_CHIP
> +
>  config ATMEL_AIC_IRQ
>  	bool
>  	select GENERIC_IRQ_CHIP
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 177f78f..4d740d8 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -5,7 +5,6 @@ obj-$(CONFIG_ARCH_BCM2835)		+= irq-bcm2836.o
>  obj-$(CONFIG_ARCH_EXYNOS)		+= exynos-combiner.o
>  obj-$(CONFIG_ARCH_HIP04)		+= irq-hip04.o
>  obj-$(CONFIG_ARCH_MMP)			+= irq-mmp.o
> -obj-$(CONFIG_ARCH_MVEBU)		+= irq-armada-370-xp.o
>  obj-$(CONFIG_IRQ_MXS)			+= irq-mxs.o
>  obj-$(CONFIG_ARCH_TEGRA)		+= irq-tegra.o
>  obj-$(CONFIG_ARCH_S3C24XX)		+= irq-s3c24xx.o
> @@ -26,6 +25,7 @@ obj-$(CONFIG_ARM_GIC_V3)		+= irq-gic-v3.o irq-gic-common.o
>  obj-$(CONFIG_ARM_GIC_V3_ITS)		+= irq-gic-v3-its.o irq-gic-v3-its-pci-msi.o irq-gic-v3-its-platform-msi.o
>  obj-$(CONFIG_ARM_NVIC)			+= irq-nvic.o
>  obj-$(CONFIG_ARM_VIC)			+= irq-vic.o
> +obj-$(CONFIG_ARMADA_370_XP_IRQ)		+= irq-armada-370-xp.o
>  obj-$(CONFIG_ATMEL_AIC_IRQ)		+= irq-atmel-aic-common.o irq-atmel-aic.o
>  obj-$(CONFIG_ATMEL_AIC5_IRQ)	+= irq-atmel-aic-common.o irq-atmel-aic5.o
>  obj-$(CONFIG_I8259)			+= irq-i8259.o
> -- 
> 2.6.4
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 2/4] irqchip: irq-armada-370-xp: use the generic MSI infrastructure

[Thomas Petazzoni]

This commit moves the irq-armada-370-xp driver from using the
PCI-specific MSI infrastructure to the generic MSI infrastructure, to
which drivers are progressively converted.

In this hardware, the MSI controller is directly bundled inside the
interrupt controller, so we have a single Device Tree node to which
multiple IRQ domaines are attached: the wired interrupt domain and the
MSI interrupt domain. In order to ensure that they can be
differentiated, we have to force the bus_token of the wired interrupt
domain to be DOMAIN_BUS_WIRED. The MSI domain bus_token is
automatically set to the appropriate value by
pci_msi_create_irq_domain().

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Suggested-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/irqchip/Kconfig             |   1 +
 drivers/irqchip/irq-armada-370-xp.c | 151 +++++++++++++++---------------------
 2 files changed, 65 insertions(+), 87 deletions(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index ab672b0..8ccb60e 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -51,6 +51,7 @@ config ARMADA_370_XP_IRQ
 	bool
 	default y if ARCH_MVEBU
 	select GENERIC_IRQ_CHIP
+	select PCI_MSI_IRQ_DOMAIN if PCI_MSI
 
 config ATMEL_AIC_IRQ
 	bool
diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 3f3a8c3..304166b 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -71,6 +71,7 @@ static u32 doorbell_mask_reg;
 static int parent_irq;
 #ifdef CONFIG_PCI_MSI
 static struct irq_domain *armada_370_xp_msi_domain;
+static struct irq_domain *armada_370_xp_msi_inner_domain;
 static DECLARE_BITMAP(msi_used, PCI_MSI_DOORBELL_NR);
 static DEFINE_MUTEX(msi_used_lock);
 static phys_addr_t msi_doorbell_addr;
@@ -115,127 +116,102 @@ static void armada_370_xp_irq_unmask(struct irq_data *d)
 
 #ifdef CONFIG_PCI_MSI
 
-static int armada_370_xp_alloc_msi(void)
-{
-	int hwirq;
-
-	mutex_lock(&msi_used_lock);
-	hwirq = find_first_zero_bit(&msi_used, PCI_MSI_DOORBELL_NR);
-	if (hwirq >= PCI_MSI_DOORBELL_NR)
-		hwirq = -ENOSPC;
-	else
-		set_bit(hwirq, msi_used);
-	mutex_unlock(&msi_used_lock);
+static struct irq_chip armada_370_xp_msi_irq_chip = {
+	.name = "armada_370_xp_msi_irq",
+	.irq_enable = pci_msi_unmask_irq,
+	.irq_disable = pci_msi_mask_irq,
+	.irq_mask = pci_msi_mask_irq,
+	.irq_unmask = pci_msi_unmask_irq,
+};
 
-	return hwirq;
-}
+static struct msi_domain_info armada_370_xp_msi_domain_info = {
+	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
+		   MSI_FLAG_MULTI_PCI_MSI),
+	.chip	= &armada_370_xp_msi_irq_chip,
+};
 
-static void armada_370_xp_free_msi(int hwirq)
+static void armada_370_xp_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 {
-	mutex_lock(&msi_used_lock);
-	if (!test_bit(hwirq, msi_used))
-		pr_err("trying to free unused MSI#%d\n", hwirq);
-	else
-		clear_bit(hwirq, msi_used);
-	mutex_unlock(&msi_used_lock);
+	msg->address_lo = lower_32_bits(msi_doorbell_addr);
+	msg->address_hi = upper_32_bits(msi_doorbell_addr);
+	msg->data = 0xf00 | (data->hwirq + PCI_MSI_DOORBELL_START);
 }
 
-static int armada_370_xp_setup_msi_irq(struct msi_controller *chip,
-				       struct pci_dev *pdev,
-				       struct msi_desc *desc)
+static int armada_370_xp_msi_set_affinity(struct irq_data *irq_data,
+					  const struct cpumask *mask, bool force)
 {
-	struct msi_msg msg;
-	int virq, hwirq;
+	 return -EINVAL;
+}
 
-	/* We support MSI, but not MSI-X */
-	if (desc->msi_attrib.is_msix)
-		return -EINVAL;
+static struct irq_chip armada_370_xp_msi_bottom_irq_chip = {
+	.name			= "MPIC MSI",
+	.irq_compose_msi_msg	= armada_370_xp_compose_msi_msg,
+	.irq_set_affinity	= armada_370_xp_msi_set_affinity,
+};
 
-	hwirq = armada_370_xp_alloc_msi();
-	if (hwirq < 0)
-		return hwirq;
+static int armada_370_xp_msi_alloc(struct irq_domain *domain, unsigned int virq,
+				   unsigned int nr_irqs, void *args)
+{
+	int hwirq;
 
-	virq = irq_create_mapping(armada_370_xp_msi_domain, hwirq);
-	if (!virq) {
-		armada_370_xp_free_msi(hwirq);
-		return -EINVAL;
+	mutex_lock(&msi_used_lock);
+	hwirq = find_first_zero_bit(&msi_used, PCI_MSI_DOORBELL_NR);
+	if (hwirq >= PCI_MSI_DOORBELL_NR) {
+		mutex_unlock(&msi_used_lock);
+		return -ENOSPC;
 	}
 
-	irq_set_msi_desc(virq, desc);
+	set_bit(hwirq, msi_used);
+	mutex_unlock(&msi_used_lock);
 
-	msg.address_lo = msi_doorbell_addr;
-	msg.address_hi = 0;
-	msg.data = 0xf00 | (hwirq + 16);
+	irq_domain_set_info(domain, virq, hwirq, &armada_370_xp_msi_bottom_irq_chip,
+			    domain->host_data, handle_simple_irq,
+			    NULL, NULL);
 
-	pci_write_msi_msg(virq, &msg);
-	return 0;
+	return hwirq;
 }
 
-static void armada_370_xp_teardown_msi_irq(struct msi_controller *chip,
-					   unsigned int irq)
+static void armada_370_xp_msi_free(struct irq_domain *domain,
+				   unsigned int virq, unsigned int nr_irqs)
 {
-	struct irq_data *d = irq_get_irq_data(irq);
-	unsigned long hwirq = d->hwirq;
+	struct irq_data *d = irq_domain_get_irq_data(domain, virq);
 
-	irq_dispose_mapping(irq);
-	armada_370_xp_free_msi(hwirq);
-}
-
-static struct irq_chip armada_370_xp_msi_irq_chip = {
-	.name = "armada_370_xp_msi_irq",
-	.irq_enable = pci_msi_unmask_irq,
-	.irq_disable = pci_msi_mask_irq,
-	.irq_mask = pci_msi_mask_irq,
-	.irq_unmask = pci_msi_unmask_irq,
-};
-
-static int armada_370_xp_msi_map(struct irq_domain *domain, unsigned int virq,
-				 irq_hw_number_t hw)
-{
-	irq_set_chip_and_handler(virq, &armada_370_xp_msi_irq_chip,
-				 handle_simple_irq);
-
-	return 0;
+	mutex_lock(&msi_used_lock);
+	if (!test_bit(d->hwirq, msi_used))
+		pr_err("trying to free unused MSI#%lu\n", d->hwirq);
+	else
+		clear_bit(d->hwirq, msi_used);
+	mutex_unlock(&msi_used_lock);
 }
 
-static const struct irq_domain_ops armada_370_xp_msi_irq_ops = {
-	.map = armada_370_xp_msi_map,
+static const struct irq_domain_ops armada_370_xp_msi_domain_ops = {
+	.alloc	= armada_370_xp_msi_alloc,
+	.free	= armada_370_xp_msi_free,
 };
 
 static int armada_370_xp_msi_init(struct device_node *node,
 				  phys_addr_t main_int_phys_base)
 {
-	struct msi_controller *msi_chip;
 	u32 reg;
-	int ret;
 
 	msi_doorbell_addr = main_int_phys_base +
 		ARMADA_370_XP_SW_TRIG_INT_OFFS;
 
-	msi_chip = kzalloc(sizeof(*msi_chip), GFP_KERNEL);
-	if (!msi_chip)
+	armada_370_xp_msi_inner_domain =
+		irq_domain_add_linear(NULL, PCI_MSI_DOORBELL_NR,
+				      &armada_370_xp_msi_domain_ops, NULL);
+	if (!armada_370_xp_msi_inner_domain)
 		return -ENOMEM;
 
-	msi_chip->setup_irq = armada_370_xp_setup_msi_irq;
-	msi_chip->teardown_irq = armada_370_xp_teardown_msi_irq;
-	msi_chip->of_node = node;
-
 	armada_370_xp_msi_domain =
-		irq_domain_add_linear(NULL, PCI_MSI_DOORBELL_NR,
-				      &armada_370_xp_msi_irq_ops,
-				      NULL);
+		pci_msi_create_irq_domain(of_node_to_fwnode(node),
+					  &armada_370_xp_msi_domain_info,
+					  armada_370_xp_msi_inner_domain);
 	if (!armada_370_xp_msi_domain) {
-		kfree(msi_chip);
+		irq_domain_remove(armada_370_xp_msi_inner_domain);
 		return -ENOMEM;
 	}
 
-	ret = of_pci_msi_chip_add(msi_chip);
-	if (ret < 0) {
-		irq_domain_remove(armada_370_xp_msi_domain);
-		kfree(msi_chip);
-		return ret;
-	}
-
 	reg = readl(per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_MSK_OFFS)
 		| PCI_MSI_DOORBELL_MASK;
 
@@ -427,12 +403,12 @@ static void armada_370_xp_handle_msi_irq(struct pt_regs *regs, bool is_chained)
 			continue;
 
 		if (is_chained) {
-			irq = irq_find_mapping(armada_370_xp_msi_domain,
+			irq = irq_find_mapping(armada_370_xp_msi_inner_domain,
 					       msinr - 16);
 			generic_handle_irq(irq);
 		} else {
 			irq = msinr - 16;
-			handle_domain_irq(armada_370_xp_msi_domain,
+			handle_domain_irq(armada_370_xp_msi_inner_domain,
 					  irq, regs);
 		}
 	}
@@ -604,6 +580,7 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,
 	armada_370_xp_mpic_domain =
 		irq_domain_add_linear(node, nr_irqs,
 				&armada_370_xp_mpic_irq_ops, NULL);
+	armada_370_xp_mpic_domain->bus_token = DOMAIN_BUS_WIRED;
 
 	BUG_ON(!armada_370_xp_mpic_domain);
 
-- 
2.6.4

[PATCH 2/4] irqchip: irq-armada-370-xp: use the generic MSI infrastructure

[Gregory CLEMENT]

Hi Thomas,
 
 On lun., d?c. 21 2015, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

> This commit moves the irq-armada-370-xp driver from using the
> PCI-specific MSI infrastructure to the generic MSI infrastructure, to
> which drivers are progressively converted.
>
> In this hardware, the MSI controller is directly bundled inside the
> interrupt controller, so we have a single Device Tree node to which
> multiple IRQ domaines are attached: the wired interrupt domain and the
> MSI interrupt domain. In order to ensure that they can be
> differentiated, we have to force the bus_token of the wired interrupt
> domain to be DOMAIN_BUS_WIRED. The MSI domain bus_token is
> automatically set to the appropriate value by
> pci_msi_create_irq_domain().

the way git generated the patch make it difficult to read, furthermore I
don't know too much about MSI. So for the bulk of this patch I trust you
and I let the irq experts comment if needed. I still have a minor remark,
see below:

>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Suggested-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  drivers/irqchip/Kconfig             |   1 +
>  drivers/irqchip/irq-armada-370-xp.c | 151 +++++++++++++++---------------------
>  2 files changed, 65 insertions(+), 87 deletions(-)
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index ab672b0..8ccb60e 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -51,6 +51,7 @@ config ARMADA_370_XP_IRQ
>  	bool
>  	default y if ARCH_MVEBU
>  	select GENERIC_IRQ_CHIP
> +	select PCI_MSI_IRQ_DOMAIN if PCI_MSI
>  
>  config ATMEL_AIC_IRQ
>  	bool
> diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
> index 3f3a8c3..304166b 100644
> --- a/drivers/irqchip/irq-armada-370-xp.c
> +++ b/drivers/irqchip/irq-armada-370-xp.c
> @@ -71,6 +71,7 @@ static u32 doorbell_mask_reg;
>  static int parent_irq;
>  #ifdef CONFIG_PCI_MSI
>  static struct irq_domain *armada_370_xp_msi_domain;
> +static struct irq_domain *armada_370_xp_msi_inner_domain;
>  static DECLARE_BITMAP(msi_used, PCI_MSI_DOORBELL_NR);
>  static DEFINE_MUTEX(msi_used_lock);
>  static phys_addr_t msi_doorbell_addr;
> @@ -115,127 +116,102 @@ static void armada_370_xp_irq_unmask(struct irq_data *d)
>  
>  #ifdef CONFIG_PCI_MSI
>  
> -static int armada_370_xp_alloc_msi(void)
> -{
> -	int hwirq;
> -
> -	mutex_lock(&msi_used_lock);
> -	hwirq = find_first_zero_bit(&msi_used, PCI_MSI_DOORBELL_NR);
> -	if (hwirq >= PCI_MSI_DOORBELL_NR)
> -		hwirq = -ENOSPC;
> -	else
> -		set_bit(hwirq, msi_used);
> -	mutex_unlock(&msi_used_lock);
> +static struct irq_chip armada_370_xp_msi_irq_chip = {
> +	.name = "armada_370_xp_msi_irq",
> +	.irq_enable = pci_msi_unmask_irq,
> +	.irq_disable = pci_msi_mask_irq,
> +	.irq_mask = pci_msi_mask_irq,
> +	.irq_unmask = pci_msi_unmask_irq,
> +};
>  
> -	return hwirq;
> -}
> +static struct msi_domain_info armada_370_xp_msi_domain_info = {
> +	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> +		   MSI_FLAG_MULTI_PCI_MSI),
> +	.chip	= &armada_370_xp_msi_irq_chip,
> +};
>  
> -static void armada_370_xp_free_msi(int hwirq)
> +static void armada_370_xp_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>  {
> -	mutex_lock(&msi_used_lock);
> -	if (!test_bit(hwirq, msi_used))
> -		pr_err("trying to free unused MSI#%d\n", hwirq);
> -	else
> -		clear_bit(hwirq, msi_used);
> -	mutex_unlock(&msi_used_lock);
> +	msg->address_lo = lower_32_bits(msi_doorbell_addr);
> +	msg->address_hi = upper_32_bits(msi_doorbell_addr);
> +	msg->data = 0xf00 | (data->hwirq + PCI_MSI_DOORBELL_START);

So here instead of using the 16 value you use PCI_MSI_DOORBELL_START
which seems better. What about using it also in
armada_370_xp_handle_msi_irq?

>  }
>  
> -static int armada_370_xp_setup_msi_irq(struct msi_controller *chip,
> -				       struct pci_dev *pdev,
> -				       struct msi_desc *desc)
> +static int armada_370_xp_msi_set_affinity(struct irq_data *irq_data,
> +					  const struct cpumask *mask, bool force)
>  {
> -	struct msi_msg msg;
> -	int virq, hwirq;
> +	 return -EINVAL;
> +}
>  
> -	/* We support MSI, but not MSI-X */
> -	if (desc->msi_attrib.is_msix)
> -		return -EINVAL;
> +static struct irq_chip armada_370_xp_msi_bottom_irq_chip = {
> +	.name			= "MPIC MSI",
> +	.irq_compose_msi_msg	= armada_370_xp_compose_msi_msg,
> +	.irq_set_affinity	= armada_370_xp_msi_set_affinity,
> +};
>  
> -	hwirq = armada_370_xp_alloc_msi();
> -	if (hwirq < 0)
> -		return hwirq;
> +static int armada_370_xp_msi_alloc(struct irq_domain *domain, unsigned int virq,
> +				   unsigned int nr_irqs, void *args)
> +{
> +	int hwirq;
>  
> -	virq = irq_create_mapping(armada_370_xp_msi_domain, hwirq);
> -	if (!virq) {
> -		armada_370_xp_free_msi(hwirq);
> -		return -EINVAL;
> +	mutex_lock(&msi_used_lock);
> +	hwirq = find_first_zero_bit(&msi_used, PCI_MSI_DOORBELL_NR);
> +	if (hwirq >= PCI_MSI_DOORBELL_NR) {
> +		mutex_unlock(&msi_used_lock);
> +		return -ENOSPC;
>  	}
>  
> -	irq_set_msi_desc(virq, desc);
> +	set_bit(hwirq, msi_used);
> +	mutex_unlock(&msi_used_lock);
>  
> -	msg.address_lo = msi_doorbell_addr;
> -	msg.address_hi = 0;
> -	msg.data = 0xf00 | (hwirq + 16);
> +	irq_domain_set_info(domain, virq, hwirq, &armada_370_xp_msi_bottom_irq_chip,
> +			    domain->host_data, handle_simple_irq,
> +			    NULL, NULL);
>  
> -	pci_write_msi_msg(virq, &msg);
> -	return 0;
> +	return hwirq;
>  }
>  
> -static void armada_370_xp_teardown_msi_irq(struct msi_controller *chip,
> -					   unsigned int irq)
> +static void armada_370_xp_msi_free(struct irq_domain *domain,
> +				   unsigned int virq, unsigned int nr_irqs)
>  {
> -	struct irq_data *d = irq_get_irq_data(irq);
> -	unsigned long hwirq = d->hwirq;
> +	struct irq_data *d = irq_domain_get_irq_data(domain, virq);
>  
> -	irq_dispose_mapping(irq);
> -	armada_370_xp_free_msi(hwirq);
> -}
> -
> -static struct irq_chip armada_370_xp_msi_irq_chip = {
> -	.name = "armada_370_xp_msi_irq",
> -	.irq_enable = pci_msi_unmask_irq,
> -	.irq_disable = pci_msi_mask_irq,
> -	.irq_mask = pci_msi_mask_irq,
> -	.irq_unmask = pci_msi_unmask_irq,
> -};
> -
> -static int armada_370_xp_msi_map(struct irq_domain *domain, unsigned int virq,
> -				 irq_hw_number_t hw)
> -{
> -	irq_set_chip_and_handler(virq, &armada_370_xp_msi_irq_chip,
> -				 handle_simple_irq);
> -
> -	return 0;
> +	mutex_lock(&msi_used_lock);
> +	if (!test_bit(d->hwirq, msi_used))
> +		pr_err("trying to free unused MSI#%lu\n", d->hwirq);
> +	else
> +		clear_bit(d->hwirq, msi_used);
> +	mutex_unlock(&msi_used_lock);
>  }
>  
> -static const struct irq_domain_ops armada_370_xp_msi_irq_ops = {
> -	.map = armada_370_xp_msi_map,
> +static const struct irq_domain_ops armada_370_xp_msi_domain_ops = {
> +	.alloc	= armada_370_xp_msi_alloc,
> +	.free	= armada_370_xp_msi_free,
>  };
>  
>  static int armada_370_xp_msi_init(struct device_node *node,
>  				  phys_addr_t main_int_phys_base)
>  {
> -	struct msi_controller *msi_chip;
>  	u32 reg;
> -	int ret;
>  
>  	msi_doorbell_addr = main_int_phys_base +
>  		ARMADA_370_XP_SW_TRIG_INT_OFFS;
>  
> -	msi_chip = kzalloc(sizeof(*msi_chip), GFP_KERNEL);
> -	if (!msi_chip)
> +	armada_370_xp_msi_inner_domain =
> +		irq_domain_add_linear(NULL, PCI_MSI_DOORBELL_NR,
> +				      &armada_370_xp_msi_domain_ops, NULL);
> +	if (!armada_370_xp_msi_inner_domain)
>  		return -ENOMEM;
>  
> -	msi_chip->setup_irq = armada_370_xp_setup_msi_irq;
> -	msi_chip->teardown_irq = armada_370_xp_teardown_msi_irq;
> -	msi_chip->of_node = node;
> -
>  	armada_370_xp_msi_domain =
> -		irq_domain_add_linear(NULL, PCI_MSI_DOORBELL_NR,
> -				      &armada_370_xp_msi_irq_ops,
> -				      NULL);
> +		pci_msi_create_irq_domain(of_node_to_fwnode(node),
> +					  &armada_370_xp_msi_domain_info,
> +					  armada_370_xp_msi_inner_domain);
>  	if (!armada_370_xp_msi_domain) {
> -		kfree(msi_chip);
> +		irq_domain_remove(armada_370_xp_msi_inner_domain);
>  		return -ENOMEM;
>  	}
>  
> -	ret = of_pci_msi_chip_add(msi_chip);
> -	if (ret < 0) {
> -		irq_domain_remove(armada_370_xp_msi_domain);
> -		kfree(msi_chip);
> -		return ret;
> -	}
> -
>  	reg = readl(per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_MSK_OFFS)
>  		| PCI_MSI_DOORBELL_MASK;
>  
> @@ -427,12 +403,12 @@ static void armada_370_xp_handle_msi_irq(struct pt_regs *regs, bool is_chained)
>  			continue;
>  
>  		if (is_chained) {
> -			irq = irq_find_mapping(armada_370_xp_msi_domain,
> +			irq = irq_find_mapping(armada_370_xp_msi_inner_domain,
>  					       msinr - 16);
Could we use also PCI_MSI_DOORBELL_START here?

>  			generic_handle_irq(irq);
>  		} else {
>  			irq = msinr - 16;
and here?

> -			handle_domain_irq(armada_370_xp_msi_domain,
> +			handle_domain_irq(armada_370_xp_msi_inner_domain,
>  					  irq, regs);
>  		}
>  	}
> @@ -604,6 +580,7 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,
>  	armada_370_xp_mpic_domain =
>  		irq_domain_add_linear(node, nr_irqs,
>  				&armada_370_xp_mpic_irq_ops, NULL);
> +	armada_370_xp_mpic_domain->bus_token = DOMAIN_BUS_WIRED;
>  
>  	BUG_ON(!armada_370_xp_mpic_domain);
>  
> -- 
> 2.6.4
>
Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 2/4] irqchip: irq-armada-370-xp: use the generic MSI infrastructure

[Thomas Petazzoni]

Gregory,

On Wed, 23 Dec 2015 12:37:43 +0100, Gregory CLEMENT wrote:

> > -	mutex_unlock(&msi_used_lock);
> > +	msg->address_lo = lower_32_bits(msi_doorbell_addr);
> > +	msg->address_hi = upper_32_bits(msi_doorbell_addr);
> > +	msg->data = 0xf00 | (data->hwirq + PCI_MSI_DOORBELL_START);
> 
> So here instead of using the 16 value you use PCI_MSI_DOORBELL_START
> which seems better. What about using it also in
> armada_370_xp_handle_msi_irq?

Good idea, I've fixed this. Since it's not related to the conversion to
the generic MSI infrastructure, I made this change as part of a
separate commit (this one is already quite difficult to read as a
patch, as you noted).

Thanks for the feedback!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 2/4] irqchip: irq-armada-370-xp: use the generic MSI infrastructure

[Marc Zyngier]

Hi Thomas,

I finally found some bandwidth to have a look at this. A few comments below:

On 21/12/15 14:19, Thomas Petazzoni wrote:
> This commit moves the irq-armada-370-xp driver from using the
> PCI-specific MSI infrastructure to the generic MSI infrastructure, to
> which drivers are progressively converted.
> 
> In this hardware, the MSI controller is directly bundled inside the
> interrupt controller, so we have a single Device Tree node to which
> multiple IRQ domaines are attached: the wired interrupt domain and the
> MSI interrupt domain. In order to ensure that they can be
> differentiated, we have to force the bus_token of the wired interrupt
> domain to be DOMAIN_BUS_WIRED. The MSI domain bus_token is
> automatically set to the appropriate value by
> pci_msi_create_irq_domain().
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Suggested-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  drivers/irqchip/Kconfig             |   1 +
>  drivers/irqchip/irq-armada-370-xp.c | 151 +++++++++++++++---------------------
>  2 files changed, 65 insertions(+), 87 deletions(-)
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index ab672b0..8ccb60e 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -51,6 +51,7 @@ config ARMADA_370_XP_IRQ
>  	bool
>  	default y if ARCH_MVEBU
>  	select GENERIC_IRQ_CHIP
> +	select PCI_MSI_IRQ_DOMAIN if PCI_MSI
>  
>  config ATMEL_AIC_IRQ
>  	bool
> diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
> index 3f3a8c3..304166b 100644
> --- a/drivers/irqchip/irq-armada-370-xp.c
> +++ b/drivers/irqchip/irq-armada-370-xp.c
> @@ -71,6 +71,7 @@ static u32 doorbell_mask_reg;
>  static int parent_irq;
>  #ifdef CONFIG_PCI_MSI
>  static struct irq_domain *armada_370_xp_msi_domain;
> +static struct irq_domain *armada_370_xp_msi_inner_domain;
>  static DECLARE_BITMAP(msi_used, PCI_MSI_DOORBELL_NR);
>  static DEFINE_MUTEX(msi_used_lock);
>  static phys_addr_t msi_doorbell_addr;
> @@ -115,127 +116,102 @@ static void armada_370_xp_irq_unmask(struct irq_data *d)
>  
>  #ifdef CONFIG_PCI_MSI
>  
> -static int armada_370_xp_alloc_msi(void)
> -{
> -	int hwirq;
> -
> -	mutex_lock(&msi_used_lock);
> -	hwirq = find_first_zero_bit(&msi_used, PCI_MSI_DOORBELL_NR);
> -	if (hwirq >= PCI_MSI_DOORBELL_NR)
> -		hwirq = -ENOSPC;
> -	else
> -		set_bit(hwirq, msi_used);
> -	mutex_unlock(&msi_used_lock);
> +static struct irq_chip armada_370_xp_msi_irq_chip = {
> +	.name = "armada_370_xp_msi_irq",
> +	.irq_enable = pci_msi_unmask_irq,
> +	.irq_disable = pci_msi_mask_irq,
> +	.irq_mask = pci_msi_mask_irq,
> +	.irq_unmask = pci_msi_unmask_irq,

Having only mask/unmask ought to be enough.

> +};
>  
> -	return hwirq;
> -}
> +static struct msi_domain_info armada_370_xp_msi_domain_info = {
> +	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> +		   MSI_FLAG_MULTI_PCI_MSI),

And not MSI-X? That seems rather strange (I'm pretty sure it just works).

> +	.chip	= &armada_370_xp_msi_irq_chip,
> +};
>  
> -static void armada_370_xp_free_msi(int hwirq)
> +static void armada_370_xp_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>  {
> -	mutex_lock(&msi_used_lock);
> -	if (!test_bit(hwirq, msi_used))
> -		pr_err("trying to free unused MSI#%d\n", hwirq);
> -	else
> -		clear_bit(hwirq, msi_used);
> -	mutex_unlock(&msi_used_lock);
> +	msg->address_lo = lower_32_bits(msi_doorbell_addr);
> +	msg->address_hi = upper_32_bits(msi_doorbell_addr);
> +	msg->data = 0xf00 | (data->hwirq + PCI_MSI_DOORBELL_START);
>  }
>  
> -static int armada_370_xp_setup_msi_irq(struct msi_controller *chip,
> -				       struct pci_dev *pdev,
> -				       struct msi_desc *desc)
> +static int armada_370_xp_msi_set_affinity(struct irq_data *irq_data,
> +					  const struct cpumask *mask, bool force)
>  {
> -	struct msi_msg msg;
> -	int virq, hwirq;
> +	 return -EINVAL;
> +}
>  
> -	/* We support MSI, but not MSI-X */
> -	if (desc->msi_attrib.is_msix)
> -		return -EINVAL;
> +static struct irq_chip armada_370_xp_msi_bottom_irq_chip = {
> +	.name			= "MPIC MSI",
> +	.irq_compose_msi_msg	= armada_370_xp_compose_msi_msg,
> +	.irq_set_affinity	= armada_370_xp_msi_set_affinity,
> +};
>  
> -	hwirq = armada_370_xp_alloc_msi();
> -	if (hwirq < 0)
> -		return hwirq;
> +static int armada_370_xp_msi_alloc(struct irq_domain *domain, unsigned int virq,
> +				   unsigned int nr_irqs, void *args)
> +{
> +	int hwirq;
>  
> -	virq = irq_create_mapping(armada_370_xp_msi_domain, hwirq);
> -	if (!virq) {
> -		armada_370_xp_free_msi(hwirq);
> -		return -EINVAL;
> +	mutex_lock(&msi_used_lock);
> +	hwirq = find_first_zero_bit(&msi_used, PCI_MSI_DOORBELL_NR);
> +	if (hwirq >= PCI_MSI_DOORBELL_NR) {
> +		mutex_unlock(&msi_used_lock);
> +		return -ENOSPC;
>  	}
>  
> -	irq_set_msi_desc(virq, desc);
> +	set_bit(hwirq, msi_used);
> +	mutex_unlock(&msi_used_lock);
>  
> -	msg.address_lo = msi_doorbell_addr;
> -	msg.address_hi = 0;
> -	msg.data = 0xf00 | (hwirq + 16);
> +	irq_domain_set_info(domain, virq, hwirq, &armada_370_xp_msi_bottom_irq_chip,
> +			    domain->host_data, handle_simple_irq,
> +			    NULL, NULL);
>  
> -	pci_write_msi_msg(virq, &msg);
> -	return 0;
> +	return hwirq;

So you are allocating one bit at a time, irrespective of nr_irqs. This
implies that you can't support MULTI_MSI here (you'd need to guarantee a
contiguous allocation of nr_irqs bits). So either you amend your
allocator to deal with those (pretty easy), or you drop MULTI_MSI from
your msi_domain_info.

>  }
>  
> -static void armada_370_xp_teardown_msi_irq(struct msi_controller *chip,
> -					   unsigned int irq)
> +static void armada_370_xp_msi_free(struct irq_domain *domain,
> +				   unsigned int virq, unsigned int nr_irqs)
>  {
> -	struct irq_data *d = irq_get_irq_data(irq);
> -	unsigned long hwirq = d->hwirq;
> +	struct irq_data *d = irq_domain_get_irq_data(domain, virq);
>  
> -	irq_dispose_mapping(irq);
> -	armada_370_xp_free_msi(hwirq);
> -}
> -
> -static struct irq_chip armada_370_xp_msi_irq_chip = {
> -	.name = "armada_370_xp_msi_irq",
> -	.irq_enable = pci_msi_unmask_irq,
> -	.irq_disable = pci_msi_mask_irq,
> -	.irq_mask = pci_msi_mask_irq,
> -	.irq_unmask = pci_msi_unmask_irq,
> -};
> -
> -static int armada_370_xp_msi_map(struct irq_domain *domain, unsigned int virq,
> -				 irq_hw_number_t hw)
> -{
> -	irq_set_chip_and_handler(virq, &armada_370_xp_msi_irq_chip,
> -				 handle_simple_irq);
> -
> -	return 0;
> +	mutex_lock(&msi_used_lock);
> +	if (!test_bit(d->hwirq, msi_used))
> +		pr_err("trying to free unused MSI#%lu\n", d->hwirq);
> +	else
> +		clear_bit(d->hwirq, msi_used);
> +	mutex_unlock(&msi_used_lock);
>  }
>  
> -static const struct irq_domain_ops armada_370_xp_msi_irq_ops = {
> -	.map = armada_370_xp_msi_map,
> +static const struct irq_domain_ops armada_370_xp_msi_domain_ops = {
> +	.alloc	= armada_370_xp_msi_alloc,
> +	.free	= armada_370_xp_msi_free,
>  };
>  
>  static int armada_370_xp_msi_init(struct device_node *node,
>  				  phys_addr_t main_int_phys_base)
>  {
> -	struct msi_controller *msi_chip;
>  	u32 reg;
> -	int ret;
>  
>  	msi_doorbell_addr = main_int_phys_base +
>  		ARMADA_370_XP_SW_TRIG_INT_OFFS;
>  
> -	msi_chip = kzalloc(sizeof(*msi_chip), GFP_KERNEL);
> -	if (!msi_chip)
> +	armada_370_xp_msi_inner_domain =
> +		irq_domain_add_linear(NULL, PCI_MSI_DOORBELL_NR,
> +				      &armada_370_xp_msi_domain_ops, NULL);

nit: Please keep the assignment on a single line.

> +	if (!armada_370_xp_msi_inner_domain)
>  		return -ENOMEM;
>  
> -	msi_chip->setup_irq = armada_370_xp_setup_msi_irq;
> -	msi_chip->teardown_irq = armada_370_xp_teardown_msi_irq;
> -	msi_chip->of_node = node;
> -
>  	armada_370_xp_msi_domain =
> -		irq_domain_add_linear(NULL, PCI_MSI_DOORBELL_NR,
> -				      &armada_370_xp_msi_irq_ops,
> -				      NULL);
> +		pci_msi_create_irq_domain(of_node_to_fwnode(node),
> +					  &armada_370_xp_msi_domain_info,
> +					  armada_370_xp_msi_inner_domain);

Same here.

>  	if (!armada_370_xp_msi_domain) {
> -		kfree(msi_chip);
> +		irq_domain_remove(armada_370_xp_msi_inner_domain);
>  		return -ENOMEM;
>  	}
>  
> -	ret = of_pci_msi_chip_add(msi_chip);
> -	if (ret < 0) {
> -		irq_domain_remove(armada_370_xp_msi_domain);
> -		kfree(msi_chip);
> -		return ret;
> -	}
> -
>  	reg = readl(per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_MSK_OFFS)
>  		| PCI_MSI_DOORBELL_MASK;
>  
> @@ -427,12 +403,12 @@ static void armada_370_xp_handle_msi_irq(struct pt_regs *regs, bool is_chained)
>  			continue;
>  
>  		if (is_chained) {
> -			irq = irq_find_mapping(armada_370_xp_msi_domain,
> +			irq = irq_find_mapping(armada_370_xp_msi_inner_domain,
>  					       msinr - 16);
>  			generic_handle_irq(irq);
>  		} else {
>  			irq = msinr - 16;
> -			handle_domain_irq(armada_370_xp_msi_domain,
> +			handle_domain_irq(armada_370_xp_msi_inner_domain,
>  					  irq, regs);
>  		}
>  	}
> @@ -604,6 +580,7 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,
>  	armada_370_xp_mpic_domain =
>  		irq_domain_add_linear(node, nr_irqs,
>  				&armada_370_xp_mpic_irq_ops, NULL);
> +	armada_370_xp_mpic_domain->bus_token = DOMAIN_BUS_WIRED;
>  
>  	BUG_ON(!armada_370_xp_mpic_domain);

Given the assignment just above, this BUG_ON has become pretty useless...

Thanks,

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

[PATCH 2/4] irqchip: irq-armada-370-xp: use the generic MSI infrastructure

[Thomas Petazzoni]

Marc,

On Tue, 26 Jan 2016 14:12:06 +0000, Marc Zyngier wrote:

> I finally found some bandwidth to have a look at this. A few comments below:

Great, thanks! Since 4.5-rc1 is out, I was about to resend this patch
series, so your review comes exactly at the right time. Some comments
below.

> > +static struct irq_chip armada_370_xp_msi_irq_chip = {
> > +	.name = "armada_370_xp_msi_irq",
> > +	.irq_enable = pci_msi_unmask_irq,
> > +	.irq_disable = pci_msi_mask_irq,
> > +	.irq_mask = pci_msi_mask_irq,
> > +	.irq_unmask = pci_msi_unmask_irq,
> 
> Having only mask/unmask ought to be enough.

OK, will fix.

> > -	return hwirq;
> > -}
> > +static struct msi_domain_info armada_370_xp_msi_domain_info = {
> > +	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> > +		   MSI_FLAG_MULTI_PCI_MSI),
> 
> And not MSI-X? That seems rather strange (I'm pretty sure it just works).

See commit 830cbe4b7a918613276aa3d3b28d24410623f92c ("irqchip:
armada-370-xp: implement the ->check_device() msi_chip operation") in
which we changed the driver to explicitly disable MSI-X support. The HW
does support it, but we haven't enabled it yet in the irqchip driver,
and we a PCI device driver tries to use MSI-X, it fails badly.

So, I'd like to keep the enabling of MSI-X as a separate exercise, if
you don't mind :-)

> > -	msg.address_lo = msi_doorbell_addr;
> > -	msg.address_hi = 0;
> > -	msg.data = 0xf00 | (hwirq + 16);
> > +	irq_domain_set_info(domain, virq, hwirq, &armada_370_xp_msi_bottom_irq_chip,
> > +			    domain->host_data, handle_simple_irq,
> > +			    NULL, NULL);
> >  
> > -	pci_write_msi_msg(virq, &msg);
> > -	return 0;
> > +	return hwirq;
> 
> So you are allocating one bit at a time, irrespective of nr_irqs. This
> implies that you can't support MULTI_MSI here (you'd need to guarantee a
> contiguous allocation of nr_irqs bits). So either you amend your
> allocator to deal with those (pretty easy), or you drop MULTI_MSI from
> your msi_domain_info.

Correct. Since the patch is already a bit complicated, I'm going to
drop MULTI_MSI from this patch, and then add another patch which adds
MULTI_MSI support (after adapting the alloc/free functions accordingly).


> > +	armada_370_xp_msi_inner_domain =
> > +		irq_domain_add_linear(NULL, PCI_MSI_DOORBELL_NR,
> > +				      &armada_370_xp_msi_domain_ops, NULL);
> 
> nit: Please keep the assignment on a single line.

Unfortunately, due to the long name of the variable and the function,
keeping the assignment on a single line quickly reaches the 80 columns
limit, and each argument has to be put on its own line, making the
thing even less pretty. I tend to prefer splitting the assignment on
several lines like done here in such cases, but if you really don't
like, then I don't mind changing that.

> > @@ -604,6 +580,7 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,
> >  	armada_370_xp_mpic_domain =
> >  		irq_domain_add_linear(node, nr_irqs,
> >  				&armada_370_xp_mpic_irq_ops, NULL);
> > +	armada_370_xp_mpic_domain->bus_token = DOMAIN_BUS_WIRED;
> >  
> >  	BUG_ON(!armada_370_xp_mpic_domain);
> 
> Given the assignment just above, this BUG_ON has become pretty useless...

Absolutely. I've changed the BUG_ON() location to make it somewhat more
useful.

Thanks again for the review!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 2/4] irqchip: irq-armada-370-xp: use the generic MSI infrastructure

[Marc Zyngier]

On 26/01/16 15:52, Thomas Petazzoni wrote:
> Marc,
> 
> On Tue, 26 Jan 2016 14:12:06 +0000, Marc Zyngier wrote:
> 
>> I finally found some bandwidth to have a look at this. A few comments below:
> 
> Great, thanks! Since 4.5-rc1 is out, I was about to resend this patch
> series, so your review comes exactly at the right time. Some comments
> below.
> 
>>> +static struct irq_chip armada_370_xp_msi_irq_chip = {
>>> +	.name = "armada_370_xp_msi_irq",
>>> +	.irq_enable = pci_msi_unmask_irq,
>>> +	.irq_disable = pci_msi_mask_irq,
>>> +	.irq_mask = pci_msi_mask_irq,
>>> +	.irq_unmask = pci_msi_unmask_irq,
>>
>> Having only mask/unmask ought to be enough.
> 
> OK, will fix.
> 
>>> -	return hwirq;
>>> -}
>>> +static struct msi_domain_info armada_370_xp_msi_domain_info = {
>>> +	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
>>> +		   MSI_FLAG_MULTI_PCI_MSI),
>>
>> And not MSI-X? That seems rather strange (I'm pretty sure it just works).
> 
> See commit 830cbe4b7a918613276aa3d3b28d24410623f92c ("irqchip:
> armada-370-xp: implement the ->check_device() msi_chip operation") in
> which we changed the driver to explicitly disable MSI-X support. The HW
> does support it, but we haven't enabled it yet in the irqchip driver,
> and we a PCI device driver tries to use MSI-X, it fails badly.
> 
> So, I'd like to keep the enabling of MSI-X as a separate exercise, if
> you don't mind :-)

Sure. It would be interesting to find out what is triggering the issue,
so having that as a separate patch is fine.

> 
>>> -	msg.address_lo = msi_doorbell_addr;
>>> -	msg.address_hi = 0;
>>> -	msg.data = 0xf00 | (hwirq + 16);
>>> +	irq_domain_set_info(domain, virq, hwirq, &armada_370_xp_msi_bottom_irq_chip,
>>> +			    domain->host_data, handle_simple_irq,
>>> +			    NULL, NULL);
>>>  
>>> -	pci_write_msi_msg(virq, &msg);
>>> -	return 0;
>>> +	return hwirq;
>>
>> So you are allocating one bit at a time, irrespective of nr_irqs. This
>> implies that you can't support MULTI_MSI here (you'd need to guarantee a
>> contiguous allocation of nr_irqs bits). So either you amend your
>> allocator to deal with those (pretty easy), or you drop MULTI_MSI from
>> your msi_domain_info.
> 
> Correct. Since the patch is already a bit complicated, I'm going to
> drop MULTI_MSI from this patch, and then add another patch which adds
> MULTI_MSI support (after adapting the alloc/free functions accordingly).

OK.

> 
>>> +	armada_370_xp_msi_inner_domain =
>>> +		irq_domain_add_linear(NULL, PCI_MSI_DOORBELL_NR,
>>> +				      &armada_370_xp_msi_domain_ops, NULL);
>>
>> nit: Please keep the assignment on a single line.
> 
> Unfortunately, due to the long name of the variable and the function,
> keeping the assignment on a single line quickly reaches the 80 columns
> limit, and each argument has to be put on its own line, making the
> thing even less pretty. I tend to prefer splitting the assignment on
> several lines like done here in such cases, but if you really don't
> like, then I don't mind changing that.

I definitely don't mind having lines larger than 80 chars (I've retired
my vt100 a while ago), but that's your call in the end.

Thanks,

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

[PATCH 3/4] irqchip: irq-armada-370-xp: use shorter names for irq_chip

[Thomas Petazzoni]

In order to make the output of /proc/interrupts, use shorter names for
the irq_chip registered by the irq-armada-370-xp driver. Using capital
letters also matches better what is done for the GIC driver, which
uses just "GIC" as the irq_chip->name.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/irqchip/irq-armada-370-xp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 304166b..31a183d 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -117,7 +117,7 @@ static void armada_370_xp_irq_unmask(struct irq_data *d)
 #ifdef CONFIG_PCI_MSI
 
 static struct irq_chip armada_370_xp_msi_irq_chip = {
-	.name = "armada_370_xp_msi_irq",
+	.name = "MSI MPIC",
 	.irq_enable = pci_msi_unmask_irq,
 	.irq_disable = pci_msi_mask_irq,
 	.irq_mask = pci_msi_mask_irq,
@@ -144,7 +144,7 @@ static int armada_370_xp_msi_set_affinity(struct irq_data *irq_data,
 }
 
 static struct irq_chip armada_370_xp_msi_bottom_irq_chip = {
-	.name			= "MPIC MSI",
+	.name			= "MSI MPIC",
 	.irq_compose_msi_msg	= armada_370_xp_compose_msi_msg,
 	.irq_set_affinity	= armada_370_xp_msi_set_affinity,
 };
@@ -256,7 +256,7 @@ static int armada_xp_set_affinity(struct irq_data *d,
 #endif
 
 static struct irq_chip armada_370_xp_irq_chip = {
-	.name		= "armada_370_xp_irq",
+	.name		= "MPIC",
 	.irq_mask       = armada_370_xp_irq_mask,
 	.irq_mask_ack   = armada_370_xp_irq_mask,
 	.irq_unmask     = armada_370_xp_irq_unmask,
-- 
2.6.4

[PATCH 3/4] irqchip: irq-armada-370-xp: use shorter names for irq_chip

[Gregory CLEMENT]

Hi Thomas,
 
 On lun., d?c. 21 2015, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

> In order to make the output of /proc/interrupts, use shorter names for
                                                 ^
                    in order to make it what ?---|
> the irq_chip registered by the irq-armada-370-xp driver. Using capital
> letters also matches better what is done for the GIC driver, which
> uses just "GIC" as the irq_chip->name.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/irqchip/irq-armada-370-xp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
> index 304166b..31a183d 100644
> --- a/drivers/irqchip/irq-armada-370-xp.c
> +++ b/drivers/irqchip/irq-armada-370-xp.c
> @@ -117,7 +117,7 @@ static void armada_370_xp_irq_unmask(struct irq_data *d)
>  #ifdef CONFIG_PCI_MSI
>  
>  static struct irq_chip armada_370_xp_msi_irq_chip = {
> -	.name = "armada_370_xp_msi_irq",
> +	.name = "MSI MPIC",
>  	.irq_enable = pci_msi_unmask_irq,
>  	.irq_disable = pci_msi_mask_irq,
>  	.irq_mask = pci_msi_mask_irq,
> @@ -144,7 +144,7 @@ static int armada_370_xp_msi_set_affinity(struct irq_data *irq_data,
>  }
>  
>  static struct irq_chip armada_370_xp_msi_bottom_irq_chip = {
> -	.name			= "MPIC MSI",
> +	.name			= "MSI MPIC",
>  	.irq_compose_msi_msg	= armada_370_xp_compose_msi_msg,
>  	.irq_set_affinity	= armada_370_xp_msi_set_affinity,
>  };
> @@ -256,7 +256,7 @@ static int armada_xp_set_affinity(struct irq_data *d,
>  #endif
>  
>  static struct irq_chip armada_370_xp_irq_chip = {
> -	.name		= "armada_370_xp_irq",
> +	.name		= "MPIC",

MPIC is the name also used by the power PC interrupt controller, so it
would be confusing to use exactly the same name.

What about calling it "MRVL MPIC" or "MVEBU MPIC"?
the name remains short but it won't be confused with the power PC ones.

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 3/4] irqchip: irq-armada-370-xp: use shorter names for irq_chip

[Thomas Petazzoni]

Gregory,

Thanks for your feedback!

On Wed, 23 Dec 2015 12:23:19 +0100, Gregory CLEMENT wrote:

> >  static struct irq_chip armada_370_xp_irq_chip = {
> > -	.name		= "armada_370_xp_irq",
> > +	.name		= "MPIC",
> 
> MPIC is the name also used by the power PC interrupt controller, so it
> would be confusing to use exactly the same name.

Not really: on a given system, you won't have the PowerPC interrupt
controller and the Marvell interrupt controller. For example, for the
ARM GIC, /proc/interrupts only shows GIC-0, not "ARM GIC-0".

> What about calling it "MRVL MPIC" or "MVEBU MPIC"?
> the name remains short but it won't be confused with the power PC ones.

Those names are still too long for a nice /proc/interrupts output:

# cat /proc/interrupts 
           CPU0       CPU1       
 17:       2878       2726     GIC-0  29 Edge      twd
 18:          0          0  MRVL MPIC   5 Level     armada_370_xp_per_cpu_tick
 21:        142          0     GIC-0  34 Level     mv64xxx_i2c
 22:        235          0     GIC-0  44 Level     serial
 37:          0          0     GIC-0  50 Level     ehci_hcd:usb1
 41:          0          0     GIC-0  53 Level     f10a3800.rtc
 42:          0          0     GIC-0  58 Level     ahci-mvebu[f10a8000.sata]
 43:          0          0     GIC-0  60 Level     ahci-mvebu[f10e0000.sata]
 44:       1040          0     GIC-0  57 Level     mmc0
 45:          0          0     GIC-0  48 Level     xhci-hcd:usb2
 46:          0          0     GIC-0  49 Level     xhci-hcd:usb4
108:          2          0     GIC-0  54 Level     f1060800.xor
109:          2          0     GIC-0  97 Level     f1060900.xor
110:          2          0  MRVL MSI MPIC 524288 Edge      eth0

Of course that's really a minor detail, but I don't think it's worth
making those names longer than "MSI MPIC" and "MPIC".

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 3/4] irqchip: irq-armada-370-xp: use shorter names for irq_chip

[Gregory CLEMENT]

Hi Thomas,
 
 On mar., janv. 26 2016, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

> Gregory,
>
> Thanks for your feedback!
>
> On Wed, 23 Dec 2015 12:23:19 +0100, Gregory CLEMENT wrote:
>
>> >  static struct irq_chip armada_370_xp_irq_chip = {
>> > -	.name		= "armada_370_xp_irq",
>> > +	.name		= "MPIC",
>> 
>> MPIC is the name also used by the power PC interrupt controller, so it
>> would be confusing to use exactly the same name.
>
> Not really: on a given system, you won't have the PowerPC interrupt
> controller and the Marvell interrupt controller. For example, for the

Do not underestimate the creativity of the hardware designers :)
But I agree that it is very unlikely.

> ARM GIC, /proc/interrupts only shows GIC-0, not "ARM GIC-0".
>
>> What about calling it "MRVL MPIC" or "MVEBU MPIC"?
>> the name remains short but it won't be confused with the power PC ones.
>
> Those names are still too long for a nice /proc/interrupts output:
>
> # cat /proc/interrupts 
>            CPU0       CPU1       
>  17:       2878       2726     GIC-0  29 Edge      twd
>  18:          0          0  MRVL MPIC   5 Level     armada_370_xp_per_cpu_tick
>  21:        142          0     GIC-0  34 Level     mv64xxx_i2c
>  22:        235          0     GIC-0  44 Level     serial
>  37:          0          0     GIC-0  50 Level     ehci_hcd:usb1
>  41:          0          0     GIC-0  53 Level     f10a3800.rtc
>  42:          0          0     GIC-0  58 Level     ahci-mvebu[f10a8000.sata]
>  43:          0          0     GIC-0  60 Level     ahci-mvebu[f10e0000.sata]
>  44:       1040          0     GIC-0  57 Level     mmc0
>  45:          0          0     GIC-0  48 Level     xhci-hcd:usb2
>  46:          0          0     GIC-0  49 Level     xhci-hcd:usb4
> 108:          2          0     GIC-0  54 Level     f1060800.xor
> 109:          2          0     GIC-0  97 Level     f1060900.xor
> 110:          2          0  MRVL MSI MPIC 524288 Edge      eth0
>

OK with this output I see your point, so let's move to MPIC.

Thanks,

Gregory

> Of course that's really a minor detail, but I don't think it's worth
> making those names longer than "MSI MPIC" and "MPIC".
>
> Thanks!
>
> Thomas
> -- 
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 4/4] ARM: mvebu: use the ARMADA_370_XP_IRQ option

[Thomas Petazzoni]

Now that there is a ARMADA_370_XP_IRQ option to enable the irqchip
driver for Armada 370, XP, 375, 38x and 39x, let's select this option
when needed. Note that this selection is currently not mandatory
because ARMADA_370_XP_IRQ is for now always enabled when ARCH_MVEBU=y,
but this is something that we will change in the future, and therefore
we should make the relevant platforms select ARMADA_370_XP_IRQ when
needed.

Due to this, selecting GENERIC_IRQ_CHIP is no longer needed.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/mach-mvebu/Kconfig | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
index e20fc41..d071080 100644
--- a/arch/arm/mach-mvebu/Kconfig
+++ b/arch/arm/mach-mvebu/Kconfig
@@ -2,7 +2,6 @@ menuconfig ARCH_MVEBU
 	bool "Marvell Engineering Business Unit (MVEBU) SoCs" if (ARCH_MULTI_V7 || ARCH_MULTI_V5)
 	select ARCH_SUPPORTS_BIG_ENDIAN
 	select CLKSRC_MMIO
-	select GENERIC_IRQ_CHIP
 	select PINCTRL
 	select PLAT_ORION
 	select SOC_BUS
@@ -27,6 +26,7 @@ config MACH_MVEBU_V7
 config MACH_ARMADA_370
 	bool "Marvell Armada 370 boards" if ARCH_MULTI_V7
 	select ARMADA_370_CLK
+	select ARMADA_370_XP_IRQ
 	select CPU_PJ4B
 	select MACH_MVEBU_V7
 	select PINCTRL_ARMADA_370
@@ -36,6 +36,7 @@ config MACH_ARMADA_370
 
 config MACH_ARMADA_375
 	bool "Marvell Armada 375 boards" if ARCH_MULTI_V7
+	select ARMADA_370_XP_IRQ
 	select ARM_ERRATA_720789
 	select ARM_ERRATA_753970
 	select ARM_GIC
@@ -54,6 +55,7 @@ config MACH_ARMADA_38X
 	select ARM_ERRATA_720789
 	select ARM_ERRATA_753970
 	select ARM_GIC
+	select ARMADA_370_XP_IRQ
 	select ARMADA_38X_CLK
 	select HAVE_ARM_SCU
 	select HAVE_ARM_TWD if SMP
@@ -67,6 +69,7 @@ config MACH_ARMADA_38X
 config MACH_ARMADA_39X
 	bool "Marvell Armada 39x boards" if ARCH_MULTI_V7
 	select ARM_GIC
+	select ARMADA_370_XP_IRQ
 	select ARMADA_39X_CLK
 	select CACHE_L2X0
 	select HAVE_ARM_SCU
@@ -80,6 +83,7 @@ config MACH_ARMADA_39X
 
 config MACH_ARMADA_XP
 	bool "Marvell Armada XP boards" if ARCH_MULTI_V7
+	select ARMADA_370_XP_IRQ
 	select ARMADA_XP_CLK
 	select CPU_PJ4B
 	select MACH_MVEBU_V7
-- 
2.6.4

[PATCH 4/4] ARM: mvebu: use the ARMADA_370_XP_IRQ option

[Gregory CLEMENT]

Hi Thomas,
 
 On lun., d?c. 21 2015, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

> Now that there is a ARMADA_370_XP_IRQ option to enable the irqchip
> driver for Armada 370, XP, 375, 38x and 39x, let's select this option
> when needed. Note that this selection is currently not mandatory
> because ARMADA_370_XP_IRQ is for now always enabled when ARCH_MVEBU=y,
> but this is something that we will change in the future, and therefore
> we should make the relevant platforms select ARMADA_370_XP_IRQ when
> needed.
>
> Due to this, selecting GENERIC_IRQ_CHIP is no longer needed.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Thanks,

Gregory

> ---
>  arch/arm/mach-mvebu/Kconfig | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
> index e20fc41..d071080 100644
> --- a/arch/arm/mach-mvebu/Kconfig
> +++ b/arch/arm/mach-mvebu/Kconfig
> @@ -2,7 +2,6 @@ menuconfig ARCH_MVEBU
>  	bool "Marvell Engineering Business Unit (MVEBU) SoCs" if (ARCH_MULTI_V7 || ARCH_MULTI_V5)
>  	select ARCH_SUPPORTS_BIG_ENDIAN
>  	select CLKSRC_MMIO
> -	select GENERIC_IRQ_CHIP
>  	select PINCTRL
>  	select PLAT_ORION
>  	select SOC_BUS
> @@ -27,6 +26,7 @@ config MACH_MVEBU_V7
>  config MACH_ARMADA_370
>  	bool "Marvell Armada 370 boards" if ARCH_MULTI_V7
>  	select ARMADA_370_CLK
> +	select ARMADA_370_XP_IRQ
>  	select CPU_PJ4B
>  	select MACH_MVEBU_V7
>  	select PINCTRL_ARMADA_370
> @@ -36,6 +36,7 @@ config MACH_ARMADA_370
>  
>  config MACH_ARMADA_375
>  	bool "Marvell Armada 375 boards" if ARCH_MULTI_V7
> +	select ARMADA_370_XP_IRQ
>  	select ARM_ERRATA_720789
>  	select ARM_ERRATA_753970
>  	select ARM_GIC
> @@ -54,6 +55,7 @@ config MACH_ARMADA_38X
>  	select ARM_ERRATA_720789
>  	select ARM_ERRATA_753970
>  	select ARM_GIC
> +	select ARMADA_370_XP_IRQ
>  	select ARMADA_38X_CLK
>  	select HAVE_ARM_SCU
>  	select HAVE_ARM_TWD if SMP
> @@ -67,6 +69,7 @@ config MACH_ARMADA_38X
>  config MACH_ARMADA_39X
>  	bool "Marvell Armada 39x boards" if ARCH_MULTI_V7
>  	select ARM_GIC
> +	select ARMADA_370_XP_IRQ
>  	select ARMADA_39X_CLK
>  	select CACHE_L2X0
>  	select HAVE_ARM_SCU
> @@ -80,6 +83,7 @@ config MACH_ARMADA_39X
>  
>  config MACH_ARMADA_XP
>  	bool "Marvell Armada XP boards" if ARCH_MULTI_V7
> +	select ARMADA_370_XP_IRQ
>  	select ARMADA_XP_CLK
>  	select CPU_PJ4B
>  	select MACH_MVEBU_V7
> -- 
> 2.6.4
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com