diff mbox

[2/2] arm64: dts: mt8173: add timer node

Message ID 1442369095-1094-2-git-send-email-yingjoe.chen@mediatek.com
State New
Headers show

Commit Message

Yingjoe Chen Sept. 16, 2015, 2:04 a.m. UTC
From: Daniel Kurtz <djkurtz@chromium.org>

Add device node to enable GPT timer. This timer will be
used as sched clock source.

Change-Id: Idc4e3f0ee80b5c36cae6f0f2328f94aafcca1253
Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
---
 arch/arm64/boot/dts/mediatek/mt8173.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Sudeep Holla Sept. 17, 2015, 1:51 p.m. UTC | #1
On 16/09/15 03:04, Yingjoe Chen wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
>
> Add device node to enable GPT timer. This timer will be
> used as sched clock source.
>

Interesting any known issues with or advantage over the arch timers
to prefer it as sched clock source. I see even arch timers are present
in DT, hence the question. Or is it just a incorrect commit log ?

How does this get selected as sched clock source ? I don't see
sched_clock_register in mtk_timer.c

To be clear, I am not against adding this timer support, but just want
to know is it preferred for sched clock source ? if yes why ? better
resolution ?

> Change-Id: Idc4e3f0ee80b5c36cae6f0f2328f94aafcca1253

^ Should be dropped

> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
> Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> ---
>   arch/arm64/boot/dts/mediatek/mt8173.dtsi | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> index d18ee42..d763803 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> @@ -238,6 +238,15 @@
>   			reg = <0 0x10007000 0 0x100>;
>   		};
>
> +		timer: timer@10008000 {
> +			compatible = "mediatek,mt8173-timer",

Missing documentation ? I am referring upstream and it might be in some 
patches already queued perhaps ?

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Yingjoe Chen Sept. 17, 2015, 2:56 p.m. UTC | #2
On Thu, 2015-09-17 at 14:51 +0100, Sudeep Holla wrote:
> 
> On 16/09/15 03:04, Yingjoe Chen wrote:
> > From: Daniel Kurtz <djkurtz@chromium.org>
> >
> > Add device node to enable GPT timer. This timer will be
> > used as sched clock source.
> >
> 
> Interesting any known issues with or advantage over the arch timers
> to prefer it as sched clock source. I see even arch timers are present
> in DT, hence the question. Or is it just a incorrect commit log ?
> 
> How does this get selected as sched clock source ? I don't see
> sched_clock_register in mtk_timer.c
> 
> To be clear, I am not against adding this timer support, but just want
> to know is it preferred for sched clock source ? if yes why ? better
> resolution ?

Hi Sudeep,

Thanks for your review.

I hit the send too soon and missed cover letter, please see:
http://lists.infradead.org/pipermail/linux-mediatek/2015-September/002303.html

The main reason to use GPT as sched clock is it won't stop during idle.


> > Change-Id: Idc4e3f0ee80b5c36cae6f0f2328f94aafcca1253
> 
> ^ Should be dropped
> 
> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> > Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
> > Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> > ---
> >   arch/arm64/boot/dts/mediatek/mt8173.dtsi | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > index d18ee42..d763803 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > @@ -238,6 +238,15 @@
> >   			reg = <0 0x10007000 0 0x100>;
> >   		};
> >
> > +		timer: timer@10008000 {
> > +			compatible = "mediatek,mt8173-timer",
> 
> Missing documentation ? I am referring upstream and it might be in some 
> patches already queued perhaps ?

This is documented in
Documentation/devicetree/bindings/timer/mediatek,mtk-timer.txt.
Do you mean I should add "mediatek,mt8173-timer" to that file?

Joe.C


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sudeep Holla Sept. 17, 2015, 4:13 p.m. UTC | #3
On 17/09/15 15:56, Yingjoe Chen wrote:
> On Thu, 2015-09-17 at 14:51 +0100, Sudeep Holla wrote:
>>
>> On 16/09/15 03:04, Yingjoe Chen wrote:
>>> From: Daniel Kurtz <djkurtz@chromium.org>
>>>
>>> Add device node to enable GPT timer. This timer will be
>>> used as sched clock source.
>>>
>>
>> Interesting any known issues with or advantage over the arch timers
>> to prefer it as sched clock source. I see even arch timers are present
>> in DT, hence the question. Or is it just a incorrect commit log ?
>>
>> How does this get selected as sched clock source ? I don't see
>> sched_clock_register in mtk_timer.c
>>
>> To be clear, I am not against adding this timer support, but just want
>> to know is it preferred for sched clock source ? if yes why ? better
>> resolution ?
>
> Hi Sudeep,
>
> Thanks for your review.
>
> I hit the send too soon and missed cover letter, please see:
> http://lists.infradead.org/pipermail/linux-mediatek/2015-September/002303.html
>

OK

> The main reason to use GPT as sched clock is it won't stop during idle.
>
>

I think your are confusing the system counter with arch timers. System
counter is always-on, but the arch timers(logic implementing timers
comparators) might not be off when the processor is powered down.

I think you need this timer and are using it for low power idle states
in which case you will use this as a clock event and not clock source.
It will be used as a hardware broadcast event source.

There's no call to sched_clock_register in mtk_timer.c, so it can't be
the sched clock, so you need to fix the commit log.

[...]

>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>>> index d18ee42..d763803 100644
>>> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>>> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>>> @@ -238,6 +238,15 @@
>>>    			reg = <0 0x10007000 0 0x100>;
>>>    		};
>>>
>>> +		timer: timer@10008000 {
>>> +			compatible = "mediatek,mt8173-timer",
>>
>> Missing documentation ? I am referring upstream and it might be in some
>> patches already queued perhaps ?
>
> This is documented in
> Documentation/devicetree/bindings/timer/mediatek,mtk-timer.txt.
> Do you mean I should add "mediatek,mt8173-timer" to that file?
>

Yes

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mark Rutland Sept. 17, 2015, 4:41 p.m. UTC | #4
On Thu, Sep 17, 2015 at 03:56:56PM +0100, Yingjoe Chen wrote:
> On Thu, 2015-09-17 at 14:51 +0100, Sudeep Holla wrote:
> > 
> > On 16/09/15 03:04, Yingjoe Chen wrote:
> > > From: Daniel Kurtz <djkurtz@chromium.org>
> > >
> > > Add device node to enable GPT timer. This timer will be
> > > used as sched clock source.
> > >
> > 
> > Interesting any known issues with or advantage over the arch timers
> > to prefer it as sched clock source. I see even arch timers are present
> > in DT, hence the question. Or is it just a incorrect commit log ?
> > 
> > How does this get selected as sched clock source ? I don't see
> > sched_clock_register in mtk_timer.c
> > 
> > To be clear, I am not against adding this timer support, but just want
> > to know is it preferred for sched clock source ? if yes why ? better
> > resolution ?
> 
> Hi Sudeep,
> 
> Thanks for your review.
> 
> I hit the send too soon and missed cover letter, please see:
> http://lists.infradead.org/pipermail/linux-mediatek/2015-September/002303.html
> 
> The main reason to use GPT as sched clock is it won't stop during idle.

You don't mean sched clock, you just mean a clock_event_device.

A sched_clock is a high-precision clocksource that is read from (which
by definition requires the CPUs to be non-idle). It doesn't have
anything to do with interrupts and therefore cannot wake devices from
idle.

While the clock_event_device for the generic timer can't necessarily
wake CPUs from idle. The generic timer system counter counts even if
CPUs are idle, so the generic timer is fine as a sched_clock. 

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Yingjoe Chen Oct. 1, 2015, 2:33 p.m. UTC | #5
On Thu, 2015-09-17 at 17:13 +0100, Sudeep Holla wrote:
> 
> On 17/09/15 15:56, Yingjoe Chen wrote:
> > On Thu, 2015-09-17 at 14:51 +0100, Sudeep Holla wrote:
> >>
> >> On 16/09/15 03:04, Yingjoe Chen wrote:
> >>> From: Daniel Kurtz <djkurtz@chromium.org>
> >>>
> >>> Add device node to enable GPT timer. This timer will be
> >>> used as sched clock source.
> >>>
> >>
> >> Interesting any known issues with or advantage over the arch timers
> >> to prefer it as sched clock source. I see even arch timers are present
> >> in DT, hence the question. Or is it just a incorrect commit log ?
> >>
> >> How does this get selected as sched clock source ? I don't see
> >> sched_clock_register in mtk_timer.c
> >>
> >> To be clear, I am not against adding this timer support, but just want
> >> to know is it preferred for sched clock source ? if yes why ? better
> >> resolution ?
> >
> > Hi Sudeep,
> >
> > Thanks for your review.
> >
> > I hit the send too soon and missed cover letter, please see:
> > http://lists.infradead.org/pipermail/linux-mediatek/2015-September/002303.html
> >
> 
> OK
> 
> > The main reason to use GPT as sched clock is it won't stop during idle.
> >
> >
> 
> I think your are confusing the system counter with arch timers. System
> counter is always-on, but the arch timers(logic implementing timers
> comparators) might not be off when the processor is powered down.
> 
> I think you need this timer and are using it for low power idle states
> in which case you will use this as a clock event and not clock source.
> It will be used as a hardware broadcast event source.
> 
> There's no call to sched_clock_register in mtk_timer.c, so it can't be
> the sched clock, so you need to fix the commit log.

Hi Sudeep,

Sorry for late reply.

For sched_clock_register, please see
http://lists.infradead.org/pipermail/linux-mediatek/2015-July/001547.html
which was accepted in
https://git.linaro.org/people/daniel.lezcano/linux.git/shortlog/refs/heads/clockevents/4.4


You are right it is also used as clock event. I think we don't need to
mention those detail in commit message, so I'll change to just:

"Add device node to enable GPT timer."

> 
> [...]
> 
> >>> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >>> index d18ee42..d763803 100644
> >>> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >>> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >>> @@ -238,6 +238,15 @@
> >>>    			reg = <0 0x10007000 0 0x100>;
> >>>    		};
> >>>
> >>> +		timer: timer@10008000 {
> >>> +			compatible = "mediatek,mt8173-timer",
> >>
> >> Missing documentation ? I am referring upstream and it might be in some
> >> patches already queued perhaps ?
> >
> > This is documented in
> > Documentation/devicetree/bindings/timer/mediatek,mtk-timer.txt.
> > Do you mean I should add "mediatek,mt8173-timer" to that file?
> >
> 
> Yes

Will do in next round.
Thanks

Joe.C


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yingjoe Chen Oct. 1, 2015, 2:50 p.m. UTC | #6
On Thu, 2015-09-17 at 17:41 +0100, Mark Rutland wrote:
> On Thu, Sep 17, 2015 at 03:56:56PM +0100, Yingjoe Chen wrote:
> > On Thu, 2015-09-17 at 14:51 +0100, Sudeep Holla wrote:
> > > 
> > > On 16/09/15 03:04, Yingjoe Chen wrote:
> > > > From: Daniel Kurtz <djkurtz@chromium.org>
> > > >
> > > > Add device node to enable GPT timer. This timer will be
> > > > used as sched clock source.
> > > >
> > > 
> > > Interesting any known issues with or advantage over the arch timers
> > > to prefer it as sched clock source. I see even arch timers are present
> > > in DT, hence the question. Or is it just a incorrect commit log ?
> > > 
> > > How does this get selected as sched clock source ? I don't see
> > > sched_clock_register in mtk_timer.c
> > > 
> > > To be clear, I am not against adding this timer support, but just want
> > > to know is it preferred for sched clock source ? if yes why ? better
> > > resolution ?
> > 
> > Hi Sudeep,
> > 
> > Thanks for your review.
> > 
> > I hit the send too soon and missed cover letter, please see:
> > http://lists.infradead.org/pipermail/linux-mediatek/2015-September/002303.html
> > 
> > The main reason to use GPT as sched clock is it won't stop during idle.
> 
> You don't mean sched clock, you just mean a clock_event_device.
> 
> A sched_clock is a high-precision clocksource that is read from (which
> by definition requires the CPUs to be non-idle). It doesn't have
> anything to do with interrupts and therefore cannot wake devices from
> idle.
> 
> While the clock_event_device for the generic timer can't necessarily
> wake CPUs from idle. The generic timer system counter counts even if
> CPUs are idle, so the generic timer is fine as a sched_clock. 

Hi, Mark,

Thanks for your info and sorry for late reply. 

I notice ARM ARM said the arch timer shouldn't stop when idle, but
unfortunately that not true for mt8173. The last CPU enter idle can
choose to enter deep idle mode and the counter value would be lost. Our
firmware backup/recover the counter so it looks like it is stopped.

We will change the firmware to add missing count when back from deep
idle to make it looks like the counter never stop.

Joe.C


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sudeep Holla Oct. 1, 2015, 3:32 p.m. UTC | #7
On 01/10/15 15:33, Yingjoe Chen wrote:
> On Thu, 2015-09-17 at 17:13 +0100, Sudeep Holla wrote:
>>

[...]

>>
>> I think your are confusing the system counter with arch timers. System
>> counter is always-on, but the arch timers(logic implementing timers
>> comparators) might not be off when the processor is powered down.
>>
>> I think you need this timer and are using it for low power idle states
>> in which case you will use this as a clock event and not clock source.
>> It will be used as a hardware broadcast event source.
>>
>> There's no call to sched_clock_register in mtk_timer.c, so it can't be
>> the sched clock, so you need to fix the commit log.
>
> Hi Sudeep,
>
> Sorry for late reply.
>
> For sched_clock_register, please see
> http://lists.infradead.org/pipermail/linux-mediatek/2015-July/001547.html
> which was accepted in
> https://git.linaro.org/people/daniel.lezcano/linux.git/shortlog/refs/heads/clockevents/4.4
>

The commit message makes no sense to me. The counters should continue to
work as long as they are in always-on domain. Only timers are lost
when you enter deeper idle states. So I agree with using MTK timer as
broadcast timer/eventsource. You still didn't answer what's the need
to use MTK timer as sched clocksource ?

Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yingjoe Chen Oct. 2, 2015, 2 p.m. UTC | #8
On Thu, 2015-10-01 at 16:32 +0100, Sudeep Holla wrote:
> 
> On 01/10/15 15:33, Yingjoe Chen wrote:
> > On Thu, 2015-09-17 at 17:13 +0100, Sudeep Holla wrote:
> >>
> 
> [...]
> 
> >>
> >> I think your are confusing the system counter with arch timers. System
> >> counter is always-on, but the arch timers(logic implementing timers
> >> comparators) might not be off when the processor is powered down.
> >>
> >> I think you need this timer and are using it for low power idle states
> >> in which case you will use this as a clock event and not clock source.
> >> It will be used as a hardware broadcast event source.
> >>
> >> There's no call to sched_clock_register in mtk_timer.c, so it can't be
> >> the sched clock, so you need to fix the commit log.
> >
> > Hi Sudeep,
> >
> > Sorry for late reply.
> >
> > For sched_clock_register, please see
> > http://lists.infradead.org/pipermail/linux-mediatek/2015-July/001547.html
> > which was accepted in
> > https://git.linaro.org/people/daniel.lezcano/linux.git/shortlog/refs/heads/clockevents/4.4
> >
> 
> The commit message makes no sense to me. The counters should continue to
> work as long as they are in always-on domain. Only timers are lost
> when you enter deeper idle states. So I agree with using MTK timer as
> broadcast timer/eventsource. You still didn't answer what's the need
> to use MTK timer as sched clocksource ?


Hi, Sudeep,

ARM ARM said the counter should be in always-on domain, but
unfortunately that not true for mt8173. The last CPU enter idle can
choose to enter deep idle mode and the counter value would be lost. Our
firmware backup/recover the counter so it looks like it is stopped.
That's why I thought we need to use it as sched clocksource.

On mt8173, we will fix the firmware to add missing counts, so it will
looks like the counter keep counting. But other mediatek platform have
similar issue, and the 2 counter have same resolution, so I still want
to keep using GPT as sched clocksource.

Joe.C


--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index d18ee42..d763803 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -238,6 +238,15 @@ 
 			reg = <0 0x10007000 0 0x100>;
 		};
 
+		timer: timer@10008000 {
+			compatible = "mediatek,mt8173-timer",
+				     "mediatek,mt6577-timer";
+			reg = <0 0x10008000 0 0x1000>;
+			interrupts = <GIC_SPI 144 IRQ_TYPE_LEVEL_LOW>;
+			clocks = <&infracfg CLK_INFRA_CLK_13M>,
+				 <&topckgen CLK_TOP_RTC_SEL>;
+		};
+
 		pwrap: pwrap@1000d000 {
 			compatible = "mediatek,mt8173-pwrap";
 			reg = <0 0x1000d000 0 0x1000>;