All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5 0/2] PM / Domains: Add support for 'required-opps' to set default perf state
@ 2021-07-20  7:07 Rajendra Nayak
  2021-07-20  7:07 ` [PATCH v5 1/2] " Rajendra Nayak
  2021-07-20  7:07 ` [PATCH v5 2/2] arm64: dts: sc7180: Add required-opps for i2c Rajendra Nayak
  0 siblings, 2 replies; 9+ messages in thread
From: Rajendra Nayak @ 2021-07-20  7:07 UTC (permalink / raw)
  To: ulf.hansson, bjorn.andersson, viresh.kumar
  Cc: linux-pm, devicetree, linux-kernel, linux-arm-msm, swboyd, rojay,
	stephan, Rajendra Nayak

v5: Dropped all default_pstate handling in runtime suspend/resume

v4: Fixed error handling in __genpd_dev_pm_attach()

This is a re-spin of the series [1] which was adding support for a new
DT binding (assigned-performance-state) and based on the discussions on
that thread [2] it was concluded that we could achieve the same with the
existing 'required-opps' binding instead.

So this series, just drops the new binding and uses required-opps to achieve
the default perf state setting thats needed by some devices.

---
Some devics within power-domains with performance states do not
support DVFS, but still need to vote on a default/static state
while they are active. Add support for this using the 'required-opps'
property in device tree.

[1] https://lore.kernel.org/patchwork/project/lkml/list/?series=501336&state=%2A&archive=both
[2] https://lore.kernel.org/patchwork/patch/1436886/

Rajendra Nayak (2):
  PM / Domains: Add support for 'required-opps' to set default perf
    state
  arm64: dts: sc7180: Add required-opps for i2c

 arch/arm64/boot/dts/qcom/sc7180.dtsi | 24 ++++++++++++++++++++++++
 drivers/base/power/domain.c          | 28 +++++++++++++++++++++++++---
 include/linux/pm_domain.h            |  1 +
 3 files changed, 50 insertions(+), 3 deletions(-)

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v5 1/2] PM / Domains: Add support for 'required-opps' to set default perf state
  2021-07-20  7:07 [PATCH v5 0/2] PM / Domains: Add support for 'required-opps' to set default perf state Rajendra Nayak
@ 2021-07-20  7:07 ` Rajendra Nayak
  2021-07-20  7:18   ` Felipe Balbi
  2021-08-02 12:59   ` Ulf Hansson
  2021-07-20  7:07 ` [PATCH v5 2/2] arm64: dts: sc7180: Add required-opps for i2c Rajendra Nayak
  1 sibling, 2 replies; 9+ messages in thread
From: Rajendra Nayak @ 2021-07-20  7:07 UTC (permalink / raw)
  To: ulf.hansson, bjorn.andersson, viresh.kumar
  Cc: linux-pm, devicetree, linux-kernel, linux-arm-msm, swboyd, rojay,
	stephan, Rajendra Nayak

Some devices within power domains with performance states do not
support DVFS, but still need to vote on a default/static state
while they are active. They can express this using the 'required-opps'
property in device tree, which points to the phandle of the OPP
supported by the corresponding power-domains.

Add support to parse this information from DT and then set the
specified performance state during attach and drop it on detach.
runtime suspend/resume callbacks already have logic to drop/set
the vote as needed and should take care of dropping the default
perf state vote on runtime suspend and restore it back on runtime
resume.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
---
 drivers/base/power/domain.c | 28 +++++++++++++++++++++++++---
 include/linux/pm_domain.h   |  1 +
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index a934c67..f454031 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2598,6 +2598,12 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
 
 	dev_dbg(dev, "removing from PM domain %s\n", pd->name);
 
+	/* Drop the default performance state */
+	if (dev_gpd_data(dev)->default_pstate) {
+		dev_pm_genpd_set_performance_state(dev, 0);
+		dev_gpd_data(dev)->default_pstate = 0;
+	}
+
 	for (i = 1; i < GENPD_RETRY_MAX_MS; i <<= 1) {
 		ret = genpd_remove_device(pd, dev);
 		if (ret != -EAGAIN)
@@ -2635,9 +2641,10 @@ static void genpd_dev_pm_sync(struct device *dev)
 static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
 				 unsigned int index, bool power_on)
 {
+	struct device_node *np;
 	struct of_phandle_args pd_args;
 	struct generic_pm_domain *pd;
-	int ret;
+	int ret, pstate;
 
 	ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
 				"#power-domain-cells", index, &pd_args);
