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

Tero Kristo
2014-05-23T06:38:17Z

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
>