[PATCH v2 0/8] MSI support for Marvell EBU PCIe driver

[PATCH v2 0/8] MSI support for Marvell EBU PCIe driver

[Thomas Petazzoni]

Hello,

This set of patches introduces Message Signaled Interrupt support in
the Marvell EBU PCIe driver. It has been successfully tested on the
Armada XP GP platform with an Intel e1000e PCIe network card that
supports MSI.

This version 2 follows the RFC version sent on March, 26th 2013. This
is based on work done by Lior Amsalem <alior@marvell.com>.

The patches do the following:

 * Patch 1 comes from Thierry Redding, and adds a minimal msi_chip
   infrastructure.

   This needs to be reviewed/acked by Bjorn Helgaas.

 * Patch 2 extends the msi_chip infrastructure with a small registry,
   so that one driver can register an msi_chip, and another driver can
   find the msi_chip associated to a particular Device Tree node.

   This needs to be reviewed/acked by Bjorn Helgaas.

 * Patch 3 makes some not very interesting preparation in the Armada
   370/XP IRQ controller driver.

 * Patch 4 implements the MSI support in the Armada 370/XP IRQ
   controller driver. It registers an msi_chip using the
   msi_chip_add() function added in PATCH 2.

 * Patch 5 adjust the Armada 370/XP Device Tree to indicate that the
   MPIC is not only an interrupt-controller, but also an
   msi-controller.

 * Patch 6 adds MSI support in the Marvell PCIe host controller
   driver. The work to do here is minimal: get a reference to the
   msi-parent controller thanks to msi_chip_find_by_of_node(), and
   link it to the pci_bus structure before the bus gets enumerated.

   This needs to be reviewed/acked by Bjorn Helgaas.

 * Patch 7 tunes Kconfig to indicate that Armada 370/XP supports MSI.

 * Patch 8 adjusts the Armada 370/XP Device Tree to add the msi-parent
   properties in the PCIe controller nodes.

This set of patches currently applies on top of the current "for-next"
branch of Jason Cooper's repository.

The Device Tree binding documentation updates will be added once the
general approach is agreed on.

Thanks,

Thomas

Thierry Reding (1):
  PCI: Introduce new MSI chip infrastructure

Thomas Petazzoni (7):
  PCI: Add registry of MSI chips
  irqchip: armada-370-xp: properly request resources
  irqchip: armada-370-xp: implement MSI support
  arm: mvebu: the MPIC now provides MSI controller features
  pci: mvebu: add support for MSI
  arm: mvebu: indicate that this platform supports MSI
  arm: mvebu: link PCIe controllers to the MSI controller

 arch/arm/boot/dts/armada-370-xp.dtsi     |   1 +
 arch/arm/boot/dts/armada-370.dtsi        |   1 +
 arch/arm/boot/dts/armada-xp-mv78230.dtsi |   1 +
 arch/arm/boot/dts/armada-xp-mv78260.dtsi |   1 +
 arch/arm/boot/dts/armada-xp-mv78460.dtsi |   1 +
 arch/arm/mach-mvebu/Kconfig              |   1 +
 drivers/irqchip/irq-armada-370-xp.c      | 186 ++++++++++++++++++++++++++++++-
 drivers/pci/host/pci-mvebu.c             |  21 ++++
 drivers/pci/msi.c                        |  59 +++++++++-
 drivers/pci/probe.c                      |   1 +
 include/linux/msi.h                      |  22 ++++
 include/linux/pci.h                      |   1 +
 12 files changed, 288 insertions(+), 8 deletions(-)

-- 
1.8.1.2

[PATCH v2 1/8] PCI: Introduce new MSI chip infrastructure

[Thomas Petazzoni]

From: Thierry Reding <thierry.reding@avionic-design.de>

The new struct msi_chip is used to associated an MSI controller with a
PCI bus. It is automatically handed down from the root to its children
during bus enumeration.

This patch provides default (weak) implementations for the architecture-
specific MSI functions (arch_setup_msi_irq(), arch_teardown_msi_irq()
and arch_msi_check_device()) which check if a PCI device's bus has an
attached MSI chip and forward the call appropriately.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/pci/msi.c   | 34 +++++++++++++++++++++++++++++++---
 drivers/pci/probe.c |  1 +
 include/linux/msi.h | 11 +++++++++++
 include/linux/pci.h |  1 +
 4 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 2c10752..4dafac6 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -29,13 +29,41 @@ static int pci_msi_enable = 1;
 
 
 /* Arch hooks */
+int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
+{
+	struct msi_chip *chip = dev->bus->msi;
+
+	if (chip && chip->setup_irq) {
+		int err;
+
+		err = chip->setup_irq(chip, dev, desc);
+		if (err < 0)
+			return err;
+
+		irq_set_chip_data(desc->irq, chip);
+		return err;
+	}
+
+	return -EINVAL;
+}
 
-#ifndef arch_msi_check_device
-int arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
+void __weak arch_teardown_msi_irq(unsigned int irq)
 {
+	struct msi_chip *chip = irq_get_chip_data(irq);
+
+	if (chip && chip->teardown_irq)
+		chip->teardown_irq(chip, irq);
+}
+
+int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
+{
+	struct msi_chip *chip = dev->bus->msi;
+
+	if (chip && chip->check_device)
+		return chip->check_device(chip, dev, nvec, type);
+
 	return 0;
 }
-#endif
 
 #ifndef arch_setup_msi_irqs
 # define arch_setup_msi_irqs default_setup_msi_irqs
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 70f10fa..c8591e4 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -634,6 +634,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 
 	child->parent = parent;
 	child->ops = parent->ops;
+	child->msi = parent->msi;
 	child->sysdata = parent->sysdata;
 	child->bus_flags = parent->bus_flags;
 
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 20c2d6d..4633529 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -58,4 +58,15 @@ int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);
 void arch_teardown_msi_irqs(struct pci_dev *dev);
 int arch_msi_check_device(struct pci_dev* dev, int nvec, int type);
 
+struct msi_chip {
+	struct module *owner;
+	struct device *dev;
+
+	int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
+			 struct msi_desc *desc);
+	void (*teardown_irq)(struct msi_chip *chip, unsigned int irq);
+	int (*check_device)(struct msi_chip *chip, struct pci_dev *dev,
+			    int nvec, int type);
+};
+
 #endif /* LINUX_MSI_H */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 3a24e4f..7ffc012 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -432,6 +432,7 @@ struct pci_bus {
 	struct resource busn_res;	/* bus numbers routed to this bus */
 
 	struct pci_ops	*ops;		/* configuration access functions */
+	struct msi_chip	*msi;		/* MSI controller */
 	void		*sysdata;	/* hook for sys-specific extension */
 	struct proc_dir_entry *procdir;	/* directory entry in /proc/bus/pci */
 
-- 
1.8.1.2

[PATCH v2 1/8] PCI: Introduce new MSI chip infrastructure

[Bjorn Helgaas]

On Thu, Jun 06, 2013 at 06:41:21PM +0200, Thomas Petazzoni wrote:
> From: Thierry Reding <thierry.reding@avionic-design.de>
> 
> The new struct msi_chip is used to associated an MSI controller with a
> PCI bus. It is automatically handed down from the root to its children
> during bus enumeration.
> 
> This patch provides default (weak) implementations for the architecture-
> specific MSI functions (arch_setup_msi_irq(), arch_teardown_msi_irq()
> and arch_msi_check_device()) which check if a PCI device's bus has an
> attached MSI chip and forward the call appropriately.
> 
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/pci/msi.c   | 34 +++++++++++++++++++++++++++++++---
>  drivers/pci/probe.c |  1 +
>  include/linux/msi.h | 11 +++++++++++
>  include/linux/pci.h |  1 +
>  4 files changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 2c10752..4dafac6 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -29,13 +29,41 @@ static int pci_msi_enable = 1;
>  
>  
>  /* Arch hooks */
> +int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
> +{
> +	struct msi_chip *chip = dev->bus->msi;
> +
> +	if (chip && chip->setup_irq) {
> +		int err;
> +
> +		err = chip->setup_irq(chip, dev, desc);
> +		if (err < 0)
> +			return err;
> +
> +		irq_set_chip_data(desc->irq, chip);
> +		return err;
> +	}
> +
> +	return -EINVAL;
> +}
>  
> -#ifndef arch_msi_check_device
> -int arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> +void __weak arch_teardown_msi_irq(unsigned int irq)

Please make a separate patch before this one for the conversion from the
"#define arch_msi_check_device" strategy to the weak function.  I think
it's a good idea to use a weak function rather than the #define, but we
need to remove the #define from arch/powerpc/include/asm/pci.h at the same
time.

I don't think these patches touch arch_setup_msi_irqs() or
arch_teardown_msi_irqs(), but I'd like to do the same with them just so we
consistently use the same strategy to solve the same problem.

