[RFC 0/2] PCI: Introduce MSI chip infrastructure

[RFC 0/2] PCI: Introduce MSI chip infrastructure

[Thierry Reding]

This pair of patches introduces a new MSI chip infrastructure aimed at
replacing the current per-architecture implementation of the MSI support
functions. The rationale being that for multi-platform support on ARM
(and of course other architectures as well) per-architecture hooks no
longer work, given that more than one implementation will be provided.

The approach chosen in this patch series is to introduce a new structure
called struct msi_chip that contains pointers to the functions that have
previously been implemented on a per-architecture basis. A pointer to
this structure is embedded with each struct pci_bus, similar to struct
pci_ops, and automatically passed to child busses during enumeration. It
is the responsibility of the PCI host bridge driver to setup the MSI
chip for the root bus.

An example for the usage of this new infrastructure is provided in the
second patch, which implements this for the Tegra PCIe controller
driver. Note that this driver has not been merged yet because this MSI
infrastructure is a prerequisite. Nevertheless it should serve as a
reference for implementors on how to use the MSI chip.

Thierry

Thierry Reding (2):
  PCI: Introduce new MSI chip infrastructure
  PCI: tegra: Use new MSI chip infrastructure

 drivers/pci/host/pci-tegra.c | 105 ++++++++++++++++++++++++-------------------
 drivers/pci/msi.c            |  35 +++++++++++++--
 drivers/pci/probe.c          |   1 +
 include/linux/msi.h          |  10 +++++
 include/linux/pci.h          |   1 +
 5 files changed, 103 insertions(+), 49 deletions(-)

-- 
1.8.1.5

[RFC 1/2] PCI: Introduce new MSI chip infrastructure

[Thierry Reding]

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>
---
TODO:
- remove arch_msi_check_device (and other arch-specificities)
---
 drivers/pci/msi.c   | 35 +++++++++++++++++++++++++++++++----
 drivers/pci/probe.c |  1 +
 include/linux/msi.h | 10 ++++++++++
 include/linux/pci.h |  1 +
 4 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 00cc78c..fce3549 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -26,14 +26,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 b494066..9307550 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 ce93a34..ea4a5be 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -58,5 +58,15 @@ extern int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);
 extern void arch_teardown_msi_irqs(struct pci_dev *dev);
 extern 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 2461033a..6aca43ea 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -416,6 +416,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.5

[RFC 2/2] PCI: tegra: Use new MSI chip infrastructure

[Thierry Reding]

Implement an MSI chip that uses the Tegra PCIe controller's built-in
support to provide MSI services to the root bus and its children.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 drivers/pci/host/pci-tegra.c | 105 ++++++++++++++++++++++++-------------------
 1 file changed, 60 insertions(+), 45 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 1efd746..19c250f 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -183,14 +183,20 @@
 #define  PADS_PLL_CTL_TXCLKREF_DIV10		(0 << 20)
 #define  PADS_PLL_CTL_TXCLKREF_DIV5		(1 << 20)
 
