[PATCH] clocksource: arm_arch_timer: Support reading clock rate from a driver

[PATCH] clocksource: arm_arch_timer: Support reading clock rate from a driver

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

On some devices using arch code for reading clock rate doesn't work. So
far the only option was to specify clock-frequency in a DT. This works
only if a clock frequency doesn't have to be calculated on runtime.

On BCM53573 SoC (with Cortex-A7) there is ILP clock that needs its own
driver. With this change we can query such clocks by using a standard:
clocks = <&foo>;

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/clocksource/arm_arch_timer.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 5effd30..5ed98a2 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -14,6 +14,7 @@
 #include <linux/smp.h>
 #include <linux/cpu.h>
 #include <linux/cpu_pm.h>
+#include <linux/clk.h>
 #include <linux/clockchips.h>
 #include <linux/clocksource.h>
 #include <linux/interrupt.h>
@@ -405,6 +406,16 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
 			arch_timer_rate = arch_timer_get_cntfrq();
 	}
 
+	/* Get clk rate through clk driver if present */
+	if (!arch_timer_rate) {
+		struct clk *clk = of_clk_get(np, 0);
+
+		if (!IS_ERR(clk)) {
+			if (!clk_prepare_enable(clk))
+				arch_timer_rate = clk_get_rate(clk);
+		}
+	}
+
 	/* Check the timer frequency. */
 	if (arch_timer_rate == 0)
 		pr_warn("Architected timer frequency not available\n");
-- 
1.8.4.5

[PATCH] clocksource: arm_arch_timer: Support reading clock rate from a driver

[Rafał Miłecki]

From: Rafa? Mi?ecki <rafal@milecki.pl>

On some devices using arch code for reading clock rate doesn't work. So
far the only option was to specify clock-frequency in a DT. This works
only if a clock frequency doesn't have to be calculated on runtime.

On BCM53573 SoC (with Cortex-A7) there is ILP clock that needs its own
driver. With this change we can query such clocks by using a standard:
clocks = <&foo>;

Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
---
 drivers/clocksource/arm_arch_timer.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 5effd30..5ed98a2 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -14,6 +14,7 @@
 #include <linux/smp.h>
 #include <linux/cpu.h>
 #include <linux/cpu_pm.h>
+#include <linux/clk.h>
 #include <linux/clockchips.h>
 #include <linux/clocksource.h>
 #include <linux/interrupt.h>
@@ -405,6 +406,16 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
 			arch_timer_rate = arch_timer_get_cntfrq();
 	}
 
+	/* Get clk rate through clk driver if present */
+	if (!arch_timer_rate) {
+		struct clk *clk = of_clk_get(np, 0);
+
+		if (!IS_ERR(clk)) {
+			if (!clk_prepare_enable(clk))
+				arch_timer_rate = clk_get_rate(clk);
+		}
+	}
+
 	/* Check the timer frequency. */
 	if (arch_timer_rate == 0)
 		pr_warn("Architected timer frequency not available\n");
-- 
1.8.4.5

Re: [PATCH] clocksource: arm_arch_timer: Support reading clock rate from a driver

[Mark Rutland]

[adding Marc to Cc]

On Fri, Jul 29, 2016 at 11:23:11AM +0200, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> On some devices using arch code for reading clock rate doesn't work. So
> far the only option was to specify clock-frequency in a DT. This works
> only if a clock frequency doesn't have to be calculated on runtime.
> 
> On BCM53573 SoC (with Cortex-A7) there is ILP clock that needs its own
> driver. With this change we can query such clocks by using a standard:
> clocks = <&foo>;

The clock for the architected timer(s) are supposed to be configured
before entering Linux. The clock-frequency property is at best a dodgy
workaround, and this is even worse.

Please fix your firmware to configure and enabled this clock before
entering Linux, and to program CNTFRQ appropriately.

As this path stands, NAK.

As a more general note, you *must* update device tree bidnings if adding
properties.

Thanks,
Mark.

> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  drivers/clocksource/arm_arch_timer.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 5effd30..5ed98a2 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -14,6 +14,7 @@
>  #include <linux/smp.h>
>  #include <linux/cpu.h>
>  #include <linux/cpu_pm.h>
> +#include <linux/clk.h>
>  #include <linux/clockchips.h>
>  #include <linux/clocksource.h>
>  #include <linux/interrupt.h>
> @@ -405,6 +406,16 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
>  			arch_timer_rate = arch_timer_get_cntfrq();
>  	}
>  
> +	/* Get clk rate through clk driver if present */
> +	if (!arch_timer_rate) {
> +		struct clk *clk = of_clk_get(np, 0);
> +
> +		if (!IS_ERR(clk)) {
> +			if (!clk_prepare_enable(clk))
> +				arch_timer_rate = clk_get_rate(clk);
> +		}
> +	}
> +
>  	/* Check the timer frequency. */
>  	if (arch_timer_rate == 0)
>  		pr_warn("Architected timer frequency not available\n");
> -- 
> 1.8.4.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] clocksource: arm_arch_timer: Support reading clock rate from a driver

