Date:   Tue, 30 Jan 2018 12:46:28 -0600
From:   David Lechner <david@...hnology.com>
To:     Rob Herring <robh@...nel.org>
Cc:     linux-clk <linux-clk@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@...ts.infradead.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Mark Rutland <mark.rutland@....com>,
        Sekhar Nori <nsekhar@...com>,
        Kevin Hilman <khilman@...nel.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Adam Ford <aford173@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 01/41] dt-bindings: clock: Add new bindings for TI
 Davinci PLL clocks

On 01/30/2018 08:50 AM, Rob Herring wrote:
> On Mon, Jan 29, 2018 at 3:14 PM, David Lechner <david@...hnology.com> wrote:
>> On 01/29/2018 01:53 PM, Rob Herring wrote:
>>>
>>> On Sat, Jan 20, 2018 at 11:13:40AM -0600, David Lechner wrote:
>>>>
>>>> This adds a new binding for the PLL IP blocks in the mach-davinci
>>>> family of processors. Currently, only da850 has device tree support
>>>> but these bindings can also work for other SoCs in this family just
>>>> by adding new compatible strings.
>>>>
>>>> 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
>>>> 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.
>>>>
>>>> Signed-off-by: David Lechner <david@...hnology.com>
>>>> ---
>>>>
>>>> v6 changes:
>>>> - Added clock-names property
>>>> - Added ti,clkmode-square-wave property
>>>> - Added pllout child node
>>>> - Added obsclk child node
>>>> - Expanded examples
>>>>
>>>>    .../devicetree/bindings/clock/ti/davinci/pll.txt   | 96
>>>> ++++++++++++++++++++++
>>>>    1 file changed, 96 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..36998e1
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt
>>>> @@ -0,0 +1,96 @@
>>>> +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
>>>> +- reg: physical base address and size of the controller's register area.
>>>> +- clocks: phandles corresponding to the clock names
>>>> +- clock-names: names of the clock sources - depends on compatible string
>>>> +       - for "ti,da850-pll0", shall be "clksrc", "extclksrc"
>>>> +       - for "ti,da850-pll1", shall be "clksrc"
>>>> +
>>>> +Optional properties:
>>>> +- ti,clkmode-square-wave: Indicates that the the board is supplying a
>>>> square
>>>> +       wave input on the OSCIN pin instead of using a crystal
>>>> oscillator.
>>>> +       This property is only valid when compatible = "ti,da850-pll0".
>>>> +
>>>> +
>>>> +Optional child nodes:
>>>> +
>>>> +pllout
>>>> +       Describes the main PLL clock output (before POSTDIV). The node
>>>> name must
>>>> +       be "pllout".
>>>> +
>>>> +       Required properties:
>>>> +       - #clock-cells: shall be 0
>>>> +
>>>> +sysclk
>>>> +       Describes the PLLDIVn divider clocks that provide the SYSCLKn
>>>> clock
>>>> +       domains. The node name must be "sysclk". Consumers of this node
>>>> should
>>>> +       use "n" in "SYSCLKn" as the index parameter for the clock cell.
>>>> +
>>>> +       Required properties:
>>>> +       - #clock-cells: shall be 1
>>>> +
>>>> +auxclk
>>>> +       Describes the AUXCLK output of the PLL. The node name must be
>>>> "auxclk".
>>>> +       This child node is only valid when compatible = "ti,da850-pll0".
>>>> +
>>>> +       Required properties:
>>>> +       - #clock-cells: shall be 0
>>>> +
>>>> +obsclk
>>>> +       Describes the OBSCLK output of the PLL. The node name must be
>>>> "obsclk".
>>>> +
>>>> +       Required properties:
>>>> +       - #clock-cells: shall be 0
>>>
>>>
>>> So why have all these child nodes vs. just defining a single number
>>> space of clock ids?
>>>
>>> Rob
>>>
>>
>> I think that it makes the bindings more self-documenting. Not all PLLs have
>> all of possible types of output clocks, so the presence or absence of a
>> child node indicates if a PLL actually has that output or not.
> 
> Doesn't the compatible string do that?

Sure.

> 
>> It is also complicated by the fact that one of the child nodes (sysclk)
>> is  already an array of clocks.
>>
>> To do what you are suggesting might look something like this...
>>
>> ---
>>
>> 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
>> - reg: physical base address and size of the controller's register area.
>> - clocks: phandles corresponding to the clock names
>> - clock-names: names of the clock sources - depends on compatible string
>>          - for "ti,da850-pll0", shall be "clksrc", "extclksrc"
>>          - for "ti,da850-pll1", shall be "clksrc"
>> - #clock-cells: shall be set to <2>.
>>
>> Consumers:
>>
>> The clock cell values for consumers work as follows...
>>
>> The first index is one of the constants defined in ti-davinci-pll.h
>>
>> The second index is 0 unless the first index is TI_DAVINCI_SYSCLK. In the
>> case
>> of TI_DAVINCI_SYSCLK the second index the SYSCLK domain ID (n in SYSCLKn).
>>
>> For compatible = "ti,da850-pll0":
>>          - <&pll0 TI_DAVINCI_PLLOUT 0> is the PLLOUT clock
>>          - <&pll0 TI_DAVINCI_SYSCLK n> is one of the SYSCLKn clock domains
>> where n is 1 to 7
>>          - <&pll0 TI_DAVINCI_AUXCLK 0> is the AUXCLK clock domain
>>          - <&pll0 TI_DAVINCI_OBSCLK 0> is the OBSCLK clock domain
>>          - all other index combinations are invalid
>>
>> For compatible = "ti,da850-pll1":
>>          - <&pll0 TI_DAVINCI_SYSCLK n> is one of the SYSCLKn clock domains
>> where n is 1 to 3
>>          - <&pll0 TI_DAVINCI_OBSCLK 0> is the OBSCLK clock domain
>>          - all other index combinations are invalid
> 
> You don't really need 2 cells here. I guess if you want to keep the
> child nodes, that is fine.

OK, I can see how it could work with one cell.

Since this is already implemented and working, I'm inclined to leave it
as-is if it is "good enough". But, I am fine going either way if there
are other opinions on the matter.

Powered by blists - more mailing lists