diff mbox series

[v1,1/5] dt-bindings: cpufreq: Add binding for NVIDIA Tegra20/30

Message ID 20180830194356.14059-2-digetx@gmail.com
State Not Applicable, archived
Headers show
Series CPUFREQ OPP's and Tegra30 support by tegra20-cpufreq driver | expand

Commit Message

Dmitry Osipenko Aug. 30, 2018, 7:43 p.m. UTC
Add device-tree binding that describes CPU frequency-scaling hardware
found on NVIDIA Tegra20/30 SoC's.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 .../cpufreq/nvidia,tegra20-cpufreq.txt        | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt

Comments

Rob Herring Sept. 25, 2018, 4:58 p.m. UTC | #1
On Thu, Aug 30, 2018 at 10:43:52PM +0300, Dmitry Osipenko wrote:
> Add device-tree binding that describes CPU frequency-scaling hardware
> found on NVIDIA Tegra20/30 SoC's.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  .../cpufreq/nvidia,tegra20-cpufreq.txt        | 38 +++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
> new file mode 100644
> index 000000000000..2c51f676e958
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
> @@ -0,0 +1,38 @@
> +Binding for NVIDIA Tegra20 CPUFreq
> +==================================
> +
> +Required properties:
> +- clocks: Must contain an entry for each entry in clock-names.
> +  See ../clocks/clock-bindings.txt for details.
> +- clock-names: Must include the following entries:
> +  - pll_x: main-parent for CPU clock, must be the first entry
> +  - backup: intermediate-parent for CPU clock
> +  - cpu: the CPU clock

The Cortex A9 has CLK, PERIPHCLK, and PERIPHCLKEN clocks and only CLK 
is used for the cpu core. You can't just define your own clocks that 
you happen to want access to.

Otherwise, you're not defining anything new here, so a binding document 
isn't required.

Rob
Dmitry Osipenko Sept. 25, 2018, 5:29 p.m. UTC | #2
On 9/25/18 7:58 PM, Rob Herring wrote:
> On Thu, Aug 30, 2018 at 10:43:52PM +0300, Dmitry Osipenko wrote:
>> Add device-tree binding that describes CPU frequency-scaling hardware
>> found on NVIDIA Tegra20/30 SoC's.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  .../cpufreq/nvidia,tegra20-cpufreq.txt        | 38 +++++++++++++++++++
>>  1 file changed, 38 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>
>> diff --git a/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>> new file mode 100644
>> index 000000000000..2c51f676e958
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>> @@ -0,0 +1,38 @@
>> +Binding for NVIDIA Tegra20 CPUFreq
>> +==================================
>> +
>> +Required properties:
>> +- clocks: Must contain an entry for each entry in clock-names.
>> +  See ../clocks/clock-bindings.txt for details.
>> +- clock-names: Must include the following entries:
>> +  - pll_x: main-parent for CPU clock, must be the first entry
>> +  - backup: intermediate-parent for CPU clock
>> +  - cpu: the CPU clock
> 
> The Cortex A9 has CLK, PERIPHCLK, and PERIPHCLKEN clocks and only CLK 
> is used for the cpu core. You can't just define your own clocks that 
> you happen to want access to.
> 
> Otherwise, you're not defining anything new here, so a binding document 
> isn't required.

PERIPHCLK is a different thing. Here we are defining the CPU clock and
its *parent* sources, the PLLX (main) and backup (intermediate clock
that is used while PLLX changes its rate). These are not some random
clocks "that you happen to want access to", they are essential for the
Tegra CPUFreq driver, CPU is running off them.

I assume that PERIPHCLK and other clocks are derived from the "CPU"
clock and their configuration is hardwired. Probably Peter knows how
it's implemented in HW.

I'm now working on v2 that will include more Tegra-specific stuff in the
binding, like custom "opp-supported-hw" property and probably some more.
Rob Herring Sept. 25, 2018, 7:36 p.m. UTC | #3
On Tue, Sep 25, 2018 at 12:29 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> On 9/25/18 7:58 PM, Rob Herring wrote:
> > On Thu, Aug 30, 2018 at 10:43:52PM +0300, Dmitry Osipenko wrote:
> >> Add device-tree binding that describes CPU frequency-scaling hardware
> >> found on NVIDIA Tegra20/30 SoC's.
> >>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >>  .../cpufreq/nvidia,tegra20-cpufreq.txt        | 38 +++++++++++++++++++
> >>  1 file changed, 38 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
> >> new file mode 100644
> >> index 000000000000..2c51f676e958
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
> >> @@ -0,0 +1,38 @@
> >> +Binding for NVIDIA Tegra20 CPUFreq
> >> +==================================
> >> +
> >> +Required properties:
> >> +- clocks: Must contain an entry for each entry in clock-names.
> >> +  See ../clocks/clock-bindings.txt for details.
> >> +- clock-names: Must include the following entries:
> >> +  - pll_x: main-parent for CPU clock, must be the first entry
> >> +  - backup: intermediate-parent for CPU clock
> >> +  - cpu: the CPU clock
> >
> > The Cortex A9 has CLK, PERIPHCLK, and PERIPHCLKEN clocks and only CLK
> > is used for the cpu core. You can't just define your own clocks that
> > you happen to want access to.
> >
> > Otherwise, you're not defining anything new here, so a binding document
> > isn't required.
>
> PERIPHCLK is a different thing.

