[PATCH v6 0/2] PM / Domains: Add support for 'required-opps' to set default perf state

[PATCH v6 0/2] PM / Domains: Add support for 'required-opps' to set default perf state

[Rajendra Nayak]

v6: Fixed up some more error handling in __genpd_dev_pm_attach()

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, 51 insertions(+), 2 deletions(-)

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

[PATCH v6 1/2] PM / Domains: Add support for 'required-opps' to set default perf state

[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, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index a934c67..b9b5a9b 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)
@@ -2637,6 +2643,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
 {
 	struct of_phandle_args pd_args;
 	struct generic_pm_domain *pd;
+	struct device_node *np;
+	int pstate;
 	int ret;
 
 	ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
@@ -2675,10 +2683,26 @@ 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 = 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);
+		} else {
+			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

[PATCH v6 2/2] arm64: dts: sc7180: Add required-opps for i2c

[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 9b65896..c5b64c6 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

Re: [PATCH v6 1/2] PM / Domains: Add support for 'required-opps' to set default perf state

[Ulf Hansson]

On Wed, 4 Aug 2021 at 12:58, 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, 27 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index a934c67..b9b5a9b 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)
> @@ -2637,6 +2643,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>  {
>         struct of_phandle_args pd_args;
>         struct generic_pm_domain *pd;
> +       struct device_node *np;
> +       int pstate;
>         int ret;
>
>         ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
> @@ -2675,10 +2683,26 @@ 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 = dev->of_node;
> +       if (of_parse_phandle(np, "required-opps", index)) {

Looks like Viresh thinks it's a good idea to drop the error print in
of_get_required_opp_performance_state() when there is no
"required-opps" specifier.

Would you mind folding in a patch for that in the series, so this code
can be simplified according to our earlier discussions?

> +               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);
> +               } else {
> +                       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;

Other than the above, this looks good to me!

Kind regards
Uffe

Re: [PATCH v6 1/2] PM / Domains: Add support for 'required-opps' to set default perf state

[Rajendra Nayak]

On 8/6/2021 3:02 PM, Ulf Hansson wrote:
> On Wed, 4 Aug 2021 at 12:58, 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, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index a934c67..b9b5a9b 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)
>> @@ -2637,6 +2643,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>>   {
>>          struct of_phandle_args pd_args;
>>          struct generic_pm_domain *pd;
>> +       struct device_node *np;
>> +       int pstate;
>>          int ret;
>>
>>          ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
>> @@ -2675,10 +2683,26 @@ 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 = dev->of_node;
>> +       if (of_parse_phandle(np, "required-opps", index)) {
> 
> Looks like Viresh thinks it's a good idea to drop the error print in
> of_get_required_opp_performance_state() when there is no
> "required-opps" specifier.
> 
> Would you mind folding in a patch for that in the series, so this code
> can be simplified according to our earlier discussions?

Sure, I can do that, apart from the error print, the function currently also
returns a -EINVAL in case of the missing 'required-opps', are we suggesting
we change that to not return an error also?

Since this is completely optional in the device node, we would want the function to
ideally not return error and only do so in case 'required-opps' exists and the
translation to performance state fails.
I am not sure that's the behavior we expect in case of 'required-opps' in the OPP
tables also, Viresh?

> 
>> +               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);
>> +               } else {
>> +                       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;
> 
> Other than the above, this looks good to me!
> 
> Kind regards
> Uffe
> 

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

Re: [PATCH v6 1/2] PM / Domains: Add support for 'required-opps' to set default perf state

[Dmitry Osipenko]

04.08.2021 13:58, Rajendra Nayak пишет:
> @@ -2637,6 +2643,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>  {
>  	struct of_phandle_args pd_args;
>  	struct generic_pm_domain *pd;
> +	struct device_node *np;
> +	int pstate;
>  	int ret;
>  
>  	ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
> @@ -2675,10 +2683,26 @@ 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 = 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);
> +		} else {
> +			dev_pm_genpd_set_performance_state(dev, pstate);
> +			dev_gpd_data(dev)->default_pstate = pstate;
> +		}
> +	}

Why performance state is set after genpd_power_on()?

Re: [PATCH v6 1/2] PM / Domains: Add support for 'required-opps' to set default perf state

[Dmitry Osipenko]

10.08.2021 01:26, Dmitry Osipenko пишет:
> 04.08.2021 13:58, Rajendra Nayak пишет:
>> @@ -2637,6 +2643,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>>  {
>>  	struct of_phandle_args pd_args;
>>  	struct generic_pm_domain *pd;
>> +	struct device_node *np;
>> +	int pstate;
>>  	int ret;
>>  
>>  	ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
>> @@ -2675,10 +2683,26 @@ 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 = 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);
>> +		} else {
>> +			dev_pm_genpd_set_performance_state(dev, pstate);

Where is error handling?

>> +			dev_gpd_data(dev)->default_pstate = pstate;
>> +		}
>> +	}
> 
> Why performance state is set after genpd_power_on()?
> 