@@ -2675,10 +2682,25 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
 		genpd_unlock(pd);
 	}
 
-	if (ret)
+	if (ret) {
 		genpd_remove_device(pd, dev);
+		return -EPROBE_DEFER;
+	}
+
+	/* Set the default performance state */
+	np = base_dev->of_node;
+	if (of_parse_phandle(np, "required-opps", index)) {
+		pstate = of_get_required_opp_performance_state(np, index);
+		if (pstate < 0) {
+			ret = pstate;
+			dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
+				pd->name, ret);
+		}
+		dev_pm_genpd_set_performance_state(dev, pstate);
+		dev_gpd_data(dev)->default_pstate = pstate;
+	}
 
-	return ret ? -EPROBE_DEFER : 1;
+	return ret ? ret : 1;
 }
 
 /**
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 21a0577..67017c9 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -198,6 +198,7 @@ struct generic_pm_domain_data {
 	struct notifier_block *power_nb;
 	int cpu;
 	unsigned int performance_state;
+	unsigned int default_pstate;
 	unsigned int rpm_pstate;
 	ktime_t	next_wakeup;
 	void *data;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v5 2/2] arm64: dts: sc7180: Add required-opps for i2c
  2021-07-20  7:07 [PATCH v5 0/2] PM / Domains: Add support for 'required-opps' to set default perf state Rajendra Nayak
  2021-07-20  7:07 ` [PATCH v5 1/2] " Rajendra Nayak
@ 2021-07-20  7:07 ` Rajendra Nayak
  1 sibling, 0 replies; 9+ messages in thread
From: Rajendra Nayak @ 2021-07-20  7:07 UTC (permalink / raw)
  To: ulf.hansson, bjorn.andersson, viresh.kumar
  Cc: linux-pm, devicetree, linux-kernel, linux-arm-msm, swboyd, rojay,
	stephan, Rajendra Nayak

qup-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 'required-opps' to pass this information from
device tree, and also add the power-domains property to specify
the CX power-domain.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
 arch/arm64/boot/dts/qcom/sc7180.dtsi | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index a5d58eb..cd30185 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -785,8 +785,10 @@
 						<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_0 0>,
 						<&aggre1_noc MASTER_QUP_0 0 &mc_virt SLAVE_EBI1 0>;
 				interconnect-names = "qup-core", "qup-config",
 							"qup-memory";
+				power-domains = <&rpmhpd SC7180_CX>;
+				required-opps = <&rpmhpd_opp_low_svs>;
 				status = "disabled";
 			};
 
 			spi0: spi@880000 {
@@ -837,8 +839,10 @@
 						<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_0 0>,
 						<&aggre1_noc MASTER_QUP_0 0 &mc_virt SLAVE_EBI1 0>;
 				interconnect-names = "qup-core", "qup-config",
 							"qup-memory";
+				power-domains = <&rpmhpd SC7180_CX>;
+				required-opps = <&rpmhpd_opp_low_svs>;
 				status = "disabled";
 			};
 
 			spi1: spi@884000 {
@@ -889,8 +893,10 @@
 						<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_0 0>,
 						<&aggre1_noc MASTER_QUP_0 0 &mc_virt SLAVE_EBI1 0>;
 				interconnect-names = "qup-core", "qup-config",
 							"qup-memory";
+				power-domains = <&rpmhpd SC7180_CX>;
+				required-opps = <&rpmhpd_opp_low_svs>;
 				status = "disabled";
 			};
 
 			uart2: serial@888000 {
@@ -923,8 +929,10 @@
 						<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_0 0>,
 						<&aggre1_noc MASTER_QUP_0 0 &mc_virt SLAVE_EBI1 0>;
 				interconnect-names = "qup-core", "qup-config",
 							"qup-memory";
+				power-domains = <&rpmhpd SC7180_CX>;
+				required-opps = <&rpmhpd_opp_low_svs>;
 				status = "disabled";
 			};
 
 			spi3: spi@88c000 {
@@ -975,8 +983,10 @@
 						<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_0 0>,
 						<&aggre1_noc MASTER_QUP_0 0 &mc_virt SLAVE_EBI1 0>;
 				interconnect-names = "qup-core", "qup-config",
 							"qup-memory";
+				power-domains = <&rpmhpd SC7180_CX>;
+				required-opps = <&rpmhpd_opp_low_svs>;
 				status = "disabled";
 			};
 
 			uart4: serial@890000 {
@@ -1009,8 +1019,10 @@
 						<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_0 0>,
 						<&aggre1_noc MASTER_QUP_0 0 &mc_virt SLAVE_EBI1 0>;
 				interconnect-names = "qup-core", "qup-config",
 							"qup-memory";
+				power-domains = <&rpmhpd SC7180_CX>;
+				required-opps = <&rpmhpd_opp_low_svs>;
 				status = "disabled";
 			};
 
 			spi5: spi@894000 {
@@ -1074,8 +1086,10 @@
 						<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_1 0>,
 						<&aggre2_noc MASTER_QUP_1 0 &mc_virt SLAVE_EBI1 0>;
 				interconnect-names = "qup-core", "qup-config",
 							"qup-memory";
+				power-domains = <&rpmhpd SC7180_CX>;
+				required-opps = <&rpmhpd_opp_low_svs>;
 				status = "disabled";
 			};
 
 			spi6: spi@a80000 {
@@ -1126,8 +1140,10 @@
 						<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_1 0>,
 						<&aggre2_noc MASTER_QUP_1 0 &mc_virt SLAVE_EBI1 0>;
 				interconnect-names = "qup-core", "qup-config",
 							"qup-memory";
+				power-domains = <&rpmhpd SC7180_CX>;
+				required-opps = <&rpmhpd_opp_low_svs>;
 				status = "disabled";
 			};
 
 			uart7: serial@a84000 {
@@ -1160,8 +1176,10 @@
 						<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_1 0>,
 						<&aggre2_noc MASTER_QUP_1 0 &mc_virt SLAVE_EBI1 0>;
 				interconnect-names = "qup-core", "qup-config",
 							"qup-memory";
+				power-domains = <&rpmhpd SC7180_CX>;
+				required-opps = <&rpmhpd_opp_low_svs>;
 				status = "disabled";
 			};
 
 			spi8: spi@a88000 {
@@ -1212,8 +1230,10 @@
 						<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_1 0>,
 						<&aggre2_noc MASTER_QUP_1 0 &mc_virt SLAVE_EBI1 0>;
 				interconnect-names = "qup-core", "qup-config",
 							"qup-memory";
+				power-domains = <&rpmhpd SC7180_CX>;
+				required-opps = <&rpmhpd_opp_low_svs>;
 				status = "disabled";
 			};
 
 			uart9: serial@a8c000 {
@@ -1246,8 +1266,10 @@
 						<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_1 0>,
 						<&aggre2_noc MASTER_QUP_1 0 &mc_virt SLAVE_EBI1 0>;
 				interconnect-names = "qup-core", "qup-config",
 							"qup-memory";
+				power-domains = <&rpmhpd SC7180_CX>;
+				required-opps = <&rpmhpd_opp_low_svs>;
 				status = "disabled";
 			};
 
 			spi10: spi@a90000 {
@@ -1298,8 +1320,10 @@
 						<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_1 0>,
 						<&aggre2_noc MASTER_QUP_1 0 &mc_virt SLAVE_EBI1 0>;
 				interconnect-names = "qup-core", "qup-config",
 							"qup-memory";
+				power-domains = <&rpmhpd SC7180_CX>;
+				required-opps = <&rpmhpd_opp_low_svs>;
 				status = "disabled";
 			};
 
 			spi11: spi@a94000 {
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 1/2] PM / Domains: Add support for 'required-opps' to set default perf state
  2021-07-20  7:07 ` [PATCH v5 1/2] " Rajendra Nayak
@ 2021-07-20  7:18   ` Felipe Balbi
  2021-08-02 12:59   ` Ulf Hansson
  1 sibling, 0 replies; 9+ messages in thread
From: Felipe Balbi @ 2021-07-20  7:18 UTC (permalink / raw)
  To: Rajendra Nayak, ulf.hansson, bjorn.andersson, viresh.kumar
  Cc: linux-pm, devicetree, linux-kernel, linux-arm-msm, swboyd, rojay,
	stephan, Rajendra Nayak

[-- Attachment #1: Type: text/plain, Size: 483 bytes --]


Hi,

Rajendra Nayak <rnayak@codeaurora.org> writes:
> @@ -2635,9 +2641,10 @@ static void genpd_dev_pm_sync(struct device *dev)
>  static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>  				 unsigned int index, bool power_on)
>  {
> +	struct device_node *np;
>  	struct of_phandle_args pd_args;
>  	struct generic_pm_domain *pd;
> -	int ret;
> +	int ret, pstate;

nit: might want to keep one variable declaration per line here.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 1/2] PM / Domains: Add support for 'required-opps' to set default perf state
  2021-07-20  7:07 ` [PATCH v5 1/2] " Rajendra Nayak
  2021-07-20  7:18   ` Felipe Balbi
@ 2021-08-02 12:59   ` Ulf Hansson
  2021-08-03  4:38     ` Rajendra Nayak
  1 sibling, 1 reply; 9+ messages in thread
From: Ulf Hansson @ 2021-08-02 12:59 UTC (permalink / raw)
  To: Rajendra Nayak
  Cc: Bjorn Andersson, Viresh Kumar, Linux PM, DTML,
	Linux Kernel Mailing List, linux-arm-msm, Stephen Boyd,
	Roja Rani Yarubandi, Stephan Gerhold

On Tue, 20 Jul 2021 at 09:12, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>
> Some devices within power domains with performance states do not
> support DVFS, but still need to vote on a default/static state
> while they are active. They can express this using the 'required-opps'
> property in device tree, which points to the phandle of the OPP
> supported by the corresponding power-domains.
>
> Add support to parse this information from DT and then set the
> specified performance state during attach and drop it on detach.
> runtime suspend/resume callbacks already have logic to drop/set
> the vote as needed and should take care of dropping the default
> perf state vote on runtime suspend and restore it back on runtime
> resume.
>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
>  drivers/base/power/domain.c | 28 +++++++++++++++++++++++++---
>  include/linux/pm_domain.h   |  1 +
>  2 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index a934c67..f454031 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2598,6 +2598,12 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
>
>         dev_dbg(dev, "removing from PM domain %s\n", pd->name);
>
> +       /* Drop the default performance state */
> +       if (dev_gpd_data(dev)->default_pstate) {
> +               dev_pm_genpd_set_performance_state(dev, 0);
> +               dev_gpd_data(dev)->default_pstate = 0;
> +       }
> +
>         for (i = 1; i < GENPD_RETRY_MAX_MS; i <<= 1) {
>                 ret = genpd_remove_device(pd, dev);
>                 if (ret != -EAGAIN)
> @@ -2635,9 +2641,10 @@ static void genpd_dev_pm_sync(struct device *dev)
>  static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>                                  unsigned int index, bool power_on)
>  {
> +       struct device_node *np;
>         struct of_phandle_args pd_args;
>         struct generic_pm_domain *pd;
> -       int ret;
> +       int ret, pstate;
>
>         ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
>                                 "#power-domain-cells", index, &pd_args);
> @@ -2675,10 +2682,25 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>                 genpd_unlock(pd);
>         }
>
> -       if (ret)
> +       if (ret) {
>                 genpd_remove_device(pd, dev);
> +               return -EPROBE_DEFER;
> +       }
> +
> +       /* Set the default performance state */
> +       np = base_dev->of_node;

