[PATCH v5 0/8] Implement clocksource for rockchip SoC using rockchip timer

[PATCH v5 0/8] Implement clocksource for rockchip SoC using rockchip timer

[Alexander Kochetkov]

Hello,

This patch series contain:
- devicetree bindings clarification for rockchip timers
- dts files fixes for rk3228-evb, rk3229-evb and rk3188
- implementation of clocksource and sched clock for rockchip SoC

The clock supplying the arm-global-timer on the rk3188 is coming from the
the cpu clock itself and thus changes its rate everytime cpufreq adjusts
the cpu frequency making this timer unsuitable as a stable clocksource.

The rk3188, rk3288 and following socs share a separate timer block already
handled by the rockchip-timer driver. Therefore adapt this driver to also
be able to act as clocksource on rk3188.

In order to test clocksource you can run following commands and check
how much time it take in real. On rk3188 it take about ~45 seconds.

       cpufreq-set -f 1.6GHZ
       date; sleep 60; date

rk3288 (and probably anything newer) is irrelevant to this patch,
as it has the arch timer interface. This patch may be usefull
for Cortex-A9/A5 based parts.

Regards,
Alexander.

Changes in v5:
- Add 'Acked-by: Rob Herring <robh@kernel.org>' to 1/8
  http://lists.infradead.org/pipermail/linux-rockchip/2016-December/013308.html
- Add 'Reviwed-by: Heiko Stübner <heiko@sntech.de>' to series
- change timer compatible property in the rk322x.dtsi 2/8
  http://lists.infradead.org/pipermail/linux-rockchip/2017-January/013784.html
- updated comment message for 4/8
  http://lists.infradead.org/pipermail/linux-rockchip/2017-January/013786.html
- updated comment message for 5/8
  http://lists.infradead.org/pipermail/linux-rockchip/2017-January/013787.html
- fixed build error for 8/8
  http://lists.infradead.org/pipermail/linux-rockchip/2017-January/013789.html

Changes in v4:
merged 7 and 8 from series 3
merged 10, 11, 12, 13 from series 3
fixed commit message

Changes in v3:
added patches:
ARM: dts: rockchip: disable arm-global-timer for rk3188
clocksource/drivers/rockchip_timer: Prevent ftrace recursion

devicetree v1 patches:
https://patchwork.ozlabs.org/patch/699019/
https://patchwork.ozlabs.org/patch/699020/

kernel v1 patches:
https://patchwork.kernel.org/patch/9443975/
https://patchwork.kernel.org/patch/9443971/
https://patchwork.kernel.org/patch/9443959/
https://patchwork.kernel.org/patch/9443963/
https://patchwork.kernel.org/patch/9443979/
https://patchwork.kernel.org/patch/9443989/
https://patchwork.kernel.org/patch/9443987/
https://patchwork.kernel.org/patch/9443977/
https://patchwork.kernel.org/patch/9443991/

Alexander Kochetkov (8):
  dt-bindings: clarify compatible property for rockchip timers
  ARM: dts: rockchip: update compatible property for rk322x timer
  ARM: dts: rockchip: add timer entries to rk3188 SoC
  ARM: dts: rockchip: disable arm-global-timer for rk3188
  clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and
    rk_clock_event_device
  clocksource/drivers/rockchip_timer: low level routines take rk_timer
    as parameter
  clocksource/drivers/rockchip_timer: move TIMER_INT_UNMASK out of
    rk_timer_enable()
  clocksource/drivers/rockchip_timer: implement clocksource timer

 .../bindings/timer/rockchip,rk-timer.txt           |   12 +-
 arch/arm/boot/dts/rk3188.dtsi                      |   17 ++
 arch/arm/boot/dts/rk322x.dtsi                      |    2 +-
 drivers/clocksource/rockchip_timer.c               |  207 +++++++++++++++-----
 4 files changed, 183 insertions(+), 55 deletions(-)

-- 
1.7.9.5

[PATCH v5 0/8] Implement clocksource for rockchip SoC using rockchip timer

[Alexander Kochetkov]

Hello,

This patch series contain:
- devicetree bindings clarification for rockchip timers
- dts files fixes for rk3228-evb, rk3229-evb and rk3188
- implementation of clocksource and sched clock for rockchip SoC

The clock supplying the arm-global-timer on the rk3188 is coming from the
the cpu clock itself and thus changes its rate everytime cpufreq adjusts
the cpu frequency making this timer unsuitable as a stable clocksource.

The rk3188, rk3288 and following socs share a separate timer block already
handled by the rockchip-timer driver. Therefore adapt this driver to also
be able to act as clocksource on rk3188.

In order to test clocksource you can run following commands and check
how much time it take in real. On rk3188 it take about ~45 seconds.

       cpufreq-set -f 1.6GHZ
       date; sleep 60; date

rk3288 (and probably anything newer) is irrelevant to this patch,
as it has the arch timer interface. This patch may be usefull
for Cortex-A9/A5 based parts.

Regards,
Alexander.

Changes in v5:
- Add 'Acked-by: Rob Herring <robh@kernel.org>' to 1/8
  http://lists.infradead.org/pipermail/linux-rockchip/2016-December/013308.html
- Add 'Reviwed-by: Heiko St?bner <heiko@sntech.de>' to series
- change timer compatible property in the rk322x.dtsi 2/8
  http://lists.infradead.org/pipermail/linux-rockchip/2017-January/013784.html
- updated comment message for 4/8
  http://lists.infradead.org/pipermail/linux-rockchip/2017-January/013786.html
- updated comment message for 5/8
  http://lists.infradead.org/pipermail/linux-rockchip/2017-January/013787.html
- fixed build error for 8/8
  http://lists.infradead.org/pipermail/linux-rockchip/2017-January/013789.html

Changes in v4:
merged 7 and 8 from series 3
merged 10, 11, 12, 13 from series 3
fixed commit message

Changes in v3:
added patches:
ARM: dts: rockchip: disable arm-global-timer for rk3188
clocksource/drivers/rockchip_timer: Prevent ftrace recursion

devicetree v1 patches:
https://patchwork.ozlabs.org/patch/699019/
https://patchwork.ozlabs.org/patch/699020/

kernel v1 patches:
https://patchwork.kernel.org/patch/9443975/
https://patchwork.kernel.org/patch/9443971/
https://patchwork.kernel.org/patch/9443959/
https://patchwork.kernel.org/patch/9443963/
https://patchwork.kernel.org/patch/9443979/
https://patchwork.kernel.org/patch/9443989/
https://patchwork.kernel.org/patch/9443987/
https://patchwork.kernel.org/patch/9443977/
https://patchwork.kernel.org/patch/9443991/

Alexander Kochetkov (8):
  dt-bindings: clarify compatible property for rockchip timers
  ARM: dts: rockchip: update compatible property for rk322x timer
  ARM: dts: rockchip: add timer entries to rk3188 SoC
  ARM: dts: rockchip: disable arm-global-timer for rk3188
  clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and
    rk_clock_event_device
  clocksource/drivers/rockchip_timer: low level routines take rk_timer
    as parameter
  clocksource/drivers/rockchip_timer: move TIMER_INT_UNMASK out of
    rk_timer_enable()
  clocksource/drivers/rockchip_timer: implement clocksource timer

 .../bindings/timer/rockchip,rk-timer.txt           |   12 +-
 arch/arm/boot/dts/rk3188.dtsi                      |   17 ++
 arch/arm/boot/dts/rk322x.dtsi                      |    2 +-
 drivers/clocksource/rockchip_timer.c               |  207 +++++++++++++++-----
 4 files changed, 183 insertions(+), 55 deletions(-)

-- 
1.7.9.5

[PATCH v5 0/8] Implement clocksource for rockchip SoC using rockchip timer

[Alexander Kochetkov]

Hello,

This patch series contain:
- devicetree bindings clarification for rockchip timers
- dts files fixes for rk3228-evb, rk3229-evb and rk3188
- implementation of clocksource and sched clock for rockchip SoC

The clock supplying the arm-global-timer on the rk3188 is coming from the
the cpu clock itself and thus changes its rate everytime cpufreq adjusts
the cpu frequency making this timer unsuitable as a stable clocksource.

The rk3188, rk3288 and following socs share a separate timer block already
handled by the rockchip-timer driver. Therefore adapt this driver to also
be able to act as clocksource on rk3188.

In order to test clocksource you can run following commands and check
how much time it take in real. On rk3188 it take about ~45 seconds.

       cpufreq-set -f 1.6GHZ
       date; sleep 60; date

rk3288 (and probably anything newer) is irrelevant to this patch,
as it has the arch timer interface. This patch may be usefull
for Cortex-A9/A5 based parts.

Regards,
Alexander.

Changes in v5:
- Add 'Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>' to 1/8
  http://lists.infradead.org/pipermail/linux-rockchip/2016-December/013308.html
- Add 'Reviwed-by: Heiko Stübner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>' to series
- change timer compatible property in the rk322x.dtsi 2/8
  http://lists.infradead.org/pipermail/linux-rockchip/2017-January/013784.html
- updated comment message for 4/8
  http://lists.infradead.org/pipermail/linux-rockchip/2017-January/013786.html
- updated comment message for 5/8
  http://lists.infradead.org/pipermail/linux-rockchip/2017-January/013787.html
- fixed build error for 8/8
  http://lists.infradead.org/pipermail/linux-rockchip/2017-January/013789.html

Changes in v4:
merged 7 and 8 from series 3
merged 10, 11, 12, 13 from series 3
fixed commit message

Changes in v3:
added patches:
ARM: dts: rockchip: disable arm-global-timer for rk3188
clocksource/drivers/rockchip_timer: Prevent ftrace recursion

devicetree v1 patches:
https://patchwork.ozlabs.org/patch/699019/
https://patchwork.ozlabs.org/patch/699020/

kernel v1 patches:
https://patchwork.kernel.org/patch/9443975/
https://patchwork.kernel.org/patch/9443971/
https://patchwork.kernel.org/patch/9443959/
https://patchwork.kernel.org/patch/9443963/
https://patchwork.kernel.org/patch/9443979/
https://patchwork.kernel.org/patch/9443989/
https://patchwork.kernel.org/patch/9443987/
https://patchwork.kernel.org/patch/9443977/
https://patchwork.kernel.org/patch/9443991/

Alexander Kochetkov (8):
  dt-bindings: clarify compatible property for rockchip timers
  ARM: dts: rockchip: update compatible property for rk322x timer
  ARM: dts: rockchip: add timer entries to rk3188 SoC
  ARM: dts: rockchip: disable arm-global-timer for rk3188
  clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and
    rk_clock_event_device
  clocksource/drivers/rockchip_timer: low level routines take rk_timer
    as parameter
  clocksource/drivers/rockchip_timer: move TIMER_INT_UNMASK out of
    rk_timer_enable()
  clocksource/drivers/rockchip_timer: implement clocksource timer

 .../bindings/timer/rockchip,rk-timer.txt           |   12 +-
 arch/arm/boot/dts/rk3188.dtsi                      |   17 ++
 arch/arm/boot/dts/rk322x.dtsi                      |    2 +-
 drivers/clocksource/rockchip_timer.c               |  207 +++++++++++++++-----
 4 files changed, 183 insertions(+), 55 deletions(-)

-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 1/8] dt-bindings: clarify compatible property for rockchip timers

[Alexander Kochetkov]

Make all properties description in form '"rockchip,<chip>-timer",
"rockchip,rk3288-timer"' for all chips found in linux kernel.

Suggested-by: Heiko Stübner <heiko@sntech.de>
Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../bindings/timer/rockchip,rk-timer.txt           |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt b/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
index a41b184..16a5f45 100644
--- a/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
+++ b/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
@@ -1,9 +1,15 @@
 Rockchip rk timer
 
 Required properties:
-- compatible: shall be one of:
-  "rockchip,rk3288-timer" - for rk3066, rk3036, rk3188, rk322x, rk3288, rk3368
-  "rockchip,rk3399-timer" - for rk3399
+- compatible: should be:
+  "rockchip,rk3036-timer", "rockchip,rk3288-timer": for Rockchip RK3036
+  "rockchip,rk3066-timer", "rockchip,rk3288-timer": for Rockchip RK3066
+  "rockchip,rk3188-timer", "rockchip,rk3288-timer": for Rockchip RK3188
+  "rockchip,rk3228-timer", "rockchip,rk3288-timer": for Rockchip RK3228
+  "rockchip,rk3229-timer", "rockchip,rk3288-timer": for Rockchip RK3229
+  "rockchip,rk3288-timer": for Rockchip RK3288
+  "rockchip,rk3368-timer", "rockchip,rk3288-timer": for Rockchip RK3368
+  "rockchip,rk3399-timer": for Rockchip 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.7.9.5

[PATCH v5 1/8] dt-bindings: clarify compatible property for rockchip timers

[Alexander Kochetkov]

Make all properties description in form '"rockchip,<chip>-timer",
"rockchip,rk3288-timer"' for all chips found in linux kernel.

Suggested-by: Heiko St?bner <heiko@sntech.de>
Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../bindings/timer/rockchip,rk-timer.txt           |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt b/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
index a41b184..16a5f45 100644
--- a/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
+++ b/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
@@ -1,9 +1,15 @@
 Rockchip rk timer
 
 Required properties:
-- compatible: shall be one of:
-  "rockchip,rk3288-timer" - for rk3066, rk3036, rk3188, rk322x, rk3288, rk3368
-  "rockchip,rk3399-timer" - for rk3399
+- compatible: should be:
+  "rockchip,rk3036-timer", "rockchip,rk3288-timer": for Rockchip RK3036
+  "rockchip,rk3066-timer", "rockchip,rk3288-timer": for Rockchip RK3066
+  "rockchip,rk3188-timer", "rockchip,rk3288-timer": for Rockchip RK3188
+  "rockchip,rk3228-timer", "rockchip,rk3288-timer": for Rockchip RK3228
+  "rockchip,rk3229-timer", "rockchip,rk3288-timer": for Rockchip RK3229
+  "rockchip,rk3288-timer": for Rockchip RK3288
+  "rockchip,rk3368-timer", "rockchip,rk3288-timer": for Rockchip RK3368
+  "rockchip,rk3399-timer": for Rockchip 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.7.9.5

[PATCH v5 1/8] dt-bindings: clarify compatible property for rockchip timers

[Alexander Kochetkov]

Make all properties description in form '"rockchip,<chip>-timer",
"rockchip,rk3288-timer"' for all chips found in linux kernel.

Suggested-by: Heiko Stübner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 .../bindings/timer/rockchip,rk-timer.txt           |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt b/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
index a41b184..16a5f45 100644
--- a/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
+++ b/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
@@ -1,9 +1,15 @@
 Rockchip rk timer
 
 Required properties:
-- compatible: shall be one of:
-  "rockchip,rk3288-timer" - for rk3066, rk3036, rk3188, rk322x, rk3288, rk3368
-  "rockchip,rk3399-timer" - for rk3399
+- compatible: should be:
+  "rockchip,rk3036-timer", "rockchip,rk3288-timer": for Rockchip RK3036
+  "rockchip,rk3066-timer", "rockchip,rk3288-timer": for Rockchip RK3066
+  "rockchip,rk3188-timer", "rockchip,rk3288-timer": for Rockchip RK3188
+  "rockchip,rk3228-timer", "rockchip,rk3288-timer": for Rockchip RK3228
+  "rockchip,rk3229-timer", "rockchip,rk3288-timer": for Rockchip RK3229
+  "rockchip,rk3288-timer": for Rockchip RK3288
+  "rockchip,rk3368-timer", "rockchip,rk3288-timer": for Rockchip RK3368
+  "rockchip,rk3399-timer": for Rockchip 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.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 2/8] ARM: dts: rockchip: update compatible property for rk322x timer

[Alexander Kochetkov]

Property set to '"rockchip,rk3228-timer", "rockchip,rk3288-timer"'
to match devicetree bindings.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Suggested-by: Heiko Stübner <heiko@sntech.de>
---
 arch/arm/boot/dts/rk322x.dtsi |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/rk322x.dtsi b/arch/arm/boot/dts/rk322x.dtsi
index 9d3aee5..aaeacf8 100644
--- a/arch/arm/boot/dts/rk322x.dtsi
+++ b/arch/arm/boot/dts/rk322x.dtsi
@@ -325,7 +325,7 @@
 	};
 
 	timer: timer@110c0000 {
-		compatible = "rockchip,rk3288-timer";
+		compatible = "rockchip,rk3228-timer", "rockchip,rk3288-timer";
 		reg = <0x110c0000 0x20>;
 		interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&xin24m>, <&cru PCLK_TIMER>;
-- 
1.7.9.5

[PATCH v5 2/8] ARM: dts: rockchip: update compatible property for rk322x timer

[Alexander Kochetkov]

Property set to '"rockchip,rk3228-timer", "rockchip,rk3288-timer"'
to match devicetree bindings.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Suggested-by: Heiko St?bner <heiko@sntech.de>
---
 arch/arm/boot/dts/rk322x.dtsi |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/rk322x.dtsi b/arch/arm/boot/dts/rk322x.dtsi
index 9d3aee5..aaeacf8 100644
--- a/arch/arm/boot/dts/rk322x.dtsi
+++ b/arch/arm/boot/dts/rk322x.dtsi
@@ -325,7 +325,7 @@
 	};
 
 	timer: timer at 110c0000 {
-		compatible = "rockchip,rk3288-timer";
+		compatible = "rockchip,rk3228-timer", "rockchip,rk3288-timer";
 		reg = <0x110c0000 0x20>;
 		interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&xin24m>, <&cru PCLK_TIMER>;
-- 
1.7.9.5

[PATCH v5 3/8] ARM: dts: rockchip: add timer entries to rk3188 SoC

[Alexander Kochetkov]

The patch add two timers to all rk3188 based boards.

The first timer is from alive subsystem and it act as a backup
for the local timers at sleep time. It act the same as other
SoC rockchip timers already present in kernel.

The second timer is from CPU subsystem and act as replacement
for the arm-global-timer clocksource and sched clock. It run
at stable frequency 24MHz.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Reviwed-by: Heiko Stübner <heiko@sntech.de>
---
 arch/arm/boot/dts/rk3188.dtsi |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
index 869e189..bcf8e03 100644
--- a/arch/arm/boot/dts/rk3188.dtsi
+++ b/arch/arm/boot/dts/rk3188.dtsi
@@ -106,6 +106,22 @@
 		};
 	};
 
