Re: [PATCH V3 1/5] ARM: dts: OMAP: Add timer nodes

[Jon Hunter <jon-hunter@ti.com>]

On 10/25/2012 07:12 AM, Hiremath, Vaibhav wrote:
> On Thu, Oct 25, 2012 at 04:30:37, Hunter, Jon wrote:
>>
>> On 10/24/2012 01:17 PM, Hiremath, Vaibhav wrote:
>>> On Wed, Oct 17, 2012 at 23:31:09, Hunter, Jon wrote:
>>>> Add the 12 GP timers nodes present in OMAP2.
>>>> Add the 12 GP timers nodes present in OMAP3.
>>>> Add the 11 GP timers nodes present in OMAP4.
>>>> Add the 7 GP timers nodes present in AM33xx.
>>>>
>>>> Add documentation for timer properties specific to OMAP.
>>>>
>>>> Please note that for OMAP2/3 devices, there is only one interrupt controller
>>>> for the ARM CPU (which has the label "intc") and so globally define this as the
>>>> interrupt parent to save duplicating the interrupt parent for all device nodes.
>>>>
>>>> Thanks to Vaibhav Hiremath for creating the AM33xx timer nodes. I have modified
>>>> Vaibhav's original nodes adding information on which timers support a PWM
>>>> output.
>>>>
>>>> Cc: Benoit Cousson <b-cousson@ti.com>
>>>> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
>>>> ---
>>>>  .../devicetree/bindings/arm/omap/timer.txt         |   29 ++++++
>>>>  arch/arm/boot/dts/am33xx.dtsi                      |   61 +++++++++++++
>>>>  arch/arm/boot/dts/omap2.dtsi                       |   86 ++++++++++++++++++
>>>>  arch/arm/boot/dts/omap2420.dtsi                    |    8 ++
>>>>  arch/arm/boot/dts/omap2430.dtsi                    |    8 ++
>>>>  arch/arm/boot/dts/omap3.dtsi                       |   96 ++++++++++++++++++++
>>>>  arch/arm/boot/dts/omap4.dtsi                       |   86 ++++++++++++++++++
>>>>  7 files changed, 374 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/arm/omap/timer.txt
>>>>
>>>
>>> Although I have not tested this version of patch series at my end, but 
>>> whole patch-series Looks ok to me.
>>>
>>> Acked-By: Vaibhav Hiremath <hvaibhav@ti.com>
>>
>> Thanks. I made a couple cosmetic changes in V4 apart from the
>> "interrupt-parent" addition which we are now dropping. Care to ACK
>> patches 2-5 of V4?
>>
> 
> Jon,
> 
> Good news, I could able to spend some time today on Timer issue on Am33xx 
> and figure out what is going wrong there. The register context is loosing,
> which leads to failure of interrupt test cases.

Thanks!

> Below log describes more on this,
> 
> 
> [root@arago /]# echo 1 > /tmp/omap-test/timer/all
> [    9.156122] Testing 48042000.timer with 24000000 Hz clock ...
> [root@arago /]#
> [root@arago /]#
> [root@arago /]#
> [root@arago /]# [   11.505497] Timer read test PASSED! No errors, 100 loops
> [   11.511493] omap_dm_timer_set_int_enable:664: irq_ena - 0
> [   11.517277] omap_dm_timer_set_int_enable:670: irq_ena - 2
> [   11.523095] omap_timer_interrupt_test:120: irq_ena - 0 	[BOOOOOOOOM]
> [   12.521111] Timer interrupt test FAILED! No interrupt occurred in 1 sec
> 
> 
> I changed the Test code as below, and not with your Timer patches, I have 
> tested all the timers without any issues.
> 
> So for all patch series, 
> 
> Acked-Reviewed-&-Tested-By: Vaibhav Hiremath <hvaibhav@ti.com>

Thanks!

> 
> Test code diff:
> ===============
> 
> diff --git a/timer_test.c b/timer_test.c
> index e502881..c87a830 100644
> --- a/timer_test.c
> +++ b/timer_test.c
> @@ -13,6 +13,7 @@
> 
>  #define OMAP1_NUM_TIMERS       8
>  #define OMAP2_NUM_TIMERS       11
> +#define AM33XX_NUM_TIMERS      7
>  #define OMAP_MAX_NUM_TIMERS    12
>  #define OMAP_TIMER_SRC_CLKS    2
>  #define TIMER_TIMEOUT          (msecs_to_jiffies(1000))
> @@ -113,6 +114,9 @@ static int omap_timer_interrupt_test(struct omap_dm_timer *gptimer)
> 
>         irq_data.gptimer = gptimer;
>         init_completion(&irq_data.complete);
> +
> +       omap_dm_timer_enable(gptimer);
> +
>         omap_dm_timer_set_int_enable(gptimer, OMAP_TIMER_INT_OVERFLOW);
>         omap_dm_timer_set_load_start(gptimer, 0, 0xffffff00);
> 
> @@ -128,6 +132,8 @@ static int omap_timer_interrupt_test(struct omap_dm_timer *gptimer)
> 
>         omap_dm_timer_stop(gptimer);
>         omap_dm_timer_set_int_enable(gptimer, 0);
> +       omap_dm_timer_disable(gptimer);
> +

Hmmm ... I am wondering if there is another bug lingering in the dmtimer
driver. Ideally, the context should be preserved by the driver.

>         free_irq(timer_irq, &irq_data);
> 
>         return r;
> @@ -141,6 +147,8 @@ static u32 omap_timer_num_timers(void)
>                 max_num_timers = OMAP1_NUM_TIMERS;
>         else if (cpu_is_omap34xx() && (omap_type() == OMAP2_DEVICE_TYPE_GP))
>                 max_num_timers = OMAP2_NUM_TIMERS + 1;
> +       else if (soc_is_am33xx())
> +               max_num_timers = AM33XX_NUM_TIMERS;

Thanks, I noticed that too when testing AM33xx.

Cheers
Jon