diff mbox

[RFC,v2,13/18] ARM: OMAP2+: AM33XX: timer: Interchance clkevt and clksrc timers

Message ID 1356959231-17335-14-git-send-email-vaibhav.bedia@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Vaibhav Bedia Dec. 31, 2012, 1:07 p.m. UTC
AM33XX has two timers (DTIMER0/1) in the WKUP domain.
On GP devices the source of DMTIMER0 is fixed to an
inaccurate internal 32k RC oscillator and this makes
the DMTIMER0 practically either as a clocksource or
as clockevent.

Currently the timer instance in WKUP domain is used
as the clockevent and the timer in non-WKUP domain
as the clocksource. DMTIMER1 in WKUP domain can keep
running in suspend from a 32K clock fed from external
OSC and can serve as the persistent clock for the kernel.
To enable this, interchange the timers used as clocksource
and clockevent for AM33XX.

For now a new DT property has been added to allow the timer code
to select the timer with the right property.

It has been pointed out by Santosh Shilimkar and Kevin Hilman
that such a change will result in soc-idle never being achieved
on AM33XX. There are other reasons why soc-idle does not look
feasible on AM33XX so for now we go ahead with the interchange
of the the timers. If at a later point of time we do come up
with an approach which makes soc-idle possible on AM33XX, this
can be revisited.

Signed-off-by: Vaibhav Bedia <vaibhav.bedia@ti.com>
Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
Cc: Tony Lingren <tony@atomide.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Jon Hunter <jon-hunter@ti.com>

---
v1->v2:
	Use DT properties for changing the timers

 .../devicetree/bindings/arm/omap/timer.txt         |    2 ++
 arch/arm/boot/dts/am33xx.dtsi                      |    1 +
 arch/arm/mach-omap2/timer.c                        |    6 +++---
 3 files changed, 6 insertions(+), 3 deletions(-)

Comments

Santosh Shilimkar Jan. 8, 2013, 3:17 p.m. UTC | #1
On Monday 31 December 2012 06:37 PM, Vaibhav Bedia wrote:
> AM33XX has two timers (DTIMER0/1) in the WKUP domain.
> On GP devices the source of DMTIMER0 is fixed to an
> inaccurate internal 32k RC oscillator and this makes
> the DMTIMER0 practically either as a clocksource or
> as clockevent.
>
> Currently the timer instance in WKUP domain is used
> as the clockevent and the timer in non-WKUP domain
> as the clocksource. DMTIMER1 in WKUP domain can keep
> running in suspend from a 32K clock fed from external
> OSC and can serve as the persistent clock for the kernel.
> To enable this, interchange the timers used as clocksource
> and clockevent for AM33XX.
>
> For now a new DT property has been added to allow the timer code
> to select the timer with the right property.
>
> It has been pointed out by Santosh Shilimkar and Kevin Hilman
> that such a change will result in soc-idle never being achieved
> on AM33XX. There are other reasons why soc-idle does not look
> feasible on AM33XX so for now we go ahead with the interchange
> of the the timers. If at a later point of time we do come up
> with an approach which makes soc-idle possible on AM33XX, this
> can be revisited.
>
Can you please explain other reasons as well for clarity ?

Regards
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vaibhav Bedia Jan. 9, 2013, 5:31 a.m. UTC | #2
On Tue, Jan 08, 2013 at 20:47:28, Shilimkar, Santosh wrote:
> On Monday 31 December 2012 06:37 PM, Vaibhav Bedia wrote:
> > AM33XX has two timers (DTIMER0/1) in the WKUP domain.
> > On GP devices the source of DMTIMER0 is fixed to an
> > inaccurate internal 32k RC oscillator and this makes
> > the DMTIMER0 practically either as a clocksource or
> > as clockevent.
> >
> > Currently the timer instance in WKUP domain is used
> > as the clockevent and the timer in non-WKUP domain
> > as the clocksource. DMTIMER1 in WKUP domain can keep
> > running in suspend from a 32K clock fed from external
> > OSC and can serve as the persistent clock for the kernel.
> > To enable this, interchange the timers used as clocksource
> > and clockevent for AM33XX.
> >
> > For now a new DT property has been added to allow the timer code
> > to select the timer with the right property.
> >
> > It has been pointed out by Santosh Shilimkar and Kevin Hilman
> > that such a change will result in soc-idle never being achieved
> > on AM33XX. There are other reasons why soc-idle does not look
> > feasible on AM33XX so for now we go ahead with the interchange
> > of the the timers. If at a later point of time we do come up
> > with an approach which makes soc-idle possible on AM33XX, this
> > can be revisited.
> >
> Can you please explain other reasons as well for clarity ?
> 