Please use dev->of_node instead (it is set to the same of_node as
base_dev by the callers of __genpd_dev_pm_attach) as it's more
consistent with existing code.

> +       if (of_parse_phandle(np, "required-opps", index)) {
> +               pstate = of_get_required_opp_performance_state(np, index);
> +               if (pstate < 0) {
> +                       ret = pstate;
> +                       dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
> +                               pd->name, ret);
> +               }
> +               dev_pm_genpd_set_performance_state(dev, pstate);
> +               dev_gpd_data(dev)->default_pstate = pstate;

This doesn't look entirely correct to me. If we fail to translate a
required opp to a performance state, we shouldn't try to set it.

Perhaps it's also easier to call
of_get_required_opp_performance_state() unconditionally of whether a
"required-opps" specifier exists. If it fails with the translation,
then we just skip setting a default state and continue with returning
1.

Would that work?

> +       }
>
> -       return ret ? -EPROBE_DEFER : 1;
> +       return ret ? ret : 1;
>  }
>
>  /**
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 21a0577..67017c9 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -198,6 +198,7 @@ struct generic_pm_domain_data {
>         struct notifier_block *power_nb;
>         int cpu;
>         unsigned int performance_state;
> +       unsigned int default_pstate;
>         unsigned int rpm_pstate;
>         ktime_t next_wakeup;
>         void *data;
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 1/2] PM / Domains: Add support for 'required-opps' to set default perf state
  2021-08-02 12:59   ` Ulf Hansson
@ 2021-08-03  4:38     ` Rajendra Nayak
  2021-08-04 11:08       ` Rajendra Nayak
  0 siblings, 1 reply; 9+ messages in thread
From: Rajendra Nayak @ 2021-08-03  4:38 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Bjorn Andersson, Viresh Kumar, Linux PM, DTML,
	Linux Kernel Mailing List, linux-arm-msm, Stephen Boyd,
	Roja Rani Yarubandi, Stephan Gerhold


On 8/2/2021 6:29 PM, Ulf Hansson wrote:
> On Tue, 20 Jul 2021 at 09:12, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>
>> Some devices within power domains with performance states do not
>> support DVFS, but still need to vote on a default/static state
>> while they are active. They can express this using the 'required-opps'
>> property in device tree, which points to the phandle of the OPP
>> supported by the corresponding power-domains.
>>
>> Add support to parse this information from DT and then set the
>> specified performance state during attach and drop it on detach.
>> runtime suspend/resume callbacks already have logic to drop/set
>> the vote as needed and should take care of dropping the default
>> perf state vote on runtime suspend and restore it back on runtime
>> resume.
>>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>>   drivers/base/power/domain.c | 28 +++++++++++++++++++++++++---
>>   include/linux/pm_domain.h   |  1 +
>>   2 files changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index a934c67..f454031 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -2598,6 +2598,12 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
>>
>>          dev_dbg(dev, "removing from PM domain %s\n", pd->name);
>>
>> +       /* Drop the default performance state */
>> +       if (dev_gpd_data(dev)->default_pstate) {
>> +               dev_pm_genpd_set_performance_state(dev, 0);
>> +               dev_gpd_data(dev)->default_pstate = 0;
>> +       }
>> +
>>          for (i = 1; i < GENPD_RETRY_MAX_MS; i <<= 1) {
>>                  ret = genpd_remove_device(pd, dev);
>>                  if (ret != -EAGAIN)
>> @@ -2635,9 +2641,10 @@ static void genpd_dev_pm_sync(struct device *dev)
>>   static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>>                                   unsigned int index, bool power_on)
>>   {
>> +       struct device_node *np;
>>          struct of_phandle_args pd_args;
>>          struct generic_pm_domain *pd;
>> -       int ret;
>> +       int ret, pstate;
>>
>>          ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
>>                                  "#power-domain-cells", index, &pd_args);
>> @@ -2675,10 +2682,25 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>>                  genpd_unlock(pd);
>>          }
>>
>> -       if (ret)
>> +       if (ret) {
>>                  genpd_remove_device(pd, dev);
>> +               return -EPROBE_DEFER;
>> +       }
>> +
>> +       /* Set the default performance state */
>> +       np = base_dev->of_node;
> 
> Please use dev->of_node instead (it is set to the same of_node as
> base_dev by the callers of __genpd_dev_pm_attach) as it's more
> consistent with existing code.
> 
>> +       if (of_parse_phandle(np, "required-opps", index)) {
>> +               pstate = of_get_required_opp_performance_state(np, index);
>> +               if (pstate < 0) {
>> +                       ret = pstate;
>> +                       dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
>> +                               pd->name, ret);
>> +               }
>> +               dev_pm_genpd_set_performance_state(dev, pstate);
>> +               dev_gpd_data(dev)->default_pstate = pstate;
> 
> This doesn't look entirely correct to me. If we fail to translate a
> required opp to a performance state, we shouldn't try to set it.

