Re: Setting CPU clock frequency on early boot

[Rob Herring]

On Wed, Sep 6, 2017 at 11:22 AM, Alexey Brodkin
 wrote:
> Hi Rob,
>
> On Wed, 2017-09-06 at 10:25 -0500, Rob Herring wrote:
>> On Wed, Sep 6, 2017 at 8:51 AM, Alexey Brodkin
>>  wrote:
>> >
>> > Hi Vineet, Rob,
>> >
>> > On Tue, 2017-09-05 at 16:40 -0700, Vineet Gupta wrote:
>> > >
>> > > On 09/05/2017 03:04 PM, Rob Herring wrote:
>> > > >
>> > > >
>> > > > On Tue, Sep 5, 2017 at 10:37 AM, Alexey Brodkin
>> > > >  wrote:
>
> [snip]
>
>> > Yeah, that's an interesting question. We may indeed move more smarts to 
>> > the clock driver
>> > but:
>> >  1. We'll have duplicate code in different clock drivers. Even today that 
>> > kind of clock
>> > setup is applicable to AXS10x and HSDK platforms (and they use 
>> > different clock drivers).
>>
>> No, you could provide a common, shared function to call. Then each
>> platform can opt-in. If you can make something that applies to every
>> single platform now or in the future, then I'd put it in arch. If you
>> have plans to decouple the timer and cpu clocks, then sounds like you
>> can't.
>
> Right so we'll implement a function which is called by a platform if required.
> That way we escape copy-pasting while keeping enough flexibility for current
> and future platforms.
>
>> IMO, if it's not part of the defined CPU architecture, then don't put
>> it in arch/. That's how we end up with multiple copies of the same
>> thing done arbitrarily different ways because few people look across
>> architectures.
>
> So do you propose to have the function [that reads "clock-frequency" from say
> CPU node and passes its value to CPU's parent clock driver] in generic
> [i.e. architecture agnostic] code somewhere in "init/main.c"?

No, just like you said above and have the platform's clock driver
initialize frequencies.

I haven't seen anyone else want such a thing as generally the
bootloader should initialize things to something reasonable.

>> >  2. Print out of CPU frequency which is used during boot process for us is 
>> > important as well
>> > especially during bring-up of new HW.
>> >
>> >  3. If there's no dedicated "clock-frequency" parameter in CPU node we 
>> > won't
>> > change anything so that non-affected platforms will live as they used 
>> > to.
>> >
>> > That said IMHO proposed implementation is what we want to kep for now.
>> >
>> > >
>> > > Also note that this code is using a new / adhoc DT binding cpu-freq in 
>> > > cou node to
>> > > do the override - is that acceptable ?
>>
>> No, I meant to point that out.
>
> Sorry, but for me it's not clear what did you mean here.
> Care to elaborate a bit more?

I noticed the same thing and meant to highlight that in my first
reply. Use clock-frequency, don't invent something new.

>> > I think we'll switch to more common "clock-frequency" in the next respin.
>> > Indeed "cpu-freq" might be a bit misleading.
>>
>> Ideally, you'd use the clock binding eventually.
>
> Again I'm probably missing something :)

"clock-frequency" property and the clock bindings (i.e. clocks =
<>) are generally mutually exclusive at least within clock
consumer side. There's the assigned-clocks binding which provides the
ability to set both parent clocks and frequency.

> I meant we will have both clock phandle and "clock-frequency" at the same 
> time.
> Something like this:
> >8---
> cpus {
> #address-cells = <1>;
> #size-cells = <0>;
>
> cpu@0 {
> device_type = "cpu";
> compatible = "snps,archs38";
> reg = <0>;
> clocks = <_clk>;
> clock-frequency = <1>;  <-- That's where we 
> want to set desired value
> };
> ...
> }
>
> core_clk: core-clk@80 {
> compatible = "snps,axs10x-arc-pll-clock";
> reg = <0x80 0x10>, <0x100 0x10>;
> #clock-cells = <0>;
> clocks = <_clk>;
> };
> >8---
>
> Or alternatively we may move "clock-frequency" right to the clock's node and 
> have
> it like that:
> >8---
> core_clk: core-clk@80 {
> compatible = "snps,axs10x-arc-pll-clock";
> reg = <0x80 0x10>, <0x100 0x10>;
>
> #clock-cells = <0>;
> clocks = <_clk>;
> clock-frequency = <1>;  <-- That's where we want to 
> set desired value

I think this is how it should be done if you use clock-frequency prop.
This is inline with how fixed-clock binding is done.

Rob

___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: Setting CPU clock frequency on early boot

[Alexey Brodkin]

Hi Rob,

On Wed, 2017-09-06 at 10:25 -0500, Rob Herring wrote:
> On Wed, Sep 6, 2017 at 8:51 AM, Alexey Brodkin
>  wrote:
> > 
> > Hi Vineet, Rob,
> > 
> > On Tue, 2017-09-05 at 16:40 -0700, Vineet Gupta wrote:
> > > 
> > > On 09/05/2017 03:04 PM, Rob Herring wrote:
> > > > 
> > > > 
> > > > On Tue, Sep 5, 2017 at 10:37 AM, Alexey Brodkin
> > > >  wrote:

[snip]

> > Yeah, that's an interesting question. We may indeed move more smarts to the 
> > clock driver
> > but:
> >  1. We'll have duplicate code in different clock drivers. Even today that 
> > kind of clock
> > setup is applicable to AXS10x and HSDK platforms (and they use 
> > different clock drivers).
> 
> No, you could provide a common, shared function to call. Then each
> platform can opt-in. If you can make something that applies to every
> single platform now or in the future, then I'd put it in arch. If you
> have plans to decouple the timer and cpu clocks, then sounds like you
> can't.

Right so we'll implement a function which is called by a platform if required.
That way we escape copy-pasting while keeping enough flexibility for current
and future platforms.

> IMO, if it's not part of the defined CPU architecture, then don't put
> it in arch/. That's how we end up with multiple copies of the same
> thing done arbitrarily different ways because few people look across
> architectures.

So do you propose to have the function [that reads "clock-frequency" from say
CPU node and passes its value to CPU's parent clock driver] in generic
[i.e. architecture agnostic] code somewhere in "init/main.c"?

> > 
> >  2. Print out of CPU frequency which is used during boot process for us is 
> > important as well
> > especially during bring-up of new HW.
> > 
> >  3. If there's no dedicated "clock-frequency" parameter in CPU node we won't
> > change anything so that non-affected platforms will live as they used 
> > to.
> > 
> > That said IMHO proposed implementation is what we want to kep for now.
> > 
> > > 
> > > Also note that this code is using a new / adhoc DT binding cpu-freq in 
> > > cou node to
> > > do the override - is that acceptable ?
> 
> No, I meant to point that out.

Sorry, but for me it's not clear what did you mean here.
Care to elaborate a bit more?

> > I think we'll switch to more common "clock-frequency" in the next respin.
> > Indeed "cpu-freq" might be a bit misleading.
> 
> Ideally, you'd use the clock binding eventually.

Again I'm probably missing something :)

I meant we will have both clock phandle and "clock-frequency" at the same time.
Something like this:
>8---
cpus {
#address-cells = <1>;
#size-cells = <0>;

cpu@0 {
device_type = "cpu";
compatible = "snps,archs38";
reg = <0>;
clocks = <_clk>;
clock-frequency = <1>;  <-- That's where we 
want to set desired value
};
...
}

core_clk: core-clk@80 {
compatible = "snps,axs10x-arc-pll-clock";
reg = <0x80 0x10>, <0x100 0x10>;
#clock-cells = <0>;
clocks = <_clk>;
};
>8---

Or alternatively we may move "clock-frequency" right to the clock's node and 
have
it like that:
>8---
core_clk: core-clk@80 {
compatible = "snps,axs10x-arc-pll-clock";
reg = <0x80 0x10>, <0x100 0x10>;

#clock-cells = <0>;
clocks = <_clk>;
clock-frequency = <1>;  <-- That's where we want to set 
desired value

};
>8---

-Alexey
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: Setting CPU clock frequency on early boot

[Rob Herring]

On Wed, Sep 6, 2017 at 8:51 AM, Alexey Brodkin
 wrote:
> Hi Vineet, Rob,
>
> On Tue, 2017-09-05 at 16:40 -0700, Vineet Gupta wrote:
>> On 09/05/2017 03:04 PM, Rob Herring wrote:
>> >
>> > On Tue, Sep 5, 2017 at 10:37 AM, Alexey Brodkin
>> >  wrote:
>> > >
>> > > Hello,
>> > >
>> > > I'd like to get some feedback on our idea as well as check
>> > > if somebody faces similar situations and if so what would be the best
>> > > way to implement some generic solution that suits everyone.
>> > >
>> > > So that's our problem:
>> > > 1. On power-on hardware might start clocking CPU with either
>> > > too high frequency (such that CPU may get stuck at some point)
>> > > or too low frequency.
>> > >
>> > > That all sounds stupid but let me elaborate a bit here.
>> > > I'm talking about FPGA-based devboards firmware for which
>> > > (here I mean just image loaded in FPGA with CPU implementation
>> > > but not some software yet) might not be stable or be even 
>> > > experimental.
>> > >
>> > > For example we may deal with dual-core or quad-core designs.
>> > > Former might be OK running @100MHz and latter is only usable
>> > > @75MHz and lower. The simplest solution might be to use some safe
>> > > value before something like CPUfreq kicks in. But we don't yet have
>> > > CPUfreq for ARC (we do plan to get it working sometime soon)
>>
>> But even if we had cpufreq driver going - I don't think it would be usable 
>> for
>> doing large freq switches, since in current implementations of SoCs (or 
>> fpga), the
>> clk/pll etc driving core (and all timers etc) are not fixed like say ARM. 
>> And as
>> discussed before (and pointed to by tglx), timer subsys can't tolerate (on
>> purpose) such large drifts.
>
> Essentially cpufreq only makes sense for "compatible" systems and 
> unfortunately
> most of our current boards are not capable due to missing constant [decoupled 
> from
> CPU frequency] clock source. But looking forward we are planning to improve 
> on that
> so that hopefully even our FPGA-based boards will be usable with cpufreq.
>
>> > > which
>> > > means simple change of CPU frequency once time-keeping infrastructure
>> > > was brought-up is not an option... I.e. we'll end up with the system 
>> > > running
>> > > much slower compared what could have been possible.
>> > >
>> > > 2. Up until now we used to do dirty hacks in early platform init code.
>> > > Namely (see axs103_early_init() in arch/arc/plat-axs10x/axs10x.c):
>> > >  1) Read CPU's "clock-frequency" from .dtb (remember we're on very 
>> > > early
>> > > boot stage still so no expanded DevTree yet exists).
>> > >  2) Check how many cores we have and which freq is usable
>> > >  3) Update PLL settings right in place if new freq != existing in 
>> > > PLL.
>> > >
>> > > Even though it is proven to work but with more platforms in the 
>> > > pipeline
>> > > we'll need to copy-paste pretty much the same stuff across all 
>> > > affected
>> > > plats. Which is not nice.
>> > >
>> > > Moreover back in the day we didn't have a proper clk driver for 
>> > > CPU's PLL.
>> > > Thus acting on PLL registers right in place was the only thing we 
>> > > were able
>> > > to do. Now with introduction of normal clk driver
>> > > (see drivers/clk/axs10x/pll_clock.c in linux-next) we'd like to 
>> > > utilize
>> > > it and have a cleaner and more universal solution to the problem.
>> > >
>> > > That's how it could be done - 
>> > > https://urldefense.proofpoint.com/v2/url?u=http-3A__patchwork.ozlabs.org_patch_801240_=DwICAg=DPL6_X_6JkXF
>> > > x7AXWqB0tg=c14YS-cH-
>> > > kdhTOW89KozFhBtBJgs1zXscZojEZQ0THs=wuUcceY8Cz5EhVklWLAgj7RzU3rvpanujvQ3qTJK0Gw=N5IBjq_eCyOf_GRkZskzqGhczBPTbxLJW_MUfauKvuA=
>> > > Basically in architecture's time_init() we check if there's 
>> > > explicitly
>> > > specified "clock-frequency" parameter in cpu's node in Device Tree 
>> > > and
>> > > if there's one we set it via just instantiated clk driver.
>> > The patch looks generally okay. I'd move all the logic to the clock
>> > driver unless perhaps how to set the cpu freq is defined by the
>> > architecture.
>
> Yeah, that's an interesting question. We may indeed move more smarts to the 
> clock driver
> but:
>  1. We'll have duplicate code in different clock drivers. Even today that 
> kind of clock
> setup is applicable to AXS10x and HSDK platforms (and they use different 
> clock drivers).