+	timer3: timer@2000e000 {
+		compatible = "rockchip,rk3188-timer", "rockchip,rk3288-timer";
+		reg = <0x2000e000 0x20>;
+		interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru SCLK_TIMER3>, <&cru PCLK_TIMER3>;
+		clock-names = "timer", "pclk";
+	};
+
+	timer6: timer@200380a0 {
+		compatible = "rockchip,rk3188-timer", "rockchip,rk3288-timer";
+		reg = <0x200380a0 0x20>;
+		interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru SCLK_TIMER6>, <&cru PCLK_TIMER0>;
+		clock-names = "timer", "pclk";
+	};
+
 	i2s0: i2s@1011a000 {
 		compatible = "rockchip,rk3188-i2s", "rockchip,rk3066-i2s";
 		reg = <0x1011a000 0x2000>;
-- 
1.7.9.5

[PATCH v5 3/8] ARM: dts: rockchip: add timer entries to rk3188 SoC

[Alexander Kochetkov]

The patch add two timers to all rk3188 based boards.

The first timer is from alive subsystem and it act as a backup
for the local timers at sleep time. It act the same as other
SoC rockchip timers already present in kernel.

The second timer is from CPU subsystem and act as replacement
for the arm-global-timer clocksource and sched clock. It run
at stable frequency 24MHz.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Reviwed-by: Heiko St?bner <heiko@sntech.de>
---
 arch/arm/boot/dts/rk3188.dtsi |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
index 869e189..bcf8e03 100644
--- a/arch/arm/boot/dts/rk3188.dtsi
+++ b/arch/arm/boot/dts/rk3188.dtsi
@@ -106,6 +106,22 @@
 		};
 	};
 
+	timer3: timer at 2000e000 {
+		compatible = "rockchip,rk3188-timer", "rockchip,rk3288-timer";
+		reg = <0x2000e000 0x20>;
+		interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru SCLK_TIMER3>, <&cru PCLK_TIMER3>;
+		clock-names = "timer", "pclk";
+	};
+
+	timer6: timer at 200380a0 {
+		compatible = "rockchip,rk3188-timer", "rockchip,rk3288-timer";
+		reg = <0x200380a0 0x20>;
+		interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru SCLK_TIMER6>, <&cru PCLK_TIMER0>;
+		clock-names = "timer", "pclk";
+	};
+
 	i2s0: i2s at 1011a000 {
 		compatible = "rockchip,rk3188-i2s", "rockchip,rk3066-i2s";
 		reg = <0x1011a000 0x2000>;
-- 
1.7.9.5

Re: [PATCH v5 3/8] ARM: dts: rockchip: add timer entries to rk3188 SoC

[Daniel Lezcano]

On Tue, Jan 24, 2017 at 03:16:38PM +0300, Alexander Kochetkov wrote:
> The patch add two timers to all rk3188 based boards.
> 
> The first timer is from alive subsystem and it act as a backup
> for the local timers at sleep time. It act the same as other
> SoC rockchip timers already present in kernel.

It is up to the RTC to wake up the system from suspend and there is no
idle state stopping the local timers on these SoCs. So, the rockchip timers
won't be ever used on the rk3188, right ?
 
> The second timer is from CPU subsystem and act as replacement
> for the arm-global-timer clocksource and sched clock. It run
> at stable frequency 24MHz.

This patch should go after the rockchip timer patches.

> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> Reviwed-by: Heiko Stübner <heiko@sntech.de>
> ---
>  arch/arm/boot/dts/rk3188.dtsi |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
> index 869e189..bcf8e03 100644
> --- a/arch/arm/boot/dts/rk3188.dtsi
> +++ b/arch/arm/boot/dts/rk3188.dtsi
> @@ -106,6 +106,22 @@
>  		};
>  	};
>  
> +	timer3: timer@2000e000 {
> +		compatible = "rockchip,rk3188-timer", "rockchip,rk3288-timer";
> +		reg = <0x2000e000 0x20>;
> +		interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru SCLK_TIMER3>, <&cru PCLK_TIMER3>;
> +		clock-names = "timer", "pclk";
> +	};
> +
> +	timer6: timer@200380a0 {
> +		compatible = "rockchip,rk3188-timer", "rockchip,rk3288-timer";
> +		reg = <0x200380a0 0x20>;
> +		interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru SCLK_TIMER6>, <&cru PCLK_TIMER0>;
> +		clock-names = "timer", "pclk";
> +	};
> +
>  	i2s0: i2s@1011a000 {
>  		compatible = "rockchip,rk3188-i2s", "rockchip,rk3066-i2s";
>  		reg = <0x1011a000 0x2000>;
> -- 
> 1.7.9.5
> 

-- 

 <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 v5 3/8] ARM: dts: rockchip: add timer entries to rk3188 SoC

[Daniel Lezcano]

On Tue, Jan 24, 2017 at 03:16:38PM +0300, Alexander Kochetkov wrote:
> The patch add two timers to all rk3188 based boards.
> 
> The first timer is from alive subsystem and it act as a backup
> for the local timers at sleep time. It act the same as other
> SoC rockchip timers already present in kernel.

It is up to the RTC to wake up the system from suspend and there is no
idle state stopping the local timers on these SoCs. So, the rockchip timers
won't be ever used on the rk3188, right ?
 
> The second timer is from CPU subsystem and act as replacement
> for the arm-global-timer clocksource and sched clock. It run
> at stable frequency 24MHz.

This patch should go after the rockchip timer patches.

> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> Reviwed-by: Heiko St?bner <heiko@sntech.de>
> ---
>  arch/arm/boot/dts/rk3188.dtsi |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
> index 869e189..bcf8e03 100644
> --- a/arch/arm/boot/dts/rk3188.dtsi
> +++ b/arch/arm/boot/dts/rk3188.dtsi
> @@ -106,6 +106,22 @@
>  		};
>  	};
>  
> +	timer3: timer at 2000e000 {
> +		compatible = "rockchip,rk3188-timer", "rockchip,rk3288-timer";
> +		reg = <0x2000e000 0x20>;
> +		interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru SCLK_TIMER3>, <&cru PCLK_TIMER3>;
> +		clock-names = "timer", "pclk";
> +	};
> +
> +	timer6: timer at 200380a0 {
> +		compatible = "rockchip,rk3188-timer", "rockchip,rk3288-timer";
> +		reg = <0x200380a0 0x20>;
> +		interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru SCLK_TIMER6>, <&cru PCLK_TIMER0>;
> +		clock-names = "timer", "pclk";
> +	};
> +
>  	i2s0: i2s at 1011a000 {
>  		compatible = "rockchip,rk3188-i2s", "rockchip,rk3066-i2s";
>  		reg = <0x1011a000 0x2000>;
> -- 
> 1.7.9.5
> 

-- 

 <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 v5 3/8] ARM: dts: rockchip: add timer entries to rk3188 SoC

[Daniel Lezcano]

On Tue, Jan 24, 2017 at 03:16:38PM +0300, Alexander Kochetkov wrote:
> The patch add two timers to all rk3188 based boards.
> 
> The first timer is from alive subsystem and it act as a backup
> for the local timers at sleep time. It act the same as other
> SoC rockchip timers already present in kernel.

It is up to the RTC to wake up the system from suspend and there is no
idle state stopping the local timers on these SoCs. So, the rockchip timers
won't be ever used on the rk3188, right ?
 
> The second timer is from CPU subsystem and act as replacement
> for the arm-global-timer clocksource and sched clock. It run
> at stable frequency 24MHz.

This patch should go after the rockchip timer patches.

> Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reviwed-by: Heiko Stübner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> ---
>  arch/arm/boot/dts/rk3188.dtsi |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
> index 869e189..bcf8e03 100644
> --- a/arch/arm/boot/dts/rk3188.dtsi
> +++ b/arch/arm/boot/dts/rk3188.dtsi
> @@ -106,6 +106,22 @@
>  		};
>  	};
>  
> +	timer3: timer@2000e000 {
> +		compatible = "rockchip,rk3188-timer", "rockchip,rk3288-timer";
> +		reg = <0x2000e000 0x20>;
> +		interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru SCLK_TIMER3>, <&cru PCLK_TIMER3>;
> +		clock-names = "timer", "pclk";
> +	};
> +
> +	timer6: timer@200380a0 {
> +		compatible = "rockchip,rk3188-timer", "rockchip,rk3288-timer";
> +		reg = <0x200380a0 0x20>;
> +		interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru SCLK_TIMER6>, <&cru PCLK_TIMER0>;
> +		clock-names = "timer", "pclk";
> +	};
> +
>  	i2s0: i2s@1011a000 {
>  		compatible = "rockchip,rk3188-i2s", "rockchip,rk3066-i2s";
>  		reg = <0x1011a000 0x2000>;
> -- 
> 1.7.9.5
> 

-- 

 <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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 3/8] ARM: dts: rockchip: add timer entries to rk3188 SoC

[Alexander Kochetkov]

> 30 янв. 2017 г., в 14:04, Daniel Lezcano <daniel.lezcano@linaro.org> написал(а):
> 
> t is up to the RTC to wake up the system from suspend and there is no
> idle state stopping the local timers on these SoCs. So, the rockchip timers
> won't be ever used on the rk3188, right ?

No rockchip timers where used on rk3188 before.

Actually, I don’t know how to test backup timer functionality. rk3228 backup timer is from alive
subsystem. It is unclear from TRM what this mean, I assume that somehow related to CLK
during suspend. So I pick the timer from alive subsystem on rk3188 also.
DT timer configuration where tested: interrupts and IO works.

> This patch should go after the rockchip timer patches


Ok, let me know, when I should send v6. Or may be you change the order of the patches
during merge.

And thank you for fixing commit message for previous patch.
Feel free to tell me than I write something really unclear or incorrect.

Alexander.

[PATCH v5 3/8] ARM: dts: rockchip: add timer entries to rk3188 SoC

[Alexander Kochetkov]

> 30 ???. 2017 ?., ? 14:04, Daniel Lezcano <daniel.lezcano@linaro.org> ???????(?):
> 
> t is up to the RTC to wake up the system from suspend and there is no
> idle state stopping the local timers on these SoCs. So, the rockchip timers
> won't be ever used on the rk3188, right ?

No rockchip timers where used on rk3188 before.

Actually, I don?t know how to test backup timer functionality. rk3228 backup timer is from alive
subsystem. It is unclear from TRM what this mean, I assume that somehow related to CLK
during suspend. So I pick the timer from alive subsystem on rk3188 also.
DT timer configuration where tested: interrupts and IO works.

> This patch should go after the rockchip timer patches


Ok, let me know, when I should send v6. Or may be you change the order of the patches
during merge.

And thank you for fixing commit message for previous patch.
Feel free to tell me than I write something really unclear or incorrect.

Alexander.

Re: [PATCH v5 3/8] ARM: dts: rockchip: add timer entries to rk3188 SoC

[Daniel Lezcano]

On Mon, Jan 30, 2017 at 02:20:13PM +0300, Alexander Kochetkov wrote:
> 
> > 30 янв. 2017 г., в 14:04, Daniel Lezcano <daniel.lezcano@linaro.org> написал(а):
> > 
> > t is up to the RTC to wake up the system from suspend and there is no
> > idle state stopping the local timers on these SoCs. So, the rockchip timers
> > won't be ever used on the rk3188, right ?
> 
> No rockchip timers where used on rk3188 before.
> 
> Actually, I don’t know how to test backup timer functionality. rk3228 backup timer is from alive
> subsystem. It is unclear from TRM what this mean, 

It means it is on an always-on power domain, so it will be always powered even
in suspend mode.

There are different clockevents on the SoC, the local timers (smp_twd) and the
rockchip timer. They are not used together, the local timers will be used as
default because they are per cpu. So the rockchip timer is not used in this case.

Then there is the situation when the cpu is powered down, the local timers,
which belongs to the same power domain, will be powered down also. If there is a
timer set for this CPU, an alternate, backup timer must be used, and that is the
rockchip timer. However this situation happens:

 - when a CPU is offlined. But the timers are moved to another CPU.
 - when the CPU enters a deep idle state. But such state does not exist on rk3188
 - when the system goes to suspend. But the timers are stopped in any case and
   CPU0 is always on.

There is no case when the rockchip timer is used for the clockevent.

If I'm not wrong, you can check /proc/interrupts and see there is no interrupt
on the rockchip timer.

So the board is typically a case where we want to use a clocksource from one IP
and the clockevents from another IP (just looping back with the clkevt macro
patch I answered a few minutes ago).

That does not call in question this patch as it describes the hardware.

> I assume that somehow related to CLK
> during suspend. So I pick the timer from alive subsystem on rk3188 also.
> DT timer configuration where tested: interrupts and IO works.
> 
> > This patch should go after the rockchip timer patches
> 
> 
> Ok, let me know, when I should send v6. Or may be you change the order of the patches
> during merge.

I have to review the last 3 patches of the series. I will let you know for a V6.

Thanks.

  -- Daniel

-- 

 <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 v5 3/8] ARM: dts: rockchip: add timer entries to rk3188 SoC

[Daniel Lezcano]

On Mon, Jan 30, 2017 at 02:20:13PM +0300, Alexander Kochetkov wrote:
> 
> > 30 ???. 2017 ?., ? 14:04, Daniel Lezcano <daniel.lezcano@linaro.org> ???????(?):
> > 
> > t is up to the RTC to wake up the system from suspend and there is no
> > idle state stopping the local timers on these SoCs. So, the rockchip timers
> > won't be ever used on the rk3188, right ?
> 
> No rockchip timers where used on rk3188 before.
> 
> Actually, I don?t know how to test backup timer functionality. rk3228 backup timer is from alive
> subsystem. It is unclear from TRM what this mean, 

It means it is on an always-on power domain, so it will be always powered even
in suspend mode.

There are different clockevents on the SoC, the local timers (smp_twd) and the
rockchip timer. They are not used together, the local timers will be used as
default because they are per cpu. So the rockchip timer is not used in this case.

Then there is the situation when the cpu is powered down, the local timers,
which belongs to the same power domain, will be powered down also. If there is a
timer set for this CPU, an alternate, backup timer must be used, and that is the
rockchip timer. However this situation happens:

 - when a CPU is offlined. But the timers are moved to another CPU.
 - when the CPU enters a deep idle state. But such state does not exist on rk3188
 - when the system goes to suspend. But the timers are stopped in any case and
   CPU0 is always on.

There is no case when the rockchip timer is used for the clockevent.

If I'm not wrong, you can check /proc/interrupts and see there is no interrupt
on the rockchip timer.

So the board is typically a case where we want to use a clocksource from one IP
and the clockevents from another IP (just looping back with the clkevt macro
patch I answered a few minutes ago).

That does not call in question this patch as it describes the hardware.

> I assume that somehow related to CLK
> during suspend. So I pick the timer from alive subsystem on rk3188 also.
> DT timer configuration where tested: interrupts and IO works.
> 
> > This patch should go after the rockchip timer patches
> 
> 
> Ok, let me know, when I should send v6. Or may be you change the order of the patches
> during merge.

I have to review the last 3 patches of the series. I will let you know for a V6.

Thanks.

  -- Daniel

-- 

 <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 v5 3/8] ARM: dts: rockchip: add timer entries to rk3188 SoC

[Alexander Kochetkov]

> 30 янв. 2017 г., в 15:04, Daniel Lezcano <daniel.lezcano@linaro.org> написал(а):
> 
> There is no case when the rockchip timer is used for the clockevent.
The is already timer entry for rk3228 in the DT. And it act as clockevent. I guess it work as backup,
but I cannot test it also. In order to not break DT compatibility I had to provide one timer to be
initialized as clockevent. And implemented implicit rule the driver (first DT timer - clockevent,
second DT timer - clocksource) already exists for other timers.

> If I'm not wrong, you can check /proc/interrupts and see there is no interrupt
> on the rockchip timer.

Yes, you right here. I’ve temporary disable smp_twd to test rockchip timer interrupts.
There is no interrupt on the rockchip timer during normal work.

> - when the CPU enters a deep idle state. But such state does not exist on rk3188
ARM chip/revision specific?

> - when the system goes to suspend. But the timers are stopped in any case and
>   CPU0 is always on.
There is timer for rk3228 in the DT. I guess there are situations where it can be used.
May be the situations are also acceptable for rk3188? May be something like suspend to RAM or
suspend to HDD? I am not expert in that questions. I can do some tests if you give hint/link.

So, as I understood you suggest to leave only one timer what can be used as clocksource
only. I can implement that, but there should be DT rule what allow to setup timer
as clocksource only. I cannot do more without timer framework support.
Looks, like I have to wait your patch to implement that.

Alexander.

[PATCH v5 3/8] ARM: dts: rockchip: add timer entries to rk3188 SoC

[Alexander Kochetkov]

> 30 ???. 2017 ?., ? 15:04, Daniel Lezcano <daniel.lezcano@linaro.org> ???????(?):
> 
> There is no case when the rockchip timer is used for the clockevent.
The is already timer entry for rk3228 in the DT. And it act as clockevent. I guess it work as backup,
but I cannot test it also. In order to not break DT compatibility I had to provide one timer to be
initialized as clockevent. And implemented implicit rule the driver (first DT timer - clockevent,
second DT timer - clocksource) already exists for other timers.

> If I'm not wrong, you can check /proc/interrupts and see there is no interrupt
> on the rockchip timer.

Yes, you right here. I?ve temporary disable smp_twd to test rockchip timer interrupts.
There is no interrupt on the rockchip timer during normal work.

> - when the CPU enters a deep idle state. But such state does not exist on rk3188
ARM chip/revision specific?

> - when the system goes to suspend. But the timers are stopped in any case and
>   CPU0 is always on.
There is timer for rk3228 in the DT. I guess there are situations where it can be used.
May be the situations are also acceptable for rk3188? May be something like suspend to RAM or
suspend to HDD? I am not expert in that questions. I can do some tests if you give hint/link.

So, as I understood you suggest to leave only one timer what can be used as clocksource
only. I can implement that, but there should be DT rule what allow to setup timer
as clocksource only. I cannot do more without timer framework support.
Looks, like I have to wait your patch to implement that.

Alexander.

Re: [PATCH v5 3/8] ARM: dts: rockchip: add timer entries to rk3188 SoC

[Alexander Kochetkov]

> 30 янв. 2017 г., в 15:04, Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> написал(а):
> 
> There is no case when the rockchip timer is used for the clockevent.
The is already timer entry for rk3228 in the DT. And it act as clockevent. I guess it work as backup,
but I cannot test it also. In order to not break DT compatibility I had to provide one timer to be
initialized as clockevent. And implemented implicit rule the driver (first DT timer - clockevent,
second DT timer - clocksource) already exists for other timers.

> If I'm not wrong, you can check /proc/interrupts and see there is no interrupt
> on the rockchip timer.

Yes, you right here. I’ve temporary disable smp_twd to test rockchip timer interrupts.
There is no interrupt on the rockchip timer during normal work.

> - when the CPU enters a deep idle state. But such state does not exist on rk3188
ARM chip/revision specific?

> - when the system goes to suspend. But the timers are stopped in any case and
>   CPU0 is always on.
There is timer for rk3228 in the DT. I guess there are situations where it can be used.
May be the situations are also acceptable for rk3188? May be something like suspend to RAM or
suspend to HDD? I am not expert in that questions. I can do some tests if you give hint/link.

So, as I understood you suggest to leave only one timer what can be used as clocksource
only. I can implement that, but there should be DT rule what allow to setup timer
as clocksource only. I cannot do more without timer framework support.
Looks, like I have to wait your patch to implement that.

