From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sekhar Nori Subject: Re: [PATCH v6 01/41] dt-bindings: clock: Add new bindings for TI Davinci PLL clocks Date: Wed, 31 Jan 2018 10:28:39 +0530 Message-ID: References: <1516468460-4908-1-git-send-email-david@lechnology.com> <1516468460-4908-2-git-send-email-david@lechnology.com> <20180129195315.bjanym7pmeh7bhaa@rob-hp-laptop> <7064a0d6-0969-81a0-00af-84cca339b813@lechnology.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <7064a0d6-0969-81a0-00af-84cca339b813@lechnology.com> Content-Language: en-US Sender: linux-clk-owner@vger.kernel.org To: David Lechner , Rob Herring Cc: linux-clk , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" , Michael Turquette , Stephen Boyd , Mark Rutland , Kevin Hilman , Bartosz Golaszewski , Adam Ford , "linux-kernel@vger.kernel.org" List-Id: devicetree@vger.kernel.org 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 >> 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 >>>>> --- >>>>> >>>>> 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