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

[Sekhar Nori <nsekhar@ti.com>]

On Wednesday 31 January 2018 12:16 AM, David Lechner wrote:
> On 01/30/2018 08:50 AM, Rob Herring wrote:
>> On Mon, Jan 29, 2018 at 3:14 PM, David Lechner <david@lechnology.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@lechnology.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.

FWIW, I think the current binding is fine too. The clocks are of
different kind (fixed clock, fixed divider, flexible divider etc). So,
its slightly better to have child nodes, I guess.

Thanks,
Sekhar