[PATCH 0/4] Enable the timer clock on DT MMP2

[PATCH 0/4] Enable the timer clock on DT MMP2

[Lubomir Rintel]

This makes the timer work for me on an OLPC 1.75, which is a
device-tree based MMP2 platform.

[PATCH 1/4] dt-bindings: mrvl,mmp-timer: add clock

[Lubomir Rintel]

The timer needs the timer clock to be enabled, otherwise it stops
ticking.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 Documentation/devicetree/bindings/timer/mrvl,mmp-timer.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/timer/mrvl,mmp-timer.txt b/Documentation/devicetree/bindings/timer/mrvl,mmp-timer.txt
index 9a6e251462e7..a2ede0bd12ca 100644
--- a/Documentation/devicetree/bindings/timer/mrvl,mmp-timer.txt
+++ b/Documentation/devicetree/bindings/timer/mrvl,mmp-timer.txt
@@ -4,10 +4,12 @@ Required properties:
 - compatible : Should be "mrvl,mmp-timer".
 - reg : Address and length of the register set of timer controller.
 - interrupts : Should be the interrupt number.
+- clocks : Should contain a single entry describing the clock input.
 
 Example:
 	timer0: timer@d4014000 {
 		compatible = "mrvl,mmp-timer";
 		reg = <0xd4014000 0x100>;
 		interrupts = <13>;
+		clocks = <&coreclk 2>;
 	};
-- 
2.17.1

[PATCH 2/4] DT: marvell,mmp2: add clock to the timer

[Lubomir Rintel]

The timer needs the timer clock to be enabled, otherwise it stops
ticking.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 arch/arm/boot/dts/mmp2.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/mmp2.dtsi b/arch/arm/boot/dts/mmp2.dtsi
index 766bbb8495b6..58e0098a007d 100644
--- a/arch/arm/boot/dts/mmp2.dtsi
+++ b/arch/arm/boot/dts/mmp2.dtsi
@@ -130,6 +130,7 @@
 				compatible = "mrvl,mmp-timer";
 				reg = <0xd4014000 0x100>;
 				interrupts = <13>;
+				clocks = <&soc_clocks MMP2_CLK_TIMER>;
 			};
 
 			uart1: uart@d4030000 {
-- 
2.17.1

[PATCH 3/4] ARM: mmp2: initialize clocks before the timer

[Lubomir Rintel]

The timer shall enable its clock.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 arch/arm/mach-mmp/mmp2-dt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-mmp/mmp2-dt.c b/arch/arm/mach-mmp/mmp2-dt.c
index 0341359b24a4..50c5e8b5be3d 100644
--- a/arch/arm/mach-mmp/mmp2-dt.c
+++ b/arch/arm/mach-mmp/mmp2-dt.c
@@ -26,8 +26,8 @@ static void __init mmp_init_time(void)
 #ifdef CONFIG_CACHE_TAUROS2
 	tauros2_init(0);
 #endif
-	mmp_dt_init_timer();
 	of_clk_init(NULL);
+	mmp_dt_init_timer();
 }
 
 static const char *const mmp2_dt_board_compat[] __initconst = {
-- 
2.17.1

[PATCH 4/4] ARM: mmp/mmp2: dt: enable the clock

[Lubomir Rintel]

The device-tree booted MMP2 needs to enable the timer clock, otherwise
the it would stop ticking when the boot finishes.

It can also use the clock rate from the clk, the non-DT boards need to
keep using the hardcoded rates.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 arch/arm/mach-mmp/common.h |  2 +-
 arch/arm/mach-mmp/mmp2.c   |  2 +-
 arch/arm/mach-mmp/pxa168.c |  2 +-
 arch/arm/mach-mmp/time.c   | 29 +++++++++++++++++------------
 4 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-mmp/common.h b/arch/arm/mach-mmp/common.h
index 7e284d9c429f..5ac2851ef5d3 100644
--- a/arch/arm/mach-mmp/common.h
+++ b/arch/arm/mach-mmp/common.h
@@ -2,7 +2,7 @@
 #include <linux/reboot.h>
 #define ARRAY_AND_SIZE(x)	(x), ARRAY_SIZE(x)
 
-extern void timer_init(int irq);
+extern void timer_init(int irq, unsigned long rate);
 
 extern void __init mmp_map_io(void);
 extern void mmp_restart(enum reboot_mode, const char *);
diff --git a/arch/arm/mach-mmp/mmp2.c b/arch/arm/mach-mmp/mmp2.c
index afba5460cdaf..fb3e7e32c882 100644
--- a/arch/arm/mach-mmp/mmp2.c
+++ b/arch/arm/mach-mmp/mmp2.c
@@ -134,7 +134,7 @@ void __init mmp2_timer_init(void)
 	clk_rst = APBC_APBCLK | APBC_FNCLK | APBC_FNCLKSEL(1);
 	__raw_writel(clk_rst, APBC_TIMERS);
 
-	timer_init(IRQ_MMP2_TIMER1);
+	timer_init(IRQ_MMP2_TIMER1, 6500000);
 }
 
 /* on-chip devices */
