Re: [PATCH RFC v5 2/2] clk: Add handling of clk parent and rate assigned from DT

[Tero Kristo <t-kristo-l0cyMroinI0@public.gmane.org>]

On 05/23/2014 04:34 AM, Mike Turquette wrote:
> Quoting Sylwester Nawrocki (2014-04-11 05:25:49)
>>>> +==Assigned clock parents and rates==
>>>> +
>>>> +Some platforms require static initial configuration of parts of the clocks
>>>> +controller. Such a configuration can be specified in a clock consumer node
>>>> +through clock-parents and clock-rates DT properties. The former should
>>>> +contain a list of parent clocks in form of phandle and clock specifier pairs,
>>>> +the latter the list of assigned clock frequency values (one cell each).
>>>> +To skip setting parent or rate of a clock its corresponding entry should be
>>>> +set to 0, or can be omitted if it is not followed by any non-zero entry.
>>>> +
>>>> +    uart@a000 {
>>>> +        compatible = "fsl,imx-uart";
>>>> +        reg = <0xa000 0x1000>;
>>>> +        ...
>>>> +        clocks = <&clkcon 0>, <&clkcon 3>;
>>>> +        clock-names = "baud", "mux";
>>>> +
>>>> +        clock-parents = <0>, <&pll 1>;
>>>> +        clock-rates = <460800>;
>>>
>>> Is this the input frequency or serial baud rate? Looks like a baud
>>> rate, but the clock framework needs input (to the uart) frequency. I
>>> would say this should be clock-frequency and specify the max baud rate
>>> as is being done with i2c bindings. The uart driver should know how to
>>> convert between input clock freq and baud rate.
>>
>> This UART example is not quite representative for the issues I have been
>> trying to address with this patch set. There is a need to set (an initial)
>> input clock frequency. E.g. in case of multimedia devices there may be
>> a need to set clock parent and frequency of an input clock to multiple IP
>> blocks, so they are clocked synchronously and data is carried properly
>> across a whole processing chain. Thus there may not be even clock output
>> in an IP block, but still input clock needs to be set. IIUC there is
>> similar issue with audio, where it is difficult to calculate the clock
>> frequencies/determine parent clocks in individual drivers algorithmically.
>>
>>>> +    };
>>>> +
>>>> +In this example the pll is set as parent of "mux" clock and frequency
>>>> of "baud"
>>>> +clock is specified as 460800 Hz.
>>>
>>> I don't really like clock-parents. The parent information is part of
>>> the clock source, not the consumer.
>>
>> I'm not sure we must always consider the parent information as property
>> of a clock source. If for example we expose a structure like below as
>> single clock object, supporting clock gating, parent and frequency
>> setting the parent setting is still accessible from within a device
>> driver.
>
> The design of the ccf implementation certainly allows one to hide
> individually addressable/configurable clock nodes within a single struct
> clk. But should we? I have always maintained that a clock driver should
> enumerate clocks in the same way that the data sheet or technical
> reference manual states. I did make a recent exception[1], but that is
> going to be rolled back after the coordinated clock rate changes land in
> mainline.
>
>> And clock parent selection may depend on a system configuration
>> not immediately obvious from within a single device driver perspective.
>>
>>                           MUX
>>                         ,-------.     DIVIDER      GATE
>> common clk source 1 -->|--.    |   ,--------.   ,--------.
>>                         |   \   |   |        |   |        |
>> common clk source 2 -->|-   '--|-->|        |-->|        |--> consumer
>>         ...             |       |   |        |   |        |
>> common clk source N -->|-      |   '--------'   '--------'
>>                         '-------'
>>
>>> We've somewhat decided against having every single clock defined in DT
>>> and rather only describe a clock controller with leaf clocks to
>>> devices. That is not a hard rule, but for complex clock trees that is
>>> the norm. Doing something like this will require all levels of the
>>> clock tree to be described. You may have multiple layers of parents
>>> that have to be configured correctly. How are you configuring the rest
>>> of the tree?
>>
>> I believe even clock controllers where clocks are represented as flat
>> array often describe the clock tree entirely by parenthood, the tree
>> structure is just not obvious from the DT binding.
>> In addition, there seems to be appearing more and more clock controller
>> DT bindings describing their clocks individually.
>
> I've been discouraging these per-clock node bindings in favor of the
> per-controller node style.
>
>>
>>>> +Configuring a clock's parent and rate through the device node that uses
>>>> +the clock can be done only for clocks that have a single user. Specifying
>>>> +conflicting parent or rate configuration in multiple consumer nodes for
>>>> +a shared clock is forbidden.
>>>> +
>>>> +Configuration of common clocks, which affect multiple consumer devices
>>>> +can be specified in a dedicated 'assigned-clocks' subnode of a clock
>>>> +provider node, e.g.:
>>>
>>> This seems like a work-around due to having clock-parents in the
>>> consumer node. If (I'm not convinced we should) we have a binding for
>>> parent config, it needs to be a single binding that works for both
>>> cases.
>>
>> When this issue was first raised during an ARM kernel summit it was
>> proposed to add 'assigned' prefix to DT properties for such bindings.
>>
>
> Yes, I like the "assigned-" prefix.
>
>> How about separate properties for the default clock configuration,
>> e.g. assigned-clocks/assigned-clock-parents/assigned-clock-rates ?
>> So a clock provider would look like:
>>
>>      clkcon {
>>          ...
>>          #clock-cells = <1>;
>>
>>          assigned-clocks = <&clkcon 16>, <&clkcon 17>;
>>          assigned-clock-parents = <0>, <&clkcon 1>;
>>          assigned-clock-rates = <200000>;
>>      };
>>
>> And a consumer device node:
>>
>>      uart@a000 {
>>          compatible = "fsl,imx-uart";
>>          reg = <0xa000 0x1000>;
>>          ...
>>          clocks = <&clkcon 0>;
>>          clock-names = "baud";
>>
>>          assigned-clocks = <&clkcon 3>, <&clkcon 0>;
>>          assigned-clock-parents = <&pll 1>;
>>          assigned-clock-rates = <0>, <460800>;
>> };
>
> It looks like this idea was dropped for v6. Can we revisit it? Take a
> look at Tero's example implementation for OMAP using this binding:
>
> http://www.spinics.net/lists/linux-omap/msg104705.html
>
> There is a bogus "default-clocks" node made solely for storing this info
> within the OMAP PRCM clock provider node. This is basically faking a
> clock consumer. I think with the proposed solution above Tero could have
> avoided that node entirely and done the following:
>
> diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
> index 649b5cd..e3ff1a7 100644
> --- a/arch/arm/boot/dts/omap4.dtsi
> +++ b/arch/arm/boot/dts/omap4.dtsi
> @@ -145,6 +145,11 @@
>   			cm2_clocks: clocks {
>   				#address-cells = <1>;
>   				#size-cells = <0>;
> +
> +				assigned-clocks = <&abe_dpll_refclk_mux_ck>,
> +					<&dpll_usb_ck>, <&dpll_abe_ck>;
> +				assigned-clock-parents = <&sys_32k_ck>;
> +				assigned-clock-rates = <0>, <960000000>, <98304000>;
>   			};
>
>   			cm2_clockdomains: clockdomains {
>
>
> Tero, what do you think?

Yeah, if we can avoid having a dummy node someplace, it is always 
better. Only issue might be the initialization order, this was the 
reason I created the dummy node if I recall right. But I guess we can 
just scan the clock provider nodes second time at a later phase of boot 
(or just store the default info for later use.)

-Tero

>
> Regards,
> Mike
>
> [1] http://www.spinics.net/lists/cpufreq/msg10071.html
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html