diff mbox

[2/6] ARM: Tegra: Add CPU's OPPs for using cpufreq-cpu0 driver

Message ID d4cd498d9b63d761039b025216e65278dc8e0e37.1375886595.git.viresh.kumar@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Viresh Kumar Aug. 7, 2013, 2:46 p.m. UTC
cpufreq-cpu0 driver needs OPPs to be present in DT which can be probed by it to
get frequency table. This patch adds OPPs and clock-latency to tegra cpu0 node
for multiple SoCs.

Voltage levels aren't used until now for tegra and so a flat value which would
eventually be ignored is used to represent voltage.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 arch/arm/boot/dts/tegra114.dtsi | 12 ++++++++++++
 arch/arm/boot/dts/tegra20.dtsi  | 12 ++++++++++++
 arch/arm/boot/dts/tegra30.dtsi  | 12 ++++++++++++
 3 files changed, 36 insertions(+)

Comments

Stephen Warren Aug. 7, 2013, 5:42 p.m. UTC | #1
On 08/07/2013 08:46 AM, Viresh Kumar wrote:
> cpufreq-cpu0 driver needs OPPs to be present in DT which can be probed by it to
> get frequency table. This patch adds OPPs and clock-latency to tegra cpu0 node
> for multiple SoCs.
> 
> Voltage levels aren't used until now for tegra and so a flat value which would
> eventually be ignored is used to represent voltage.

This patch is problematic w.r.t. DT being an ABI.

We can certainly add new optional properties to a DT binding that enable
new features. However, a new version of a binding can't require new
properties to exist that didn't before, since that means that old DTs
won't work with new kernels that require the new properties.

As such, I believe we do need some Tegra-specific piece of code that
defines these OPP tables in the kernel, so that the operating-points
property is not needed.

Similarly, we can't put invalid voltages into the DT, since if a later
kernel version starts actually using that field, the HW will no longer
work correctly. Unless perhaps we put 0 into the DT and make the binding
define that 0 means "you can't change the voltage at all away from the
boot value"?

Is the operating-points property documented in
Documentation/devicetree/bindings/ somewhere?

(Also Cc'ing the DT mailing list and maintainers)

> diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
> index abf6c40..730e0d9 100644
> --- a/arch/arm/boot/dts/tegra114.dtsi
> +++ b/arch/arm/boot/dts/tegra114.dtsi
> @@ -438,6 +438,18 @@
>  			device_type = "cpu";
>  			compatible = "arm,cortex-a15";
>  			reg = <0>;
> +			operating-points = <
> +				/* kHz    ignored */
> +				 216000   1000000
> +				 312000   1000000
> +				 456000   1000000
> +				 608000   1000000
> +				 760000   1000000
> +				 816000   1000000
> +				 912000   1000000
> +				 1000000  1000000
> +			>;
> +			clock-latency = <300000>;
>  		};
Viresh Kumar Aug. 7, 2013, 6:06 p.m. UTC | #2
On 7 August 2013 23:12, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/07/2013 08:46 AM, Viresh Kumar wrote:
>> cpufreq-cpu0 driver needs OPPs to be present in DT which can be probed by it to
>> get frequency table. This patch adds OPPs and clock-latency to tegra cpu0 node
>> for multiple SoCs.
>>
>> Voltage levels aren't used until now for tegra and so a flat value which would
>> eventually be ignored is used to represent voltage.
>
> This patch is problematic w.r.t. DT being an ABI.

