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