[PATCH v3 0/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

[PATCH v3 0/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

[Taniya Das]

 [v3]
  * Remove index check from 'qcom_cpufreq_fw_target_index'.
  * Update the Documentation binding to add the platform specific properties in
    the CPU nodes, node name "qcom,freq-domain".
  * Update return value to '0' from -ENODEV from 'qcom_cpufreq_fw_get'.
  * Update the logic for boost frequency to use local variables instead of
    cpufreq driver data in 'qcom_read_lut'.
  * Update the logic in 'qcom_get_related_cpus' to find the related cpus.
  * Update the reg-names to remove "_base" and also update the binding with the
    description of these registers.
  * Update the logic in 'qcom_resources_init' to address the new device tree
    notation of handling the frequency domain phandles.

 [v2]
 * Fixed the alignment issues in "qcom_cpufreq_fw_target_index" for dev_err and
   also for "qcom_cpu_resources_init".
 * Removed ret = 0 from qcom_get_related_cpus and added to check for
   cpu_mask_empty to return -ENOENT.
 * Fixes qcom_cpu_resources_init function
   * Remove initialization of 'index'
   * Check for valid 'c'
 * Removed initialization of 'prev_cc' from 'qcom_read_lut'.
 * Remove initialization of 'ret' from function qcom_resources_init and add
   return -ENODEV based on 'of_get_available_child_count'.
 * Removed initialization of 'rc' from qcom_cpufreq_fw_driver_probe
 * Removed module_exit as this driver would not be used as module, also updated
   the Kconfig to bool from tristate.
 * Updated the subsystem in device tree bindings.

 [v1]
   * Fixed compilation reported by Amit K.

Taniya Das (2):
  dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings
  cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

 .../bindings/cpufreq/cpufreq-qcom-fw.txt           | 173 +++++++++++
 drivers/cpufreq/Kconfig.arm                        |   9 +
 drivers/cpufreq/Makefile                           |   1 +
 drivers/cpufreq/qcom-cpufreq-fw.c                  | 318 +++++++++++++++++++++
 4 files changed, 501 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
 create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c

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

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

[Taniya Das]

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           | 173 +++++++++++++++++++++
 1 file changed, 173 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..e3087ec
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
@@ -0,0 +1,173 @@
+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".
+
+* Property qcom,freq-domain
+Devices supporting freq-domain must set their "qcom,freq-domain" property with
+phandle to a freq_domain_table in their DT node.
+
+* Frequency Domain Table Node
+
+This describes the frequency domain belonging to a device.
+This node can have following properties:
+
+- reg
+	Usage:		required
+	Value type:	<prop-encoded-array>
+	Definition:	Addresses and sizes for the memory of the perf
+			, lut and enable bases.
+			perf - indicates the base address for the desired
+			performance state to be set.
+			lut - indicates the look up table base address for the
+			cpufreq	driver to read frequencies.
+			enable - indicates the enable register for firmware.
+- reg-names
+	Usage:		required
+	Value type:	<stringlist>
+	Definition:	Address names. Must be "perf", "lut", "enable".
+			Must be specified in the same order as the reg property.
+
+Example:
+
+Example 1: Dual-cluster, Quad-core per cluster. CPUs within a cluster switch
+DCVS state together.
+
+/ {
+	cpus {
+		#address-cells = <2>;
+		#size-cells = <0>;
+
+		CPU0: cpu@0 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x0>;
+			enable-method = "psci";
+			next-level-cache = <&L2_0>;
+			qcom,freq-domain = <&freq_domain_table0>;
+			L2_0: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+				L3_0: l3-cache {
+				      compatible = "cache";
+				};
+			};
+		};
+
+		CPU1: cpu@100 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x100>;
+			enable-method = "psci";
+			next-level-cache = <&L2_100>;
+			qcom,freq-domain = <&freq_domain_table0>;
+			L2_100: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU2: cpu@200 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x200>;
+			enable-method = "psci";
+			next-level-cache = <&L2_200>;
+			qcom,freq-domain = <&freq_domain_table0>;
+			L2_200: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU3: cpu@300 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x300>;
+			enable-method = "psci";
+			next-level-cache = <&L2_300>;
+			qcom,freq-domain = <&freq_domain_table0>;
+			L2_300: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU4: cpu@400 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x400>;
+			enable-method = "psci";
+			next-level-cache = <&L2_400>;
+			qcom,freq-domain = <&freq_domain_table1>;
+			L2_400: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU5: cpu@500 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x500>;
+			enable-method = "psci";
+			next-level-cache = <&L2_500>;
+			qcom,freq-domain = <&freq_domain_table1>;
+			L2_500: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU6: cpu@600 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x600>;
+			enable-method = "psci";
+			next-level-cache = <&L2_600>;
+			qcom,freq-domain = <&freq_domain_table1>;
+			L2_600: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+
+		CPU7: cpu@700 {
+			device_type = "cpu";
+			compatible = "qcom,kryo385";
+			reg = <0x0 0x700>;
+			enable-method = "psci";
+			next-level-cache = <&L2_700>;
+			qcom,freq-domain = <&freq_domain_table1>;
+			L2_700: l2-cache {
+				compatible = "cache";
+				next-level-cache = <&L3_0>;
+			};
+		};
+	};
+
+	qcom,cpufreq-fw {
+		compatible = "qcom,cpufreq-fw";
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		freq_domain_table0 : freq_table0 {
+			reg = <0x17d43920 0x4>, <0x17d43110 0x500>,
+				 <0x17d41000 0x4>;
+			reg-names = "perf", "lut", "enable";
+		};
+
+		freq_domain_table1 : freq_table1 {
+			reg = <0x17d46120 0x4>, <0x17d45910 0x500>,
+				<0x17d45800 0x4> ;
+			reg-names = "perf", "lut", "enable";
+		};
+	};
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

Re: [PATCH 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings

[Sudeep Holla]

On Mon, Jun 04, 2018 at 04:16:33PM +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           | 173 +++++++++++++++++++++
>  1 file changed, 173 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..e3087ec
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
> @@ -0,0 +1,173 @@
> +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".
> +
> +* Property qcom,freq-domain
> +Devices supporting freq-domain must set their "qcom,freq-domain" property with
> +phandle to a freq_domain_table in their DT node.
> +
> +* Frequency Domain Table Node
> +
> +This describes the frequency domain belonging to a device.
> +This node can have following properties:
> +
> +- reg
> +	Usage:		required
> +	Value type:	<prop-encoded-array>
> +	Definition:	Addresses and sizes for the memory of the perf
> +			, lut and enable bases.
> +			perf - indicates the base address for the desired
> +			performance state to be set.
> +			lut - indicates the look up table base address for the
> +			cpufreq	driver to read frequencies.
> +			enable - indicates the enable register for firmware.
> +- reg-names
> +	Usage:		required
> +	Value type:	<stringlist>
> +	Definition:	Address names. Must be "perf", "lut", "enable".
> +			Must be specified in the same order as the reg property.
> +

[...]

> +
> +	qcom,cpufreq-fw {
> +		compatible = "qcom,cpufreq-fw";
> +
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +
> +		freq_domain_table0 : freq_table0 {
> +			reg = <0x17d43920 0x4>, <0x17d43110 0x500>,
> +				 <0x17d41000 0x4>;

Are "perf", "lut", "enable" registers part of single IP block / share memory ?
I am just trying to understand the reason for separate entries in this fashion
as part of DT register property. I am wondering if there will be multiple
entries that fall with the page size.

--
Regards,
Sudeep

Re: [PATCH 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings

[Taniya Das]

Hello Sudeep,

Thanks for the review comments.

On 6/4/2018 4:25 PM, Sudeep Holla wrote:
> On Mon, Jun 04, 2018 at 04:16:33PM +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           | 173 +++++++++++++++++++++
>>   1 file changed, 173 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..e3087ec
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
>> @@ -0,0 +1,173 @@
>> +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".
>> +
>> +* Property qcom,freq-domain
>> +Devices supporting freq-domain must set their "qcom,freq-domain" property with
>> +phandle to a freq_domain_table in their DT node.
>> +
>> +* Frequency Domain Table Node
>> +
>> +This describes the frequency domain belonging to a device.
>> +This node can have following properties:
>> +
>> +- reg
>> +	Usage:		required
>> +	Value type:	<prop-encoded-array>
>> +	Definition:	Addresses and sizes for the memory of the perf
>> +			, lut and enable bases.
>> +			perf - indicates the base address for the desired
>> +			performance state to be set.
>> +			lut - indicates the look up table base address for the
>> +			cpufreq	driver to read frequencies.
>> +			enable - indicates the enable register for firmware.
>> +- reg-names
>> +	Usage:		required
>> +	Value type:	<stringlist>
>> +	Definition:	Address names. Must be "perf", "lut", "enable".
>> +			Must be specified in the same order as the reg property.
>> +
> 
> [...]
> 
>> +
>> +	qcom,cpufreq-fw {
>> +		compatible = "qcom,cpufreq-fw";
>> +
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +
>> +		freq_domain_table0 : freq_table0 {
>> +			reg = <0x17d43920 0x4>, <0x17d43110 0x500>,
>> +				 <0x17d41000 0x4>;
> 
> Are "perf", "lut", "enable" registers part of single IP block / share memory ?
> I am just trying to understand the reason for separate entries in this fashion
> as part of DT register property. I am wondering if there will be multiple
> entries that fall with the page size.
> 

They are part of the same IP block and these are the only register 
offsets which is required to be accessed by HLOS.

> --
> Regards,
> Sudeep
> 

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

--

Re: [PATCH 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings

[Sudeep Holla]

On 07/06/18 08:20, Taniya Das wrote:
> Hello Sudeep,
> 
> Thanks for the review comments.
> 
> On 6/4/2018 4:25 PM, Sudeep Holla wrote:
>> On Mon, Jun 04, 2018 at 04:16:33PM +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           | 173
>>> +++++++++++++++++++++
>>>   1 file changed, 173 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..e3087ec
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
>>> @@ -0,0 +1,173 @@
>>> +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".
>>> +
>>> +* Property qcom,freq-domain
>>> +Devices supporting freq-domain must set their "qcom,freq-domain"
>>> property with
>>> +phandle to a freq_domain_table in their DT node.
>>> +
>>> +* Frequency Domain Table Node
>>> +
>>> +This describes the frequency domain belonging to a device.
>>> +This node can have following properties:
>>> +
>>> +- reg
>>> +    Usage:        required
>>> +    Value type:    <prop-encoded-array>
>>> +    Definition:    Addresses and sizes for the memory of the perf
>>> +            , lut and enable bases.
>>> +            perf - indicates the base address for the desired
>>> +            performance state to be set.
>>> +            lut - indicates the look up table base address for the
>>> +            cpufreq    driver to read frequencies.
>>> +            enable - indicates the enable register for firmware.
>>> +- reg-names
>>> +    Usage:        required
>>> +    Value type:    <stringlist>
>>> +    Definition:    Address names. Must be "perf", "lut", "enable".
>>> +            Must be specified in the same order as the reg property.
>>> +
>>
>> [...]
>>
>>> +
>>> +    qcom,cpufreq-fw {
>>> +        compatible = "qcom,cpufreq-fw";
>>> +
>>> +        #address-cells = <1>;
>>> +        #size-cells = <1>;
>>> +
>>> +        freq_domain_table0 : freq_table0 {
>>> +            reg = <0x17d43920 0x4>, <0x17d43110 0x500>,
>>> +                 <0x17d41000 0x4>;
>>
>> Are "perf", "lut", "enable" registers part of single IP block / share
>> memory ?
>> I am just trying to understand the reason for separate entries in this
>> fashion
>> as part of DT register property. I am wondering if there will be multiple
>> entries that fall with the page size.
>>
> 
> They are part of the same IP block and these are the only register
> offsets which is required to be accessed by HLOS.
> 

HLOS ?

Anyways, OS might touch one or 2 registers in lots of IP blocks. I am
not sure why those are any different from these. Are you trying to align
with any other bindings or specification. Are you trying to make this
binding generic here ? I understand if it was trying to generalize the
firmware interface, but you state it's a hardware engine. So I fail to
see the need for such specificity here. Why not define the whole IP
block and the driver knows where to access these specific ones as they
are specific to this hardware block.

-- 
Regards,
Sudeep

Re: [PATCH 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings

[Viresh Kumar]

On 04-06-18, 16:16, 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           | 173 +++++++++++++++++++++
>  1 file changed, 173 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..e3087ec
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
> @@ -0,0 +1,173 @@
> +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".
> +
> +* Property qcom,freq-domain
> +Devices supporting freq-domain must set their "qcom,freq-domain" property with
> +phandle to a freq_domain_table in their DT node.
> +
> +* Frequency Domain Table Node
> +
> +This describes the frequency domain belonging to a device.
> +This node can have following properties:
> +
> +- reg
> +	Usage:		required
> +	Value type:	<prop-encoded-array>
> +	Definition:	Addresses and sizes for the memory of the perf
> +			, lut and enable bases.
> +			perf - indicates the base address for the desired
> +			performance state to be set.
> +			lut - indicates the look up table base address for the
> +			cpufreq	driver to read frequencies.
> +			enable - indicates the enable register for firmware.
> +- reg-names
> +	Usage:		required
> +	Value type:	<stringlist>
> +	Definition:	Address names. Must be "perf", "lut", "enable".
> +			Must be specified in the same order as the reg property.
> +
> +Example:
> +
> +Example 1: Dual-cluster, Quad-core per cluster. CPUs within a cluster switch
> +DCVS state together.
> +
> +/ {
> +	cpus {
> +		#address-cells = <2>;
> +		#size-cells = <0>;
> +
> +		CPU0: cpu@0 {
> +			device_type = "cpu";
> +			compatible = "qcom,kryo385";
> +			reg = <0x0 0x0>;
> +			enable-method = "psci";
> +			next-level-cache = <&L2_0>;
> +			qcom,freq-domain = <&freq_domain_table0>;
> +			L2_0: l2-cache {
> +				compatible = "cache";
> +				next-level-cache = <&L3_0>;
> +				L3_0: l3-cache {
> +				      compatible = "cache";
> +				};
> +			};
> +		};
> +
> +		CPU1: cpu@100 {
> +			device_type = "cpu";
> +			compatible = "qcom,kryo385";
> +			reg = <0x0 0x100>;
> +			enable-method = "psci";
> +			next-level-cache = <&L2_100>;
> +			qcom,freq-domain = <&freq_domain_table0>;
> +			L2_100: l2-cache {
> +				compatible = "cache";
> +				next-level-cache = <&L3_0>;
> +			};
> +		};
> +
> +		CPU2: cpu@200 {
> +			device_type = "cpu";
> +			compatible = "qcom,kryo385";
> +			reg = <0x0 0x200>;
> +			enable-method = "psci";
> +			next-level-cache = <&L2_200>;
> +			qcom,freq-domain = <&freq_domain_table0>;
> +			L2_200: l2-cache {
> +				compatible = "cache";
> +				next-level-cache = <&L3_0>;
> +			};
> +		};
> +
> +		CPU3: cpu@300 {
> +			device_type = "cpu";
> +			compatible = "qcom,kryo385";
> +			reg = <0x0 0x300>;
> +			enable-method = "psci";
> +			next-level-cache = <&L2_300>;
> +			qcom,freq-domain = <&freq_domain_table0>;
> +			L2_300: l2-cache {
> +				compatible = "cache";
> +				next-level-cache = <&L3_0>;
> +			};
> +		};
> +
> +		CPU4: cpu@400 {
> +			device_type = "cpu";
> +			compatible = "qcom,kryo385";
> +			reg = <0x0 0x400>;
> +			enable-method = "psci";
> +			next-level-cache = <&L2_400>;
> +			qcom,freq-domain = <&freq_domain_table1>;
> +			L2_400: l2-cache {
> +				compatible = "cache";
> +				next-level-cache = <&L3_0>;
> +			};
> +		};
> +
> +		CPU5: cpu@500 {
> +			device_type = "cpu";
> +			compatible = "qcom,kryo385";
> +			reg = <0x0 0x500>;
> +			enable-method = "psci";
> +			next-level-cache = <&L2_500>;
> +			qcom,freq-domain = <&freq_domain_table1>;
> +			L2_500: l2-cache {
> +				compatible = "cache";
> +				next-level-cache = <&L3_0>;
> +			};
> +		};
> +
> +		CPU6: cpu@600 {
> +			device_type = "cpu";
> +			compatible = "qcom,kryo385";
> +			reg = <0x0 0x600>;
> +			enable-method = "psci";
> +			next-level-cache = <&L2_600>;
> +			qcom,freq-domain = <&freq_domain_table1>;
> +			L2_600: l2-cache {
> +				compatible = "cache";
> +				next-level-cache = <&L3_0>;
> +			};
> +		};
> +
> +		CPU7: cpu@700 {
> +			device_type = "cpu";
> +			compatible = "qcom,kryo385";
> +			reg = <0x0 0x700>;
> +			enable-method = "psci";
> +			next-level-cache = <&L2_700>;
> +			qcom,freq-domain = <&freq_domain_table1>;
> +			L2_700: l2-cache {
> +				compatible = "cache";
> +				next-level-cache = <&L3_0>;
> +			};
> +		};
> +	};
> +
> +	qcom,cpufreq-fw {
> +		compatible = "qcom,cpufreq-fw";
> +
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +
> +		freq_domain_table0 : freq_table0 {
> +			reg = <0x17d43920 0x4>, <0x17d43110 0x500>,
> +				 <0x17d41000 0x4>;
> +			reg-names = "perf", "lut", "enable";
> +		};
> +
> +		freq_domain_table1 : freq_table1 {
> +			reg = <0x17d46120 0x4>, <0x17d45910 0x500>,
> +				<0x17d45800 0x4> ;
> +			reg-names = "perf", "lut", "enable";
> +		};
> +	};

Looks better now from design point of view now, no ugly CPU lists :)

Unless Rob finds something wrong with the bindings:

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[PATCH 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

[Taniya Das]

The CPUfreq FW present in some QCOM chipsets offloads the steps necessary
for changing the frequency of CPUs. The driver implements the cpufreq
driver interface for this firmware.

Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
Signed-off-by: Taniya Das <tdas@codeaurora.org>
---
 drivers/cpufreq/Kconfig.arm       |   9 ++
 drivers/cpufreq/Makefile          |   1 +
 drivers/cpufreq/qcom-cpufreq-fw.c | 316 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 326 insertions(+)
 create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index c7ce928..82c391e 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -312,3 +312,12 @@ config ARM_PXA2xx_CPUFREQ
 	  This add the CPUFreq driver support for Intel PXA2xx SOCs.

 	  If in doubt, say N.
+
+config ARM_QCOM_CPUFREQ_FW
+	bool "QCOM CPUFreq FW driver"
+	help
+	 Support for the CPUFreq FW driver.
+	 The CPUfreq FW preset in some QCOM chipsets offloads the steps
+	 necessary for changing the frequency of CPUs. The driver
+	 implements the cpufreq driver interface for this firmware.
+	 Say Y if you want to support CPUFreq FW.
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index fb4a2ec..34691a2 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -86,6 +86,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
 obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
 obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
 obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
+obj-$(CONFIG_ARM_QCOM_CPUFREQ_FW)	+= qcom-cpufreq-fw.o


 ##################################################################################
diff --git a/drivers/cpufreq/qcom-cpufreq-fw.c b/drivers/cpufreq/qcom-cpufreq-fw.c
new file mode 100644
index 0000000..2135a08
--- /dev/null
+++ b/drivers/cpufreq/qcom-cpufreq-fw.c
@@ -0,0 +1,316 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/cpufreq.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+
+#define INIT_RATE			300000000UL
+#define XO_RATE				19200000UL
+#define LUT_MAX_ENTRIES			40U
+#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
+#define LUT_ROW_SIZE			32
+
+struct cpufreq_qcom {
+	struct cpufreq_frequency_table *table;
+	struct device *dev;
+	void __iomem *perf_base;
+	void __iomem *lut_base;
+	cpumask_t related_cpus;
+	unsigned int max_cores;
+};
+
+static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
+
+static int
+qcom_cpufreq_fw_target_index(struct cpufreq_policy *policy,
+			     unsigned int index)
+{
+	struct cpufreq_qcom *c = policy->driver_data;
+
+	writel_relaxed(index, c->perf_base);
+
+	return 0;
+}
+
+static unsigned int qcom_cpufreq_fw_get(unsigned int cpu)
+{
+	struct cpufreq_qcom *c;
+	unsigned int index;
+
+	c = qcom_freq_domain_map[cpu];
+	if (!c)
+		return 0;
+
+	index = readl_relaxed(c->perf_base);
+	index = min(index, LUT_MAX_ENTRIES - 1);
+
+	return c->table[index].frequency;
+}
+
+static int qcom_cpufreq_fw_cpu_init(struct cpufreq_policy *policy)
+{
+	struct cpufreq_qcom *c;
+
+	c = qcom_freq_domain_map[policy->cpu];
+	if (!c) {
+		pr_err("No scaling support for CPU%d\n", policy->cpu);
+		return -ENODEV;
+	}
+
+	cpumask_copy(policy->cpus, &c->related_cpus);
+
+	policy->freq_table = c->table;
+	policy->driver_data = c;
+
+	return 0;
+}
+
+static struct freq_attr *qcom_cpufreq_fw_attr[] = {
+	&cpufreq_freq_attr_scaling_available_freqs,
+	&cpufreq_freq_attr_scaling_boost_freqs,
+	NULL
+};
+
+static struct cpufreq_driver cpufreq_qcom_fw_driver = {
+	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
+			  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
+	.verify		= cpufreq_generic_frequency_table_verify,
+	.target_index	= qcom_cpufreq_fw_target_index,
+	.get		= qcom_cpufreq_fw_get,
+	.init		= qcom_cpufreq_fw_cpu_init,
+	.name		= "qcom-cpufreq-fw",
+	.attr		= qcom_cpufreq_fw_attr,
+	.boost_enabled	= true,
+};
+
+static int qcom_read_lut(struct platform_device *pdev,
+			 struct cpufreq_qcom *c)
+{
+	struct device *dev = &pdev->dev;
+	u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
+
+	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
+				sizeof(*c->table), GFP_KERNEL);
+	if (!c->table)
+		return -ENOMEM;
+
+	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
+		data = readl_relaxed(c->lut_base + i * LUT_ROW_SIZE);
+		src = ((data & GENMASK(31, 30)) >> 30);
+		lval = (data & GENMASK(7, 0));
+		core_count = CORE_COUNT_VAL(data);
+
+		if (!src)
+			c->table[i].frequency = INIT_RATE / 1000;
+		else
+			c->table[i].frequency = XO_RATE * lval / 1000;
+
+		cur_freq = c->table[i].frequency;
+
+		dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
+			i, c->table[i].frequency, core_count);
+
+		if (core_count != c->max_cores)
+			cur_freq = CPUFREQ_ENTRY_INVALID;
+
+		/*
+		 * Two of the same frequencies with the same core counts means
+		 * end of table.
+		 */
+		if (i > 0 && c->table[i - 1].frequency ==
+		   c->table[i].frequency && prev_cc == core_count) {
+			struct cpufreq_frequency_table *prev = &c->table[i - 1];
+
+			if (prev_freq == CPUFREQ_ENTRY_INVALID)
+				prev->flags = CPUFREQ_BOOST_FREQ;
+			break;
+		}
+		prev_cc = core_count;
+		prev_freq = cur_freq;
+	}
+
+	c->table[i].frequency = CPUFREQ_TABLE_END;
+
+	return 0;
+}
+
+static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)
+{
+	struct device_node  *cpu_dev;
+	int cpu;
+
+	for_each_possible_cpu(cpu) {
+		cpu_dev = of_cpu_device_node_get(cpu);
+		if (!cpu_dev)
+			continue;
+		cpu_dev = of_parse_phandle(cpu_dev, "qcom,freq-domain", 0);
+		if (!cpu_dev)
+			continue;
+		if (cpu_dev == np)
+			cpumask_set_cpu(cpu, m);
+	}
+
+	if (cpumask_empty(m))
+		return -ENOENT;
+
+	return 0;
+}
+
+static int qcom_cpu_resources_init(struct platform_device *pdev,
+				   struct device_node *np)
+{
+	struct cpufreq_qcom *c;
+	struct resource res;
+	struct device *dev = &pdev->dev;
+	void __iomem *en_base;
+	int cpu, index, ret;
+
+	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
+	if (!c)
+		return -ENOMEM;
+
+	index = of_property_match_string(np, "reg-names", "enable");
+	if (index < 0)
+		return index;
+
+	if (of_address_to_resource(np, index, &res))
+		return -ENOMEM;
+
+	en_base = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!en_base) {
+		dev_err(dev, "Unable to map %s enable-base\n", np->name);
+		return -ENOMEM;
+	}
+
+	/* FW should be in enabled state to proceed */
+	if (!(readl_relaxed(en_base) & 0x1)) {
+		dev_err(dev, "%s firmware not enabled\n", np->name);
+		return -ENODEV;
+	}
+	devm_iounmap(&pdev->dev, en_base);
+
+	index = of_property_match_string(np, "reg-names", "perf");
+	if (index < 0)
+		return index;
+
+	if (of_address_to_resource(np, index, &res))
+		return -ENOMEM;
+
+	c->perf_base = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!c->perf_base) {
+		dev_err(dev, "Unable to map %s perf-base\n", np->name);
+		return -ENOMEM;
+	}
+
+	index = of_property_match_string(np, "reg-names", "lut");
+	if (index < 0)
+		return index;
+
+	if (of_address_to_resource(np, index, &res))
+		return -ENOMEM;
+
+	c->lut_base = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!c->lut_base) {
+		dev_err(dev, "Unable to map %s lut-base\n", np->name);
+		return -ENOMEM;
+	}
+
+	ret = qcom_get_related_cpus(np, &c->related_cpus);
+	if (ret) {
+		dev_err(dev, "%s failed to get core phandles\n", np->name);
+		return ret;
+	}
+
+	c->max_cores = cpumask_weight(&c->related_cpus);
+
+	ret = qcom_read_lut(pdev, c);
+	if (ret) {
+		dev_err(dev, "%s failed to read LUT\n", np->name);
+		return ret;
+	}
+	for_each_cpu(cpu, &c->related_cpus)
+		qcom_freq_domain_map[cpu] = c;
+
+	return 0;
+}
+
+static int qcom_resources_init(struct platform_device *pdev)
+{
+	struct device_node *np, *cpu_dev;
+	unsigned int cpu;
+	int ret;
+
+	for_each_possible_cpu(cpu) {
+		cpu_dev = of_cpu_device_node_get(cpu);
+		if (!cpu_dev) {
+			dev_err(&pdev->dev, "Failed to get cpu %d device\n",
+				cpu);
+			continue;
+		}
+
+		np = of_parse_phandle(cpu_dev, "qcom,freq-domain", 0);
+		if (!np) {
+			dev_err(&pdev->dev, "Failed to get freq-domain device\n");
+			continue;
+		}
+
+		of_node_put(cpu_dev);
+
+		ret = qcom_cpu_resources_init(pdev, np);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int qcom_cpufreq_fw_driver_probe(struct platform_device *pdev)
+{
+	int rc;
+
+	/* Get the bases of cpufreq for domains */
+	rc = qcom_resources_init(pdev);
+	if (rc) {
+		dev_err(&pdev->dev, "CPUFreq resource init failed\n");
+		return rc;
+	}
+
+	rc = cpufreq_register_driver(&cpufreq_qcom_fw_driver);
+	if (rc) {
+		dev_err(&pdev->dev, "CPUFreq FW driver failed to register\n");
+		return rc;
+	}
+
+	dev_info(&pdev->dev, "QCOM CPUFreq FW driver inited\n");
+
+	return 0;
+}
+
+static const struct of_device_id match_table[] = {
+	{ .compatible = "qcom,cpufreq-fw" },
+	{}
+};
+
+static struct platform_driver qcom_cpufreq_fw_driver = {
+	.probe = qcom_cpufreq_fw_driver_probe,
+	.driver = {
+		.name = "qcom-cpufreq-fw",
+		.of_match_table = match_table,
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init qcom_cpufreq_fw_init(void)
+{
+	return platform_driver_register(&qcom_cpufreq_fw_driver);
+}
+subsys_initcall(qcom_cpufreq_fw_init);
+
+MODULE_DESCRIPTION("QCOM CPU Frequency FW");
+MODULE_LICENSE("GPL v2");
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

Re: [PATCH 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

[Viresh Kumar]

On 04-06-18, 16:16, Taniya Das wrote:
> The CPUfreq FW present in some QCOM chipsets offloads the steps necessary
> for changing the frequency of CPUs. The driver implements the cpufreq
> driver interface for this firmware.
> 
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> Signed-off-by: Taniya Das <tdas@codeaurora.org>
> ---
>  drivers/cpufreq/Kconfig.arm       |   9 ++
>  drivers/cpufreq/Makefile          |   1 +
>  drivers/cpufreq/qcom-cpufreq-fw.c | 316 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 326 insertions(+)
>  create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index c7ce928..82c391e 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -312,3 +312,12 @@ config ARM_PXA2xx_CPUFREQ
>  	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
> 
>  	  If in doubt, say N.
> +
> +config ARM_QCOM_CPUFREQ_FW
> +	bool "QCOM CPUFreq FW driver"
> +	help
> +	 Support for the CPUFreq FW driver.
> +	 The CPUfreq FW preset in some QCOM chipsets offloads the steps
> +	 necessary for changing the frequency of CPUs. The driver
> +	 implements the cpufreq driver interface for this firmware.
> +	 Say Y if you want to support CPUFreq FW.
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index fb4a2ec..34691a2 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -86,6 +86,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
>  obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
>  obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
>  obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
> +obj-$(CONFIG_ARM_QCOM_CPUFREQ_FW)	+= qcom-cpufreq-fw.o
> 
> 
>  ##################################################################################
> diff --git a/drivers/cpufreq/qcom-cpufreq-fw.c b/drivers/cpufreq/qcom-cpufreq-fw.c
> new file mode 100644
> index 0000000..2135a08
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-fw.c
> @@ -0,0 +1,316 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/cpufreq.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +
> +#define INIT_RATE			300000000UL
> +#define XO_RATE				19200000UL
> +#define LUT_MAX_ENTRIES			40U
> +#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
> +#define LUT_ROW_SIZE			32
> +
> +struct cpufreq_qcom {
> +	struct cpufreq_frequency_table *table;
> +	struct device *dev;
> +	void __iomem *perf_base;
> +	void __iomem *lut_base;
> +	cpumask_t related_cpus;
> +	unsigned int max_cores;
> +};
> +
> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
> +
> +static int
> +qcom_cpufreq_fw_target_index(struct cpufreq_policy *policy,
> +			     unsigned int index)
> +{
> +	struct cpufreq_qcom *c = policy->driver_data;
> +
> +	writel_relaxed(index, c->perf_base);
> +
> +	return 0;
> +}
> +
> +static unsigned int qcom_cpufreq_fw_get(unsigned int cpu)
> +{
> +	struct cpufreq_qcom *c;
> +	unsigned int index;
> +
> +	c = qcom_freq_domain_map[cpu];
> +	if (!c)
> +		return 0;
> +
> +	index = readl_relaxed(c->perf_base);
> +	index = min(index, LUT_MAX_ENTRIES - 1);
> +
> +	return c->table[index].frequency;
> +}
> +
> +static int qcom_cpufreq_fw_cpu_init(struct cpufreq_policy *policy)
> +{
> +	struct cpufreq_qcom *c;
> +
> +	c = qcom_freq_domain_map[policy->cpu];
> +	if (!c) {
> +		pr_err("No scaling support for CPU%d\n", policy->cpu);
> +		return -ENODEV;
> +	}
> +
> +	cpumask_copy(policy->cpus, &c->related_cpus);
> +
> +	policy->freq_table = c->table;
> +	policy->driver_data = c;

What about fast cpufreq switching ? I think you can enable that option as well
here..

> +
> +	return 0;
> +}
> +
> +static struct freq_attr *qcom_cpufreq_fw_attr[] = {
> +	&cpufreq_freq_attr_scaling_available_freqs,
> +	&cpufreq_freq_attr_scaling_boost_freqs,
> +	NULL
> +};
> +
> +static struct cpufreq_driver cpufreq_qcom_fw_driver = {
> +	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
> +			  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
> +	.verify		= cpufreq_generic_frequency_table_verify,
> +	.target_index	= qcom_cpufreq_fw_target_index,
> +	.get		= qcom_cpufreq_fw_get,
> +	.init		= qcom_cpufreq_fw_cpu_init,

What about CPU hotplug ? We can still do that, right ? So what will happen if
all CPUs of a freq-domain are removed (hence cpufreq policy is removed) and then
someone calls qcom_cpufreq_fw_get() ? You should really work on cpufreq_policy
there to get 'c'.

> +	.name		= "qcom-cpufreq-fw",
> +	.attr		= qcom_cpufreq_fw_attr,
> +	.boost_enabled	= true,
> +};
> +
> +static int qcom_read_lut(struct platform_device *pdev,
> +			 struct cpufreq_qcom *c)
> +{
> +	struct device *dev = &pdev->dev;
> +	u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
> +
> +	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
> +				sizeof(*c->table), GFP_KERNEL);
> +	if (!c->table)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> +		data = readl_relaxed(c->lut_base + i * LUT_ROW_SIZE);
> +		src = ((data & GENMASK(31, 30)) >> 30);
> +		lval = (data & GENMASK(7, 0));
> +		core_count = CORE_COUNT_VAL(data);
> +
> +		if (!src)
> +			c->table[i].frequency = INIT_RATE / 1000;
> +		else
> +			c->table[i].frequency = XO_RATE * lval / 1000;
> +
> +		cur_freq = c->table[i].frequency;
> +
> +		dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
> +			i, c->table[i].frequency, core_count);
> +
> +		if (core_count != c->max_cores)
> +			cur_freq = CPUFREQ_ENTRY_INVALID;
> +
> +		/*
> +		 * Two of the same frequencies with the same core counts means
> +		 * end of table.
> +		 */
> +		if (i > 0 && c->table[i - 1].frequency ==
> +		   c->table[i].frequency && prev_cc == core_count) {
> +			struct cpufreq_frequency_table *prev = &c->table[i - 1];
> +
> +			if (prev_freq == CPUFREQ_ENTRY_INVALID)
> +				prev->flags = CPUFREQ_BOOST_FREQ;
> +			break;
> +		}
> +		prev_cc = core_count;
> +		prev_freq = cur_freq;
> +	}
> +
> +	c->table[i].frequency = CPUFREQ_TABLE_END;
> +
> +	return 0;
> +}

Looks like there are many problems here.
- You are assigning prev_freq with cur_freq (which may be uninitialized local
  variable here).
- In this version, you never write CPUFREQ_ENTRY_INVALID to table[i].frequency,
  which looks wrong as well.

> +
> +static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)
> +{
> +	struct device_node  *cpu_dev;

s/cpu_dev/cpu_np/

> +	int cpu;
> +
> +	for_each_possible_cpu(cpu) {
> +		cpu_dev = of_cpu_device_node_get(cpu);
> +		if (!cpu_dev)
> +			continue;
> +		cpu_dev = of_parse_phandle(cpu_dev, "qcom,freq-domain", 0);

What's returned here is a pointer to the qcom,freq-domain node, and you assign
that to a variable named cpu_dev. Either use two variables for different node
types, or rename it to temp_np or something similar.

> +		if (!cpu_dev)
> +			continue;
> +		if (cpu_dev == np)
> +			cpumask_set_cpu(cpu, m);
> +	}
> +
> +	if (cpumask_empty(m))
> +		return -ENOENT;
> +
> +	return 0;
> +}
> +
> +static int qcom_cpu_resources_init(struct platform_device *pdev,
> +				   struct device_node *np)
> +{
> +	struct cpufreq_qcom *c;
> +	struct resource res;
> +	struct device *dev = &pdev->dev;
> +	void __iomem *en_base;
> +	int cpu, index, ret;
> +
> +	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
> +	if (!c)
> +		return -ENOMEM;
> +
> +	index = of_property_match_string(np, "reg-names", "enable");
> +	if (index < 0)
> +		return index;
> +
> +	if (of_address_to_resource(np, index, &res))
> +		return -ENOMEM;
> +
> +	en_base = devm_ioremap(dev, res.start, resource_size(&res));
> +	if (!en_base) {
> +		dev_err(dev, "Unable to map %s enable-base\n", np->name);
> +		return -ENOMEM;
> +	}
> +
> +	/* FW should be in enabled state to proceed */
> +	if (!(readl_relaxed(en_base) & 0x1)) {
> +		dev_err(dev, "%s firmware not enabled\n", np->name);
> +		return -ENODEV;
> +	}
> +	devm_iounmap(&pdev->dev, en_base);
> +
> +	index = of_property_match_string(np, "reg-names", "perf");
> +	if (index < 0)
> +		return index;
> +
> +	if (of_address_to_resource(np, index, &res))
> +		return -ENOMEM;
> +
> +	c->perf_base = devm_ioremap(dev, res.start, resource_size(&res));
> +	if (!c->perf_base) {
> +		dev_err(dev, "Unable to map %s perf-base\n", np->name);
> +		return -ENOMEM;
> +	}
> +
> +	index = of_property_match_string(np, "reg-names", "lut");
> +	if (index < 0)
> +		return index;
> +
> +	if (of_address_to_resource(np, index, &res))
> +		return -ENOMEM;
> +
> +	c->lut_base = devm_ioremap(dev, res.start, resource_size(&res));
> +	if (!c->lut_base) {
> +		dev_err(dev, "Unable to map %s lut-base\n", np->name);
> +		return -ENOMEM;
> +	}
> +
> +	ret = qcom_get_related_cpus(np, &c->related_cpus);
> +	if (ret) {
> +		dev_err(dev, "%s failed to get core phandles\n", np->name);

Maybe write a more relevant error message here ?

> +		return ret;
> +	}
> +
> +	c->max_cores = cpumask_weight(&c->related_cpus);

Maybe remove the error checking conditional from qcom_get_related_cpus() and
check !c->max_cores here for the same.

> +
> +	ret = qcom_read_lut(pdev, c);
> +	if (ret) {
> +		dev_err(dev, "%s failed to read LUT\n", np->name);
> +		return ret;
> +	}

Enter a blank line here.

> +	for_each_cpu(cpu, &c->related_cpus)
> +		qcom_freq_domain_map[cpu] = c;

This whole setup looks a bit confusing to me. This is what you are doing
essentially:

qcom_resources_init()
{
        for_each_possible_cpu() {
                qcom_cpu_resources_init()
                {
                        populate c->related_cpus;

                        for_each_related_cpu() {
                                qcom_freq_domain_map[cpu] = c;
                        }
                }
        }
}

So if there are 4 CPUs that share a freq domain, then you are allocating 'c' 4
times and (over)writing qcom_freq_domain_map[] for all these CPUs 4 times and
finally keeping value of 'c' only once.

You must be running most of the work done in qcom_resources_init() only once per
freq-domain.

> +
> +	return 0;
> +}
> +
> +static int qcom_resources_init(struct platform_device *pdev)
> +{
> +	struct device_node *np, *cpu_dev;

cpu_dev is normally used in kernel for struct device *, maybe use cpu_np ?

> +	unsigned int cpu;
> +	int ret;
> +
> +	for_each_possible_cpu(cpu) {
> +		cpu_dev = of_cpu_device_node_get(cpu);
> +		if (!cpu_dev) {
> +			dev_err(&pdev->dev, "Failed to get cpu %d device\n",
> +				cpu);
> +			continue;
> +		}
> +
> +		np = of_parse_phandle(cpu_dev, "qcom,freq-domain", 0);
> +		if (!np) {
> +			dev_err(&pdev->dev, "Failed to get freq-domain device\n");
> +			continue;

I am not sure if we should continue or error out here. Why would you want only a
group of CPUs to have this property set ? Or if you really have a case for that
currently ?

> +		}
> +
> +		of_node_put(cpu_dev);
> +
> +		ret = qcom_cpu_resources_init(pdev, np);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int qcom_cpufreq_fw_driver_probe(struct platform_device *pdev)
> +{
> +	int rc;
> +
> +	/* Get the bases of cpufreq for domains */
> +	rc = qcom_resources_init(pdev);
> +	if (rc) {
> +		dev_err(&pdev->dev, "CPUFreq resource init failed\n");
> +		return rc;
> +	}
> +
> +	rc = cpufreq_register_driver(&cpufreq_qcom_fw_driver);
> +	if (rc) {
> +		dev_err(&pdev->dev, "CPUFreq FW driver failed to register\n");
> +		return rc;
> +	}
> +
> +	dev_info(&pdev->dev, "QCOM CPUFreq FW driver inited\n");

s/inited/initialized/ ?

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id match_table[] = {
> +	{ .compatible = "qcom,cpufreq-fw" },
> +	{}
> +};
> +
> +static struct platform_driver qcom_cpufreq_fw_driver = {
> +	.probe = qcom_cpufreq_fw_driver_probe,
> +	.driver = {
> +		.name = "qcom-cpufreq-fw",
> +		.of_match_table = match_table,
> +		.owner = THIS_MODULE,
> +	},
> +};
> +
> +static int __init qcom_cpufreq_fw_init(void)
> +{
> +	return platform_driver_register(&qcom_cpufreq_fw_driver);
> +}
> +subsys_initcall(qcom_cpufreq_fw_init);
> +
> +MODULE_DESCRIPTION("QCOM CPU Frequency FW");
> +MODULE_LICENSE("GPL v2");
> --
> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
> of the Code Aurora Forum, hosted by the  Linux Foundation.

-- 
viresh

Re: [PATCH 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

[Taniya Das]

Hello Viresh,

Thank you for the review comments.

On 6/6/2018 11:31 AM, Viresh Kumar wrote:
> On 04-06-18, 16:16, Taniya Das wrote:
>> The CPUfreq FW present in some QCOM chipsets offloads the steps necessary
>> for changing the frequency of CPUs. The driver implements the cpufreq
>> driver interface for this firmware.
>>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> Signed-off-by: Taniya Das <tdas@codeaurora.org>
>> ---
>>   drivers/cpufreq/Kconfig.arm       |   9 ++
>>   drivers/cpufreq/Makefile          |   1 +
>>   drivers/cpufreq/qcom-cpufreq-fw.c | 316 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 326 insertions(+)
>>   create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index c7ce928..82c391e 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -312,3 +312,12 @@ config ARM_PXA2xx_CPUFREQ
>>   	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
>>
>>   	  If in doubt, say N.
>> +
>> +config ARM_QCOM_CPUFREQ_FW
>> +	bool "QCOM CPUFreq FW driver"
>> +	help
>> +	 Support for the CPUFreq FW driver.
>> +	 The CPUfreq FW preset in some QCOM chipsets offloads the steps
>> +	 necessary for changing the frequency of CPUs. The driver
>> +	 implements the cpufreq driver interface for this firmware.
>> +	 Say Y if you want to support CPUFreq FW.
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index fb4a2ec..34691a2 100644
>> --- a/drivers/cpufreq/Makefile
>> +++ b/drivers/cpufreq/Makefile
>> @@ -86,6 +86,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
>>   obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
>>   obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
>>   obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
>> +obj-$(CONFIG_ARM_QCOM_CPUFREQ_FW)	+= qcom-cpufreq-fw.o
>>
>>
>>   ##################################################################################
>> diff --git a/drivers/cpufreq/qcom-cpufreq-fw.c b/drivers/cpufreq/qcom-cpufreq-fw.c
>> new file mode 100644
>> index 0000000..2135a08
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-fw.c
>> @@ -0,0 +1,316 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/cpufreq.h>
>> +#include <linux/init.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> +
>> +#define INIT_RATE			300000000UL
>> +#define XO_RATE				19200000UL
>> +#define LUT_MAX_ENTRIES			40U
>> +#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
>> +#define LUT_ROW_SIZE			32
>> +
>> +struct cpufreq_qcom {
>> +	struct cpufreq_frequency_table *table;
>> +	struct device *dev;
>> +	void __iomem *perf_base;
>> +	void __iomem *lut_base;
>> +	cpumask_t related_cpus;
>> +	unsigned int max_cores;
>> +};
>> +
>> +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
>> +
>> +static int
>> +qcom_cpufreq_fw_target_index(struct cpufreq_policy *policy,
>> +			     unsigned int index)
>> +{
>> +	struct cpufreq_qcom *c = policy->driver_data;
>> +
>> +	writel_relaxed(index, c->perf_base);
>> +
>> +	return 0;
>> +}
>> +
>> +static unsigned int qcom_cpufreq_fw_get(unsigned int cpu)
>> +{
>> +	struct cpufreq_qcom *c;
>> +	unsigned int index;
>> +
>> +	c = qcom_freq_domain_map[cpu];
>> +	if (!c)
>> +		return 0;
>> +
>> +	index = readl_relaxed(c->perf_base);
>> +	index = min(index, LUT_MAX_ENTRIES - 1);
>> +
>> +	return c->table[index].frequency;
>> +}
>> +
>> +static int qcom_cpufreq_fw_cpu_init(struct cpufreq_policy *policy)
>> +{
>> +	struct cpufreq_qcom *c;
>> +
>> +	c = qcom_freq_domain_map[policy->cpu];
>> +	if (!c) {
>> +		pr_err("No scaling support for CPU%d\n", policy->cpu);
>> +		return -ENODEV;
>> +	}
>> +
>> +	cpumask_copy(policy->cpus, &c->related_cpus);
>> +
>> +	policy->freq_table = c->table;
>> +	policy->driver_data = c;
> 
> What about fast cpufreq switching ? I think you can enable that option as well
> here..
> 
Sure I will take a look and update.

>> +
>> +	return 0;
>> +}
>> +
>> +static struct freq_attr *qcom_cpufreq_fw_attr[] = {
>> +	&cpufreq_freq_attr_scaling_available_freqs,
>> +	&cpufreq_freq_attr_scaling_boost_freqs,
>> +	NULL
>> +};
>> +
>> +static struct cpufreq_driver cpufreq_qcom_fw_driver = {
>> +	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
>> +			  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
>> +	.verify		= cpufreq_generic_frequency_table_verify,
>> +	.target_index	= qcom_cpufreq_fw_target_index,
>> +	.get		= qcom_cpufreq_fw_get,
>> +	.init		= qcom_cpufreq_fw_cpu_init,
> 
> What about CPU hotplug ? We can still do that, right ? So what will happen if
> all CPUs of a freq-domain are removed (hence cpufreq policy is removed) and then
> someone calls qcom_cpufreq_fw_get() ? You should really work on cpufreq_policy
> there to get 'c'.
> 

You want the _get to do something as below.
Please correct me if my understanding is wrong.
....

  policy = cpufreq_cpu_get_raw(cpu);
  if (!policy)
     return 0;

  c = policy->driver_data;

  index = readl_relaxed(c->perf_base);
  index = min(index, LUT_MAX_ENTRIES - 1);

  return c->table[index].frequency;

....

>> +	.name		= "qcom-cpufreq-fw",
>> +	.attr		= qcom_cpufreq_fw_attr,
>> +	.boost_enabled	= true,
>> +};
>> +
>> +static int qcom_read_lut(struct platform_device *pdev,
>> +			 struct cpufreq_qcom *c)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
>> +
>> +	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
>> +				sizeof(*c->table), GFP_KERNEL);
>> +	if (!c->table)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>> +		data = readl_relaxed(c->lut_base + i * LUT_ROW_SIZE);
>> +		src = ((data & GENMASK(31, 30)) >> 30);
>> +		lval = (data & GENMASK(7, 0));
>> +		core_count = CORE_COUNT_VAL(data);
>> +
>> +		if (!src)
>> +			c->table[i].frequency = INIT_RATE / 1000;
>> +		else
>> +			c->table[i].frequency = XO_RATE * lval / 1000;
>> +
>> +		cur_freq = c->table[i].frequency;
>> +
>> +		dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
>> +			i, c->table[i].frequency, core_count);
>> +
>> +		if (core_count != c->max_cores)
>> +			cur_freq = CPUFREQ_ENTRY_INVALID;
>> +
>> +		/*
>> +		 * Two of the same frequencies with the same core counts means
>> +		 * end of table.
>> +		 */
>> +		if (i > 0 && c->table[i - 1].frequency ==
>> +		   c->table[i].frequency && prev_cc == core_count) {
>> +			struct cpufreq_frequency_table *prev = &c->table[i - 1];
>> +
>> +			if (prev_freq == CPUFREQ_ENTRY_INVALID)
>> +				prev->flags = CPUFREQ_BOOST_FREQ;
>> +			break;
>> +		}
>> +		prev_cc = core_count;
>> +		prev_freq = cur_freq;
>> +	}
>> +
>> +	c->table[i].frequency = CPUFREQ_TABLE_END;
>> +
>> +	return 0;
>> +}
> 
> Looks like there are many problems here.
> - You are assigning prev_freq with cur_freq (which may be uninitialized local
>    variable here).
> - In this version, you never write CPUFREQ_ENTRY_INVALID to table[i].frequency,
>    which looks wrong as well.
> 

- The code to detect boost, would only enter for i > 0 and the prev_freq 
   would be initialized with the cur_freq.
- In the case where the core_count != max_cores, the cur_freq is marked 
INVALID, and when both prev_freq == cur_freq && prev_cc && cur_cc match, 
that is the time the prev table flags need to be updated. Marking the 
table[i].frequency as INVALID is not required as cur_freq is already 
marked with the same. Please correct me if you think otherwise.

>> +
>> +static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)
>> +{
>> +	struct device_node  *cpu_dev;
> 
> s/cpu_dev/cpu_np/
> 
>> +	int cpu;
>> +
>> +	for_each_possible_cpu(cpu) {
>> +		cpu_dev = of_cpu_device_node_get(cpu);
>> +		if (!cpu_dev)
>> +			continue;
>> +		cpu_dev = of_parse_phandle(cpu_dev, "qcom,freq-domain", 0);
> 
> What's returned here is a pointer to the qcom,freq-domain node, and you assign
> that to a variable named cpu_dev. Either use two variables for different node
> types, or rename it to temp_np or something similar.
> 

I would use a different node.

>> +		if (!cpu_dev)
>> +			continue;
>> +		if (cpu_dev == np)
>> +			cpumask_set_cpu(cpu, m);
>> +	}
>> +
>> +	if (cpumask_empty(m))
>> +		return -ENOENT;
>> +
>> +	return 0;
>> +}
>> +
>> +static int qcom_cpu_resources_init(struct platform_device *pdev,
>> +				   struct device_node *np)
>> +{
>> +	struct cpufreq_qcom *c;
>> +	struct resource res;
>> +	struct device *dev = &pdev->dev;
>> +	void __iomem *en_base;
>> +	int cpu, index, ret;
>> +
>> +	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
>> +	if (!c)
>> +		return -ENOMEM;
>> +
>> +	index = of_property_match_string(np, "reg-names", "enable");
>> +	if (index < 0)
>> +		return index;
>> +
>> +	if (of_address_to_resource(np, index, &res))
>> +		return -ENOMEM;
>> +
>> +	en_base = devm_ioremap(dev, res.start, resource_size(&res));
>> +	if (!en_base) {
>> +		dev_err(dev, "Unable to map %s enable-base\n", np->name);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	/* FW should be in enabled state to proceed */
>> +	if (!(readl_relaxed(en_base) & 0x1)) {
>> +		dev_err(dev, "%s firmware not enabled\n", np->name);
>> +		return -ENODEV;
>> +	}
>> +	devm_iounmap(&pdev->dev, en_base);
>> +
>> +	index = of_property_match_string(np, "reg-names", "perf");
>> +	if (index < 0)
>> +		return index;
>> +
>> +	if (of_address_to_resource(np, index, &res))
>> +		return -ENOMEM;
>> +
>> +	c->perf_base = devm_ioremap(dev, res.start, resource_size(&res));
>> +	if (!c->perf_base) {
>> +		dev_err(dev, "Unable to map %s perf-base\n", np->name);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	index = of_property_match_string(np, "reg-names", "lut");
>> +	if (index < 0)
>> +		return index;
>> +
>> +	if (of_address_to_resource(np, index, &res))
>> +		return -ENOMEM;
>> +
>> +	c->lut_base = devm_ioremap(dev, res.start, resource_size(&res));
>> +	if (!c->lut_base) {
>> +		dev_err(dev, "Unable to map %s lut-base\n", np->name);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	ret = qcom_get_related_cpus(np, &c->related_cpus);
>> +	if (ret) {
>> +		dev_err(dev, "%s failed to get core phandles\n", np->name);
> 
> Maybe write a more relevant error message here ?
> 

Sure, would take care of it in the next patch.

>> +		return ret;
>> +	}
>> +
>> +	c->max_cores = cpumask_weight(&c->related_cpus);
> 
> Maybe remove the error checking conditional from qcom_get_related_cpus() and
> check !c->max_cores here for the same.
> 

I would update it in the next patch.

>> +
>> +	ret = qcom_read_lut(pdev, c);
>> +	if (ret) {
>> +		dev_err(dev, "%s failed to read LUT\n", np->name);
>> +		return ret;
>> +	}
> 
> Enter a blank line here.

I would update in the next patch.
> 
>> +	for_each_cpu(cpu, &c->related_cpus)
>> +		qcom_freq_domain_map[cpu] = c;
> 
> This whole setup looks a bit confusing to me. This is what you are doing
> essentially:
> 
> qcom_resources_init()
> {
>          for_each_possible_cpu() {
>                  qcom_cpu_resources_init()
>                  {
>                          populate c->related_cpus;
> 
>                          for_each_related_cpu() {
>                                  qcom_freq_domain_map[cpu] = c;
>                          }
>                  }
>          }
> }
> 
> So if there are 4 CPUs that share a freq domain, then you are allocating 'c' 4
> times and (over)writing qcom_freq_domain_map[] for all these CPUs 4 times and
> finally keeping value of 'c' only once.
> 
> You must be running most of the work done in qcom_resources_init() only once per
> freq-domain.
> 

Thanks, you are correct, I carried the earlier logic, would cleanup in 
the next patch.

>> +
>> +	return 0;
>> +}
>> +
>> +static int qcom_resources_init(struct platform_device *pdev)
>> +{
>> +	struct device_node *np, *cpu_dev;
> 
> cpu_dev is normally used in kernel for struct device *, maybe use cpu_np ?

I would update it in the next patch.
> 
>> +	unsigned int cpu;
>> +	int ret;
>> +
>> +	for_each_possible_cpu(cpu) {
>> +		cpu_dev = of_cpu_device_node_get(cpu);
>> +		if (!cpu_dev) {
>> +			dev_err(&pdev->dev, "Failed to get cpu %d device\n",
>> +				cpu);
>> +			continue;
>> +		}
>> +
>> +		np = of_parse_phandle(cpu_dev, "qcom,freq-domain", 0);
>> +		if (!np) {
>> +			dev_err(&pdev->dev, "Failed to get freq-domain device\n");
>> +			continue;
> 
> I am not sure if we should continue or error out here. Why would you want only a
> group of CPUs to have this property set ? Or if you really have a case for that
> currently ?
> 

I don't have any usecase, but I was of the opinion in case there is no 
freq domains attached to a cluster. But it might be better I error out.

>> +		}
>> +
>> +		of_node_put(cpu_dev);
>> +
>> +		ret = qcom_cpu_resources_init(pdev, np);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int qcom_cpufreq_fw_driver_probe(struct platform_device *pdev)
>> +{
>> +	int rc;
>> +
>> +	/* Get the bases of cpufreq for domains */
>> +	rc = qcom_resources_init(pdev);
>> +	if (rc) {
>> +		dev_err(&pdev->dev, "CPUFreq resource init failed\n");
>> +		return rc;
>> +	}
>> +
>> +	rc = cpufreq_register_driver(&cpufreq_qcom_fw_driver);
>> +	if (rc) {
>> +		dev_err(&pdev->dev, "CPUFreq FW driver failed to register\n");
>> +		return rc;
>> +	}
>> +
>> +	dev_info(&pdev->dev, "QCOM CPUFreq FW driver inited\n");
> 
> s/inited/initialized/ ?
> 

Sure, will fix in the next patch.

>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id match_table[] = {
>> +	{ .compatible = "qcom,cpufreq-fw" },
>> +	{}
>> +};
>> +
>> +static struct platform_driver qcom_cpufreq_fw_driver = {
>> +	.probe = qcom_cpufreq_fw_driver_probe,
>> +	.driver = {
>> +		.name = "qcom-cpufreq-fw",
>> +		.of_match_table = match_table,
>> +		.owner = THIS_MODULE,
>> +	},
>> +};
>> +
>> +static int __init qcom_cpufreq_fw_init(void)
>> +{
>> +	return platform_driver_register(&qcom_cpufreq_fw_driver);
>> +}
>> +subsys_initcall(qcom_cpufreq_fw_init);
>> +
>> +MODULE_DESCRIPTION("QCOM CPU Frequency FW");
>> +MODULE_LICENSE("GPL v2");
>> --
>> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
>> of the Code Aurora Forum, hosted by the  Linux Foundation.
> 

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

--

Re: [PATCH 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

[Viresh Kumar]

Sorry for being late..

On 07-06-18, 12:48, Taniya Das wrote:
> On 6/6/2018 11:31 AM, Viresh Kumar wrote:
> > On 04-06-18, 16:16, Taniya Das wrote:

> > > +static struct cpufreq_driver cpufreq_qcom_fw_driver = {
> > > +	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
> > > +			  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
> > > +	.verify		= cpufreq_generic_frequency_table_verify,
> > > +	.target_index	= qcom_cpufreq_fw_target_index,
> > > +	.get		= qcom_cpufreq_fw_get,
> > > +	.init		= qcom_cpufreq_fw_cpu_init,
> > 
> > What about CPU hotplug ? We can still do that, right ? So what will happen if
> > all CPUs of a freq-domain are removed (hence cpufreq policy is removed) and then
> > someone calls qcom_cpufreq_fw_get() ? You should really work on cpufreq_policy
> > there to get 'c'.
> > 
> 
> You want the _get to do something as below.
> Please correct me if my understanding is wrong.
> ....
> 
>  policy = cpufreq_cpu_get_raw(cpu);
>  if (!policy)
>     return 0;
> 
>  c = policy->driver_data;
> 
>  index = readl_relaxed(c->perf_base);
>  index = min(index, LUT_MAX_ENTRIES - 1);
> 
>  return c->table[index].frequency;
> 
> ....

Right.

> > > +static int qcom_read_lut(struct platform_device *pdev,
> > > +			 struct cpufreq_qcom *c)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
> > > +
> > > +	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
> > > +				sizeof(*c->table), GFP_KERNEL);
> > > +	if (!c->table)
> > > +		return -ENOMEM;
> > > +
> > > +	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> > > +		data = readl_relaxed(c->lut_base + i * LUT_ROW_SIZE);
> > > +		src = ((data & GENMASK(31, 30)) >> 30);
> > > +		lval = (data & GENMASK(7, 0));
> > > +		core_count = CORE_COUNT_VAL(data);
> > > +
> > > +		if (!src)
> > > +			c->table[i].frequency = INIT_RATE / 1000;
> > > +		else
> > > +			c->table[i].frequency = XO_RATE * lval / 1000;
> > > +
> > > +		cur_freq = c->table[i].frequency;
> > > +
> > > +		dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
> > > +			i, c->table[i].frequency, core_count);
> > > +
> > > +		if (core_count != c->max_cores)
> > > +			cur_freq = CPUFREQ_ENTRY_INVALID;
> > > +
> > > +		/*
> > > +		 * Two of the same frequencies with the same core counts means
> > > +		 * end of table.
> > > +		 */
> > > +		if (i > 0 && c->table[i - 1].frequency ==
> > > +		   c->table[i].frequency && prev_cc == core_count) {
> > > +			struct cpufreq_frequency_table *prev = &c->table[i - 1];
> > > +
> > > +			if (prev_freq == CPUFREQ_ENTRY_INVALID)
> > > +				prev->flags = CPUFREQ_BOOST_FREQ;
> > > +			break;
> > > +		}
> > > +		prev_cc = core_count;
> > > +		prev_freq = cur_freq;
> > > +	}
> > > +
> > > +	c->table[i].frequency = CPUFREQ_TABLE_END;
> > > +
> > > +	return 0;
> > > +}
> > 
> > Looks like there are many problems here.
> > - You are assigning prev_freq with cur_freq (which may be uninitialized local
> >    variable here).
> > - In this version, you never write CPUFREQ_ENTRY_INVALID to table[i].frequency,
> >    which looks wrong as well.
> > 
> 
> - The code to detect boost, would only enter for i > 0 and the prev_freq
> would be initialized with the cur_freq.
> - In the case where the core_count != max_cores, the cur_freq is marked
> INVALID, and when both prev_freq == cur_freq && prev_cc && cur_cc match,
> that is the time the prev table flags need to be updated. Marking the
> table[i].frequency as INVALID is not required as cur_freq is already marked
> with the same. Please correct me if you think otherwise.

Yeah but the value of cur_freq isn't written to the table entries now. This
wasn't the case in the earlier version. Have a look at that one.

-- 
viresh

Re: [PATCH 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

[Taniya Das]

On 6/19/2018 2:24 PM, Viresh Kumar wrote:
> Sorry for being late..
> 
> On 07-06-18, 12:48, Taniya Das wrote:
>> On 6/6/2018 11:31 AM, Viresh Kumar wrote:
>>> On 04-06-18, 16:16, Taniya Das wrote:
> 
>>>> +static struct cpufreq_driver cpufreq_qcom_fw_driver = {
>>>> +	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
>>>> +			  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
>>>> +	.verify		= cpufreq_generic_frequency_table_verify,
>>>> +	.target_index	= qcom_cpufreq_fw_target_index,
>>>> +	.get		= qcom_cpufreq_fw_get,
>>>> +	.init		= qcom_cpufreq_fw_cpu_init,
>>>
>>> What about CPU hotplug ? We can still do that, right ? So what will happen if
>>> all CPUs of a freq-domain are removed (hence cpufreq policy is removed) and then
>>> someone calls qcom_cpufreq_fw_get() ? You should really work on cpufreq_policy
>>> there to get 'c'.
>>>
>>
>> You want the _get to do something as below.
>> Please correct me if my understanding is wrong.
>> ....
>>
>>   policy = cpufreq_cpu_get_raw(cpu);
>>   if (!policy)
>>      return 0;
>>
>>   c = policy->driver_data;
>>
>>   index = readl_relaxed(c->perf_base);
>>   index = min(index, LUT_MAX_ENTRIES - 1);
>>
>>   return c->table[index].frequency;
>>
>> ....
> 
> Right.
> 
>>>> +static int qcom_read_lut(struct platform_device *pdev,
>>>> +			 struct cpufreq_qcom *c)
>>>> +{
>>>> +	struct device *dev = &pdev->dev;
>>>> +	u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
>>>> +
>>>> +	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
>>>> +				sizeof(*c->table), GFP_KERNEL);
>>>> +	if (!c->table)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
>>>> +		data = readl_relaxed(c->lut_base + i * LUT_ROW_SIZE);
>>>> +		src = ((data & GENMASK(31, 30)) >> 30);
>>>> +		lval = (data & GENMASK(7, 0));
>>>> +		core_count = CORE_COUNT_VAL(data);
>>>> +
>>>> +		if (!src)
>>>> +			c->table[i].frequency = INIT_RATE / 1000;
>>>> +		else
>>>> +			c->table[i].frequency = XO_RATE * lval / 1000;
>>>> +
>>>> +		cur_freq = c->table[i].frequency;
>>>> +
>>>> +		dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
>>>> +			i, c->table[i].frequency, core_count);
>>>> +
>>>> +		if (core_count != c->max_cores)
>>>> +			cur_freq = CPUFREQ_ENTRY_INVALID;
>>>> +
>>>> +		/*
>>>> +		 * Two of the same frequencies with the same core counts means
>>>> +		 * end of table.
>>>> +		 */
>>>> +		if (i > 0 && c->table[i - 1].frequency ==
>>>> +		   c->table[i].frequency && prev_cc == core_count) {
>>>> +			struct cpufreq_frequency_table *prev = &c->table[i - 1];
>>>> +
>>>> +			if (prev_freq == CPUFREQ_ENTRY_INVALID)
>>>> +				prev->flags = CPUFREQ_BOOST_FREQ;
>>>> +			break;
>>>> +		}
>>>> +		prev_cc = core_count;
>>>> +		prev_freq = cur_freq;
>>>> +	}
>>>> +
>>>> +	c->table[i].frequency = CPUFREQ_TABLE_END;
>>>> +
>>>> +	return 0;
>>>> +}
>>>
>>> Looks like there are many problems here.
>>> - You are assigning prev_freq with cur_freq (which may be uninitialized local
>>>     variable here).
>>> - In this version, you never write CPUFREQ_ENTRY_INVALID to table[i].frequency,
>>>     which looks wrong as well.
>>>
>>
>> - The code to detect boost, would only enter for i > 0 and the prev_freq
>> would be initialized with the cur_freq.
>> - In the case where the core_count != max_cores, the cur_freq is marked
>> INVALID, and when both prev_freq == cur_freq && prev_cc && cur_cc match,
>> that is the time the prev table flags need to be updated. Marking the
>> table[i].frequency as INVALID is not required as cur_freq is already marked
>> with the same. Please correct me if you think otherwise.
> 
> Yeah but the value of cur_freq isn't written to the table entries now. This
> wasn't the case in the earlier version. Have a look at that one.
> 

Yes, Viresh, earlier code was updating the table frequency as I was 
marking the table frequency INVALID.
	if (core_count != c->max_cores)	
		c->table[i].frequency = CPUFREQ_ENTRY_INVALID;	

And thus I had to update the table frequency.

But now I have used the cur_freq instead and the table frequency is not 
touched.
	if (core_count != c->max_cores)
		cur_freq = CPUFREQ_ENTRY_INVALID;

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

--

Re: [PATCH 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

[Viresh Kumar]

On 19-06-18, 15:55, Taniya Das wrote:
> Yes, Viresh, earlier code was updating the table frequency as I was marking
> the table frequency INVALID.
> 	if (core_count != c->max_cores)	
> 		c->table[i].frequency = CPUFREQ_ENTRY_INVALID;	
> 
> And thus I had to update the table frequency.
> 
> But now I have used the cur_freq instead and the table frequency is not
> touched.
> 	if (core_count != c->max_cores)
> 		cur_freq = CPUFREQ_ENTRY_INVALID;

Unless I am reading your versions incorrectly, they behave differently.

Until V2, if core_count wasn't equal to max_cores then the frequency was getting
marked as INVALID straight away in the table itself. Now if the next freq is
also same then you abort and overwrite the previous one as valid, but otherwise
it remains INVALID for ever. And this last thing doesn't happen anymore.

So if in your table there are few frequency entries which aren't repeating, but
the core_count != max_cores for some of them, they remain valid in the newer
versions.

-- 
viresh