[PATCH 0/5] clocksource: rockchip/timer: Support rktimer for rk3399

[PATCH 0/5] clocksource: rockchip/timer: Support rktimer for rk3399

[Caesar Wang]

This series patches had been tested on rockchip inside kernel.
In order to support the rk3399 SoC timer and turn off interrupts and IPIs to
save power in idle.
Okay, it still works bootup on rk3288/other SoCs, even though many socs hasn't used
the broadcast timer.

Easy to test for my borad.
localhost / # cat /proc/interrupts
CPU0       CPU1       CPU2       CPU3       CPU4       CPU5
1:          0          0          0          0          0          0     GICv3  29 Edge      arch_timer
...
5:          0          0          0          0          0          0     GICv3 113 Level     rk_timer
..

localhost / # cat /proc/timer_list | grep event_handler
get "event_handler:  hrtimer_interrupt"
event_handler:  tick_handle_oneshot_broadcast
event_handler:  hrtimer_interrupt

That should work for my board.



Huang Tao (5):
  dt-bindings: document rk3399 rk-timer bindings
  clocksource: rockchip: remove unnecessary clear irq before request_irq
  clocksource: rockchip: add dynamic irq flag to the timer
  clocksource: rockchip: add support for rk3399 SoC
  ARM64: dts: rockchip: add rktimer device node for rk3399

 ...chip,rk3288-timer.txt => rockchip,rk-timer.txt} |   6 +-
 arch/arm64/boot/dts/rockchip/rk3399.dtsi           |   8 ++
 drivers/clocksource/rockchip_timer.c               | 120 ++++++++++++++++-----
 3 files changed, 106 insertions(+), 28 deletions(-)
 rename Documentation/devicetree/bindings/timer/{rockchip,rk3288-timer.txt => rockchip,rk-timer.txt} (75%)

-- 
1.9.1

[PATCH 1/5] dt-bindings: document rk3399 rk-timer bindings

[Caesar Wang]

From: Huang Tao <huangtao@rock-chips.com>

Add compatible string for rk3399 because which timer is a little
different from older SoCs. So rename the file name from
rockchip,rk3288-timer.txt to rockchip,rk-timer.txt.
Clarify rockchip,rk3288-timer supported SoCs.

Signed-off-by: Huang Tao <huangtao@rock-chips.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Heiko Stuebner <heiko@sntech.de>
Signed-off-by: Caesar Wang <wxt@rock-chips.com>
---

 .../timer/{rockchip,rk3288-timer.txt => rockchip,rk-timer.txt}      | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
 rename Documentation/devicetree/bindings/timer/{rockchip,rk3288-timer.txt => rockchip,rk-timer.txt} (75%)

diff --git a/Documentation/devicetree/bindings/timer/rockchip,rk3288-timer.txt b/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
similarity index 75%
rename from Documentation/devicetree/bindings/timer/rockchip,rk3288-timer.txt
rename to Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
index 87f0b00..a41b184 100644
--- a/Documentation/devicetree/bindings/timer/rockchip,rk3288-timer.txt
+++ b/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
@@ -1,7 +1,9 @@
-Rockchip rk3288 timer
+Rockchip rk timer
 
 Required properties:
-- compatible: shall be "rockchip,rk3288-timer"
+- compatible: shall be one of:
+  "rockchip,rk3288-timer" - for rk3066, rk3036, rk3188, rk322x, rk3288, rk3368
+  "rockchip,rk3399-timer" - for rk3399
 - reg: base address of the timer register starting with TIMERS CONTROL register
 - interrupts: should contain the interrupts for Timer0
 - clocks : must contain an entry for each entry in clock-names
-- 
1.9.1

Re: [PATCH 1/5] dt-bindings: document rk3399 rk-timer bindings

[Rob Herring]

On Wed, May 25, 2016 at 05:49:58PM +0800, Caesar Wang wrote:
> From: Huang Tao <huangtao@rock-chips.com>
> 
> Add compatible string for rk3399 because which timer is a little
> different from older SoCs. So rename the file name from
> rockchip,rk3288-timer.txt to rockchip,rk-timer.txt.
> Clarify rockchip,rk3288-timer supported SoCs.
> 
> Signed-off-by: Huang Tao <huangtao@rock-chips.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> ---
> 
>  .../timer/{rockchip,rk3288-timer.txt => rockchip,rk-timer.txt}      | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>  rename Documentation/devicetree/bindings/timer/{rockchip,rk3288-timer.txt => rockchip,rk-timer.txt} (75%)

Acked-by: Rob Herring <robh@kernel.org>

[PATCH 2/5] clocksource: rockchip: remove unnecessary clear irq before request_irq

[Caesar Wang]

From: Huang Tao <huangtao@rock-chips.com>

rk_timer_interrupt_clear and rk_timer_disable is unnecessary before
request_irq. Timer should keep disabled before booting Linux.

Signed-off-by: Huang Tao <huangtao@rock-chips.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Heiko Stuebner <heiko@sntech.de>
Tested-by: Jianqun Xu <jay.xu@rock-chips.com>
Signed-off-by: Caesar Wang <wxt@rock-chips.com>
---

 drivers/clocksource/rockchip_timer.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index b991b28..b93fed6 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -158,9 +158,6 @@ static void __init rk_timer_init(struct device_node *np)
 	ce->cpumask = cpumask_of(0);
 	ce->rating = 250;
 