>  {
> +	struct msi_chip *chip = irq_get_chip_data(irq);
> +
> +	if (chip && chip->teardown_irq)
> +		chip->teardown_irq(chip, irq);
> +}
> +
> +int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> +{
> +	struct msi_chip *chip = dev->bus->msi;
> +
> +	if (chip && chip->check_device)
> +		return chip->check_device(chip, dev, nvec, type);
> +
>  	return 0;
>  }
> -#endif
>  
>  #ifndef arch_setup_msi_irqs
>  # define arch_setup_msi_irqs default_setup_msi_irqs
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 70f10fa..c8591e4 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -634,6 +634,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>  
>  	child->parent = parent;
>  	child->ops = parent->ops;
> +	child->msi = parent->msi;
>  	child->sysdata = parent->sysdata;
>  	child->bus_flags = parent->bus_flags;
>  
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index 20c2d6d..4633529 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -58,4 +58,15 @@ int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);
>  void arch_teardown_msi_irqs(struct pci_dev *dev);
>  int arch_msi_check_device(struct pci_dev* dev, int nvec, int type);
>  
> +struct msi_chip {
> +	struct module *owner;

Can the MSI chip driver be a loadable module?  Does it need to be?

> +	struct device *dev;
> +
> +	int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
> +			 struct msi_desc *desc);
> +	void (*teardown_irq)(struct msi_chip *chip, unsigned int irq);
> +	int (*check_device)(struct msi_chip *chip, struct pci_dev *dev,
> +			    int nvec, int type);
> +};
> +
>  #endif /* LINUX_MSI_H */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 3a24e4f..7ffc012 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -432,6 +432,7 @@ struct pci_bus {
>  	struct resource busn_res;	/* bus numbers routed to this bus */
>  
>  	struct pci_ops	*ops;		/* configuration access functions */
> +	struct msi_chip	*msi;		/* MSI controller */
>  	void		*sysdata;	/* hook for sys-specific extension */
>  	struct proc_dir_entry *procdir;	/* directory entry in /proc/bus/pci */
>  
> -- 
> 1.8.1.2
> 

[PATCH v2 1/8] PCI: Introduce new MSI chip infrastructure

[Thomas Petazzoni]

Dear Bjorn Helgaas,

On Tue, 18 Jun 2013 16:46:31 -0600, Bjorn Helgaas wrote:

> > -#ifndef arch_msi_check_device
> > -int arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> > +void __weak arch_teardown_msi_irq(unsigned int irq)
> 
> Please make a separate patch before this one for the conversion from the
> "#define arch_msi_check_device" strategy to the weak function.  I think
> it's a good idea to use a weak function rather than the #define, but we
> need to remove the #define from arch/powerpc/include/asm/pci.h at the same
> time.
> 
> I don't think these patches touch arch_setup_msi_irqs() or
> arch_teardown_msi_irqs(), but I'd like to do the same with them just so we
> consistently use the same strategy to solve the same problem.

Ok, I've tried to refactor all those MSI operations that can be
overriden on a per-architecture basis, it will be part of v3 to be sent
soon.