Alexander.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 3/8] ARM: dts: rockchip: add timer entries to rk3188 SoC

[Daniel Lezcano]

On Mon, Jan 30, 2017 at 04:13:07PM +0300, Alexander Kochetkov wrote:
> 
> > 30 янв. 2017 г., в 15:04, Daniel Lezcano <daniel.lezcano@linaro.org> написал(а):
> > 
> > There is no case when the rockchip timer is used for the clockevent.
> The is already timer entry for rk3228 in the DT. And it act as clockevent. I guess it work as backup,
> but I cannot test it also. In order to not break DT compatibility I had to provide one timer to be
> initialized as clockevent. And implemented implicit rule the driver (first DT timer - clockevent,
> second DT timer - clocksource) already exists for other timers.
> 
> > If I'm not wrong, you can check /proc/interrupts and see there is no interrupt
> > on the rockchip timer.
> 
> Yes, you right here. I’ve temporary disable smp_twd to test rockchip timer interrupts.
> There is no interrupt on the rockchip timer during normal work.
> 
> > - when the CPU enters a deep idle state. But such state does not exist on rk3188
> ARM chip/revision specific?

rk3188 specific. Applies also for rk3288 AFAICT. Without entering in the
details, the SoC can't handle that.

> > - when the system goes to suspend. But the timers are stopped in any case and
> >   CPU0 is always on.
> There is timer for rk3228 in the DT. I guess there are situations where it can be used.
> May be the situations are also acceptable for rk3188? May be something like suspend to RAM or
> suspend to HDD? I am not expert in that questions. I can do some tests if you give hint/link.

Nope, rockchip clockevents won't be used, smp_twd will instead.

You can check by commenting the local-timer in the DT, and you should see the
/proc/interrupts line for the rockchip incrementing while the 'twd' are zero.

> So, as I understood you suggest to leave only one timer what can be used as clocksource
> only. I can implement that, but there should be DT rule what allow to setup timer
> as clocksource only. I cannot do more without timer framework support.
> Looks, like I have to wait your patch to implement that.

No, actually I meant the rockchip clockevents won't be used for both rk3188 and
rk3288, they are not needed.

It is for information, this patch is ok.

  -- Daniel

-- 

 <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 v5 3/8] ARM: dts: rockchip: add timer entries to rk3188 SoC

[Daniel Lezcano]

On Mon, Jan 30, 2017 at 04:13:07PM +0300, Alexander Kochetkov wrote:
> 
> > 30 ???. 2017 ?., ? 15:04, Daniel Lezcano <daniel.lezcano@linaro.org> ???????(?):
> > 
> > There is no case when the rockchip timer is used for the clockevent.
> The is already timer entry for rk3228 in the DT. And it act as clockevent. I guess it work as backup,
> but I cannot test it also. In order to not break DT compatibility I had to provide one timer to be
> initialized as clockevent. And implemented implicit rule the driver (first DT timer - clockevent,
> second DT timer - clocksource) already exists for other timers.
> 
> > If I'm not wrong, you can check /proc/interrupts and see there is no interrupt
> > on the rockchip timer.
> 
> Yes, you right here. I?ve temporary disable smp_twd to test rockchip timer interrupts.
> There is no interrupt on the rockchip timer during normal work.
> 
> > - when the CPU enters a deep idle state. But such state does not exist on rk3188
> ARM chip/revision specific?

rk3188 specific. Applies also for rk3288 AFAICT. Without entering in the
details, the SoC can't handle that.

> > - when the system goes to suspend. But the timers are stopped in any case and
> >   CPU0 is always on.
> There is timer for rk3228 in the DT. I guess there are situations where it can be used.
> May be the situations are also acceptable for rk3188? May be something like suspend to RAM or
> suspend to HDD? I am not expert in that questions. I can do some tests if you give hint/link.

Nope, rockchip clockevents won't be used, smp_twd will instead.

You can check by commenting the local-timer in the DT, and you should see the
/proc/interrupts line for the rockchip incrementing while the 'twd' are zero.

> So, as I understood you suggest to leave only one timer what can be used as clocksource
> only. I can implement that, but there should be DT rule what allow to setup timer
> as clocksource only. I cannot do more without timer framework support.
> Looks, like I have to wait your patch to implement that.

No, actually I meant the rockchip clockevents won't be used for both rk3188 and
rk3288, they are not needed.

It is for information, this patch is ok.

  -- Daniel

-- 

 <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 v5 3/8] ARM: dts: rockchip: add timer entries to rk3188 SoC

[Daniel Lezcano]

On Mon, Jan 30, 2017 at 04:13:07PM +0300, Alexander Kochetkov wrote:
> 
> > 30 янв. 2017 г., в 15:04, Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> написал(а):
> > 
> > There is no case when the rockchip timer is used for the clockevent.
> The is already timer entry for rk3228 in the DT. And it act as clockevent. I guess it work as backup,
> but I cannot test it also. In order to not break DT compatibility I had to provide one timer to be
> initialized as clockevent. And implemented implicit rule the driver (first DT timer - clockevent,
> second DT timer - clocksource) already exists for other timers.
> 
> > If I'm not wrong, you can check /proc/interrupts and see there is no interrupt
> > on the rockchip timer.
> 
> Yes, you right here. I’ve temporary disable smp_twd to test rockchip timer interrupts.
> There is no interrupt on the rockchip timer during normal work.
> 
> > - when the CPU enters a deep idle state. But such state does not exist on rk3188
> ARM chip/revision specific?

rk3188 specific. Applies also for rk3288 AFAICT. Without entering in the
details, the SoC can't handle that.

> > - when the system goes to suspend. But the timers are stopped in any case and
> >   CPU0 is always on.
> There is timer for rk3228 in the DT. I guess there are situations where it can be used.
> May be the situations are also acceptable for rk3188? May be something like suspend to RAM or
> suspend to HDD? I am not expert in that questions. I can do some tests if you give hint/link.

Nope, rockchip clockevents won't be used, smp_twd will instead.

You can check by commenting the local-timer in the DT, and you should see the
/proc/interrupts line for the rockchip incrementing while the 'twd' are zero.

> So, as I understood you suggest to leave only one timer what can be used as clocksource
> only. I can implement that, but there should be DT rule what allow to setup timer
> as clocksource only. I cannot do more without timer framework support.
> Looks, like I have to wait your patch to implement that.

No, actually I meant the rockchip clockevents won't be used for both rk3188 and
rk3288, they are not needed.

It is for information, this patch is ok.

  -- Daniel

-- 

 <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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 4/8] ARM: dts: rockchip: disable arm-global-timer for rk3188

[Alexander Kochetkov]

clocksource and shed_clock provided by arm-global-timer is quite
unstable, because their rate depends on cpu frequency.
So disable arm-global-timer and use clocksource and sched_clock
from rockchip_timer.

It is impossible get stable clocksource having rockchip_timer and
arm-global-timer enabled at the same time. Because arm-global-timer
looks like a better candidate for the kernel: it has higher
frequency and rating.

Disabling arm-global-timer doesn't leave kernel without
clockevents as there is another clockevent provider (smp-twd).

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Reviwed-by: Heiko Stübner <heiko@sntech.de>
---
 arch/arm/boot/dts/rk3188.dtsi |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
