[RFC] [PATCH 0/3] of: allow to support irq domain register as platform_device for platform_device

[RFC] [PATCH 0/3] of: allow to support irq domain register as platform_device for platform_device

[Jean-Christophe PLAGNIOL-VILLARD]

Hi,

	Today in the current of implementation we populate all the ressources
	at of_platform_populate time. But this leed to a chicken-egg dilemat
	some the irq present in DT are from platform_device too. And you can
	not resolve them as of_platform_populate. So delay the populate of irq
	at platform_drv_probe.

	And if the irq_domain is not yet present just defer the probe (GPIO as example)

	we still have one issue is that the gpio irq provide by DT need to have
	the gpio requested and configured which I'll fix later by handling this automatically
	by a gpio_irq_chip and dorp this from every drivers

The following changes since commit e4aa937ec75df0eea0bee03bffa3303ad36c986b:

  Linux 3.10-rc3 (2013-05-26 16:00:47 -0700)

are available in the git repository at:

  git://github.com/at91linux/linux-at91.git j/for-3.11-platform_irq_resolve

for you to fetch changes up to 898eab5ad414bfc48dd1160369965a677f135d26:

  IRQ: irq domain: defer of irq ressoure resolve at platform_drv_probe (2013-05-28 05:58:31 +0800)

----------------------------------------------------------------
Jean-Christophe PLAGNIOL-VILLARD (3):
      of: irq: rename of_irq_count to of_irq_valid_count
      of: add of_irq_count: Count the number of IRQs a node present
      IRQ: irq domain: defer of irq ressoure resolve at platform_drv_probe

 arch/mips/lantiq/irq.c           |    2 +-
 arch/powerpc/sysdev/ppc4xx_msi.c |    2 +-
 drivers/base/platform.c          |    5 +++++
 drivers/clocksource/exynos_mct.c |    2 +-
 drivers/gpio/gpio-mvebu.c        |    2 +-
 drivers/irqchip/irq-vt8500.c     |    4 ++--
 drivers/of/irq.c                 |   15 +++++++++++++++
 drivers/of/platform.c            |   27 ++++++++++++++++++++++++++-
 include/linux/of_irq.h           |    1 +
 include/linux/of_platform.h      |    7 +++++++
 10 files changed, 60 insertions(+), 7 deletions(-)

Best Regards,
J.

[RFC] [PATCH 1/3] of: irq: rename of_irq_count to of_irq_valid_count

[Jean-Christophe PLAGNIOL-VILLARD]

as we count valid mapped irq not just present irq

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
Cc: Ralf Baechle <ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
Cc: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
---
 arch/mips/lantiq/irq.c           |    2 +-
 arch/powerpc/sysdev/ppc4xx_msi.c |    2 +-
 drivers/clocksource/exynos_mct.c |    2 +-
 drivers/gpio/gpio-mvebu.c        |    2 +-
 drivers/irqchip/irq-vt8500.c     |    4 ++--
 drivers/of/irq.c                 |    4 ++--
 drivers/of/platform.c            |    2 +-
 include/linux/of_irq.h           |    2 +-
 8 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/mips/lantiq/irq.c b/arch/mips/lantiq/irq.c
index 5119487..f1f54bd 100644
--- a/arch/mips/lantiq/irq.c
+++ b/arch/mips/lantiq/irq.c
@@ -382,7 +382,7 @@ int __init icu_of_init(struct device_node *node, struct device_node *parent)
 	eiu_node = of_find_compatible_node(NULL, NULL, "lantiq,eiu-xway");
 	if (eiu_node && !of_address_to_resource(eiu_node, 0, &res)) {
 		/* find out how many external irq sources we have */
-		exin_avail = of_irq_count(eiu_node);
+		exin_avail = of_irq_valid_count(eiu_node);
 
 		if (exin_avail > MAX_EIU)
 			exin_avail = MAX_EIU;
diff --git a/arch/powerpc/sysdev/ppc4xx_msi.c b/arch/powerpc/sysdev/ppc4xx_msi.c
index 43948da..b836b68 100644
--- a/arch/powerpc/sysdev/ppc4xx_msi.c
+++ b/arch/powerpc/sysdev/ppc4xx_msi.c
@@ -243,7 +243,7 @@ static int ppc4xx_msi_probe(struct platform_device *dev)
 		goto error_out;
 	}
 
-	msi_irqs = of_irq_count(dev->dev.of_node);
+	msi_irqs = of_irq_valid_count(dev->dev.of_node);
 	if (!msi_irqs)
 		return -ENODEV;
 
diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index 662fcc0..a0bee5b 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -532,7 +532,7 @@ static void __init mct_init_dt(struct device_node *np, unsigned int int_type)
 	 * irqs are specified.
 	 */
 #ifdef CONFIG_OF
-	nr_irqs = of_irq_count(np);
+	nr_irqs = of_irq_valid_count(np);
 #else
 	nr_irqs = 0;
 #endif
diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index bf69a7e..e3e4f05 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -668,7 +668,7 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 	gpiochip_add(&mvchip->chip);
 
 	/* Some gpio controllers do not provide irq support */
-	if (!of_irq_count(np))
+	if (!of_irq_valid_count(np))
 		return 0;
 
 	/* Setup the interrupt handlers. Each chip can have up to 4
diff --git a/drivers/irqchip/irq-vt8500.c b/drivers/irqchip/irq-vt8500.c
index d970595..bf1d6e7 100644
--- a/drivers/irqchip/irq-vt8500.c
+++ b/drivers/irqchip/irq-vt8500.c
@@ -237,9 +237,9 @@ int __init vt8500_irq_init(struct device_node *node, struct device_node *parent)
 	active_cnt++;
 
 	/* check if this is a slaved controller */