No, you could provide a common, shared function to call. Then each
platform can opt-in. If you can make something that applies to every
single platform now or in the future, then I'd put it in arch. If you
have plans to decouple the timer and cpu clocks, then sounds like you
can't.

IMO, if it's not part of the defined CPU architecture, then don't put
it in arch/. That's how we end up with multiple copies of the same

Re: Setting CPU clock frequency on early boot

[Alexey Brodkin]

Hi Vineet, Rob,

On Tue, 2017-09-05 at 16:40 -0700, Vineet Gupta wrote:
> On 09/05/2017 03:04 PM, Rob Herring wrote:
> > 
> > On Tue, Sep 5, 2017 at 10:37 AM, Alexey Brodkin
> >  wrote:
> > > 
> > > Hello,
> > > 
> > > I'd like to get some feedback on our idea as well as check
> > > if somebody faces similar situations and if so what would be the best
> > > way to implement some generic solution that suits everyone.
> > > 
> > > So that's our problem:
> > > 1. On power-on hardware might start clocking CPU with either
> > > too high frequency (such that CPU may get stuck at some point)
> > > or too low frequency.
> > > 
> > > That all sounds stupid but let me elaborate a bit here.
> > > I'm talking about FPGA-based devboards firmware for which
> > > (here I mean just image loaded in FPGA with CPU implementation
> > > but not some software yet) might not be stable or be even 
> > > experimental.
> > > 
> > > For example we may deal with dual-core or quad-core designs.
> > > Former might be OK running @100MHz and latter is only usable
> > > @75MHz and lower. The simplest solution might be to use some safe
> > > value before something like CPUfreq kicks in. But we don't yet have
> > > CPUfreq for ARC (we do plan to get it working sometime soon)
> 
> But even if we had cpufreq driver going - I don't think it would be usable 
> for 
> doing large freq switches, since in current implementations of SoCs (or 
> fpga), the 
> clk/pll etc driving core (and all timers etc) are not fixed like say ARM. And 
> as 
> discussed before (and pointed to by tglx), timer subsys can't tolerate (on 
> purpose) such large drifts.

