Use new MSI infrastructure on Marvell EBU

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 <[email protected]>

---
 drivers/irqchip/Kconfig  | 5 +++++
 drivers/irqchip/Makefile | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

drivers/irqchip/Kconfig +5 -0

index 4d7294e..ab672b0 100644
@@ -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

drivers/irqchip/Makefile +1 -1

index 177f78f..4d740d8 100644
@@ -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

Signature · 1 line

2.6.4

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 <[email protected]>

Suggested-by Marc Zyngier <[email protected]>

---
 drivers/irqchip/Kconfig             |   1 +
 drivers/irqchip/irq-armada-370-xp.c | 151 +++++++++++++++---------------------
 2 files changed, 65 insertions(+), 87 deletions(-)

drivers/irqchip/Kconfig +1 -0

index ab672b0..8ccb60e 100644
@@ -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

drivers/irqchip/irq-armada-370-xp.c +14 -14

index 3f3a8c3..304166b 100644
@@ -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)

drivers/irqchip/irq-armada-370-xp.c +44 -65

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

drivers/irqchip/irq-armada-370-xp.c +2 -1

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

Signature · 1 line

2.6.4

drivers/irqchip/irq-armada-370-xp.c static void armada_370_xp_irq_unmask(struct irq_data *d) · line 137

Quoted 4 lines

drivers/irqchip/irq-armada-370-xp.c +3 -1

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

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 <[email protected]>

Suggested-by Marc Zyngier <[email protected]>

---
 drivers/irqchip/Kconfig             |   1 +
 drivers/irqchip/irq-armada-370-xp.c | 151 +++++++++++++++---------------------
 2 files changed, 65 insertions(+), 87 deletions(-)

drivers/irqchip/Kconfig +1 -0

index ab672b0..8ccb60e 100644
@@ -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

drivers/irqchip/irq-armada-370-xp.c +1 -7

index 3f3a8c3..304166b 100644
@@ -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)

drivers/irqchip/irq-armada-370-xp.c +26 -23

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

drivers/irqchip/irq-armada-370-xp.c +12 -26

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

drivers/irqchip/irq-armada-370-xp.c +1 -1

+	if (!armada_370_xp_msi_inner_domain)
 		return -ENOMEM;
 
-	msi_chip->setup_irq = armada_370_xp_setup_msi_irq;

drivers/irqchip/irq-armada-370-xp.c +3 -10

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

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

drivers/irqchip/irq-armada-370-xp.c static void armada_370_xp_irq_unmask(struct irq_data *d) · line 124

Quoted 6 lines

drivers/irqchip/irq-armada-370-xp.c +6 -0

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

drivers/irqchip/irq-armada-370-xp.c static void armada_370_xp_irq_unmask(struct irq_data *d) · line 129

Quoted 5 lines

drivers/irqchip/irq-armada-370-xp.c +3 -2

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

Quoted 7 lines

drivers/irqchip/irq-armada-370-xp.c +3 -3

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

drivers/irqchip/irq-armada-370-xp.c static void armada_370_xp_irq_unmask(struct irq_data *d) · line 171

Quoted 3 lines

drivers/irqchip/irq-armada-370-xp.c +1 -2

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

drivers/irqchip/irq-armada-370-xp.c static void armada_370_xp_irq_unmask(struct irq_data *d) · line 202

Quoted 3 lines

drivers/irqchip/irq-armada-370-xp.c +3 -0

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

drivers/irqchip/irq-armada-370-xp.c static int __init armada_370_xp_mpic_of_init(struct device_node *node, · line 585

Quoted 7 lines

drivers/irqchip/irq-armada-370-xp.c +1 -0

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

Marc,

Quoted 1 line · Marc Zyngier

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.

Quoted 8 lines

drivers/irqchip/irq-armada-370-xp.c static void armada_370_xp_irq_unmask(struct irq_data *d) · line 124

Quoted 6 lines

drivers/irqchip/irq-armada-370-xp.c +6 -0

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

Quoted 7 lines

drivers/irqchip/irq-armada-370-xp.c static void armada_370_xp_irq_unmask(struct irq_data *d) · line 129

Quoted 5 lines

drivers/irqchip/irq-armada-370-xp.c +3 -2

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

Quoted 8 lines

Quoted 8 lines

drivers/irqchip/irq-armada-370-xp.c +3 -3

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

Quoted 9 lines

drivers/irqchip/irq-armada-370-xp.c static void armada_370_xp_irq_unmask(struct irq_data *d) · line 171

Quoted 3 lines

drivers/irqchip/irq-armada-370-xp.c +1 -2

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

Quoted 6 lines

drivers/irqchip/irq-armada-370-xp.c static void armada_370_xp_irq_unmask(struct irq_data *d) · line 202

Quoted 4 lines

drivers/irqchip/irq-armada-370-xp.c +3 -0

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

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 <[email protected]>

---
 drivers/irqchip/irq-armada-370-xp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/irqchip/irq-armada-370-xp.c +2 -2

index 304166b..31a183d 100644
@@ -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,
 };

drivers/irqchip/irq-armada-370-xp.c static int armada_xp_set_affinity(struct irq_data *d, · line 259

Quoted 3 lines

static struct irq_chip armada_370_xp_irq_chip = {

drivers/irqchip/irq-armada-370-xp.c +1 -1

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

Thanks for your feedback!

Quoted 6 lines · Gregory CLEMENT

drivers/irqchip/irq-armada-370-xp.c static int armada_xp_set_affinity(struct irq_data *d, · line 259

Quoted 3 lines

static struct irq_chip armada_370_xp_irq_chip = {

drivers/irqchip/irq-armada-370-xp.c +1 -1

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

Quoted 2 lines

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

Signature · 3 lines

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

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 <[email protected]>

---
 arch/arm/mach-mvebu/Kconfig | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

arch/arm/mach-mvebu/Kconfig +5 -1

index e20fc41..d071080 100644
@@ -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

Signature · 1 line

2.6.4