yeah, that does not seem right at all :(

> Perhaps it's also easier to call
> of_get_required_opp_performance_state() unconditionally of whether a
> "required-opps" specifier exists. If it fails with the translation,
> then we just skip setting a default state and continue with returning
> 1.
> 
> Would that work?

I think it should, I'll redo the error handling, hopefully right this time,
and re-post. Thanks for the review.

> 
>> +       }
>>
>> -       return ret ? -EPROBE_DEFER : 1;
>> +       return ret ? ret : 1;
>>   }
>>
>>   /**
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index 21a0577..67017c9 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -198,6 +198,7 @@ struct generic_pm_domain_data {
>>          struct notifier_block *power_nb;
>>          int cpu;
>>          unsigned int performance_state;
>> +       unsigned int default_pstate;
>>          unsigned int rpm_pstate;
>>          ktime_t next_wakeup;
>>          void *data;
>> --
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
>> of Code Aurora Forum, hosted by The Linux Foundation
>>
> 
> Kind regards
> Uffe
> 

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 1/2] PM / Domains: Add support for 'required-opps' to set default perf state
  2021-08-03  4:38     ` Rajendra Nayak
@ 2021-08-04 11:08       ` Rajendra Nayak
  2021-08-04 11:39         ` Ulf Hansson
  0 siblings, 1 reply; 9+ messages in thread
From: Rajendra Nayak @ 2021-08-04 11:08 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Bjorn Andersson, Viresh Kumar, Linux PM, DTML,
	Linux Kernel Mailing List, linux-arm-msm, Stephen Boyd,
	Roja Rani Yarubandi, Stephan Gerhold


On 8/3/2021 10:08 AM, Rajendra Nayak wrote:
> 
> On 8/2/2021 6:29 PM, Ulf Hansson wrote:
>> On Tue, 20 Jul 2021 at 09:12, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>>
>>> Some devices within power domains with performance states do not
>>> support DVFS, but still need to vote on a default/static state
>>> while they are active. They can express this using the 'required-opps'
>>> property in device tree, which points to the phandle of the OPP
>>> supported by the corresponding power-domains.
>>>
>>> Add support to parse this information from DT and then set the
>>> specified performance state during attach and drop it on detach.
>>> runtime suspend/resume callbacks already have logic to drop/set
>>> the vote as needed and should take care of dropping the default
>>> perf state vote on runtime suspend and restore it back on runtime
>>> resume.
>>>
>>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>>> ---
>>>   drivers/base/power/domain.c | 28 +++++++++++++++++++++++++---
>>>   include/linux/pm_domain.h   |  1 +
>>>   2 files changed, 26 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>> index a934c67..f454031 100644
>>> --- a/drivers/base/power/domain.c
>>> +++ b/drivers/base/power/domain.c
>>> @@ -2598,6 +2598,12 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
>>>
>>>          dev_dbg(dev, "removing from PM domain %s\n", pd->name);
>>>
>>> +       /* Drop the default performance state */
>>> +       if (dev_gpd_data(dev)->default_pstate) {
>>> +               dev_pm_genpd_set_performance_state(dev, 0);
>>> +               dev_gpd_data(dev)->default_pstate = 0;
>>> +       }
>>> +
>>>          for (i = 1; i < GENPD_RETRY_MAX_MS; i <<= 1) {
>>>                  ret = genpd_remove_device(pd, dev);
>>>                  if (ret != -EAGAIN)
>>> @@ -2635,9 +2641,10 @@ static void genpd_dev_pm_sync(struct device *dev)
>>>   static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>>>                                   unsigned int index, bool power_on)
>>>   {
>>> +       struct device_node *np;
>>>          struct of_phandle_args pd_args;
>>>          struct generic_pm_domain *pd;
>>> -       int ret;
>>> +       int ret, pstate;
>>>
>>>          ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
>>>                                  "#power-domain-cells", index, &pd_args);
>>> @@ -2675,10 +2682,25 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>>>                  genpd_unlock(pd);
>>>          }
>>>
>>> -       if (ret)
>>> +       if (ret) {
>>>                  genpd_remove_device(pd, dev);
>>> +               return -EPROBE_DEFER;
>>> +       }
>>> +
>>> +       /* Set the default performance state */
>>> +       np = base_dev->of_node;
>>
>> Please use dev->of_node instead (it is set to the same of_node as
>> base_dev by the callers of __genpd_dev_pm_attach) as it's more
>> consistent with existing code.
>>
>>> +       if (of_parse_phandle(np, "required-opps", index)) {
>>> +               pstate = of_get_required_opp_performance_state(np, index);
>>> +               if (pstate < 0) {
>>> +                       ret = pstate;
>>> +                       dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
>>> +                               pd->name, ret);
>>> +               }
>>> +               dev_pm_genpd_set_performance_state(dev, pstate);
>>> +               dev_gpd_data(dev)->default_pstate = pstate;
>>
>> This doesn't look entirely correct to me. If we fail to translate a
>> required opp to a performance state, we shouldn't try to set it.
> 
> yeah, that does not seem right at all :(
> 
>> Perhaps it's also easier to call
>> of_get_required_opp_performance_state() unconditionally of whether a
>> "required-opps" specifier exists. If it fails with the translation,
>> then we just skip setting a default state and continue with returning
>> 1.
>>
>> Would that work?

Looks like calling of_get_required_opp_performance_state() unconditionally
makes it spit out a pr_err() in case the node is missing "required-opps" property,
so I posted a v6 [1] with the check in place and adding the missing else
condition.

[1] https://lore.kernel.org/patchwork/project/lkml/list/?series=510727

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 1/2] PM / Domains: Add support for 'required-opps' to set default perf state
  2021-08-04 11:08       ` Rajendra Nayak
@ 2021-08-04 11:39         ` Ulf Hansson
  2021-08-05  4:05           ` Viresh Kumar
  0 siblings, 1 reply; 9+ messages in thread
From: Ulf Hansson @ 2021-08-04 11:39 UTC (permalink / raw)
  To: Rajendra Nayak, Viresh Kumar
  Cc: Bjorn Andersson, Linux PM, DTML, Linux Kernel Mailing List,
	linux-arm-msm, Stephen Boyd, Roja Rani Yarubandi,
	Stephan Gerhold

On Wed, 4 Aug 2021 at 13:08, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>
>
> On 8/3/2021 10:08 AM, Rajendra Nayak wrote:
> >
> > On 8/2/2021 6:29 PM, Ulf Hansson wrote:
> >> On Tue, 20 Jul 2021 at 09:12, Rajendra Nayak <rnayak@codeaurora.org> wrote:
> >>>
> >>> Some devices within power domains with performance states do not
> >>> support DVFS, but still need to vote on a default/static state
> >>> while they are active. They can express this using the 'required-opps'
> >>> property in device tree, which points to the phandle of the OPP
> >>> supported by the corresponding power-domains.
> >>>
> >>> Add support to parse this information from DT and then set the
> >>> specified performance state during attach and drop it on detach.
> >>> runtime suspend/resume callbacks already have logic to drop/set
> >>> the vote as needed and should take care of dropping the default
> >>> perf state vote on runtime suspend and restore it back on runtime
> >>> resume.
> >>>
> >>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> >>> ---
> >>>   drivers/base/power/domain.c | 28 +++++++++++++++++++++++++---
> >>>   include/linux/pm_domain.h   |  1 +
> >>>   2 files changed, 26 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> >>> index a934c67..f454031 100644
> >>> --- a/drivers/base/power/domain.c
> >>> +++ b/drivers/base/power/domain.c
> >>> @@ -2598,6 +2598,12 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
> >>>
> >>>          dev_dbg(dev, "removing from PM domain %s\n", pd->name);
> >>>
> >>> +       /* Drop the default performance state */
> >>> +       if (dev_gpd_data(dev)->default_pstate) {
> >>> +               dev_pm_genpd_set_performance_state(dev, 0);
> >>> +               dev_gpd_data(dev)->default_pstate = 0;
> >>> +       }
> >>> +
> >>>          for (i = 1; i < GENPD_RETRY_MAX_MS; i <<= 1) {
> >>>                  ret = genpd_remove_device(pd, dev);
> >>>                  if (ret != -EAGAIN)
> >>> @@ -2635,9 +2641,10 @@ static void genpd_dev_pm_sync(struct device *dev)
> >>>   static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
> >>>                                   unsigned int index, bool power_on)
> >>>   {
> >>> +       struct device_node *np;
> >>>          struct of_phandle_args pd_args;
> >>>          struct generic_pm_domain *pd;
> >>> -       int ret;
> >>> +       int ret, pstate;
> >>>
> >>>          ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
> >>>                                  "#power-domain-cells", index, &pd_args);
> >>> @@ -2675,10 +2682,25 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
> >>>                  genpd_unlock(pd);
> >>>          }
> >>>
> >>> -       if (ret)
> >>> +       if (ret) {
> >>>                  genpd_remove_device(pd, dev);
> >>> +               return -EPROBE_DEFER;
> >>> +       }
> >>> +
> >>> +       /* Set the default performance state */
> >>> +       np = base_dev->of_node;
> >>
> >> Please use dev->of_node instead (it is set to the same of_node as
> >> base_dev by the callers of __genpd_dev_pm_attach) as it's more
> >> consistent with existing code.
> >>
> >>> +       if (of_parse_phandle(np, "required-opps", index)) {
> >>> +               pstate = of_get_required_opp_performance_state(np, index);
> >>> +               if (pstate < 0) {
> >>> +                       ret = pstate;
> >>> +                       dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
> >>> +                               pd->name, ret);
> >>> +               }
> >>> +               dev_pm_genpd_set_performance_state(dev, pstate);
> >>> +               dev_gpd_data(dev)->default_pstate = pstate;
> >>
> >> This doesn't look entirely correct to me. If we fail to translate a
> >> required opp to a performance state, we shouldn't try to set it.
> >
> > yeah, that does not seem right at all :(
> >
> >> Perhaps it's also easier to call
> >> of_get_required_opp_performance_state() unconditionally of whether a
> >> "required-opps" specifier exists. If it fails with the translation,
> >> then we just skip setting a default state and continue with returning
> >> 1.
> >>
> >> Would that work?
>
> Looks like calling of_get_required_opp_performance_state() unconditionally
> makes it spit out a pr_err() in case the node is missing "required-opps" property,
> so I posted a v6 [1] with the check in place and adding the missing else
> condition.

I see.

Viresh, would it make sense to remove that print? I mean, the
required-opps property could be considered as optional and it seems a
bit silly that a pre-parsing of the property is needed to figure that
out.

>
> [1] https://lore.kernel.org/patchwork/project/lkml/list/?series=510727

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v5 1/2] PM / Domains: Add support for 'required-opps' to set default perf state
  2021-08-04 11:39         ` Ulf Hansson