diff --git a/arch/arm/mach-mmp/pxa168.c b/arch/arm/mach-mmp/pxa168.c
index 0f5f16fb8c66..77a358165a56 100644
--- a/arch/arm/mach-mmp/pxa168.c
+++ b/arch/arm/mach-mmp/pxa168.c
@@ -79,7 +79,7 @@ void __init pxa168_timer_init(void)
 	/* 3.25MHz, bus/functional clock enabled, release reset */
 	__raw_writel(TIMER_CLK_RST, APBC_TIMERS);
 
-	timer_init(IRQ_PXA168_TIMER1);
+	timer_init(IRQ_PXA168_TIMER1, 6500000);
 }
 
 void pxa168_clear_keypad_wakeup(void)
diff --git a/arch/arm/mach-mmp/time.c b/arch/arm/mach-mmp/time.c
index 96ad1db0b04b..ed754a8eef79 100644
--- a/arch/arm/mach-mmp/time.c
+++ b/arch/arm/mach-mmp/time.c
@@ -22,6 +22,7 @@
 #include <linux/kernel.h>
 #include <linux/interrupt.h>
 #include <linux/clockchips.h>
+#include <linux/clk.h>
 
 #include <linux/io.h>
 #include <linux/irq.h>
@@ -38,12 +39,6 @@
 #include "cputype.h"
 #include "clock.h"
 
-#ifdef CONFIG_CPU_MMP2
-#define MMP_CLOCK_FREQ		6500000
-#else
-#define MMP_CLOCK_FREQ		3250000
-#endif
-
 #define TIMERS_VIRT_BASE	TIMERS1_VIRT_BASE
 
 #define MAX_DELTA		(0xfffffffe)
@@ -189,19 +184,18 @@ static struct irqaction timer_irq = {
 	.dev_id		= &ckevt,
 };
 
-void __init timer_init(int irq)
+void __init timer_init(int irq, unsigned long rate)
 {
 	timer_config();
 
-	sched_clock_register(mmp_read_sched_clock, 32, MMP_CLOCK_FREQ);
+	sched_clock_register(mmp_read_sched_clock, 32, rate);
 
 	ckevt.cpumask = cpumask_of(0);
 
 	setup_irq(irq, &timer_irq);
 
-	clocksource_register_hz(&cksrc, MMP_CLOCK_FREQ);
-	clockevents_config_and_register(&ckevt, MMP_CLOCK_FREQ,
-					MIN_DELTA, MAX_DELTA);
+	clocksource_register_hz(&cksrc, rate);
+	clockevents_config_and_register(&ckevt, rate, MIN_DELTA, MAX_DELTA);
 }
 
 #ifdef CONFIG_OF