Essentially cpufreq only makes sense for "compatible" systems and unfortunately
most of our current boards are not capable due to missing constant [decoupled 
from
CPU frequency] clock source. But looking forward we are planning to improve on 
that
so that hopefully even our FPGA-based boards will be usable with cpufreq.

> > > which
> > > means simple change of CPU frequency once time-keeping infrastructure
> > > was brought-up is not an option... I.e. we'll end up with the system 
> > > running
> > > much slower compared what could have been possible.
> > > 
> > > 2. Up until now we used to do dirty hacks in early platform init code.
> > > Namely (see axs103_early_init() in arch/arc/plat-axs10x/axs10x.c):
> > >  1) Read CPU's "clock-frequency" from .dtb (remember we're on very 
> > > early
> > > boot stage still so no expanded DevTree yet exists).
> > >  2) Check how many cores we have and which freq is usable
> > >  3) Update PLL settings right in place if new freq != existing in PLL.
> > > 
> > > Even though it is proven to work but with more platforms in the 
> > > pipeline
> > > we'll need to copy-paste pretty much the same stuff across all 
> > > affected
> > > plats. Which is not nice.
> > > 
> > > Moreover back in the day we didn't have a proper clk driver for CPU's 
> > > PLL.
> > > Thus acting on PLL registers right in place was the only thing we 
> > > were able
> > > to do. Now with introduction of normal clk driver
> > > (see drivers/clk/axs10x/pll_clock.c in linux-next) we'd like to 
> > > utilize
> > > it and have a cleaner and more universal solution to the problem.
> > > 
> > > That's how it could be done - 
> > > https://urldefense.proofpoint.com/v2/url?u=http-3A__patchwork.ozlabs.org_patch_801240_=DwICAg=DPL6_X_6JkXF
> > > x7AXWqB0tg=c14YS-cH-
> > > kdhTOW89KozFhBtBJgs1zXscZojEZQ0THs=wuUcceY8Cz5EhVklWLAgj7RzU3rvpanujvQ3qTJK0Gw=N5IBjq_eCyOf_GRkZskzqGhczBPTbxLJW_MUfauKvuA=
> > > Basically in architecture's time_init() we check if there's explicitly
> > > specified "clock-frequency" parameter in cpu's node in Device Tree and
> > > if there's one we set it via just instantiated clk driver.
> > The patch looks generally okay. I'd move all the logic to the clock
> > driver unless perhaps how to set the cpu freq is defined by the
> > architecture.