-struct tegra_pcie_msi {
+struct tegra_msi {
 	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
 	struct irq_domain *domain;
+	struct msi_chip chip;
 	unsigned long pages;
 	struct mutex lock;
 	int irq;
 };
 
+static inline struct tegra_msi *to_tegra_msi(struct msi_chip *chip)
+{
+	return container_of(chip, struct tegra_msi, chip);
+}
+
 struct tegra_pcie {
 	struct device *dev;
 
@@ -211,7 +217,7 @@ struct tegra_pcie {
 	struct clk *pcie_xclk;
 	struct clk *pll_e;
 
-	struct tegra_pcie_msi msi;
+	struct tegra_msi msi;
 
 	struct list_head ports;
 	unsigned int num_ports;
@@ -605,6 +611,9 @@ static struct pci_bus *tegra_pcie_scan_bus(int nr, struct pci_sys_data *sys)
 	if (!bus)
 		return NULL;
 
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		bus->msi = &pcie->msi.chip;
+
 	pci_scan_child_bus(bus);
 
 	return bus;
@@ -1001,38 +1010,41 @@ static int tegra_pcie_put_resources(struct tegra_pcie *pcie)
 	return 0;
 }
 
-static int tegra_pcie_msi_alloc(struct tegra_pcie *pcie)
+static int tegra_msi_alloc(struct tegra_msi *chip)
 {
 	int msi;
 
-	mutex_lock(&pcie->msi.lock);
+	mutex_lock(&chip->lock);
 
-	msi = find_first_zero_bit(pcie->msi.used, INT_PCI_MSI_NR);
+	msi = find_first_zero_bit(chip->used, INT_PCI_MSI_NR);
 	if (msi < INT_PCI_MSI_NR)
-		set_bit(msi, pcie->msi.used);
+		set_bit(msi, chip->used);
 	else
 		msi = -ENOSPC;
 
-	mutex_unlock(&pcie->msi.lock);
+	mutex_unlock(&chip->lock);
 
 	return msi;
 }
 
-static void tegra_pcie_msi_free(struct tegra_pcie *pcie, unsigned long irq)
+static void tegra_msi_free(struct tegra_msi *chip, unsigned long irq)
 {
-	mutex_lock(&pcie->msi.lock);
+	struct device *dev = chip->chip.dev;
+
+	mutex_lock(&chip->lock);
 
-	if (!test_bit(irq, pcie->msi.used))
-		dev_err(pcie->dev, "trying to free unused MSI#%lu\n", irq);
+	if (!test_bit(irq, chip->used))
+		dev_err(dev, "trying to free unused MSI#%lu\n", irq);
 	else
-		clear_bit(irq, pcie->msi.used);
+		clear_bit(irq, chip->used);
 
-	mutex_unlock(&pcie->msi.lock);
+	mutex_unlock(&chip->lock);
 }
 
 static irqreturn_t tegra_pcie_msi_irq(int irq, void *data)
 {
 	struct tegra_pcie *pcie = data;
+	struct tegra_msi *msi = &pcie->msi;
 	unsigned int i;
 
 	for (i = 0; i < 8; i++) {
@@ -1046,9 +1058,9 @@ static irqreturn_t tegra_pcie_msi_irq(int irq, void *data)
 			/* clear the interrupt */
 			afi_writel(pcie, 1 << offset, AFI_MSI_VEC0 + i * 4);
 
-			irq = irq_find_mapping(pcie->msi.domain, index);
+			irq = irq_find_mapping(msi->domain, index);
 			if (irq) {
-				if (test_bit(index, pcie->msi.used))
+				if (test_bit(index, msi->used))
 					generic_handle_irq(irq);
 				else
 					dev_info(pcie->dev, "unhandled MSI\n");
@@ -1068,20 +1080,20 @@ static irqreturn_t tegra_pcie_msi_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-#ifdef CONFIG_PCI_MSI
-/* called by arch_setup_msi_irqs in drivers/pci/msi.c */
-int arch_setup_msi_irq(struct pci_dev *pdev, struct msi_desc *desc)
+static int tegra_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev,
+			       struct msi_desc *desc)
 {
-	struct tegra_pcie *pcie = sys_to_pcie(pdev->bus->sysdata);
+	struct tegra_msi *msi = to_tegra_msi(chip);
+	struct tegra_pcie *pcie = container_of(chip, struct tegra_pcie, msi.chip);
 	struct msi_msg msg;
 	unsigned int irq;
 	int hwirq;
 
-	hwirq = tegra_pcie_msi_alloc(pcie);
+	hwirq = tegra_msi_alloc(msi);
 	if (hwirq < 0)
 		return hwirq;
 
-	irq = irq_create_mapping(pcie->msi.domain, hwirq);
+	irq = irq_create_mapping(msi->domain, hwirq);
 	if (!irq)
 		return -EINVAL;
 
@@ -1097,16 +1109,15 @@ int arch_setup_msi_irq(struct pci_dev *pdev, struct msi_desc *desc)
 	return 0;
 }
 
-void arch_teardown_msi_irq(unsigned int irq)
+static void tegra_msi_teardown_irq(struct msi_chip *chip, unsigned int irq)
 {
-	struct tegra_pcie *pcie = irq_get_chip_data(irq);
+	struct tegra_msi *msi = to_tegra_msi(chip);
 	struct irq_data *d = irq_get_irq_data(irq);
 
-	tegra_pcie_msi_free(pcie, d->hwirq);
+	tegra_msi_free(msi, d->hwirq);
 }
-#endif
 
-static struct irq_chip tegra_pcie_msi_irq_chip = {
+static struct irq_chip tegra_msi_irq_chip = {
 	.name = "Tegra PCIe MSI",
 	.irq_enable = unmask_msi_irq,
 	.irq_disable = mask_msi_irq,
@@ -1114,11 +1125,10 @@ static struct irq_chip tegra_pcie_msi_irq_chip = {
 	.irq_unmask = unmask_msi_irq,
 };
 
-static int tegra_pcie_msi_map(struct irq_domain *domain, unsigned int irq,
-			      irq_hw_number_t hwirq)
+static int tegra_msi_map(struct irq_domain *domain, unsigned int irq,
+			 irq_hw_number_t hwirq)
 {
-	irq_set_chip_and_handler(irq, &tegra_pcie_msi_irq_chip,
-				 handle_simple_irq);
+	irq_set_chip_and_handler(irq, &tegra_msi_irq_chip, handle_simple_irq);
 	irq_set_chip_data(irq, domain->host_data);
 	set_irq_flags(irq, IRQF_VALID);
 
@@ -1126,22 +1136,26 @@ static int tegra_pcie_msi_map(struct irq_domain *domain, unsigned int irq,
 }
 
 static const struct irq_domain_ops msi_domain_ops = {
-	.map = tegra_pcie_msi_map,
+	.map = tegra_msi_map,
 };
 
 static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
 {
 	struct platform_device *pdev = to_platform_device(pcie->dev);
+	struct tegra_msi *msi = &pcie->msi;
 	unsigned long base;
 	int err;
 	u32 reg;
 
-	mutex_init(&pcie->msi.lock);
+	mutex_init(&msi->lock);
+
+	msi->chip.dev = pcie->dev;
+	msi->chip.setup_irq = tegra_msi_setup_irq;
+	msi->chip.teardown_irq = tegra_msi_teardown_irq;
 
-	pcie->msi.domain = irq_domain_add_linear(pcie->dev->of_node,
-						 INT_PCI_MSI_NR,
-						 &msi_domain_ops, pcie);
-	if (!pcie->msi.domain) {
+	msi->domain = irq_domain_add_linear(pcie->dev->of_node, INT_PCI_MSI_NR,
+					    &msi_domain_ops, &msi->chip);
+	if (!msi->domain) {
 		dev_err(&pdev->dev, "failed to create IRQ domain\n");
 		return -ENOMEM;
 	}
@@ -1152,18 +1166,18 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
 		goto err;
 	}
 
-	pcie->msi.irq = err;
+	msi->irq = err;
 
-	err = devm_request_irq(&pdev->dev, pcie->msi.irq, tegra_pcie_msi_irq,
-			       0, tegra_pcie_msi_irq_chip.name, pcie);
+	err = devm_request_irq(&pdev->dev, msi->irq, tegra_pcie_msi_irq,
+			       0, tegra_msi_irq_chip.name, pcie);
 	if (err < 0) {
 		dev_err(&pdev->dev, "failed to request IRQ: %d\n", err);
 		goto err;
 	}
 
 	/* setup AFI/FPCI range */
-	pcie->msi.pages = __get_free_pages(GFP_KERNEL, 3);
-	base = virt_to_phys((void *)pcie->msi.pages);
+	msi->pages = __get_free_pages(GFP_KERNEL, 3);
+	base = virt_to_phys((void *)msi->pages);
 
 	afi_writel(pcie, base, AFI_MSI_FPCI_BAR_ST);
 	afi_writel(pcie, base, AFI_MSI_AXI_BAR_ST);
@@ -1188,12 +1202,13 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
 	return 0;
 
 err:
-	irq_domain_remove(pcie->msi.domain);
+	irq_domain_remove(msi->domain);
 	return err;
 }
 
 static int tegra_pcie_disable_msi(struct tegra_pcie *pcie)
 {
+	struct tegra_msi *msi = &pcie->msi;
 	unsigned int i, irq;
 	u32 value;
 
@@ -1212,15 +1227,15 @@ static int tegra_pcie_disable_msi(struct tegra_pcie *pcie)
 	afi_writel(pcie, 0, AFI_MSI_EN_VEC6);
 	afi_writel(pcie, 0, AFI_MSI_EN_VEC7);
 
-	free_pages(pcie->msi.pages, 3);
+	free_pages(msi->pages, 3);
 
 	for (i = 0; i < INT_PCI_MSI_NR; i++) {
-		irq = irq_find_mapping(pcie->msi.domain, i);
+		irq = irq_find_mapping(msi->domain, i);
 		if (irq > 0)
 			irq_dispose_mapping(irq);
 	}
 
-	irq_domain_remove(pcie->msi.domain);
+	irq_domain_remove(msi->domain);
 
 	return 0;
 }
-- 
1.8.1.5

Re: [RFC 0/2] PCI: Introduce MSI chip infrastructure

[Andrew Murray]

On Fri, Mar 22, 2013 at 08:51:45AM +0000, Thierry Reding wrote:
> It
> is the responsibility of the PCI host bridge driver to setup the MSI
> chip for the root bus.

I think this could work well. In the future if the use of an independent MSI
controller is required, then new DT bindings for host-bridges could use
phandles to reference independent MSI controllers as their providers of
MSIs. I guess this functionality can be built on top of what you have proposed
later as the need arises.

Andrew Murray

Re: [RFC 1/2] PCI: Introduce new MSI chip infrastructure

[Andrew Murray]

On Fri, Mar 22, 2013 at 08:51:46AM +0000, Thierry Reding wrote:
> index ce93a34..ea4a5be 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -58,5 +58,15 @@ extern int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);
>  extern void arch_teardown_msi_irqs(struct pci_dev *dev);
>  extern 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);
> +};

Is there a need to add setup_irqs and teardown_irqs functions here? This will
allow your MSI chips to support multiple MSIs per requesting device.

What about restore_msi_irqs? Does this fit in here too?

Andrew Murray

Re: [RFC 1/2] PCI: Introduce new MSI chip infrastructure

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1477 bytes --]

On Fri, Mar 22, 2013 at 09:37:50AM +0000, Andrew Murray wrote:
> On Fri, Mar 22, 2013 at 08:51:46AM +0000, Thierry Reding wrote:
> > index ce93a34..ea4a5be 100644
> > --- a/include/linux/msi.h
> > +++ b/include/linux/msi.h
> > @@ -58,5 +58,15 @@ extern int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);
> >  extern void arch_teardown_msi_irqs(struct pci_dev *dev);
> >  extern 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);
> > +};
> 
> Is there a need to add setup_irqs and teardown_irqs functions here? This will
> allow your MSI chips to support multiple MSIs per requesting device.
> 
> What about restore_msi_irqs? Does this fit in here too?