index bcf8e03..f677130 100644
--- a/arch/arm/boot/dts/rk3188.dtsi
+++ b/arch/arm/boot/dts/rk3188.dtsi
@@ -546,6 +546,7 @@
 
 &global_timer {
 	interrupts = <GIC_PPI 11 0xf04>;
+	status = "disabled";
 };
 
 &local_timer {
-- 
1.7.9.5

[PATCH v5 4/8] ARM: dts: rockchip: disable arm-global-timer for rk3188

[Alexander Kochetkov]

clocksource and shed_clock provided by arm-global-timer is quite
unstable, because their rate depends on cpu frequency.
So disable arm-global-timer and use clocksource and sched_clock
from rockchip_timer.

It is impossible get stable clocksource having rockchip_timer and
arm-global-timer enabled at the same time. Because arm-global-timer
looks like a better candidate for the kernel: it has higher
frequency and rating.

Disabling arm-global-timer doesn't leave kernel without
clockevents as there is another clockevent provider (smp-twd).

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Reviwed-by: Heiko St?bner <heiko@sntech.de>
---
 arch/arm/boot/dts/rk3188.dtsi |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
index bcf8e03..f677130 100644
--- a/arch/arm/boot/dts/rk3188.dtsi
+++ b/arch/arm/boot/dts/rk3188.dtsi
@@ -546,6 +546,7 @@
 
 &global_timer {
 	interrupts = <GIC_PPI 11 0xf04>;
+	status = "disabled";
 };
 
 &local_timer {
-- 
1.7.9.5

Re: [PATCH v5 4/8] ARM: dts: rockchip: disable arm-global-timer for rk3188

[Daniel Lezcano]

On Tue, Jan 24, 2017 at 03:16:39PM +0300, Alexander Kochetkov wrote:
> clocksource and shed_clock provided by arm-global-timer is quite
> unstable, because their rate depends on cpu frequency.
> So disable arm-global-timer and use clocksource and sched_clock
> from rockchip_timer.
> It is impossible get stable clocksource having rockchip_timer and
> arm-global-timer enabled at the same time. Because arm-global-timer
> looks like a better candidate for the kernel: it has higher
> frequency and rating.
> 
> Disabling arm-global-timer doesn't leave kernel without
> clockevents as there is another clockevent provider (smp-twd).

Hi Alexander,

sorry, I will rewrite the description because of the grammatical errors. I
don't want to give you the feeling I'm lecturing you, my English is not perfect
but it will be simpler to give the full descr with the typos fixed.

"
The clocksource and the sched_clock provided by the arm_global_timer are quite
unstable because their rates depend on the cpu frequency.

On the other side, the arm_global_timer has a higher rating than the
rockchip_timer, it will be selected by default by the time framework while
we want to use the stable rockchip clocksource.

Let's disable the arm_global_timer in order to have the rockchip clocksource
selected by default.
"

This patch should go at the end of the patchset when the clocksource is
implemented in the rockchip_timer.

> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> Reviwed-by: Heiko Stübner <heiko@sntech.de>
> ---
>  arch/arm/boot/dts/rk3188.dtsi |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
> index bcf8e03..f677130 100644
> --- a/arch/arm/boot/dts/rk3188.dtsi
> +++ b/arch/arm/boot/dts/rk3188.dtsi
> @@ -546,6 +546,7 @@
>  
>  &global_timer {
>  	interrupts = <GIC_PPI 11 0xf04>;
> +	status = "disabled";
>  };
>  
>  &local_timer {
> -- 
> 1.7.9.5
> 

-- 

 <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 v5 4/8] ARM: dts: rockchip: disable arm-global-timer for rk3188

[Daniel Lezcano]

On Tue, Jan 24, 2017 at 03:16:39PM +0300, Alexander Kochetkov wrote:
> clocksource and shed_clock provided by arm-global-timer is quite
> unstable, because their rate depends on cpu frequency.
> So disable arm-global-timer and use clocksource and sched_clock
> from rockchip_timer.
> It is impossible get stable clocksource having rockchip_timer and
> arm-global-timer enabled at the same time. Because arm-global-timer
> looks like a better candidate for the kernel: it has higher
> frequency and rating.
> 
> Disabling arm-global-timer doesn't leave kernel without
> clockevents as there is another clockevent provider (smp-twd).

Hi Alexander,

sorry, I will rewrite the description because of the grammatical errors. I
don't want to give you the feeling I'm lecturing you, my English is not perfect
but it will be simpler to give the full descr with the typos fixed.

"
The clocksource and the sched_clock provided by the arm_global_timer are quite
unstable because their rates depend on the cpu frequency.

On the other side, the arm_global_timer has a higher rating than the
rockchip_timer, it will be selected by default by the time framework while
we want to use the stable rockchip clocksource.

Let's disable the arm_global_timer in order to have the rockchip clocksource
selected by default.
"

This patch should go at the end of the patchset when the clocksource is
implemented in the rockchip_timer.

> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> Reviwed-by: Heiko St?bner <heiko@sntech.de>
> ---
>  arch/arm/boot/dts/rk3188.dtsi |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
> index bcf8e03..f677130 100644
> --- a/arch/arm/boot/dts/rk3188.dtsi
> +++ b/arch/arm/boot/dts/rk3188.dtsi
> @@ -546,6 +546,7 @@
>  
>  &global_timer {
>  	interrupts = <GIC_PPI 11 0xf04>;
> +	status = "disabled";
>  };
>  
>  &local_timer {
> -- 
> 1.7.9.5
> 

-- 

 <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 v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Alexander Kochetkov]

The patch move ce field out of struct bc_timer into struct
rk_clock_event_device and rename struct bc_timer to struct rk_timer.

The main idea for the commit is to exctact low level timer
routines from current implementation so they could be
reused in the following clocksource implementation commit.

This is refactoring step without functional changes.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Reviwed-by: Heiko Stübner <heiko@sntech.de>
---
 drivers/clocksource/rockchip_timer.c |   33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index 23e267a..6d68d4c 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -29,18 +29,28 @@
 #define TIMER_MODE_USER_DEFINED_COUNT		(1 << 1)
 #define TIMER_INT_UNMASK			(1 << 2)
 
-struct bc_timer {
-	struct clock_event_device ce;
+struct rk_timer {
 	void __iomem *base;
 	void __iomem *ctrl;
 	u32 freq;
 };
 
-static struct bc_timer bc_timer;
+struct rk_clock_event_device {
+	struct clock_event_device ce;
+	struct rk_timer timer;
+};
+
+static struct rk_clock_event_device bc_timer;
+
+static inline struct rk_clock_event_device*
+rk_clock_event_device(struct clock_event_device *ce)
+{
+	return container_of(ce, struct rk_clock_event_device, ce);
+}
 
-static inline struct bc_timer *rk_timer(struct clock_event_device *ce)
+static inline struct rk_timer *rk_timer(struct clock_event_device *ce)
 {
-	return container_of(ce, struct bc_timer, ce);
+	return &rk_clock_event_device(ce)->timer;
 }
 
 static inline void __iomem *rk_base(struct clock_event_device *ce)
@@ -116,16 +126,17 @@ static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
 static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 {
 	struct clock_event_device *ce = &bc_timer.ce;
+	struct rk_timer *timer = &bc_timer.timer;
 	struct clk *timer_clk;
 	struct clk *pclk;
 	int ret = -EINVAL, irq;
 
-	bc_timer.base = of_iomap(np, 0);
-	if (!bc_timer.base) {
+	timer->base = of_iomap(np, 0);
+	if (!timer->base) {
 		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
 		return -ENXIO;
 	}
-	bc_timer.ctrl = bc_timer.base + ctrl_reg;
+	timer->ctrl = timer->base + ctrl_reg;
 
 	pclk = of_clk_get_by_name(np, "pclk");
 	if (IS_ERR(pclk)) {
@@ -153,7 +164,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 		goto out_timer_clk;
 	}
 
-	bc_timer.freq = clk_get_rate(timer_clk);
+	timer->freq = clk_get_rate(timer_clk);
 
 	irq = irq_of_parse_and_map(np, 0);
 	if (!irq) {
@@ -181,7 +192,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 		goto out_irq;
 	}
 
-	clockevents_config_and_register(ce, bc_timer.freq, 1, UINT_MAX);
+	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
 
 	return 0;
 
@@ -190,7 +201,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 out_timer_clk:
 	clk_disable_unprepare(pclk);
 out_unmap:
-	iounmap(bc_timer.base);
+	iounmap(timer->base);
 
 	return ret;
 }
-- 
1.7.9.5

[PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Alexander Kochetkov]

The patch move ce field out of struct bc_timer into struct
rk_clock_event_device and rename struct bc_timer to struct rk_timer.

The main idea for the commit is to exctact low level timer
routines from current implementation so they could be
reused in the following clocksource implementation commit.

This is refactoring step without functional changes.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Reviwed-by: Heiko St?bner <heiko@sntech.de>
---
 drivers/clocksource/rockchip_timer.c |   33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index 23e267a..6d68d4c 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -29,18 +29,28 @@
 #define TIMER_MODE_USER_DEFINED_COUNT		(1 << 1)
 #define TIMER_INT_UNMASK			(1 << 2)
 
-struct bc_timer {
-	struct clock_event_device ce;
+struct rk_timer {
 	void __iomem *base;
 	void __iomem *ctrl;
 	u32 freq;
 };
 
-static struct bc_timer bc_timer;
+struct rk_clock_event_device {
+	struct clock_event_device ce;
+	struct rk_timer timer;
+};
+
+static struct rk_clock_event_device bc_timer;
+
+static inline struct rk_clock_event_device*
+rk_clock_event_device(struct clock_event_device *ce)
+{
+	return container_of(ce, struct rk_clock_event_device, ce);
+}
 
-static inline struct bc_timer *rk_timer(struct clock_event_device *ce)
+static inline struct rk_timer *rk_timer(struct clock_event_device *ce)
 {
-	return container_of(ce, struct bc_timer, ce);
+	return &rk_clock_event_device(ce)->timer;
 }
 
 static inline void __iomem *rk_base(struct clock_event_device *ce)
@@ -116,16 +126,17 @@ static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
 static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 {
 	struct clock_event_device *ce = &bc_timer.ce;
+	struct rk_timer *timer = &bc_timer.timer;
 	struct clk *timer_clk;
 	struct clk *pclk;
 	int ret = -EINVAL, irq;
 
-	bc_timer.base = of_iomap(np, 0);
-	if (!bc_timer.base) {
+	timer->base = of_iomap(np, 0);
+	if (!timer->base) {
 		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
 		return -ENXIO;
 	}
-	bc_timer.ctrl = bc_timer.base + ctrl_reg;
+	timer->ctrl = timer->base + ctrl_reg;
 
 	pclk = of_clk_get_by_name(np, "pclk");
 	if (IS_ERR(pclk)) {
@@ -153,7 +164,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 		goto out_timer_clk;
 	}
 
-	bc_timer.freq = clk_get_rate(timer_clk);
+	timer->freq = clk_get_rate(timer_clk);
 
 	irq = irq_of_parse_and_map(np, 0);
 	if (!irq) {
@@ -181,7 +192,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 		goto out_irq;
 	}
 
-	clockevents_config_and_register(ce, bc_timer.freq, 1, UINT_MAX);
+	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
 
 	return 0;
 
@@ -190,7 +201,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 out_timer_clk:
 	clk_disable_unprepare(pclk);
 out_unmap:
-	iounmap(bc_timer.base);
+	iounmap(timer->base);
 
 	return ret;
 }
-- 
1.7.9.5

[PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Alexander Kochetkov]

The patch move ce field out of struct bc_timer into struct
rk_clock_event_device and rename struct bc_timer to struct rk_timer.

The main idea for the commit is to exctact low level timer
routines from current implementation so they could be
reused in the following clocksource implementation commit.

This is refactoring step without functional changes.

Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Reviwed-by: Heiko Stübner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
---
 drivers/clocksource/rockchip_timer.c |   33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index 23e267a..6d68d4c 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -29,18 +29,28 @@
 #define TIMER_MODE_USER_DEFINED_COUNT		(1 << 1)
 #define TIMER_INT_UNMASK			(1 << 2)
 
-struct bc_timer {
-	struct clock_event_device ce;
+struct rk_timer {
 	void __iomem *base;
 	void __iomem *ctrl;
 	u32 freq;
 };
 
-static struct bc_timer bc_timer;
+struct rk_clock_event_device {
+	struct clock_event_device ce;
+	struct rk_timer timer;
+};
+
+static struct rk_clock_event_device bc_timer;
+
+static inline struct rk_clock_event_device*
+rk_clock_event_device(struct clock_event_device *ce)
+{
+	return container_of(ce, struct rk_clock_event_device, ce);
+}
 
-static inline struct bc_timer *rk_timer(struct clock_event_device *ce)
+static inline struct rk_timer *rk_timer(struct clock_event_device *ce)
 {
-	return container_of(ce, struct bc_timer, ce);
+	return &rk_clock_event_device(ce)->timer;
 }
 
 static inline void __iomem *rk_base(struct clock_event_device *ce)
@@ -116,16 +126,17 @@ static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
 static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 {
 	struct clock_event_device *ce = &bc_timer.ce;
+	struct rk_timer *timer = &bc_timer.timer;
 	struct clk *timer_clk;
 	struct clk *pclk;
 	int ret = -EINVAL, irq;
 
-	bc_timer.base = of_iomap(np, 0);
-	if (!bc_timer.base) {
+	timer->base = of_iomap(np, 0);
+	if (!timer->base) {
 		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
 		return -ENXIO;
 	}
-	bc_timer.ctrl = bc_timer.base + ctrl_reg;
+	timer->ctrl = timer->base + ctrl_reg;
 
 	pclk = of_clk_get_by_name(np, "pclk");
 	if (IS_ERR(pclk)) {
@@ -153,7 +164,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 		goto out_timer_clk;
 	}
 
-	bc_timer.freq = clk_get_rate(timer_clk);
+	timer->freq = clk_get_rate(timer_clk);
 
 	irq = irq_of_parse_and_map(np, 0);
 	if (!irq) {
@@ -181,7 +192,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 		goto out_irq;
 	}
 
-	clockevents_config_and_register(ce, bc_timer.freq, 1, UINT_MAX);
+	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
 
 	return 0;
 
@@ -190,7 +201,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 out_timer_clk:
 	clk_disable_unprepare(pclk);
 out_unmap:
-	iounmap(bc_timer.base);
+	iounmap(timer->base);
 
 	return ret;
 }
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Heiko Stübner]

Am Dienstag, 24. Januar 2017, 15:16:40 CET schrieb Alexander Kochetkov:
> The patch move ce field out of struct bc_timer into struct
> rk_clock_event_device and rename struct bc_timer to struct rk_timer.
> 
> The main idea for the commit is to exctact low level timer
> routines from current implementation so they could be
> reused in the following clocksource implementation commit.
> 
> This is refactoring step without functional changes.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> Reviwed-by: Heiko Stübner <heiko@sntech.de>

Please don't add Reviewed-by tags without explicit mention of them by 
reviewers. (Also it's spelled wrong).

I haven't looked that deeply into the driver itself and the changes made to 
feel comfortable with a "Reviewed-by".

> ---
>  drivers/clocksource/rockchip_timer.c |   33
> ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11
> deletions(-)
> 
> diff --git a/drivers/clocksource/rockchip_timer.c
> b/drivers/clocksource/rockchip_timer.c index 23e267a..6d68d4c 100644
> --- a/drivers/clocksource/rockchip_timer.c
> +++ b/drivers/clocksource/rockchip_timer.c
> @@ -29,18 +29,28 @@
>  #define TIMER_MODE_USER_DEFINED_COUNT		(1 << 1)
>  #define TIMER_INT_UNMASK			(1 << 2)
> 
> -struct bc_timer {
> -	struct clock_event_device ce;
> +struct rk_timer {
>  	void __iomem *base;
>  	void __iomem *ctrl;
>  	u32 freq;
>  };
> 
> -static struct bc_timer bc_timer;
> +struct rk_clock_event_device {
> +	struct clock_event_device ce;
> +	struct rk_timer timer;
> +};
> +
> +static struct rk_clock_event_device bc_timer;
> +
> +static inline struct rk_clock_event_device*
> +rk_clock_event_device(struct clock_event_device *ce)
> +{
> +	return container_of(ce, struct rk_clock_event_device, ce);
> +}
> 
> -static inline struct bc_timer *rk_timer(struct clock_event_device *ce)
> +static inline struct rk_timer *rk_timer(struct clock_event_device *ce)
>  {
> -	return container_of(ce, struct bc_timer, ce);
> +	return &rk_clock_event_device(ce)->timer;
>  }
> 
>  static inline void __iomem *rk_base(struct clock_event_device *ce)
> @@ -116,16 +126,17 @@ static irqreturn_t rk_timer_interrupt(int irq, void
> *dev_id) static int __init rk_timer_init(struct device_node *np, u32
> ctrl_reg) {
>  	struct clock_event_device *ce = &bc_timer.ce;
> +	struct rk_timer *timer = &bc_timer.timer;
>  	struct clk *timer_clk;
>  	struct clk *pclk;
>  	int ret = -EINVAL, irq;
> 
> -	bc_timer.base = of_iomap(np, 0);
> -	if (!bc_timer.base) {
> +	timer->base = of_iomap(np, 0);
> +	if (!timer->base) {
>  		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
>  		return -ENXIO;
>  	}
> -	bc_timer.ctrl = bc_timer.base + ctrl_reg;
> +	timer->ctrl = timer->base + ctrl_reg;
> 
>  	pclk = of_clk_get_by_name(np, "pclk");
>  	if (IS_ERR(pclk)) {
> @@ -153,7 +164,7 @@ static int __init rk_timer_init(struct device_node *np,
> u32 ctrl_reg) goto out_timer_clk;
>  	}
> 
> -	bc_timer.freq = clk_get_rate(timer_clk);
> +	timer->freq = clk_get_rate(timer_clk);
> 
>  	irq = irq_of_parse_and_map(np, 0);
>  	if (!irq) {
> @@ -181,7 +192,7 @@ static int __init rk_timer_init(struct device_node *np,
> u32 ctrl_reg) goto out_irq;
>  	}
> 
> -	clockevents_config_and_register(ce, bc_timer.freq, 1, UINT_MAX);
> +	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
> 
>  	return 0;
> 
> @@ -190,7 +201,7 @@ static int __init rk_timer_init(struct device_node *np,
> u32 ctrl_reg) out_timer_clk:
>  	clk_disable_unprepare(pclk);
>  out_unmap:
> -	iounmap(bc_timer.base);
> +	iounmap(timer->base);
> 
>  	return ret;
>  }

[PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Heiko Stübner]

Am Dienstag, 24. Januar 2017, 15:16:40 CET schrieb Alexander Kochetkov:
> The patch move ce field out of struct bc_timer into struct
> rk_clock_event_device and rename struct bc_timer to struct rk_timer.
> 
> The main idea for the commit is to exctact low level timer
> routines from current implementation so they could be
> reused in the following clocksource implementation commit.
> 
> This is refactoring step without functional changes.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> Reviwed-by: Heiko St?bner <heiko@sntech.de>

Please don't add Reviewed-by tags without explicit mention of them by 
reviewers. (Also it's spelled wrong).

I haven't looked that deeply into the driver itself and the changes made to 
feel comfortable with a "Reviewed-by".

> ---
>  drivers/clocksource/rockchip_timer.c |   33
> ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11
> deletions(-)
> 
> diff --git a/drivers/clocksource/rockchip_timer.c
> b/drivers/clocksource/rockchip_timer.c index 23e267a..6d68d4c 100644
> --- a/drivers/clocksource/rockchip_timer.c
> +++ b/drivers/clocksource/rockchip_timer.c
> @@ -29,18 +29,28 @@
>  #define TIMER_MODE_USER_DEFINED_COUNT		(1 << 1)
>  #define TIMER_INT_UNMASK			(1 << 2)
> 
> -struct bc_timer {
> -	struct clock_event_device ce;
> +struct rk_timer {
>  	void __iomem *base;
>  	void __iomem *ctrl;
>  	u32 freq;
>  };
> 
> -static struct bc_timer bc_timer;
> +struct rk_clock_event_device {
> +	struct clock_event_device ce;
> +	struct rk_timer timer;
> +};
> +
> +static struct rk_clock_event_device bc_timer;
> +
> +static inline struct rk_clock_event_device*
> +rk_clock_event_device(struct clock_event_device *ce)
> +{
> +	return container_of(ce, struct rk_clock_event_device, ce);
> +}
> 
> -static inline struct bc_timer *rk_timer(struct clock_event_device *ce)
> +static inline struct rk_timer *rk_timer(struct clock_event_device *ce)
>  {
> -	return container_of(ce, struct bc_timer, ce);
> +	return &rk_clock_event_device(ce)->timer;
>  }
> 
>  static inline void __iomem *rk_base(struct clock_event_device *ce)
> @@ -116,16 +126,17 @@ static irqreturn_t rk_timer_interrupt(int irq, void
> *dev_id) static int __init rk_timer_init(struct device_node *np, u32
> ctrl_reg) {
>  	struct clock_event_device *ce = &bc_timer.ce;
> +	struct rk_timer *timer = &bc_timer.timer;
>  	struct clk *timer_clk;
>  	struct clk *pclk;
>  	int ret = -EINVAL, irq;
> 
> -	bc_timer.base = of_iomap(np, 0);
> -	if (!bc_timer.base) {
> +	timer->base = of_iomap(np, 0);
> +	if (!timer->base) {
>  		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
>  		return -ENXIO;
>  	}
> -	bc_timer.ctrl = bc_timer.base + ctrl_reg;
> +	timer->ctrl = timer->base + ctrl_reg;
> 
>  	pclk = of_clk_get_by_name(np, "pclk");
>  	if (IS_ERR(pclk)) {
> @@ -153,7 +164,7 @@ static int __init rk_timer_init(struct device_node *np,
> u32 ctrl_reg) goto out_timer_clk;
>  	}
> 
> -	bc_timer.freq = clk_get_rate(timer_clk);
> +	timer->freq = clk_get_rate(timer_clk);
> 
>  	irq = irq_of_parse_and_map(np, 0);
>  	if (!irq) {
> @@ -181,7 +192,7 @@ static int __init rk_timer_init(struct device_node *np,
> u32 ctrl_reg) goto out_irq;
>  	}
> 
> -	clockevents_config_and_register(ce, bc_timer.freq, 1, UINT_MAX);
> +	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
> 
>  	return 0;
> 
> @@ -190,7 +201,7 @@ static int __init rk_timer_init(struct device_node *np,
> u32 ctrl_reg) out_timer_clk:
>  	clk_disable_unprepare(pclk);
>  out_unmap:
> -	iounmap(bc_timer.base);
> +	iounmap(timer->base);
> 
>  	return ret;
>  }

Re: [PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Heiko Stübner]

Am Dienstag, 24. Januar 2017, 15:16:40 CET schrieb Alexander Kochetkov:
> The patch move ce field out of struct bc_timer into struct
> rk_clock_event_device and rename struct bc_timer to struct rk_timer.
> 
> The main idea for the commit is to exctact low level timer
> routines from current implementation so they could be
> reused in the following clocksource implementation commit.
> 
> This is refactoring step without functional changes.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reviwed-by: Heiko Stübner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>

Please don't add Reviewed-by tags without explicit mention of them by 
reviewers. (Also it's spelled wrong).

I haven't looked that deeply into the driver itself and the changes made to 
feel comfortable with a "Reviewed-by".

> ---
>  drivers/clocksource/rockchip_timer.c |   33
> ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11
> deletions(-)
> 
> diff --git a/drivers/clocksource/rockchip_timer.c
> b/drivers/clocksource/rockchip_timer.c index 23e267a..6d68d4c 100644
> --- a/drivers/clocksource/rockchip_timer.c
> +++ b/drivers/clocksource/rockchip_timer.c
> @@ -29,18 +29,28 @@
>  #define TIMER_MODE_USER_DEFINED_COUNT		(1 << 1)
>  #define TIMER_INT_UNMASK			(1 << 2)
> 
> -struct bc_timer {
> -	struct clock_event_device ce;
> +struct rk_timer {
>  	void __iomem *base;
>  	void __iomem *ctrl;
>  	u32 freq;
>  };
> 
> -static struct bc_timer bc_timer;
> +struct rk_clock_event_device {
> +	struct clock_event_device ce;
> +	struct rk_timer timer;
> +};
> +
> +static struct rk_clock_event_device bc_timer;
> +
> +static inline struct rk_clock_event_device*
> +rk_clock_event_device(struct clock_event_device *ce)
> +{
> +	return container_of(ce, struct rk_clock_event_device, ce);
> +}
> 
> -static inline struct bc_timer *rk_timer(struct clock_event_device *ce)
> +static inline struct rk_timer *rk_timer(struct clock_event_device *ce)
>  {
> -	return container_of(ce, struct bc_timer, ce);
> +	return &rk_clock_event_device(ce)->timer;
>  }
> 
>  static inline void __iomem *rk_base(struct clock_event_device *ce)
> @@ -116,16 +126,17 @@ static irqreturn_t rk_timer_interrupt(int irq, void
> *dev_id) static int __init rk_timer_init(struct device_node *np, u32
> ctrl_reg) {
>  	struct clock_event_device *ce = &bc_timer.ce;
> +	struct rk_timer *timer = &bc_timer.timer;
>  	struct clk *timer_clk;
>  	struct clk *pclk;
>  	int ret = -EINVAL, irq;
> 
> -	bc_timer.base = of_iomap(np, 0);
> -	if (!bc_timer.base) {
> +	timer->base = of_iomap(np, 0);
> +	if (!timer->base) {
>  		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
>  		return -ENXIO;
>  	}
> -	bc_timer.ctrl = bc_timer.base + ctrl_reg;
> +	timer->ctrl = timer->base + ctrl_reg;
> 
>  	pclk = of_clk_get_by_name(np, "pclk");
>  	if (IS_ERR(pclk)) {
> @@ -153,7 +164,7 @@ static int __init rk_timer_init(struct device_node *np,
> u32 ctrl_reg) goto out_timer_clk;
>  	}
> 
> -	bc_timer.freq = clk_get_rate(timer_clk);
> +	timer->freq = clk_get_rate(timer_clk);
> 
>  	irq = irq_of_parse_and_map(np, 0);
>  	if (!irq) {
> @@ -181,7 +192,7 @@ static int __init rk_timer_init(struct device_node *np,
> u32 ctrl_reg) goto out_irq;
>  	}
> 
> -	clockevents_config_and_register(ce, bc_timer.freq, 1, UINT_MAX);
> +	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
> 
>  	return 0;
> 
> @@ -190,7 +201,7 @@ static int __init rk_timer_init(struct device_node *np,
> u32 ctrl_reg) out_timer_clk:
>  	clk_disable_unprepare(pclk);
>  out_unmap:
> -	iounmap(bc_timer.base);
> +	iounmap(timer->base);
> 
>  	return ret;
>  }


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Alexander Kochetkov]

> 24 янв. 2017 г., в 18:02, Heiko Stübner <heiko@sntech.de> написал(а):
> 
> Please don't add Reviewed-by tags without explicit mention of them by 
> reviewers. (Also it's spelled wrong).
> 
> I haven't looked that deeply into the driver itself and the changes made to 
> feel comfortable with a "Reviewed-by".

Than I posted another linux code I was asked to add such tags (and others)
to add credits to people.

Sorry for that.

v6? or feel free to drop any tags during merge.

[PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Alexander Kochetkov]

> 24 ???. 2017 ?., ? 18:02, Heiko St?bner <heiko@sntech.de> ???????(?):
> 
> Please don't add Reviewed-by tags without explicit mention of them by 
> reviewers. (Also it's spelled wrong).
> 
> I haven't looked that deeply into the driver itself and the changes made to 
> feel comfortable with a "Reviewed-by".

Than I posted another linux code I was asked to add such tags (and others)
to add credits to people.

Sorry for that.

v6? or feel free to drop any tags during merge.

Re: [PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Alexander Kochetkov]

> 24 янв. 2017 г., в 18:02, Heiko Stübner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> написал(а):
> 
> Please don't add Reviewed-by tags without explicit mention of them by 
> reviewers. (Also it's spelled wrong).
> 
> I haven't looked that deeply into the driver itself and the changes made to 
> feel comfortable with a "Reviewed-by".

Than I posted another linux code I was asked to add such tags (and others)
to add credits to people.

Sorry for that.

v6? or feel free to drop any tags during merge.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Heiko Stuebner]

Hi Alexander,

Am Dienstag, 24. Januar 2017, 18:16:20 CET schrieb Alexander Kochetkov:
> > 24 янв. 2017 г., в 18:02, Heiko Stübner <heiko@sntech.de> написал(а):
> > 
> > Please don't add Reviewed-by tags without explicit mention of them by
> > reviewers. (Also it's spelled wrong).
> > 
> > I haven't looked that deeply into the driver itself and the changes made
> > to
> > feel comfortable with a "Reviewed-by".
> 
> Than I posted another linux code I was asked to add such tags (and others)
> to add credits to people.
> 
> Sorry for that.

no problem. In general people reviewing patches will respond to the patch with 
tags they are comfortable with being added. And a Reviewed-by is a pretty 
strong tag, indicating that some thought went into reviewing a specific patch, 
which I hadn't done with my cursory glance at the time :-)

Heiko

[PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Heiko Stuebner]

Hi Alexander,

Am Dienstag, 24. Januar 2017, 18:16:20 CET schrieb Alexander Kochetkov:
> > 24 ???. 2017 ?., ? 18:02, Heiko St?bner <heiko@sntech.de> ???????(?):
> > 
> > Please don't add Reviewed-by tags without explicit mention of them by
> > reviewers. (Also it's spelled wrong).
> > 
> > I haven't looked that deeply into the driver itself and the changes made
> > to
> > feel comfortable with a "Reviewed-by".
> 
> Than I posted another linux code I was asked to add such tags (and others)
> to add credits to people.
> 
> Sorry for that.

no problem. In general people reviewing patches will respond to the patch with 
tags they are comfortable with being added. And a Reviewed-by is a pretty 
strong tag, indicating that some thought went into reviewing a specific patch, 
which I hadn't done with my cursory glance at the time :-)

Heiko

Re: [PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Heiko Stuebner]

Hi Alexander,

Am Dienstag, 24. Januar 2017, 18:16:20 CET schrieb Alexander Kochetkov:
> > 24 янв. 2017 г., в 18:02, Heiko Stübner <heiko@sntech.de> написал(а):
> > 
> > Please don't add Reviewed-by tags without explicit mention of them by
> > reviewers. (Also it's spelled wrong).
> > 
> > I haven't looked that deeply into the driver itself and the changes made
> > to
> > feel comfortable with a "Reviewed-by".
> 
> Than I posted another linux code I was asked to add such tags (and others)
> to add credits to people.
> 
> Sorry for that.

no problem. In general people reviewing patches will respond to the patch with 
tags they are comfortable with being added. And a Reviewed-by is a pretty 
strong tag, indicating that some thought went into reviewing a specific patch, 
which I hadn't done with my cursory glance at the time :-)

Heiko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Daniel Lezcano]

On Tue, Jan 24, 2017 at 03:16:40PM +0300, Alexander Kochetkov wrote:
> The patch move ce field out of struct bc_timer into struct
> rk_clock_event_device and rename struct bc_timer to struct rk_timer.
> 
> The main idea for the commit is to exctact low level timer
> routines from current implementation so they could be
> reused in the following clocksource implementation commit.
> 
> This is refactoring step without functional changes.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> Reviwed-by: Heiko Stübner <heiko@sntech.de>

I don't get the point of these changes. The patch does not explain why they are
needed.

> ---
>  drivers/clocksource/rockchip_timer.c |   33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
> index 23e267a..6d68d4c 100644
> --- a/drivers/clocksource/rockchip_timer.c
> +++ b/drivers/clocksource/rockchip_timer.c
> @@ -29,18 +29,28 @@
>  #define TIMER_MODE_USER_DEFINED_COUNT		(1 << 1)
>  #define TIMER_INT_UNMASK			(1 << 2)
>  
> -struct bc_timer {
> -	struct clock_event_device ce;
> +struct rk_timer {
>  	void __iomem *base;
>  	void __iomem *ctrl;
>  	u32 freq;
>  };
>  
> -static struct bc_timer bc_timer;
> +struct rk_clock_event_device {
> +	struct clock_event_device ce;
> +	struct rk_timer timer;
> +};
> +
> +static struct rk_clock_event_device bc_timer;
> +
> +static inline struct rk_clock_event_device*
> +rk_clock_event_device(struct clock_event_device *ce)
> +{
> +	return container_of(ce, struct rk_clock_event_device, ce);
> +}
>  
> -static inline struct bc_timer *rk_timer(struct clock_event_device *ce)
> +static inline struct rk_timer *rk_timer(struct clock_event_device *ce)
>  {
> -	return container_of(ce, struct bc_timer, ce);
> +	return &rk_clock_event_device(ce)->timer;
>  }
>  
>  static inline void __iomem *rk_base(struct clock_event_device *ce)
> @@ -116,16 +126,17 @@ static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
>  static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  {
>  	struct clock_event_device *ce = &bc_timer.ce;
> +	struct rk_timer *timer = &bc_timer.timer;
>  	struct clk *timer_clk;
>  	struct clk *pclk;
>  	int ret = -EINVAL, irq;
>  
> -	bc_timer.base = of_iomap(np, 0);
> -	if (!bc_timer.base) {
> +	timer->base = of_iomap(np, 0);
> +	if (!timer->base) {
>  		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
>  		return -ENXIO;
>  	}
> -	bc_timer.ctrl = bc_timer.base + ctrl_reg;
> +	timer->ctrl = timer->base + ctrl_reg;
>  
>  	pclk = of_clk_get_by_name(np, "pclk");
>  	if (IS_ERR(pclk)) {
> @@ -153,7 +164,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  		goto out_timer_clk;
>  	}
>  
> -	bc_timer.freq = clk_get_rate(timer_clk);
> +	timer->freq = clk_get_rate(timer_clk);
>  
>  	irq = irq_of_parse_and_map(np, 0);
>  	if (!irq) {
> @@ -181,7 +192,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  		goto out_irq;
>  	}
>  
> -	clockevents_config_and_register(ce, bc_timer.freq, 1, UINT_MAX);
> +	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
>  
>  	return 0;
>  
> @@ -190,7 +201,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  out_timer_clk:
>  	clk_disable_unprepare(pclk);
>  out_unmap:
> -	iounmap(bc_timer.base);
> +	iounmap(timer->base);
>  
>  	return ret;
>  }
> -- 
> 1.7.9.5
> 

-- 

 <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 v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Daniel Lezcano]

On Tue, Jan 24, 2017 at 03:16:40PM +0300, Alexander Kochetkov wrote:
> The patch move ce field out of struct bc_timer into struct
> rk_clock_event_device and rename struct bc_timer to struct rk_timer.
> 
> The main idea for the commit is to exctact low level timer
> routines from current implementation so they could be
> reused in the following clocksource implementation commit.
> 
> This is refactoring step without functional changes.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> Reviwed-by: Heiko St?bner <heiko@sntech.de>

I don't get the point of these changes. The patch does not explain why they are
needed.

> ---
>  drivers/clocksource/rockchip_timer.c |   33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
> index 23e267a..6d68d4c 100644
> --- a/drivers/clocksource/rockchip_timer.c
> +++ b/drivers/clocksource/rockchip_timer.c
> @@ -29,18 +29,28 @@
>  #define TIMER_MODE_USER_DEFINED_COUNT		(1 << 1)
>  #define TIMER_INT_UNMASK			(1 << 2)
>  
> -struct bc_timer {
> -	struct clock_event_device ce;
> +struct rk_timer {
>  	void __iomem *base;
>  	void __iomem *ctrl;
>  	u32 freq;
>  };
>  
> -static struct bc_timer bc_timer;
> +struct rk_clock_event_device {
> +	struct clock_event_device ce;
> +	struct rk_timer timer;
> +};
> +
> +static struct rk_clock_event_device bc_timer;
> +
> +static inline struct rk_clock_event_device*
> +rk_clock_event_device(struct clock_event_device *ce)
> +{
> +	return container_of(ce, struct rk_clock_event_device, ce);
> +}
>  
> -static inline struct bc_timer *rk_timer(struct clock_event_device *ce)
> +static inline struct rk_timer *rk_timer(struct clock_event_device *ce)
>  {
> -	return container_of(ce, struct bc_timer, ce);
> +	return &rk_clock_event_device(ce)->timer;
>  }
>  
>  static inline void __iomem *rk_base(struct clock_event_device *ce)
> @@ -116,16 +126,17 @@ static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
>  static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  {
>  	struct clock_event_device *ce = &bc_timer.ce;
> +	struct rk_timer *timer = &bc_timer.timer;
>  	struct clk *timer_clk;
>  	struct clk *pclk;
>  	int ret = -EINVAL, irq;
>  
> -	bc_timer.base = of_iomap(np, 0);
> -	if (!bc_timer.base) {
> +	timer->base = of_iomap(np, 0);
> +	if (!timer->base) {
>  		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
>  		return -ENXIO;
>  	}
> -	bc_timer.ctrl = bc_timer.base + ctrl_reg;
> +	timer->ctrl = timer->base + ctrl_reg;
>  
>  	pclk = of_clk_get_by_name(np, "pclk");
>  	if (IS_ERR(pclk)) {
> @@ -153,7 +164,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  		goto out_timer_clk;
>  	}
>  
> -	bc_timer.freq = clk_get_rate(timer_clk);
> +	timer->freq = clk_get_rate(timer_clk);
>  
>  	irq = irq_of_parse_and_map(np, 0);
>  	if (!irq) {
> @@ -181,7 +192,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  		goto out_irq;
>  	}
>  
> -	clockevents_config_and_register(ce, bc_timer.freq, 1, UINT_MAX);
> +	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
>  
>  	return 0;
>  
> @@ -190,7 +201,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  out_timer_clk:
>  	clk_disable_unprepare(pclk);
>  out_unmap:
> -	iounmap(bc_timer.base);
> +	iounmap(timer->base);
>  
>  	return ret;
>  }
> -- 
> 1.7.9.5
> 

-- 

 <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 v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Daniel Lezcano]

On Tue, Jan 24, 2017 at 03:16:40PM +0300, Alexander Kochetkov wrote:
> The patch move ce field out of struct bc_timer into struct
> rk_clock_event_device and rename struct bc_timer to struct rk_timer.
> 
> The main idea for the commit is to exctact low level timer
> routines from current implementation so they could be
> reused in the following clocksource implementation commit.
> 
> This is refactoring step without functional changes.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reviwed-by: Heiko Stübner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>

I don't get the point of these changes. The patch does not explain why they are
needed.

> ---
>  drivers/clocksource/rockchip_timer.c |   33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
> index 23e267a..6d68d4c 100644
> --- a/drivers/clocksource/rockchip_timer.c
> +++ b/drivers/clocksource/rockchip_timer.c
> @@ -29,18 +29,28 @@
>  #define TIMER_MODE_USER_DEFINED_COUNT		(1 << 1)
>  #define TIMER_INT_UNMASK			(1 << 2)
>  
> -struct bc_timer {
> -	struct clock_event_device ce;
> +struct rk_timer {
>  	void __iomem *base;
>  	void __iomem *ctrl;
>  	u32 freq;
>  };
>  
> -static struct bc_timer bc_timer;
> +struct rk_clock_event_device {
> +	struct clock_event_device ce;
> +	struct rk_timer timer;
> +};
> +
> +static struct rk_clock_event_device bc_timer;
> +
> +static inline struct rk_clock_event_device*
> +rk_clock_event_device(struct clock_event_device *ce)
> +{
> +	return container_of(ce, struct rk_clock_event_device, ce);
> +}
>  
> -static inline struct bc_timer *rk_timer(struct clock_event_device *ce)
> +static inline struct rk_timer *rk_timer(struct clock_event_device *ce)
>  {
> -	return container_of(ce, struct bc_timer, ce);
> +	return &rk_clock_event_device(ce)->timer;
>  }
>  
>  static inline void __iomem *rk_base(struct clock_event_device *ce)
> @@ -116,16 +126,17 @@ static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
>  static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  {
>  	struct clock_event_device *ce = &bc_timer.ce;
> +	struct rk_timer *timer = &bc_timer.timer;
>  	struct clk *timer_clk;
>  	struct clk *pclk;
>  	int ret = -EINVAL, irq;
>  
> -	bc_timer.base = of_iomap(np, 0);
> -	if (!bc_timer.base) {
> +	timer->base = of_iomap(np, 0);
> +	if (!timer->base) {
>  		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
>  		return -ENXIO;
>  	}
> -	bc_timer.ctrl = bc_timer.base + ctrl_reg;
> +	timer->ctrl = timer->base + ctrl_reg;
>  
>  	pclk = of_clk_get_by_name(np, "pclk");
>  	if (IS_ERR(pclk)) {
> @@ -153,7 +164,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  		goto out_timer_clk;
>  	}
>  
> -	bc_timer.freq = clk_get_rate(timer_clk);
> +	timer->freq = clk_get_rate(timer_clk);
>  
>  	irq = irq_of_parse_and_map(np, 0);
>  	if (!irq) {
> @@ -181,7 +192,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  		goto out_irq;
>  	}
>  
> -	clockevents_config_and_register(ce, bc_timer.freq, 1, UINT_MAX);
> +	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
>  
>  	return 0;
>  
> @@ -190,7 +201,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  out_timer_clk:
>  	clk_disable_unprepare(pclk);
>  out_unmap:
> -	iounmap(bc_timer.base);
> +	iounmap(timer->base);
>  
>  	return ret;
>  }
> -- 
> 1.7.9.5
> 

-- 

 <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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Alexander Kochetkov]

> 30 янв. 2017 г., в 16:12, Daniel Lezcano <daniel.lezcano@linaro.org> написал(а):
> 
> I don't get the point of these changes. The patch does not explain why they are
> needed.

I’d like to extract timer API from current implementation.
And to make code more readable I’d like to introduce 'struct rk_timer’ what can be
reused with current implementation and with my patch (8/8). And in order keep patches
simple and readable I split that into three patches: 5/8, 6/8, 7/8.

Current implementation named rockchip timer as ‘struct bc_timer’ (broadcast timer).
I renamed it to more suitable to it role (may be bad choice).

Yes, the patch itself looks strange. You are right.

What do you think about that solution:
- in the patch 6/8 i will Introduce 'struct rk_timer’ and 'struct rk_time_clkevt’ (renamed ‘struct bc_timer’).
- rk_timer_init() changes from 5/8 I will merge with 8/8
- 8/8 introduce 'struct rk_time_clksrc' 
- 5/8 drop

Alexander.

[PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Alexander Kochetkov]

> 30 ???. 2017 ?., ? 16:12, Daniel Lezcano <daniel.lezcano@linaro.org> ???????(?):
> 
> I don't get the point of these changes. The patch does not explain why they are
> needed.

I?d like to extract timer API from current implementation.
And to make code more readable I?d like to introduce 'struct rk_timer? what can be
reused with current implementation and with my patch (8/8). And in order keep patches
simple and readable I split that into three patches: 5/8, 6/8, 7/8.

Current implementation named rockchip timer as ?struct bc_timer? (broadcast timer).
I renamed it to more suitable to it role (may be bad choice).

Yes, the patch itself looks strange. You are right.

What do you think about that solution:
- in the patch 6/8 i will Introduce 'struct rk_timer? and 'struct rk_time_clkevt? (renamed ?struct bc_timer?).
- rk_timer_init() changes from 5/8 I will merge with 8/8
- 8/8 introduce 'struct rk_time_clksrc' 
- 5/8 drop

Alexander.

Re: [PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Alexander Kochetkov]

> 30 янв. 2017 г., в 16:12, Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> написал(а):
> 
> I don't get the point of these changes. The patch does not explain why they are
> needed.

I’d like to extract timer API from current implementation.
And to make code more readable I’d like to introduce 'struct rk_timer’ what can be
reused with current implementation and with my patch (8/8). And in order keep patches
simple and readable I split that into three patches: 5/8, 6/8, 7/8.

Current implementation named rockchip timer as ‘struct bc_timer’ (broadcast timer).
I renamed it to more suitable to it role (may be bad choice).

Yes, the patch itself looks strange. You are right.

What do you think about that solution:
- in the patch 6/8 i will Introduce 'struct rk_timer’ and 'struct rk_time_clkevt’ (renamed ‘struct bc_timer’).
- rk_timer_init() changes from 5/8 I will merge with 8/8
- 8/8 introduce 'struct rk_time_clksrc' 
- 5/8 drop

Alexander.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Daniel Lezcano]

On Mon, Jan 30, 2017 at 04:55:33PM +0300, Alexander Kochetkov wrote:
> 
> > 30 янв. 2017 г., в 16:12, Daniel Lezcano <daniel.lezcano@linaro.org> написал(а):
> > 
> > I don't get the point of these changes. The patch does not explain why they are
> > needed.
> 
> I’d like to extract timer API from current implementation.
> And to make code more readable I’d like to introduce 'struct rk_timer’ what can be
> reused with current implementation and with my patch (8/8). And in order keep patches
> simple and readable I split that into three patches: 5/8, 6/8, 7/8.
> 
> Current implementation named rockchip timer as ‘struct bc_timer’ (broadcast timer).
> I renamed it to more suitable to it role (may be bad choice).
> 
> Yes, the patch itself looks strange. You are right.
> 
> What do you think about that solution:
> - in the patch 6/8 i will Introduce 'struct rk_timer’ and 'struct rk_time_clkevt’ (renamed ‘struct bc_timer’).

I prefer rk_clksrc and rk_clkevt.

> - rk_timer_init() changes from 5/8 I will merge with 8/8
> - 8/8 introduce 'struct rk_time_clksrc' 
> - 5/8 drop

Ok, let's see what that gives.

  -- Daniel

-- 

 <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 v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Daniel Lezcano]

On Mon, Jan 30, 2017 at 04:55:33PM +0300, Alexander Kochetkov wrote:
> 
> > 30 ???. 2017 ?., ? 16:12, Daniel Lezcano <daniel.lezcano@linaro.org> ???????(?):
> > 
> > I don't get the point of these changes. The patch does not explain why they are
> > needed.
> 
> I?d like to extract timer API from current implementation.
> And to make code more readable I?d like to introduce 'struct rk_timer? what can be
> reused with current implementation and with my patch (8/8). And in order keep patches
> simple and readable I split that into three patches: 5/8, 6/8, 7/8.
> 
> Current implementation named rockchip timer as ?struct bc_timer? (broadcast timer).
> I renamed it to more suitable to it role (may be bad choice).
> 
> Yes, the patch itself looks strange. You are right.
> 
> What do you think about that solution:
> - in the patch 6/8 i will Introduce 'struct rk_timer? and 'struct rk_time_clkevt? (renamed ?struct bc_timer?).

I prefer rk_clksrc and rk_clkevt.

> - rk_timer_init() changes from 5/8 I will merge with 8/8
> - 8/8 introduce 'struct rk_time_clksrc' 
> - 5/8 drop

Ok, let's see what that gives.

  -- Daniel

-- 

 <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 v5 5/8] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device

[Daniel Lezcano]

On Mon, Jan 30, 2017 at 04:55:33PM +0300, Alexander Kochetkov wrote:
> 
> > 30 янв. 2017 г., в 16:12, Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> написал(а):
> > 
> > I don't get the point of these changes. The patch does not explain why they are
> > needed.
> 
> I’d like to extract timer API from current implementation.
> And to make code more readable I’d like to introduce 'struct rk_timer’ what can be
> reused with current implementation and with my patch (8/8). And in order keep patches
> simple and readable I split that into three patches: 5/8, 6/8, 7/8.
> 
> Current implementation named rockchip timer as ‘struct bc_timer’ (broadcast timer).
> I renamed it to more suitable to it role (may be bad choice).
> 
> Yes, the patch itself looks strange. You are right.
> 
> What do you think about that solution:
> - in the patch 6/8 i will Introduce 'struct rk_timer’ and 'struct rk_time_clkevt’ (renamed ‘struct bc_timer’).

I prefer rk_clksrc and rk_clkevt.

> - rk_timer_init() changes from 5/8 I will merge with 8/8
> - 8/8 introduce 'struct rk_time_clksrc' 
> - 5/8 drop

Ok, let's see what that gives.

  -- Daniel

-- 

 <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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 6/8] clocksource/drivers/rockchip_timer: low level routines take rk_timer as parameter

[Alexander Kochetkov]

Pass rk_timer instead of clock_event_device to low lever timer routines.
So that code could be reused by clocksource implementation.

Drop rk_base() and rk_ctrl().

This is refactoring step without functional changes.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Reviwed-by: Heiko Stübner <heiko@sntech.de>
---
 drivers/clocksource/rockchip_timer.c |   57 ++++++++++++++++------------------
 1 file changed, 27 insertions(+), 30 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index 6d68d4c..a17dc61 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -53,70 +53,67 @@ static inline struct rk_timer *rk_timer(struct clock_event_device *ce)
 	return &rk_clock_event_device(ce)->timer;
 }
 
-static inline void __iomem *rk_base(struct clock_event_device *ce)
+static inline void rk_timer_disable(struct rk_timer *timer)
 {
-	return rk_timer(ce)->base;
+	writel_relaxed(TIMER_DISABLE, timer->ctrl);
 }
 
-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_ctrl(ce));
-}
-
-static inline void rk_timer_enable(struct clock_event_device *ce, u32 flags)
+static inline void rk_timer_enable(struct rk_timer *timer, u32 flags)
 {
 	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
-		       rk_ctrl(ce));
+		       timer->ctrl);
 }
 
 static void rk_timer_update_counter(unsigned long cycles,
-				    struct clock_event_device *ce)
+				    struct rk_timer *timer)
 {
-	writel_relaxed(cycles, rk_base(ce) + TIMER_LOAD_COUNT0);
-	writel_relaxed(0, rk_base(ce) + TIMER_LOAD_COUNT1);
+	writel_relaxed(cycles, timer->base + TIMER_LOAD_COUNT0);
+	writel_relaxed(0, timer->base + TIMER_LOAD_COUNT1);
 }
 
-static void rk_timer_interrupt_clear(struct clock_event_device *ce)
+static void rk_timer_interrupt_clear(struct rk_timer *timer)
 {
-	writel_relaxed(1, rk_base(ce) + TIMER_INT_STATUS);
+	writel_relaxed(1, timer->base + TIMER_INT_STATUS);
 }
 
 static inline int rk_timer_set_next_event(unsigned long cycles,
 					  struct clock_event_device *ce)
 {
-	rk_timer_disable(ce);
-	rk_timer_update_counter(cycles, ce);
-	rk_timer_enable(ce, TIMER_MODE_USER_DEFINED_COUNT);
+	struct rk_timer *timer = rk_timer(ce);
+
+	rk_timer_disable(timer);
+	rk_timer_update_counter(cycles, timer);
+	rk_timer_enable(timer, TIMER_MODE_USER_DEFINED_COUNT);
 	return 0;
 }
 
 static int rk_timer_shutdown(struct clock_event_device *ce)
 {
-	rk_timer_disable(ce);
+	struct rk_timer *timer = rk_timer(ce);
+
+	rk_timer_disable(timer);
 	return 0;
 }
 
 static int rk_timer_set_periodic(struct clock_event_device *ce)
 {
-	rk_timer_disable(ce);
-	rk_timer_update_counter(rk_timer(ce)->freq / HZ - 1, ce);
-	rk_timer_enable(ce, TIMER_MODE_FREE_RUNNING);
+	struct rk_timer *timer = rk_timer(ce);
+
+	rk_timer_disable(timer);
+	rk_timer_update_counter(timer->freq / HZ - 1, timer);
+	rk_timer_enable(timer, TIMER_MODE_FREE_RUNNING);
 	return 0;
 }
 
 static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
 {
 	struct clock_event_device *ce = dev_id;
+	struct rk_timer *timer = rk_timer(ce);
 
-	rk_timer_interrupt_clear(ce);
+	rk_timer_interrupt_clear(timer);
 
 	if (clockevent_state_oneshot(ce))
-		rk_timer_disable(ce);
+		rk_timer_disable(timer);
 
 	ce->event_handler(ce);
 
@@ -183,8 +180,8 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 	ce->cpumask = cpu_possible_mask;
 	ce->rating = 250;
 
-	rk_timer_interrupt_clear(ce);
-	rk_timer_disable(ce);
+	rk_timer_interrupt_clear(timer);
+	rk_timer_disable(timer);
 
 	ret = request_irq(irq, rk_timer_interrupt, IRQF_TIMER, TIMER_NAME, ce);
 	if (ret) {
-- 
1.7.9.5

[PATCH v5 6/8] clocksource/drivers/rockchip_timer: low level routines take rk_timer as parameter

[Alexander Kochetkov]

Pass rk_timer instead of clock_event_device to low lever timer routines.
So that code could be reused by clocksource implementation.

Drop rk_base() and rk_ctrl().

This is refactoring step without functional changes.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Reviwed-by: Heiko St?bner <heiko@sntech.de>
---
 drivers/clocksource/rockchip_timer.c |   57 ++++++++++++++++------------------
 1 file changed, 27 insertions(+), 30 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index 6d68d4c..a17dc61 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -53,70 +53,67 @@ static inline struct rk_timer *rk_timer(struct clock_event_device *ce)
 	return &rk_clock_event_device(ce)->timer;
 }
 
-static inline void __iomem *rk_base(struct clock_event_device *ce)
+static inline void rk_timer_disable(struct rk_timer *timer)
 {
-	return rk_timer(ce)->base;
+	writel_relaxed(TIMER_DISABLE, timer->ctrl);
 }
 
-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_ctrl(ce));
-}
-
-static inline void rk_timer_enable(struct clock_event_device *ce, u32 flags)
+static inline void rk_timer_enable(struct rk_timer *timer, u32 flags)
 {
 	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
-		       rk_ctrl(ce));
+		       timer->ctrl);
 }
 
 static void rk_timer_update_counter(unsigned long cycles,
-				    struct clock_event_device *ce)
+				    struct rk_timer *timer)
 {
-	writel_relaxed(cycles, rk_base(ce) + TIMER_LOAD_COUNT0);
-	writel_relaxed(0, rk_base(ce) + TIMER_LOAD_COUNT1);
+	writel_relaxed(cycles, timer->base + TIMER_LOAD_COUNT0);
+	writel_relaxed(0, timer->base + TIMER_LOAD_COUNT1);
 }
 
-static void rk_timer_interrupt_clear(struct clock_event_device *ce)
+static void rk_timer_interrupt_clear(struct rk_timer *timer)
 {
-	writel_relaxed(1, rk_base(ce) + TIMER_INT_STATUS);
+	writel_relaxed(1, timer->base + TIMER_INT_STATUS);
 }
 
 static inline int rk_timer_set_next_event(unsigned long cycles,
 					  struct clock_event_device *ce)
 {
-	rk_timer_disable(ce);
-	rk_timer_update_counter(cycles, ce);
-	rk_timer_enable(ce, TIMER_MODE_USER_DEFINED_COUNT);
+	struct rk_timer *timer = rk_timer(ce);
+
+	rk_timer_disable(timer);
+	rk_timer_update_counter(cycles, timer);
+	rk_timer_enable(timer, TIMER_MODE_USER_DEFINED_COUNT);
 	return 0;
 }
 
 static int rk_timer_shutdown(struct clock_event_device *ce)
 {
-	rk_timer_disable(ce);
+	struct rk_timer *timer = rk_timer(ce);
+
+	rk_timer_disable(timer);
 	return 0;
 }
 
 static int rk_timer_set_periodic(struct clock_event_device *ce)
 {
-	rk_timer_disable(ce);
-	rk_timer_update_counter(rk_timer(ce)->freq / HZ - 1, ce);
-	rk_timer_enable(ce, TIMER_MODE_FREE_RUNNING);
+	struct rk_timer *timer = rk_timer(ce);
+
+	rk_timer_disable(timer);
+	rk_timer_update_counter(timer->freq / HZ - 1, timer);
+	rk_timer_enable(timer, TIMER_MODE_FREE_RUNNING);
 	return 0;
 }
 
 static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
 {
 	struct clock_event_device *ce = dev_id;
+	struct rk_timer *timer = rk_timer(ce);
 
-	rk_timer_interrupt_clear(ce);
+	rk_timer_interrupt_clear(timer);
 
 	if (clockevent_state_oneshot(ce))
-		rk_timer_disable(ce);
+		rk_timer_disable(timer);
 
 	ce->event_handler(ce);
 
@@ -183,8 +180,8 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 	ce->cpumask = cpu_possible_mask;
 	ce->rating = 250;
 
-	rk_timer_interrupt_clear(ce);
-	rk_timer_disable(ce);
+	rk_timer_interrupt_clear(timer);
+	rk_timer_disable(timer);
 
 	ret = request_irq(irq, rk_timer_interrupt, IRQF_TIMER, TIMER_NAME, ce);
 	if (ret) {
-- 
1.7.9.5

[PATCH v5 7/8] clocksource/drivers/rockchip_timer: move TIMER_INT_UNMASK out of rk_timer_enable()

[Alexander Kochetkov]

This allow to enable timer without enabling interrupts from it.
As that mode will be used in clocksource implementation.

This is refactoring step without functional changes.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Reviwed-by: Heiko Stübner <heiko@sntech.de>
---
 drivers/clocksource/rockchip_timer.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index a17dc61..61c3bb1 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -60,8 +60,7 @@ static inline void rk_timer_disable(struct rk_timer *timer)
 
 static inline void rk_timer_enable(struct rk_timer *timer, u32 flags)
 {
-	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
-		       timer->ctrl);
+	writel_relaxed(TIMER_ENABLE | flags, timer->ctrl);
 }
 
 static void rk_timer_update_counter(unsigned long cycles,
@@ -83,7 +82,8 @@ static inline int rk_timer_set_next_event(unsigned long cycles,
 
 	rk_timer_disable(timer);
 	rk_timer_update_counter(cycles, timer);
-	rk_timer_enable(timer, TIMER_MODE_USER_DEFINED_COUNT);
+	rk_timer_enable(timer, TIMER_MODE_USER_DEFINED_COUNT |
+			       TIMER_INT_UNMASK);
 	return 0;
 }
 
@@ -101,7 +101,7 @@ static int rk_timer_set_periodic(struct clock_event_device *ce)
 
 	rk_timer_disable(timer);
 	rk_timer_update_counter(timer->freq / HZ - 1, timer);
-	rk_timer_enable(timer, TIMER_MODE_FREE_RUNNING);
+	rk_timer_enable(timer, TIMER_MODE_FREE_RUNNING | TIMER_INT_UNMASK);
 	return 0;
 }
 
-- 
1.7.9.5

[PATCH v5 7/8] clocksource/drivers/rockchip_timer: move TIMER_INT_UNMASK out of rk_timer_enable()

[Alexander Kochetkov]

This allow to enable timer without enabling interrupts from it.
As that mode will be used in clocksource implementation.

This is refactoring step without functional changes.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Reviwed-by: Heiko St?bner <heiko@sntech.de>
---
 drivers/clocksource/rockchip_timer.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index a17dc61..61c3bb1 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -60,8 +60,7 @@ static inline void rk_timer_disable(struct rk_timer *timer)
 
 static inline void rk_timer_enable(struct rk_timer *timer, u32 flags)
 {
-	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
-		       timer->ctrl);
+	writel_relaxed(TIMER_ENABLE | flags, timer->ctrl);
 }
 
 static void rk_timer_update_counter(unsigned long cycles,
@@ -83,7 +82,8 @@ static inline int rk_timer_set_next_event(unsigned long cycles,
 
 	rk_timer_disable(timer);
 	rk_timer_update_counter(cycles, timer);
-	rk_timer_enable(timer, TIMER_MODE_USER_DEFINED_COUNT);
+	rk_timer_enable(timer, TIMER_MODE_USER_DEFINED_COUNT |
+			       TIMER_INT_UNMASK);
 	return 0;
 }
 
@@ -101,7 +101,7 @@ static int rk_timer_set_periodic(struct clock_event_device *ce)
 
 	rk_timer_disable(timer);
 	rk_timer_update_counter(timer->freq / HZ - 1, timer);
-	rk_timer_enable(timer, TIMER_MODE_FREE_RUNNING);
+	rk_timer_enable(timer, TIMER_MODE_FREE_RUNNING | TIMER_INT_UNMASK);
 	return 0;
 }
 
-- 
1.7.9.5

[PATCH v5 8/8] clocksource/drivers/rockchip_timer: implement clocksource timer

[Alexander Kochetkov]

The clock supplying the arm-global-timer on the rk3188 is coming from the
the cpu clock itself and thus changes its rate everytime cpufreq adjusts
the cpu frequency making this timer unsuitable as a stable clocksource
and sched clock.

The rk3188, rk3288 and following socs share a separate timer block already
handled by the rockchip-timer driver. Therefore adapt this driver to also
be able to act as clocksource and sched clock on rk3188.

In order to test clocksource you can run following commands and check
how much time it take in real. On rk3188 it take about ~45 seconds.

    cpufreq-set -f 1.6GHZ
    date; sleep 60; date

In order to use the patch you need to declare two timers in the dts
file. The first timer will be initialized as clockevent provider
and the second one as clocksource. The clockevent must be from
alive subsystem as it used as backup for the local timers at sleep
time.

The patch does not break compatibility with older device tree files.
The older device tree files contain only one timer. The timer
will be initialized as clockevent, as expected.

rk3288 (and probably anything newer) is irrelevant to this patch,
as it has the arch timer interface. This patch may be usefull
for Cortex-A9/A5 based parts.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Reviwed-by: Heiko Stübner <heiko@sntech.de>
---
 drivers/clocksource/rockchip_timer.c |  137 +++++++++++++++++++++++++++++-----
 1 file changed, 117 insertions(+), 20 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index 61c3bb1..3ff533c 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -11,6 +11,7 @@
 #include <linux/clockchips.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
+#include <linux/sched_clock.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
@@ -19,6 +20,8 @@
 
 #define TIMER_LOAD_COUNT0	0x00
 #define TIMER_LOAD_COUNT1	0x04
+#define TIMER_CURRENT_VALUE0	0x08
+#define TIMER_CURRENT_VALUE1	0x0C
 #define TIMER_CONTROL_REG3288	0x10
 #define TIMER_CONTROL_REG3399	0x1c
 #define TIMER_INT_STATUS	0x18
@@ -40,7 +43,19 @@ struct rk_clock_event_device {
 	struct rk_timer timer;
 };
 
+struct rk_clocksource {
+	struct clocksource cs;
+	struct rk_timer timer;
+};
+
+enum {
+	ROCKCHIP_CLKSRC_CLOCKEVENT = 0,
+	ROCKCHIP_CLKSRC_CLOCKSOURCE = 1,
+};
+
 static struct rk_clock_event_device bc_timer;
+static struct rk_clocksource cs_timer;
+static int rk_next_clksrc = ROCKCHIP_CLKSRC_CLOCKEVENT;
 
 static inline struct rk_clock_event_device*
 rk_clock_event_device(struct clock_event_device *ce)
@@ -63,11 +78,37 @@ static inline void rk_timer_enable(struct rk_timer *timer, u32 flags)
 	writel_relaxed(TIMER_ENABLE | flags, timer->ctrl);
 }
 
-static void rk_timer_update_counter(unsigned long cycles,
-				    struct rk_timer *timer)
+static void rk_timer_update_counter(u64 cycles, struct rk_timer *timer)
+{
+	u32 lower = cycles & 0xFFFFFFFF;
+	u32 upper = (cycles >> 32) & 0xFFFFFFFF;
+
+	writel_relaxed(lower, timer->base + TIMER_LOAD_COUNT0);
+	writel_relaxed(upper, timer->base + TIMER_LOAD_COUNT1);
+}
+
+static u64 notrace _rk_timer_counter_read(struct rk_timer *timer)
 {
-	writel_relaxed(cycles, timer->base + TIMER_LOAD_COUNT0);
-	writel_relaxed(0, timer->base + TIMER_LOAD_COUNT1);
+	u64 counter;
+	u32 lower;
+	u32 upper, old_upper;
+
+	upper = readl_relaxed(timer->base + TIMER_CURRENT_VALUE1);
+	do {
+		old_upper = upper;
+		lower = readl_relaxed(timer->base + TIMER_CURRENT_VALUE0);
+		upper = readl_relaxed(timer->base + TIMER_CURRENT_VALUE1);
+	} while (upper != old_upper);
+
+	counter = upper;
+	counter <<= 32;
+	counter |= lower;
+	return counter;
+}
+
+static u64 rk_timer_counter_read(struct rk_timer *timer)
+{
+	return _rk_timer_counter_read(timer);
 }
 
 static void rk_timer_interrupt_clear(struct rk_timer *timer)
@@ -120,13 +161,46 @@ static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static u64 rk_timer_clocksource_read(struct clocksource *cs)
+{
+	struct rk_clocksource *_cs =
+		container_of(cs, struct rk_clocksource, cs);
+
+	return ~rk_timer_counter_read(&_cs->timer);
+}
+
+static u64 notrace rk_timer_sched_clock_read(void)
+{
+	struct rk_clocksource *_cs = &cs_timer;
+
+	return ~_rk_timer_counter_read(&_cs->timer);
+}
+
 static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 {
-	struct clock_event_device *ce = &bc_timer.ce;
-	struct rk_timer *timer = &bc_timer.timer;
+	struct clock_event_device *ce = NULL;
+	struct clocksource *cs = NULL;
+	struct rk_timer *timer = NULL;
 	struct clk *timer_clk;
 	struct clk *pclk;
 	int ret = -EINVAL, irq;
+	int clksrc;
+
+	clksrc = rk_next_clksrc;
+	rk_next_clksrc++;
+
+	switch (clksrc) {
+	case ROCKCHIP_CLKSRC_CLOCKEVENT:
+		ce = &bc_timer.ce;
+		timer = &bc_timer.timer;
+		break;
+	case ROCKCHIP_CLKSRC_CLOCKSOURCE:
+		cs = &cs_timer.cs;
+		timer = &cs_timer.timer;
+		break;
+	default:
+		return -ENODEV;
+	}
 
 	timer->base = of_iomap(np, 0);
 	if (!timer->base) {
@@ -170,26 +244,49 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 		goto out_irq;
 	}
 
-	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_possible_mask;
-	ce->rating = 250;
+	if (ce) {
+		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_possible_mask;
+		ce->rating = 250;
+	}
+
+	if (cs) {
+		cs->name = TIMER_NAME;
+		cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
+		cs->mask = CLOCKSOURCE_MASK(64);
+		cs->read = rk_timer_clocksource_read;
+		cs->rating = 250;
+	}
 
 	rk_timer_interrupt_clear(timer);
 	rk_timer_disable(timer);
 
-	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);
-		goto out_irq;
+	if (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);
+			goto out_irq;
+		}
+
+		clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
 	}
 
-	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
+	if (cs) {
+		rk_timer_update_counter(U64_MAX, timer);
+		rk_timer_enable(timer, 0);
+		clocksource_register_hz(cs, timer->freq);
+		sched_clock_register(rk_timer_sched_clock_read, 64,
+				     timer->freq);
+	}
 
 	return 0;
 
-- 
1.7.9.5

[PATCH v5 8/8] clocksource/drivers/rockchip_timer: implement clocksource timer

[Alexander Kochetkov]

The clock supplying the arm-global-timer on the rk3188 is coming from the
the cpu clock itself and thus changes its rate everytime cpufreq adjusts
the cpu frequency making this timer unsuitable as a stable clocksource
and sched clock.

The rk3188, rk3288 and following socs share a separate timer block already
handled by the rockchip-timer driver. Therefore adapt this driver to also
be able to act as clocksource and sched clock on rk3188.

In order to test clocksource you can run following commands and check
how much time it take in real. On rk3188 it take about ~45 seconds.

    cpufreq-set -f 1.6GHZ
    date; sleep 60; date

In order to use the patch you need to declare two timers in the dts
file. The first timer will be initialized as clockevent provider
and the second one as clocksource. The clockevent must be from
alive subsystem as it used as backup for the local timers at sleep
time.

The patch does not break compatibility with older device tree files.
The older device tree files contain only one timer. The timer
will be initialized as clockevent, as expected.

rk3288 (and probably anything newer) is irrelevant to this patch,
as it has the arch timer interface. This patch may be usefull
for Cortex-A9/A5 based parts.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
Reviwed-by: Heiko St?bner <heiko@sntech.de>
---
 drivers/clocksource/rockchip_timer.c |  137 +++++++++++++++++++++++++++++-----
 1 file changed, 117 insertions(+), 20 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index 61c3bb1..3ff533c 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -11,6 +11,7 @@
 #include <linux/clockchips.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
+#include <linux/sched_clock.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
@@ -19,6 +20,8 @@
 
 #define TIMER_LOAD_COUNT0	0x00
 #define TIMER_LOAD_COUNT1	0x04
+#define TIMER_CURRENT_VALUE0	0x08
+#define TIMER_CURRENT_VALUE1	0x0C
 #define TIMER_CONTROL_REG3288	0x10
 #define TIMER_CONTROL_REG3399	0x1c
 #define TIMER_INT_STATUS	0x18
@@ -40,7 +43,19 @@ struct rk_clock_event_device {
 	struct rk_timer timer;
 };
 
+struct rk_clocksource {
+	struct clocksource cs;
+	struct rk_timer timer;
+};
+
+enum {
+	ROCKCHIP_CLKSRC_CLOCKEVENT = 0,
+	ROCKCHIP_CLKSRC_CLOCKSOURCE = 1,
+};
+
 static struct rk_clock_event_device bc_timer;
+static struct rk_clocksource cs_timer;
+static int rk_next_clksrc = ROCKCHIP_CLKSRC_CLOCKEVENT;
 
 static inline struct rk_clock_event_device*
 rk_clock_event_device(struct clock_event_device *ce)
@@ -63,11 +78,37 @@ static inline void rk_timer_enable(struct rk_timer *timer, u32 flags)
 	writel_relaxed(TIMER_ENABLE | flags, timer->ctrl);
 }
 
-static void rk_timer_update_counter(unsigned long cycles,
-				    struct rk_timer *timer)
+static void rk_timer_update_counter(u64 cycles, struct rk_timer *timer)
+{
+	u32 lower = cycles & 0xFFFFFFFF;
+	u32 upper = (cycles >> 32) & 0xFFFFFFFF;
+
+	writel_relaxed(lower, timer->base + TIMER_LOAD_COUNT0);
+	writel_relaxed(upper, timer->base + TIMER_LOAD_COUNT1);
+}
+
+static u64 notrace _rk_timer_counter_read(struct rk_timer *timer)
 {
-	writel_relaxed(cycles, timer->base + TIMER_LOAD_COUNT0);
-	writel_relaxed(0, timer->base + TIMER_LOAD_COUNT1);
+	u64 counter;
+	u32 lower;
+	u32 upper, old_upper;
+
+	upper = readl_relaxed(timer->base + TIMER_CURRENT_VALUE1);
+	do {
+		old_upper = upper;
+		lower = readl_relaxed(timer->base + TIMER_CURRENT_VALUE0);
+		upper = readl_relaxed(timer->base + TIMER_CURRENT_VALUE1);
+	} while (upper != old_upper);
+
+	counter = upper;
+	counter <<= 32;
+	counter |= lower;
+	return counter;
+}
+
+static u64 rk_timer_counter_read(struct rk_timer *timer)
+{
+	return _rk_timer_counter_read(timer);
 }
 
 static void rk_timer_interrupt_clear(struct rk_timer *timer)
@@ -120,13 +161,46 @@ static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static u64 rk_timer_clocksource_read(struct clocksource *cs)
+{
+	struct rk_clocksource *_cs =
+		container_of(cs, struct rk_clocksource, cs);
+
+	return ~rk_timer_counter_read(&_cs->timer);
+}
+
+static u64 notrace rk_timer_sched_clock_read(void)
+{
+	struct rk_clocksource *_cs = &cs_timer;
+
+	return ~_rk_timer_counter_read(&_cs->timer);
+}
+
 static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 {
-	struct clock_event_device *ce = &bc_timer.ce;
-	struct rk_timer *timer = &bc_timer.timer;
+	struct clock_event_device *ce = NULL;
+	struct clocksource *cs = NULL;
+	struct rk_timer *timer = NULL;
 	struct clk *timer_clk;
 	struct clk *pclk;
 	int ret = -EINVAL, irq;
+	int clksrc;
+
+	clksrc = rk_next_clksrc;
+	rk_next_clksrc++;
+
+	switch (clksrc) {
+	case ROCKCHIP_CLKSRC_CLOCKEVENT:
+		ce = &bc_timer.ce;
+		timer = &bc_timer.timer;
+		break;
+	case ROCKCHIP_CLKSRC_CLOCKSOURCE:
+		cs = &cs_timer.cs;
+		timer = &cs_timer.timer;
+		break;
+	default:
+		return -ENODEV;
+	}
 
 	timer->base = of_iomap(np, 0);
 	if (!timer->base) {
@@ -170,26 +244,49 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 		goto out_irq;
 	}
 
-	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_possible_mask;
-	ce->rating = 250;
+	if (ce) {
+		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_possible_mask;
+		ce->rating = 250;
+	}
+
+	if (cs) {
+		cs->name = TIMER_NAME;
+		cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
+		cs->mask = CLOCKSOURCE_MASK(64);
+		cs->read = rk_timer_clocksource_read;
+		cs->rating = 250;
+	}
 
 	rk_timer_interrupt_clear(timer);
 	rk_timer_disable(timer);
 
-	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);
-		goto out_irq;
+	if (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);
+			goto out_irq;
+		}
+
+		clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
 	}
 
-	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
+	if (cs) {
+		rk_timer_update_counter(U64_MAX, timer);
+		rk_timer_enable(timer, 0);
+		clocksource_register_hz(cs, timer->freq);
+		sched_clock_register(rk_timer_sched_clock_read, 64,
+				     timer->freq);
+	}
 
 	return 0;
 
-- 
1.7.9.5

[PATCH v5 8/8] clocksource/drivers/rockchip_timer: implement clocksource timer

[Alexander Kochetkov]

The clock supplying the arm-global-timer on the rk3188 is coming from the
the cpu clock itself and thus changes its rate everytime cpufreq adjusts
the cpu frequency making this timer unsuitable as a stable clocksource
and sched clock.

The rk3188, rk3288 and following socs share a separate timer block already
handled by the rockchip-timer driver. Therefore adapt this driver to also
be able to act as clocksource and sched clock on rk3188.

In order to test clocksource you can run following commands and check
how much time it take in real. On rk3188 it take about ~45 seconds.

    cpufreq-set -f 1.6GHZ
    date; sleep 60; date

In order to use the patch you need to declare two timers in the dts
file. The first timer will be initialized as clockevent provider
and the second one as clocksource. The clockevent must be from
alive subsystem as it used as backup for the local timers at sleep
time.

The patch does not break compatibility with older device tree files.
The older device tree files contain only one timer. The timer
will be initialized as clockevent, as expected.

rk3288 (and probably anything newer) is irrelevant to this patch,
as it has the arch timer interface. This patch may be usefull
for Cortex-A9/A5 based parts.

Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Reviwed-by: Heiko Stübner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
---
 drivers/clocksource/rockchip_timer.c |  137 +++++++++++++++++++++++++++++-----
 1 file changed, 117 insertions(+), 20 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index 61c3bb1..3ff533c 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -11,6 +11,7 @@
 #include <linux/clockchips.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
+#include <linux/sched_clock.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
@@ -19,6 +20,8 @@
 
 #define TIMER_LOAD_COUNT0	0x00
 #define TIMER_LOAD_COUNT1	0x04
+#define TIMER_CURRENT_VALUE0	0x08
+#define TIMER_CURRENT_VALUE1	0x0C
 #define TIMER_CONTROL_REG3288	0x10
 #define TIMER_CONTROL_REG3399	0x1c
 #define TIMER_INT_STATUS	0x18
@@ -40,7 +43,19 @@ struct rk_clock_event_device {
 	struct rk_timer timer;
 };
 
+struct rk_clocksource {
+	struct clocksource cs;
+	struct rk_timer timer;
+};
+
+enum {
+	ROCKCHIP_CLKSRC_CLOCKEVENT = 0,
+	ROCKCHIP_CLKSRC_CLOCKSOURCE = 1,
+};
+
 static struct rk_clock_event_device bc_timer;
+static struct rk_clocksource cs_timer;
+static int rk_next_clksrc = ROCKCHIP_CLKSRC_CLOCKEVENT;
 
 static inline struct rk_clock_event_device*
 rk_clock_event_device(struct clock_event_device *ce)
@@ -63,11 +78,37 @@ static inline void rk_timer_enable(struct rk_timer *timer, u32 flags)
 	writel_relaxed(TIMER_ENABLE | flags, timer->ctrl);
 }
 
-static void rk_timer_update_counter(unsigned long cycles,
-				    struct rk_timer *timer)
+static void rk_timer_update_counter(u64 cycles, struct rk_timer *timer)
+{
+	u32 lower = cycles & 0xFFFFFFFF;
+	u32 upper = (cycles >> 32) & 0xFFFFFFFF;
+
+	writel_relaxed(lower, timer->base + TIMER_LOAD_COUNT0);
+	writel_relaxed(upper, timer->base + TIMER_LOAD_COUNT1);
+}
+
+static u64 notrace _rk_timer_counter_read(struct rk_timer *timer)
 {
-	writel_relaxed(cycles, timer->base + TIMER_LOAD_COUNT0);
-	writel_relaxed(0, timer->base + TIMER_LOAD_COUNT1);
+	u64 counter;
+	u32 lower;
+	u32 upper, old_upper;
+
+	upper = readl_relaxed(timer->base + TIMER_CURRENT_VALUE1);
+	do {
+		old_upper = upper;
+		lower = readl_relaxed(timer->base + TIMER_CURRENT_VALUE0);
+		upper = readl_relaxed(timer->base + TIMER_CURRENT_VALUE1);
+	} while (upper != old_upper);
+
+	counter = upper;
+	counter <<= 32;
+	counter |= lower;
+	return counter;
+}
+
+static u64 rk_timer_counter_read(struct rk_timer *timer)
+{
+	return _rk_timer_counter_read(timer);
 }
 
 static void rk_timer_interrupt_clear(struct rk_timer *timer)
@@ -120,13 +161,46 @@ static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static u64 rk_timer_clocksource_read(struct clocksource *cs)
+{
+	struct rk_clocksource *_cs =
+		container_of(cs, struct rk_clocksource, cs);
+
+	return ~rk_timer_counter_read(&_cs->timer);
+}
+
+static u64 notrace rk_timer_sched_clock_read(void)
+{
+	struct rk_clocksource *_cs = &cs_timer;
+
+	return ~_rk_timer_counter_read(&_cs->timer);
+}
+
 static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 {
-	struct clock_event_device *ce = &bc_timer.ce;
-	struct rk_timer *timer = &bc_timer.timer;
+	struct clock_event_device *ce = NULL;
+	struct clocksource *cs = NULL;
+	struct rk_timer *timer = NULL;
 	struct clk *timer_clk;
 	struct clk *pclk;
 	int ret = -EINVAL, irq;
+	int clksrc;
+
+	clksrc = rk_next_clksrc;
+	rk_next_clksrc++;
+
+	switch (clksrc) {
+	case ROCKCHIP_CLKSRC_CLOCKEVENT:
+		ce = &bc_timer.ce;
+		timer = &bc_timer.timer;
+		break;
+	case ROCKCHIP_CLKSRC_CLOCKSOURCE:
+		cs = &cs_timer.cs;
+		timer = &cs_timer.timer;
+		break;
+	default:
+		return -ENODEV;
+	}
 
 	timer->base = of_iomap(np, 0);
 	if (!timer->base) {
@@ -170,26 +244,49 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 		goto out_irq;
 	}
 
-	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_possible_mask;
-	ce->rating = 250;
+	if (ce) {
+		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_possible_mask;
+		ce->rating = 250;
+	}
+
+	if (cs) {
+		cs->name = TIMER_NAME;
+		cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
+		cs->mask = CLOCKSOURCE_MASK(64);
+		cs->read = rk_timer_clocksource_read;
+		cs->rating = 250;
+	}
 
 	rk_timer_interrupt_clear(timer);
 	rk_timer_disable(timer);
 
-	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);
-		goto out_irq;
+	if (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);
+			goto out_irq;
+		}
+
+		clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
 	}
 