Yeah, that's an interesting question. We may indeed move more smarts to the 
clock driver
but:
 1. We'll have duplicate code in different clock drivers. Even today that kind 
of clock
    setup is applicable to AXS10x and HSDK platforms (and they use different 
clock drivers).

 2. Print out of CPU frequency which is used during boot process for us is 
important as well
    especially during bring-up of new HW.

 3. If there's no dedicated "clock-frequency" parameter in CPU node we won't
    change anything so that non-affected platforms will live as they used to.

That said IMHO proposed implementation is what we want to kep for now.

> Also note that this code is using a new / adhoc DT binding cpu-freq in cou 
> node to 
> do the override - is that acceptable ?

I think we'll switch to more common "clock-frequency" in the next respin.
Indeed "cpu-freq" might be a bit misleading.

Also FWIW 

Re: Setting CPU clock frequency on early boot

[Vineet Gupta]

On 09/05/2017 03:04 PM, Rob Herring wrote:

On Tue, Sep 5, 2017 at 10:37 AM, Alexey Brodkin
 wrote:

Hello,

I'd like to get some feedback on our idea as well as check
if somebody faces similar situations and if so what would be the best
way to implement some generic solution that suits everyone.

So that's our problem:
1. On power-on hardware might start clocking CPU with either
too high frequency (such that CPU may get stuck at some point)
or too low frequency.

