[PATCH v2 0/2] Add support for QCOM cpufreq FW driver

[PATCH v2 0/2] Add support for QCOM cpufreq FW driver

[Taniya Das]

 [v2]
   * Address comments given in v0 series.

 [v1]
    * Fixed compilation reported by Amit K.

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.

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           |  68 +++++
 drivers/cpufreq/Kconfig.arm                        |   9 +
 drivers/cpufreq/Makefile                           |   1 +
 drivers/cpufreq/qcom-cpufreq-fw.c                  | 317 +++++++++++++++++++++
 4 files changed, 395 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 v2 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           | 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".
+
+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>;
+		};
+	};
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

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

[Rob Herring]

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.
> 

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

[Viresh Kumar]

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.

-- 
viresh

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

[Rob Herring]

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

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

[Taniya Das]

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
> 

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

--

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

[Viresh Kumar]

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.

-- 
viresh

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

[Taniya Das]

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.
> 

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

--

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

[Sudeep Holla]

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.

-- 
Regards,
Sudeep

[1] https://lkml.org/lkml/2018/5/18/136

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

[Taniya Das]

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.


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

--

[PATCH v2 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 | 317 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 327 insertions(+)
 create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 96b35b8..571f6b4 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -301,3 +301,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 8d24ade..a3edbce 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -85,6 +85,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..0e66de0
--- /dev/null
+++ b/drivers/cpufreq/qcom-cpufreq-fw.c
@@ -0,0 +1,317 @@
+// 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;
+
+	if (index >= LUT_MAX_ENTRIES) {
+		dev_err(c->dev,
+			"Passing an index (%u) that's greater than max (%d)\n",
+					index, LUT_MAX_ENTRIES - 1);
+		return -EINVAL;
+	}
+
+	writel_relaxed(index, c->perf_base);
+
+	/* Make sure the write goes through before proceeding */
+	mb();
+	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 -ENODEV;
+
+	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;
+
+	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;
+
+		c->table[i].driver_data = 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)
+			c->table[i].frequency = CPUFREQ_ENTRY_INVALID;
+
+		/*
+		 * Two of the same frequencies with the same core counts means
+		 * end of table.
+		 */
+		if (i > 0 && c->table[i - 1].driver_data ==
+			c->table[i].driver_data && prev_cc == core_count) {
+			struct cpufreq_frequency_table *prev = &c->table[i - 1];
+
+			if (prev->frequency == CPUFREQ_ENTRY_INVALID) {
+				prev->flags = CPUFREQ_BOOST_FREQ;
+				prev->frequency = prev->driver_data;
+			}
+
+			break;
+		}
+		prev_cc = core_count;
+	}
+	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 *dev_phandle;
+	struct device *cpu_dev;
+	int cpu, i = 0;
+
+	dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
+	while (dev_phandle) {
+		for_each_possible_cpu(cpu) {
+			cpu_dev = get_cpu_device(cpu);
+			if (cpu_dev && cpu_dev->of_node == dev_phandle) {
+				cpumask_set_cpu(cpu, m);
+				break;
+			}
+		}
+		dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
+	}
+
+	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", "en_base");
+	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 en-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_base");
+	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_base");
+	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;
+	int ret;
+
+	if (!of_get_available_child_count(pdev->dev.of_node))
+		return -ENODEV;
+
+	for_each_available_child_of_node(pdev->dev.of_node, np) {
+		if (of_device_is_compatible(np, "cpufreq")) {
+			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 v2 2/2] cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

[Viresh Kumar]

On 19-05-18, 23:04, 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 | 317 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 327 insertions(+)
>  create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 96b35b8..571f6b4 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -301,3 +301,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"

During last review I didn't say that this driver shouldn't be a
module, but that you need to fix things to make it a module. I am fine
though if you don't want this to be a module ever.

> +	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 8d24ade..a3edbce 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -85,6 +85,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..0e66de0
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq-fw.c
> @@ -0,0 +1,317 @@
> +// 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;
> +
> +	if (index >= LUT_MAX_ENTRIES) {
> +		dev_err(c->dev,
> +			"Passing an index (%u) that's greater than max (%d)\n",
> +					index, LUT_MAX_ENTRIES - 1);
> +		return -EINVAL;
> +	}

This is never going to happen unless there is a bug in cpufreq core.
You are allocating only 40 entries for the cpufreq table and this will
always be 0-39. None of the other drivers is checking this I believe
and neither should you. This is the only routine which will get call
very frequently and we better not add unnecessary stuff here.

> +	writel_relaxed(index, c->perf_base);
> +
> +	/* Make sure the write goes through before proceeding */
> +	mb();

Btw what happens right after this is done ? Are we guaranteed that the
frequency is updated in the hardware after this ? What about enabling
fast-switch for your platform ? Look at drivers/cpufreq/scmi-cpufreq.c
to see how that is done.

> +	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 -ENODEV;

Return 0 on error here.

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

Will the hardware ever read a value over 39 here ?

> +
> +	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;
> +
> +	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;
> +
> +		c->table[i].driver_data = c->table[i].frequency;

Why do you need to use driver_data here? Why can't you simple use
frequency field in the below conditional expressions ?

> +
> +		dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
> +			i, c->table[i].frequency, core_count);
> +
> +		if (core_count != c->max_cores)
> +			c->table[i].frequency = CPUFREQ_ENTRY_INVALID;
> +
> +		/*
> +		 * Two of the same frequencies with the same core counts means
> +		 * end of table.
> +		 */
> +		if (i > 0 && c->table[i - 1].driver_data ==
> +			c->table[i].driver_data && prev_cc == core_count) {
> +			struct cpufreq_frequency_table *prev = &c->table[i - 1];
> +
> +			if (prev->frequency == CPUFREQ_ENTRY_INVALID) {

There can only be a single boost frequency at max ?

> +				prev->flags = CPUFREQ_BOOST_FREQ;
> +				prev->frequency = prev->driver_data;

Okay you are using driver_data as a local variable to keep this value
safe which you might have overwritten. Maybe use a simple variable
prev_freq for this. It would be much more readable in that case and
you wouldn't end up abusing the driver_data field.

> +			}
> +
> +			break;
> +		}
> +		prev_cc = core_count;
> +	}
> +	c->table[i].frequency = CPUFREQ_TABLE_END;

