Re: [PATCH v6 01/41] dt-bindings: clock: Add new bindings for TI Davinci PLL clocks
From: David Lechner <david@lechnology.com>
Date: 2018-01-30 18:46:33
Also in:
linux-arm-kernel, linux-clk, lkml
On 01/30/2018 08:50 AM, Rob Herring wrote:
On Mon, Jan 29, 2018 at 3:14 PM, David Lechner [off-list ref] wrote:quoted
On 01/29/2018 01:53 PM, Rob Herring wrote:quoted
On Sat, Jan 20, 2018 at 11:13:40AM -0600, David Lechner wrote:quoted
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.txtdiff --git a/Documentation/devicetree/bindings/clock/ti/davinci/pll.txtb/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. Inaddition +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 0So why have all these child nodes vs. just defining a single number space of clock ids? RobI 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.
quoted
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 invalidYou 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.