[PATCH 0/2] interconnect: qcom: Remove IP0 resource

[PATCH 0/2] interconnect: qcom: Remove IP0 resource

[Stephen Boyd]

These two patches remove the IP0 interconnect used for IPA because
they're also present in the clk-rpmh driver. I see there are some more
IP0 usages in the interconnect drivers, but I don't see a corresponding
IPA clk in clk-rpmh, so I left these out. We can remove all of them if
desired, but the sc7180 patch is most important to me as it fixes
boot on my trogdor lazor device.
 
Stephen Boyd (2):
  interconnect: qcom: sc7180: Drop IP0 interconnects
  interconnect: qcom: sdx55: Drop IP0 interconnects

 drivers/interconnect/qcom/sc7180.c | 21 ---------------------
 drivers/interconnect/qcom/sdx55.c  | 21 ---------------------
 2 files changed, 42 deletions(-)

Cc: Alex Elder <elder@linaro.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Taniya Das <quic_tdas@quicinc.com>
Cc: Mike Tipton <quic_mdtipton@quicinc.com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

base-commit: 3123109284176b1532874591f7c81f3837bbdc17
-- 
https://chromeos.dev

[PATCH 1/2] interconnect: qcom: sc7180: Drop IP0 interconnects

[Stephen Boyd]

The IPA BCM resource ("IP0") on sc7180 was moved to the clk-rpmh driver
in commit bcd63d222b60 ("clk: qcom: rpmh: Add IPA clock for SC7180") and
modeled as a clk, but this interconnect driver still had it modeled as
an interconnect. This was mostly OK because nobody used the interconnect
definition, until the interconnect framework started dropping bandwidth
requests on interconnects that aren't used via the sync_state callback
in commit 7d3b0b0d8184 ("interconnect: qcom: Use icc_sync_state"). Once
that patch was applied the IP0 resource was going to be controlled from
two places, the clk framework and the interconnect framework.