-	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
+	if (cs) {
+		rk_timer_update_counter(U64_MAX, timer);
+		rk_timer_enable(timer, 0);
+		clocksource_register_hz(cs, timer->freq);
+		sched_clock_register(rk_timer_sched_clock_read, 64,
+				     timer->freq);
+	}
 
 	return 0;
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 8/8] clocksource/drivers/rockchip_timer: implement clocksource timer

[Daniel Lezcano]

On Tue, Jan 24, 2017 at 03:16:43PM +0300, Alexander Kochetkov wrote:
> The clock supplying the arm-global-timer on the rk3188 is coming from the
> the cpu clock itself and thus changes its rate everytime cpufreq adjusts
> the cpu frequency making this timer unsuitable as a stable clocksource
> and sched clock.
> 
> The rk3188, rk3288 and following socs share a separate timer block already
> handled by the rockchip-timer driver. Therefore adapt this driver to also
> be able to act as clocksource and sched clock on rk3188.
> 
> In order to test clocksource you can run following commands and check
> how much time it take in real. On rk3188 it take about ~45 seconds.
> 
>     cpufreq-set -f 1.6GHZ
>     date; sleep 60; date
> 
> In order to use the patch you need to declare two timers in the dts
> file. The first timer will be initialized as clockevent provider
> and the second one as clocksource. The clockevent must be from
> alive subsystem as it used as backup for the local timers at sleep
> time.
> 
> The patch does not break compatibility with older device tree files.
> The older device tree files contain only one timer. The timer
> will be initialized as clockevent, as expected.
> 
> rk3288 (and probably anything newer) is irrelevant to this patch,
> as it has the arch timer interface. This patch may be usefull
> for Cortex-A9/A5 based parts.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> Reviwed-by: Heiko Stübner <heiko@sntech.de>
> ---
>  drivers/clocksource/rockchip_timer.c |  137 +++++++++++++++++++++++++++++-----
>  1 file changed, 117 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
> index 61c3bb1..3ff533c 100644
> --- a/drivers/clocksource/rockchip_timer.c
> +++ b/drivers/clocksource/rockchip_timer.c
> @@ -11,6 +11,7 @@
>  #include <linux/clockchips.h>
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
> +#include <linux/sched_clock.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
> @@ -19,6 +20,8 @@
>  
>  #define TIMER_LOAD_COUNT0	0x00
>  #define TIMER_LOAD_COUNT1	0x04
> +#define TIMER_CURRENT_VALUE0	0x08
> +#define TIMER_CURRENT_VALUE1	0x0C
>  #define TIMER_CONTROL_REG3288	0x10
>  #define TIMER_CONTROL_REG3399	0x1c
>  #define TIMER_INT_STATUS	0x18
> @@ -40,7 +43,19 @@ struct rk_clock_event_device {
>  	struct rk_timer timer;
>  };
>  
> +struct rk_clocksource {
> +	struct clocksource cs;
> +	struct rk_timer timer;
> +};
> +
> +enum {
> +	ROCKCHIP_CLKSRC_CLOCKEVENT = 0,
> +	ROCKCHIP_CLKSRC_CLOCKSOURCE = 1,
> +};
> +

