[PATCH 1/2] arm64: mediatek: enable MTK_TIMER

[PATCH 1/2] arm64: mediatek: enable MTK_TIMER

[Yingjoe Chen]

Enable MTK_TIMER for MediaTek plaform, which will be used as
schedule clock.

Change-Id: Ib77a0bf01193102c755077b6e72e73e477b18e5f
Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
---
 arch/arm64/Kconfig.platforms | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 23800a1..8176455 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -42,6 +42,7 @@ config ARCH_MEDIATEK
 	bool "Mediatek MT65xx & MT81xx ARMv8 SoC"
 	select ARM_GIC
 	select PINCTRL
+	select MTK_TIMER
 	help
 	  Support for Mediatek MT65xx & MT81xx ARMv8 SoCs
 
-- 
1.9.1

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

[Yingjoe Chen]

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(+)

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>;
-- 
1.9.1

Re: [PATCH 1/2] arm64: mediatek: enable MTK_TIMER

[Yingjoe Chen]

On Wed, 2015-09-16 at 10:04 +0800, Yingjoe Chen wrote:
> Enable MTK_TIMER for MediaTek plaform, which will be used as
> schedule clock.

Sorry, sending this series too early without cover letter and removing
Change-Id. Here's the cover letter:


This is actually v3 of "add GPT timer support for mt8173" series. This
is based on v4.3-rc1 + clockevents-4.4[1] and James's mediatek-clk
tree[2].

Changes compare to previous version[3]:
- the first two mtk_timer related changes are removed because they are
replaced/accepted in clockevents tree.
- Remove 'add 13mhz clock for MT8173' because it is accepted in
mediatek-clk tree.
- Kconfig.platforms path change.

So we only have 2 patches left here.
Matthias, can you take these and help to remove the Change-Id?


Daniel Kurtz (1):
  arm64: dts: mt8173: add timer node

Yingjoe Chen (1):
  arm64: mediatek: enable MTK_TIMER



[1]
https://git.linaro.org/people/daniel.lezcano/linux.git clockevents/4.4
[2]
http://lists.infradead.org/pipermail/linux-mediatek/2015-August/002069.html
[3]
http://lists.infradead.org/pipermail/linux-mediatek/2015-July/001544.html

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

[Sudeep Holla]

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

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

[Yingjoe Chen]

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

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

[Sudeep Holla]

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

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

[Mark Rutland]

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.

Re: [PATCH 1/2] arm64: mediatek: enable MTK_TIMER

[Matthias Brugger]

On 16/09/15 04:21, Yingjoe Chen wrote:
> On Wed, 2015-09-16 at 10:04 +0800, Yingjoe Chen wrote:
>> Enable MTK_TIMER for MediaTek plaform, which will be used as
>> schedule clock.
>
> Sorry, sending this series too early without cover letter and removing
> Change-Id. Here's the cover letter:
>
>
> This is actually v3 of "add GPT timer support for mt8173" series. This
> is based on v4.3-rc1 + clockevents-4.4[1] and James's mediatek-clk
> tree[2].
>
> Changes compare to previous version[3]:
> - the first two mtk_timer related changes are removed because they are
> replaced/accepted in clockevents tree.
> - Remove 'add 13mhz clock for MT8173' because it is accepted in
> mediatek-clk tree.
> - Kconfig.platforms path change.
>
> So we only have 2 patches left here.
> Matthias, can you take these and help to remove the Change-Id?

Yes I can take this two patches, but I don't see the mediatek-clk tree 
accepted by the maintainers yet. As far as I can see, without the 13 MHz 
clock [1], the sched clock won't work.

[1] 
https://github.com/jamesjjliao/linux/commit/add5e89d657fa6c0e5a517890f996c796b768064

>
>
> Daniel Kurtz (1):
>    arm64: dts: mt8173: add timer node
>
> Yingjoe Chen (1):
>    arm64: mediatek: enable MTK_TIMER
>
>
>
> [1]
> https://git.linaro.org/people/daniel.lezcano/linux.git clockevents/4.4
> [2]
> http://lists.infradead.org/pipermail/linux-mediatek/2015-August/002069.html
> [3]
> http://lists.infradead.org/pipermail/linux-mediatek/2015-July/001544.html
>
>
>
>

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

[Yingjoe Chen]

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

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

[Yingjoe Chen]

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

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

[Sudeep Holla]

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

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

[Yingjoe Chen]

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