[PATCH v2 0/8] Introducing per-device MSI domain

MSI-like interrupts are starting to creep out of the PCI world, and
can now be seen into a number of "platform"-type busses. The MSI
domain patches recognise that fact, and start providing a way to
implement this.

Another problem we have to solve is to identify which MSI domain a
device is "connected" to. Currently, PCI gets away with a mixture of
arch-specific callbacks, and a msi_controller structure that can
optionally carry a pointer to an MSI domain. As we add new bus types
and start dealing with topologies that do not map to what PCI does,
this doesn't scale anymore.

This patch series tries to address some of it by providing a basic
link between 'struct device' and an MSI domain. It also adds (yet
another) way for PCI to propagate the domain pointer through the PCI
device hierarchy, provides a method for OF to kick-start the
propagation process, and finally allows the PCI/MSI layer to use that
information. Hopefully this can serve as a model to implement support
for different but types.

Additionally, the last three patches use all the above to remove any
trace of the msi_controller structure from the two GIC interrupt
controllers we use on arm64, so that they solely rely on the above
infrastructure. We take this opportunity to also kill the domain
pointer from the msi_controller structure.

My hope is to eventually kill msi_controller entirely, and only rely
on the msi_domain contained in the device structure (any help
welcomed).

This has been tested on arm64 with GICv2m (AMD Seattle) and GICv3 ITS
(FVP model).

Patches are on top of 3.19-rc3 and available at:

git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git irq/msi_domain

As always, comments most welcome.

	M.

From v1:

- Allow arch code to set the MSI domain before we try to do so in core
  code (which is used as a fallback).
- Allow the MSI domain to be looked-up by using the PHB node.
- Remove domain field from msi_controller

[1] https://lkml.org/lkml/2014/12/8/581

Marc Zyngier (8):
  device core: Introduce per-device MSI domain pointer
  PCI/MSI: Add hooks to populate the msi_domain field
  PCI/MSI: of: Add support for OF-provided msi_domain
  PCI/MSI: of: Allow msi_domain lookup using the PHB node
  PCI/MSI: Let pci_msi_get_domain use struct device's msi_domain
  irqchip: GICv2m: Get rid of struct msi_controller
  irqchip: gicv3-its: Get rid of struct msi_controller
  PCI/MSI: Drop domain field from msi_controller

 drivers/irqchip/irq-gic-v2m.c    | 26 +++++++++-----------------
 drivers/irqchip/irq-gic-v3-its.c | 33 ++++++++++++++-------------------
 drivers/pci/msi.c                |  3 +--
 drivers/pci/of.c                 | 20 ++++++++++++++++++++
 drivers/pci/probe.c              | 31 +++++++++++++++++++++++++++++++
 include/linux/device.h           | 20 ++++++++++++++++++++
 include/linux/msi.h              |  3 ---
 include/linux/pci.h              |  3 +++
 8 files changed, 98 insertions(+), 41 deletions(-)

-- 
2.1.4

As MSI-type features are creeping into non-PCI devices, it is
starting to make sense to give our struct device some form of
support for this, by allowing a pointer to an MSI irq domain to
be set/retrieved.

Signed-off-by: Marc Zyngier <redacted>
---
 include/linux/device.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index fb50673..ec4cee5 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h