@@ -213,6 +207,7 @@ static const struct of_device_id mmp_timer_dt_ids[] = {
 void __init mmp_dt_init_timer(void)
 {
 	struct device_node *np;
+	struct clk *clk;
 	int irq, ret;
 
 	np = of_find_matching_node(NULL, mmp_timer_dt_ids);
@@ -221,6 +216,16 @@ void __init mmp_dt_init_timer(void)
 		goto out;
 	}
 
+	clk = of_clk_get(np, 0);
+	if (IS_ERR(clk)) {
+		ret = PTR_ERR(clk);
+		goto out;
+	}
+
+	ret = clk_prepare_enable(clk);
+	if (ret)
+		goto out;
+
 	irq = irq_of_parse_and_map(np, 0);
 	if (!irq) {
 		ret = -EINVAL;
@@ -231,7 +236,7 @@ void __init mmp_dt_init_timer(void)
 		ret = -ENOMEM;
 		goto out;
 	}
-	timer_init(irq);
+	timer_init(irq, clk_get_rate(clk) / 2);
 	return;
 out:
 	pr_err("Failed to get timer from device tree with error:%d\n", ret);
-- 
2.17.1

Re: [PATCH 1/4] dt-bindings: mrvl,mmp-timer: add clock

[Rob Herring]

On Mon, Sep 10, 2018 at 6:30 AM Lubomir Rintel <lkundrak@v3.sk> wrote:
>
> The timer needs the timer clock to be enabled, otherwise it stops
> ticking.
>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---
>  Documentation/devicetree/bindings/timer/mrvl,mmp-timer.txt | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/timer/mrvl,mmp-timer.txt b/Documentation/devicetree/bindings/timer/mrvl,mmp-timer.txt
> index 9a6e251462e7..a2ede0bd12ca 100644
> --- a/Documentation/devicetree/bindings/timer/mrvl,mmp-timer.txt
> +++ b/Documentation/devicetree/bindings/timer/mrvl,mmp-timer.txt
> @@ -4,10 +4,12 @@ Required properties:
>  - compatible : Should be "mrvl,mmp-timer".
>  - reg : Address and length of the register set of timer controller.
>  - interrupts : Should be the interrupt number.
> +- clocks : Should contain a single entry describing the clock input.

So now pxa910 and pxa168 dts files have errors because they are
missing a required clock. Plus somehow other MMP2 platforms either
worked without a clock or have been broken.

You can't add new required properties to a binding. That breaks
backward compatibility.

Rob
>
>  Example:
>         timer0: timer@d4014000 {
>                 compatible = "mrvl,mmp-timer";
>                 reg = <0xd4014000 0x100>;
>                 interrupts = <13>;
> +               clocks = <&coreclk 2>;
>         };
> --
> 2.17.1
>

Re: [PATCH 1/4] dt-bindings: mrvl,mmp-timer: add clock

[Lubomir Rintel]

On Mon, 2018-09-10 at 10:21 -0500, Rob Herring wrote:
> On Mon, Sep 10, 2018 at 6:30 AM Lubomir Rintel <lkundrak@v3.sk>
> wrote:
> > 
> > The timer needs the timer clock to be enabled, otherwise it stops
> > ticking.
> > 
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > ---
> >  Documentation/devicetree/bindings/timer/mrvl,mmp-timer.txt | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/timer/mrvl,mmp-
> > timer.txt b/Documentation/devicetree/bindings/timer/mrvl,mmp-
> > timer.txt
> > index 9a6e251462e7..a2ede0bd12ca 100644
> > --- a/Documentation/devicetree/bindings/timer/mrvl,mmp-timer.txt
> > +++ b/Documentation/devicetree/bindings/timer/mrvl,mmp-timer.txt
> > @@ -4,10 +4,12 @@ Required properties:
> >  - compatible : Should be "mrvl,mmp-timer".
> >  - reg : Address and length of the register set of timer
> > controller.
> >  - interrupts : Should be the interrupt number.
> > +- clocks : Should contain a single entry describing the clock
> > input.
> 
> So now pxa910 and pxa168 dts files have errors because they are
> missing a required clock.

Ah, right. Sorry. Will make it optional then.

A brief look at 16x datasheet suggests that the "fast clock" source
circuitry needs to be enabled in the PMU, whereas the internal "32.768"
might not be. Perhaps the timer clk is always enabled in PMU, or the
slow clock is used on those platforms? No idea.

> Plus somehow other MMP2 platforms either
> worked without a clock or have been broken.

I don't have a MMP2 datasheet, but if things work the same as on a 16x,
then mach-mmp/time.c indeed chooses the fast timer.

The DT MMP2 platform certainly is certainly broken*. Not sure about the
BSP-based platforms.

* in other ways too: even when the missing clock is worked around with
  "clk_ignore_unused" the wrong rate gets chosen and the timer runs too
  slow. (Fix posted recently.)

> You can't add new required properties to a binding. That breaks
> backward compatibility.

Should I make an effort to make the driver look up and enable the clock
on MMP2 even if it doesn't find the clock property in DT?

It won't work otherwise, which is why I thought I can't break it
further (I didn't notice that the same binding and driver are used on
PXA168 and PXA910).

> Rob

Thank you,
Lubo

> > 
> >  Example:
> >         timer0: timer@d4014000 {
> >                 compatible = "mrvl,mmp-timer";
> >                 reg = <0xd4014000 0x100>;
> >                 interrupts = <13>;
> > +               clocks = <&coreclk 2>;
> >         };
> > --
> > 2.17.1
> > 

Re: [PATCH 4/4] ARM: mmp/mmp2: dt: enable the clock

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1155 bytes --]