Wouldn't you end up writing on c->table[40].frequency here if there
are 40 frequency value present ?

> +
> +	return 0;
> +}
> +
> +static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)
> +{
> +	struct device_node *dev_phandle;
> +	struct device *cpu_dev;
> +	int cpu, i = 0;
> +
> +	dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
> +	while (dev_phandle) {
> +		for_each_possible_cpu(cpu) {
> +			cpu_dev = get_cpu_device(cpu);
> +			if (cpu_dev && cpu_dev->of_node == dev_phandle) {

You can use of_cpu_device_node_get() here.

> +				cpumask_set_cpu(cpu, m);
> +				break;
> +			}
> +		}
> +		dev_phandle = of_parse_phandle(np, "qcom,cpulist", i++);
> +	}
> +
> +	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", "en_base");
> +	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 en-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_base");
> +	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_base");
> +	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;
> +	int ret;
> +
> +	if (!of_get_available_child_count(pdev->dev.of_node))
> +		return -ENODEV;
> +
> +	for_each_available_child_of_node(pdev->dev.of_node, np) {
> +		if (of_device_is_compatible(np, "cpufreq")) {
> +			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.

-- 
viresh

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

[skannan]

On 2018-05-21 02:01, Viresh Kumar wrote:
> On 19-05-18, 23:04, 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 | 317 
>> ++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 327 insertions(+)
>>  create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c
>> 
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 96b35b8..571f6b4 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -301,3 +301,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"
> 
> During last review I didn't say that this driver shouldn't be a
> module, but that you need to fix things to make it a module. I am fine
> though if you don't want this to be a module ever.
> 
>> +	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 8d24ade..a3edbce 100644
>> --- a/drivers/cpufreq/Makefile
>> +++ b/drivers/cpufreq/Makefile
>> @@ -85,6 +85,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..0e66de0
>> --- /dev/null
>> +++ b/drivers/cpufreq/qcom-cpufreq-fw.c
>> @@ -0,0 +1,317 @@
>> +// 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;
>> +
>> +	if (index >= LUT_MAX_ENTRIES) {
>> +		dev_err(c->dev,
>> +			"Passing an index (%u) that's greater than max (%d)\n",
>> +					index, LUT_MAX_ENTRIES - 1);
>> +		return -EINVAL;
>> +	}
> 
> This is never going to happen unless there is a bug in cpufreq core.
> You are allocating only 40 entries for the cpufreq table and this will
> always be 0-39. None of the other drivers is checking this I believe
> and neither should you. This is the only routine which will get call
> very frequently and we better not add unnecessary stuff here.
> 
>> +	writel_relaxed(index, c->perf_base);
>> +
>> +	/* Make sure the write goes through before proceeding */
>> +	mb();
> 
> Btw what happens right after this is done ? Are we guaranteed that the
> frequency is updated in the hardware after this ? What about enabling
> fast-switch for your platform ? Look at drivers/cpufreq/scmi-cpufreq.c
> to see how that is done.

Yeah, I don't think this is needed really.

> 
>> +	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 -ENODEV;
> 
> Return 0 on error here.
> 
>> +
>> +	index = readl_relaxed(c->perf_base);
>> +	index = min(index, LUT_MAX_ENTRIES - 1);
> 
> Will the hardware ever read a value over 39 here ?

The register could be initialized to whatever before the kernel is 
brought up. Don't want to depend on it being correct to avoid out of 
bounds access that could leak data.


>> +
>> +	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;
>> +
>> +	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;
>> +
>> +		c->table[i].driver_data = c->table[i].frequency;
> 
> Why do you need to use driver_data here? Why can't you simple use
> frequency field in the below conditional expressions ?
> 
>> +
>> +		dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
>> +			i, c->table[i].frequency, core_count);
>> +
>> +		if (core_count != c->max_cores)
>> +			c->table[i].frequency = CPUFREQ_ENTRY_INVALID;
>> +
>> +		/*
>> +		 * Two of the same frequencies with the same core counts means
>> +		 * end of table.
>> +		 */
>> +		if (i > 0 && c->table[i - 1].driver_data ==
>> +			c->table[i].driver_data && prev_cc == core_count) {
>> +			struct cpufreq_frequency_table *prev = &c->table[i - 1];
>> +
>> +			if (prev->frequency == CPUFREQ_ENTRY_INVALID) {
> 
> There can only be a single boost frequency at max ?

As of now, yes. If that changes, we'll change this code later.

>> +				prev->flags = CPUFREQ_BOOST_FREQ;
>> +				prev->frequency = prev->driver_data;
> 
> Okay you are using driver_data as a local variable to keep this value
> safe which you might have overwritten. Maybe use a simple variable
> prev_freq for this. It would be much more readable in that case and
> you wouldn't end up abusing the driver_data field.
> 
>> +			}
>> +
>> +			break;
>> +		}
>> +		prev_cc = core_count;
>> +	}
>> +	c->table[i].frequency = CPUFREQ_TABLE_END;
> 
> Wouldn't you end up writing on c->table[40].frequency here if there
> are 40 frequency value present ?

Yeah, the loop condition needs to be fixed.

-Saravana

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

[Taniya Das]

Hello Viresh,

Thanks for your comments.

On 5/22/2018 12:36 AM, skannan@codeaurora.org wrote:
> On 2018-05-21 02:01, Viresh Kumar wrote:
>> On 19-05-18, 23:04, 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 | 317 
>>> ++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 327 insertions(+)
>>>  create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c
>>>
>>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>>> index 96b35b8..571f6b4 100644
>>> --- a/drivers/cpufreq/Kconfig.arm
>>> +++ b/drivers/cpufreq/Kconfig.arm
>>> @@ -301,3 +301,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"
>>
>> During last review I didn't say that this driver shouldn't be a
>> module, but that you need to fix things to make it a module. I am fine
>> though if you don't want this to be a module ever.
>>
>>> +    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 8d24ade..a3edbce 100644
>>> --- a/drivers/cpufreq/Makefile
>>> +++ b/drivers/cpufreq/Makefile
>>> @@ -85,6 +85,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..0e66de0
>>> --- /dev/null
>>> +++ b/drivers/cpufreq/qcom-cpufreq-fw.c
>>> @@ -0,0 +1,317 @@
>>> +// 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;
>>> +
>>> +    if (index >= LUT_MAX_ENTRIES) {
>>> +        dev_err(c->dev,
>>> +            "Passing an index (%u) that's greater than max (%d)\n",
>>> +                    index, LUT_MAX_ENTRIES - 1);
>>> +        return -EINVAL;
>>> +    }
>>
>> This is never going to happen unless there is a bug in cpufreq core.
>> You are allocating only 40 entries for the cpufreq table and this will
>> always be 0-39. None of the other drivers is checking this I believe
>> and neither should you. This is the only routine which will get call
>> very frequently and we better not add unnecessary stuff here.
>>

Yes, I would remove this in the next series.

>>> +    writel_relaxed(index, c->perf_base);
>>> +
>>> +    /* Make sure the write goes through before proceeding */
>>> +    mb();
>>
>> Btw what happens right after this is done ? Are we guaranteed that the
>> frequency is updated in the hardware after this ? What about enabling
>> fast-switch for your platform ? Look at drivers/cpufreq/scmi-cpufreq.c
>> to see how that is done.
> 
> Yeah, I don't think this is needed really.
> 

Just want to make sure it doesn't really sit in the write buffer before 
return.

>>
>>> +    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 -ENODEV;
>>
>> Return 0 on error here.
>>

Would update this in the next patch.

>>> +
>>> +    index = readl_relaxed(c->perf_base);
>>> +    index = min(index, LUT_MAX_ENTRIES - 1);
>>
>> Will the hardware ever read a value over 39 here ?
> 
> The register could be initialized to whatever before the kernel is 
> brought up. Don't want to depend on it being correct to avoid out of 
> bounds access that could leak data.
> 
> 
>>> +
>>> +    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;
>>> +
>>> +    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;
>>> +
>>> +        c->table[i].driver_data = c->table[i].frequency;
>>
>> Why do you need to use driver_data here? Why can't you simple use
>> frequency field in the below conditional expressions ?
>>

The frequency field would be marked INVALID in case the core count does 
not match and the frequency data would be lost.

>>> +
>>> +        dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
>>> +            i, c->table[i].frequency, core_count);
>>> +
>>> +        if (core_count != c->max_cores)
>>> +            c->table[i].frequency = CPUFREQ_ENTRY_INVALID;
>>> +
>>> +        /*
>>> +         * Two of the same frequencies with the same core counts means
>>> +         * end of table.
>>> +         */
>>> +        if (i > 0 && c->table[i - 1].driver_data ==
>>> +            c->table[i].driver_data && prev_cc == core_count) {
>>> +            struct cpufreq_frequency_table *prev = &c->table[i - 1];
>>> +
>>> +            if (prev->frequency == CPUFREQ_ENTRY_INVALID) {
>>
>> There can only be a single boost frequency at max ?
> 
> As of now, yes. If that changes, we'll change this code later.
> 
>>> +                prev->flags = CPUFREQ_BOOST_FREQ;
>>> +                prev->frequency = prev->driver_data;
>>
>> Okay you are using driver_data as a local variable to keep this value
>> safe which you might have overwritten. Maybe use a simple variable
>> prev_freq for this. It would be much more readable in that case and
>> you wouldn't end up abusing the driver_data field.
>>

Please correct me, currently the driver_data is not used by cpufreq core 
and that was the reason to use it. In case you still think it is not a 
good way to handle it, I would try to handle it differently.

>>> +            }
>>> +
>>> +            break;
>>> +        }
>>> +        prev_cc = core_count;
>>> +    }
>>> +    c->table[i].frequency = CPUFREQ_TABLE_END;
>>
>> Wouldn't you end up writing on c->table[40].frequency here if there
>> are 40 frequency value present ?
> 
> Yeah, the loop condition needs to be fixed.
> 