Even then, things were probably going to be OK, because commit
b95b668eaaa2 ("interconnect: qcom: icc-rpmh: Add BCMs to commit list in
pre_aggregate") was needed to actually drop bandwidth requests on unused
interconnects, of which the IPA was one of the interconnect that wasn't
getting dropped to zero. Combining the three commits together leads to
bad behavior where the interconnect framework is disabling the IP0
resource because it has no users while the clk framework thinks the IP0
resource is on because the only user, the IPA driver, has turned it on
via clk_prepare_enable(). Depending on when sync_state is called, we can
get into a situation like below:

  IPA driver probes
  IPA driver gets notified modem started
   runtime PM get()
    IPA clk enabled -> IP0 resource is ON
  sync_state runs
   interconnect zeroes out the IP0 resource -> IP0 resource is off
  IPA driver tries to access a register and blows up

The crash is an unclocked access that manifest as an SError.

 SError Interrupt on CPU0, code 0xbe000011 -- SError
 CPU: 0 PID: 3595 Comm: mmdata_mgr Not tainted 5.17.1+ #166
 Hardware name: Google Lazor (rev1 - 2) with LTE (DT)
 pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
 pc : mutex_lock+0x4c/0x80
 lr : mutex_lock+0x30/0x80
 sp : ffffffc00da9b9c0
 x29: ffffffc00da9b9c0 x28: 0000000000000000 x27: 0000000000000000
 x26: ffffffc00da9bc90 x25: ffffff80c2024010 x24: ffffff80c2024000
 x23: ffffff8083100000 x22: ffffff80831000d0 x21: ffffff80831000a8
 x20: ffffff80831000a8 x19: ffffff8083100070 x18: 00000000ffff0a00
 x17: 000000002f7254f1 x16: 0000000000000100 x15: 0000000000000000
 x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
 x11: 000000000001f0b8 x10: ffffffc00931f0b8 x9 : 0000000000000000
 x8 : 0000000000000000 x7 : fefefefefeff2f60 x6 : 0000808080808080
 x5 : 0000000000000000 x4 : 8080808080800000 x3 : ffffff80d2d4ee28
 x2 : ffffff808c1d6e40 x1 : 0000000000000000 x0 : ffffff8083100070
 Kernel panic - not syncing: Asynchronous SError Interrupt
 CPU: 0 PID: 3595 Comm: mmdata_mgr Not tainted 5.17.1+ #166
 Hardware name: Google Lazor (rev1 - 2) with LTE (DT)
 Call trace:
  dump_backtrace+0xf4/0x114
  show_stack+0x24/0x30
  dump_stack_lvl+0x64/0x7c
  dump_stack+0x18/0x38
  panic+0x150/0x38c
  nmi_panic+0x88/0xa0
  arm64_serror_panic+0x74/0x80
  do_serror+0x0/0x80
  do_serror+0x58/0x80
  el1h_64_error_handler+0x34/0x4c
  el1h_64_error+0x78/0x7c
  mutex_lock+0x4c/0x80
  __gsi_channel_start+0x50/0x17c
  gsi_channel_start+0x54/0x90
  ipa_endpoint_enable_one+0x34/0xc0
  ipa_open+0x4c/0x120

Remove all IP0 resource management from the interconnect driver so that
clk-rpmh is the sole owner. This fixes the issue by preventing the
interconnect driver from overwriting the IP0 resource data that the
clk-rpmh driver wrote.

Cc: Alex Elder <elder@linaro.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Taniya Das <quic_tdas@quicinc.com>
Cc: Mike Tipton <quic_mdtipton@quicinc.com>
Fixes: b95b668eaaa2 ("interconnect: qcom: icc-rpmh: Add BCMs to commit list in pre_aggregate")
Fixes: bcd63d222b60 ("clk: qcom: rpmh: Add IPA clock for SC7180")
Fixes: 7d3b0b0d8184 ("interconnect: qcom: Use icc_sync_state")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/interconnect/qcom/sc7180.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/drivers/interconnect/qcom/sc7180.c b/drivers/interconnect/qcom/sc7180.c
index 12d59c36df53..5f7c0f85fa8e 100644
--- a/drivers/interconnect/qcom/sc7180.c
+++ b/drivers/interconnect/qcom/sc7180.c
@@ -47,7 +47,6 @@ DEFINE_QNODE(qnm_mnoc_sf, SC7180_MASTER_MNOC_SF_MEM_NOC, 1, 32, SC7180_SLAVE_GEM
 DEFINE_QNODE(qnm_snoc_gc, SC7180_MASTER_SNOC_GC_MEM_NOC, 1, 8, SC7180_SLAVE_LLCC);
 DEFINE_QNODE(qnm_snoc_sf, SC7180_MASTER_SNOC_SF_MEM_NOC, 1, 16, SC7180_SLAVE_LLCC);
 DEFINE_QNODE(qxm_gpu, SC7180_MASTER_GFX3D, 2, 32, SC7180_SLAVE_GEM_NOC_SNOC, SC7180_SLAVE_LLCC);
-DEFINE_QNODE(ipa_core_master, SC7180_MASTER_IPA_CORE, 1, 8, SC7180_SLAVE_IPA_CORE);
 DEFINE_QNODE(llcc_mc, SC7180_MASTER_LLCC, 2, 4, SC7180_SLAVE_EBI1);
 DEFINE_QNODE(qhm_mnoc_cfg, SC7180_MASTER_CNOC_MNOC_CFG, 1, 4, SC7180_SLAVE_SERVICE_MNOC);
 DEFINE_QNODE(qxm_camnoc_hf0, SC7180_MASTER_CAMNOC_HF0, 2, 32, SC7180_SLAVE_MNOC_HF_MEM_NOC);
@@ -129,7 +128,6 @@ DEFINE_QNODE(qhs_mdsp_ms_mpu_cfg, SC7180_SLAVE_MSS_PROC_MS_MPU_CFG, 1, 4);
 DEFINE_QNODE(qns_gem_noc_snoc, SC7180_SLAVE_GEM_NOC_SNOC, 1, 8, SC7180_MASTER_GEM_NOC_SNOC);
 DEFINE_QNODE(qns_llcc, SC7180_SLAVE_LLCC, 1, 16, SC7180_MASTER_LLCC);
 DEFINE_QNODE(srvc_gemnoc, SC7180_SLAVE_SERVICE_GEM_NOC, 1, 4);
-DEFINE_QNODE(ipa_core_slave, SC7180_SLAVE_IPA_CORE, 1, 8);
 DEFINE_QNODE(ebi, SC7180_SLAVE_EBI1, 2, 4);
 DEFINE_QNODE(qns_mem_noc_hf, SC7180_SLAVE_MNOC_HF_MEM_NOC, 1, 32, SC7180_MASTER_MNOC_HF_MEM_NOC);
 DEFINE_QNODE(qns_mem_noc_sf, SC7180_SLAVE_MNOC_SF_MEM_NOC, 1, 32, SC7180_MASTER_MNOC_SF_MEM_NOC);
@@ -160,7 +158,6 @@ DEFINE_QBCM(bcm_mc0, "MC0", true, &ebi);
 DEFINE_QBCM(bcm_sh0, "SH0", true, &qns_llcc);
 DEFINE_QBCM(bcm_mm0, "MM0", false, &qns_mem_noc_hf);
 DEFINE_QBCM(bcm_ce0, "CE0", false, &qxm_crypto);
-DEFINE_QBCM(bcm_ip0, "IP0", false, &ipa_core_slave);
 DEFINE_QBCM(bcm_cn0, "CN0", true, &qnm_snoc, &xm_qdss_dap, &qhs_a1_noc_cfg, &qhs_a2_noc_cfg, &qhs_ahb2phy0, &qhs_aop, &qhs_aoss, &qhs_boot_rom, &qhs_camera_cfg, &qhs_camera_nrt_throttle_cfg, &qhs_camera_rt_throttle_cfg, &qhs_clk_ctl, &qhs_cpr_cx, &qhs_cpr_mx, &qhs_crypto0_cfg, &qhs_dcc_cfg, &qhs_ddrss_cfg, &qhs_display_cfg, &qhs_display_rt_throttle_cfg, &qhs_display_throttle_cfg, &qhs_glm, &qhs_gpuss_cfg, &qhs_imem_cfg, &qhs_ipa, &qhs_mnoc_cfg, &qhs_mss_cfg, &qhs_npu_cfg, &qhs_npu_dma_throttle_cfg, &qhs_npu_dsp_throttle_cfg, &qhs_pimem_cfg, &qhs_prng, &qhs_qdss_cfg, &qhs_qm_cfg, &qhs_qm_mpu_cfg, &qhs_qup0, &qhs_qup1, &qhs_security, &qhs_snoc_cfg, &qhs_tcsr, &qhs_tlmm_1, &qhs_tlmm_2, &qhs_tlmm_3, &qhs_ufs_mem_cfg, &qhs_usb3, &qhs_venus_cfg, &qhs_venus_throttle_cfg, &qhs_vsense_ctrl_cfg, &srvc_cnoc);
 DEFINE_QBCM(bcm_mm1, "MM1", false, &qxm_camnoc_hf0_uncomp, &qxm_camnoc_hf1_uncomp, &qxm_camnoc_sf_uncomp, &qhm_mnoc_cfg, &qxm_mdp0, &qxm_rot, &qxm_venus0, &qxm_venus_arm9);
 DEFINE_QBCM(bcm_sh2, "SH2", false, &acm_sys_tcu);
@@ -372,22 +369,6 @@ static struct qcom_icc_desc sc7180_gem_noc = {
 	.num_bcms = ARRAY_SIZE(gem_noc_bcms),
 };
 
-static struct qcom_icc_bcm *ipa_virt_bcms[] = {
-	&bcm_ip0,
-};
-
-static struct qcom_icc_node *ipa_virt_nodes[] = {
-	[MASTER_IPA_CORE] = &ipa_core_master,
-	[SLAVE_IPA_CORE] = &ipa_core_slave,
-};
-
-static struct qcom_icc_desc sc7180_ipa_virt = {
-	.nodes = ipa_virt_nodes,
-	.num_nodes = ARRAY_SIZE(ipa_virt_nodes),
-	.bcms = ipa_virt_bcms,
-	.num_bcms = ARRAY_SIZE(ipa_virt_bcms),
-};
-
 static struct qcom_icc_bcm *mc_virt_bcms[] = {
 	&bcm_acv,
 	&bcm_mc0,
@@ -519,8 +500,6 @@ static const struct of_device_id qnoc_of_match[] = {
 	  .data = &sc7180_dc_noc},
 	{ .compatible = "qcom,sc7180-gem-noc",
 	  .data = &sc7180_gem_noc},
-	{ .compatible = "qcom,sc7180-ipa-virt",
-	  .data = &sc7180_ipa_virt},
 	{ .compatible = "qcom,sc7180-mc-virt",
 	  .data = &sc7180_mc_virt},
 	{ .compatible = "qcom,sc7180-mmss-noc",
-- 
https://chromeos.dev

[PATCH 2/2] interconnect: qcom: sdx55: Drop IP0 interconnects

[Stephen Boyd]

Similar to the sc7180 commit, let's drop the IP0 interconnects here
because the IP0 resource is also used in the clk-rpmh driver on sdx55.
It's bad to have the clk framework and interconnect framework control
the same RPMh resource without any coordination. The rpmh driver in the
kernel doesn't aggregate resources between clients either, so leaving
control to clk-rpmh avoids any issues with unused interconnects turning
off IP0 behind the back of the clk framework.

Cc: Alex Elder <elder@linaro.org>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Taniya Das <quic_tdas@quicinc.com>
Cc: Mike Tipton <quic_mdtipton@quicinc.com>
Fixes: b2150cab9a97 ("clk: qcom: rpmh: add support for SDX55 rpmh IPA clock")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/interconnect/qcom/sdx55.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/drivers/interconnect/qcom/sdx55.c b/drivers/interconnect/qcom/sdx55.c
index 03d604f84cc5..e3ac25a997b7 100644
--- a/drivers/interconnect/qcom/sdx55.c
+++ b/drivers/interconnect/qcom/sdx55.c
@@ -18,7 +18,6 @@
 #include "icc-rpmh.h"
 #include "sdx55.h"
 
-DEFINE_QNODE(ipa_core_master, SDX55_MASTER_IPA_CORE, 1, 8, SDX55_SLAVE_IPA_CORE);
 DEFINE_QNODE(llcc_mc, SDX55_MASTER_LLCC, 4, 4, SDX55_SLAVE_EBI_CH0);
 DEFINE_QNODE(acm_tcu, SDX55_MASTER_TCU_0, 1, 8, SDX55_SLAVE_LLCC, SDX55_SLAVE_MEM_NOC_SNOC, SDX55_SLAVE_MEM_NOC_PCIE_SNOC);
 DEFINE_QNODE(qnm_snoc_gc, SDX55_MASTER_SNOC_GC_MEM_NOC, 1, 8, SDX55_SLAVE_LLCC);
@@ -40,7 +39,6 @@ DEFINE_QNODE(xm_pcie, SDX55_MASTER_PCIE, 1, 8, SDX55_SLAVE_ANOC_SNOC);
 DEFINE_QNODE(xm_qdss_etr, SDX55_MASTER_QDSS_ETR, 1, 8, SDX55_SLAVE_SNOC_CFG, SDX55_SLAVE_EMAC_CFG, SDX55_SLAVE_USB3, SDX55_SLAVE_AOSS, SDX55_SLAVE_SPMI_FETCHER, SDX55_SLAVE_QDSS_CFG, SDX55_SLAVE_PDM, SDX55_SLAVE_SNOC_MEM_NOC_GC, SDX55_SLAVE_TCSR, SDX55_SLAVE_CNOC_DDRSS, SDX55_SLAVE_SPMI_VGI_COEX, SDX55_SLAVE_QPIC, SDX55_SLAVE_OCIMEM, SDX55_SLAVE_IPA_CFG, SDX55_SLAVE_USB3_PHY_CFG, SDX55_SLAVE_AOP, SDX55_SLAVE_BLSP_1, SDX55_SLAVE_SDCC_1, SDX55_SLAVE_CNOC_MSS, SDX55_SLAVE_PCIE_PARF, SDX55_SLAVE_ECC_CFG, SDX55_SLAVE_AUDIO, SDX55_SLAVE_AOSS, SDX55_SLAVE_PRNG, SDX55_SLAVE_CRYPTO_0_CFG, SDX55_SLAVE_TCU, SDX55_SLAVE_CLK_CTL, SDX55_SLAVE_IMEM_CFG);
 DEFINE_QNODE(xm_sdc1, SDX55_MASTER_SDCC_1, 1, 8, SDX55_SLAVE_AOSS, SDX55_SLAVE_IPA_CFG, SDX55_SLAVE_ANOC_SNOC, SDX55_SLAVE_AOP, SDX55_SLAVE_AUDIO);
 DEFINE_QNODE(xm_usb3, SDX55_MASTER_USB3, 1, 8, SDX55_SLAVE_ANOC_SNOC);
-DEFINE_QNODE(ipa_core_slave, SDX55_SLAVE_IPA_CORE, 1, 8);
 DEFINE_QNODE(ebi, SDX55_SLAVE_EBI_CH0, 1, 4);
 DEFINE_QNODE(qns_llcc, SDX55_SLAVE_LLCC, 1, 16, SDX55_SLAVE_EBI_CH0);
 DEFINE_QNODE(qns_memnoc_snoc, SDX55_SLAVE_MEM_NOC_SNOC, 1, 8, SDX55_MASTER_MEM_NOC_SNOC);
@@ -82,7 +80,6 @@ DEFINE_QNODE(xs_sys_tcu_cfg, SDX55_SLAVE_TCU, 1, 8);
 DEFINE_QBCM(bcm_mc0, "MC0", true, &ebi);
 DEFINE_QBCM(bcm_sh0, "SH0", true, &qns_llcc);
 DEFINE_QBCM(bcm_ce0, "CE0", false, &qxm_crypto);
-DEFINE_QBCM(bcm_ip0, "IP0", false, &ipa_core_slave);
 DEFINE_QBCM(bcm_pn0, "PN0", false, &qhm_snoc_cfg);
 DEFINE_QBCM(bcm_sh3, "SH3", false, &xm_apps_rdwr);
 DEFINE_QBCM(bcm_sh4, "SH4", false, &qns_memnoc_snoc, &qns_sys_pcie);
@@ -219,22 +216,6 @@ static const struct qcom_icc_desc sdx55_system_noc = {
 	.num_bcms = ARRAY_SIZE(system_noc_bcms),
 };
 
-static struct qcom_icc_bcm *ipa_virt_bcms[] = {
-	&bcm_ip0,
-};
-
-static struct qcom_icc_node *ipa_virt_nodes[] = {
-	[MASTER_IPA_CORE] = &ipa_core_master,
-	[SLAVE_IPA_CORE] = &ipa_core_slave,
-};
-
-static const struct qcom_icc_desc sdx55_ipa_virt = {
-	.nodes = ipa_virt_nodes,
-	.num_nodes = ARRAY_SIZE(ipa_virt_nodes),
-	.bcms = ipa_virt_bcms,
-	.num_bcms = ARRAY_SIZE(ipa_virt_bcms),
-};
-
 static const struct of_device_id qnoc_of_match[] = {
 	{ .compatible = "qcom,sdx55-mc-virt",
 	  .data = &sdx55_mc_virt},
@@ -242,8 +223,6 @@ static const struct of_device_id qnoc_of_match[] = {
 	  .data = &sdx55_mem_noc},
 	{ .compatible = "qcom,sdx55-system-noc",
 	  .data = &sdx55_system_noc},
-	{ .compatible = "qcom,sdx55-ipa-virt",
-	  .data = &sdx55_ipa_virt},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, qnoc_of_match);
-- 
https://chromeos.dev

Re: [PATCH 1/2] interconnect: qcom: sc7180: Drop IP0 interconnects

[Alex Elder]

On 4/12/22 5:00 PM, Stephen Boyd wrote:
> The IPA BCM resource ("IP0") on sc7180 was moved to the clk-rpmh driver
> in commit bcd63d222b60 ("clk: qcom: rpmh: Add IPA clock for SC7180") and
> modeled as a clk, but this interconnect driver still had it modeled as
> an interconnect. This was mostly OK because nobody used the interconnect
> definition, until the interconnect framework started dropping bandwidth
> requests on interconnects that aren't used via the sync_state callback
> in commit 7d3b0b0d8184 ("interconnect: qcom: Use icc_sync_state"). Once
> that patch was applied the IP0 resource was going to be controlled from
> two places, the clk framework and the interconnect framework.
> 
> Even then, things were probably going to be OK, because commit
> b95b668eaaa2 ("interconnect: qcom: icc-rpmh: Add BCMs to commit list in
> pre_aggregate") was needed to actually drop bandwidth requests on unused
> interconnects, of which the IPA was one of the interconnect that wasn't
> getting dropped to zero. Combining the three commits together leads to
> bad behavior where the interconnect framework is disabling the IP0
> resource because it has no users while the clk framework thinks the IP0
> resource is on because the only user, the IPA driver, has turned it on
> via clk_prepare_enable(). Depending on when sync_state is called, we can
> get into a situation like below:
> 
>    IPA driver probes
>    IPA driver gets notified modem started
>     runtime PM get()
>      IPA clk enabled -> IP0 resource is ON
>    sync_state runs
>     interconnect zeroes out the IP0 resource -> IP0 resource is off
>    IPA driver tries to access a register and blows up
> 
> The crash is an unclocked access that manifest as an SError.
> 
>   SError Interrupt on CPU0, code 0xbe000011 -- SError
>   CPU: 0 PID: 3595 Comm: mmdata_mgr Not tainted 5.17.1+ #166
>   Hardware name: Google Lazor (rev1 - 2) with LTE (DT)
>   pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>   pc : mutex_lock+0x4c/0x80
>   lr : mutex_lock+0x30/0x80
>   sp : ffffffc00da9b9c0
>   x29: ffffffc00da9b9c0 x28: 0000000000000000 x27: 0000000000000000
>   x26: ffffffc00da9bc90 x25: ffffff80c2024010 x24: ffffff80c2024000
>   x23: ffffff8083100000 x22: ffffff80831000d0 x21: ffffff80831000a8
>   x20: ffffff80831000a8 x19: ffffff8083100070 x18: 00000000ffff0a00
>   x17: 000000002f7254f1 x16: 0000000000000100 x15: 0000000000000000
>   x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
>   x11: 000000000001f0b8 x10: ffffffc00931f0b8 x9 : 0000000000000000
>   x8 : 0000000000000000 x7 : fefefefefeff2f60 x6 : 0000808080808080
>   x5 : 0000000000000000 x4 : 8080808080800000 x3 : ffffff80d2d4ee28
>   x2 : ffffff808c1d6e40 x1 : 0000000000000000 x0 : ffffff8083100070
>   Kernel panic - not syncing: Asynchronous SError Interrupt
>   CPU: 0 PID: 3595 Comm: mmdata_mgr Not tainted 5.17.1+ #166
>   Hardware name: Google Lazor (rev1 - 2) with LTE (DT)
>   Call trace:
>    dump_backtrace+0xf4/0x114
>    show_stack+0x24/0x30
>    dump_stack_lvl+0x64/0x7c
>    dump_stack+0x18/0x38
>    panic+0x150/0x38c
>    nmi_panic+0x88/0xa0
>    arm64_serror_panic+0x74/0x80
>    do_serror+0x0/0x80
>    do_serror+0x58/0x80
>    el1h_64_error_handler+0x34/0x4c
>    el1h_64_error+0x78/0x7c
>    mutex_lock+0x4c/0x80
>    __gsi_channel_start+0x50/0x17c
>    gsi_channel_start+0x54/0x90
>    ipa_endpoint_enable_one+0x34/0xc0
>    ipa_open+0x4c/0x120
> 
> Remove all IP0 resource management from the interconnect driver so that
> clk-rpmh is the sole owner. This fixes the issue by preventing the
> interconnect driver from overwriting the IP0 resource data that the
> clk-rpmh driver wrote.

Very nice description.

I tested this change yesterday, but now I've tested this
actual patch as well 10  or so times with and without it
applied and can confirm it makes the crash go away, so:

Tested-by: Alex Elder <elder@linaro.org>

Reviewed-by: Alex Elder <elder@linaro.org>

> 
> Cc: Alex Elder <elder@linaro.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Taniya Das <quic_tdas@quicinc.com>
> Cc: Mike Tipton <quic_mdtipton@quicinc.com>
> Fixes: b95b668eaaa2 ("interconnect: qcom: icc-rpmh: Add BCMs to commit list in pre_aggregate")
> Fixes: bcd63d222b60 ("clk: qcom: rpmh: Add IPA clock for SC7180")
> Fixes: 7d3b0b0d8184 ("interconnect: qcom: Use icc_sync_state")
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>   drivers/interconnect/qcom/sc7180.c | 21 ---------------------
>   1 file changed, 21 deletions(-)
> 
> diff --git a/drivers/interconnect/qcom/sc7180.c b/drivers/interconnect/qcom/sc7180.c
> index 12d59c36df53..5f7c0f85fa8e 100644
> --- a/drivers/interconnect/qcom/sc7180.c
> +++ b/drivers/interconnect/qcom/sc7180.c
> @@ -47,7 +47,6 @@ DEFINE_QNODE(qnm_mnoc_sf, SC7180_MASTER_MNOC_SF_MEM_NOC, 1, 32, SC7180_SLAVE_GEM
>   DEFINE_QNODE(qnm_snoc_gc, SC7180_MASTER_SNOC_GC_MEM_NOC, 1, 8, SC7180_SLAVE_LLCC);
>   DEFINE_QNODE(qnm_snoc_sf, SC7180_MASTER_SNOC_SF_MEM_NOC, 1, 16, SC7180_SLAVE_LLCC);
>   DEFINE_QNODE(qxm_gpu, SC7180_MASTER_GFX3D, 2, 32, SC7180_SLAVE_GEM_NOC_SNOC, SC7180_SLAVE_LLCC);
> -DEFINE_QNODE(ipa_core_master, SC7180_MASTER_IPA_CORE, 1, 8, SC7180_SLAVE_IPA_CORE);
>   DEFINE_QNODE(llcc_mc, SC7180_MASTER_LLCC, 2, 4, SC7180_SLAVE_EBI1);
>   DEFINE_QNODE(qhm_mnoc_cfg, SC7180_MASTER_CNOC_MNOC_CFG, 1, 4, SC7180_SLAVE_SERVICE_MNOC);
>   DEFINE_QNODE(qxm_camnoc_hf0, SC7180_MASTER_CAMNOC_HF0, 2, 32, SC7180_SLAVE_MNOC_HF_MEM_NOC);
> @@ -129,7 +128,6 @@ DEFINE_QNODE(qhs_mdsp_ms_mpu_cfg, SC7180_SLAVE_MSS_PROC_MS_MPU_CFG, 1, 4);
>   DEFINE_QNODE(qns_gem_noc_snoc, SC7180_SLAVE_GEM_NOC_SNOC, 1, 8, SC7180_MASTER_GEM_NOC_SNOC);
>   DEFINE_QNODE(qns_llcc, SC7180_SLAVE_LLCC, 1, 16, SC7180_MASTER_LLCC);
>   DEFINE_QNODE(srvc_gemnoc, SC7180_SLAVE_SERVICE_GEM_NOC, 1, 4);
> -DEFINE_QNODE(ipa_core_slave, SC7180_SLAVE_IPA_CORE, 1, 8);
>   DEFINE_QNODE(ebi, SC7180_SLAVE_EBI1, 2, 4);
>   DEFINE_QNODE(qns_mem_noc_hf, SC7180_SLAVE_MNOC_HF_MEM_NOC, 1, 32, SC7180_MASTER_MNOC_HF_MEM_NOC);
>   DEFINE_QNODE(qns_mem_noc_sf, SC7180_SLAVE_MNOC_SF_MEM_NOC, 1, 32, SC7180_MASTER_MNOC_SF_MEM_NOC);
> @@ -160,7 +158,6 @@ DEFINE_QBCM(bcm_mc0, "MC0", true, &ebi);
>   DEFINE_QBCM(bcm_sh0, "SH0", true, &qns_llcc);
>   DEFINE_QBCM(bcm_mm0, "MM0", false, &qns_mem_noc_hf);
>   DEFINE_QBCM(bcm_ce0, "CE0", false, &qxm_crypto);
> -DEFINE_QBCM(bcm_ip0, "IP0", false, &ipa_core_slave);
>   DEFINE_QBCM(bcm_cn0, "CN0", true, &qnm_snoc, &xm_qdss_dap, &qhs_a1_noc_cfg, &qhs_a2_noc_cfg, &qhs_ahb2phy0, &qhs_aop, &qhs_aoss, &qhs_boot_rom, &qhs_camera_cfg, &qhs_camera_nrt_throttle_cfg, &qhs_camera_rt_throttle_cfg, &qhs_clk_ctl, &qhs_cpr_cx, &qhs_cpr_mx, &qhs_crypto0_cfg, &qhs_dcc_cfg, &qhs_ddrss_cfg, &qhs_display_cfg, &qhs_display_rt_throttle_cfg, &qhs_display_throttle_cfg, &qhs_glm, &qhs_gpuss_cfg, &qhs_imem_cfg, &qhs_ipa, &qhs_mnoc_cfg, &qhs_mss_cfg, &qhs_npu_cfg, &qhs_npu_dma_throttle_cfg, &qhs_npu_dsp_throttle_cfg, &qhs_pimem_cfg, &qhs_prng, &qhs_qdss_cfg, &qhs_qm_cfg, &qhs_qm_mpu_cfg, &qhs_qup0, &qhs_qup1, &qhs_security, &qhs_snoc_cfg, &qhs_tcsr, &qhs_tlmm_1, &qhs_tlmm_2, &qhs_tlmm_3, &qhs_ufs_mem_cfg, &qhs_usb3, &qhs_venus_cfg, &qhs_venus_throttle_cfg, &qhs_vsense_ctrl_cfg, &srvc_cnoc);
>   DEFINE_QBCM(bcm_mm1, "MM1", false, &qxm_camnoc_hf0_uncomp, &qxm_camnoc_hf1_uncomp, &qxm_camnoc_sf_uncomp, &qhm_mnoc_cfg, &qxm_mdp0, &qxm_rot, &qxm_venus0, &qxm_venus_arm9);
>   DEFINE_QBCM(bcm_sh2, "SH2", false, &acm_sys_tcu);
> @@ -372,22 +369,6 @@ static struct qcom_icc_desc sc7180_gem_noc = {
>   	.num_bcms = ARRAY_SIZE(gem_noc_bcms),
>   };
>   
> -static struct qcom_icc_bcm *ipa_virt_bcms[] = {
> -	&bcm_ip0,
> -};
> -
> -static struct qcom_icc_node *ipa_virt_nodes[] = {
> -	[MASTER_IPA_CORE] = &ipa_core_master,
> -	[SLAVE_IPA_CORE] = &ipa_core_slave,
> -};
> -
> -static struct qcom_icc_desc sc7180_ipa_virt = {
> -	.nodes = ipa_virt_nodes,
> -	.num_nodes = ARRAY_SIZE(ipa_virt_nodes),
> -	.bcms = ipa_virt_bcms,
> -	.num_bcms = ARRAY_SIZE(ipa_virt_bcms),
> -};
> -
>   static struct qcom_icc_bcm *mc_virt_bcms[] = {
>   	&bcm_acv,
>   	&bcm_mc0,
> @@ -519,8 +500,6 @@ static const struct of_device_id qnoc_of_match[] = {
>   	  .data = &sc7180_dc_noc},
>   	{ .compatible = "qcom,sc7180-gem-noc",
>   	  .data = &sc7180_gem_noc},
> -	{ .compatible = "qcom,sc7180-ipa-virt",
> -	  .data = &sc7180_ipa_virt},
>   	{ .compatible = "qcom,sc7180-mc-virt",
>   	  .data = &sc7180_mc_virt},
>   	{ .compatible = "qcom,sc7180-mmss-noc",

Re: [PATCH 2/2] interconnect: qcom: sdx55: Drop IP0 interconnects

[Alex Elder]

On 4/12/22 5:00 PM, Stephen Boyd wrote:
> Similar to the sc7180 commit, let's drop the IP0 interconnects here
> because the IP0 resource is also used in the clk-rpmh driver on sdx55.
> It's bad to have the clk framework and interconnect framework control
> the same RPMh resource without any coordination. The rpmh driver in the
> kernel doesn't aggregate resources between clients either, so leaving
> control to clk-rpmh avoids any issues with unused interconnects turning
> off IP0 behind the back of the clk framework.
> 
> Cc: Alex Elder <elder@linaro.org>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Taniya Das <quic_tdas@quicinc.com>
> Cc: Mike Tipton <quic_mdtipton@quicinc.com>
> Fixes: b2150cab9a97 ("clk: qcom: rpmh: add support for SDX55 rpmh IPA clock")
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

Looks good.

Reviewed-by: Alex Elder <elder@linaro.org>

> ---
>   drivers/interconnect/qcom/sdx55.c | 21 ---------------------
>   1 file changed, 21 deletions(-)
> 
> diff --git a/drivers/interconnect/qcom/sdx55.c b/drivers/interconnect/qcom/sdx55.c
> index 03d604f84cc5..e3ac25a997b7 100644
> --- a/drivers/interconnect/qcom/sdx55.c
> +++ b/drivers/interconnect/qcom/sdx55.c
> @@ -18,7 +18,6 @@
>   #include "icc-rpmh.h"
>   #include "sdx55.h"
>   
> -DEFINE_QNODE(ipa_core_master, SDX55_MASTER_IPA_CORE, 1, 8, SDX55_SLAVE_IPA_CORE);
>   DEFINE_QNODE(llcc_mc, SDX55_MASTER_LLCC, 4, 4, SDX55_SLAVE_EBI_CH0);
>   DEFINE_QNODE(acm_tcu, SDX55_MASTER_TCU_0, 1, 8, SDX55_SLAVE_LLCC, SDX55_SLAVE_MEM_NOC_SNOC, SDX55_SLAVE_MEM_NOC_PCIE_SNOC);
>   DEFINE_QNODE(qnm_snoc_gc, SDX55_MASTER_SNOC_GC_MEM_NOC, 1, 8, SDX55_SLAVE_LLCC);
> @@ -40,7 +39,6 @@ DEFINE_QNODE(xm_pcie, SDX55_MASTER_PCIE, 1, 8, SDX55_SLAVE_ANOC_SNOC);
>   DEFINE_QNODE(xm_qdss_etr, SDX55_MASTER_QDSS_ETR, 1, 8, SDX55_SLAVE_SNOC_CFG, SDX55_SLAVE_EMAC_CFG, SDX55_SLAVE_USB3, SDX55_SLAVE_AOSS, SDX55_SLAVE_SPMI_FETCHER, SDX55_SLAVE_QDSS_CFG, SDX55_SLAVE_PDM, SDX55_SLAVE_SNOC_MEM_NOC_GC, SDX55_SLAVE_TCSR, SDX55_SLAVE_CNOC_DDRSS, SDX55_SLAVE_SPMI_VGI_COEX, SDX55_SLAVE_QPIC, SDX55_SLAVE_OCIMEM, SDX55_SLAVE_IPA_CFG, SDX55_SLAVE_USB3_PHY_CFG, SDX55_SLAVE_AOP, SDX55_SLAVE_BLSP_1, SDX55_SLAVE_SDCC_1, SDX55_SLAVE_CNOC_MSS, SDX55_SLAVE_PCIE_PARF, SDX55_SLAVE_ECC_CFG, SDX55_SLAVE_AUDIO, SDX55_SLAVE_AOSS, SDX55_SLAVE_PRNG, SDX55_SLAVE_CRYPTO_0_CFG, SDX55_SLAVE_TCU, SDX55_SLAVE_CLK_CTL, SDX55_SLAVE_IMEM_CFG);
>   DEFINE_QNODE(xm_sdc1, SDX55_MASTER_SDCC_1, 1, 8, SDX55_SLAVE_AOSS, SDX55_SLAVE_IPA_CFG, SDX55_SLAVE_ANOC_SNOC, SDX55_SLAVE_AOP, SDX55_SLAVE_AUDIO);
>   DEFINE_QNODE(xm_usb3, SDX55_MASTER_USB3, 1, 8, SDX55_SLAVE_ANOC_SNOC);
> -DEFINE_QNODE(ipa_core_slave, SDX55_SLAVE_IPA_CORE, 1, 8);
>   DEFINE_QNODE(ebi, SDX55_SLAVE_EBI_CH0, 1, 4);
>   DEFINE_QNODE(qns_llcc, SDX55_SLAVE_LLCC, 1, 16, SDX55_SLAVE_EBI_CH0);
>   DEFINE_QNODE(qns_memnoc_snoc, SDX55_SLAVE_MEM_NOC_SNOC, 1, 8, SDX55_MASTER_MEM_NOC_SNOC);
> @@ -82,7 +80,6 @@ DEFINE_QNODE(xs_sys_tcu_cfg, SDX55_SLAVE_TCU, 1, 8);
>   DEFINE_QBCM(bcm_mc0, "MC0", true, &ebi);
>   DEFINE_QBCM(bcm_sh0, "SH0", true, &qns_llcc);
>   DEFINE_QBCM(bcm_ce0, "CE0", false, &qxm_crypto);
> -DEFINE_QBCM(bcm_ip0, "IP0", false, &ipa_core_slave);
>   DEFINE_QBCM(bcm_pn0, "PN0", false, &qhm_snoc_cfg);
>   DEFINE_QBCM(bcm_sh3, "SH3", false, &xm_apps_rdwr);
>   DEFINE_QBCM(bcm_sh4, "SH4", false, &qns_memnoc_snoc, &qns_sys_pcie);
> @@ -219,22 +216,6 @@ static const struct qcom_icc_desc sdx55_system_noc = {
>   	.num_bcms = ARRAY_SIZE(system_noc_bcms),
>   };
>   
> -static struct qcom_icc_bcm *ipa_virt_bcms[] = {
> -	&bcm_ip0,
> -};
> -
> -static struct qcom_icc_node *ipa_virt_nodes[] = {
> -	[MASTER_IPA_CORE] = &ipa_core_master,
> -	[SLAVE_IPA_CORE] = &ipa_core_slave,
> -};
> -
> -static const struct qcom_icc_desc sdx55_ipa_virt = {
> -	.nodes = ipa_virt_nodes,
> -	.num_nodes = ARRAY_SIZE(ipa_virt_nodes),
> -	.bcms = ipa_virt_bcms,
> -	.num_bcms = ARRAY_SIZE(ipa_virt_bcms),
> -};
> -
>   static const struct of_device_id qnoc_of_match[] = {
>   	{ .compatible = "qcom,sdx55-mc-virt",
>   	  .data = &sdx55_mc_virt},
> @@ -242,8 +223,6 @@ static const struct of_device_id qnoc_of_match[] = {
>   	  .data = &sdx55_mem_noc},
>   	{ .compatible = "qcom,sdx55-system-noc",
>   	  .data = &sdx55_system_noc},
> -	{ .compatible = "qcom,sdx55-ipa-virt",
> -	  .data = &sdx55_ipa_virt},
>   	{ }
>   };
>   MODULE_DEVICE_TABLE(of, qnoc_of_match);

Re: [PATCH 2/2] interconnect: qcom: sdx55: Drop IP0 interconnects

[Manivannan Sadhasivam]

On Tue, Apr 12, 2022 at 03:00:33PM -0700, Stephen Boyd wrote:
> Similar to the sc7180 commit, let's drop the IP0 interconnects here
> because the IP0 resource is also used in the clk-rpmh driver on sdx55.
> It's bad to have the clk framework and interconnect framework control
> the same RPMh resource without any coordination. The rpmh driver in the
> kernel doesn't aggregate resources between clients either, so leaving
> control to clk-rpmh avoids any issues with unused interconnects turning
> off IP0 behind the back of the clk framework.
> 
> Cc: Alex Elder <elder@linaro.org>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Taniya Das <quic_tdas@quicinc.com>
> Cc: Mike Tipton <quic_mdtipton@quicinc.com>
> Fixes: b2150cab9a97 ("clk: qcom: rpmh: add support for SDX55 rpmh IPA clock")
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  drivers/interconnect/qcom/sdx55.c | 21 ---------------------
>  1 file changed, 21 deletions(-)
> 
> diff --git a/drivers/interconnect/qcom/sdx55.c b/drivers/interconnect/qcom/sdx55.c
> index 03d604f84cc5..e3ac25a997b7 100644
> --- a/drivers/interconnect/qcom/sdx55.c
> +++ b/drivers/interconnect/qcom/sdx55.c
> @@ -18,7 +18,6 @@
>  #include "icc-rpmh.h"
>  #include "sdx55.h"
>  
> -DEFINE_QNODE(ipa_core_master, SDX55_MASTER_IPA_CORE, 1, 8, SDX55_SLAVE_IPA_CORE);
>  DEFINE_QNODE(llcc_mc, SDX55_MASTER_LLCC, 4, 4, SDX55_SLAVE_EBI_CH0);
>  DEFINE_QNODE(acm_tcu, SDX55_MASTER_TCU_0, 1, 8, SDX55_SLAVE_LLCC, SDX55_SLAVE_MEM_NOC_SNOC, SDX55_SLAVE_MEM_NOC_PCIE_SNOC);
>  DEFINE_QNODE(qnm_snoc_gc, SDX55_MASTER_SNOC_GC_MEM_NOC, 1, 8, SDX55_SLAVE_LLCC);
> @@ -40,7 +39,6 @@ DEFINE_QNODE(xm_pcie, SDX55_MASTER_PCIE, 1, 8, SDX55_SLAVE_ANOC_SNOC);
>  DEFINE_QNODE(xm_qdss_etr, SDX55_MASTER_QDSS_ETR, 1, 8, SDX55_SLAVE_SNOC_CFG, SDX55_SLAVE_EMAC_CFG, SDX55_SLAVE_USB3, SDX55_SLAVE_AOSS, SDX55_SLAVE_SPMI_FETCHER, SDX55_SLAVE_QDSS_CFG, SDX55_SLAVE_PDM, SDX55_SLAVE_SNOC_MEM_NOC_GC, SDX55_SLAVE_TCSR, SDX55_SLAVE_CNOC_DDRSS, SDX55_SLAVE_SPMI_VGI_COEX, SDX55_SLAVE_QPIC, SDX55_SLAVE_OCIMEM, SDX55_SLAVE_IPA_CFG, SDX55_SLAVE_USB3_PHY_CFG, SDX55_SLAVE_AOP, SDX55_SLAVE_BLSP_1, SDX55_SLAVE_SDCC_1, SDX55_SLAVE_CNOC_MSS, SDX55_SLAVE_PCIE_PARF, SDX55_SLAVE_ECC_CFG, SDX55_SLAVE_AUDIO, SDX55_SLAVE_AOSS, SDX55_SLAVE_PRNG, SDX55_SLAVE_CRYPTO_0_CFG, SDX55_SLAVE_TCU, SDX55_SLAVE_CLK_CTL, SDX55_SLAVE_IMEM_CFG);
>  DEFINE_QNODE(xm_sdc1, SDX55_MASTER_SDCC_1, 1, 8, SDX55_SLAVE_AOSS, SDX55_SLAVE_IPA_CFG, SDX55_SLAVE_ANOC_SNOC, SDX55_SLAVE_AOP, SDX55_SLAVE_AUDIO);
>  DEFINE_QNODE(xm_usb3, SDX55_MASTER_USB3, 1, 8, SDX55_SLAVE_ANOC_SNOC);
> -DEFINE_QNODE(ipa_core_slave, SDX55_SLAVE_IPA_CORE, 1, 8);
>  DEFINE_QNODE(ebi, SDX55_SLAVE_EBI_CH0, 1, 4);
>  DEFINE_QNODE(qns_llcc, SDX55_SLAVE_LLCC, 1, 16, SDX55_SLAVE_EBI_CH0);
>  DEFINE_QNODE(qns_memnoc_snoc, SDX55_SLAVE_MEM_NOC_SNOC, 1, 8, SDX55_MASTER_MEM_NOC_SNOC);
> @@ -82,7 +80,6 @@ DEFINE_QNODE(xs_sys_tcu_cfg, SDX55_SLAVE_TCU, 1, 8);
>  DEFINE_QBCM(bcm_mc0, "MC0", true, &ebi);
>  DEFINE_QBCM(bcm_sh0, "SH0", true, &qns_llcc);
>  DEFINE_QBCM(bcm_ce0, "CE0", false, &qxm_crypto);
> -DEFINE_QBCM(bcm_ip0, "IP0", false, &ipa_core_slave);
>  DEFINE_QBCM(bcm_pn0, "PN0", false, &qhm_snoc_cfg);
>  DEFINE_QBCM(bcm_sh3, "SH3", false, &xm_apps_rdwr);
>  DEFINE_QBCM(bcm_sh4, "SH4", false, &qns_memnoc_snoc, &qns_sys_pcie);
> @@ -219,22 +216,6 @@ static const struct qcom_icc_desc sdx55_system_noc = {
>  	.num_bcms = ARRAY_SIZE(system_noc_bcms),
>  };
>  
> -static struct qcom_icc_bcm *ipa_virt_bcms[] = {
> -	&bcm_ip0,
> -};
> -
> -static struct qcom_icc_node *ipa_virt_nodes[] = {
> -	[MASTER_IPA_CORE] = &ipa_core_master,
> -	[SLAVE_IPA_CORE] = &ipa_core_slave,
> -};
> -
> -static const struct qcom_icc_desc sdx55_ipa_virt = {
> -	.nodes = ipa_virt_nodes,
> -	.num_nodes = ARRAY_SIZE(ipa_virt_nodes),
> -	.bcms = ipa_virt_bcms,
> -	.num_bcms = ARRAY_SIZE(ipa_virt_bcms),
> -};
> -
>  static const struct of_device_id qnoc_of_match[] = {
>  	{ .compatible = "qcom,sdx55-mc-virt",
>  	  .data = &sdx55_mc_virt},
> @@ -242,8 +223,6 @@ static const struct of_device_id qnoc_of_match[] = {
>  	  .data = &sdx55_mem_noc},
>  	{ .compatible = "qcom,sdx55-system-noc",
>  	  .data = &sdx55_system_noc},
> -	{ .compatible = "qcom,sdx55-ipa-virt",
> -	  .data = &sdx55_ipa_virt},
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, qnoc_of_match);
> -- 
> https://chromeos.dev
> 

Re: [PATCH 1/2] interconnect: qcom: sc7180: Drop IP0 interconnects

[Doug Anderson]

Hi,

On Tue, Apr 12, 2022 at 4:20 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> @@ -519,8 +500,6 @@ static const struct of_device_id qnoc_of_match[] = {
>           .data = &sc7180_dc_noc},
>         { .compatible = "qcom,sc7180-gem-noc",
>           .data = &sc7180_gem_noc},
> -       { .compatible = "qcom,sc7180-ipa-virt",
> -         .data = &sc7180_ipa_virt},
>         { .compatible = "qcom,sc7180-mc-virt",
>           .data = &sc7180_mc_virt},
>         { .compatible = "qcom,sc7180-mmss-noc",

I have no objection to ${SUBJECT} change landing and based on all your
research and Alex's review/testing I think it's good to go.

However, now that you're removed the driver that cared about
"qcom,sc7180-ipa-virt", should we also be removing it from the
`bindings/interconnect/qcom,rpmh.yaml` file and the `sc7180.dtsi`
file? I think that removing it from _either_ the driver (like your
patch here does) _or_ the sc7180.dtsi file would fix the bug, right?
...and then removing it from the yaml would just be cleanup...

-Doug

Re: [PATCH 1/2] interconnect: qcom: sc7180: Drop IP0 interconnects

[Alex Elder]

On 4/13/22 3:55 PM, Doug Anderson wrote:
> Hi,
> 
> On Tue, Apr 12, 2022 at 4:20 PM Stephen Boyd <swboyd@chromium.org> wrote:
>>
>> @@ -519,8 +500,6 @@ static const struct of_device_id qnoc_of_match[] = {
>>            .data = &sc7180_dc_noc},
>>          { .compatible = "qcom,sc7180-gem-noc",
>>            .data = &sc7180_gem_noc},
>> -       { .compatible = "qcom,sc7180-ipa-virt",
>> -         .data = &sc7180_ipa_virt},
>>          { .compatible = "qcom,sc7180-mc-virt",
>>            .data = &sc7180_mc_virt},
>>          { .compatible = "qcom,sc7180-mmss-noc",
> 
> I have no objection to ${SUBJECT} change landing and based on all your
> research and Alex's review/testing I think it's good to go.
> 
> However, now that you're removed the driver that cared about
> "qcom,sc7180-ipa-virt", should we also be removing it from the
> `bindings/interconnect/qcom,rpmh.yaml` file and the `sc7180.dtsi`
> file? I think that removing it from _either_ the driver (like your
> patch here does) _or_ the sc7180.dtsi file would fix the bug, right?
> ...and then removing it from the yaml would just be cleanup...

That's a good point, I hadn't thought about that but you're right.

I think we were too pleased about identifying the problem and
proving it could happen (and cause a crash), so we didn't think
hard enough about this other piece.

Stephen, I think you should re-spin the series and add the
proper change to the binding.  You can keep the tags I gave
before.

I've got a note to follow up with similar changes to other
platforms where the interconnect driver includes resource "IP0"
and will plan to do what Doug suggests there too.

					-Alex

> -Doug

Re: [PATCH 1/2] interconnect: qcom: sc7180: Drop IP0 interconnects

[Stephen Boyd]

Quoting Alex Elder (2022-04-13 14:02:00)
> On 4/13/22 3:55 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Apr 12, 2022 at 4:20 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >>
> >> @@ -519,8 +500,6 @@ static const struct of_device_id qnoc_of_match[] = {
> >>            .data = &sc7180_dc_noc},
> >>          { .compatible = "qcom,sc7180-gem-noc",
> >>            .data = &sc7180_gem_noc},
> >> -       { .compatible = "qcom,sc7180-ipa-virt",
> >> -         .data = &sc7180_ipa_virt},
> >>          { .compatible = "qcom,sc7180-mc-virt",
> >>            .data = &sc7180_mc_virt},
> >>          { .compatible = "qcom,sc7180-mmss-noc",
> >
> > I have no objection to ${SUBJECT} change landing and based on all your
> > research and Alex's review/testing I think it's good to go.
> >
> > However, now that you're removed the driver that cared about
> > "qcom,sc7180-ipa-virt", should we also be removing it from the
> > `bindings/interconnect/qcom,rpmh.yaml` file and the `sc7180.dtsi`
> > file? I think that removing it from _either_ the driver (like your
> > patch here does) _or_ the sc7180.dtsi file would fix the bug, right?
> > ...and then removing it from the yaml would just be cleanup...

Yes, but that's mostly a cleanup. I didn't include it in this series
because DTB is supposed to be "stable" and thus backporting a fix to the
kernel by removing something from DT is sort of wrong. I don't know or
expect that the kernel DTS files will be used from the stable kernels.
It's better to fix the kernel C code. We can of course remove the
binding, but there's a part of me that would prefer that we put the IPA
clk back into the interconnect driver, so leaving the binding is another
motivator for me to hopefully excise the IPA clk from the rpmh-clk
driver in the future.

Anyway, I'm happy to remove the compatible string from the binding if
folks want that. Having the DT node is wasteful because the kernel makes
a device so we can certainly remove that as well. I'll send another
patch for that if this patch is accepted by Georgi.

>
> That's a good point, I hadn't thought about that but you're right.
>
> I think we were too pleased about identifying the problem and
> proving it could happen (and cause a crash), so we didn't think
> hard enough about this other piece.
>
> Stephen, I think you should re-spin the series and add the
> proper change to the binding.  You can keep the tags I gave
> before.

I will not combine the removal of the binding from this patch. This
patch is good as is and fixes the problem while ignoring the DT binding
and that larger discussion.

>
> I've got a note to follow up with similar changes to other
> platforms where the interconnect driver includes resource "IP0"
> and will plan to do what Doug suggests there too.

Re: [PATCH 1/2] interconnect: qcom: sc7180: Drop IP0 interconnects

[Alex Elder]

On 4/13/22 6:14 PM, Stephen Boyd wrote:
>> Stephen, I think you should re-spin the series and add the
>> proper change to the binding.  You can keep the tags I gave
>> before.
> I will not combine the removal of the binding from this patch. This
> patch is good as is and fixes the problem while ignoring the DT binding
> and that larger discussion.

OK, and I concur it's better to make the change in the kernel only,
without changing the DTB.  It doesn't hurt to permit (define) those
other definitions in the binding, even if we agree to never use them.

					-Alex

Re: [PATCH 0/2] interconnect: qcom: Remove IP0 resource

[Bjorn Andersson]

On Tue 12 Apr 15:00 PDT 2022, Stephen Boyd wrote:

> These two patches remove the IP0 interconnect used for IPA because
> they're also present in the clk-rpmh driver. I see there are some more
> IP0 usages in the interconnect drivers, but I don't see a corresponding
> IPA clk in clk-rpmh, so I left these out. We can remove all of them if
> desired, but the sc7180 patch is most important to me as it fixes
> boot on my trogdor lazor device.
>  
> Stephen Boyd (2):
>   interconnect: qcom: sc7180: Drop IP0 interconnects
>   interconnect: qcom: sdx55: Drop IP0 interconnects
> 
>  drivers/interconnect/qcom/sc7180.c | 21 ---------------------
>  drivers/interconnect/qcom/sdx55.c  | 21 ---------------------
>  2 files changed, 42 deletions(-)
> 
> Cc: Alex Elder <elder@linaro.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Taniya Das <quic_tdas@quicinc.com>
> Cc: Mike Tipton <quic_mdtipton@quicinc.com>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> base-commit: 3123109284176b1532874591f7c81f3837bbdc17
> -- 
> https://chromeos.dev
> 

[PATCH 3/2] arm64: dts: qcom: sc7180: Remove ipa interconnect node

[Stephen Boyd]

This device node is unused now that we've removed the driver that
consumed it in the kernel. Drop the unused node to save some space.

Cc: Alex Elder <elder@linaro.org>
Cc: Taniya Das <quic_tdas@quicinc.com>
Cc: Mike Tipton <quic_mdtipton@quicinc.com>
Cc: Georgi Djakov <djakov@kernel.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 arch/arm64/boot/dts/qcom/sc7180.dtsi | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index e1c46b80f14a..1ff96ef30e3f 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -1421,13 +1421,6 @@ mmss_noc: interconnect@1740000 {
 			qcom,bcm-voters = <&apps_bcm_voter>;
 		};
 
-		ipa_virt: interconnect@1e00000 {
-			compatible = "qcom,sc7180-ipa-virt";
-			reg = <0 0x01e00000 0 0x1000>;
-			#interconnect-cells = <2>;
-			qcom,bcm-voters = <&apps_bcm_voter>;
-		};
-
 		ipa: ipa@1e40000 {
 			compatible = "qcom,sc7180-ipa";
 
-- 
https://chromeos.dev

[PATCH 4/2] ARM: dts: qcom: sdx55: Remove ipa interconnect node

[Stephen Boyd]

This device node is unused now that we've removed the driver that
consumed it in the kernel. Drop the unused node to save some space.

Cc: Alex Elder <elder@linaro.org>
Cc: Taniya Das <quic_tdas@quicinc.com>
Cc: Mike Tipton <quic_mdtipton@quicinc.com>
Cc: Georgi Djakov <djakov@kernel.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 arch/arm/boot/dts/qcom-sdx55.dtsi | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/arch/arm/boot/dts/qcom-sdx55.dtsi b/arch/arm/boot/dts/qcom-sdx55.dtsi
index d455795da44c..f1e12a112cd0 100644
--- a/arch/arm/boot/dts/qcom-sdx55.dtsi
+++ b/arch/arm/boot/dts/qcom-sdx55.dtsi
@@ -275,13 +275,6 @@ system_noc: interconnect@162c000 {
 			qcom,bcm-voters = <&apps_bcm_voter>;
 		};
 
-		ipa_virt: interconnect@1e00000 {
-			compatible = "qcom,sdx55-ipa-virt";
-			reg = <0x01e00000 0x100000>;
-			#interconnect-cells = <1>;
-			qcom,bcm-voters = <&apps_bcm_voter>;
-		};
-
 		qpic_bam: dma-controller@1b04000 {
 			compatible = "qcom,bam-v1.7.0";
 			reg = <0x01b04000 0x1c000>;
-- 
https://chromeos.dev

[PATCH 5/2] dt-bindings: interconnect: Remove sc7180/sdx55 ipa compatibles

[Stephen Boyd]

These interconnects are modeled as clks, not interconnects, therefore
remove the compatibles from the binding as they're unused.

Cc: Alex Elder <elder@linaro.org>
Cc: Taniya Das <quic_tdas@quicinc.com>
Cc: Mike Tipton <quic_mdtipton@quicinc.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---

I don't know who should apply this. Probably whoever takes the dtsi
patches, Bjorn?, because otherwise dt_bindings_check will fail.

 Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml b/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml
index 5a911be0c2ea..ab859150c7f7 100644
--- a/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml
+++ b/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml
@@ -31,7 +31,6 @@ properties:
       - qcom,sc7180-config-noc
       - qcom,sc7180-dc-noc
       - qcom,sc7180-gem-noc
-      - qcom,sc7180-ipa-virt
       - qcom,sc7180-mc-virt
       - qcom,sc7180-mmss-noc
       - qcom,sc7180-npu-noc
@@ -68,7 +67,6 @@ properties:
       - qcom,sdm845-mem-noc
       - qcom,sdm845-mmss-noc
       - qcom,sdm845-system-noc
-      - qcom,sdx55-ipa-virt
       - qcom,sdx55-mc-virt
       - qcom,sdx55-mem-noc
       - qcom,sdx55-system-noc
-- 
https://chromeos.dev

Re: [PATCH 3/2] arm64: dts: qcom: sc7180: Remove ipa interconnect node

[Alex Elder]

On 4/14/22 7:58 PM, Stephen Boyd wrote:
> This device node is unused now that we've removed the driver that
> consumed it in the kernel. Drop the unused node to save some space.
> 
> Cc: Alex Elder <elder@linaro.org>
> Cc: Taniya Das <quic_tdas@quicinc.com>
> Cc: Mike Tipton <quic_mdtipton@quicinc.com>
> Cc: Georgi Djakov <djakov@kernel.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

Looks good to me.

Reviewed-by: Alex Elder <elder@linaro.org>


> ---
>   arch/arm64/boot/dts/qcom/sc7180.dtsi | 7 -------
>   1 file changed, 7 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index e1c46b80f14a..1ff96ef30e3f 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -1421,13 +1421,6 @@ mmss_noc: interconnect@1740000 {
>   			qcom,bcm-voters = <&apps_bcm_voter>;
>   		};
>   
> -		ipa_virt: interconnect@1e00000 {
> -			compatible = "qcom,sc7180-ipa-virt";
> -			reg = <0 0x01e00000 0 0x1000>;
> -			#interconnect-cells = <2>;
> -			qcom,bcm-voters = <&apps_bcm_voter>;
> -		};
> -
>   		ipa: ipa@1e40000 {
>   			compatible = "qcom,sc7180-ipa";
>   

Re: [PATCH 4/2] ARM: dts: qcom: sdx55: Remove ipa interconnect node

[Alex Elder]

On 4/14/22 7:58 PM, Stephen Boyd wrote:
> This device node is unused now that we've removed the driver that
> consumed it in the kernel. Drop the unused node to save some space.

Look good.

Reviewed-by: Alex Elder <elder@linaro.org>

> Cc: Alex Elder <elder@linaro.org>
> Cc: Taniya Das <quic_tdas@quicinc.com>
> Cc: Mike Tipton <quic_mdtipton@quicinc.com>
> Cc: Georgi Djakov <djakov@kernel.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>   arch/arm/boot/dts/qcom-sdx55.dtsi | 7 -------
>   1 file changed, 7 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/qcom-sdx55.dtsi b/arch/arm/boot/dts/qcom-sdx55.dtsi
> index d455795da44c..f1e12a112cd0 100644
> --- a/arch/arm/boot/dts/qcom-sdx55.dtsi
> +++ b/arch/arm/boot/dts/qcom-sdx55.dtsi
> @@ -275,13 +275,6 @@ system_noc: interconnect@162c000 {
>   			qcom,bcm-voters = <&apps_bcm_voter>;
>   		};
>   
> -		ipa_virt: interconnect@1e00000 {
> -			compatible = "qcom,sdx55-ipa-virt";
> -			reg = <0x01e00000 0x100000>;
> -			#interconnect-cells = <1>;
> -			qcom,bcm-voters = <&apps_bcm_voter>;
> -		};
> -
>   		qpic_bam: dma-controller@1b04000 {
>   			compatible = "qcom,bam-v1.7.0";
>   			reg = <0x01b04000 0x1c000>;

Re: [PATCH 5/2] dt-bindings: interconnect: Remove sc7180/sdx55 ipa compatibles

[Alex Elder]

On 4/14/22 7:58 PM, Stephen Boyd wrote:
> These interconnects are modeled as clks, not interconnects, therefore
> remove the compatibles from the binding as they're unused.
> 
> Cc: Alex Elder <elder@linaro.org>
> Cc: Taniya Das <quic_tdas@quicinc.com>
> Cc: Mike Tipton <quic_mdtipton@quicinc.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

What's the proper thing to do for properties that stop being
used?  Do we delete them, like this, or deprecate them somehow?
Old DTBs might define the values that are deleted here.

Shouldn't devicetree@vger.kernel.org
  be copied on this and
the other DTS patches?

					-Alex

> ---
> 
> I don't know who should apply this. Probably whoever takes the dtsi
> patches, Bjorn?, because otherwise dt_bindings_check will fail.
> 
>   Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml b/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml
> index 5a911be0c2ea..ab859150c7f7 100644
> --- a/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml
> +++ b/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml
> @@ -31,7 +31,6 @@ properties:
>         - qcom,sc7180-config-noc
>         - qcom,sc7180-dc-noc
>         - qcom,sc7180-gem-noc
> -      - qcom,sc7180-ipa-virt
>         - qcom,sc7180-mc-virt
>         - qcom,sc7180-mmss-noc
>         - qcom,sc7180-npu-noc
> @@ -68,7 +67,6 @@ properties:
>         - qcom,sdm845-mem-noc
>         - qcom,sdm845-mmss-noc
>         - qcom,sdm845-system-noc
> -      - qcom,sdx55-ipa-virt
>         - qcom,sdx55-mc-virt
>         - qcom,sdx55-mem-noc
>         - qcom,sdx55-system-noc

Re: [PATCH 5/2] dt-bindings: interconnect: Remove sc7180/sdx55 ipa compatibles

[Stephen Boyd]

Quoting Alex Elder (2022-04-15 13:15:21)
> On 4/14/22 7:58 PM, Stephen Boyd wrote:
> > These interconnects are modeled as clks, not interconnects, therefore
> > remove the compatibles from the binding as they're unused.
> >
> > Cc: Alex Elder <elder@linaro.org>
> > Cc: Taniya Das <quic_tdas@quicinc.com>
> > Cc: Mike Tipton <quic_mdtipton@quicinc.com>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>
> What's the proper thing to do for properties that stop being
> used?  Do we delete them, like this, or deprecate them somehow?
> Old DTBs might define the values that are deleted here.

I think we leave them around until the last dts user stops using them.

>
> Shouldn't devicetree@vger.kernel.org
>   be copied on this and
> the other DTS patches?

Sure. I added it now. Thanks.

>
> > ---
> >
> > I don't know who should apply this. Probably whoever takes the dtsi
> > patches, Bjorn?, because otherwise dt_bindings_check will fail.
> >
> >   Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml | 2 --
> >   1 file changed, 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml b/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml
> > index 5a911be0c2ea..ab859150c7f7 100644
> > --- a/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml
> > +++ b/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml
> > @@ -31,7 +31,6 @@ properties:
> >         - qcom,sc7180-config-noc
> >         - qcom,sc7180-dc-noc
> >         - qcom,sc7180-gem-noc
> > -      - qcom,sc7180-ipa-virt
> >         - qcom,sc7180-mc-virt
> >         - qcom,sc7180-mmss-noc
> >         - qcom,sc7180-npu-noc
> > @@ -68,7 +67,6 @@ properties:
> >         - qcom,sdm845-mem-noc
> >         - qcom,sdm845-mmss-noc
> >         - qcom,sdm845-system-noc
> > -      - qcom,sdx55-ipa-virt
> >         - qcom,sdx55-mc-virt
> >         - qcom,sdx55-mem-noc
> >         - qcom,sdx55-system-noc
>

Re: [PATCH 3/2] arm64: dts: qcom: sc7180: Remove ipa interconnect node

[Bjorn Andersson]

On Thu 14 Apr 17:58 PDT 2022, Stephen Boyd wrote:

> This device node is unused now that we've removed the driver that
> consumed it in the kernel. Drop the unused node to save some space.
> 

I'm expecting that merging patch 3 and 4 will work, but cause sync_state
to not happen until the driver changes are merged.

Can you confirm my expectation? And perhaps confirm that it's fine for
Georgi to pick the driver changes independently of the dts changes...

Regards,
Bjorn

> Cc: Alex Elder <elder@linaro.org>
> Cc: Taniya Das <quic_tdas@quicinc.com>
> Cc: Mike Tipton <quic_mdtipton@quicinc.com>
> Cc: Georgi Djakov <djakov@kernel.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  arch/arm64/boot/dts/qcom/sc7180.dtsi | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index e1c46b80f14a..1ff96ef30e3f 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -1421,13 +1421,6 @@ mmss_noc: interconnect@1740000 {
>  			qcom,bcm-voters = <&apps_bcm_voter>;
>  		};
>  
> -		ipa_virt: interconnect@1e00000 {
> -			compatible = "qcom,sc7180-ipa-virt";
> -			reg = <0 0x01e00000 0 0x1000>;
> -			#interconnect-cells = <2>;
> -			qcom,bcm-voters = <&apps_bcm_voter>;
> -		};
> -
>  		ipa: ipa@1e40000 {
>  			compatible = "qcom,sc7180-ipa";
>  
> -- 
> https://chromeos.dev
> 

Re: [PATCH 3/2] arm64: dts: qcom: sc7180: Remove ipa interconnect node

[Georgi Djakov]

On 20.04.22 5:56, Bjorn Andersson wrote:
> On Thu 14 Apr 17:58 PDT 2022, Stephen Boyd wrote:
> 
>> This device node is unused now that we've removed the driver that
>> consumed it in the kernel. Drop the unused node to save some space.
>>
> 
> I'm expecting that merging patch 3 and 4 will work, but cause sync_state
> to not happen until the driver changes are merged.
> 
> Can you confirm my expectation? And perhaps confirm that it's fine for
> Georgi to pick the driver changes independently of the dts changes...

I have picked the driver changes, as the boot failure definitely needs to
be addressed. The sync-state might not happen until we have the DT changes
merged, as the framework is matching the count of probed drivers with the
count of providers in DT.

>> Cc: Alex Elder <elder@linaro.org>
>> Cc: Taniya Das <quic_tdas@quicinc.com>
>> Cc: Mike Tipton <quic_mdtipton@quicinc.com>
>> Cc: Georgi Djakov <djakov@kernel.org>
>> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

Acked-by: Georgi Djakov <djakov@kernel.org>

Thanks,
Georgi

>> ---
>>   arch/arm64/boot/dts/qcom/sc7180.dtsi | 7 -------
>>   1 file changed, 7 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> index e1c46b80f14a..1ff96ef30e3f 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
>> @@ -1421,13 +1421,6 @@ mmss_noc: interconnect@1740000 {
>>   			qcom,bcm-voters = <&apps_bcm_voter>;
>>   		};
>>   
>> -		ipa_virt: interconnect@1e00000 {
>> -			compatible = "qcom,sc7180-ipa-virt";
>> -			reg = <0 0x01e00000 0 0x1000>;
>> -			#interconnect-cells = <2>;
>> -			qcom,bcm-voters = <&apps_bcm_voter>;
>> -		};
>> -
>>   		ipa: ipa@1e40000 {
>>   			compatible = "qcom,sc7180-ipa";
>>   
>> -- 
>> https://chromeos.dev
>>

Re: [PATCH 3/2] arm64: dts: qcom: sc7180: Remove ipa interconnect node

[Stephen Boyd]

Quoting Georgi Djakov (2022-04-22 00:01:24)
> On 20.04.22 5:56, Bjorn Andersson wrote:
> > On Thu 14 Apr 17:58 PDT 2022, Stephen Boyd wrote:
> >
> >> This device node is unused now that we've removed the driver that
> >> consumed it in the kernel. Drop the unused node to save some space.
> >>
> >
> > I'm expecting that merging patch 3 and 4 will work, but cause sync_state
> > to not happen until the driver changes are merged.
> >
> > Can you confirm my expectation? And perhaps confirm that it's fine for
> > Georgi to pick the driver changes independently of the dts changes...

It should be OK to pick the driver changes independently of the dts.
They fix a boot up issue.

>
> I have picked the driver changes, as the boot failure definitely needs to
> be addressed. The sync-state might not happen until we have the DT changes
> merged, as the framework is matching the count of probed drivers with the
> count of providers in DT.

Indeed. The DT change is required to actually have sync-state happen
when the driver changes are merged. Without the DT change I'm not able
to enter suspend. I didn't notice this earlier, ugh.

This means that the DTS change needs to be backported to fix suspend,
because otherwise the _other_ interconnects that aren't IPA keep the
initial sync-state request forever, waiting for the IPA provider in DT
to be probed by a driver that doesn't exist. One solution is to have the
DT change, which makes the probed driver and provider counts match.

Maybe a better solution is to ignore these compatibles in the provider
count? That way we aren't required to backport a DTS change and
everything is still contained to driver code.

----8<----
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 80ed03f4dfd0..bfa6788afca1 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -1087,9 +1087,15 @@ static int of_count_icc_providers(struct device_node *np)
 {
 	struct device_node *child;
 	int count = 0;
+	const struct of_device_id ignore_list[] = {
+		{ .compatible = "qcom,sc7180-ipa-virt", },
+		{ .compatible = "qcom,sdx55-ipa-virt", },
+		{}
+	};

 	for_each_available_child_of_node(np, child) {
-		if (of_property_read_bool(child, "#interconnect-cells"))
+		if (of_property_read_bool(child, "#interconnect-cells") &&
+		    likely(!of_match_node(ignore_list, child)))
 			count++;
 		count += of_count_icc_providers(child);
 	}

Re: (subset) [PATCH 3/2] arm64: dts: qcom: sc7180: Remove ipa interconnect node

[Bjorn Andersson]

On Thu, 14 Apr 2022 17:58:26 -0700, Stephen Boyd wrote:
> This device node is unused now that we've removed the driver that
> consumed it in the kernel. Drop the unused node to save some space.
> 
> 

Applied, thanks!

[3/3] arm64: dts: qcom: sc7180: Remove ipa interconnect node
      commit: 067bc653b85e466048914c48e46659a50a907fa6

Best regards,
-- 
Bjorn Andersson <bjorn.andersson@linaro.org>

Re: (subset) [PATCH 4/2] ARM: dts: qcom: sdx55: Remove ipa interconnect node

[Bjorn Andersson]

On Thu, 14 Apr 2022 17:58:27 -0700, Stephen Boyd wrote:
> This device node is unused now that we've removed the driver that
> consumed it in the kernel. Drop the unused node to save some space.
> 
> 

Applied, thanks!

[4/4] ARM: dts: qcom: sdx55: Remove ipa interconnect node
      commit: 02fbeb1649534d564b100a994b05d4eaa167ea19

Best regards,
-- 
Bjorn Andersson <bjorn.andersson@linaro.org>

Re: [PATCH 5/2] dt-bindings: interconnect: Remove sc7180/sdx55 ipa compatibles

[Alex Elder]

On 4/14/22 7:58 PM, Stephen Boyd wrote:
> These interconnects are modeled as clks, not interconnects, therefore
> remove the compatibles from the binding as they're unused.
> 
> Cc: Alex Elder <elder@linaro.org>
> Cc: Taniya Das <quic_tdas@quicinc.com>
> Cc: Mike Tipton <quic_mdtipton@quicinc.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> 
> I don't know who should apply this. Probably whoever takes the dtsi
> patches, Bjorn?, because otherwise dt_bindings_check will fail.

I don't see this commit applied anywhere, though I
might have missed it.  Is this for Bjorn, or Georgi,
or someone else?

					-Alex

>   Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml b/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml
> index 5a911be0c2ea..ab859150c7f7 100644
> --- a/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml
> +++ b/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml
> @@ -31,7 +31,6 @@ properties:
>         - qcom,sc7180-config-noc
>         - qcom,sc7180-dc-noc
>         - qcom,sc7180-gem-noc
> -      - qcom,sc7180-ipa-virt
>         - qcom,sc7180-mc-virt
>         - qcom,sc7180-mmss-noc
>         - qcom,sc7180-npu-noc
> @@ -68,7 +67,6 @@ properties:
>         - qcom,sdm845-mem-noc
>         - qcom,sdm845-mmss-noc
>         - qcom,sdm845-system-noc
> -      - qcom,sdx55-ipa-virt
>         - qcom,sdx55-mc-virt
>         - qcom,sdx55-mem-noc
>         - qcom,sdx55-system-noc

Re: [PATCH 5/2] dt-bindings: interconnect: Remove sc7180/sdx55 ipa compatibles

[Georgi Djakov]

On 17.05.22 1:16, Alex Elder wrote:
> On 4/14/22 7:58 PM, Stephen Boyd wrote:
>> These interconnects are modeled as clks, not interconnects, therefore
>> remove the compatibles from the binding as they're unused.
>>
>> Cc: Alex Elder <elder@linaro.org>
>> Cc: Taniya Das <quic_tdas@quicinc.com>
>> Cc: Mike Tipton <quic_mdtipton@quicinc.com>
>> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>> ---
>>
>> I don't know who should apply this. Probably whoever takes the dtsi
>> patches, Bjorn?, because otherwise dt_bindings_check will fail.
> 
> I don't see this commit applied anywhere, though I
> might have missed it.  Is this for Bjorn, or Georgi,
> or someone else?

I merged it as Bjorn has already sent his pull request.

Thanks,
Georgi

> 
>                      -Alex
> 
>>   Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml 
>> b/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml
>> index 5a911be0c2ea..ab859150c7f7 100644
>> --- a/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml
>> +++ b/Documentation/devicetree/bindings/interconnect/qcom,rpmh.yaml
>> @@ -31,7 +31,6 @@ properties:
>>         - qcom,sc7180-config-noc
>>         - qcom,sc7180-dc-noc
>>         - qcom,sc7180-gem-noc
>> -      - qcom,sc7180-ipa-virt
>>         - qcom,sc7180-mc-virt
>>         - qcom,sc7180-mmss-noc
>>         - qcom,sc7180-npu-noc
>> @@ -68,7 +67,6 @@ properties:
>>         - qcom,sdm845-mem-noc
>>         - qcom,sdm845-mmss-noc
>>         - qcom,sdm845-system-noc
>> -      - qcom,sdx55-ipa-virt
>>         - qcom,sdx55-mc-virt
>>         - qcom,sdx55-mem-noc
>>         - qcom,sdx55-system-noc
>