[RFC PATCH 0/4] DT support for ARM GIC and TWD

[RFC PATCH 0/4] DT support for ARM GIC and TWD

[Marc Zyngier]

This patch series expands my previous SMP timer effort, and adds
device tree support to two very common elements: the GIC and the TWD
timer.

As this is my first trip into DT land, I'm pretty sure there's a lot
to say about the bindings, and I'm specially looking for comments
around the following topics:

- The device_type attribute: when is it valid to use it?

- I used the compatible attribute as a generic way to probe for a
  whole class of device, and I feel this is not the indended usage.

- The TWD binding describes its interrupts inside sub nodes, which
  means the platform device doesn't contain the IRQ resources, and the
  driver has to parse the tree itself. As I'm under the impression
  that a platform driver should only parse the tree to get platform
  data, I wonder if there is some DT magic to solve this particular
  problem in an elegant way?

- early platform devices and DT: I've taken the brute-force approach
  here, and would be very glad if someone could come up with a better
  alternative.

This patch series also depends on a few patches by Grant Likely:
 dt/platform: allow device name to be overridden
 drivers/amba: create devices from device tree
 dt: add of_platform_populate() for creating device from the device tree
 dt: Add default match table for bus ids
 irq: add irq_domain translation infrastructure

The whole branch including the PPI infrastructure, the SMP timer
rework, Grant's patches, this patch series and a few others are (or
will soon be) in the following repository:

git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git local_timer_devices-v3.0-rc4

Marc Zyngier (4):
  dt: early platform devices support
  ARM: dt: register local timers as early platform devices
  ARM: dt: add GIC bindings and driver support
  ARM: dt: Add TWD bindings and driver support

 Documentation/devicetree/bindings/arm/gic.txt |   92 ++++++++
 Documentation/devicetree/bindings/arm/twd.txt |   54 +++++
 arch/arm/common/gic.c                         |  312 ++++++++++++++++++++-----
 arch/arm/include/asm/hardware/gic.h           |    4 +
 arch/arm/kernel/time.c                        |    4 +
 drivers/clocksource/arm_smp_twd.c             |   92 +++++++-
 drivers/of/platform.c                         |   26 ++
 include/linux/of_platform.h                   |    1 +
 8 files changed, 517 insertions(+), 68 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/gic.txt
 create mode 100644 Documentation/devicetree/bindings/arm/twd.txt

[RFC PATCH 0/4] DT support for ARM GIC and TWD

[Marc Zyngier]

This patch series expands my previous SMP timer effort, and adds
device tree support to two very common elements: the GIC and the TWD
timer.

As this is my first trip into DT land, I'm pretty sure there's a lot
to say about the bindings, and I'm specially looking for comments
around the following topics:

- The device_type attribute: when is it valid to use it?

- I used the compatible attribute as a generic way to probe for a
  whole class of device, and I feel this is not the indended usage.

- The TWD binding describes its interrupts inside sub nodes, which
  means the platform device doesn't contain the IRQ resources, and the
  driver has to parse the tree itself. As I'm under the impression
  that a platform driver should only parse the tree to get platform
  data, I wonder if there is some DT magic to solve this particular
  problem in an elegant way?

- early platform devices and DT: I've taken the brute-force approach
  here, and would be very glad if someone could come up with a better
  alternative.

This patch series also depends on a few patches by Grant Likely:
 dt/platform: allow device name to be overridden
 drivers/amba: create devices from device tree
 dt: add of_platform_populate() for creating device from the device tree
 dt: Add default match table for bus ids
 irq: add irq_domain translation infrastructure

The whole branch including the PPI infrastructure, the SMP timer
rework, Grant's patches, this patch series and a few others are (or
will soon be) in the following repository:

git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git local_timer_devices-v3.0-rc4

Marc Zyngier (4):
  dt: early platform devices support
  ARM: dt: register local timers as early platform devices
  ARM: dt: add GIC bindings and driver support
  ARM: dt: Add TWD bindings and driver support

 Documentation/devicetree/bindings/arm/gic.txt |   92 ++++++++
 Documentation/devicetree/bindings/arm/twd.txt |   54 +++++
 arch/arm/common/gic.c                         |  312 ++++++++++++++++++++-----
 arch/arm/include/asm/hardware/gic.h           |    4 +
 arch/arm/kernel/time.c                        |    4 +
 drivers/clocksource/arm_smp_twd.c             |   92 +++++++-
 drivers/of/platform.c                         |   26 ++
 include/linux/of_platform.h                   |    1 +
 8 files changed, 517 insertions(+), 68 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/gic.txt
 create mode 100644 Documentation/devicetree/bindings/arm/twd.txt

[RFC PATCH 1/4] dt: early platform devices support

[Marc Zyngier]

Add support for populating early platform devices from the device
tree, by walking the tree and adding nodes whose 'compatible'
property matches the 'class' string passed as a parameter.

This allows devices to be probed long before the whole device
infrastructure is available.

Signed-off-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
---
 drivers/of/platform.c       |   26 ++++++++++++++++++++++++++
 include/linux/of_platform.h |    1 +
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index e75af39..2a323ee 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -458,4 +458,30 @@ int of_platform_populate(struct device_node *root,
 	of_node_put(root);
 	return rc;
 }
+
+/**
+ * of_early_platform_populate() - Populate early platform devices from DT
+ * @class: string to compare to the 'compatible' attributes
+ *
+ * This function walks the device tree and register devices whose
+ * 'compatible' property matches the 'class' parameter.
+ *
+ * Returns 0 on success, < 0 on failure.
+ */
+int of_early_platform_populate(const char *class)
+{
+	struct platform_device *pdev;
+	struct device_node *np = NULL;
+
+	while ((np = of_find_compatible_node(np, NULL, class))) {
+		pdev = of_device_alloc(np, NULL, NULL);
+		if (!pdev)
+			return -ENOMEM;
+		list_del(&pdev->dev.devres_head);
+		memset(&pdev->dev.devres_head, 0, sizeof(pdev->dev.devres_head));
+		early_platform_add_devices(&pdev, 1);
+	}
+
+	return 0;
+}
 #endif /* !CONFIG_SPARC */
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
index 5a6f458..dd1dc90 100644
--- a/include/linux/of_platform.h
+++ b/include/linux/of_platform.h
@@ -95,6 +95,7 @@ extern int of_platform_populate(struct device_node *root,
 				const struct of_device_id *matches,
 				const struct of_dev_auxdata *lookup,
 				struct device *parent);
+extern int of_early_platform_populate(const char *class);
 #endif /* !CONFIG_SPARC */
 
 #endif /* CONFIG_OF_DEVICE */
-- 
1.7.0.4

[RFC PATCH 1/4] dt: early platform devices support

[Marc Zyngier]

Add support for populating early platform devices from the device
tree, by walking the tree and adding nodes whose 'compatible'
property matches the 'class' string passed as a parameter.

This allows devices to be probed long before the whole device
infrastructure is available.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/of/platform.c       |   26 ++++++++++++++++++++++++++
 include/linux/of_platform.h |    1 +
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index e75af39..2a323ee 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -458,4 +458,30 @@ int of_platform_populate(struct device_node *root,
 	of_node_put(root);
 	return rc;
 }
+
+/**
+ * of_early_platform_populate() - Populate early platform devices from DT
+ * @class: string to compare to the 'compatible' attributes
+ *
+ * This function walks the device tree and register devices whose
+ * 'compatible' property matches the 'class' parameter.
+ *
+ * Returns 0 on success, < 0 on failure.
+ */
+int of_early_platform_populate(const char *class)
+{
+	struct platform_device *pdev;
+	struct device_node *np = NULL;
+
+	while ((np = of_find_compatible_node(np, NULL, class))) {
+		pdev = of_device_alloc(np, NULL, NULL);
+		if (!pdev)
+			return -ENOMEM;
+		list_del(&pdev->dev.devres_head);
+		memset(&pdev->dev.devres_head, 0, sizeof(pdev->dev.devres_head));
+		early_platform_add_devices(&pdev, 1);
+	}
+
+	return 0;
+}
 #endif /* !CONFIG_SPARC */
diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h
index 5a6f458..dd1dc90 100644
--- a/include/linux/of_platform.h
+++ b/include/linux/of_platform.h
@@ -95,6 +95,7 @@ extern int of_platform_populate(struct device_node *root,
 				const struct of_device_id *matches,
 				const struct of_dev_auxdata *lookup,
 				struct device *parent);
+extern int of_early_platform_populate(const char *class);
 #endif /* !CONFIG_SPARC */
 
 #endif /* CONFIG_OF_DEVICE */
-- 
1.7.0.4

Re: [RFC PATCH 1/4] dt: early platform devices support

[Grant Likely]

