drivers/opp/core.c | 28 ++++++++++++++++++++++++++++ include/linux/pm_opp.h | 6 ++++++ 2 files changed, 34 insertions(+)
Currently it's not possible to set just the bandwidth if the OPP
describes other properties (required-opps, opp-hz etc.).
Introduce dev_pm_opp_set_bw() to solve this problem.
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
drivers/opp/core.c | 28 ++++++++++++++++++++++++++++
include/linux/pm_opp.h | 6 ++++++
2 files changed, 34 insertions(+)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 85cbc8de407c..00d7f7c20189 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1278,6 +1278,34 @@ int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp)
}
EXPORT_SYMBOL_GPL(dev_pm_opp_set_opp);
+/**
+ * dev_pm_opp_set_bw() - Configure device's interconnect bandwidth from OPP
+ * @dev: device for which we do this operation
+ * @opp: OPP to get the bandwidth data from
+ *
+ * This configures the interconnect bandwidth based on the properties of the
+ * OPP passed to this routine.
+ *
+ * Return: 0 on success, a negative error number otherwise.
+ */
+int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp)
+{
+ struct opp_table *opp_table;
+ int ret;
+
+ opp_table = _find_opp_table(dev);
+ if (IS_ERR(opp_table)) {
+ dev_err(dev, "%s: device opp doesn't exist\n", __func__);
+ return PTR_ERR(opp_table);
+ }
+
+ ret = _set_opp_bw(opp_table, opp, dev);
+ dev_pm_opp_put_opp_table(opp_table);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_set_bw);
+
/* OPP-dev Helpers */
static void _remove_opp_dev(struct opp_device *opp_dev,
struct opp_table *opp_table)
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index dc1fb5890792..5e074ac7a68e 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -167,6 +167,7 @@ struct dev_pm_opp *dev_pm_opp_xlate_required_opp(struct opp_table *src_table, st
int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct opp_table *dst_table, unsigned int pstate);
int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp);
+int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp);
int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
void dev_pm_opp_remove_table(struct device *dev);
@@ -373,6 +374,11 @@ static inline int dev_pm_opp_set_opp(struct device *dev, struct dev_pm_opp *opp)
return -EOPNOTSUPP;
}
+static inline int dev_pm_opp_set_bw(struct device *dev, struct dev_pm_opp *opp)
+{
+ return -EOPNOTSUPP;
+}
+
static inline int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask)
{
return -EOPNOTSUPP;
---
base-commit: 6a3d37b4d885129561e1cef361216f00472f7d2e
change-id: 20230526-topic-opp_bw-8d04a85a93a8
Best regards,
--
Konrad Dybcio <konrad.dybcio@linaro.org>
Hi Konrad, On 26.05.23 12:17, Konrad Dybcio wrote: > Currently it's not possible to set just the bandwidth if the OPP > describes other properties (required-opps, opp-hz etc.). > > Introduce dev_pm_opp_set_bw() to solve this problem. > Who is going to use this? BR, Georgi
On 29.05.2023 12:38, Georgi Djakov wrote: > Hi Konrad, > > On 26.05.23 12:17, Konrad Dybcio wrote: >> Currently it's not possible to set just the bandwidth if the OPP >> describes other properties (required-opps, opp-hz etc.). >> >> Introduce dev_pm_opp_set_bw() to solve this problem. >> > > Who is going to use this? I wanted to use it with Adreno for fine-grained control during system suspend/resume. It turned out that I was essentially then trying to power on the hardware without its power supply. https://lore.kernel.org/linux-kernel/8b7f4b22-76e2-e812-92ed-35e5ecdd6309@kernel.org/T/#t Konrad > > BR, > Georgi >
On 26-05-23, 11:17, Konrad Dybcio wrote: > Currently it's not possible to set just the bandwidth if the OPP > describes other properties (required-opps, opp-hz etc.). > > Introduce dev_pm_opp_set_bw() to solve this problem. The OPP core (intentionally) doesn't provide any such interface for freq, volt, bw, level, etc. Setting just one of the properties for a device is potentially harmful and the OPP must be set as a whole and so this isn't allowed. There is dev_pm_opp_set_freq(), but it uses freq to just find the OPP and sets entire OPP only. Why should be break this for bw ? -- viresh
On 26.05.2023 12:53, Viresh Kumar wrote: > On 26-05-23, 11:17, Konrad Dybcio wrote: >> Currently it's not possible to set just the bandwidth if the OPP >> describes other properties (required-opps, opp-hz etc.). >> >> Introduce dev_pm_opp_set_bw() to solve this problem. > > The OPP core (intentionally) doesn't provide any such interface for > freq, volt, bw, level, etc. > > Setting just one of the properties for a device is potentially harmful and the > OPP must be set as a whole and so this isn't allowed. There is > dev_pm_opp_set_freq(), but it uses freq to just find the OPP and sets entire OPP > only. > > Why should be break this for bw ? > There are some users which have tight power sequencing requirements, like the Qualcomm Adreno GPU. Dropping the entire OPP kills clocks, bw and required-opps at once, but on certain Adrenos we need something like: disable memory clock (clk) disable all other clocks, including the opp-managed core clock (clk_bulk) kill one, fully manually controlled genpd (manual runtime pm) remove bus vote (func proposed in this patch) kill another genpd (manual runtime pm) kill the opp-managed genpd (automatic pm calls) Changing the order kills the chip until you reboot the whole board and setting freq=0 using dev_pm_opp_set_rate doesn't drop the bw vote. Konrad
On 26-05-23, 12:59, Konrad Dybcio wrote: > There are some users which have tight power sequencing requirements, > like the Qualcomm Adreno GPU. > > Dropping the entire OPP kills clocks, bw and required-opps at once, > but on certain Adrenos we need something like: > > > disable memory clock (clk) > disable all other clocks, including the opp-managed core clock (clk_bulk) > kill one, fully manually controlled genpd (manual runtime pm) > remove bus vote (func proposed in this patch) > kill another genpd (manual runtime pm) > kill the opp-managed genpd (automatic pm calls) > > Changing the order kills the chip until you reboot the whole board and > setting freq=0 using dev_pm_opp_set_rate doesn't drop the bw vote. I am a bit confused now. What's the exact problem with dev_pm_opp_set_rate(dev, 0) ? It does set the bandwidth too, from what I can see. -- viresh
On 29.05.2023 06:58, Viresh Kumar wrote: > On 26-05-23, 12:59, Konrad Dybcio wrote: >> There are some users which have tight power sequencing requirements, >> like the Qualcomm Adreno GPU. >> >> Dropping the entire OPP kills clocks, bw and required-opps at once, >> but on certain Adrenos we need something like: >> >> >> disable memory clock (clk) >> disable all other clocks, including the opp-managed core clock (clk_bulk) >> kill one, fully manually controlled genpd (manual runtime pm) >> remove bus vote (func proposed in this patch) >> kill another genpd (manual runtime pm) >> kill the opp-managed genpd (automatic pm calls) >> >> Changing the order kills the chip until you reboot the whole board and >> setting freq=0 using dev_pm_opp_set_rate doesn't drop the bw vote. > > I am a bit confused now. > > What's the exact problem with dev_pm_opp_set_rate(dev, 0) ? It does set the > bandwidth too, from what I can see. I think I didn't state my intentions correctly.. The proposed function would set *just* the bandwidth through OPP, so it'd be essentially equal to loop over num_paths { icc_get(...) icc_set(...) icc_put(...) } but since OPP already picked up these interconnect paths, it makes little sense to mess with them through raw APIs. Konrad >
On 29-05-23, 11:35, Konrad Dybcio wrote: > I think I didn't state my intentions correctly.. > > The proposed function would set *just* the bandwidth through OPP, > so it'd be essentially equal to > > loop over num_paths { > icc_get(...) > > icc_set(...) > > icc_put(...) > } Right, but why do you need to do it ? Why isn't this done as part of dev_pm_opp_set_{freq|opp}() ? > but since OPP already picked up these interconnect paths, it makes > little sense to mess with them through raw APIs. I am not sure if I would want to expose an API to just configure part of the entire OPP switching mechanism. I would rather let you do that via ICC APIs, instead of going through the OPP core. Since there is a possibility of accidental misuse of the same here. -- viresh
On 29.05.2023 11:40, Viresh Kumar wrote: > On 29-05-23, 11:35, Konrad Dybcio wrote: >> I think I didn't state my intentions correctly.. >> >> The proposed function would set *just* the bandwidth through OPP, >> so it'd be essentially equal to >> >> loop over num_paths { >> icc_get(...) >> >> icc_set(...) >> >> icc_put(...) >> } > > Right, but why do you need to do it ? Why isn't this done as part of > dev_pm_opp_set_{freq|opp}() ? > >> but since OPP already picked up these interconnect paths, it makes >> little sense to mess with them through raw APIs. > > I am not sure if I would want to expose an API to just configure part of the > entire OPP switching mechanism. I would rather let you do that via ICC APIs, > instead of going through the OPP core. Since there is a possibility of > accidental misuse of the same here. I did some more testing and I think that I was just trying to abuse the APIs.. Adding power-domains + pm calls + required-opps to the genpd provider made the set_opp func work fine on suspend.. Consider this solved! Konrad >
On 29-05-23, 13:38, Konrad Dybcio wrote: > I did some more testing and I think that I was just trying to abuse > the APIs.. Adding power-domains + pm calls + required-opps to the genpd > provider made the set_opp func work fine on suspend.. Consider this > solved! Nice! -- viresh
© 2016 - 2023 Red Hat, Inc.