I guess from h/w perspective it boils down to lack of HW-AUTO and IO-Daisy
chaining on AM33xx [1]. 

Since there's no 32ksynctimer, do we also need to register DMTIMER1 as the persistent
clock on AM33xx? This can only be done from DMTIMER1 which is currently serving as
the clockevent.

Regards,
Vaibhav

[1] http://marc.info/?l=linux-arm-kernel&m=135238055717206&w=4
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hunter, Jon Jan. 17, 2013, 7:09 p.m. UTC | #3
On 12/31/2012 07:07 AM, Vaibhav Bedia wrote:
> AM33XX has two timers (DTIMER0/1) in the WKUP domain.
> On GP devices the source of DMTIMER0 is fixed to an
> inaccurate internal 32k RC oscillator and this makes
> the DMTIMER0 practically either as a clocksource or
> as clockevent.
> 
> Currently the timer instance in WKUP domain is used
> as the clockevent and the timer in non-WKUP domain
> as the clocksource. DMTIMER1 in WKUP domain can keep
> running in suspend from a 32K clock fed from external
> OSC and can serve as the persistent clock for the kernel.
> To enable this, interchange the timers used as clocksource
> and clockevent for AM33XX.
> 
> For now a new DT property has been added to allow the timer code
> to select the timer with the right property.
> 
> It has been pointed out by Santosh Shilimkar and Kevin Hilman
> that such a change will result in soc-idle never being achieved
> on AM33XX. There are other reasons why soc-idle does not look
> feasible on AM33XX so for now we go ahead with the interchange
> of the the timers. If at a later point of time we do come up
> with an approach which makes soc-idle possible on AM33XX, this
> can be revisited.
> 
> Signed-off-by: Vaibhav Bedia <vaibhav.bedia@ti.com>
> Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
> Cc: Tony Lingren <tony@atomide.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Benoit Cousson <b-cousson@ti.com>
> Cc: Paul Walmsley <paul@pwsan.com>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> Cc: Jon Hunter <jon-hunter@ti.com>
> 
> ---
> v1->v2:
> 	Use DT properties for changing the timers
> 
>  .../devicetree/bindings/arm/omap/timer.txt         |    2 ++
>  arch/arm/boot/dts/am33xx.dtsi                      |    1 +
>  arch/arm/mach-omap2/timer.c                        |    6 +++---
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt b/Documentation/devicetree/bindings/arm/omap/timer.txt
> index 8732d4d..62d4f2c 100644
> --- a/Documentation/devicetree/bindings/arm/omap/timer.txt
> +++ b/Documentation/devicetree/bindings/arm/omap/timer.txt
> @@ -18,6 +18,8 @@ Optional properties:
>  - ti,timer-pwm: 	Indicates the timer can generate a PWM output.
>  - ti,timer-secure: 	Indicates the timer is reserved on a secure OMAP device
>  			and therefore cannot be used by the kernel.
> +- ti,timer-non-wkup	Indicates the timer is in non-wkup power domain and hence
> +			will lose register context when the power domain transitions

I was hoping that we could avoid adding another property, especially
given that his equivalent to a timer that does not have the
"ti,timer-alwon" property.

>  Example:
>  
> diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> index 4731748..b4e8bf7 100644
> --- a/arch/arm/boot/dts/am33xx.dtsi
> +++ b/arch/arm/boot/dts/am33xx.dtsi
> @@ -251,6 +251,7 @@
>  			reg = <0x48040000 0x400>;
>  			interrupts = <68>;
>  			ti,hwmods = "timer2";
> +			ti,timer-non-wkup;

Is this is only one not in the wake-up domain?

