Patchwork [PATCHv5,05/11] of: pci: add registry of MSI chips

login
register
mail settings
Submitter Thomas Petazzoni
Date July 15, 2013, 11:52 a.m.
Message ID <1373889167-27878-6-git-send-email-thomas.petazzoni@free-electrons.com>
Download mbox | patch
Permalink /patch/259019/
State Not Applicable
Headers show

Comments

Thomas Petazzoni - July 15, 2013, 11:52 a.m.
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/of/of_pci.c    | 45 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/msi.h    |  2 ++
 include/linux/of_pci.h | 12 ++++++++++++
 3 files changed, 59 insertions(+)
Rob Herring - July 15, 2013, 4:12 p.m.
On 07/15/2013 06:52 AM, 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>

Acked-by: Rob Herring <rob.herring@calxeda.com>


> ---
>  drivers/of/of_pci.c    | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/msi.h    |  2 ++
>  include/linux/of_pci.h | 12 ++++++++++++
>  3 files changed, 59 insertions(+)
> 
> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> index 42c687a..e5ca008 100644
> --- a/drivers/of/of_pci.c
> +++ b/drivers/of/of_pci.c
> @@ -89,3 +89,48 @@ int of_pci_parse_bus_range(struct device_node *node, struct resource *res)
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(of_pci_parse_bus_range);
> +
> +#ifdef CONFIG_PCI_MSI
> +
> +static LIST_HEAD(of_pci_msi_chip_list);
> +static DEFINE_MUTEX(of_pci_msi_chip_mutex);
> +
> +int of_pci_msi_chip_add(struct msi_chip *chip)
> +{
> +	if (!of_property_read_bool(chip->of_node, "msi-controller"))
> +		return -EINVAL;
> +
> +	mutex_lock(&of_pci_msi_chip_mutex);
> +	list_add(&chip->list, &of_pci_msi_chip_list);
> +	mutex_unlock(&of_pci_msi_chip_mutex);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_pci_msi_chip_add);
> +
> +void of_pci_msi_chip_remove(struct msi_chip *chip)
> +{
> +	mutex_lock(&of_pci_msi_chip_mutex);
> +	list_del(&chip->list);
> +	mutex_unlock(&of_pci_msi_chip_mutex);
> +}
> +EXPORT_SYMBOL_GPL(of_pci_msi_chip_remove);
> +
> +struct msi_chip *of_pci_find_msi_chip_by_node(struct device_node *of_node)
> +{
> +	struct msi_chip *c;
> +
> +	mutex_lock(&of_pci_msi_chip_mutex);
> +	list_for_each_entry(c, &of_pci_msi_chip_list, list) {
> +		if (c->of_node == of_node) {
> +			mutex_unlock(&of_pci_msi_chip_mutex);
> +			return c;
> +		}
> +	}
> +	mutex_unlock(&of_pci_msi_chip_mutex);
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(of_pci_find_msi_chip_by_node);
> +
> +#endif /* CONFIG_PCI_MSI */
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index cbf5d05..80900e0 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -67,6 +67,8 @@ void default_teardown_msi_irqs(struct pci_dev *dev);
>  struct msi_chip {
>  	struct module *owner;
>  	struct device *dev;
> +	struct device_node *of_node;
> +	struct list_head list;
>  
>  	int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
>  			 struct msi_desc *desc);
> diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
> index 7a04826..29631eb 100644
> --- a/include/linux/of_pci.h
> +++ b/include/linux/of_pci.h
> @@ -2,6 +2,7 @@
>  #define __OF_PCI_H
>  
>  #include <linux/pci.h>
> +#include <linux/msi.h>
>  
>  struct pci_dev;
>  struct of_irq;
> @@ -13,4 +14,15 @@ struct device_node *of_pci_find_child_device(struct device_node *parent,
>  int of_pci_get_devfn(struct device_node *np);
>  int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
>  
> +#if defined(CONFIG_OF) && defined(CONFIG_PCI_MSI)
> +int of_pci_msi_chip_add(struct msi_chip *chip);
> +void of_pci_msi_chip_remove(struct msi_chip *chip);
> +struct msi_chip *of_pci_find_msi_chip_by_node(struct device_node *of_node);
> +#else
> +static inline int of_pci_msi_chip_add(struct msi_chip *chip) { return -EINVAL; }
> +static inline void of_pci_msi_chip_remove(struct msi_chip *chip) { }
> +static inline struct msi_chip *
> +of_pci_find_msi_chip_by_node(struct device_node *of_node) { return NULL };
> +#endif
> +
>  #endif
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Grant Likely - July 28, 2013, 4:33 a.m.
On Mon, 15 Jul 2013 11:12:42 -0500, Rob Herring <robherring2@gmail.com> wrote:
> On 07/15/2013 06:52 AM, 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>
> 
> Acked-by: Rob Herring <rob.herring@calxeda.com>

Actually, I'm going to disagree on this one and say NAK. I don't think
it is a good idea to create a completely separate registry of msi_chips
for binding to dt nodes. I think it would be better to include the
msi_chip pointer directly into the irq_domain which has to be there
anyway. It then becomes another feature for irq controllers if it can
support doing MSI.

In fact, I would even go so far as to say it would make sense for the
msi_chip functionality to be rolled directly into the irq_domain without
a separate structure; but I'm not nacking on that point.

g.

> 
> 
> > ---
> >  drivers/of/of_pci.c    | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/msi.h    |  2 ++
> >  include/linux/of_pci.h | 12 ++++++++++++
> >  3 files changed, 59 insertions(+)
> > 
> > diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> > index 42c687a..e5ca008 100644
> > --- a/drivers/of/of_pci.c
> > +++ b/drivers/of/of_pci.c
> > @@ -89,3 +89,48 @@ int of_pci_parse_bus_range(struct device_node *node, struct resource *res)
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(of_pci_parse_bus_range);
> > +
> > +#ifdef CONFIG_PCI_MSI
> > +
> > +static LIST_HEAD(of_pci_msi_chip_list);
> > +static DEFINE_MUTEX(of_pci_msi_chip_mutex);
> > +
> > +int of_pci_msi_chip_add(struct msi_chip *chip)
> > +{
> > +	if (!of_property_read_bool(chip->of_node, "msi-controller"))
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&of_pci_msi_chip_mutex);
> > +	list_add(&chip->list, &of_pci_msi_chip_list);
> > +	mutex_unlock(&of_pci_msi_chip_mutex);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(of_pci_msi_chip_add);
> > +
> > +void of_pci_msi_chip_remove(struct msi_chip *chip)
> > +{
> > +	mutex_lock(&of_pci_msi_chip_mutex);
> > +	list_del(&chip->list);
> > +	mutex_unlock(&of_pci_msi_chip_mutex);
> > +}
> > +EXPORT_SYMBOL_GPL(of_pci_msi_chip_remove);
> > +
> > +struct msi_chip *of_pci_find_msi_chip_by_node(struct device_node *of_node)
> > +{
> > +	struct msi_chip *c;
> > +
> > +	mutex_lock(&of_pci_msi_chip_mutex);
> > +	list_for_each_entry(c, &of_pci_msi_chip_list, list) {
> > +		if (c->of_node == of_node) {
> > +			mutex_unlock(&of_pci_msi_chip_mutex);
> > +			return c;
> > +		}
> > +	}
> > +	mutex_unlock(&of_pci_msi_chip_mutex);
> > +
> > +	return NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(of_pci_find_msi_chip_by_node);
> > +
> > +#endif /* CONFIG_PCI_MSI */
> > diff --git a/include/linux/msi.h b/include/linux/msi.h
> > index cbf5d05..80900e0 100644
> > --- a/include/linux/msi.h
> > +++ b/include/linux/msi.h
> > @@ -67,6 +67,8 @@ void default_teardown_msi_irqs(struct pci_dev *dev);
> >  struct msi_chip {
> >  	struct module *owner;
> >  	struct device *dev;
> > +	struct device_node *of_node;
> > +	struct list_head list;
> >  
> >  	int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
> >  			 struct msi_desc *desc);
> > diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
> > index 7a04826..29631eb 100644
> > --- a/include/linux/of_pci.h
> > +++ b/include/linux/of_pci.h
> > @@ -2,6 +2,7 @@
> >  #define __OF_PCI_H
> >  
> >  #include <linux/pci.h>
> > +#include <linux/msi.h>
> >  
> >  struct pci_dev;
> >  struct of_irq;
> > @@ -13,4 +14,15 @@ struct device_node *of_pci_find_child_device(struct device_node *parent,
> >  int of_pci_get_devfn(struct device_node *np);
> >  int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
> >  
> > +#if defined(CONFIG_OF) && defined(CONFIG_PCI_MSI)
> > +int of_pci_msi_chip_add(struct msi_chip *chip);
> > +void of_pci_msi_chip_remove(struct msi_chip *chip);
> > +struct msi_chip *of_pci_find_msi_chip_by_node(struct device_node *of_node);
> > +#else
> > +static inline int of_pci_msi_chip_add(struct msi_chip *chip) { return -EINVAL; }
> > +static inline void of_pci_msi_chip_remove(struct msi_chip *chip) { }
> > +static inline struct msi_chip *
> > +of_pci_find_msi_chip_by_node(struct device_node *of_node) { return NULL };
> > +#endif
> > +
> >  #endif
> > 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Petazzoni - July 28, 2013, 2:27 p.m.
Dear Grant Likely,

Thanks for your feedback! Some comments below.

On Sat, 27 Jul 2013 22:33:10 -0600, Grant Likely wrote:

> > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > 
> > Acked-by: Rob Herring <rob.herring@calxeda.com>
> 
> Actually, I'm going to disagree on this one and say NAK. I don't think
> it is a good idea to create a completely separate registry of msi_chips
> for binding to dt nodes. I think it would be better to include the
> msi_chip pointer directly into the irq_domain which has to be there
> anyway. It then becomes another feature for irq controllers if it can
> support doing MSI.

The problem that this patch tries to solve is how can the PCIe driver
work get a pointer to the msi_chip structure from the DT device node
pointed to by the 'msi-parent' property. I.e, we have:

	interrupt-parent = <&mpic>;

	mpic {
		reg = <...>;
		compatible = "...";
		interrupt-controller;
		msi-controller;
	};

	pcie-controller {
		msi-parent = <&mpic>;
	};

The 'mpic' driver registers two irq_domains, one for the "normal"
interrupts, and one for the MSI interrupts. Both irq_domain cannot be
associated to the same &mpic node, or the irq_domain lookup for
interrupt-parent and msi-parent is going to be confused.

In the very first version of this patch set, I was using two separate
DT device nodes to avoid this problem:

	interrupt-parent = <&mpic>;

	interrupt-controller {
		reg = <...>;
		compatible = "...";

		mpic {
			interrupt-controller;
		};

		msi {
			msi-controller;
		};
	};

	pcie-controller {
		msi-parent = <&msi>;
	};

This way, each of the two irq_domain was associated to a distinct DT
device node, and everything was working fine. But during the review, I
was pointed by Arnd that it wasn't the proper way of describing the
interrupt controller, and that there should be only one DT device node
having both the interrupt-controller and msi-controller roles.

So what is your suggestion to allow the PCIe controller to retrieve the
correct irq_domain if we have only one DT node for the IRQ controller
that registers two irq_domains ?

See:

   [RFCv1 00/11] MSI support for Marvell EBU PCIe driver
   https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-March/030578.html

and particularly:

   [RFCv1 07/11] irqchip: armada-370-xp: add MSI support to interrupt
   controller driver
   https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-March/030584.html

Thanks,

Thomas
Thierry Reding - July 29, 2013, 6:54 a.m.
On Sun, Jul 28, 2013 at 04:27:11PM +0200, Thomas Petazzoni wrote:
> Dear Grant Likely,
> 
> Thanks for your feedback! Some comments below.
> 
> On Sat, 27 Jul 2013 22:33:10 -0600, Grant Likely wrote:
> 
> > > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > > 
> > > Acked-by: Rob Herring <rob.herring@calxeda.com>
> > 
> > Actually, I'm going to disagree on this one and say NAK. I don't think
> > it is a good idea to create a completely separate registry of msi_chips
> > for binding to dt nodes. I think it would be better to include the
> > msi_chip pointer directly into the irq_domain which has to be there
> > anyway. It then becomes another feature for irq controllers if it can
> > support doing MSI.
> 
> The problem that this patch tries to solve is how can the PCIe driver
> work get a pointer to the msi_chip structure from the DT device node
> pointed to by the 'msi-parent' property. I.e, we have:
> 
> 	interrupt-parent = <&mpic>;
> 
> 	mpic {
> 		reg = <...>;
> 		compatible = "...";
> 		interrupt-controller;
> 		msi-controller;
> 	};
> 
> 	pcie-controller {
> 		msi-parent = <&mpic>;
> 	};
> 
> The 'mpic' driver registers two irq_domains, one for the "normal"
> interrupts, and one for the MSI interrupts. Both irq_domain cannot be
> associated to the same &mpic node, or the irq_domain lookup for
> interrupt-parent and msi-parent is going to be confused.
> 
> In the very first version of this patch set, I was using two separate
> DT device nodes to avoid this problem:
> 
> 	interrupt-parent = <&mpic>;
> 
> 	interrupt-controller {
> 		reg = <...>;
> 		compatible = "...";
> 
> 		mpic {
> 			interrupt-controller;
> 		};
> 
> 		msi {
> 			msi-controller;
> 		};
> 	};
> 
> 	pcie-controller {
> 		msi-parent = <&msi>;
> 	};
> 
> This way, each of the two irq_domain was associated to a distinct DT
> device node, and everything was working fine. But during the review, I
> was pointed by Arnd that it wasn't the proper way of describing the
> interrupt controller, and that there should be only one DT device node
> having both the interrupt-controller and msi-controller roles.
> 
> So what is your suggestion to allow the PCIe controller to retrieve the
> correct irq_domain if we have only one DT node for the IRQ controller
> that registers two irq_domains ?

If I understand correctly, Grant isn't objecting to the introduction of
the lookup function, but rather its implementation. You could add a
pointer to a struct msi_chip within struct irq_domain and then iterate
over all irq_domain instances (see irq_find_host()) and find one which
has the correct device_node pointer and the msi_chip pointer set.

So I think that pretty much boils down to setting the (new) .msi_chip
field of armada_370_xp_msi_domain to the newly allocated MSI chip in
armada_370_xp_msi_init() and modify the lookup function to use the
irq_domain_list instead of the list of of_pci_msi_chip_list(). And a
whole lot of the infrastructure code can go away because it's already
there for irq_domain.

The good thing about it is that it couples the MSI chip more strongly
with its associated IRQ domain. And as a bonus we get to omit the list
and of_node fields from struct msi_chip. =)

Thierry

Patch

diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 42c687a..e5ca008 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -89,3 +89,48 @@  int of_pci_parse_bus_range(struct device_node *node, struct resource *res)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_pci_parse_bus_range);
+
+#ifdef CONFIG_PCI_MSI
+
+static LIST_HEAD(of_pci_msi_chip_list);
+static DEFINE_MUTEX(of_pci_msi_chip_mutex);
+
+int of_pci_msi_chip_add(struct msi_chip *chip)
+{
+	if (!of_property_read_bool(chip->of_node, "msi-controller"))
+		return -EINVAL;
+
+	mutex_lock(&of_pci_msi_chip_mutex);
+	list_add(&chip->list, &of_pci_msi_chip_list);
+	mutex_unlock(&of_pci_msi_chip_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_pci_msi_chip_add);
+
+void of_pci_msi_chip_remove(struct msi_chip *chip)
+{
+	mutex_lock(&of_pci_msi_chip_mutex);
+	list_del(&chip->list);
+	mutex_unlock(&of_pci_msi_chip_mutex);
+}
+EXPORT_SYMBOL_GPL(of_pci_msi_chip_remove);
+
+struct msi_chip *of_pci_find_msi_chip_by_node(struct device_node *of_node)
+{
+	struct msi_chip *c;
+
+	mutex_lock(&of_pci_msi_chip_mutex);
+	list_for_each_entry(c, &of_pci_msi_chip_list, list) {
+		if (c->of_node == of_node) {
+			mutex_unlock(&of_pci_msi_chip_mutex);
+			return c;
+		}
+	}
+	mutex_unlock(&of_pci_msi_chip_mutex);
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(of_pci_find_msi_chip_by_node);
+
+#endif /* CONFIG_PCI_MSI */
diff --git a/include/linux/msi.h b/include/linux/msi.h
index cbf5d05..80900e0 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -67,6 +67,8 @@  void default_teardown_msi_irqs(struct pci_dev *dev);
 struct msi_chip {
 	struct module *owner;
 	struct device *dev;
+	struct device_node *of_node;
+	struct list_head list;
 
 	int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
 			 struct msi_desc *desc);
diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
index 7a04826..29631eb 100644
--- a/include/linux/of_pci.h
+++ b/include/linux/of_pci.h
@@ -2,6 +2,7 @@ 
 #define __OF_PCI_H
 
 #include <linux/pci.h>
+#include <linux/msi.h>
 
 struct pci_dev;
 struct of_irq;
@@ -13,4 +14,15 @@  struct device_node *of_pci_find_child_device(struct device_node *parent,
 int of_pci_get_devfn(struct device_node *np);
 int of_pci_parse_bus_range(struct device_node *node, struct resource *res);
 
+#if defined(CONFIG_OF) && defined(CONFIG_PCI_MSI)
+int of_pci_msi_chip_add(struct msi_chip *chip);
+void of_pci_msi_chip_remove(struct msi_chip *chip);
+struct msi_chip *of_pci_find_msi_chip_by_node(struct device_node *of_node);
+#else
+static inline int of_pci_msi_chip_add(struct msi_chip *chip) { return -EINVAL; }
+static inline void of_pci_msi_chip_remove(struct msi_chip *chip) { }
+static inline struct msi_chip *
+of_pci_find_msi_chip_by_node(struct device_node *of_node) { return NULL };
+#endif
+
 #endif