-	if (of_irq_count(np) != 0) {
+	if (of_irq_valid_count(np) != 0) {
 		/* check that we have the correct number of interrupts */
-		if (of_irq_count(np) != 8) {
+		if (of_irq_valid_count(np) != 8) {
 			pr_err("%s: Incorrect IRQ map for slaved controller\n",
 					__func__);
 			return -EINVAL;
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index a3c1c5a..d1c5825 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -362,10 +362,10 @@ int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
 EXPORT_SYMBOL_GPL(of_irq_to_resource);
 
 /**
- * of_irq_count - Count the number of IRQs a node uses
+ * of_irq_valid_count - Count the number of mapped IRQs a node uses
  * @dev: pointer to device tree node
  */
-int of_irq_count(struct device_node *dev)
+int of_irq_valid_count(struct device_node *dev)
 {
 	int nr = 0;
 
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index e0a6514..00a0971 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -152,7 +152,7 @@ struct platform_device *of_device_alloc(struct device_node *np,
 	if (of_can_translate_address(np))
 		while (of_address_to_resource(np, num_reg, &temp_res) == 0)
 			num_reg++;
-	num_irq = of_irq_count(np);
+	num_irq = of_irq_valid_count(np);
 
 	/* Populate the resource table */
 	if (num_irq || num_reg) {
diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index 535cecf..88bb58c 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -68,7 +68,7 @@ extern unsigned int irq_create_of_mapping(struct device_node *controller,
 					  unsigned int intsize);
 extern int of_irq_to_resource(struct device_node *dev, int index,
 			      struct resource *r);
-extern int of_irq_count(struct device_node *dev);
+extern int of_irq_valid_count(struct device_node *dev);
 extern int of_irq_to_resource_table(struct device_node *dev,
 		struct resource *res, int nr_irqs);
 extern struct device_node *of_irq_find_parent(struct device_node *child);
-- 
1.7.10.4

[RFC] [PATCH 2/3] of: add of_irq_count: Count the number of IRQs a node present

[Jean-Christophe PLAGNIOL-VILLARD]

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
Cc: Ralf Baechle <ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
Cc: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
---
 drivers/of/irq.c       |   15 +++++++++++++++
 include/linux/of_irq.h |    1 +
 2 files changed, 16 insertions(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index d1c5825..4426223 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -362,6 +362,21 @@ int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
 EXPORT_SYMBOL_GPL(of_irq_to_resource);
 
 /**
+ * of_irq_count - Count the number of IRQs a node uses
+ * @dev: pointer to device tree node
+ */
+int of_irq_count(struct device_node *dev)
+{
+	struct of_irq oirq;
+	int nr = 0;
+
+	while (!of_irq_map_one(dev, nr, &oirq))
+		nr++;
+
+	return nr;
+}
+
+/**
  * of_irq_valid_count - Count the number of mapped IRQs a node uses
  * @dev: pointer to device tree node
  */
diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index 88bb58c..86b78ef 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -68,6 +68,7 @@ extern unsigned int irq_create_of_mapping(struct device_node *controller,
 					  unsigned int intsize);
 extern int of_irq_to_resource(struct device_node *dev, int index,
 			      struct resource *r);
+extern int of_irq_count(struct device_node *dev);
 extern int of_irq_valid_count(struct device_node *dev);
 extern int of_irq_to_resource_table(struct device_node *dev,
 		struct resource *res, int nr_irqs);
-- 
1.7.10.4

Re: [RFC] [PATCH 2/3] of: add of_irq_count: Count the number of IRQs a node present

[Grant Likely]

On Tue, 28 May 2013 17:08:48 +0200, Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org> wrote:
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> Cc: Ralf Baechle <ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
> Cc: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/of/irq.c       |   15 +++++++++++++++
>  include/linux/of_irq.h |    1 +
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index d1c5825..4426223 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -362,6 +362,21 @@ int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
>  EXPORT_SYMBOL_GPL(of_irq_to_resource);
>  
>  /**
> + * of_irq_count - Count the number of IRQs a node uses
> + * @dev: pointer to device tree node
> + */
> +int of_irq_count(struct device_node *dev)

Following on from my comment on patch 1/3, How about "of_irq_count_entries()"?

> +{
> +	struct of_irq oirq;
> +	int nr = 0;
> +
> +	while (!of_irq_map_one(dev, nr, &oirq))
> +		nr++;

The concept is good, but this ends up being very inefficient. It would
be better to take the relevant parts out of of_irq_map_one() to retrieve
the value of #interrupt-cells and compare it to the size of the
interrupts property.

g.

Re: [RFC] [PATCH 2/3] of: add of_irq_count: Count the number of IRQs a node present

[Jean-Christophe PLAGNIOL-VILLARD]

On 23:33 Wed 29 May     , Grant Likely wrote:
> On Tue, 28 May 2013 17:08:48 +0200, Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org> wrote:
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> > Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> > Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> > Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> > Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> > Cc: Ralf Baechle <ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
> > Cc: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> > ---
> >  drivers/of/irq.c       |   15 +++++++++++++++
> >  include/linux/of_irq.h |    1 +
> >  2 files changed, 16 insertions(+)
> > 
> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > index d1c5825..4426223 100644
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> > @@ -362,6 +362,21 @@ int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
> >  EXPORT_SYMBOL_GPL(of_irq_to_resource);
> >  
> >  /**
> > + * of_irq_count - Count the number of IRQs a node uses
> > + * @dev: pointer to device tree node
> > + */
> > +int of_irq_count(struct device_node *dev)
> 
> Following on from my comment on patch 1/3, How about "of_irq_count_entries()"?

yeah look much better

so I drop the patch 1/3
> 
> > +{
> > +	struct of_irq oirq;
> > +	int nr = 0;
> > +
> > +	while (!of_irq_map_one(dev, nr, &oirq))
> > +		nr++;
> 
> The concept is good, but this ends up being very inefficient. It would
> be better to take the relevant parts out of of_irq_map_one() to retrieve
> the value of #interrupt-cells and compare it to the size of the
> interrupts property.

ok

Best Regards,
J.

[RFC] [PATCH 3/3] IRQ: irq domain: defer of irq ressoure resolve at platform_drv_probe

[Jean-Christophe PLAGNIOL-VILLARD]

Today in the current of implementation we populate all the ressources
at of_platform_populate time. But this leed to a chicken-egg dilemat
some the irq present in DT are from platform_device too. And you can
not resolve them as of_platform_populate. So delay the populate of irq
at platform_drv_probe.

And if the irq_domain is not yet present just defer the probe (GPIO as example)

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
Cc: Ralf Baechle <ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
Cc: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
---
 drivers/base/platform.c     |    5 +++++
 drivers/of/platform.c       |   29 +++++++++++++++++++++++++++--
 include/linux/of_platform.h |    7 +++++++
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 9eda842..c0f7906 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -13,6 +13,7 @@
 #include <linux/string.h>
 #include <linux/platform_device.h>
 #include <linux/of_device.h>
+#include <linux/of_platform.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/dma-mapping.h>
@@ -484,6 +485,10 @@ static int platform_drv_probe(struct device *_dev)
 	struct platform_device *dev = to_platform_device(_dev);
 	int ret;
 
+	ret = of_device_init_irq(dev);
+	if (ret)
+		return ret;
+
 	if (ACPI_HANDLE(_dev))
 		acpi_dev_pm_attach(_dev, true);
 
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 00a0971..c290943 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -131,6 +131,32 @@ void of_device_make_bus_id(struct device *dev)
 }
 
 /**
+ * of_device_alloc_irq - initialize irq of an platfrom_device
+ * @dev: plaform_device to work on
+ */
+int of_device_init_irq(struct platform_device *dev)
+{
+	struct device_node *np = dev->dev.of_node;
+	int num_irq;
+	int ret;
+	struct resource *res = dev->resource;
+
+	if (!np)
+		return 0;
+
+	num_irq = of_irq_count(np);
+	if (!num_irq)
+		return 0;
+
+	res += dev->num_resources - num_irq;
+	ret = of_irq_to_resource_table(np, res, num_irq);
+	if (ret != num_irq)
+		return -EPROBE_DEFER;
+
+	return 0;
+}
+
+/**
  * of_device_alloc - Allocate and initialize an of_device
  * @np: device node to assign to device
  * @bus_id: Name to assign to the device.  May be null to use default name.
@@ -152,7 +178,7 @@ struct platform_device *of_device_alloc(struct device_node *np,
 	if (of_can_translate_address(np))
 		while (of_address_to_resource(np, num_reg, &temp_res) == 0)
 			num_reg++;
-	num_irq = of_irq_valid_count(np);
+	num_irq = of_irq_count(np);
 
 	/* Populate the resource table */
 	if (num_irq || num_reg) {
@@ -168,7 +194,6 @@ struct platform_device *of_device_alloc(struct device_node *np,
 			rc = of_address_to_resource(np, i, res);
 			WARN_ON(rc);
 		}
-		WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
 	}
 
 	dev->dev.of_node = of_node_get(np);
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
index 3863a4d..b5f9bb6 100644
--- a/include/linux/of_platform.h
+++ b/include/linux/of_platform.h
@@ -79,6 +79,7 @@ extern const struct of_device_id of_default_bus_match_table[];
 extern struct platform_device *of_device_alloc(struct device_node *np,
 					 const char *bus_id,
 					 struct device *parent);
+extern int of_device_init_irq(struct platform_device *dev);
 extern struct platform_device *of_find_device_by_node(struct device_node *np);
 
 #ifdef CONFIG_OF_ADDRESS /* device reg helpers depend on OF_ADDRESS */
@@ -101,6 +102,7 @@ extern int of_platform_populate(struct device_node *root,
 #if !defined(CONFIG_OF_ADDRESS)
 struct of_dev_auxdata;
 struct device;
+struct platform_device;
 static inline int of_platform_populate(struct device_node *root,
 					const struct of_device_id *matches,
 					const struct of_dev_auxdata *lookup,
@@ -108,6 +110,11 @@ static inline int of_platform_populate(struct device_node *root,
 {
 	return -ENODEV;
 }
+
+static inline int of_device_init_irq(struct platform_device *dev)
+{
+	return 0;
+}
 #endif /* !CONFIG_OF_ADDRESS */
 
 #endif	/* _LINUX_OF_PLATFORM_H */
-- 
1.7.10.4

Re: [RFC] [PATCH 3/3] IRQ: irq domain: defer of irq ressoure resolve at platform_drv_probe

[Rob Herring]

On 05/28/2013 10:08 AM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> Today in the current of implementation we populate all the ressources

typo

> at of_platform_populate time. But this leed to a chicken-egg dilemat

2 typos

> some the irq present in DT are from platform_device too. And you can
> not resolve them as of_platform_populate. So delay the populate of irq
> at platform_drv_probe.
> 
> And if the irq_domain is not yet present just defer the probe (GPIO as example)
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> Cc: Ralf Baechle <ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
> Cc: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/base/platform.c     |    5 +++++
>  drivers/of/platform.c       |   29 +++++++++++++++++++++++++++--
>  include/linux/of_platform.h |    7 +++++++
>  3 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 9eda842..c0f7906 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -13,6 +13,7 @@
>  #include <linux/string.h>
>  #include <linux/platform_device.h>
>  #include <linux/of_device.h>
> +#include <linux/of_platform.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/dma-mapping.h>
> @@ -484,6 +485,10 @@ static int platform_drv_probe(struct device *_dev)
>  	struct platform_device *dev = to_platform_device(_dev);
>  	int ret;
>  
> +	ret = of_device_init_irq(dev);
> +	if (ret)
> +		return ret;
> +
>  	if (ACPI_HANDLE(_dev))
>  		acpi_dev_pm_attach(_dev, true);
>  
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 00a0971..c290943 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -131,6 +131,32 @@ void of_device_make_bus_id(struct device *dev)
>  }
>  
>  /**
> + * of_device_alloc_irq - initialize irq of an platfrom_device

typo

> + * @dev: plaform_device to work on

same word, different typo.

> + */
> +int of_device_init_irq(struct platform_device *dev)
> +{
> +	struct device_node *np = dev->dev.of_node;
> +	int num_irq;
> +	int ret;
> +	struct resource *res = dev->resource;
> +
> +	if (!np)
> +		return 0;
> +
> +	num_irq = of_irq_count(np);
> +	if (!num_irq)
> +		return 0;
> +
> +	res += dev->num_resources - num_irq;

This looks fragile to me as it relies on how of_device_alloc happens to
be implemented.

> +	ret = of_irq_to_resource_table(np, res, num_irq);
> +	if (ret != num_irq)
> +		return -EPROBE_DEFER;
> +
> +	return 0;
> +}
> +
> +/**
>   * of_device_alloc - Allocate and initialize an of_device
>   * @np: device node to assign to device
>   * @bus_id: Name to assign to the device.  May be null to use default name.
> @@ -152,7 +178,7 @@ struct platform_device *of_device_alloc(struct device_node *np,
>  	if (of_can_translate_address(np))
>  		while (of_address_to_resource(np, num_reg, &temp_res) == 0)
>  			num_reg++;
> -	num_irq = of_irq_valid_count(np);
> +	num_irq = of_irq_count(np);
>  
>  	/* Populate the resource table */
>  	if (num_irq || num_reg) {
> @@ -168,7 +194,6 @@ struct platform_device *of_device_alloc(struct device_node *np,
>  			rc = of_address_to_resource(np, i, res);
>  			WARN_ON(rc);
>  		}
> -		WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
>  	}
>  
>  	dev->dev.of_node = of_node_get(np);
> diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
> index 3863a4d..b5f9bb6 100644
> --- a/include/linux/of_platform.h
> +++ b/include/linux/of_platform.h
> @@ -79,6 +79,7 @@ extern const struct of_device_id of_default_bus_match_table[];
>  extern struct platform_device *of_device_alloc(struct device_node *np,
>  					 const char *bus_id,
>  					 struct device *parent);
> +extern int of_device_init_irq(struct platform_device *dev);
>  extern struct platform_device *of_find_device_by_node(struct device_node *np);
>  
>  #ifdef CONFIG_OF_ADDRESS /* device reg helpers depend on OF_ADDRESS */
> @@ -101,6 +102,7 @@ extern int of_platform_populate(struct device_node *root,
>  #if !defined(CONFIG_OF_ADDRESS)
>  struct of_dev_auxdata;
>  struct device;
> +struct platform_device;
>  static inline int of_platform_populate(struct device_node *root,
>  					const struct of_device_id *matches,
>  					const struct of_dev_auxdata *lookup,
> @@ -108,6 +110,11 @@ static inline int of_platform_populate(struct device_node *root,
>  {
>  	return -ENODEV;
>  }
> +
> +static inline int of_device_init_irq(struct platform_device *dev)
> +{
> +	return 0;
> +}
>  #endif /* !CONFIG_OF_ADDRESS */
>  
>  #endif	/* _LINUX_OF_PLATFORM_H */
> 

Re: [RFC] [PATCH 3/3] IRQ: irq domain: defer of irq ressoure resolve at platform_drv_probe

[Jean-Christophe PLAGNIOL-VILLARD]

> > + */
> > +int of_device_init_irq(struct platform_device *dev)
> > +{
> > +	struct device_node *np = dev->dev.of_node;
> > +	int num_irq;
> > +	int ret;
> > +	struct resource *res = dev->resource;
> > +
> > +	if (!np)
> > +		return 0;
> > +
> > +	num_irq = of_irq_count(np);
> > +	if (!num_irq)
> > +		return 0;
> > +
> > +	res += dev->num_resources - num_irq;
> 
> This looks fragile to me as it relies on how of_device_alloc happens to
> be implemented.
> 
> > +	ret = of_irq_to_resource_table(np, res, num_irq);
> > +	if (ret != num_irq)
> > +		return -EPROBE_DEFER;

I could reduce the depency on the resource allocation order
but I do want to preallocate the ressource

Best Regards,
J.

Re: [RFC] [PATCH 3/3] IRQ: irq domain: defer of irq ressoure resolve at platform_drv_probe

[Grant Likely]

On Tue, 28 May 2013 17:08:49 +0200, Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org> wrote:
> Today in the current of implementation we populate all the ressources
> at of_platform_populate time. But this leed to a chicken-egg dilemat
> some the irq present in DT are from platform_device too. And you can
> not resolve them as of_platform_populate. So delay the populate of irq
> at platform_drv_probe.
> 
> And if the irq_domain is not yet present just defer the probe (GPIO as example)

Hi Jean-Christophe,

Thanks for looking at this.

Hmmm, this is an interesting idea. I had been thinking about taping into
the platform_get_irq() call to lookup irq numbers at probe time, but
that assumes that all platform drivers use that call (which they clearly
do not). This approach should work for all drivers as is.

However care needs to be taken to not waste too much time processing
irqs every time probe is called. Mapping irqs over and over will get
expensive, especially with the current code. It would be better to
rework the mapping function to iterate once over the interrupts property
and map all the IRQs at once instead of calling of_irq_to_resource()
over and over again. (I would however accept the argument that this is a
bug fix and the refactoring should be a separate patch)

> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> Cc: Ralf Baechle <ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
> Cc: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/base/platform.c     |    5 +++++
>  drivers/of/platform.c       |   29 +++++++++++++++++++++++++++--
>  include/linux/of_platform.h |    7 +++++++
>  3 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 9eda842..c0f7906 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -13,6 +13,7 @@
>  #include <linux/string.h>
>  #include <linux/platform_device.h>
>  #include <linux/of_device.h>
> +#include <linux/of_platform.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/dma-mapping.h>
> @@ -484,6 +485,10 @@ static int platform_drv_probe(struct device *_dev)
>  	struct platform_device *dev = to_platform_device(_dev);
>  	int ret;
>  
> +	ret = of_device_init_irq(dev);
> +	if (ret)
> +		return ret;
> +
>  	if (ACPI_HANDLE(_dev))
>  		acpi_dev_pm_attach(_dev, true);
>  
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 00a0971..c290943 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -131,6 +131,32 @@ void of_device_make_bus_id(struct device *dev)
>  }
>  
>  /**
> + * of_device_alloc_irq - initialize irq of an platfrom_device
> + * @dev: plaform_device to work on
> + */
> +int of_device_init_irq(struct platform_device *dev)
> +{
> +	struct device_node *np = dev->dev.of_node;
> +	int num_irq;
> +	int ret;
> +	struct resource *res = dev->resource;
> +
> +	if (!np)
> +		return 0;
> +
> +	num_irq = of_irq_count(np);
> +	if (!num_irq)
> +		return 0;
> +
> +	res += dev->num_resources - num_irq;

This is a little fragile. Instead of statically calculating the offset,
scan the table and make *absolutely* sure that there is a free block of
irq resources.

Also, if the table has already been populated successfully, then that
should be checked for. That would prevent the same table from being
parsed over and over in the case of a device that keeps getting
deferred.

Finally, if it fails, make sure any entries that have been filled in get
cleared out again before exiting.

Cheers,
g.

> +	ret = of_irq_to_resource_table(np, res, num_irq);
> +	if (ret != num_irq)
> +		return -EPROBE_DEFER;
> +
> +	return 0;
> +}
> +
> +/**
>   * of_device_alloc - Allocate and initialize an of_device
>   * @np: device node to assign to device
>   * @bus_id: Name to assign to the device.  May be null to use default name.
> @@ -152,7 +178,7 @@ struct platform_device *of_device_alloc(struct device_node *np,
>  	if (of_can_translate_address(np))
>  		while (of_address_to_resource(np, num_reg, &temp_res) == 0)
>  			num_reg++;
> -	num_irq = of_irq_valid_count(np);
> +	num_irq = of_irq_count(np);
>  
>  	/* Populate the resource table */
>  	if (num_irq || num_reg) {
> @@ -168,7 +194,6 @@ struct platform_device *of_device_alloc(struct device_node *np,
>  			rc = of_address_to_resource(np, i, res);
>  			WARN_ON(rc);
>  		}
> -		WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
>  	}
>  
>  	dev->dev.of_node = of_node_get(np);
> diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
> index 3863a4d..b5f9bb6 100644
> --- a/include/linux/of_platform.h
> +++ b/include/linux/of_platform.h
> @@ -79,6 +79,7 @@ extern const struct of_device_id of_default_bus_match_table[];
>  extern struct platform_device *of_device_alloc(struct device_node *np,
>  					 const char *bus_id,
>  					 struct device *parent);
> +extern int of_device_init_irq(struct platform_device *dev);
>  extern struct platform_device *of_find_device_by_node(struct device_node *np);
>  
>  #ifdef CONFIG_OF_ADDRESS /* device reg helpers depend on OF_ADDRESS */
> @@ -101,6 +102,7 @@ extern int of_platform_populate(struct device_node *root,
>  #if !defined(CONFIG_OF_ADDRESS)
>  struct of_dev_auxdata;
>  struct device;
> +struct platform_device;
>  static inline int of_platform_populate(struct device_node *root,
>  					const struct of_device_id *matches,
>  					const struct of_dev_auxdata *lookup,
> @@ -108,6 +110,11 @@ static inline int of_platform_populate(struct device_node *root,
>  {
>  	return -ENODEV;
>  }
> +
> +static inline int of_device_init_irq(struct platform_device *dev)
> +{
> +	return 0;
> +}
>  #endif /* !CONFIG_OF_ADDRESS */
>  
>  #endif	/* _LINUX_OF_PLATFORM_H */
> -- 
> 1.7.10.4
> 

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

Re: [RFC] [PATCH 3/3] IRQ: irq domain: defer of irq ressoure resolve at platform_drv_probe

[Arnd Bergmann]

On Thursday 30 May 2013, Grant Likely wrote:
> On Tue, 28 May 2013 17:08:49 +0200, Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org> wrote:
> >  
> >  /**
> > + * of_device_alloc_irq - initialize irq of an platfrom_device
> > + * @dev: plaform_device to work on
> > + */
> > +int of_device_init_irq(struct platform_device *dev)
> > +{
> > +	struct device_node *np = dev->dev.of_node;
> > +	int num_irq;
> > +	int ret;
> > +	struct resource *res = dev->resource;
> > +
> > +	if (!np)
> > +		return 0;
> > +
> > +	num_irq = of_irq_count(np);
> > +	if (!num_irq)
> > +		return 0;
> > +
> > +	res += dev->num_resources - num_irq;
> 
> This is a little fragile. Instead of statically calculating the offset,
> scan the table and make *absolutely* sure that there is a free block of
> irq resources.
> 
> Also, if the table has already been populated successfully, then that
> should be checked for. That would prevent the same table from being
> parsed over and over in the case of a device that keeps getting
> deferred.
> 
> Finally, if it fails, make sure any entries that have been filled in get
> cleared out again before exiting.

Can we actually ever get a case where mapping a subset of the interrupts
fail but works at a later point? If not, we could just return a fatal
error here and won't get called again.

Since a platform device only has one "interrupt-parent", as soon as
one of the interrupts can get mapped, I would expect that the controller
is present, and we don't need to defer the probing any more.

	Arnd

Re: [RFC] [PATCH 3/3] IRQ: irq domain: defer of irq ressoure resolve at platform_drv_probe

[Grant Likely]

On Thu, May 30, 2013 at 8:52 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Thursday 30 May 2013, Grant Likely wrote:
>> On Tue, 28 May 2013 17:08:49 +0200, Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org> wrote:
>> >
>> >  /**
>> > + * of_device_alloc_irq - initialize irq of an platfrom_device
>> > + * @dev: plaform_device to work on
>> > + */
>> > +int of_device_init_irq(struct platform_device *dev)
>> > +{
>> > +   struct device_node *np = dev->dev.of_node;
>> > +   int num_irq;
>> > +   int ret;
>> > +   struct resource *res = dev->resource;
>> > +
>> > +   if (!np)
>> > +           return 0;
>> > +
>> > +   num_irq = of_irq_count(np);
>> > +   if (!num_irq)
>> > +           return 0;
>> > +
>> > +   res += dev->num_resources - num_irq;
>>
>> This is a little fragile. Instead of statically calculating the offset,
>> scan the table and make *absolutely* sure that there is a free block of
>> irq resources.
>>
>> Also, if the table has already been populated successfully, then that
>> should be checked for. That would prevent the same table from being
>> parsed over and over in the case of a device that keeps getting
>> deferred.
>>
>> Finally, if it fails, make sure any entries that have been filled in get
>> cleared out again before exiting.
>
> Can we actually ever get a case where mapping a subset of the interrupts
> fail but works at a later point? If not, we could just return a fatal
> error here and won't get called again.
>
> Since a platform device only has one "interrupt-parent", as soon as
> one of the interrupts can get mapped, I would expect that the controller
> is present, and we don't need to defer the probing any more.

If it goes through an interrupt map node then it may only be able to
resolve a subset.

g.

Re: [RFC] [PATCH 3/3] IRQ: irq domain: defer of irq ressoure resolve at platform_drv_probe

[Arnd Bergmann]

On Thursday 30 May 2013, Grant Likely wrote:
> On Thu, May 30, 2013 at 8:52 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> >
> > Since a platform device only has one "interrupt-parent", as soon as
> > one of the interrupts can get mapped, I would expect that the controller
> > is present, and we don't need to defer the probing any more.
> 
> If it goes through an interrupt map node then it may only be able to
> resolve a subset.

Right, so we clearly need it. I also though of a second case, which
is that the xlate() function might itself return -EPROBE_DEFER in
the case that the irqchip driver is registered but e.g. not the
parent of a cascaded irqchip.

	Arnd

Re: [RFC] [PATCH 3/3] IRQ: irq domain: defer of irq ressoure resolve at platform_drv_probe

[Grant Likely]

On Thu, May 30, 2013 at 2:10 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Thursday 30 May 2013, Grant Likely wrote:
>> On Thu, May 30, 2013 at 8:52 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>> >
>> > Since a platform device only has one "interrupt-parent", as soon as
>> > one of the interrupts can get mapped, I would expect that the controller
>> > is present, and we don't need to defer the probing any more.
>>
>> If it goes through an interrupt map node then it may only be able to
>> resolve a subset.
>
> Right, so we clearly need it. I also though of a second case, which
> is that the xlate() function might itself return -EPROBE_DEFER in
> the case that the irqchip driver is registered but e.g. not the
> parent of a cascaded irqchip.

That shouldn't happen. If the parent isn't registered, then how can
the child set itself up?

g.

Re: [RFC] [PATCH 3/3] IRQ: irq domain: defer of irq ressoure resolve at platform_drv_probe

[Arnd Bergmann]

On Thursday 30 May 2013, Grant Likely wrote:
> On Thu, May 30, 2013 at 2:10 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> > On Thursday 30 May 2013, Grant Likely wrote:

> > Right, so we clearly need it. I also though of a second case, which
> > is that the xlate() function might itself return -EPROBE_DEFER in
> > the case that the irqchip driver is registered but e.g. not the
> > parent of a cascaded irqchip.
> 
> That shouldn't happen. If the parent isn't registered, then how can
> the child set itself up?

I guess it can just register the domain, but as long as no irqs
are needed, it doesn't have to set up a chained handler. I don't
think any irqchip driver today does it that way, but I think there
is nothing stopping us from doing a driver that does.

	Arnd

Re: [RFC] [PATCH 3/3] IRQ: irq domain: defer of irq ressoure resolve at platform_drv_probe

[Jean-Christophe PLAGNIOL-VILLARD]

On 09:52 Thu 30 May     , Arnd Bergmann wrote:
> On Thursday 30 May 2013, Grant Likely wrote:
> > On Tue, 28 May 2013 17:08:49 +0200, Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org> wrote:
> > >  
> > >  /**
> > > + * of_device_alloc_irq - initialize irq of an platfrom_device
> > > + * @dev: plaform_device to work on
> > > + */
> > > +int of_device_init_irq(struct platform_device *dev)
> > > +{
> > > +	struct device_node *np = dev->dev.of_node;
> > > +	int num_irq;
> > > +	int ret;
> > > +	struct resource *res = dev->resource;
> > > +
> > > +	if (!np)
> > > +		return 0;
> > > +
> > > +	num_irq = of_irq_count(np);
> > > +	if (!num_irq)
> > > +		return 0;
> > > +
> > > +	res += dev->num_resources - num_irq;
> > 
> > This is a little fragile. Instead of statically calculating the offset,
> > scan the table and make *absolutely* sure that there is a free block of
> > irq resources.
> > 
> > Also, if the table has already been populated successfully, then that
> > should be checked for. That would prevent the same table from being
> > parsed over and over in the case of a device that keeps getting
> > deferred.
> > 
> > Finally, if it fails, make sure any entries that have been filled in get
> > cleared out again before exiting.
> 
> Can we actually ever get a case where mapping a subset of the interrupts
> fail but works at a later point? If not, we could just return a fatal
> error here and won't get called again.
> 
> Since a platform device only has one "interrupt-parent", as soon as
> one of the interrupts can get mapped, I would expect that the controller
> is present, and we don't need to defer the probing any more.
this is where I disagre we can have 2 interrupt-parent, this is why I get
other issue on some devices as you get interrupts from the code controller and
then use an irq on a gpio controller so you end up with 2 :(

Assume we can only have one will end-up with issue to descript such hardware

as example I comment some TI patch where we need such configuration

Best Regards,
J.
> 
> 	Arnd

Re: [RFC] [PATCH 3/3] IRQ: irq domain: defer of irq ressoure resolve at platform_drv_probe

[Jean-Christophe PLAGNIOL-VILLARD]

On 23:48 Wed 29 May     , Grant Likely wrote:
> On Tue, 28 May 2013 17:08:49 +0200, Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org> wrote:
> > Today in the current of implementation we populate all the ressources
> > at of_platform_populate time. But this leed to a chicken-egg dilemat
> > some the irq present in DT are from platform_device too. And you can
> > not resolve them as of_platform_populate. So delay the populate of irq
> > at platform_drv_probe.
> > 
> > And if the irq_domain is not yet present just defer the probe (GPIO as example)
> 
> Hi Jean-Christophe,
> 
> Thanks for looking at this.
> 
> Hmmm, this is an interesting idea. I had been thinking about taping into
> the platform_get_irq() call to lookup irq numbers at probe time, but
> that assumes that all platform drivers use that call (which they clearly
> do not). This approach should work for all drivers as is.

Thanks, I try to do not introduce dt or c work around as done in other
drivers (mmc_spi & co) as I do not want to handle gpio as irq in ANY drivers
and just provide the irq number of the gpio via pdev resource or DT interrupts

while discussing with Arnd on IRC the idea come to do this and that we should
do the same for reset and clock by handling it at probe time
> 
> However care needs to be taken to not waste too much time processing
> irqs every time probe is called. Mapping irqs over and over will get
> expensive, especially with the current code. It would be better to
> rework the mapping function to iterate once over the interrupts property
> and map all the IRQs at once instead of calling of_irq_to_resource()
> over and over again. (I would however accept the argument that this is a
> bug fix and the refactoring should be a separate patch)

I'm very concern too by booting time I'll take a look on those issues

> 
> > 
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> > Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> > Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> > Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> > Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> > Cc: Ralf Baechle <ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
> > Cc: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> > ---
> >  drivers/base/platform.c     |    5 +++++
> >  drivers/of/platform.c       |   29 +++++++++++++++++++++++++++--
> >  include/linux/of_platform.h |    7 +++++++
> >  3 files changed, 39 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 9eda842..c0f7906 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/string.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/of_device.h>
> > +#include <linux/of_platform.h>
> >  #include <linux/module.h>
> >  #include <linux/init.h>
> >  #include <linux/dma-mapping.h>
> > @@ -484,6 +485,10 @@ static int platform_drv_probe(struct device *_dev)
> >  	struct platform_device *dev = to_platform_device(_dev);
> >  	int ret;
> >  
> > +	ret = of_device_init_irq(dev);
> > +	if (ret)
> > +		return ret;
> > +
> >  	if (ACPI_HANDLE(_dev))
> >  		acpi_dev_pm_attach(_dev, true);
> >  
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index 00a0971..c290943 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -131,6 +131,32 @@ void of_device_make_bus_id(struct device *dev)
> >  }
> >  
> >  /**
> > + * of_device_alloc_irq - initialize irq of an platfrom_device
> > + * @dev: plaform_device to work on
> > + */
> > +int of_device_init_irq(struct platform_device *dev)
> > +{
> > +	struct device_node *np = dev->dev.of_node;
> > +	int num_irq;
> > +	int ret;
> > +	struct resource *res = dev->resource;
> > +
> > +	if (!np)
> > +		return 0;
> > +
> > +	num_irq = of_irq_count(np);
> > +	if (!num_irq)
> > +		return 0;
> > +
> > +	res += dev->num_resources - num_irq;
> 
> This is a little fragile. Instead of statically calculating the offset,
> scan the table and make *absolutely* sure that there is a free block of
> irq resources.

yeah was thinking about it too, but did simple for RFC
> 
> Also, if the table has already been populated successfully, then that
> should be checked for. That would prevent the same table from being
> parsed over and over in the case of a device that keeps getting
> deferred.

can I add a new boolean for this in pdev of device_node?
> 
> Finally, if it fails, make sure any entries that have been filled in get
> cleared out again before exiting.
ok

Best Regards,
J.

Re: [RFC] [PATCH 3/3] IRQ: irq domain: defer of irq ressoure resolve at platform_drv_probe

[Grant Likely]

On Thu, May 30, 2013 at 11:36 AM, Jean-Christophe PLAGNIOL-VILLARD
<plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org> wrote:
> On 23:48 Wed 29 May     , Grant Likely wrote:
>> Also, if the table has already been populated successfully, then that
>> should be checked for. That would prevent the same table from being
>> parsed over and over in the case of a device that keeps getting
>> deferred.
>
> can I add a new boolean for this in pdev of device_node?

Try it out and see what it looks like. It would probably be better to
have it as a flag in the platform device itself. Regardless, do it as
a follow-on patch so that the functionality can be split from the
optimization change. I think the optimization could still require a
few iterations.

g.

Re: [RFC] [PATCH 3/3] IRQ: irq domain: defer of irq ressoure resolve at platform_drv_probe

[Alexander Sverdlin]

Hi!

On 05/28/2013 05:08 PM, ext Jean-Christophe PLAGNIOL-VILLARD wrote:
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -131,6 +131,32 @@ void of_device_make_bus_id(struct device *dev)
>  }
>  
>  /**
> + * of_device_alloc_irq - initialize irq of an platfrom_device
      ^^^^^^^^^^^^^^^^^^^
> + * @dev: plaform_device to work on
> + */
> +int of_device_init_irq(struct platform_device *dev)
       ^^^^^^^^^^^^^^^^^^

I think, mismatch in function names in the comment and function itself wasn't yet reported...

> +{
> +	struct device_node *np = dev->dev.of_node;
> +	int num_irq;
> +	int ret;
> +	struct resource *res = dev->resource;
> +
> +	if (!np)
> +		return 0;
> +
> +	num_irq = of_irq_count(np);
> +	if (!num_irq)
> +		return 0;
> +
> +	res += dev->num_resources - num_irq;
> +	ret = of_irq_to_resource_table(np, res, num_irq);
> +	if (ret != num_irq)
> +		return -EPROBE_DEFER;
> +
> +	return 0;
> +}
> +
> +/**
>   * of_device_alloc - Allocate and initialize an of_device
>   * @np: device node to assign to device
>   * @bus_id: Name to assign to the device.  May be null to use default name.

I was trying to get attention to this problem already on the list in January-February time frame, but
without success... I will give your code a try... We have exactly these chained interrupt controllers dependencies
and irq_create_of_mapping() was failing... I'll inform you on the test results...

-- 
Best regards,
Alexander Sverdlin.

Re: [RFC] [PATCH 3/3] IRQ: irq domain: defer of irq ressoure resolve at platform_drv_probe

[Alexander Sverdlin]

Hi!

On 05/28/2013 05:08 PM, ext Jean-Christophe PLAGNIOL-VILLARD wrote:
> Today in the current of implementation we populate all the ressources
> at of_platform_populate time. But this leed to a chicken-egg dilemat
> some the irq present in DT are from platform_device too. And you can
> not resolve them as of_platform_populate. So delay the populate of irq
> at platform_drv_probe.
> 
> And if the irq_domain is not yet present just defer the probe (GPIO as example)
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>

Tested-by: Alexander Sverdlin <alexander.sverdlin-OYasijW0DpE@public.gmane.org>

Tested the whole series. This is definitely a necessary feature.
I've tested it on MIPS64 system with merge-able device-subtrees.
If you would come with the new patch series, please put me on CC, I'll test it again.

> Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> Cc: Ralf Baechle <ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>
> Cc: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/base/platform.c     |    5 +++++
>  drivers/of/platform.c       |   29 +++++++++++++++++++++++++++--
>  include/linux/of_platform.h |    7 +++++++
>  3 files changed, 39 insertions(+), 2 deletions(-)

...

-- 
Best regards,
Alexander Sverdlin.

Re: [RFC] [PATCH 1/3] of: irq: rename of_irq_count to of_irq_valid_count

[Grant Likely]

On Tue, 28 May 2013 17:08:47 +0200, Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org> wrote:
> as we count valid mapped irq not just present irq
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>

Hmmm, even with it being in-accurately named, I think it is dangerous to
rename the existing function and re-use the old name. Anyone with
out-of-tree code will get the behaviour changed under their feet. I
think it would be better to drop this patch from the series.

g.

Re: [RFC] [PATCH 1/3] of: irq: rename of_irq_count to of_irq_valid_count

[Jean-Christophe PLAGNIOL-VILLARD]

On 23:25 Wed 29 May     , Grant Likely wrote:
> On Tue, 28 May 2013 17:08:47 +0200, Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org> wrote:
> > as we count valid mapped irq not just present irq
> > 
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> 
> Hmmm, even with it being in-accurately named, I think it is dangerous to
> rename the existing function and re-use the old name. Anyone with
> out-of-tree code will get the behaviour changed under their feet. I
> think it would be better to drop this patch from the series.

after think I known this can be consider as noisy change but I think we could
rename it anyway to make it more clear and use a different name as you propose for the counting irq
in PATCH 2/3

Best Regards,
J.

Re: [RFC] [PATCH 0/3] of: allow to support irq domain register as platform_device for platform_device

[Arnd Bergmann]

On Tuesday 28 May 2013, Jean-Christophe PLAGNIOL-VILLARD wrote:
>         Today in the current of implementation we populate all the ressources
>         at of_platform_populate time. But this leed to a chicken-egg dilemat
>         some the irq present in DT are from platform_device too. And you can
>         not resolve them as of_platform_populate. So delay the populate of irq
>         at platform_drv_probe.

I like it a lot, thanks for doing this!

Obviously this needs to be tested well on all architectures, but I think
it's the right solution for the problem. I was originally thinking we'd
just allocated the resources dynamically using devm_kmalloc in
platform_drv_probe, but your solution to preallocate them at boot
time seems nicer.

I believe we still need the same patch for the amba bus, and possibly
a couple more, but platform_bus covers most of the interesting cases.

	Arnd

Re: [RFC] [PATCH 0/3] of: allow to support irq domain register as platform_device for platform_device

[Jean-Christophe PLAGNIOL-VILLARD]

On 17:41 Tue 28 May     , Arnd Bergmann wrote:
> On Tuesday 28 May 2013, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >         Today in the current of implementation we populate all the ressources
> >         at of_platform_populate time. But this leed to a chicken-egg dilemat
> >         some the irq present in DT are from platform_device too. And you can
> >         not resolve them as of_platform_populate. So delay the populate of irq
> >         at platform_drv_probe.
> 
> I like it a lot, thanks for doing this!
> 
> Obviously this needs to be tested well on all architectures, but I think
> it's the right solution for the problem.
I've tested it today on a ARM at91sam9n12ek using a gpio as irq connected to a
ks8851-ml
> I was originally thinking we'd
> just allocated the resources dynamically using devm_kmalloc in
> platform_drv_probe, but your solution to preallocate them at boot
> time seems nicer.

yes I should have put a comment in the patch 3/3
as the device tree is a fixed data, preallocate them at boot t time save the
kalloc kree everytime you probe the device and the irq_domain is not yet
present
> 
> I believe we still need the same patch for the amba bus, and possibly
> a couple more, but platform_bus covers most of the interesting cases.
yes exactly

Best Regards,
J.
> 
> 	Arnd