The table allocation is done for 'LUT_MAX_ENTRIES + 1'.
Yes in case we have all [0-39] (i.e 40 entries) read from the hardware, 
would store the same and mark the 40th index as table end. Please 
correct if I missed something in your comment.

> -Saravana

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

--

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

[Viresh Kumar]

On 22-05-18, 16:13, Taniya Das wrote:
> On 5/22/2018 12:36 AM, skannan@codeaurora.org wrote:
> > On 2018-05-21 02:01, Viresh Kumar wrote:
> > > On 19-05-18, 23:04, Taniya Das wrote:

> > > > +    /* Make sure the write goes through before proceeding */
> > > > +    mb();
> > > 
> > > Btw what happens right after this is done ? Are we guaranteed that the
> > > frequency is updated in the hardware after this ? What about enabling
> > > fast-switch for your platform ? Look at drivers/cpufreq/scmi-cpufreq.c
> > > to see how that is done.
> > 
> > Yeah, I don't think this is needed really.
> > 
> 
> Just want to make sure it doesn't really sit in the write buffer before
> return.

As per Saravana you will be dropping it now, right ?

> > > > +    index = readl_relaxed(c->perf_base);
> > > > +    index = min(index, LUT_MAX_ENTRIES - 1);
> > > 
> > > Will the hardware ever read a value over 39 here ?
> > 
> > The register could be initialized to whatever before the kernel is
> > brought up. Don't want to depend on it being correct to avoid out of
> > bounds access that could leak data.