This is over-engineered.

Simply convert bc_timer and cs_timer to pointers (may be take the opportunity to
change the name eg. bc_timer -> rk_clkevt and cs -> rk_clksrc).

Then in the init function check:

if (!rk_clkevt) {
	init clkevt
} else if (!rk_clksrc) {
	init clksrc
} else {
	Toooo many timer definitions ...
}

>  static struct rk_clock_event_device bc_timer;
> +static struct rk_clocksource cs_timer;
> +static int rk_next_clksrc = ROCKCHIP_CLKSRC_CLOCKEVENT;
>  
>  static inline struct rk_clock_event_device*
>  rk_clock_event_device(struct clock_event_device *ce)
> @@ -63,11 +78,37 @@ static inline void rk_timer_enable(struct rk_timer *timer, u32 flags)
>  	writel_relaxed(TIMER_ENABLE | flags, timer->ctrl);
>  }
>  
> -static void rk_timer_update_counter(unsigned long cycles,
> -				    struct rk_timer *timer)
> +static void rk_timer_update_counter(u64 cycles, struct rk_timer *timer)
> +{
> +	u32 lower = cycles & 0xFFFFFFFF;
> +	u32 upper = (cycles >> 32) & 0xFFFFFFFF;
> +
> +	writel_relaxed(lower, timer->base + TIMER_LOAD_COUNT0);
> +	writel_relaxed(upper, timer->base + TIMER_LOAD_COUNT1);
> +}
> +
> +static u64 notrace _rk_timer_counter_read(struct rk_timer *timer)
>  {
> -	writel_relaxed(cycles, timer->base + TIMER_LOAD_COUNT0);
> -	writel_relaxed(0, timer->base + TIMER_LOAD_COUNT1);
> +	u64 counter;
> +	u32 lower;
> +	u32 upper, old_upper;
> +
> +	upper = readl_relaxed(timer->base + TIMER_CURRENT_VALUE1);
> +	do {
> +		old_upper = upper;
> +		lower = readl_relaxed(timer->base + TIMER_CURRENT_VALUE0);
> +		upper = readl_relaxed(timer->base + TIMER_CURRENT_VALUE1);
> +	} while (upper != old_upper);
> +
> +	counter = upper;
> +	counter <<= 32;
> +	counter |= lower;
> +	return counter;
> +}
> +