I guess those could be added as well. I've concentrated on the most
common use-cases here, which seem to be the three included functions.
Most other implementations use the generic implementations for multiple
MSIs.

Note that the proposed framework is in no way fixed and can be extended
at will. The important step is to get rid of the one implementation for
the whole kernel concept.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC 0/2] PCI: Introduce MSI chip infrastructure

[Thomas Petazzoni]

Andrew, Thierry,

On Fri, 22 Mar 2013 09:30:27 +0000, Andrew Murray wrote:

> I think this could work well. In the future if the use of an independent MSI
> controller is required, then new DT bindings for host-bridges could use
> phandles to reference independent MSI controllers as their providers of
> MSIs. I guess this functionality can be built on top of what you have proposed
> later as the need arises.

On Marvell HW (at least Armada 370/XP), MSIs are handled by the
main interrupt controller directly, or more precisely, managing the
MSIs requires fiddling with registers that are part of the interrupt
controller registers, and not part of the PCIe controller registers.

Basically, when a MSI interrupt is raised, it corresponds to IRQ 1 on
the main interrupt controller. Then, one has to read a register of the
main interrupt controller to find out which MSI interrupt was actually
triggered. So in our case, the MSI irq_chip really belongs to the
interrupt controller driver, and not the PCIe driver. Also, the
physical address to be added in the 'struct msi_msg' is the physical
address of an interrupt controller register.

