Re: [PATCH v2] ARM: at91: pit add DT support

Re: [PATCH v2] ARM: at91: pit add DT support

[Jamie Iles]

Hi Nicolas, Jean-Christophe,

On Thu, Jan 05, 2012 at 02:48:28PM +0100, Nicolas Ferre wrote:
> From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> 
> Retreive registers address and IRQ from device tree entry. Fall back
> to built-in values if an error occurs.
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> [nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org: change error path and interrupts property handling]
> Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> ---
[...]
> diff --git a/arch/arm/mach-at91/at91sam926x_time.c 
> b/arch/arm/mach-at91/at91sam926x_time.c
> index d89ead7..5d4c308 100644
> --- a/arch/arm/mach-at91/at91sam926x_time.c
> +++ b/arch/arm/mach-at91/at91sam926x_time.c
> @@ -14,6 +14,8 @@
[...]
> +static int __init of_at91sam926x_pit_init(void)
> +{
> +	struct device_node *np;
> +
> +	np = of_find_matching_node(NULL, timer_ids);
> +	if (!np)
> +		goto err;
> +
> +	pit_base_addr = of_iomap(np, 0);
> +	if (!pit_base_addr)
> +		goto node_err;
> +
> +	/* Get the interrupts property */
> +	if (of_property_read_u32(np, "interrupts", &at91sam926x_pit_irq.irq))
> +		goto ioremap_err;

I think you want:

	at91sam926x_pit_irq.irq = irq_of_parse_and_map(np, 0);

to make sure the IRQ is converted to a Linux IRQ and not a hwirq.  Other 
than that looks fine to me.

Jamie

[PATCH v2] ARM: at91: pit add DT support

[Nicolas Ferre]

From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>

Retreive registers address and IRQ from device tree entry. Fall back
to built-in values if an error occurs.

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
[nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org: change error path and interrupts property handling]
Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/arm/atmel-at91.txt         |    8 +++
 arch/arm/boot/dts/at91sam9g20.dtsi                 |    5 ++
 arch/arm/boot/dts/at91sam9g45.dtsi                 |    6 ++
 arch/arm/mach-at91/at91sam926x_time.c              |   49 +++++++++++++++++++-
 4 files changed, 66 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/atmel-at91.txt

diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt b/Documentation/devicetree/bindings/arm/atmel-at91.txt
new file mode 100644
index 0000000..380f711
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt
@@ -0,0 +1,8 @@
+Atmel AT91 device tree bindings.
+================================
+
+PIT Timer required properties:
+- compatible: Should be "atmel,at91sam9260-pit"
+- reg: Should contain registers location and length
+- interrupts: Should contain interrupt for the PIT which is the IRQ line
+  shared across all System Controller members.
diff --git a/arch/arm/boot/dts/at91sam9g20.dtsi b/arch/arm/boot/dts/at91sam9g20.dtsi
index 0d21872..5c4be7e 100644
--- a/arch/arm/boot/dts/at91sam9g20.dtsi
+++ b/arch/arm/boot/dts/at91sam9g20.dtsi
@@ -57,6 +57,11 @@
 				reg = <0xfffff000 0x200>;
 			};
 
+			pit: timer@fffffd30 {
+				compatible = "atmel,at91sam9260-pit";
+				reg = <0xfffffd30 0xf>;
+				interrupts = <1 4>;
+			};
 
 			pioA: gpio@fffff400 {
 				compatible = "atmel,at91rm9200-gpio";
diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
index b977a79..0d21c8e 100644
--- a/arch/arm/boot/dts/at91sam9g45.dtsi
+++ b/arch/arm/boot/dts/at91sam9g45.dtsi
@@ -58,6 +58,12 @@
 				reg = <0xfffff000 0x200>;
 			};
 
+			pit: timer@fffffd30 {
+				compatible = "atmel,at91sam9260-pit";
+				reg = <0xfffffd30 0xf>;
+				interrupts = <1 4>;
+			};
+
 			dma: dma-controller@ffffec00 {
 				compatible = "atmel,at91sam9g45-dma";
 				reg = <0xffffec00 0x200>;
diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c
index d89ead7..5d4c308 100644
--- a/arch/arm/mach-at91/at91sam926x_time.c
+++ b/arch/arm/mach-at91/at91sam926x_time.c
@@ -14,6 +14,8 @@
 #include <linux/kernel.h>
 #include <linux/clk.h>
 #include <linux/clockchips.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
 
 #include <asm/mach/time.h>
 
@@ -133,7 +135,8 @@ static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
 static struct irqaction at91sam926x_pit_irq = {
 	.name		= "at91_tick",
 	.flags		= IRQF_SHARED | IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
-	.handler	= at91sam926x_pit_interrupt
+	.handler	= at91sam926x_pit_interrupt,
+	.irq		= AT91_ID_SYS,
 };
 
 static void at91sam926x_pit_reset(void)
@@ -149,6 +152,46 @@ static void at91sam926x_pit_reset(void)
 	pit_write(AT91_PIT_MR, (pit_cycle - 1) | AT91_PIT_PITEN);
 }
 
+#ifdef CONFIG_OF
+static struct of_device_id timer_ids[] = {
+	{ .compatible = "atmel,at91sam9260-pit" },
+	{ /* sentinel */ }
+};
+
+static int __init of_at91sam926x_pit_init(void)
+{
+	struct device_node *np;
+
+	np = of_find_matching_node(NULL, timer_ids);
+	if (!np)
+		goto err;
+
+	pit_base_addr = of_iomap(np, 0);
+	if (!pit_base_addr)
+		goto node_err;
+
+	/* Get the interrupts property */
+	if (of_property_read_u32(np, "interrupts", &at91sam926x_pit_irq.irq))
+		goto ioremap_err;
+
+	of_node_put(np);
+
+	return 0;
+
+ioremap_err:
+	iounmap(pit_base_addr);
+node_err:
+	of_node_put(np);
+err:
+	return -EINVAL;
+}
+#else
+static int __init of_at91sam926x_pit_init(void)
+{
+	return -EINVAL;
+}
+#endif
+
 /*
  * Set up both clocksource and clockevent support.
  */
@@ -177,7 +220,7 @@ static void __init at91sam926x_pit_init(void)
 	clocksource_register_hz(&pit_clk, pit_rate);
 
 	/* Set up irq handler */
-	setup_irq(AT91_ID_SYS, &at91sam926x_pit_irq);
+	setup_irq(at91sam926x_pit_irq.irq, &at91sam926x_pit_irq);
 
 	/* Set up and register clockevents */
 	pit_clkevt.mult = div_sc(pit_rate, NSEC_PER_SEC, pit_clkevt.shift);
