| Message ID | 20230720-topic-qcm2290_icc-v1-2-7f67f2e259c1@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | QCM2290 icc fixes | expand |
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
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
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
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 --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);
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(+)