Therefore, I'm not sure how to do the interaction between the PCIe
driver and the interrupt controller driver.

Suggestions?

I'll try to post some ugly code next week just to show what is
happening.

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

Re: [RFC 0/2] PCI: Introduce MSI chip infrastructure

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2446 bytes --]

On Sun, Mar 24, 2013 at 12:06:49PM +0100, Thomas Petazzoni wrote:
> Andrew, Thierry,
> 
> On Fri, 22 Mar 2013 09:30:27 +0000, Andrew Murray wrote:
> 
> > I think this could work well. In the future if the use of an independent MSI
> > controller is required, then new DT bindings for host-bridges could use
> > phandles to reference independent MSI controllers as their providers of
> > MSIs. I guess this functionality can be built on top of what you have proposed
> > later as the need arises.
> 
> On Marvell HW (at least Armada 370/XP), MSIs are handled by the
> main interrupt controller directly, or more precisely, managing the
> MSIs requires fiddling with registers that are part of the interrupt
> controller registers, and not part of the PCIe controller registers.
> 
> Basically, when a MSI interrupt is raised, it corresponds to IRQ 1 on
> the main interrupt controller. Then, one has to read a register of the
> main interrupt controller to find out which MSI interrupt was actually
> triggered. So in our case, the MSI irq_chip really belongs to the
> interrupt controller driver, and not the PCIe driver. Also, the
> physical address to be added in the 'struct msi_msg' is the physical
> address of an interrupt controller register.
> 
> Therefore, I'm not sure how to do the interaction between the PCIe
> driver and the interrupt controller driver.
> 
> Suggestions?

