cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
v9 cover cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
[v9]
* Update the Documentation binding for freq-domain-cells & cpufreq node.
* Address comments in Kconfig.arm & Makefile.
* Remove include file & MODULE_DESCRIPTION not required.
* Fix the code for 'of_node_put(cpu_np)'.
[v8]
* Address comments to update code to take cpufreq_hw phandle and index from the CPU nodes.
* Updated the Documentation for the above change in DT.
* Updated logic for assigning 'qcom_freq_domain_map' for related CPUs.
* Clock input to the HW block is taken from DT which has been updated in code and Device tree documentation.
[v7]
* Updated the logic to check for related CPUs.
[v6]
* Renamed match table 'qcom_cpufreq_hw_match'.
* Renamed 'qcom_read_lut' to 'qcom_cpufreq_hw_read_lut'.
* Updated the logic to check for related CPUs at the beginning of the 'qcom_cpu_resources_init'.
* Use devm_ioremap_resource instead of devm_ioremap.
* Update the use of of_node_put to handle error conditions.
* Use policy->cached_resolved_idx in fast switch callback.
* Keep precalculated offsets 'reg_bases'.
* XO clock is taken from Device tree.
* Update documentation binding for clocks/clock-names.
* Minor comments in Kconfig.arm.
* Comments to move dev_info to dev_dbg.
[v5]
* Remove mapping different register regions of perf/lut/enable, instead map the entire HW region.
* Add reg_offset/cpufreq_qcom_std_offsets to be supplied as device data.
* Check of src == 0 during lut read.
* Add of_node_put(cpu_np) in qcom_get_related_cpus
* Update the qcom_cpu_resources_init for register offset data, and cleanup the related cpus to keep a single copy of CPUfreq.
* Replace FW with HW, update Kconfig, rename filename qcom-cpufreq-hw.c
* Update the documentation binding to reflect the changes of mapping the
* entire HW region.
[v4]
* Fixed console messages as per comments.
* Return error from qcom_resources_init()
in the cases where failed to get frequency domain.
* Rename cpu_dev to cpu_np in qcom_resources_init, qcom_get_related_cpus(). Also use temp variable freq_np in qcom_get_related_cpus().
* Update qcom_cpufreq_fw_get() to use the policy data to incoporate the hotplug use case.
* Update code to use of fast_switching.
* Check for !c->max_cores instead of cpumask_empty in qcom_get_related_cpus().
* Update the logic of assigning 'c' to qcom_freq_domain_map[cpu].
[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'.
Taniya Das (2):
dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings
cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
.../bindings/cpufreq/cpufreq-qcom-hw.txt | 173 ++++++++++ drivers/cpufreq/Kconfig.arm | 11 + drivers/cpufreq/Makefile | 1 + drivers/cpufreq/qcom-cpufreq-hw.c | 354 +++++++++++++++++++++ 4 files changed, 539 insertions(+) create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member of the Code Aurora Forum, hosted by the Linux Foundation.
PATCH v9 1/2 dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings
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 the hardware engine.
--- .../bindings/cpufreq/cpufreq-qcom-hw.txt | 173 +++++++++++++++++++++ 1 file changed, 173 insertions(+) create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt new file +173 -0
@@ -0,0 +1,173 @@ +Qualcomm Technologies, Inc. CPUFREQ Bindings + +CPUFREQ HW 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-hw". + +- clocks + Usage: required + Value type: <phandle> From common clock binding. + Definition: clock handle for XO clock and GPLL0 clock. + +- clock-names + Usage: required + Value type: <string> From common clock binding. + Definition: must be "xo", "cpu_clk". + +- reg + Usage: required + Value type: <prop-encoded-array> + Definition: Addresses and sizes for the memory of the HW bases in + each frequency domain. +- reg-names + Usage: Optional + Value type: <string> + Definition: Frequency domain name i.e. + "freq-domain0", "freq-domain1". + +- freq-domain-cells: + Usage: required. + Definition: Number of cells in a freqency domain specifier. + +* Property qcom,freq-domain +Devices supporting freq-domain must set their "qcom,freq-domain" property with +phandle to a cpufreq_hw followed by the Domain ID(0/1) in the CPU DT node. + + +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 = <&cpufreq_hw 0>; + 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 = <&cpufreq_hw 0>; + 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 = <&cpufreq_hw 0>; + 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 = <&cpufreq_hw 0>; + 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 = <&cpufreq_hw 1>; + 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 = <&cpufreq_hw 1>; + 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 = <&cpufreq_hw 1>; + 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 = <&cpufreq_hw 1>; + L2_700: l2-cache { + compatible = "cache"; + next-level-cache = <&L3_0>; + }; + }; + }; + + soc { + cpufreq_hw: cpufreq@17d43000 { + compatible = "qcom,cpufreq-hw"; + reg = <0x17d43000 0x1400>, <0x17d45800 0x1400>; + reg-names = "freq-domain0", "freq-domain1"; + + clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>; + clock-names = "xo", "cpu_clk"; + + #freq-domain-cells = <1> + + }; +}
--
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 Firmware bindings
Quoted 10 lines · 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 the hardware engine.
Signed-off-by Taniya Das <[email protected]>--- .../bindings/cpufreq/cpufreq-qcom-hw.txt | 173 +++++++++++++++++++++ 1 file changed, 173 insertions(+) create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
Re: [PATCH 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings
In reply to Taniya DasQuoting Taniya Das (2018-10-11 04:36:00)
Quoted 6 lines
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 the hardware engine.
Signed-off-by Taniya Das <[email protected]>---
Re: [PATCH 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings
In reply to Taniya DasQuoted 176 lines · 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 the hardware engine.
Signed-off-by Taniya Das <[email protected]>--- .../bindings/cpufreq/cpufreq-qcom-hw.txt | 173 +++++++++++++++++++++ 1 file changed, 173 insertions(+) create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txtDocumentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt new file +160
-0@@ -0,0 +1,173 @@ +Qualcomm Technologies, Inc. CPUFREQ Bindings + +CPUFREQ HW 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-hw". + +- clocks + Usage: required + Value type: <phandle> From common clock binding. + Definition: clock handle for XO clock and GPLL0 clock. + +- clock-names + Usage: required + Value type: <string> From common clock binding. + Definition: must be "xo", "cpu_clk". + +- reg + Usage: required + Value type: <prop-encoded-array> + Definition: Addresses and sizes for the memory of the HW bases in + each frequency domain. +- reg-names + Usage: Optional + Value type: <string> + Definition: Frequency domain name i.e. + "freq-domain0", "freq-domain1". + +- freq-domain-cells: + Usage: required. + Definition: Number of cells in a freqency domain specifier. + +* Property qcom,freq-domain +Devices supporting freq-domain must set their "qcom,freq-domain" property with +phandle to a cpufreq_hw followed by the Domain ID(0/1) in the CPU DT node. + + +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 = <&cpufreq_hw 0>; + 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 = <&cpufreq_hw 0>; + 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 = <&cpufreq_hw 0>; + 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 = <&cpufreq_hw 0>; + 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 = <&cpufreq_hw 1>; + 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 = <&cpufreq_hw 1>; + 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 = <&cpufreq_hw 1>; + 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 = <&cpufreq_hw 1>; + L2_700: l2-cache { + compatible = "cache"; + next-level-cache = <&L3_0>; + }; + }; + }; +
Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt line 170
Quoted 10 lines
Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt new file +10
-0+ soc { + cpufreq_hw: cpufreq@17d43000 { + compatible = "qcom,cpufreq-hw"; + reg = <0x17d43000 0x1400>, <0x17d45800 0x1400>; + reg-names = "freq-domain0", "freq-domain1"; + + clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>; + clock-names = "xo", "cpu_clk"; + + #freq-domain-cells = <1>
semicolon is missing.
Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt line 171
Quoted 1 line
Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt new file +1
-0+
delete empty line
Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt line 173
Quoted 2 lines
Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt new file +2
-0+ }; +}
Thanks
Matthias
Re: [PATCH 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings
In reply to Taniya DasHi Taniya,
Both the patches are missing v9 in their subject line - this threw off patchwork when trying to download the patches.
Quoted 4 lines · 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 the hardware engine.
I tested these patches on the sdm845-mtp against 4.19 and found that the frequency gets stuck at the highest opp (the boost frequency) after running a couple of 'yes > /dev/null &' instances. Have you tested these against a mainline kernel?
See cpufreq statistics below:
linaro-test [rc=0]# cat policy?/scaling_cur_freq
300000
2803200
linaro-test [rc=0]# cat policy?/stats/time_in_state 300000 100840
403200 388
480000 71
576000 54
652800 22
748800 11
825600 5
902400 5
979200 9
1056000 3
1132800 2
1228800 5
1324800 8
1420800 2
1516800 1
1612800 0
1689600 0
1766400 392
825600 22048
902400 21
979200 4
1056000 15
1209600 6
1286400 0
1363200 1
1459200 0
1536000 0
1612800 1
1689600 0
1766400 0
1843200 2
1920000 2
1996800 0
2092800 0
2169600 0
2246400 0
2323200 0
2400000 0
2476800 0
2553600 0
2649600 0
2707200 0
2764800 0
2784000 0
2803200 79718
Quoted 189 lines
Signed-off-by Taniya Das <[email protected]>--- .../bindings/cpufreq/cpufreq-qcom-hw.txt | 173 +++++++++++++++++++++ 1 file changed, 173 insertions(+) create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txtDocumentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt new file +173
-1@@ -0,0 +1,173 @@ +Qualcomm Technologies, Inc. CPUFREQ Bindings + +CPUFREQ HW 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-hw". + +- clocks + Usage: required + Value type: <phandle> From common clock binding. + Definition: clock handle for XO clock and GPLL0 clock. + +- clock-names + Usage: required + Value type: <string> From common clock binding. + Definition: must be "xo", "cpu_clk". + +- reg + Usage: required + Value type: <prop-encoded-array> + Definition: Addresses and sizes for the memory of the HW bases in + each frequency domain. +- reg-names + Usage: Optional + Value type: <string> + Definition: Frequency domain name i.e. + "freq-domain0", "freq-domain1". + +- freq-domain-cells: + Usage: required. + Definition: Number of cells in a freqency domain specifier. + +* Property qcom,freq-domain +Devices supporting freq-domain must set their "qcom,freq-domain" property with +phandle to a cpufreq_hw followed by the Domain ID(0/1) in the CPU DT node. + + +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 = <&cpufreq_hw 0>; + 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 = <&cpufreq_hw 0>; + 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 = <&cpufreq_hw 0>; + 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 = <&cpufreq_hw 0>; + 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 = <&cpufreq_hw 1>; + 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 = <&cpufreq_hw 1>; + 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 = <&cpufreq_hw 1>; + 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 = <&cpufreq_hw 1>; + L2_700: l2-cache { + compatible = "cache"; + next-level-cache = <&L3_0>; + }; + }; + }; + + soc { + cpufreq_hw: cpufreq@17d43000 { + compatible = "qcom,cpufreq-hw"; + reg = <0x17d43000 0x1400>, <0x17d45800 0x1400>; + reg-names = "freq-domain0", "freq-domain1"; + + clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>; + clock-names = "xo", "cpu_clk"; + + #freq-domain-cells = <1> + + }; +} --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 Firmware bindings
Hi,
Quoted 60 lines · Amit Kucheria
Hi Taniya,
Both the patches are missing v9 in their subject line - this threw off patchwork when trying to download the patches.
Quoted 4 lines · 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 the hardware engine.
I tested these patches on the sdm845-mtp against 4.19 and found that the frequency gets stuck at the highest opp (the boost frequency) after running a couple of 'yes >
/dev/null&' instances. Have you tested these against a mainline kernel?See cpufreq statistics below:
linaro-test [rc=0]# cat policy?/scaling_cur_freq
300000
2803200linaro-test [rc=0]# cat policy?/stats/time_in_state 300000 100840
403200 388
480000 71
576000 54
652800 22
748800 11
825600 5
902400 5
979200 9
1056000 3
1132800 2
1228800 5
1324800 8
1420800 2
1516800 1
1612800 0
1689600 0
1766400 392
825600 22048
902400 21
979200 4
1056000 15
1209600 6
1286400 0
1363200 1
1459200 0
1536000 0
1612800 1
1689600 0
1766400 0
1843200 2
1920000 2
1996800 0
2092800 0
2169600 0
2246400 0
2323200 0
Quoted 8 lines
2400000 0
2476800 0
2553600 0
2649600 0
2707200 0
2764800 0
2784000 0
2803200 79718
I can repro this on SDM845 with a v4.19 kernel.
Since the little cores don't have a boost frequency I think maxing out can be expected with a high workload and no thermal throttling. However the big cores have a boost frequency (2.803 MHz), so the driver shouldn't be stuck at it. Though in practice I also wonder if the ~1% 'boost' makes a big difference in terms of performance or CPU overload ...
Cheers
Matthias
Re: [PATCH 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings
Quoted 74 lines · Matthias Kaehlcke
Hi,
Quoted 60 lines · Amit Kucheria
Hi Taniya,
Both the patches are missing v9 in their subject line - this threw off patchwork when trying to download the patches.
Quoted 4 lines · 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 the hardware engine.
I tested these patches on the sdm845-mtp against 4.19 and found that the frequency gets stuck at the highest opp (the boost frequency) after running a couple of 'yes >
/dev/null&' instances. Have you tested these against a mainline kernel?See cpufreq statistics below:
linaro-test [rc=0]# cat policy?/scaling_cur_freq
300000
2803200linaro-test [rc=0]# cat policy?/stats/time_in_state 300000 100840
403200 388
480000 71
576000 54
652800 22
748800 11
825600 5
902400 5
979200 9
1056000 3
1132800 2
1228800 5
1324800 8
1420800 2
1516800 1
1612800 0
1689600 0
1766400 392
825600 22048
902400 21
979200 4
1056000 15
1209600 6
1286400 0
1363200 1
1459200 0
1536000 0
1612800 1
1689600 0
1766400 0
1843200 2
1920000 2
1996800 0
2092800 0
2169600 0
2246400 0
2323200 0Quoted 8 lines
2400000 0
2476800 0
2553600 0
2649600 0
2707200 0
2764800 0
2784000 0
2803200 79718I can repro this on SDM845 with a v4.19 kernel.
Quoted 8 lines
Since the little cores don't have a boost frequency I think maxing out can be expected with a high workload and no thermal throttling. However the big cores have a boost frequency (2.803 MHz), so the driver shouldn't be stuck at it. Though in practice I also wonder if the ~1% 'boost' makes a big difference in terms of performance or CPU overload ...
From
Documentation/cpu-freq/boost.txt:
"Some CPUs support a functionality to raise the operating frequency of some cores in a multi-core package if certain conditions apply, mostly if the whole chip is not fully utilized and below it's intended thermal budget."
According to this it is not per se an issue that the cores are operating at the boost frequency for a prolonged time. The use of the highest frequency can be expected with a certain system load and the/a boost frequency may be used unless a thermal or other conditions prevents it.
I think the real question is: why is the last frequency automatically considered a boost frequency? On (my) SDM845 it is only about 1% higher than the previous one (2.803 GHz vs 2.784 GHz), that hardly seems like a 'boost'.
Thanks
Matthias
PATCH v9 2/2 cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver
In reply to Taniya DasThe CPUfreq HW present in some QCOM chipsets offloads the steps necessary for changing the frequency of CPUs. The driver implements the cpufreq driver interface for this hardware engine.
--- drivers/cpufreq/Kconfig.arm | 11 ++ drivers/cpufreq/Makefile | 1 + drivers/cpufreq/qcom-cpufreq-hw.c | 354 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 366 insertions(+) create mode 100644 drivers/cpufreq/qcom-cpufreq-hw.c
drivers/cpufreq/Kconfig.arm +11 -0
@@ -121,6 +121,17 @@ config ARM_QCOM_CPUFREQ_KRYO If in doubt, say N. +config ARM_QCOM_CPUFREQ_HW + bool "QCOM CPUFreq HW driver" + depends on ARCH_QCOM || COMPILE_TEST + help + Support for the CPUFreq HW driver. + Some QCOM chipsets have a HW engine to offload the steps + necessary for changing the frequency of the CPUs. Firmware loaded + in this engine exposes a programming interface to the OS. + The driver implements the cpufreq interface for this HW engine. + Say Y if you want to support CPUFreq HW. + config ARM_S3C_CPUFREQ bool help
drivers/cpufreq/Makefile +1 -0
@@ -65,6 +65,7 @@ obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ) += omap-cpufreq.o obj-$(CONFIG_ARM_PXA2xx_CPUFREQ) += pxa2xx-cpufreq.o obj-$(CONFIG_PXA3xx) += pxa3xx-cpufreq.o obj-$(CONFIG_ARM_QCOM_CPUFREQ_KRYO) += qcom-cpufreq-kryo.o +obj-$(CONFIG_ARM_QCOM_CPUFREQ_HW) += qcom-cpufreq-hw.o obj-$(CONFIG_ARM_S3C2410_CPUFREQ) += s3c2410-cpufreq.o obj-$(CONFIG_ARM_S3C2412_CPUFREQ) += s3c2412-cpufreq.o obj-$(CONFIG_ARM_S3C2416_CPUFREQ) += s3c2416-cpufreq.o
drivers/cpufreq/qcom-cpufreq-hw.c new file +354 -0
354 lines not on this page · raw message
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member of the Code Aurora Forum, hosted by the Linux Foundation.
Quoting Taniya Das (2018-10-11 04:36:01)
drivers/cpufreq/Kconfig.arm config ARM_QCOM_CPUFREQ_KRYO · line 125
Quoted 8 lines
drivers/cpufreq/Kconfig.arm +2
-0@@ -121,6 +121,17 @@ config ARM_QCOM_CPUFREQ_KRYO If in doubt, say N. +config ARM_QCOM_CPUFREQ_HW + bool "QCOM CPUFreq HW driver"
Is there any reason this can't be a module?
Quoted 42 lines
drivers/cpufreq/Kconfig.arm +9
-0+ depends on ARCH_QCOM || COMPILE_TEST + help + Support for the CPUFreq HW driver. + Some QCOM chipsets have a HW engine to offload the steps + necessary for changing the frequency of the CPUs. Firmware loaded + in this engine exposes a programming interface to the OS. + The driver implements the cpufreq interface for this HW engine. + Say Y if you want to support CPUFreq HW. + config ARM_S3C_CPUFREQ bool helpdrivers/cpufreq/qcom-cpufreq-hw.c new file +24
-0@@ -0,0 +1,354 @@ +// 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/of_address.h> +#include <linux/of_platform.h> + +#define LUT_MAX_ENTRIES 40U +#define CORE_COUNT_VAL (val) (((val) & (GENMASK(18, 16))) >> 16) +#define LUT_ROW_SIZE 32 +#define CLK_HW_DIV 2 + +enum { +REG_ENABLE , +REG_LUT_TABLE , +REG_PERF_STATE , + +REG_ARRAY_SIZE , +}; +
drivers/cpufreq/qcom-cpufreq-hw.c line 34
Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10
-0+struct cpufreq_qcom { +struct cpufreq_frequency_table *table; +void __iomem *reg_bases[REG_ARRAY_SIZE ]; +cpumask_t related_cpus; +unsigned int max_cores; +unsigned long xo_rate; +unsigned long cpu_hw_rate; +}; + +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE ] = {
Is this going to change in the future?
Quoted 4 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +4
-0+ [REG_ENABLE] = 0x0, + [REG_LUT_TABLE] = 0x110, + [REG_PERF_STATE] = 0x920, +};
drivers/cpufreq/qcom-cpufreq-hw.c line 48
Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10
-0+ +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS ]; + +static int +qcom_cpufreq_hw_target_index (struct cpufreq_policy *policy, +unsigned int index) +{ +struct cpufreq_qcom *c = policy->driver_data; + +writel_relaxed (index, c->reg_bases[REG_PERF_STATE ]);
Why can't we avoid the indirection here and store the perf_state pointer in probe? Then we don't have to indirect through a table to perform the register write.
Quoted 4 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +4
-0+ +return 0 ; +} +
[..]
Quoted 8 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +8
-0+static int qcom_resources_init (struct platform_device *pdev) +{ +struct device_node *cpu_np; +struct of_phandle_args args; +struct clk *clk; +unsigned int cpu; +unsigned long xo_rate, cpu_hw_rate; +int ret;
drivers/cpufreq/qcom-cpufreq-hw.c line 283
Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10
-0+ + clk =clk_get (&pdev->dev,"xo" ); +if (IS_ERR (clk)) +return PTR_ERR (clk); + + xo_rate =clk_get_rate (clk); + +clk_put (clk); + + clk =clk_get (&pdev->dev,"cpu_clk" );
Sad that the name is cpu_clk, instead of something like 'backup' or whatever the name really is in hardware.
Hello Stephen,
Quoted 5 lines · Stephen Boyd
Quoting Taniya Das (2018-10-11 04:36:01)
Quoted 4 lines
drivers/cpufreq/Kconfig.arm +0
-0@@ -121,6 +121,17 @@ config ARM_QCOM_CPUFREQ_KRYO
Quoted 7 lines
drivers/cpufreq/Kconfig.arm config ARM_QCOM_CPUFREQ_KRYO · line 125
Quoted 4 lines
drivers/cpufreq/Kconfig.arm +2
-0If in doubt, say N. +config ARM_QCOM_CPUFREQ_HW + bool "QCOM CPUFreq HW driver"Is there any reason this can't be a module?
We do not have any use cases where we need to support it as module.
Quoted 46 lines
Quoted 46 lines
drivers/cpufreq/Kconfig.arm +9
-0+ depends on ARCH_QCOM || COMPILE_TEST + help + Support for the CPUFreq HW driver. + Some QCOM chipsets have a HW engine to offload the steps + necessary for changing the frequency of the CPUs. Firmware loaded + in this engine exposes a programming interface to the OS. + The driver implements the cpufreq interface for this HW engine. + Say Y if you want to support CPUFreq HW. + config ARM_S3C_CPUFREQ bool helpdrivers/cpufreq/qcom-cpufreq-hw.c new file +28
-0@@ -0,0 +1,354 @@ +// 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/of_address.h> +#include <linux/of_platform.h> + +#define LUT_MAX_ENTRIES 40U +#define CORE_COUNT_VAL (val) (((val) & (GENMASK(18, 16))) >> 16) +#define LUT_ROW_SIZE 32 +#define CLK_HW_DIV 2 + +enum { +REG_ENABLE , +REG_LUT_TABLE , +REG_PERF_STATE , + +REG_ARRAY_SIZE , +}; + +struct cpufreq_qcom { +struct cpufreq_frequency_table *table; +void __iomem *reg_bases[REG_ARRAY_SIZE ]; +cpumask_t related_cpus;
Quoted 9 lines
drivers/cpufreq/qcom-cpufreq-hw.c line 34
Quoted 6 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +6
-0+unsigned int max_cores; +unsigned long xo_rate; +unsigned long cpu_hw_rate; +}; + +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE ] = {Is this going to change in the future?
Yes, they could change and that was the reason to introduce the offsets. This was discussed earlier too with Sudeep and was to add them.
Quoted 10 lines
Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +10
-0+ [REG_ENABLE] = 0x0, + [REG_LUT_TABLE] = 0x110, + [REG_PERF_STATE] = 0x920, +}; + +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS ]; + +static int +qcom_cpufreq_hw_target_index (struct cpufreq_policy *policy, +unsigned int index)
Quoted 9 lines
drivers/cpufreq/qcom-cpufreq-hw.c line 48
Quoted 4 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +4
-0+{ +struct cpufreq_qcom *c = policy->driver_data; + +writel_relaxed (index, c->reg_bases[REG_PERF_STATE ]);Why can't we avoid the indirection here and store the
perf_statepointer in probe? Then we don't have to indirect through a table to perform the register write.
As the offsets could change and that was the reason to add this.
Quoted 18 lines
Quoted 4 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +4
-0+ +return 0 ; +} +[..]
Quoted 13 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +13
-0+static int qcom_resources_init (struct platform_device *pdev) +{ +struct device_node *cpu_np; +struct of_phandle_args args; +struct clk *clk; +unsigned int cpu; +unsigned long xo_rate, cpu_hw_rate; +int ret; + + clk =clk_get (&pdev->dev,"xo" ); +if (IS_ERR (clk)) +return PTR_ERR (clk); +
Quoted 9 lines
drivers/cpufreq/qcom-cpufreq-hw.c line 283
Quoted 5 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +5
-0+ xo_rate =clk_get_rate (clk); + +clk_put (clk); + + clk =clk_get (&pdev->dev,"cpu_clk" );Sad that the name is
cpu_clk, instead of something like 'backup' or whatever the name really is in hardware.
Sure, would update it.
Signature · 4 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation. --
Quoting Taniya Das (2018-11-02 20:06:00)
Quoted 10 lines
Hello Stephen,
Quoted 7 lines · Stephen Boyd
Quoting Taniya Das (2018-10-11 04:36:01)
Quoted 6 lines
drivers/cpufreq/Kconfig.arm +0
-0@@ -121,6 +121,17 @@ config ARM_QCOM_CPUFREQ_KRYO If in doubt, say N.
Quoted 7 lines
Quoted 5 lines
drivers/cpufreq/Kconfig.arm config ARM_QCOM_CPUFREQ_KRYO · line 125
Quoted 2 lines
drivers/cpufreq/Kconfig.arm +2
-0+config ARM_QCOM_CPUFREQ_HW + bool "QCOM CPUFreq HW driver"Is there any reason this can't be a module?
We do not have any use cases where we need to support it as module.
Ok, so it could easily be tristate then? Why not allow it?
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +4
-0@@ -0,0 +1,354 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Copyright (c) 2018, The Linux Foundation. All rights reserved. +*/
[...]
Quoted 10 lines
Quoted 5 lines
drivers/cpufreq/qcom-cpufreq-hw.c
Quoted 2 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +2
-0+ +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE ] = {Is this going to change in the future?
Yes, they could change and that was the reason to introduce the offsets. This was discussed earlier too with Sudeep and was to add them.
Quoted 1 line
Quoted 1 line
+ [
REG_ENABLE] =0x0,
This is only used once? Maybe it could be removed.
Quoted 1 line
Quoted 1 line
Quoted 1 line
+ [
REG_LUT_TABLE] =0x110,
And this is only used during probe to figure out the supported frequencies. So we definitely don't need to store around the registers after probe in an array of iomem pointers. The only one that we need after probe is the one below.
Quoted 18 lines
Quoted 8 lines
Quoted 8 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +8
-0+ [REG_PERF_STATE] = 0x920, +}; + +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS ]; + +static int +qcom_cpufreq_hw_target_index (struct cpufreq_policy *policy, +unsigned int index)Quoted 9 lines
drivers/cpufreq/qcom-cpufreq-hw.c line 48
Quoted 4 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +4
-0+{ +struct cpufreq_qcom *c = policy->driver_data; + +writel_relaxed (index, c->reg_bases[REG_PERF_STATE ]);Why can't we avoid the indirection here and store the
perf_statepointer in probe? Then we don't have to indirect through a table to perform the register write.
Quoted 1 line
As the offsets could change and that was the reason to add this.
With fast switching we can avoid incurring any extra instructions, so please make another iomem pointer in the cpufreq_qcom struct just for writing the index or if possible, just pass the iomem pointer that points to the REG_PERF_STATE as the policy->driver_data variable here. Then we have the address in hand without any extra load. If my understanding is correct, we don't need to keep around anything besides this register address anyway so we should be able to just load it and write it immediately.
Hello Stephen,
Thanks for your comments.
Quoted 17 lines · Stephen Boyd
Quoting Taniya Das (2018-11-02 20:06:00)
Quoted 16 lines
Hello Stephen,
Quoted 12 lines · Stephen Boyd
Quoting Taniya Das (2018-10-11 04:36:01)
drivers/cpufreq/Kconfig.arm config ARM_QCOM_CPUFREQ_KRYO · line 125
Quoted 8 lines
drivers/cpufreq/Kconfig.arm +2
-0@@ -121,6 +121,17 @@ config ARM_QCOM_CPUFREQ_KRYO If in doubt, say N. +config ARM_QCOM_CPUFREQ_HW + bool "QCOM CPUFreq HW driver"Is there any reason this can't be a module?
Quoted 4 lines
Quoted 1 line
We do not have any use cases where we need to support it as module.
Ok, so it could easily be tristate then? Why not allow it?
I have checked other vendors CPUfreq drivers and those too support only "bool".
Quoted 27 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +4
-0@@ -0,0 +1,354 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Copyright (c) 2018, The Linux Foundation. All rights reserved. +*/ [...]
Quoted 10 lines
Quoted 5 lines
drivers/cpufreq/qcom-cpufreq-hw.c
Quoted 2 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +2
-0+ +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE ] = {Is this going to change in the future?
Yes, they could change and that was the reason to introduce the offsets. This was discussed earlier too with Sudeep and was to add them.
Quoted 1 line
Quoted 1 line
+ [
REG_ENABLE] =0x0,This is only used once? Maybe it could be removed.
Quoted 1 line
Quoted 1 line
Quoted 1 line
+ [
REG_LUT_TABLE] =0x110,
Quoted 8 lines
And this is only used during probe to figure out the supported frequencies. So we definitely don't need to store around the registers after probe in an array of iomem pointers. The only one that we need after probe is the one below.
Quoted 3 lines
Quoted 3 lines
drivers/cpufreq/qcom-cpufreq-hw.c line 39
Quoted 3 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +3
-0+ [REG_PERF_STATE] = 0x920, +}; +
As these address offsets could change, so I am of the opinion to leave them as it is.
Quoted 17 lines
Quoted 15 lines
Quoted 5 lines
Quoted 5 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +5
-0+static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS ]; + +static int +qcom_cpufreq_hw_target_index (struct cpufreq_policy *policy, +unsigned int index)Quoted 9 lines
drivers/cpufreq/qcom-cpufreq-hw.c line 48
Quoted 4 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +4
-0+{ +struct cpufreq_qcom *c = policy->driver_data; + +writel_relaxed (index, c->reg_bases[REG_PERF_STATE ]);Why can't we avoid the indirection here and store the
perf_statepointer in probe? Then we don't have to indirect through a table to perform the register write.Quoted 1 line
As the offsets could change and that was the reason to add this.
Quoted 9 lines
With fast switching we can avoid incurring any extra instructions, so please make another iomem pointer in the
cpufreq_qcomstruct just for writing the index or if possible, just pass the iomem pointer that points to theREG_PERF_STATEas thepolicy->driver_datavariable here. Then we have the address in hand without any extra load. If my understanding is correct, we don't need to keep around anything besides this register address anyway so we should be able to just load it and write it immediately.
The c->reg_bases[] is just an index to the updated bases addresses. I am not clear as to why it would incur an extra instruction.
The below code would already take care of it.
+ for (i = REG_ENABLE; i < REG_ARRAY_SIZE; i++)
+ c->reg_bases[i] = base + offsets[i];
+
Signature · 4 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation. --
Quoted 22 lines · Taniya Das
Hello Stephen,
Thanks for your comments.
Quoted 17 lines · Stephen Boyd
Quoting Taniya Das (2018-11-02 20:06:00)
Quoted 16 lines
Hello Stephen,
Quoted 12 lines · Stephen Boyd
Quoting Taniya Das (2018-10-11 04:36:01)
drivers/cpufreq/Kconfig.arm config ARM_QCOM_CPUFREQ_KRYO · line 125
Quoted 8 lines
drivers/cpufreq/Kconfig.arm +2
-0@@ -121,6 +121,17 @@ config ARM_QCOM_CPUFREQ_KRYO If in doubt, say N. +config ARM_QCOM_CPUFREQ_HW + bool "QCOM CPUFreq HW driver"Is there any reason this can't be a module?
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
We do not have any use cases where we need to support it as module.
Ok, so it could easily be tristate then? Why not allow it?
I have checked other vendors CPUfreq drivers and those too support only "bool".
That's not entirely correct. Most drivers in Kconfig are 'tristate' and about 50% of those in KConfig.arm are. I'd say make it 'tristate' unless there are good reasons not to do so.
Quoted 31 lines
Quoted 31 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +4
-0@@ -0,0 +1,354 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Copyright (c) 2018, The Linux Foundation. All rights reserved. +*/ [...]
Quoted 10 lines
Quoted 5 lines
drivers/cpufreq/qcom-cpufreq-hw.c
Quoted 2 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +2
-0+ +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE ] = {Is this going to change in the future?
Yes, they could change and that was the reason to introduce the offsets. This was discussed earlier too with Sudeep and was to add them.
Quoted 1 line
Quoted 1 line
+ [
REG_ENABLE] =0x0,This is only used once? Maybe it could be removed.
Quoted 1 line
Quoted 1 line
Quoted 1 line
+ [
REG_LUT_TABLE] =0x110,And this is only used during probe to figure out the supported frequencies. So we definitely don't need to store around the registers after probe in an array of iomem pointers. The only one that we need after probe is the one below.
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/cpufreq/qcom-cpufreq-hw.c line 39
Quoted 3 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +3
-0+ [REG_PERF_STATE] = 0x920, +}; +As these address offsets could change, so I am of the opinion to leave them as it is.
As of now there is only one set of offsets. Let's just keep the code simple while this is the case and address different offsets when it is actually needed, as suggested by Stephen and Sudeep.
Quoted 36 lines
Quoted 17 lines
Quoted 15 lines
Quoted 5 lines
Quoted 5 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +5
-0+static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS ]; + +static int +qcom_cpufreq_hw_target_index (struct cpufreq_policy *policy, +unsigned int index)Quoted 9 lines
drivers/cpufreq/qcom-cpufreq-hw.c line 48
Quoted 4 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +4
-0+{ +struct cpufreq_qcom *c = policy->driver_data; + +writel_relaxed (index, c->reg_bases[REG_PERF_STATE ]);Why can't we avoid the indirection here and store the
perf_statepointer in probe? Then we don't have to indirect through a table to perform the register write.Quoted 1 line
As the offsets could change and that was the reason to add this.
Quoted 9 lines
With fast switching we can avoid incurring any extra instructions, so please make another iomem pointer in the
cpufreq_qcomstruct just for writing the index or if possible, just pass the iomem pointer that points to theREG_PERF_STATEas thepolicy->driver_datavariable here. Then we have the address in hand without any extra load. If my understanding is correct, we don't need to keep around anything besides this register address anyway so we should be able to just load it and write it immediately.The c->reg_bases[] is just an index to the updated bases addresses. I am not clear as to why it would incur an extra instruction.
The below code would already take care of it.
drivers/cpufreq/qcom-cpufreq-hw.c new file +3
-0+for (i =REG_ENABLE ; i <REG_ARRAY_SIZE ; i++) + c->reg_bases[i] = base + offsets[i]; +
Quoted 1 line
From a performance point of view using a direct iomem pointer
seems like a micro-optimization that probably doesn't have a measurable impact. However I think the code shouldn't be more complex than necessary, and at this point the indirection isn't needed.
Cheers
Matthias
Quoting Matthias Kaehlcke (2018-11-15 16:23:37)
Quoted 22 lines · Taniya Das
Quoted 21 lines · Stephen Boyd
Quoted 19 lines
Quoting Taniya Das (2018-11-02 20:06:00)
Quoted 8 lines · Stephen Boyd
Quoted 7 lines
Quoting Taniya Das (2018-10-11 04:36:01)
Quoted 6 lines
drivers/cpufreq/Kconfig.arm +0
-0@@ -121,6 +121,17 @@ config ARM_QCOM_CPUFREQ_KRYO If in doubt, say N.Quoted 7 lines
Quoted 5 lines
drivers/cpufreq/Kconfig.arm config ARM_QCOM_CPUFREQ_KRYO · line 125
Quoted 2 lines
drivers/cpufreq/Kconfig.arm +2
-0+config ARM_QCOM_CPUFREQ_HW + bool "QCOM CPUFreq HW driver"Is there any reason this can't be a module?
We do not have any use cases where we need to support it as module.
Ok, so it could easily be tristate then? Why not allow it?
Quoted 6 lines
Quoted 2 lines
I have checked other vendors CPUfreq drivers and those too support only "bool".
That's not entirely correct. Most drivers in
Kconfigare 'tristate' and about 50% of those in KConfig.arm are. I'd say make it 'tristate' unless there are good reasons not to do so.
Yes, please make tristate.
Quoted 39 lines
Quoted 32 lines
Quoted 32 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +4
-0@@ -0,0 +1,354 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Copyright (c) 2018, The Linux Foundation. All rights reserved. +*/ [...]
Quoted 10 lines
Quoted 5 lines
drivers/cpufreq/qcom-cpufreq-hw.c
Quoted 2 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +2
-0+ +static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE ] = {Is this going to change in the future?
Yes, they could change and that was the reason to introduce the offsets. This was discussed earlier too with Sudeep and was to add them.
Quoted 1 line
Quoted 1 line
+ [
REG_ENABLE] =0x0,This is only used once? Maybe it could be removed.
Quoted 1 line
Quoted 1 line
Quoted 1 line
+ [
REG_LUT_TABLE] =0x110,And this is only used during probe to figure out the supported frequencies. So we definitely don't need to store around the registers after probe in an array of iomem pointers. The only one that we need after probe is the one below.
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/cpufreq/qcom-cpufreq-hw.c line 39
Quoted 3 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +3
-0+ [REG_PERF_STATE] = 0x920, +}; +As these address offsets could change, so I am of the opinion to leave them as it is.
Quoted 3 lines
As of now there is only one set of offsets. Let's just keep the code simple while this is the case and address different offsets when it is actually needed, as suggested by Stephen and Sudeep.
Yes, please simplify by getting rid of this and not storing anything in the struct that's only used during probe.
Quoted 17 lines
Quoted 17 lines
Quoted 11 lines
With fast switching we can avoid incurring any extra instructions, so please make another iomem pointer in the
cpufreq_qcomstruct just for writing the index or if possible, just pass the iomem pointer that points to theREG_PERF_STATEas thepolicy->driver_datavariable here. Then we have the address in hand without any extra load. If my understanding is correct, we don't need to keep around anything besides this register address anyway so we should be able to just load it and write it immediately.The c->reg_bases[] is just an index to the updated bases addresses. I am not clear as to why it would incur an extra instruction.
The below code would already take care of it.
Quoted 9 lines
drivers/cpufreq/qcom-cpufreq-hw.c line 234
Quoted 3 lines
drivers/cpufreq/qcom-cpufreq-hw.c new file +3
-0+for (i =REG_ENABLE ; i <REG_ARRAY_SIZE ; i++) + c->reg_bases[i] = base + offsets[i]; +From a performance point of view using a direct iomem pointer seems like a micro-optimization that probably doesn't have a measurable impact. However I think the code shouldn't be more complex than necessary, and at this point the indirection isn't needed.
Yes it's a micro-optimization for sure, in the task switching path so it may actually be useful. Either way, I think we can greatly simplify by just having the iomem pointer be the only pointer that is stored in the policy driver_data.
Quoted 6 lines · Taniya Das
Hello Stephen,
Quoted 3 lines · Stephen Boyd
Quoting Taniya Das (2018-10-11 04:36:01)
Quoted 2 lines
drivers/cpufreq/Kconfig.arm +0
-0
[...]
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
+static const u16 cpufreq_qcom_std_offsets[REG_ARRAY_SIZE] = {
Is this going to change in the future?
Yes, they could change and that was the reason to introduce the offsets. This was discussed earlier too with Sudeep and was to add them.
Sorry, I didn't like these registers to be coming from DT and I had the same question: will this keep changing ? And IIRC, the answer was yes. But I agree with Stephen, if and when we see the change, you can introduce the array and keep it simple until then.
--
Regards,
Sudeep