Do you really want to use the 64 bits version? It has a non negligeable impact on
the performances and there is no deep idle state allowing a long and huge
energy saving. IOW, we consume more energy and time by computing the 64bits
clocksource for zero benefit. I recommend to convert to 32bits.

> +static u64 rk_timer_counter_read(struct rk_timer *timer)
> +{
> +	return _rk_timer_counter_read(timer);
>  }
>  
>  static void rk_timer_interrupt_clear(struct rk_timer *timer)
> @@ -120,13 +161,46 @@ static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +static u64 rk_timer_clocksource_read(struct clocksource *cs)
> +{
> +	struct rk_clocksource *_cs =
> +		container_of(cs, struct rk_clocksource, cs);
> +
> +	return ~rk_timer_counter_read(&_cs->timer);
> +}
> +
> +static u64 notrace rk_timer_sched_clock_read(void)
> +{
> +	struct rk_clocksource *_cs = &cs_timer;
> +
> +	return ~_rk_timer_counter_read(&_cs->timer);
> +}
> +

You should be able to replace all this code by:

clocksource_mmio_init() with clocksource_mmio_readl_down().

>  static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  {
> -	struct clock_event_device *ce = &bc_timer.ce;
> -	struct rk_timer *timer = &bc_timer.timer;
> +	struct clock_event_device *ce = NULL;
> +	struct clocksource *cs = NULL;
> +	struct rk_timer *timer = NULL;
>  	struct clk *timer_clk;
>  	struct clk *pclk;
>  	int ret = -EINVAL, irq;
> +	int clksrc;
> +
> +	clksrc = rk_next_clksrc;
> +	rk_next_clksrc++;
> +
> +	switch (clksrc) {
> +	case ROCKCHIP_CLKSRC_CLOCKEVENT:
> +		ce = &bc_timer.ce;
> +		timer = &bc_timer.timer;
> +		break;
> +	case ROCKCHIP_CLKSRC_CLOCKSOURCE:
> +		cs = &cs_timer.cs;
> +		timer = &cs_timer.timer;
> +		break;
> +	default:
> +		return -ENODEV;
> +	}
>  
>  	timer->base = of_iomap(np, 0);
>  	if (!timer->base) {
> @@ -170,26 +244,49 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  		goto out_irq;
>  	}
>  
> -	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_possible_mask;
> -	ce->rating = 250;
> +	if (ce) {
> +		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_possible_mask;
> +		ce->rating = 250;
> +	}
> +
> +	if (cs) {
> +		cs->name = TIMER_NAME;
> +		cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
> +		cs->mask = CLOCKSOURCE_MASK(64);
> +		cs->read = rk_timer_clocksource_read;
> +		cs->rating = 250;
> +	}
>  
>  	rk_timer_interrupt_clear(timer);
>  	rk_timer_disable(timer);
>  
> -	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);
> -		goto out_irq;
> +	if (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);
> +			goto out_irq;
> +		}
> +
> +		clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
>  	}