@@ -690,6 +690,7 @@ struct acpi_dev_node {
  * 		along with subsystem-level and driver-level callbacks.
  * @pins:	For device pin management.
  *		See Documentation/pinctrl.txt for details.
+ * @msi_domain: The generic MSI domain this device is using.
  * @numa_node:	NUMA node this device is close to.
  * @dma_mask:	Dma mask (if dma'ble device).
  * @coherent_dma_mask: Like dma_mask, but for alloc_coherent mapping as not all

@@ -750,6 +751,9 @@ struct device {
 	struct dev_pm_info	power;
 	struct dev_pm_domain	*pm_domain;
 
+#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
+	struct irq_domain	*msi_domain; /* MSI domain device uses */
+#endif
 #ifdef CONFIG_PINCTRL
 	struct dev_pin_info	*pins;
 #endif

@@ -837,6 +841,22 @@ static inline void set_dev_node(struct device *dev, int node)
 }
 #endif
 
+static inline struct irq_domain *dev_get_msi_domain(const struct device *dev)
+{
+#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
+	return dev->msi_domain;
+#else
+	return NULL;
+#endif
+}
+
+static inline void dev_set_msi_domain(struct device *dev, struct irq_domain *d)
+{
+#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
+	dev->msi_domain = d;
+#endif
+}
+
 static inline void *dev_get_drvdata(const struct device *dev)
 {
 	return dev->driver_data;

-- 
2.1.4

On Thu, Jan 8, 2015 at 11:06 AM, Marc Zyngier [off-list ref] wrote:

This is not a comment on this patch specifically, but a question about other
MSI specific fields that might be needed in struct device.

Currently the generic MSI domain handling has hardcoded assumptions
that devices are PCI-- see the for_each_msi_entry() iterator in msi.h:

  #define dev_to_msi_list(dev)            (&to_pci_dev((dev))->msi_list)

  #define for_each_msi_entry(desc, dev)   \
        list_for_each_entry((desc), dev_to_msi_list((dev)), list)

One approach would be to move the msi_list out of pci_dev and put
it in struct device, so all devices can have an msi_list.

The other approach would be to keep msi_list in a bus specific
device struct, and then dev_to_msi_list() would need to be
implemented as a bus specific callback of some kind.

The above hardcoded PCI assumption isn't going to work.   Wanted to
see if there is any advice in which direction to go.

Thanks,
Stuart Yoder

Hi Stuart,

On 15/01/15 20:35, Stuart Yoder wrote:

On Thu, Jan 8, 2015 at 11:06 AM, Marc Zyngier [off-list ref] wrote:

quoted

As MSI-type features are creeping into non-PCI devices, it is
starting to make sense to give our struct device some form of
support for this, by allowing a pointer to an MSI irq domain to
be set/retrieved.

Signed-off-by: Marc Zyngier <redacted>
---
 include/linux/device.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index fb50673..ec4cee5 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h

@@ -690,6 +690,7 @@ struct acpi_dev_node {
  *             along with subsystem-level and driver-level callbacks.
  * @pins:      For device pin management.
  *             See Documentation/pinctrl.txt for details.
+ * @msi_domain: The generic MSI domain this device is using.
  * @numa_node: NUMA node this device is close to.
  * @dma_mask:  Dma mask (if dma'ble device).
  * @coherent_dma_mask: Like dma_mask, but for alloc_coherent mapping as not all

@@ -750,6 +751,9 @@ struct device {
        struct dev_pm_info      power;
        struct dev_pm_domain    *pm_domain;

+#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
+       struct irq_domain       *msi_domain; /* MSI domain device uses */
+#endif

This is not a comment on this patch specifically, but a question about other
MSI specific fields that might be needed in struct device.

Currently the generic MSI domain handling has hardcoded assumptions
that devices are PCI-- see the for_each_msi_entry() iterator in msi.h:

  #define dev_to_msi_list(dev)            (&to_pci_dev((dev))->msi_list)

  #define for_each_msi_entry(desc, dev)   \
        list_for_each_entry((desc), dev_to_msi_list((dev)), list)

One approach would be to move the msi_list out of pci_dev and put
it in struct device, so all devices can have an msi_list.

The other approach would be to keep msi_list in a bus specific
device struct, and then dev_to_msi_list() would need to be
implemented as a bus specific callback of some kind.

The above hardcoded PCI assumption isn't going to work.   Wanted to
see if there is any advice in which direction to go.

The question is: can we define a generic msi_desc? If yes, then your
first proposal make sense. If not, then it is the second one.

My hunch is that we'll have to move to a model that would look like this:

struct mybus_msi_desc {
	struct msi_desc desc;
	struct mybus_stuff stuff;
};

and move the PCI-specific stuff out of msi_desc.

Thoughts?

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

On 2015/1/16 4:35, Stuart Yoder wrote:

On Thu, Jan 8, 2015 at 11:06 AM, Marc Zyngier [off-list ref] wrote:

quoted

As MSI-type features are creeping into non-PCI devices, it is
starting to make sense to give our struct device some form of
support for this, by allowing a pointer to an MSI irq domain to
be set/retrieved.

Signed-off-by: Marc Zyngier <redacted>
---
 include/linux/device.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index fb50673..ec4cee5 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h

@@ -690,6 +690,7 @@ struct acpi_dev_node {
  *             along with subsystem-level and driver-level callbacks.
  * @pins:      For device pin management.
  *             See Documentation/pinctrl.txt for details.
+ * @msi_domain: The generic MSI domain this device is using.
  * @numa_node: NUMA node this device is close to.
  * @dma_mask:  Dma mask (if dma'ble device).
  * @coherent_dma_mask: Like dma_mask, but for alloc_coherent mapping as not all

@@ -750,6 +751,9 @@ struct device {
        struct dev_pm_info      power;
        struct dev_pm_domain    *pm_domain;

+#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
+       struct irq_domain       *msi_domain; /* MSI domain device uses */
+#endif

This is not a comment on this patch specifically, but a question about other
MSI specific fields that might be needed in struct device.

Currently the generic MSI domain handling has hardcoded assumptions
that devices are PCI-- see the for_each_msi_entry() iterator in msi.h:

  #define dev_to_msi_list(dev)            (&to_pci_dev((dev))->msi_list)

  #define for_each_msi_entry(desc, dev)   \
        list_for_each_entry((desc), dev_to_msi_list((dev)), list)

One approach would be to move the msi_list out of pci_dev and put
it in struct device, so all devices can have an msi_list.

The other approach would be to keep msi_list in a bus specific
device struct, and then dev_to_msi_list() would need to be
implemented as a bus specific callback of some kind.

The above hardcoded PCI assumption isn't going to work.   Wanted to
see if there is any advice in which direction to go.

Hi Stuart,
	I already have some a patch set to go that direction waiting
send out for review:)
Thanks!
Gerry

Thanks,
Stuart Yoder
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Gerry,

So which direction did you take in your patch set--  a) common,
generic msi_desc, or b)  bus-specific msi_desc like Marc showed
(mybus_msi_desc)?

Thanks,
Stuart

On Sun, Jan 18, 2015 at 8:10 PM, Jiang Liu [off-list ref] wrote:

On 2015/1/16 4:35, Stuart Yoder wrote:

quoted

On Thu, Jan 8, 2015 at 11:06 AM, Marc Zyngier [off-list ref] wrote:

quoted

As MSI-type features are creeping into non-PCI devices, it is
starting to make sense to give our struct device some form of
support for this, by allowing a pointer to an MSI irq domain to
be set/retrieved.

Signed-off-by: Marc Zyngier <redacted>
---
 include/linux/device.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index fb50673..ec4cee5 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h

@@ -690,6 +690,7 @@ struct acpi_dev_node {
  *             along with subsystem-level and driver-level callbacks.
  * @pins:      For device pin management.
  *             See Documentation/pinctrl.txt for details.
+ * @msi_domain: The generic MSI domain this device is using.
  * @numa_node: NUMA node this device is close to.
  * @dma_mask:  Dma mask (if dma'ble device).
  * @coherent_dma_mask: Like dma_mask, but for alloc_coherent mapping as not all

@@ -750,6 +751,9 @@ struct device {
        struct dev_pm_info      power;
        struct dev_pm_domain    *pm_domain;

+#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
+       struct irq_domain       *msi_domain; /* MSI domain device uses */
+#endif

This is not a comment on this patch specifically, but a question about other
MSI specific fields that might be needed in struct device.

Currently the generic MSI domain handling has hardcoded assumptions
that devices are PCI-- see the for_each_msi_entry() iterator in msi.h:

  #define dev_to_msi_list(dev)            (&to_pci_dev((dev))->msi_list)

  #define for_each_msi_entry(desc, dev)   \
        list_for_each_entry((desc), dev_to_msi_list((dev)), list)

One approach would be to move the msi_list out of pci_dev and put
it in struct device, so all devices can have an msi_list.

The other approach would be to keep msi_list in a bus specific
device struct, and then dev_to_msi_list() would need to be
implemented as a bus specific callback of some kind.

The above hardcoded PCI assumption isn't going to work.   Wanted to
see if there is any advice in which direction to go.

Hi Stuart,
        I already have some a patch set to go that direction waiting
send out for review:)
Thanks!
Gerry

quoted

Thanks,
Stuart Yoder
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

On 2015/1/21 1:17, Stuart Yoder wrote:

Gerry,

So which direction did you take in your patch set--  a) common,
generic msi_desc, or b)  bus-specific msi_desc like Marc showed
(mybus_msi_desc)?

Hi Stuart,
	Currently I'm trying to go the former way as below.
Regards,
Gerry
-----------------------------------------------------------------------
struct msi_desc {
        struct list_head                list;
        unsigned int                    irq;
        unsigned int                    nvec_used;      /* number of
messages */
        struct device *                 dev;
        struct msi_msg                  msg;            /* Last set MSI
message */

#ifdef CONFIG_PCI_MSI
        union {
                struct {                                /* For PCI
MSI/MSI-X */
                        u32 masked;                     /* mask bits */
                        struct {
                                __u8    is_msix : 1;
                                __u8    multiple: 3;    /* log2 num of
messages allocated */
                                __u8    multi_cap : 3;  /* log2 num of
messages supported */
                                __u8    maskbit : 1;    /* mask-pending
bit supported ? */
                                __u8    is_64   : 1;    /* Address size:
0=32bit 1=64bit */
                                __u16   entry_nr;       /* specific
enabled entry */
                                unsigned default_irq;   /* default
pre-assigned irq */
                        } msi_attrib;
                        union {
                                u8      mask_pos;
                                void __iomem *mask_base;
                        };
                };
        };
#endif /* CONFIG_PCI_MSI */
};

Thanks,
Stuart

On Sun, Jan 18, 2015 at 8:10 PM, Jiang Liu [off-list ref] wrote:

quoted

On 2015/1/16 4:35, Stuart Yoder wrote:

quoted

On Thu, Jan 8, 2015 at 11:06 AM, Marc Zyngier [off-list ref] wrote:

quoted

As MSI-type features are creeping into non-PCI devices, it is
starting to make sense to give our struct device some form of
support for this, by allowing a pointer to an MSI irq domain to
be set/retrieved.

Signed-off-by: Marc Zyngier <redacted>
---
 include/linux/device.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index fb50673..ec4cee5 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h

@@ -690,6 +690,7 @@ struct acpi_dev_node {
  *             along with subsystem-level and driver-level callbacks.
  * @pins:      For device pin management.
  *             See Documentation/pinctrl.txt for details.
+ * @msi_domain: The generic MSI domain this device is using.
  * @numa_node: NUMA node this device is close to.
  * @dma_mask:  Dma mask (if dma'ble device).
  * @coherent_dma_mask: Like dma_mask, but for alloc_coherent mapping as not all

@@ -750,6 +751,9 @@ struct device {
        struct dev_pm_info      power;
        struct dev_pm_domain    *pm_domain;

+#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
+       struct irq_domain       *msi_domain; /* MSI domain device uses */
+#endif

This is not a comment on this patch specifically, but a question about other
MSI specific fields that might be needed in struct device.

Currently the generic MSI domain handling has hardcoded assumptions
that devices are PCI-- see the for_each_msi_entry() iterator in msi.h:

  #define dev_to_msi_list(dev)            (&to_pci_dev((dev))->msi_list)

  #define for_each_msi_entry(desc, dev)   \
        list_for_each_entry((desc), dev_to_msi_list((dev)), list)

One approach would be to move the msi_list out of pci_dev and put
it in struct device, so all devices can have an msi_list.

The other approach would be to keep msi_list in a bus specific
device struct, and then dev_to_msi_list() would need to be
implemented as a bus specific callback of some kind.

The above hardcoded PCI assumption isn't going to work.   Wanted to
see if there is any advice in which direction to go.

Hi Stuart,
        I already have some a patch set to go that direction waiting
send out for review:)
Thanks!
Gerry

quoted

Thanks,
Stuart Yoder
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

A number of platforms do not need to use the msi-parent property,
as the host bridge itself provides the MSI controller.

Allow this configuration by performing an irq domain lookup based
on the PHB node if it doesn't have a valid msi-parent property.

Signed-off-by: Marc Zyngier <redacted>
---
 drivers/pci/of.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index d8d1274..75bfb85 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c

@@ -68,9 +68,14 @@ void pci_set_phb_of_msi_domain(struct pci_bus *bus)
 
 	if (!bus->dev.of_node)
 		return;
+	/* Start looking for a phandle to an MSI controller. */
 	np = of_parse_phandle(bus->dev.of_node, "msi-parent", 0);
+	/*
+	 * If we don't have an msi-parent property, look for a domain
+	 * directly attached to the host bridge.
+	 */
 	if (!np)
-		return;
+		np = bus->dev.of_node;
 	dev_set_msi_domain(&bus->dev, irq_find_host(np));
 #endif
 }

-- 
2.1.4

In order to be able to populate the device msi_domain field,
add the necesary hooks to propagate the PHB msi_domain across
secondary busses to devices.

So far, nobody populates the initial msi_domain.

Signed-off-by: Marc Zyngier <redacted>
---
 drivers/pci/probe.c | 30 ++++++++++++++++++++++++++++++
 include/linux/pci.h |  1 +
 2 files changed, 31 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 23212f8..977c079 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c

@@ -660,6 +660,20 @@ static void pci_set_bus_speed(struct pci_bus *bus)
 	}
 }
 
+void __weak pcibios_set_phb_msi_domain(struct pci_bus *bus)
+{
+}
+
+static void pci_set_bus_msi_domain(struct pci_bus *bus)
+{
+	struct pci_dev *bridge = bus->self;
+
+	if (!bridge)
+		pcibios_set_phb_msi_domain(bus);
+	else
+		dev_set_msi_domain(&bus->dev, dev_get_msi_domain(&bridge->dev));
+}
+
 static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 					   struct pci_dev *bridge, int busnr)
 {

@@ -713,6 +727,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 	bridge->subordinate = child;
 
 add_dev:
+	pci_set_bus_msi_domain(child);
 	ret = device_register(&child->dev);
 	WARN_ON(ret < 0);
 

@@ -1507,6 +1522,17 @@ static void pci_init_capabilities(struct pci_dev *dev)
 	pci_enable_acs(dev);
 }
 
+static void pci_set_msi_domain(struct pci_dev *dev)
+{
+	/*
+	 * If no domain has been set through the pcibios callback,
+	 * inherit the default from the bus device.
+	 */
+	if (!dev_get_msi_domain(&dev->dev))
+		dev_set_msi_domain(&dev->dev,
+				   dev_get_msi_domain(&dev->bus->dev));
+}
+
 void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 {
 	int ret;

@@ -1547,6 +1573,9 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 	ret = pcibios_add_device(dev);
 	WARN_ON(ret < 0);
 
+	/* Setup MSI irq domain */
+	pci_set_msi_domain(dev);
+
 	/* Notifier could use PCI capabilities */
 	dev->match_driver = false;
 	ret = device_add(&dev->dev);

@@ -1937,6 +1966,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	b->bridge = get_device(&bridge->dev);
 	device_enable_async_suspend(b->bridge);
 	pci_set_bus_of_node(b);
+	pci_set_bus_msi_domain(b);
 
 	if (!parent)
 		set_dev_node(b->bridge, pcibus_to_node(b));

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 360a966..13c65ab 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h

@@ -1640,6 +1640,7 @@ int pcibios_set_pcie_reset_state(struct pci_dev *dev,
 int pcibios_add_device(struct pci_dev *dev);
 void pcibios_release_device(struct pci_dev *dev);
 void pcibios_penalize_isa_irq(int irq, int active);
+void pcibios_set_phb_msi_domain(struct pci_bus *bus);
 
 #ifdef CONFIG_HIBERNATE_CALLBACKS
 extern struct dev_pm_ops pcibios_pm_ops;

-- 
2.1.4

Hi Marc, now we have two ways to associate the pci_dev and msi_domain, right ?

1. associate pci_dev and msi_domain in pcibios_add_device() like x86.

2. Inherit msi_domain from pci_dev->bus.

My question is if all pci devices inherit msi_domain from the pci_bus,
so all pci devices under same pci host bridge have the same msi_domain assigned by
weak pcibios_set_phb_msi_domain(). So why not save the pci host bridge specific
msi_domain in pci_host_bridge. Then pci devices could inherit the msi_domain from
its pci host bridge directly, no need to involve pci bus in the assignment.

If I misunderstood, please let me know, :)

Thanks!
Yijing.

-- 
Thanks!
Yijing

On 13/01/15 12:34, Yijing Wang wrote:

quoted

 static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 					   struct pci_dev *bridge, int busnr)
 {

@@ -713,6 +727,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 	bridge->subordinate = child;
 
 add_dev:
+	pci_set_bus_msi_domain(child);
 	ret = device_register(&child->dev);
 	WARN_ON(ret < 0);
 

@@ -1507,6 +1522,17 @@ static void pci_init_capabilities(struct pci_dev *dev)
 	pci_enable_acs(dev);
 }
 
+static void pci_set_msi_domain(struct pci_dev *dev)
+{
+	/*
+	 * If no domain has been set through the pcibios callback,
+	 * inherit the default from the bus device.
+	 */
+	if (!dev_get_msi_domain(&dev->dev))
+		dev_set_msi_domain(&dev->dev,
+				   dev_get_msi_domain(&dev->bus->dev));
+}

Hi Marc, now we have two ways to associate the pci_dev and msi_domain, right ?

1. associate pci_dev and msi_domain in pcibios_add_device() like x86.

2. Inherit msi_domain from pci_dev->bus.

My question is if all pci devices inherit msi_domain from the pci_bus,
so all pci devices under same pci host bridge have the same msi_domain assigned by
weak pcibios_set_phb_msi_domain(). So why not save the pci host bridge specific
msi_domain in pci_host_bridge. Then pci devices could inherit the msi_domain from
its pci host bridge directly, no need to involve pci bus in the assignment.

But then, you would end-up maintaining another msi_domain field inside
the pci_host bridge structure. What do you gain by doing so?

With this series, msi_domain has the nice property of always being tied
to a device (and struct pci_bus always has a device). We always have
phb->bus->dev.msi_domain within reach, and architecture code can decide
to override it on a per-device basis.

What else do you need? What am I missing from your proposal?

Thanks,

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

quoted

quoted

+static void pci_set_msi_domain(struct pci_dev *dev)
+{
+	/*
+	 * If no domain has been set through the pcibios callback,
+	 * inherit the default from the bus device.
+	 */
+	if (!dev_get_msi_domain(&dev->dev))
+		dev_set_msi_domain(&dev->dev,
+				   dev_get_msi_domain(&dev->bus->dev));
+}

Hi Marc, now we have two ways to associate the pci_dev and msi_domain, right ?

1. associate pci_dev and msi_domain in pcibios_add_device() like x86.

2. Inherit msi_domain from pci_dev->bus.

My question is if all pci devices inherit msi_domain from the pci_bus,
so all pci devices under same pci host bridge have the same msi_domain assigned by
weak pcibios_set_phb_msi_domain(). So why not save the pci host bridge specific
msi_domain in pci_host_bridge. Then pci devices could inherit the msi_domain from
its pci host bridge directly, no need to involve pci bus in the assignment.

But then, you would end-up maintaining another msi_domain field inside
the pci_host bridge structure. What do you gain by doing so?

My original thought is holding msi_domain field inside the pci_host_bridge is
more simple than every bus maintaining the msi_domain, but this proposal has a
disadvantage that sometimes we must setup for every device. I checked x86 DMAR code,
and found most DMAR would report PCIe root port device associating the msi_domain, not the EP device.
So pcibios_add_device could only associate these bridge device msi_domain, and its children
devices will propagate from their parent bus(get msi_domain from its bridge).

So now I agree your idea, please forgive my nagging :)

Thanks!
Yijing.

With this series, msi_domain has the nice property of always being tied
to a device (and struct pci_bus always has a device). We always have
phb->bus->dev.msi_domain within reach, and architecture code can decide
to override it on a per-device basis.

What else do you need? What am I missing from your proposal?

Thanks,

	M.

-- 
Thanks!
Yijing

On 2015/1/9 1:06, Marc Zyngier wrote:

In order to be able to populate the device msi_domain field,
add the necesary hooks to propagate the PHB msi_domain across
secondary busses to devices.

So far, nobody populates the initial msi_domain.

Acked-by: Yijing Wang <redacted>

-- 
Thanks!
Yijing

Now that we can easily find which MSI domain a PCI device is
using, use dev_get_msi_domain as a way to retrieve the information.

The original code is still used as a fallback.

Signed-off-by: Marc Zyngier <redacted>
---
 drivers/pci/msi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index fd60806..003040b 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c

@@ -41,7 +41,8 @@ static struct irq_domain *pci_msi_get_domain(struct pci_dev *dev)
 {
 	struct irq_domain *domain = NULL;
 
-	if (dev->bus->msi)
+	domain = dev_get_msi_domain(&dev->dev);
+	if (!domain && dev->bus->msi)
 		domain = dev->bus->msi->domain;
 	if (!domain)
 		domain = arch_get_pci_msi_domain(dev);

-- 
2.1.4

In order to populate the PHB msi_domain, use the "msi-parent"
attribute to lookup a corresponding irq domain. If found,
this is our MSI domain.

This gets plugged into the core PCI code.

Signed-off-by: Marc Zyngier <redacted>
---
 drivers/pci/of.c    | 15 +++++++++++++++
 drivers/pci/probe.c |  1 +
 include/linux/pci.h |  2 ++
 3 files changed, 18 insertions(+)

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index f092993..d8d1274 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c

@@ -9,6 +9,7 @@
  * 2 of the License, or (at your option) any later version.
  */
 
+#include <linux/irqdomain.h>
 #include <linux/kernel.h>
 #include <linux/pci.h>
 #include <linux/of.h>

@@ -59,3 +60,17 @@ struct device_node * __weak pcibios_get_phb_of_node(struct pci_bus *bus)
 		return of_node_get(bus->bridge->parent->of_node);
 	return NULL;
 }
+
+void pci_set_phb_of_msi_domain(struct pci_bus *bus)
+{
+#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
+	struct device_node *np;
+
+	if (!bus->dev.of_node)
+		return;
+	np = of_parse_phandle(bus->dev.of_node, "msi-parent", 0);
+	if (!np)
+		return;
+	dev_set_msi_domain(&bus->dev, irq_find_host(np));
+#endif
+}

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 977c079..82fbd12 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c

@@ -662,6 +662,7 @@ static void pci_set_bus_speed(struct pci_bus *bus)
 
 void __weak pcibios_set_phb_msi_domain(struct pci_bus *bus)
 {
+	pci_set_phb_of_msi_domain(bus);
 }
 
 static void pci_set_bus_msi_domain(struct pci_bus *bus)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 13c65ab..66c493e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h

@@ -1828,6 +1828,7 @@ void pci_set_of_node(struct pci_dev *dev);
 void pci_release_of_node(struct pci_dev *dev);
 void pci_set_bus_of_node(struct pci_bus *bus);
 void pci_release_bus_of_node(struct pci_bus *bus);
+void pci_set_phb_of_msi_domain(struct pci_bus *bus);
 
 /* Arch may override this (weak) */
 struct device_node *pcibios_get_phb_of_node(struct pci_bus *bus);

@@ -1848,6 +1849,7 @@ static inline void pci_set_of_node(struct pci_dev *dev) { }
 static inline void pci_release_of_node(struct pci_dev *dev) { }
 static inline void pci_set_bus_of_node(struct pci_bus *bus) { }
 static inline void pci_release_bus_of_node(struct pci_bus *bus) { }
+static inline void pci_set_phb_of_msi_domain(struct pci_bus *bus) {}
 #endif  /* CONFIG_OF */
 
 #ifdef CONFIG_EEH

-- 
2.1.4

On 2015/1/9 1:06, Marc Zyngier wrote:

In order to populate the PHB msi_domain, use the "msi-parent"
attribute to lookup a corresponding irq domain. If found,
this is our MSI domain.

This gets plugged into the core PCI code.

Hi Marc,

Since the whole patch series based on the fact that non PCI type
buses will use MSI-like interrupts, would it be better if getting
this field settled during OF populating?

Regards,
	Abel

Hi Abel,

On 14/01/15 08:17, Yun Wu (Abel) wrote:

On 2015/1/9 1:06, Marc Zyngier wrote:

quoted

In order to populate the PHB msi_domain, use the "msi-parent"
attribute to lookup a corresponding irq domain. If found,
this is our MSI domain.

This gets plugged into the core PCI code.

Hi Marc,

Since the whole patch series based on the fact that non PCI type
buses will use MSI-like interrupts, would it be better if getting
this field settled during OF populating?

Are you're thinking of letting random platform devices automagically
gain an MSI domain as they are discovered by DT? I feel like you looking
at it the wrong way.

Note that DT discovery doesn't imply a particular bus, and non-PCI
doesn't mean bus-agnostic. MSI-like interrupts really are a bus
property, and I'd rather see you implementing support for your favourite
bus (and do the MSI domain matching there).

Thanks,

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

GICv2m only uses the msi_controller structure as a way to match
the PHB with its MSI HW, and thus the msi_domain. But now that
we can directly associate an msi_domain with a device, there is
no use keeping this msi_controller around.

Just remove all traces of msi_controller from the driver.

Signed-off-by: Marc Zyngier <redacted>
---
 drivers/irqchip/irq-gic-v2m.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
index fdf7065..a76b802 100644
--- a/drivers/irqchip/irq-gic-v2m.c
+++ b/drivers/irqchip/irq-gic-v2m.c

@@ -45,7 +45,6 @@
 
 struct v2m_data {
 	spinlock_t msi_cnt_lock;
-	struct msi_controller mchip;
 	struct resource res;	/* GICv2m resource */
 	void __iomem *base;	/* GICv2m virt address */
 	u32 spi_start;		/* The SPI number that MSIs start */

@@ -218,6 +217,7 @@ static int __init gicv2m_init_one(struct device_node *node,
 {
 	int ret;
 	struct v2m_data *v2m;
+	struct irq_domain *inner_domain;
 
 	v2m = kzalloc(sizeof(struct v2m_data), GFP_KERNEL);
 	if (!v2m) {

@@ -261,19 +261,17 @@ static int __init gicv2m_init_one(struct device_node *node,
 		goto err_iounmap;
 	}
 
-	v2m->domain = irq_domain_add_tree(NULL, &gicv2m_domain_ops, v2m);
-	if (!v2m->domain) {
+	inner_domain = irq_domain_add_tree(NULL, &gicv2m_domain_ops, v2m);
+	if (!inner_domain) {
 		pr_err("Failed to create GICv2m domain\n");
 		ret = -ENOMEM;
 		goto err_free_bm;
 	}
 
-	v2m->domain->parent = parent;
-	v2m->mchip.of_node = node;
-	v2m->mchip.domain = pci_msi_create_irq_domain(node,
-						      &gicv2m_msi_domain_info,
-						      v2m->domain);
-	if (!v2m->mchip.domain) {
+	inner_domain->parent = parent;
+	v2m->domain = pci_msi_create_irq_domain(node, &gicv2m_msi_domain_info,
+						inner_domain);
+	if (!v2m->domain) {
 		pr_err("Failed to create MSI domain\n");
 		ret = -ENOMEM;
 		goto err_free_domains;

@@ -281,12 +279,6 @@ static int __init gicv2m_init_one(struct device_node *node,
 
 	spin_lock_init(&v2m->msi_cnt_lock);
 
-	ret = of_pci_msi_chip_add(&v2m->mchip);
-	if (ret) {
-		pr_err("Failed to add msi_chip.\n");
-		goto err_free_domains;
-	}
-
 	pr_info("Node %s: range[%#lx:%#lx], SPI[%d:%d]\n", node->name,
 		(unsigned long)v2m->res.start, (unsigned long)v2m->res.end,
 		v2m->spi_start, (v2m->spi_start + v2m->nr_spis));

@@ -294,10 +286,10 @@ static int __init gicv2m_init_one(struct device_node *node,
 	return 0;
 
 err_free_domains:
-	if (v2m->mchip.domain)
-		irq_domain_remove(v2m->mchip.domain);
 	if (v2m->domain)
 		irq_domain_remove(v2m->domain);
+	if (inner_domain)
+		irq_domain_remove(inner_domain);
 err_free_bm:
 	kfree(v2m->bm);
 err_iounmap:

-- 
2.1.4

The GICv3 ITS only uses the msi_controller structure as a way
to match the PHB with its MSI HW, and thus the msi_domain.
But now that we can directly associate an msi_domain with a device,
there is no use keeping this msi_controller around.

Just remove all traces of msi_controller from the driver.

Signed-off-by: Marc Zyngier <redacted>
---
 drivers/irqchip/irq-gic-v3-its.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 86e4684..2a2fb2e 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c

@@ -54,13 +54,12 @@ struct its_collection {
 
 /*
  * The ITS structure - contains most of the infrastructure, with the
- * msi_controller, the command queue, the collections, and the list of
- * devices writing to it.
+ * top-level MSI domain, the command queue, the collections, and the
+ * list of devices writing to it.
  */
 struct its_node {
 	raw_spinlock_t		lock;
 	struct list_head	entry;
-	struct msi_controller	msi_chip;
 	struct irq_domain	*domain;
 	void __iomem		*base;
 	unsigned long		phys_base;

@@ -875,7 +874,7 @@ retry_baser:
 
 		if (val != tmp) {
 			pr_err("ITS: %s: GITS_BASER%d doesn't stick: %lx %lx\n",
-			       its->msi_chip.of_node->full_name, i,
+			       its->domain->of_node->full_name, i,
 			       (unsigned long) val, (unsigned long) tmp);
 			err = -ENXIO;
 			goto out_free;

@@ -1260,6 +1259,7 @@ static int its_probe(struct device_node *node, struct irq_domain *parent)
 	struct resource res;
 	struct its_node *its;
 	void __iomem *its_base;
+	struct irq_domain *inner_domain = NULL;
 	u32 val;
 	u64 baser, tmp;
 	int err;

@@ -1296,7 +1296,6 @@ static int its_probe(struct device_node *node, struct irq_domain *parent)
 	INIT_LIST_HEAD(&its->its_device_list);
 	its->base = its_base;
 	its->phys_base = res.start;
-	its->msi_chip.of_node = node;
 	its->ite_size = ((readl_relaxed(its_base + GITS_TYPER) >> 4) & 0xf) + 1;
 
 	its->cmd_base = kzalloc(ITS_CMD_QUEUE_SZ, GFP_KERNEL);

@@ -1330,26 +1329,22 @@ static int its_probe(struct device_node *node, struct irq_domain *parent)
 		its->flags |= ITS_FLAGS_CMDQ_NEEDS_FLUSHING;
 	}
 
-	if (of_property_read_bool(its->msi_chip.of_node, "msi-controller")) {
-		its->domain = irq_domain_add_tree(NULL, &its_domain_ops, its);
-		if (!its->domain) {
+	if (of_property_read_bool(node, "msi-controller")) {
+		inner_domain = irq_domain_add_tree(NULL, &its_domain_ops, its);
+		if (!inner_domain) {
 			err = -ENOMEM;
 			goto out_free_tables;
 		}
 
-		its->domain->parent = parent;
+		inner_domain->parent = parent;
 
-		its->msi_chip.domain = pci_msi_create_irq_domain(node,
-								 &its_pci_msi_domain_info,
-								 its->domain);
-		if (!its->msi_chip.domain) {
+		its->domain = pci_msi_create_irq_domain(node,
+							&its_pci_msi_domain_info,
+							inner_domain);
+		if (!its->domain) {
 			err = -ENOMEM;
 			goto out_free_domains;
 		}
-
-		err = of_pci_msi_chip_add(&its->msi_chip);
-		if (err)
-			goto out_free_domains;
 	}
 
 	spin_lock(&its_lock);

@@ -1359,10 +1354,10 @@ static int its_probe(struct device_node *node, struct irq_domain *parent)
 	return 0;
 
 out_free_domains:
-	if (its->msi_chip.domain)
-		irq_domain_remove(its->msi_chip.domain);
 	if (its->domain)
 		irq_domain_remove(its->domain);
+	if (inner_domain)
+		irq_domain_remove(inner_domain);
 out_free_tables:
 	its_free_tables(its);
 out_free_cmd:

-- 
2.1.4

The only two users of that field are not using the msi_controller
structure anymore, so drop it altogether.

Signed-off-by: Marc Zyngier <redacted>
---
 drivers/pci/msi.c   | 2 --
 include/linux/msi.h | 3 ---
 2 files changed, 5 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 003040b..a36894f 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c

@@ -42,8 +42,6 @@ static struct irq_domain *pci_msi_get_domain(struct pci_dev *dev)
 	struct irq_domain *domain = NULL;
 
 	domain = dev_get_msi_domain(&dev->dev);
-	if (!domain && dev->bus->msi)
-		domain = dev->bus->msi->domain;
 	if (!domain)
 		domain = arch_get_pci_msi_domain(dev);
 

diff --git a/include/linux/msi.h b/include/linux/msi.h
index 8ac4a68..692f217 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h

@@ -108,9 +108,6 @@ struct msi_controller {
 	struct device *dev;
 	struct device_node *of_node;
 	struct list_head list;
-#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
-	struct irq_domain *domain;
-#endif
 
 	int (*setup_irq)(struct msi_controller *chip, struct pci_dev *dev,
 			 struct msi_desc *desc);

-- 
2.1.4

On Thu, Jan 08, 2015 at 05:06:04PM +0000, Marc Zyngier wrote:

MSI-like interrupts are starting to creep out of the PCI world, and
can now be seen into a number of "platform"-type busses. The MSI
domain patches recognise that fact, and start providing a way to
implement this.

Another problem we have to solve is to identify which MSI domain a
device is "connected" to. Currently, PCI gets away with a mixture of
arch-specific callbacks, and a msi_controller structure that can
optionally carry a pointer to an MSI domain. As we add new bus types
and start dealing with topologies that do not map to what PCI does,
this doesn't scale anymore.

This patch series tries to address some of it by providing a basic
link between 'struct device' and an MSI domain. It also adds (yet
another) way for PCI to propagate the domain pointer through the PCI
device hierarchy, provides a method for OF to kick-start the
propagation process, and finally allows the PCI/MSI layer to use that
information. Hopefully this can serve as a model to implement support
for different but types.

Additionally, the last three patches use all the above to remove any
trace of the msi_controller structure from the two GIC interrupt
controllers we use on arm64, so that they solely rely on the above
infrastructure. We take this opportunity to also kill the domain
pointer from the msi_controller structure.

My hope is to eventually kill msi_controller entirely, and only rely
on the msi_domain contained in the device structure (any help
welcomed).

This has been tested on arm64 with GICv2m (AMD Seattle) and GICv3 ITS
(FVP model).

Patches are on top of 3.19-rc3 and available at:

git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git irq/msi_domain

As always, comments most welcome.

These generally look fine to me.  I made a couple minor comments (sorry; I
replied to the v1 series, not to this one).  Since most of the recent IRQ
work has gone via you, Thomas, does it make sense for you to do these, too?

Bjorn

From v1:

- Allow arch code to set the MSI domain before we try to do so in core
  code (which is used as a fallback).
- Allow the MSI domain to be looked-up by using the PHB node.
- Remove domain field from msi_controller

[1] https://lkml.org/lkml/2014/12/8/581

Marc Zyngier (8):
  device core: Introduce per-device MSI domain pointer
  PCI/MSI: Add hooks to populate the msi_domain field
  PCI/MSI: of: Add support for OF-provided msi_domain
  PCI/MSI: of: Allow msi_domain lookup using the PHB node
  PCI/MSI: Let pci_msi_get_domain use struct device's msi_domain
  irqchip: GICv2m: Get rid of struct msi_controller
  irqchip: gicv3-its: Get rid of struct msi_controller
  PCI/MSI: Drop domain field from msi_controller

 drivers/irqchip/irq-gic-v2m.c    | 26 +++++++++-----------------
 drivers/irqchip/irq-gic-v3-its.c | 33 ++++++++++++++-------------------
 drivers/pci/msi.c                |  3 +--
 drivers/pci/of.c                 | 20 ++++++++++++++++++++
 drivers/pci/probe.c              | 31 +++++++++++++++++++++++++++++++
 include/linux/device.h           | 20 ++++++++++++++++++++
 include/linux/msi.h              |  3 ---
 include/linux/pci.h              |  3 +++
 8 files changed, 98 insertions(+), 41 deletions(-)

-- 
2.1.4