>  		};
>  
>  		timer3: timer@48042000 {
> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> index 38f9cbc..cfb3413 100644
> --- a/arch/arm/mach-omap2/timer.c
> +++ b/arch/arm/mach-omap2/timer.c
> @@ -264,7 +264,7 @@ static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
>  	int r = 0;
>  
>  	if (of_have_populated_dt()) {
> -		np = omap_get_timer_dt(omap_timer_match, NULL);
> +		np = omap_get_timer_dt(omap_timer_match, property);
>  		if (!np)
>  			return -ENODEV;
>  
> @@ -633,8 +633,8 @@ OMAP_SYS_TIMER(3_gp, gptimer);
>  #endif /* CONFIG_ARCH_OMAP3 */
>  
>  #ifdef CONFIG_SOC_AM33XX
> -OMAP_SYS_GP_TIMER_INIT(3_am33xx, 1, OMAP4_MPU_SOURCE, "ti,timer-alwon",
> -		       2, OMAP4_MPU_SOURCE);
> +OMAP_SYS_GP_TIMER_INIT(3_am33xx, 2, OMAP4_MPU_SOURCE, "ti,timer-non-wkup",
> +		       1, OMAP4_MPU_SOURCE);

It seems to me here that we should specify the property "ti,timer-alwon"
for the clocksource and then no property for the clockevent. Hence, may
be the code needs to be adjusted so that if clockevent or clocksource
can use any timer (ie. no property specified), we look for a timer that
has no "ti-timer-xxxx" properties. This will ensure that if we need a
particular timer for clocksource, which we look for after clockevent, it
will be available.

Cheers
Jon


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hunter, Jon Jan. 24, 2013, 10:30 p.m. UTC | #4
Hi Vaibhav,

On 12/31/2012 07:07 AM, Vaibhav Bedia wrote:
> AM33XX has two timers (DTIMER0/1) in the WKUP domain.
> On GP devices the source of DMTIMER0 is fixed to an
> inaccurate internal 32k RC oscillator and this makes
> the DMTIMER0 practically either as a clocksource or
> as clockevent.
> 
> Currently the timer instance in WKUP domain is used
> as the clockevent and the timer in non-WKUP domain
> as the clocksource. DMTIMER1 in WKUP domain can keep
> running in suspend from a 32K clock fed from external
> OSC and can serve as the persistent clock for the kernel.
> To enable this, interchange the timers used as clocksource
> and clockevent for AM33XX.

I have been thinking about this some more. In the case where we are
using gptimers for both clock-events and clock-source (on both AM33xx
and OMAP) and I am wondering if it makes sense to switch the timers so
that we use the always-on timer for clock-source and a different one
from clock-events.

For OMAP, if we are not using the 32k-sync for clock-source, then we are
never going to achieve low power states during idle as we will always
have one gptimer running. And in this case, to your point below, it
would be better to use the always-on for clock-source so that in suspend
we can at least hit low power states and maintain time.

> For now a new DT property has been added to allow the timer code
> to select the timer with the right property.
> 
> It has been pointed out by Santosh Shilimkar and Kevin Hilman
> that such a change will result in soc-idle never being achieved
> on AM33XX. There are other reasons why soc-idle does not look
> feasible on AM33XX so for now we go ahead with the interchange
> of the the timers. If at a later point of time we do come up
> with an approach which makes soc-idle possible on AM33XX, this
> can be revisited.

Right, but this would also be true for OMAP if we don't use the 32k-sync
as we only have one gptimer in the wake-up domain.

Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hunter, Jon Jan. 30, 2013, 5:48 p.m. UTC | #5
On 01/24/2013 04:30 PM, Jon Hunter wrote:
> Hi Vaibhav,
> 
> On 12/31/2012 07:07 AM, Vaibhav Bedia wrote:
>> AM33XX has two timers (DTIMER0/1) in the WKUP domain.
>> On GP devices the source of DMTIMER0 is fixed to an
>> inaccurate internal 32k RC oscillator and this makes
>> the DMTIMER0 practically either as a clocksource or
>> as clockevent.
>>
>> Currently the timer instance in WKUP domain is used
>> as the clockevent and the timer in non-WKUP domain
>> as the clocksource. DMTIMER1 in WKUP domain can keep
>> running in suspend from a 32K clock fed from external
>> OSC and can serve as the persistent clock for the kernel.
>> To enable this, interchange the timers used as clocksource
>> and clockevent for AM33XX.
> 
> I have been thinking about this some more. In the case where we are
> using gptimers for both clock-events and clock-source (on both AM33xx
> and OMAP) and I am wondering if it makes sense to switch the timers so
> that we use the always-on timer for clock-source and a different one
> from clock-events.
> 
> For OMAP, if we are not using the 32k-sync for clock-source, then we are
> never going to achieve low power states during idle as we will always
> have one gptimer running. And in this case, to your point below, it
> would be better to use the always-on for clock-source so that in suspend
> we can at least hit low power states and maintain time.

I have posted a patch today [1] that I hope will address this issue for
you. Can you give that a try?

Cheers
Jon

[1] http://www.spinics.net/lists/arm-kernel/msg221265.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hunter, Jon Jan. 30, 2013, 5:49 p.m. UTC | #6
On 01/30/2013 11:48 AM, Jon Hunter wrote:
> 
> On 01/24/2013 04:30 PM, Jon Hunter wrote:
>> Hi Vaibhav,
>>
>> On 12/31/2012 07:07 AM, Vaibhav Bedia wrote:
>>> AM33XX has two timers (DTIMER0/1) in the WKUP domain.
>>> On GP devices the source of DMTIMER0 is fixed to an
>>> inaccurate internal 32k RC oscillator and this makes
>>> the DMTIMER0 practically either as a clocksource or
>>> as clockevent.
>>>
>>> Currently the timer instance in WKUP domain is used
>>> as the clockevent and the timer in non-WKUP domain
>>> as the clocksource. DMTIMER1 in WKUP domain can keep
>>> running in suspend from a 32K clock fed from external
>>> OSC and can serve as the persistent clock for the kernel.
>>> To enable this, interchange the timers used as clocksource
>>> and clockevent for AM33XX.
>>
>> I have been thinking about this some more. In the case where we are
>> using gptimers for both clock-events and clock-source (on both AM33xx
>> and OMAP) and I am wondering if it makes sense to switch the timers so
>> that we use the always-on timer for clock-source and a different one
>> from clock-events.
>>
>> For OMAP, if we are not using the 32k-sync for clock-source, then we are
>> never going to achieve low power states during idle as we will always
>> have one gptimer running. And in this case, to your point below, it
>> would be better to use the always-on for clock-source so that in suspend
>> we can at least hit low power states and maintain time.
> 
> I have posted a patch today [1] that I hope will address this issue for
> you. Can you give that a try?

By the way, this need to be applied on top of the fix I sent yesterday
to pass the property.

Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vaibhav Bedia Jan. 31, 2013, 11:29 a.m. UTC | #7
On Wed, Jan 30, 2013 at 23:19:34, Hunter, Jon wrote:
> 
> By the way, this need to be applied on top of the fix I sent yesterday
> to pass the property.
> 

Ok. Thanks for pointing this out.
 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/arm/omap/timer.txt b/Documentation/devicetree/bindings/arm/omap/timer.txt
index 8732d4d..62d4f2c 100644
--- a/Documentation/devicetree/bindings/arm/omap/timer.txt
+++ b/Documentation/devicetree/bindings/arm/omap/timer.txt
@@ -18,6 +18,8 @@  Optional properties:
 - ti,timer-pwm: 	Indicates the timer can generate a PWM output.
 - ti,timer-secure: 	Indicates the timer is reserved on a secure OMAP device
 			and therefore cannot be used by the kernel.
+- ti,timer-non-wkup	Indicates the timer is in non-wkup power domain and hence
+			will lose register context when the power domain transitions
 
 Example:
 
diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 4731748..b4e8bf7 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -251,6 +251,7 @@ 
 			reg = <0x48040000 0x400>;
 			interrupts = <68>;
 			ti,hwmods = "timer2";
+			ti,timer-non-wkup;
 		};
 
 		timer3: timer@48042000 {
diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index 38f9cbc..cfb3413 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -264,7 +264,7 @@  static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
 	int r = 0;
 
 	if (of_have_populated_dt()) {
-		np = omap_get_timer_dt(omap_timer_match, NULL);
+		np = omap_get_timer_dt(omap_timer_match, property);
 		if (!np)
 			return -ENODEV;
 
@@ -633,8 +633,8 @@  OMAP_SYS_TIMER(3_gp, gptimer);
 #endif /* CONFIG_ARCH_OMAP3 */
 
 #ifdef CONFIG_SOC_AM33XX
-OMAP_SYS_GP_TIMER_INIT(3_am33xx, 1, OMAP4_MPU_SOURCE, "ti,timer-alwon",
-		       2, OMAP4_MPU_SOURCE);
+OMAP_SYS_GP_TIMER_INIT(3_am33xx, 2, OMAP4_MPU_SOURCE, "ti,timer-non-wkup",
+		       1, OMAP4_MPU_SOURCE);
 OMAP_SYS_TIMER(3_am33xx, gptimer);
 #endif /* CONFIG_SOC_AM33XX */