diff mbox

[v2,1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings

Message ID 1526751291-17873-2-git-send-email-tdas@codeaurora.org (mailing list archive)
State Changes Requested, archived
Headers show

Commit Message

Taniya Das May 19, 2018, 5:34 p.m. UTC
Add QCOM cpufreq firmware device bindings for Qualcomm Technology Inc's
SoCs. This is required for managing the cpu frequency transitions which are
controlled by firmware.

Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 .../bindings/cpufreq/cpufreq-qcom-fw.txt           | 68 ++++++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt

--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

Comments

Rob Herring (Arm) May 22, 2018, 7:31 p.m. UTC | #1
On Sat, May 19, 2018 at 11:04:50PM +0530, Taniya Das wrote:
> Add QCOM cpufreq firmware device bindings for Qualcomm Technology Inc's
> SoCs. This is required for managing the cpu frequency transitions which are
> controlled by firmware.
> 
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>  .../bindings/cpufreq/cpufreq-qcom-fw.txt           | 68 ++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
> new file mode 100644
> index 0000000..bc912f4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
> @@ -0,0 +1,68 @@
> +Qualcomm Technologies, Inc. CPUFREQ Bindings
> +
> +CPUFREQ FW is a hardware engine used by some Qualcomm Technologies, Inc. (QTI)
> +SoCs to manage frequency in hardware. It is capable of controlling frequency
> +for multiple clusters.
> +
> +Properties:
> +- compatible
> +	Usage:		required
> +	Value type:	<string>
> +	Definition:	must be "qcom,cpufreq-fw".

Only 1 version ever?

> +
> +Note that #address-cells, #size-cells, and ranges shall be present to ensure
> +the cpufreq can address a freq-domain registers.
> +
> +A freq-domain sub-node would be defined for the cpus with the following
> +properties:
> +
> +- compatible:
> +	Usage:		required
> +	Value type:	<string>
> +	Definition:	must be "cpufreq".

Too generic. This is very much a QCom specific binding. Perhaps just 
drop the compatible.

> +
> +- reg
> +	Usage:		required
> +	Value type:	<prop-encoded-array>
> +	Definition:	Addresses and sizes for the memory of the perf_base
> +			, lut_base and en_base.
> +- reg-names
> +	Usage:		required
> +	Value type:	<stringlist>
> +	Definition:	Address names. Must be "perf_base", "lut_base",
> +			"en_base".

_base is redundant.

> +			Must be specified in the same order as the
> +			corresponding addresses are specified in the reg
> +			property.

Actually, must be specified in the order you specify here.

> +
> +- qcom,cpulist
> +	Usage:		required
> +	Value type:	<phandles of CPU>
> +	Definition:	List of related cpu handles which are under a cluster.
> +
> +Example:
> +	qcom,cpufreq-fw {
> +		compatible = "qcom,cpufreq-fw";
> +
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;

There's not a smaller address range you can restrict children to?

> +
> +		freq-domain-0 {
> +			compatible = "cpufreq";
> +			reg = <0x17d43920 0x4>,
> +			     <0x17d43110 0x500>,
> +			     <0x17d41000 0x4>;
> +			reg-names = "perf_base", "lut_base", "en_base";
> +			qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3>;
> +		};
> +
> +		freq-domain-1 {
> +			compatible = "cpufreq";
> +			reg = <0x17d46120 0x4>,
> +			    <0x17d45910 0x500>,
> +			    <0x17d45800 0x4>;
> +			reg-names = "perf_base", "lut_base", "en_base";
> +			qcom,cpulist = <&CPU4 &CPU5 &CPU6 &CPU7>;
> +		};
> +	};
> --
> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
> of the Code Aurora Forum, hosted by the  Linux Foundation.
>
Viresh Kumar May 23, 2018, 5:48 a.m. UTC | #2
On 22-05-18, 14:31, Rob Herring wrote:
> On Sat, May 19, 2018 at 11:04:50PM +0530, Taniya Das wrote:
> > +		freq-domain-0 {
> > +			compatible = "cpufreq";
> > +			reg = <0x17d43920 0x4>,
> > +			     <0x17d43110 0x500>,
> > +			     <0x17d41000 0x4>;
> > +			reg-names = "perf_base", "lut_base", "en_base";
> > +			qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3>;

I was thinking, can't we add platform specific properties in the CPU
nodes ? If yes, then we can point the phandle of fw node from the CPUs
and this awkward list can go away.
Rob Herring (Arm) May 23, 2018, 2:18 p.m. UTC | #3
On Wed, May 23, 2018 at 12:48 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 22-05-18, 14:31, Rob Herring wrote:
>> On Sat, May 19, 2018 at 11:04:50PM +0530, Taniya Das wrote:
>> > +           freq-domain-0 {
>> > +                   compatible = "cpufreq";
>> > +                   reg = <0x17d43920 0x4>,
>> > +                        <0x17d43110 0x500>,
>> > +                        <0x17d41000 0x4>;
>> > +                   reg-names = "perf_base", "lut_base", "en_base";
>> > +                   qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3>;
>
> I was thinking, can't we add platform specific properties in the CPU
> nodes ? If yes, then we can point the phandle of fw node from the CPUs
> and this awkward list can go away.

Yes, that's fine. That would be more like OPP binding in that the CPU
points to the OPP table rather than the OPP pointing to the CPUs.

With that, you can get rid of the child nodes completely. Just make
the parent reg property N sets of 3 addresses for N domains.

Rob
Sudeep Holla May 23, 2018, 3:13 p.m. UTC | #4
On 19/05/18 18:34, Taniya Das wrote:
> Add QCOM cpufreq firmware device bindings for Qualcomm Technology Inc's
> SoCs. This is required for managing the cpu frequency transitions which are
> controlled by firmware.
> 
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>  .../bindings/cpufreq/cpufreq-qcom-fw.txt           | 68 ++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
> new file mode 100644
> index 0000000..bc912f4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
> @@ -0,0 +1,68 @@
> +Qualcomm Technologies, Inc. CPUFREQ Bindings
> +
> +CPUFREQ FW is a hardware engine used by some Qualcomm Technologies, Inc. (QTI)
> +SoCs to manage frequency in hardware. It is capable of controlling frequency
> +for multiple clusters.
> +
> +Properties:
> +- compatible
> +	Usage:		required
> +	Value type:	<string>
> +	Definition:	must be "qcom,cpufreq-fw".
> +

If the firmware is referred with some other name, better to use that
than cpufreq
> +Note that #address-cells, #size-cells, and ranges shall be present to ensure
> +the cpufreq can address a freq-domain registers.
> +
> +A freq-domain sub-node would be defined for the cpus with the following
> +properties:
> +
> +- compatible:
> +	Usage:		required
> +	Value type:	<string>
> +	Definition:	must be "cpufreq".
> +
> +- reg
> +	Usage:		required
> +	Value type:	<prop-encoded-array>
> +	Definition:	Addresses and sizes for the memory of the perf_base
> +			, lut_base and en_base.

Can you explicitly define each one of them ? Either here or in reg-names.

> +- reg-names
> +	Usage:		required
> +	Value type:	<stringlist>
> +	Definition:	Address names. Must be "perf_base", "lut_base",
> +			"en_base".
> +			Must be specified in the same order as the
> +			corresponding addresses are specified in the reg
> +			property.
> +
> +- qcom,cpulist
> +	Usage:		required
> +	Value type:	<phandles of CPU>
> +	Definition:	List of related cpu handles which are under a cluster.
> +

As already mentioned by Rob and Viresh, better to align with OPP style
to avoid phandle list of CPUs.

Also I see similar bindings for devfreq, can't they be aligned ?
E.g. lut_base here while it's ftbl_base in devfreq.
Taniya Das May 24, 2018, 5:18 a.m. UTC | #5
Hello Rob, Viresh,

Thank you for the comments. If I understand correctly, the device tree 
nodes should look something like the below.

Though I am not sure if any vendor name could be associated in the cpu 
nodes.

Please suggest if my understanding is wrong.

cpu@0 {
	qcom,freq-domain = <&freq_domain_table0>;
	…
};

same follows for cpu 1/2/3

cpu@400 {
	qcom,freq-domain = <&freq_domain_table1>;
	…
};
same follows for cpu 5/6/7

freq_domain_table0 : freq_table {
	reg = < >, < >, < > ;
	reg-names = "perf_base", "lut_base", "en_base";
};

freq_domain_table1 : freq_table {
	reg = < >, < >, < > ;
	reg-names = "perf_base", "lut_base", "en_base";
};



On 5/23/2018 7:48 PM, Rob Herring wrote:
> On Wed, May 23, 2018 at 12:48 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> On 22-05-18, 14:31, Rob Herring wrote:
>>> On Sat, May 19, 2018 at 11:04:50PM +0530, Taniya Das wrote:
>>>> +           freq-domain-0 {
>>>> +                   compatible = "cpufreq";
>>>> +                   reg = <0x17d43920 0x4>,
>>>> +                        <0x17d43110 0x500>,
>>>> +                        <0x17d41000 0x4>;
>>>> +                   reg-names = "perf_base", "lut_base", "en_base";
>>>> +                   qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3>;
>>
>> I was thinking, can't we add platform specific properties in the CPU
>> nodes ? If yes, then we can point the phandle of fw node from the CPUs
>> and this awkward list can go away.
> 
> Yes, that's fine. That would be more like OPP binding in that the CPU
> points to the OPP table rather than the OPP pointing to the CPUs.
> 
> With that, you can get rid of the child nodes completely. Just make
> the parent reg property N sets of 3 addresses for N domains.
> 
> Rob
>
Viresh Kumar May 24, 2018, 5:43 a.m. UTC | #6
On 24-05-18, 10:48, Taniya Das wrote:
> Hello Rob, Viresh,
> 
> Thank you for the comments. If I understand correctly, the device tree nodes
> should look something like the below.
> 
> Though I am not sure if any vendor name could be associated in the cpu
> nodes.

Well Rob said Yes to that I think.

> Please suggest if my understanding is wrong.
> 
> cpu@0 {
> 	qcom,freq-domain = <&freq_domain_table0>;
> 	…
> };
> 
> same follows for cpu 1/2/3
> 
> cpu@400 {
> 	qcom,freq-domain = <&freq_domain_table1>;
> 	…
> };
> same follows for cpu 5/6/7
> 
> freq_domain_table0 : freq_table {
> 	reg = < >, < >, < > ;
> 	reg-names = "perf_base", "lut_base", "en_base";
> };
> 
> freq_domain_table1 : freq_table {
> 	reg = < >, < >, < > ;
> 	reg-names = "perf_base", "lut_base", "en_base";
> };

Mostly yes.
Taniya Das May 24, 2018, 5:46 a.m. UTC | #7
On 5/23/2018 8:43 PM, Sudeep Holla wrote:
> 
> 
> On 19/05/18 18:34, Taniya Das wrote:
>> Add QCOM cpufreq firmware device bindings for Qualcomm Technology Inc's
>> SoCs. This is required for managing the cpu frequency transitions which are
>> controlled by firmware.
>>
>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>> ---
>>   .../bindings/cpufreq/cpufreq-qcom-fw.txt           | 68 ++++++++++++++++++++++
>>   1 file changed, 68 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
>>
>> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
>> new file mode 100644
>> index 0000000..bc912f4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
>> @@ -0,0 +1,68 @@
>> +Qualcomm Technologies, Inc. CPUFREQ Bindings
>> +
>> +CPUFREQ FW is a hardware engine used by some Qualcomm Technologies, Inc. (QTI)
>> +SoCs to manage frequency in hardware. It is capable of controlling frequency
>> +for multiple clusters.
>> +
>> +Properties:
>> +- compatible
>> +	Usage:		required
>> +	Value type:	<string>
>> +	Definition:	must be "qcom,cpufreq-fw".
>> +
> 
> If the firmware is referred with some other name, better to use that
> than cpufreq
>> +Note that #address-cells, #size-cells, and ranges shall be present to ensure
>> +the cpufreq can address a freq-domain registers.
>> +
>> +A freq-domain sub-node would be defined for the cpus with the following
>> +properties:
>> +
>> +- compatible:
>> +	Usage:		required
>> +	Value type:	<string>
>> +	Definition:	must be "cpufreq".
>> +
>> +- reg
>> +	Usage:		required
>> +	Value type:	<prop-encoded-array>
>> +	Definition:	Addresses and sizes for the memory of the perf_base
>> +			, lut_base and en_base.
> 
> Can you explicitly define each one of them ? Either here or in reg-names.
> 

Sure will define each of them in the next series.

>> +- reg-names
>> +	Usage:		required
>> +	Value type:	<stringlist>
>> +	Definition:	Address names. Must be "perf_base", "lut_base",
>> +			"en_base".
>> +			Must be specified in the same order as the
>> +			corresponding addresses are specified in the reg
>> +			property.
>> +
>> +- qcom,cpulist
>> +	Usage:		required
>> +	Value type:	<phandles of CPU>
>> +	Definition:	List of related cpu handles which are under a cluster.
>> +
> 
> As already mentioned by Rob and Viresh, better to align with OPP style
> to avoid phandle list of CPUs.
> 

I have put down an example device tree node for Rob & Viresh to review.

> Also I see similar bindings for devfreq, can't they be aligned ?
> E.g. lut_base here while it's ftbl_base in devfreq.
> 

It is actually the look up table, thus was the name given as "lut". Will 
check with Saravana for devfreq and align accordingly.
Taniya Das May 28, 2018, 7:29 a.m. UTC | #8
Hello Rob,

Could you please suggest if the below looks okay to be implemented?

On 5/24/2018 11:13 AM, Viresh Kumar wrote:
> On 24-05-18, 10:48, Taniya Das wrote:
>> Hello Rob, Viresh,
>>
>> Thank you for the comments. If I understand correctly, the device tree nodes
>> should look something like the below.
>>
>> Though I am not sure if any vendor name could be associated in the cpu
>> nodes.
> 
> Well Rob said Yes to that I think.
> 
>> Please suggest if my understanding is wrong.
>>
>> cpu@0 {
>> 	qcom,freq-domain = <&freq_domain_table0>;
>> 	…
>> };
>>
>> same follows for cpu 1/2/3
>>
>> cpu@400 {
>> 	qcom,freq-domain = <&freq_domain_table1>;
>> 	…
>> };
>> same follows for cpu 5/6/7
>>
>> freq_domain_table0 : freq_table {
>> 	reg = < >, < >, < > ;
>> 	reg-names = "perf_base", "lut_base", "en_base";
>> };
>>
>> freq_domain_table1 : freq_table {
>> 	reg = < >, < >, < > ;
>> 	reg-names = "perf_base", "lut_base", "en_base";
>> };
> 
> Mostly yes.
>
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
new file mode 100644
index 0000000..bc912f4
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
@@ -0,0 +1,68 @@ 
+Qualcomm Technologies, Inc. CPUFREQ Bindings
+
+CPUFREQ FW is a hardware engine used by some Qualcomm Technologies, Inc. (QTI)
+SoCs to manage frequency in hardware. It is capable of controlling frequency
+for multiple clusters.
+
+Properties:
+- compatible
+	Usage:		required
+	Value type:	<string>
+	Definition:	must be "qcom,cpufreq-fw".
+
+Note that #address-cells, #size-cells, and ranges shall be present to ensure
+the cpufreq can address a freq-domain registers.
+
+A freq-domain sub-node would be defined for the cpus with the following
+properties:
+
+- compatible:
+	Usage:		required
+	Value type:	<string>
+	Definition:	must be "cpufreq".
+
+- reg
+	Usage:		required
+	Value type:	<prop-encoded-array>
+	Definition:	Addresses and sizes for the memory of the perf_base
+			, lut_base and en_base.
+- reg-names
+	Usage:		required
+	Value type:	<stringlist>
+	Definition:	Address names. Must be "perf_base", "lut_base",
+			"en_base".
+			Must be specified in the same order as the
+			corresponding addresses are specified in the reg
+			property.
+
+- qcom,cpulist
+	Usage:		required
+	Value type:	<phandles of CPU>
+	Definition:	List of related cpu handles which are under a cluster.
+
+Example:
+	qcom,cpufreq-fw {
+		compatible = "qcom,cpufreq-fw";
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+
+		freq-domain-0 {
+			compatible = "cpufreq";
+			reg = <0x17d43920 0x4>,
+			     <0x17d43110 0x500>,
+			     <0x17d41000 0x4>;
+			reg-names = "perf_base", "lut_base", "en_base";
+			qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3>;
+		};
+
+		freq-domain-1 {
+			compatible = "cpufreq";
+			reg = <0x17d46120 0x4>,
+			    <0x17d45910 0x500>,
+			    <0x17d45800 0x4>;
+			reg-names = "perf_base", "lut_base", "en_base";
+			qcom,cpulist = <&CPU4 &CPU5 &CPU6 &CPU7>;
+		};
+	};