That sounds very much like one of the use-cases that were discussed. The
easiest solution would probably be to add an API to look up an MSI chip
from a DT phandle, so that the PCIe controller's device node could have
it as a property, somewhat like this:

	msi: interrupt-controller {
	};

	pcie-controller {
		...
		marvell,msi = <&msi>;
		...
	};

Then add some basic infrastructure to register the MSI chip with a
global list, call that from the interrupt controller initialization:

	...
	msi_chip_add(&msi);
	...

And finally look it up from the PCIe controller driver:

	node = of_parse_phandle(dev->of_node, "marvell,msi", 0);
	if (node)
		msi = of_find_msi_chip_by_node(node);

That's roughly what other subsystems do. I wrote something similar once
for backlight devices, though the registration step (msi_chip_add)
wasn't necessary there since backlight devices all go into a common
struct class so class_find_device() can be used instead of going through
a separate registry.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC 0/2] PCI: Introduce MSI chip infrastructure

[Thomas Petazzoni]

Dear Thierry Reding,

Thanks for your feedback!

On Mon, 25 Mar 2013 08:58:10 +0100, Thierry Reding wrote:

> That sounds very much like one of the use-cases that were discussed. The
> easiest solution would probably be to add an API to look up an MSI chip
> from a DT phandle, so that the PCIe controller's device node could have
> it as a property, somewhat like this:
> 
> 	msi: interrupt-controller {
> 	};
> 
> 	pcie-controller {
> 		...
> 		marvell,msi = <&msi>;
> 		...
> 	};

I'm not sure how to handle this msi interrupt controller with the main
interrupt controller. For now, I have:

        mpic: interrupt-controller@d0020000 {
              reg = <0xd0020a00 0x2d0>,
                    <0xd0021070 0x58>;
        };

	[...]

	soc {
                interrupt-parent = <&mpic>;
		[...]
	};

And the MSI interrupt controller shares the same registers as the MPIC.
So should it be something like:

	interrupt-controller {
		reg = <0xd0020a00 0x2d0>,
                    <0xd0021070 0x58>;

		mpic {
			/* Not sure what to have here */
		};

		msi {

			/* Here either */
		};
	};

	soc {
                interrupt-parent = <&mpic>;

		pcie-controller {
			marvell,msi = <&msi>;
		};
	};

Or some other idea?

> Then add some basic infrastructure to register the MSI chip with a
> global list, call that from the interrupt controller initialization:
> 
> 	...
> 	msi_chip_add(&msi);
> 	...
> 
> And finally look it up from the PCIe controller driver:
> 
> 	node = of_parse_phandle(dev->of_node, "marvell,msi", 0);
> 	if (node)
> 		msi = of_find_msi_chip_by_node(node);
> 
> That's roughly what other subsystems do. I wrote something similar once
> for backlight devices, though the registration step (msi_chip_add)
> wasn't necessary there since backlight devices all go into a common
> struct class so class_find_device() can be used instead of going through
> a separate registry.

Ok, that part sounds good to me. I'm still unsure about the DT
representation, though (see above), and experience has shown that's
it's a pretty good idea to discuss a little bit the DT representation
before going on with some code :)

Thanks again for your feedback!

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

Re: [RFC 0/2] PCI: Introduce MSI chip infrastructure

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1800 bytes --]

