[PATCH 0/2] QCM2290 icc fixes

[PATCH 0/2] QCM2290 icc fixes

[Konrad Dybcio]

This series contains fixes necessary for icc to behave correctly on
QCM2290.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Konrad Dybcio (2):
      interconnect: qcom: qcm2290: Enable keep_alive on all buses
      interconnect: qcom: qcm2290: Enable sync state

 drivers/interconnect/qcom/qcm2290.c | 7 +++++++
 1 file changed, 7 insertions(+)
---
base-commit: c58c49dd89324b18a812762a2bfa5a0458e4f252
change-id: 20230720-topic-qcm2290_icc-211dc925c2e8

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@linaro.org>

[PATCH 1/2] interconnect: qcom: qcm2290: Enable keep_alive on all buses

[Konrad Dybcio]

QCM2290 expects all buses to be up at all times when the CPU is
active. Enable keep_alive on all of them to achieve that.

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

diff --git a/drivers/interconnect/qcom/qcm2290.c b/drivers/interconnect/qcom/qcm2290.c
index 3c3b24264a5b..c22354f3e667 100644
--- a/drivers/interconnect/qcom/qcm2290.c
+++ b/drivers/interconnect/qcom/qcm2290.c
@@ -1196,6 +1196,7 @@ static const struct qcom_icc_desc qcm2290_bimc = {
 	.num_nodes = ARRAY_SIZE(qcm2290_bimc_nodes),
 	.bus_clk_desc = &bimc_clk,
 	.regmap_cfg = &qcm2290_bimc_regmap_config,
+	.keep_alive = true,
 	/* M_REG_BASE() in vendor msm_bus_bimc_adhoc driver */
 	.qos_offset = 0x8000,
 };
@@ -1252,6 +1253,7 @@ static const struct qcom_icc_desc qcm2290_cnoc = {
 	.num_nodes = ARRAY_SIZE(qcm2290_cnoc_nodes),
 	.bus_clk_desc = &bus_1_clk,
 	.regmap_cfg = &qcm2290_cnoc_regmap_config,
+	.keep_alive = true,
 };
 
 static struct qcom_icc_node * const qcm2290_snoc_nodes[] = {
@@ -1294,6 +1296,7 @@ static const struct qcom_icc_desc qcm2290_snoc = {
 	.num_nodes = ARRAY_SIZE(qcm2290_snoc_nodes),
 	.bus_clk_desc = &bus_2_clk,
 	.regmap_cfg = &qcm2290_snoc_regmap_config,
+	.keep_alive = true,
 	/* Vendor DT node fab-sys_noc property 'qcom,base-offset' */
 	.qos_offset = 0x15000,
 };
@@ -1308,6 +1311,7 @@ static const struct qcom_icc_desc qcm2290_qup_virt = {
 	.nodes = qcm2290_qup_virt_nodes,
 	.num_nodes = ARRAY_SIZE(qcm2290_qup_virt_nodes),
 	.bus_clk_desc = &qup_clk,
+	.keep_alive = true,
 };
 
 static struct qcom_icc_node * const qcm2290_mmnrt_virt_nodes[] = {
@@ -1323,6 +1327,7 @@ static const struct qcom_icc_desc qcm2290_mmnrt_virt = {
 	.num_nodes = ARRAY_SIZE(qcm2290_mmnrt_virt_nodes),
 	.bus_clk_desc = &mmaxi_0_clk,
 	.regmap_cfg = &qcm2290_snoc_regmap_config,
+	.keep_alive = true,
 	.qos_offset = 0x15000,
 };
 
@@ -1338,6 +1343,7 @@ static const struct qcom_icc_desc qcm2290_mmrt_virt = {
 	.num_nodes = ARRAY_SIZE(qcm2290_mmrt_virt_nodes),
 	.bus_clk_desc = &mmaxi_1_clk,
 	.regmap_cfg = &qcm2290_snoc_regmap_config,
+	.keep_alive = true,
 	.qos_offset = 0x15000,
 };
 

-- 
2.41.0

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

[Konrad Dybcio]

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(+)

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);

-- 
2.41.0

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

[Stephan Gerhold]

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

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

[Konrad Dybcio]

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

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

[Stephan Gerhold]

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

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

[Konrad Dybcio]

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