@@ -193,6 +236,8 @@ static void at91sam926x_pit_suspend(void)
 
 void __init at91sam926x_ioremap_pit(u32 addr)
 {
+	if (!of_at91sam926x_pit_init())
+		return;
 	pit_base_addr = ioremap(addr, 16);
 
 	if (!pit_base_addr)
-- 
1.7.5.4

Re: [PATCH v2] ARM: at91: pit add DT support

[Nicolas Ferre]

On 01/05/2012 01:00 PM, Jamie Iles :
> Hi Nicolas, Jean-Christophe,
> 
> On Thu, Jan 05, 2012 at 02:48:28PM +0100, Nicolas Ferre wrote:
>> From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
>>
>> Retreive registers address and IRQ from device tree entry. Fall back
>> to built-in values if an error occurs.
>>
>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
>> [nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org: change error path and interrupts property handling]
>> Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>> ---
> [...]
>> diff --git a/arch/arm/mach-at91/at91sam926x_time.c 
>> b/arch/arm/mach-at91/at91sam926x_time.c
>> index d89ead7..5d4c308 100644
>> --- a/arch/arm/mach-at91/at91sam926x_time.c
>> +++ b/arch/arm/mach-at91/at91sam926x_time.c
>> @@ -14,6 +14,8 @@
> [...]
>> +static int __init of_at91sam926x_pit_init(void)
>> +{
>> +	struct device_node *np;
>> +
>> +	np = of_find_matching_node(NULL, timer_ids);
>> +	if (!np)
>> +		goto err;
>> +
>> +	pit_base_addr = of_iomap(np, 0);
>> +	if (!pit_base_addr)
>> +		goto node_err;
>> +
>> +	/* Get the interrupts property */
>> +	if (of_property_read_u32(np, "interrupts", &at91sam926x_pit_irq.irq))
>> +		goto ioremap_err;
> 
> I think you want:
> 
> 	at91sam926x_pit_irq.irq = irq_of_parse_and_map(np, 0);
> 
> to make sure the IRQ is converted to a Linux IRQ and not a hwirq.

Yes, that definitively makes sense. BTW, without a DT entry, how should
I map a hwirq, with the irq_domain_to_irq() helper? But I will need
access to the interrupt controller "struct irq_domain"...

> Other than that looks fine to me.

Thanks a lot for your review, Jamie.

Best regards,
-- 
Nicolas Ferre

Re: [PATCH v2] ARM: at91: pit add DT support

[Jamie Iles]

On Thu, Jan 05, 2012 at 03:50:09PM +0100, Nicolas Ferre wrote:
> On 01/05/2012 01:00 PM, Jamie Iles :
> > Hi Nicolas, Jean-Christophe,
> > 
> > On Thu, Jan 05, 2012 at 02:48:28PM +0100, Nicolas Ferre wrote:
> >> From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> >>
> >> Retreive registers address and IRQ from device tree entry. Fall back
> >> to built-in values if an error occurs.
> >>
> >> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> >> [nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org: change error path and interrupts property handling]
> >> Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> >> ---
> > [...]
> >> diff --git a/arch/arm/mach-at91/at91sam926x_time.c 
> >> b/arch/arm/mach-at91/at91sam926x_time.c
> >> index d89ead7..5d4c308 100644
> >> --- a/arch/arm/mach-at91/at91sam926x_time.c
> >> +++ b/arch/arm/mach-at91/at91sam926x_time.c
> >> @@ -14,6 +14,8 @@
> > [...]
> >> +static int __init of_at91sam926x_pit_init(void)
> >> +{
> >> +	struct device_node *np;
> >> +
> >> +	np = of_find_matching_node(NULL, timer_ids);
> >> +	if (!np)
> >> +		goto err;
> >> +
> >> +	pit_base_addr = of_iomap(np, 0);
> >> +	if (!pit_base_addr)
> >> +		goto node_err;
> >> +
> >> +	/* Get the interrupts property */
> >> +	if (of_property_read_u32(np, "interrupts", &at91sam926x_pit_irq.irq))
> >> +		goto ioremap_err;
> > 
> > I think you want:
> > 
> > 	at91sam926x_pit_irq.irq = irq_of_parse_and_map(np, 0);
> > 
> > to make sure the IRQ is converted to a Linux IRQ and not a hwirq.
> 
> Yes, that definitively makes sense. BTW, without a DT entry, how should
> I map a hwirq, with the irq_domain_to_irq() helper? But I will need
> access to the interrupt controller "struct irq_domain"...

If you aren't booting with DT then at the moment I think on AT91 you 
have statically assigned irq_descs and IRQ numbers so you don't need to 
map them, you just know what IRQ number to request at compile time.

Jamie

Re: [PATCH v3] ARM: at91: pit add DT support

[Jamie Iles]

On Thu, Jan 05, 2012 at 06:25:48PM +0100, Nicolas Ferre wrote:
> From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> 
> Retreive registers address and IRQ from device tree entry. Fall back
> to built-in values if an error occurs.
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> [nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org: change error path and interrupts property handling]
> Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

Reviewed-by: Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>

Jamie

Re: [PATCH v3] ARM: at91: pit add DT support

[Rob Herring]

On 01/05/2012 11:25 AM, Nicolas Ferre wrote:
> From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> 
> Retreive registers address and IRQ from device tree entry. Fall back
> to built-in values if an error occurs.
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> [nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org: change error path and interrupts property handling]
> Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> ---
> v3: - use irq_of_parse_and_map() for handling irq numbers specified by DT.
>       Correction proposed by Jamie Iles.
> 
> v2: - new specification of irq numbers in DT (due to modification of AIC code)
>     - new error path in of_at91sam926x_pit_init()
>     - fall back to built-in values if an error occurs
>     - use of of_property_read_u32() to get irq property
> 
>  .../devicetree/bindings/arm/atmel-at91.txt         |    8 +++
>  arch/arm/boot/dts/at91sam9g20.dtsi                 |    5 ++
>  arch/arm/boot/dts/at91sam9g45.dtsi                 |    6 ++
>  arch/arm/mach-at91/at91sam926x_time.c              |   53 +++++++++++++++++++-
>  4 files changed, 70 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/atmel-at91.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt b/Documentation/devicetree/bindings/arm/atmel-at91.txt
> new file mode 100644
> index 0000000..380f711
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt
> @@ -0,0 +1,8 @@
> +Atmel AT91 device tree bindings.
> +================================
> +
> +PIT Timer required properties:
> +- compatible: Should be "atmel,at91sam9260-pit"
> +- reg: Should contain registers location and length
> +- interrupts: Should contain interrupt for the PIT which is the IRQ line
> +  shared across all System Controller members.
> diff --git a/arch/arm/boot/dts/at91sam9g20.dtsi b/arch/arm/boot/dts/at91sam9g20.dtsi
> index 0d21872..5c4be7e 100644
> --- a/arch/arm/boot/dts/at91sam9g20.dtsi
> +++ b/arch/arm/boot/dts/at91sam9g20.dtsi
> @@ -57,6 +57,11 @@
>  				reg = <0xfffff000 0x200>;
>  			};
>  
> +			pit: timer@fffffd30 {
> +				compatible = "atmel,at91sam9260-pit";
> +				reg = <0xfffffd30 0xf>;
> +				interrupts = <1 4>;
> +			};
>  
>  			pioA: gpio@fffff400 {
>  				compatible = "atmel,at91rm9200-gpio";
> diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
> index b977a79..0d21c8e 100644
> --- a/arch/arm/boot/dts/at91sam9g45.dtsi
> +++ b/arch/arm/boot/dts/at91sam9g45.dtsi
> @@ -58,6 +58,12 @@
>  				reg = <0xfffff000 0x200>;
>  			};
>  
> +			pit: timer@fffffd30 {
> +				compatible = "atmel,at91sam9260-pit";
> +				reg = <0xfffffd30 0xf>;
> +				interrupts = <1 4>;
> +			};
> +
>  			dma: dma-controller@ffffec00 {
>  				compatible = "atmel,at91sam9g45-dma";
>  				reg = <0xffffec00 0x200>;
> diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c
> index d89ead7..534a992 100644
> --- a/arch/arm/mach-at91/at91sam926x_time.c
> +++ b/arch/arm/mach-at91/at91sam926x_time.c
> @@ -14,6 +14,9 @@
>  #include <linux/kernel.h>
>  #include <linux/clk.h>
>  #include <linux/clockchips.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
>  
>  #include <asm/mach/time.h>
>  
> @@ -133,7 +136,8 @@ static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
>  static struct irqaction at91sam926x_pit_irq = {
>  	.name		= "at91_tick",
>  	.flags		= IRQF_SHARED | IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
> -	.handler	= at91sam926x_pit_interrupt
> +	.handler	= at91sam926x_pit_interrupt,
> +	.irq		= AT91_ID_SYS,
>  };
>  
>  static void at91sam926x_pit_reset(void)
> @@ -149,6 +153,49 @@ static void at91sam926x_pit_reset(void)
>  	pit_write(AT91_PIT_MR, (pit_cycle - 1) | AT91_PIT_PITEN);
>  }
>  
> +#ifdef CONFIG_OF
> +static struct of_device_id timer_ids[] = {
> +	{ .compatible = "atmel,at91sam9260-pit" },
> +	{ /* sentinel */ }
> +};
> +
> +static int __init of_at91sam926x_pit_init(void)
> +{
> +	struct device_node	*np;
> +	int			ret;
> +
> +	np = of_find_matching_node(NULL, timer_ids);
> +	if (!np)
> +		goto err;
> +
> +	pit_base_addr = of_iomap(np, 0);
> +	if (!pit_base_addr)
> +		goto node_err;
> +
> +	/* Get the interrupts property */
> +	ret = irq_of_parse_and_map(np, 0);
> +	if (ret <= 0)
> +		goto ioremap_err;
> +	at91sam926x_pit_irq.irq = ret;
> +
> +	of_node_put(np);
> +
> +	return 0;
> +
> +ioremap_err:
> +	iounmap(pit_base_addr);
> +node_err:
> +	of_node_put(np);
> +err:
> +	return -EINVAL;
> +}
> +#else
> +static int __init of_at91sam926x_pit_init(void)
> +{
> +	return -EINVAL;
> +}
> +#endif
> +
>  /*
>   * Set up both clocksource and clockevent support.
>   */
> @@ -177,7 +224,7 @@ static void __init at91sam926x_pit_init(void)
>  	clocksource_register_hz(&pit_clk, pit_rate);
>  
>  	/* Set up irq handler */
> -	setup_irq(AT91_ID_SYS, &at91sam926x_pit_irq);
> +	setup_irq(at91sam926x_pit_irq.irq, &at91sam926x_pit_irq);
>  
>  	/* Set up and register clockevents */
>  	pit_clkevt.mult = div_sc(pit_rate, NSEC_PER_SEC, pit_clkevt.shift);
> @@ -193,6 +240,8 @@ static void at91sam926x_pit_suspend(void)
>  
>  void __init at91sam926x_ioremap_pit(u32 addr)
>  {
> +	if (!of_at91sam926x_pit_init())
> +		return;

This seems backwards to me. I don't have the ioremap changes in my tree,
but shouldn't the caller of at91sam926x_ioremap_pit be changed to
something like this:

	if (of_at91sam926x_pit_init() < 0)
		at91sam926x_ioremap_pit(addr);

Otherwise,

Acked-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>

Rob

>  	pit_base_addr = ioremap(addr, 16);
>  
>  	if (!pit_base_addr)

[PATCH v3] ARM: at91: pit add DT support

[Nicolas Ferre]

From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>

Retreive registers address and IRQ from device tree entry. Fall back
to built-in values if an error occurs.

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
[nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org: change error path and interrupts property handling]
Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
---
v3: - use irq_of_parse_and_map() for handling irq numbers specified by DT.
      Correction proposed by Jamie Iles.

v2: - new specification of irq numbers in DT (due to modification of AIC code)
    - new error path in of_at91sam926x_pit_init()
    - fall back to built-in values if an error occurs
    - use of of_property_read_u32() to get irq property

 .../devicetree/bindings/arm/atmel-at91.txt         |    8 +++
 arch/arm/boot/dts/at91sam9g20.dtsi                 |    5 ++
 arch/arm/boot/dts/at91sam9g45.dtsi                 |    6 ++
 arch/arm/mach-at91/at91sam926x_time.c              |   53 +++++++++++++++++++-
 4 files changed, 70 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/atmel-at91.txt

diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt b/Documentation/devicetree/bindings/arm/atmel-at91.txt
new file mode 100644
index 0000000..380f711
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt
@@ -0,0 +1,8 @@
+Atmel AT91 device tree bindings.
+================================
+
+PIT Timer required properties:
+- compatible: Should be "atmel,at91sam9260-pit"
+- reg: Should contain registers location and length
+- interrupts: Should contain interrupt for the PIT which is the IRQ line
+  shared across all System Controller members.
diff --git a/arch/arm/boot/dts/at91sam9g20.dtsi b/arch/arm/boot/dts/at91sam9g20.dtsi
index 0d21872..5c4be7e 100644
--- a/arch/arm/boot/dts/at91sam9g20.dtsi
+++ b/arch/arm/boot/dts/at91sam9g20.dtsi
@@ -57,6 +57,11 @@
 				reg = <0xfffff000 0x200>;
 			};
 
+			pit: timer@fffffd30 {
+				compatible = "atmel,at91sam9260-pit";
+				reg = <0xfffffd30 0xf>;
+				interrupts = <1 4>;
+			};
 
 			pioA: gpio@fffff400 {
 				compatible = "atmel,at91rm9200-gpio";
diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
index b977a79..0d21c8e 100644
--- a/arch/arm/boot/dts/at91sam9g45.dtsi
+++ b/arch/arm/boot/dts/at91sam9g45.dtsi
@@ -58,6 +58,12 @@
 				reg = <0xfffff000 0x200>;
 			};
 
+			pit: timer@fffffd30 {
+				compatible = "atmel,at91sam9260-pit";
+				reg = <0xfffffd30 0xf>;
+				interrupts = <1 4>;
+			};
+
 			dma: dma-controller@ffffec00 {
 				compatible = "atmel,at91sam9g45-dma";
 				reg = <0xffffec00 0x200>;
diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c
index d89ead7..534a992 100644
--- a/arch/arm/mach-at91/at91sam926x_time.c
+++ b/arch/arm/mach-at91/at91sam926x_time.c
@@ -14,6 +14,9 @@
 #include <linux/kernel.h>
 #include <linux/clk.h>
 #include <linux/clockchips.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
 
 #include <asm/mach/time.h>
 
@@ -133,7 +136,8 @@ static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
 static struct irqaction at91sam926x_pit_irq = {
 	.name		= "at91_tick",
 	.flags		= IRQF_SHARED | IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
-	.handler	= at91sam926x_pit_interrupt
+	.handler	= at91sam926x_pit_interrupt,
+	.irq		= AT91_ID_SYS,
 };
 
 static void at91sam926x_pit_reset(void)
@@ -149,6 +153,49 @@ static void at91sam926x_pit_reset(void)
 	pit_write(AT91_PIT_MR, (pit_cycle - 1) | AT91_PIT_PITEN);
 }
 
+#ifdef CONFIG_OF
+static struct of_device_id timer_ids[] = {
+	{ .compatible = "atmel,at91sam9260-pit" },
+	{ /* sentinel */ }
+};
+
+static int __init of_at91sam926x_pit_init(void)
+{
+	struct device_node	*np;
+	int			ret;
+
+	np = of_find_matching_node(NULL, timer_ids);
+	if (!np)
+		goto err;
+
+	pit_base_addr = of_iomap(np, 0);
+	if (!pit_base_addr)
+		goto node_err;
+
+	/* Get the interrupts property */
+	ret = irq_of_parse_and_map(np, 0);
+	if (ret <= 0)
+		goto ioremap_err;
+	at91sam926x_pit_irq.irq = ret;
+
+	of_node_put(np);
+
+	return 0;
+
+ioremap_err:
+	iounmap(pit_base_addr);
+node_err:
+	of_node_put(np);
+err:
+	return -EINVAL;
+}
+#else
+static int __init of_at91sam926x_pit_init(void)
+{
+	return -EINVAL;
+}
+#endif
+
 /*
  * Set up both clocksource and clockevent support.
  */
@@ -177,7 +224,7 @@ static void __init at91sam926x_pit_init(void)
 	clocksource_register_hz(&pit_clk, pit_rate);
 
 	/* Set up irq handler */
-	setup_irq(AT91_ID_SYS, &at91sam926x_pit_irq);
+	setup_irq(at91sam926x_pit_irq.irq, &at91sam926x_pit_irq);
 
 	/* Set up and register clockevents */
 	pit_clkevt.mult = div_sc(pit_rate, NSEC_PER_SEC, pit_clkevt.shift);
@@ -193,6 +240,8 @@ static void at91sam926x_pit_suspend(void)
 
 void __init at91sam926x_ioremap_pit(u32 addr)
 {
+	if (!of_at91sam926x_pit_init())
+		return;
 	pit_base_addr = ioremap(addr, 16);
 
 	if (!pit_base_addr)
-- 
1.7.5.4

Re: [PATCH v3] ARM: at91: pit add DT support

[Grant Likely]

On Thu, Jan 05, 2012 at 06:25:48PM +0100, Nicolas Ferre wrote:
> From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> 
> Retreive registers address and IRQ from device tree entry. Fall back
> to built-in values if an error occurs.
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> [nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org: change error path and interrupts property handling]
> Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> ---
> v3: - use irq_of_parse_and_map() for handling irq numbers specified by DT.
>       Correction proposed by Jamie Iles.
> 
> v2: - new specification of irq numbers in DT (due to modification of AIC code)
>     - new error path in of_at91sam926x_pit_init()
>     - fall back to built-in values if an error occurs
>     - use of of_property_read_u32() to get irq property
> 
>  .../devicetree/bindings/arm/atmel-at91.txt         |    8 +++
>  arch/arm/boot/dts/at91sam9g20.dtsi                 |    5 ++
>  arch/arm/boot/dts/at91sam9g45.dtsi                 |    6 ++
>  arch/arm/mach-at91/at91sam926x_time.c              |   53 +++++++++++++++++++-
>  4 files changed, 70 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/atmel-at91.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt b/Documentation/devicetree/bindings/arm/atmel-at91.txt
> new file mode 100644
> index 0000000..380f711
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt
> @@ -0,0 +1,8 @@
> +Atmel AT91 device tree bindings.
> +================================
> +
> +PIT Timer required properties:
> +- compatible: Should be "atmel,at91sam9260-pit"
> +- reg: Should contain registers location and length
> +- interrupts: Should contain interrupt for the PIT which is the IRQ line
> +  shared across all System Controller members.
> diff --git a/arch/arm/boot/dts/at91sam9g20.dtsi b/arch/arm/boot/dts/at91sam9g20.dtsi
> index 0d21872..5c4be7e 100644
> --- a/arch/arm/boot/dts/at91sam9g20.dtsi
> +++ b/arch/arm/boot/dts/at91sam9g20.dtsi
> @@ -57,6 +57,11 @@
>  				reg = <0xfffff000 0x200>;
>  			};
>  
> +			pit: timer@fffffd30 {
> +				compatible = "atmel,at91sam9260-pit";
> +				reg = <0xfffffd30 0xf>;
> +				interrupts = <1 4>;
> +			};
>  
>  			pioA: gpio@fffff400 {
>  				compatible = "atmel,at91rm9200-gpio";
> diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
> index b977a79..0d21c8e 100644
> --- a/arch/arm/boot/dts/at91sam9g45.dtsi
> +++ b/arch/arm/boot/dts/at91sam9g45.dtsi
> @@ -58,6 +58,12 @@
>  				reg = <0xfffff000 0x200>;
>  			};
>  
> +			pit: timer@fffffd30 {
> +				compatible = "atmel,at91sam9260-pit";
> +				reg = <0xfffffd30 0xf>;
> +				interrupts = <1 4>;
> +			};
> +
>  			dma: dma-controller@ffffec00 {
>  				compatible = "atmel,at91sam9g45-dma";
>  				reg = <0xffffec00 0x200>;
> diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c
> index d89ead7..534a992 100644
> --- a/arch/arm/mach-at91/at91sam926x_time.c
> +++ b/arch/arm/mach-at91/at91sam926x_time.c
> @@ -14,6 +14,9 @@
>  #include <linux/kernel.h>
>  #include <linux/clk.h>
>  #include <linux/clockchips.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
>  
>  #include <asm/mach/time.h>
>  
> @@ -133,7 +136,8 @@ static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
>  static struct irqaction at91sam926x_pit_irq = {
>  	.name		= "at91_tick",
>  	.flags		= IRQF_SHARED | IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
> -	.handler	= at91sam926x_pit_interrupt
> +	.handler	= at91sam926x_pit_interrupt,
> +	.irq		= AT91_ID_SYS,
>  };
>  
>  static void at91sam926x_pit_reset(void)
> @@ -149,6 +153,49 @@ static void at91sam926x_pit_reset(void)
>  	pit_write(AT91_PIT_MR, (pit_cycle - 1) | AT91_PIT_PITEN);
>  }
>  
> +#ifdef CONFIG_OF
> +static struct of_device_id timer_ids[] = {
> +	{ .compatible = "atmel,at91sam9260-pit" },
> +	{ /* sentinel */ }
> +};
> +
> +static int __init of_at91sam926x_pit_init(void)
> +{
> +	struct device_node	*np;
> +	int			ret;
> +
> +	np = of_find_matching_node(NULL, timer_ids);
> +	if (!np)
> +		goto err;
> +
> +	pit_base_addr = of_iomap(np, 0);
> +	if (!pit_base_addr)
> +		goto node_err;
> +
> +	/* Get the interrupts property */
> +	ret = irq_of_parse_and_map(np, 0);
> +	if (ret <= 0)
> +		goto ioremap_err;

if (!ret)

The DT irq functions return 0 on failure.

g.

Re: [PATCH v3] ARM: at91: pit add DT support

[Nicolas Ferre]

On 01/05/2012 05:42 PM, Rob Herring :
> On 01/05/2012 11:25 AM, Nicolas Ferre wrote:
>> From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
>>
>> Retreive registers address and IRQ from device tree entry. Fall back
>> to built-in values if an error occurs.
>>
>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
>> [nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org: change error path and interrupts property handling]
>> Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>> ---
>> v3: - use irq_of_parse_and_map() for handling irq numbers specified by DT.
>>       Correction proposed by Jamie Iles.
>>
>> v2: - new specification of irq numbers in DT (due to modification of AIC code)
>>     - new error path in of_at91sam926x_pit_init()
>>     - fall back to built-in values if an error occurs
>>     - use of of_property_read_u32() to get irq property
>>
>>  .../devicetree/bindings/arm/atmel-at91.txt         |    8 +++
>>  arch/arm/boot/dts/at91sam9g20.dtsi                 |    5 ++
>>  arch/arm/boot/dts/at91sam9g45.dtsi                 |    6 ++
>>  arch/arm/mach-at91/at91sam926x_time.c              |   53 +++++++++++++++++++-
>>  4 files changed, 70 insertions(+), 2 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/arm/atmel-at91.txt

[..]

>> --- a/arch/arm/mach-at91/at91sam926x_time.c
>> +++ b/arch/arm/mach-at91/at91sam926x_time.c

[..]

>>  void __init at91sam926x_ioremap_pit(u32 addr)
>>  {
>> +	if (!of_at91sam926x_pit_init())
>> +		return;
> 
> This seems backwards to me. I don't have the ioremap changes in my tree,
> but shouldn't the caller of at91sam926x_ioremap_pit be changed to
> something like this:
> 
> 	if (of_at91sam926x_pit_init() < 0)
> 		at91sam926x_ioremap_pit(addr);

Yes, you are right, it will be more readable the other way around.

I repost another revision now.

> Otherwise,
> Acked-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>

Thanks for your review.

Best regards,
-- 
Nicolas Ferre

Re: [PATCH v3] ARM: at91: pit add DT support

[Nicolas Ferre]

On 01/05/2012 07:00 PM, Grant Likely :
> On Thu, Jan 05, 2012 at 06:25:48PM +0100, Nicolas Ferre wrote:
>> From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
>>
>> Retreive registers address and IRQ from device tree entry. Fall back
>> to built-in values if an error occurs.
>>
>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
>> [nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org: change error path and interrupts property handling]
>> Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>> ---
>> v3: - use irq_of_parse_and_map() for handling irq numbers specified by DT.
>>       Correction proposed by Jamie Iles.
>>
>> v2: - new specification of irq numbers in DT (due to modification of AIC code)
>>     - new error path in of_at91sam926x_pit_init()
>>     - fall back to built-in values if an error occurs
>>     - use of of_property_read_u32() to get irq property
>>
>>  .../devicetree/bindings/arm/atmel-at91.txt         |    8 +++
>>  arch/arm/boot/dts/at91sam9g20.dtsi                 |    5 ++
>>  arch/arm/boot/dts/at91sam9g45.dtsi                 |    6 ++
>>  arch/arm/mach-at91/at91sam926x_time.c              |   53 +++++++++++++++++++-
>>  4 files changed, 70 insertions(+), 2 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/arm/atmel-at91.txt

[..]

>> --- a/arch/arm/mach-at91/at91sam926x_time.c
>> +++ b/arch/arm/mach-at91/at91sam926x_time.c

[..]

>> +	/* Get the interrupts property */
>> +	ret = irq_of_parse_and_map(np, 0);
>> +	if (ret <= 0)
>> +		goto ioremap_err;
> 
> if (!ret)
> 
> The DT irq functions return 0 on failure.

Ok, modified in new revision.

Best regards,
-- 
Nicolas Ferre

Re: [PATCH v4] ARM: at91: pit add DT support

[Rob Herring]

On 01/06/2012 10:20 AM, Nicolas Ferre wrote:
> From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> 
> Retreive registers address and IRQ from device tree entry. Fall back
> to built-in values if an error occurs.
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> [nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org: change error path and interrupts property handling]
> Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>
> Acked-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> ---
> v4: - change of_at91sam926x_pit_init() return value usage logic as
>       suggested by Rob Herring
>     - irq_of_parse_and_map() returns 0 on error: change test according to 
>       Grant Likely note.
> 
> v3: - use irq_of_parse_and_map() for handling irq numbers specified by DT.
>       Correction proposed by Jamie Iles.
> 
> v2: - new specification of irq numbers in DT (due to modification of AIC code)
>     - new error path in of_at91sam926x_pit_init()
>     - fall back to built-in values if an error occurs
>     - use of of_property_read_u32() to get irq property
> 
>  .../devicetree/bindings/arm/atmel-at91.txt         |    8 +++
>  arch/arm/boot/dts/at91sam9g20.dtsi                 |    5 ++
>  arch/arm/boot/dts/at91sam9g45.dtsi                 |    6 ++
>  arch/arm/mach-at91/at91sam926x_time.c              |   60 ++++++++++++++++++--
>  4 files changed, 73 insertions(+), 6 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/atmel-at91.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt b/Documentation/devicetree/bindings/arm/atmel-at91.txt
> new file mode 100644
> index 0000000..380f711
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt
> @@ -0,0 +1,8 @@
> +Atmel AT91 device tree bindings.
> +================================
> +
> +PIT Timer required properties:
> +- compatible: Should be "atmel,at91sam9260-pit"
> +- reg: Should contain registers location and length
> +- interrupts: Should contain interrupt for the PIT which is the IRQ line
> +  shared across all System Controller members.
> diff --git a/arch/arm/boot/dts/at91sam9g20.dtsi b/arch/arm/boot/dts/at91sam9g20.dtsi
> index ea942b5..e10842a 100644
> --- a/arch/arm/boot/dts/at91sam9g20.dtsi
> +++ b/arch/arm/boot/dts/at91sam9g20.dtsi
> @@ -57,6 +57,11 @@
>  				reg = <0xfffff000 0x200>;
>  			};
>  
> +			pit: timer@fffffd30 {
> +				compatible = "atmel,at91sam9260-pit";
> +				reg = <0xfffffd30 0xf>;
> +				interrupts = <1 4>;
> +			};
>  
>  			pioA: gpio@fffff400 {
>  				compatible = "atmel,at91rm9200-gpio";
> diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
> index ebc9617..28a678f 100644
> --- a/arch/arm/boot/dts/at91sam9g45.dtsi
> +++ b/arch/arm/boot/dts/at91sam9g45.dtsi
> @@ -58,6 +58,12 @@
>  				reg = <0xfffff000 0x200>;
>  			};
>  
> +			pit: timer@fffffd30 {
> +				compatible = "atmel,at91sam9260-pit";
> +				reg = <0xfffffd30 0xf>;
> +				interrupts = <1 4>;
> +			};
> +
>  			dma: dma-controller@ffffec00 {
>  				compatible = "atmel,at91sam9g45-dma";
>  				reg = <0xffffec00 0x200>;
> diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c
> index d89ead7..802fea3 100644
> --- a/arch/arm/mach-at91/at91sam926x_time.c
> +++ b/arch/arm/mach-at91/at91sam926x_time.c
> @@ -14,6 +14,9 @@
>  #include <linux/kernel.h>
>  #include <linux/clk.h>
>  #include <linux/clockchips.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
>  
>  #include <asm/mach/time.h>
>  
> @@ -133,7 +136,8 @@ static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
>  static struct irqaction at91sam926x_pit_irq = {
>  	.name		= "at91_tick",
>  	.flags		= IRQF_SHARED | IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
> -	.handler	= at91sam926x_pit_interrupt
> +	.handler	= at91sam926x_pit_interrupt,
> +	.irq		= AT91_ID_SYS,
>  };
>  
>  static void at91sam926x_pit_reset(void)
> @@ -149,6 +153,49 @@ static void at91sam926x_pit_reset(void)
>  	pit_write(AT91_PIT_MR, (pit_cycle - 1) | AT91_PIT_PITEN);
>  }
>  
> +#ifdef CONFIG_OF
> +static struct of_device_id timer_ids[] = {
> +	{ .compatible = "atmel,at91sam9260-pit" },
> +	{ /* sentinel */ }
> +};
> +
> +static int __init of_at91sam926x_pit_init(void)
> +{
> +	struct device_node	*np;
> +	int			ret;
> +
> +	np = of_find_matching_node(NULL, timer_ids);
> +	if (!np)
> +		goto err;
> +
> +	pit_base_addr = of_iomap(np, 0);
> +	if (!pit_base_addr)
> +		goto node_err;
> +
> +	/* Get the interrupts property */
> +	ret = irq_of_parse_and_map(np, 0);
> +	if (!ret)
> +		goto ioremap_err;
> +	at91sam926x_pit_irq.irq = ret;
> +
> +	of_node_put(np);
> +
> +	return 0;
> +
> +ioremap_err:
> +	iounmap(pit_base_addr);
> +node_err:
> +	of_node_put(np);
> +err:
> +	return -EINVAL;
> +}
> +#else
> +static int __init of_at91sam926x_pit_init(void)
> +{
> +	return -EINVAL;
> +}
> +#endif
> +
>  /*
>   * Set up both clocksource and clockevent support.
>   */
> @@ -177,7 +224,7 @@ static void __init at91sam926x_pit_init(void)
>  	clocksource_register_hz(&pit_clk, pit_rate);
>  
>  	/* Set up irq handler */
> -	setup_irq(AT91_ID_SYS, &at91sam926x_pit_irq);
> +	setup_irq(at91sam926x_pit_irq.irq, &at91sam926x_pit_irq);
>  
>  	/* Set up and register clockevents */
>  	pit_clkevt.mult = div_sc(pit_rate, NSEC_PER_SEC, pit_clkevt.shift);
> @@ -193,10 +240,11 @@ static void at91sam926x_pit_suspend(void)
>  
>  void __init at91sam926x_ioremap_pit(u32 addr)
>  {
> -	pit_base_addr = ioremap(addr, 16);
> -
> -	if (!pit_base_addr)
> -		panic("Impossible to ioremap PIT\n");
> +	if (of_at91sam926x_pit_init() < 0) {
> +		pit_base_addr = ioremap(addr, 16);
> +		if (!pit_base_addr)
> +			panic("Impossible to ioremap PIT\n");
> +	}

This is not what I meant. I meant call either at91sam926x_ioremap_pit or
of_at91sam926x_pit_init. Don't nest the calls and keep the OF and non-OF
init somewhat separate. The fact that you are passing in the physical
address and then ignoring it for the OF case is what I have issue with.

Rob

>  }
>  
>  struct sys_timer at91sam926x_timer = {

[PATCH v4] ARM: at91: pit add DT support

[Nicolas Ferre]

From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>

Retreive registers address and IRQ from device tree entry. Fall back
to built-in values if an error occurs.

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
[nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org: change error path and interrupts property handling]
Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
Reviewed-by: Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>
Acked-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
---
v4: - change of_at91sam926x_pit_init() return value usage logic as
      suggested by Rob Herring
    - irq_of_parse_and_map() returns 0 on error: change test according to 
      Grant Likely note.

v3: - use irq_of_parse_and_map() for handling irq numbers specified by DT.
      Correction proposed by Jamie Iles.

v2: - new specification of irq numbers in DT (due to modification of AIC code)
    - new error path in of_at91sam926x_pit_init()
    - fall back to built-in values if an error occurs
    - use of of_property_read_u32() to get irq property

 .../devicetree/bindings/arm/atmel-at91.txt         |    8 +++
 arch/arm/boot/dts/at91sam9g20.dtsi                 |    5 ++
 arch/arm/boot/dts/at91sam9g45.dtsi                 |    6 ++
 arch/arm/mach-at91/at91sam926x_time.c              |   60 ++++++++++++++++++--
 4 files changed, 73 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/atmel-at91.txt

diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt b/Documentation/devicetree/bindings/arm/atmel-at91.txt
new file mode 100644
index 0000000..380f711
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt
@@ -0,0 +1,8 @@
+Atmel AT91 device tree bindings.
+================================
+
+PIT Timer required properties:
+- compatible: Should be "atmel,at91sam9260-pit"
+- reg: Should contain registers location and length
+- interrupts: Should contain interrupt for the PIT which is the IRQ line
+  shared across all System Controller members.
diff --git a/arch/arm/boot/dts/at91sam9g20.dtsi b/arch/arm/boot/dts/at91sam9g20.dtsi
index ea942b5..e10842a 100644
--- a/arch/arm/boot/dts/at91sam9g20.dtsi
+++ b/arch/arm/boot/dts/at91sam9g20.dtsi
@@ -57,6 +57,11 @@
 				reg = <0xfffff000 0x200>;
 			};
 
+			pit: timer@fffffd30 {
+				compatible = "atmel,at91sam9260-pit";
+				reg = <0xfffffd30 0xf>;
+				interrupts = <1 4>;
+			};
 
 			pioA: gpio@fffff400 {
 				compatible = "atmel,at91rm9200-gpio";
diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
index ebc9617..28a678f 100644
--- a/arch/arm/boot/dts/at91sam9g45.dtsi
+++ b/arch/arm/boot/dts/at91sam9g45.dtsi
@@ -58,6 +58,12 @@
 				reg = <0xfffff000 0x200>;
 			};
 
+			pit: timer@fffffd30 {
+				compatible = "atmel,at91sam9260-pit";
+				reg = <0xfffffd30 0xf>;
+				interrupts = <1 4>;
+			};
+
 			dma: dma-controller@ffffec00 {
 				compatible = "atmel,at91sam9g45-dma";
 				reg = <0xffffec00 0x200>;
diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c
index d89ead7..802fea3 100644
--- a/arch/arm/mach-at91/at91sam926x_time.c
+++ b/arch/arm/mach-at91/at91sam926x_time.c
@@ -14,6 +14,9 @@
 #include <linux/kernel.h>
 #include <linux/clk.h>
 #include <linux/clockchips.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
 
 #include <asm/mach/time.h>
 
@@ -133,7 +136,8 @@ static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
 static struct irqaction at91sam926x_pit_irq = {
 	.name		= "at91_tick",
 	.flags		= IRQF_SHARED | IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
-	.handler	= at91sam926x_pit_interrupt
+	.handler	= at91sam926x_pit_interrupt,
+	.irq		= AT91_ID_SYS,
 };
 
 static void at91sam926x_pit_reset(void)
@@ -149,6 +153,49 @@ static void at91sam926x_pit_reset(void)
 	pit_write(AT91_PIT_MR, (pit_cycle - 1) | AT91_PIT_PITEN);
 }
 
+#ifdef CONFIG_OF
+static struct of_device_id timer_ids[] = {
+	{ .compatible = "atmel,at91sam9260-pit" },
+	{ /* sentinel */ }
+};
+
+static int __init of_at91sam926x_pit_init(void)
+{
+	struct device_node	*np;
+	int			ret;
+
+	np = of_find_matching_node(NULL, timer_ids);
+	if (!np)
+		goto err;
+
+	pit_base_addr = of_iomap(np, 0);
+	if (!pit_base_addr)
+		goto node_err;
+
+	/* Get the interrupts property */
+	ret = irq_of_parse_and_map(np, 0);
+	if (!ret)
+		goto ioremap_err;
+	at91sam926x_pit_irq.irq = ret;
+
+	of_node_put(np);
+
+	return 0;
+
+ioremap_err:
+	iounmap(pit_base_addr);
+node_err:
+	of_node_put(np);
+err:
+	return -EINVAL;
+}
+#else
+static int __init of_at91sam926x_pit_init(void)
+{
+	return -EINVAL;
+}
+#endif
+
 /*
  * Set up both clocksource and clockevent support.
  */
@@ -177,7 +224,7 @@ static void __init at91sam926x_pit_init(void)
 	clocksource_register_hz(&pit_clk, pit_rate);
 
 	/* Set up irq handler */
-	setup_irq(AT91_ID_SYS, &at91sam926x_pit_irq);
+	setup_irq(at91sam926x_pit_irq.irq, &at91sam926x_pit_irq);
 
 	/* Set up and register clockevents */
 	pit_clkevt.mult = div_sc(pit_rate, NSEC_PER_SEC, pit_clkevt.shift);
@@ -193,10 +240,11 @@ static void at91sam926x_pit_suspend(void)
 
 void __init at91sam926x_ioremap_pit(u32 addr)
 {
-	pit_base_addr = ioremap(addr, 16);
-
-	if (!pit_base_addr)
-		panic("Impossible to ioremap PIT\n");
+	if (of_at91sam926x_pit_init() < 0) {
+		pit_base_addr = ioremap(addr, 16);
+		if (!pit_base_addr)
+			panic("Impossible to ioremap PIT\n");
+	}
 }
 
 struct sys_timer at91sam926x_timer = {
-- 
1.7.5.4

Re: [PATCH v4] ARM: at91: pit add DT support

[Nicolas Ferre]

On 01/06/2012 04:47 PM, Rob Herring :
> On 01/06/2012 10:20 AM, Nicolas Ferre wrote:
>> From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
>>
>> Retreive registers address and IRQ from device tree entry. Fall back
>> to built-in values if an error occurs.
>>
>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
>> [nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org: change error path and interrupts property handling]
>> Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>> Reviewed-by: Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>
>> Acked-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>> ---
>> v4: - change of_at91sam926x_pit_init() return value usage logic as
>>       suggested by Rob Herring
>>     - irq_of_parse_and_map() returns 0 on error: change test according to 
>>       Grant Likely note.
>>
>> v3: - use irq_of_parse_and_map() for handling irq numbers specified by DT.
>>       Correction proposed by Jamie Iles.
>>
>> v2: - new specification of irq numbers in DT (due to modification of AIC code)
>>     - new error path in of_at91sam926x_pit_init()
>>     - fall back to built-in values if an error occurs
>>     - use of of_property_read_u32() to get irq property
>>
>>  .../devicetree/bindings/arm/atmel-at91.txt         |    8 +++
>>  arch/arm/boot/dts/at91sam9g20.dtsi                 |    5 ++
>>  arch/arm/boot/dts/at91sam9g45.dtsi                 |    6 ++
>>  arch/arm/mach-at91/at91sam926x_time.c              |   60 ++++++++++++++++++--
>>  4 files changed, 73 insertions(+), 6 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/arm/atmel-at91.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt b/Documentation/devicetree/bindings/arm/atmel-at91.txt
>> new file mode 100644
>> index 0000000..380f711
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt
>> @@ -0,0 +1,8 @@
>> +Atmel AT91 device tree bindings.
>> +================================
>> +
>> +PIT Timer required properties:
>> +- compatible: Should be "atmel,at91sam9260-pit"
>> +- reg: Should contain registers location and length
>> +- interrupts: Should contain interrupt for the PIT which is the IRQ line
>> +  shared across all System Controller members.
>> diff --git a/arch/arm/boot/dts/at91sam9g20.dtsi b/arch/arm/boot/dts/at91sam9g20.dtsi
>> index ea942b5..e10842a 100644
>> --- a/arch/arm/boot/dts/at91sam9g20.dtsi
>> +++ b/arch/arm/boot/dts/at91sam9g20.dtsi
>> @@ -57,6 +57,11 @@
>>  				reg = <0xfffff000 0x200>;
>>  			};
>>  
>> +			pit: timer@fffffd30 {
>> +				compatible = "atmel,at91sam9260-pit";
>> +				reg = <0xfffffd30 0xf>;
>> +				interrupts = <1 4>;
>> +			};
>>  
>>  			pioA: gpio@fffff400 {
>>  				compatible = "atmel,at91rm9200-gpio";
>> diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
>> index ebc9617..28a678f 100644
>> --- a/arch/arm/boot/dts/at91sam9g45.dtsi
>> +++ b/arch/arm/boot/dts/at91sam9g45.dtsi
>> @@ -58,6 +58,12 @@
>>  				reg = <0xfffff000 0x200>;
>>  			};
>>  
>> +			pit: timer@fffffd30 {
>> +				compatible = "atmel,at91sam9260-pit";
>> +				reg = <0xfffffd30 0xf>;
>> +				interrupts = <1 4>;
>> +			};
>> +
>>  			dma: dma-controller@ffffec00 {
>>  				compatible = "atmel,at91sam9g45-dma";
>>  				reg = <0xffffec00 0x200>;
>> diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c
>> index d89ead7..802fea3 100644
>> --- a/arch/arm/mach-at91/at91sam926x_time.c
>> +++ b/arch/arm/mach-at91/at91sam926x_time.c
>> @@ -14,6 +14,9 @@
>>  #include <linux/kernel.h>
>>  #include <linux/clk.h>
>>  #include <linux/clockchips.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>>  
>>  #include <asm/mach/time.h>
>>  
>> @@ -133,7 +136,8 @@ static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
>>  static struct irqaction at91sam926x_pit_irq = {
>>  	.name		= "at91_tick",
>>  	.flags		= IRQF_SHARED | IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
>> -	.handler	= at91sam926x_pit_interrupt
>> +	.handler	= at91sam926x_pit_interrupt,
>> +	.irq		= AT91_ID_SYS,
>>  };
>>  
>>  static void at91sam926x_pit_reset(void)
>> @@ -149,6 +153,49 @@ static void at91sam926x_pit_reset(void)
>>  	pit_write(AT91_PIT_MR, (pit_cycle - 1) | AT91_PIT_PITEN);
>>  }
>>  
>> +#ifdef CONFIG_OF
>> +static struct of_device_id timer_ids[] = {
>> +	{ .compatible = "atmel,at91sam9260-pit" },
>> +	{ /* sentinel */ }
>> +};
>> +
>> +static int __init of_at91sam926x_pit_init(void)
>> +{
>> +	struct device_node	*np;
>> +	int			ret;
>> +
>> +	np = of_find_matching_node(NULL, timer_ids);
>> +	if (!np)
>> +		goto err;
>> +
>> +	pit_base_addr = of_iomap(np, 0);
>> +	if (!pit_base_addr)
>> +		goto node_err;
>> +
>> +	/* Get the interrupts property */
>> +	ret = irq_of_parse_and_map(np, 0);
>> +	if (!ret)
>> +		goto ioremap_err;
>> +	at91sam926x_pit_irq.irq = ret;
>> +
>> +	of_node_put(np);
>> +
>> +	return 0;
>> +
>> +ioremap_err:
>> +	iounmap(pit_base_addr);
>> +node_err:
>> +	of_node_put(np);
>> +err:
>> +	return -EINVAL;
>> +}
>> +#else
>> +static int __init of_at91sam926x_pit_init(void)
>> +{
>> +	return -EINVAL;
>> +}
>> +#endif
>> +
>>  /*
>>   * Set up both clocksource and clockevent support.
>>   */
>> @@ -177,7 +224,7 @@ static void __init at91sam926x_pit_init(void)
>>  	clocksource_register_hz(&pit_clk, pit_rate);
>>  
>>  	/* Set up irq handler */
>> -	setup_irq(AT91_ID_SYS, &at91sam926x_pit_irq);
>> +	setup_irq(at91sam926x_pit_irq.irq, &at91sam926x_pit_irq);
>>  
>>  	/* Set up and register clockevents */
>>  	pit_clkevt.mult = div_sc(pit_rate, NSEC_PER_SEC, pit_clkevt.shift);
>> @@ -193,10 +240,11 @@ static void at91sam926x_pit_suspend(void)
>>  
>>  void __init at91sam926x_ioremap_pit(u32 addr)
>>  {
>> -	pit_base_addr = ioremap(addr, 16);
>> -
>> -	if (!pit_base_addr)
>> -		panic("Impossible to ioremap PIT\n");
>> +	if (of_at91sam926x_pit_init() < 0) {
>> +		pit_base_addr = ioremap(addr, 16);
>> +		if (!pit_base_addr)
>> +			panic("Impossible to ioremap PIT\n");
>> +	}
> 
> This is not what I meant. I meant call either at91sam926x_ioremap_pit or
> of_at91sam926x_pit_init. Don't nest the calls and keep the OF and non-OF
> init somewhat separate. The fact that you are passing in the physical
> address and then ignoring it for the OF case is what I have issue with.

I see...

I did not understand this way because I was so obsessed by the fact to
not introduce another interface to the PIT.

But it is true that as our future SoCs will only rely on DT to get PIT
data, we will not have the knowledge of its physical address: this is
speaking in favor of what you are saying -> I should keep those init
separated and called from the <soc_name>_ioremap_registers() functions.

Jean-Christophe, do you agree with this? If yes, I have already the
patch ready and will send a v5 of this one...

Bye,
-- 
Nicolas Ferre

Re: [PATCH v4] ARM: at91: pit add DT support

[Jean-Christophe PLAGNIOL-VILLARD]

On 09:47 Fri 06 Jan     , Rob Herring wrote:
> On 01/06/2012 10:20 AM, Nicolas Ferre wrote:
> > From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > 
> > Retreive registers address and IRQ from device tree entry. Fall back
> > to built-in values if an error occurs.
> > 
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > [nicolas.ferre@atmel.com: change error path and interrupts property handling]
> > Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> > Reviewed-by: Jamie Iles <jamie@jamieiles.com>
> > Acked-by: Rob Herring <rob.herring@calxeda.com>
> > ---
> > v4: - change of_at91sam926x_pit_init() return value usage logic as
> >       suggested by Rob Herring
> >     - irq_of_parse_and_map() returns 0 on error: change test according to 
> >       Grant Likely note.
> > 
> > v3: - use irq_of_parse_and_map() for handling irq numbers specified by DT.
> >       Correction proposed by Jamie Iles.
> > 
> > v2: - new specification of irq numbers in DT (due to modification of AIC code)
> >     - new error path in of_at91sam926x_pit_init()
> >     - fall back to built-in values if an error occurs
> >     - use of of_property_read_u32() to get irq property
> > 
> >  .../devicetree/bindings/arm/atmel-at91.txt         |    8 +++
> >  arch/arm/boot/dts/at91sam9g20.dtsi                 |    5 ++
> >  arch/arm/boot/dts/at91sam9g45.dtsi                 |    6 ++
> >  arch/arm/mach-at91/at91sam926x_time.c              |   60 ++++++++++++++++++--
> >  4 files changed, 73 insertions(+), 6 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/arm/atmel-at91.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt b/Documentation/devicetree/bindings/arm/atmel-at91.txt
> > new file mode 100644
> > index 0000000..380f711
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt
> > @@ -0,0 +1,8 @@
> > +Atmel AT91 device tree bindings.
> > +================================
> > +
> > +PIT Timer required properties:
> > +- compatible: Should be "atmel,at91sam9260-pit"
> > +- reg: Should contain registers location and length
> > +- interrupts: Should contain interrupt for the PIT which is the IRQ line
> > +  shared across all System Controller members.
> > diff --git a/arch/arm/boot/dts/at91sam9g20.dtsi b/arch/arm/boot/dts/at91sam9g20.dtsi
> > index ea942b5..e10842a 100644
> > --- a/arch/arm/boot/dts/at91sam9g20.dtsi
> > +++ b/arch/arm/boot/dts/at91sam9g20.dtsi
> > @@ -57,6 +57,11 @@
> >  				reg = <0xfffff000 0x200>;
> >  			};
> >  
> > +			pit: timer@fffffd30 {
> > +				compatible = "atmel,at91sam9260-pit";
> > +				reg = <0xfffffd30 0xf>;
> > +				interrupts = <1 4>;
> > +			};
> >  
> >  			pioA: gpio@fffff400 {
> >  				compatible = "atmel,at91rm9200-gpio";
> > diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
> > index ebc9617..28a678f 100644
> > --- a/arch/arm/boot/dts/at91sam9g45.dtsi
> > +++ b/arch/arm/boot/dts/at91sam9g45.dtsi
> > @@ -58,6 +58,12 @@
> >  				reg = <0xfffff000 0x200>;
> >  			};
> >  
> > +			pit: timer@fffffd30 {
> > +				compatible = "atmel,at91sam9260-pit";
> > +				reg = <0xfffffd30 0xf>;
> > +				interrupts = <1 4>;
> > +			};
> > +
> >  			dma: dma-controller@ffffec00 {
> >  				compatible = "atmel,at91sam9g45-dma";
> >  				reg = <0xffffec00 0x200>;
> > diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c
> > index d89ead7..802fea3 100644
> > --- a/arch/arm/mach-at91/at91sam926x_time.c
> > +++ b/arch/arm/mach-at91/at91sam926x_time.c
> > @@ -14,6 +14,9 @@
> >  #include <linux/kernel.h>
> >  #include <linux/clk.h>
> >  #include <linux/clockchips.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> >  
> >  #include <asm/mach/time.h>
> >  
> > @@ -133,7 +136,8 @@ static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
> >  static struct irqaction at91sam926x_pit_irq = {
> >  	.name		= "at91_tick",
> >  	.flags		= IRQF_SHARED | IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
> > -	.handler	= at91sam926x_pit_interrupt
> > +	.handler	= at91sam926x_pit_interrupt,
> > +	.irq		= AT91_ID_SYS,
> >  };
> >  
> >  static void at91sam926x_pit_reset(void)
> > @@ -149,6 +153,49 @@ static void at91sam926x_pit_reset(void)
> >  	pit_write(AT91_PIT_MR, (pit_cycle - 1) | AT91_PIT_PITEN);
> >  }
> >  
> > +#ifdef CONFIG_OF
> > +static struct of_device_id timer_ids[] = {
> > +	{ .compatible = "atmel,at91sam9260-pit" },
> > +	{ /* sentinel */ }
> > +};
> > +
> > +static int __init of_at91sam926x_pit_init(void)
> > +{
> > +	struct device_node	*np;
> > +	int			ret;
> > +
> > +	np = of_find_matching_node(NULL, timer_ids);
> > +	if (!np)
> > +		goto err;
> > +
> > +	pit_base_addr = of_iomap(np, 0);
> > +	if (!pit_base_addr)
> > +		goto node_err;
> > +
> > +	/* Get the interrupts property */
> > +	ret = irq_of_parse_and_map(np, 0);
> > +	if (!ret)
> > +		goto ioremap_err;
> > +	at91sam926x_pit_irq.irq = ret;
> > +
> > +	of_node_put(np);
> > +
> > +	return 0;
> > +
> > +ioremap_err:
> > +	iounmap(pit_base_addr);
> > +node_err:
> > +	of_node_put(np);
> > +err:
> > +	return -EINVAL;
> > +}
> > +#else
> > +static int __init of_at91sam926x_pit_init(void)
> > +{
> > +	return -EINVAL;
> > +}
> > +#endif
> > +
> >  /*
> >   * Set up both clocksource and clockevent support.
> >   */
> > @@ -177,7 +224,7 @@ static void __init at91sam926x_pit_init(void)
> >  	clocksource_register_hz(&pit_clk, pit_rate);
> >  
> >  	/* Set up irq handler */
> > -	setup_irq(AT91_ID_SYS, &at91sam926x_pit_irq);
> > +	setup_irq(at91sam926x_pit_irq.irq, &at91sam926x_pit_irq);
> >  
> >  	/* Set up and register clockevents */
> >  	pit_clkevt.mult = div_sc(pit_rate, NSEC_PER_SEC, pit_clkevt.shift);
> > @@ -193,10 +240,11 @@ static void at91sam926x_pit_suspend(void)
> >  
> >  void __init at91sam926x_ioremap_pit(u32 addr)
> >  {
> > -	pit_base_addr = ioremap(addr, 16);
> > -
> > -	if (!pit_base_addr)
> > -		panic("Impossible to ioremap PIT\n");
> > +	if (of_at91sam926x_pit_init() < 0) {
> > +		pit_base_addr = ioremap(addr, 16);
> > +		if (!pit_base_addr)
> > +			panic("Impossible to ioremap PIT\n");
> > +	}
> 
> This is not what I meant. I meant call either at91sam926x_ioremap_pit or
> of_at91sam926x_pit_init. Don't nest the calls and keep the OF and non-OF
> init somewhat separate. The fact that you are passing in the physical
> address and then ignoring it for the OF case is what I have issue with.

the DT pure soc will pass NULL for soc that support both or only non of we
pass the PHY addr

Best Regards,
J.

Re: [PATCH v4] ARM: at91: pit add DT support

[Nicolas Ferre]

On 01/09/2012 06:39 PM, Jean-Christophe PLAGNIOL-VILLARD :
> On 09:47 Fri 06 Jan     , Rob Herring wrote:
>> On 01/06/2012 10:20 AM, Nicolas Ferre wrote:
>>> From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
>>>
>>> Retreive registers address and IRQ from device tree entry. Fall back
>>> to built-in values if an error occurs.
>>>
>>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
>>> [nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org: change error path and interrupts property handling]
>>> Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>>> Reviewed-by: Jamie Iles <jamie-wmLquQDDieKakBO8gow8eQ@public.gmane.org>
>>> Acked-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>>> ---
>>> v4: - change of_at91sam926x_pit_init() return value usage logic as
>>>       suggested by Rob Herring
>>>     - irq_of_parse_and_map() returns 0 on error: change test according to 
>>>       Grant Likely note.
>>>
>>> v3: - use irq_of_parse_and_map() for handling irq numbers specified by DT.
>>>       Correction proposed by Jamie Iles.
>>>
>>> v2: - new specification of irq numbers in DT (due to modification of AIC code)
>>>     - new error path in of_at91sam926x_pit_init()
>>>     - fall back to built-in values if an error occurs
>>>     - use of of_property_read_u32() to get irq property
>>>
>>>  .../devicetree/bindings/arm/atmel-at91.txt         |    8 +++
>>>  arch/arm/boot/dts/at91sam9g20.dtsi                 |    5 ++
>>>  arch/arm/boot/dts/at91sam9g45.dtsi                 |    6 ++
>>>  arch/arm/mach-at91/at91sam926x_time.c              |   60 ++++++++++++++++++--
>>>  4 files changed, 73 insertions(+), 6 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/arm/atmel-at91.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt b/Documentation/devicetree/bindings/arm/atmel-at91.txt
>>> new file mode 100644
>>> index 0000000..380f711
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt
>>> @@ -0,0 +1,8 @@
>>> +Atmel AT91 device tree bindings.
>>> +================================
>>> +
>>> +PIT Timer required properties:
>>> +- compatible: Should be "atmel,at91sam9260-pit"
>>> +- reg: Should contain registers location and length
>>> +- interrupts: Should contain interrupt for the PIT which is the IRQ line
>>> +  shared across all System Controller members.
>>> diff --git a/arch/arm/boot/dts/at91sam9g20.dtsi b/arch/arm/boot/dts/at91sam9g20.dtsi
>>> index ea942b5..e10842a 100644
>>> --- a/arch/arm/boot/dts/at91sam9g20.dtsi
>>> +++ b/arch/arm/boot/dts/at91sam9g20.dtsi
>>> @@ -57,6 +57,11 @@
>>>  				reg = <0xfffff000 0x200>;
>>>  			};
>>>  
>>> +			pit: timer@fffffd30 {
>>> +				compatible = "atmel,at91sam9260-pit";
>>> +				reg = <0xfffffd30 0xf>;
>>> +				interrupts = <1 4>;
>>> +			};
>>>  
>>>  			pioA: gpio@fffff400 {
>>>  				compatible = "atmel,at91rm9200-gpio";
>>> diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
>>> index ebc9617..28a678f 100644
>>> --- a/arch/arm/boot/dts/at91sam9g45.dtsi
>>> +++ b/arch/arm/boot/dts/at91sam9g45.dtsi
>>> @@ -58,6 +58,12 @@
>>>  				reg = <0xfffff000 0x200>;
>>>  			};
>>>  
>>> +			pit: timer@fffffd30 {
>>> +				compatible = "atmel,at91sam9260-pit";
>>> +				reg = <0xfffffd30 0xf>;
>>> +				interrupts = <1 4>;
>>> +			};
>>> +
>>>  			dma: dma-controller@ffffec00 {
>>>  				compatible = "atmel,at91sam9g45-dma";
>>>  				reg = <0xffffec00 0x200>;
>>> diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c
>>> index d89ead7..802fea3 100644
>>> --- a/arch/arm/mach-at91/at91sam926x_time.c
>>> +++ b/arch/arm/mach-at91/at91sam926x_time.c
>>> @@ -14,6 +14,9 @@
>>>  #include <linux/kernel.h>
>>>  #include <linux/clk.h>
>>>  #include <linux/clockchips.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/of_irq.h>
>>>  
>>>  #include <asm/mach/time.h>
>>>  
>>> @@ -133,7 +136,8 @@ static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
>>>  static struct irqaction at91sam926x_pit_irq = {
>>>  	.name		= "at91_tick",
>>>  	.flags		= IRQF_SHARED | IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
>>> -	.handler	= at91sam926x_pit_interrupt
>>> +	.handler	= at91sam926x_pit_interrupt,
>>> +	.irq		= AT91_ID_SYS,
>>>  };
>>>  
>>>  static void at91sam926x_pit_reset(void)
>>> @@ -149,6 +153,49 @@ static void at91sam926x_pit_reset(void)
>>>  	pit_write(AT91_PIT_MR, (pit_cycle - 1) | AT91_PIT_PITEN);
>>>  }
>>>  
>>> +#ifdef CONFIG_OF
>>> +static struct of_device_id timer_ids[] = {
>>> +	{ .compatible = "atmel,at91sam9260-pit" },
>>> +	{ /* sentinel */ }
>>> +};
>>> +
>>> +static int __init of_at91sam926x_pit_init(void)
>>> +{
>>> +	struct device_node	*np;
>>> +	int			ret;
>>> +
>>> +	np = of_find_matching_node(NULL, timer_ids);
>>> +	if (!np)
>>> +		goto err;
>>> +
>>> +	pit_base_addr = of_iomap(np, 0);
>>> +	if (!pit_base_addr)
>>> +		goto node_err;
>>> +
>>> +	/* Get the interrupts property */
>>> +	ret = irq_of_parse_and_map(np, 0);
>>> +	if (!ret)
>>> +		goto ioremap_err;
>>> +	at91sam926x_pit_irq.irq = ret;
>>> +
>>> +	of_node_put(np);
>>> +
>>> +	return 0;
>>> +
>>> +ioremap_err:
>>> +	iounmap(pit_base_addr);
>>> +node_err:
>>> +	of_node_put(np);
>>> +err:
>>> +	return -EINVAL;
>>> +}
>>> +#else
>>> +static int __init of_at91sam926x_pit_init(void)
>>> +{
>>> +	return -EINVAL;
>>> +}
>>> +#endif
>>> +
>>>  /*
>>>   * Set up both clocksource and clockevent support.
>>>   */
>>> @@ -177,7 +224,7 @@ static void __init at91sam926x_pit_init(void)
>>>  	clocksource_register_hz(&pit_clk, pit_rate);
>>>  
>>>  	/* Set up irq handler */
>>> -	setup_irq(AT91_ID_SYS, &at91sam926x_pit_irq);
>>> +	setup_irq(at91sam926x_pit_irq.irq, &at91sam926x_pit_irq);
>>>  
>>>  	/* Set up and register clockevents */
>>>  	pit_clkevt.mult = div_sc(pit_rate, NSEC_PER_SEC, pit_clkevt.shift);
>>> @@ -193,10 +240,11 @@ static void at91sam926x_pit_suspend(void)
>>>  
>>>  void __init at91sam926x_ioremap_pit(u32 addr)
>>>  {
>>> -	pit_base_addr = ioremap(addr, 16);
>>> -
>>> -	if (!pit_base_addr)
>>> -		panic("Impossible to ioremap PIT\n");
>>> +	if (of_at91sam926x_pit_init() < 0) {
>>> +		pit_base_addr = ioremap(addr, 16);
>>> +		if (!pit_base_addr)
>>> +			panic("Impossible to ioremap PIT\n");
>>> +	}
>>
>> This is not what I meant. I meant call either at91sam926x_ioremap_pit or
>> of_at91sam926x_pit_init. Don't nest the calls and keep the OF and non-OF
>> init somewhat separate. The fact that you are passing in the physical
>> address and then ignoring it for the OF case is what I have issue with.
> 
> the DT pure soc will pass NULL for soc that support both or only non of we
> pass the PHY addr

Yes, but don't your prefer to have another call that we can use as the
interface? In the future we will only have DT enabled SoC so maintaining
the need for this NULL parameter does not make much sense...

Bye,
-- 
Nicolas Ferre

[PATCH v5 1/2] ARM: at91: pit add DT support

[Nicolas Ferre]

From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>

Retreive registers address and IRQ from device tree entry.
Called from at91_dt_init_irq() so that timers are up-n-running
when timers initialization will occur.

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
[nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org: change error path and interrupts property handling]
Signed-off-by: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
---
Hi,

I have removed Rob's "Acked-by" and Jamie's "Reviewed-by" because I have changed
the way PIT is initialized. Do you want to put it back? 

v5: - of_at91sam926x_pit_init() now called from at91sam926x_pit_init()
      which is the init function of the system tick timer.
    - get out of at91sam926x_ioremap_pit() if matching node found in DT.

v4: - change of_at91sam926x_pit_init() return value usage logic as
      suggested by Rob Herring
    - irq_of_parse_and_map() returns 0 on error: change test according to 
      Grant Likely note.

v3: - use irq_of_parse_and_map() for handling irq numbers specified by DT.
      Correction proposed by Jamie Iles.

v2: - new specification of irq numbers in DT (due to modification of AIC code)
    - new error path in of_at91sam926x_pit_init()
    - fall back to built-in values if an error occurs
    - use of of_property_read_u32() to get irq property

 .../devicetree/bindings/arm/atmel-at91.txt         |    8 +++
 arch/arm/boot/dts/at91sam9g20.dtsi                 |    6 ++
 arch/arm/boot/dts/at91sam9g45.dtsi                 |    6 ++
 arch/arm/mach-at91/at91sam926x_time.c              |   63 +++++++++++++++++++-
 arch/arm/mach-at91/at91sam9x5.c                    |    2 -
 arch/arm/mach-at91/generic.h                       |    1 +
 6 files changed, 82 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/atmel-at91.txt

diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt b/Documentation/devicetree/bindings/arm/atmel-at91.txt
new file mode 100644
index 0000000..380f711
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt
@@ -0,0 +1,8 @@
+Atmel AT91 device tree bindings.
+================================
+
+PIT Timer required properties:
+- compatible: Should be "atmel,at91sam9260-pit"
+- reg: Should contain registers location and length
+- interrupts: Should contain interrupt for the PIT which is the IRQ line
+  shared across all System Controller members.
diff --git a/arch/arm/boot/dts/at91sam9g20.dtsi b/arch/arm/boot/dts/at91sam9g20.dtsi
index 325989a..04c56c4 100644
--- a/arch/arm/boot/dts/at91sam9g20.dtsi
+++ b/arch/arm/boot/dts/at91sam9g20.dtsi
@@ -57,6 +57,12 @@
 				reg = <0xfffff000 0x200>;
 			};
 
+			pit: timer@fffffd30 {
+				compatible = "atmel,at91sam9260-pit";
+				reg = <0xfffffd30 0xf>;
+				interrupts = <1 4>;
+			};
+
 			pioA: gpio@fffff400 {
 				compatible = "atmel,at91rm9200-gpio";
 				reg = <0xfffff400 0x100>;
diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
index a9dbbb5..3881cab 100644
--- a/arch/arm/boot/dts/at91sam9g45.dtsi
+++ b/arch/arm/boot/dts/at91sam9g45.dtsi
@@ -58,6 +58,12 @@
 				reg = <0xfffff000 0x200>;
 			};
 
+			pit: timer@fffffd30 {
+				compatible = "atmel,at91sam9260-pit";
+				reg = <0xfffffd30 0xf>;
+				interrupts = <1 4>;
+			};
+
 			dma: dma-controller@ffffec00 {
 				compatible = "atmel,at91sam9g45-dma";
 				reg = <0xffffec00 0x200>;
diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c
index d89ead7..8e5cc37 100644
--- a/arch/arm/mach-at91/at91sam926x_time.c
+++ b/arch/arm/mach-at91/at91sam926x_time.c
@@ -14,6 +14,9 @@
 #include <linux/kernel.h>
 #include <linux/clk.h>
 #include <linux/clockchips.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
 
 #include <asm/mach/time.h>
 
@@ -133,7 +136,8 @@ static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
 static struct irqaction at91sam926x_pit_irq = {
 	.name		= "at91_tick",
 	.flags		= IRQF_SHARED | IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
-	.handler	= at91sam926x_pit_interrupt
+	.handler	= at91sam926x_pit_interrupt,
+	.irq		= AT91_ID_SYS,
 };
 
 static void at91sam926x_pit_reset(void)
@@ -149,6 +153,49 @@ static void at91sam926x_pit_reset(void)
 	pit_write(AT91_PIT_MR, (pit_cycle - 1) | AT91_PIT_PITEN);
 }
 
+#ifdef CONFIG_OF
+static struct of_device_id pit_timer_ids[] = {
+	{ .compatible = "atmel,at91sam9260-pit" },
+	{ /* sentinel */ }
+};
+
+int __init of_at91sam926x_pit_init(void)
+{
+	struct device_node	*np;
+	int			ret;
+
+	np = of_find_matching_node(NULL, pit_timer_ids);
+	if (!np)
+		goto err;
+
+	pit_base_addr = of_iomap(np, 0);
+	if (!pit_base_addr)
+		goto node_err;
+
+	/* Get the interrupts property */
+	ret = irq_of_parse_and_map(np, 0);
+	if (!ret)
+		goto ioremap_err;
+	at91sam926x_pit_irq.irq = ret;
+
+	of_node_put(np);
+
+	return 0;
+
+ioremap_err:
+	iounmap(pit_base_addr);
+node_err:
+	of_node_put(np);
+err:
+	return -EINVAL;
+}
+#else
+int __init of_at91sam926x_pit_init(void)
+{
+	return -EINVAL;
+}
+#endif
+
 /*
  * Set up both clocksource and clockevent support.
  */
@@ -157,6 +204,9 @@ static void __init at91sam926x_pit_init(void)
 	unsigned long	pit_rate;
 	unsigned	bits;
 
+	/* For device tree enabled device: initialize here */
+	of_at91sam926x_pit_init();
+
 	/*
 	 * Use our actual MCK to figure out how many MCK/16 ticks per
 	 * 1/HZ period (instead of a compile-time constant LATCH).
@@ -177,7 +227,7 @@ static void __init at91sam926x_pit_init(void)
 	clocksource_register_hz(&pit_clk, pit_rate);
 
 	/* Set up irq handler */
-	setup_irq(AT91_ID_SYS, &at91sam926x_pit_irq);
+	setup_irq(at91sam926x_pit_irq.irq, &at91sam926x_pit_irq);
 
 	/* Set up and register clockevents */
 	pit_clkevt.mult = div_sc(pit_rate, NSEC_PER_SEC, pit_clkevt.shift);
@@ -193,6 +243,15 @@ static void at91sam926x_pit_suspend(void)
 
 void __init at91sam926x_ioremap_pit(u32 addr)
 {
+#if defined(CONFIG_OF)
+	struct device_node *np =
+		of_find_matching_node(NULL, pit_timer_ids);
+
+	if (np) {
+		of_node_put(np);
+		return;
+	}
+#endif
 	pit_base_addr = ioremap(addr, 16);
 
 	if (!pit_base_addr)
diff --git a/arch/arm/mach-at91/at91sam9x5.c b/arch/arm/mach-at91/at91sam9x5.c
index 1c3444d..3e33711 100644
--- a/arch/arm/mach-at91/at91sam9x5.c
+++ b/arch/arm/mach-at91/at91sam9x5.c
@@ -301,8 +301,6 @@ static void __init at91sam9x5_map_io(void)
 
 static void __init at91sam9x5_ioremap_registers(void)
 {
-	if (of_at91sam926x_pit_init() < 0)
-		panic("Impossible to find PIT\n");
 }
 
 void __init at91sam9x5_initialize(void)
diff --git a/arch/arm/mach-at91/generic.h b/arch/arm/mach-at91/generic.h
index 0726f42..c5d16e5 100644
--- a/arch/arm/mach-at91/generic.h
+++ b/arch/arm/mach-at91/generic.h
@@ -33,6 +33,7 @@ extern int  __init at91_aic_of_init(struct device_node *node,
  /* Timer */
 struct sys_timer;
 extern struct sys_timer at91rm9200_timer;
+extern int of_at91sam926x_pit_init(void);
 extern void at91sam926x_ioremap_pit(u32 addr);
 extern struct sys_timer at91sam926x_timer;
 extern struct sys_timer at91x40_timer;
-- 
1.7.9

[PATCH v5 2/2] ARM: at91/pit: add traces in case of error

[Nicolas Ferre]

Traces related to IRQ management are useful for timers in case of
non-working IRQ subsystem (switch to irq_domain for instance).

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 arch/arm/mach-at91/at91sam926x_time.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c
index 8e5cc37..e4f7ba0 100644
--- a/arch/arm/mach-at91/at91sam926x_time.c
+++ b/arch/arm/mach-at91/at91sam926x_time.c
@@ -174,8 +174,10 @@ int __init of_at91sam926x_pit_init(void)
 
 	/* Get the interrupts property */
 	ret = irq_of_parse_and_map(np, 0);
-	if (!ret)
+	if (!ret) {
+		pr_crit("AT91: PIT: Unable to get IRQ from DT\n");
 		goto ioremap_err;
+	}
 	at91sam926x_pit_irq.irq = ret;
 
 	of_node_put(np);
@@ -203,6 +205,7 @@ static void __init at91sam926x_pit_init(void)
 {
 	unsigned long	pit_rate;
 	unsigned	bits;
+	int		ret;
 
 	/* For device tree enabled device: initialize here */
 	of_at91sam926x_pit_init();
@@ -227,7 +230,9 @@ static void __init at91sam926x_pit_init(void)
 	clocksource_register_hz(&pit_clk, pit_rate);
 
 	/* Set up irq handler */
-	setup_irq(at91sam926x_pit_irq.irq, &at91sam926x_pit_irq);
+	ret = setup_irq(at91sam926x_pit_irq.irq, &at91sam926x_pit_irq);
+	if (ret)
+		pr_crit("AT91: PIT: Unable to setup IRQ\n");
 
 	/* Set up and register clockevents */
 	pit_clkevt.mult = div_sc(pit_rate, NSEC_PER_SEC, pit_clkevt.shift);
-- 
1.7.9