-	rk_timer_interrupt_clear(ce);
-	rk_timer_disable(ce);
-
 	ret = request_irq(irq, rk_timer_interrupt, IRQF_TIMER, TIMER_NAME, ce);
 	if (ret) {
 		pr_err("Failed to initialize '%s': %d\n", TIMER_NAME, ret);
-- 
1.9.1

Re: [PATCH 2/5] clocksource: rockchip: remove unnecessary clear irq before request_irq

[Daniel Lezcano]

On 05/25/2016 11:49 AM, Caesar Wang wrote:
> From: Huang Tao <huangtao@rock-chips.com>
>
> rk_timer_interrupt_clear and rk_timer_disable is unnecessary before
> request_irq. Timer should keep disabled before booting Linux.

That's true in the perfect world :/ Some version has u-boot letting the 
timer with irq enabled, therefore as soon as request_irq is done, an irq 
fires and leads to a kernel panic.

On the other side, this timer is not used on the other rockchip version 
than rk3399 because of no need of a broadcast timer, so removing these 
two lines may be acceptable.

Can try the changes with another board, eg rk3288 (and forcing to use 
this timer). Can you do the test and confirm it does not break with 
different version of u-boot ?

Thanks!

   -- Daniel

> Signed-off-by: Huang Tao <huangtao@rock-chips.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Jianqun Xu <jay.xu@rock-chips.com>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> ---
>
>   drivers/clocksource/rockchip_timer.c | 3 ---
>   1 file changed, 3 deletions(-)
>
> diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
> index b991b28..b93fed6 100644
> --- a/drivers/clocksource/rockchip_timer.c
> +++ b/drivers/clocksource/rockchip_timer.c
> @@ -158,9 +158,6 @@ static void __init rk_timer_init(struct device_node *np)
>   	ce->cpumask = cpumask_of(0);
>   	ce->rating = 250;
>
> -	rk_timer_interrupt_clear(ce);
> -	rk_timer_disable(ce);
> -
>   	ret = request_irq(irq, rk_timer_interrupt, IRQF_TIMER, TIMER_NAME, ce);
>   	if (ret) {
>   		pr_err("Failed to initialize '%s': %d\n", TIMER_NAME, ret);
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 2/5] clocksource: rockchip: remove unnecessary clear irq before request_irq

[Doug Anderson]

Hi,

On Mon, May 30, 2016 at 4:09 PM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 05/25/2016 11:49 AM, Caesar Wang wrote:
>>
>> From: Huang Tao <huangtao@rock-chips.com>
>>
>> rk_timer_interrupt_clear and rk_timer_disable is unnecessary before
>> request_irq. Timer should keep disabled before booting Linux.
>
>
> That's true in the perfect world :/ Some version has u-boot letting the
> timer with irq enabled, therefore as soon as request_irq is done, an irq
> fires and leads to a kernel panic.
>
> On the other side, this timer is not used on the other rockchip version than
> rk3399 because of no need of a broadcast timer, so removing these two lines
> may be acceptable.
>
> Can try the changes with another board, eg rk3288 (and forcing to use this
> timer). Can you do the test and confirm it does not break with different
> version of u-boot ?

Actually, I'm not even sure that's true in a perfect world.  ;)  There
are two main problems that might be lurking here:

1. On exynos5 devices I've worked with, the private timer (MCT)
actually shared the same physical counter with the ARM Architected
Timer.  IIRC stopping or resetting the MCT had the effect of stopping
/ resetting the Arch Timer.  Is it the same for you?  As I understand
it the Arch Timer isn't supposed to ever be stopped or reset.  If
firmware left the timer stopped and the kernel happened to be compiled
without support for the Rockchip timer (but had the Arch Timers) then
things would be very broken.  Also the early kernel boot might be
broken if the Arch Timer inits before the Rockchip timer.

NOTE: If your timer and the Arch Timer are totally separate then point
#1 is not important.


2. Historically in Chrome OS there's been an unofficial agreement that
the firmware would start its high speed timer as soon as possible at
bootup and that this could be used to (roughly) measure the time
between the start of firmware and the start of the kernel.  That means
that the kernel was expecting the timer to actually be running when it
started up.  Yup, this is a bit of a hack and I'm not sure it's
terribly well documented, but it does provide a reason that firmware
might have left the timer running.


-Doug

Re: [PATCH 2/5] clocksource: rockchip: remove unnecessary clear irq before request_irq

[Huang, Tao]

Hi Doug:
On 2016年06月01日 01:03, Doug Anderson wrote:
> Hi,
> 
> On Mon, May 30, 2016 at 4:09 PM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> On 05/25/2016 11:49 AM, Caesar Wang wrote:
>>>
>>> From: Huang Tao <huangtao@rock-chips.com>
>>>
>>> rk_timer_interrupt_clear and rk_timer_disable is unnecessary before
>>> request_irq. Timer should keep disabled before booting Linux.
>>
>>
>> That's true in the perfect world :/ Some version has u-boot letting the
>> timer with irq enabled, therefore as soon as request_irq is done, an irq
>> fires and leads to a kernel panic.
>>
>> On the other side, this timer is not used on the other rockchip version than
>> rk3399 because of no need of a broadcast timer, so removing these two lines
>> may be acceptable.
>>
>> Can try the changes with another board, eg rk3288 (and forcing to use this
>> timer). Can you do the test and confirm it does not break with different
>> version of u-boot ?
> 
> Actually, I'm not even sure that's true in a perfect world.  ;)  There
> are two main problems that might be lurking here:
> 
> 1. On exynos5 devices I've worked with, the private timer (MCT)
> actually shared the same physical counter with the ARM Architected
> Timer.  IIRC stopping or resetting the MCT had the effect of stopping
> / resetting the Arch Timer.  Is it the same for you?  As I understand
> it the Arch Timer isn't supposed to ever be stopped or reset.  If
> firmware left the timer stopped and the kernel happened to be compiled
> without support for the Rockchip timer (but had the Arch Timers) then
> things would be very broken.  Also the early kernel boot might be
> broken if the Arch Timer inits before the Rockchip timer.
> 
> NOTE: If your timer and the Arch Timer are totally separate then point
> #1 is not important.

We never use the timer which provide clock source of arch timer as
clockevent timer. If we do such stupid thing, when rk timer disabled,
the arch timer will stop too. Generally, we use this special timer as
clocksouce or never touch it again when it is running.

