On 19/01/2023 22:18, Vivek Aknurwar wrote:
Hi Bryan,
Thanks for taking time to review the patch.
On 1/13/2023 5:40 PM, Bryan O'Donoghue wrote:
On 14/01/2023 01:24, Bryan O'Donoghue wrote:
On 13/01/2023 22:07, Vivek Aknurwar wrote:
Currently framework sets bw even when init bw requirements are zero
during
provider registration, thus resulting bulk of set bw to hw.
Avoid this behaviour by skipping provider set bw calls if init bw is
zero.
Signed-off-by: Vivek Aknurwar <quic_viveka@xxxxxxxxxxx>
---
drivers/interconnect/core.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 25debde..43ed595 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -977,14 +977,17 @@ void icc_node_add(struct icc_node *node,
struct icc_provider *provider)
node->avg_bw = node->init_avg;
node->peak_bw = node->init_peak;
- if (provider->pre_aggregate)
- provider->pre_aggregate(node);
-
- if (provider->aggregate)
- provider->aggregate(node, 0, node->init_avg, node->init_peak,
- &node->avg_bw, &node->peak_bw);
+ if (node->avg_bw || node->peak_bw) {
+ if (provider->pre_aggregate)
+ provider->pre_aggregate(node);
+
+ if (provider->aggregate)
+ provider->aggregate(node, 0, node->init_avg,
node->init_peak,
+ &node->avg_bw, &node->peak_bw);
+ if (provider->set)
+ provider->set(node, node);
+ }
- provider->set(node, node);
node->avg_bw = 0;
node->peak_bw = 0;
I have the same comment/question for this patch that I had for the
qcom arch specific version of it. This patch seems to be doing at a
higher level what the patch below was doing at a lower level.
https://lore.kernel.org/lkml/1039a507-c4cd-e92f-dc29-1e2169ce5078@xxxxxxxxxx/T/#m0c90588d0d1e2ab88c39be8f5f3a8f0b61396349
what happens to earlier silicon - qcom silicon which previously made
explicit zero requests ?
This patch is to optimize and avoid all those bw 0 requests on each node
addition during probe (which results in rpmh remote calls) for upcoming
targets.
So why not change it just for rpmh ?
You are changing it for rpm here, as well as for Samsung and NXP
interconnects.
Taking rpm as an example, for certain generations of silicon we make an
explicit zero call.
https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.BR.1.2.9-00810-8x09.0/drivers/platform/msm/msm_bus/msm_bus_bimc.c#L1367
Here's the original RPM commit that sets a zero
https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/commit/d91d108656a7a44a6dfcfb318a25d39c5418e54b
https://lore.kernel.org/lkml/1039a507-c4cd-e92f-dc29-1e2169ce5078@xxxxxxxxxx/T/#m589e8280de470e038249bb362634221771d845dd
https://lkml.org/lkml/2023/1/3/1232
Isn't it a better idea to let lower layer drivers differentiate what
they do ?
AFAIU lower layer driver can/should not differentiate between normal
flow calls vs made as a result from probe/initialization of driver.
Hence even bw 0 request is honored as like client in general wish to
vote 0 as in an normal use case.
But surely if I vote zero, then I mean to vote zero ?
Do we know that for every architecture and for every different supported
that ignoring a zero vote is the right thing to do ?
I don't think we do know that.
https://lore.kernel.org/linux-arm-msm/20230116132152.405535-1-konrad.dybcio@xxxxxxxxxx/
I think for older rpm this is a departure from long existing logic.
Maybe its entirely benign but, IMO you should be proposing this change
at the rpmh level only, not at the top level across multiple different
interconnect arches.
---
bod
