Add support for assigned-performance-states for geni i2c driver
Subsystems (7)
cover Add support for assigned-performance-states for geni i2c driver
Roja Rani Yarubandi (3):
dt-bindings: power: Introduce 'assigned-performance-states' property
arm64: dts: sc7180: Add assigned-performance-states for i2c
i2c: i2c-qcom-geni: Add support for 'assigned-performance-states'
.../bindings/power/power-domain.yaml | 49 +++++++++++++++++++ arch/arm64/boot/dts/qcom/sc7180.dtsi | 24 +++++++++ drivers/i2c/busses/i2c-qcom-geni.c | 49 +++++++++++++++++++ 3 files changed, 122 insertions(+)
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH 1/3 dt-bindings: power: Introduce 'assigned-performance-states' property
While most devices within power-domains which support performance states, scale the performance state dynamically, some devices might want to set a static/default performance state while the device is active. These devices typically would also run off a fixed clock and not support dynamically scaling the device's performance, also known as DVFS techniques.
Add a property 'assigned-performance-states' which client devices can use to set this default performance state on their power-domains.
--- .../bindings/power/power-domain.yaml | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+)
Documentation/devicetree/bindings/power/power-domain.yaml +49 -0
@@ -66,6 +66,18 @@ properties: by the given provider should be subdomains of the domain specified by this binding. + assigned-performance-states: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: + Some devices might need to configure their power domains in a default + performance state while the device is active. These devices typcially + would also run off a fixed clock and not support dynamically scaling + the device's performance, also known as DVFS techniques. Each cell in + performance state value corresponds to one power domain specified as + part of the power-domains property. Performance state value can be an + opp-level inside an OPP table of the power-domain and need not match + with any OPP table performance state. + required: - "#power-domain-cells"
@@ -131,3 +143,40 @@ examples: min-residency-us = <7000>; }; }; + + - | + parent4: power-controller@12340000 { + compatible = "foo,power-controller"; + reg = <0x12340000 0x1000>; + #power-domain-cells = <0>; + }; + + parent5: power-controller@43210000 { + compatible = "foo,power-controller"; + reg = <0x43210000 0x1000>; + #power-domain-cells = <0>; + operating-points-v2 = <&power_opp_table>; + + power_opp_table: opp-table { + compatible = "operating-points-v2"; + + power_opp_low: opp1 { + opp-level = <16>; + }; + + rpmpd_opp_ret: opp2 { + opp-level = <64>; + }; + + rpmpd_opp_svs: opp3 { + opp-level = <256>; + }; + }; + }; + + child4: consumer@12341000 { + compatible = "foo,consumer"; + reg = <0x12341000 0x1000>; + power-domains = <&parent4>, <&parent5>; + assigned-performance-states = <0>, <256>; + };
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Rob Herring
maintainerRe: [PATCH 1/3] dt-bindings: power: Introduce 'assigned-performance-states' property
Quoted 7 lines · Roja Rani Yarubandi
While most devices within power-domains which support performance states, scale the performance state dynamically, some devices might want to set a static/default performance state while the device is active. These devices typically would also run off a fixed clock and not support dynamically scaling the device's performance, also known as DVFS techniques.
Quoted 8 lines
Add a property 'assigned-performance-states' which client devices can use to set this default performance state on their power-domains.
Signed-off-by Roja Rani Yarubandi <[email protected]>--- .../bindings/power/power-domain.yaml | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+)
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:./Documentation/devicetree/bindings/power/power-domain.yaml:72:8: [warning] wrong indentation: expected 6 but found 7 (indentation)
dtschema/dtc warnings/errors:
See https://patchwork.ozlabs.org/patch/1420485
This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:
pip3 install dtschema --upgrade
Please check and re-submit.
Rob Herring
maintainerRe: [PATCH 1/3] dt-bindings: power: Introduce 'assigned-performance-states' property
In reply to Roja Rani YarubandiQuoted 7 lines · Roja Rani Yarubandi
While most devices within power-domains which support performance states, scale the performance state dynamically, some devices might want to set a static/default performance state while the device is active. These devices typically would also run off a fixed clock and not support dynamically scaling the device's performance, also known as DVFS techniques.
Quoted 8 lines
Add a property 'assigned-performance-states' which client devices can use to set this default performance state on their power-domains.
Signed-off-by Roja Rani Yarubandi <[email protected]>--- .../bindings/power/power-domain.yaml | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+)
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:./Documentation/devicetree/bindings/power/power-domain.yaml:72:8: [warning] wrong indentation: expected 6 but found 7 (indentation)
dtschema/dtc warnings/errors:
See https://patchwork.ozlabs.org/patch/1420485
This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:
pip3 install dtschema --upgrade
Please check and re-submit.
Rob Herring
maintainerRe: [PATCH 1/3] dt-bindings: power: Introduce 'assigned-performance-states' property
In reply to Roja Rani YarubandiQuoted 24 lines · Roja Rani Yarubandi
While most devices within power-domains which support performance states, scale the performance state dynamically, some devices might want to set a static/default performance state while the device is active. These devices typically would also run off a fixed clock and not support dynamically scaling the device's performance, also known as DVFS techniques.
Add a property 'assigned-performance-states' which client devices can use to set this default performance state on their power-domains.
Signed-off-by Roja Rani Yarubandi <[email protected]>--- .../bindings/power/power-domain.yaml | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+)Documentation/devicetree/bindings/power/power-domain.yaml +1
-0@@ -66,6 +66,18 @@ properties: by the given provider should be subdomains of the domain specified by this binding. + assigned-performance-states:
Documentation/devicetree/bindings/power/power-domain.yaml properties: · line 79
Quoted 10 lines
Documentation/devicetree/bindings/power/power-domain.yaml +10
-0+ $ref: /schemas/types.yaml#/definitions/uint32-array + description: + Some devices might need to configure their power domains in a default + performance state while the device is active. These devices typcially + would also run off a fixed clock and not support dynamically scaling + the device's performance, also known as DVFS techniques. Each cell in + performance state value corresponds to one power domain specified as + part of the power-domains property. Performance state value can be an + opp-level inside an OPP table of the power-domain and need not match + with any OPP table performance state.
Couldn't this just be an additional cell in 'power-domains'?
Quoted 49 lines
Documentation/devicetree/bindings/power/power-domain.yaml +38
-0+ required: - "#power-domain-cells"@@ -131,3 +143,40 @@ examples: min-residency-us = <7000>; }; }; + + - | + parent4: power-controller@12340000 { + compatible = "foo,power-controller"; + reg = <0x12340000 0x1000>; + #power-domain-cells = <0>; + }; + + parent5: power-controller@43210000 { + compatible = "foo,power-controller"; + reg = <0x43210000 0x1000>; + #power-domain-cells = <0>; + operating-points-v2 = <&power_opp_table>; + + power_opp_table: opp-table { + compatible = "operating-points-v2"; + + power_opp_low: opp1 { + opp-level = <16>; + }; + + rpmpd_opp_ret: opp2 { + opp-level = <64>; + }; + + rpmpd_opp_svs: opp3 { + opp-level = <256>; + }; + }; + }; + + child4: consumer@12341000 { + compatible = "foo,consumer"; + reg = <0x12341000 0x1000>; + power-domains = <&parent4>, <&parent5>; + assigned-performance-states = <0>, <256>; + };Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 1/3] dt-bindings: power: Introduce 'assigned-performance-states' property
Quoted 25 lines · Rob Herring
Quoted 23 lines · Roja Rani Yarubandi
While most devices within power-domains which support performance states, scale the performance state dynamically, some devices might want to set a static/default performance state while the device is active. These devices typically would also run off a fixed clock and not support dynamically scaling the device's performance, also known as DVFS techniques.
Add a property 'assigned-performance-states' which client devices can use to set this default performance state on their power-domains.
Signed-off-by Roja Rani Yarubandi <[email protected]>--- .../bindings/power/power-domain.yaml | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+)Documentation/devicetree/bindings/power/power-domain.yaml +0
-0@@ -66,6 +66,18 @@ properties: by the given provider should be subdomains of the domain specified by this binding.
Quoted 13 lines
Documentation/devicetree/bindings/power/power-domain.yaml properties: · line 79
Quoted 11 lines
Documentation/devicetree/bindings/power/power-domain.yaml +11
-0+ assigned-performance-states: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: + Some devices might need to configure their power domains in a default + performance state while the device is active. These devices typcially + would also run off a fixed clock and not support dynamically scaling + the device's performance, also known as DVFS techniques. Each cell in + performance state value corresponds to one power domain specified as + part of the power-domains property. Performance state value can be an + opp-level inside an OPP table of the power-domain and need not match + with any OPP table performance state.Couldn't this just be an additional cell in 'power-domains'?
Right. Some SoCs already use the cell to specify per device SoC specific data [1].
Although, I am wondering if we shouldn't consider "assigned-performance-states" as a more generic binding. I think it would be somewhat comparable with the existing "assigned-clock-rates" binding, don't you think?
[...]
Kind regards
Uffe
[1] Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
Re: [PATCH 1/3] dt-bindings: power: Introduce 'assigned-performance-states' property
In reply to Roja Rani YarubandiQuoted 68 lines · Roja Rani Yarubandi
While most devices within power-domains which support performance states, scale the performance state dynamically, some devices might want to set a static/default performance state while the device is active. These devices typically would also run off a fixed clock and not support dynamically scaling the device's performance, also known as DVFS techniques.
Add a property 'assigned-performance-states' which client devices can use to set this default performance state on their power-domains.
Signed-off-by Roja Rani Yarubandi <[email protected]>--- .../bindings/power/power-domain.yaml | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+)Documentation/devicetree/bindings/power/power-domain.yaml +38
-0@@ -66,6 +66,18 @@ properties: by the given provider should be subdomains of the domain specified by this binding. + assigned-performance-states: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: + Some devices might need to configure their power domains in a default + performance state while the device is active. These devices typcially + would also run off a fixed clock and not support dynamically scaling + the device's performance, also known as DVFS techniques. Each cell in + performance state value corresponds to one power domain specified as + part of the power-domains property. Performance state value can be an + opp-level inside an OPP table of the power-domain and need not match + with any OPP table performance state. + required: - "#power-domain-cells"@@ -131,3 +143,40 @@ examples: min-residency-us = <7000>; }; }; + + - | + parent4: power-controller@12340000 { + compatible = "foo,power-controller"; + reg = <0x12340000 0x1000>; + #power-domain-cells = <0>; + }; + + parent5: power-controller@43210000 { + compatible = "foo,power-controller"; + reg = <0x43210000 0x1000>; + #power-domain-cells = <0>; + operating-points-v2 = <&power_opp_table>; + + power_opp_table: opp-table { + compatible = "operating-points-v2"; + + power_opp_low: opp1 { + opp-level = <16>; + }; + + rpmpd_opp_ret: opp2 { + opp-level = <64>; + }; + + rpmpd_opp_svs: opp3 {
Documentation/devicetree/bindings/power/power-domain.yaml examples: · line 181
Quoted 10 lines
Documentation/devicetree/bindings/power/power-domain.yaml +10
-0+ opp-level = <256>; + }; + }; + }; + + child4: consumer@12341000 { + compatible = "foo,consumer"; + reg = <0x12341000 0x1000>; + power-domains = <&parent4>, <&parent5>; + assigned-performance-states = <0>, <256>;
May I ask how this is different from saying something like:
required-opps = <&??>, <&rpmpd_opp_svs>:
Regards,
Bjorn
Quoted 5 lines
Documentation/devicetree/bindings/power/power-domain.yaml +1
-0+ };Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 1/3] dt-bindings: power: Introduce 'assigned-performance-states' property
Quoted 83 lines · Bjorn Andersson
Quoted 68 lines · Roja Rani Yarubandi
While most devices within power-domains which support performance states, scale the performance state dynamically, some devices might want to set a static/default performance state while the device is active. These devices typically would also run off a fixed clock and not support dynamically scaling the device's performance, also known as DVFS techniques.
Add a property 'assigned-performance-states' which client devices can use to set this default performance state on their power-domains.
Signed-off-by Roja Rani Yarubandi <[email protected]>--- .../bindings/power/power-domain.yaml | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+)Documentation/devicetree/bindings/power/power-domain.yaml +38
-0@@ -66,6 +66,18 @@ properties: by the given provider should be subdomains of the domain specified by this binding. + assigned-performance-states: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: + Some devices might need to configure their power domains in a default + performance state while the device is active. These devices typcially + would also run off a fixed clock and not support dynamically scaling + the device's performance, also known as DVFS techniques. Each cell in + performance state value corresponds to one power domain specified as + part of the power-domains property. Performance state value can be an + opp-level inside an OPP table of the power-domain and need not match + with any OPP table performance state. + required: - "#power-domain-cells"@@ -131,3 +143,40 @@ examples: min-residency-us = <7000>; }; }; + + - | + parent4: power-controller@12340000 { + compatible = "foo,power-controller"; + reg = <0x12340000 0x1000>; + #power-domain-cells = <0>; + }; + + parent5: power-controller@43210000 { + compatible = "foo,power-controller"; + reg = <0x43210000 0x1000>; + #power-domain-cells = <0>; + operating-points-v2 = <&power_opp_table>; + + power_opp_table: opp-table { + compatible = "operating-points-v2"; + + power_opp_low: opp1 { + opp-level = <16>; + }; + + rpmpd_opp_ret: opp2 { + opp-level = <64>; + }; + + rpmpd_opp_svs: opp3 {Documentation/devicetree/bindings/power/power-domain.yaml examples: · line 181
Quoted 10 lines
Documentation/devicetree/bindings/power/power-domain.yaml +10
-0+ opp-level = <256>; + }; + }; + }; + + child4: consumer@12341000 { + compatible = "foo,consumer"; + reg = <0x12341000 0x1000>; + power-domains = <&parent4>, <&parent5>; + assigned-performance-states = <0>, <256>;May I ask how this is different from saying something like:
Quoted 1 line
required-opps = <&??>, <&rpmpd_opp_svs>:
I think its potentially the same. We just don't have any code to handle this binding in kernel yet (when this property is part of the device/consumer node)
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH 2/3 arm64: dts: sc7180: Add assigned-performance-states for i2c
In reply to Roja Rani Yarubandiqup-i2c devices on sc7180 are clocked with a fixed clock (19.2 MHz). Though qup-i2c does not support DVFS, it still needs to vote for a performance state on 'CX' to satisfy the 19.2 MHz clock frequency requirement.
Use 'assigned-performance-states' to pass this information from device tree, and also add the power-domains property to specify the CX power-domain.
--- arch/arm64/boot/dts/qcom/sc7180.dtsi | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
arch/arm64/boot/dts/qcom/sc7180.dtsi +24 -0
@@ -782,6 +782,8 @@ i2c0: i2c@880000 { <&aggre1_noc MASTER_QUP_0 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + assigned-performance-states = <RPMH_REGULATOR_LEVEL_LOW_SVS>; status = "disabled"; };
@@ -834,6 +836,8 @@ i2c1: i2c@884000 { <&aggre1_noc MASTER_QUP_0 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + assigned-performance-states = <RPMH_REGULATOR_LEVEL_LOW_SVS>; status = "disabled"; };
@@ -886,6 +890,8 @@ i2c2: i2c@888000 { <&aggre1_noc MASTER_QUP_0 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + assigned-performance-states = <RPMH_REGULATOR_LEVEL_LOW_SVS>; status = "disabled"; };
@@ -920,6 +926,8 @@ i2c3: i2c@88c000 { <&aggre1_noc MASTER_QUP_0 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + assigned-performance-states = <RPMH_REGULATOR_LEVEL_LOW_SVS>; status = "disabled"; };
@@ -972,6 +980,8 @@ i2c4: i2c@890000 { <&aggre1_noc MASTER_QUP_0 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + assigned-performance-states = <RPMH_REGULATOR_LEVEL_LOW_SVS>; status = "disabled"; };
@@ -1006,6 +1016,8 @@ i2c5: i2c@894000 { <&aggre1_noc MASTER_QUP_0 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + assigned-performance-states = <RPMH_REGULATOR_LEVEL_LOW_SVS>; status = "disabled"; };
@@ -1073,6 +1085,8 @@ i2c6: i2c@a80000 { <&aggre2_noc MASTER_QUP_1 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + assigned-performance-states = <RPMH_REGULATOR_LEVEL_LOW_SVS>; status = "disabled"; };
@@ -1125,6 +1139,8 @@ i2c7: i2c@a84000 { <&aggre2_noc MASTER_QUP_1 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + assigned-performance-states = <RPMH_REGULATOR_LEVEL_LOW_SVS>; status = "disabled"; };
@@ -1159,6 +1175,8 @@ i2c8: i2c@a88000 { <&aggre2_noc MASTER_QUP_1 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + assigned-performance-states = <RPMH_REGULATOR_LEVEL_LOW_SVS>; status = "disabled"; };
@@ -1211,6 +1229,8 @@ i2c9: i2c@a8c000 { <&aggre2_noc MASTER_QUP_1 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + assigned-performance-states = <RPMH_REGULATOR_LEVEL_LOW_SVS>; status = "disabled"; };
@@ -1245,6 +1265,8 @@ i2c10: i2c@a90000 { <&aggre2_noc MASTER_QUP_1 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + assigned-performance-states = <RPMH_REGULATOR_LEVEL_LOW_SVS>; status = "disabled"; };
@@ -1297,6 +1319,8 @@ i2c11: i2c@a94000 { <&aggre2_noc MASTER_QUP_1 0 &mc_virt SLAVE_EBI1 0>; interconnect-names = "qup-core", "qup-config", "qup-memory"; + power-domains = <&rpmhpd SC7180_CX>; + assigned-performance-states = <RPMH_REGULATOR_LEVEL_LOW_SVS>; status = "disabled"; };
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH 3/3 i2c: i2c-qcom-geni: Add support for 'assigned-performance-states'
In reply to Roja Rani YarubandiFor devices which have 'assigned-performance-states' specified in DT, set the specified performance state during probe and drop it on remove. Also drop/set as part of runtime suspend/resume callbacks.
--- drivers/i2c/busses/i2c-qcom-geni.c | 49 ++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)
drivers/i2c/busses/i2c-qcom-geni.c +49 -0
@@ -11,6 +11,7 @@#include <linux/module.h> #include <linux/of.h> #include <linux/platform_device.h> +#include <linux/pm_domain.h> #include <linux/pm_runtime.h> #include <linux/qcom-geni-se.h> #include <linux/spinlock.h>
@@ -86,6 +87,7 @@ struct geni_i2c_dev {u32 clk_freq_out;const struct geni_i2c_clk_fld *clk_fld;int suspended; +unsigned int assigned_pstate; };struct geni_i2c_err_log {
@@ -497,6 +499,7 @@ static int geni_i2c_probe(struct platform_device *pdev)u32 proto, tx_depth;int ret;struct device *dev = &pdev->dev; +unsigned int assigned_pstate; gi2c =devm_kzalloc (dev,sizeof (*gi2c),GFP_KERNEL );if (!gi2c)
@@ -520,6 +523,20 @@ static int geni_i2c_probe(struct platform_device *pdev) gi2c->clk_freq_out =KHZ (100 ); } +/* Set the assigned performance state */ +if (!of_property_read_u32 (pdev->dev.of_node,"assigned-performance-states" , + &assigned_pstate)) { +if (assigned_pstate) { + ret =dev_pm_genpd_set_performance_state (dev, + assigned_pstate); +if (ret) { +dev_err (dev,"Failed to set performance state\n" ); +return ret; + } + gi2c->assigned_pstate = assigned_pstate; + } + } +if (has_acpi_companion (dev))ACPI_COMPANION_SET (&gi2c->adap.dev,ACPI_COMPANION (dev));
@@ -616,10 +633,22 @@ static int geni_i2c_probe(struct platform_device *pdev)static int geni_i2c_remove (struct platform_device *pdev) { +int ret; +struct device *dev = &pdev->dev;struct geni_i2c_dev *gi2c =platform_get_drvdata (pdev);i2c_del_adapter (&gi2c->adap);pm_runtime_disable (gi2c->se.dev); + +/* Drop the assigned performance state */ +if (gi2c->assigned_pstate) { + ret =dev_pm_genpd_set_performance_state (dev,0 ); +if (ret) { +dev_err (dev,"Failed to set performance state\n" ); +return ret; + } + } +return 0 ; }
@@ -629,6 +658,16 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)struct geni_i2c_dev *gi2c =dev_get_drvdata (dev);disable_irq (gi2c->irq); + +/* Drop the assigned performance state */ +if (gi2c->assigned_pstate) { + ret =dev_pm_genpd_set_performance_state (dev,0 ); +if (ret) { +dev_err (dev,"Failed to set performance state\n" ); +return ret; + } + } + ret =geni_se_resources_off (&gi2c->se);if (ret) {enable_irq (gi2c->irq);
@@ -654,6 +693,16 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)if (ret)return ret; +/* Set the assigned performance state */ +if (gi2c->assigned_pstate) { + ret =dev_pm_genpd_set_performance_state (dev, + gi2c->assigned_pstate); +if (ret) { +dev_err (dev,"Failed to set performance state\n" ); +return ret; + } + } +enable_irq (gi2c->irq); gi2c->suspended =0 ;return 0 ;
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 3/3] i2c: i2c-qcom-geni: Add support for 'assigned-performance-states'
Quoted 4 lines · Roja Rani Yarubandi
drivers/i2c/busses/i2c-qcom-geni.c +0
-0@@ -629,6 +658,16 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)struct geni_i2c_dev *gi2c =dev_get_drvdata (dev);disable_irq (gi2c->irq);
drivers/i2c/busses/i2c-qcom-geni.c static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev) · line 670
Quoted 10 lines
drivers/i2c/busses/i2c-qcom-geni.c +10
-0+ +/* Drop the assigned performance state */ +if (gi2c->assigned_pstate) { + ret =dev_pm_genpd_set_performance_state (dev,0 ); +if (ret) { +dev_err (dev,"Failed to set performance state\n" ); +return ret; + } + } +
Ulf, Viresh, I think we discussed this at the time of introducing the performance states.
The client's state does not affect if its performance_state should be included in the calculation of the aggregated performance_state, so each driver that needs to keep some minimum performance state needs to have these two snippets.
Would it not make sense to on enable/disable re-evaluate the performance_state and potentially reconfigure the hardware automatically?
Regards,
Bjorn
Quoted 24 lines
drivers/i2c/busses/i2c-qcom-geni.c +10
-0ret =geni_se_resources_off (&gi2c->se);if (ret) {enable_irq (gi2c->irq);@@ -654,6 +693,16 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)if (ret)return ret; +/* Set the assigned performance state */ +if (gi2c->assigned_pstate) { + ret =dev_pm_genpd_set_performance_state (dev, + gi2c->assigned_pstate); +if (ret) { +dev_err (dev,"Failed to set performance state\n" ); +return ret; + } + } +enable_irq (gi2c->irq); gi2c->suspended =0 ;return 0 ;Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 3/3] i2c: i2c-qcom-geni: Add support for 'assigned-performance-states'
Quoted 20 lines · Bjorn Andersson
Quoted 4 lines · Roja Rani Yarubandi
drivers/i2c/busses/i2c-qcom-geni.c +0
-0@@ -629,6 +658,16 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)struct geni_i2c_dev *gi2c =dev_get_drvdata (dev);disable_irq (gi2c->irq);drivers/i2c/busses/i2c-qcom-geni.c static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev) · line 670
Quoted 10 lines
drivers/i2c/busses/i2c-qcom-geni.c +10
-0+ +/* Drop the assigned performance state */ +if (gi2c->assigned_pstate) { + ret =dev_pm_genpd_set_performance_state (dev,0 ); +if (ret) { +dev_err (dev,"Failed to set performance state\n" ); +return ret; + } + } +Ulf, Viresh, I think we discussed this at the time of introducing the performance states.
Quoted 8 lines
The client's state does not affect if its
performance_stateshould be included in the calculation of the aggregatedperformance_state, so each driver that needs to keep some minimum performance state needs to have these two snippets.Would it not make sense to on enable/disable re-evaluate the
performance_stateand potentially reconfigure the hardware automatically?
I agree, this will be repeated across multiple drivers which would need some minimal vote while they are active, handling this during genpd enable/disable in genpd core makes sense.
Quoted 28 lines
Regards,
BjornQuoted 24 lines
drivers/i2c/busses/i2c-qcom-geni.c +10
-0ret =geni_se_resources_off (&gi2c->se);if (ret) {enable_irq (gi2c->irq);@@ -654,6 +693,16 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)if (ret)return ret; +/* Set the assigned performance state */ +if (gi2c->assigned_pstate) { + ret =dev_pm_genpd_set_performance_state (dev, + gi2c->assigned_pstate); +if (ret) { +dev_err (dev,"Failed to set performance state\n" ); +return ret; + } + } +enable_irq (gi2c->irq); gi2c->suspended =0 ;return 0 ;Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 3/3] i2c: i2c-qcom-geni: Add support for 'assigned-performance-states'
Quoted 28 lines · Rajendra Nayak
Quoted 25 lines · Bjorn Andersson
Quoted 4 lines · Roja Rani Yarubandi
drivers/i2c/busses/i2c-qcom-geni.c +0
-0@@ -629,6 +658,16 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)struct geni_i2c_dev *gi2c =dev_get_drvdata (dev);disable_irq (gi2c->irq);drivers/i2c/busses/i2c-qcom-geni.c static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev) · line 670
Quoted 10 lines
drivers/i2c/busses/i2c-qcom-geni.c +10
-0+ +/* Drop the assigned performance state */ +if (gi2c->assigned_pstate) { + ret =dev_pm_genpd_set_performance_state (dev,0 ); +if (ret) { +dev_err (dev,"Failed to set performance state\n" ); +return ret; + } + } +Ulf, Viresh, I think we discussed this at the time of introducing the performance states.
The client's state does not affect if its
performance_stateshould be included in the calculation of the aggregatedperformance_state, so each driver that needs to keep some minimum performance state needs to have these two snippets.
Quoted 7 lines
Quoted 3 lines
Would it not make sense to on enable/disable re-evaluate the
performance_stateand potentially reconfigure the hardware automatically?I agree, this will be repeated across multiple drivers which would need some minimal vote while they are active, handling this during genpd enable/disable in genpd core makes sense.
Initially that's what we tried out, but we realized that it was difficult to deal with this internally in genpd, but more importantly it also removed some flexibility from consumers and providers. See commit 68de2fe57a8f ("PM / Domains: Make genpd performance states orthogonal to the idlestates").
As a matter of fact this was quite recently discussed [1], which also pointed out some issues when using the "required-opps" in combination, but perhaps that got resolved? Viresh?
My concern is, if we would make this kind of change to the internals of genpd, it would lead to the following limitation: A consumer driver can no longer make its vote for its device to stick around, when the device becomes runtime suspended - and how do we know that we never need to support such a case?
Quoted 12 lines
Quoted 12 lines
Regards,
BjornQuoted 7 lines
drivers/i2c/busses/i2c-qcom-geni.c +0
-0ret =geni_se_resources_off (&gi2c->se);if (ret) {enable_irq (gi2c->irq);@@ -654,6 +693,16 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)if (ret)return ret;
Quoted 13 lines
Quoted 5 lines
Quoted 5 lines
drivers/i2c/busses/i2c-qcom-geni.c +5
-0+/* Set the assigned performance state */ +if (gi2c->assigned_pstate) { + ret =dev_pm_genpd_set_performance_state (dev, + gi2c->assigned_pstate); +if (ret) {Quoted 8 lines
drivers/i2c/busses/i2c-qcom-geni.c static int __maybe_unused geni_i2c_runtime_resume(struct device *dev) · line 708
Quoted 8 lines
drivers/i2c/busses/i2c-qcom-geni.c +5
-0+dev_err (dev,"Failed to set performance state\n" ); +return ret; + } + } +enable_irq (gi2c->irq); gi2c->suspended =0 ;return 0 ;
Kind regards
Uffe
Re: [PATCH 3/3] i2c: i2c-qcom-geni: Add support for 'assigned-performance-states'
Quoted 3 lines · Ulf Hansson
As a matter of fact this was quite recently discussed [1], which also pointed out some issues when using the "required-opps" in combination, but perhaps that got resolved? Viresh?
Perhaps we never did anything there ..
Signature · 1 line
viresh
Re: [PATCH 3/3] i2c: i2c-qcom-geni: Add support for 'assigned-performance-states'
Quoted 7 lines · Viresh Kumar
Quoted 3 lines · Ulf Hansson
As a matter of fact this was quite recently discussed [1], which also pointed out some issues when using the "required-opps" in combination, but perhaps that got resolved? Viresh?
Perhaps we never did anything there ..
Okay. Looks like we should pick up that discussion again, to conclude on how to move forward.
Quoted 3 lines
--
viresh
Kind regards
Uffe
Re: [PATCH 3/3] i2c: i2c-qcom-geni: Add support for 'assigned-performance-states'
Quoted 12 lines · Ulf Hansson
Quoted 7 lines · Viresh Kumar
Quoted 3 lines · Ulf Hansson
As a matter of fact this was quite recently discussed [1], which also pointed out some issues when using the "required-opps" in combination, but perhaps that got resolved? Viresh?
Perhaps we never did anything there ..
Okay. Looks like we should pick up that discussion again, to conclude on how to move forward.
Soft Reminder!
Quoted 6 lines
Quoted 3 lines
--
vireshKind regards
Uffe
- Roja
Re: [PATCH 3/3] i2c: i2c-qcom-geni: Add support for 'assigned-performance-states'
Quoted 8 lines · On 2021-02-12 14:51, [email protected] wrote:
Quoted 7 lines · Ulf Hansson
Quoted 6 lines · Viresh Kumar
Quoted 3 lines · Ulf Hansson
As a matter of fact this was quite recently discussed [1], which also pointed out some issues when using the "required-opps" in combination, but perhaps that got resolved? Viresh?
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
Perhaps we never did anything there ..
Okay. Looks like we should pick up that discussion again, to conclude on how to move forward.
Soft Reminder!
Request Viresh, Uffe to discuss on the way forward.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
--
vireshKind regards
Uffe- Roja
Re: [PATCH 3/3] i2c: i2c-qcom-geni: Add support for 'assigned-performance-states'
Quoted 11 lines · On 2021-04-01 12:09, [email protected] wrote:
Quoted 10 lines · On 2021-02-12 14:51, [email protected] wrote:
Quoted 9 lines · Ulf Hansson
Quoted 7 lines · Viresh Kumar
Quoted 3 lines · Ulf Hansson
As a matter of fact this was quite recently discussed [1], which also pointed out some issues when using the "required-opps" in combination, but perhaps that got resolved? Viresh?
Perhaps we never did anything there ..
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
Okay. Looks like we should pick up that discussion again, to conclude on how to move forward.
Soft Reminder!
Request Viresh, Uffe to discuss on the way forward.
Hi Viresh, Uffe, looking forward for your discussion/updates.
Thanks,
Roja
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
--
vireshKind regards
Uffe- Roja
Re: [PATCH 3/3] i2c: i2c-qcom-geni: Add support for 'assigned-performance-states'
In reply to Ulf HanssonSorry Roja for dragging this too long, unfortunately I didn't have a lot to add on. Lemme try start this thread again.
Quoted 43 lines · Ulf Hansson
Quoted 28 lines · Rajendra Nayak
Quoted 25 lines · Bjorn Andersson
Quoted 4 lines · Roja Rani Yarubandi
drivers/i2c/busses/i2c-qcom-geni.c +0
-0@@ -629,6 +658,16 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)struct geni_i2c_dev *gi2c =dev_get_drvdata (dev);disable_irq (gi2c->irq);drivers/i2c/busses/i2c-qcom-geni.c static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev) · line 670
Quoted 10 lines
drivers/i2c/busses/i2c-qcom-geni.c +10
-0+ +/* Drop the assigned performance state */ +if (gi2c->assigned_pstate) { + ret =dev_pm_genpd_set_performance_state (dev,0 ); +if (ret) { +dev_err (dev,"Failed to set performance state\n" ); +return ret; + } + } +Ulf, Viresh, I think we discussed this at the time of introducing the performance states.
The client's state does not affect if its
performance_stateshould be included in the calculation of the aggregatedperformance_state, so each driver that needs to keep some minimum performance state needs to have these two snippets.Quoted 7 lines
Quoted 3 lines
Would it not make sense to on enable/disable re-evaluate the
performance_stateand potentially reconfigure the hardware automatically?I agree, this will be repeated across multiple drivers which would need some minimal vote while they are active, handling this during genpd enable/disable in genpd core makes sense.
Initially that's what we tried out, but we realized that it was difficult to deal with this internally in genpd, but more importantly it also removed some flexibility from consumers and providers. See commit
68de2fe57a8f("PM / Domains: Make genpd performance states orthogonal to the idlestates").
Quoted 3 lines
As a matter of fact this was quite recently discussed [1], which also pointed out some issues when using the "required-opps" in combination, but perhaps that got resolved? Viresh?
So I looked again at that thread in detail today. The basic idea was to enable/disable the genpd from within the OPP core and there were doubts on how to do that efficiently as there are cases where domains may be enabled for an OPP, but not for others.. etc. etc.
I am not sure if I consider that thread as part of the discussion we are having here, they may be related, but that thread doesn't block anything to be done in the genpd core.
Quoted 5 lines
My concern is, if we would make this kind of change to the internals of genpd, it would lead to the following limitation: A consumer driver can no longer make its vote for its device to stick around, when the device becomes runtime suspended - and how do we know that we never need to support such a case?
What about doing this just for the assigned-performance-state case as the clients don't want to play with it at all.
Signature · 1 line
viresh
Re: [PATCH 3/3] i2c: i2c-qcom-geni: Add support for 'assigned-performance-states'
[]...
Quoted 27 lines
Quoted 23 lines
Quoted 9 lines
Quoted 9 lines
Ulf, Viresh, I think we discussed this at the time of introducing the performance states.
The client's state does not affect if its
performance_stateshould be included in the calculation of the aggregatedperformance_state, so each driver that needs to keep some minimum performance state needs to have these two snippets.Quoted 7 lines
Quoted 3 lines
Would it not make sense to on enable/disable re-evaluate the
performance_stateand potentially reconfigure the hardware automatically?I agree, this will be repeated across multiple drivers which would need some minimal vote while they are active, handling this during genpd enable/disable in genpd core makes sense.
Initially that's what we tried out, but we realized that it was difficult to deal with this internally in genpd, but more importantly it also removed some flexibility from consumers and providers. See commit
68de2fe57a8f("PM / Domains: Make genpd performance states orthogonal to the idlestates").Quoted 3 lines
As a matter of fact this was quite recently discussed [1], which also pointed out some issues when using the "required-opps" in combination, but perhaps that got resolved? Viresh?
Quoted 8 lines
So I looked again at that thread in detail today. The basic idea was to enable/disable the genpd from within the OPP core and there were doubts on how to do that efficiently as there are cases where domains may be enabled for an OPP, but not for others.. etc. etc.
I am not sure if I consider that thread as part of the discussion we are having here, they may be related, but that thread doesn't block anything to be done in the genpd core.
That's true, the 2 threads are different in the sense that one talks about having OPP core managing power on/off along with setting perf state, while the other talks about genpd core managing a default perf state along with power on/off, but they are similar in the sense that both are related to the discussion whether we should treat powering on and off a domain related to setting its performance state or if it should be considered completely orthogonal.
I think the clock framework treats setting clock rates and turning on/off a clock orthogonal because there is an inherent assumption that once the clock is turned off, what rate it was set to should not matter, and it can be running at the same rate when we turn the clock back on.
I guess we can have the same assumption here that a perf state of a power domain should not matter if the power domain is turned off and hence the perf state need not be dropped explicitly during power off, atleast that should be true for the qcom power domains supporting perf state upstream.
Should that be the approach taken here? I guess that would mean the patch
I had proposed earlier [1] to manage this in the genpd core would have to set the default perf state at attach and remove it only during a detach of the device to the pm_domain, and not manage it during the runtime_suspend/resume of the device.
Quoted 4 lines
Quoted 4 lines
A consumer driver
can no longer make its vote for its device to stick around, when the device becomes runtime suspended - and how do we know that we never need to support such a case?
The above approach should take care of this but the down side of it would be, unlike in the case of clocks where the devices assigning a default clock rate might be doing so on a device specific clock (rarely shared with other devices) in case of power domain, and especially in the qcom implementation of these power domains which support perf state, these can be large domains with lots of devices, and any device being active (not necessarily wanting any default perf state) will keep the domain at the default perf state, requested by a device which isn't really active.
Quoted 2 lines
What about doing this just for the assigned-performance-state case as the clients don't want to play with it at all.
well, thats possible too, but you obviously can't reuse the same bindings in such cases
[1] https://lore.kernel.org/patchwork/patch/1284042/
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 3/3] i2c: i2c-qcom-geni: Add support for 'assigned-performance-states'
Quoted 58 lines · Rajendra Nayak
[]...
Quoted 27 lines
Quoted 23 lines
Quoted 9 lines
Quoted 9 lines
Ulf, Viresh, I think we discussed this at the time of introducing the performance states.
The client's state does not affect if its
performance_stateshould be included in the calculation of the aggregatedperformance_state, so each driver that needs to keep some minimum performance state needs to have these two snippets.Quoted 7 lines
Quoted 3 lines
Would it not make sense to on enable/disable re-evaluate the
performance_stateand potentially reconfigure the hardware automatically?I agree, this will be repeated across multiple drivers which would need some minimal vote while they are active, handling this during genpd enable/disable in genpd core makes sense.
Initially that's what we tried out, but we realized that it was difficult to deal with this internally in genpd, but more importantly it also removed some flexibility from consumers and providers. See commit
68de2fe57a8f("PM / Domains: Make genpd performance states orthogonal to the idlestates").Quoted 3 lines
As a matter of fact this was quite recently discussed [1], which also pointed out some issues when using the "required-opps" in combination, but perhaps that got resolved? Viresh?
Quoted 8 lines
So I looked again at that thread in detail today. The basic idea was to enable/disable the genpd from within the OPP core and there were doubts on how to do that efficiently as there are cases where domains may be enabled for an OPP, but not for others.. etc. etc.
I am not sure if I consider that thread as part of the discussion we are having here, they may be related, but that thread doesn't block anything to be done in the genpd core.
That's true, the 2 threads are different in the sense that one talks about having OPP core managing power on/off along with setting perf state, while the other talks about genpd core managing a default perf state along with power on/off, but they are similar in the sense that both are related to the discussion whether we should treat powering on and off a domain related to setting its performance state or if it should be considered completely orthogonal.
I think the clock framework treats setting clock rates and turning on/off a clock orthogonal because there is an inherent assumption that once the clock is turned off, what rate it was set to should not matter, and it can be running at the same rate when we turn the clock back on.
I guess we can have the same assumption here that a perf state of a power domain should not matter if the power domain is turned off and hence the perf state need not be dropped explicitly during power off, atleast that should be true for the qcom power domains supporting perf state upstream.
Quoted 4 lines
Should that be the approach taken here? I guess that would mean the patch
I had proposed earlier [1] to manage this in the genpd core would have to set the default perf state at attach and remove it only during a detach of the device to thepm_domain, and not manage it during the runtime_suspend/resume of the device.
Right, I think this would be a step in the right direction, but it's not sufficient to solve the complete problem. As you also point out below.
Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
A consumer driver
can no longer make its vote for its device to stick around, when the device becomes runtime suspended - and how do we know that we never need to support such a case?
Quoted 7 lines
The above approach should take care of this but the down side of it would be, unlike in the case of clocks where the devices assigning a default clock rate might be doing so on a device specific clock (rarely shared with other devices) in case of power domain, and especially in the qcom implementation of these power domains which support perf state, these can be large domains with lots of devices, and any device being active (not necessarily wanting any default perf state) will keep the domain at the default perf state, requested by a device which isn't really active.
Yep, this certainly sounds suboptimal. To me, this isn't good enough.
Quoted 6 lines
Quoted 3 lines
What about doing this just for the assigned-performance-state case as the clients don't want to play with it at all.
well, thats possible too, but you obviously can't reuse the same bindings in such cases
Not sure I understand the issue with the DT binding? Let me elaborate on how I think we could move forward.
It looks like we have two problems to solve:
*) We need a new DT binding.
If that becomes a generic property along the lines of the "assigned-performance-state" as suggested - or if we decide to add a SoC specific binding via using an additional cell in "power-domains" (suggested by Rob), doesn't really matter much to me. The arguments for the new DT property are very much similar to why we added "assigned-clock-rates" for clocks.
**) We want to avoid boiler-plate code in drivers to manage "assigned-performance-state" for their devices.
No matter what DT property we decide on (generic or SoC specific), we should be able to manage this from the PM domain (genpd) layer. No changes in the drivers should be needed.
If a generic binding is used, we could consider to let genpd internally manage the whole thing (DT parsing and updating performance state votes for assigned-performance-state only).
If we go for an SoC specific binding, the genpd provider needs to be updated. It can manage DT parsing from the ->attach|detach_dev() callbacks and update performance votes from the ->start|stop() callbacks.
We could also consider a hybrid of these two solutions.
Quoted 2 lines
Kind regards
Uffe
Re: [PATCH 3/3] i2c: i2c-qcom-geni: Add support for 'assigned-performance-states'
Quoted 104 lines · Ulf Hansson
Quoted 58 lines · Rajendra Nayak
[]...
Quoted 27 lines
Quoted 23 lines
Quoted 9 lines
Quoted 9 lines
Ulf, Viresh, I think we discussed this at the time of introducing the performance states.
The client's state does not affect if its
performance_stateshould be included in the calculation of the aggregatedperformance_state, so each driver that needs to keep some minimum performance state needs to have these two snippets.Quoted 7 lines
Quoted 3 lines
Would it not make sense to on enable/disable re-evaluate the
performance_stateand potentially reconfigure the hardware automatically?I agree, this will be repeated across multiple drivers which would need some minimal vote while they are active, handling this during genpd enable/disable in genpd core makes sense.
Initially that's what we tried out, but we realized that it was difficult to deal with this internally in genpd, but more importantly it also removed some flexibility from consumers and providers. See commit
68de2fe57a8f("PM / Domains: Make genpd performance states orthogonal to the idlestates").Quoted 3 lines
As a matter of fact this was quite recently discussed [1], which also pointed out some issues when using the "required-opps" in combination, but perhaps that got resolved? Viresh?
Quoted 8 lines
So I looked again at that thread in detail today. The basic idea was to enable/disable the genpd from within the OPP core and there were doubts on how to do that efficiently as there are cases where domains may be enabled for an OPP, but not for others.. etc. etc.
I am not sure if I consider that thread as part of the discussion we are having here, they may be related, but that thread doesn't block anything to be done in the genpd core.
That's true, the 2 threads are different in the sense that one talks about having OPP core managing power on/off along with setting perf state, while the other talks about genpd core managing a default perf state along with power on/off, but they are similar in the sense that both are related to the discussion whether we should treat powering on and off a domain related to setting its performance state or if it should be considered completely orthogonal.
I think the clock framework treats setting clock rates and turning on/off a clock orthogonal because there is an inherent assumption that once the clock is turned off, what rate it was set to should not matter, and it can be running at the same rate when we turn the clock back on.
I guess we can have the same assumption here that a perf state of a power domain should not matter if the power domain is turned off and hence the perf state need not be dropped explicitly during power off, atleast that should be true for the qcom power domains supporting perf state upstream.
Quoted 4 lines
Should that be the approach taken here? I guess that would mean the patch
I had proposed earlier [1] to manage this in the genpd core would have to set the default perf state at attach and remove it only during a detach of the device to thepm_domain, and not manage it during the runtime_suspend/resume of the device.Right, I think this would be a step in the right direction, but it's not sufficient to solve the complete problem. As you also point out below.
Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
A consumer driver
can no longer make its vote for its device to stick around, when the device becomes runtime suspended - and how do we know that we never need to support such a case?Quoted 7 lines
The above approach should take care of this but the down side of it would be, unlike in the case of clocks where the devices assigning a default clock rate might be doing so on a device specific clock (rarely shared with other devices) in case of power domain, and especially in the qcom implementation of these power domains which support perf state, these can be large domains with lots of devices, and any device being active (not necessarily wanting any default perf state) will keep the domain at the default perf state, requested by a device which isn't really active.
Yep, this certainly sounds suboptimal. To me, this isn't good enough.
Quoted 6 lines
Quoted 3 lines
What about doing this just for the assigned-performance-state case as the clients don't want to play with it at all.
well, thats possible too, but you obviously can't reuse the same bindings in such cases
Not sure I understand the issue with the DT binding? Let me elaborate on how I think we could move forward.
It looks like we have two problems to solve:
*) We need a new DT binding.
If that becomes a generic property along the lines of the "assigned-performance-state" as suggested - or if we decide to add a SoC specific binding via using an additional cell in "power-domains" (suggested by Rob), doesn't really matter much to me. The arguments for the new DT property are very much similar to why we added "assigned-clock-rates" for clocks.
Quoted 8 lines
**) We want to avoid boiler-plate code in drivers to manage "assigned-performance-state" for their devices.
No matter what DT property we decide on (generic or SoC specific), we should be able to manage this from the PM domain (genpd) layer. No changes in the drivers should be needed.
If a generic binding is used, we could consider to let genpd internally manage the whole thing (DT parsing and updating performance state votes for assigned-performance-state only).
Sure, so for starters does that mean I should re-spin my series which adds the generic 'assigned-performance-states' bindings and see if Rob is OK with that? I am guessing you are OK with the way that binding gets used within genpd core in that series, or would you want it to be handled differently?
Quoted 11 lines
If we go for an SoC specific binding, the genpd provider needs to be updated. It can manage DT parsing from the ->attach|detach_dev() callbacks and update performance votes from the ->start|stop() callbacks.
We could also consider a hybrid of these two solutions.Quoted 2 lines
Kind regards
Uffe
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 3/3] i2c: i2c-qcom-geni: Add support for 'assigned-performance-states'
Quoted 117 lines · Rajendra Nayak
Quoted 104 lines · Ulf Hansson
Quoted 58 lines · Rajendra Nayak
[]...
Quoted 27 lines
Quoted 23 lines
Quoted 9 lines
Quoted 9 lines
Ulf, Viresh, I think we discussed this at the time of introducing the performance states.
The client's state does not affect if its
performance_stateshould be included in the calculation of the aggregatedperformance_state, so each driver that needs to keep some minimum performance state needs to have these two snippets.Quoted 7 lines
Quoted 3 lines
Would it not make sense to on enable/disable re-evaluate the
performance_stateand potentially reconfigure the hardware automatically?I agree, this will be repeated across multiple drivers which would need some minimal vote while they are active, handling this during genpd enable/disable in genpd core makes sense.
Initially that's what we tried out, but we realized that it was difficult to deal with this internally in genpd, but more importantly it also removed some flexibility from consumers and providers. See commit
68de2fe57a8f("PM / Domains: Make genpd performance states orthogonal to the idlestates").Quoted 3 lines
As a matter of fact this was quite recently discussed [1], which also pointed out some issues when using the "required-opps" in combination, but perhaps that got resolved? Viresh?
Quoted 8 lines
So I looked again at that thread in detail today. The basic idea was to enable/disable the genpd from within the OPP core and there were doubts on how to do that efficiently as there are cases where domains may be enabled for an OPP, but not for others.. etc. etc.
I am not sure if I consider that thread as part of the discussion we are having here, they may be related, but that thread doesn't block anything to be done in the genpd core.
That's true, the 2 threads are different in the sense that one talks about having OPP core managing power on/off along with setting perf state, while the other talks about genpd core managing a default perf state along with power on/off, but they are similar in the sense that both are related to the discussion whether we should treat powering on and off a domain related to setting its performance state or if it should be considered completely orthogonal.
I think the clock framework treats setting clock rates and turning on/off a clock orthogonal because there is an inherent assumption that once the clock is turned off, what rate it was set to should not matter, and it can be running at the same rate when we turn the clock back on.
I guess we can have the same assumption here that a perf state of a power domain should not matter if the power domain is turned off and hence the perf state need not be dropped explicitly during power off, atleast that should be true for the qcom power domains supporting perf state upstream.
Quoted 4 lines
Should that be the approach taken here? I guess that would mean the patch
I had proposed earlier [1] to manage this in the genpd core would have to set the default perf state at attach and remove it only during a detach of the device to thepm_domain, and not manage it during the runtime_suspend/resume of the device.Right, I think this would be a step in the right direction, but it's not sufficient to solve the complete problem. As you also point out below.
Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
A consumer driver
can no longer make its vote for its device to stick around, when the device becomes runtime suspended - and how do we know that we never need to support such a case?Quoted 7 lines
The above approach should take care of this but the down side of it would be, unlike in the case of clocks where the devices assigning a default clock rate might be doing so on a device specific clock (rarely shared with other devices) in case of power domain, and especially in the qcom implementation of these power domains which support perf state, these can be large domains with lots of devices, and any device being active (not necessarily wanting any default perf state) will keep the domain at the default perf state, requested by a device which isn't really active.
Yep, this certainly sounds suboptimal. To me, this isn't good enough.
Quoted 6 lines
Quoted 3 lines
What about doing this just for the assigned-performance-state case as the clients don't want to play with it at all.
well, thats possible too, but you obviously can't reuse the same bindings in such cases
Not sure I understand the issue with the DT binding? Let me elaborate on how I think we could move forward.
It looks like we have two problems to solve:
*) We need a new DT binding.
If that becomes a generic property along the lines of the "assigned-performance-state" as suggested - or if we decide to add a SoC specific binding via using an additional cell in "power-domains" (suggested by Rob), doesn't really matter much to me. The arguments for the new DT property are very much similar to why we added "assigned-clock-rates" for clocks.Quoted 8 lines
**) We want to avoid boiler-plate code in drivers to manage "assigned-performance-state" for their devices.
No matter what DT property we decide on (generic or SoC specific), we should be able to manage this from the PM domain (genpd) layer. No changes in the drivers should be needed.
If a generic binding is used, we could consider to let genpd internally manage the whole thing (DT parsing and updating performance state votes for assigned-performance-state only).
Quoted 5 lines
Sure, so for starters does that mean I should re-spin my series which adds the generic 'assigned-performance-states' bindings and see if Rob is OK with that? I am guessing you are OK with the way that binding gets used within genpd core in that series, or would you want it to be handled differently?
A re-spin would definitely move this forward. If we can convince Rob about the generic DT binding, I will certainly agree to change genpd to help/manage the DT parsing.
Although, I am not sure yet what is the best. Let genpd to do the parsing internally in genpd_add_device() or just provide a helper function that can be called from an ->attach|detach_dev() callback.
In the end it boils done to decide whether we should use the ->start|stop() callbacks or let genpd internally manage the "assigned performance state". Honestly, I would prefer to look at how the code would need to be changed, before answering this.
Quoted 8 lines
Quoted 8 lines
If we go for an SoC specific binding, the genpd provider needs to be updated. It can manage DT parsing from the ->attach|detach_dev() callbacks and update performance votes from the ->start|stop() callbacks.
We could also consider a hybrid of these two solutions.Quoted 2 lines
Kind regards
Uffe