@ 2021-08-05  4:05           ` Viresh Kumar
  0 siblings, 0 replies; 9+ messages in thread
From: Viresh Kumar @ 2021-08-05  4:05 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Rajendra Nayak, Bjorn Andersson, Linux PM, DTML,
	Linux Kernel Mailing List, linux-arm-msm, Stephen Boyd,
	Roja Rani Yarubandi, Stephan Gerhold

On 04-08-21, 13:39, Ulf Hansson wrote:
> Viresh, would it make sense to remove that print? I mean, the
> required-opps property could be considered as optional and it seems a
> bit silly that a pre-parsing of the property is needed to figure that
> out.

Sure, np.

-- 
viresh

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-08-05  4:05 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-20  7:07 [PATCH v5 0/2] PM / Domains: Add support for 'required-opps' to set default perf state Rajendra Nayak
2021-07-20  7:07 ` [PATCH v5 1/2] " Rajendra Nayak
2021-07-20  7:18   ` Felipe Balbi
2021-08-02 12:59   ` Ulf Hansson
2021-08-03  4:38     ` Rajendra Nayak
2021-08-04 11:08       ` Rajendra Nayak
2021-08-04 11:39         ` Ulf Hansson
2021-08-05  4:05           ` Viresh Kumar
2021-07-20  7:07 ` [PATCH v5 2/2] arm64: dts: sc7180: Add required-opps for i2c Rajendra Nayak

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.