* [PATCH v4 0/2] PM / Domains: Add support for 'required-opps' to set default perf state @ 2021-07-16 10:00 Rajendra Nayak 2021-07-16 10:00 ` [PATCH v4 1/2] " Rajendra Nayak 2021-07-16 10:00 ` [PATCH v4 2/2] arm64: dts: sc7180: Add required-opps for i2c Rajendra Nayak 0 siblings, 2 replies; 19+ messages in thread From: Rajendra Nayak @ 2021-07-16 10:00 UTC (permalink / raw) To: ulf.hansson, bjorn.andersson, viresh.kumar Cc: linux-pm, devicetree, linux-kernel, linux-arm-msm, swboyd, rojay, stephan, Rajendra Nayak 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 | 37 +++++++++++++++++++++++++++++++++--- include/linux/pm_domain.h | 1 + 3 files changed, 59 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] 19+ messages in thread
* [PATCH v4 1/2] PM / Domains: Add support for 'required-opps' to set default perf state 2021-07-16 10:00 [PATCH v4 0/2] PM / Domains: Add support for 'required-opps' to set default perf state Rajendra Nayak @ 2021-07-16 10:00 ` Rajendra Nayak 2021-07-16 20:19 ` Stephen Boyd 2021-07-16 10:00 ` [PATCH v4 2/2] arm64: dts: sc7180: Add required-opps for i2c Rajendra Nayak 1 sibling, 1 reply; 19+ messages in thread From: Rajendra Nayak @ 2021-07-16 10:00 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 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. 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. Also drop/set as part of runtime suspend/resume callbacks. Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> --- drivers/base/power/domain.c | 37 ++++++++++++++++++++++++++++++++++--- include/linux/pm_domain.h | 1 + 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index a934c67..dcc0b71 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -917,6 +917,10 @@ static int genpd_runtime_suspend(struct device *dev) if (irq_safe_dev_in_no_sleep_domain(dev, genpd)) return 0; + /* Drop the default performance state */ + if (dev_gpd_data(dev)->default_pstate) + dev_pm_genpd_set_performance_state(dev, 0); + genpd_lock(genpd); gpd_data->rpm_pstate = genpd_drop_performance_state(dev); genpd_power_off(genpd, true, 0); @@ -937,6 +941,7 @@ static int genpd_runtime_resume(struct device *dev) { struct generic_pm_domain *genpd; struct generic_pm_domain_data *gpd_data = dev_gpd_data(dev); + unsigned int default_pstate = gpd_data->default_pstate; struct gpd_timing_data *td = &gpd_data->td; bool runtime_pm = pm_runtime_enabled(dev); ktime_t time_start; @@ -968,6 +973,9 @@ static int genpd_runtime_resume(struct device *dev) if (ret) return ret; + /* Set the default performance state */ + if (default_pstate) + dev_pm_genpd_set_performance_state(dev, default_pstate); out: /* Measure resume latency. */ time_start = 0; @@ -1000,6 +1008,8 @@ static int genpd_runtime_resume(struct device *dev) genpd_stop_dev(genpd, dev); err_poweroff: if (!pm_runtime_is_irq_safe(dev) || genpd_is_irq_safe(genpd)) { + if (default_pstate) + dev_pm_genpd_set_performance_state(dev, 0); genpd_lock(genpd); gpd_data->rpm_pstate = genpd_drop_performance_state(dev); genpd_power_off(genpd, true, 0); @@ -2598,6 +2608,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 +2651,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 +2692,24 @@ 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) { + dev_err(dev, "failed to set pstate:%d", pstate); + ret = pstate; + } + 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 related [flat|nested] 19+ messages in thread
* Re: [PATCH v4 1/2] PM / Domains: Add support for 'required-opps' to set default perf state 2021-07-16 10:00 ` [PATCH v4 1/2] " Rajendra Nayak @ 2021-07-16 20:19 ` Stephen Boyd 2021-07-19 11:50 ` Rajendra Nayak 0 siblings, 1 reply; 19+ messages in thread From: Stephen Boyd @ 2021-07-16 20:19 UTC (permalink / raw) To: Rajendra Nayak, bjorn.andersson, ulf.hansson, viresh.kumar Cc: linux-pm, devicetree, linux-kernel, linux-arm-msm, rojay, stephan Quoting Rajendra Nayak (2021-07-16 03:00:57) > Some devics within power domains with performance states do not devices > 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. > Also drop/set as part of runtime suspend/resume callbacks. > > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > --- > drivers/base/power/domain.c | 37 ++++++++++++++++++++++++++++++++++--- > include/linux/pm_domain.h | 1 + > 2 files changed, 35 insertions(+), 3 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index a934c67..dcc0b71 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1000,6 +1008,8 @@ static int genpd_runtime_resume(struct device *dev) > genpd_stop_dev(genpd, dev); > err_poweroff: > if (!pm_runtime_is_irq_safe(dev) || genpd_is_irq_safe(genpd)) { > + if (default_pstate) > + dev_pm_genpd_set_performance_state(dev, 0); > genpd_lock(genpd); > gpd_data->rpm_pstate = genpd_drop_performance_state(dev); Maybe this should be prev_state = genpd_drop_performance_state(dev); if (!default_pstate) gdp_data->rpm_pstate = prev_state; so we don't call dev_pm_genpd_set_performance_state() effectively twice? Also it would make sure we call dev_pm_genpd_set_performance_state() underneath the genpd_lock() if that is important. Similarly do that on suspend path. > genpd_power_off(genpd, true, 0); > @@ -2598,6 +2608,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) > @@ -2675,10 +2692,24 @@ 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) { > + dev_err(dev, "failed to set pstate:%d", pstate); Missing newline on printk. Also can we spell out pstate as "failed to set required performance state %d for power-domain %d"? > + ret = pstate; > + } > + dev_pm_genpd_set_performance_state(dev, pstate); > + dev_gpd_data(dev)->default_pstate = pstate; > + } > > - return ret ? -EPROBE_DEFER : 1; > + return ret ? ret : 1; > } > > /** ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v4 1/2] PM / Domains: Add support for 'required-opps' to set default perf state 2021-07-16 20:19 ` Stephen Boyd @ 2021-07-19 11:50 ` Rajendra Nayak 0 siblings, 0 replies; 19+ messages in thread From: Rajendra Nayak @ 2021-07-19 11:50 UTC (permalink / raw) To: Stephen Boyd, bjorn.andersson, ulf.hansson, viresh.kumar Cc: linux-pm, devicetree, linux-kernel, linux-arm-msm, rojay, stephan On 7/17/2021 1:49 AM, Stephen Boyd wrote: > Quoting Rajendra Nayak (2021-07-16 03:00:57) >> Some devics within power domains with performance states do not > > devices > >> 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. >> Also drop/set as part of runtime suspend/resume callbacks. >> >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> >> --- >> drivers/base/power/domain.c | 37 ++++++++++++++++++++++++++++++++++--- >> include/linux/pm_domain.h | 1 + >> 2 files changed, 35 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >> index a934c67..dcc0b71 100644 >> --- a/drivers/base/power/domain.c >> +++ b/drivers/base/power/domain.c >> @@ -1000,6 +1008,8 @@ static int genpd_runtime_resume(struct device *dev) >> genpd_stop_dev(genpd, dev); >> err_poweroff: >> if (!pm_runtime_is_irq_safe(dev) || genpd_is_irq_safe(genpd)) { >> + if (default_pstate) >> + dev_pm_genpd_set_performance_state(dev, 0); >> genpd_lock(genpd); >> gpd_data->rpm_pstate = genpd_drop_performance_state(dev); > > Maybe this should be > > prev_state = genpd_drop_performance_state(dev); > if (!default_pstate) > gdp_data->rpm_pstate = prev_state; > > so we don't call dev_pm_genpd_set_performance_state() effectively twice? > Also it would make sure we call dev_pm_genpd_set_performance_state() > underneath the genpd_lock() if that is important. Similarly do that on > suspend path. looking through this more, I think I can completely drop any special handling for default_pstate in runtime suspend and resume. The existing drop/restore login Ulf has added should take care of it. I'll test and respin soon. > >> genpd_power_off(genpd, true, 0); >> @@ -2598,6 +2608,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) >> @@ -2675,10 +2692,24 @@ 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) { >> + dev_err(dev, "failed to set pstate:%d", pstate); > > Missing newline on printk. Also can we spell out pstate as "failed to > set required performance state %d for power-domain %d"? thanks, will fix when I respin. Thanks for the review. > >> + ret = pstate; >> + } >> + dev_pm_genpd_set_performance_state(dev, pstate); >> + dev_gpd_data(dev)->default_pstate = pstate; >> + } >> >> - return ret ? -EPROBE_DEFER : 1; >> + return ret ? ret : 1; >> } >> >> /** -- 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] 19+ messages in thread
* [PATCH v4 2/2] arm64: dts: sc7180: Add required-opps for i2c 2021-07-16 10:00 [PATCH v4 0/2] PM / Domains: Add support for 'required-opps' to set default perf state Rajendra Nayak 2021-07-16 10:00 ` [PATCH v4 1/2] " Rajendra Nayak @ 2021-07-16 10:00 ` Rajendra Nayak 2021-07-16 19:39 ` Stephen Boyd 2021-07-16 20:18 ` Bjorn Andersson 1 sibling, 2 replies; 19+ messages in thread From: Rajendra Nayak @ 2021-07-16 10:00 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> --- 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 related [flat|nested] 19+ messages in thread
* Re: [PATCH v4 2/2] arm64: dts: sc7180: Add required-opps for i2c 2021-07-16 10:00 ` [PATCH v4 2/2] arm64: dts: sc7180: Add required-opps for i2c Rajendra Nayak @ 2021-07-16 19:39 ` Stephen Boyd 2021-07-16 20:18 ` Bjorn Andersson 1 sibling, 0 replies; 19+ messages in thread From: Stephen Boyd @ 2021-07-16 19:39 UTC (permalink / raw) To: Rajendra Nayak, bjorn.andersson, ulf.hansson, viresh.kumar Cc: linux-pm, devicetree, linux-kernel, linux-arm-msm, rojay, stephan Quoting Rajendra Nayak (2021-07-16 03:00:58) > 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> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v4 2/2] arm64: dts: sc7180: Add required-opps for i2c 2021-07-16 10:00 ` [PATCH v4 2/2] arm64: dts: sc7180: Add required-opps for i2c Rajendra Nayak 2021-07-16 19:39 ` Stephen Boyd @ 2021-07-16 20:18 ` Bjorn Andersson 2021-07-16 20:21 ` Stephen Boyd 1 sibling, 1 reply; 19+ messages in thread From: Bjorn Andersson @ 2021-07-16 20:18 UTC (permalink / raw) To: Rajendra Nayak Cc: ulf.hansson, viresh.kumar, linux-pm, devicetree, linux-kernel, linux-arm-msm, swboyd, rojay, stephan On Fri 16 Jul 05:00 CDT 2021, Rajendra Nayak wrote: > 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. > Sounds good, but... > Use 'required-opps' to pass this information from > device tree, and also add the power-domains property to specify > the CX power-domain. > ..is the required-opps really needed with my rpmhpd patch in place? Regards, Bjorn > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.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] 19+ messages in thread
* Re: [PATCH v4 2/2] arm64: dts: sc7180: Add required-opps for i2c 2021-07-16 20:18 ` Bjorn Andersson @ 2021-07-16 20:21 ` Stephen Boyd 2021-07-16 20:52 ` Bjorn Andersson 0 siblings, 1 reply; 19+ messages in thread From: Stephen Boyd @ 2021-07-16 20:21 UTC (permalink / raw) To: Bjorn Andersson, Rajendra Nayak Cc: ulf.hansson, viresh.kumar, linux-pm, devicetree, linux-kernel, linux-arm-msm, rojay, stephan Quoting Bjorn Andersson (2021-07-16 13:18:56) > On Fri 16 Jul 05:00 CDT 2021, Rajendra Nayak wrote: > > > 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. > > > > Sounds good, but... > > > Use 'required-opps' to pass this information from > > device tree, and also add the power-domains property to specify > > the CX power-domain. > > > > ..is the required-opps really needed with my rpmhpd patch in place? > Yes? Because rpmhpd_opp_low_svs is not the lowest performance state for CX. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v4 2/2] arm64: dts: sc7180: Add required-opps for i2c 2021-07-16 20:21 ` Stephen Boyd @ 2021-07-16 20:52 ` Bjorn Andersson 2021-07-16 21:49 ` Stephen Boyd 0 siblings, 1 reply; 19+ messages in thread From: Bjorn Andersson @ 2021-07-16 20:52 UTC (permalink / raw) To: Stephen Boyd Cc: Rajendra Nayak, ulf.hansson, viresh.kumar, linux-pm, devicetree, linux-kernel, linux-arm-msm, rojay, stephan On Fri 16 Jul 15:21 CDT 2021, Stephen Boyd wrote: > Quoting Bjorn Andersson (2021-07-16 13:18:56) > > On Fri 16 Jul 05:00 CDT 2021, Rajendra Nayak wrote: > > > > > 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. > > > > > > > Sounds good, but... > > > > > Use 'required-opps' to pass this information from > > > device tree, and also add the power-domains property to specify > > > the CX power-domain. > > > > > > > ..is the required-opps really needed with my rpmhpd patch in place? > > > > Yes? Because rpmhpd_opp_low_svs is not the lowest performance state for > CX. On e.g. sm8250 the first available non-zero corner presented in cmd-db is low_svs. And if this (which?) clock requires a higher corner than the lowest possible in order to tick at this "lowest" frequency, I'm certainly interested in some more details. Regards, Bjorn ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v4 2/2] arm64: dts: sc7180: Add required-opps for i2c 2021-07-16 20:52 ` Bjorn Andersson @ 2021-07-16 21:49 ` Stephen Boyd 2021-07-16 21:59 ` Bjorn Andersson 0 siblings, 1 reply; 19+ messages in thread From: Stephen Boyd @ 2021-07-16 21:49 UTC (permalink / raw) To: Bjorn Andersson Cc: Rajendra Nayak, ulf.hansson, viresh.kumar, linux-pm, devicetree, linux-kernel, linux-arm-msm, rojay, stephan Quoting Bjorn Andersson (2021-07-16 13:52:12) > On Fri 16 Jul 15:21 CDT 2021, Stephen Boyd wrote: > > > Quoting Bjorn Andersson (2021-07-16 13:18:56) > > > On Fri 16 Jul 05:00 CDT 2021, Rajendra Nayak wrote: > > > > > > > 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. > > > > > > > > > > Sounds good, but... > > > > > > > Use 'required-opps' to pass this information from > > > > device tree, and also add the power-domains property to specify > > > > the CX power-domain. > > > > > > > > > > ..is the required-opps really needed with my rpmhpd patch in place? > > > > > > > Yes? Because rpmhpd_opp_low_svs is not the lowest performance state for > > CX. > > On e.g. sm8250 the first available non-zero corner presented in cmd-db > is low_svs. Indeed. On sc7180 it's not the first non-zero corner. I suppose retention for CX isn't actually used when the SoC is awake so your rpmhpd patch is putting in a vote for something that doesn't do anything at runtime for CX? I imagine that rpmh only sets the aggregate corner to retention when the whole SoC is suspended/sleeping, otherwise things wouldn't go very well. Similarly, min_svs may be VDD minimization? If so, those first two states are basically states that shouldn't be used at runtime, almost like sleep states. > > And if this (which?) clock requires a higher corner than the lowest > possible in order to tick at this "lowest" frequency, I'm certainly > interested in some more details. > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v4 2/2] arm64: dts: sc7180: Add required-opps for i2c 2021-07-16 21:49 ` Stephen Boyd @ 2021-07-16 21:59 ` Bjorn Andersson 2021-07-19 9:37 ` Rajendra Nayak 0 siblings, 1 reply; 19+ messages in thread From: Bjorn Andersson @ 2021-07-16 21:59 UTC (permalink / raw) To: Stephen Boyd Cc: Rajendra Nayak, ulf.hansson, viresh.kumar, linux-pm, devicetree, linux-kernel, linux-arm-msm, rojay, stephan On Fri 16 Jul 16:49 CDT 2021, Stephen Boyd wrote: > Quoting Bjorn Andersson (2021-07-16 13:52:12) > > On Fri 16 Jul 15:21 CDT 2021, Stephen Boyd wrote: > > > > > Quoting Bjorn Andersson (2021-07-16 13:18:56) > > > > On Fri 16 Jul 05:00 CDT 2021, Rajendra Nayak wrote: > > > > > > > > > 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. > > > > > > > > > > > > > Sounds good, but... > > > > > > > > > Use 'required-opps' to pass this information from > > > > > device tree, and also add the power-domains property to specify > > > > > the CX power-domain. > > > > > > > > > > > > > ..is the required-opps really needed with my rpmhpd patch in place? > > > > > > > > > > Yes? Because rpmhpd_opp_low_svs is not the lowest performance state for > > > CX. > > > > On e.g. sm8250 the first available non-zero corner presented in cmd-db > > is low_svs. > > Indeed. On sc7180 it's not the first non-zero corner. I suppose > retention for CX isn't actually used when the SoC is awake so your > rpmhpd patch is putting in a vote for something that doesn't do anything > at runtime for CX? I imagine that rpmh only sets the aggregate corner to > retention when the whole SoC is suspended/sleeping, otherwise things > wouldn't go very well. Similarly, min_svs may be VDD minimization? If > so, those first two states are basically states that shouldn't be used > at runtime, almost like sleep states. > But if that's the case, I don't think it's appropriate for the "enabled state" of the domain to use any of those corners. As this means that anyone who needs any of the rpmhpd domains active also needs to specify required-opps, which wouldn't be needed for any other power domain provider. And more importantly it means that a device sitting in a GDSC, which would be parented by a rpmhpd domain has no way to specify the GDSC and trickle the minimum-vote up to the rpmhpd domain. (And I know that we don't describe the parentship of the GDSCs today, but this patch tells me that it's around the corner - for more than MMCX) Regards, Bjorn > > > > And if this (which?) clock requires a higher corner than the lowest > > possible in order to tick at this "lowest" frequency, I'm certainly > > interested in some more details. > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v4 2/2] arm64: dts: sc7180: Add required-opps for i2c 2021-07-16 21:59 ` Bjorn Andersson @ 2021-07-19 9:37 ` Rajendra Nayak 2021-07-19 19:19 ` Bjorn Andersson 0 siblings, 1 reply; 19+ messages in thread From: Rajendra Nayak @ 2021-07-19 9:37 UTC (permalink / raw) To: Bjorn Andersson, Stephen Boyd Cc: ulf.hansson, viresh.kumar, linux-pm, devicetree, linux-kernel, linux-arm-msm, rojay, stephan On 7/17/2021 3:29 AM, Bjorn Andersson wrote: > On Fri 16 Jul 16:49 CDT 2021, Stephen Boyd wrote: > >> Quoting Bjorn Andersson (2021-07-16 13:52:12) >>> On Fri 16 Jul 15:21 CDT 2021, Stephen Boyd wrote: >>> >>>> Quoting Bjorn Andersson (2021-07-16 13:18:56) >>>>> On Fri 16 Jul 05:00 CDT 2021, Rajendra Nayak wrote: >>>>> >>>>>> 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. >>>>>> >>>>> >>>>> Sounds good, but... >>>>> >>>>>> Use 'required-opps' to pass this information from >>>>>> device tree, and also add the power-domains property to specify >>>>>> the CX power-domain. >>>>>> >>>>> >>>>> ..is the required-opps really needed with my rpmhpd patch in place? >>>>> >>>> >>>> Yes? Because rpmhpd_opp_low_svs is not the lowest performance state for >>>> CX. >>> >>> On e.g. sm8250 the first available non-zero corner presented in cmd-db >>> is low_svs. what rail is this? the mmcx? Perhaps it does not support RET. cx usually supports both collapse state and RET. >> >> Indeed. On sc7180 it's not the first non-zero corner. I suppose >> retention for CX isn't actually used when the SoC is awake so your >> rpmhpd patch is putting in a vote for something that doesn't do anything >> at runtime for CX? I imagine that rpmh only sets the aggregate corner to >> retention when the whole SoC is suspended/sleeping, otherwise things >> wouldn't go very well. Similarly, min_svs may be VDD minimization? If >> so, those first two states are basically states that shouldn't be used >> at runtime, almost like sleep states. >> > > But if that's the case, I don't think it's appropriate for the "enabled > state" of the domain to use any of those corners. I rechecked the downstream kernels where all this voting happens from within the clock drivers, and I do see votes to min_svs for some clocks, but Stephen is right that RET is not something that's voted on while in active state. But always going with something just above the ret level while active will also not work for all devices, for instance for i2c on 7180, it needs a cx vote of low svs while the rail (cx) does support something lower than that which is min svs. (why can't it just work with min svs?, I don't know, these values and recommendations come in from the voltage plans published by HW teams for every SoC and we just end up using them in SW, perhaps something to dig further and understand which I will try and do but these are the values in voltage plans and downstream kernels which work for now) > > As this means that anyone who needs any of the rpmhpd domains active > also needs to specify required-opps, which wouldn't be needed for any > other power domain provider. > > And more importantly it means that a device sitting in a GDSC, which > would be parented by a rpmhpd domain has no way to specify the GDSC and > trickle the minimum-vote up to the rpmhpd domain. (And I know that we > don't describe the parentship of the GDSCs today, but this patch > tells me that it's around the corner - for more than MMCX) > > Regards, > Bjorn > >>> >>> And if this (which?) clock requires a higher corner than the lowest >>> possible in order to tick at this "lowest" frequency, I'm certainly >>> interested in some more details. >>> -- 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] 19+ messages in thread
* Re: [PATCH v4 2/2] arm64: dts: sc7180: Add required-opps for i2c 2021-07-19 9:37 ` Rajendra Nayak @ 2021-07-19 19:19 ` Bjorn Andersson 2021-07-20 4:29 ` Rajendra Nayak 0 siblings, 1 reply; 19+ messages in thread From: Bjorn Andersson @ 2021-07-19 19:19 UTC (permalink / raw) To: Rajendra Nayak Cc: Stephen Boyd, ulf.hansson, viresh.kumar, linux-pm, devicetree, linux-kernel, linux-arm-msm, rojay, stephan On Mon 19 Jul 04:37 CDT 2021, Rajendra Nayak wrote: > > > On 7/17/2021 3:29 AM, Bjorn Andersson wrote: > > On Fri 16 Jul 16:49 CDT 2021, Stephen Boyd wrote: > > > > > Quoting Bjorn Andersson (2021-07-16 13:52:12) > > > > On Fri 16 Jul 15:21 CDT 2021, Stephen Boyd wrote: > > > > > > > > > Quoting Bjorn Andersson (2021-07-16 13:18:56) > > > > > > On Fri 16 Jul 05:00 CDT 2021, Rajendra Nayak wrote: > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > Sounds good, but... > > > > > > > > > > > > > Use 'required-opps' to pass this information from > > > > > > > device tree, and also add the power-domains property to specify > > > > > > > the CX power-domain. > > > > > > > > > > > > > > > > > > > ..is the required-opps really needed with my rpmhpd patch in place? > > > > > > > > > > > > > > > > Yes? Because rpmhpd_opp_low_svs is not the lowest performance state for > > > > > CX. > > > > > > > > On e.g. sm8250 the first available non-zero corner presented in cmd-db > > > > is low_svs. > > what rail is this? the mmcx? Perhaps it does not support RET. > cx usually supports both collapse state and RET. > That was the one I was specifically looking at for the MDSS_GDSC->MMCX issue, so it's likely I didn't look elsewhere. > > > > > > Indeed. On sc7180 it's not the first non-zero corner. I suppose > > > retention for CX isn't actually used when the SoC is awake so your > > > rpmhpd patch is putting in a vote for something that doesn't do anything > > > at runtime for CX? I imagine that rpmh only sets the aggregate corner to > > > retention when the whole SoC is suspended/sleeping, otherwise things > > > wouldn't go very well. Similarly, min_svs may be VDD minimization? If > > > so, those first two states are basically states that shouldn't be used > > > at runtime, almost like sleep states. > > > > > > > But if that's the case, I don't think it's appropriate for the "enabled > > state" of the domain to use any of those corners. > > I rechecked the downstream kernels where all this voting happens from within > the clock drivers, and I do see votes to min_svs for some clocks, but Stephen is > right that RET is not something that's voted on while in active state. > > But always going with something just above the ret level while active will also > not work for all devices, for instance for i2c on 7180, it needs a cx vote of > low svs while the rail (cx) does support something lower than that which is min svs. > (why can't it just work with min svs?, I don't know, these values and recommendations > come in from the voltage plans published by HW teams for every SoC and we just end up > using them in SW, perhaps something to dig further and understand which I will try and > do but these are the values in voltage plans and downstream kernels which work for now) > So to some degree this invalidates my argumentation about the enabled_corner in rpmhpd, given that "enabled" means a different corner for each rail - not just the one with lowest non-zero value. So perhaps instead of introducing the enabled_corner we need to introduce your patch and slap a WARN_ON(corner == 0) in rpmhpd_power_on() - to ensure that all clients that uses a rpmhpd domain actually do vote for a high enough corner? Regards, Bjorn > > > > As this means that anyone who needs any of the rpmhpd domains active > > also needs to specify required-opps, which wouldn't be needed for any > > other power domain provider. > > > > And more importantly it means that a device sitting in a GDSC, which > > would be parented by a rpmhpd domain has no way to specify the GDSC and > > trickle the minimum-vote up to the rpmhpd domain. (And I know that we > > don't describe the parentship of the GDSCs today, but this patch > > tells me that it's around the corner - for more than MMCX) > > > > Regards, > > Bjorn > > > > > > > > > > And if this (which?) clock requires a higher corner than the lowest > > > > possible in order to tick at this "lowest" frequency, I'm certainly > > > > interested in some more details. > > > > > > -- > 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] 19+ messages in thread
* Re: [PATCH v4 2/2] arm64: dts: sc7180: Add required-opps for i2c 2021-07-19 19:19 ` Bjorn Andersson @ 2021-07-20 4:29 ` Rajendra Nayak 2021-07-25 17:01 ` Bjorn Andersson 0 siblings, 1 reply; 19+ messages in thread From: Rajendra Nayak @ 2021-07-20 4:29 UTC (permalink / raw) To: Bjorn Andersson Cc: Stephen Boyd, ulf.hansson, viresh.kumar, linux-pm, devicetree, linux-kernel, linux-arm-msm, rojay, stephan On 7/20/2021 12:49 AM, Bjorn Andersson wrote: > On Mon 19 Jul 04:37 CDT 2021, Rajendra Nayak wrote: > >> >> >> On 7/17/2021 3:29 AM, Bjorn Andersson wrote: >>> On Fri 16 Jul 16:49 CDT 2021, Stephen Boyd wrote: >>> >>>> Quoting Bjorn Andersson (2021-07-16 13:52:12) >>>>> On Fri 16 Jul 15:21 CDT 2021, Stephen Boyd wrote: >>>>> >>>>>> Quoting Bjorn Andersson (2021-07-16 13:18:56) >>>>>>> On Fri 16 Jul 05:00 CDT 2021, Rajendra Nayak wrote: >>>>>>> >>>>>>>> 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. >>>>>>>> >>>>>>> >>>>>>> Sounds good, but... >>>>>>> >>>>>>>> Use 'required-opps' to pass this information from >>>>>>>> device tree, and also add the power-domains property to specify >>>>>>>> the CX power-domain. >>>>>>>> >>>>>>> >>>>>>> ..is the required-opps really needed with my rpmhpd patch in place? >>>>>>> >>>>>> >>>>>> Yes? Because rpmhpd_opp_low_svs is not the lowest performance state for >>>>>> CX. >>>>> >>>>> On e.g. sm8250 the first available non-zero corner presented in cmd-db >>>>> is low_svs. >> >> what rail is this? the mmcx? Perhaps it does not support RET. >> cx usually supports both collapse state and RET. >> > > That was the one I was specifically looking at for the MDSS_GDSC->MMCX > issue, so it's likely I didn't look elsewhere. > >>>> >>>> Indeed. On sc7180 it's not the first non-zero corner. I suppose >>>> retention for CX isn't actually used when the SoC is awake so your >>>> rpmhpd patch is putting in a vote for something that doesn't do anything >>>> at runtime for CX? I imagine that rpmh only sets the aggregate corner to >>>> retention when the whole SoC is suspended/sleeping, otherwise things >>>> wouldn't go very well. Similarly, min_svs may be VDD minimization? If >>>> so, those first two states are basically states that shouldn't be used >>>> at runtime, almost like sleep states. >>>> >>> >>> But if that's the case, I don't think it's appropriate for the "enabled >>> state" of the domain to use any of those corners. >> >> I rechecked the downstream kernels where all this voting happens from within >> the clock drivers, and I do see votes to min_svs for some clocks, but Stephen is >> right that RET is not something that's voted on while in active state. >> >> But always going with something just above the ret level while active will also >> not work for all devices, for instance for i2c on 7180, it needs a cx vote of >> low svs while the rail (cx) does support something lower than that which is min svs. >> (why can't it just work with min svs?, I don't know, these values and recommendations >> come in from the voltage plans published by HW teams for every SoC and we just end up >> using them in SW, perhaps something to dig further and understand which I will try and >> do but these are the values in voltage plans and downstream kernels which work for now) >> > > So to some degree this invalidates my argumentation about the > enabled_corner in rpmhpd, given that "enabled" means a different corner > for each rail - not just the one with lowest non-zero value. Right, it might work in some cases but might not work for all. > > So perhaps instead of introducing the enabled_corner we need to > introduce your patch and slap a WARN_ON(corner == 0) in > rpmhpd_power_on() - to ensure that all clients that uses a rpmhpd domain > actually do vote for a high enough corner? So this would mean the expectation is that the clients set the perf state/corner before they call power_on? I don;t think that's the case today with most clients, infact its the opposite, we power on first and then make a call to set the perf state of the domain. > > Regards, > Bjorn > >>> >>> As this means that anyone who needs any of the rpmhpd domains active >>> also needs to specify required-opps, which wouldn't be needed for any >>> other power domain provider. >>> >>> And more importantly it means that a device sitting in a GDSC, which >>> would be parented by a rpmhpd domain has no way to specify the GDSC and >>> trickle the minimum-vote up to the rpmhpd domain. (And I know that we >>> don't describe the parentship of the GDSCs today, but this patch >>> tells me that it's around the corner - for more than MMCX) >>> >>> Regards, >>> Bjorn >>> >>>>> >>>>> And if this (which?) clock requires a higher corner than the lowest >>>>> possible in order to tick at this "lowest" frequency, I'm certainly >>>>> interested in some more details. >>>>> >> >> -- >> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member >> of Code Aurora Forum, hosted by The Linux Foundation -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v4 2/2] arm64: dts: sc7180: Add required-opps for i2c 2021-07-20 4:29 ` Rajendra Nayak @ 2021-07-25 17:01 ` Bjorn Andersson 2021-07-27 7:35 ` Rajendra Nayak 0 siblings, 1 reply; 19+ messages in thread From: Bjorn Andersson @ 2021-07-25 17:01 UTC (permalink / raw) To: Rajendra Nayak Cc: Stephen Boyd, ulf.hansson, viresh.kumar, linux-pm, devicetree, linux-kernel, linux-arm-msm, rojay, stephan On Mon 19 Jul 23:29 CDT 2021, Rajendra Nayak wrote: > > > On 7/20/2021 12:49 AM, Bjorn Andersson wrote: > > On Mon 19 Jul 04:37 CDT 2021, Rajendra Nayak wrote: > > > > > > > > > > > On 7/17/2021 3:29 AM, Bjorn Andersson wrote: > > > > On Fri 16 Jul 16:49 CDT 2021, Stephen Boyd wrote: > > > > > > > > > Quoting Bjorn Andersson (2021-07-16 13:52:12) > > > > > > On Fri 16 Jul 15:21 CDT 2021, Stephen Boyd wrote: > > > > > > > > > > > > > Quoting Bjorn Andersson (2021-07-16 13:18:56) > > > > > > > > On Fri 16 Jul 05:00 CDT 2021, Rajendra Nayak wrote: > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > Sounds good, but... > > > > > > > > > > > > > > > > > Use 'required-opps' to pass this information from > > > > > > > > > device tree, and also add the power-domains property to specify > > > > > > > > > the CX power-domain. > > > > > > > > > > > > > > > > > > > > > > > > > ..is the required-opps really needed with my rpmhpd patch in place? > > > > > > > > > > > > > > > > > > > > > > Yes? Because rpmhpd_opp_low_svs is not the lowest performance state for > > > > > > > CX. > > > > > > > > > > > > On e.g. sm8250 the first available non-zero corner presented in cmd-db > > > > > > is low_svs. > > > > > > what rail is this? the mmcx? Perhaps it does not support RET. > > > cx usually supports both collapse state and RET. > > > > > > > That was the one I was specifically looking at for the MDSS_GDSC->MMCX > > issue, so it's likely I didn't look elsewhere. > > > > > > > > > > > > Indeed. On sc7180 it's not the first non-zero corner. I suppose > > > > > retention for CX isn't actually used when the SoC is awake so your > > > > > rpmhpd patch is putting in a vote for something that doesn't do anything > > > > > at runtime for CX? I imagine that rpmh only sets the aggregate corner to > > > > > retention when the whole SoC is suspended/sleeping, otherwise things > > > > > wouldn't go very well. Similarly, min_svs may be VDD minimization? If > > > > > so, those first two states are basically states that shouldn't be used > > > > > at runtime, almost like sleep states. > > > > > > > > > > > > > But if that's the case, I don't think it's appropriate for the "enabled > > > > state" of the domain to use any of those corners. > > > > > > I rechecked the downstream kernels where all this voting happens from within > > > the clock drivers, and I do see votes to min_svs for some clocks, but Stephen is > > > right that RET is not something that's voted on while in active state. > > > > > > But always going with something just above the ret level while active will also > > > not work for all devices, for instance for i2c on 7180, it needs a cx vote of > > > low svs while the rail (cx) does support something lower than that which is min svs. > > > (why can't it just work with min svs?, I don't know, these values and recommendations > > > come in from the voltage plans published by HW teams for every SoC and we just end up > > > using them in SW, perhaps something to dig further and understand which I will try and > > > do but these are the values in voltage plans and downstream kernels which work for now) > > > > > > > So to some degree this invalidates my argumentation about the > > enabled_corner in rpmhpd, given that "enabled" means a different corner > > for each rail - not just the one with lowest non-zero value. > > Right, it might work in some cases but might not work for all. > Which makes it way less desirable. The enable state for rpmhpd power domains doesn't meet my expectations for how a power domain should behave, but we should at least be consistent across all consumers of it then... But the original issue remains, that when a device is powered by MDSS_GDSC, which is a subdomain of MMCX we still need to ensure that "on" for MMCX is actually "on" - which just happens to be the first non-0 corner. But I presume we will end up having to do the same with &gcc's GDSCs, which are subdomains of CX and MX where this isn't true. > > > > So perhaps instead of introducing the enabled_corner we need to > > introduce your patch and slap a WARN_ON(corner == 0) in > > rpmhpd_power_on() - to ensure that all clients that uses a rpmhpd domain > > actually do vote for a high enough corner? > > So this would mean the expectation is that the clients set the perf state/corner > before they call power_on? I don;t think that's the case today with most clients, > infact its the opposite, we power on first and then make a call to set the perf > state of the domain. > You're right, it's pretty much always the opposite, given that genpd will always enable the domain during attach. Regards, Bjorn > > > > Regards, > > Bjorn > > > > > > > > > > As this means that anyone who needs any of the rpmhpd domains active > > > > also needs to specify required-opps, which wouldn't be needed for any > > > > other power domain provider. > > > > > > > > And more importantly it means that a device sitting in a GDSC, which > > > > would be parented by a rpmhpd domain has no way to specify the GDSC and > > > > trickle the minimum-vote up to the rpmhpd domain. (And I know that we > > > > don't describe the parentship of the GDSCs today, but this patch > > > > tells me that it's around the corner - for more than MMCX) > > > > > > > > Regards, > > > > Bjorn > > > > > > > > > > > > > > > > And if this (which?) clock requires a higher corner than the lowest > > > > > > possible in order to tick at this "lowest" frequency, I'm certainly > > > > > > interested in some more details. > > > > > > > > > > > > -- > > > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > > > of Code Aurora Forum, hosted by The Linux Foundation > > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v4 2/2] arm64: dts: sc7180: Add required-opps for i2c 2021-07-25 17:01 ` Bjorn Andersson @ 2021-07-27 7:35 ` Rajendra Nayak 2021-07-28 3:46 ` Bjorn Andersson 0 siblings, 1 reply; 19+ messages in thread From: Rajendra Nayak @ 2021-07-27 7:35 UTC (permalink / raw) To: Bjorn Andersson Cc: Stephen Boyd, ulf.hansson, viresh.kumar, linux-pm, devicetree, linux-kernel, linux-arm-msm, rojay, stephan On 7/25/2021 10:31 PM, Bjorn Andersson wrote: > On Mon 19 Jul 23:29 CDT 2021, Rajendra Nayak wrote: > >> >> >> On 7/20/2021 12:49 AM, Bjorn Andersson wrote: >>> On Mon 19 Jul 04:37 CDT 2021, Rajendra Nayak wrote: >>> >>>> >>>> >>>> On 7/17/2021 3:29 AM, Bjorn Andersson wrote: >>>>> On Fri 16 Jul 16:49 CDT 2021, Stephen Boyd wrote: >>>>> >>>>>> Quoting Bjorn Andersson (2021-07-16 13:52:12) >>>>>>> On Fri 16 Jul 15:21 CDT 2021, Stephen Boyd wrote: >>>>>>> >>>>>>>> Quoting Bjorn Andersson (2021-07-16 13:18:56) >>>>>>>>> On Fri 16 Jul 05:00 CDT 2021, Rajendra Nayak wrote: >>>>>>>>> >>>>>>>>>> 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. >>>>>>>>>> >>>>>>>>> >>>>>>>>> Sounds good, but... >>>>>>>>> >>>>>>>>>> Use 'required-opps' to pass this information from >>>>>>>>>> device tree, and also add the power-domains property to specify >>>>>>>>>> the CX power-domain. >>>>>>>>>> >>>>>>>>> >>>>>>>>> ..is the required-opps really needed with my rpmhpd patch in place? >>>>>>>>> >>>>>>>> >>>>>>>> Yes? Because rpmhpd_opp_low_svs is not the lowest performance state for >>>>>>>> CX. >>>>>>> >>>>>>> On e.g. sm8250 the first available non-zero corner presented in cmd-db >>>>>>> is low_svs. >>>> >>>> what rail is this? the mmcx? Perhaps it does not support RET. >>>> cx usually supports both collapse state and RET. >>>> >>> >>> That was the one I was specifically looking at for the MDSS_GDSC->MMCX >>> issue, so it's likely I didn't look elsewhere. >>> >>>>>> >>>>>> Indeed. On sc7180 it's not the first non-zero corner. I suppose >>>>>> retention for CX isn't actually used when the SoC is awake so your >>>>>> rpmhpd patch is putting in a vote for something that doesn't do anything >>>>>> at runtime for CX? I imagine that rpmh only sets the aggregate corner to >>>>>> retention when the whole SoC is suspended/sleeping, otherwise things >>>>>> wouldn't go very well. Similarly, min_svs may be VDD minimization? If >>>>>> so, those first two states are basically states that shouldn't be used >>>>>> at runtime, almost like sleep states. >>>>>> >>>>> >>>>> But if that's the case, I don't think it's appropriate for the "enabled >>>>> state" of the domain to use any of those corners. >>>> >>>> I rechecked the downstream kernels where all this voting happens from within >>>> the clock drivers, and I do see votes to min_svs for some clocks, but Stephen is >>>> right that RET is not something that's voted on while in active state. >>>> >>>> But always going with something just above the ret level while active will also >>>> not work for all devices, for instance for i2c on 7180, it needs a cx vote of >>>> low svs while the rail (cx) does support something lower than that which is min svs. >>>> (why can't it just work with min svs?, I don't know, these values and recommendations >>>> come in from the voltage plans published by HW teams for every SoC and we just end up >>>> using them in SW, perhaps something to dig further and understand which I will try and >>>> do but these are the values in voltage plans and downstream kernels which work for now) >>>> >>> >>> So to some degree this invalidates my argumentation about the >>> enabled_corner in rpmhpd, given that "enabled" means a different corner >>> for each rail - not just the one with lowest non-zero value. >> >> Right, it might work in some cases but might not work for all. >> > > Which makes it way less desirable. > > The enable state for rpmhpd power domains doesn't meet my expectations > for how a power domain should behave, Right and that's perhaps because these are not the usual power-domains, which have one "on/active" state and one or more "off/inactive" states (off/ret/clock-stop) Rpmhpd has multiple "on/active" states, and whats "on/active" for one consumer might not be "on/active" for another, so this information is hard to be managed at a generic level and these requests in some way or the other need to come in explicitly from the resp. consumers. > but we should at least be > consistent across all consumers of it then... > > > But the original issue remains, that when a device is powered by > MDSS_GDSC, which is a subdomain of MMCX we still need to ensure that > "on" for MMCX is actually "on" - which just happens to be the first > non-0 corner. > > But I presume we will end up having to do the same with &gcc's GDSCs, > which are subdomains of CX and MX where this isn't true. > >>> >>> So perhaps instead of introducing the enabled_corner we need to >>> introduce your patch and slap a WARN_ON(corner == 0) in >>> rpmhpd_power_on() - to ensure that all clients that uses a rpmhpd domain >>> actually do vote for a high enough corner? >> >> So this would mean the expectation is that the clients set the perf state/corner >> before they call power_on? I don;t think that's the case today with most clients, >> infact its the opposite, we power on first and then make a call to set the perf >> state of the domain. >> > > You're right, it's pretty much always the opposite, given that genpd > will always enable the domain during attach. > > Regards, > Bjorn > >>> >>> Regards, >>> Bjorn >>> >>>>> >>>>> As this means that anyone who needs any of the rpmhpd domains active >>>>> also needs to specify required-opps, which wouldn't be needed for any >>>>> other power domain provider. >>>>> >>>>> And more importantly it means that a device sitting in a GDSC, which >>>>> would be parented by a rpmhpd domain has no way to specify the GDSC and >>>>> trickle the minimum-vote up to the rpmhpd domain. (And I know that we >>>>> don't describe the parentship of the GDSCs today, but this patch >>>>> tells me that it's around the corner - for more than MMCX) >>>>> >>>>> Regards, >>>>> Bjorn >>>>> >>>>>>> >>>>>>> And if this (which?) clock requires a higher corner than the lowest >>>>>>> possible in order to tick at this "lowest" frequency, I'm certainly >>>>>>> interested in some more details. >>>>>>> >>>> >>>> -- >>>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member >>>> of Code Aurora Forum, hosted by The Linux Foundation >> >> -- >> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member >> of Code Aurora Forum, hosted by The Linux Foundation -- 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] 19+ messages in thread
* Re: [PATCH v4 2/2] arm64: dts: sc7180: Add required-opps for i2c 2021-07-27 7:35 ` Rajendra Nayak @ 2021-07-28 3:46 ` Bjorn Andersson 2021-07-28 14:01 ` Dmitry Baryshkov 0 siblings, 1 reply; 19+ messages in thread From: Bjorn Andersson @ 2021-07-28 3:46 UTC (permalink / raw) To: Rajendra Nayak Cc: Stephen Boyd, ulf.hansson, viresh.kumar, linux-pm, devicetree, linux-kernel, linux-arm-msm, rojay, stephan On Tue 27 Jul 02:35 CDT 2021, Rajendra Nayak wrote: > > On 7/25/2021 10:31 PM, Bjorn Andersson wrote: > > On Mon 19 Jul 23:29 CDT 2021, Rajendra Nayak wrote: > > > > > > > > > > > On 7/20/2021 12:49 AM, Bjorn Andersson wrote: > > > > On Mon 19 Jul 04:37 CDT 2021, Rajendra Nayak wrote: > > > > > > > > > > > > > > > > > > > On 7/17/2021 3:29 AM, Bjorn Andersson wrote: > > > > > > On Fri 16 Jul 16:49 CDT 2021, Stephen Boyd wrote: > > > > > > > > > > > > > Quoting Bjorn Andersson (2021-07-16 13:52:12) > > > > > > > > On Fri 16 Jul 15:21 CDT 2021, Stephen Boyd wrote: > > > > > > > > > > > > > > > > > Quoting Bjorn Andersson (2021-07-16 13:18:56) > > > > > > > > > > On Fri 16 Jul 05:00 CDT 2021, Rajendra Nayak wrote: > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Sounds good, but... > > > > > > > > > > > > > > > > > > > > > Use 'required-opps' to pass this information from > > > > > > > > > > > device tree, and also add the power-domains property to specify > > > > > > > > > > > the CX power-domain. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ..is the required-opps really needed with my rpmhpd patch in place? > > > > > > > > > > > > > > > > > > > > > > > > > > > > Yes? Because rpmhpd_opp_low_svs is not the lowest performance state for > > > > > > > > > CX. > > > > > > > > > > > > > > > > On e.g. sm8250 the first available non-zero corner presented in cmd-db > > > > > > > > is low_svs. > > > > > > > > > > what rail is this? the mmcx? Perhaps it does not support RET. > > > > > cx usually supports both collapse state and RET. > > > > > > > > > > > > > That was the one I was specifically looking at for the MDSS_GDSC->MMCX > > > > issue, so it's likely I didn't look elsewhere. > > > > > > > > > > > > > > > > > > Indeed. On sc7180 it's not the first non-zero corner. I suppose > > > > > > > retention for CX isn't actually used when the SoC is awake so your > > > > > > > rpmhpd patch is putting in a vote for something that doesn't do anything > > > > > > > at runtime for CX? I imagine that rpmh only sets the aggregate corner to > > > > > > > retention when the whole SoC is suspended/sleeping, otherwise things > > > > > > > wouldn't go very well. Similarly, min_svs may be VDD minimization? If > > > > > > > so, those first two states are basically states that shouldn't be used > > > > > > > at runtime, almost like sleep states. > > > > > > > > > > > > > > > > > > > But if that's the case, I don't think it's appropriate for the "enabled > > > > > > state" of the domain to use any of those corners. > > > > > > > > > > I rechecked the downstream kernels where all this voting happens from within > > > > > the clock drivers, and I do see votes to min_svs for some clocks, but Stephen is > > > > > right that RET is not something that's voted on while in active state. > > > > > > > > > > But always going with something just above the ret level while active will also > > > > > not work for all devices, for instance for i2c on 7180, it needs a cx vote of > > > > > low svs while the rail (cx) does support something lower than that which is min svs. > > > > > (why can't it just work with min svs?, I don't know, these values and recommendations > > > > > come in from the voltage plans published by HW teams for every SoC and we just end up > > > > > using them in SW, perhaps something to dig further and understand which I will try and > > > > > do but these are the values in voltage plans and downstream kernels which work for now) > > > > > > > > > > > > > So to some degree this invalidates my argumentation about the > > > > enabled_corner in rpmhpd, given that "enabled" means a different corner > > > > for each rail - not just the one with lowest non-zero value. > > > > > > Right, it might work in some cases but might not work for all. > > > > > > > Which makes it way less desirable. > > > > The enable state for rpmhpd power domains doesn't meet my expectations > > for how a power domain should behave, > > Right and that's perhaps because these are not the usual power-domains, > which have one "on/active" state and one or more "off/inactive" states (off/ret/clock-stop) > Rpmhpd has multiple "on/active" states, and whats "on/active" for one consumer > might not be "on/active" for another, so this information is hard to be managed > at a generic level and these requests in some way or the other need to come > in explicitly from the resp. consumers. > I think it's fine if we just acknowledge that this is how the rpmhpd domains works. But I am worried about how we're going to handle the case where the consumer is indirectly referencing one of these power-domains using a subdomain (gdsc). And the open question is if a solution to that problem will solve this problem as well, or if we need to have this and some mechanism to describe the "on state" for the parent of a subdomain. Regards, Bjorn ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v4 2/2] arm64: dts: sc7180: Add required-opps for i2c 2021-07-28 3:46 ` Bjorn Andersson @ 2021-07-28 14:01 ` Dmitry Baryshkov 2021-07-28 18:47 ` Bjorn Andersson 0 siblings, 1 reply; 19+ messages in thread From: Dmitry Baryshkov @ 2021-07-28 14:01 UTC (permalink / raw) To: Bjorn Andersson, Rajendra Nayak Cc: Stephen Boyd, ulf.hansson, viresh.kumar, linux-pm, devicetree, linux-kernel, linux-arm-msm, rojay, stephan On 28/07/2021 06:46, Bjorn Andersson wrote: > On Tue 27 Jul 02:35 CDT 2021, Rajendra Nayak wrote: > >> >> On 7/25/2021 10:31 PM, Bjorn Andersson wrote: >>> On Mon 19 Jul 23:29 CDT 2021, Rajendra Nayak wrote: >>> >>>> >>>> >>>> On 7/20/2021 12:49 AM, Bjorn Andersson wrote: >>>>> On Mon 19 Jul 04:37 CDT 2021, Rajendra Nayak wrote: >>>>> >>>>>> >>>>>> >>>>>> On 7/17/2021 3:29 AM, Bjorn Andersson wrote: >>>>>>> On Fri 16 Jul 16:49 CDT 2021, Stephen Boyd wrote: >>>>>>> >>>>>>>> Quoting Bjorn Andersson (2021-07-16 13:52:12) >>>>>>>>> On Fri 16 Jul 15:21 CDT 2021, Stephen Boyd wrote: >>>>>>>>> >>>>>>>>>> Quoting Bjorn Andersson (2021-07-16 13:18:56) >>>>>>>>>>> On Fri 16 Jul 05:00 CDT 2021, Rajendra Nayak wrote: >>>>>>>>>>> >>>>>>>>>>>> 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. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Sounds good, but... >>>>>>>>>>> >>>>>>>>>>>> Use 'required-opps' to pass this information from >>>>>>>>>>>> device tree, and also add the power-domains property to specify >>>>>>>>>>>> the CX power-domain. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> ..is the required-opps really needed with my rpmhpd patch in place? >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Yes? Because rpmhpd_opp_low_svs is not the lowest performance state for >>>>>>>>>> CX. >>>>>>>>> >>>>>>>>> On e.g. sm8250 the first available non-zero corner presented in cmd-db >>>>>>>>> is low_svs. >>>>>> >>>>>> what rail is this? the mmcx? Perhaps it does not support RET. >>>>>> cx usually supports both collapse state and RET. >>>>>> >>>>> >>>>> That was the one I was specifically looking at for the MDSS_GDSC->MMCX >>>>> issue, so it's likely I didn't look elsewhere. >>>>> >>>>>>>> >>>>>>>> Indeed. On sc7180 it's not the first non-zero corner. I suppose >>>>>>>> retention for CX isn't actually used when the SoC is awake so your >>>>>>>> rpmhpd patch is putting in a vote for something that doesn't do anything >>>>>>>> at runtime for CX? I imagine that rpmh only sets the aggregate corner to >>>>>>>> retention when the whole SoC is suspended/sleeping, otherwise things >>>>>>>> wouldn't go very well. Similarly, min_svs may be VDD minimization? If >>>>>>>> so, those first two states are basically states that shouldn't be used >>>>>>>> at runtime, almost like sleep states. >>>>>>>> >>>>>>> >>>>>>> But if that's the case, I don't think it's appropriate for the "enabled >>>>>>> state" of the domain to use any of those corners. >>>>>> >>>>>> I rechecked the downstream kernels where all this voting happens from within >>>>>> the clock drivers, and I do see votes to min_svs for some clocks, but Stephen is >>>>>> right that RET is not something that's voted on while in active state. >>>>>> >>>>>> But always going with something just above the ret level while active will also >>>>>> not work for all devices, for instance for i2c on 7180, it needs a cx vote of >>>>>> low svs while the rail (cx) does support something lower than that which is min svs. >>>>>> (why can't it just work with min svs?, I don't know, these values and recommendations >>>>>> come in from the voltage plans published by HW teams for every SoC and we just end up >>>>>> using them in SW, perhaps something to dig further and understand which I will try and >>>>>> do but these are the values in voltage plans and downstream kernels which work for now) >>>>>> >>>>> >>>>> So to some degree this invalidates my argumentation about the >>>>> enabled_corner in rpmhpd, given that "enabled" means a different corner >>>>> for each rail - not just the one with lowest non-zero value. >>>> >>>> Right, it might work in some cases but might not work for all. >>>> >>> >>> Which makes it way less desirable. >>> >>> The enable state for rpmhpd power domains doesn't meet my expectations >>> for how a power domain should behave, >> >> Right and that's perhaps because these are not the usual power-domains, >> which have one "on/active" state and one or more "off/inactive" states (off/ret/clock-stop) >> Rpmhpd has multiple "on/active" states, and whats "on/active" for one consumer >> might not be "on/active" for another, so this information is hard to be managed >> at a generic level and these requests in some way or the other need to come >> in explicitly from the resp. consumers. >> > > I think it's fine if we just acknowledge that this is how the rpmhpd > domains works. > > But I am worried about how we're going to handle the case where the > consumer is indirectly referencing one of these power-domains using a > subdomain (gdsc). With the proper subdomain relationship in place and with Ulf's patches, this seems to be handled correctly. gdsc sets proper level for the parent power domain, which gets voted and unvoted by the core pm code when gdsc's power domain is powered on or off. > And the open question is if a solution to that problem will solve this > problem as well, or if we need to have this and some mechanism to > describe the "on state" for the parent of a subdomain. -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v4 2/2] arm64: dts: sc7180: Add required-opps for i2c 2021-07-28 14:01 ` Dmitry Baryshkov @ 2021-07-28 18:47 ` Bjorn Andersson 0 siblings, 0 replies; 19+ messages in thread From: Bjorn Andersson @ 2021-07-28 18:47 UTC (permalink / raw) To: Dmitry Baryshkov Cc: Rajendra Nayak, Stephen Boyd, ulf.hansson, viresh.kumar, linux-pm, devicetree, linux-kernel, linux-arm-msm, rojay, stephan On Wed 28 Jul 07:01 PDT 2021, Dmitry Baryshkov wrote: > On 28/07/2021 06:46, Bjorn Andersson wrote: > > On Tue 27 Jul 02:35 CDT 2021, Rajendra Nayak wrote: > > > > > > > > On 7/25/2021 10:31 PM, Bjorn Andersson wrote: > > > > On Mon 19 Jul 23:29 CDT 2021, Rajendra Nayak wrote: > > > > > > > > > > > > > > > > > > > On 7/20/2021 12:49 AM, Bjorn Andersson wrote: > > > > > > On Mon 19 Jul 04:37 CDT 2021, Rajendra Nayak wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > On 7/17/2021 3:29 AM, Bjorn Andersson wrote: > > > > > > > > On Fri 16 Jul 16:49 CDT 2021, Stephen Boyd wrote: > > > > > > > > > > > > > > > > > Quoting Bjorn Andersson (2021-07-16 13:52:12) > > > > > > > > > > On Fri 16 Jul 15:21 CDT 2021, Stephen Boyd wrote: > > > > > > > > > > > > > > > > > > > > > Quoting Bjorn Andersson (2021-07-16 13:18:56) > > > > > > > > > > > > On Fri 16 Jul 05:00 CDT 2021, Rajendra Nayak wrote: > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Sounds good, but... > > > > > > > > > > > > > > > > > > > > > > > > > Use 'required-opps' to pass this information from > > > > > > > > > > > > > device tree, and also add the power-domains property to specify > > > > > > > > > > > > > the CX power-domain. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ..is the required-opps really needed with my rpmhpd patch in place? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Yes? Because rpmhpd_opp_low_svs is not the lowest performance state for > > > > > > > > > > > CX. > > > > > > > > > > > > > > > > > > > > On e.g. sm8250 the first available non-zero corner presented in cmd-db > > > > > > > > > > is low_svs. > > > > > > > > > > > > > > what rail is this? the mmcx? Perhaps it does not support RET. > > > > > > > cx usually supports both collapse state and RET. > > > > > > > > > > > > > > > > > > > That was the one I was specifically looking at for the MDSS_GDSC->MMCX > > > > > > issue, so it's likely I didn't look elsewhere. > > > > > > > > > > > > > > > > > > > > > > > > Indeed. On sc7180 it's not the first non-zero corner. I suppose > > > > > > > > > retention for CX isn't actually used when the SoC is awake so your > > > > > > > > > rpmhpd patch is putting in a vote for something that doesn't do anything > > > > > > > > > at runtime for CX? I imagine that rpmh only sets the aggregate corner to > > > > > > > > > retention when the whole SoC is suspended/sleeping, otherwise things > > > > > > > > > wouldn't go very well. Similarly, min_svs may be VDD minimization? If > > > > > > > > > so, those first two states are basically states that shouldn't be used > > > > > > > > > at runtime, almost like sleep states. > > > > > > > > > > > > > > > > > > > > > > > > > But if that's the case, I don't think it's appropriate for the "enabled > > > > > > > > state" of the domain to use any of those corners. > > > > > > > > > > > > > > I rechecked the downstream kernels where all this voting happens from within > > > > > > > the clock drivers, and I do see votes to min_svs for some clocks, but Stephen is > > > > > > > right that RET is not something that's voted on while in active state. > > > > > > > > > > > > > > But always going with something just above the ret level while active will also > > > > > > > not work for all devices, for instance for i2c on 7180, it needs a cx vote of > > > > > > > low svs while the rail (cx) does support something lower than that which is min svs. > > > > > > > (why can't it just work with min svs?, I don't know, these values and recommendations > > > > > > > come in from the voltage plans published by HW teams for every SoC and we just end up > > > > > > > using them in SW, perhaps something to dig further and understand which I will try and > > > > > > > do but these are the values in voltage plans and downstream kernels which work for now) > > > > > > > > > > > > > > > > > > > So to some degree this invalidates my argumentation about the > > > > > > enabled_corner in rpmhpd, given that "enabled" means a different corner > > > > > > for each rail - not just the one with lowest non-zero value. > > > > > > > > > > Right, it might work in some cases but might not work for all. > > > > > > > > > > > > > Which makes it way less desirable. > > > > > > > > The enable state for rpmhpd power domains doesn't meet my expectations > > > > for how a power domain should behave, > > > > > > Right and that's perhaps because these are not the usual power-domains, > > > which have one "on/active" state and one or more "off/inactive" states (off/ret/clock-stop) > > > Rpmhpd has multiple "on/active" states, and whats "on/active" for one consumer > > > might not be "on/active" for another, so this information is hard to be managed > > > at a generic level and these requests in some way or the other need to come > > > in explicitly from the resp. consumers. > > > > > > > I think it's fine if we just acknowledge that this is how the rpmhpd > > domains works. > > > > But I am worried about how we're going to handle the case where the > > consumer is indirectly referencing one of these power-domains using a > > subdomain (gdsc). > > With the proper subdomain relationship in place and with Ulf's patches, this > seems to be handled correctly. gdsc sets proper level for the parent power > domain, which gets voted and unvoted by the core pm code when gdsc's power > domain is powered on or off. > Right, but this works only in our case because "on" for MMCX happens to be the first non-zero corner. What this patch points out is that for some of the other power domains my patch in the rpmhpd driver isn't sufficient - and presumably wouldn't work for other gdsc's (that are parented by CX or MX). Regards, Bjorn > > And the open question is if a solution to that problem will solve this > > problem as well, or if we need to have this and some mechanism to > > describe the "on state" for the parent of a subdomain. > > > -- > With best wishes > Dmitry ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2021-07-28 18:49 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-16 10:00 [PATCH v4 0/2] PM / Domains: Add support for 'required-opps' to set default perf state Rajendra Nayak 2021-07-16 10:00 ` [PATCH v4 1/2] " Rajendra Nayak 2021-07-16 20:19 ` Stephen Boyd 2021-07-19 11:50 ` Rajendra Nayak 2021-07-16 10:00 ` [PATCH v4 2/2] arm64: dts: sc7180: Add required-opps for i2c Rajendra Nayak 2021-07-16 19:39 ` Stephen Boyd 2021-07-16 20:18 ` Bjorn Andersson 2021-07-16 20:21 ` Stephen Boyd 2021-07-16 20:52 ` Bjorn Andersson 2021-07-16 21:49 ` Stephen Boyd 2021-07-16 21:59 ` Bjorn Andersson 2021-07-19 9:37 ` Rajendra Nayak 2021-07-19 19:19 ` Bjorn Andersson 2021-07-20 4:29 ` Rajendra Nayak 2021-07-25 17:01 ` Bjorn Andersson 2021-07-27 7:35 ` Rajendra Nayak 2021-07-28 3:46 ` Bjorn Andersson 2021-07-28 14:01 ` Dmitry Baryshkov 2021-07-28 18:47 ` Bjorn Andersson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).