| Message ID | 552BFED9.3020105@adapteva.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 13, 2015 at 06:37:29PM +0100, Ola Jeppsson wrote: > Some Cortex A9 CPU:s (e.g. zynq) have the tick tied to the CPU > frequency. On those CPU:s we cannot use the global-timer as a reliable > clocksource with CPU frequency scaling enabled since this is not > currently taken into account by the driver. The global timer is _always_ tied to the CPU frequency -- it's fed by PERIPHCLK, which must have a period which is a multiple of CLK. > Add a "tied-to-cpu-freq" boolean to the global-timer dt node indicate > this condition. Is this indicating the truism above, or that the kernel is performing frequency scaling? The latter is an OS property rather than a HW property, and thus doesn't belong in the DT. The SMP TWD driver tries to account for frequency scaling. Assuming that works, the same should be possible here. Otherwise, the global timer is simply unusable with any frequency scaling. In either case the property seems pointless; all you care about is whether the platform actually performs CPU frequency scaling. Mark. > > When the global-timer register function sees this property return > immediately and don't register the clocksource. > > Signed-off-by: Ola Jeppsson <ola@adapteva.com> > --- > Documentation/devicetree/bindings/arm/global_timer.txt | 4 ++++ > drivers/clocksource/arm_global_timer.c | 7 +++++++ > 2 files changed, 11 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/global_timer.txt b/Documentation/devicetree/bindings/arm/global_timer.txt > index bdae3a818793..465e02c17b5b 100644 > --- a/Documentation/devicetree/bindings/arm/global_timer.txt > +++ b/Documentation/devicetree/bindings/arm/global_timer.txt > @@ -17,6 +17,10 @@ > > - clocks : Should be phandle to a clock. > > +** Timer node optional properties: > + > +- tied-to-cpu-freq : indicates that the timer scales with the CPU frequency. > + > Example: > > timer@2c000600 { > diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c > index e6833771a716..8913ebda3f09 100644 > --- a/drivers/clocksource/arm_global_timer.c > +++ b/drivers/clocksource/arm_global_timer.c > @@ -268,6 +268,13 @@ static void __init global_timer_of_register(struct device_node *np) > return; > } > > +#ifdef CONFIG_CPU_FREQ > + if (of_property_read_bool(np, "tied-to-cpu-freq")) { > + pr_warn("global-timer: tied to cpu frequency, not supported with scaling\n"); > + return; > + } > +#endif > + > gt_clk = of_clk_get(np, 0); > if (!IS_ERR(gt_clk)) { > err = clk_prepare_enable(gt_clk); > -- > 2.3.3 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
+Adding Pete and Maxime Hi Ola, Thankyou for sending the patch, I like the Idea, but I have some specific concerns which would break existing SOCs. On 13/04/15 18:37, Ola Jeppsson wrote: > Some Cortex A9 CPU:s (e.g. zynq) have the tick tied to the CPU > frequency. On those CPU:s we cannot use the global-timer as a reliable > clocksource with CPU frequency scaling enabled since this is not > currently taken into account by the driver. > > Add a "tied-to-cpu-freq" boolean to the global-timer dt node indicate > this condition. > > When the global-timer register function sees this property return > immediately and don't register the clocksource. > > Signed-off-by: Ola Jeppsson <ola@adapteva.com> > --- > Documentation/devicetree/bindings/arm/global_timer.txt | 4 ++++ > drivers/clocksource/arm_global_timer.c | 7 +++++++ > 2 files changed, 11 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/global_timer.txt b/Documentation/devicetree/bindings/arm/global_timer.txt > index bdae3a818793..465e02c17b5b 100644 > --- a/Documentation/devicetree/bindings/arm/global_timer.txt > +++ b/Documentation/devicetree/bindings/arm/global_timer.txt > @@ -17,6 +17,10 @@ > > - clocks : Should be phandle to a clock. > > +** Timer node optional properties: > + > +- tied-to-cpu-freq : indicates that the timer scales with the CPU frequency. > + > Example: > > timer@2c000600 { > diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c > index e6833771a716..8913ebda3f09 100644 > --- a/drivers/clocksource/arm_global_timer.c > +++ b/drivers/clocksource/arm_global_timer.c > @@ -268,6 +268,13 @@ static void __init global_timer_of_register(struct device_node *np) > return; > } > > +#ifdef CONFIG_CPU_FREQ > + if (of_property_read_bool(np, "tied-to-cpu-freq")) { > + pr_warn("global-timer: tied to cpu frequency, not supported with scaling\n"); > + return; > + } > +#endif > + This patch would not let the SOC like STiH415/416 or zynq with "tied-to-cpu-freq" property to boot with multi_v7_defconfig. Which is not correct thing to do, as STi SOC's do not use cpufreq driver however the tick is tied to this clocksource. --srini > gt_clk = of_clk_get(np, 0); > if (!IS_ERR(gt_clk)) { > err = clk_prepare_enable(gt_clk); >
Hi Srini, Ola, On 04/14/2015 09:41 AM, Srinivas Kandagatla wrote: > +Adding Pete and Maxime > > Hi Ola, > Thankyou for sending the patch, > > I like the Idea, but I have some specific concerns which would break > existing SOCs. I like the idea too. > > > On 13/04/15 18:37, Ola Jeppsson wrote: >> Some Cortex A9 CPU:s (e.g. zynq) have the tick tied to the CPU >> frequency. On those CPU:s we cannot use the global-timer as a reliable >> clocksource with CPU frequency scaling enabled since this is not >> currently taken into account by the driver. >> >> Add a "tied-to-cpu-freq" boolean to the global-timer dt node indicate >> this condition. >> >> When the global-timer register function sees this property return >> immediately and don't register the clocksource. >> >> Signed-off-by: Ola Jeppsson <ola@adapteva.com> >> --- >> Documentation/devicetree/bindings/arm/global_timer.txt | 4 ++++ >> drivers/clocksource/arm_global_timer.c | 7 +++++++ >> 2 files changed, 11 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/arm/global_timer.txt >> b/Documentation/devicetree/bindings/arm/global_timer.txt >> index bdae3a818793..465e02c17b5b 100644 >> --- a/Documentation/devicetree/bindings/arm/global_timer.txt >> +++ b/Documentation/devicetree/bindings/arm/global_timer.txt >> @@ -17,6 +17,10 @@ >> >> - clocks : Should be phandle to a clock. >> >> +** Timer node optional properties: >> + >> +- tied-to-cpu-freq : indicates that the timer scales with the CPU >> frequency. >> + >> Example: >> >> timer@2c000600 { >> diff --git a/drivers/clocksource/arm_global_timer.c >> b/drivers/clocksource/arm_global_timer.c >> index e6833771a716..8913ebda3f09 100644 >> --- a/drivers/clocksource/arm_global_timer.c >> +++ b/drivers/clocksource/arm_global_timer.c >> @@ -268,6 +268,13 @@ static void __init >> global_timer_of_register(struct device_node *np) >> return; >> } >> >> +#ifdef CONFIG_CPU_FREQ >> + if (of_property_read_bool(np, "tied-to-cpu-freq")) { >> + pr_warn("global-timer: tied to cpu frequency, not supported >> with scaling\n"); >> + return; >> + } >> +#endif >> + > > This patch would not let the SOC like STiH415/416 or zynq with > "tied-to-cpu-freq" property to boot with multi_v7_defconfig. Which is > not correct thing to do, as STi SOC's do not use cpufreq driver > however the tick is tied to this clocksource. Yes, you are right, but I don't see any cleaner way to do this. On STi, we have another timer we can use as a clocksource when doing CPU Freq, the ST LPC timer. It is not upstreamed yet, but we will try to have it for next release. I propose we set the "tied-to-cpu-freq" in GT node of STi family as soon as we enable the LPC timer one. Doing that, the STi boot won't break in multi_v7 config. Kind regards, Maxime > > > > --srini > >> gt_clk = of_clk_get(np, 0); >> if (!IS_ERR(gt_clk)) { >> err = clk_prepare_enable(gt_clk); >>
On Tue, 2015-04-14 at 08:41AM +0100, Srinivas Kandagatla wrote: > +Adding Pete and Maxime > > Hi Ola, > Thankyou for sending the patch, > > I like the Idea, but I have some specific concerns which would break > existing SOCs. > > On 13/04/15 18:37, Ola Jeppsson wrote: > >Some Cortex A9 CPU:s (e.g. zynq) have the tick tied to the CPU > >frequency. On those CPU:s we cannot use the global-timer as a reliable > >clocksource with CPU frequency scaling enabled since this is not > >currently taken into account by the driver. > > > >Add a "tied-to-cpu-freq" boolean to the global-timer dt node indicate > >this condition. > > > >When the global-timer register function sees this property return > >immediately and don't register the clocksource. > > > >Signed-off-by: Ola Jeppsson <ola@adapteva.com> > >--- > > Documentation/devicetree/bindings/arm/global_timer.txt | 4 ++++ > > drivers/clocksource/arm_global_timer.c | 7 +++++++ > > 2 files changed, 11 insertions(+) > > > >diff --git a/Documentation/devicetree/bindings/arm/global_timer.txt b/Documentation/devicetree/bindings/arm/global_timer.txt > >index bdae3a818793..465e02c17b5b 100644 > >--- a/Documentation/devicetree/bindings/arm/global_timer.txt > >+++ b/Documentation/devicetree/bindings/arm/global_timer.txt > >@@ -17,6 +17,10 @@ > > > > - clocks : Should be phandle to a clock. > > > >+** Timer node optional properties: > >+ > >+- tied-to-cpu-freq : indicates that the timer scales with the CPU frequency. > >+ > > Example: > > > > timer@2c000600 { > >diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c > >index e6833771a716..8913ebda3f09 100644 > >--- a/drivers/clocksource/arm_global_timer.c > >+++ b/drivers/clocksource/arm_global_timer.c > >@@ -268,6 +268,13 @@ static void __init global_timer_of_register(struct device_node *np) > > return; > > } > > > >+#ifdef CONFIG_CPU_FREQ > >+ if (of_property_read_bool(np, "tied-to-cpu-freq")) { > >+ pr_warn("global-timer: tied to cpu frequency, not supported with scaling\n"); > >+ return; > >+ } > >+#endif > >+ > > This patch would not let the SOC like STiH415/416 or zynq with > "tied-to-cpu-freq" property to boot with multi_v7_defconfig. Which is not > correct thing to do, as STi SOC's do not use cpufreq driver however the tick > is tied to this clocksource. For Zynq, it should be OK, since we have the cadence_ttc as alternative clocksource. Though, I have to admit not having tested this patch. Sören
Hi Srini, Ola, Maxime, On Tue, 14 Apr 2015, Maxime Coquelin wrote: > On 04/14/2015 09:41 AM, Srinivas Kandagatla wrote: > >+Adding Pete and Maxime Thanks Srini, as I wouldn't have seen this otherwise. <snip> > >>+ > > > >This patch would not let the SOC like STiH415/416 or zynq with > >"tied-to-cpu-freq" property to boot with multi_v7_defconfig. Which > >is not correct thing to do, as STi SOC's do not use cpufreq driver > >however the tick is tied to this clocksource. > Yes, you are right, but I don't see any cleaner way to do this. > > On STi, we have another timer we can use as a clocksource when doing > CPU Freq, the ST LPC timer. > It is not upstreamed yet, but we will try to have it for next release. I just sent a series to add the ST LPC timer to LKML, see here https://lkml.org/lkml/2015/4/17/181. I've tested this on stih410 and it works ok. > I propose we set the "tied-to-cpu-freq" in GT node of STi family as > soon as we enable the LPC timer one. > Doing that, the STi boot won't break in multi_v7 config. However this lpc timer IP does not exist on stih415/6 SoCs, and I can't (so far at least) see any other timer which we could use. So basically we would still have a breakage on stih415/6 STi platforms with this proposed change. Upstreaming st_lpc.c timer is still worthwhile though as it paves the way for adding cpufreq support upstream for stih407 family. regards, Peter.
diff --git a/Documentation/devicetree/bindings/arm/global_timer.txt b/Documentation/devicetree/bindings/arm/global_timer.txt index bdae3a818793..465e02c17b5b 100644 --- a/Documentation/devicetree/bindings/arm/global_timer.txt +++ b/Documentation/devicetree/bindings/arm/global_timer.txt @@ -17,6 +17,10 @@ - clocks : Should be phandle to a clock. +** Timer node optional properties: + +- tied-to-cpu-freq : indicates that the timer scales with the CPU frequency. + Example: timer@2c000600 { diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c index e6833771a716..8913ebda3f09 100644 --- a/drivers/clocksource/arm_global_timer.c +++ b/drivers/clocksource/arm_global_timer.c @@ -268,6 +268,13 @@ static void __init global_timer_of_register(struct device_node *np) return; } +#ifdef CONFIG_CPU_FREQ + if (of_property_read_bool(np, "tied-to-cpu-freq")) { + pr_warn("global-timer: tied to cpu frequency, not supported with scaling\n"); + return; + } +#endif + gt_clk = of_clk_get(np, 0); if (!IS_ERR(gt_clk)) { err = clk_prepare_enable(gt_clk);
Some Cortex A9 CPU:s (e.g. zynq) have the tick tied to the CPU frequency. On those CPU:s we cannot use the global-timer as a reliable clocksource with CPU frequency scaling enabled since this is not currently taken into account by the driver. Add a "tied-to-cpu-freq" boolean to the global-timer dt node indicate this condition. When the global-timer register function sees this property return immediately and don't register the clocksource. Signed-off-by: Ola Jeppsson <ola@adapteva.com> --- Documentation/devicetree/bindings/arm/global_timer.txt | 4 ++++ drivers/clocksource/arm_global_timer.c | 7 +++++++ 2 files changed, 11 insertions(+)