:(

> We can certainly add new optional properties to a DT binding that enable
> new features. However, a new version of a binding can't require new
> properties to exist that didn't before, since that means that old DTs
> won't work with new kernels that require the new properties.

To be honest I didn't get it completely. You meant operating-points
wasn't present before? Its here:

Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
Documentation/devicetree/bindings/power/opp.txt

Or you meant, Tegra never required voltage levels and we are getting
them in here.

> As such, I believe we do need some Tegra-specific piece of code that
> defines these OPP tables in the kernel, so that the operating-points
> property is not needed.

Generic cpufreq driver depends on OPP library and so somebody has
to provide them. Now you can do it by calling opp_add() for each OPP
you have or otherwise.

Btw, you must have some specific voltage level for each freq, we can
get them here..

> Similarly, we can't put invalid voltages into the DT, since if a later
> kernel version starts actually using that field, the HW will no longer
> work correctly. Unless perhaps we put 0 into the DT and make the binding
> define that 0 means "you can't change the voltage at all away from the
> boot value"?

Hmm.. so if you help me in getting actual voltage levels for these freqs
then this problem will be resolved. Otherwise I can check what will happen
if we pass zero to voltage.

> Is the operating-points property documented in
> Documentation/devicetree/bindings/ somewhere?

Check above.

> (Also Cc'ing the DT mailing list and maintainers)

Thanks.
Stephen Warren Aug. 7, 2013, 6:55 p.m. UTC | #3
On 08/07/2013 12:06 PM, Viresh Kumar wrote:
> On 7 August 2013 23:12, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 08/07/2013 08:46 AM, Viresh Kumar wrote:
>>> cpufreq-cpu0 driver needs OPPs to be present in DT which can be probed by it to
>>> get frequency table. This patch adds OPPs and clock-latency to tegra cpu0 node
>>> for multiple SoCs.
>>>
>>> Voltage levels aren't used until now for tegra and so a flat value which would
>>> eventually be ignored is used to represent voltage.
>>
>> This patch is problematic w.r.t. DT being an ABI.
> 
> :(
> 
>> We can certainly add new optional properties to a DT binding that enable
>> new features. However, a new version of a binding can't require new
>> properties to exist that didn't before, since that means that old DTs
>> won't work with new kernels that require the new properties.
> 
> To be honest I didn't get it completely. You meant operating-points
> wasn't present before? Its here:
> 
> Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
> Documentation/devicetree/bindings/power/opp.txt
> 
> Or you meant, Tegra never required voltage levels and we are getting
> them in here.

The current Tegra *.dts files do not contain this property. The current
Tegra *.dts files must continue to work without modification in future
kernels.

>> As such, I believe we do need some Tegra-specific piece of code that
>> defines these OPP tables in the kernel, so that the operating-points
>> property is not needed.
> 
> Generic cpufreq driver depends on OPP library and so somebody has
> to provide them. Now you can do it by calling opp_add() for each OPP
> you have or otherwise.

Sure. That's what the Tegra-specific cpufreq driver should do. It should
be the top-level cpufreq driver. If parts of the code can be implemented
by library functions or a core parameterizable driver, then presumably
the Tegra driver would simply exist to provide those parameters and/or
callback functions to the generic driver.

> Btw, you must have some specific voltage level for each freq, we can
> get them here..

Yes, I'm sure we do, but I have no idea what they are:-( It may even be
board-specific or SoC-SKU-specific. I think we should defer this aspect
for now.
Viresh Kumar Aug. 8, 2013, 2:57 a.m. UTC | #4
On 8 August 2013 00:25, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/07/2013 12:06 PM, Viresh Kumar wrote:
>> On 7 August 2013 23:12, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 08/07/2013 08:46 AM, Viresh Kumar wrote:
>>>> cpufreq-cpu0 driver needs OPPs to be present in DT which can be probed by it to
>>>> get frequency table. This patch adds OPPs and clock-latency to tegra cpu0 node
>>>> for multiple SoCs.
>>>>
>>>> Voltage levels aren't used until now for tegra and so a flat value which would
>>>> eventually be ignored is used to represent voltage.
>>>
>>> This patch is problematic w.r.t. DT being an ABI.
>>
>> :(
>>
>>> We can certainly add new optional properties to a DT binding that enable
>>> new features. However, a new version of a binding can't require new
>>> properties to exist that didn't before, since that means that old DTs
>>> won't work with new kernels that require the new properties.
>>
>> To be honest I didn't get it completely. You meant operating-points
>> wasn't present before? Its here:
>>
>> Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
>> Documentation/devicetree/bindings/power/opp.txt
>>
>> Or you meant, Tegra never required voltage levels and we are getting
>> them in here.
>
> The current Tegra *.dts files do not contain this property. The current
> Tegra *.dts files must continue to work without modification in future
> kernels.

But that can't be true always.. Specially when we are moving things to
DT...

For example, we are moving your DMA driver to DT and hence in the
platform code, we are making a new DT node + removing static
platform device.

Now, old DT can't work with new kernel... That is just not possible.
That statement might be true for cases where we are just upgrading
existing DT support (but I doubt it there as well :) )..

>>> As such, I believe we do need some Tegra-specific piece of code that
>>> defines these OPP tables in the kernel, so that the operating-points
>>> property is not needed.
>>
>> Generic cpufreq driver depends on OPP library and so somebody has
>> to provide them. Now you can do it by calling opp_add() for each OPP
>> you have or otherwise.
>
> Sure. That's what the Tegra-specific cpufreq driver should do. It should
> be the top-level cpufreq driver. If parts of the code can be implemented
> by library functions or a core parameterizable driver, then presumably
> the Tegra driver would simply exist to provide those parameters and/or
> callback functions to the generic driver.

That would be something similar to what we are discussing on other
thread about new platform device...

You are asking me to go back to platform specific code instead of DT.
When there exists a generic enough way of providing this information
via DT, why should we put this in a driver?


>> Btw, you must have some specific voltage level for each freq, we can
>> get them here..
>
> Yes, I'm sure we do, but I have no idea what they are:-( It may even be
> board-specific or SoC-SKU-specific. I think we should defer this aspect
> for now.

Ok.
Lucas Stach Aug. 8, 2013, 1:58 p.m. UTC | #5
Am Mittwoch, den 07.08.2013, 12:55 -0600 schrieb Stephen Warren:
> On 08/07/2013 12:06 PM, Viresh Kumar wrote:
> > On 7 August 2013 23:12, Stephen Warren <swarren@wwwdotorg.org> wrote:
> >> On 08/07/2013 08:46 AM, Viresh Kumar wrote:
> >>> cpufreq-cpu0 driver needs OPPs to be present in DT which can be probed by it to
> >>> get frequency table. This patch adds OPPs and clock-latency to tegra cpu0 node
> >>> for multiple SoCs.
> >>>
> >>> Voltage levels aren't used until now for tegra and so a flat value which would
> >>> eventually be ignored is used to represent voltage.
> >>
> >> This patch is problematic w.r.t. DT being an ABI.
> > 
> > :(
> > 
> >> We can certainly add new optional properties to a DT binding that enable
> >> new features. However, a new version of a binding can't require new
> >> properties to exist that didn't before, since that means that old DTs
> >> won't work with new kernels that require the new properties.
> > 
> > To be honest I didn't get it completely. You meant operating-points
> > wasn't present before? Its here:
> > 
> > Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
> > Documentation/devicetree/bindings/power/opp.txt
> > 
> > Or you meant, Tegra never required voltage levels and we are getting
> > them in here.
> 
> The current Tegra *.dts files do not contain this property. The current
> Tegra *.dts files must continue to work without modification in future
> kernels.
> 
> >> As such, I believe we do need some Tegra-specific piece of code that
> >> defines these OPP tables in the kernel, so that the operating-points
> >> property is not needed.
> > 
> > Generic cpufreq driver depends on OPP library and so somebody has
> > to provide them. Now you can do it by calling opp_add() for each OPP
> > you have or otherwise.
> 
> Sure. That's what the Tegra-specific cpufreq driver should do. It should
> be the top-level cpufreq driver. If parts of the code can be implemented
> by library functions or a core parameterizable driver, then presumably
> the Tegra driver would simply exist to provide those parameters and/or
> callback functions to the generic driver.
> 
> > Btw, you must have some specific voltage level for each freq, we can
> > get them here..
> 
> Yes, I'm sure we do, but I have no idea what they are:-( It may even be
> board-specific or SoC-SKU-specific. I think we should defer this aspect
> for now.

From what I learned those voltage levels are dependent on both the
Speedo and the process ID of the specific Tegra processor. So you really
get a two dimensional mapping table instead of a single OPP.
Also you can not scale the CPU voltage on it's own, but have to make
sure the core voltage isn't too far away from. Then core voltage also
depends on the operating states of engines like GR2D or even display.

Regards,
Lucas
Viresh Kumar Aug. 8, 2013, 2:11 p.m. UTC | #6
On 8 August 2013 19:28, Lucas Stach <l.stach@pengutronix.de> wrote:
> From what I learned those voltage levels are dependent on both the
> Speedo and the process ID of the specific Tegra processor. So you really
> get a two dimensional mapping table instead of a single OPP.
> Also you can not scale the CPU voltage on it's own, but have to make
> sure the core voltage isn't too far away from. Then core voltage also
> depends on the operating states of engines like GR2D or even display.

So if they depend on a certain type of SoC, which they should, then we
can get these initialized from that SoC's dts/dtsi file instead of a common
file.. And so that would resolve the issue you just reported.

Now I haven't proposed in the patch that we will change these voltage
levels at all.. This is regulator specific code and would come into play
only when regulators are registered for cpu.. Otherwise we will just
play with frequency..

Passing OPP instead of just list of frequencies is the generic way this
is done now a days..
Lucas Stach Aug. 8, 2013, 2:22 p.m. UTC | #7
Am Donnerstag, den 08.08.2013, 19:41 +0530 schrieb Viresh Kumar:
> On 8 August 2013 19:28, Lucas Stach <l.stach@pengutronix.de> wrote:
> > From what I learned those voltage levels are dependent on both the
> > Speedo and the process ID of the specific Tegra processor. So you really
> > get a two dimensional mapping table instead of a single OPP.
> > Also you can not scale the CPU voltage on it's own, but have to make
> > sure the core voltage isn't too far away from. Then core voltage also
> > depends on the operating states of engines like GR2D or even display.
> 
> So if they depend on a certain type of SoC, which they should, then we
> can get these initialized from that SoC's dts/dtsi file instead of a common
> file.. And so that would resolve the issue you just reported.
> 
You can certainly define the mapping table in DT where a specialized
Tegra cpufreq driver could read it in and then map frequency to voltage.
But that's a runtime decision, as Speedo and process ID are fuse values
and can not be represented in DT.

> Now I haven't proposed in the patch that we will change these voltage
> levels at all.. This is regulator specific code and would come into play
> only when regulators are registered for cpu.. Otherwise we will just
> play with frequency..
> 
> Passing OPP instead of just list of frequencies is the generic way this
> is done now a days..

The problem with this is that the hardware description now associates
voltages with certain frequencies and even if they are not used by the
Linux driver they are plain wrong.

Regards,
Lucas
Viresh Kumar Aug. 8, 2013, 2:37 p.m. UTC | #8
On 8 August 2013 19:52, Lucas Stach <l.stach@pengutronix.de> wrote:
> You can certainly define the mapping table in DT where a specialized
> Tegra cpufreq driver could read it in and then map frequency to voltage.
> But that's a runtime decision, as Speedo and process ID are fuse values
> and can not be represented in DT.

> The problem with this is that the hardware description now associates
> voltages with certain frequencies and even if they are not used by the
> Linux driver they are plain wrong.

Hmm. I understand.
Then we probably need mach-tegra/opp.c to call opp_add() for all such
OPPs.. Neither DT nor cpufreq driver are the right place for this.
Nishanth Menon Aug. 8, 2013, 3:52 p.m. UTC | #9
On Thu, Aug 8, 2013 at 9:37 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 8 August 2013 19:52, Lucas Stach <l.stach@pengutronix.de> wrote:
>> You can certainly define the mapping table in DT where a specialized
>> Tegra cpufreq driver could read it in and then map frequency to voltage.
>> But that's a runtime decision, as Speedo and process ID are fuse values
>> and can not be represented in DT.
>
>> The problem with this is that the hardware description now associates
>> voltages with certain frequencies and even if they are not used by the
>> Linux driver they are plain wrong.
>
> Hmm. I understand.
> Then we probably need mach-tegra/opp.c to call opp_add() for all such
> OPPs.. Neither DT nor cpufreq driver are the right place for this.

This is similar to what I suspected might be the case on other
platforms (in addition to known iMx and OMAP). Could you see/comment
on [1] to see if it meets your needs.

We should like to avoid dealing custom SoC specific OPP, if we are
able to generalize the need.  ofcourse, I am yet to submit a official
proposal, but more SoCs the current proposal can handle, the better it
will be for all of us.

[1] http://marc.info/?l=linux-pm&m=137589225305971&w=2
--
Regards,
Nishanth Menon
Stephen Warren Aug. 8, 2013, 6:55 p.m. UTC | #10
On 08/07/2013 08:57 PM, Viresh Kumar wrote:
> On 8 August 2013 00:25, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 08/07/2013 12:06 PM, Viresh Kumar wrote:
>>> On 7 August 2013 23:12, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>> On 08/07/2013 08:46 AM, Viresh Kumar wrote:
>>>>> cpufreq-cpu0 driver needs OPPs to be present in DT which can be probed by it to
>>>>> get frequency table. This patch adds OPPs and clock-latency to tegra cpu0 node
>>>>> for multiple SoCs.
>>>>>
>>>>> Voltage levels aren't used until now for tegra and so a flat value which would
>>>>> eventually be ignored is used to represent voltage.
>>>>
>>>> This patch is problematic w.r.t. DT being an ABI.
>>>
>>> :(
>>>
>>>> We can certainly add new optional properties to a DT binding that enable
>>>> new features. However, a new version of a binding can't require new
>>>> properties to exist that didn't before, since that means that old DTs
>>>> won't work with new kernels that require the new properties.
>>>
>>> To be honest I didn't get it completely. You meant operating-points
>>> wasn't present before? Its here:
>>>
>>> Documentation/devicetree/bindings/cpufreq/cpufreq-cpu0.txt
>>> Documentation/devicetree/bindings/power/opp.txt
>>>
>>> Or you meant, Tegra never required voltage levels and we are getting
>>> them in here.
>>
>> The current Tegra *.dts files do not contain this property. The current
>> Tegra *.dts files must continue to work without modification in future
>> kernels.
> 
> But that can't be true always.. Specially when we are moving things to
> DT...

It has to be, or DT isn't an ABI.

Since DT is defined as being an ABI, it has to be true.

The solution here that allows DT to be an ABI is to be the data into the
drivers (or core SoC support code) rather than DT.

> For example, we are moving your DMA driver to DT and hence in the
> platform code, we are making a new DT node + removing static
> platform device.
> 
> Now, old DT can't work with new kernel... That is just not possible.
> That statement might be true for cases where we are just upgrading
> existing DT support (but I doubt it there as well :) )..

Well yes, converting existing platforms to DT piece-meal was probably a
mistake in retrospect. What we should have done is added parallel DT and
non-DT support, and only allow features to be enabled when booting DT if
they were triggered by DT nodes, and never allowed additional drivers to
be registered by board files.

Your point is indeed why suddenly deciding that DT is an ABI when it
wasn't being enforced before is painful.

>>>> As such, I believe we do need some Tegra-specific piece of code that
>>>> defines these OPP tables in the kernel, so that the operating-points
>>>> property is not needed.
>>>
>>> Generic cpufreq driver depends on OPP library and so somebody has
>>> to provide them. Now you can do it by calling opp_add() for each OPP
>>> you have or otherwise.
>>
>> Sure. That's what the Tegra-specific cpufreq driver should do. It should
>> be the top-level cpufreq driver. If parts of the code can be implemented
>> by library functions or a core parameterizable driver, then presumably
>> the Tegra driver would simply exist to provide those parameters and/or
>> callback functions to the generic driver.
> 
> That would be something similar to what we are discussing on other
> thread about new platform device...
> 
> You are asking me to go back to platform specific code instead of DT.
> When there exists a generic enough way of providing this information
> via DT, why should we put this in a driver?

I think that drivers should include all data that doesn't need to vary;
there's no point putting data into DT just to parse it out into the same
tables that the driver could have embedded itself from the start.
diff mbox

Patch

diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
index abf6c40..730e0d9 100644
--- a/arch/arm/boot/dts/tegra114.dtsi
+++ b/arch/arm/boot/dts/tegra114.dtsi
@@ -438,6 +438,18 @@ 
 			device_type = "cpu";
 			compatible = "arm,cortex-a15";
 			reg = <0>;
+			operating-points = <
+				/* kHz    ignored */
+				 216000   1000000
+				 312000   1000000
+				 456000   1000000
+				 608000   1000000
+				 760000   1000000
+				 816000   1000000
+				 912000   1000000
+				 1000000  1000000
+			>;
+			clock-latency = <300000>;
 		};
 
 		cpu@1 {
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 9653fd8..5696f98 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -577,6 +577,18 @@ 
 			device_type = "cpu";
 			compatible = "arm,cortex-a9";
 			reg = <0>;
+			operating-points = <
+				/* kHz    ignored */
+				 216000   1000000
+				 312000   1000000
+				 456000   1000000
+				 608000   1000000
+				 760000   1000000
+				 816000   1000000
+				 912000   1000000
+				 1000000  1000000
+			>;
+			clock-latency = <300000>;
 		};
 
 		cpu@1 {
diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index d8783f0..5930290 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -569,6 +569,18 @@ 
 			device_type = "cpu";
 			compatible = "arm,cortex-a9";
 			reg = <0>;
+			operating-points = <
+				/* kHz    ignored */
+				 216000   1000000
+				 312000   1000000
+				 456000   1000000
+				 608000   1000000
+				 760000   1000000
+				 816000   1000000
+				 912000   1000000
+				 1000000  1000000
+			>;
+			clock-latency = <300000>;
 		};
 
 		cpu@1 {