> 
> 
> 2. Historically in Chrome OS there's been an unofficial agreement that
> the firmware would start its high speed timer as soon as possible at
> bootup and that this could be used to (roughly) measure the time
> between the start of firmware and the start of the kernel.  That means
> that the kernel was expecting the timer to actually be running when it
> started up.  Yup, this is a bit of a hack and I'm not sure it's
> terribly well documented, but it does provide a reason that firmware
> might have left the timer running.

Why you chose the timer shared with Linux kernel, there are so many
timer? I think loader should do the right thing, uninit the resources
when it boot the kernel. I believe this code is lagacy from very old
chip such as rk2908 which is Cortex-A8. There are not arch timer, so the
loader may keep the timer running when enter kernel. Any way, if we
adopt the code suggested by Daniel, it is safe to keep the code.

Re: [PATCH 2/5] clocksource: rockchip: remove unnecessary clear irq before request_irq

[Doug Anderson]

Hi,

On Tue, May 31, 2016 at 7:30 PM, Huang, Tao <huangtao@rock-chips.com> wrote:
>> Actually, I'm not even sure that's true in a perfect world.  ;)  There
>> are two main problems that might be lurking here:
>>
>> 1. On exynos5 devices I've worked with, the private timer (MCT)
>> actually shared the same physical counter with the ARM Architected
>> Timer.  IIRC stopping or resetting the MCT had the effect of stopping
>> / resetting the Arch Timer.  Is it the same for you?  As I understand
>> it the Arch Timer isn't supposed to ever be stopped or reset.  If
>> firmware left the timer stopped and the kernel happened to be compiled
>> without support for the Rockchip timer (but had the Arch Timers) then
>> things would be very broken.  Also the early kernel boot might be
>> broken if the Arch Timer inits before the Rockchip timer.
>>
>> NOTE: If your timer and the Arch Timer are totally separate then point
>> #1 is not important.
>
> We never use the timer which provide clock source of arch timer as
> clockevent timer. If we do such stupid thing, when rk timer disabled,
> the arch timer will stop too. Generally, we use this special timer as
> clocksouce or never touch it again when it is running.

Ah, OK.  :)  I didn't go through and review / test the code.  I just
wanted to make sure we weren't going to run into the same bug I
remember running into before.  ;)


>> 2. Historically in Chrome OS there's been an unofficial agreement that
>> the firmware would start its high speed timer as soon as possible at
>> bootup and that this could be used to (roughly) measure the time
>> between the start of firmware and the start of the kernel.  That means
>> that the kernel was expecting the timer to actually be running when it
>> started up.  Yup, this is a bit of a hack and I'm not sure it's
>> terribly well documented, but it does provide a reason that firmware
>> might have left the timer running.
>
> Why you chose the timer shared with Linux kernel, there are so many
> timer? I think loader should do the right thing, uninit the resources
> when it boot the kernel. I believe this code is lagacy from very old
> chip such as rk2908 which is Cortex-A8. There are not arch timer, so the
> loader may keep the timer running when enter kernel. Any way, if we
> adopt the code suggested by Daniel, it is safe to keep the code.

If this is a separate / distinct timer than the main clocksource
timer, then you can ignore my comments.  ;)

...but obviously the comments from Daniel are much more important to
address and it sounds like you're all set for doing that.  :)

-Doug

[PATCH 3/5] clocksource: rockchip: add dynamic irq flag to the timer

[Caesar Wang]

From: Huang Tao <huangtao@rock-chips.com>

The rockchip timer is broadcast timer. Add CLOCK_EVT_FEAT_DYNIRQ
flag and set cpumask to all cpu to save power by avoid unnecessary
wakeups and IPIs.

Signed-off-by: Huang Tao <huangtao@rock-chips.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Heiko Stuebner <heiko@sntech.de>
Tested-by: Jianqun Xu <jay.xu@rock-chips.com>
Signed-off-by: Caesar Wang <wxt@rock-chips.com>
---

 drivers/clocksource/rockchip_timer.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index b93fed6..f3dfb1a 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -150,12 +150,13 @@ static void __init rk_timer_init(struct device_node *np)
 	}
 
 	ce->name = TIMER_NAME;
-	ce->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
+	ce->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
+		       CLOCK_EVT_FEAT_DYNIRQ;
 	ce->set_next_event = rk_timer_set_next_event;
 	ce->set_state_shutdown = rk_timer_shutdown;
 	ce->set_state_periodic = rk_timer_set_periodic;
 	ce->irq = irq;
-	ce->cpumask = cpumask_of(0);
+	ce->cpumask = cpu_all_mask;
 	ce->rating = 250;
 
 	ret = request_irq(irq, rk_timer_interrupt, IRQF_TIMER, TIMER_NAME, ce);
-- 
1.9.1

Re: [PATCH 3/5] clocksource: rockchip: add dynamic irq flag to the timer

[Daniel Lezcano]

On 05/25/2016 11:50 AM, Caesar Wang wrote:
> From: Huang Tao <huangtao@rock-chips.com>
>
> The rockchip timer is broadcast timer. Add CLOCK_EVT_FEAT_DYNIRQ
> flag and set cpumask to all cpu to save power by avoid unnecessary
> wakeups and IPIs.
>
> Signed-off-by: Huang Tao <huangtao@rock-chips.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Jianqun Xu <jay.xu@rock-chips.com>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> ---
>
>   drivers/clocksource/rockchip_timer.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
> index b93fed6..f3dfb1a 100644
> --- a/drivers/clocksource/rockchip_timer.c
> +++ b/drivers/clocksource/rockchip_timer.c
> @@ -150,12 +150,13 @@ static void __init rk_timer_init(struct device_node *np)
>   	}
>
>   	ce->name = TIMER_NAME;
> -	ce->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
> +	ce->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
> +		       CLOCK_EVT_FEAT_DYNIRQ;
>   	ce->set_next_event = rk_timer_set_next_event;
>   	ce->set_state_shutdown = rk_timer_shutdown;
>   	ce->set_state_periodic = rk_timer_set_periodic;
>   	ce->irq = irq;
> -	ce->cpumask = cpumask_of(0);
> +	ce->cpumask = cpu_all_mask;