Right, that's what I meant. Only CLK is used.

> Here we are defining the CPU clock and
> its *parent* sources, the PLLX (main) and backup (intermediate clock
> that is used while PLLX changes its rate). These are not some random
> clocks "that you happen to want access to", they are essential for the
> Tegra CPUFreq driver, CPU is running off them.

assigned-clocks is generally how we get parent clocks in this
situation. "clocks" is for what physical clocks are attached to a
given block. ARM very clearly documents the clocks for their IP
blocks.

> I assume that PERIPHCLK and other clocks are derived from the "CPU"
> clock and their configuration is hardwired. Probably Peter knows how
> it's implemented in HW.

Yes, because PERIPHCLK must be synchronous. In any case, it is
irrelevant to cpu nodes and applies to timer, SCU, and GIC nodes.

> I'm now working on v2 that will include more Tegra-specific stuff in the
> binding, like custom "opp-supported-hw" property and probably some more.
Dmitry Osipenko Sept. 25, 2018, 9:57 p.m. UTC | #4
On 9/25/18 10:36 PM, Rob Herring wrote:
> On Tue, Sep 25, 2018 at 12:29 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> On 9/25/18 7:58 PM, Rob Herring wrote:
>>> On Thu, Aug 30, 2018 at 10:43:52PM +0300, Dmitry Osipenko wrote:
>>>> Add device-tree binding that describes CPU frequency-scaling hardware
>>>> found on NVIDIA Tegra20/30 SoC's.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>>  .../cpufreq/nvidia,tegra20-cpufreq.txt        | 38 +++++++++++++++++++
>>>>  1 file changed, 38 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>> new file mode 100644
>>>> index 000000000000..2c51f676e958
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>> @@ -0,0 +1,38 @@
>>>> +Binding for NVIDIA Tegra20 CPUFreq
>>>> +==================================
>>>> +
>>>> +Required properties:
>>>> +- clocks: Must contain an entry for each entry in clock-names.
>>>> +  See ../clocks/clock-bindings.txt for details.
>>>> +- clock-names: Must include the following entries:
>>>> +  - pll_x: main-parent for CPU clock, must be the first entry
>>>> +  - backup: intermediate-parent for CPU clock
>>>> +  - cpu: the CPU clock
>>>
>>> The Cortex A9 has CLK, PERIPHCLK, and PERIPHCLKEN clocks and only CLK
>>> is used for the cpu core. You can't just define your own clocks that
>>> you happen to want access to.
>>>
>>> Otherwise, you're not defining anything new here, so a binding document
>>> isn't required.
>>
>> PERIPHCLK is a different thing.
> 
> Right, that's what I meant. Only CLK is used.
> 
>> Here we are defining the CPU clock and
>> its *parent* sources, the PLLX (main) and backup (intermediate clock
>> that is used while PLLX changes its rate). These are not some random
>> clocks "that you happen to want access to", they are essential for the
>> Tegra CPUFreq driver, CPU is running off them.
> 
> assigned-clocks is generally how we get parent clocks in this
> situation. "clocks" is for what physical clocks are attached to a
> given block. ARM very clearly documents the clocks for their IP
> blocks.

Okay, seems I see now what you're meaning. The PLLX is specifically
dedicated to CPU HW on Tegra, so it shouldn't be questioning to put it
within the CPU node, unlike the backup.

The assigned-clocks are about static clocks configuration, that is
exactly opposite to what is needed. I'm not sure what you are trying to
convey.. we don't need to get the parent clock, but set it.

How the backup/intermediate clock should be represented then? It is a
part of HW description which potentially may vary depending on the board
configuration.
Jon Hunter Oct. 17, 2018, 8:40 a.m. UTC | #5
On 30/08/2018 20:43, Dmitry Osipenko wrote:
> Add device-tree binding that describes CPU frequency-scaling hardware
> found on NVIDIA Tegra20/30 SoC's.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  .../cpufreq/nvidia,tegra20-cpufreq.txt        | 38 +++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
> new file mode 100644
> index 000000000000..2c51f676e958
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
> @@ -0,0 +1,38 @@
> +Binding for NVIDIA Tegra20 CPUFreq
> +==================================
> +
> +Required properties:
> +- clocks: Must contain an entry for each entry in clock-names.
> +  See ../clocks/clock-bindings.txt for details.
> +- clock-names: Must include the following entries:
> +  - pll_x: main-parent for CPU clock, must be the first entry
> +  - backup: intermediate-parent for CPU clock
> +  - cpu: the CPU clock