[cc'ing linux kernel since I discuss driver core issues]
[cc'ing greg and kay, 'cause I've included a patch I'd like you to
look at (see end of email)]

On Fri, Jun 24, 2011 at 8:10 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Add support for populating early platform devices from the device
> tree, by walking the tree and adding nodes whose 'compatible'
> property matches the 'class' string passed as a parameter.
>
> This allows devices to be probed long before the whole device
> infrastructure is available.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  drivers/of/platform.c       |   26 ++++++++++++++++++++++++++
>  include/linux/of_platform.h |    1 +
>  2 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index e75af39..2a323ee 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -458,4 +458,30 @@ int of_platform_populate(struct device_node *root,
>        of_node_put(root);
>        return rc;
>  }
> +
> +/**
> + * of_early_platform_populate() - Populate early platform devices from DT
> + * @class: string to compare to the 'compatible' attributes
> + *
> + * This function walks the device tree and register devices whose
> + * 'compatible' property matches the 'class' parameter.
> + *
> + * Returns 0 on success, < 0 on failure.
> + */
> +int of_early_platform_populate(const char *class)
> +{
> +       struct platform_device *pdev;
> +       struct device_node *np = NULL;
> +
> +       while ((np = of_find_compatible_node(np, NULL, class))) {

for_each_compatible_node()

> +               pdev = of_device_alloc(np, NULL, NULL);
> +               if (!pdev)
> +                       return -ENOMEM;
> +               list_del(&pdev->dev.devres_head);
> +               memset(&pdev->dev.devres_head, 0, sizeof(pdev->dev.devres_head));
> +               early_platform_add_devices(&pdev, 1);

I'm not sure this will work reliably.  The of_platform semantics are
still slightly different from 'regular' platform devices (which I do
intend to fix though), so it may cause problems with how the device
name gets assigned.  I'd need to dig into it though.

> +       }
> +
> +       return 0;
> +}

Hmmm, I'd really rather not go down the path of having different
'classes' of devices that need to be registered at different times.
There just isn't a really good generic way to know which devices
should be the 'early' ones, it gets hairy with regard to making sure
devices don't get registered twice, and it doesn't actually solve the
problem that there is no way to handle dependencies between devices in
the Linux device model.

What I /want/ to do is allow drivers to defer initialization at .probe
time if some of the resources it needs aren't yet available.
Unfortunately, that doesn't work so well for interrupt controllers
since irq numbers are expected to be correctly populated in the
platform device resource table.  What complicates things further is
that most gpio controllers are doubling as irq controllers nowdays,
and those are typically modelled with platform devices themselves.

One possible approach is to manipulate the order that devices get
registered in by looking explicitly at each device's interrupt-parent
and *-gpio properties, and making sure that interrupt-controllers and
gpio-controllers get registered before any dependent modules, but that
could get complex in a hurry and it doesn't actually solve anything if
one of the irq controller drivers is loaded as a module.  To make it
work, of_platform_populate() would have to actually defer registration
dependent devices until after the irq controller is bound to a driver.
 Blech!  It would be doable by keeping a list of nodes that a device
is dependant on, and snooping bus registrations to look for drivers
bound to the devices referencing that node, but it would be ugly and
would just gloss over the lack of handling dependencies in the driver
core.  Right now we fudge it by messing about with initcall ordering
on the device drivers so that the device driver registration order
doesn't matter, but that doesn't solve the problem caused by drivers
loaded by modules.

More and more, I'm think the solution is to do dependency checking at
.probe time.  If some asynchronous resources (irqs, gpios, phys, etc)
aren't available yet, then it need to be possible to defer
initialization, either by not binding the driver and returning
something like -EAGAIN to say "try me again later", or by letting the
driver explicitly call a deferred init api that with a callback for
when the resources do become available.  I'm really not sure which is
best.  I worry that a deferred init api will end up being complex and
unwieldy, so I'm kind of leaning towards the -EAGAIN solution since it
doesn't require drivers to create a new initialization path.  It would
mean that the driver core would have to keep a list of devices that
had a driver return -EAGAIN, and arrange to reattempt binding at some
point in the future.  Probably after a 'significant' event, such as
another device getting bound to a driver.

...

I should probably actually propose some kind of solution rather than
babbling on about the problem.  I took some time tonight to hack
together what I think the solution should look like.  Take a look at
the following and let me know what you think.  Greg/Kay, I could
certainly use your feedback on this (apologies for any work wrapping,
I'm pasting this into gmail).

---

commit efc3b3168879d0bd9bf9fa3ebfbb1a348394c76f
Author: Grant Likely <grant.likely@secretlab.ca>
Date:   Fri Jun 24 22:33:46 2011 -0600

    drivercore: Add driver probe deferral mechanism

    Allow drivers to report at probe time that they cannot get all the resources
    required by the device, and should be retried at a later time.

    This should completely solve the problem of getting devices
    initialized in the right order.  Right now this is mostly handled by
    mucking about with initcall ordering which is a complete hack, and
    doesn't even remotely handle the case where device drivers are in
    modules.  This approach completely sidesteps the issues by allowing
    driver registration to occur in any order, and any driver can request
    to be retried after a few more other drivers get probed.

    This patch is *completely untested* and probably utterly broken.
    There is absolutely no locking implemented, and there needs to be
    something around the accesses to the deferred_probe_list (most likely
    a spin lock that gets dropped before each call to bus_probe_device()).
    I'd like to get feedback on if this is the correct approach before I
    do the work of actually making it correct.  :-)

    Signed-off-by: Grant Likely <grant.likely@secretlab.ca>

diff --git a/drivers/base/core.c b/drivers/base/core.c
index bc8729d..83aafff 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -588,6 +588,7 @@ void device_initialize(struct device *dev)
 {
 	dev->kobj.kset = devices_kset;
 	kobject_init(&dev->kobj, &device_ktype);
+	INIT_LIST_HEAD(&dev->deferred_probe);
 	INIT_LIST_HEAD(&dev->dma_pools);
 	mutex_init(&dev->mutex);
 	lockdep_set_novalidate_class(&dev->mutex);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 6658da7..3694661 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -28,6 +28,18 @@
 #include "base.h"
 #include "power/power.h"

+/**
+ * deferred_probe_work_func() - Retry probing devices in the deferred list.
+ */
+static LIST_HEAD(deferred_probe_list);
+static void deferred_probe_work_func(struct work_struct *work)
+{
+	struct device *dev;
+	list_for_each_entry(dev, &deferred_probe_list, deferred_probe) {
+		bus_probe_device(dev);
+	}
+}
+static DECLARE_WORK(deferred_probe_work, deferred_probe_work_func);

 static void driver_bound(struct device *dev)
 {
@@ -41,6 +53,9 @@ static void driver_bound(struct device *dev)
 		 __func__, dev->driver->name);

 	klist_add_tail(&dev->p->knode_driver, &dev->driver->p->klist_devices);
+	if (!list_empty(&dev->deferred_probe))
+		list_del_init(&dev->deferred_probe);
+	schedule_work(&deferred_probe_work);

 	if (dev->bus)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
@@ -142,7 +157,17 @@ probe_failed:
 	driver_sysfs_remove(dev);
 	dev->driver = NULL;

-	if (ret != -ENODEV && ret != -ENXIO) {
+	if (ret == -EAGAIN) {
+		/*
+		 * Probe could not obtain all resources for device and requested
+		 * to be reprobed with the availble drivers at some future point
+		 * in time.  Log the deferral request and add the device to the
+		 * deferred probe list.
+		 */
+		dev_info(dev, "Driver %s requests probe deferral\n", drv->name);
+		if (list_empty(&dev->deferred_probe))
+			list_add(&dev->deferred_probe, &deferred_probe_list);
+	} else if (ret != -ENODEV && ret != -ENXIO) {
 		/* driver matched but the probe failed */
 		printk(KERN_WARNING
 		       "%s: probe of %s failed with error %d\n",
diff --git a/include/linux/device.h b/include/linux/device.h
index c66111a..50cc399 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -506,6 +506,10 @@ struct device_dma_parameters {
  * @mutex:	Mutex to synchronize calls to its driver.
  * @bus:	Type of bus device is on.
  * @driver:	Which driver has allocated this
+ * @deferred_probe: entry in deferred_probe_list which is used to retry the
+ * 		binding of drivers which were unable to get all the resources
+ * 		needed by the device; typically because it depends on another
+ * 		driver getting probed first.
  * @platform_data: Platform data specific to the device.
  * 		Example: For devices on custom boards, as typical of embedded
  * 		and SOC based hardware, Linux often uses platform_data to point
@@ -565,6 +569,7 @@ struct device {
 	struct bus_type	*bus;		/* type of bus device is on */
 	struct device_driver *driver;	/* which driver has allocated this
 					   device */
+	struct list_head	deferred_probe;
 	void		*platform_data;	/* Platform specific data, device
 					   core doesn't touch it */
 	struct dev_pm_info	power;

[RFC PATCH 1/4] dt: early platform devices support

[Grant Likely]

[cc'ing linux kernel since I discuss driver core issues]
[cc'ing greg and kay, 'cause I've included a patch I'd like you to
look at (see end of email)]

On Fri, Jun 24, 2011 at 8:10 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Add support for populating early platform devices from the device
> tree, by walking the tree and adding nodes whose 'compatible'
> property matches the 'class' string passed as a parameter.
>
> This allows devices to be probed long before the whole device
> infrastructure is available.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> ?drivers/of/platform.c ? ? ? | ? 26 ++++++++++++++++++++++++++
> ?include/linux/of_platform.h | ? ?1 +
> ?2 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index e75af39..2a323ee 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -458,4 +458,30 @@ int of_platform_populate(struct device_node *root,
> ? ? ? ?of_node_put(root);
> ? ? ? ?return rc;
> ?}
> +
> +/**
> + * of_early_platform_populate() - Populate early platform devices from DT
> + * @class: string to compare to the 'compatible' attributes
> + *
> + * This function walks the device tree and register devices whose
> + * 'compatible' property matches the 'class' parameter.
> + *
> + * Returns 0 on success, < 0 on failure.
> + */
> +int of_early_platform_populate(const char *class)
> +{
> + ? ? ? struct platform_device *pdev;
> + ? ? ? struct device_node *np = NULL;
> +
> + ? ? ? while ((np = of_find_compatible_node(np, NULL, class))) {

for_each_compatible_node()

> + ? ? ? ? ? ? ? pdev = of_device_alloc(np, NULL, NULL);
> + ? ? ? ? ? ? ? if (!pdev)
> + ? ? ? ? ? ? ? ? ? ? ? return -ENOMEM;
> + ? ? ? ? ? ? ? list_del(&pdev->dev.devres_head);
> + ? ? ? ? ? ? ? memset(&pdev->dev.devres_head, 0, sizeof(pdev->dev.devres_head));
> + ? ? ? ? ? ? ? early_platform_add_devices(&pdev, 1);

I'm not sure this will work reliably.  The of_platform semantics are
still slightly different from 'regular' platform devices (which I do
intend to fix though), so it may cause problems with how the device
name gets assigned.  I'd need to dig into it though.

> + ? ? ? }
> +
> + ? ? ? return 0;
> +}

Hmmm, I'd really rather not go down the path of having different
'classes' of devices that need to be registered at different times.
There just isn't a really good generic way to know which devices
should be the 'early' ones, it gets hairy with regard to making sure
devices don't get registered twice, and it doesn't actually solve the
problem that there is no way to handle dependencies between devices in
the Linux device model.

What I /want/ to do is allow drivers to defer initialization at .probe
time if some of the resources it needs aren't yet available.
Unfortunately, that doesn't work so well for interrupt controllers
since irq numbers are expected to be correctly populated in the
platform device resource table.  What complicates things further is
that most gpio controllers are doubling as irq controllers nowdays,
and those are typically modelled with platform devices themselves.

One possible approach is to manipulate the order that devices get
registered in by looking explicitly at each device's interrupt-parent
and *-gpio properties, and making sure that interrupt-controllers and
gpio-controllers get registered before any dependent modules, but that
could get complex in a hurry and it doesn't actually solve anything if
one of the irq controller drivers is loaded as a module.  To make it
work, of_platform_populate() would have to actually defer registration
dependent devices until after the irq controller is bound to a driver.
 Blech!  It would be doable by keeping a list of nodes that a device
is dependant on, and snooping bus registrations to look for drivers
bound to the devices referencing that node, but it would be ugly and
would just gloss over the lack of handling dependencies in the driver
core.  Right now we fudge it by messing about with initcall ordering
on the device drivers so that the device driver registration order
doesn't matter, but that doesn't solve the problem caused by drivers
loaded by modules.

More and more, I'm think the solution is to do dependency checking at
.probe time.  If some asynchronous resources (irqs, gpios, phys, etc)
aren't available yet, then it need to be possible to defer
initialization, either by not binding the driver and returning
something like -EAGAIN to say "try me again later", or by letting the
driver explicitly call a deferred init api that with a callback for
when the resources do become available.  I'm really not sure which is
best.  I worry that a deferred init api will end up being complex and
unwieldy, so I'm kind of leaning towards the -EAGAIN solution since it
doesn't require drivers to create a new initialization path.  It would
mean that the driver core would have to keep a list of devices that
had a driver return -EAGAIN, and arrange to reattempt binding at some
point in the future.  Probably after a 'significant' event, such as
another device getting bound to a driver.

...

I should probably actually propose some kind of solution rather than
babbling on about the problem.  I took some time tonight to hack
together what I think the solution should look like.  Take a look at
the following and let me know what you think.  Greg/Kay, I could
certainly use your feedback on this (apologies for any work wrapping,
I'm pasting this into gmail).

---

commit efc3b3168879d0bd9bf9fa3ebfbb1a348394c76f
Author: Grant Likely <grant.likely@secretlab.ca>
Date:   Fri Jun 24 22:33:46 2011 -0600

    drivercore: Add driver probe deferral mechanism

    Allow drivers to report at probe time that they cannot get all the resources
    required by the device, and should be retried at a later time.

    This should completely solve the problem of getting devices
    initialized in the right order.  Right now this is mostly handled by
    mucking about with initcall ordering which is a complete hack, and
    doesn't even remotely handle the case where device drivers are in
    modules.  This approach completely sidesteps the issues by allowing
    driver registration to occur in any order, and any driver can request
    to be retried after a few more other drivers get probed.

    This patch is *completely untested* and probably utterly broken.
    There is absolutely no locking implemented, and there needs to be
    something around the accesses to the deferred_probe_list (most likely
    a spin lock that gets dropped before each call to bus_probe_device()).
    I'd like to get feedback on if this is the correct approach before I
    do the work of actually making it correct.  :-)

    Signed-off-by: Grant Likely <grant.likely@secretlab.ca>

diff --git a/drivers/base/core.c b/drivers/base/core.c
index bc8729d..83aafff 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -588,6 +588,7 @@ void device_initialize(struct device *dev)
 {
 	dev->kobj.kset = devices_kset;
 	kobject_init(&dev->kobj, &device_ktype);
+	INIT_LIST_HEAD(&dev->deferred_probe);
 	INIT_LIST_HEAD(&dev->dma_pools);
 	mutex_init(&dev->mutex);
 	lockdep_set_novalidate_class(&dev->mutex);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 6658da7..3694661 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -28,6 +28,18 @@
 #include "base.h"
 #include "power/power.h"

+/**
+ * deferred_probe_work_func() - Retry probing devices in the deferred list.
+ */
+static LIST_HEAD(deferred_probe_list);
+static void deferred_probe_work_func(struct work_struct *work)
+{
+	struct device *dev;
+	list_for_each_entry(dev, &deferred_probe_list, deferred_probe) {
+		bus_probe_device(dev);
+	}
+}
+static DECLARE_WORK(deferred_probe_work, deferred_probe_work_func);

 static void driver_bound(struct device *dev)
 {
@@ -41,6 +53,9 @@ static void driver_bound(struct device *dev)
 		 __func__, dev->driver->name);

 	klist_add_tail(&dev->p->knode_driver, &dev->driver->p->klist_devices);
+	if (!list_empty(&dev->deferred_probe))
+		list_del_init(&dev->deferred_probe);
+	schedule_work(&deferred_probe_work);

 	if (dev->bus)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
@@ -142,7 +157,17 @@ probe_failed:
 	driver_sysfs_remove(dev);
 	dev->driver = NULL;

-	if (ret != -ENODEV && ret != -ENXIO) {
+	if (ret == -EAGAIN) {
+		/*
+		 * Probe could not obtain all resources for device and requested
+		 * to be reprobed with the availble drivers at some future point
+		 * in time.  Log the deferral request and add the device to the
+		 * deferred probe list.
+		 */
+		dev_info(dev, "Driver %s requests probe deferral\n", drv->name);
+		if (list_empty(&dev->deferred_probe))
+			list_add(&dev->deferred_probe, &deferred_probe_list);
+	} else if (ret != -ENODEV && ret != -ENXIO) {
 		/* driver matched but the probe failed */
 		printk(KERN_WARNING
 		       "%s: probe of %s failed with error %d\n",
diff --git a/include/linux/device.h b/include/linux/device.h
index c66111a..50cc399 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -506,6 +506,10 @@ struct device_dma_parameters {
  * @mutex:	Mutex to synchronize calls to its driver.
  * @bus:	Type of bus device is on.
  * @driver:	Which driver has allocated this
+ * @deferred_probe: entry in deferred_probe_list which is used to retry the
+ * 		binding of drivers which were unable to get all the resources
+ * 		needed by the device; typically because it depends on another
+ * 		driver getting probed first.
  * @platform_data: Platform data specific to the device.
  * 		Example: For devices on custom boards, as typical of embedded
  * 		and SOC based hardware, Linux often uses platform_data to point
@@ -565,6 +569,7 @@ struct device {
 	struct bus_type	*bus;		/* type of bus device is on */
 	struct device_driver *driver;	/* which driver has allocated this
 					   device */
+	struct list_head	deferred_probe;
 	void		*platform_data;	/* Platform specific data, device
 					   core doesn't touch it */
 	struct dev_pm_info	power;

Re: [RFC PATCH 1/4] dt: early platform devices support

[Grant Likely]

[cc'ing linux kernel since I discuss driver core issues]
[cc'ing greg and kay, 'cause I've included a patch I'd like you to
look at (see end of email)]

On Fri, Jun 24, 2011 at 8:10 AM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> Add support for populating early platform devices from the device
> tree, by walking the tree and adding nodes whose 'compatible'
> property matches the 'class' string passed as a parameter.
>
> This allows devices to be probed long before the whole device
> infrastructure is available.
>
> Signed-off-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/of/platform.c       |   26 ++++++++++++++++++++++++++
>  include/linux/of_platform.h |    1 +
>  2 files changed, 27 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index e75af39..2a323ee 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -458,4 +458,30 @@ int of_platform_populate(struct device_node *root,
>        of_node_put(root);
>        return rc;
>  }
> +
> +/**
> + * of_early_platform_populate() - Populate early platform devices from DT
> + * @class: string to compare to the 'compatible' attributes
> + *
> + * This function walks the device tree and register devices whose
> + * 'compatible' property matches the 'class' parameter.
> + *
> + * Returns 0 on success, < 0 on failure.
> + */
> +int of_early_platform_populate(const char *class)
> +{
> +       struct platform_device *pdev;
> +       struct device_node *np = NULL;
> +
> +       while ((np = of_find_compatible_node(np, NULL, class))) {

for_each_compatible_node()

> +               pdev = of_device_alloc(np, NULL, NULL);
> +               if (!pdev)
> +                       return -ENOMEM;
> +               list_del(&pdev->dev.devres_head);
> +               memset(&pdev->dev.devres_head, 0, sizeof(pdev->dev.devres_head));
> +               early_platform_add_devices(&pdev, 1);

I'm not sure this will work reliably.  The of_platform semantics are
still slightly different from 'regular' platform devices (which I do
intend to fix though), so it may cause problems with how the device
name gets assigned.  I'd need to dig into it though.

> +       }
> +
> +       return 0;
> +}

Hmmm, I'd really rather not go down the path of having different
'classes' of devices that need to be registered at different times.
There just isn't a really good generic way to know which devices
should be the 'early' ones, it gets hairy with regard to making sure
devices don't get registered twice, and it doesn't actually solve the
problem that there is no way to handle dependencies between devices in
the Linux device model.

What I /want/ to do is allow drivers to defer initialization at .probe
time if some of the resources it needs aren't yet available.
Unfortunately, that doesn't work so well for interrupt controllers
since irq numbers are expected to be correctly populated in the
platform device resource table.  What complicates things further is
that most gpio controllers are doubling as irq controllers nowdays,
and those are typically modelled with platform devices themselves.

One possible approach is to manipulate the order that devices get
registered in by looking explicitly at each device's interrupt-parent
and *-gpio properties, and making sure that interrupt-controllers and
gpio-controllers get registered before any dependent modules, but that
could get complex in a hurry and it doesn't actually solve anything if
one of the irq controller drivers is loaded as a module.  To make it
work, of_platform_populate() would have to actually defer registration
dependent devices until after the irq controller is bound to a driver.
 Blech!  It would be doable by keeping a list of nodes that a device
is dependant on, and snooping bus registrations to look for drivers
bound to the devices referencing that node, but it would be ugly and
would just gloss over the lack of handling dependencies in the driver
core.  Right now we fudge it by messing about with initcall ordering
on the device drivers so that the device driver registration order
doesn't matter, but that doesn't solve the problem caused by drivers
loaded by modules.

More and more, I'm think the solution is to do dependency checking at
.probe time.  If some asynchronous resources (irqs, gpios, phys, etc)
aren't available yet, then it need to be possible to defer
initialization, either by not binding the driver and returning
something like -EAGAIN to say "try me again later", or by letting the
driver explicitly call a deferred init api that with a callback for
when the resources do become available.  I'm really not sure which is
best.  I worry that a deferred init api will end up being complex and
unwieldy, so I'm kind of leaning towards the -EAGAIN solution since it
doesn't require drivers to create a new initialization path.  It would
mean that the driver core would have to keep a list of devices that
had a driver return -EAGAIN, and arrange to reattempt binding at some
point in the future.  Probably after a 'significant' event, such as
another device getting bound to a driver.

...

I should probably actually propose some kind of solution rather than
babbling on about the problem.  I took some time tonight to hack
together what I think the solution should look like.  Take a look at
the following and let me know what you think.  Greg/Kay, I could
certainly use your feedback on this (apologies for any work wrapping,
I'm pasting this into gmail).

---

commit efc3b3168879d0bd9bf9fa3ebfbb1a348394c76f
Author: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
Date:   Fri Jun 24 22:33:46 2011 -0600

    drivercore: Add driver probe deferral mechanism

    Allow drivers to report at probe time that they cannot get all the resources
    required by the device, and should be retried at a later time.

    This should completely solve the problem of getting devices
    initialized in the right order.  Right now this is mostly handled by
    mucking about with initcall ordering which is a complete hack, and
    doesn't even remotely handle the case where device drivers are in
    modules.  This approach completely sidesteps the issues by allowing
    driver registration to occur in any order, and any driver can request
    to be retried after a few more other drivers get probed.

    This patch is *completely untested* and probably utterly broken.
    There is absolutely no locking implemented, and there needs to be
    something around the accesses to the deferred_probe_list (most likely
    a spin lock that gets dropped before each call to bus_probe_device()).
    I'd like to get feedback on if this is the correct approach before I
    do the work of actually making it correct.  :-)

    Signed-off-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>

diff --git a/drivers/base/core.c b/drivers/base/core.c
index bc8729d..83aafff 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -588,6 +588,7 @@ void device_initialize(struct device *dev)
 {
 	dev->kobj.kset = devices_kset;
 	kobject_init(&dev->kobj, &device_ktype);
+	INIT_LIST_HEAD(&dev->deferred_probe);
 	INIT_LIST_HEAD(&dev->dma_pools);
 	mutex_init(&dev->mutex);
 	lockdep_set_novalidate_class(&dev->mutex);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 6658da7..3694661 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -28,6 +28,18 @@
 #include "base.h"
 #include "power/power.h"

+/**
+ * deferred_probe_work_func() - Retry probing devices in the deferred list.
+ */
+static LIST_HEAD(deferred_probe_list);
+static void deferred_probe_work_func(struct work_struct *work)
+{
+	struct device *dev;
+	list_for_each_entry(dev, &deferred_probe_list, deferred_probe) {
+		bus_probe_device(dev);
+	}
+}
+static DECLARE_WORK(deferred_probe_work, deferred_probe_work_func);

 static void driver_bound(struct device *dev)
 {
@@ -41,6 +53,9 @@ static void driver_bound(struct device *dev)
 		 __func__, dev->driver->name);

 	klist_add_tail(&dev->p->knode_driver, &dev->driver->p->klist_devices);
+	if (!list_empty(&dev->deferred_probe))
+		list_del_init(&dev->deferred_probe);
+	schedule_work(&deferred_probe_work);

 	if (dev->bus)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
@@ -142,7 +157,17 @@ probe_failed:
 	driver_sysfs_remove(dev);
 	dev->driver = NULL;

-	if (ret != -ENODEV && ret != -ENXIO) {
+	if (ret == -EAGAIN) {
+		/*
+		 * Probe could not obtain all resources for device and requested
+		 * to be reprobed with the availble drivers at some future point
+		 * in time.  Log the deferral request and add the device to the
+		 * deferred probe list.
+		 */
+		dev_info(dev, "Driver %s requests probe deferral\n", drv->name);
+		if (list_empty(&dev->deferred_probe))
+			list_add(&dev->deferred_probe, &deferred_probe_list);
+	} else if (ret != -ENODEV && ret != -ENXIO) {
 		/* driver matched but the probe failed */
 		printk(KERN_WARNING
 		       "%s: probe of %s failed with error %d\n",
diff --git a/include/linux/device.h b/include/linux/device.h
index c66111a..50cc399 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -506,6 +506,10 @@ struct device_dma_parameters {
  * @mutex:	Mutex to synchronize calls to its driver.
  * @bus:	Type of bus device is on.
  * @driver:	Which driver has allocated this
+ * @deferred_probe: entry in deferred_probe_list which is used to retry the
+ * 		binding of drivers which were unable to get all the resources
+ * 		needed by the device; typically because it depends on another
+ * 		driver getting probed first.
  * @platform_data: Platform data specific to the device.
  * 		Example: For devices on custom boards, as typical of embedded
  * 		and SOC based hardware, Linux often uses platform_data to point
@@ -565,6 +569,7 @@ struct device {
 	struct bus_type	*bus;		/* type of bus device is on */
 	struct device_driver *driver;	/* which driver has allocated this
 					   device */
+	struct list_head	deferred_probe;
 	void		*platform_data;	/* Platform specific data, device
 					   core doesn't touch it */
 	struct dev_pm_info	power;

Re: [RFC PATCH 1/4] dt: early platform devices support

[Marc Zyngier]

On Fri, 24 Jun 2011 22:49:56 -0600
Grant Likely <grant.likely@secretlab.ca> wrote:

> [cc'ing linux kernel since I discuss driver core issues]
> [cc'ing greg and kay, 'cause I've included a patch I'd like you to
> look at (see end of email)]
> 
> On Fri, Jun 24, 2011 at 8:10 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > Add support for populating early platform devices from the device
> > tree, by walking the tree and adding nodes whose 'compatible'
> > property matches the 'class' string passed as a parameter.
> >
> > This allows devices to be probed long before the whole device
> > infrastructure is available.
> >
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > ---
> >  drivers/of/platform.c       |   26 ++++++++++++++++++++++++++
> >  include/linux/of_platform.h |    1 +
> >  2 files changed, 27 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index e75af39..2a323ee 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -458,4 +458,30 @@ int of_platform_populate(struct device_node *root,
> >        of_node_put(root);
> >        return rc;
> >  }
> > +
> > +/**
> > + * of_early_platform_populate() - Populate early platform devices from DT
> > + * @class: string to compare to the 'compatible' attributes
> > + *
> > + * This function walks the device tree and register devices whose
> > + * 'compatible' property matches the 'class' parameter.
> > + *
> > + * Returns 0 on success, < 0 on failure.
> > + */
> > +int of_early_platform_populate(const char *class)
> > +{
> > +       struct platform_device *pdev;
> > +       struct device_node *np = NULL;
> > +
> > +       while ((np = of_find_compatible_node(np, NULL, class))) {
> 
> for_each_compatible_node()
> 
> > +               pdev = of_device_alloc(np, NULL, NULL);
> > +               if (!pdev)
> > +                       return -ENOMEM;
> > +               list_del(&pdev->dev.devres_head);
> > +               memset(&pdev->dev.devres_head, 0, sizeof(pdev->dev.devres_head));
> > +               early_platform_add_devices(&pdev, 1);
> 
> I'm not sure this will work reliably.  The of_platform semantics are
> still slightly different from 'regular' platform devices (which I do
> intend to fix though), so it may cause problems with how the device
> name gets assigned.  I'd need to dig into it though.
> 
> > +       }
> > +
> > +       return 0;
> > +}
> 
> Hmmm, I'd really rather not go down the path of having different
> 'classes' of devices that need to be registered at different times.
> There just isn't a really good generic way to know which devices
> should be the 'early' ones, it gets hairy with regard to making sure
> devices don't get registered twice, and it doesn't actually solve the
> problem that there is no way to handle dependencies between devices in
> the Linux device model.
> 
> What I /want/ to do is allow drivers to defer initialization at .probe
> time if some of the resources it needs aren't yet available.
> Unfortunately, that doesn't work so well for interrupt controllers
> since irq numbers are expected to be correctly populated in the
> platform device resource table.  What complicates things further is
> that most gpio controllers are doubling as irq controllers nowdays,
> and those are typically modelled with platform devices themselves.

While I totally agree with all the above, this patch tries to address a
slightly different problem. On top of device/device dependencies, there
is also a number of implicit dependencies.

In the case I'm currently trying to solve, the kernel expects a timer
to be up and running when hitting the delay loop calibration. At that
stage, it is impossible to register a platform device, hence the use
(abuse?) of early platform devices.

The current ARM code relies on the timers *not* being a standard
device, and being directly setup by an board specific method. The SMP
timers are even worse, as they are directly called by the core code,
meaning that we can only have *one* implementation at a time in the
kernel.

So the early platform stuff is a potential solution for that, though
there is no way it would handle any kind of dependency. What I dream of
is to have the full device/driver infrastructure available much
earlier, before the rest of the kernel starts relying on some hardware
being up and running...

Cheers,

	M.
-- 
I'm the slime oozin' out from your TV set...

[RFC PATCH 1/4] dt: early platform devices support

[Marc Zyngier]

On Fri, 24 Jun 2011 22:49:56 -0600
Grant Likely <grant.likely@secretlab.ca> wrote:

> [cc'ing linux kernel since I discuss driver core issues]
> [cc'ing greg and kay, 'cause I've included a patch I'd like you to
> look at (see end of email)]
> 
> On Fri, Jun 24, 2011 at 8:10 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > Add support for populating early platform devices from the device
> > tree, by walking the tree and adding nodes whose 'compatible'
> > property matches the 'class' string passed as a parameter.
> >
> > This allows devices to be probed long before the whole device
> > infrastructure is available.
> >
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > ---
> > ?drivers/of/platform.c ? ? ? | ? 26 ++++++++++++++++++++++++++
> > ?include/linux/of_platform.h | ? ?1 +
> > ?2 files changed, 27 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index e75af39..2a323ee 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -458,4 +458,30 @@ int of_platform_populate(struct device_node *root,
> > ? ? ? ?of_node_put(root);
> > ? ? ? ?return rc;
> > ?}
> > +
> > +/**
> > + * of_early_platform_populate() - Populate early platform devices from DT
> > + * @class: string to compare to the 'compatible' attributes
> > + *
> > + * This function walks the device tree and register devices whose
> > + * 'compatible' property matches the 'class' parameter.
> > + *
> > + * Returns 0 on success, < 0 on failure.
> > + */
> > +int of_early_platform_populate(const char *class)
> > +{
> > + ? ? ? struct platform_device *pdev;
> > + ? ? ? struct device_node *np = NULL;
> > +
> > + ? ? ? while ((np = of_find_compatible_node(np, NULL, class))) {
> 
> for_each_compatible_node()
> 
> > + ? ? ? ? ? ? ? pdev = of_device_alloc(np, NULL, NULL);
> > + ? ? ? ? ? ? ? if (!pdev)
> > + ? ? ? ? ? ? ? ? ? ? ? return -ENOMEM;
> > + ? ? ? ? ? ? ? list_del(&pdev->dev.devres_head);
> > + ? ? ? ? ? ? ? memset(&pdev->dev.devres_head, 0, sizeof(pdev->dev.devres_head));
> > + ? ? ? ? ? ? ? early_platform_add_devices(&pdev, 1);
> 
> I'm not sure this will work reliably.  The of_platform semantics are
> still slightly different from 'regular' platform devices (which I do
> intend to fix though), so it may cause problems with how the device
> name gets assigned.  I'd need to dig into it though.
> 
> > + ? ? ? }
> > +
> > + ? ? ? return 0;
> > +}
> 
> Hmmm, I'd really rather not go down the path of having different
> 'classes' of devices that need to be registered at different times.
> There just isn't a really good generic way to know which devices
> should be the 'early' ones, it gets hairy with regard to making sure
> devices don't get registered twice, and it doesn't actually solve the
> problem that there is no way to handle dependencies between devices in
> the Linux device model.
> 
> What I /want/ to do is allow drivers to defer initialization at .probe
> time if some of the resources it needs aren't yet available.
> Unfortunately, that doesn't work so well for interrupt controllers
> since irq numbers are expected to be correctly populated in the
> platform device resource table.  What complicates things further is
> that most gpio controllers are doubling as irq controllers nowdays,
> and those are typically modelled with platform devices themselves.

While I totally agree with all the above, this patch tries to address a
slightly different problem. On top of device/device dependencies, there
is also a number of implicit dependencies.

In the case I'm currently trying to solve, the kernel expects a timer
to be up and running when hitting the delay loop calibration. At that
stage, it is impossible to register a platform device, hence the use
(abuse?) of early platform devices.

The current ARM code relies on the timers *not* being a standard
device, and being directly setup by an board specific method. The SMP
timers are even worse, as they are directly called by the core code,
meaning that we can only have *one* implementation at a time in the
kernel.

So the early platform stuff is a potential solution for that, though
there is no way it would handle any kind of dependency. What I dream of
is to have the full device/driver infrastructure available much
earlier, before the rest of the kernel starts relying on some hardware
being up and running...

Cheers,

	M.
-- 
I'm the slime oozin' out from your TV set...

Re: [RFC PATCH 1/4] dt: early platform devices support

[Marc Zyngier]

On Fri, 24 Jun 2011 22:49:56 -0600
Grant Likely <grant.likely@secretlab.ca> wrote:

> [cc'ing linux kernel since I discuss driver core issues]
> [cc'ing greg and kay, 'cause I've included a patch I'd like you to
> look at (see end of email)]
> 
> On Fri, Jun 24, 2011 at 8:10 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > Add support for populating early platform devices from the device
> > tree, by walking the tree and adding nodes whose 'compatible'
> > property matches the 'class' string passed as a parameter.
> >
> > This allows devices to be probed long before the whole device
> > infrastructure is available.
> >
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > ---
> >  drivers/of/platform.c       |   26 ++++++++++++++++++++++++++
> >  include/linux/of_platform.h |    1 +
> >  2 files changed, 27 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index e75af39..2a323ee 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -458,4 +458,30 @@ int of_platform_populate(struct device_node *root,
> >        of_node_put(root);
> >        return rc;
> >  }
> > +
> > +/**
> > + * of_early_platform_populate() - Populate early platform devices from DT
> > + * @class: string to compare to the 'compatible' attributes
> > + *
> > + * This function walks the device tree and register devices whose
> > + * 'compatible' property matches the 'class' parameter.
> > + *
> > + * Returns 0 on success, < 0 on failure.
> > + */
> > +int of_early_platform_populate(const char *class)
> > +{
> > +       struct platform_device *pdev;
> > +       struct device_node *np = NULL;
> > +
> > +       while ((np = of_find_compatible_node(np, NULL, class))) {
> 
> for_each_compatible_node()
> 
> > +               pdev = of_device_alloc(np, NULL, NULL);
> > +               if (!pdev)
> > +                       return -ENOMEM;
> > +               list_del(&pdev->dev.devres_head);
> > +               memset(&pdev->dev.devres_head, 0, sizeof(pdev->dev.devres_head));
> > +               early_platform_add_devices(&pdev, 1);
> 
> I'm not sure this will work reliably.  The of_platform semantics are
> still slightly different from 'regular' platform devices (which I do
> intend to fix though), so it may cause problems with how the device
> name gets assigned.  I'd need to dig into it though.
> 
> > +       }
> > +
> > +       return 0;
> > +}
> 
> Hmmm, I'd really rather not go down the path of having different
> 'classes' of devices that need to be registered at different times.
> There just isn't a really good generic way to know which devices
> should be the 'early' ones, it gets hairy with regard to making sure
> devices don't get registered twice, and it doesn't actually solve the
> problem that there is no way to handle dependencies between devices in
> the Linux device model.
> 
> What I /want/ to do is allow drivers to defer initialization at .probe
> time if some of the resources it needs aren't yet available.
> Unfortunately, that doesn't work so well for interrupt controllers
> since irq numbers are expected to be correctly populated in the
> platform device resource table.  What complicates things further is
> that most gpio controllers are doubling as irq controllers nowdays,
> and those are typically modelled with platform devices themselves.

While I totally agree with all the above, this patch tries to address a
slightly different problem. On top of device/device dependencies, there
is also a number of implicit dependencies.

In the case I'm currently trying to solve, the kernel expects a timer
to be up and running when hitting the delay loop calibration. At that
stage, it is impossible to register a platform device, hence the use
(abuse?) of early platform devices.

The current ARM code relies on the timers *not* being a standard
device, and being directly setup by an board specific method. The SMP
timers are even worse, as they are directly called by the core code,
meaning that we can only have *one* implementation at a time in the
kernel.

So the early platform stuff is a potential solution for that, though
there is no way it would handle any kind of dependency. What I dream of
is to have the full device/driver infrastructure available much
earlier, before the rest of the kernel starts relying on some hardware
being up and running...

Cheers,

	M.
-- 
I'm the slime oozin' out from your TV set...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 1/4] dt: early platform devices support

[Grant Likely]

On Sat, Jun 25, 2011 at 12:11:42PM +0100, Marc Zyngier wrote:
> On Fri, 24 Jun 2011 22:49:56 -0600
> Grant Likely <grant.likely@secretlab.ca> wrote:
> 
> > [cc'ing linux kernel since I discuss driver core issues]
> > [cc'ing greg and kay, 'cause I've included a patch I'd like you to
> > look at (see end of email)]
> > 
> > On Fri, Jun 24, 2011 at 8:10 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > > Add support for populating early platform devices from the device
> > > tree, by walking the tree and adding nodes whose 'compatible'
> > > property matches the 'class' string passed as a parameter.
> > >
> > > This allows devices to be probed long before the whole device
> > > infrastructure is available.
> > >
> > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > > ---
> > >  drivers/of/platform.c       |   26 ++++++++++++++++++++++++++
> > >  include/linux/of_platform.h |    1 +
> > >  2 files changed, 27 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > > index e75af39..2a323ee 100644
> > > --- a/drivers/of/platform.c
> > > +++ b/drivers/of/platform.c
> > > @@ -458,4 +458,30 @@ int of_platform_populate(struct device_node *root,
> > >        of_node_put(root);
> > >        return rc;
> > >  }
> > > +
> > > +/**
> > > + * of_early_platform_populate() - Populate early platform devices from DT
> > > + * @class: string to compare to the 'compatible' attributes
> > > + *
> > > + * This function walks the device tree and register devices whose
> > > + * 'compatible' property matches the 'class' parameter.
> > > + *
> > > + * Returns 0 on success, < 0 on failure.
> > > + */
> > > +int of_early_platform_populate(const char *class)
> > > +{
> > > +       struct platform_device *pdev;
> > > +       struct device_node *np = NULL;
> > > +
> > > +       while ((np = of_find_compatible_node(np, NULL, class))) {
> > 
> > for_each_compatible_node()
> > 
> > > +               pdev = of_device_alloc(np, NULL, NULL);
> > > +               if (!pdev)
> > > +                       return -ENOMEM;
> > > +               list_del(&pdev->dev.devres_head);
> > > +               memset(&pdev->dev.devres_head, 0, sizeof(pdev->dev.devres_head));
> > > +               early_platform_add_devices(&pdev, 1);
> > 
> > I'm not sure this will work reliably.  The of_platform semantics are
> > still slightly different from 'regular' platform devices (which I do
> > intend to fix though), so it may cause problems with how the device
> > name gets assigned.  I'd need to dig into it though.
> > 
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > 
> > Hmmm, I'd really rather not go down the path of having different
> > 'classes' of devices that need to be registered at different times.
> > There just isn't a really good generic way to know which devices
> > should be the 'early' ones, it gets hairy with regard to making sure
> > devices don't get registered twice, and it doesn't actually solve the
> > problem that there is no way to handle dependencies between devices in
> > the Linux device model.
> > 
> > What I /want/ to do is allow drivers to defer initialization at .probe
> > time if some of the resources it needs aren't yet available.
> > Unfortunately, that doesn't work so well for interrupt controllers
> > since irq numbers are expected to be correctly populated in the
> > platform device resource table.  What complicates things further is
> > that most gpio controllers are doubling as irq controllers nowdays,
> > and those are typically modelled with platform devices themselves.
> 
> While I totally agree with all the above, this patch tries to address a
> slightly different problem. On top of device/device dependencies, there
> is also a number of implicit dependencies.
> 
> In the case I'm currently trying to solve, the kernel expects a timer
> to be up and running when hitting the delay loop calibration. At that
> stage, it is impossible to register a platform device, hence the use
> (abuse?) of early platform devices.

:-)  Unfortunately I never did get a chance to read through your
entire patch set, so I missed this.

> The current ARM code relies on the timers *not* being a standard
> device, and being directly setup by an board specific method. The SMP
> timers are even worse, as they are directly called by the core code,
> meaning that we can only have *one* implementation at a time in the
> kernel.
> 
> So the early platform stuff is a potential solution for that, though
> there is no way it would handle any kind of dependency. What I dream of
> is to have the full device/driver infrastructure available much
> earlier, before the rest of the kernel starts relying on some hardware
> being up and running...

My suggestion: don't use platform_device for this.  I think it is
entirely the wrong model.  Keep the timers *not* standard devices.

Instead, use a timer setup function that accepts a list of compatible
properties and an initialization hook for each timer type.  Since
timers have exist, have to be called by core code, and cannot ever be
built as modules, the Linux device model really doesn't offer much
advantage.

It is completely valid and often done to access device tree data from
early setup code without any context of a struct device.

g.

[RFC PATCH 1/4] dt: early platform devices support

[Grant Likely]

On Sat, Jun 25, 2011 at 12:11:42PM +0100, Marc Zyngier wrote:
> On Fri, 24 Jun 2011 22:49:56 -0600
> Grant Likely <grant.likely@secretlab.ca> wrote:
> 
> > [cc'ing linux kernel since I discuss driver core issues]
> > [cc'ing greg and kay, 'cause I've included a patch I'd like you to
> > look at (see end of email)]
> > 
> > On Fri, Jun 24, 2011 at 8:10 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > > Add support for populating early platform devices from the device
> > > tree, by walking the tree and adding nodes whose 'compatible'
> > > property matches the 'class' string passed as a parameter.
> > >
> > > This allows devices to be probed long before the whole device
> > > infrastructure is available.
> > >
> > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > > ---
> > > ?drivers/of/platform.c ? ? ? | ? 26 ++++++++++++++++++++++++++
> > > ?include/linux/of_platform.h | ? ?1 +
> > > ?2 files changed, 27 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > > index e75af39..2a323ee 100644
> > > --- a/drivers/of/platform.c
> > > +++ b/drivers/of/platform.c
> > > @@ -458,4 +458,30 @@ int of_platform_populate(struct device_node *root,
> > > ? ? ? ?of_node_put(root);
> > > ? ? ? ?return rc;
> > > ?}
> > > +
> > > +/**
> > > + * of_early_platform_populate() - Populate early platform devices from DT
> > > + * @class: string to compare to the 'compatible' attributes
> > > + *
> > > + * This function walks the device tree and register devices whose
> > > + * 'compatible' property matches the 'class' parameter.
> > > + *
> > > + * Returns 0 on success, < 0 on failure.
> > > + */
> > > +int of_early_platform_populate(const char *class)
> > > +{
> > > + ? ? ? struct platform_device *pdev;
> > > + ? ? ? struct device_node *np = NULL;
> > > +
> > > + ? ? ? while ((np = of_find_compatible_node(np, NULL, class))) {
> > 
> > for_each_compatible_node()
> > 
> > > + ? ? ? ? ? ? ? pdev = of_device_alloc(np, NULL, NULL);
> > > + ? ? ? ? ? ? ? if (!pdev)
> > > + ? ? ? ? ? ? ? ? ? ? ? return -ENOMEM;
> > > + ? ? ? ? ? ? ? list_del(&pdev->dev.devres_head);
> > > + ? ? ? ? ? ? ? memset(&pdev->dev.devres_head, 0, sizeof(pdev->dev.devres_head));
> > > + ? ? ? ? ? ? ? early_platform_add_devices(&pdev, 1);
> > 
> > I'm not sure this will work reliably.  The of_platform semantics are
> > still slightly different from 'regular' platform devices (which I do
> > intend to fix though), so it may cause problems with how the device
> > name gets assigned.  I'd need to dig into it though.
> > 
> > > + ? ? ? }
> > > +
> > > + ? ? ? return 0;
> > > +}
> > 
> > Hmmm, I'd really rather not go down the path of having different
> > 'classes' of devices that need to be registered at different times.
> > There just isn't a really good generic way to know which devices
> > should be the 'early' ones, it gets hairy with regard to making sure
> > devices don't get registered twice, and it doesn't actually solve the
> > problem that there is no way to handle dependencies between devices in
> > the Linux device model.
> > 
> > What I /want/ to do is allow drivers to defer initialization at .probe
> > time if some of the resources it needs aren't yet available.
> > Unfortunately, that doesn't work so well for interrupt controllers
> > since irq numbers are expected to be correctly populated in the
> > platform device resource table.  What complicates things further is
> > that most gpio controllers are doubling as irq controllers nowdays,
> > and those are typically modelled with platform devices themselves.
> 
> While I totally agree with all the above, this patch tries to address a
> slightly different problem. On top of device/device dependencies, there
> is also a number of implicit dependencies.
> 
> In the case I'm currently trying to solve, the kernel expects a timer
> to be up and running when hitting the delay loop calibration. At that
> stage, it is impossible to register a platform device, hence the use
> (abuse?) of early platform devices.

:-)  Unfortunately I never did get a chance to read through your
entire patch set, so I missed this.

> The current ARM code relies on the timers *not* being a standard
> device, and being directly setup by an board specific method. The SMP
> timers are even worse, as they are directly called by the core code,
> meaning that we can only have *one* implementation at a time in the
> kernel.
> 
> So the early platform stuff is a potential solution for that, though
> there is no way it would handle any kind of dependency. What I dream of
> is to have the full device/driver infrastructure available much
> earlier, before the rest of the kernel starts relying on some hardware
> being up and running...

My suggestion: don't use platform_device for this.  I think it is
entirely the wrong model.  Keep the timers *not* standard devices.

Instead, use a timer setup function that accepts a list of compatible
properties and an initialization hook for each timer type.  Since
timers have exist, have to be called by core code, and cannot ever be
built as modules, the Linux device model really doesn't offer much
advantage.

It is completely valid and often done to access device tree data from
early setup code without any context of a struct device.

g.

Re: [RFC PATCH 1/4] dt: early platform devices support

[Grant Likely]

On Sat, Jun 25, 2011 at 12:11:42PM +0100, Marc Zyngier wrote:
> On Fri, 24 Jun 2011 22:49:56 -0600
> Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> 
> > [cc'ing linux kernel since I discuss driver core issues]
> > [cc'ing greg and kay, 'cause I've included a patch I'd like you to
> > look at (see end of email)]
> > 
> > On Fri, Jun 24, 2011 at 8:10 AM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> > > Add support for populating early platform devices from the device
> > > tree, by walking the tree and adding nodes whose 'compatible'
> > > property matches the 'class' string passed as a parameter.
> > >
> > > This allows devices to be probed long before the whole device
> > > infrastructure is available.
> > >
> > > Signed-off-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
> > > ---
> > >  drivers/of/platform.c       |   26 ++++++++++++++++++++++++++
> > >  include/linux/of_platform.h |    1 +
> > >  2 files changed, 27 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > > index e75af39..2a323ee 100644
> > > --- a/drivers/of/platform.c
> > > +++ b/drivers/of/platform.c
> > > @@ -458,4 +458,30 @@ int of_platform_populate(struct device_node *root,
> > >        of_node_put(root);
> > >        return rc;
> > >  }
> > > +
> > > +/**
> > > + * of_early_platform_populate() - Populate early platform devices from DT
> > > + * @class: string to compare to the 'compatible' attributes
> > > + *
> > > + * This function walks the device tree and register devices whose
> > > + * 'compatible' property matches the 'class' parameter.
> > > + *
> > > + * Returns 0 on success, < 0 on failure.
> > > + */
> > > +int of_early_platform_populate(const char *class)
> > > +{
> > > +       struct platform_device *pdev;
> > > +       struct device_node *np = NULL;
> > > +
> > > +       while ((np = of_find_compatible_node(np, NULL, class))) {
> > 
> > for_each_compatible_node()
> > 
> > > +               pdev = of_device_alloc(np, NULL, NULL);
> > > +               if (!pdev)
> > > +                       return -ENOMEM;
> > > +               list_del(&pdev->dev.devres_head);
> > > +               memset(&pdev->dev.devres_head, 0, sizeof(pdev->dev.devres_head));
> > > +               early_platform_add_devices(&pdev, 1);
> > 
> > I'm not sure this will work reliably.  The of_platform semantics are
> > still slightly different from 'regular' platform devices (which I do
> > intend to fix though), so it may cause problems with how the device
> > name gets assigned.  I'd need to dig into it though.
> > 
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > 
> > Hmmm, I'd really rather not go down the path of having different
> > 'classes' of devices that need to be registered at different times.
> > There just isn't a really good generic way to know which devices
> > should be the 'early' ones, it gets hairy with regard to making sure
> > devices don't get registered twice, and it doesn't actually solve the
> > problem that there is no way to handle dependencies between devices in
> > the Linux device model.
> > 
> > What I /want/ to do is allow drivers to defer initialization at .probe
> > time if some of the resources it needs aren't yet available.
> > Unfortunately, that doesn't work so well for interrupt controllers
> > since irq numbers are expected to be correctly populated in the
> > platform device resource table.  What complicates things further is
> > that most gpio controllers are doubling as irq controllers nowdays,
> > and those are typically modelled with platform devices themselves.
> 
> While I totally agree with all the above, this patch tries to address a
> slightly different problem. On top of device/device dependencies, there
> is also a number of implicit dependencies.
> 
> In the case I'm currently trying to solve, the kernel expects a timer
> to be up and running when hitting the delay loop calibration. At that
> stage, it is impossible to register a platform device, hence the use
> (abuse?) of early platform devices.

:-)  Unfortunately I never did get a chance to read through your
entire patch set, so I missed this.

> The current ARM code relies on the timers *not* being a standard
> device, and being directly setup by an board specific method. The SMP
> timers are even worse, as they are directly called by the core code,
> meaning that we can only have *one* implementation at a time in the
> kernel.
> 
> So the early platform stuff is a potential solution for that, though
> there is no way it would handle any kind of dependency. What I dream of
> is to have the full device/driver infrastructure available much
> earlier, before the rest of the kernel starts relying on some hardware
> being up and running...

My suggestion: don't use platform_device for this.  I think it is
entirely the wrong model.  Keep the timers *not* standard devices.

Instead, use a timer setup function that accepts a list of compatible
properties and an initialization hook for each timer type.  Since
timers have exist, have to be called by core code, and cannot ever be
built as modules, the Linux device model really doesn't offer much
advantage.

It is completely valid and often done to access device tree data from
early setup code without any context of a struct device.

g.

Re: [RFC PATCH 1/4] dt: early platform devices support

[Marc Zyngier]

On 25/06/11 21:44, Grant Likely wrote:
> On Sat, Jun 25, 2011 at 12:11:42PM +0100, Marc Zyngier wrote:
>> On Fri, 24 Jun 2011 22:49:56 -0600
>> The current ARM code relies on the timers *not* being a standard
>> device, and being directly setup by an board specific method. The SMP
>> timers are even worse, as they are directly called by the core code,
>> meaning that we can only have *one* implementation at a time in the
>> kernel.
>>
>> So the early platform stuff is a potential solution for that, though
>> there is no way it would handle any kind of dependency. What I dream of
>> is to have the full device/driver infrastructure available much
>> earlier, before the rest of the kernel starts relying on some hardware
>> being up and running...
> 
> My suggestion: don't use platform_device for this.  I think it is
> entirely the wrong model.  Keep the timers *not* standard devices.
> 
> Instead, use a timer setup function that accepts a list of compatible
> properties and an initialization hook for each timer type.  Since
> timers have exist, have to be called by core code, and cannot ever be
> built as modules, the Linux device model really doesn't offer much
> advantage.
> 
> It is completely valid and often done to access device tree data from
> early setup code without any context of a struct device.

Grant,

There's a few conflicting aspects we need to reconcile here:
- We want to keep the early setup code as simple as possible so it
doesn't rely on anything (well, as little as possible)
- We want to move driver code out of arch/arm (they often are generic,
not specific to a particular architecture)
- We want to support DT and non-DT in a similar way (some ARM platforms
will never see a DT port)

If I follow your advice, I end up with something like the following
thing in the core code:

extern int timerx_probe(struct of_device_id *id);
struct of_device_id timerx_ids[] = {
	...
};

extern int timery_probe(struct of_device_id *id);
struct of_device_id timery_ids[] = {
	...
};

extern int timerz_probe(struct of_device_id *id);
struct of_device_id timerz_ids[] = {
	...
};

struct arm_timer_probe {
	struct of_device_if **ids;
	int (*probe)(struct of_device_id *);
};

struct arm_timer_probe arm_timer_probe_list[] = {
	{ .ids = timerx_ids, probe = timerx_probe, },
	{ .ids = timery_ids, probe = timery_probe, },
	{ .ids = timerz_ids, probe = timerz_probe, },
	{ },
};

... with some additional #ifdef-ery to make that work. This could solve
the first two points above, though each timer_probe function has to
parse the tree itself to gather resources (code duplication).

If you add the non-DT support in the picture, you get an additional,
completely separate code path, where the platform code directly calls
into the driver code, each platform inventing its own stuff. Again.

I think we all agree that early_platform_device are distasteful. They
abuse the platform_device and command line. They give you a false sense
of infrastructure.

But they also offer you:
- a clean separation between core code and drivers, with a standardized
interface
- drivers shared across architectures (sh & shmobile, for example)
- use of common code (fetching resources from DT)
- a relative independence from the level of DT support (the driver only
knows about platform devices).

If there are issues in the early platform stuff, I'd rather tackle them
instead of just inventing yet another custom mechanism. And if we decide
that it is terminally broken, I hope we can agree on something generic
enough to satisfy the above requirements.

Cheers,

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

[RFC PATCH 1/4] dt: early platform devices support

[Marc Zyngier]

On 25/06/11 21:44, Grant Likely wrote:
> On Sat, Jun 25, 2011 at 12:11:42PM +0100, Marc Zyngier wrote:
>> On Fri, 24 Jun 2011 22:49:56 -0600
>> The current ARM code relies on the timers *not* being a standard
>> device, and being directly setup by an board specific method. The SMP
>> timers are even worse, as they are directly called by the core code,
>> meaning that we can only have *one* implementation at a time in the
>> kernel.
>>
>> So the early platform stuff is a potential solution for that, though
>> there is no way it would handle any kind of dependency. What I dream of
>> is to have the full device/driver infrastructure available much
>> earlier, before the rest of the kernel starts relying on some hardware
>> being up and running...
> 
> My suggestion: don't use platform_device for this.  I think it is
> entirely the wrong model.  Keep the timers *not* standard devices.
> 
> Instead, use a timer setup function that accepts a list of compatible
> properties and an initialization hook for each timer type.  Since
> timers have exist, have to be called by core code, and cannot ever be
> built as modules, the Linux device model really doesn't offer much
> advantage.
> 
> It is completely valid and often done to access device tree data from
> early setup code without any context of a struct device.

Grant,

There's a few conflicting aspects we need to reconcile here:
- We want to keep the early setup code as simple as possible so it
doesn't rely on anything (well, as little as possible)
- We want to move driver code out of arch/arm (they often are generic,
not specific to a particular architecture)
- We want to support DT and non-DT in a similar way (some ARM platforms
will never see a DT port)

If I follow your advice, I end up with something like the following
thing in the core code:

extern int timerx_probe(struct of_device_id *id);
struct of_device_id timerx_ids[] = {
	...
};

extern int timery_probe(struct of_device_id *id);
struct of_device_id timery_ids[] = {
	...
};

extern int timerz_probe(struct of_device_id *id);
struct of_device_id timerz_ids[] = {
	...
};

struct arm_timer_probe {
	struct of_device_if **ids;
	int (*probe)(struct of_device_id *);
};

struct arm_timer_probe arm_timer_probe_list[] = {
	{ .ids = timerx_ids, probe = timerx_probe, },
	{ .ids = timery_ids, probe = timery_probe, },
	{ .ids = timerz_ids, probe = timerz_probe, },
	{ },
};

... with some additional #ifdef-ery to make that work. This could solve
the first two points above, though each timer_probe function has to
parse the tree itself to gather resources (code duplication).

If you add the non-DT support in the picture, you get an additional,
completely separate code path, where the platform code directly calls
into the driver code, each platform inventing its own stuff. Again.

I think we all agree that early_platform_device are distasteful. They
abuse the platform_device and command line. They give you a false sense
of infrastructure.

But they also offer you:
- a clean separation between core code and drivers, with a standardized
interface
- drivers shared across architectures (sh & shmobile, for example)
- use of common code (fetching resources from DT)
- a relative independence from the level of DT support (the driver only
knows about platform devices).

If there are issues in the early platform stuff, I'd rather tackle them
instead of just inventing yet another custom mechanism. And if we decide
that it is terminally broken, I hope we can agree on something generic
enough to satisfy the above requirements.

Cheers,

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

Re: [RFC PATCH 1/4] dt: early platform devices support

[Marc Zyngier]

On 25/06/11 21:44, Grant Likely wrote:
> On Sat, Jun 25, 2011 at 12:11:42PM +0100, Marc Zyngier wrote:
>> On Fri, 24 Jun 2011 22:49:56 -0600
>> The current ARM code relies on the timers *not* being a standard
>> device, and being directly setup by an board specific method. The SMP
>> timers are even worse, as they are directly called by the core code,
>> meaning that we can only have *one* implementation at a time in the
>> kernel.
>>
>> So the early platform stuff is a potential solution for that, though
>> there is no way it would handle any kind of dependency. What I dream of
>> is to have the full device/driver infrastructure available much
>> earlier, before the rest of the kernel starts relying on some hardware
>> being up and running...
> 
> My suggestion: don't use platform_device for this.  I think it is
> entirely the wrong model.  Keep the timers *not* standard devices.
> 
> Instead, use a timer setup function that accepts a list of compatible
> properties and an initialization hook for each timer type.  Since
> timers have exist, have to be called by core code, and cannot ever be
> built as modules, the Linux device model really doesn't offer much
> advantage.
> 
> It is completely valid and often done to access device tree data from
> early setup code without any context of a struct device.

Grant,

There's a few conflicting aspects we need to reconcile here:
- We want to keep the early setup code as simple as possible so it
doesn't rely on anything (well, as little as possible)
- We want to move driver code out of arch/arm (they often are generic,
not specific to a particular architecture)
- We want to support DT and non-DT in a similar way (some ARM platforms
will never see a DT port)

If I follow your advice, I end up with something like the following
thing in the core code:

extern int timerx_probe(struct of_device_id *id);
struct of_device_id timerx_ids[] = {
	...
};

extern int timery_probe(struct of_device_id *id);
struct of_device_id timery_ids[] = {
	...
};

extern int timerz_probe(struct of_device_id *id);
struct of_device_id timerz_ids[] = {
	...
};

struct arm_timer_probe {
	struct of_device_if **ids;
	int (*probe)(struct of_device_id *);
};

struct arm_timer_probe arm_timer_probe_list[] = {
	{ .ids = timerx_ids, probe = timerx_probe, },
	{ .ids = timery_ids, probe = timery_probe, },
	{ .ids = timerz_ids, probe = timerz_probe, },
	{ },
};

... with some additional #ifdef-ery to make that work. This could solve
the first two points above, though each timer_probe function has to
parse the tree itself to gather resources (code duplication).

If you add the non-DT support in the picture, you get an additional,
completely separate code path, where the platform code directly calls
into the driver code, each platform inventing its own stuff. Again.

I think we all agree that early_platform_device are distasteful. They
abuse the platform_device and command line. They give you a false sense
of infrastructure.

But they also offer you:
- a clean separation between core code and drivers, with a standardized
interface
- drivers shared across architectures (sh & shmobile, for example)
- use of common code (fetching resources from DT)
- a relative independence from the level of DT support (the driver only
knows about platform devices).

If there are issues in the early platform stuff, I'd rather tackle them
instead of just inventing yet another custom mechanism. And if we decide
that it is terminally broken, I hope we can agree on something generic
enough to satisfy the above requirements.

Cheers,

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

[RFC PATCH 2/4] ARM: dt: register local timers as early platform devices

[Marc Zyngier]

Use of_early_platform_populate() to collect nodes with the
"localtimer" compatible property and register them with
the early platform "bus".

Signed-off-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
---
 arch/arm/kernel/time.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/time.c b/arch/arm/kernel/time.c
index 32d0df8..08a28ef 100644
--- a/arch/arm/kernel/time.c
+++ b/arch/arm/kernel/time.c
@@ -25,6 +25,7 @@
 #include <linux/timer.h>
 #include <linux/irq.h>
 #include <linux/platform_device.h>
+#include <linux/of_platform.h>
 
 #include <linux/mc146818rtc.h>
 
@@ -156,6 +157,9 @@ static void __init __arm_late_time_init(void)
 		arm_late_time_init();
 
 #ifdef CONFIG_LOCAL_TIMER_DEVICES
+#ifdef CONFIG_OF_FLATTREE
+	of_early_platform_populate("localtimer");
+#endif
 	early_platform_driver_register_all("localtimer");
 	early_platform_driver_probe("localtimer", 1, 0);
 #endif
-- 
1.7.0.4

[RFC PATCH 2/4] ARM: dt: register local timers as early platform devices

[Marc Zyngier]

Use of_early_platform_populate() to collect nodes with the
"localtimer" compatible property and register them with
the early platform "bus".

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kernel/time.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/time.c b/arch/arm/kernel/time.c
index 32d0df8..08a28ef 100644
--- a/arch/arm/kernel/time.c
+++ b/arch/arm/kernel/time.c
@@ -25,6 +25,7 @@
 #include <linux/timer.h>
 #include <linux/irq.h>
 #include <linux/platform_device.h>
+#include <linux/of_platform.h>
 
 #include <linux/mc146818rtc.h>
 
@@ -156,6 +157,9 @@ static void __init __arm_late_time_init(void)
 		arm_late_time_init();
 
 #ifdef CONFIG_LOCAL_TIMER_DEVICES
+#ifdef CONFIG_OF_FLATTREE
+	of_early_platform_populate("localtimer");
+#endif
 	early_platform_driver_register_all("localtimer");
 	early_platform_driver_probe("localtimer", 1, 0);
 #endif
-- 
1.7.0.4

Re: [RFC PATCH 2/4] ARM: dt: register local timers as early platform devices

[Grant Likely]

On Fri, Jun 24, 2011 at 03:10:57PM +0100, Marc Zyngier wrote:
> Use of_early_platform_populate() to collect nodes with the
> "localtimer" compatible property and register them with
> the early platform "bus".
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/kernel/time.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/kernel/time.c b/arch/arm/kernel/time.c
> index 32d0df8..08a28ef 100644
> --- a/arch/arm/kernel/time.c
> +++ b/arch/arm/kernel/time.c
> @@ -25,6 +25,7 @@
>  #include <linux/timer.h>
>  #include <linux/irq.h>
>  #include <linux/platform_device.h>
> +#include <linux/of_platform.h>
>  
>  #include <linux/mc146818rtc.h>
>  
> @@ -156,6 +157,9 @@ static void __init __arm_late_time_init(void)
>  		arm_late_time_init();
>  
>  #ifdef CONFIG_LOCAL_TIMER_DEVICES
> +#ifdef CONFIG_OF_FLATTREE
> +	of_early_platform_populate("localtimer");
> +#endif

Rather than #ifdeffing around the function call, it is often cleaner
to have an #else in the header file that defines an empty static
inline.

>  	early_platform_driver_register_all("localtimer");
>  	early_platform_driver_probe("localtimer", 1, 0);

I suggested in my other reply that early_platform_driver should not be
used.  It looks like it is already being used, so I'll back off a bit
from that position.  However, the structure of the code really
shouldn't be any different between clock devices being statically
declared vs. clock data being obtained from the DT.

g.

[RFC PATCH 2/4] ARM: dt: register local timers as early platform devices

[Grant Likely]

On Fri, Jun 24, 2011 at 03:10:57PM +0100, Marc Zyngier wrote:
> Use of_early_platform_populate() to collect nodes with the
> "localtimer" compatible property and register them with
> the early platform "bus".
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm/kernel/time.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/kernel/time.c b/arch/arm/kernel/time.c
> index 32d0df8..08a28ef 100644
> --- a/arch/arm/kernel/time.c
> +++ b/arch/arm/kernel/time.c
> @@ -25,6 +25,7 @@
>  #include <linux/timer.h>
>  #include <linux/irq.h>
>  #include <linux/platform_device.h>
> +#include <linux/of_platform.h>
>  
>  #include <linux/mc146818rtc.h>
>  
> @@ -156,6 +157,9 @@ static void __init __arm_late_time_init(void)
>  		arm_late_time_init();
>  
>  #ifdef CONFIG_LOCAL_TIMER_DEVICES
> +#ifdef CONFIG_OF_FLATTREE
> +	of_early_platform_populate("localtimer");
> +#endif

Rather than #ifdeffing around the function call, it is often cleaner
to have an #else in the header file that defines an empty static
inline.

>  	early_platform_driver_register_all("localtimer");
>  	early_platform_driver_probe("localtimer", 1, 0);

I suggested in my other reply that early_platform_driver should not be
used.  It looks like it is already being used, so I'll back off a bit
from that position.  However, the structure of the code really
shouldn't be any different between clock devices being statically
declared vs. clock data being obtained from the DT.

g.

Re: [RFC PATCH 2/4] ARM: dt: register local timers as early platform devices

[Rob Herring]

Grant,

On 06/25/2011 03:47 PM, Grant Likely wrote:
> On Fri, Jun 24, 2011 at 03:10:57PM +0100, Marc Zyngier wrote:
>> Use of_early_platform_populate() to collect nodes with the
>> "localtimer" compatible property and register them with
>> the early platform "bus".
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
>> ---
>>  arch/arm/kernel/time.c |    4 ++++
>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/kernel/time.c b/arch/arm/kernel/time.c
>> index 32d0df8..08a28ef 100644
>> --- a/arch/arm/kernel/time.c
>> +++ b/arch/arm/kernel/time.c
>> @@ -25,6 +25,7 @@
>>  #include <linux/timer.h>
>>  #include <linux/irq.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/of_platform.h>
>>  
>>  #include <linux/mc146818rtc.h>
>>  
>> @@ -156,6 +157,9 @@ static void __init __arm_late_time_init(void)
>>  		arm_late_time_init();
>>  
>>  #ifdef CONFIG_LOCAL_TIMER_DEVICES
>> +#ifdef CONFIG_OF_FLATTREE
>> +	of_early_platform_populate("localtimer");
>> +#endif
> 
> Rather than #ifdeffing around the function call, it is often cleaner
> to have an #else in the header file that defines an empty static
> inline.
> 
>>  	early_platform_driver_register_all("localtimer");
>>  	early_platform_driver_probe("localtimer", 1, 0);
> 
> I suggested in my other reply that early_platform_driver should not be
> used.  It looks like it is already being used, so I'll back off a bit
> from that position.  However, the structure of the code really
> shouldn't be any different between clock devices being statically
> declared vs. clock data being obtained from the DT.
> 

It's not really already being used. It is added in Marc's previous patch
series to move timers to drivers/clocksource.

Deferring driver probe doesn't really help for timers as they have to be
up early. If early platform drivers shouldn't be used, then why was it
accepted into the kernel in the first place? It doesn't make sense that
it is okay for one arch (sh), but not another (arm), or that it is okay
for non-DT, but not for DT.

Rob

[RFC PATCH 2/4] ARM: dt: register local timers as early platform devices

[Rob Herring]

Grant,

On 06/25/2011 03:47 PM, Grant Likely wrote:
> On Fri, Jun 24, 2011 at 03:10:57PM +0100, Marc Zyngier wrote:
>> Use of_early_platform_populate() to collect nodes with the
>> "localtimer" compatible property and register them with
>> the early platform "bus".
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm/kernel/time.c |    4 ++++
>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/kernel/time.c b/arch/arm/kernel/time.c
>> index 32d0df8..08a28ef 100644
>> --- a/arch/arm/kernel/time.c
>> +++ b/arch/arm/kernel/time.c
>> @@ -25,6 +25,7 @@
>>  #include <linux/timer.h>
>>  #include <linux/irq.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/of_platform.h>
>>  
>>  #include <linux/mc146818rtc.h>
>>  
>> @@ -156,6 +157,9 @@ static void __init __arm_late_time_init(void)
>>  		arm_late_time_init();
>>  
>>  #ifdef CONFIG_LOCAL_TIMER_DEVICES
>> +#ifdef CONFIG_OF_FLATTREE
>> +	of_early_platform_populate("localtimer");
>> +#endif
> 
> Rather than #ifdeffing around the function call, it is often cleaner
> to have an #else in the header file that defines an empty static
> inline.
> 
>>  	early_platform_driver_register_all("localtimer");
>>  	early_platform_driver_probe("localtimer", 1, 0);
> 
> I suggested in my other reply that early_platform_driver should not be
> used.  It looks like it is already being used, so I'll back off a bit
> from that position.  However, the structure of the code really
> shouldn't be any different between clock devices being statically
> declared vs. clock data being obtained from the DT.
> 

It's not really already being used. It is added in Marc's previous patch
series to move timers to drivers/clocksource.

Deferring driver probe doesn't really help for timers as they have to be
up early. If early platform drivers shouldn't be used, then why was it
accepted into the kernel in the first place? It doesn't make sense that
it is okay for one arch (sh), but not another (arm), or that it is okay
for non-DT, but not for DT.

Rob

Re: [RFC PATCH 2/4] ARM: dt: register local timers as early platform devices

[Grant Likely]

[cc'ing Russell since discussing addition of early_platform_device to
arm core code]

On Sat, Jun 25, 2011 at 3:20 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Grant,
>
> On 06/25/2011 03:47 PM, Grant Likely wrote:
>> On Fri, Jun 24, 2011 at 03:10:57PM +0100, Marc Zyngier wrote:
>>> Use of_early_platform_populate() to collect nodes with the
>>> "localtimer" compatible property and register them with
>>> the early platform "bus".
>>>
>>> Signed-off-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
>>> ---
>>>  arch/arm/kernel/time.c |    4 ++++
>>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/arm/kernel/time.c b/arch/arm/kernel/time.c
>>> index 32d0df8..08a28ef 100644
>>> --- a/arch/arm/kernel/time.c
>>> +++ b/arch/arm/kernel/time.c
>>> @@ -25,6 +25,7 @@
>>>  #include <linux/timer.h>
>>>  #include <linux/irq.h>
>>>  #include <linux/platform_device.h>
>>> +#include <linux/of_platform.h>
>>>
>>>  #include <linux/mc146818rtc.h>
>>>
>>> @@ -156,6 +157,9 @@ static void __init __arm_late_time_init(void)
>>>              arm_late_time_init();
>>>
>>>  #ifdef CONFIG_LOCAL_TIMER_DEVICES
>>> +#ifdef CONFIG_OF_FLATTREE
>>> +    of_early_platform_populate("localtimer");
>>> +#endif
>>
>> Rather than #ifdeffing around the function call, it is often cleaner
>> to have an #else in the header file that defines an empty static
>> inline.
>>
>>>      early_platform_driver_register_all("localtimer");
>>>      early_platform_driver_probe("localtimer", 1, 0);
>>
>> I suggested in my other reply that early_platform_driver should not be
>> used.  It looks like it is already being used, so I'll back off a bit
>> from that position.  However, the structure of the code really
>> shouldn't be any different between clock devices being statically
>> declared vs. clock data being obtained from the DT.
>>
>
> It's not really already being used. It is added in Marc's previous patch
> series to move timers to drivers/clocksource.
>
> Deferring driver probe doesn't really help for timers as they have to be
> up early. If early platform drivers shouldn't be used, then why was it
> accepted into the kernel in the first place? It doesn't make sense that
> it is okay for one arch (sh), but not another (arm), ...

There is lots of things in the kernel that aren't necessarily a good
idea.  From what I've seen of early platform devices, I'm not thrilled
with the approach.  If Russell & crew think it is the right solution
for ARM, then I'm not going to make a big stink about it, but I cannot
say I like the model.

>... or that it is okay
> for non-DT, but not for DT.

For registering devices from the DT, it is definitely problematic in a
way that it isn't for non-DT.  When using the DT, how does the
platform know which devices should be registered as 'early' devices?
And once that is done, the population code has then ensure that
devices registered early don't end up with duplicate registrations,
while not difficult, it does complicate both the code and the
conceptual model of registering DT nodes as devices.  I tried to
implement something very similar with of_platform_prepare(), but when
I look at the result I'm just ashamed with myself.

>From my perspective, there is a very limited set of devices that need
to be dealt with early; timer, irq controller, and lldebug console.
>From what I've been told, timers are currently called directly by core
code, and need to be reworked to allow multiplatform kernels.  irq
controllers (or at least the root ones) are initialized with (struct
machine_desc*)->init_irq().  lldebug is currently selected at build
time so that it is available right from head.S.  That's a pretty small
list.  Everything else is just another device, and I don't see
fiddling about with early registration or fiddling with init order to
be a maintainable approach in the long run.  Doing so means that for
each system, someone has to /choose/ which devices are special, and
the decision could be different for each board, and for each kernel
version.  It would be far better to have a driver model that treats
all devices as peers, and is intelligent enough to handle ordering
issues gracefully.

This isn't a big deal for non-DT because using individual board .c
files makes it easy to encode those per-board decisions, whereas the
DT model is a whole lot simpler if a generic & consistent approach can
be used.

g.


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

[RFC PATCH 2/4] ARM: dt: register local timers as early platform devices

[Grant Likely]

[cc'ing Russell since discussing addition of early_platform_device to
arm core code]

On Sat, Jun 25, 2011 at 3:20 PM, Rob Herring <robherring2@gmail.com> wrote:
> Grant,
>
> On 06/25/2011 03:47 PM, Grant Likely wrote:
>> On Fri, Jun 24, 2011 at 03:10:57PM +0100, Marc Zyngier wrote:
>>> Use of_early_platform_populate() to collect nodes with the
>>> "localtimer" compatible property and register them with
>>> the early platform "bus".
>>>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>> ---
>>> ?arch/arm/kernel/time.c | ? ?4 ++++
>>> ?1 files changed, 4 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/arm/kernel/time.c b/arch/arm/kernel/time.c
>>> index 32d0df8..08a28ef 100644
>>> --- a/arch/arm/kernel/time.c
>>> +++ b/arch/arm/kernel/time.c
>>> @@ -25,6 +25,7 @@
>>> ?#include <linux/timer.h>
>>> ?#include <linux/irq.h>
>>> ?#include <linux/platform_device.h>
>>> +#include <linux/of_platform.h>
>>>
>>> ?#include <linux/mc146818rtc.h>
>>>
>>> @@ -156,6 +157,9 @@ static void __init __arm_late_time_init(void)
>>> ? ? ? ? ? ? ?arm_late_time_init();
>>>
>>> ?#ifdef CONFIG_LOCAL_TIMER_DEVICES
>>> +#ifdef CONFIG_OF_FLATTREE
>>> + ? ?of_early_platform_populate("localtimer");
>>> +#endif
>>
>> Rather than #ifdeffing around the function call, it is often cleaner
>> to have an #else in the header file that defines an empty static
>> inline.
>>
>>> ? ? ?early_platform_driver_register_all("localtimer");
>>> ? ? ?early_platform_driver_probe("localtimer", 1, 0);
>>
>> I suggested in my other reply that early_platform_driver should not be
>> used. ?It looks like it is already being used, so I'll back off a bit
>> from that position. ?However, the structure of the code really
>> shouldn't be any different between clock devices being statically
>> declared vs. clock data being obtained from the DT.
>>
>
> It's not really already being used. It is added in Marc's previous patch
> series to move timers to drivers/clocksource.
>
> Deferring driver probe doesn't really help for timers as they have to be
> up early. If early platform drivers shouldn't be used, then why was it
> accepted into the kernel in the first place? It doesn't make sense that
> it is okay for one arch (sh), but not another (arm), ...

There is lots of things in the kernel that aren't necessarily a good
idea.  From what I've seen of early platform devices, I'm not thrilled
with the approach.  If Russell & crew think it is the right solution
for ARM, then I'm not going to make a big stink about it, but I cannot
say I like the model.

>... or that it is okay
> for non-DT, but not for DT.

For registering devices from the DT, it is definitely problematic in a
way that it isn't for non-DT.  When using the DT, how does the
platform know which devices should be registered as 'early' devices?
And once that is done, the population code has then ensure that
devices registered early don't end up with duplicate registrations,
while not difficult, it does complicate both the code and the
conceptual model of registering DT nodes as devices.  I tried to
implement something very similar with of_platform_prepare(), but when
I look at the result I'm just ashamed with myself.


>From bogus@does.not.exist.com  Wed Jun  1 12:03:18 2011
From: bogus@does.not.exist.com ()
Date: Wed, 01 Jun 2011 16:03:18 -0000
Subject: No subject
Message-ID: <mailman.34.1309071675.24103.linux-arm-kernel@lists.infradead.org>

to be dealt with early; timer, irq controller, and lldebug console.

>From bogus@does.not.exist.com  Wed Jun  1 12:03:18 2011
From: bogus@does.not.exist.com ()
Date: Wed, 01 Jun 2011 16:03:18 -0000
Subject: No subject
Message-ID: <mailman.35.1309071675.24103.linux-arm-kernel@lists.infradead.org>

code, and need to be reworked to allow multiplatform kernels.  irq
controllers (or at least the root ones) are initialized with (struct
machine_desc*)->init_irq().  lldebug is currently selected at build
time so that it is available right from head.S.  That's a pretty small
list.  Everything else is just another device, and I don't see
fiddling about with early registration or fiddling with init order to
be a maintainable approach in the long run.  Doing so means that for
each system, someone has to /choose/ which devices are special, and
the decision could be different for each board, and for each kernel
version.  It would be far better to have a driver model that treats
all devices as peers, and is intelligent enough to handle ordering
issues gracefully.

This isn't a big deal for non-DT because using individual board .c
files makes it easy to encode those per-board decisions, whereas the
DT model is a whole lot simpler if a generic & consistent approach can
be used.

g.


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

"Early" devices and the DT

[Mitch Bradley]

Some solutions to the early device problem

a) Most complex:  Use new properties to create a dependency graph.  This 
is probably the most general solution, but also probably the hardest for 
people to think about and maintain.

b) Define a set of init phases 0,1,2,...  and mark each early device 
node with a phase number property.  Scan the tree for each phase and 
handle only the devices with that phase number.  Unmarked nodes are 
handled last.

c) Similar to b, but instead of properties in nodes, have properties 
like "linux-phase0", "linux-phase1", etc in /chosen, whose values are 
lists of phandles.

Admittedly, this is to some extent Linux-specific, but there is a fair 
chance that it would be useful to other OSs, as most OSs have similar 
ordering requirements for certain core devices.  I know that Windows 
works this way - the Windows kernel startup code scans the NT device 
tree multiple times, picking off different devices in different phases.

Some devices might appear in multiple phases.  For example, you might 
want to init a serial device in phase 0, in a simplified mode that 
doesn't depend on interrupts, then do the interrupt controller in phase 
1, then do the timer in phase 2 and also reinit the serial port to use 
interrupts in phase 2.

Re: "Early" devices and the DT

[glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org]

Mitch Bradley <wmb-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org> wrote:

>Some solutions to the early device problem
>
>a) Most complex:  Use new properties to create a dependency graph. 
>This 
>is probably the most general solution, but also probably the hardest
>for 
>people to think about and maintain.
>
>b) Define a set of init phases 0,1,2,...  and mark each early device 
>node with a phase number property.  Scan the tree for each phase and 
>handle only the devices with that phase number.  Unmarked nodes are 
>handled last.
>
>c) Similar to b, but instead of properties in nodes, have properties 
>like "linux-phase0", "linux-phase1", etc in /chosen, whose values are 
>lists of phandles.

d) don't try to encode init order at all in the DT, and let device drivers request probe to be deferred.  If this works out, it will be by far the least complex and won't require anybody to sit and work out the init order for each machine.  I've even already posted a draft patch to do this.

e) work out probe order dynamically by looking at known phandle types at device registration time. This is potentially more 'efficient' than option d, but it requires a lot of knowledge to be built into the registration code which will get really complex in a hurry.

Right now I'm strongly leaning in the direction of option d.

g.

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

"Early" devices and the DT

[glikely@secretlab.ca]

Mitch Bradley <wmb@firmworks.com> wrote:

>Some solutions to the early device problem
>
>a) Most complex:  Use new properties to create a dependency graph. 
>This 
>is probably the most general solution, but also probably the hardest
>for 
>people to think about and maintain.
>
>b) Define a set of init phases 0,1,2,...  and mark each early device 
>node with a phase number property.  Scan the tree for each phase and 
>handle only the devices with that phase number.  Unmarked nodes are 
>handled last.
>
>c) Similar to b, but instead of properties in nodes, have properties 
>like "linux-phase0", "linux-phase1", etc in /chosen, whose values are 
>lists of phandles.

d) don't try to encode init order at all in the DT, and let device drivers request probe to be deferred.  If this works out, it will be by far the least complex and won't require anybody to sit and work out the init order for each machine.  I've even already posted a draft patch to do this.

e) work out probe order dynamically by looking at known phandle types at device registration time. This is potentially more 'efficient' than option d, but it requires a lot of knowledge to be built into the registration code which will get really complex in a hurry.

Right now I'm strongly leaning in the direction of option d.

g.

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

[RFC PATCH 3/4] ARM: dt: add GIC bindings and driver support

[Marc Zyngier]

Convert the GIC driver to use the DT infrastructure. That involves
introducing IRQ domains for the various types of interrupts (PPI and
SPI).

The DT bindings for the GIC are defined as such:
- one interrupt-controller for SPIs
- one interrupt-controller for PPIs per CPU
- interrupt signalling for secondary GICs.

Tested on RealView PB11MPCore and VExpress.

Signed-off-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
---
 Documentation/devicetree/bindings/arm/gic.txt |   92 ++++++++
 arch/arm/common/gic.c                         |  312 ++++++++++++++++++++-----
 arch/arm/include/asm/hardware/gic.h           |    4 +
 3 files changed, 352 insertions(+), 56 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/gic.txt

diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
new file mode 100644
index 0000000..9975345
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/gic.txt
@@ -0,0 +1,92 @@
+* ARM Generic Interrupt Controller
+
+ARM SMP cores are often associated with a GIC, providing per processor
+interrupts (PPI), shared processor interrupts (SPI) and software
+generated interrupts (SGI).
+
+Primary GIC is attached directly to the CPU. Secondary GICs are
+cascaded into the upward interrupt controller.
+
+Main node properties:
+
+- compatible	: "arm,gic"
+- reg		: one register mapping for the distributor interface
+  		  another one for the CPU interface
+- #address-cells: <1>
+- #size-cells	: <0>
+- interrupts	: optionnal, used on secondary GICs only
+
+PPI sub nodes: one node per CPU (primary GIC only)
+
+- interrupt-controller	: required
+- #interrupt-cells	: <1>
+- reg			: index of the CPU this PPI controller is
+			  connected to.
+
+SPI sub node:
+
+- interrupt-controller	: required
+- #interrupt-cells	: <2> First parameter is the interrupt number,
+  			  second is 0 for level-high, 1 for edge-rising.
+
+Primary GIC example (ARM RealView PB11-MPCore):
+
+gic@1f001000 {
+	compatible = "arm,gic";
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	reg = <0x1f001000 0x1000>, /* Distributor interface */
+	      <0x1f000100 0x100>;  /* CPU interface */
+
+	/* One PPI controller per CPU, only on primary GIC */
+	gic0ppi0: gic-ppi@0 {
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		reg = <0>;
+	};
+
+	gic0ppi1: gic-ppi@1 {
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		reg = <1>;
+	};
+
+	gic0ppi2: gic-ppi@2 {
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		reg = <2>;
+	};
+
+	gic0ppi3: gic-ppi@3 {
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		reg = <3>;
+	};
+
+	/* One global SPI controller per GIC, defined on all GICs */
+	gic0spi: gic-spi {
+		interrupt-controller;
+		#interrupt-cells = <2>; /* IRQ, level/edge */
+	};
+};
+
+Secondary GIC example (ARM RealView PB11-MPCore):
+
+gic@1e001000 {
+	compatible = "arm,gic";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	interrupt-parent = <&gic0spi>;
+
+	reg = <0x1e001000 0x1000>, /* Distributor interface */
+	      <0x1e000000 0x100>;  /* CPU interface */
+
+	interrupts = <42 0>; /* Cascade */
+
+	/* One global SPI controller per GIC, defined on all GICs */
+	gic1spi: gic-spi {
+		interrupt-controller;
+		#interrupt-cells = <2>;
+	};
+};
diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index c5acc76..34907ea 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -28,6 +28,9 @@
 #include <linux/smp.h>
 #include <linux/cpumask.h>
 #include <linux/io.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
 
 #include <asm/irq.h>
 #include <asm/mach/irq.h>
@@ -40,15 +43,15 @@ void __iomem *gic_cpu_base_addr __read_mostly;
 
 struct gic_chip_data {
 	unsigned int irq_offset;
+	unsigned int nr_irqs;
 	void __iomem *dist_base;
 	void __iomem *cpu_base;
+	struct irq_domain spi_dom;
+};
+
 #ifdef CONFIG_ARM_GIC_VPPI
-	/* These fields must be 0 on secondary GICs */
-	int	     ppi_base;
-	int	     vppi_base;
-	u16	     nrppis;
+static DEFINE_PER_CPU(struct irq_domain, gic_ppi_domains);
 #endif
-};
 
 /*
  * Supported arch specific GIC irq extension.
@@ -271,36 +274,26 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
 #ifdef CONFIG_ARM_GIC_VPPI
 unsigned int gic_ppi_to_vppi(unsigned int irq)
 {
-	struct gic_chip_data *chip_data = irq_get_chip_data(irq);
-	unsigned int vppi_irq;
-	unsigned int ppi;
+	struct irq_domain *dom = &__get_cpu_var(gic_ppi_domains);
 
-	WARN_ON(!chip_data->vppi_base);
+	return irq_domain_to_irq(dom, irq);
+}
 
-	ppi = irq - chip_data->ppi_base;
-	vppi_irq = ppi + chip_data->nrppis * smp_processor_id();
-	vppi_irq += chip_data->vppi_base;
+unsigned int gic_ppi_to_cpu_vppi(unsigned int irq, int cpu)
+{
+	struct irq_domain *dom = &per_cpu(gic_ppi_domains, cpu);
 
-	return vppi_irq;
+	return irq_domain_to_irq(dom, irq);
 }
 
 static void gic_handle_ppi(unsigned int irq, struct irq_desc *desc)
 {
-	unsigned int vppi_irq;
-
-	vppi_irq = gic_ppi_to_vppi(irq);
-	generic_handle_irq(vppi_irq);
+	generic_handle_irq(gic_ppi_to_vppi(irq));
 }
 
 static struct irq_data *gic_vppi_to_ppi(struct irq_data *d)
 {
-	struct gic_chip_data *chip_data = irq_data_get_irq_chip_data(d);
-	unsigned int ppi_irq;
-
-	ppi_irq = d->irq - chip_data->vppi_base - chip_data->nrppis * smp_processor_id();
-	ppi_irq += chip_data->ppi_base;
-
-	return irq_get_irq_data(ppi_irq);
+	return irq_get_irq_data(d->irq - d->domain->irq_base);
 }
 
 static void gic_ppi_eoi_irq(struct irq_data *d)
@@ -345,15 +338,115 @@ static struct irq_chip gic_ppi_chip = {
 	.irq_set_type		= gic_ppi_set_type,
 	.irq_set_wake		= gic_ppi_set_wake,
 };
+
+#ifdef CONFIG_OF
+static int git_dt_translate(struct irq_domain *dom,
+			    struct device_node *controler,
+			    const u32 *intspec, unsigned int intsize,
+			    irq_hw_number_t *out_hwirq, unsigned int *out_type)
+{
+	if (dom->of_node != controler)
+		return -1;
+
+	if (intsize > 1) {
+		if (intspec[1])
+ 			*out_type = IRQ_TYPE_EDGE_RISING;
+		else
+			*out_type = IRQ_TYPE_LEVEL_HIGH;
+	} else
+		*out_type = IRQ_TYPE_NONE;
+
+	*out_hwirq = intspec[0];
+	return 0;
+}
+
+static struct irq_domain_ops gic_ppi_domain_ops = {
+	.dt_translate = git_dt_translate,
+};
+
+static struct of_device_id gic_ids[] = {
+	{ .compatible = "arm,gic", },
+	{ },
+};
+
+static struct device_node *gic_get_node(struct device_node *np)
+{
+	return of_find_matching_node(np, gic_ids);
+}
+
+static struct device_node *gic_get_ppi_node(struct device_node *gic_np,
+					    int cpu_nr)
+{
+	struct device_node *np = NULL;
+
+	if (!gic_np)
+		return NULL;
+
+	while ((np = of_get_next_child(gic_np, np))) {
+		const __be32 *reg;
+
+		if (strcmp(np->name, "gic-ppi"))
+			continue;
+		reg = of_get_property(np, "reg", NULL);
+		if (!reg)	/* Duh? */
+			continue;
+		if (be32_to_cpu(*reg) == cpu_nr)
+			break;
+	}
+
+	return np;
+}
+
+static struct device_node *gic_get_spi_node(struct device_node *gic_np)
+{
+	struct device_node *np = NULL;
+
+	if (!gic_np)
+		return NULL;
+
+	while ((np = of_get_next_child(gic_np, np))) {
+		if (!strcmp(np->name, "gic-spi"))
+			break;
+	}
+
+	return np;
+}
+#else
+static struct irq_domain_ops gic_ppi_domain_ops; /* Use the defaults */
+
+static struct device_node *gic_get_node(struct device_node *np)
+{
+	return NULL;
+}
+
+static struct device_node *gic_get_ppi_node(struct device_node *gic_np,
+					    int cpu_nr)
+{
+	return NULL;
+}
+
+static struct device_node *gic_get_spi_node(struct device_node *gic_np)
+{
+	return NULL;
+}
+#endif
 #endif
 
+static const char *get_of_name(struct device_node *np)
+{
+	return np ? np->full_name : "";
+}
+
 static void __init gic_dist_init(struct gic_chip_data *gic,
-	unsigned int irq_start)
+				 unsigned int irq_start,
+				 struct device_node *gic_node)
 {
-	unsigned int gic_irqs, irq_limit, i, nrvppis = 0;
+	unsigned int gic_irqs, irq_limit, i, c;
 	void __iomem *base = gic->dist_base;
 	u32 cpumask = 1 << smp_processor_id();
-	u32 dist_ctr, nrcpus;
+	u32 dist_ctr, nrcpus, reg;
+	u32 ppi_base = 0;
+	u16 nrppis = 0;
 
 	cpumask |= cpumask << 8;
 	cpumask |= cpumask << 16;
@@ -372,29 +465,26 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
 	/* Find out how many CPUs are supported (8 max). */
 	nrcpus = ((dist_ctr >> 5) & 7) + 1;
 
-#ifdef CONFIG_ARM_GIC_VPPI
 	/*
 	 * Nobody would be insane enough to use PPIs on a secondary
 	 * GIC, right?
 	 */
 	if (gic == &gic_data[0]) {
-		gic->nrppis = 16 - (irq_start % 16);
-		gic->ppi_base = gic->irq_offset + 32 - gic->nrppis;
-		nrvppis = gic->nrppis * nrcpus;
-	} else {
-		gic->ppi_base = 0;
-		gic->vppi_base = 0;
+		ppi_base = irq_start & 31;
+		nrppis = 16 - (ppi_base & 15);
 	}
-#endif
 
 	pr_info("Configuring GIC with %d sources (%d additional PPIs)\n",
-		gic_irqs, nrvppis);
+		gic_irqs, nrppis * nrcpus);
 
 	/*
-	 * Set all global interrupts to be level triggered, active low.
+	 * Set all global interrupts to be level triggered, active high.
 	 */
-	for (i = 32; i < gic_irqs; i += 16)
-		writel_relaxed(0, base + GIC_DIST_CONFIG + i * 4 / 16);
+	for (i = 32; i < gic_irqs; i += 16) {
+		reg = readl_relaxed(base + GIC_DIST_CONFIG + i * 4 / 16);
+		reg &= 0x55555555;
+		writel_relaxed(reg, base + GIC_DIST_CONFIG + i * 4 / 16);
+	}
 
 	/*
 	 * Set all global interrupts to this CPU only.
@@ -425,30 +515,69 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
 	/*
 	 * Setup the Linux IRQ subsystem.
 	 */
-	for (i = irq_start; i < irq_limit; i++) {
+
+	gic->nr_irqs = gic_irqs;
+	gic->spi_dom.irq_base = irq_start & ~31;
+	gic->spi_dom.ops = &gic_ppi_domain_ops;
+	gic->spi_dom.of_node = gic_get_spi_node(gic_node);
+
+	pr_info("GIC SPI domain %s [%d:%d]\n",
+		get_of_name(gic->spi_dom.of_node), irq_start, irq_limit - 1);
+
+	irq_domain_add(&gic->spi_dom);
+	
+	for (i = ppi_base; i < (irq_limit - gic->spi_dom.irq_base); i++) {
+		unsigned int irq = irq_domain_map(&gic->spi_dom, i);
+		pr_debug("GIC mapping %s:%d to IRQ%d\n",
+			 get_of_name(gic->spi_dom.of_node), i, irq);
 #ifdef CONFIG_ARM_GIC_VPPI
-		if (nrvppis && gic_irq_is_ppi(gic, i))
-			irq_set_chip_and_handler(i, &gic_chip, gic_handle_ppi);
+		if (nrppis && gic_irq_is_ppi(gic, irq))
+			irq_set_chip_and_handler(irq, &gic_chip, gic_handle_ppi);
 		else
 #endif
 		{
-			irq_set_chip_and_handler(i, &gic_chip,
+			irq_set_chip_and_handler(irq, &gic_chip,
 						 handle_fasteoi_irq);
-			set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
+			set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
 		}
-		irq_set_chip_data(i, gic);
+		irq_set_chip_data(irq, gic);
 	}
 
 #ifdef CONFIG_ARM_GIC_VPPI
-	if (!nrvppis)
+	if (!nrppis)
 		goto out;
-	gic->vppi_base = irq_alloc_descs(-1, 0, nrvppis, 0);
-	if (WARN_ON(gic->vppi_base < 0))
-		goto out;
-	for (i = gic->vppi_base; i < (gic->vppi_base + nrvppis); i++) {
-		irq_set_chip_and_handler(i, &gic_ppi_chip, handle_percpu_irq);
-		irq_set_chip_data(i, gic);
-		set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
+	for (c = 0; c < nrcpus; c++) {
+		struct irq_domain *dom = &per_cpu(gic_ppi_domains, c);
+		int base;
+
+		base = irq_alloc_descs(-1, 0, nrppis, 0);
+		if (WARN_ON(base < 0))
+			goto out;
+
+		/*
+		 * We cheat here, and fold ppi_base into irq_base, as
+		 * it saves us some work at runtime.
+		 */
+		dom->irq_base = base - ppi_base;
+		dom->ops = &gic_ppi_domain_ops;
+		dom->of_node = gic_get_ppi_node(gic_node, c);
+
+		pr_info("GIC PPI domain %s [%d:%d] for CPU %d\n",
+			get_of_name(dom->of_node), base, base + nrppis - 1, c);
+
+		irq_domain_add(dom);
+
+		for (i = 0; i < nrppis; i++) {
+			unsigned int irq = irq_domain_map(dom, i + irq_start);
+
+			pr_debug("GIC mapping %s:%d to IRQ%d\n",
+				 get_of_name(dom->of_node), i, irq);
+
+			irq_set_chip_and_handler(irq, &gic_ppi_chip,
+						 handle_percpu_irq);
+			irq_set_chip_data(irq, gic);
+			set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
+		}
 	}
 out:
 #endif
@@ -479,8 +608,9 @@ static void __cpuinit gic_cpu_init(struct gic_chip_data *gic)
 	writel_relaxed(1, base + GIC_CPU_CTRL);
 }
 
-void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
-	void __iomem *dist_base, void __iomem *cpu_base)
+static void __init gic_init_one(unsigned int gic_nr, unsigned int irq_start,
+				void __iomem *dist_base, void __iomem *cpu_base,
+				struct device_node *gic_node)
 {
 	struct gic_chip_data *gic;
 
@@ -494,10 +624,80 @@ void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
 	if (gic_nr == 0)
 		gic_cpu_base_addr = cpu_base;
 
-	gic_dist_init(gic, irq_start);
+	gic_dist_init(gic, irq_start, gic_node);
 	gic_cpu_init(gic);
 }
 
+void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
+		     void __iomem *dist_base, void __iomem *cpu_base)
+{
+	gic_init_one(gic_nr, irq_start, dist_base, cpu_base, NULL);
+}
+
+#ifdef CONFIG_OF
+static void __init gic_of_init_one(int idx, unsigned int irq_start,
+				   struct device_node *np)
+{
+	void __iomem *dist_base;
+	void __iomem *cpu_base;
+
+	dist_base = of_iomap(np, 0);
+	cpu_base  = of_iomap(np, 1);
+	if (!dist_base || ! cpu_base) {
+		pr_err("Cannot map %s\n", np->full_name);
+		return;
+	}
+
+	/* Assume we use the full 16 PPIs */
+	gic_init_one(idx, irq_start, dist_base, cpu_base, np);
+}
+
+void __init gic_of_init(void)
+{
+	struct device_node *np = NULL;
+	unsigned int irq_start = 0;
+	int i = 0;
+
+	/* Look for the GIC providing PPIs first */
+	while((np = gic_get_node(np))) {
+		struct device_node *ppi_np;
+		ppi_np = gic_get_ppi_node(np, 0);
+		of_node_put(ppi_np);
+		if (!ppi_np)
+			continue;
+			
+		/* Assume we use the full 16 PPIs */
+		gic_of_init_one(i, 16, np);
+		irq_start += gic_data[i].nr_irqs;
+		i++;
+		break;
+	}
+	of_node_put(np);
+	np = NULL;
+
+	/* Now all the others */
+	while((np = gic_get_node(np))) {
+		struct device_node *ppi_np;
+		int irq;
+		ppi_np = gic_get_ppi_node(np, 0);
+		of_node_put(ppi_np);
+		if (ppi_np)
+			continue;
+			
+		gic_of_init_one(i, irq_start, np);
+		irq = irq_of_parse_and_map(np, 0);
+		if (irq != IRQ_NONE) {
+			pr_info("GIC %s cascading to IRQ%d",
+				np->full_name, irq);
+			gic_cascade_irq(i, irq);
+		}
+		irq_start += gic_data[i].nr_irqs;
+		i++;
+	}
+	of_node_put(np);
+}
+#endif
+
 void __cpuinit gic_secondary_init(unsigned int gic_nr)
 {
 	BUG_ON(gic_nr >= MAX_GIC_NR);
diff --git a/arch/arm/include/asm/hardware/gic.h b/arch/arm/include/asm/hardware/gic.h
index 069d4c0..04cfe33 100644
--- a/arch/arm/include/asm/hardware/gic.h
+++ b/arch/arm/include/asm/hardware/gic.h
@@ -39,16 +39,20 @@ extern void __iomem *gic_cpu_base_addr;
 extern struct irq_chip gic_arch_extn;
 
 void gic_init(unsigned int, unsigned int, void __iomem *, void __iomem *);
+void gic_of_init(void);
 void gic_secondary_init(unsigned int);
 void gic_cascade_irq(unsigned int gic_nr, unsigned int irq);
 void gic_raise_softirq(const struct cpumask *mask, unsigned int irq);
 #ifdef CONFIG_ARM_GIC_VPPI
 unsigned int gic_ppi_to_vppi(unsigned int irq);
+unsigned int gic_ppi_to_cpu_vppi(unsigned int irq, int cpu);
 #else
 static inline unsigned int gic_ppi_to_vppi(unsigned int irq)
 {
 	return irq;
 }
+
+#define gic_ppi_to_cpu_vppi(i,c) gic_ppi_to_vppi(i)
 #endif
 
 #endif
-- 
1.7.0.4

[RFC PATCH 3/4] ARM: dt: add GIC bindings and driver support

[Marc Zyngier]

Convert the GIC driver to use the DT infrastructure. That involves
introducing IRQ domains for the various types of interrupts (PPI and
SPI).

The DT bindings for the GIC are defined as such:
- one interrupt-controller for SPIs
- one interrupt-controller for PPIs per CPU
- interrupt signalling for secondary GICs.

Tested on RealView PB11MPCore and VExpress.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 Documentation/devicetree/bindings/arm/gic.txt |   92 ++++++++
 arch/arm/common/gic.c                         |  312 ++++++++++++++++++++-----
 arch/arm/include/asm/hardware/gic.h           |    4 +
 3 files changed, 352 insertions(+), 56 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/gic.txt

diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
new file mode 100644
index 0000000..9975345
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/gic.txt
@@ -0,0 +1,92 @@
+* ARM Generic Interrupt Controller
+
+ARM SMP cores are often associated with a GIC, providing per processor
+interrupts (PPI), shared processor interrupts (SPI) and software
+generated interrupts (SGI).
+
+Primary GIC is attached directly to the CPU. Secondary GICs are
+cascaded into the upward interrupt controller.
+
+Main node properties:
+
+- compatible	: "arm,gic"
+- reg		: one register mapping for the distributor interface
+  		  another one for the CPU interface
+- #address-cells: <1>
+- #size-cells	: <0>
+- interrupts	: optionnal, used on secondary GICs only
+
+PPI sub nodes: one node per CPU (primary GIC only)
+
+- interrupt-controller	: required
+- #interrupt-cells	: <1>
+- reg			: index of the CPU this PPI controller is
+			  connected to.
+
+SPI sub node:
+
+- interrupt-controller	: required
+- #interrupt-cells	: <2> First parameter is the interrupt number,
+  			  second is 0 for level-high, 1 for edge-rising.
+
+Primary GIC example (ARM RealView PB11-MPCore):
+
+gic at 1f001000 {
+	compatible = "arm,gic";
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	reg = <0x1f001000 0x1000>, /* Distributor interface */
+	      <0x1f000100 0x100>;  /* CPU interface */
+
+	/* One PPI controller per CPU, only on primary GIC */
+	gic0ppi0: gic-ppi at 0 {
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		reg = <0>;
+	};
+
+	gic0ppi1: gic-ppi at 1 {
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		reg = <1>;
+	};
+
+	gic0ppi2: gic-ppi at 2 {
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		reg = <2>;
+	};
+
+	gic0ppi3: gic-ppi at 3 {
+		interrupt-controller;
+		#interrupt-cells = <1>;
+		reg = <3>;
+	};
+
+	/* One global SPI controller per GIC, defined on all GICs */
+	gic0spi: gic-spi {
+		interrupt-controller;
+		#interrupt-cells = <2>; /* IRQ, level/edge */
+	};
+};
+
+Secondary GIC example (ARM RealView PB11-MPCore):
+
+gic at 1e001000 {
+	compatible = "arm,gic";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	interrupt-parent = <&gic0spi>;
+
+	reg = <0x1e001000 0x1000>, /* Distributor interface */
+	      <0x1e000000 0x100>;  /* CPU interface */
+
+	interrupts = <42 0>; /* Cascade */
+
+	/* One global SPI controller per GIC, defined on all GICs */
+	gic1spi: gic-spi {
+		interrupt-controller;
+		#interrupt-cells = <2>;
+	};
+};
diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index c5acc76..34907ea 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -28,6 +28,9 @@
 #include <linux/smp.h>
 #include <linux/cpumask.h>
 #include <linux/io.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
 
 #include <asm/irq.h>
 #include <asm/mach/irq.h>
@@ -40,15 +43,15 @@ void __iomem *gic_cpu_base_addr __read_mostly;
 
 struct gic_chip_data {
 	unsigned int irq_offset;
+	unsigned int nr_irqs;
 	void __iomem *dist_base;
 	void __iomem *cpu_base;
+	struct irq_domain spi_dom;
+};
+
 #ifdef CONFIG_ARM_GIC_VPPI
-	/* These fields must be 0 on secondary GICs */
-	int	     ppi_base;
-	int	     vppi_base;
-	u16	     nrppis;
+static DEFINE_PER_CPU(struct irq_domain, gic_ppi_domains);
 #endif
-};
 
 /*
  * Supported arch specific GIC irq extension.
@@ -271,36 +274,26 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
 #ifdef CONFIG_ARM_GIC_VPPI
 unsigned int gic_ppi_to_vppi(unsigned int irq)
 {
-	struct gic_chip_data *chip_data = irq_get_chip_data(irq);
-	unsigned int vppi_irq;
-	unsigned int ppi;
+	struct irq_domain *dom = &__get_cpu_var(gic_ppi_domains);
 
-	WARN_ON(!chip_data->vppi_base);
+	return irq_domain_to_irq(dom, irq);
+}
 
-	ppi = irq - chip_data->ppi_base;
-	vppi_irq = ppi + chip_data->nrppis * smp_processor_id();
-	vppi_irq += chip_data->vppi_base;
+unsigned int gic_ppi_to_cpu_vppi(unsigned int irq, int cpu)
+{
+	struct irq_domain *dom = &per_cpu(gic_ppi_domains, cpu);
 
-	return vppi_irq;
+	return irq_domain_to_irq(dom, irq);
 }
 
 static void gic_handle_ppi(unsigned int irq, struct irq_desc *desc)
 {
-	unsigned int vppi_irq;
-
-	vppi_irq = gic_ppi_to_vppi(irq);
-	generic_handle_irq(vppi_irq);
+	generic_handle_irq(gic_ppi_to_vppi(irq));
 }
 
 static struct irq_data *gic_vppi_to_ppi(struct irq_data *d)
 {
-	struct gic_chip_data *chip_data = irq_data_get_irq_chip_data(d);
-	unsigned int ppi_irq;
-
-	ppi_irq = d->irq - chip_data->vppi_base - chip_data->nrppis * smp_processor_id();
-	ppi_irq += chip_data->ppi_base;
-
-	return irq_get_irq_data(ppi_irq);
+	return irq_get_irq_data(d->irq - d->domain->irq_base);
 }
 
 static void gic_ppi_eoi_irq(struct irq_data *d)
@@ -345,15 +338,115 @@ static struct irq_chip gic_ppi_chip = {
 	.irq_set_type		= gic_ppi_set_type,
 	.irq_set_wake		= gic_ppi_set_wake,
 };
+
+#ifdef CONFIG_OF
+static int git_dt_translate(struct irq_domain *dom,
+			    struct device_node *controler,
+			    const u32 *intspec, unsigned int intsize,
+			    irq_hw_number_t *out_hwirq, unsigned int *out_type)
+{
+	if (dom->of_node != controler)
+		return -1;
+
+	if (intsize > 1) {
+		if (intspec[1])
+ 			*out_type = IRQ_TYPE_EDGE_RISING;
+		else
+			*out_type = IRQ_TYPE_LEVEL_HIGH;
+	} else
+		*out_type = IRQ_TYPE_NONE;
+
+	*out_hwirq = intspec[0];
+	return 0;
+}
+
+static struct irq_domain_ops gic_ppi_domain_ops = {
+	.dt_translate = git_dt_translate,
+};
+
+static struct of_device_id gic_ids[] = {
+	{ .compatible = "arm,gic", },
+	{ },
+};
+
+static struct device_node *gic_get_node(struct device_node *np)
+{
+	return of_find_matching_node(np, gic_ids);
+}
+
+static struct device_node *gic_get_ppi_node(struct device_node *gic_np,
+					    int cpu_nr)
+{
+	struct device_node *np = NULL;
+
+	if (!gic_np)
+		return NULL;
+
+	while ((np = of_get_next_child(gic_np, np))) {
+		const __be32 *reg;
+
+		if (strcmp(np->name, "gic-ppi"))
+			continue;
+		reg = of_get_property(np, "reg", NULL);
+		if (!reg)	/* Duh? */
+			continue;
+		if (be32_to_cpu(*reg) == cpu_nr)
+			break;
+	}
+
+	return np;
+}
+
+static struct device_node *gic_get_spi_node(struct device_node *gic_np)
+{
+	struct device_node *np = NULL;
+
+	if (!gic_np)
+		return NULL;
+
+	while ((np = of_get_next_child(gic_np, np))) {
+		if (!strcmp(np->name, "gic-spi"))
+			break;
+	}
+
+	return np;
+}
+#else
+static struct irq_domain_ops gic_ppi_domain_ops; /* Use the defaults */
+
+static struct device_node *gic_get_node(struct device_node *np)
+{
+	return NULL;
+}
+
+static struct device_node *gic_get_ppi_node(struct device_node *gic_np,
+					    int cpu_nr)
+{
+	return NULL;
+}
+
+static struct device_node *gic_get_spi_node(struct device_node *gic_np)
+{
+	return NULL;
+}
+#endif
 #endif
 
+static const char *get_of_name(struct device_node *np)
+{
+	return np ? np->full_name : "";
+}
+
 static void __init gic_dist_init(struct gic_chip_data *gic,
-	unsigned int irq_start)
+				 unsigned int irq_start,
+				 struct device_node *gic_node)
 {
-	unsigned int gic_irqs, irq_limit, i, nrvppis = 0;
+	unsigned int gic_irqs, irq_limit, i, c;
 	void __iomem *base = gic->dist_base;
 	u32 cpumask = 1 << smp_processor_id();
-	u32 dist_ctr, nrcpus;
+	u32 dist_ctr, nrcpus, reg;
+	u32 ppi_base = 0;
+	u16 nrppis = 0;
 
 	cpumask |= cpumask << 8;
 	cpumask |= cpumask << 16;
@@ -372,29 +465,26 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
 	/* Find out how many CPUs are supported (8 max). */
 	nrcpus = ((dist_ctr >> 5) & 7) + 1;
 
-#ifdef CONFIG_ARM_GIC_VPPI
 	/*
 	 * Nobody would be insane enough to use PPIs on a secondary
 	 * GIC, right?
 	 */
 	if (gic == &gic_data[0]) {
-		gic->nrppis = 16 - (irq_start % 16);
-		gic->ppi_base = gic->irq_offset + 32 - gic->nrppis;
-		nrvppis = gic->nrppis * nrcpus;
-	} else {
-		gic->ppi_base = 0;
-		gic->vppi_base = 0;
+		ppi_base = irq_start & 31;
+		nrppis = 16 - (ppi_base & 15);
 	}
-#endif
 
 	pr_info("Configuring GIC with %d sources (%d additional PPIs)\n",
-		gic_irqs, nrvppis);
+		gic_irqs, nrppis * nrcpus);
 
 	/*
-	 * Set all global interrupts to be level triggered, active low.
+	 * Set all global interrupts to be level triggered, active high.
 	 */
-	for (i = 32; i < gic_irqs; i += 16)
-		writel_relaxed(0, base + GIC_DIST_CONFIG + i * 4 / 16);
+	for (i = 32; i < gic_irqs; i += 16) {
+		reg = readl_relaxed(base + GIC_DIST_CONFIG + i * 4 / 16);
+		reg &= 0x55555555;
+		writel_relaxed(reg, base + GIC_DIST_CONFIG + i * 4 / 16);
+	}
 
 	/*
 	 * Set all global interrupts to this CPU only.
@@ -425,30 +515,69 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
 	/*
 	 * Setup the Linux IRQ subsystem.
 	 */
-	for (i = irq_start; i < irq_limit; i++) {
+
+	gic->nr_irqs = gic_irqs;
+	gic->spi_dom.irq_base = irq_start & ~31;
+	gic->spi_dom.ops = &gic_ppi_domain_ops;
+	gic->spi_dom.of_node = gic_get_spi_node(gic_node);
+
+	pr_info("GIC SPI domain %s [%d:%d]\n",
+		get_of_name(gic->spi_dom.of_node), irq_start, irq_limit - 1);
+
+	irq_domain_add(&gic->spi_dom);
+	
+	for (i = ppi_base; i < (irq_limit - gic->spi_dom.irq_base); i++) {
+		unsigned int irq = irq_domain_map(&gic->spi_dom, i);
+		pr_debug("GIC mapping %s:%d to IRQ%d\n",
+			 get_of_name(gic->spi_dom.of_node), i, irq);
 #ifdef CONFIG_ARM_GIC_VPPI
-		if (nrvppis && gic_irq_is_ppi(gic, i))
-			irq_set_chip_and_handler(i, &gic_chip, gic_handle_ppi);
+		if (nrppis && gic_irq_is_ppi(gic, irq))
+			irq_set_chip_and_handler(irq, &gic_chip, gic_handle_ppi);
 		else
 #endif
 		{
-			irq_set_chip_and_handler(i, &gic_chip,
+			irq_set_chip_and_handler(irq, &gic_chip,
 						 handle_fasteoi_irq);
-			set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
+			set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
 		}
-		irq_set_chip_data(i, gic);
+		irq_set_chip_data(irq, gic);
 	}
 
 #ifdef CONFIG_ARM_GIC_VPPI
-	if (!nrvppis)
+	if (!nrppis)
 		goto out;
-	gic->vppi_base = irq_alloc_descs(-1, 0, nrvppis, 0);
-	if (WARN_ON(gic->vppi_base < 0))
-		goto out;
-	for (i = gic->vppi_base; i < (gic->vppi_base + nrvppis); i++) {
-		irq_set_chip_and_handler(i, &gic_ppi_chip, handle_percpu_irq);
-		irq_set_chip_data(i, gic);
-		set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
+	for (c = 0; c < nrcpus; c++) {
+		struct irq_domain *dom = &per_cpu(gic_ppi_domains, c);
+		int base;
+
+		base = irq_alloc_descs(-1, 0, nrppis, 0);
+		if (WARN_ON(base < 0))
+			goto out;
+
+		/*
+		 * We cheat here, and fold ppi_base into irq_base, as
+		 * it saves us some work@runtime.
+		 */
+		dom->irq_base = base - ppi_base;
+		dom->ops = &gic_ppi_domain_ops;
+		dom->of_node = gic_get_ppi_node(gic_node, c);
+
+		pr_info("GIC PPI domain %s [%d:%d] for CPU %d\n",
+			get_of_name(dom->of_node), base, base + nrppis - 1, c);
+
+		irq_domain_add(dom);
+
+		for (i = 0; i < nrppis; i++) {
+			unsigned int irq = irq_domain_map(dom, i + irq_start);
+
+			pr_debug("GIC mapping %s:%d to IRQ%d\n",
+				 get_of_name(dom->of_node), i, irq);
+
+			irq_set_chip_and_handler(irq, &gic_ppi_chip,
+						 handle_percpu_irq);
+			irq_set_chip_data(irq, gic);
+			set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
+		}
 	}
 out:
 #endif
@@ -479,8 +608,9 @@ static void __cpuinit gic_cpu_init(struct gic_chip_data *gic)
 	writel_relaxed(1, base + GIC_CPU_CTRL);
 }
 
-void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
-	void __iomem *dist_base, void __iomem *cpu_base)
+static void __init gic_init_one(unsigned int gic_nr, unsigned int irq_start,
+				void __iomem *dist_base, void __iomem *cpu_base,
+				struct device_node *gic_node)
 {
 	struct gic_chip_data *gic;
 
@@ -494,10 +624,80 @@ void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
 	if (gic_nr == 0)
 		gic_cpu_base_addr = cpu_base;
 
-	gic_dist_init(gic, irq_start);
+	gic_dist_init(gic, irq_start, gic_node);
 	gic_cpu_init(gic);
 }
 
+void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
+		     void __iomem *dist_base, void __iomem *cpu_base)
+{
+	gic_init_one(gic_nr, irq_start, dist_base, cpu_base, NULL);
+}
+
+#ifdef CONFIG_OF
+static void __init gic_of_init_one(int idx, unsigned int irq_start,
+				   struct device_node *np)
+{
+	void __iomem *dist_base;
+	void __iomem *cpu_base;
+
+	dist_base = of_iomap(np, 0);
+	cpu_base  = of_iomap(np, 1);
+	if (!dist_base || ! cpu_base) {
+		pr_err("Cannot map %s\n", np->full_name);
+		return;
+	}
+
+	/* Assume we use the full 16 PPIs */
+	gic_init_one(idx, irq_start, dist_base, cpu_base, np);
+}
+
+void __init gic_of_init(void)
+{
+	struct device_node *np = NULL;
+	unsigned int irq_start = 0;
+	int i = 0;
+
+	/* Look for the GIC providing PPIs first */
+	while((np = gic_get_node(np))) {
+		struct device_node *ppi_np;
+		ppi_np = gic_get_ppi_node(np, 0);
+		of_node_put(ppi_np);
+		if (!ppi_np)
+			continue;
+			
+		/* Assume we use the full 16 PPIs */
+		gic_of_init_one(i, 16, np);
+		irq_start += gic_data[i].nr_irqs;
+		i++;
+		break;
+	}
+	of_node_put(np);
+	np = NULL;
+
+	/* Now all the others */
+	while((np = gic_get_node(np))) {
+		struct device_node *ppi_np;
+		int irq;
+		ppi_np = gic_get_ppi_node(np, 0);
+		of_node_put(ppi_np);
+		if (ppi_np)
+			continue;
+			
+		gic_of_init_one(i, irq_start, np);
+		irq = irq_of_parse_and_map(np, 0);
+		if (irq != IRQ_NONE) {
+			pr_info("GIC %s cascading to IRQ%d",
+				np->full_name, irq);
+			gic_cascade_irq(i, irq);
+		}
+		irq_start += gic_data[i].nr_irqs;
+		i++;
+	}
+	of_node_put(np);
+}
+#endif
+
 void __cpuinit gic_secondary_init(unsigned int gic_nr)
 {
 	BUG_ON(gic_nr >= MAX_GIC_NR);
diff --git a/arch/arm/include/asm/hardware/gic.h b/arch/arm/include/asm/hardware/gic.h
index 069d4c0..04cfe33 100644
--- a/arch/arm/include/asm/hardware/gic.h
+++ b/arch/arm/include/asm/hardware/gic.h
@@ -39,16 +39,20 @@ extern void __iomem *gic_cpu_base_addr;
 extern struct irq_chip gic_arch_extn;
 
 void gic_init(unsigned int, unsigned int, void __iomem *, void __iomem *);
+void gic_of_init(void);
 void gic_secondary_init(unsigned int);
 void gic_cascade_irq(unsigned int gic_nr, unsigned int irq);
 void gic_raise_softirq(const struct cpumask *mask, unsigned int irq);
 #ifdef CONFIG_ARM_GIC_VPPI
 unsigned int gic_ppi_to_vppi(unsigned int irq);
+unsigned int gic_ppi_to_cpu_vppi(unsigned int irq, int cpu);
 #else
 static inline unsigned int gic_ppi_to_vppi(unsigned int irq)
 {
 	return irq;
 }
+
+#define gic_ppi_to_cpu_vppi(i,c) gic_ppi_to_vppi(i)
 #endif
 
 #endif
-- 
1.7.0.4

Re: [RFC PATCH 3/4] ARM: dt: add GIC bindings and driver support

[Grant Likely]

On Fri, Jun 24, 2011 at 03:10:58PM +0100, Marc Zyngier wrote:
> Convert the GIC driver to use the DT infrastructure. That involves
> introducing IRQ domains for the various types of interrupts (PPI and
> SPI).
> 
> The DT bindings for the GIC are defined as such:
> - one interrupt-controller for SPIs
> - one interrupt-controller for PPIs per CPU
> - interrupt signalling for secondary GICs.
> 
> Tested on RealView PB11MPCore and VExpress.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>

Hi Mark,

Thanks for doing this work.  Comments below.

> ---
>  Documentation/devicetree/bindings/arm/gic.txt |   92 ++++++++
>  arch/arm/common/gic.c                         |  312 ++++++++++++++++++++-----
>  arch/arm/include/asm/hardware/gic.h           |    4 +
>  3 files changed, 352 insertions(+), 56 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/gic.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
> new file mode 100644
> index 0000000..9975345
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/gic.txt
> @@ -0,0 +1,92 @@
> +* ARM Generic Interrupt Controller
> +
> +ARM SMP cores are often associated with a GIC, providing per processor
> +interrupts (PPI), shared processor interrupts (SPI) and software
> +generated interrupts (SGI).
> +
> +Primary GIC is attached directly to the CPU. Secondary GICs are
> +cascaded into the upward interrupt controller.
> +
> +Main node properties:
> +
> +- compatible	: "arm,gic"

I'm nervous about simply "arm,gic" with no specific silicon or core
version number attached to it.  Specifications for standard cores such
as this could very well change with newer versions of the core.  I'd
like to see either the core version specified, or this value anchored
on a specific chip implementation.

> +- reg		: one register mapping for the distributor interface
> +  		  another one for the CPU interface
> +- #address-cells: <1>
> +- #size-cells	: <0>
> +- interrupts	: optionnal, used on secondary GICs only

sp.  optional

> +
> +PPI sub nodes: one node per CPU (primary GIC only)
> +
> +- interrupt-controller	: required
> +- #interrupt-cells	: <1>
> +- reg			: index of the CPU this PPI controller is
> +			  connected to.
> +
> +SPI sub node:
> +
> +- interrupt-controller	: required
> +- #interrupt-cells	: <2> First parameter is the interrupt number,
> +  			  second is 0 for level-high, 1 for edge-rising.

Is there any possibility of level-low or edge-falling?  Since irq flags
are essentially arbitrary values chosen by the writer of the binding,
I generally recommend that the best arbitrary values are the ones
currently used by the kernel for IRQF_TRIGGER_*:
	edge-rising: 1
	edge-falling: 2
	level-high: 4
	level-low: 8

> +
> +Primary GIC example (ARM RealView PB11-MPCore):
> +
> +gic@1f001000 {
> +	compatible = "arm,gic";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	reg = <0x1f001000 0x1000>, /* Distributor interface */
> +	      <0x1f000100 0x100>;  /* CPU interface */
> +
> +	/* One PPI controller per CPU, only on primary GIC */
> +	gic0ppi0: gic-ppi@0 {
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		reg = <0>;
> +	};
> +
> +	gic0ppi1: gic-ppi@1 {
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		reg = <1>;
> +	};
> +
> +	gic0ppi2: gic-ppi@2 {
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		reg = <2>;
> +	};
> +
> +	gic0ppi3: gic-ppi@3 {
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		reg = <3>;
> +	};

I don't know much about the GIC PPIs, so it is hard to provide good
feedback on this binding.  Are the PPI interrupts configurable in any
way, or are certain PPI inputs hard wired to specific ppi
controllers?  In other words, does this binding reflect how the device
is hard wired, or is irq affinity a software decision?  If it is
hardwired, then the above binding probably makes sense.  If it is a
software configuration, then I would suggest dropping the per-ppi
nodes and having a flat irq number space that only reflects the
hardware attachments, and use a flags field in the irq specifier if
the ppi affinity isn't something that the kernel can dynamically
assign.

> +
> +	/* One global SPI controller per GIC, defined on all GICs */
> +	gic0spi: gic-spi {
> +		interrupt-controller;
> +		#interrupt-cells = <2>; /* IRQ, level/edge */
> +	};
> +};
> +
> +Secondary GIC example (ARM RealView PB11-MPCore):
> +
> +gic@1e001000 {
> +	compatible = "arm,gic";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	interrupt-parent = <&gic0spi>;
> +
> +	reg = <0x1e001000 0x1000>, /* Distributor interface */
> +	      <0x1e000000 0x100>;  /* CPU interface */
> +
> +	interrupts = <42 0>; /* Cascade */
> +
> +	/* One global SPI controller per GIC, defined on all GICs */
> +	gic1spi: gic-spi {
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +	};
> +};
> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> index c5acc76..34907ea 100644
> --- a/arch/arm/common/gic.c
> +++ b/arch/arm/common/gic.c
> @@ -28,6 +28,9 @@
>  #include <linux/smp.h>
>  #include <linux/cpumask.h>
>  #include <linux/io.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
>  
>  #include <asm/irq.h>
>  #include <asm/mach/irq.h>
> @@ -40,15 +43,15 @@ void __iomem *gic_cpu_base_addr __read_mostly;
>  
>  struct gic_chip_data {
>  	unsigned int irq_offset;
> +	unsigned int nr_irqs;

nr_irq probably needs to be part of struct irq_domain.  I should
change that.

>  	void __iomem *dist_base;
>  	void __iomem *cpu_base;
> +	struct irq_domain spi_dom;
> +};
> +
>  #ifdef CONFIG_ARM_GIC_VPPI
> -	/* These fields must be 0 on secondary GICs */
> -	int	     ppi_base;
> -	int	     vppi_base;
> -	u16	     nrppis;
> +static DEFINE_PER_CPU(struct irq_domain, gic_ppi_domains);
>  #endif
> -};
>  
>  /*
>   * Supported arch specific GIC irq extension.
> @@ -271,36 +274,26 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
>  #ifdef CONFIG_ARM_GIC_VPPI
>  unsigned int gic_ppi_to_vppi(unsigned int irq)
>  {
> -	struct gic_chip_data *chip_data = irq_get_chip_data(irq);
> -	unsigned int vppi_irq;
> -	unsigned int ppi;
> +	struct irq_domain *dom = &__get_cpu_var(gic_ppi_domains);
>  
> -	WARN_ON(!chip_data->vppi_base);
> +	return irq_domain_to_irq(dom, irq);
> +}
>  
> -	ppi = irq - chip_data->ppi_base;
> -	vppi_irq = ppi + chip_data->nrppis * smp_processor_id();
> -	vppi_irq += chip_data->vppi_base;
> +unsigned int gic_ppi_to_cpu_vppi(unsigned int irq, int cpu)
> +{
> +	struct irq_domain *dom = &per_cpu(gic_ppi_domains, cpu);
>  
> -	return vppi_irq;
> +	return irq_domain_to_irq(dom, irq);
>  }
>  
>  static void gic_handle_ppi(unsigned int irq, struct irq_desc *desc)
>  {
> -	unsigned int vppi_irq;
> -
> -	vppi_irq = gic_ppi_to_vppi(irq);
> -	generic_handle_irq(vppi_irq);
> +	generic_handle_irq(gic_ppi_to_vppi(irq));
>  }
>  
>  static struct irq_data *gic_vppi_to_ppi(struct irq_data *d)
>  {
> -	struct gic_chip_data *chip_data = irq_data_get_irq_chip_data(d);
> -	unsigned int ppi_irq;
> -
> -	ppi_irq = d->irq - chip_data->vppi_base - chip_data->nrppis * smp_processor_id();
> -	ppi_irq += chip_data->ppi_base;
> -
> -	return irq_get_irq_data(ppi_irq);
> +	return irq_get_irq_data(d->irq - d->domain->irq_base);
>  }
>  
>  static void gic_ppi_eoi_irq(struct irq_data *d)
> @@ -345,15 +338,115 @@ static struct irq_chip gic_ppi_chip = {
>  	.irq_set_type		= gic_ppi_set_type,
>  	.irq_set_wake		= gic_ppi_set_wake,
>  };
> +
> +#ifdef CONFIG_OF
> +static int git_dt_translate(struct irq_domain *dom,
> +			    struct device_node *controler,
> +			    const u32 *intspec, unsigned int intsize,
> +			    irq_hw_number_t *out_hwirq, unsigned int *out_type)
> +{
> +	if (dom->of_node != controler)
> +		return -1;

-EINVAL

> +
> +	if (intsize > 1) {
> +		if (intspec[1])
> + 			*out_type = IRQ_TYPE_EDGE_RISING;

nit: inconsistent whitespace.

> +		else
> +			*out_type = IRQ_TYPE_LEVEL_HIGH;
> +	} else
> +		*out_type = IRQ_TYPE_NONE;

I would do something like (assuming you take my suggestion above about
arbitrary values):

	if (intsize < 2 || intspec[0] > (max_gic_hwirq_number))
		return -EINVAL;
	/* Binding uses save values as IRQF_TRIGGER_* macros */
	*out_type = intspec[1] & IRQ_TRIGGER_MASK;
	*out_hwirq = intspec[0];
	return 0;

> +
> +static struct irq_domain_ops gic_ppi_domain_ops = {
> +	.dt_translate = git_dt_translate,
> +};
> +
> +static struct of_device_id gic_ids[] = {
> +	{ .compatible = "arm,gic", },
> +	{ },
> +};
> +
> +static struct device_node *gic_get_node(struct device_node *np)
> +{
> +	return of_find_matching_node(np, gic_ids);
> +}
> +
> +static struct device_node *gic_get_ppi_node(struct device_node *gic_np,
> +					    int cpu_nr)
> +{
> +	struct device_node *np = NULL;
> +
> +	if (!gic_np)
> +		return NULL;
> +
> +	while ((np = of_get_next_child(gic_np, np))) {

for_each_child_of_node()

> +		const __be32 *reg;
> +
> +		if (strcmp(np->name, "gic-ppi"))
> +			continue;

Generally, its a good idea to differentiate what a node represents by
compatible property, not name.  This would be a slight change to your
binding, but I think it would be more consistent overall.

> +		reg = of_get_property(np, "reg", NULL);
> +		if (!reg)	/* Duh? */
> +			continue;
> +		if (be32_to_cpu(*reg) == cpu_nr)
> +			break;
> +	}
> +
> +	return np;
> +}
> +
> +static struct device_node *gic_get_spi_node(struct device_node *gic_np)
> +{
> +	struct device_node *np = NULL;
> +
> +	if (!gic_np)
> +		return NULL;
> +
> +	while ((np = of_get_next_child(gic_np, np))) {

for_each_child_of_node()

> +		if (!strcmp(np->name, "gic-spi"))
> +			break;
> +	}
> +
> +	return np;
> +}
> +#else

/* CONFIG_BLAH */ comments are generally appreciated on #else/#endif
statements.

> +static struct irq_domain_ops gic_ppi_domain_ops; /* Use the defaults */
> +
> +static struct device_node *gic_get_node(struct device_node *np)
> +{
> +	return NULL;
> +}
> +
> +static struct device_node *gic_get_ppi_node(struct device_node *gic_np,
> +					    int cpu_nr)
> +{
> +	return NULL;
> +}
> +
> +static struct device_node *gic_get_spi_node(struct device_node *gic_np)
> +{
> +	return NULL;
> +}
> +#endif
>  #endif
>  
> +static const char *get_of_name(struct device_node *np)
> +{
> +	return np ? np->full_name : "";
> +}
> +
>  static void __init gic_dist_init(struct gic_chip_data *gic,
> -	unsigned int irq_start)
> +				 unsigned int irq_start,
> +				 struct device_node *gic_node)
>  {
> -	unsigned int gic_irqs, irq_limit, i, nrvppis = 0;
> +	unsigned int gic_irqs, irq_limit, i, c;
>  	void __iomem *base = gic->dist_base;
>  	u32 cpumask = 1 << smp_processor_id();
> -	u32 dist_ctr, nrcpus;
> +	u32 dist_ctr, nrcpus, reg;
> +	u32 ppi_base = 0;
> +	u16 nrppis = 0;
>  
>  	cpumask |= cpumask << 8;
>  	cpumask |= cpumask << 16;
> @@ -372,29 +465,26 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
>  	/* Find out how many CPUs are supported (8 max). */
>  	nrcpus = ((dist_ctr >> 5) & 7) + 1;
>  
> -#ifdef CONFIG_ARM_GIC_VPPI
>  	/*
>  	 * Nobody would be insane enough to use PPIs on a secondary
>  	 * GIC, right?
>  	 */
>  	if (gic == &gic_data[0]) {
> -		gic->nrppis = 16 - (irq_start % 16);
> -		gic->ppi_base = gic->irq_offset + 32 - gic->nrppis;
> -		nrvppis = gic->nrppis * nrcpus;
> -	} else {
> -		gic->ppi_base = 0;
> -		gic->vppi_base = 0;
> +		ppi_base = irq_start & 31;
> +		nrppis = 16 - (ppi_base & 15);
>  	}
> -#endif
>  
>  	pr_info("Configuring GIC with %d sources (%d additional PPIs)\n",
> -		gic_irqs, nrvppis);
> +		gic_irqs, nrppis * nrcpus);
>  
>  	/*
> -	 * Set all global interrupts to be level triggered, active low.
> +	 * Set all global interrupts to be level triggered, active high.
>  	 */
> -	for (i = 32; i < gic_irqs; i += 16)
> -		writel_relaxed(0, base + GIC_DIST_CONFIG + i * 4 / 16);
> +	for (i = 32; i < gic_irqs; i += 16) {
> +		reg = readl_relaxed(base + GIC_DIST_CONFIG + i * 4 / 16);
> +		reg &= 0x55555555;
> +		writel_relaxed(reg, base + GIC_DIST_CONFIG + i * 4 / 16);
> +	}

Have you got some unrelated fixups in this patch?  It isn't obvious to
me whether or not this hunk is related to setting up irq_domains.

>  
>  	/*
>  	 * Set all global interrupts to this CPU only.
> @@ -425,30 +515,69 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
>  	/*
>  	 * Setup the Linux IRQ subsystem.
>  	 */
> -	for (i = irq_start; i < irq_limit; i++) {
> +
> +	gic->nr_irqs = gic_irqs;
> +	gic->spi_dom.irq_base = irq_start & ~31;
> +	gic->spi_dom.ops = &gic_ppi_domain_ops;
> +	gic->spi_dom.of_node = gic_get_spi_node(gic_node);
> +
> +	pr_info("GIC SPI domain %s [%d:%d]\n",
> +		get_of_name(gic->spi_dom.of_node), irq_start, irq_limit - 1);
> +
> +	irq_domain_add(&gic->spi_dom);
> +	
> +	for (i = ppi_base; i < (irq_limit - gic->spi_dom.irq_base); i++) {
> +		unsigned int irq = irq_domain_map(&gic->spi_dom, i);
> +		pr_debug("GIC mapping %s:%d to IRQ%d\n",
> +			 get_of_name(gic->spi_dom.of_node), i, irq);
>  #ifdef CONFIG_ARM_GIC_VPPI
> -		if (nrvppis && gic_irq_is_ppi(gic, i))
> -			irq_set_chip_and_handler(i, &gic_chip, gic_handle_ppi);
> +		if (nrppis && gic_irq_is_ppi(gic, irq))
> +			irq_set_chip_and_handler(irq, &gic_chip, gic_handle_ppi);
>  		else
>  #endif
>  		{
> -			irq_set_chip_and_handler(i, &gic_chip,
> +			irq_set_chip_and_handler(irq, &gic_chip,
>  						 handle_fasteoi_irq);
> -			set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
> +			set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
>  		}
> -		irq_set_chip_data(i, gic);
> +		irq_set_chip_data(irq, gic);
>  	}
>  
>  #ifdef CONFIG_ARM_GIC_VPPI
> -	if (!nrvppis)
> +	if (!nrppis)
>  		goto out;
> -	gic->vppi_base = irq_alloc_descs(-1, 0, nrvppis, 0);
> -	if (WARN_ON(gic->vppi_base < 0))
> -		goto out;
> -	for (i = gic->vppi_base; i < (gic->vppi_base + nrvppis); i++) {
> -		irq_set_chip_and_handler(i, &gic_ppi_chip, handle_percpu_irq);
> -		irq_set_chip_data(i, gic);
> -		set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
> +	for (c = 0; c < nrcpus; c++) {
> +		struct irq_domain *dom = &per_cpu(gic_ppi_domains, c);
> +		int base;
> +
> +		base = irq_alloc_descs(-1, 0, nrppis, 0);
> +		if (WARN_ON(base < 0))
> +			goto out;
> +
> +		/*
> +		 * We cheat here, and fold ppi_base into irq_base, as
> +		 * it saves us some work at runtime.
> +		 */
> +		dom->irq_base = base - ppi_base;
> +		dom->ops = &gic_ppi_domain_ops;
> +		dom->of_node = gic_get_ppi_node(gic_node, c);
> +
> +		pr_info("GIC PPI domain %s [%d:%d] for CPU %d\n",
> +			get_of_name(dom->of_node), base, base + nrppis - 1, c);
> +
> +		irq_domain_add(dom);
> +
> +		for (i = 0; i < nrppis; i++) {
> +			unsigned int irq = irq_domain_map(dom, i + irq_start);
> +
> +			pr_debug("GIC mapping %s:%d to IRQ%d\n",
> +				 get_of_name(dom->of_node), i, irq);
> +
> +			irq_set_chip_and_handler(irq, &gic_ppi_chip,
> +						 handle_percpu_irq);
> +			irq_set_chip_data(irq, gic);
> +			set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
> +		}
>  	}
>  out:
>  #endif
> @@ -479,8 +608,9 @@ static void __cpuinit gic_cpu_init(struct gic_chip_data *gic)
>  	writel_relaxed(1, base + GIC_CPU_CTRL);
>  }
>  
> -void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
> -	void __iomem *dist_base, void __iomem *cpu_base)
> +static void __init gic_init_one(unsigned int gic_nr, unsigned int irq_start,
> +				void __iomem *dist_base, void __iomem *cpu_base,
> +				struct device_node *gic_node)
>  {
>  	struct gic_chip_data *gic;
>  
> @@ -494,10 +624,80 @@ void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
>  	if (gic_nr == 0)
>  		gic_cpu_base_addr = cpu_base;
>  
> -	gic_dist_init(gic, irq_start);
> +	gic_dist_init(gic, irq_start, gic_node);
>  	gic_cpu_init(gic);
>  }
>  
> +void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
> +		     void __iomem *dist_base, void __iomem *cpu_base)
> +{
> +	gic_init_one(gic_nr, irq_start, dist_base, cpu_base, NULL);
> +}
> +
> +#ifdef CONFIG_OF
> +static void __init gic_of_init_one(int idx, unsigned int irq_start,
> +				   struct device_node *np)
> +{
> +	void __iomem *dist_base;
> +	void __iomem *cpu_base;
> +
> +	dist_base = of_iomap(np, 0);
> +	cpu_base  = of_iomap(np, 1);
> +	if (!dist_base || ! cpu_base) {
> +		pr_err("Cannot map %s\n", np->full_name);
> +		return;
> +	}
> +
> +	/* Assume we use the full 16 PPIs */
> +	gic_init_one(idx, irq_start, dist_base, cpu_base, np);
> +}
> +
> +void __init gic_of_init(void)
> +{
> +	struct device_node *np = NULL;
> +	unsigned int irq_start = 0;
> +	int i = 0;
> +
> +	/* Look for the GIC providing PPIs first */
> +	while((np = gic_get_node(np))) {
> +		struct device_node *ppi_np;
> +		ppi_np = gic_get_ppi_node(np, 0);
> +		of_node_put(ppi_np);
> +		if (!ppi_np)
> +			continue;
> +			
> +		/* Assume we use the full 16 PPIs */
> +		gic_of_init_one(i, 16, np);
> +		irq_start += gic_data[i].nr_irqs;
> +		i++;
> +		break;
> +	}
> +	of_node_put(np);
> +	np = NULL;
> +
> +	/* Now all the others */
> +	while((np = gic_get_node(np))) {
> +		struct device_node *ppi_np;
> +		int irq;
> +		ppi_np = gic_get_ppi_node(np, 0);
> +		of_node_put(ppi_np);
> +		if (ppi_np)
> +			continue;
> +			
> +		gic_of_init_one(i, irq_start, np);
> +		irq = irq_of_parse_and_map(np, 0);
> +		if (irq != IRQ_NONE) {
> +			pr_info("GIC %s cascading to IRQ%d",
> +				np->full_name, irq);
> +			gic_cascade_irq(i, irq);
> +		}
> +		irq_start += gic_data[i].nr_irqs;
> +		i++;
> +	}
> +	of_node_put(np);
> +}
> +#endif

I see what you're doing here, but I don't think it is the best
approach in the long run.  Rather than walking all the
interrupt-controller nodes looking for primary and secondary gics
while ignoring all others, I think there needs to be a function that
finds all the interrupt controller nodes, sorts them based on
dependencies, and calls their initialization hook in order.  Other
architectures will use such a thing.  I've been intending to do it for
powerpc for quite a while node.

Would you be willing to tackle such a beast?  I've got a fairly good
idea what it needs to look like, but I just haven't had the time to do
it.

> +

Overall the patch looks like it is in the right direction.

g.

>  void __cpuinit gic_secondary_init(unsigned int gic_nr)
>  {
>  	BUG_ON(gic_nr >= MAX_GIC_NR);
> diff --git a/arch/arm/include/asm/hardware/gic.h b/arch/arm/include/asm/hardware/gic.h
> index 069d4c0..04cfe33 100644
> --- a/arch/arm/include/asm/hardware/gic.h
> +++ b/arch/arm/include/asm/hardware/gic.h
> @@ -39,16 +39,20 @@ extern void __iomem *gic_cpu_base_addr;
>  extern struct irq_chip gic_arch_extn;
>  
>  void gic_init(unsigned int, unsigned int, void __iomem *, void __iomem *);
> +void gic_of_init(void);
>  void gic_secondary_init(unsigned int);
>  void gic_cascade_irq(unsigned int gic_nr, unsigned int irq);
>  void gic_raise_softirq(const struct cpumask *mask, unsigned int irq);
>  #ifdef CONFIG_ARM_GIC_VPPI
>  unsigned int gic_ppi_to_vppi(unsigned int irq);
> +unsigned int gic_ppi_to_cpu_vppi(unsigned int irq, int cpu);
>  #else
>  static inline unsigned int gic_ppi_to_vppi(unsigned int irq)
>  {
>  	return irq;
>  }
> +
> +#define gic_ppi_to_cpu_vppi(i,c) gic_ppi_to_vppi(i)
>  #endif
>  
>  #endif
> -- 
> 1.7.0.4
> 
> 

[RFC PATCH 3/4] ARM: dt: add GIC bindings and driver support

[Grant Likely]

On Fri, Jun 24, 2011 at 03:10:58PM +0100, Marc Zyngier wrote:
> Convert the GIC driver to use the DT infrastructure. That involves
> introducing IRQ domains for the various types of interrupts (PPI and
> SPI).
> 
> The DT bindings for the GIC are defined as such:
> - one interrupt-controller for SPIs
> - one interrupt-controller for PPIs per CPU
> - interrupt signalling for secondary GICs.
> 
> Tested on RealView PB11MPCore and VExpress.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Hi Mark,

Thanks for doing this work.  Comments below.

> ---
>  Documentation/devicetree/bindings/arm/gic.txt |   92 ++++++++
>  arch/arm/common/gic.c                         |  312 ++++++++++++++++++++-----
>  arch/arm/include/asm/hardware/gic.h           |    4 +
>  3 files changed, 352 insertions(+), 56 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/gic.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
> new file mode 100644
> index 0000000..9975345
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/gic.txt
> @@ -0,0 +1,92 @@
> +* ARM Generic Interrupt Controller
> +
> +ARM SMP cores are often associated with a GIC, providing per processor
> +interrupts (PPI), shared processor interrupts (SPI) and software
> +generated interrupts (SGI).
> +
> +Primary GIC is attached directly to the CPU. Secondary GICs are
> +cascaded into the upward interrupt controller.
> +
> +Main node properties:
> +
> +- compatible	: "arm,gic"

I'm nervous about simply "arm,gic" with no specific silicon or core
version number attached to it.  Specifications for standard cores such
as this could very well change with newer versions of the core.  I'd
like to see either the core version specified, or this value anchored
on a specific chip implementation.

> +- reg		: one register mapping for the distributor interface
> +  		  another one for the CPU interface
> +- #address-cells: <1>
> +- #size-cells	: <0>
> +- interrupts	: optionnal, used on secondary GICs only

sp.  optional

> +
> +PPI sub nodes: one node per CPU (primary GIC only)
> +
> +- interrupt-controller	: required
> +- #interrupt-cells	: <1>
> +- reg			: index of the CPU this PPI controller is
> +			  connected to.
> +
> +SPI sub node:
> +
> +- interrupt-controller	: required
> +- #interrupt-cells	: <2> First parameter is the interrupt number,
> +  			  second is 0 for level-high, 1 for edge-rising.

Is there any possibility of level-low or edge-falling?  Since irq flags
are essentially arbitrary values chosen by the writer of the binding,
I generally recommend that the best arbitrary values are the ones
currently used by the kernel for IRQF_TRIGGER_*:
	edge-rising: 1
	edge-falling: 2
	level-high: 4
	level-low: 8

> +
> +Primary GIC example (ARM RealView PB11-MPCore):
> +
> +gic at 1f001000 {
> +	compatible = "arm,gic";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	reg = <0x1f001000 0x1000>, /* Distributor interface */
> +	      <0x1f000100 0x100>;  /* CPU interface */
> +
> +	/* One PPI controller per CPU, only on primary GIC */
> +	gic0ppi0: gic-ppi at 0 {
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		reg = <0>;
> +	};
> +
> +	gic0ppi1: gic-ppi at 1 {
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		reg = <1>;
> +	};
> +
> +	gic0ppi2: gic-ppi at 2 {
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		reg = <2>;
> +	};
> +
> +	gic0ppi3: gic-ppi at 3 {
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +		reg = <3>;
> +	};

I don't know much about the GIC PPIs, so it is hard to provide good
feedback on this binding.  Are the PPI interrupts configurable in any
way, or are certain PPI inputs hard wired to specific ppi
controllers?  In other words, does this binding reflect how the device
is hard wired, or is irq affinity a software decision?  If it is
hardwired, then the above binding probably makes sense.  If it is a
software configuration, then I would suggest dropping the per-ppi
nodes and having a flat irq number space that only reflects the
hardware attachments, and use a flags field in the irq specifier if
the ppi affinity isn't something that the kernel can dynamically
assign.

> +
> +	/* One global SPI controller per GIC, defined on all GICs */
> +	gic0spi: gic-spi {
> +		interrupt-controller;
> +		#interrupt-cells = <2>; /* IRQ, level/edge */
> +	};
> +};
> +
> +Secondary GIC example (ARM RealView PB11-MPCore):
> +
> +gic at 1e001000 {
> +	compatible = "arm,gic";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	interrupt-parent = <&gic0spi>;
> +
> +	reg = <0x1e001000 0x1000>, /* Distributor interface */
> +	      <0x1e000000 0x100>;  /* CPU interface */
> +
> +	interrupts = <42 0>; /* Cascade */
> +
> +	/* One global SPI controller per GIC, defined on all GICs */
> +	gic1spi: gic-spi {
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +	};
> +};
> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> index c5acc76..34907ea 100644
> --- a/arch/arm/common/gic.c
> +++ b/arch/arm/common/gic.c
> @@ -28,6 +28,9 @@
>  #include <linux/smp.h>
>  #include <linux/cpumask.h>
>  #include <linux/io.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
>  
>  #include <asm/irq.h>
>  #include <asm/mach/irq.h>
> @@ -40,15 +43,15 @@ void __iomem *gic_cpu_base_addr __read_mostly;
>  
>  struct gic_chip_data {
>  	unsigned int irq_offset;
> +	unsigned int nr_irqs;

nr_irq probably needs to be part of struct irq_domain.  I should
change that.

>  	void __iomem *dist_base;
>  	void __iomem *cpu_base;
> +	struct irq_domain spi_dom;
> +};
> +
>  #ifdef CONFIG_ARM_GIC_VPPI
> -	/* These fields must be 0 on secondary GICs */
> -	int	     ppi_base;
> -	int	     vppi_base;
> -	u16	     nrppis;
> +static DEFINE_PER_CPU(struct irq_domain, gic_ppi_domains);
>  #endif
> -};
>  
>  /*
>   * Supported arch specific GIC irq extension.
> @@ -271,36 +274,26 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
>  #ifdef CONFIG_ARM_GIC_VPPI
>  unsigned int gic_ppi_to_vppi(unsigned int irq)
>  {
> -	struct gic_chip_data *chip_data = irq_get_chip_data(irq);
> -	unsigned int vppi_irq;
> -	unsigned int ppi;
> +	struct irq_domain *dom = &__get_cpu_var(gic_ppi_domains);
>  
> -	WARN_ON(!chip_data->vppi_base);
> +	return irq_domain_to_irq(dom, irq);
> +}
>  
> -	ppi = irq - chip_data->ppi_base;
> -	vppi_irq = ppi + chip_data->nrppis * smp_processor_id();
> -	vppi_irq += chip_data->vppi_base;
> +unsigned int gic_ppi_to_cpu_vppi(unsigned int irq, int cpu)
> +{
> +	struct irq_domain *dom = &per_cpu(gic_ppi_domains, cpu);
>  
> -	return vppi_irq;
> +	return irq_domain_to_irq(dom, irq);
>  }
>  
>  static void gic_handle_ppi(unsigned int irq, struct irq_desc *desc)
>  {
> -	unsigned int vppi_irq;
> -
> -	vppi_irq = gic_ppi_to_vppi(irq);
> -	generic_handle_irq(vppi_irq);
> +	generic_handle_irq(gic_ppi_to_vppi(irq));
>  }
>  
>  static struct irq_data *gic_vppi_to_ppi(struct irq_data *d)
>  {
> -	struct gic_chip_data *chip_data = irq_data_get_irq_chip_data(d);
> -	unsigned int ppi_irq;
> -
> -	ppi_irq = d->irq - chip_data->vppi_base - chip_data->nrppis * smp_processor_id();
> -	ppi_irq += chip_data->ppi_base;
> -
> -	return irq_get_irq_data(ppi_irq);
> +	return irq_get_irq_data(d->irq - d->domain->irq_base);
>  }
>  
>  static void gic_ppi_eoi_irq(struct irq_data *d)
> @@ -345,15 +338,115 @@ static struct irq_chip gic_ppi_chip = {
>  	.irq_set_type		= gic_ppi_set_type,
>  	.irq_set_wake		= gic_ppi_set_wake,
>  };
> +
> +#ifdef CONFIG_OF
> +static int git_dt_translate(struct irq_domain *dom,
> +			    struct device_node *controler,
> +			    const u32 *intspec, unsigned int intsize,
> +			    irq_hw_number_t *out_hwirq, unsigned int *out_type)
> +{
> +	if (dom->of_node != controler)
> +		return -1;

-EINVAL

> +
> +	if (intsize > 1) {
> +		if (intspec[1])
> + 			*out_type = IRQ_TYPE_EDGE_RISING;

nit: inconsistent whitespace.

> +		else
> +			*out_type = IRQ_TYPE_LEVEL_HIGH;
> +	} else
> +		*out_type = IRQ_TYPE_NONE;

I would do something like (assuming you take my suggestion above about
arbitrary values):

	if (intsize < 2 || intspec[0] > (max_gic_hwirq_number))
		return -EINVAL;
	/* Binding uses save values as IRQF_TRIGGER_* macros */
	*out_type = intspec[1] & IRQ_TRIGGER_MASK;
	*out_hwirq = intspec[0];
	return 0;

> +
> +static struct irq_domain_ops gic_ppi_domain_ops = {
> +	.dt_translate = git_dt_translate,
> +};
> +
> +static struct of_device_id gic_ids[] = {
> +	{ .compatible = "arm,gic", },
> +	{ },
> +};
> +
> +static struct device_node *gic_get_node(struct device_node *np)
> +{
> +	return of_find_matching_node(np, gic_ids);
> +}
> +
> +static struct device_node *gic_get_ppi_node(struct device_node *gic_np,
> +					    int cpu_nr)
> +{
> +	struct device_node *np = NULL;
> +
> +	if (!gic_np)
> +		return NULL;
> +
> +	while ((np = of_get_next_child(gic_np, np))) {

for_each_child_of_node()

> +		const __be32 *reg;
> +
> +		if (strcmp(np->name, "gic-ppi"))
> +			continue;

Generally, its a good idea to differentiate what a node represents by
compatible property, not name.  This would be a slight change to your
binding, but I think it would be more consistent overall.

> +		reg = of_get_property(np, "reg", NULL);
> +		if (!reg)	/* Duh? */
> +			continue;
> +		if (be32_to_cpu(*reg) == cpu_nr)
> +			break;
> +	}
> +
> +	return np;
> +}
> +
> +static struct device_node *gic_get_spi_node(struct device_node *gic_np)
> +{
> +	struct device_node *np = NULL;
> +
> +	if (!gic_np)
> +		return NULL;
> +
> +	while ((np = of_get_next_child(gic_np, np))) {

for_each_child_of_node()

> +		if (!strcmp(np->name, "gic-spi"))
> +			break;
> +	}
> +
> +	return np;
> +}
> +#else

/* CONFIG_BLAH */ comments are generally appreciated on #else/#endif
statements.

> +static struct irq_domain_ops gic_ppi_domain_ops; /* Use the defaults */
> +
> +static struct device_node *gic_get_node(struct device_node *np)
> +{
> +	return NULL;
> +}
> +
> +static struct device_node *gic_get_ppi_node(struct device_node *gic_np,
> +					    int cpu_nr)
> +{
> +	return NULL;
> +}
> +
> +static struct device_node *gic_get_spi_node(struct device_node *gic_np)
> +{
> +	return NULL;
> +}
> +#endif
>  #endif
>  
> +static const char *get_of_name(struct device_node *np)
> +{
> +	return np ? np->full_name : "";
> +}
> +
>  static void __init gic_dist_init(struct gic_chip_data *gic,
> -	unsigned int irq_start)
> +				 unsigned int irq_start,
> +				 struct device_node *gic_node)
>  {
> -	unsigned int gic_irqs, irq_limit, i, nrvppis = 0;
> +	unsigned int gic_irqs, irq_limit, i, c;
>  	void __iomem *base = gic->dist_base;
>  	u32 cpumask = 1 << smp_processor_id();
> -	u32 dist_ctr, nrcpus;
> +	u32 dist_ctr, nrcpus, reg;
> +	u32 ppi_base = 0;
> +	u16 nrppis = 0;
>  
>  	cpumask |= cpumask << 8;
>  	cpumask |= cpumask << 16;
> @@ -372,29 +465,26 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
>  	/* Find out how many CPUs are supported (8 max). */
>  	nrcpus = ((dist_ctr >> 5) & 7) + 1;
>  
> -#ifdef CONFIG_ARM_GIC_VPPI
>  	/*
>  	 * Nobody would be insane enough to use PPIs on a secondary
>  	 * GIC, right?
>  	 */
>  	if (gic == &gic_data[0]) {
> -		gic->nrppis = 16 - (irq_start % 16);
> -		gic->ppi_base = gic->irq_offset + 32 - gic->nrppis;
> -		nrvppis = gic->nrppis * nrcpus;
> -	} else {
> -		gic->ppi_base = 0;
> -		gic->vppi_base = 0;
> +		ppi_base = irq_start & 31;
> +		nrppis = 16 - (ppi_base & 15);
>  	}
> -#endif
>  
>  	pr_info("Configuring GIC with %d sources (%d additional PPIs)\n",
> -		gic_irqs, nrvppis);
> +		gic_irqs, nrppis * nrcpus);
>  
>  	/*
> -	 * Set all global interrupts to be level triggered, active low.
> +	 * Set all global interrupts to be level triggered, active high.
>  	 */
> -	for (i = 32; i < gic_irqs; i += 16)
> -		writel_relaxed(0, base + GIC_DIST_CONFIG + i * 4 / 16);
> +	for (i = 32; i < gic_irqs; i += 16) {
> +		reg = readl_relaxed(base + GIC_DIST_CONFIG + i * 4 / 16);
> +		reg &= 0x55555555;
> +		writel_relaxed(reg, base + GIC_DIST_CONFIG + i * 4 / 16);
> +	}

Have you got some unrelated fixups in this patch?  It isn't obvious to
me whether or not this hunk is related to setting up irq_domains.

>  
>  	/*
>  	 * Set all global interrupts to this CPU only.
> @@ -425,30 +515,69 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
>  	/*
>  	 * Setup the Linux IRQ subsystem.
>  	 */
> -	for (i = irq_start; i < irq_limit; i++) {
> +
> +	gic->nr_irqs = gic_irqs;
> +	gic->spi_dom.irq_base = irq_start & ~31;
> +	gic->spi_dom.ops = &gic_ppi_domain_ops;
> +	gic->spi_dom.of_node = gic_get_spi_node(gic_node);
> +
> +	pr_info("GIC SPI domain %s [%d:%d]\n",
> +		get_of_name(gic->spi_dom.of_node), irq_start, irq_limit - 1);
> +
> +	irq_domain_add(&gic->spi_dom);
> +	
> +	for (i = ppi_base; i < (irq_limit - gic->spi_dom.irq_base); i++) {
> +		unsigned int irq = irq_domain_map(&gic->spi_dom, i);
> +		pr_debug("GIC mapping %s:%d to IRQ%d\n",
> +			 get_of_name(gic->spi_dom.of_node), i, irq);
>  #ifdef CONFIG_ARM_GIC_VPPI
> -		if (nrvppis && gic_irq_is_ppi(gic, i))
> -			irq_set_chip_and_handler(i, &gic_chip, gic_handle_ppi);
> +		if (nrppis && gic_irq_is_ppi(gic, irq))
> +			irq_set_chip_and_handler(irq, &gic_chip, gic_handle_ppi);
>  		else
>  #endif
>  		{
> -			irq_set_chip_and_handler(i, &gic_chip,
> +			irq_set_chip_and_handler(irq, &gic_chip,
>  						 handle_fasteoi_irq);
> -			set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
> +			set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
>  		}
> -		irq_set_chip_data(i, gic);
> +		irq_set_chip_data(irq, gic);
>  	}
>  
>  #ifdef CONFIG_ARM_GIC_VPPI
> -	if (!nrvppis)
> +	if (!nrppis)
>  		goto out;
> -	gic->vppi_base = irq_alloc_descs(-1, 0, nrvppis, 0);
> -	if (WARN_ON(gic->vppi_base < 0))
> -		goto out;
> -	for (i = gic->vppi_base; i < (gic->vppi_base + nrvppis); i++) {
> -		irq_set_chip_and_handler(i, &gic_ppi_chip, handle_percpu_irq);
> -		irq_set_chip_data(i, gic);
> -		set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
> +	for (c = 0; c < nrcpus; c++) {
> +		struct irq_domain *dom = &per_cpu(gic_ppi_domains, c);
> +		int base;
> +
> +		base = irq_alloc_descs(-1, 0, nrppis, 0);
> +		if (WARN_ON(base < 0))
> +			goto out;
> +
> +		/*
> +		 * We cheat here, and fold ppi_base into irq_base, as
> +		 * it saves us some work at runtime.
> +		 */
> +		dom->irq_base = base - ppi_base;
> +		dom->ops = &gic_ppi_domain_ops;
> +		dom->of_node = gic_get_ppi_node(gic_node, c);
> +
> +		pr_info("GIC PPI domain %s [%d:%d] for CPU %d\n",
> +			get_of_name(dom->of_node), base, base + nrppis - 1, c);
> +
> +		irq_domain_add(dom);
> +
> +		for (i = 0; i < nrppis; i++) {
> +			unsigned int irq = irq_domain_map(dom, i + irq_start);
> +
> +			pr_debug("GIC mapping %s:%d to IRQ%d\n",
> +				 get_of_name(dom->of_node), i, irq);
> +
> +			irq_set_chip_and_handler(irq, &gic_ppi_chip,
> +						 handle_percpu_irq);
> +			irq_set_chip_data(irq, gic);
> +			set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
> +		}
>  	}
>  out:
>  #endif
> @@ -479,8 +608,9 @@ static void __cpuinit gic_cpu_init(struct gic_chip_data *gic)
>  	writel_relaxed(1, base + GIC_CPU_CTRL);
>  }
>  
> -void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
> -	void __iomem *dist_base, void __iomem *cpu_base)
> +static void __init gic_init_one(unsigned int gic_nr, unsigned int irq_start,
> +				void __iomem *dist_base, void __iomem *cpu_base,
> +				struct device_node *gic_node)
>  {
>  	struct gic_chip_data *gic;
>  
> @@ -494,10 +624,80 @@ void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
>  	if (gic_nr == 0)
>  		gic_cpu_base_addr = cpu_base;
>  
> -	gic_dist_init(gic, irq_start);
> +	gic_dist_init(gic, irq_start, gic_node);
>  	gic_cpu_init(gic);
>  }
>  
> +void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
> +		     void __iomem *dist_base, void __iomem *cpu_base)
> +{
> +	gic_init_one(gic_nr, irq_start, dist_base, cpu_base, NULL);
> +}
> +
> +#ifdef CONFIG_OF
> +static void __init gic_of_init_one(int idx, unsigned int irq_start,
> +				   struct device_node *np)
> +{
> +	void __iomem *dist_base;
> +	void __iomem *cpu_base;
> +
> +	dist_base = of_iomap(np, 0);
> +	cpu_base  = of_iomap(np, 1);
> +	if (!dist_base || ! cpu_base) {
> +		pr_err("Cannot map %s\n", np->full_name);
> +		return;
> +	}
> +
> +	/* Assume we use the full 16 PPIs */
> +	gic_init_one(idx, irq_start, dist_base, cpu_base, np);
> +}
> +
> +void __init gic_of_init(void)
> +{
> +	struct device_node *np = NULL;
> +	unsigned int irq_start = 0;
> +	int i = 0;
> +
> +	/* Look for the GIC providing PPIs first */
> +	while((np = gic_get_node(np))) {
> +		struct device_node *ppi_np;
> +		ppi_np = gic_get_ppi_node(np, 0);
> +		of_node_put(ppi_np);
> +		if (!ppi_np)
> +			continue;
> +			
> +		/* Assume we use the full 16 PPIs */
> +		gic_of_init_one(i, 16, np);
> +		irq_start += gic_data[i].nr_irqs;
> +		i++;
> +		break;
> +	}
> +	of_node_put(np);
> +	np = NULL;
> +
> +	/* Now all the others */
> +	while((np = gic_get_node(np))) {
> +		struct device_node *ppi_np;
> +		int irq;
> +		ppi_np = gic_get_ppi_node(np, 0);
> +		of_node_put(ppi_np);
> +		if (ppi_np)
> +			continue;
> +			
> +		gic_of_init_one(i, irq_start, np);
> +		irq = irq_of_parse_and_map(np, 0);
> +		if (irq != IRQ_NONE) {
> +			pr_info("GIC %s cascading to IRQ%d",
> +				np->full_name, irq);
> +			gic_cascade_irq(i, irq);
> +		}
> +		irq_start += gic_data[i].nr_irqs;
> +		i++;
> +	}
> +	of_node_put(np);
> +}
> +#endif

I see what you're doing here, but I don't think it is the best
approach in the long run.  Rather than walking all the
interrupt-controller nodes looking for primary and secondary gics
while ignoring all others, I think there needs to be a function that
finds all the interrupt controller nodes, sorts them based on
dependencies, and calls their initialization hook in order.  Other
architectures will use such a thing.  I've been intending to do it for
powerpc for quite a while node.

Would you be willing to tackle such a beast?  I've got a fairly good
idea what it needs to look like, but I just haven't had the time to do
it.

> +

Overall the patch looks like it is in the right direction.

g.

>  void __cpuinit gic_secondary_init(unsigned int gic_nr)
>  {
>  	BUG_ON(gic_nr >= MAX_GIC_NR);
> diff --git a/arch/arm/include/asm/hardware/gic.h b/arch/arm/include/asm/hardware/gic.h
> index 069d4c0..04cfe33 100644
> --- a/arch/arm/include/asm/hardware/gic.h
> +++ b/arch/arm/include/asm/hardware/gic.h
> @@ -39,16 +39,20 @@ extern void __iomem *gic_cpu_base_addr;
>  extern struct irq_chip gic_arch_extn;
>  
>  void gic_init(unsigned int, unsigned int, void __iomem *, void __iomem *);
> +void gic_of_init(void);
>  void gic_secondary_init(unsigned int);
>  void gic_cascade_irq(unsigned int gic_nr, unsigned int irq);
>  void gic_raise_softirq(const struct cpumask *mask, unsigned int irq);
>  #ifdef CONFIG_ARM_GIC_VPPI
>  unsigned int gic_ppi_to_vppi(unsigned int irq);
> +unsigned int gic_ppi_to_cpu_vppi(unsigned int irq, int cpu);
>  #else
>  static inline unsigned int gic_ppi_to_vppi(unsigned int irq)
>  {
>  	return irq;
>  }
> +
> +#define gic_ppi_to_cpu_vppi(i,c) gic_ppi_to_vppi(i)
>  #endif
>  
>  #endif
> -- 
> 1.7.0.4
> 
> 

Re: [RFC PATCH 3/4] ARM: dt: add GIC bindings and driver support

[Marc Zyngier]

On 26/06/11 09:01, Grant Likely wrote:
> On Fri, Jun 24, 2011 at 03:10:58PM +0100, Marc Zyngier wrote:
>> Convert the GIC driver to use the DT infrastructure. That involves
>> introducing IRQ domains for the various types of interrupts (PPI and
>> SPI).
>>
>> The DT bindings for the GIC are defined as such:
>> - one interrupt-controller for SPIs
>> - one interrupt-controller for PPIs per CPU
>> - interrupt signalling for secondary GICs.
>>
>> Tested on RealView PB11MPCore and VExpress.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
> 
> Hi Mark,
> 
> Thanks for doing this work.  Comments below.
> 
>> ---
>>  Documentation/devicetree/bindings/arm/gic.txt |   92 ++++++++
>>  arch/arm/common/gic.c                         |  312 ++++++++++++++++++++-----
>>  arch/arm/include/asm/hardware/gic.h           |    4 +
>>  3 files changed, 352 insertions(+), 56 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/arm/gic.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
>> new file mode 100644
>> index 0000000..9975345
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/gic.txt
>> @@ -0,0 +1,92 @@
>> +* ARM Generic Interrupt Controller
>> +
>> +ARM SMP cores are often associated with a GIC, providing per processor
>> +interrupts (PPI), shared processor interrupts (SPI) and software
>> +generated interrupts (SGI).
>> +
>> +Primary GIC is attached directly to the CPU. Secondary GICs are
>> +cascaded into the upward interrupt controller.
>> +
>> +Main node properties:
>> +
>> +- compatible : "arm,gic"
> 
> I'm nervous about simply "arm,gic" with no specific silicon or core
> version number attached to it.  Specifications for standard cores such
> as this could very well change with newer versions of the core.  I'd
> like to see either the core version specified, or this value anchored
> on a specific chip implementation.

Some suggestions: "arm,gic-v1", "arm,gic-v2", "arm,gic-11mpcore",
"arm,gic-cortex-a9"...

>> +- reg                : one register mapping for the distributor interface
>> +               another one for the CPU interface
>> +- #address-cells: <1>
>> +- #size-cells        : <0>
>> +- interrupts : optionnal, used on secondary GICs only
> 
> sp.  optional

Ok.

>> +
>> +PPI sub nodes: one node per CPU (primary GIC only)
>> +
>> +- interrupt-controller       : required
>> +- #interrupt-cells   : <1>
>> +- reg                        : index of the CPU this PPI controller is
>> +                       connected to.
>> +
>> +SPI sub node:
>> +
>> +- interrupt-controller       : required
>> +- #interrupt-cells   : <2> First parameter is the interrupt number,
>> +                       second is 0 for level-high, 1 for edge-rising.
> 
> Is there any possibility of level-low or edge-falling?  Since irq flags
> are essentially arbitrary values chosen by the writer of the binding,
> I generally recommend that the best arbitrary values are the ones
> currently used by the kernel for IRQF_TRIGGER_*:
>         edge-rising: 1
>         edge-falling: 2
>         level-high: 4
>         level-low: 8

No, the GIC can only cope with level-high or edge-rising. I'll follow
your recommendation to directly the IRQF_* flag values.

>> +
>> +Primary GIC example (ARM RealView PB11-MPCore):
>> +
>> +gic@1f001000 {
>> +     compatible = "arm,gic";
>> +     #address-cells = <1>;
>> +     #size-cells = <0>;
>> +
>> +     reg = <0x1f001000 0x1000>, /* Distributor interface */
>> +           <0x1f000100 0x100>;  /* CPU interface */
>> +
>> +     /* One PPI controller per CPU, only on primary GIC */
>> +     gic0ppi0: gic-ppi@0 {
>> +             interrupt-controller;
>> +             #interrupt-cells = <1>;
>> +             reg = <0>;
>> +     };
>> +
>> +     gic0ppi1: gic-ppi@1 {
>> +             interrupt-controller;
>> +             #interrupt-cells = <1>;
>> +             reg = <1>;
>> +     };
>> +
>> +     gic0ppi2: gic-ppi@2 {
>> +             interrupt-controller;
>> +             #interrupt-cells = <1>;
>> +             reg = <2>;
>> +     };
>> +
>> +     gic0ppi3: gic-ppi@3 {
>> +             interrupt-controller;
>> +             #interrupt-cells = <1>;
>> +             reg = <3>;
>> +     };
> 
> I don't know much about the GIC PPIs, so it is hard to provide good
> feedback on this binding.  Are the PPI interrupts configurable in any
> way, or are certain PPI inputs hard wired to specific ppi
> controllers?  In other words, does this binding reflect how the device
> is hard wired, or is irq affinity a software decision?  If it is
> hardwired, then the above binding probably makes sense.  If it is a
> software configuration, then I would suggest dropping the per-ppi
> nodes and having a flat irq number space that only reflects the
> hardware attachments, and use a flags field in the irq specifier if
> the ppi affinity isn't something that the kernel can dynamically
> assign.

PPIs are not configurable at all. A device wired to a PPI line
interrupts one particular CPU, and this one only. Even the registers are
banked per-CPU. You can think of PPIs as private interrupts for one CPU.
They are usually used to implement hardware that is replicated on all
cores of the system (the timer being the typical example).

>> +
>> +     /* One global SPI controller per GIC, defined on all GICs */
>> +     gic0spi: gic-spi {
>> +             interrupt-controller;
>> +             #interrupt-cells = <2>; /* IRQ, level/edge */
>> +     };
>> +};
>> +
>> +Secondary GIC example (ARM RealView PB11-MPCore):
>> +
>> +gic@1e001000 {
>> +     compatible = "arm,gic";
>> +     #address-cells = <1>;
>> +     #size-cells = <0>;
>> +     interrupt-parent = <&gic0spi>;
>> +
>> +     reg = <0x1e001000 0x1000>, /* Distributor interface */
>> +           <0x1e000000 0x100>;  /* CPU interface */
>> +
>> +     interrupts = <42 0>; /* Cascade */
>> +
>> +     /* One global SPI controller per GIC, defined on all GICs */
>> +     gic1spi: gic-spi {
>> +             interrupt-controller;
>> +             #interrupt-cells = <2>;
>> +     };
>> +};
>> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
>> index c5acc76..34907ea 100644
>> --- a/arch/arm/common/gic.c
>> +++ b/arch/arm/common/gic.c
>> @@ -28,6 +28,9 @@
>>  #include <linux/smp.h>
>>  #include <linux/cpumask.h>
>>  #include <linux/io.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>>
>>  #include <asm/irq.h>
>>  #include <asm/mach/irq.h>
>> @@ -40,15 +43,15 @@ void __iomem *gic_cpu_base_addr __read_mostly;
>>
>>  struct gic_chip_data {
>>       unsigned int irq_offset;
>> +     unsigned int nr_irqs;
> 
> nr_irq probably needs to be part of struct irq_domain.  I should
> change that.

That would definitely make sense.

>>       void __iomem *dist_base;
>>       void __iomem *cpu_base;
>> +     struct irq_domain spi_dom;
>> +};
>> +
>>  #ifdef CONFIG_ARM_GIC_VPPI
>> -     /* These fields must be 0 on secondary GICs */
>> -     int          ppi_base;
>> -     int          vppi_base;
>> -     u16          nrppis;
>> +static DEFINE_PER_CPU(struct irq_domain, gic_ppi_domains);
>>  #endif
>> -};
>>
>>  /*
>>   * Supported arch specific GIC irq extension.
>> @@ -271,36 +274,26 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
>>  #ifdef CONFIG_ARM_GIC_VPPI
>>  unsigned int gic_ppi_to_vppi(unsigned int irq)
>>  {
>> -     struct gic_chip_data *chip_data = irq_get_chip_data(irq);
>> -     unsigned int vppi_irq;
>> -     unsigned int ppi;
>> +     struct irq_domain *dom = &__get_cpu_var(gic_ppi_domains);
>>
>> -     WARN_ON(!chip_data->vppi_base);
>> +     return irq_domain_to_irq(dom, irq);
>> +}
>>
>> -     ppi = irq - chip_data->ppi_base;
>> -     vppi_irq = ppi + chip_data->nrppis * smp_processor_id();
>> -     vppi_irq += chip_data->vppi_base;
>> +unsigned int gic_ppi_to_cpu_vppi(unsigned int irq, int cpu)
>> +{
>> +     struct irq_domain *dom = &per_cpu(gic_ppi_domains, cpu);
>>
>> -     return vppi_irq;
>> +     return irq_domain_to_irq(dom, irq);
>>  }
>>
>>  static void gic_handle_ppi(unsigned int irq, struct irq_desc *desc)
>>  {
>> -     unsigned int vppi_irq;
>> -
>> -     vppi_irq = gic_ppi_to_vppi(irq);
>> -     generic_handle_irq(vppi_irq);
>> +     generic_handle_irq(gic_ppi_to_vppi(irq));
>>  }
>>
>>  static struct irq_data *gic_vppi_to_ppi(struct irq_data *d)
>>  {
>> -     struct gic_chip_data *chip_data = irq_data_get_irq_chip_data(d);
>> -     unsigned int ppi_irq;
>> -
>> -     ppi_irq = d->irq - chip_data->vppi_base - chip_data->nrppis * smp_processor_id();
>> -     ppi_irq += chip_data->ppi_base;
>> -
>> -     return irq_get_irq_data(ppi_irq);
>> +     return irq_get_irq_data(d->irq - d->domain->irq_base);
>>  }
>>
>>  static void gic_ppi_eoi_irq(struct irq_data *d)
>> @@ -345,15 +338,115 @@ static struct irq_chip gic_ppi_chip = {
>>       .irq_set_type           = gic_ppi_set_type,
>>       .irq_set_wake           = gic_ppi_set_wake,
>>  };
>> +
>> +#ifdef CONFIG_OF
>> +static int git_dt_translate(struct irq_domain *dom,
>> +                         struct device_node *controler,
>> +                         const u32 *intspec, unsigned int intsize,
>> +                         irq_hw_number_t *out_hwirq, unsigned int *out_type)
>> +{
>> +     if (dom->of_node != controler)
>> +             return -1;
> 
> -EINVAL

Ok.

>> +
>> +     if (intsize > 1) {
>> +             if (intspec[1])
>> +                     *out_type = IRQ_TYPE_EDGE_RISING;
> 
> nit: inconsistent whitespace.
> 
>> +             else
>> +                     *out_type = IRQ_TYPE_LEVEL_HIGH;
>> +     } else
>> +             *out_type = IRQ_TYPE_NONE;
> 
> I would do something like (assuming you take my suggestion above about
> arbitrary values):
> 
>         if (intsize < 2 || intspec[0] > (max_gic_hwirq_number))

max_gic_hwirq_number could be easily replaced by the above nr_irqs in
the irq_domain structure).

>                 return -EINVAL;
>         /* Binding uses save values as IRQF_TRIGGER_* macros */
>         *out_type = intspec[1] & IRQ_TRIGGER_MASK;
>         *out_hwirq = intspec[0];
>         return 0;

Looks good to me.

>> +
>> +static struct irq_domain_ops gic_ppi_domain_ops = {
>> +     .dt_translate = git_dt_translate,
>> +};
>> +
>> +static struct of_device_id gic_ids[] = {
>> +     { .compatible = "arm,gic", },
>> +     { },
>> +};
>> +
>> +static struct device_node *gic_get_node(struct device_node *np)
>> +{
>> +     return of_find_matching_node(np, gic_ids);
>> +}
>> +
>> +static struct device_node *gic_get_ppi_node(struct device_node *gic_np,
>> +                                         int cpu_nr)
>> +{
>> +     struct device_node *np = NULL;
>> +
>> +     if (!gic_np)
>> +             return NULL;
>> +
>> +     while ((np = of_get_next_child(gic_np, np))) {
> 
> for_each_child_of_node()

Ok.

>> +             const __be32 *reg;
>> +
>> +             if (strcmp(np->name, "gic-ppi"))
>> +                     continue;
> 
> Generally, its a good idea to differentiate what a node represents by
> compatible property, not name.  This would be a slight change to your
> binding, but I think it would be more consistent overall.

Happy to do so. I was wondering if sub-nodes were meant to have a
compatible property or not...

>> +             reg = of_get_property(np, "reg", NULL);
>> +             if (!reg)       /* Duh? */
>> +                     continue;
>> +             if (be32_to_cpu(*reg) == cpu_nr)
>> +                     break;
>> +     }
>> +
>> +     return np;
>> +}
>> +
>> +static struct device_node *gic_get_spi_node(struct device_node *gic_np)
>> +{
>> +     struct device_node *np = NULL;
>> +
>> +     if (!gic_np)
>> +             return NULL;
>> +
>> +     while ((np = of_get_next_child(gic_np, np))) {
> 
> for_each_child_of_node()
> 
>> +             if (!strcmp(np->name, "gic-spi"))
>> +                     break;
>> +     }
>> +
>> +     return np;
>> +}
>> +#else
> 
> /* CONFIG_BLAH */ comments are generally appreciated on #else/#endif
> statements.

Ok.

>> +static struct irq_domain_ops gic_ppi_domain_ops; /* Use the defaults */
>> +
>> +static struct device_node *gic_get_node(struct device_node *np)
>> +{
>> +     return NULL;
>> +}
>> +
>> +static struct device_node *gic_get_ppi_node(struct device_node *gic_np,
>> +                                         int cpu_nr)
>> +{
>> +     return NULL;
>> +}
>> +
>> +static struct device_node *gic_get_spi_node(struct device_node *gic_np)
>> +{
>> +     return NULL;
>> +}
>> +#endif
>>  #endif
>>
>> +static const char *get_of_name(struct device_node *np)
>> +{
>> +     return np ? np->full_name : "";
>> +}
>> +
>>  static void __init gic_dist_init(struct gic_chip_data *gic,
>> -     unsigned int irq_start)
>> +                              unsigned int irq_start,
>> +                              struct device_node *gic_node)
>>  {
>> -     unsigned int gic_irqs, irq_limit, i, nrvppis = 0;
>> +     unsigned int gic_irqs, irq_limit, i, c;
>>       void __iomem *base = gic->dist_base;
>>       u32 cpumask = 1 << smp_processor_id();
>> -     u32 dist_ctr, nrcpus;
>> +     u32 dist_ctr, nrcpus, reg;
>> +     u32 ppi_base = 0;
>> +     u16 nrppis = 0;
>>
>>       cpumask |= cpumask << 8;
>>       cpumask |= cpumask << 16;
>> @@ -372,29 +465,26 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
>>       /* Find out how many CPUs are supported (8 max). */
>>       nrcpus = ((dist_ctr >> 5) & 7) + 1;
>>
>> -#ifdef CONFIG_ARM_GIC_VPPI
>>       /*
>>        * Nobody would be insane enough to use PPIs on a secondary
>>        * GIC, right?
>>        */
>>       if (gic == &gic_data[0]) {
>> -             gic->nrppis = 16 - (irq_start % 16);
>> -             gic->ppi_base = gic->irq_offset + 32 - gic->nrppis;
>> -             nrvppis = gic->nrppis * nrcpus;
>> -     } else {
>> -             gic->ppi_base = 0;
>> -             gic->vppi_base = 0;
>> +             ppi_base = irq_start & 31;
>> +             nrppis = 16 - (ppi_base & 15);
>>       }
>> -#endif
>>
>>       pr_info("Configuring GIC with %d sources (%d additional PPIs)\n",
>> -             gic_irqs, nrvppis);
>> +             gic_irqs, nrppis * nrcpus);
>>
>>       /*
>> -      * Set all global interrupts to be level triggered, active low.
>> +      * Set all global interrupts to be level triggered, active high.
>>        */
>> -     for (i = 32; i < gic_irqs; i += 16)
>> -             writel_relaxed(0, base + GIC_DIST_CONFIG + i * 4 / 16);
>> +     for (i = 32; i < gic_irqs; i += 16) {
>> +             reg = readl_relaxed(base + GIC_DIST_CONFIG + i * 4 / 16);
>> +             reg &= 0x55555555;
>> +             writel_relaxed(reg, base + GIC_DIST_CONFIG + i * 4 / 16);
>> +     }
> 
> Have you got some unrelated fixups in this patch?  It isn't obvious to
> me whether or not this hunk is related to setting up irq_domains.

Erm... This should have never made it here, looks like I squashed two
separate patches... Will fix.

>>
>>       /*
>>        * Set all global interrupts to this CPU only.
>> @@ -425,30 +515,69 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
>>       /*
>>        * Setup the Linux IRQ subsystem.
>>        */
>> -     for (i = irq_start; i < irq_limit; i++) {
>> +
>> +     gic->nr_irqs = gic_irqs;
>> +     gic->spi_dom.irq_base = irq_start & ~31;
>> +     gic->spi_dom.ops = &gic_ppi_domain_ops;
>> +     gic->spi_dom.of_node = gic_get_spi_node(gic_node);
>> +
>> +     pr_info("GIC SPI domain %s [%d:%d]\n",
>> +             get_of_name(gic->spi_dom.of_node), irq_start, irq_limit - 1);
>> +
>> +     irq_domain_add(&gic->spi_dom);
>> +
>> +     for (i = ppi_base; i < (irq_limit - gic->spi_dom.irq_base); i++) {
>> +             unsigned int irq = irq_domain_map(&gic->spi_dom, i);
>> +             pr_debug("GIC mapping %s:%d to IRQ%d\n",
>> +                      get_of_name(gic->spi_dom.of_node), i, irq);
>>  #ifdef CONFIG_ARM_GIC_VPPI
>> -             if (nrvppis && gic_irq_is_ppi(gic, i))
>> -                     irq_set_chip_and_handler(i, &gic_chip, gic_handle_ppi);
>> +             if (nrppis && gic_irq_is_ppi(gic, irq))
>> +                     irq_set_chip_and_handler(irq, &gic_chip, gic_handle_ppi);
>>               else
>>  #endif
>>               {
>> -                     irq_set_chip_and_handler(i, &gic_chip,
>> +                     irq_set_chip_and_handler(irq, &gic_chip,
>>                                                handle_fasteoi_irq);
>> -                     set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
>> +                     set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
>>               }
>> -             irq_set_chip_data(i, gic);
>> +             irq_set_chip_data(irq, gic);
>>       }
>>
>>  #ifdef CONFIG_ARM_GIC_VPPI
>> -     if (!nrvppis)
>> +     if (!nrppis)
>>               goto out;
>> -     gic->vppi_base = irq_alloc_descs(-1, 0, nrvppis, 0);
>> -     if (WARN_ON(gic->vppi_base < 0))
>> -             goto out;
>> -     for (i = gic->vppi_base; i < (gic->vppi_base + nrvppis); i++) {
>> -             irq_set_chip_and_handler(i, &gic_ppi_chip, handle_percpu_irq);
>> -             irq_set_chip_data(i, gic);
>> -             set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
>> +     for (c = 0; c < nrcpus; c++) {
>> +             struct irq_domain *dom = &per_cpu(gic_ppi_domains, c);
>> +             int base;
>> +
>> +             base = irq_alloc_descs(-1, 0, nrppis, 0);
>> +             if (WARN_ON(base < 0))
>> +                     goto out;
>> +
>> +             /*
>> +              * We cheat here, and fold ppi_base into irq_base, as
>> +              * it saves us some work at runtime.
>> +              */
>> +             dom->irq_base = base - ppi_base;
>> +             dom->ops = &gic_ppi_domain_ops;
>> +             dom->of_node = gic_get_ppi_node(gic_node, c);
>> +
>> +             pr_info("GIC PPI domain %s [%d:%d] for CPU %d\n",
>> +                     get_of_name(dom->of_node), base, base + nrppis - 1, c);
>> +
>> +             irq_domain_add(dom);
>> +
>> +             for (i = 0; i < nrppis; i++) {
>> +                     unsigned int irq = irq_domain_map(dom, i + irq_start);
>> +
>> +                     pr_debug("GIC mapping %s:%d to IRQ%d\n",
>> +                              get_of_name(dom->of_node), i, irq);
>> +
>> +                     irq_set_chip_and_handler(irq, &gic_ppi_chip,
>> +                                              handle_percpu_irq);
>> +                     irq_set_chip_data(irq, gic);
>> +                     set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
>> +             }
>>       }
>>  out:
>>  #endif
>> @@ -479,8 +608,9 @@ static void __cpuinit gic_cpu_init(struct gic_chip_data *gic)
>>       writel_relaxed(1, base + GIC_CPU_CTRL);
>>  }
>>
>> -void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
>> -     void __iomem *dist_base, void __iomem *cpu_base)
>> +static void __init gic_init_one(unsigned int gic_nr, unsigned int irq_start,
>> +                             void __iomem *dist_base, void __iomem *cpu_base,
>> +                             struct device_node *gic_node)
>>  {
>>       struct gic_chip_data *gic;
>>
>> @@ -494,10 +624,80 @@ void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
>>       if (gic_nr == 0)
>>               gic_cpu_base_addr = cpu_base;
>>
>> -     gic_dist_init(gic, irq_start);
>> +     gic_dist_init(gic, irq_start, gic_node);
>>       gic_cpu_init(gic);
>>  }
>>
>> +void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
>> +                  void __iomem *dist_base, void __iomem *cpu_base)
>> +{
>> +     gic_init_one(gic_nr, irq_start, dist_base, cpu_base, NULL);
>> +}
>> +
>> +#ifdef CONFIG_OF
>> +static void __init gic_of_init_one(int idx, unsigned int irq_start,
>> +                                struct device_node *np)
>> +{
>> +     void __iomem *dist_base;
>> +     void __iomem *cpu_base;
>> +
>> +     dist_base = of_iomap(np, 0);
>> +     cpu_base  = of_iomap(np, 1);
>> +     if (!dist_base || ! cpu_base) {
>> +             pr_err("Cannot map %s\n", np->full_name);
>> +             return;
>> +     }
>> +
>> +     /* Assume we use the full 16 PPIs */
>> +     gic_init_one(idx, irq_start, dist_base, cpu_base, np);
>> +}
>> +
>> +void __init gic_of_init(void)
>> +{
>> +     struct device_node *np = NULL;
>> +     unsigned int irq_start = 0;
>> +     int i = 0;
>> +
>> +     /* Look for the GIC providing PPIs first */
>> +     while((np = gic_get_node(np))) {
>> +             struct device_node *ppi_np;
>> +             ppi_np = gic_get_ppi_node(np, 0);
>> +             of_node_put(ppi_np);
>> +             if (!ppi_np)
>> +                     continue;
>> +
>> +             /* Assume we use the full 16 PPIs */
>> +             gic_of_init_one(i, 16, np);
>> +             irq_start += gic_data[i].nr_irqs;
>> +             i++;
>> +             break;
>> +     }
>> +     of_node_put(np);
>> +     np = NULL;
>> +
>> +     /* Now all the others */
>> +     while((np = gic_get_node(np))) {
>> +             struct device_node *ppi_np;
>> +             int irq;
>> +             ppi_np = gic_get_ppi_node(np, 0);
>> +             of_node_put(ppi_np);
>> +             if (ppi_np)
>> +                     continue;
>> +
>> +             gic_of_init_one(i, irq_start, np);
>> +             irq = irq_of_parse_and_map(np, 0);
>> +             if (irq != IRQ_NONE) {
>> +                     pr_info("GIC %s cascading to IRQ%d",
>> +                             np->full_name, irq);
>> +                     gic_cascade_irq(i, irq);
>> +             }
>> +             irq_start += gic_data[i].nr_irqs;
>> +             i++;
>> +     }
>> +     of_node_put(np);
>> +}
>> +#endif
> 
> I see what you're doing here, but I don't think it is the best
> approach in the long run.  Rather than walking all the
> interrupt-controller nodes looking for primary and secondary gics
> while ignoring all others, I think there needs to be a function that
> finds all the interrupt controller nodes, sorts them based on
> dependencies, and calls their initialization hook in order.  Other
> architectures will use such a thing.  I've been intending to do it for
> powerpc for quite a while node.
> 
> Would you be willing to tackle such a beast?  I've got a fairly good
> idea what it needs to look like, but I just haven't had the time to do
> it.

I can give it a go.

> Overall the patch looks like it is in the right direction.

Thanks, comments much appreciated. I'll respin the patch series after
adressing the other comments and scratching my head a bit more on the
early device crap^H^H^H^Hproblem.

Cheers,

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

[RFC PATCH 3/4] ARM: dt: add GIC bindings and driver support

[Marc Zyngier]

On 26/06/11 09:01, Grant Likely wrote:
> On Fri, Jun 24, 2011 at 03:10:58PM +0100, Marc Zyngier wrote:
>> Convert the GIC driver to use the DT infrastructure. That involves
>> introducing IRQ domains for the various types of interrupts (PPI and
>> SPI).
>>
>> The DT bindings for the GIC are defined as such:
>> - one interrupt-controller for SPIs
>> - one interrupt-controller for PPIs per CPU
>> - interrupt signalling for secondary GICs.
>>
>> Tested on RealView PB11MPCore and VExpress.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> Hi Mark,
> 
> Thanks for doing this work.  Comments below.
> 
>> ---
>>  Documentation/devicetree/bindings/arm/gic.txt |   92 ++++++++
>>  arch/arm/common/gic.c                         |  312 ++++++++++++++++++++-----
>>  arch/arm/include/asm/hardware/gic.h           |    4 +
>>  3 files changed, 352 insertions(+), 56 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/arm/gic.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
>> new file mode 100644
>> index 0000000..9975345
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/gic.txt
>> @@ -0,0 +1,92 @@
>> +* ARM Generic Interrupt Controller
>> +
>> +ARM SMP cores are often associated with a GIC, providing per processor
>> +interrupts (PPI), shared processor interrupts (SPI) and software
>> +generated interrupts (SGI).
>> +
>> +Primary GIC is attached directly to the CPU. Secondary GICs are
>> +cascaded into the upward interrupt controller.
>> +
>> +Main node properties:
>> +
>> +- compatible : "arm,gic"
> 
> I'm nervous about simply "arm,gic" with no specific silicon or core
> version number attached to it.  Specifications for standard cores such
> as this could very well change with newer versions of the core.  I'd
> like to see either the core version specified, or this value anchored
> on a specific chip implementation.

Some suggestions: "arm,gic-v1", "arm,gic-v2", "arm,gic-11mpcore",
"arm,gic-cortex-a9"...

>> +- reg                : one register mapping for the distributor interface
>> +               another one for the CPU interface
>> +- #address-cells: <1>
>> +- #size-cells        : <0>
>> +- interrupts : optionnal, used on secondary GICs only
> 
> sp.  optional

Ok.

>> +
>> +PPI sub nodes: one node per CPU (primary GIC only)
>> +
>> +- interrupt-controller       : required
>> +- #interrupt-cells   : <1>
>> +- reg                        : index of the CPU this PPI controller is
>> +                       connected to.
>> +
>> +SPI sub node:
>> +
>> +- interrupt-controller       : required
>> +- #interrupt-cells   : <2> First parameter is the interrupt number,
>> +                       second is 0 for level-high, 1 for edge-rising.
> 
> Is there any possibility of level-low or edge-falling?  Since irq flags
> are essentially arbitrary values chosen by the writer of the binding,
> I generally recommend that the best arbitrary values are the ones
> currently used by the kernel for IRQF_TRIGGER_*:
>         edge-rising: 1
>         edge-falling: 2
>         level-high: 4
>         level-low: 8

No, the GIC can only cope with level-high or edge-rising. I'll follow
your recommendation to directly the IRQF_* flag values.

>> +
>> +Primary GIC example (ARM RealView PB11-MPCore):
>> +
>> +gic at 1f001000 {
>> +     compatible = "arm,gic";
>> +     #address-cells = <1>;
>> +     #size-cells = <0>;
>> +
>> +     reg = <0x1f001000 0x1000>, /* Distributor interface */
>> +           <0x1f000100 0x100>;  /* CPU interface */
>> +
>> +     /* One PPI controller per CPU, only on primary GIC */
>> +     gic0ppi0: gic-ppi at 0 {
>> +             interrupt-controller;
>> +             #interrupt-cells = <1>;
>> +             reg = <0>;
>> +     };
>> +
>> +     gic0ppi1: gic-ppi at 1 {
>> +             interrupt-controller;
>> +             #interrupt-cells = <1>;
>> +             reg = <1>;
>> +     };
>> +
>> +     gic0ppi2: gic-ppi at 2 {
>> +             interrupt-controller;
>> +             #interrupt-cells = <1>;
>> +             reg = <2>;
>> +     };
>> +
>> +     gic0ppi3: gic-ppi at 3 {
>> +             interrupt-controller;
>> +             #interrupt-cells = <1>;
>> +             reg = <3>;
>> +     };
> 
> I don't know much about the GIC PPIs, so it is hard to provide good
> feedback on this binding.  Are the PPI interrupts configurable in any
> way, or are certain PPI inputs hard wired to specific ppi
> controllers?  In other words, does this binding reflect how the device
> is hard wired, or is irq affinity a software decision?  If it is
> hardwired, then the above binding probably makes sense.  If it is a
> software configuration, then I would suggest dropping the per-ppi
> nodes and having a flat irq number space that only reflects the
> hardware attachments, and use a flags field in the irq specifier if
> the ppi affinity isn't something that the kernel can dynamically
> assign.

PPIs are not configurable at all. A device wired to a PPI line
interrupts one particular CPU, and this one only. Even the registers are
banked per-CPU. You can think of PPIs as private interrupts for one CPU.
They are usually used to implement hardware that is replicated on all
cores of the system (the timer being the typical example).

>> +
>> +     /* One global SPI controller per GIC, defined on all GICs */
>> +     gic0spi: gic-spi {
>> +             interrupt-controller;
>> +             #interrupt-cells = <2>; /* IRQ, level/edge */
>> +     };
>> +};
>> +
>> +Secondary GIC example (ARM RealView PB11-MPCore):
>> +
>> +gic at 1e001000 {
>> +     compatible = "arm,gic";
>> +     #address-cells = <1>;
>> +     #size-cells = <0>;
>> +     interrupt-parent = <&gic0spi>;
>> +
>> +     reg = <0x1e001000 0x1000>, /* Distributor interface */
>> +           <0x1e000000 0x100>;  /* CPU interface */
>> +
>> +     interrupts = <42 0>; /* Cascade */
>> +
>> +     /* One global SPI controller per GIC, defined on all GICs */
>> +     gic1spi: gic-spi {
>> +             interrupt-controller;
>> +             #interrupt-cells = <2>;
>> +     };
>> +};
>> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
>> index c5acc76..34907ea 100644
>> --- a/arch/arm/common/gic.c
>> +++ b/arch/arm/common/gic.c
>> @@ -28,6 +28,9 @@
>>  #include <linux/smp.h>
>>  #include <linux/cpumask.h>
>>  #include <linux/io.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>>
>>  #include <asm/irq.h>
>>  #include <asm/mach/irq.h>
>> @@ -40,15 +43,15 @@ void __iomem *gic_cpu_base_addr __read_mostly;
>>
>>  struct gic_chip_data {
>>       unsigned int irq_offset;
>> +     unsigned int nr_irqs;
> 
> nr_irq probably needs to be part of struct irq_domain.  I should
> change that.

That would definitely make sense.

>>       void __iomem *dist_base;
>>       void __iomem *cpu_base;
>> +     struct irq_domain spi_dom;
>> +};
>> +
>>  #ifdef CONFIG_ARM_GIC_VPPI
>> -     /* These fields must be 0 on secondary GICs */
>> -     int          ppi_base;
>> -     int          vppi_base;
>> -     u16          nrppis;
>> +static DEFINE_PER_CPU(struct irq_domain, gic_ppi_domains);
>>  #endif
>> -};
>>
>>  /*
>>   * Supported arch specific GIC irq extension.
>> @@ -271,36 +274,26 @@ void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
>>  #ifdef CONFIG_ARM_GIC_VPPI
>>  unsigned int gic_ppi_to_vppi(unsigned int irq)
>>  {
>> -     struct gic_chip_data *chip_data = irq_get_chip_data(irq);
>> -     unsigned int vppi_irq;
>> -     unsigned int ppi;
>> +     struct irq_domain *dom = &__get_cpu_var(gic_ppi_domains);
>>
>> -     WARN_ON(!chip_data->vppi_base);
>> +     return irq_domain_to_irq(dom, irq);
>> +}
>>
>> -     ppi = irq - chip_data->ppi_base;
>> -     vppi_irq = ppi + chip_data->nrppis * smp_processor_id();
>> -     vppi_irq += chip_data->vppi_base;
>> +unsigned int gic_ppi_to_cpu_vppi(unsigned int irq, int cpu)
>> +{
>> +     struct irq_domain *dom = &per_cpu(gic_ppi_domains, cpu);
>>
>> -     return vppi_irq;
>> +     return irq_domain_to_irq(dom, irq);
>>  }
>>
>>  static void gic_handle_ppi(unsigned int irq, struct irq_desc *desc)
>>  {
>> -     unsigned int vppi_irq;
>> -
>> -     vppi_irq = gic_ppi_to_vppi(irq);
>> -     generic_handle_irq(vppi_irq);
>> +     generic_handle_irq(gic_ppi_to_vppi(irq));
>>  }
>>
>>  static struct irq_data *gic_vppi_to_ppi(struct irq_data *d)
>>  {
>> -     struct gic_chip_data *chip_data = irq_data_get_irq_chip_data(d);
>> -     unsigned int ppi_irq;
>> -
>> -     ppi_irq = d->irq - chip_data->vppi_base - chip_data->nrppis * smp_processor_id();
>> -     ppi_irq += chip_data->ppi_base;
>> -
>> -     return irq_get_irq_data(ppi_irq);
>> +     return irq_get_irq_data(d->irq - d->domain->irq_base);
>>  }
>>
>>  static void gic_ppi_eoi_irq(struct irq_data *d)
>> @@ -345,15 +338,115 @@ static struct irq_chip gic_ppi_chip = {
>>       .irq_set_type           = gic_ppi_set_type,
>>       .irq_set_wake           = gic_ppi_set_wake,
>>  };
>> +
>> +#ifdef CONFIG_OF
>> +static int git_dt_translate(struct irq_domain *dom,
>> +                         struct device_node *controler,
>> +                         const u32 *intspec, unsigned int intsize,
>> +                         irq_hw_number_t *out_hwirq, unsigned int *out_type)
>> +{
>> +     if (dom->of_node != controler)
>> +             return -1;
> 
> -EINVAL

Ok.

>> +
>> +     if (intsize > 1) {
>> +             if (intspec[1])
>> +                     *out_type = IRQ_TYPE_EDGE_RISING;
> 
> nit: inconsistent whitespace.
> 
>> +             else
>> +                     *out_type = IRQ_TYPE_LEVEL_HIGH;
>> +     } else
>> +             *out_type = IRQ_TYPE_NONE;
> 
> I would do something like (assuming you take my suggestion above about
> arbitrary values):
> 
>         if (intsize < 2 || intspec[0] > (max_gic_hwirq_number))

max_gic_hwirq_number could be easily replaced by the above nr_irqs in
the irq_domain structure).

>                 return -EINVAL;
>         /* Binding uses save values as IRQF_TRIGGER_* macros */
>         *out_type = intspec[1] & IRQ_TRIGGER_MASK;
>         *out_hwirq = intspec[0];
>         return 0;

Looks good to me.

>> +
>> +static struct irq_domain_ops gic_ppi_domain_ops = {
>> +     .dt_translate = git_dt_translate,
>> +};
>> +
>> +static struct of_device_id gic_ids[] = {
>> +     { .compatible = "arm,gic", },
>> +     { },
>> +};
>> +
>> +static struct device_node *gic_get_node(struct device_node *np)
>> +{
>> +     return of_find_matching_node(np, gic_ids);
>> +}
>> +
>> +static struct device_node *gic_get_ppi_node(struct device_node *gic_np,
>> +                                         int cpu_nr)
>> +{
>> +     struct device_node *np = NULL;
>> +
>> +     if (!gic_np)
>> +             return NULL;
>> +
>> +     while ((np = of_get_next_child(gic_np, np))) {
> 
> for_each_child_of_node()

Ok.

>> +             const __be32 *reg;
>> +
>> +             if (strcmp(np->name, "gic-ppi"))
>> +                     continue;
> 
> Generally, its a good idea to differentiate what a node represents by
> compatible property, not name.  This would be a slight change to your
> binding, but I think it would be more consistent overall.

Happy to do so. I was wondering if sub-nodes were meant to have a
compatible property or not...

>> +             reg = of_get_property(np, "reg", NULL);
>> +             if (!reg)       /* Duh? */
>> +                     continue;
>> +             if (be32_to_cpu(*reg) == cpu_nr)
>> +                     break;
>> +     }
>> +
>> +     return np;
>> +}
>> +
>> +static struct device_node *gic_get_spi_node(struct device_node *gic_np)
>> +{
>> +     struct device_node *np = NULL;
>> +
>> +     if (!gic_np)
>> +             return NULL;
>> +
>> +     while ((np = of_get_next_child(gic_np, np))) {
> 
> for_each_child_of_node()
> 
>> +             if (!strcmp(np->name, "gic-spi"))
>> +                     break;
>> +     }
>> +
>> +     return np;
>> +}
>> +#else
> 
> /* CONFIG_BLAH */ comments are generally appreciated on #else/#endif
> statements.

Ok.

>> +static struct irq_domain_ops gic_ppi_domain_ops; /* Use the defaults */
>> +
>> +static struct device_node *gic_get_node(struct device_node *np)
>> +{
>> +     return NULL;
>> +}
>> +
>> +static struct device_node *gic_get_ppi_node(struct device_node *gic_np,
>> +                                         int cpu_nr)
>> +{
>> +     return NULL;
>> +}
>> +
>> +static struct device_node *gic_get_spi_node(struct device_node *gic_np)
>> +{
>> +     return NULL;
>> +}
>> +#endif
>>  #endif
>>
>> +static const char *get_of_name(struct device_node *np)
>> +{
>> +     return np ? np->full_name : "";
>> +}
>> +
>>  static void __init gic_dist_init(struct gic_chip_data *gic,
>> -     unsigned int irq_start)
>> +                              unsigned int irq_start,
>> +                              struct device_node *gic_node)
>>  {
>> -     unsigned int gic_irqs, irq_limit, i, nrvppis = 0;
>> +     unsigned int gic_irqs, irq_limit, i, c;
>>       void __iomem *base = gic->dist_base;
>>       u32 cpumask = 1 << smp_processor_id();
>> -     u32 dist_ctr, nrcpus;
>> +     u32 dist_ctr, nrcpus, reg;
>> +     u32 ppi_base = 0;
>> +     u16 nrppis = 0;
>>
>>       cpumask |= cpumask << 8;
>>       cpumask |= cpumask << 16;
>> @@ -372,29 +465,26 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
>>       /* Find out how many CPUs are supported (8 max). */
>>       nrcpus = ((dist_ctr >> 5) & 7) + 1;
>>
>> -#ifdef CONFIG_ARM_GIC_VPPI
>>       /*
>>        * Nobody would be insane enough to use PPIs on a secondary
>>        * GIC, right?
>>        */
>>       if (gic == &gic_data[0]) {
>> -             gic->nrppis = 16 - (irq_start % 16);
>> -             gic->ppi_base = gic->irq_offset + 32 - gic->nrppis;
>> -             nrvppis = gic->nrppis * nrcpus;
>> -     } else {
>> -             gic->ppi_base = 0;
>> -             gic->vppi_base = 0;
>> +             ppi_base = irq_start & 31;
>> +             nrppis = 16 - (ppi_base & 15);
>>       }
>> -#endif
>>
>>       pr_info("Configuring GIC with %d sources (%d additional PPIs)\n",
>> -             gic_irqs, nrvppis);
>> +             gic_irqs, nrppis * nrcpus);
>>
>>       /*
>> -      * Set all global interrupts to be level triggered, active low.
>> +      * Set all global interrupts to be level triggered, active high.
>>        */
>> -     for (i = 32; i < gic_irqs; i += 16)
>> -             writel_relaxed(0, base + GIC_DIST_CONFIG + i * 4 / 16);
>> +     for (i = 32; i < gic_irqs; i += 16) {
>> +             reg = readl_relaxed(base + GIC_DIST_CONFIG + i * 4 / 16);
>> +             reg &= 0x55555555;
>> +             writel_relaxed(reg, base + GIC_DIST_CONFIG + i * 4 / 16);
>> +     }
> 
> Have you got some unrelated fixups in this patch?  It isn't obvious to
> me whether or not this hunk is related to setting up irq_domains.

Erm... This should have never made it here, looks like I squashed two
separate patches... Will fix.

>>
>>       /*
>>        * Set all global interrupts to this CPU only.
>> @@ -425,30 +515,69 @@ static void __init gic_dist_init(struct gic_chip_data *gic,
>>       /*
>>        * Setup the Linux IRQ subsystem.
>>        */
>> -     for (i = irq_start; i < irq_limit; i++) {
>> +
>> +     gic->nr_irqs = gic_irqs;
>> +     gic->spi_dom.irq_base = irq_start & ~31;
>> +     gic->spi_dom.ops = &gic_ppi_domain_ops;
>> +     gic->spi_dom.of_node = gic_get_spi_node(gic_node);
>> +
>> +     pr_info("GIC SPI domain %s [%d:%d]\n",
>> +             get_of_name(gic->spi_dom.of_node), irq_start, irq_limit - 1);
>> +
>> +     irq_domain_add(&gic->spi_dom);
>> +
>> +     for (i = ppi_base; i < (irq_limit - gic->spi_dom.irq_base); i++) {
>> +             unsigned int irq = irq_domain_map(&gic->spi_dom, i);
>> +             pr_debug("GIC mapping %s:%d to IRQ%d\n",
>> +                      get_of_name(gic->spi_dom.of_node), i, irq);
>>  #ifdef CONFIG_ARM_GIC_VPPI
>> -             if (nrvppis && gic_irq_is_ppi(gic, i))
>> -                     irq_set_chip_and_handler(i, &gic_chip, gic_handle_ppi);
>> +             if (nrppis && gic_irq_is_ppi(gic, irq))
>> +                     irq_set_chip_and_handler(irq, &gic_chip, gic_handle_ppi);
>>               else
>>  #endif
>>               {
>> -                     irq_set_chip_and_handler(i, &gic_chip,
>> +                     irq_set_chip_and_handler(irq, &gic_chip,
>>                                                handle_fasteoi_irq);
>> -                     set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
>> +                     set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
>>               }
>> -             irq_set_chip_data(i, gic);
>> +             irq_set_chip_data(irq, gic);
>>       }
>>
>>  #ifdef CONFIG_ARM_GIC_VPPI
>> -     if (!nrvppis)
>> +     if (!nrppis)
>>               goto out;
>> -     gic->vppi_base = irq_alloc_descs(-1, 0, nrvppis, 0);
>> -     if (WARN_ON(gic->vppi_base < 0))
>> -             goto out;
>> -     for (i = gic->vppi_base; i < (gic->vppi_base + nrvppis); i++) {
>> -             irq_set_chip_and_handler(i, &gic_ppi_chip, handle_percpu_irq);
>> -             irq_set_chip_data(i, gic);
>> -             set_irq_flags(i, IRQF_VALID | IRQF_PROBE);
>> +     for (c = 0; c < nrcpus; c++) {
>> +             struct irq_domain *dom = &per_cpu(gic_ppi_domains, c);
>> +             int base;
>> +
>> +             base = irq_alloc_descs(-1, 0, nrppis, 0);
>> +             if (WARN_ON(base < 0))
>> +                     goto out;
>> +
>> +             /*
>> +              * We cheat here, and fold ppi_base into irq_base, as
>> +              * it saves us some work at runtime.
>> +              */
>> +             dom->irq_base = base - ppi_base;
>> +             dom->ops = &gic_ppi_domain_ops;
>> +             dom->of_node = gic_get_ppi_node(gic_node, c);
>> +
>> +             pr_info("GIC PPI domain %s [%d:%d] for CPU %d\n",
>> +                     get_of_name(dom->of_node), base, base + nrppis - 1, c);
>> +
>> +             irq_domain_add(dom);
>> +
>> +             for (i = 0; i < nrppis; i++) {
>> +                     unsigned int irq = irq_domain_map(dom, i + irq_start);
>> +
>> +                     pr_debug("GIC mapping %s:%d to IRQ%d\n",
>> +                              get_of_name(dom->of_node), i, irq);
>> +
>> +                     irq_set_chip_and_handler(irq, &gic_ppi_chip,
>> +                                              handle_percpu_irq);
>> +                     irq_set_chip_data(irq, gic);
>> +                     set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
>> +             }
>>       }
>>  out:
>>  #endif
>> @@ -479,8 +608,9 @@ static void __cpuinit gic_cpu_init(struct gic_chip_data *gic)
>>       writel_relaxed(1, base + GIC_CPU_CTRL);
>>  }
>>
>> -void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
>> -     void __iomem *dist_base, void __iomem *cpu_base)
>> +static void __init gic_init_one(unsigned int gic_nr, unsigned int irq_start,
>> +                             void __iomem *dist_base, void __iomem *cpu_base,
>> +                             struct device_node *gic_node)
>>  {
>>       struct gic_chip_data *gic;
>>
>> @@ -494,10 +624,80 @@ void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
>>       if (gic_nr == 0)
>>               gic_cpu_base_addr = cpu_base;
>>
>> -     gic_dist_init(gic, irq_start);
>> +     gic_dist_init(gic, irq_start, gic_node);
>>       gic_cpu_init(gic);
>>  }
>>
>> +void __init gic_init(unsigned int gic_nr, unsigned int irq_start,
>> +                  void __iomem *dist_base, void __iomem *cpu_base)
>> +{
>> +     gic_init_one(gic_nr, irq_start, dist_base, cpu_base, NULL);
>> +}
>> +
>> +#ifdef CONFIG_OF
>> +static void __init gic_of_init_one(int idx, unsigned int irq_start,
>> +                                struct device_node *np)
>> +{
>> +     void __iomem *dist_base;
>> +     void __iomem *cpu_base;
>> +
>> +     dist_base = of_iomap(np, 0);
>> +     cpu_base  = of_iomap(np, 1);
>> +     if (!dist_base || ! cpu_base) {
>> +             pr_err("Cannot map %s\n", np->full_name);
>> +             return;
>> +     }
>> +
>> +     /* Assume we use the full 16 PPIs */
>> +     gic_init_one(idx, irq_start, dist_base, cpu_base, np);
>> +}
>> +
>> +void __init gic_of_init(void)
>> +{
>> +     struct device_node *np = NULL;
>> +     unsigned int irq_start = 0;
>> +     int i = 0;
>> +
>> +     /* Look for the GIC providing PPIs first */
>> +     while((np = gic_get_node(np))) {
>> +             struct device_node *ppi_np;
>> +             ppi_np = gic_get_ppi_node(np, 0);
>> +             of_node_put(ppi_np);
>> +             if (!ppi_np)
>> +                     continue;
>> +
>> +             /* Assume we use the full 16 PPIs */
>> +             gic_of_init_one(i, 16, np);
>> +             irq_start += gic_data[i].nr_irqs;
>> +             i++;
>> +             break;
>> +     }
>> +     of_node_put(np);
>> +     np = NULL;
>> +
>> +     /* Now all the others */
>> +     while((np = gic_get_node(np))) {
>> +             struct device_node *ppi_np;
>> +             int irq;
>> +             ppi_np = gic_get_ppi_node(np, 0);
>> +             of_node_put(ppi_np);
>> +             if (ppi_np)
>> +                     continue;
>> +
>> +             gic_of_init_one(i, irq_start, np);
>> +             irq = irq_of_parse_and_map(np, 0);
>> +             if (irq != IRQ_NONE) {
>> +                     pr_info("GIC %s cascading to IRQ%d",
>> +                             np->full_name, irq);
>> +                     gic_cascade_irq(i, irq);
>> +             }
>> +             irq_start += gic_data[i].nr_irqs;
>> +             i++;
>> +     }
>> +     of_node_put(np);
>> +}
>> +#endif
> 
> I see what you're doing here, but I don't think it is the best
> approach in the long run.  Rather than walking all the
> interrupt-controller nodes looking for primary and secondary gics
> while ignoring all others, I think there needs to be a function that
> finds all the interrupt controller nodes, sorts them based on
> dependencies, and calls their initialization hook in order.  Other
> architectures will use such a thing.  I've been intending to do it for
> powerpc for quite a while node.
> 
> Would you be willing to tackle such a beast?  I've got a fairly good
> idea what it needs to look like, but I just haven't had the time to do
> it.

I can give it a go.

> Overall the patch looks like it is in the right direction.

Thanks, comments much appreciated. I'll respin the patch series after
adressing the other comments and scratching my head a bit more on the
early device crap^H^H^H^Hproblem.

Cheers,

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

Re: [RFC PATCH 3/4] ARM: dt: add GIC bindings and driver support

[Grant Likely]

On Mon, Jun 27, 2011 at 4:27 AM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> On 26/06/11 09:01, Grant Likely wrote:
>> I see what you're doing here, but I don't think it is the best
>> approach in the long run.  Rather than walking all the
>> interrupt-controller nodes looking for primary and secondary gics
>> while ignoring all others, I think there needs to be a function that
>> finds all the interrupt controller nodes, sorts them based on
>> dependencies, and calls their initialization hook in order.  Other
>> architectures will use such a thing.  I've been intending to do it for
>> powerpc for quite a while node.
>>
>> Would you be willing to tackle such a beast?  I've got a fairly good
>> idea what it needs to look like, but I just haven't had the time to do
>> it.
>
> I can give it a go.

Awesome, thanks.

Okay, here is the general description of what needs to happen in ugly
pseudo code:

typedef void (*irq_init_cb_t)(struct intc_desc *intc_desc);

of_irq_init(struct of_device_id *irq_init)
{
        struct device_node *np;
        struct intc_desc *intc_desc;

        for_each_matching_node(np, match_table) {
                if (!of_find_property(np, "interrupt-controller"))
                        continue;
                /* Here, we allocate and populate an intc_desc with
the node pointer, interrupt-parent device_node etc. */
                intc_desc = of_alloc_intc_desc(np);
                list_add(&intc_desc->list, intc_desc_list);
        }

        /* The root irq controller is the one without an
interrupt-parent.  That one
         * goes first, followed by the controllers that reference it,
followed by the
         * ones that reference the 2nd level controllers, etc
         */
        of_irq_sort_descs();

        list_for_each_entry(intc_desc, intc_desc_list, list) {
                irq_init_cb_t *irq_init_cb;
                match = of_match_node(matches, desc->np);
                irq_init_cb = match->data;
                irq_init_cp(intc_desc);
        }
}

There's some ugliness in there with using of_device_id to get the
callback pointer because there is no typechecking when doing it that
way, but that just reflects the current infrastructure in
include/linux/of.h.  It doesn't have to be that way.

This doesn't work for all irq controllers, nor does it have to.
Anything on GPIO lines for instance can probably be deferred to
regular platform devices.  The main thing that is interesting here is
to get the core interrupt controllers bootstrapped so that other core
hardware, like timers, can use it immediately.

g.

[RFC PATCH 3/4] ARM: dt: add GIC bindings and driver support

[Grant Likely]

On Mon, Jun 27, 2011 at 4:27 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 26/06/11 09:01, Grant Likely wrote:
>> I see what you're doing here, but I don't think it is the best
>> approach in the long run. ?Rather than walking all the
>> interrupt-controller nodes looking for primary and secondary gics
>> while ignoring all others, I think there needs to be a function that
>> finds all the interrupt controller nodes, sorts them based on
>> dependencies, and calls their initialization hook in order. ?Other
>> architectures will use such a thing. ?I've been intending to do it for
>> powerpc for quite a while node.
>>
>> Would you be willing to tackle such a beast? ?I've got a fairly good
>> idea what it needs to look like, but I just haven't had the time to do
>> it.
>
> I can give it a go.

Awesome, thanks.

Okay, here is the general description of what needs to happen in ugly
pseudo code:

typedef void (*irq_init_cb_t)(struct intc_desc *intc_desc);

of_irq_init(struct of_device_id *irq_init)
{
        struct device_node *np;
        struct intc_desc *intc_desc;

        for_each_matching_node(np, match_table) {
                if (!of_find_property(np, "interrupt-controller"))
                        continue;
                /* Here, we allocate and populate an intc_desc with
the node pointer, interrupt-parent device_node etc. */
                intc_desc = of_alloc_intc_desc(np);
                list_add(&intc_desc->list, intc_desc_list);
        }

        /* The root irq controller is the one without an
interrupt-parent.  That one
         * goes first, followed by the controllers that reference it,
followed by the
         * ones that reference the 2nd level controllers, etc
         */
        of_irq_sort_descs();

        list_for_each_entry(intc_desc, intc_desc_list, list) {
                irq_init_cb_t *irq_init_cb;
                match = of_match_node(matches, desc->np);
                irq_init_cb = match->data;
                irq_init_cp(intc_desc);
        }
}

There's some ugliness in there with using of_device_id to get the
callback pointer because there is no typechecking when doing it that
way, but that just reflects the current infrastructure in
include/linux/of.h.  It doesn't have to be that way.

This doesn't work for all irq controllers, nor does it have to.
Anything on GPIO lines for instance can probably be deferred to
regular platform devices.  The main thing that is interesting here is
to get the core interrupt controllers bootstrapped so that other core
hardware, like timers, can use it immediately.

g.

[RFC PATCH 4/4] ARM: dt: Add TWD bindings and driver support

[Marc Zyngier]

Add device tree support to the arm_smp_twd driver.

The DT bindings for the TWD are defined as such:
- one timer node per CPU, using corresponding PPI
  interrupt controller
- provides both timer and watchdog interrupt

Tested on RealView PB11MPCore and VExpress.

Signed-off-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
---
 Documentation/devicetree/bindings/arm/twd.txt |   54 ++++++++++++++
 drivers/clocksource/arm_smp_twd.c             |   92 +++++++++++++++++++++---
 2 files changed, 134 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/twd.txt

diff --git a/Documentation/devicetree/bindings/arm/twd.txt b/Documentation/devicetree/bindings/arm/twd.txt
new file mode 100644
index 0000000..3823a81
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/twd.txt
@@ -0,0 +1,54 @@
+* ARM Timer Watchdog
+
+ARM SMP cores are often associated with a TWD providing a per-cpu timer
+and a per-cpu watchdog. Usually wired to the PPI interface of a GIC.
+
+Main node properties:
+
+- compatible		: "arm,smp-twd", "localtimer"
+- reg			: register mapping for the registers.
+- #address-cells	: <1>
+- #size-cells		: <0>
+
+Timer sub nodes (one per CPU)
+
+- interrupt-parent	: phandle to the corresponding gic-ppi.
+- interrupts		: <IRQtimer IRQwatchdog> (usually <29 30>)
+- reg			: index of the CPU this timer is connected to.
+
+Example (ARM RealView PB11-MPCore):
+
+localtimer@1f000600 {
+	compatible = "arm,smp-twd", "localtimer";
+	reg = <0x1f000600 0x100>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	/*
+	 * One timer per CPU, bound to the corresponding
+	 * PPI interface.
+	 */
+	timer@0 {
+		interrupt-parent = <&gic0ppi0>;
+		interrupts = <29 30>;
+		reg = <0>;
+	};
+
+	timer@1 {
+		interrupt-parent = <&gic0ppi1>;
+		interrupts = <29 30>;
+		reg = <1>;
+	};
+
+	timer@2 {
+		interrupt-parent = <&gic0ppi2>;
+		interrupts = <29 30>;
+		reg = <2>;
+	};
+
+	timer@3 {
+		interrupt-parent = <&gic0ppi3>;
+		interrupts = <29 30>;
+		reg = <3>;
+	};
+};
diff --git a/drivers/clocksource/arm_smp_twd.c b/drivers/clocksource/arm_smp_twd.c
index 65f4669..586acad 100644
--- a/drivers/clocksource/arm_smp_twd.c
+++ b/drivers/clocksource/arm_smp_twd.c
@@ -19,6 +19,9 @@
 #include <linux/interrupt.h>
 #include <linux/ioport.h>
 #include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
 #include <linux/clk.h>
 #include <linux/cpufreq.h>
 #include <linux/err.h>
@@ -37,11 +40,11 @@
 #define TWD_TIMER_CONTROL_IT_ENABLE	(1 << 2)
 
 static void __iomem *twd_base;
-static int twd_ppi;
 
 static struct clk *twd_clk;
 static unsigned long twd_timer_rate;
 static DEFINE_PER_CPU(bool, irq_reqd);
+static DEFINE_PER_CPU(int, twd_irq);
 static struct clock_event_device __percpu *twd_evt;
 
 static void twd_set_mode(enum clock_event_mode mode,
@@ -223,7 +226,7 @@ static void __cpuinit twd_setup(void *data)
 	clk->rating = 450;
 	clk->set_mode = twd_set_mode;
 	clk->set_next_event = twd_set_next_event;
-	clk->irq = gic_ppi_to_vppi(twd_ppi);
+	clk->irq = __get_cpu_var(twd_irq);
 	clk->cpumask = cpumask_of(smp_processor_id());
 
 	pr_debug("Configuring %s on cpu #%d\n", clk->name, smp_processor_id());
@@ -290,31 +293,90 @@ static struct notifier_block __cpuinitdata twd_cpu_nb = {
 	.notifier_call = twd_cpu_notify,
 };
 
-static int twd_probe(struct platform_device *pdev)
+#ifdef CONFIG_OF
+static struct device_node *twd_get_timer_node(struct device_node *twd_np,
+					      struct device_node *child,
+					      int *timer_nr)
+{
+	child = of_get_next_child(twd_np, child);
+	if (child) {
+		const __be32 *reg;
+		reg = of_get_property(child, "reg", NULL);
+		if (!reg)
+			*timer_nr = -1;
+		else
+			*timer_nr = be32_to_cpu(*reg);
+	}
+
+	return child;
+}
+
+static int __devinit twd_of_probe_irq(struct platform_device *pdev)
+{
+	struct device_node *timer = NULL;
+	int timer_nr;
+
+	if (!pdev->dev.of_node)
+		return -1;
+
+	while((timer = twd_get_timer_node(pdev->dev.of_node,
+					  timer, &timer_nr))) {
+		if (timer_nr < 0 || timer_nr >= NR_CPUS) {
+			dev_info(&pdev->dev, "Unknown timer id %d\n", timer_nr);
+			continue;
+		}
+
+		per_cpu(twd_irq, timer_nr) = irq_of_parse_and_map(timer, 0);
+		pr_info("%s IRQ%d", timer->full_name, per_cpu(twd_irq, timer_nr));
+	}
+
+	return 0;
+}
+#else
+static int __devinit twd_of_probe_irq(struct platform_device *pdev)
+{
+	return -1;
+}
+#endif
+
+static int __devinit twd_probe(struct platform_device *pdev)
 {
 	struct resource *mem;
 	struct clock_event_device *clk;
-	int irq;
 
 	if (twd_base)
 		return -EBUSY;
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	irq = platform_get_irq(pdev, 0);
-	if (!mem || irq < 0)
+	if (!mem)
 		return -EINVAL;
+	twd_base = ioremap(mem->start, resource_size(mem));
+
+	if (!twd_base)
+		return -ENOMEM;
+
+	if (twd_of_probe_irq(pdev)) {
+		int irq;
+		int i;
 
-	twd_base	= ioremap(mem->start, resource_size(mem));
-	twd_evt		= alloc_percpu(struct clock_event_device);
-	if (!twd_base || !twd_evt) {
+		irq = platform_get_irq(pdev, 0);
+		if (irq < 0) {
+			iounmap(twd_base);
+			return -EINVAL;
+		}
+
+		for (i = 0; i < NR_CPUS; i++)
+			per_cpu(twd_irq, i) = gic_ppi_to_cpu_vppi(irq, i);
+	}
+
+	twd_evt	= alloc_percpu(struct clock_event_device);
+	if (!twd_evt) {
 		iounmap(twd_base);
 		twd_base = NULL;
 		free_percpu(twd_evt);
 		return -ENOMEM;
 	}
 
-	twd_ppi = irq;
-
 	/* Immediately configure the timer on the boot CPU */
 	clk = per_cpu_ptr(twd_evt, smp_processor_id());
 	twd_setup(clk);
@@ -329,11 +391,17 @@ static int twd_remove(struct platform_device *pdev)
 	return -EBUSY;
 }
 
+static const struct of_device_id twd_of_ids[] = {
+	{ .compatible	= "arm,smp-twd" },
+	{ },
+};
+
 static struct platform_driver twd_driver = {
 	.probe	= twd_probe,
 	.remove = __devexit_p(twd_remove),
 	.driver = {
-		.name	= "arm_smp_twd",
+		.name		= "arm_smp_twd",
+		.of_match_table	= twd_of_ids,
 	},
 };
 
-- 
1.7.0.4

[RFC PATCH 4/4] ARM: dt: Add TWD bindings and driver support

[Marc Zyngier]

Add device tree support to the arm_smp_twd driver.

The DT bindings for the TWD are defined as such:
- one timer node per CPU, using corresponding PPI
  interrupt controller
- provides both timer and watchdog interrupt

Tested on RealView PB11MPCore and VExpress.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 Documentation/devicetree/bindings/arm/twd.txt |   54 ++++++++++++++
 drivers/clocksource/arm_smp_twd.c             |   92 +++++++++++++++++++++---
 2 files changed, 134 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/twd.txt

diff --git a/Documentation/devicetree/bindings/arm/twd.txt b/Documentation/devicetree/bindings/arm/twd.txt
new file mode 100644
index 0000000..3823a81
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/twd.txt
@@ -0,0 +1,54 @@
+* ARM Timer Watchdog
+
+ARM SMP cores are often associated with a TWD providing a per-cpu timer
+and a per-cpu watchdog. Usually wired to the PPI interface of a GIC.
+
+Main node properties:
+
+- compatible		: "arm,smp-twd", "localtimer"
+- reg			: register mapping for the registers.
+- #address-cells	: <1>
+- #size-cells		: <0>
+
+Timer sub nodes (one per CPU)
+
+- interrupt-parent	: phandle to the corresponding gic-ppi.
+- interrupts		: <IRQtimer IRQwatchdog> (usually <29 30>)
+- reg			: index of the CPU this timer is connected to.
+
+Example (ARM RealView PB11-MPCore):
+
+localtimer at 1f000600 {
+	compatible = "arm,smp-twd", "localtimer";
+	reg = <0x1f000600 0x100>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	/*
+	 * One timer per CPU, bound to the corresponding
+	 * PPI interface.
+	 */
+	timer at 0 {
+		interrupt-parent = <&gic0ppi0>;
+		interrupts = <29 30>;
+		reg = <0>;
+	};
+
+	timer at 1 {
+		interrupt-parent = <&gic0ppi1>;
+		interrupts = <29 30>;
+		reg = <1>;
+	};
+
+	timer at 2 {
+		interrupt-parent = <&gic0ppi2>;
+		interrupts = <29 30>;
+		reg = <2>;
+	};
+
+	timer at 3 {
+		interrupt-parent = <&gic0ppi3>;
+		interrupts = <29 30>;
+		reg = <3>;
+	};
+};
diff --git a/drivers/clocksource/arm_smp_twd.c b/drivers/clocksource/arm_smp_twd.c
index 65f4669..586acad 100644
--- a/drivers/clocksource/arm_smp_twd.c
+++ b/drivers/clocksource/arm_smp_twd.c
@@ -19,6 +19,9 @@
 #include <linux/interrupt.h>
 #include <linux/ioport.h>
 #include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
 #include <linux/clk.h>
 #include <linux/cpufreq.h>
 #include <linux/err.h>
@@ -37,11 +40,11 @@
 #define TWD_TIMER_CONTROL_IT_ENABLE	(1 << 2)
 
 static void __iomem *twd_base;
-static int twd_ppi;
 
 static struct clk *twd_clk;
 static unsigned long twd_timer_rate;
 static DEFINE_PER_CPU(bool, irq_reqd);
+static DEFINE_PER_CPU(int, twd_irq);
 static struct clock_event_device __percpu *twd_evt;
 
 static void twd_set_mode(enum clock_event_mode mode,
@@ -223,7 +226,7 @@ static void __cpuinit twd_setup(void *data)
 	clk->rating = 450;
 	clk->set_mode = twd_set_mode;
 	clk->set_next_event = twd_set_next_event;
-	clk->irq = gic_ppi_to_vppi(twd_ppi);
+	clk->irq = __get_cpu_var(twd_irq);
 	clk->cpumask = cpumask_of(smp_processor_id());
 
 	pr_debug("Configuring %s on cpu #%d\n", clk->name, smp_processor_id());
@@ -290,31 +293,90 @@ static struct notifier_block __cpuinitdata twd_cpu_nb = {
 	.notifier_call = twd_cpu_notify,
 };
 
-static int twd_probe(struct platform_device *pdev)
+#ifdef CONFIG_OF
+static struct device_node *twd_get_timer_node(struct device_node *twd_np,
+					      struct device_node *child,
+					      int *timer_nr)
+{
+	child = of_get_next_child(twd_np, child);
+	if (child) {
+		const __be32 *reg;
+		reg = of_get_property(child, "reg", NULL);
+		if (!reg)
+			*timer_nr = -1;
+		else
+			*timer_nr = be32_to_cpu(*reg);
+	}
+
+	return child;
+}
+
+static int __devinit twd_of_probe_irq(struct platform_device *pdev)
+{
+	struct device_node *timer = NULL;
+	int timer_nr;
+
+	if (!pdev->dev.of_node)
+		return -1;
+
+	while((timer = twd_get_timer_node(pdev->dev.of_node,
+					  timer, &timer_nr))) {
+		if (timer_nr < 0 || timer_nr >= NR_CPUS) {
+			dev_info(&pdev->dev, "Unknown timer id %d\n", timer_nr);
+			continue;
+		}
+
+		per_cpu(twd_irq, timer_nr) = irq_of_parse_and_map(timer, 0);
+		pr_info("%s IRQ%d", timer->full_name, per_cpu(twd_irq, timer_nr));
+	}
+
+	return 0;
+}
+#else
+static int __devinit twd_of_probe_irq(struct platform_device *pdev)
+{
+	return -1;
+}
+#endif
+
+static int __devinit twd_probe(struct platform_device *pdev)
 {
 	struct resource *mem;
 	struct clock_event_device *clk;
-	int irq;
 
 	if (twd_base)
 		return -EBUSY;
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	irq = platform_get_irq(pdev, 0);
-	if (!mem || irq < 0)
+	if (!mem)
 		return -EINVAL;
+	twd_base = ioremap(mem->start, resource_size(mem));
+
+	if (!twd_base)
+		return -ENOMEM;
+
+	if (twd_of_probe_irq(pdev)) {
+		int irq;
+		int i;
 
-	twd_base	= ioremap(mem->start, resource_size(mem));
-	twd_evt		= alloc_percpu(struct clock_event_device);
-	if (!twd_base || !twd_evt) {
+		irq = platform_get_irq(pdev, 0);
+		if (irq < 0) {
+			iounmap(twd_base);
+			return -EINVAL;
+		}
+
+		for (i = 0; i < NR_CPUS; i++)
+			per_cpu(twd_irq, i) = gic_ppi_to_cpu_vppi(irq, i);
+	}
+
+	twd_evt	= alloc_percpu(struct clock_event_device);
+	if (!twd_evt) {
 		iounmap(twd_base);
 		twd_base = NULL;
 		free_percpu(twd_evt);
 		return -ENOMEM;
 	}
 
-	twd_ppi = irq;
-
 	/* Immediately configure the timer on the boot CPU */
 	clk = per_cpu_ptr(twd_evt, smp_processor_id());
 	twd_setup(clk);
@@ -329,11 +391,17 @@ static int twd_remove(struct platform_device *pdev)
 	return -EBUSY;
 }
 
+static const struct of_device_id twd_of_ids[] = {
+	{ .compatible	= "arm,smp-twd" },
+	{ },
+};
+
 static struct platform_driver twd_driver = {
 	.probe	= twd_probe,
 	.remove = __devexit_p(twd_remove),
 	.driver = {
-		.name	= "arm_smp_twd",
+		.name		= "arm_smp_twd",
+		.of_match_table	= twd_of_ids,
 	},
 };
 
-- 
1.7.0.4

Re: [RFC PATCH 4/4] ARM: dt: Add TWD bindings and driver support

[Grant Likely]

On Fri, Jun 24, 2011 at 03:10:59PM +0100, Marc Zyngier wrote:
> Add device tree support to the arm_smp_twd driver.
> 
> The DT bindings for the TWD are defined as such:
> - one timer node per CPU, using corresponding PPI
>   interrupt controller
> - provides both timer and watchdog interrupt
> 
> Tested on RealView PB11MPCore and VExpress.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/arm/twd.txt |   54 ++++++++++++++
>  drivers/clocksource/arm_smp_twd.c             |   92 +++++++++++++++++++++---
>  2 files changed, 134 insertions(+), 12 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/twd.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/twd.txt b/Documentation/devicetree/bindings/arm/twd.txt
> new file mode 100644
> index 0000000..3823a81
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/twd.txt
> @@ -0,0 +1,54 @@
> +* ARM Timer Watchdog
> +
> +ARM SMP cores are often associated with a TWD providing a per-cpu timer
> +and a per-cpu watchdog. Usually wired to the PPI interface of a GIC.
> +
> +Main node properties:
> +
> +- compatible		: "arm,smp-twd", "localtimer"

"localtimer" isn't a very good compatible value.  Compatible is
intended to identify the specific hardware, and "localtimer" is a very
generic name.

Also, as commented on for the gic, I'd like to see some level of
versioning on the arm,smp-twd string.

> +- reg			: register mapping for the registers.
> +- #address-cells	: <1>
> +- #size-cells		: <0>
> +
> +Timer sub nodes (one per CPU)
> +
> +- interrupt-parent	: phandle to the corresponding gic-ppi.
> +- interrupts		: <IRQtimer IRQwatchdog> (usually <29 30>)
> +- reg			: index of the CPU this timer is connected to.
> +
> +Example (ARM RealView PB11-MPCore):
> +
> +localtimer@1f000600 {
> +	compatible = "arm,smp-twd", "localtimer";
> +	reg = <0x1f000600 0x100>;
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	/*
> +	 * One timer per CPU, bound to the corresponding
> +	 * PPI interface.
> +	 */
> +	timer@0 {
> +		interrupt-parent = <&gic0ppi0>;
> +		interrupts = <29 30>;
> +		reg = <0>;
> +	};
> +
> +	timer@1 {
> +		interrupt-parent = <&gic0ppi1>;
> +		interrupts = <29 30>;
> +		reg = <1>;
> +	};
> +
> +	timer@2 {
> +		interrupt-parent = <&gic0ppi2>;
> +		interrupts = <29 30>;
> +		reg = <2>;
> +	};
> +
> +	timer@3 {
> +		interrupt-parent = <&gic0ppi3>;
> +		interrupts = <29 30>;
> +		reg = <3>;
> +	};
> +};
> diff --git a/drivers/clocksource/arm_smp_twd.c b/drivers/clocksource/arm_smp_twd.c
> index 65f4669..586acad 100644
> --- a/drivers/clocksource/arm_smp_twd.c
> +++ b/drivers/clocksource/arm_smp_twd.c
> @@ -19,6 +19,9 @@
>  #include <linux/interrupt.h>
>  #include <linux/ioport.h>
>  #include <linux/platform_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
>  #include <linux/clk.h>
>  #include <linux/cpufreq.h>
>  #include <linux/err.h>
> @@ -37,11 +40,11 @@
>  #define TWD_TIMER_CONTROL_IT_ENABLE	(1 << 2)
>  
>  static void __iomem *twd_base;
> -static int twd_ppi;
>  
>  static struct clk *twd_clk;
>  static unsigned long twd_timer_rate;
>  static DEFINE_PER_CPU(bool, irq_reqd);
> +static DEFINE_PER_CPU(int, twd_irq);
>  static struct clock_event_device __percpu *twd_evt;
>  
>  static void twd_set_mode(enum clock_event_mode mode,
> @@ -223,7 +226,7 @@ static void __cpuinit twd_setup(void *data)
>  	clk->rating = 450;
>  	clk->set_mode = twd_set_mode;
>  	clk->set_next_event = twd_set_next_event;
> -	clk->irq = gic_ppi_to_vppi(twd_ppi);
> +	clk->irq = __get_cpu_var(twd_irq);
>  	clk->cpumask = cpumask_of(smp_processor_id());
>  
>  	pr_debug("Configuring %s on cpu #%d\n", clk->name, smp_processor_id());
> @@ -290,31 +293,90 @@ static struct notifier_block __cpuinitdata twd_cpu_nb = {
>  	.notifier_call = twd_cpu_notify,
>  };
>  
> -static int twd_probe(struct platform_device *pdev)
> +#ifdef CONFIG_OF
> +static struct device_node *twd_get_timer_node(struct device_node *twd_np,
> +					      struct device_node *child,
> +					      int *timer_nr)
> +{
> +	child = of_get_next_child(twd_np, child);
> +	if (child) {
> +		const __be32 *reg;
> +		reg = of_get_property(child, "reg", NULL);
> +		if (!reg)
> +			*timer_nr = -1;
> +		else
> +			*timer_nr = be32_to_cpu(*reg);

There's a new function that should be done and merged in time for
v3.1 that will help with reading properties.  Keep an eye out for
of_getprop_u32()

g.

[RFC PATCH 4/4] ARM: dt: Add TWD bindings and driver support

[Grant Likely]

On Fri, Jun 24, 2011 at 03:10:59PM +0100, Marc Zyngier wrote:
> Add device tree support to the arm_smp_twd driver.
> 
> The DT bindings for the TWD are defined as such:
> - one timer node per CPU, using corresponding PPI
>   interrupt controller
> - provides both timer and watchdog interrupt
> 
> Tested on RealView PB11MPCore and VExpress.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  Documentation/devicetree/bindings/arm/twd.txt |   54 ++++++++++++++
>  drivers/clocksource/arm_smp_twd.c             |   92 +++++++++++++++++++++---
>  2 files changed, 134 insertions(+), 12 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/twd.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/twd.txt b/Documentation/devicetree/bindings/arm/twd.txt
> new file mode 100644
> index 0000000..3823a81
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/twd.txt
> @@ -0,0 +1,54 @@
> +* ARM Timer Watchdog
> +
> +ARM SMP cores are often associated with a TWD providing a per-cpu timer
> +and a per-cpu watchdog. Usually wired to the PPI interface of a GIC.
> +
> +Main node properties:
> +
> +- compatible		: "arm,smp-twd", "localtimer"

"localtimer" isn't a very good compatible value.  Compatible is
intended to identify the specific hardware, and "localtimer" is a very
generic name.

Also, as commented on for the gic, I'd like to see some level of
versioning on the arm,smp-twd string.

> +- reg			: register mapping for the registers.
> +- #address-cells	: <1>
> +- #size-cells		: <0>
> +
> +Timer sub nodes (one per CPU)
> +
> +- interrupt-parent	: phandle to the corresponding gic-ppi.
> +- interrupts		: <IRQtimer IRQwatchdog> (usually <29 30>)
> +- reg			: index of the CPU this timer is connected to.
> +
> +Example (ARM RealView PB11-MPCore):
> +
> +localtimer at 1f000600 {
> +	compatible = "arm,smp-twd", "localtimer";
> +	reg = <0x1f000600 0x100>;
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	/*
> +	 * One timer per CPU, bound to the corresponding
> +	 * PPI interface.
> +	 */
> +	timer at 0 {
> +		interrupt-parent = <&gic0ppi0>;
> +		interrupts = <29 30>;
> +		reg = <0>;
> +	};
> +
> +	timer at 1 {
> +		interrupt-parent = <&gic0ppi1>;
> +		interrupts = <29 30>;
> +		reg = <1>;
> +	};
> +
> +	timer at 2 {
> +		interrupt-parent = <&gic0ppi2>;
> +		interrupts = <29 30>;
> +		reg = <2>;
> +	};
> +
> +	timer at 3 {
> +		interrupt-parent = <&gic0ppi3>;
> +		interrupts = <29 30>;
> +		reg = <3>;
> +	};
> +};
> diff --git a/drivers/clocksource/arm_smp_twd.c b/drivers/clocksource/arm_smp_twd.c
> index 65f4669..586acad 100644
> --- a/drivers/clocksource/arm_smp_twd.c
> +++ b/drivers/clocksource/arm_smp_twd.c
> @@ -19,6 +19,9 @@
>  #include <linux/interrupt.h>
>  #include <linux/ioport.h>
>  #include <linux/platform_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
>  #include <linux/clk.h>
>  #include <linux/cpufreq.h>
>  #include <linux/err.h>
> @@ -37,11 +40,11 @@
>  #define TWD_TIMER_CONTROL_IT_ENABLE	(1 << 2)
>  
>  static void __iomem *twd_base;
> -static int twd_ppi;
>  
>  static struct clk *twd_clk;
>  static unsigned long twd_timer_rate;
>  static DEFINE_PER_CPU(bool, irq_reqd);
> +static DEFINE_PER_CPU(int, twd_irq);
>  static struct clock_event_device __percpu *twd_evt;
>  
>  static void twd_set_mode(enum clock_event_mode mode,
> @@ -223,7 +226,7 @@ static void __cpuinit twd_setup(void *data)
>  	clk->rating = 450;
>  	clk->set_mode = twd_set_mode;
>  	clk->set_next_event = twd_set_next_event;
> -	clk->irq = gic_ppi_to_vppi(twd_ppi);
> +	clk->irq = __get_cpu_var(twd_irq);
>  	clk->cpumask = cpumask_of(smp_processor_id());
>  
>  	pr_debug("Configuring %s on cpu #%d\n", clk->name, smp_processor_id());
> @@ -290,31 +293,90 @@ static struct notifier_block __cpuinitdata twd_cpu_nb = {
>  	.notifier_call = twd_cpu_notify,
>  };
>  
> -static int twd_probe(struct platform_device *pdev)
> +#ifdef CONFIG_OF
> +static struct device_node *twd_get_timer_node(struct device_node *twd_np,
> +					      struct device_node *child,
> +					      int *timer_nr)
> +{
> +	child = of_get_next_child(twd_np, child);
> +	if (child) {
> +		const __be32 *reg;
> +		reg = of_get_property(child, "reg", NULL);
> +		if (!reg)
> +			*timer_nr = -1;
> +		else
> +			*timer_nr = be32_to_cpu(*reg);

There's a new function that should be done and merged in time for
v3.1 that will help with reading properties.  Keep an eye out for
of_getprop_u32()

g.

Re: [RFC PATCH 4/4] ARM: dt: Add TWD bindings and driver support

[Marc Zyngier]

On 26/06/11 09:09, Grant Likely wrote:
> On Fri, Jun 24, 2011 at 03:10:59PM +0100, Marc Zyngier wrote:
>> Add device tree support to the arm_smp_twd driver.
>>
>> The DT bindings for the TWD are defined as such:
>> - one timer node per CPU, using corresponding PPI
>>   interrupt controller
>> - provides both timer and watchdog interrupt
>>
>> Tested on RealView PB11MPCore and VExpress.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/arm/twd.txt |   54 ++++++++++++++
>>  drivers/clocksource/arm_smp_twd.c             |   92 +++++++++++++++++++++---
>>  2 files changed, 134 insertions(+), 12 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/arm/twd.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/twd.txt b/Documentation/devicetree/bindings/arm/twd.txt
>> new file mode 100644
>> index 0000000..3823a81
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/twd.txt
>> @@ -0,0 +1,54 @@
>> +* ARM Timer Watchdog
>> +
>> +ARM SMP cores are often associated with a TWD providing a per-cpu timer
>> +and a per-cpu watchdog. Usually wired to the PPI interface of a GIC.
>> +
>> +Main node properties:
>> +
>> +- compatible		: "arm,smp-twd", "localtimer"
> 
> "localtimer" isn't a very good compatible value.  Compatible is
> intended to identify the specific hardware, and "localtimer" is a very
> generic name.

This is the early_device class name creeping in. I usually used the
"device_type" property, but been told this wasn't the right way...

Of course, if we get rid of the idea of early devices, this is a moot point.

> Also, as commented on for the gic, I'd like to see some level of
> versioning on the arm,smp-twd string.

There's only one version of TWD at the moment, so I guess I'll tie it to
the CPU core implementation, e.g: "arm,twd-11mpcore", "arm,twd-cortex-a9"...

>> +- reg			: register mapping for the registers.
>> +- #address-cells	: <1>
>> +- #size-cells		: <0>
>> +
>> +Timer sub nodes (one per CPU)
>> +
>> +- interrupt-parent	: phandle to the corresponding gic-ppi.
>> +- interrupts		: <IRQtimer IRQwatchdog> (usually <29 30>)
>> +- reg			: index of the CPU this timer is connected to.
>> +
>> +Example (ARM RealView PB11-MPCore):
>> +
>> +localtimer@1f000600 {
>> +	compatible = "arm,smp-twd", "localtimer";
>> +	reg = <0x1f000600 0x100>;
>> +	#address-cells = <1>;
>> +	#size-cells = <0>;
>> +
>> +	/*
>> +	 * One timer per CPU, bound to the corresponding
>> +	 * PPI interface.
>> +	 */
>> +	timer@0 {
>> +		interrupt-parent = <&gic0ppi0>;
>> +		interrupts = <29 30>;
>> +		reg = <0>;
>> +	};
>> +
>> +	timer@1 {
>> +		interrupt-parent = <&gic0ppi1>;
>> +		interrupts = <29 30>;
>> +		reg = <1>;
>> +	};
>> +
>> +	timer@2 {
>> +		interrupt-parent = <&gic0ppi2>;
>> +		interrupts = <29 30>;
>> +		reg = <2>;
>> +	};
>> +
>> +	timer@3 {
>> +		interrupt-parent = <&gic0ppi3>;
>> +		interrupts = <29 30>;
>> +		reg = <3>;
>> +	};
>> +};
>> diff --git a/drivers/clocksource/arm_smp_twd.c b/drivers/clocksource/arm_smp_twd.c
>> index 65f4669..586acad 100644
>> --- a/drivers/clocksource/arm_smp_twd.c
>> +++ b/drivers/clocksource/arm_smp_twd.c
>> @@ -19,6 +19,9 @@
>>  #include <linux/interrupt.h>
>>  #include <linux/ioport.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>>  #include <linux/clk.h>
>>  #include <linux/cpufreq.h>
>>  #include <linux/err.h>
>> @@ -37,11 +40,11 @@
>>  #define TWD_TIMER_CONTROL_IT_ENABLE	(1 << 2)
>>  
>>  static void __iomem *twd_base;
>> -static int twd_ppi;
>>  
>>  static struct clk *twd_clk;
>>  static unsigned long twd_timer_rate;
>>  static DEFINE_PER_CPU(bool, irq_reqd);
>> +static DEFINE_PER_CPU(int, twd_irq);
>>  static struct clock_event_device __percpu *twd_evt;
>>  
>>  static void twd_set_mode(enum clock_event_mode mode,
>> @@ -223,7 +226,7 @@ static void __cpuinit twd_setup(void *data)
>>  	clk->rating = 450;
>>  	clk->set_mode = twd_set_mode;
>>  	clk->set_next_event = twd_set_next_event;
>> -	clk->irq = gic_ppi_to_vppi(twd_ppi);
>> +	clk->irq = __get_cpu_var(twd_irq);
>>  	clk->cpumask = cpumask_of(smp_processor_id());
>>  
>>  	pr_debug("Configuring %s on cpu #%d\n", clk->name, smp_processor_id());
>> @@ -290,31 +293,90 @@ static struct notifier_block __cpuinitdata twd_cpu_nb = {
>>  	.notifier_call = twd_cpu_notify,
>>  };
>>  
>> -static int twd_probe(struct platform_device *pdev)
>> +#ifdef CONFIG_OF
>> +static struct device_node *twd_get_timer_node(struct device_node *twd_np,
>> +					      struct device_node *child,
>> +					      int *timer_nr)
>> +{
>> +	child = of_get_next_child(twd_np, child);
>> +	if (child) {
>> +		const __be32 *reg;
>> +		reg = of_get_property(child, "reg", NULL);
>> +		if (!reg)
>> +			*timer_nr = -1;
>> +		else
>> +			*timer_nr = be32_to_cpu(*reg);
> 
> There's a new function that should be done and merged in time for
> v3.1 that will help with reading properties.  Keep an eye out for
> of_getprop_u32()

Yup, spotted. Will use it as soon as it appears in a tree nearby.

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

[RFC PATCH 4/4] ARM: dt: Add TWD bindings and driver support

[Marc Zyngier]

On 26/06/11 09:09, Grant Likely wrote:
> On Fri, Jun 24, 2011 at 03:10:59PM +0100, Marc Zyngier wrote:
>> Add device tree support to the arm_smp_twd driver.
>>
>> The DT bindings for the TWD are defined as such:
>> - one timer node per CPU, using corresponding PPI
>>   interrupt controller
>> - provides both timer and watchdog interrupt
>>
>> Tested on RealView PB11MPCore and VExpress.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  Documentation/devicetree/bindings/arm/twd.txt |   54 ++++++++++++++
>>  drivers/clocksource/arm_smp_twd.c             |   92 +++++++++++++++++++++---
>>  2 files changed, 134 insertions(+), 12 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/arm/twd.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/twd.txt b/Documentation/devicetree/bindings/arm/twd.txt
>> new file mode 100644
>> index 0000000..3823a81
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/twd.txt
>> @@ -0,0 +1,54 @@
>> +* ARM Timer Watchdog
>> +
>> +ARM SMP cores are often associated with a TWD providing a per-cpu timer
>> +and a per-cpu watchdog. Usually wired to the PPI interface of a GIC.
>> +
>> +Main node properties:
>> +
>> +- compatible		: "arm,smp-twd", "localtimer"
> 
> "localtimer" isn't a very good compatible value.  Compatible is
> intended to identify the specific hardware, and "localtimer" is a very
> generic name.

This is the early_device class name creeping in. I usually used the
"device_type" property, but been told this wasn't the right way...

Of course, if we get rid of the idea of early devices, this is a moot point.

> Also, as commented on for the gic, I'd like to see some level of
> versioning on the arm,smp-twd string.

There's only one version of TWD at the moment, so I guess I'll tie it to
the CPU core implementation, e.g: "arm,twd-11mpcore", "arm,twd-cortex-a9"...

>> +- reg			: register mapping for the registers.
>> +- #address-cells	: <1>
>> +- #size-cells		: <0>
>> +
>> +Timer sub nodes (one per CPU)
>> +
>> +- interrupt-parent	: phandle to the corresponding gic-ppi.
>> +- interrupts		: <IRQtimer IRQwatchdog> (usually <29 30>)
>> +- reg			: index of the CPU this timer is connected to.
>> +
>> +Example (ARM RealView PB11-MPCore):
>> +
>> +localtimer at 1f000600 {
>> +	compatible = "arm,smp-twd", "localtimer";
>> +	reg = <0x1f000600 0x100>;
>> +	#address-cells = <1>;
>> +	#size-cells = <0>;
>> +
>> +	/*
>> +	 * One timer per CPU, bound to the corresponding
>> +	 * PPI interface.
>> +	 */
>> +	timer at 0 {
>> +		interrupt-parent = <&gic0ppi0>;
>> +		interrupts = <29 30>;
>> +		reg = <0>;
>> +	};
>> +
>> +	timer at 1 {
>> +		interrupt-parent = <&gic0ppi1>;
>> +		interrupts = <29 30>;
>> +		reg = <1>;
>> +	};
>> +
>> +	timer at 2 {
>> +		interrupt-parent = <&gic0ppi2>;
>> +		interrupts = <29 30>;
>> +		reg = <2>;
>> +	};
>> +
>> +	timer at 3 {
>> +		interrupt-parent = <&gic0ppi3>;
>> +		interrupts = <29 30>;
>> +		reg = <3>;
>> +	};
>> +};
>> diff --git a/drivers/clocksource/arm_smp_twd.c b/drivers/clocksource/arm_smp_twd.c
>> index 65f4669..586acad 100644
>> --- a/drivers/clocksource/arm_smp_twd.c
>> +++ b/drivers/clocksource/arm_smp_twd.c
>> @@ -19,6 +19,9 @@
>>  #include <linux/interrupt.h>
>>  #include <linux/ioport.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>>  #include <linux/clk.h>
>>  #include <linux/cpufreq.h>
>>  #include <linux/err.h>
>> @@ -37,11 +40,11 @@
>>  #define TWD_TIMER_CONTROL_IT_ENABLE	(1 << 2)
>>  
>>  static void __iomem *twd_base;
>> -static int twd_ppi;
>>  
>>  static struct clk *twd_clk;
>>  static unsigned long twd_timer_rate;
>>  static DEFINE_PER_CPU(bool, irq_reqd);
>> +static DEFINE_PER_CPU(int, twd_irq);
>>  static struct clock_event_device __percpu *twd_evt;
>>  
>>  static void twd_set_mode(enum clock_event_mode mode,
>> @@ -223,7 +226,7 @@ static void __cpuinit twd_setup(void *data)
>>  	clk->rating = 450;
>>  	clk->set_mode = twd_set_mode;
>>  	clk->set_next_event = twd_set_next_event;
>> -	clk->irq = gic_ppi_to_vppi(twd_ppi);
>> +	clk->irq = __get_cpu_var(twd_irq);
>>  	clk->cpumask = cpumask_of(smp_processor_id());
>>  
>>  	pr_debug("Configuring %s on cpu #%d\n", clk->name, smp_processor_id());
>> @@ -290,31 +293,90 @@ static struct notifier_block __cpuinitdata twd_cpu_nb = {
>>  	.notifier_call = twd_cpu_notify,
>>  };
>>  
>> -static int twd_probe(struct platform_device *pdev)
>> +#ifdef CONFIG_OF
>> +static struct device_node *twd_get_timer_node(struct device_node *twd_np,
>> +					      struct device_node *child,
>> +					      int *timer_nr)
>> +{
>> +	child = of_get_next_child(twd_np, child);
>> +	if (child) {
>> +		const __be32 *reg;
>> +		reg = of_get_property(child, "reg", NULL);
>> +		if (!reg)
>> +			*timer_nr = -1;
>> +		else
>> +			*timer_nr = be32_to_cpu(*reg);
> 
> There's a new function that should be done and merged in time for
> v3.1 that will help with reading properties.  Keep an eye out for
> of_getprop_u32()

Yup, spotted. Will use it as soon as it appears in a tree nearby.

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