Messages in this thread | | | Subject | Re: [PATCH v6 01/41] dt-bindings: clock: Add new bindings for TI Davinci PLL clocks | From | David Lechner <> | Date | Tue, 30 Jan 2018 12:46:28 -0600 |
| |
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.
| |