Is it likely that 'backup' will be anything other that pll_p? If not why
not just call it pll_p? Personally, I don't 'backup' to descriptive even
though I can see what you mean.

I can see that you want to make this flexible, but if the likelihood is
that we will just use pll_p then I am not sure it is warranted at this
point.

Cheers
Jon
Dmitry Osipenko Oct. 17, 2018, 12:37 p.m. UTC | #6
On 10/17/18 11:40 AM, Jon Hunter wrote:
> 
> On 30/08/2018 20:43, Dmitry Osipenko wrote:
>> Add device-tree binding that describes CPU frequency-scaling hardware
>> found on NVIDIA Tegra20/30 SoC's.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  .../cpufreq/nvidia,tegra20-cpufreq.txt        | 38 +++++++++++++++++++
>>  1 file changed, 38 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>
>> diff --git a/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>> new file mode 100644
>> index 000000000000..2c51f676e958
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>> @@ -0,0 +1,38 @@
>> +Binding for NVIDIA Tegra20 CPUFreq
>> +==================================
>> +
>> +Required properties:
>> +- clocks: Must contain an entry for each entry in clock-names.
>> +  See ../clocks/clock-bindings.txt for details.
>> +- clock-names: Must include the following entries:
>> +  - pll_x: main-parent for CPU clock, must be the first entry
>> +  - backup: intermediate-parent for CPU clock
>> +  - cpu: the CPU clock
> 
> Is it likely that 'backup' will be anything other that pll_p? If not why
> not just call it pll_p? Personally, I don't 'backup' to descriptive even
> though I can see what you mean.
> 
> I can see that you want to make this flexible, but if the likelihood is
> that we will just use pll_p then I am not sure it is warranted at this
> point.

That won't describe HW, but software. And device tree should describe HW.
Dmitry Osipenko Oct. 17, 2018, 12:42 p.m. UTC | #7
On 10/17/18 3:37 PM, Dmitry Osipenko wrote:
> On 10/17/18 11:40 AM, Jon Hunter wrote:
>>
>> On 30/08/2018 20:43, Dmitry Osipenko wrote:
>>> Add device-tree binding that describes CPU frequency-scaling hardware
>>> found on NVIDIA Tegra20/30 SoC's.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>  .../cpufreq/nvidia,tegra20-cpufreq.txt        | 38 +++++++++++++++++++
>>>  1 file changed, 38 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>> new file mode 100644
>>> index 000000000000..2c51f676e958
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>> @@ -0,0 +1,38 @@
>>> +Binding for NVIDIA Tegra20 CPUFreq
>>> +==================================
>>> +
>>> +Required properties:
>>> +- clocks: Must contain an entry for each entry in clock-names.
>>> +  See ../clocks/clock-bindings.txt for details.
>>> +- clock-names: Must include the following entries:
>>> +  - pll_x: main-parent for CPU clock, must be the first entry
>>> +  - backup: intermediate-parent for CPU clock
>>> +  - cpu: the CPU clock
>>
>> Is it likely that 'backup' will be anything other that pll_p? If not why
>> not just call it pll_p? Personally, I don't 'backup' to descriptive even
>> though I can see what you mean.
>>
>> I can see that you want to make this flexible, but if the likelihood is
>> that we will just use pll_p then I am not sure it is warranted at this
>> point.
> 
> That won't describe HW, but software. And device tree should describe HW.
> 

Though indeed it is unlikely that anything else other than pll_p will be used, so it is a software/firmware description anyway.
Jon Hunter Oct. 17, 2018, 12:59 p.m. UTC | #8
On 17/10/2018 13:37, Dmitry Osipenko wrote:
> On 10/17/18 11:40 AM, Jon Hunter wrote:
>>
>> On 30/08/2018 20:43, Dmitry Osipenko wrote:
>>> Add device-tree binding that describes CPU frequency-scaling hardware
>>> found on NVIDIA Tegra20/30 SoC's.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>  .../cpufreq/nvidia,tegra20-cpufreq.txt        | 38 +++++++++++++++++++
>>>  1 file changed, 38 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>> new file mode 100644
>>> index 000000000000..2c51f676e958
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>> @@ -0,0 +1,38 @@
>>> +Binding for NVIDIA Tegra20 CPUFreq
>>> +==================================
>>> +
>>> +Required properties:
>>> +- clocks: Must contain an entry for each entry in clock-names.
>>> +  See ../clocks/clock-bindings.txt for details.
>>> +- clock-names: Must include the following entries:
>>> +  - pll_x: main-parent for CPU clock, must be the first entry
>>> +  - backup: intermediate-parent for CPU clock
>>> +  - cpu: the CPU clock
>>
>> Is it likely that 'backup' will be anything other that pll_p? If not why
>> not just call it pll_p? Personally, I don't 'backup' to descriptive even
>> though I can see what you mean.
>>
>> I can see that you want to make this flexible, but if the likelihood is
>> that we will just use pll_p then I am not sure it is warranted at this
>> point.
> 
> That won't describe HW, but software. And device tree should describe HW.

Hmm ... well that's my point exactly. So why call it 'backup'? Sounds
like a software description to me.

Jon
Dmitry Osipenko Oct. 17, 2018, 1:07 p.m. UTC | #9
On 10/17/18 3:59 PM, Jon Hunter wrote:
> 
> On 17/10/2018 13:37, Dmitry Osipenko wrote:
>> On 10/17/18 11:40 AM, Jon Hunter wrote:
>>>
>>> On 30/08/2018 20:43, Dmitry Osipenko wrote:
>>>> Add device-tree binding that describes CPU frequency-scaling hardware
>>>> found on NVIDIA Tegra20/30 SoC's.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>>  .../cpufreq/nvidia,tegra20-cpufreq.txt        | 38 +++++++++++++++++++
>>>>  1 file changed, 38 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>> new file mode 100644
>>>> index 000000000000..2c51f676e958
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>> @@ -0,0 +1,38 @@
>>>> +Binding for NVIDIA Tegra20 CPUFreq
>>>> +==================================
>>>> +
>>>> +Required properties:
>>>> +- clocks: Must contain an entry for each entry in clock-names.
>>>> +  See ../clocks/clock-bindings.txt for details.
>>>> +- clock-names: Must include the following entries:
>>>> +  - pll_x: main-parent for CPU clock, must be the first entry
>>>> +  - backup: intermediate-parent for CPU clock
>>>> +  - cpu: the CPU clock
>>>
>>> Is it likely that 'backup' will be anything other that pll_p? If not why
>>> not just call it pll_p? Personally, I don't 'backup' to descriptive even
>>> though I can see what you mean.
>>>
>>> I can see that you want to make this flexible, but if the likelihood is
>>> that we will just use pll_p then I am not sure it is warranted at this
>>> point.
>>
>> That won't describe HW, but software. And device tree should describe HW.
> 
> Hmm ... well that's my point exactly. So why call it 'backup'? Sounds
> like a software description to me.

Because HW is designed the way that CPU parent need to be switched to the backup clock source while main clock changes its rate. HW also allow to select among different parents, pll_p is one of those parents.

I think cpufreq-mediatek has a similar description, see Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek.txt
Jon Hunter Oct. 17, 2018, 1:34 p.m. UTC | #10
On 17/10/2018 14:07, Dmitry Osipenko wrote:
> On 10/17/18 3:59 PM, Jon Hunter wrote:
>>
>> On 17/10/2018 13:37, Dmitry Osipenko wrote:
>>> On 10/17/18 11:40 AM, Jon Hunter wrote:
>>>>
>>>> On 30/08/2018 20:43, Dmitry Osipenko wrote:
>>>>> Add device-tree binding that describes CPU frequency-scaling hardware
>>>>> found on NVIDIA Tegra20/30 SoC's.
>>>>>
>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>> ---
>>>>>  .../cpufreq/nvidia,tegra20-cpufreq.txt        | 38 +++++++++++++++++++
>>>>>  1 file changed, 38 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>>> new file mode 100644
>>>>> index 000000000000..2c51f676e958
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>>> @@ -0,0 +1,38 @@
>>>>> +Binding for NVIDIA Tegra20 CPUFreq
>>>>> +==================================
>>>>> +
>>>>> +Required properties:
>>>>> +- clocks: Must contain an entry for each entry in clock-names.
>>>>> +  See ../clocks/clock-bindings.txt for details.
>>>>> +- clock-names: Must include the following entries:
>>>>> +  - pll_x: main-parent for CPU clock, must be the first entry
>>>>> +  - backup: intermediate-parent for CPU clock
>>>>> +  - cpu: the CPU clock
>>>>
>>>> Is it likely that 'backup' will be anything other that pll_p? If not why
>>>> not just call it pll_p? Personally, I don't 'backup' to descriptive even
>>>> though I can see what you mean.
>>>>
>>>> I can see that you want to make this flexible, but if the likelihood is
>>>> that we will just use pll_p then I am not sure it is warranted at this
>>>> point.
>>>
>>> That won't describe HW, but software. And device tree should describe HW.
>>
>> Hmm ... well that's my point exactly. So why call it 'backup'? Sounds
>> like a software description to me.
> 
> Because HW is designed the way that CPU parent need to be switched to the backup clock source while main clock changes its rate. HW also allow to select among different parents, pll_p is one of those parents.

Yes that part is understood. I am just splitting hairs over the actual
name. We do the same for tegra124 but we just call it 'pll_p'. See ...

Documentation/devicetree/bindings/cpufreq/nvidia,tegra124-cpufreq.txt

Cheers
Jon
Dmitry Osipenko Oct. 17, 2018, 1:46 p.m. UTC | #11
On 10/17/18 4:34 PM, Jon Hunter wrote:
> 
> On 17/10/2018 14:07, Dmitry Osipenko wrote:
>> On 10/17/18 3:59 PM, Jon Hunter wrote:
>>>
>>> On 17/10/2018 13:37, Dmitry Osipenko wrote:
>>>> On 10/17/18 11:40 AM, Jon Hunter wrote:
>>>>>
>>>>> On 30/08/2018 20:43, Dmitry Osipenko wrote:
>>>>>> Add device-tree binding that describes CPU frequency-scaling hardware
>>>>>> found on NVIDIA Tegra20/30 SoC's.
>>>>>>
>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>> ---
>>>>>>  .../cpufreq/nvidia,tegra20-cpufreq.txt        | 38 +++++++++++++++++++
>>>>>>  1 file changed, 38 insertions(+)
>>>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>>>> new file mode 100644
>>>>>> index 000000000000..2c51f676e958
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>>>> @@ -0,0 +1,38 @@
>>>>>> +Binding for NVIDIA Tegra20 CPUFreq
>>>>>> +==================================
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- clocks: Must contain an entry for each entry in clock-names.
>>>>>> +  See ../clocks/clock-bindings.txt for details.
>>>>>> +- clock-names: Must include the following entries:
>>>>>> +  - pll_x: main-parent for CPU clock, must be the first entry
>>>>>> +  - backup: intermediate-parent for CPU clock
>>>>>> +  - cpu: the CPU clock
>>>>>
>>>>> Is it likely that 'backup' will be anything other that pll_p? If not why
>>>>> not just call it pll_p? Personally, I don't 'backup' to descriptive even
>>>>> though I can see what you mean.
>>>>>
>>>>> I can see that you want to make this flexible, but if the likelihood is
>>>>> that we will just use pll_p then I am not sure it is warranted at this
>>>>> point.
>>>>
>>>> That won't describe HW, but software. And device tree should describe HW.
>>>
>>> Hmm ... well that's my point exactly. So why call it 'backup'? Sounds
>>> like a software description to me.
>>
>> Because HW is designed the way that CPU parent need to be switched to the backup clock source while main clock changes its rate. HW also allow to select among different parents, pll_p is one of those parents.
> 
> Yes that part is understood. I am just splitting hairs over the actual
> name. We do the same for tegra124 but we just call it 'pll_p'. See ...
> 
> Documentation/devicetree/bindings/cpufreq/nvidia,tegra124-cpufreq.txt

tegra124-cpufreq choose to hardwire to the pll_p, but it could be other clocks. Technically abstracting backup clock should be more correct, but result is the same in the end.
Jon Hunter Oct. 17, 2018, 2:14 p.m. UTC | #12
On 17/10/2018 14:46, Dmitry Osipenko wrote:
> On 10/17/18 4:34 PM, Jon Hunter wrote:
>>
>> On 17/10/2018 14:07, Dmitry Osipenko wrote:
>>> On 10/17/18 3:59 PM, Jon Hunter wrote:
>>>>
>>>> On 17/10/2018 13:37, Dmitry Osipenko wrote:
>>>>> On 10/17/18 11:40 AM, Jon Hunter wrote:
>>>>>>
>>>>>> On 30/08/2018 20:43, Dmitry Osipenko wrote:
>>>>>>> Add device-tree binding that describes CPU frequency-scaling hardware
>>>>>>> found on NVIDIA Tegra20/30 SoC's.
>>>>>>>
>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>> ---
>>>>>>>  .../cpufreq/nvidia,tegra20-cpufreq.txt        | 38 +++++++++++++++++++
>>>>>>>  1 file changed, 38 insertions(+)
>>>>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..2c51f676e958
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>>>>> @@ -0,0 +1,38 @@
>>>>>>> +Binding for NVIDIA Tegra20 CPUFreq
>>>>>>> +==================================
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> +- clocks: Must contain an entry for each entry in clock-names.
>>>>>>> +  See ../clocks/clock-bindings.txt for details.
>>>>>>> +- clock-names: Must include the following entries:
>>>>>>> +  - pll_x: main-parent for CPU clock, must be the first entry
>>>>>>> +  - backup: intermediate-parent for CPU clock
>>>>>>> +  - cpu: the CPU clock
>>>>>>
>>>>>> Is it likely that 'backup' will be anything other that pll_p? If not why
>>>>>> not just call it pll_p? Personally, I don't 'backup' to descriptive even
>>>>>> though I can see what you mean.
>>>>>>
>>>>>> I can see that you want to make this flexible, but if the likelihood is
>>>>>> that we will just use pll_p then I am not sure it is warranted at this
>>>>>> point.
>>>>>
>>>>> That won't describe HW, but software. And device tree should describe HW.
>>>>
>>>> Hmm ... well that's my point exactly. So why call it 'backup'? Sounds
>>>> like a software description to me.
>>>
>>> Because HW is designed the way that CPU parent need to be switched to the backup clock source while main clock changes its rate. HW also allow to select among different parents, pll_p is one of those parents.
>>
>> Yes that part is understood. I am just splitting hairs over the actual
>> name. We do the same for tegra124 but we just call it 'pll_p'. See ...
>>
>> Documentation/devicetree/bindings/cpufreq/nvidia,tegra124-cpufreq.txt
> 
> tegra124-cpufreq choose to hardwire to the pll_p, but it could be other clocks. Technically abstracting backup clock should be more correct, but result is the same in the end.

Yes and that is probably intentional as that is the configuration that
has been validated. So unless we are planning to test and verify every
possibility, my preference is to keep it simple. But yes the result is
the same.

I was simply curious of your intention here because of the other series
you posted with regard to the cpu clocks.

Cheers
Jon
Dmitry Osipenko Oct. 17, 2018, 2:43 p.m. UTC | #13
On 10/17/18 5:14 PM, Jon Hunter wrote:
> 
> On 17/10/2018 14:46, Dmitry Osipenko wrote:
>> On 10/17/18 4:34 PM, Jon Hunter wrote:
>>>
>>> On 17/10/2018 14:07, Dmitry Osipenko wrote:
>>>> On 10/17/18 3:59 PM, Jon Hunter wrote:
>>>>>
>>>>> On 17/10/2018 13:37, Dmitry Osipenko wrote:
>>>>>> On 10/17/18 11:40 AM, Jon Hunter wrote:
>>>>>>>
>>>>>>> On 30/08/2018 20:43, Dmitry Osipenko wrote:
>>>>>>>> Add device-tree binding that describes CPU frequency-scaling hardware
>>>>>>>> found on NVIDIA Tegra20/30 SoC's.
>>>>>>>>
>>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>>> ---
>>>>>>>>  .../cpufreq/nvidia,tegra20-cpufreq.txt        | 38 +++++++++++++++++++
>>>>>>>>  1 file changed, 38 insertions(+)
>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>>>>>> new file mode 100644
>>>>>>>> index 000000000000..2c51f676e958
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>>>>>> @@ -0,0 +1,38 @@
>>>>>>>> +Binding for NVIDIA Tegra20 CPUFreq
>>>>>>>> +==================================
>>>>>>>> +
>>>>>>>> +Required properties:
>>>>>>>> +- clocks: Must contain an entry for each entry in clock-names.
>>>>>>>> +  See ../clocks/clock-bindings.txt for details.
>>>>>>>> +- clock-names: Must include the following entries:
>>>>>>>> +  - pll_x: main-parent for CPU clock, must be the first entry
>>>>>>>> +  - backup: intermediate-parent for CPU clock
>>>>>>>> +  - cpu: the CPU clock
>>>>>>>
>>>>>>> Is it likely that 'backup' will be anything other that pll_p? If not why
>>>>>>> not just call it pll_p? Personally, I don't 'backup' to descriptive even
>>>>>>> though I can see what you mean.
>>>>>>>
>>>>>>> I can see that you want to make this flexible, but if the likelihood is
>>>>>>> that we will just use pll_p then I am not sure it is warranted at this
>>>>>>> point.
>>>>>>
>>>>>> That won't describe HW, but software. And device tree should describe HW.
>>>>>
>>>>> Hmm ... well that's my point exactly. So why call it 'backup'? Sounds
>>>>> like a software description to me.
>>>>
>>>> Because HW is designed the way that CPU parent need to be switched to the backup clock source while main clock changes its rate. HW also allow to select among different parents, pll_p is one of those parents.
>>>
>>> Yes that part is understood. I am just splitting hairs over the actual
>>> name. We do the same for tegra124 but we just call it 'pll_p'. See ...
>>>
>>> Documentation/devicetree/bindings/cpufreq/nvidia,tegra124-cpufreq.txt
>>
>> tegra124-cpufreq choose to hardwire to the pll_p, but it could be other clocks. Technically abstracting backup clock should be more correct, but result is the same in the end.
> 
> Yes and that is probably intentional as that is the configuration that
> has been validated. So unless we are planning to test and verify every
> possibility, my preference is to keep it simple. But yes the result is
> the same.
> 
> I was simply curious of your intention here because of the other series
> you posted with regard to the cpu clocks.

Actually I tried other parents on T30 and they worked with no problems. The intention of having 'backup' is to describe HW properly in the device tree, that's it. We'll have backup set to pll_p by default. ACK?
Jon Hunter Oct. 17, 2018, 7:29 p.m. UTC | #14
On 17/10/2018 15:43, Dmitry Osipenko wrote:
> On 10/17/18 5:14 PM, Jon Hunter wrote:
>>
>> On 17/10/2018 14:46, Dmitry Osipenko wrote:
>>> On 10/17/18 4:34 PM, Jon Hunter wrote:
>>>>
>>>> On 17/10/2018 14:07, Dmitry Osipenko wrote:
>>>>> On 10/17/18 3:59 PM, Jon Hunter wrote:
>>>>>>
>>>>>> On 17/10/2018 13:37, Dmitry Osipenko wrote:
>>>>>>> On 10/17/18 11:40 AM, Jon Hunter wrote:
>>>>>>>>
>>>>>>>> On 30/08/2018 20:43, Dmitry Osipenko wrote:
>>>>>>>>> Add device-tree binding that describes CPU frequency-scaling hardware
>>>>>>>>> found on NVIDIA Tegra20/30 SoC's.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>>>> ---
>>>>>>>>>  .../cpufreq/nvidia,tegra20-cpufreq.txt        | 38 +++++++++++++++++++
>>>>>>>>>  1 file changed, 38 insertions(+)
>>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>>>>>>> new file mode 100644
>>>>>>>>> index 000000000000..2c51f676e958
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>>>>>>> @@ -0,0 +1,38 @@
>>>>>>>>> +Binding for NVIDIA Tegra20 CPUFreq
>>>>>>>>> +==================================
>>>>>>>>> +
>>>>>>>>> +Required properties:
>>>>>>>>> +- clocks: Must contain an entry for each entry in clock-names.
>>>>>>>>> +  See ../clocks/clock-bindings.txt for details.
>>>>>>>>> +- clock-names: Must include the following entries:
>>>>>>>>> +  - pll_x: main-parent for CPU clock, must be the first entry
>>>>>>>>> +  - backup: intermediate-parent for CPU clock
>>>>>>>>> +  - cpu: the CPU clock
>>>>>>>>
>>>>>>>> Is it likely that 'backup' will be anything other that pll_p? If not why
>>>>>>>> not just call it pll_p? Personally, I don't 'backup' to descriptive even
>>>>>>>> though I can see what you mean.
>>>>>>>>
>>>>>>>> I can see that you want to make this flexible, but if the likelihood is
>>>>>>>> that we will just use pll_p then I am not sure it is warranted at this
>>>>>>>> point.
>>>>>>>
>>>>>>> That won't describe HW, but software. And device tree should describe HW.
>>>>>>
>>>>>> Hmm ... well that's my point exactly. So why call it 'backup'? Sounds
>>>>>> like a software description to me.
>>>>>
>>>>> Because HW is designed the way that CPU parent need to be switched to the backup clock source while main clock changes its rate. HW also allow to select among different parents, pll_p is one of those parents.
>>>>
>>>> Yes that part is understood. I am just splitting hairs over the actual
>>>> name. We do the same for tegra124 but we just call it 'pll_p'. See ...
>>>>
>>>> Documentation/devicetree/bindings/cpufreq/nvidia,tegra124-cpufreq.txt
>>>
>>> tegra124-cpufreq choose to hardwire to the pll_p, but it could be other clocks. Technically abstracting backup clock should be more correct, but result is the same in the end.
>>
>> Yes and that is probably intentional as that is the configuration that
>> has been validated. So unless we are planning to test and verify every
>> possibility, my preference is to keep it simple. But yes the result is
>> the same.
>>
>> I was simply curious of your intention here because of the other series
>> you posted with regard to the cpu clocks.
> 
> Actually I tried other parents on T30 and they worked with no problems. The intention of having 'backup' is to describe HW properly in the device tree, that's it. We'll have backup set to pll_p by default. ACK?

Yes but I don't find the name 'backup' very descriptive because like you
say it is an intermediate clock for switching frequency. But at the same
time I don't have a good suggestion. Anyway it is more of a
bike-shedding comment.