'ce' blocks can be grouped together.

>  
> -	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
> +	if (cs) {
> +		rk_timer_update_counter(U64_MAX, timer);
> +		rk_timer_enable(timer, 0);
> +		clocksource_register_hz(cs, timer->freq);
> +		sched_clock_register(rk_timer_sched_clock_read, 64,
> +				     timer->freq);

	The 'cs' can be replaced by clocksource_mmio_init.

> +	}
>  
>  	return 0;
>  
> -- 
> 1.7.9.5
> 

-- 

 <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 v5 8/8] clocksource/drivers/rockchip_timer: implement clocksource timer

[Daniel Lezcano]

On Tue, Jan 24, 2017 at 03:16:43PM +0300, Alexander Kochetkov wrote:
> The clock supplying the arm-global-timer on the rk3188 is coming from the
> the cpu clock itself and thus changes its rate everytime cpufreq adjusts
> the cpu frequency making this timer unsuitable as a stable clocksource
> and sched clock.
> 
> The rk3188, rk3288 and following socs share a separate timer block already
> handled by the rockchip-timer driver. Therefore adapt this driver to also
> be able to act as clocksource and sched clock on rk3188.
> 
> In order to test clocksource you can run following commands and check
> how much time it take in real. On rk3188 it take about ~45 seconds.
> 
>     cpufreq-set -f 1.6GHZ
>     date; sleep 60; date
> 
> In order to use the patch you need to declare two timers in the dts
> file. The first timer will be initialized as clockevent provider
> and the second one as clocksource. The clockevent must be from
> alive subsystem as it used as backup for the local timers at sleep
> time.
> 
> The patch does not break compatibility with older device tree files.
> The older device tree files contain only one timer. The timer
> will be initialized as clockevent, as expected.
> 
> rk3288 (and probably anything newer) is irrelevant to this patch,
> as it has the arch timer interface. This patch may be usefull
> for Cortex-A9/A5 based parts.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
> Reviwed-by: Heiko St?bner <heiko@sntech.de>
> ---
>  drivers/clocksource/rockchip_timer.c |  137 +++++++++++++++++++++++++++++-----
>  1 file changed, 117 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
> index 61c3bb1..3ff533c 100644
> --- a/drivers/clocksource/rockchip_timer.c
> +++ b/drivers/clocksource/rockchip_timer.c
> @@ -11,6 +11,7 @@
>  #include <linux/clockchips.h>
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
> +#include <linux/sched_clock.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
> @@ -19,6 +20,8 @@
>  
>  #define TIMER_LOAD_COUNT0	0x00
>  #define TIMER_LOAD_COUNT1	0x04
> +#define TIMER_CURRENT_VALUE0	0x08
> +#define TIMER_CURRENT_VALUE1	0x0C
>  #define TIMER_CONTROL_REG3288	0x10
>  #define TIMER_CONTROL_REG3399	0x1c
>  #define TIMER_INT_STATUS	0x18
> @@ -40,7 +43,19 @@ struct rk_clock_event_device {
>  	struct rk_timer timer;
>  };
>  
> +struct rk_clocksource {
> +	struct clocksource cs;
> +	struct rk_timer timer;
> +};
> +
> +enum {
> +	ROCKCHIP_CLKSRC_CLOCKEVENT = 0,
> +	ROCKCHIP_CLKSRC_CLOCKSOURCE = 1,
> +};
> +

This is over-engineered.

Simply convert bc_timer and cs_timer to pointers (may be take the opportunity to
change the name eg. bc_timer -> rk_clkevt and cs -> rk_clksrc).

Then in the init function check:

if (!rk_clkevt) {
	init clkevt
} else if (!rk_clksrc) {
	init clksrc
} else {
	Toooo many timer definitions ...
}

>  static struct rk_clock_event_device bc_timer;
> +static struct rk_clocksource cs_timer;
> +static int rk_next_clksrc = ROCKCHIP_CLKSRC_CLOCKEVENT;
>  
>  static inline struct rk_clock_event_device*
>  rk_clock_event_device(struct clock_event_device *ce)
> @@ -63,11 +78,37 @@ static inline void rk_timer_enable(struct rk_timer *timer, u32 flags)
>  	writel_relaxed(TIMER_ENABLE | flags, timer->ctrl);
>  }
>  
> -static void rk_timer_update_counter(unsigned long cycles,
> -				    struct rk_timer *timer)
> +static void rk_timer_update_counter(u64 cycles, struct rk_timer *timer)
> +{
> +	u32 lower = cycles & 0xFFFFFFFF;
> +	u32 upper = (cycles >> 32) & 0xFFFFFFFF;
> +
> +	writel_relaxed(lower, timer->base + TIMER_LOAD_COUNT0);
> +	writel_relaxed(upper, timer->base + TIMER_LOAD_COUNT1);
> +}
> +
> +static u64 notrace _rk_timer_counter_read(struct rk_timer *timer)
>  {
> -	writel_relaxed(cycles, timer->base + TIMER_LOAD_COUNT0);
> -	writel_relaxed(0, timer->base + TIMER_LOAD_COUNT1);
> +	u64 counter;
> +	u32 lower;
> +	u32 upper, old_upper;
> +
> +	upper = readl_relaxed(timer->base + TIMER_CURRENT_VALUE1);
> +	do {
> +		old_upper = upper;
> +		lower = readl_relaxed(timer->base + TIMER_CURRENT_VALUE0);
> +		upper = readl_relaxed(timer->base + TIMER_CURRENT_VALUE1);
> +	} while (upper != old_upper);
> +
> +	counter = upper;
> +	counter <<= 32;
> +	counter |= lower;
> +	return counter;
> +}
> +

Do you really want to use the 64 bits version? It has a non negligeable impact on
the performances and there is no deep idle state allowing a long and huge
energy saving. IOW, we consume more energy and time by computing the 64bits
clocksource for zero benefit. I recommend to convert to 32bits.

> +static u64 rk_timer_counter_read(struct rk_timer *timer)
> +{
> +	return _rk_timer_counter_read(timer);
>  }
>  
>  static void rk_timer_interrupt_clear(struct rk_timer *timer)
> @@ -120,13 +161,46 @@ static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +static u64 rk_timer_clocksource_read(struct clocksource *cs)
> +{
> +	struct rk_clocksource *_cs =
> +		container_of(cs, struct rk_clocksource, cs);
> +
> +	return ~rk_timer_counter_read(&_cs->timer);
> +}
> +
> +static u64 notrace rk_timer_sched_clock_read(void)
> +{
> +	struct rk_clocksource *_cs = &cs_timer;
> +
> +	return ~_rk_timer_counter_read(&_cs->timer);
> +}
> +

You should be able to replace all this code by:

clocksource_mmio_init() with clocksource_mmio_readl_down().

>  static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  {
> -	struct clock_event_device *ce = &bc_timer.ce;
> -	struct rk_timer *timer = &bc_timer.timer;
> +	struct clock_event_device *ce = NULL;
> +	struct clocksource *cs = NULL;
> +	struct rk_timer *timer = NULL;
>  	struct clk *timer_clk;
>  	struct clk *pclk;
>  	int ret = -EINVAL, irq;
> +	int clksrc;
> +
> +	clksrc = rk_next_clksrc;
> +	rk_next_clksrc++;
> +
> +	switch (clksrc) {
> +	case ROCKCHIP_CLKSRC_CLOCKEVENT:
> +		ce = &bc_timer.ce;
> +		timer = &bc_timer.timer;
> +		break;
> +	case ROCKCHIP_CLKSRC_CLOCKSOURCE:
> +		cs = &cs_timer.cs;
> +		timer = &cs_timer.timer;
> +		break;
> +	default:
> +		return -ENODEV;
> +	}
>  
>  	timer->base = of_iomap(np, 0);
>  	if (!timer->base) {
> @@ -170,26 +244,49 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  		goto out_irq;
>  	}
>  
> -	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_possible_mask;
> -	ce->rating = 250;
> +	if (ce) {
> +		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_possible_mask;
> +		ce->rating = 250;
> +	}
> +
> +	if (cs) {
> +		cs->name = TIMER_NAME;
> +		cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
> +		cs->mask = CLOCKSOURCE_MASK(64);
> +		cs->read = rk_timer_clocksource_read;
> +		cs->rating = 250;
> +	}
>  
>  	rk_timer_interrupt_clear(timer);
>  	rk_timer_disable(timer);
>  
> -	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);
> -		goto out_irq;
> +	if (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);
> +			goto out_irq;
> +		}
> +
> +		clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
>  	}

'ce' blocks can be grouped together.

>  
> -	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
> +	if (cs) {
> +		rk_timer_update_counter(U64_MAX, timer);
> +		rk_timer_enable(timer, 0);
> +		clocksource_register_hz(cs, timer->freq);
> +		sched_clock_register(rk_timer_sched_clock_read, 64,
> +				     timer->freq);

	The 'cs' can be replaced by clocksource_mmio_init.

> +	}
>  
>  	return 0;
>  
> -- 
> 1.7.9.5
> 

-- 

 <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 v5 8/8] clocksource/drivers/rockchip_timer: implement clocksource timer

[Daniel Lezcano]

On Tue, Jan 24, 2017 at 03:16:43PM +0300, Alexander Kochetkov wrote:
> The clock supplying the arm-global-timer on the rk3188 is coming from the
> the cpu clock itself and thus changes its rate everytime cpufreq adjusts
> the cpu frequency making this timer unsuitable as a stable clocksource
> and sched clock.
> 
> The rk3188, rk3288 and following socs share a separate timer block already
> handled by the rockchip-timer driver. Therefore adapt this driver to also
> be able to act as clocksource and sched clock on rk3188.
> 
> In order to test clocksource you can run following commands and check
> how much time it take in real. On rk3188 it take about ~45 seconds.
> 
>     cpufreq-set -f 1.6GHZ
>     date; sleep 60; date
> 
> In order to use the patch you need to declare two timers in the dts
> file. The first timer will be initialized as clockevent provider
> and the second one as clocksource. The clockevent must be from
> alive subsystem as it used as backup for the local timers at sleep
> time.
> 
> The patch does not break compatibility with older device tree files.
> The older device tree files contain only one timer. The timer
> will be initialized as clockevent, as expected.
> 
> rk3288 (and probably anything newer) is irrelevant to this patch,
> as it has the arch timer interface. This patch may be usefull
> for Cortex-A9/A5 based parts.
> 
> Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reviwed-by: Heiko Stübner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> ---
>  drivers/clocksource/rockchip_timer.c |  137 +++++++++++++++++++++++++++++-----
>  1 file changed, 117 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
> index 61c3bb1..3ff533c 100644
> --- a/drivers/clocksource/rockchip_timer.c
> +++ b/drivers/clocksource/rockchip_timer.c
> @@ -11,6 +11,7 @@
>  #include <linux/clockchips.h>
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
> +#include <linux/sched_clock.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
> @@ -19,6 +20,8 @@
>  
>  #define TIMER_LOAD_COUNT0	0x00
>  #define TIMER_LOAD_COUNT1	0x04
> +#define TIMER_CURRENT_VALUE0	0x08
> +#define TIMER_CURRENT_VALUE1	0x0C
>  #define TIMER_CONTROL_REG3288	0x10
>  #define TIMER_CONTROL_REG3399	0x1c
>  #define TIMER_INT_STATUS	0x18
> @@ -40,7 +43,19 @@ struct rk_clock_event_device {
>  	struct rk_timer timer;
>  };
>  
> +struct rk_clocksource {
> +	struct clocksource cs;
> +	struct rk_timer timer;
> +};
> +
> +enum {
> +	ROCKCHIP_CLKSRC_CLOCKEVENT = 0,
> +	ROCKCHIP_CLKSRC_CLOCKSOURCE = 1,
> +};
> +

This is over-engineered.

Simply convert bc_timer and cs_timer to pointers (may be take the opportunity to
change the name eg. bc_timer -> rk_clkevt and cs -> rk_clksrc).

Then in the init function check:

if (!rk_clkevt) {
	init clkevt
} else if (!rk_clksrc) {
	init clksrc
} else {
	Toooo many timer definitions ...
}

>  static struct rk_clock_event_device bc_timer;
> +static struct rk_clocksource cs_timer;
> +static int rk_next_clksrc = ROCKCHIP_CLKSRC_CLOCKEVENT;
>  
>  static inline struct rk_clock_event_device*
>  rk_clock_event_device(struct clock_event_device *ce)
> @@ -63,11 +78,37 @@ static inline void rk_timer_enable(struct rk_timer *timer, u32 flags)
>  	writel_relaxed(TIMER_ENABLE | flags, timer->ctrl);
>  }
>  
> -static void rk_timer_update_counter(unsigned long cycles,
> -				    struct rk_timer *timer)
> +static void rk_timer_update_counter(u64 cycles, struct rk_timer *timer)
> +{
> +	u32 lower = cycles & 0xFFFFFFFF;
> +	u32 upper = (cycles >> 32) & 0xFFFFFFFF;
> +
> +	writel_relaxed(lower, timer->base + TIMER_LOAD_COUNT0);
> +	writel_relaxed(upper, timer->base + TIMER_LOAD_COUNT1);
> +}
> +
> +static u64 notrace _rk_timer_counter_read(struct rk_timer *timer)
>  {
> -	writel_relaxed(cycles, timer->base + TIMER_LOAD_COUNT0);
> -	writel_relaxed(0, timer->base + TIMER_LOAD_COUNT1);
> +	u64 counter;
> +	u32 lower;
> +	u32 upper, old_upper;
> +
> +	upper = readl_relaxed(timer->base + TIMER_CURRENT_VALUE1);
> +	do {
> +		old_upper = upper;
> +		lower = readl_relaxed(timer->base + TIMER_CURRENT_VALUE0);
> +		upper = readl_relaxed(timer->base + TIMER_CURRENT_VALUE1);
> +	} while (upper != old_upper);
> +
> +	counter = upper;
> +	counter <<= 32;
> +	counter |= lower;
> +	return counter;
> +}
> +

Do you really want to use the 64 bits version? It has a non negligeable impact on
the performances and there is no deep idle state allowing a long and huge
energy saving. IOW, we consume more energy and time by computing the 64bits
clocksource for zero benefit. I recommend to convert to 32bits.

> +static u64 rk_timer_counter_read(struct rk_timer *timer)
> +{
> +	return _rk_timer_counter_read(timer);
>  }
>  
>  static void rk_timer_interrupt_clear(struct rk_timer *timer)
> @@ -120,13 +161,46 @@ static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +static u64 rk_timer_clocksource_read(struct clocksource *cs)
> +{
> +	struct rk_clocksource *_cs =
> +		container_of(cs, struct rk_clocksource, cs);
> +
> +	return ~rk_timer_counter_read(&_cs->timer);
> +}
> +
> +static u64 notrace rk_timer_sched_clock_read(void)
> +{
> +	struct rk_clocksource *_cs = &cs_timer;
> +
> +	return ~_rk_timer_counter_read(&_cs->timer);
> +}
> +

You should be able to replace all this code by:

clocksource_mmio_init() with clocksource_mmio_readl_down().

>  static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  {
> -	struct clock_event_device *ce = &bc_timer.ce;
> -	struct rk_timer *timer = &bc_timer.timer;
> +	struct clock_event_device *ce = NULL;
> +	struct clocksource *cs = NULL;
> +	struct rk_timer *timer = NULL;
>  	struct clk *timer_clk;
>  	struct clk *pclk;
>  	int ret = -EINVAL, irq;
> +	int clksrc;
> +
> +	clksrc = rk_next_clksrc;
> +	rk_next_clksrc++;
> +
> +	switch (clksrc) {
> +	case ROCKCHIP_CLKSRC_CLOCKEVENT:
> +		ce = &bc_timer.ce;
> +		timer = &bc_timer.timer;
> +		break;
> +	case ROCKCHIP_CLKSRC_CLOCKSOURCE:
> +		cs = &cs_timer.cs;
> +		timer = &cs_timer.timer;
> +		break;
> +	default:
> +		return -ENODEV;
> +	}
>  
>  	timer->base = of_iomap(np, 0);
>  	if (!timer->base) {
> @@ -170,26 +244,49 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
>  		goto out_irq;
>  	}
>  
> -	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_possible_mask;
> -	ce->rating = 250;
> +	if (ce) {
> +		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_possible_mask;
> +		ce->rating = 250;
> +	}
> +
> +	if (cs) {
> +		cs->name = TIMER_NAME;
> +		cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
> +		cs->mask = CLOCKSOURCE_MASK(64);
> +		cs->read = rk_timer_clocksource_read;
> +		cs->rating = 250;
> +	}
>  
>  	rk_timer_interrupt_clear(timer);
>  	rk_timer_disable(timer);
>  
> -	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);
> -		goto out_irq;
> +	if (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);
> +			goto out_irq;
> +		}
> +
> +		clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
>  	}

'ce' blocks can be grouped together.

>  
> -	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
> +	if (cs) {
> +		rk_timer_update_counter(U64_MAX, timer);
> +		rk_timer_enable(timer, 0);
> +		clocksource_register_hz(cs, timer->freq);
> +		sched_clock_register(rk_timer_sched_clock_read, 64,
> +				     timer->freq);

	The 'cs' can be replaced by clocksource_mmio_init.

> +	}
>  
>  	return 0;
>  
> -- 
> 1.7.9.5
> 

-- 

 <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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html