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@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? > > 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