On Mon, Mar 25, 2013 at 09:38:47AM +0100, Thomas Petazzoni wrote:
> Dear Thierry Reding,
> 
> Thanks for your feedback!
> 
> On Mon, 25 Mar 2013 08:58:10 +0100, Thierry Reding wrote:
> 
> > That sounds very much like one of the use-cases that were discussed. The
> > easiest solution would probably be to add an API to look up an MSI chip
> > from a DT phandle, so that the PCIe controller's device node could have
> > it as a property, somewhat like this:
> > 
> > 	msi: interrupt-controller {
> > 	};
> > 
> > 	pcie-controller {
> > 		...
> > 		marvell,msi = <&msi>;
> > 		...
> > 	};
> 
> I'm not sure how to handle this msi interrupt controller with the main
> interrupt controller. For now, I have:
> 
>         mpic: interrupt-controller@d0020000 {
>               reg = <0xd0020a00 0x2d0>,
>                     <0xd0021070 0x58>;
>         };
> 
> 	[...]
> 
> 	soc {
>                 interrupt-parent = <&mpic>;
> 		[...]
> 	};
> 
> And the MSI interrupt controller shares the same registers as the MPIC.
> So should it be something like:
> 
> 	interrupt-controller {
> 		reg = <0xd0020a00 0x2d0>,
>                     <0xd0021070 0x58>;
> 
> 		mpic {
> 			/* Not sure what to have here */
> 		};
> 
> 		msi {
> 
> 			/* Here either */
> 		};
> 	};
> 
> 	soc {
>                 interrupt-parent = <&mpic>;
> 
> 		pcie-controller {
> 			marvell,msi = <&msi>;
> 		};
> 	};
> 
> Or some other idea?

I think you can just make this:

	mpic: interrupt-controller@d0020000 {
		...
	};

	...

	soc {
		pcie-controller {
			marvell,msi = <&mpic>;
		};
	};

And everything else should just work given the APIs I mentioned. But as
you said it'd be good if somebody else could share their opinion about
this.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC 0/2] PCI: Introduce MSI chip infrastructure

[Arnd Bergmann]

On Monday 25 March 2013, Thierry Reding wrote:
> I think you can just make this:
> 
>         mpic: interrupt-controller@d0020000 {
>                 ...
>         };
> 
>         ...
> 
>         soc {
>                 pcie-controller {
>                         marvell,msi = <&mpic>;
>                 };
>         };
> 
> And everything else should just work given the APIs I mentioned. But as
> you said it'd be good if somebody else could share their opinion about
> this.


I think the property referring to the msi controller should have a fixed
name, such as "msi-parent", to go along with "interrupt-parent".
Similarly, I would suggest using an empty "msi-controller" property
to mark the controller that is capable of serving MSIs. The Linux
implementation doesn't currently require the "interrupt-controller"
property, but I think it's good to stay close to the original interrupt
binding here for consistency.

	Arnd

Re: [RFC 2/2] PCI: tegra: Use new MSI chip infrastructure

[Stephen Warren]

On 03/22/2013 02:51 AM, Thierry Reding wrote:
> Implement an MSI chip that uses the Tegra PCIe controller's built-in
> support to provide MSI services to the root bus and its children.

Acked-by: Stephen Warren <swarren@nvidia.com>

Thierry, what Tegra-related PCIe patches will you posted for 3.10? If
it's just this, I'm happy for this to go through the PCI tree along with
patch 1/2. If you're still planning on doing the whole Tegra PCIe driver
conversion and move for 3.10, perhaps there are some additional
dependencies that require thinking about?

Re: [RFC 2/2] PCI: tegra: Use new MSI chip infrastructure

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]

On Mon, Mar 25, 2013 at 11:01:15AM -0600, Stephen Warren wrote:
> On 03/22/2013 02:51 AM, Thierry Reding wrote:
> > Implement an MSI chip that uses the Tegra PCIe controller's built-in
> > support to provide MSI services to the root bus and its children.
> 
> Acked-by: Stephen Warren <swarren@nvidia.com>
> 
> Thierry, what Tegra-related PCIe patches will you posted for 3.10? If
> it's just this, I'm happy for this to go through the PCI tree along with
> patch 1/2. If you're still planning on doing the whole Tegra PCIe driver
> conversion and move for 3.10, perhaps there are some additional
> dependencies that require thinking about?

This one patch depends on the whole series. At this point it doesn't
make any sense to merge anything but all of them. Andrew Murray was
working on getting the OF specific bits ready for merge, so I think
there are only a couple of Tegra-specific patches that would need to be
taken care of, such as minor PMC things. I'll look into posting a
complete series soon.

My plan was to squash this patch into the large rewrite/move patch from
the series because that introduces MSI support in the first place. But I
thought for this particular mini-series it'd be easier to see what needs
to be done to support it if I post only the relevant bits.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]