s/cpu_all_mask/cpu_possible_mask/

>   	ce->rating = 250;
>
>   	ret = request_irq(irq, rk_timer_interrupt, IRQF_TIMER, TIMER_NAME, ce);
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 3/5] clocksource: rockchip: add dynamic irq flag to the timer

[Huang, Tao]

Hi Daniel:
On 2016年05月31日 07:16, Daniel Lezcano wrote:
> On 05/25/2016 11:50 AM, Caesar Wang wrote:
>> From: Huang Tao <huangtao@rock-chips.com>
>>
>> The rockchip timer is broadcast timer. Add CLOCK_EVT_FEAT_DYNIRQ
>> flag and set cpumask to all cpu to save power by avoid unnecessary
>> wakeups and IPIs.
>>
>> Signed-off-by: Huang Tao <huangtao@rock-chips.com>
>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Heiko Stuebner <heiko@sntech.de>
>> Tested-by: Jianqun Xu <jay.xu@rock-chips.com>
>> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
>> ---
>>
>>   drivers/clocksource/rockchip_timer.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
>> index b93fed6..f3dfb1a 100644
>> --- a/drivers/clocksource/rockchip_timer.c
>> +++ b/drivers/clocksource/rockchip_timer.c
>> @@ -150,12 +150,13 @@ static void __init rk_timer_init(struct device_node *np)
>>   	}
>>
>>   	ce->name = TIMER_NAME;
>> -	ce->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
>> +	ce->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
>> +		       CLOCK_EVT_FEAT_DYNIRQ;
>>   	ce->set_next_event = rk_timer_set_next_event;
>>   	ce->set_state_shutdown = rk_timer_shutdown;
>>   	ce->set_state_periodic = rk_timer_set_periodic;
>>   	ce->irq = irq;
>> -	ce->cpumask = cpumask_of(0);
>> +	ce->cpumask = cpu_all_mask;
> 
> s/cpu_all_mask/cpu_possible_mask/

Okay for me.
It seems drivers in drivers/clocksource are very confusing about use
cpu_all_mask or cpu_possible_mask. For example arm_arch_timer.c use
cpu_all_mask, while mtk_timer.c use cpu_possible_mask. I think all just
work.

Thanks,
Huang, Tao

[PATCH 4/5] clocksource: rockchip: add support for rk3399 SoC

[Caesar Wang]

From: Huang Tao <huangtao@rock-chips.com>

The CONTROL register offset is different from old SoCs.
For Linux driver, there are not functional changes at all.
Let's call it v2.

Signed-off-by: Huang Tao <huangtao@rock-chips.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Heiko Stuebner <heiko@sntech.de>
Tested-by: Jianqun Xu <jay.xu@rock-chips.com>
Signed-off-by: Caesar Wang <wxt@rock-chips.com>
---

 drivers/clocksource/rockchip_timer.c | 112 ++++++++++++++++++++++++++++-------
 1 file changed, 91 insertions(+), 21 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index f3dfb1a..7ce1d08 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -19,8 +19,9 @@
 
 #define TIMER_LOAD_COUNT0	0x00
 #define TIMER_LOAD_COUNT1	0x04
-#define TIMER_CONTROL_REG	0x10
+#define TIMER_V1_CONTROL_REG	0x10
 #define TIMER_INT_STATUS	0x18
+#define TIMER_V2_CONTROL_REG	0x1c
 
 #define TIMER_DISABLE		0x0
 #define TIMER_ENABLE		0x1
@@ -46,15 +47,26 @@ static inline void __iomem *rk_base(struct clock_event_device *ce)
 	return rk_timer(ce)->base;
 }
 
-static inline void rk_timer_disable(struct clock_event_device *ce)
+static inline void rk_timer_v1_disable(struct clock_event_device *ce)
 {
-	writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
+	writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_V1_CONTROL_REG);
 }
 
-static inline void rk_timer_enable(struct clock_event_device *ce, u32 flags)
+static inline void rk_timer_v1_enable(struct clock_event_device *ce, u32 flags)
 {
 	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
-		       rk_base(ce) + TIMER_CONTROL_REG);
+		       rk_base(ce) + TIMER_V1_CONTROL_REG);
+}
+
+static inline void rk_timer_v2_disable(struct clock_event_device *ce)
+{
+	writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_V2_CONTROL_REG);
+}
+
+static inline void rk_timer_v2_enable(struct clock_event_device *ce, u32 flags)
+{
+	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
+		       rk_base(ce) + TIMER_V2_CONTROL_REG);
 }
 
 static void rk_timer_update_counter(unsigned long cycles,
@@ -69,44 +81,82 @@ static void rk_timer_interrupt_clear(struct clock_event_device *ce)
 	writel_relaxed(1, rk_base(ce) + TIMER_INT_STATUS);
 }
 
-static inline int rk_timer_set_next_event(unsigned long cycles,
-					  struct clock_event_device *ce)
+static int rk_timer_v1_set_next_event(unsigned long cycles,
+				      struct clock_event_device *ce)
 {
-	rk_timer_disable(ce);
+	rk_timer_v1_disable(ce);
 	rk_timer_update_counter(cycles, ce);
-	rk_timer_enable(ce, TIMER_MODE_USER_DEFINED_COUNT);
+	rk_timer_v1_enable(ce, TIMER_MODE_USER_DEFINED_COUNT);
 	return 0;
 }
 
-static int rk_timer_shutdown(struct clock_event_device *ce)
+static int rk_timer_v1_shutdown(struct clock_event_device *ce)
 {
-	rk_timer_disable(ce);
+	rk_timer_v1_disable(ce);
 	return 0;
 }
 