Re: [PATCH v6 1/2] PM / Domains: Add support for 'required-opps' to set default perf state

[Dmitry Osipenko]

10.08.2021 01:44, Dmitry Osipenko пишет:
> 10.08.2021 01:26, Dmitry Osipenko пишет:
>> 04.08.2021 13:58, Rajendra Nayak пишет:
>>> @@ -2637,6 +2643,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>>>  {
>>>  	struct of_phandle_args pd_args;
>>>  	struct generic_pm_domain *pd;
>>> +	struct device_node *np;
>>> +	int pstate;
>>>  	int ret;
>>>  
>>>  	ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
>>> @@ -2675,10 +2683,26 @@ 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 = dev->of_node;
>>> +	if (of_parse_phandle(np, "required-opps", index)) {

The OF node returned by of_parse_phandle() must be put.

>>> +		pstate = of_get_required_opp_performance_state(np, index);

If you have more than one power domain, then this will override the
pstate which was set for a previous domain. This code doesn't feel solid
to me, at least a clarifying comment is needed about how it's supposed
to work.

>>> +		if (pstate < 0) {
>>> +			ret = pstate;
>>> +			dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
>>> +				pd->name, ret);
>>> +		} else {
>>> +			dev_pm_genpd_set_performance_state(dev, pstate);
> 
> Where is error handling?
> 
>>> +			dev_gpd_data(dev)->default_pstate = pstate;
>>> +		}
>>> +	}
>>
>> Why performance state is set after genpd_power_on()?

Should set_performance_state() be invoked when power_on=false?

I assume it should be:

if (power_on) {
	dev_pm_genpd_set_performance_state(dev, pstate);
	dev_gpd_data(dev)->default_pstate = pstate;
} else {
	dev_gpd_data(dev)->rpm_pstate = pstate;
}

Re: [PATCH v6 1/2] PM / Domains: Add support for 'required-opps' to set default perf state

[Dmitry Osipenko]

10.08.2021 02:46, Dmitry Osipenko пишет:
> 10.08.2021 01:44, Dmitry Osipenko пишет:
>> 10.08.2021 01:26, Dmitry Osipenko пишет:
>>> 04.08.2021 13:58, Rajendra Nayak пишет:
>>>> @@ -2637,6 +2643,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>>>>  {
>>>>  	struct of_phandle_args pd_args;
>>>>  	struct generic_pm_domain *pd;
>>>> +	struct device_node *np;
>>>> +	int pstate;
>>>>  	int ret;
>>>>  
>>>>  	ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
>>>> @@ -2675,10 +2683,26 @@ 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 = dev->of_node;
>>>> +	if (of_parse_phandle(np, "required-opps", index)) {
> 
> The OF node returned by of_parse_phandle() must be put.
> 
>>>> +		pstate = of_get_required_opp_performance_state(np, index);
> 
> If you have more than one power domain, then this will override the
> pstate which was set for a previous domain. This code doesn't feel solid
> to me, at least a clarifying comment is needed about how it's supposed
> to work.
> 
>>>> +		if (pstate < 0) {
>>>> +			ret = pstate;
>>>> +			dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
>>>> +				pd->name, ret);
>>>> +		} else {
>>>> +			dev_pm_genpd_set_performance_state(dev, pstate);
>>
>> Where is error handling?
>>
>>>> +			dev_gpd_data(dev)->default_pstate = pstate;
>>>> +		}
>>>> +	}
>>>
>>> Why performance state is set after genpd_power_on()?
> 
> Should set_performance_state() be invoked when power_on=false?
> 
> I assume it should be:
> 
> if (power_on) {
> 	dev_pm_genpd_set_performance_state(dev, pstate);
> 	dev_gpd_data(dev)->default_pstate = pstate;
> } else {
> 	dev_gpd_data(dev)->rpm_pstate = pstate;
> }
> 

Although, thinking a bit more about this, it should be better to skip
setting perf state in a case of power_on=false and let drivers to handle
it, IMO. Too much trouble with it for the core.

Re: [PATCH v6 1/2] PM / Domains: Add support for 'required-opps' to set default perf state

[Viresh Kumar]

On 09-08-21, 16:38, Rajendra Nayak wrote:
> Sure, I can do that, apart from the error print, the function currently also
> returns a -EINVAL in case of the missing 'required-opps', are we suggesting
> we change that to not return an error also?

No.

> Since this is completely optional in the device node, we would want the function to
> ideally not return error and only do so in case 'required-opps' exists and the
> translation to performance state fails.