That all sounds stupid but let me elaborate a bit here.
I'm talking about FPGA-based devboards firmware for which
(here I mean just image loaded in FPGA with CPU implementation
but not some software yet) might not be stable or be even experimental.

For example we may deal with dual-core or quad-core designs.
Former might be OK running @100MHz and latter is only usable
@75MHz and lower. The simplest solution might be to use some safe
value before something like CPUfreq kicks in. But we don't yet have
CPUfreq for ARC (we do plan to get it working sometime soon)


But even if we had cpufreq driver going - I don't think it would be usable for 
doing large freq switches, since in current implementations of SoCs (or fpga), the 
clk/pll etc driving core (and all timers etc) are not fixed like say ARM. And as 
discussed before (and pointed to by tglx), timer subsys can't tolerate (on 
purpose) such large drifts.



which
means simple change of CPU frequency once time-keeping infrastructure
was brought-up is not an option... I.e. we'll end up with the system running
much slower compared what could have been possible.

2. Up until now we used to do dirty hacks in early platform init code.
Namely (see axs103_early_init() in arch/arc/plat-axs10x/axs10x.c):
 1) Read CPU's "clock-frequency" from .dtb (remember we're on very early
boot stage still so no expanded DevTree yet exists).
 2) Check how many cores we have and which freq is usable
 3) Update PLL settings right in place if new freq != existing in PLL.

Even though it is proven to work but with more platforms in the pipeline
we'll need to copy-paste pretty much the same stuff across all affected
plats. Which is not nice.

Moreover back in the day we didn't have a proper clk driver for CPU's PLL.
Thus acting on PLL registers right in place was the only thing we were able
to do. Now with introduction of normal clk driver
(see drivers/clk/axs10x/pll_clock.c in linux-next) we'd like to utilize
it and have a cleaner and more universal solution to the problem.

