| Message ID | 1373889167-27878-6-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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
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
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
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
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
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(+)