-static int rk_timer_set_periodic(struct clock_event_device *ce)
+static int rk_timer_v1_set_periodic(struct clock_event_device *ce)
 {
-	rk_timer_disable(ce);
+	rk_timer_v1_disable(ce);
 	rk_timer_update_counter(rk_timer(ce)->freq / HZ - 1, ce);
-	rk_timer_enable(ce, TIMER_MODE_FREE_RUNNING);
+	rk_timer_v1_enable(ce, TIMER_MODE_FREE_RUNNING);
 	return 0;
 }
 
-static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
+static irqreturn_t rk_timer_v1_interrupt(int irq, void *dev_id)
 {
 	struct clock_event_device *ce = dev_id;
 
 	rk_timer_interrupt_clear(ce);
 
 	if (clockevent_state_oneshot(ce))
-		rk_timer_disable(ce);
+		rk_timer_v1_disable(ce);
 
 	ce->event_handler(ce);
 
 	return IRQ_HANDLED;
 }
 
-static void __init rk_timer_init(struct device_node *np)
+static int rk_timer_v2_set_next_event(unsigned long cycles,
+				      struct clock_event_device *ce)
+{
+	rk_timer_v2_disable(ce);
+	rk_timer_update_counter(cycles, ce);
+	rk_timer_v2_enable(ce, TIMER_MODE_USER_DEFINED_COUNT);
+	return 0;
+}
+
+static int rk_timer_v2_shutdown(struct clock_event_device *ce)
+{
+	rk_timer_v2_disable(ce);
+	return 0;
+}
+
+static int rk_timer_v2_set_periodic(struct clock_event_device *ce)
+{
+	rk_timer_v2_disable(ce);
+	rk_timer_update_counter(rk_timer(ce)->freq / HZ - 1, ce);
+	rk_timer_v2_enable(ce, TIMER_MODE_FREE_RUNNING);
+	return 0;
+}
+
+static irqreturn_t rk_timer_v2_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *ce = dev_id;
+
+	rk_timer_interrupt_clear(ce);
+
+	if (clockevent_state_oneshot(ce))
+		rk_timer_v2_disable(ce);
+
+	ce->event_handler(ce);
+
+	return IRQ_HANDLED;
+}
+
+static void __init rk_timer_init(struct device_node *np,
+				 irq_handler_t rk_timer_interrupt)
 {
 	struct clock_event_device *ce = &bc_timer.ce;
 	struct clk *timer_clk;
@@ -152,9 +202,6 @@ static void __init rk_timer_init(struct device_node *np)
 	ce->name = TIMER_NAME;
 	ce->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
 		       CLOCK_EVT_FEAT_DYNIRQ;
-	ce->set_next_event = rk_timer_set_next_event;
-	ce->set_state_shutdown = rk_timer_shutdown;
-	ce->set_state_periodic = rk_timer_set_periodic;
 	ce->irq = irq;
 	ce->cpumask = cpu_all_mask;
 	ce->rating = 250;
@@ -177,4 +224,27 @@ out_unmap:
 	iounmap(bc_timer.base);
 }
 
-CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
+static void __init rk_timer_v1_init(struct device_node *np)
+{
+	struct clock_event_device *ce = &bc_timer.ce;
+
+	ce->set_next_event = rk_timer_v1_set_next_event;
+	ce->set_state_shutdown = rk_timer_v1_shutdown;
+	ce->set_state_periodic = rk_timer_v1_set_periodic;
+
+	rk_timer_init(np, rk_timer_v1_interrupt);
+}
+
+static void __init rk_timer_v2_init(struct device_node *np)
+{
+	struct clock_event_device *ce = &bc_timer.ce;
+
+	ce->set_next_event = rk_timer_v2_set_next_event;
+	ce->set_state_shutdown = rk_timer_v2_shutdown;
+	ce->set_state_periodic = rk_timer_v2_set_periodic;
+
+	rk_timer_init(np, rk_timer_v2_interrupt);
+}
+
+CLOCKSOURCE_OF_DECLARE(rk3288_timer, "rockchip,rk3288-timer", rk_timer_v1_init);
+CLOCKSOURCE_OF_DECLARE(rk3399_timer, "rockchip,rk3399-timer", rk_timer_v2_init);
-- 
1.9.1

Re: [PATCH 4/5] clocksource: rockchip: add support for rk3399 SoC

[Daniel Lezcano]

On 05/25/2016 11:50 AM, Caesar Wang wrote:
> From: Huang Tao <huangtao@rock-chips.com>
>
> The CONTROL register offset is different from old SoCs.
> For Linux driver, there are not functional changes at all.
> Let's call it v2.
>
> Signed-off-by: Huang Tao <huangtao@rock-chips.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Jianqun Xu <jay.xu@rock-chips.com>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> ---

That's hackish.

Please consider something like:

diff --git a/drivers/clocksource/rockchip_timer.c 
b/drivers/clocksource/rockchip_timer.c
index b991b28..b6ba6f9 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -19,7 +19,8 @@

  #define TIMER_LOAD_COUNT0	0x00
  #define TIMER_LOAD_COUNT1	0x04
-#define TIMER_CONTROL_REG	0x10
+#define TIMER_CONTROL_REG3288	0x10
+#define TIMER_CONTROL_REG3399	0x1C
  #define TIMER_INT_STATUS	0x18

  #define TIMER_DISABLE		0x0
@@ -31,6 +32,7 @@
  struct bc_timer {
  	struct clock_event_device ce;
  	void __iomem *base;
+	void __iomem *ctrl;
  	u32 freq;
  };

@@ -46,15 +48,20 @@ static inline void __iomem *rk_base(struct 
clock_event_device *ce)
  	return rk_timer(ce)->base;
  }

+static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
+{
+        return rk_timer(ce)->ctrl;
+}
+
  static inline void rk_timer_disable(struct clock_event_device *ce)
  {
-	writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
+	writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
  }

  static inline void rk_timer_enable(struct clock_event_device *ce, u32 
flags)
  {
  	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
-		       rk_base(ce) + TIMER_CONTROL_REG);
+		       rk_ctrl(ce));
  }

  static void rk_timer_update_counter(unsigned long cycles,
@@ -179,4 +186,18 @@ out_unmap:
  	iounmap(bc_timer.base);
  }

-CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
+static void __init rk3288_timer_init(struct device_node *np)
+{
+	bc_timer.ctrl = TIMER_CONTROL_REG3288;
+	rk_timer_init(np);
+}
+
+static void __init rk3399_timer_init(struct device_node *np)
+{
+        bc_timer.ctrl = TIMER_CONTROL_REG3399;
+	rk_timer_init(np);
+}
+
+
+CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", 
rk3288_timer_init);
+CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3399-timer", 
rk3399_timer_init);




-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 4/5] clocksource: rockchip: add support for rk3399 SoC

[Huang, Tao]

Hi Daniel:
On 2016年05月31日 07:28, Daniel Lezcano wrote:
> On 05/25/2016 11:50 AM, Caesar Wang wrote:
>> From: Huang Tao <huangtao@rock-chips.com>
>>
>> The CONTROL register offset is different from old SoCs.
>> For Linux driver, there are not functional changes at all.
>> Let's call it v2.
>>
>> Signed-off-by: Huang Tao <huangtao@rock-chips.com>
>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Heiko Stuebner <heiko@sntech.de>
>> Tested-by: Jianqun Xu <jay.xu@rock-chips.com>
>> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
>> ---
> 
> That's hackish.
Yes:( I blamed our IC guy.
> 
> Please consider something like:
> 
> diff --git a/drivers/clocksource/rockchip_timer.c 
> b/drivers/clocksource/rockchip_timer.c
> index b991b28..b6ba6f9 100644
> --- a/drivers/clocksource/rockchip_timer.c
> +++ b/drivers/clocksource/rockchip_timer.c
> @@ -19,7 +19,8 @@
> 
>   #define TIMER_LOAD_COUNT0	0x00
>   #define TIMER_LOAD_COUNT1	0x04
> -#define TIMER_CONTROL_REG	0x10
> +#define TIMER_CONTROL_REG3288	0x10
> +#define TIMER_CONTROL_REG3399	0x1C
>   #define TIMER_INT_STATUS	0x18
> 
>   #define TIMER_DISABLE		0x0
> @@ -31,6 +32,7 @@
>   struct bc_timer {
>   	struct clock_event_device ce;
>   	void __iomem *base;
> +	void __iomem *ctrl;
>   	u32 freq;
>   };
> 
> @@ -46,15 +48,20 @@ static inline void __iomem *rk_base(struct 
> clock_event_device *ce)
>   	return rk_timer(ce)->base;
>   }
> 
> +static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
> +{
> +        return rk_timer(ce)->ctrl;
> +}
> +
>   static inline void rk_timer_disable(struct clock_event_device *ce)
>   {
> -	writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
> +	writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
>   }
> 
>   static inline void rk_timer_enable(struct clock_event_device *ce, u32 
> flags)
>   {
>   	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
> -		       rk_base(ce) + TIMER_CONTROL_REG);
> +		       rk_ctrl(ce));
>   }
> 
>   static void rk_timer_update_counter(unsigned long cycles,
> @@ -179,4 +186,18 @@ out_unmap:
>   	iounmap(bc_timer.base);
>   }
> 
> -CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
> +static void __init rk3288_timer_init(struct device_node *np)
> +{
> +	bc_timer.ctrl = TIMER_CONTROL_REG3288;
> +	rk_timer_init(np);
> +}
> +
> +static void __init rk3399_timer_init(struct device_node *np)
> +{
> +        bc_timer.ctrl = TIMER_CONTROL_REG3399;
> +	rk_timer_init(np);
> +}
> +
> +
> +CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", 
> rk3288_timer_init);
> +CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3399-timer", 
> rk3399_timer_init);
> 
> 

I think you mean this patch otherwise compile will fail:
@@ -19,7 +19,8 @@

 #define TIMER_LOAD_COUNT0	0x00
 #define TIMER_LOAD_COUNT1	0x04
-#define TIMER_CONTROL_REG	0x10
+#define TIMER_CONTROL_REG3288	0x10
+#define TIMER_CONTROL_REG3399	0x1C
 #define TIMER_INT_STATUS	0x18

 #define TIMER_DISABLE		0x0
@@ -31,6 +32,7 @@
 struct bc_timer {
 	struct clock_event_device ce;
 	void __iomem *base;
+	u32 ctrl;
 	u32 freq;
 };

@@ -46,15 +48,20 @@ static inline void __iomem *rk_base(struct
clock_event_device *ce)
 	return rk_timer(ce)->base;
 }

+static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
+{
+	return rk_timer(ce)->base + rk_timer(ce)->ctrl;
+}
+
 static inline void rk_timer_disable(struct clock_event_device *ce)
 {
-	writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
+	writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
 }

 static inline void rk_timer_enable(struct clock_event_device *ce, u32
flags)
 {
 	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
-		       rk_base(ce) + TIMER_CONTROL_REG);
+		       rk_ctrl(ce));
 }

 static void rk_timer_update_counter(unsigned long cycles,
@@ -179,4 +186,19 @@ out_unmap:
 	iounmap(bc_timer.base);
 }

-CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
+static void __init rk3288_timer_init(struct device_node *np)
+{
+	bc_timer.ctrl = TIMER_CONTROL_REG3288;
+	rk_timer_init(np);
+}
+
+static void __init rk3399_timer_init(struct device_node *np)
+{
+	bc_timer.ctrl = TIMER_CONTROL_REG3399;
+	rk_timer_init(np);
+}
+
+CLOCKSOURCE_OF_DECLARE(rk3288_timer, "rockchip,rk3288-timer",
+		       rk3288_timer_init);
+CLOCKSOURCE_OF_DECLARE(rk3399_timer, "rockchip,rk3399-timer",
+		       rk3399_timer_init);

This patch will give us a little lager text size. If we do disassemble,
we can see additional LDR is called. I can accept this performance drop.
So we will send new patches.
BTW, the patch "clocksource: rockchip: remove unnecessary clear irq
before request_irq" can drop if we use this patch.