That's how it could be done - 
https://urldefense.proofpoint.com/v2/url?u=http-3A__patchwork.ozlabs.org_patch_801240_=DwICAg=DPL6_X_6JkXFx7AXWqB0tg=c14YS-cH-kdhTOW89KozFhBtBJgs1zXscZojEZQ0THs=wuUcceY8Cz5EhVklWLAgj7RzU3rvpanujvQ3qTJK0Gw=N5IBjq_eCyOf_GRkZskzqGhczBPTbxLJW_MUfauKvuA=
Basically in architecture's time_init() we check if there's explicitly
specified "clock-frequency" parameter in cpu's node in Device Tree and
if there's one we set it via just instantiated clk driver.

The patch looks generally okay. I'd move all the logic to the clock
driver unless perhaps how to set the cpu freq is defined by the
architecture.


But the above patch is clk driver agnostic and it would have to be added each clk 
driver (axs10x, hsdk - both in linux-next) ?
Also note that this code is using a new / adhoc DT binding cpu-freq in cou node to 
do the override - is that acceptable ?


-Vineet




We may indeed proceed with mentioned solution for ARC but if that makes
sense for somebody else it might worth getting something similar in generic
init code. Any thoughts?

If any ARM platforms are doing something similar, then it's done in
their clock driver via of_clk_init. Or they just wait for cpufreq to
kick in and expect the bootloader has initialized things to a
reasonable frequency.



___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: Setting CPU clock frequency on early boot

[Rob Herring]

On Tue, Sep 5, 2017 at 10:37 AM, Alexey Brodkin
 wrote:
> Hello,
>
> I'd like to get some feedback on our idea as well as check
> if somebody faces similar situations and if so what would be the best
> way to implement some generic solution that suits everyone.
>
> So that's our problem:
> 1. On power-on hardware might start clocking CPU with either
>too high frequency (such that CPU may get stuck at some point)
>or too low frequency.
>
>That all sounds stupid but let me elaborate a bit here.
>I'm talking about FPGA-based devboards firmware for which
>(here I mean just image loaded in FPGA with CPU implementation
>but not some software yet) might not be stable or be even experimental.
>
>For example we may deal with dual-core or quad-core designs.
>Former might be OK running @100MHz and latter is only usable
>@75MHz and lower. The simplest solution might be to use some safe
>value before something like CPUfreq kicks in. But we don't yet have
>CPUfreq for ARC (we do plan to get it working sometime soon) which
>means simple change of CPU frequency once time-keeping infrastructure
>was brought-up is not an option... I.e. we'll end up with the system 
> running
>much slower compared what could have been possible.
>
> 2. Up until now we used to do dirty hacks in early platform init code.
>Namely (see axs103_early_init() in arch/arc/plat-axs10x/axs10x.c):
> 1) Read CPU's "clock-frequency" from .dtb (remember we're on very early
>boot stage still so no expanded DevTree yet exists).
> 2) Check how many cores we have and which freq is usable
> 3) Update PLL settings right in place if new freq != existing in PLL.
>
>Even though it is proven to work but with more platforms in the pipeline
>we'll need to copy-paste pretty much the same stuff across all affected
>plats. Which is not nice.
>
>Moreover back in the day we didn't have a proper clk driver for CPU's PLL.
>Thus acting on PLL registers right in place was the only thing we were able
>to do. Now with introduction of normal clk driver
>(see drivers/clk/axs10x/pll_clock.c in linux-next) we'd like to utilize
>it and have a cleaner and more universal solution to the problem.
>
>That's how it could be done - http://patchwork.ozlabs.org/patch/801240/
>Basically in architecture's time_init() we check if there's explicitly
>specified "clock-frequency" parameter in cpu's node in Device Tree and
>if there's one we set it via just instantiated clk driver.

The patch looks generally okay. I'd move all the logic to the clock
driver unless perhaps how to set the cpu freq is defined by the
architecture.

> We may indeed proceed with mentioned solution for ARC but if that makes
> sense for somebody else it might worth getting something similar in generic
> init code. Any thoughts?

If any ARM platforms are doing something similar, then it's done in
their clock driver via of_clk_init. Or they just wait for cpufreq to
kick in and expect the bootloader has initialized things to a
reasonable frequency.

Rob

___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc