| Message ID | 20180830194356.14059-2-digetx@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | CPUFREQ OPP's and Tegra30 support by tegra20-cpufreq driver | expand |
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
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.
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.
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.
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
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.
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.
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
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
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
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.
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
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?
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
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 --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>; + }; + };
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