On Tue 2018-09-18 22:54:55, Lubomir Rintel wrote:
> The device-tree booted MMP2 needs to enable the timer clock, otherwise
> the it would stop ticking when the boot finishes.

"the it" -> "it"

> It can also use the clock rate from the clk, the non-DT boards need to
> keep using the hardcoded rates.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

Acked-by: Pavel Machek <pavel@ucw.cz>

> +	clk = of_clk_get(np, 0);
> +	if (!IS_ERR(clk)) {
> +		ret = clk_prepare_enable(clk);
> +		if (ret)
> +			goto out;
> +		rate = clk_get_rate(clk) / 2;
> +	} else if (cpu_is_pj4 ()) {

extra space.

> +		rate = 6500000;
> +	} else {
> +		rate = 3250000;
> +	}
> +
>  	irq = irq_of_parse_and_map(np, 0);
>  	if (!irq) {
>  		ret = -EINVAL;
> @@ -231,7 +239,7 @@ void __init mmp_dt_init_timer(void)
>  		ret = -ENOMEM;
>  		goto out;
>  	}
> -	timer_init(irq);
> +	timer_init(irq, rate);
>  	return;
>  out:
>  	pr_err("Failed to get timer from device tree with error:%d\n", ret);

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH 4/4] ARM: mmp/mmp2: dt: enable the clock

[Lubomir Rintel]

The device-tree booted MMP2 needs to enable the timer clock, otherwise
the it would stop ticking when the boot finishes.

It can also use the clock rate from the clk, the non-DT boards need to
keep using the hardcoded rates.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

---
Changes since v1:
- Made the clock optional, to keep compatibility with DTs without it

 arch/arm/mach-mmp/common.h |  2 +-
 arch/arm/mach-mmp/mmp2.c   |  2 +-
 arch/arm/mach-mmp/pxa168.c |  2 +-
 arch/arm/mach-mmp/time.c   | 32 ++++++++++++++++++++------------
 4 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mach-mmp/common.h b/arch/arm/mach-mmp/common.h
index 7e284d9c429f..5ac2851ef5d3 100644
--- a/arch/arm/mach-mmp/common.h
+++ b/arch/arm/mach-mmp/common.h
@@ -2,7 +2,7 @@
 #include <linux/reboot.h>
 #define ARRAY_AND_SIZE(x)	(x), ARRAY_SIZE(x)
 
-extern void timer_init(int irq);
+extern void timer_init(int irq, unsigned long rate);
 
 extern void __init mmp_map_io(void);
 extern void mmp_restart(enum reboot_mode, const char *);
diff --git a/arch/arm/mach-mmp/mmp2.c b/arch/arm/mach-mmp/mmp2.c
index afba5460cdaf..fb3e7e32c882 100644
--- a/arch/arm/mach-mmp/mmp2.c
+++ b/arch/arm/mach-mmp/mmp2.c
@@ -134,7 +134,7 @@ void __init mmp2_timer_init(void)
 	clk_rst = APBC_APBCLK | APBC_FNCLK | APBC_FNCLKSEL(1);
 	__raw_writel(clk_rst, APBC_TIMERS);
 