Cheers
Jon
Dmitry Osipenko Oct. 17, 2018, 8:57 p.m. UTC | #15
On 10/17/18 10:29 PM, Jon Hunter wrote:
> 
> On 17/10/2018 15:43, Dmitry Osipenko wrote:
>> On 10/17/18 5:14 PM, Jon Hunter wrote:
>>>
>>> On 17/10/2018 14:46, Dmitry Osipenko wrote:
>>>> On 10/17/18 4:34 PM, Jon Hunter wrote:
>>>>>
>>>>> On 17/10/2018 14:07, Dmitry Osipenko wrote:
>>>>>> On 10/17/18 3:59 PM, Jon Hunter wrote:
>>>>>>>
>>>>>>> On 17/10/2018 13:37, Dmitry Osipenko wrote:
>>>>>>>> On 10/17/18 11:40 AM, Jon Hunter wrote:
>>>>>>>>>
>>>>>>>>> On 30/08/2018 20:43, Dmitry Osipenko wrote:
>>>>>>>>>> Add device-tree binding that describes CPU frequency-scaling hardware
>>>>>>>>>> found on NVIDIA Tegra20/30 SoC's.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>>>>> ---
>>>>>>>>>>  .../cpufreq/nvidia,tegra20-cpufreq.txt        | 38 +++++++++++++++++++
>>>>>>>>>>  1 file changed, 38 insertions(+)
>>>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>>>>>>>>
>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>>>>>>>> new file mode 100644
>>>>>>>>>> index 000000000000..2c51f676e958
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
>>>>>>>>>> @@ -0,0 +1,38 @@
>>>>>>>>>> +Binding for NVIDIA Tegra20 CPUFreq
>>>>>>>>>> +==================================
>>>>>>>>>> +
>>>>>>>>>> +Required properties:
>>>>>>>>>> +- clocks: Must contain an entry for each entry in clock-names.
>>>>>>>>>> +  See ../clocks/clock-bindings.txt for details.
>>>>>>>>>> +- clock-names: Must include the following entries:
>>>>>>>>>> +  - pll_x: main-parent for CPU clock, must be the first entry
>>>>>>>>>> +  - backup: intermediate-parent for CPU clock
>>>>>>>>>> +  - cpu: the CPU clock
>>>>>>>>>
>>>>>>>>> Is it likely that 'backup' will be anything other that pll_p? If not why
>>>>>>>>> not just call it pll_p? Personally, I don't 'backup' to descriptive even
>>>>>>>>> though I can see what you mean.
>>>>>>>>>
>>>>>>>>> I can see that you want to make this flexible, but if the likelihood is
>>>>>>>>> that we will just use pll_p then I am not sure it is warranted at this
>>>>>>>>> point.
>>>>>>>>
>>>>>>>> That won't describe HW, but software. And device tree should describe HW.
>>>>>>>
>>>>>>> Hmm ... well that's my point exactly. So why call it 'backup'? Sounds
>>>>>>> like a software description to me.
>>>>>>
>>>>>> Because HW is designed the way that CPU parent need to be switched to the backup clock source while main clock changes its rate. HW also allow to select among different parents, pll_p is one of those parents.
>>>>>
>>>>> Yes that part is understood. I am just splitting hairs over the actual
>>>>> name. We do the same for tegra124 but we just call it 'pll_p'. See ...
>>>>>
>>>>> Documentation/devicetree/bindings/cpufreq/nvidia,tegra124-cpufreq.txt
>>>>
>>>> tegra124-cpufreq choose to hardwire to the pll_p, but it could be other clocks. Technically abstracting backup clock should be more correct, but result is the same in the end.
>>>
>>> Yes and that is probably intentional as that is the configuration that
>>> has been validated. So unless we are planning to test and verify every
>>> possibility, my preference is to keep it simple. But yes the result is
>>> the same.
>>>
>>> I was simply curious of your intention here because of the other series
>>> you posted with regard to the cpu clocks.
>>
>> Actually I tried other parents on T30 and they worked with no problems. The intention of having 'backup' is to describe HW properly in the device tree, that's it. We'll have backup set to pll_p by default. ACK?
> 
> Yes but I don't find the name 'backup' very descriptive because like you
> say it is an intermediate clock for switching frequency. But at the same
> time I don't have a good suggestion. Anyway it is more of a
> bike-shedding comment.

I'll consider renaming 'backup' to 'intermediate' in the v2 which I'll probably manage to send out tomorrow as RFC (CPUFreq now depends on yet-not-reviewed coupled-regulators patches).
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
new file mode 100644
index 000000000000..2c51f676e958
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/nvidia,tegra20-cpufreq.txt
@@ -0,0 +1,38 @@ 
+Binding for NVIDIA Tegra20 CPUFreq
+==================================
+
+Required properties:
+- clocks: Must contain an entry for each entry in clock-names.
+  See ../clocks/clock-bindings.txt for details.
+- clock-names: Must include the following entries:
+  - pll_x: main-parent for CPU clock, must be the first entry
+  - backup: intermediate-parent for CPU clock
+  - cpu: the CPU clock
+- operating-points-v2: See ../bindings/opp/opp.txt for details.
+- #cooling-cells: Should be 2. See ../thermal/thermal.txt for details.
+
+Example:
+	cpu0_opp_table: opp_table0 {
+		compatible = "operating-points-v2";
+
+		opp@216000000 {
+			clock-latency-ns = <300000>;
+			opp-hz = /bits/ 64 <216000000>;
+			opp-microvolt = <7500000>;
+			opp-suspend;
+		};
+
+		...
+	};
+
+	cpus {
+		cpu@0 {
+			compatible = "arm,cortex-a9";
+			clocks = <&tegra_car TEGRA20_CLK_PLL_X>,
+				 <&tegra_car TEGRA20_CLK_PLL_P>,
+				 <&tegra_car TEGRA20_CLK_CCLK>;
+			clock-names = "pll_x", "backup", "cpu";
+			operating-points-v2 = <&cpu0_opp_table>;
+			#cooling-cells = <2>;
+		};
+	};