Message-ID: <ba7fa842-cfc5-6f6f-f9bf-09c74ab08ec2@lechnology.com>
Date:   Tue, 9 Jan 2018 21:01:29 -0600
From:   David Lechner <david@...hnology.com>
To:     Sekhar Nori <nsekhar@...com>, linux-clk@...r.kernel.org,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Cc:     Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Kevin Hilman <khilman@...nel.org>,
        Adam Ford <aford173@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 01/44] dt-bindings: clock: Add new bindings for TI
 Davinci PLL clocks

On 01/09/2018 06:35 AM, Sekhar Nori wrote:
> On Monday 08 January 2018 09:59 PM, David Lechner wrote:
>> On 01/08/2018 08:00 AM, Sekhar Nori wrote:
>>> On Monday 08 January 2018 07:47 AM, David Lechner wrote:
>>>> This adds a new binding for the PLL IP blocks in the mach-davinci family
>>>> of processors. Currently, only the SYSCLKn and AUXCLK outputs are
>>>> needed,
>>>> but in the future additional child nodes could be added for OBSCLK and
>>>> BPDIV.
>>>>
>>>> Note: Although these PLL controllers are very similar to the TI Keystone
>>>> SoCs, we are not re-using those bindings. The Keystone bindings use a
>>>> legacy one-node-per-clock binding. Furthermore, the mach-davinici SoCs
>>>
>>> Not sure what is meant by "legacy one-node-per-clock binding"
>>
>> It's a term I picked up from of_clk_detect_critical()
>>
>>   * Do not use this function. It exists only for legacy Device Tree
>>   * bindings, such as the one-clock-per-node style that are outdated.
>>   * Those bindings typically put all clock data into .dts and the Linux
>>   * driver has no clock data, thus making it impossible to set this flag
>>   * correctly from the driver. Only those drivers may call
>>   * of_clk_detect_critical from their setup functions.
> 
> Okay, I still don't understand the outdated style. I looked at clocks
> defined in arch/arm/boot/dts/stih407-clock.dtsi which is the only file
> that uses clock-critical and don't particularly see anything wrong with
> the way clocks are defined there.
> 
> Anyway, I guess we digress. As long as this patch series is not using
> the "legacy style", we are good :)
> 
>>>> have a slightly different PLL register layout and a number of quirks
>>>> that
>>>> can't be handled by the existing bindings, so the keystone bindings
>>>> could
>>>> not be used as-is anyway.
>>>
>>> Right, I think different register layout between the processors is the
>>> main reason for a new driver. This should be sufficient reason IMO.
>>>
>>>>
>>>> Signed-off-by: David Lechner <david@...hnology.com>
>>>> ---
>>>>    .../devicetree/bindings/clock/ti/davinci/pll.txt   | 47
>>>> ++++++++++++++++++++++
>>>>    1 file changed, 47 insertions(+)
>>>>    create mode 100644
>>>> Documentation/devicetree/bindings/clock/ti/davinci/pll.txt
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt
>>>> b/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt
>>>> new file mode 100644
>>>> index 0000000..99bf5da
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt
>>>> @@ -0,0 +1,47 @@
>>>> +Binding for TI DaVinci PLL Controllers
>>>> +
>>>> +The PLL provides clocks to most of the components on the SoC. In
>>>> addition
>>>> +to the PLL itself, this controller also contains bypasses, gates,
>>>> dividers,
>>>> +an multiplexers for various clock signals.
>>>> +
>>>> +Required properties:
>>>> +- compatible: shall be one of:
>>>> +    - "ti,da850-pll0" for PLL0 on DA850/OMAP-L138/AM18XX
>>>> +    - "ti,da850-pll1" for PLL1 on DA850/OMAP-L138/AM18XX
>>>
>>> These PLLs are same IP so they should use the same compatible. You can
>>> initialize both PLLs for DA850 based on the same compatible.
>>>
>>
>> But they are not exactly the same. For example, PLL0 has 7 PLLDIV clocks
>> while
>> PLL1 only has 3. PLL0 has PREDIV while PLL1 does not. PLL0 has certain
>> SYSCLKs
>> that are fixed-ratio but PLL1 does not have any of these. There are even
>> more
>> differences, but these are the ones we are actually using.
> 
> We need each element of the PLLC to be modeled individually as a clock
> node.

I gave this a good think while I have been working on this series
and I came to the conclusion that we really don't need to do this.
These components are all internal to the PLL IP block, so the
compatible string is enough to tell us what we have. They only
thing we need really in the device tree bindings are the connections
that are external to the IP block.


> That is, PLL should only model the multiplier, the dividers
> including post and prediv should be modeled as divider clocks (hopefully
> being able to use the clk-divider.c library). The sysclks can be
> fixed-factor-clock type clocks.
> 
> Without this flexible mechanism, we cannot (at least later) model things
> like DIV4.5 clock which is the only clock which derives from the output
> of PLL multiplier before the post divider is applied.
> 
> Since with DT there are are no retakes, we need to get this right the
> first time and modifying later will not be an option.
> 

So, the full device tree binding would look something like this:

+
+	pll0: clock-controller@...00 {
+		compatible = "ti,da850-pll0";
+		reg = <0x11000 0x1000>;
+		clocks = <&ref_clk>, <&pll1_sysclk 3>, <&pll1_obsclk>;
+		clock-names = "oscin", pll1_sysclk3", "pll1_osbclk";
+		oscin-square-wave;
+
+		pll0_sysclk: sysclk {
+			#clock-cells = <1>;
+		};
+
+		pll0_auxclk: auxclk {
+			#clock-cells = <0>;
+		};
+
+		pll0_div45: div4.5 {
+			#clock-cells = <0>;
+		};
+
+		pll0_obsclk: obsclk {
+			#clock-cells = <0>;
+			assigned-clocks = <&pll0_sysclk 1>;
+			assigned-clock-names = "ocsrc";
+		};
+	};

There are three clocks coming into the IP block and there are 11 clocks
going out (sysclk is 7 clocks). And you can specify the board-specific
configuration, like having the "oscin-square-wave" flag when a square wave
is used instead of a crystal oscillator and you can assign the multiplexer
input that will be used by obsclk. (And, this binding is totally compatible
with the binding I have already proposed - although, I see now it would
be better to go ahead and add the clocks-names property.)

How everything connects together is all implemented in the driver and the
driver will be able to know what to do just by looking at the compatible
string.

Part of my motivation for doing things this way comes from my recent
experience with writing some bindings for some LCD panels. These
device tree bindings were notorious for trying to be one-size-fits
all will lots of properties to try to describe all of the internal
workings. And it turns out that just having a new compatible string
for each device or variant and pushing all of details of the quirks
into the driver is much simpler and cleaner and will make it easier
for other projects to reuse the bindings.

Powered by blists - more mailing lists