Thanks,
Huang, Tao

Re: [PATCH 4/5] clocksource: rockchip: add support for rk3399 SoC

[Daniel Lezcano]

On 05/31/2016 03:46 PM, Huang, Tao wrote:
> Hi Daniel:
> On 2016年05月31日 07:28, Daniel Lezcano wrote:
>> On 05/25/2016 11:50 AM, Caesar Wang wrote:
>>> From: Huang Tao <huangtao@rock-chips.com>
>>>
>>> The CONTROL register offset is different from old SoCs.
>>> For Linux driver, there are not functional changes at all.
>>> Let's call it v2.
>>>
>>> Signed-off-by: Huang Tao <huangtao@rock-chips.com>
>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Cc: Heiko Stuebner <heiko@sntech.de>
>>> Tested-by: Jianqun Xu <jay.xu@rock-chips.com>
>>> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
>>> ---
>>
>> That's hackish.
> Yes:( I blamed our IC guy.
>>
>> Please consider something like:
>>
>> diff --git a/drivers/clocksource/rockchip_timer.c
>> b/drivers/clocksource/rockchip_timer.c
>> index b991b28..b6ba6f9 100644
>> --- a/drivers/clocksource/rockchip_timer.c
>> +++ b/drivers/clocksource/rockchip_timer.c
>> @@ -19,7 +19,8 @@
>>
>>    #define TIMER_LOAD_COUNT0	0x00
>>    #define TIMER_LOAD_COUNT1	0x04
>> -#define TIMER_CONTROL_REG	0x10
>> +#define TIMER_CONTROL_REG3288	0x10
>> +#define TIMER_CONTROL_REG3399	0x1C
>>    #define TIMER_INT_STATUS	0x18
>>
>>    #define TIMER_DISABLE		0x0
>> @@ -31,6 +32,7 @@
>>    struct bc_timer {
>>    	struct clock_event_device ce;
>>    	void __iomem *base;
>> +	void __iomem *ctrl;
>>    	u32 freq;
>>    };
>>
>> @@ -46,15 +48,20 @@ static inline void __iomem *rk_base(struct
>> clock_event_device *ce)
>>    	return rk_timer(ce)->base;
>>    }
>>
>> +static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
>> +{
>> +        return rk_timer(ce)->ctrl;
>> +}
>> +
>>    static inline void rk_timer_disable(struct clock_event_device *ce)
>>    {
>> -	writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
>> +	writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
>>    }
>>
>>    static inline void rk_timer_enable(struct clock_event_device *ce, u32
>> flags)
>>    {
>>    	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
>> -		       rk_base(ce) + TIMER_CONTROL_REG);
>> +		       rk_ctrl(ce));
>>    }
>>
>>    static void rk_timer_update_counter(unsigned long cycles,
>> @@ -179,4 +186,18 @@ out_unmap:
>>    	iounmap(bc_timer.base);
>>    }
>>
>> -CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
>> +static void __init rk3288_timer_init(struct device_node *np)
>> +{
>> +	bc_timer.ctrl = TIMER_CONTROL_REG3288;
>> +	rk_timer_init(np);
>> +}
>> +
>> +static void __init rk3399_timer_init(struct device_node *np)
>> +{
>> +        bc_timer.ctrl = TIMER_CONTROL_REG3399;
>> +	rk_timer_init(np);
>> +}
>> +
>> +
>> +CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer",
>> rk3288_timer_init);
>> +CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3399-timer",
>> rk3399_timer_init);
>>
>>
>
> I think you mean this patch otherwise compile will fail:
> @@ -19,7 +19,8 @@
>
>   #define TIMER_LOAD_COUNT0	0x00
>   #define TIMER_LOAD_COUNT1	0x04
> -#define TIMER_CONTROL_REG	0x10
> +#define TIMER_CONTROL_REG3288	0x10
> +#define TIMER_CONTROL_REG3399	0x1C
>   #define TIMER_INT_STATUS	0x18
>
>   #define TIMER_DISABLE		0x0
> @@ -31,6 +32,7 @@
>   struct bc_timer {
>   	struct clock_event_device ce;
>   	void __iomem *base;
> +	u32 ctrl;
>   	u32 freq;
>   };
>
> @@ -46,15 +48,20 @@ static inline void __iomem *rk_base(struct
> clock_event_device *ce)
>   	return rk_timer(ce)->base;
>   }
>
> +static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
> +{
> +	return rk_timer(ce)->base + rk_timer(ce)->ctrl;

You can do a small optimization by pre-computing 'ctrl' at init time, so 
no need to do this addition each time.

> +}
> +
>   static inline void rk_timer_disable(struct clock_event_device *ce)
>   {
> -	writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
> +	writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
>   }
>
>   static inline void rk_timer_enable(struct clock_event_device *ce, u32
> flags)
>   {
>   	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
> -		       rk_base(ce) + TIMER_CONTROL_REG);
> +		       rk_ctrl(ce));
>   }
>
>   static void rk_timer_update_counter(unsigned long cycles,
> @@ -179,4 +186,19 @@ out_unmap:
>   	iounmap(bc_timer.base);
>   }
>
> -CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
> +static void __init rk3288_timer_init(struct device_node *np)
> +{
> +	bc_timer.ctrl = TIMER_CONTROL_REG3288;
> +	rk_timer_init(np);

	rk_timer_init(np);
	bc_timer.ctrl = bc_timer.base + TIMER_CONTROL_REG3288;

> +}
> +
> +static void __init rk3399_timer_init(struct device_node *np)
> +{
> +	bc_timer.ctrl = TIMER_CONTROL_REG3399;
> +	rk_timer_init(np);

	rk_timer_init(np);
	bc_timer.ctrl = bc_timer.base + TIMER_CONTROL_REG3399;

> +}
> +
> +CLOCKSOURCE_OF_DECLARE(rk3288_timer, "rockchip,rk3288-timer",
> +		       rk3288_timer_init);
> +CLOCKSOURCE_OF_DECLARE(rk3399_timer, "rockchip,rk3399-timer",
> +		       rk3399_timer_init);
>
> This patch will give us a little lager text size. If we do disassemble,
> we can see additional LDR is called. I can accept this performance drop.
> So we will send new patches.
> BTW, the patch "clocksource: rockchip: remove unnecessary clear irq
> before request_irq" can drop if we use this patch.


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 4/5] clocksource: rockchip: add support for rk3399 SoC