[Mark Rutland]

[adding Marc to Cc]

On Fri, Jul 29, 2016 at 11:23:11AM +0200, Rafa? Mi?ecki wrote:
> From: Rafa? Mi?ecki <rafal@milecki.pl>
> 
> On some devices using arch code for reading clock rate doesn't work. So
> far the only option was to specify clock-frequency in a DT. This works
> only if a clock frequency doesn't have to be calculated on runtime.
> 
> On BCM53573 SoC (with Cortex-A7) there is ILP clock that needs its own
> driver. With this change we can query such clocks by using a standard:
> clocks = <&foo>;

The clock for the architected timer(s) are supposed to be configured
before entering Linux. The clock-frequency property is at best a dodgy
workaround, and this is even worse.

Please fix your firmware to configure and enabled this clock before
entering Linux, and to program CNTFRQ appropriately.

As this path stands, NAK.

As a more general note, you *must* update device tree bidnings if adding
properties.

Thanks,
Mark.

> 
> Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
> ---
>  drivers/clocksource/arm_arch_timer.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 5effd30..5ed98a2 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -14,6 +14,7 @@
>  #include <linux/smp.h>
>  #include <linux/cpu.h>
>  #include <linux/cpu_pm.h>
> +#include <linux/clk.h>
>  #include <linux/clockchips.h>
>  #include <linux/clocksource.h>
>  #include <linux/interrupt.h>
> @@ -405,6 +406,16 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
>  			arch_timer_rate = arch_timer_get_cntfrq();
>  	}
>  
> +	/* Get clk rate through clk driver if present */
> +	if (!arch_timer_rate) {
> +		struct clk *clk = of_clk_get(np, 0);
> +
> +		if (!IS_ERR(clk)) {
> +			if (!clk_prepare_enable(clk))
> +				arch_timer_rate = clk_get_rate(clk);
> +		}
> +	}
> +
>  	/* Check the timer frequency. */
>  	if (arch_timer_rate == 0)
>  		pr_warn("Architected timer frequency not available\n");
> -- 
> 1.8.4.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] clocksource: arm_arch_timer: Support reading clock rate from a driver

[Marc Zyngier]

On 29/07/16 11:18, Mark Rutland wrote:
> [adding Marc to Cc]
> 
> On Fri, Jul 29, 2016 at 11:23:11AM +0200, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> On some devices using arch code for reading clock rate doesn't work. So
>> far the only option was to specify clock-frequency in a DT. This works
>> only if a clock frequency doesn't have to be calculated on runtime.
>>
>> On BCM53573 SoC (with Cortex-A7) there is ILP clock that needs its own
>> driver. With this change we can query such clocks by using a standard:
>> clocks = <&foo>;
> 
> The clock for the architected timer(s) are supposed to be configured
> before entering Linux. The clock-frequency property is at best a dodgy
> workaround, and this is even worse.
> 
> Please fix your firmware to configure and enabled this clock before
> entering Linux, and to program CNTFRQ appropriately.

I'll add that failure to do so breaks other subsystems which do rely on
CNTFRQ to be correctly programmed *on each CPU* (like KVM and Xen).

Please follow the requirements of the architecture and don't just paper
over such a bug.

> As this path stands, NAK.

Seconded.

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH] clocksource: arm_arch_timer: Support reading clock rate from a driver

[Marc Zyngier]

On 29/07/16 11:18, Mark Rutland wrote:
> [adding Marc to Cc]
> 
> On Fri, Jul 29, 2016 at 11:23:11AM +0200, Rafa? Mi?ecki wrote:
>> From: Rafa? Mi?ecki <rafal@milecki.pl>
>>
>> On some devices using arch code for reading clock rate doesn't work. So
>> far the only option was to specify clock-frequency in a DT. This works
>> only if a clock frequency doesn't have to be calculated on runtime.
>>
>> On BCM53573 SoC (with Cortex-A7) there is ILP clock that needs its own
>> driver. With this change we can query such clocks by using a standard:
>> clocks = <&foo>;
> 
> The clock for the architected timer(s) are supposed to be configured
> before entering Linux. The clock-frequency property is at best a dodgy
> workaround, and this is even worse.
> 
> Please fix your firmware to configure and enabled this clock before
> entering Linux, and to program CNTFRQ appropriately.

I'll add that failure to do so breaks other subsystems which do rely on
CNTFRQ to be correctly programmed *on each CPU* (like KVM and Xen).

Please follow the requirements of the architecture and don't just paper
over such a bug.

> As this path stands, NAK.

Seconded.

	M.
-- 
Jazz is not dead. It just smells funny...