> > +struct msi_chip {
> > +	struct module *owner;
> 
> Can the MSI chip driver be a loadable module?  Does it need to be?

In the case of the Marvell SoC, the MSI logic is part of the IRQ
controller driver, so it's very unlikely that it can be built as a
module.

However, in the case of the Tegra PCIe hardware, the MSI logic is part
of the PCIe hardware itself. And the PCIe driver could potentially be
built as a module (even though some other implementations details in
the support of PCI on ARM currently prevents this, it should be
possible in theory). And since this code comes from Thierry Redding,
who was writing it with the Tegra PCIe in mind, it makes sense to
assume the MSI code could be built as a module.

Best regards,

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

[PATCH v2 2/8] PCI: Add registry of MSI chips

[Thomas Petazzoni]

This commit adds a very basic registry of msi_chip structures, so that
an IRQ controller driver can register an msi_chip, and a PCIe host
controller can find it, based on a 'struct device_node'.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/pci/msi.c   | 25 +++++++++++++++++++++++++
 include/linux/msi.h | 11 +++++++++++
 2 files changed, 36 insertions(+)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 4dafac6..0f177ee 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -20,6 +20,7 @@
 #include <linux/errno.h>
 #include <linux/io.h>
 #include <linux/slab.h>
+#include <linux/of.h>
 
 #include "pci.h"
 
@@ -70,6 +71,30 @@ int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
 # define HAVE_DEFAULT_MSI_SETUP_IRQS
 #endif
 
+#ifdef CONFIG_OF
+static LIST_HEAD(msi_chip_list);
+static DEFINE_MUTEX(msi_chip_mutex);
+
+void msi_chip_add(struct msi_chip *chip)
+{
+	mutex_lock(&msi_chip_mutex);
+	list_add(&chip->link, &msi_chip_list);
+	mutex_unlock(&msi_chip_mutex);
+}
+
+struct msi_chip *msi_chip_find_by_of_node(struct device_node *of_node)
+{
+	struct msi_chip *c;
+	list_for_each_entry(c, &msi_chip_list, link) {
+		if (c->of_node == of_node &&
+		    of_property_read_bool(c->of_node, "msi-controller"))
+			return c;
+	}
+
+	return NULL;
+}
+#endif
+
 #ifdef HAVE_DEFAULT_MSI_SETUP_IRQS
 int default_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 {
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 4633529..a1a6084 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -61,6 +61,8 @@ int arch_msi_check_device(struct pci_dev* dev, int nvec, int type);
 struct msi_chip {
 	struct module *owner;
 	struct device *dev;
+	struct device_node *of_node;
+	struct list_head link;
 
 	int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
 			 struct msi_desc *desc);
@@ -69,4 +71,13 @@ struct msi_chip {
 			    int nvec, int type);
 };
 
+#if defined(CONFIG_PCI_MSI) && defined(CONFIG_OF)
+void msi_chip_add(struct msi_chip *chip);
+struct msi_chip *msi_chip_find_by_of_node(struct device_node *of_node);
+#else
+static inline void msi_chip_add(struct msi_chip *chip) {}
+static inline struct msi_chip *
+msi_chip_find_by_of_node(struct device_node *of_node) { return NULL; }
+#endif
+
 #endif /* LINUX_MSI_H */
-- 
1.8.1.2

[PATCH v2 2/8] PCI: Add registry of MSI chips

[Thierry Reding]

On Thu, Jun 06, 2013 at 06:41:22PM +0200, Thomas Petazzoni wrote:
[...]
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index 4633529..a1a6084 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -61,6 +61,8 @@ int arch_msi_check_device(struct pci_dev* dev, int nvec, int type);
>  struct msi_chip {
>  	struct module *owner;
>  	struct device *dev;
> +	struct device_node *of_node;

Shouldn't we be reusing the dev.of_node field to look this information
up? On Tegra we do register the MSI chip and set the dev member to the
PCIe host bridge device. Can't the same thing be done for the Armada?

I assume that the MSI controller provider will be instantiated from the
device tree and hence there will already be a struct platform_device and
therefore also a struct device associated with it which should have the
of_node field set to the correct value automatically.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130612/674c4214/attachment.sig>

[PATCH v2 2/8] PCI: Add registry of MSI chips

[Bjorn Helgaas]

On Thu, Jun 06, 2013 at 06:41:22PM +0200, Thomas Petazzoni wrote:
> This commit adds a very basic registry of msi_chip structures, so that
> an IRQ controller driver can register an msi_chip, and a PCIe host
> controller can find it, based on a 'struct device_node'.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/pci/msi.c   | 25 +++++++++++++++++++++++++
>  include/linux/msi.h | 11 +++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 4dafac6..0f177ee 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -20,6 +20,7 @@
>  #include <linux/errno.h>
>  #include <linux/io.h>
>  #include <linux/slab.h>
> +#include <linux/of.h>
>  
>  #include "pci.h"
>  
> @@ -70,6 +71,30 @@ int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
>  # define HAVE_DEFAULT_MSI_SETUP_IRQS
>  #endif
>  
> +#ifdef CONFIG_OF
> +static LIST_HEAD(msi_chip_list);
> +static DEFINE_MUTEX(msi_chip_mutex);
> +
> +void msi_chip_add(struct msi_chip *chip)
> +{
> +	mutex_lock(&msi_chip_mutex);
> +	list_add(&chip->link, &msi_chip_list);
> +	mutex_unlock(&msi_chip_mutex);
> +}
> +
> +struct msi_chip *msi_chip_find_by_of_node(struct device_node *of_node)
> +{
> +	struct msi_chip *c;
> +	list_for_each_entry(c, &msi_chip_list, link) {
> +		if (c->of_node == of_node &&
> +		    of_property_read_bool(c->of_node, "msi-controller"))
> +			return c;
> +	}
> +
> +	return NULL;
> +}
> +#endif

I don't really understand why the msi_chip_list lives here, since
nothing in drivers/pci/msi.c uses it.  Seems like maybe something
that could be in the OF code somewhere?

> +
>  #ifdef HAVE_DEFAULT_MSI_SETUP_IRQS
>  int default_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
>  {
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index 4633529..a1a6084 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -61,6 +61,8 @@ int arch_msi_check_device(struct pci_dev* dev, int nvec, int type);
>  struct msi_chip {
>  	struct module *owner;
>  	struct device *dev;
> +	struct device_node *of_node;
> +	struct list_head link;
>  
>  	int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
>  			 struct msi_desc *desc);
> @@ -69,4 +71,13 @@ struct msi_chip {
>  			    int nvec, int type);
>  };
>  
> +#if defined(CONFIG_PCI_MSI) && defined(CONFIG_OF)
> +void msi_chip_add(struct msi_chip *chip);
> +struct msi_chip *msi_chip_find_by_of_node(struct device_node *of_node);
> +#else
> +static inline void msi_chip_add(struct msi_chip *chip) {}
> +static inline struct msi_chip *
> +msi_chip_find_by_of_node(struct device_node *of_node) { return NULL; }
> +#endif
> +
>  #endif /* LINUX_MSI_H */
> -- 
> 1.8.1.2
> 

[PATCH v2 2/8] PCI: Add registry of MSI chips

[Thomas Petazzoni]

Dear Bjorn Helgaas,

On Tue, 18 Jun 2013 16:48:44 -0600, Bjorn Helgaas wrote:

> I don't really understand why the msi_chip_list lives here, since
> nothing in drivers/pci/msi.c uses it.  Seems like maybe something
> that could be in the OF code somewhere?

Ok, I'll move this to drivers/of/, we'll see what Grant says about
this. Will be part of v3.

Best regards,

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

[PATCH v2 3/8] irqchip: armada-370-xp: properly request resources

[Thomas Petazzoni]

Instead of using of_iomap(), we now use of_address_to_resource(),
request_mem_region() and ioremap(). This allows the corresponding I/O
regions to be properly requested and visible in /proc/iomem.

The main motivation for this change is that the introduction of the
MSI support requires us to get the physical address of the main
interrupt controller registers, so we will need the corresponding
'struct resource' anyway.

We also take this opportunity to change a panic() to BUG_ON(), in
order to be consistent with the rest of the driver.

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

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index bb328a3..26adc74 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -248,12 +248,25 @@ armada_370_xp_handle_irq(struct pt_regs *regs)
 static int __init armada_370_xp_mpic_of_init(struct device_node *node,
 					     struct device_node *parent)
 {
+	struct resource main_int_res, per_cpu_int_res;
 	u32 control;
 
-	main_int_base = of_iomap(node, 0);
-	per_cpu_int_base = of_iomap(node, 1);
+	BUG_ON(of_address_to_resource(node, 0, &main_int_res));
+	BUG_ON(of_address_to_resource(node, 1, &per_cpu_int_res));
 
+	BUG_ON(!request_mem_region(main_int_res.start,
+				   resource_size(&main_int_res),
+				   node->full_name));
+	BUG_ON(!request_mem_region(per_cpu_int_res.start,
+				   resource_size(&per_cpu_int_res),
+				   node->full_name));
+
+	main_int_base = ioremap(main_int_res.start,
+				resource_size(&main_int_res));
 	BUG_ON(!main_int_base);
+
+	per_cpu_int_base = ioremap(per_cpu_int_res.start,
+				   resource_size(&per_cpu_int_res));
 	BUG_ON(!per_cpu_int_base);
 
 	control = readl(main_int_base + ARMADA_370_XP_INT_CONTROL);
@@ -262,8 +275,7 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,
 		irq_domain_add_linear(node, (control >> 2) & 0x3ff,
 				&armada_370_xp_mpic_irq_ops, NULL);
 
-	if (!armada_370_xp_mpic_domain)
-		panic("Unable to add Armada_370_Xp MPIC irq domain (DT)\n");
+	BUG_ON(!armada_370_xp_mpic_domain);
 
 	irq_set_default_host(armada_370_xp_mpic_domain);
 
-- 
1.8.1.2

[PATCH v2 4/8] irqchip: armada-370-xp: implement MSI support

[Thomas Petazzoni]

This commit introduces the support for the MSI interrupts in the
armada-370-xp interrupt controller driver. It registers an MSI chip to
the MSI chip registry, which will be used by the Marvell PCIe host
controller driver.

The MSI interrupts use the 16 high doorbells, and are therefore
notified using IRQ1 of the main interrupt controller.

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

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 26adc74..c7c904d 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -22,6 +22,8 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/irqdomain.h>
+#include <linux/slab.h>
+#include <linux/msi.h>
 #include <asm/mach/arch.h>
 #include <asm/exception.h>
 #include <asm/smp_plat.h>
@@ -51,12 +53,22 @@
 #define IPI_DOORBELL_START                      (0)
 #define IPI_DOORBELL_END                        (8)
 #define IPI_DOORBELL_MASK                       0xFF
+#define PCI_MSI_DOORBELL_START                  (16)
+#define PCI_MSI_DOORBELL_NR                     (16)
+#define PCI_MSI_DOORBELL_END                    (32)
+#define PCI_MSI_DOORBELL_MASK                   0xFFFF0000
 
 static DEFINE_RAW_SPINLOCK(irq_controller_lock);
 
 static void __iomem *per_cpu_int_base;
 static void __iomem *main_int_base;
 static struct irq_domain *armada_370_xp_mpic_domain;
+#ifdef CONFIG_PCI_MSI
+static struct irq_domain *armada_370_xp_msi_domain;
+static DECLARE_BITMAP(msi_used, PCI_MSI_DOORBELL_NR);
+static DEFINE_MUTEX(msi_used_lock);
+static phys_addr_t msi_doorbell_addr;
+#endif
 
 /*
  * In SMP mode:
@@ -87,6 +99,126 @@ static void armada_370_xp_irq_unmask(struct irq_data *d)
 				ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
 }
 
+#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);
+
+	return hwirq;
+}
+
+static void armada_370_xp_free_msi(int hwirq)
+{
+	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);
+}
+
+static int armada_370_xp_setup_msi_irq(struct msi_chip *chip,
+				       struct pci_dev *pdev,
+				       struct msi_desc *desc)
+{
+	struct msi_msg msg;
+	int hwirq, irq;
+
+	hwirq = armada_370_xp_alloc_msi();
+	if (hwirq < 0)
+		return hwirq;
+
+	irq = irq_create_mapping(armada_370_xp_msi_domain, hwirq);
+	if (!irq) {
+		armada_370_xp_free_msi(hwirq);
+		return -EINVAL;
+	}
+
+	irq_set_msi_desc(irq, desc);
+
+	msg.address_lo = msi_doorbell_addr;
+	msg.address_hi = 0;
+	msg.data = 0xf00 | (hwirq + 16);
+
+	write_msi_msg(irq, &msg);
+	return 0;
+}
+
+static void armada_370_xp_teardown_msi_irq(struct msi_chip *chip,
+					   unsigned int irq)
+{
+	struct irq_data *d = irq_get_irq_data(irq);
+	armada_370_xp_free_msi(d->hwirq);
+}
+
+static struct irq_chip armada_370_xp_msi_irq_chip = {
+	.name = "armada_370_xp_msi_irq",
+	.irq_enable = unmask_msi_irq,
+	.irq_disable = mask_msi_irq,
+	.irq_mask = mask_msi_irq,
+	.irq_unmask = unmask_msi_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);
+	set_irq_flags(virq, IRQF_VALID);
+
+	return 0;
+}
+
+static const struct irq_domain_ops armada_370_xp_msi_irq_ops = {
+	.map = armada_370_xp_msi_map,
+};
+
+static int armada_370_xp_msi_init(struct device_node *node)
+{
+	struct msi_chip *msi_chip;
+	u32 reg;
+
+	msi_chip = kzalloc(sizeof(*msi_chip), GFP_KERNEL);
+	if (!msi_chip)
+		return -ENOMEM;
+
+	armada_370_xp_msi_domain =
+		irq_domain_add_linear(NULL, PCI_MSI_DOORBELL_NR,
+				      &armada_370_xp_msi_irq_ops, NULL);
+	if (!armada_370_xp_msi_domain) {
+		kfree(msi_chip);
+		return -ENOMEM;
+	}
+
+	msi_chip->of_node = node;
+	msi_chip->setup_irq = armada_370_xp_setup_msi_irq;
+	msi_chip->teardown_irq = armada_370_xp_teardown_msi_irq;
+
+	msi_chip_add(msi_chip);
+
+	reg = readl(per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_MSK_OFFS)
+		| PCI_MSI_DOORBELL_MASK;
+
+	writel(reg, per_cpu_int_base +
+	       ARMADA_370_XP_IN_DRBEL_MSK_OFFS);
+
+	/* Unmask IPI interrupt */
+	writel(1, per_cpu_int_base + ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
+
+	return 0;
+}
+#else
+static inline int armada_370_xp_msi_init(struct device_node *node) { return 0; }
+#endif
+
 #ifdef CONFIG_SMP
 static int armada_xp_set_affinity(struct irq_data *d,
 				  const struct cpumask *mask_val, bool force)
@@ -214,12 +346,39 @@ armada_370_xp_handle_irq(struct pt_regs *regs)
 		if (irqnr > 1022)
 			break;
 
-		if (irqnr > 0) {
+		if (irqnr > 1) {
 			irqnr =	irq_find_mapping(armada_370_xp_mpic_domain,
 					irqnr);
 			handle_IRQ(irqnr, regs);
 			continue;
 		}
+
+#ifdef CONFIG_PCI_MSI
+		/* MSI handling */
+		if (irqnr == 1) {
+			u32 msimask, msinr;
+
+			msimask = readl_relaxed(per_cpu_int_base +
+						ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS)
+				& PCI_MSI_DOORBELL_MASK;
+
+			writel(~PCI_MSI_DOORBELL_MASK, per_cpu_int_base +
+			       ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS);
+
+			for (msinr = PCI_MSI_DOORBELL_START;
+			     msinr < PCI_MSI_DOORBELL_END; msinr++) {
+				int irq;
+
+				if (!(msimask & BIT(msinr)))
+					continue;
+
+				irq = irq_find_mapping(armada_370_xp_msi_domain,
+						       msinr - 16);
+				handle_IRQ(irq, regs);
+			}
+		}
+#endif
+
 #ifdef CONFIG_SMP
 		/* IPI Handling */
 		if (irqnr == 0) {
@@ -269,6 +428,9 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,
 				   resource_size(&per_cpu_int_res));
 	BUG_ON(!per_cpu_int_base);
 
+	msi_doorbell_addr = main_int_res.start +
+		ARMADA_370_XP_SW_TRIG_INT_OFFS;
+
 	control = readl(main_int_base + ARMADA_370_XP_INT_CONTROL);
 
 	armada_370_xp_mpic_domain =
@@ -292,6 +454,8 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,
 
 #endif
 
+	armada_370_xp_msi_init(node);
+
 	set_handle_irq(armada_370_xp_handle_irq);
 
 	return 0;
-- 
1.8.1.2

[PATCH v2 4/8] irqchip: armada-370-xp: implement MSI support

[Grant Likely]

On Thu,  6 Jun 2013 18:41:24 +0200, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:
> This commit introduces the support for the MSI interrupts in the
> armada-370-xp interrupt controller driver. It registers an MSI chip to
> the MSI chip registry, which will be used by the Marvell PCIe host
> controller driver.
> 
> The MSI interrupts use the 16 high doorbells, and are therefore
> notified using IRQ1 of the main interrupt controller.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/irqchip/irq-armada-370-xp.c | 166 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 165 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
> index 26adc74..c7c904d 100644
> --- a/drivers/irqchip/irq-armada-370-xp.c
> +++ b/drivers/irqchip/irq-armada-370-xp.c
> @@ -22,6 +22,8 @@
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
>  #include <linux/irqdomain.h>
> +#include <linux/slab.h>
> +#include <linux/msi.h>
>  #include <asm/mach/arch.h>
>  #include <asm/exception.h>
>  #include <asm/smp_plat.h>
> @@ -51,12 +53,22 @@
>  #define IPI_DOORBELL_START                      (0)
>  #define IPI_DOORBELL_END                        (8)
>  #define IPI_DOORBELL_MASK                       0xFF
> +#define PCI_MSI_DOORBELL_START                  (16)
> +#define PCI_MSI_DOORBELL_NR                     (16)
> +#define PCI_MSI_DOORBELL_END                    (32)
> +#define PCI_MSI_DOORBELL_MASK                   0xFFFF0000
>  
>  static DEFINE_RAW_SPINLOCK(irq_controller_lock);
>  
>  static void __iomem *per_cpu_int_base;
>  static void __iomem *main_int_base;
>  static struct irq_domain *armada_370_xp_mpic_domain;
> +#ifdef CONFIG_PCI_MSI
> +static struct irq_domain *armada_370_xp_msi_domain;
> +static DECLARE_BITMAP(msi_used, PCI_MSI_DOORBELL_NR);
> +static DEFINE_MUTEX(msi_used_lock);
> +static phys_addr_t msi_doorbell_addr;
> +#endif
>  
>  /*
>   * In SMP mode:
> @@ -87,6 +99,126 @@ static void armada_370_xp_irq_unmask(struct irq_data *d)
>  				ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
>  }
>  
> +#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);
> +
> +	return hwirq;
> +}
> +
> +static void armada_370_xp_free_msi(int hwirq)
> +{
> +	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);
> +}
> +
> +static int armada_370_xp_setup_msi_irq(struct msi_chip *chip,
> +				       struct pci_dev *pdev,
> +				       struct msi_desc *desc)
> +{
> +	struct msi_msg msg;
> +	int hwirq, irq;
> +
> +	hwirq = armada_370_xp_alloc_msi();
> +	if (hwirq < 0)
> +		return hwirq;
> +
> +	irq = irq_create_mapping(armada_370_xp_msi_domain, hwirq);
> +	if (!irq) {
> +		armada_370_xp_free_msi(hwirq);
> +		return -EINVAL;
> +	}

This looks like something that the irq_domain should handle for you.
It would be good to have a variant of irq_create_mapping() that keeps
track of available hwirqs, allocates a free one, and maps it all with
one function call. I can see that being useful by other MSI interrupt
controllers and would eliminate needing to open-code it above.

take a look at the irqdomain/test branch on git://git.secretlab.ca/git/linux

I'm hoping to push that series into v3.11 and it simplifies the
irq_domain code quite a bit. It would be easy to build an
irq_create_mapping() variant on top of that. Take a look at
irq_create_direct_mapping() for inspiration (although that function does
it the other way around. It allocates a virq and uses that number for
the hwirq number)

> +static int armada_370_xp_msi_init(struct device_node *node)
> +{
> +	struct msi_chip *msi_chip;
> +	u32 reg;
> +
> +	msi_chip = kzalloc(sizeof(*msi_chip), GFP_KERNEL);

devm_kzalloc() so it gets freed on failure or unbind; although for this
device you may not care.

> +	if (!msi_chip)
> +		return -ENOMEM;
> +
> +	armada_370_xp_msi_domain =
> +		irq_domain_add_linear(NULL, PCI_MSI_DOORBELL_NR,
> +				      &armada_370_xp_msi_irq_ops, NULL);
> +	if (!armada_370_xp_msi_domain) {
> +		kfree(msi_chip);
> +		return -ENOMEM;
> +	}
> +
> +	msi_chip->of_node = node;
> +	msi_chip->setup_irq = armada_370_xp_setup_msi_irq;
> +	msi_chip->teardown_irq = armada_370_xp_teardown_msi_irq;
> +
> +	msi_chip_add(msi_chip);
> +
> +	reg = readl(per_cpu_int_base + ARMADA_370_XP_IN_DRBEL_MSK_OFFS)
> +		| PCI_MSI_DOORBELL_MASK;
> +
> +	writel(reg, per_cpu_int_base +
> +	       ARMADA_370_XP_IN_DRBEL_MSK_OFFS);
> +
> +	/* Unmask IPI interrupt */
> +	writel(1, per_cpu_int_base + ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
> +
> +	return 0;
> +}
> +#else
> +static inline int armada_370_xp_msi_init(struct device_node *node) { return 0; }
> +#endif
> +
>  #ifdef CONFIG_SMP
>  static int armada_xp_set_affinity(struct irq_data *d,
>  				  const struct cpumask *mask_val, bool force)
> @@ -214,12 +346,39 @@ armada_370_xp_handle_irq(struct pt_regs *regs)
>  		if (irqnr > 1022)
>  			break;
>  
> -		if (irqnr > 0) {
> +		if (irqnr > 1) {
>  			irqnr =	irq_find_mapping(armada_370_xp_mpic_domain,
>  					irqnr);
>  			handle_IRQ(irqnr, regs);
>  			continue;
>  		}
> +
> +#ifdef CONFIG_PCI_MSI
> +		/* MSI handling */
> +		if (irqnr == 1) {
> +			u32 msimask, msinr;
> +
> +			msimask = readl_relaxed(per_cpu_int_base +
> +						ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS)
> +				& PCI_MSI_DOORBELL_MASK;
> +
> +			writel(~PCI_MSI_DOORBELL_MASK, per_cpu_int_base +
> +			       ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS);
> +
> +			for (msinr = PCI_MSI_DOORBELL_START;
> +			     msinr < PCI_MSI_DOORBELL_END; msinr++) {
> +				int irq;
> +
> +				if (!(msimask & BIT(msinr)))
> +					continue;
> +
> +				irq = irq_find_mapping(armada_370_xp_msi_domain,
> +						       msinr - 16);
> +				handle_IRQ(irq, regs);
> +			}
> +		}
> +#endif
> +
>  #ifdef CONFIG_SMP
>  		/* IPI Handling */
>  		if (irqnr == 0) {
> @@ -269,6 +428,9 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,
>  				   resource_size(&per_cpu_int_res));
>  	BUG_ON(!per_cpu_int_base);
>  
> +	msi_doorbell_addr = main_int_res.start +
> +		ARMADA_370_XP_SW_TRIG_INT_OFFS;
> +
>  	control = readl(main_int_base + ARMADA_370_XP_INT_CONTROL);
>  
>  	armada_370_xp_mpic_domain =
> @@ -292,6 +454,8 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,
>  
>  #endif
>  
> +	armada_370_xp_msi_init(node);
> +
>  	set_handle_irq(armada_370_xp_handle_irq);
>  
>  	return 0;
> -- 
> 1.8.1.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

[PATCH v2 4/8] irqchip: armada-370-xp: implement MSI support

[Thomas Petazzoni]

Dear Grant Likely,

On Tue, 11 Jun 2013 14:37:45 +0100, Grant Likely wrote:

> > +static int armada_370_xp_setup_msi_irq(struct msi_chip *chip,
> > +				       struct pci_dev *pdev,
> > +				       struct msi_desc *desc)
> > +{
> > +	struct msi_msg msg;
> > +	int hwirq, irq;
> > +
> > +	hwirq = armada_370_xp_alloc_msi();
> > +	if (hwirq < 0)
> > +		return hwirq;
> > +
> > +	irq = irq_create_mapping(armada_370_xp_msi_domain, hwirq);
> > +	if (!irq) {
> > +		armada_370_xp_free_msi(hwirq);
> > +		return -EINVAL;
> > +	}
> 
> This looks like something that the irq_domain should handle for you.
> It would be good to have a variant of irq_create_mapping() that keeps
> track of available hwirqs, allocates a free one, and maps it all with
> one function call. I can see that being useful by other MSI interrupt
> controllers and would eliminate needing to open-code it above.
> 
> take a look at the irqdomain/test branch on git://git.secretlab.ca/git/linux
> 
> I'm hoping to push that series into v3.11 and it simplifies the
> irq_domain code quite a bit. It would be easy to build an
> irq_create_mapping() variant on top of that. Take a look at
> irq_create_direct_mapping() for inspiration (although that function does
> it the other way around. It allocates a virq and uses that number for
> the hwirq number)

Ok, I've implemented something according to your idea. Will be part of
the v3 of this series. For reference, the piece of code I've added in
irqdomain.c looks like:

 /**
+ * irq_alloc_mapping() - Allocate an irq for mapping
+ * @domain: domain to allocate the irq for or NULL for default domain
+ * @hwirq:  reference to the returned hwirq
+ *
+ * This routine are used for irq controllers which can choose the
+ * hardware interrupt number from a range [ 0 ; domain size ], such as
+ * is often the case with PCI MSI controllers. The function will
+ * returned the allocated hwirq number in the hwirq pointer, and the
+ * corresponding virq number as the return value.
+ */
+unsigned int irq_alloc_mapping(struct irq_domain *domain,
+                              irq_hw_number_t *out_hwirq)
+{
+       unsigned int virq;
+       irq_hw_number_t hwirq;
+
+       pr_debug("irq_alloc_mapping(0x%p)\n", domain);
+
+       if (domain == NULL)
+               domain = irq_default_domain;
+
+       for (hwirq = 0; hwirq < domain->hwirq_max; hwirq++)
+               if (!irq_find_mapping(domain, hwirq))
+                       break;
+
+       if (hwirq == domain->hwirq_max) {
+               pr_debug("-> no available hwirq found\n");
+               return 0;
+       }
+
+       virq = irq_alloc_desc_from(1, of_node_to_nid(domain->of_node));
+       if (virq <= 0) {
+               pr_debug("-> virq allocation failed\n");
+               return 0;
+       }
+
+       if (irq_domain_associate(domain, virq, hwirq)) {
+               irq_free_desc(virq);
+               return 0;
+       }
+
+       pr_debug("irq %lu on domain %s mapped to virtual irq %u\n",
+                hwirq, of_node_full_name(domain->of_node), virq);
+
+       *out_hwirq = hwirq;
+
+       return virq;
+}
+EXPORT_SYMBOL_GPL(irq_alloc_mapping);
+
+/**

> > +static int armada_370_xp_msi_init(struct device_node *node)
> > +{
> > +	struct msi_chip *msi_chip;
> > +	u32 reg;
> > +
> > +	msi_chip = kzalloc(sizeof(*msi_chip), GFP_KERNEL);
> 
> devm_kzalloc() so it gets freed on failure or unbind; although for this
> device you may not care.

Unfortunately, we don't have access to a platform_device by the time
the IRQ controller driver is initialized. Thierry Redding suggested to
use of_find_device_by_node(), but it returns NULL. I guess it's because
this IRQ controller driver is initialized much before the
platform_device structures are created.

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

[PATCH v2 4/8] irqchip: armada-370-xp: implement MSI support

[Grant Likely]

On Tue, 18 Jun 2013 10:42:18 +0200, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:
> Dear Grant Likely,
> 
> On Tue, 11 Jun 2013 14:37:45 +0100, Grant Likely wrote:
> 
> > > +static int armada_370_xp_setup_msi_irq(struct msi_chip *chip,
> > > +				       struct pci_dev *pdev,
> > > +				       struct msi_desc *desc)
> > > +{
> > > +	struct msi_msg msg;
> > > +	int hwirq, irq;
> > > +
> > > +	hwirq = armada_370_xp_alloc_msi();
> > > +	if (hwirq < 0)
> > > +		return hwirq;
> > > +
> > > +	irq = irq_create_mapping(armada_370_xp_msi_domain, hwirq);
> > > +	if (!irq) {
> > > +		armada_370_xp_free_msi(hwirq);
> > > +		return -EINVAL;
> > > +	}
> > 
> > This looks like something that the irq_domain should handle for you.
> > It would be good to have a variant of irq_create_mapping() that keeps
> > track of available hwirqs, allocates a free one, and maps it all with
> > one function call. I can see that being useful by other MSI interrupt
> > controllers and would eliminate needing to open-code it above.
> > 
> > take a look at the irqdomain/test branch on git://git.secretlab.ca/git/linux
> > 
> > I'm hoping to push that series into v3.11 and it simplifies the
> > irq_domain code quite a bit. It would be easy to build an
> > irq_create_mapping() variant on top of that. Take a look at
> > irq_create_direct_mapping() for inspiration (although that function does
> > it the other way around. It allocates a virq and uses that number for
> > the hwirq number)
> 
> Ok, I've implemented something according to your idea. Will be part of
> the v3 of this series. For reference, the piece of code I've added in
> irqdomain.c looks like:
> 
>  /**
> + * irq_alloc_mapping() - Allocate an irq for mapping
> + * @domain: domain to allocate the irq for or NULL for default domain
> + * @hwirq:  reference to the returned hwirq
> + *
> + * This routine are used for irq controllers which can choose the
> + * hardware interrupt number from a range [ 0 ; domain size ], such as
> + * is often the case with PCI MSI controllers. The function will
> + * returned the allocated hwirq number in the hwirq pointer, and the
> + * corresponding virq number as the return value.
> + */
> +unsigned int irq_alloc_mapping(struct irq_domain *domain,
> +                              irq_hw_number_t *out_hwirq)
> +{
> +       unsigned int virq;
> +       irq_hw_number_t hwirq;
> +
> +       pr_debug("irq_alloc_mapping(0x%p)\n", domain);
> +
> +       if (domain == NULL)
> +               domain = irq_default_domain;

Drop the above 2 lines. You absolutely must know what irq_domain you
want to operate on when calling this function. There is no situation
where the default domain is what should be used.

> +
> +       for (hwirq = 0; hwirq < domain->hwirq_max; hwirq++)
> +               if (!irq_find_mapping(domain, hwirq))
> +                       break;

Ugh. This will be slow on domains with a high hwirq_max and low
revmap_size since all the lookups will go out to the radix tree. Blech.
Not much to do about it though at this point without implementing some
kind of fast lookup path. To do it right would require iterating over
the radix tree looking for a hole.

> +
> +       if (hwirq == domain->hwirq_max) {
> +               pr_debug("-> no available hwirq found\n");
> +               return 0;
> +       }
> +
> +       virq = irq_alloc_desc_from(1, of_node_to_nid(domain->of_node));
> +       if (virq <= 0) {
> +               pr_debug("-> virq allocation failed\n");
> +               return 0;
> +       }
> +
> +       if (irq_domain_associate(domain, virq, hwirq)) {
> +               irq_free_desc(virq);
> +               return 0;
> +       }

Once a free hwirq has been found, it would be better to call
irq_create_mapping() directly rather than open coding it.

> +
> +       pr_debug("irq %lu on domain %s mapped to virtual irq %u\n",
> +                hwirq, of_node_full_name(domain->of_node), virq);
> +
> +       *out_hwirq = hwirq;
> +
> +       return virq;
> +}
> +EXPORT_SYMBOL_GPL(irq_alloc_mapping);
> +
> +/**

[PATCH v2 4/8] irqchip: armada-370-xp: implement MSI support

[Thomas Petazzoni]

Dear Grant Likely,

On Tue, 18 Jun 2013 11:15:38 +0100, Grant Likely wrote:

> > +       if (domain == NULL)
> > +               domain = irq_default_domain;
> 
> Drop the above 2 lines. You absolutely must know what irq_domain you
> want to operate on when calling this function. There is no situation
> where the default domain is what should be used.

Sure, makes sense.

> > +
> > +       for (hwirq = 0; hwirq < domain->hwirq_max; hwirq++)
> > +               if (!irq_find_mapping(domain, hwirq))
> > +                       break;
> 
> Ugh. This will be slow on domains with a high hwirq_max and low
> revmap_size since all the lookups will go out to the radix tree. Blech.
> Not much to do about it though at this point without implementing some
> kind of fast lookup path. To do it right would require iterating over
> the radix tree looking for a hole.

So to conclude you would leave it as I proposed for now?

An option is to make irq_alloc_mapping() work only on linear domains,
where hwirq_max == revmap_size, and return an error otherwise.

> Once a free hwirq has been found, it would be better to call
> irq_create_mapping() directly rather than open coding it.

Thanks, will do.

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

[PATCH v2 4/8] irqchip: armada-370-xp: implement MSI support

[Thierry Reding]

On Thu, Jun 06, 2013 at 06:41:24PM +0200, Thomas Petazzoni wrote:
[...]
> @@ -292,6 +454,8 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,
>  
>  #endif
>  
> +	armada_370_xp_msi_init(node);
> +

So I see that you don't have access to the original platform device
here, but you could use of_find_device_by_node() to obtain it and pass
that into armada_370_xp_msi_init() in order to set the msi_chip.dev
field. Or you could do the lookup in armada_370_xp_msi_init() if you
don't need it for anything else in armada_370_xp_mpic_of_init().

Doing the above will also allow you to use devm_kzalloc() as Grant
suggested in his reply.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130612/6af6e30d/attachment.sig>

[PATCH v2 4/8] irqchip: armada-370-xp: implement MSI support

[Thomas Petazzoni]

Dear Thierry Reding,

On Wed, 12 Jun 2013 12:42:40 +0200, Thierry Reding wrote:
> On Thu, Jun 06, 2013 at 06:41:24PM +0200, Thomas Petazzoni wrote:
> [...]
> > @@ -292,6 +454,8 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,
> >  
> >  #endif
> >  
> > +	armada_370_xp_msi_init(node);
> > +
> 
> So I see that you don't have access to the original platform device
> here, but you could use of_find_device_by_node() to obtain it and pass
> that into armada_370_xp_msi_init() in order to set the msi_chip.dev
> field. Or you could do the lookup in armada_370_xp_msi_init() if you
> don't need it for anything else in armada_370_xp_mpic_of_init().

As I replied to Grant, of_find_device_by_node() returns NULL, I believe
because the all IRQ controller driver initialization is done pretty
early, before the of_platform_populate() call is made, so there is no
platform_device associated with the IRQ controller node at the time the
armada_370_xp_mpic_of_init() function is called.

Do you see another approach, especially in relation to your comment on
PATCH 2/8 ?

Best regards,

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

[PATCH v2 4/8] irqchip: armada-370-xp: implement MSI support

[Thierry Reding]

On Tue, Jun 18, 2013 at 10:43:38AM +0200, Thomas Petazzoni wrote:
> Dear Thierry Reding,
> 
> On Wed, 12 Jun 2013 12:42:40 +0200, Thierry Reding wrote:
> > On Thu, Jun 06, 2013 at 06:41:24PM +0200, Thomas Petazzoni wrote:
> > [...]
> > > @@ -292,6 +454,8 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,
> > >  
> > >  #endif
> > >  
> > > +	armada_370_xp_msi_init(node);
> > > +
> > 
> > So I see that you don't have access to the original platform device
> > here, but you could use of_find_device_by_node() to obtain it and pass
> > that into armada_370_xp_msi_init() in order to set the msi_chip.dev
> > field. Or you could do the lookup in armada_370_xp_msi_init() if you
> > don't need it for anything else in armada_370_xp_mpic_of_init().
> 
> As I replied to Grant, of_find_device_by_node() returns NULL, I believe
> because the all IRQ controller driver initialization is done pretty
> early, before the of_platform_populate() call is made, so there is no
> platform_device associated with the IRQ controller node at the time the
> armada_370_xp_mpic_of_init() function is called.
> 
> Do you see another approach, especially in relation to your comment on
> PATCH 2/8 ?

Hmmm, that's too bad. The only other possibility that I see is that you
could associate the struct device at a later point when it becomes
available, but looking at the irqchip driver it doesn't look like you
get notification of that either. I suppose you could add a
platform_driver to it and hook things up in its .probe() callback, but
I'm not sure if that's in line with how the irqchip was designed. Adding
Grant Likely and Thomas Gleixner on Cc, maybe they have better advice.

Looking at other irqchip drivers I find it a bit odd to see how they're
structured, though. We've been preaching for years that drivers should
be well-encapsulated and told everybody it was bad to use globals and
they should be associating driver-specific data with each instance of a
device. Then comes along irqchip and all of a sudden it's okay to use
globals again. It feels a bit fragile.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130618/51731f1c/attachment.sig>

[PATCH v2 4/8] irqchip: armada-370-xp: implement MSI support

[Thomas Petazzoni]

Dear Thierry Reding,

On Tue, 18 Jun 2013 13:26:04 +0200, Thierry Reding wrote:

> > As I replied to Grant, of_find_device_by_node() returns NULL, I believe
> > because the all IRQ controller driver initialization is done pretty
> > early, before the of_platform_populate() call is made, so there is no
> > platform_device associated with the IRQ controller node at the time the
> > armada_370_xp_mpic_of_init() function is called.
> > 
> > Do you see another approach, especially in relation to your comment on
> > PATCH 2/8 ?
> 
> Hmmm, that's too bad. The only other possibility that I see is that you
> could associate the struct device at a later point when it becomes
> available, but looking at the irqchip driver it doesn't look like you
> get notification of that either. I suppose you could add a
> platform_driver to it and hook things up in its .probe() callback, but
> I'm not sure if that's in line with how the irqchip was designed. Adding
> Grant Likely and Thomas Gleixner on Cc, maybe they have better advice.

If we do hook the MSI stuff in a ->probe() callback, then we'd have a
dependency between the ->probe() of the PCIe driver and the ->probe()
of the IRQ controller driver. In order for the PCIe ->probe() to
succeed, it needs the MSI controller to be registered, which wouldn't
appear until the ->probe() of the IRQ controller driver gets called. A
typical case of platform device dependency where the two platform
devices don't have a bus -> child dependency. Could be handled by a
-EPROBE_DEFER trick, though.

> Looking at other irqchip drivers I find it a bit odd to see how they're
> structured, though. We've been preaching for years that drivers should
> be well-encapsulated and told everybody it was bad to use globals and
> they should be associating driver-specific data with each instance of a
> device. Then comes along irqchip and all of a sudden it's okay to use
> globals again. It feels a bit fragile.

Yes, I've seen this as well, and I'm thinking of doing some
improvements in this area if there's some interest. But I believe this
is fairly separate from the specific discussion of this patch set.

Best regards,

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

[PATCH v2 5/8] arm: mvebu: the MPIC now provides MSI controller features

[Thomas Petazzoni]

Adds the 'msi-controller' property to the main interrupt controller
Device Tree node, to indicate that it can now behave as a MSI
controller.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-xp.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index bb6224c..9020478 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -52,6 +52,7 @@
 				#interrupt-cells = <1>;
 				#size-cells = <1>;
 				interrupt-controller;
+				msi-controller;
 			};
 
 			coherency-fabric at 20200 {
-- 
1.8.1.2

[PATCH v2 6/8] pci: mvebu: add support for MSI

[Thomas Petazzoni]

This commit adds support for Message Signaled Interrupts in the
Marvell PCIe host controller. The work is very simple: it simply gets
a reference to the msi_chip associated to the PCIe controller thanks
to the msi-parent DT property, and stores this reference in the
pci_bus structure. This is enough to let the Linux PCI core use the
functions of msi_chip to setup and teardown MSIs.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/pci/host/pci-mvebu.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 13a633b..0dc30bd 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -11,6 +11,7 @@
 #include <linux/clk.h>
 #include <linux/module.h>
 #include <linux/mbus.h>
+#include <linux/msi.h>
 #include <linux/slab.h>
 #include <linux/platform_device.h>
 #include <linux/of_address.h>
@@ -107,6 +108,7 @@ struct mvebu_pcie_port;
 struct mvebu_pcie {
 	struct platform_device *pdev;
 	struct mvebu_pcie_port *ports;
+	struct msi_chip *msi;
 	struct resource io;
 	struct resource realio;
 	struct resource mem;
@@ -690,6 +692,8 @@ static struct pci_bus *mvebu_pcie_scan_bus(int nr, struct pci_sys_data *sys)
 	if (!bus)
 		return NULL;
 
+	bus->msi = pcie->msi;
+
 	pci_scan_child_bus(bus);
 
 	return bus;
@@ -755,6 +759,21 @@ mvebu_pcie_map_registers(struct platform_device *pdev,
 	return devm_request_and_ioremap(&pdev->dev, &regs);
 }
 
+static void __init mvebu_pcie_msi_enable(struct mvebu_pcie *pcie)
+{
+	struct device_node *msi_node;
+
+	msi_node = of_parse_phandle(pcie->pdev->dev.of_node,
+				    "msi-parent", 0);
+	if (!msi_node)
+		return;
+
+	pcie->msi = msi_chip_find_by_of_node(msi_node);
+
+	if (pcie->msi)
+		pcie->msi->dev = &pcie->pdev->dev;
+}
+
 static int __init mvebu_pcie_probe(struct platform_device *pdev)
 {
 	struct mvebu_pcie *pcie;
@@ -879,6 +898,8 @@ static int __init mvebu_pcie_probe(struct platform_device *pdev)
 		i++;
 	}
 
+	mvebu_pcie_msi_enable(pcie);
+
 	mvebu_pcie_enable(pcie);
 
 	return 0;
-- 
1.8.1.2

[PATCH v2 6/8] pci: mvebu: add support for MSI

[Bjorn Helgaas]

On Thu, Jun 06, 2013 at 06:41:26PM +0200, Thomas Petazzoni wrote:
> This commit adds support for Message Signaled Interrupts in the
> Marvell PCIe host controller. The work is very simple: it simply gets
> a reference to the msi_chip associated to the PCIe controller thanks
> to the msi-parent DT property, and stores this reference in the
> pci_bus structure. This is enough to let the Linux PCI core use the
> functions of msi_chip to setup and teardown MSIs.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/pci/host/pci-mvebu.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index 13a633b..0dc30bd 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -11,6 +11,7 @@
>  #include <linux/clk.h>
>  #include <linux/module.h>
>  #include <linux/mbus.h>
> +#include <linux/msi.h>
>  #include <linux/slab.h>
>  #include <linux/platform_device.h>
>  #include <linux/of_address.h>
> @@ -107,6 +108,7 @@ struct mvebu_pcie_port;
>  struct mvebu_pcie {
>  	struct platform_device *pdev;
>  	struct mvebu_pcie_port *ports;
> +	struct msi_chip *msi;
>  	struct resource io;
>  	struct resource realio;
>  	struct resource mem;
> @@ -690,6 +692,8 @@ static struct pci_bus *mvebu_pcie_scan_bus(int nr, struct pci_sys_data *sys)
>  	if (!bus)
>  		return NULL;
>  
> +	bus->msi = pcie->msi;

Ideally, arches could use pci_scan_root_bus(), which does the
pci_scan_child_bus() itself, so I never like to add arch-specific code
between pci_create_root_bus() and pci_scan_child_bus().

But you should be able to accomplish this by setting bus->msi in a
pcibios_add_bus() hook.  There is a corresponding pcibios_remove_bus() hook
that might be useful for deallocating the msi_chip.  I'm not too clear on
how you manage the msi_chip lifetime -- I see the alloc in
armada_370_xp_msi_init(), but I'm not sure whether you ever deallocate it
or how you know when it would be safe to do so.

>  	pci_scan_child_bus(bus);
>  
>  	return bus;
> @@ -755,6 +759,21 @@ mvebu_pcie_map_registers(struct platform_device *pdev,
>  	return devm_request_and_ioremap(&pdev->dev, &regs);
>  }
>  
> +static void __init mvebu_pcie_msi_enable(struct mvebu_pcie *pcie)
> +{
> +	struct device_node *msi_node;
> +
> +	msi_node = of_parse_phandle(pcie->pdev->dev.of_node,
> +				    "msi-parent", 0);
> +	if (!msi_node)
> +		return;
> +
> +	pcie->msi = msi_chip_find_by_of_node(msi_node);
> +
> +	if (pcie->msi)
> +		pcie->msi->dev = &pcie->pdev->dev;
> +}
> +
>  static int __init mvebu_pcie_probe(struct platform_device *pdev)
>  {
>  	struct mvebu_pcie *pcie;
> @@ -879,6 +898,8 @@ static int __init mvebu_pcie_probe(struct platform_device *pdev)
>  		i++;
>  	}
>  
> +	mvebu_pcie_msi_enable(pcie);
> +
>  	mvebu_pcie_enable(pcie);
>  
>  	return 0;
> -- 
> 1.8.1.2
> 

[PATCH v2 7/8] arm: mvebu: indicate that this platform supports MSI

[Thomas Petazzoni]

Now that all the pieces are in place, we can adjust the
mach-mvebu/Kconfig file to indicate that this platform supports MSIs.

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

diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
index 9eb63d7..36951b1 100644
--- a/arch/arm/mach-mvebu/Kconfig
+++ b/arch/arm/mach-mvebu/Kconfig
@@ -15,6 +15,7 @@ config ARCH_MVEBU
 	select ARCH_REQUIRE_GPIOLIB
 	select MIGHT_HAVE_PCI
 	select PCI_QUIRKS if PCI
+	select ARCH_SUPPORTS_MSI if PCI
 
 if ARCH_MVEBU
 
-- 
1.8.1.2

[PATCH v2 8/8] arm: mvebu: link PCIe controllers to the MSI controller

[Thomas Petazzoni]

This commit adjusts the Armada 370 and Armada XP PCIe controllers
Device Tree informations to reference their MSI controller. In the
case of this platform, the MSI controller is implemented by the MPIC.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/boot/dts/armada-370.dtsi        | 1 +
 arch/arm/boot/dts/armada-xp-mv78230.dtsi | 1 +
 arch/arm/boot/dts/armada-xp-mv78260.dtsi | 1 +
 arch/arm/boot/dts/armada-xp-mv78460.dtsi | 1 +
 4 files changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index aee2b18..3c4e6d7 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -178,6 +178,7 @@
 				#address-cells = <3>;
 				#size-cells = <2>;
 
+				msi-parent = <&mpic>;
 				bus-range = <0x00 0xff>;
 
 				reg = <0x40000 0x2000>, <0x80000 0x2000>;
diff --git a/arch/arm/boot/dts/armada-xp-mv78230.dtsi b/arch/arm/boot/dts/armada-xp-mv78230.dtsi
index f8eaa38..4a9a305 100644
--- a/arch/arm/boot/dts/armada-xp-mv78230.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78230.dtsi
@@ -91,6 +91,7 @@
 #address-cells = <3>;
 #size-cells = <2>;
 
+				msi-parent = <&mpic>;
 				bus-range = <0x00 0xff>;
 
 				ranges = <0x82000000 0 0x40000 0x40000 0 0x00002000   /* Port 0.0 registers */
diff --git a/arch/arm/boot/dts/armada-xp-mv78260.dtsi b/arch/arm/boot/dts/armada-xp-mv78260.dtsi
index f4029f0..bc63783 100644
--- a/arch/arm/boot/dts/armada-xp-mv78260.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78260.dtsi
@@ -111,6 +111,7 @@
 				#address-cells = <3>;
 				#size-cells = <2>;
 
+				msi-parent = <&mpic>;
 				bus-range = <0x00 0xff>;
 
 				ranges = <0x82000000 0 0x40000 0x40000 0 0x00002000   /* Port 0.0 registers */
diff --git a/arch/arm/boot/dts/armada-xp-mv78460.dtsi b/arch/arm/boot/dts/armada-xp-mv78460.dtsi
index c7b1f4d..77db2bc 100644
--- a/arch/arm/boot/dts/armada-xp-mv78460.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78460.dtsi
@@ -127,6 +127,7 @@
 				#address-cells = <3>;
 				#size-cells = <2>;
 
+				msi-parent = <&mpic>;
 				bus-range = <0x00 0xff>;
 
 				ranges = <0x82000000 0 0x40000 0x40000 0 0x00002000   /* Port 0.0 registers */
-- 
1.8.1.2

[PATCH v2 0/8] MSI support for Marvell EBU PCIe driver

[Jason Cooper]

On Thu, Jun 06, 2013 at 06:41:20PM +0200, Thomas Petazzoni wrote:
> This set of patches currently applies on top of the current "for-next"
> branch of Jason Cooper's repository.

Please be aware that for-next is *not* stable.  I rebuild it every time
I pull new stuff in.  It's much more helpful to list only the branches
you need to build/test.

In this case I imagine it's mvebu/pcie...

thx,

Jason.

[PATCH v2 0/8] MSI support for Marvell EBU PCIe driver

[Thomas Petazzoni]

Dear Jason Cooper,

On Thu, 6 Jun 2013 13:17:47 -0400, Jason Cooper wrote:

> Please be aware that for-next is *not* stable.  I rebuild it every time
> I pull new stuff in.  It's much more helpful to list only the branches
> you need to build/test.

Yes, I'm aware that for-next is not stable. Since the previous
iteration of this patch series generated a lot of discussion, I was
assuming there would be additional discussions on this new iteration,
and that it would certainly not be the final version. So for ease of
work, I've just based it on the branch that 'contains everything'.

For sure, if I get the proper ACKs and acceptance of the general idea,
I'll go ahead and send a proper version based on the correct branches.

You suggested mvebu/pcie as a base, but it means I also need to merge
mvebu/of_pci before that, and maybe also mvebu/pcie_bridge to have the
latest changes in the PCIe driver, etc. Which is why I just went the
easy/lazy way and did everything based on for-next :)

Best regards,

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

[PATCH v2 0/8] MSI support for Marvell EBU PCIe driver

[Jason Cooper]

On Fri, Jun 07, 2013 at 10:14:29AM +0200, Thomas Petazzoni wrote:
> Dear Jason Cooper,
> 
> On Thu, 6 Jun 2013 13:17:47 -0400, Jason Cooper wrote:
> 
> > Please be aware that for-next is *not* stable.  I rebuild it every time
> > I pull new stuff in.  It's much more helpful to list only the branches
> > you need to build/test.
> 
> Yes, I'm aware that for-next is not stable. Since the previous
> iteration of this patch series generated a lot of discussion, I was
> assuming there would be additional discussions on this new iteration,
> and that it would certainly not be the final version. So for ease of
> work, I've just based it on the branch that 'contains everything'.

Ahh, ok, I've done the same.  Thanks for the clarification.

thx,

Jason.

[PATCH v2 0/8] MSI support for Marvell EBU PCIe driver

[Jason Cooper]

Thomas,

On Thu, Jun 06, 2013 at 06:41:20PM +0200, Thomas Petazzoni wrote:
> Hello,
> 
> This set of patches introduces Message Signaled Interrupt support in
> the Marvell EBU PCIe driver. It has been successfully tested on the
> Armada XP GP platform with an Intel e1000e PCIe network card that
> supports MSI.
> 
> This version 2 follows the RFC version sent on March, 26th 2013. This
> is based on work done by Lior Amsalem <alior@marvell.com>.
> 
> The patches do the following:
> 
>  * Patch 1 comes from Thierry Redding, and adds a minimal msi_chip
>    infrastructure.
> 
>    This needs to be reviewed/acked by Bjorn Helgaas.
> 
>  * Patch 2 extends the msi_chip infrastructure with a small registry,
>    so that one driver can register an msi_chip, and another driver can
>    find the msi_chip associated to a particular Device Tree node.
> 
>    This needs to be reviewed/acked by Bjorn Helgaas.
> 
>  * Patch 3 makes some not very interesting preparation in the Armada
>    370/XP IRQ controller driver.
> 
>  * Patch 4 implements the MSI support in the Armada 370/XP IRQ
>    controller driver. It registers an msi_chip using the
>    msi_chip_add() function added in PATCH 2.
> 
>  * Patch 5 adjust the Armada 370/XP Device Tree to indicate that the
>    MPIC is not only an interrupt-controller, but also an
>    msi-controller.
> 
>  * Patch 6 adds MSI support in the Marvell PCIe host controller
>    driver. The work to do here is minimal: get a reference to the
>    msi-parent controller thanks to msi_chip_find_by_of_node(), and
>    link it to the pci_bus structure before the bus gets enumerated.
> 
>    This needs to be reviewed/acked by Bjorn Helgaas.
> 
>  * Patch 7 tunes Kconfig to indicate that Armada 370/XP supports MSI.
> 
>  * Patch 8 adjusts the Armada 370/XP Device Tree to add the msi-parent
>    properties in the PCIe controller nodes.
> 
> This set of patches currently applies on top of the current "for-next"
> branch of Jason Cooper's repository.
> 
> The Device Tree binding documentation updates will be added once the
> general approach is agreed on.
> 
> Thanks,
> 
> Thomas
> 
> Thierry Reding (1):
>   PCI: Introduce new MSI chip infrastructure
> 
> Thomas Petazzoni (7):
>   PCI: Add registry of MSI chips
>   irqchip: armada-370-xp: properly request resources
>   irqchip: armada-370-xp: implement MSI support
>   arm: mvebu: the MPIC now provides MSI controller features
>   pci: mvebu: add support for MSI
>   arm: mvebu: indicate that this platform supports MSI
>   arm: mvebu: link PCIe controllers to the MSI controller
> 

Is there a reason why the following breakdown wouldn't work?

>  arch/arm/boot/dts/armada-370-xp.dtsi     |   1 +
>  arch/arm/boot/dts/armada-370.dtsi        |   1 +
>  arch/arm/boot/dts/armada-xp-mv78230.dtsi |   1 +
>  arch/arm/boot/dts/armada-xp-mv78260.dtsi |   1 +
>  arch/arm/boot/dts/armada-xp-mv78460.dtsi |   1 +

through mvebu/arm-soc

>  arch/arm/mach-mvebu/Kconfig              |   1 +

through mvebu/arm-soc after the other three have landed (v3.11-rc1)

>  drivers/irqchip/irq-armada-370-xp.c      | 186 ++++++++++++++++++++++++++++++-

through tglx

>  drivers/pci/host/pci-mvebu.c             |  21 ++++
>  drivers/pci/msi.c                        |  59 +++++++++-
>  drivers/pci/probe.c                      |   1 +
>  include/linux/msi.h                      |  22 ++++
>  include/linux/pci.h                      |   1 +

through Bjorn

I think we should view the manner in which we brought in the initial
mvebu-pcie series (all through arm-soc) as the exception, not the rule.
I have no problem, and in fact, prefer to have them reviewed as a
series, but if at *all* possible, the series should be structured so
relevant maintainers can pick up the relevant patches into their trees.

thx,

Jason.

[PATCH v2 0/8] MSI support for Marvell EBU PCIe driver

[Thomas Petazzoni]

Dear Jason Cooper,

On Thu, 6 Jun 2013 14:51:10 -0400, Jason Cooper wrote:
> > Thierry Reding (1):
> >   PCI: Introduce new MSI chip infrastructure
> > 
> > Thomas Petazzoni (7):
> >   PCI: Add registry of MSI chips
> >   irqchip: armada-370-xp: properly request resources
> >   irqchip: armada-370-xp: implement MSI support
> >   arm: mvebu: the MPIC now provides MSI controller features
> >   pci: mvebu: add support for MSI
> >   arm: mvebu: indicate that this platform supports MSI
> >   arm: mvebu: link PCIe controllers to the MSI controller
> > 
> 
> Is there a reason why the following breakdown wouldn't work?

No, it should work. And it's actually possible with how the patches are
organized currently.

> >  arch/arm/boot/dts/armada-370-xp.dtsi     |   1 +
> >  arch/arm/boot/dts/armada-370.dtsi        |   1 +
> >  arch/arm/boot/dts/armada-xp-mv78230.dtsi |   1 +
> >  arch/arm/boot/dts/armada-xp-mv78260.dtsi |   1 +
> >  arch/arm/boot/dts/armada-xp-mv78460.dtsi |   1 +
> 
> through mvebu/arm-soc

Patches 5 and 8.

> 
> >  arch/arm/mach-mvebu/Kconfig              |   1 +
> 
> through mvebu/arm-soc after the other three have landed (v3.11-rc1)

Patch 7.

> >  drivers/irqchip/irq-armada-370-xp.c      | 186 ++++++++++++++++++++++++++++++-
> 
> through tglx

Patches 3 and 4.

> 
> >  drivers/pci/host/pci-mvebu.c             |  21 ++++
> >  drivers/pci/msi.c                        |  59 +++++++++-
> >  drivers/pci/probe.c                      |   1 +
> >  include/linux/msi.h                      |  22 ++++
> >  include/linux/pci.h                      |   1 +
> 
> through Bjorn

Patches 1, 2 and 6.

So as you can see, the patches are already broken down in a way that
allows each maintainer to pick its part. But I admit I could probably
reorder them in the following way:

 1. PCI: Introduce new MSI chip infrastructure
 2. PCI: Add registry of MSI chips
 3. pci: mvebu: add support for MSI
 4. irqchip: armada-370-xp: properly request resources
 5. irqchip: armada-370-xp: implement MSI support
 6. arm: mvebu: the MPIC now provides MSI controller features
 7. arm: mvebu: link PCIe controllers to the MSI controller
 8. arm: mvebu: indicate that this platform supports MSI

1-3 through Bjorn, 4-5 through tglx, 6-8 through you.

The only problem that I see is that 'irqchip: armada-370-xp: implement
MSI support' (which goes through tglx) has a build dependency on 'PCI:
Add registry of MSI chips' (which goes through Bjorn). This is due to
the IRQ controller using the msi_chip_add() function that is introduced
earlier in the PCI core.

How would you solve that?

In any case, at this point I'd like first to have the ACK from Bjorn
Helgaas and Arnd Bergmann on the general approach.

Best regards, 

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

[PATCH v2 0/8] MSI support for Marvell EBU PCIe driver

[Jason Cooper]

On Fri, Jun 07, 2013 at 10:23:04AM +0200, Thomas Petazzoni wrote:
> Dear Jason Cooper,
> 
> On Thu, 6 Jun 2013 14:51:10 -0400, Jason Cooper wrote:
> > > Thierry Reding (1):
> > >   PCI: Introduce new MSI chip infrastructure
> > > 
> > > Thomas Petazzoni (7):
> > >   PCI: Add registry of MSI chips
> > >   irqchip: armada-370-xp: properly request resources
> > >   irqchip: armada-370-xp: implement MSI support
> > >   arm: mvebu: the MPIC now provides MSI controller features
> > >   pci: mvebu: add support for MSI
> > >   arm: mvebu: indicate that this platform supports MSI
> > >   arm: mvebu: link PCIe controllers to the MSI controller
> > > 
> > 
> > Is there a reason why the following breakdown wouldn't work?
> 
> No, it should work. And it's actually possible with how the patches are
> organized currently.

Ok.  good.

> > >  arch/arm/boot/dts/armada-370-xp.dtsi     |   1 +
> > >  arch/arm/boot/dts/armada-370.dtsi        |   1 +
> > >  arch/arm/boot/dts/armada-xp-mv78230.dtsi |   1 +
> > >  arch/arm/boot/dts/armada-xp-mv78260.dtsi |   1 +
> > >  arch/arm/boot/dts/armada-xp-mv78460.dtsi |   1 +
> > 
> > through mvebu/arm-soc
> 
> Patches 5 and 8.
> 
> > 
> > >  arch/arm/mach-mvebu/Kconfig              |   1 +
> > 
> > through mvebu/arm-soc after the other three have landed (v3.11-rc1)
> 
> Patch 7.
> 
> > >  drivers/irqchip/irq-armada-370-xp.c      | 186 ++++++++++++++++++++++++++++++-
> > 
> > through tglx
> 
> Patches 3 and 4.
> 
> > 
> > >  drivers/pci/host/pci-mvebu.c             |  21 ++++
> > >  drivers/pci/msi.c                        |  59 +++++++++-
> > >  drivers/pci/probe.c                      |   1 +
> > >  include/linux/msi.h                      |  22 ++++
> > >  include/linux/pci.h                      |   1 +
> > 
> > through Bjorn
> 
> Patches 1, 2 and 6.
> 
> So as you can see, the patches are already broken down in a way that
> allows each maintainer to pick its part. But I admit I could probably
> reorder them in the following way:
> 
>  1. PCI: Introduce new MSI chip infrastructure
>  2. PCI: Add registry of MSI chips
>  3. pci: mvebu: add support for MSI
>  4. irqchip: armada-370-xp: properly request resources
>  5. irqchip: armada-370-xp: implement MSI support
>  6. arm: mvebu: the MPIC now provides MSI controller features
>  7. arm: mvebu: link PCIe controllers to the MSI controller
>  8. arm: mvebu: indicate that this platform supports MSI
> 
> 1-3 through Bjorn, 4-5 through tglx, 6-8 through you.
> 
> The only problem that I see is that 'irqchip: armada-370-xp: implement
> MSI support' (which goes through tglx) has a build dependency on 'PCI:
> Add registry of MSI chips' (which goes through Bjorn). This is due to
> the IRQ controller using the msi_chip_add() function that is introduced
> earlier in the PCI core.

Yeah, that's what I was afraid of...

> How would you solve that?

Let me take a closer look at it later today or this weekend.  I just
wanted to make sure that we always defer to the appropriate maintainers
first, and merge all through arm-soc as a last resort.

> In any case, at this point I'd like first to have the ACK from Bjorn
> Helgaas and Arnd Bergmann on the general approach.

Of course.

thx,

Jason.

[PATCH v2 0/8] MSI support for Marvell EBU PCIe driver

[Thomas Petazzoni]

Dear Jason Cooper,

On Fri, 7 Jun 2013 11:08:14 -0400, Jason Cooper wrote:

> > 1-3 through Bjorn, 4-5 through tglx, 6-8 through you.
> > 
> > The only problem that I see is that 'irqchip: armada-370-xp: implement
> > MSI support' (which goes through tglx) has a build dependency on 'PCI:
> > Add registry of MSI chips' (which goes through Bjorn). This is due to
> > the IRQ controller using the msi_chip_add() function that is introduced
> > earlier in the PCI core.
> 
> Yeah, that's what I was afraid of...
> 
> > How would you solve that?
> 
> Let me take a closer look at it later today or this weekend.  I just
> wanted to make sure that we always defer to the appropriate maintainers
> first, and merge all through arm-soc as a last resort.

Now that I think of it, there is no problem in fact. Until you apply the
patch 'arm: mvebu: indicate that this platform supports MSI', which
modifies Kconfig to say that mvebu supports MSI, there is no way for
the user to enable CONFIG_PCI_MSI.

And since in the IRQ controller driver the MSI code is inside a #ifdef
CONFIG_PCI_MSI, this driver will build perfectly fine, as long as 'arm:
mvebu: indicate that this platform supports MSI' is not merged before
'PCI: Add registry of MSI chips'.

So we can perfectly let Bjorn take the three PCI patches, tglx take the
two IRQ controller patches, and you take the ARM stuff. The only thing
is that the ARM stuff should not surface until both the PCI patches and
the IRQ controller patches have landed in mainline.

But well, before even thinking of merging all this, I'd like to hear
from Bjorn about what he thinks of those patches.

Thanks!

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

[PATCH v2 0/8] MSI support for Marvell EBU PCIe driver

[Thomas Petazzoni]

Bjorn,

On Thu,  6 Jun 2013 18:41:20 +0200, Thomas Petazzoni wrote:

> This set of patches introduces Message Signaled Interrupt support in
> the Marvell EBU PCIe driver. It has been successfully tested on the
> Armada XP GP platform with an Intel e1000e PCIe network card that
> supports MSI.
> 
> This version 2 follows the RFC version sent on March, 26th 2013. This
> is based on work done by Lior Amsalem <alior@marvell.com>.
> 
> The patches do the following:
> 
>  * Patch 1 comes from Thierry Redding, and adds a minimal msi_chip
>    infrastructure.
> 
>    This needs to be reviewed/acked by Bjorn Helgaas.
> 
>  * Patch 2 extends the msi_chip infrastructure with a small registry,
>    so that one driver can register an msi_chip, and another driver can
>    find the msi_chip associated to a particular Device Tree node.
> 
>    This needs to be reviewed/acked by Bjorn Helgaas.

Could you have a look at those two patches and let me know what you
think?

Thanks,

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