diff mbox series

[2/2] interconnect: qcom: qcm2290: Enable sync state

Message ID 20230720-topic-qcm2290_icc-v1-2-7f67f2e259c1@linaro.org
State New
Headers
Series QCM2290 icc fixes |

Commit Message

Konrad Dybcio July 20, 2023, 6:24 p.m. UTC 
  Very surprisingly, qcm2290 does not seem to require any interface
clocks. It's therefore safe to enable sync_state to park unused devices.
Do so.

Fixes: 1a14b1ac3935 ("interconnect: qcom: Add QCM2290 driver support")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/qcm2290.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Stephan Gerhold July 20, 2023, 7:52 p.m. UTC | #1 
On Thu, Jul 20, 2023 at 08:24:01PM +0200, Konrad Dybcio wrote:
> Very surprisingly, qcm2290 does not seem to require any interface
> clocks.

What does this mean exactly? The interconnect .sync_state() is
responsible to drop the initial maximum bandwidth votes, with the
assumption that all active devices have voted for the bandwidth they
need. How does this relate to "requiring interface clocks"?

> It's therefore safe to enable sync_state to park unused devices.
> Do so.

Doesn't this make everything painfully slow? There are no interconnect
consumers at all in qcm2290.dtsi. I would expect that all bandwidths
end up at minimum.

Thanks,
Stephan
Konrad Dybcio July 21, 2023, 12:03 p.m. UTC | #2 
On 20.07.2023 21:52, Stephan Gerhold wrote:
> On Thu, Jul 20, 2023 at 08:24:01PM +0200, Konrad Dybcio wrote:
>> Very surprisingly, qcm2290 does not seem to require any interface
>> clocks.
> 
> What does this mean exactly? The interconnect .sync_state() is
> responsible to drop the initial maximum bandwidth votes, with the
> assumption that all active devices have voted for the bandwidth they
> need. How does this relate to "requiring interface clocks"?
If it required such clocks to be present, sync_state could not
complete, as trying to access some nodes would crash the platform
due to unclocked access.

> 
>> It's therefore safe to enable sync_state to park unused devices.
>> Do so.
> 
> Doesn't this make everything painfully slow? There are no interconnect
> consumers at all in qcm2290.dtsi. I would expect that all bandwidths
> end up at minimum.
There are no interconnect providers defined in qcm2290.dtsi.

Konrad
Stephan Gerhold July 23, 2023, 11:01 a.m. UTC | #3 
On Fri, Jul 21, 2023 at 02:03:16PM +0200, Konrad Dybcio wrote:
> On 20.07.2023 21:52, Stephan Gerhold wrote:
> > On Thu, Jul 20, 2023 at 08:24:01PM +0200, Konrad Dybcio wrote:
> >> Very surprisingly, qcm2290 does not seem to require any interface
> >> clocks.
> > 
> > What does this mean exactly? The interconnect .sync_state() is
> > responsible to drop the initial maximum bandwidth votes, with the
> > assumption that all active devices have voted for the bandwidth they
> > need. How does this relate to "requiring interface clocks"?
> If it required such clocks to be present, sync_state could not
> complete, as trying to access some nodes would crash the platform
> due to unclocked access.

You mean something like the IPA clock that must be active to do the QoS
writes?

Wouldn't it already crash before .sync_state() then, when the initial
max bandwidth votes are being made?

> 
> > 
> >> It's therefore safe to enable sync_state to park unused devices.
> >> Do so.
> > 
> > Doesn't this make everything painfully slow? There are no interconnect
> > consumers at all in qcm2290.dtsi. I would expect that all bandwidths
> > end up at minimum.
> There are no interconnect providers defined in qcm2290.dtsi.

Ack, so I guess you're going to add them together with the actual
consumers?

I think the patch itself is fine. Only the commit message is a bit
misleading. The actual change that is being done here is enabling the
bandwidth scaling (dropping the max bandwidth votes after
.sync_state()). Can you try to clarify the commit message a bit?

Thanks,
Stephan
Konrad Dybcio July 24, 2023, 10:31 a.m. UTC | #4 
On 23.07.2023 13:01, Stephan Gerhold wrote:
> On Fri, Jul 21, 2023 at 02:03:16PM +0200, Konrad Dybcio wrote:
>> On 20.07.2023 21:52, Stephan Gerhold wrote:
>>> On Thu, Jul 20, 2023 at 08:24:01PM +0200, Konrad Dybcio wrote:
>>>> Very surprisingly, qcm2290 does not seem to require any interface
>>>> clocks.
>>>
>>> What does this mean exactly? The interconnect .sync_state() is
>>> responsible to drop the initial maximum bandwidth votes, with the
>>> assumption that all active devices have voted for the bandwidth they
>>> need. How does this relate to "requiring interface clocks"?
>> If it required such clocks to be present, sync_state could not
>> complete, as trying to access some nodes would crash the platform
>> due to unclocked access.
> 
> You mean something like the IPA clock that must be active to do the QoS
> writes?
> 
> Wouldn't it already crash before .sync_state() then, when the initial
> max bandwidth votes are being made?
No, the bandwidth votes are fully handled by RPM with no external deps.

Only accessing the QoS registers from the AP seems to trigger crashes
on unclocked access. But surprisingly, not on this SoC.

>>>> It's therefore safe to enable sync_state to park unused devices.
>>>> Do so.
>>>
>>> Doesn't this make everything painfully slow? There are no interconnect
>>> consumers at all in qcm2290.dtsi. I would expect that all bandwidths
>>> end up at minimum.
>> There are no interconnect providers defined in qcm2290.dtsi.
> 
> Ack, so I guess you're going to add them together with the actual
> consumers?
Correct.

> 
> I think the patch itself is fine. Only the commit message is a bit
> misleading. The actual change that is being done here is enabling the
> bandwidth scaling (dropping the max bandwidth votes after
> .sync_state()). Can you try to clarify the commit message a bit?
Yes, I'll resend.

Konrad
diff mbox series

Patch

diff --git a/drivers/interconnect/qcom/qcm2290.c b/drivers/interconnect/qcom/qcm2290.c
index c22354f3e667..5bc4b7516608 100644
--- a/drivers/interconnect/qcom/qcm2290.c
+++ b/drivers/interconnect/qcom/qcm2290.c
@@ -1364,6 +1364,7 @@  static struct platform_driver qcm2290_noc_driver = {
 	.driver = {
 		.name = "qnoc-qcm2290",
 		.of_match_table = qcm2290_noc_of_match,
+		.sync_state = icc_sync_state,
 	},
 };
 module_platform_driver(qcm2290_noc_driver);