Fair enough.

> > > > +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;
> > > > +
> > > > +    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;
> > > > +
> > > > +        c->table[i].driver_data = c->table[i].frequency;
> > > 
> > > Why do you need to use driver_data here? Why can't you simple use
> > > frequency field in the below conditional expressions ?
> > > 
> 
> The frequency field would be marked INVALID in case the core count does not
> match and the frequency data would be lost.
> 
> > > > +
> > > > +        dev_dbg(dev, "index=%d freq=%d, core_count %d\n",
> > > > +            i, c->table[i].frequency, core_count);
> > > > +
> > > > +        if (core_count != c->max_cores)
> > > > +            c->table[i].frequency = CPUFREQ_ENTRY_INVALID;
> > > > +
> > > > +        /*
> > > > +         * Two of the same frequencies with the same core counts means
> > > > +         * end of table.
> > > > +         */
> > > > +        if (i > 0 && c->table[i - 1].driver_data ==
> > > > +            c->table[i].driver_data && prev_cc == core_count) {
> > > > +            struct cpufreq_frequency_table *prev = &c->table[i - 1];
> > > > +
> > > > +            if (prev->frequency == CPUFREQ_ENTRY_INVALID) {
> > > 
> > > There can only be a single boost frequency at max ?
> > 
> > As of now, yes. If that changes, we'll change this code later.
> > 
> > > > +                prev->flags = CPUFREQ_BOOST_FREQ;
> > > > +                prev->frequency = prev->driver_data;
> > > 
> > > Okay you are using driver_data as a local variable to keep this value
> > > safe which you might have overwritten. Maybe use a simple variable
> > > prev_freq for this. It would be much more readable in that case and
> > > you wouldn't end up abusing the driver_data field.
> > > 
> 
> Please correct me, currently the driver_data is not used by cpufreq core and
> that was the reason to use it. In case you still think it is not a good way
> to handle it, I would try to handle it differently.

Yeah, the driver data will not be used by cpufreq core, but I think
the code would be far more readable if you use a local variable like
prev_freq instead.

> > > > +            }
> > > > +
> > > > +            break;
> > > > +        }
> > > > +        prev_cc = core_count;
> > > > +    }
> > > > +    c->table[i].frequency = CPUFREQ_TABLE_END;
> > > 
> > > Wouldn't you end up writing on c->table[40].frequency here if there
> > > are 40 frequency value present ?
> > 
> > Yeah, the loop condition needs to be fixed.
> > 
> 
> The table allocation is done for 'LUT_MAX_ENTRIES + 1'.
> Yes in case we have all [0-39] (i.e 40 entries) read from the hardware,
> would store the same and mark the 40th index as table end. Please correct if
> I missed something in your comment.

