On Wed, Sep 6, 2017 at 11:22 AM, Alexey Brodkin
<Alexey.Brodkin at synopsys.com> 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
>> <Alexey.Brodkin at synopsys.com> 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
>> > > > <Alexey.Brodkin at synopsys.com> 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 =
<&phandle>) 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 at 0 {
> device_type = "cpu";
> compatible = "snps,archs38";
> reg = <0>;
> clocks = <&core_clk>;
> clock-frequency = <100000000>; <-- That's where we want to set desired value
> };
> ...
> }
>
> core_clk: core-clk at 80 {
> compatible = "snps,axs10x-arc-pll-clock";
> reg = <0x80 0x10>, <0x100 0x10>;
> #clock-cells = <0>;
> clocks = <&input_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 at 80 {
> compatible = "snps,axs10x-arc-pll-clock";
> reg = <0x80 0x10>, <0x100 0x10>;
>
> #clock-cells = <0>;
> clocks = <&input_clk>;
> clock-frequency = <100000000>; <-- 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
