Re: [PATCH v6 01/41] dt-bindings: clock: Add new bindings for TI Davinci PLL clocks

[Rob Herring]

On Mon, Jan 29, 2018 at 3:14 PM, David Lechner <david@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>>> ---
>>>
>>> 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?

> 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.

Rob