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

[Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>]

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