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

Message ID 1516468460-4908-2-git-send-email-david@lechnology.com
State New
Headers show
Series
  • ARM: davinci: convert to common clock framework​
Related show

Commit Message

David Lechner Jan. 20, 2018, 5:13 p.m.
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

Comments

Rob Herring Jan. 29, 2018, 7:53 p.m. | #1
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Lechner Jan. 29, 2018, 9:14 p.m. | #2
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.

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

---

I guess it is not too bad (writing a clock lookup function that knows
all of these rules could get interesting though).

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring Jan. 30, 2018, 2:50 p.m. | #3
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?

> 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.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Lechner Jan. 30, 2018, 6:46 p.m. | #4
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.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sekhar Nori Jan. 31, 2018, 4:58 a.m. | #5
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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
+
+
+Examples:
+
+	pll0: clock-controller@11000 {
+		compatible = "ti,da850-pll0";
+		reg = <0x11000 0x1000>;
+		clocks = <&ref_clk>, <&pll1_sysclk 3>;
+		clock-names = "clksrc", "extclksrc";
+		ti,clkmode-square-wave;
+
+		pll0_pllout: pllout {
+			#clock-cells = <0>;
+		};
+
+		pll0_sysclk: sysclk {
+			#clock-cells = <1>;
+		};
+
+		pll0_auxclk: auxclk {
+			#clock-cells = <0>;
+		};
+
+		pll0_obsclk: obsclk {
+			#clock-cells = <0>;
+		};
+	};
+
+	pll1: clock-controller@21a000 {
+		compatible = "ti,da850-pll1";
+		reg = <0x21a000 0x1000>;
+		clocks = <&ref_clk>;
+		clock-names = "clksrc";
+
+		pll0_sysclk: sysclk {
+			#clock-cells = <1>;
+		};
+
+		pll0_obsclk: obsclk {
+			#clock-cells = <0>;
+		};
+	};
+
+Also see:
+- Documentation/devicetree/bindings/clock/clock-bindings.txt