Not really. The function should return failure if the property isn't
there, but it shouldn't be EINVAL but ENODEV.

-- 
viresh

Re: [PATCH v6 1/2] PM / Domains: Add support for 'required-opps' to set default perf state

[Rajendra Nayak]

On 8/10/2021 5:16 AM, Dmitry Osipenko wrote:
> 10.08.2021 01:44, Dmitry Osipenko пишет:
>> 10.08.2021 01:26, Dmitry Osipenko пишет:
>>> 04.08.2021 13:58, Rajendra Nayak пишет:
>>>> @@ -2637,6 +2643,8 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
>>>>   {
>>>>   	struct of_phandle_args pd_args;
>>>>   	struct generic_pm_domain *pd;
>>>> +	struct device_node *np;
>>>> +	int pstate;
>>>>   	int ret;
>>>>   
>>>>   	ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
>>>> @@ -2675,10 +2683,26 @@ 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 = dev->of_node;
>>>> +	if (of_parse_phandle(np, "required-opps", index)) {
> 
> The OF node returned by of_parse_phandle() must be put.

Thanks for spotting that, I will fix it, though I might just drop
the use of of_parse_phandle() and call of_get_required_opp_performance_state()
unconditionally as suggested by Ulf.

> 
>>>> +		pstate = of_get_required_opp_performance_state(np, index);
> 
> If you have more than one power domain, then this will override the
> pstate which was set for a previous domain. This code doesn't feel solid
> to me, at least a clarifying comment is needed about how it's supposed
> to work.

I don't quite understand the concern here, this should work with devices
having multiple power-domains as well. __genpd_dev_pm_attach gets called
once per power-domain, and we use the index above to identify the power-domain.
  
>>>> +		if (pstate < 0) {
>>>> +			ret = pstate;
>>>> +			dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
>>>> +				pd->name, ret);
>>>> +		} else {
>>>> +			dev_pm_genpd_set_performance_state(dev, pstate);
>>
>> Where is error handling?

Sure, I'll fix that.

>>
>>>> +			dev_gpd_data(dev)->default_pstate = pstate;
>>>> +		}
>>>> +	}
>>>
>>> Why performance state is set after genpd_power_on()?

The requirement is that the domain is operating at a given performance
state before the device becomes operational. The driver should ideally wait
for the attach to complete before expecting the device to be functional.
So the order here should really not matter, or rather the small amount of time
while the power-domain is on but not at the right performance state should
not matter if that's the concern.

> 
> Should set_performance_state() be invoked when power_on=false?
> 
> I assume it should be:
> 
> if (power_on) {
> 	dev_pm_genpd_set_performance_state(dev, pstate);
> 	dev_gpd_data(dev)->default_pstate = pstate;
> } else {
> 	dev_gpd_data(dev)->rpm_pstate = pstate;
> }
> 

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

Re: [PATCH v6 1/2] PM / Domains: Add support for 'required-opps' to set default perf state

[Rajendra Nayak]

On 8/10/2021 8:13 AM, Viresh Kumar wrote:
> On 09-08-21, 16:38, Rajendra Nayak wrote:
>> Sure, I can do that, apart from the error print, the function currently also
>> returns a -EINVAL in case of the missing 'required-opps', are we suggesting
>> we change that to not return an error also?
> 
> No.
> 
>> Since this is completely optional in the device node, we would want the function to
>> ideally not return error and only do so in case 'required-opps' exists and the
>> translation to performance state fails.
> 
> Not really. The function should return failure if the property isn't
> there, but it shouldn't be EINVAL but ENODEV.

In my case I don't want to error out if the property is missing, I want to error out
only when the property exists but can't be translated into a performance state.

So currently I check if the property exists and *only then* try to translate it, Ulf asked
me to skip the check. If I do that and I call of_get_required_opp_performance_state()
unconditionally, and if it errors out I will need to put in additional logic (check for
return value of ENODEV) to distinguish between the property-does-not-exist vs
property-exists-but-cannot-be-translated case.
It just seems more straight-forward to call this only when the property exists, Ulf?
  

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

Re: [PATCH v6 1/2] PM / Domains: Add support for 'required-opps' to set default perf state

[Viresh Kumar]

On 11-08-21, 15:30, Rajendra Nayak wrote:
> In my case I don't want to error out if the property is missing, I want to error out
> only when the property exists but can't be translated into a performance state.
> 
> So currently I check if the property exists and *only then* try to translate it, Ulf asked
> me to skip the check. If I do that and I call of_get_required_opp_performance_state()
> unconditionally, and if it errors out I will need to put in additional logic (check for
> return value of ENODEV) to distinguish between the property-does-not-exist vs
> property-exists-but-cannot-be-translated case.
> It just seems more straight-forward to call this only when the property exists, Ulf?