Ahh, you are right. I misread it.

-- 
viresh

Re: [PATCH v2 0/2] Add support for QCOM cpufreq FW driver

[Viresh Kumar]

On 19-05-18, 23:04, Taniya Das wrote:
>  [v2]
>    * Address comments given in v0 series.

That's not how you do it. You need to explain every change in enough
detail here so that the reviewers don't need to go to their previous
emails to see what changed.

>  [v1]
>     * Fixed compilation reported by Amit K.
> 
> 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.
> 
> 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           |  68 +++++
>  drivers/cpufreq/Kconfig.arm                        |   9 +
>  drivers/cpufreq/Makefile                           |   1 +
>  drivers/cpufreq/qcom-cpufreq-fw.c                  | 317 +++++++++++++++++++++
>  4 files changed, 395 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.

-- 
viresh

Re: [PATCH v2 0/2] Add support for QCOM cpufreq FW driver

[Taniya Das]

Hello Viresh,

Sure, will do it next time. Hope updating the fixes in this email is 
fine. Do let me know in case you need me to send it across again the 
series with cover letter updated.

On 5/21/2018 11:35 AM, Viresh Kumar wrote:
> On 19-05-18, 23:04, Taniya Das wrote:
>>   [v2]
>>     * Address comments given in v0 series.
> 
> That's not how you do it. You need to explain every change in enough
> detail here so that the reviewers don't need to go to their previous
> emails to see what changed.
> 
Fixes in [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.
>>
>> 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.
>>
>> 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           |  68 +++++
>>   drivers/cpufreq/Kconfig.arm                        |   9 +
>>   drivers/cpufreq/Makefile                           |   1 +
>>   drivers/cpufreq/qcom-cpufreq-fw.c                  | 317 +++++++++++++++++++++
>>   4 files changed, 395 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.
> 

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

--

Re: [PATCH v2 0/2] Add support for QCOM cpufreq FW driver

[Viresh Kumar]

On 21-05-18, 13:05, Taniya Das wrote:
> Hello Viresh,
> 
> Sure, will do it next time. Hope updating the fixes in this email is fine.
> Do let me know in case you need me to send it across again the series with
> cover letter updated.
> 
> On 5/21/2018 11:35 AM, Viresh Kumar wrote:
> > On 19-05-18, 23:04, Taniya Das wrote:
> > >   [v2]
> > >     * Address comments given in v0 series.
> > 
> > That's not how you do it. You need to explain every change in enough
> > detail here so that the reviewers don't need to go to their previous
> > emails to see what changed.
> > 
> Fixes in [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.

That's what it should look like. Thanks.

-- 
viresh