[Huang, Tao]

Hi Daniel:
On 2016年05月31日 22:06, Daniel Lezcano wrote:
>>
>> @@ -46,15 +48,20 @@ static inline void __iomem *rk_base(struct
>> clock_event_device *ce)
>>   	return rk_timer(ce)->base;
>>   }
>>
>> +static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
>> +{
>> +	return rk_timer(ce)->base + rk_timer(ce)->ctrl;
> 
> You can do a small optimization by pre-computing 'ctrl' at init time, so 
> no need to do this addition each time.

I understand what you mean, please see comment below.
And even we use ctrl as pointer, we still will get addition LDR other
then ADD.
This is disassemble code before:
   0:	f9408021 	ldr	x1, [x1,#256]
   4:	52800003 	mov	w3, #0x0
   8:	91004022 	add	x2, x1, #0x10
   c:	b9000043 	str	w3, [x2]
This is disassemble code after change:
   0:	52800003 	mov	w3, #0x0
   4:	f9408422 	ldr	x2, [x1,#264]
   8:	b9000043 	str	w3, [x2]
   c:	f9408021 	ldr	x1, [x1,#256]
Of course we can assume cache hit.

> 
>> +}
>> +
>>   static inline void rk_timer_disable(struct clock_event_device *ce)
>>   {
>> -	writel_relaxed(TIMER_DISABLE, rk_base(ce) + TIMER_CONTROL_REG);
>> +	writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
>>   }
>>
>>   static inline void rk_timer_enable(struct clock_event_device *ce, u32
>> flags)
>>   {
>>   	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
>> -		       rk_base(ce) + TIMER_CONTROL_REG);
>> +		       rk_ctrl(ce));
>>   }
>>
>>   static void rk_timer_update_counter(unsigned long cycles,
>> @@ -179,4 +186,19 @@ out_unmap:
>>   	iounmap(bc_timer.base);
>>   }
>>
>> -CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
>> +static void __init rk3288_timer_init(struct device_node *np)
>> +{
>> +	bc_timer.ctrl = TIMER_CONTROL_REG3288;
>> +	rk_timer_init(np);
> 
> 	rk_timer_init(np);
> 	bc_timer.ctrl = bc_timer.base + TIMER_CONTROL_REG3288;

No. It's not such simple. You will access null pointer when
rk_timer_init, if we keep rk_timer_disable call in init or after
request_irq/clockevents_config_and_register and interrupt happen
immediately.

So the code maybe:
static void __init rk3288_timer_init(struct device_node *np)
{
	bc_timer.base = of_iomap(np, 0);
	if (!bc_timer.base) {
		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
		return;
	}
	bc_timer.ctrl = bc_timer.base + TIMER_CONTROL_REG3288;
	rk_imter_init(np); // of course remove of_iomap from init.

Is this what you want?

Re: [PATCH 4/5] clocksource: rockchip: add support for rk3399 SoC

[Daniel Lezcano]

On 06/01/2016 03:58 AM, Huang, Tao wrote:
> Hi Daniel:
> On 2016年05月31日 22:06, Daniel Lezcano wrote:

[ ... ]

>>> -CLOCKSOURCE_OF_DECLARE(rk_timer, "rockchip,rk3288-timer", rk_timer_init);
>>> +static void __init rk3288_timer_init(struct device_node *np)
>>> +{
>>> +	bc_timer.ctrl = TIMER_CONTROL_REG3288;
>>> +	rk_timer_init(np);
>>
>> 	rk_timer_init(np);
>> 	bc_timer.ctrl = bc_timer.base + TIMER_CONTROL_REG3288;
>
> No. It's not such simple. You will access null pointer when
> rk_timer_init, if we keep rk_timer_disable call in init or after
> request_irq/clockevents_config_and_register and interrupt happen
> immediately.
>
> So the code maybe:
> static void __init rk3288_timer_init(struct device_node *np)
> {
> 	bc_timer.base = of_iomap(np, 0);
> 	if (!bc_timer.base) {
> 		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
> 		return;
> 	}
> 	bc_timer.ctrl = bc_timer.base + TIMER_CONTROL_REG3288;
> 	rk_imter_init(np); // of course remove of_iomap from init.
>
> Is this what you want?

Not necessarily. There are plenty of variants.

eg. rk_timer_init(np, TIMER_CONTROL_REG3288);



-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH 5/5] ARM64: dts: rockchip: add rktimer device node for rk3399

[Caesar Wang]

From: Huang Tao <huangtao@rock-chips.com>

Select rktimer0 as broadcast timer.

Signed-off-by: Huang Tao <huangtao@rock-chips.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Heiko Stuebner <heiko@sntech.de>
Tested-by: Jianqun Xu <jay.xu@rock-chips.com>

Signed-off-by: Caesar Wang <wxt@rock-chips.com>
---

 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index 46f325a..f0c0d76 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -492,6 +492,14 @@
 		interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
 	};
 
+	rktimer: rktimer@ff850000 {
+		compatible = "rockchip,rk3399-timer";
+		reg = <0x0 0xff850000 0x0 0x1000>;
+		interrupts = <GIC_SPI 81 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru PCLK_TIMER0>, <&cru SCLK_TIMER00>;
+		clock-names = "pclk", "timer";
+	};
+
 	spdif: spdif@ff870000 {
 		compatible = "rockchip,rk3399-spdif";
 		reg = <0x0 0xff870000 0x0 0x1000>;
-- 
1.9.1