-	timer_init(IRQ_MMP2_TIMER1);
+	timer_init(IRQ_MMP2_TIMER1, 6500000);
 }
 
 /* on-chip devices */
diff --git a/arch/arm/mach-mmp/pxa168.c b/arch/arm/mach-mmp/pxa168.c
index 0f5f16fb8c66..77a358165a56 100644
--- a/arch/arm/mach-mmp/pxa168.c
+++ b/arch/arm/mach-mmp/pxa168.c
@@ -79,7 +79,7 @@ void __init pxa168_timer_init(void)
 	/* 3.25MHz, bus/functional clock enabled, release reset */
 	__raw_writel(TIMER_CLK_RST, APBC_TIMERS);
 
-	timer_init(IRQ_PXA168_TIMER1);
+	timer_init(IRQ_PXA168_TIMER1, 6500000);
 }
 
 void pxa168_clear_keypad_wakeup(void)
diff --git a/arch/arm/mach-mmp/time.c b/arch/arm/mach-mmp/time.c
index 96ad1db0b04b..351f4714bba6 100644
--- a/arch/arm/mach-mmp/time.c
+++ b/arch/arm/mach-mmp/time.c
@@ -22,6 +22,7 @@
 #include <linux/kernel.h>
 #include <linux/interrupt.h>
 #include <linux/clockchips.h>
+#include <linux/clk.h>
 
 #include <linux/io.h>
 #include <linux/irq.h>
@@ -38,12 +39,6 @@
 #include "cputype.h"
 #include "clock.h"
 
-#ifdef CONFIG_CPU_MMP2
-#define MMP_CLOCK_FREQ		6500000
-#else
-#define MMP_CLOCK_FREQ		3250000
-#endif
-
 #define TIMERS_VIRT_BASE	TIMERS1_VIRT_BASE
 
 #define MAX_DELTA		(0xfffffffe)
@@ -189,19 +184,18 @@ static struct irqaction timer_irq = {
 	.dev_id		= &ckevt,
 };
 
-void __init timer_init(int irq)
+void __init timer_init(int irq, unsigned long rate)
 {
 	timer_config();
 
-	sched_clock_register(mmp_read_sched_clock, 32, MMP_CLOCK_FREQ);
+	sched_clock_register(mmp_read_sched_clock, 32, rate);
 
 	ckevt.cpumask = cpumask_of(0);
 
 	setup_irq(irq, &timer_irq);
 
-	clocksource_register_hz(&cksrc, MMP_CLOCK_FREQ);
-	clockevents_config_and_register(&ckevt, MMP_CLOCK_FREQ,
-					MIN_DELTA, MAX_DELTA);
+	clocksource_register_hz(&cksrc, rate);
+	clockevents_config_and_register(&ckevt, rate, MIN_DELTA, MAX_DELTA);
 }
 
 #ifdef CONFIG_OF
@@ -213,7 +207,9 @@ static const struct of_device_id mmp_timer_dt_ids[] = {
 void __init mmp_dt_init_timer(void)
 {
 	struct device_node *np;
+	struct clk *clk;
 	int irq, ret;
+	unsigned long rate;
 
 	np = of_find_matching_node(NULL, mmp_timer_dt_ids);
 	if (!np) {
@@ -221,6 +217,18 @@ void __init mmp_dt_init_timer(void)
 		goto out;
 	}
 
+	clk = of_clk_get(np, 0);
+	if (!IS_ERR(clk)) {
+		ret = clk_prepare_enable(clk);
+		if (ret)
+			goto out;
+		rate = clk_get_rate(clk) / 2;
+	} else if (cpu_is_pj4 ()) {
+		rate = 6500000;
+	} else {
+		rate = 3250000;
+	}
+
 	irq = irq_of_parse_and_map(np, 0);
 	if (!irq) {
 		ret = -EINVAL;
@@ -231,7 +239,7 @@ void __init mmp_dt_init_timer(void)
 		ret = -ENOMEM;
 		goto out;
 	}
-	timer_init(irq);
+	timer_init(irq, rate);
 	return;
 out:
 	pr_err("Failed to get timer from device tree with error:%d\n", ret);
-- 
2.17.1