The same check will be done by OPP core as well, so it is better to
optimize for the success case here. I will say, don't error out on
ENODEV, rest you know well.

-- 
viresh

Re: [PATCH v6 1/2] PM / Domains: Add support for 'required-opps' to set default perf state

[Ulf Hansson]

On Wed, 11 Aug 2021 at 12:07, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 11-08-21, 15:30, Rajendra Nayak wrote:
> > In my case I don't want to error out if the property is missing, I want to error out
> > only when the property exists but can't be translated into a performance state.
> >
> > So currently I check if the property exists and *only then* try to translate it, Ulf asked
> > me to skip the check. If I do that and I call of_get_required_opp_performance_state()
> > unconditionally, and if it errors out I will need to put in additional logic (check for
> > return value of ENODEV) to distinguish between the property-does-not-exist vs
> > property-exists-but-cannot-be-translated case.
> > It just seems more straight-forward to call this only when the property exists, Ulf?
>
> The same check will be done by OPP core as well, so it is better to
> optimize for the success case here. I will say, don't error out on
> ENODEV, rest you know well.

This should work, while I generally favor not having to parse for
specific return codes.

Another option is to add a new OPP OF helperfunction that just informs
the caller whether the required-opps property exists (instead of
open-coding that part), and if so, the caller can continue with
of_get_required_opp_performance_state() and then expect it to succeed.

I have no strong opinion though! Whatever works for me.

Kind regards
Uffe

Re: [PATCH v6 1/2] PM / Domains: Add support for 'required-opps' to set default perf state

[Viresh Kumar]

On 11-08-21, 12:52, Ulf Hansson wrote:
> On Wed, 11 Aug 2021 at 12:07, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 11-08-21, 15:30, Rajendra Nayak wrote:
> > > In my case I don't want to error out if the property is missing, I want to error out
> > > only when the property exists but can't be translated into a performance state.
> > >
> > > So currently I check if the property exists and *only then* try to translate it, Ulf asked
> > > me to skip the check. If I do that and I call of_get_required_opp_performance_state()
> > > unconditionally, and if it errors out I will need to put in additional logic (check for
> > > return value of ENODEV) to distinguish between the property-does-not-exist vs
> > > property-exists-but-cannot-be-translated case.
> > > It just seems more straight-forward to call this only when the property exists, Ulf?
> >
> > The same check will be done by OPP core as well, so it is better to
> > optimize for the success case here. I will say, don't error out on
> > ENODEV, rest you know well.
> 
> This should work, while I generally favor not having to parse for
> specific return codes.
> 
> Another option is to add a new OPP OF helperfunction that just informs
> the caller whether the required-opps property exists (instead of
> open-coding that part), and if so, the caller can continue with
> of_get_required_opp_performance_state() and then expect it to succeed.
> 
> I have no strong opinion though! Whatever works for me.

I would like to work with the existing set of APIs, as the OPP core is
going to do that check anyways, again.

-- 
viresh

Re: [PATCH v6 1/2] PM / Domains: Add support for 'required-opps' to set default perf state

[Dmitry Osipenko]

11.08.2021 12:45, Rajendra Nayak пишет:
>>
>> If you have more than one power domain, then this will override the
>> pstate which was set for a previous domain. This code doesn't feel solid
>> to me, at least a clarifying comment is needed about how it's supposed
>> to work.
> 
> I don't quite understand the concern here, this should work with devices
> having multiple power-domains as well. __genpd_dev_pm_attach gets called
> once per power-domain, and we use the index above to identify the
> power-domain.

The domain core code supports only one domain per device, see what
genpd_set_performance_state() does. This means that the second domain
will set the state of the *first* domain, which doesn't make sense. The
genpd_set_performance_state() will actually fail with -ENODEV for all
domains if you will try to do that.

Re: [PATCH v6 1/2] PM / Domains: Add support for 'required-opps' to set default perf state

[Dmitry Osipenko]

11.08.2021 22:48, Dmitry Osipenko пишет:
> 11.08.2021 12:45, Rajendra Nayak пишет:
>>>
>>> If you have more than one power domain, then this will override the
>>> pstate which was set for a previous domain. This code doesn't feel solid
>>> to me, at least a clarifying comment is needed about how it's supposed
>>> to work.
>>
>> I don't quite understand the concern here, this should work with devices
>> having multiple power-domains as well. __genpd_dev_pm_attach gets called
>> once per power-domain, and we use the index above to identify the
>> power-domain.
> 
> The domain core code supports only one domain per device, see what
> genpd_set_performance_state() does. This means that the second domain
> will set the state of the *first* domain, which doesn't make sense. The
> genpd_set_performance_state() will actually fail with -ENODEV for all
> domains if you will try to do that.
> 

I confused the base device with the virtual device there, looks like it
should be okay then.