On Mon 02 Sep 14:19 PDT 2019, Brian Masney wrote:
> Add driver for the Qualcomm MSM8974 interconnect providers that support
> setting system bandwidth requirements between various network-on-chip
> fabrics.
>
> I marked this as a PATCH RFC since I'm not able to write to all of the
> master IDs with qcom_icc_rpm_smd_send(). I included tables below that
> shows which of the 20 master IDs that I'm able to activate with
> qcom_icc_rpm_smd_send() [1] and the remaining 37 where
> qcom_icc_rpm_smd_send() fails with -ENXIO [2].
>
> The device tree snippets that I'm using is in patch 1 of this series,
> however the relevant interconnect properties for the mdp5 are:
>
> interconnects = <&mmssnoc MNOC_MAS_GRAPHICS_3D &bimc BIMC_SLV_EBI_CH0>,
> <&ocmemnoc OCMEM_VNOC_MAS_GFX3D &ocmemnoc
> OCMEM_SLV_OCMEM>;
> interconnect-names = "mdp0-mem", "mdp1-mem";
>
> The two interconnects have the following paths:
>
> - mdp0-mem
> - mas_graphics_3d
> - mnoc_to_bimc
> - bimc_to_mnoc
> - slv_ebi_ch0
>
> - mdp1-mem
> - mas_v_ocmem_gfx3d
> - ocmem_vnoc_to_onoc
> - ocmem_noc_to_ocmem_vnoc
> - slv_ocmem
>
> ocmem_noc_to_ocmem_vnoc is the only one that is successfully activated
> and the remaining fail in these two paths.
>
> With this interconnect driver, I am able to remove a clock hack that I
> had in place that set the speed of MDSS_AXI_CLK high and I'm able to use
> kmscube. However, I do see a clock warning on system startup [3].
>
> The display doesn't work for me with the downstream MSM sources so I may
> have to get that working to debug this some more unless anyone has any
> suggestions. I verified that the downstream msm8974 sources are using
> rpm smd to setup the interconnects for the msm bus:
> https://github.com/AICP/kernel_lge_hammerhead/blob/n7.1/arch/arm/mach-msm/msm_bus/msm_bus_rpm_smd.c#L153
> The only difference I noticed is that that upstream uses a 32 bit field
> for the 'value' field in drivers/interconnect/qcom/smd-rpm.c for
> qcom_icc_rpm_smd_send(), and downstream uses a 64 bit value instead.
> Changing that upstream doesn't make a difference.
>
> Downstream msm8974 msm bus code:
> https://github.com/AICP/kernel_lge_hammerhead/blob/n7.1/arch/arm/mach-msm/msm_bus/msm_bus_board_8974.c
>
> [1] Master IDs that I'm able to activate with qcom_icc_rpm_smd_send():
>
> HW ID Name
> -------------------------------
> 3 mas_mss_proc
> 18 slv_tcsr
> 21 slv_crypto_1_cfg
> 22 slv_imem_cfg
> 25 slv_boot_rom
> 29 slv_mpm
> 33 cnoc_to_snoc
> 34 slv_cnoc_onoc_cfg
> 35 slv_cnoc_mnoc_mmss_cfg
> 36 slv_cnoc_mnoc_cfg
> 37 slv_pnoc_cfg
> 38 slv_snoc_mpu_cfg
> 39 slv_snoc_cfg
> 40 slv_ebi1_dll_cfg
> 41 slv_phy_apu_cfg
> 42 slv_ebi1_phy_cfg
> 43 slv_rpm
> 44 slv_service_cnoc
> 52 mnoc_to_bimc
> 54 slv_display_cfg
>
> [2] Master IDs where qcom_icc_rpm_smd_send() fails with -ENXIO. The
> -ENXIO error comes from qcom_smd_rpm_callback() in
>
> https://elixir.bootlin.com/linux/latest/source/drivers/soc/qcom/smd-rpm.c#L179
>
> HW ID Name
> -------------------------------
> 1 mas_ampss_m0
> 2 mas_ampss_m1
> 4 bimc_to_mnoc
> 5 bimc_to_snoc
> 6 slv_ebi_ch0
> 7 slv_ampss_l2
> 8 mas_rpm_inst
> 9 mas_rpm_data
> 10 mas_rpm_sys
> 11 mas_dehr
> 12 mas_qdss_dap
> 13 mas_spdm
> 14 mas_tic
> 15 slv_clk_ctl
> 16 slv_cnoc_mss
> 17 slv_security
> 19 slv_tlmm
> 20 slv_crypto_0_cfg
> 23 slv_message_ram
> 24 slv_bimc_cfg
> 26 slv_pmic_arb
> 27 slv_spdm_wrapper
> 28 slv_dehr_cfg
> 30 slv_qdss_cfg
> 31 slv_rbcpr_cfg
> 32 slv_rbcpr_qdss_apu_cfg
> 45 mas_graphics_3d
> 46 mas_jpeg
> 47 mas_mdp_port0
> 48 mas_video_p0
> 49 mas_video_p1
> 50 mas_vfe
> 51 mnoc_to_cnoc
> 53 slv_camera_cfg
> 55 slv_ocmem_cfg
> 56 slv_cpr_cfg
> 57 slv_cpr_xpu_cfg
>
> [3] Clock warning on system startup:
>
> WARNING: CPU: 3 PID: 26 at drivers/clk/qcom/clk-rcg2.c:121
> update_config+0xe8/0xf0
> gfx3d_clk_src: rcg didn't update its configuration.
> Modules linked in: msm qcom_spmi_vadc qcom_vadc_common pm8941_pwrkey
> qcom_spmi_iadc msm_vibrator brcmfmac qnoc_msm8974 icc_core inv_mpu6050_i2c(+)
> inv_mpu6050 brcmutil cfg80211 bq24190_charger tsl2772 rmi_i2c rmi_core
> icc_smd_rpm usb_f_rndis dm_mod
> CPU: 3 PID: 26 Comm: kworker/3:0 Not tainted
> 5.3.0-rc4-next-20190816-00021-gd27e1e778708-dirty #148
> Hardware name: Generic DT based system
> Workqueue: events deferred_probe_work_func
> [<c0312328>] (unwind_backtrace) from [<c030d618>] (show_stack+0x10/0x14)
> [<c030d618>] (show_stack) from [<c0a8f8e4>] (dump_stack+0x78/0x8c)
> [<c0a8f8e4>] (dump_stack) from [<c0321b64>] (__warn.part.0+0xb8/0xd4)
> [<c0321b64>] (__warn.part.0) from [<c0321be8>]
> (warn_slowpath_fmt+0x68/0x94)
> [<c0321be8>] (warn_slowpath_fmt) from [<c07262cc>]
> (update_config+0xe8/0xf0)
> [<c07262cc>] (update_config) from [<c071d1cc>]
> (clk_change_rate+0x9c/0x594)
> [<c071d1cc>] (clk_change_rate) from [<c071d850>]
> (clk_core_set_rate_nolock+0x18c/0x1d4)
> [<c071d850>] (clk_core_set_rate_nolock) from [<c071d8c8>]
> (clk_set_rate+0x30/0x88)
> [<c071d8c8>] (clk_set_rate) from [<bf16edf8>]
> (msm_gpu_pm_resume+0x104/0x168 [msm])
> [<bf16edf8>] (msm_gpu_pm_resume [msm]) from [<c07d7c50>]
> (genpd_runtime_resume+0x9c/0x2a4)
> [<c07d7c50>] (genpd_runtime_resume) from [<c07cd344>]
> (__rpm_callback+0x74/0x12c)
> [<c07cd344>] (__rpm_callback) from [<c07cd41c>] (rpm_callback+0x20/0x80)
> [<c07cd41c>] (rpm_callback) from [<c07ccf98>] (rpm_resume+0x640/0x814)
> [<c07ccf98>] (rpm_resume) from [<c07cd1bc>]
> (__pm_runtime_resume+0x50/0x68)
> [<c07cd1bc>] (__pm_runtime_resume) from [<bf11e498>]
> (adreno_load_gpu+0x50/0x154 [msm])
> [<bf11e498>] (adreno_load_gpu [msm]) from [<bf1690d4>]
> (msm_open+0x80/0x94 [msm])
> [<bf1690d4>] (msm_open [msm]) from [<c078b32c>]
> (drm_file_alloc+0x138/0x1f0)
> [<c078b32c>] (drm_file_alloc) from [<c07aebdc>]
> (drm_client_init+0xa8/0x124)
> [<c07aebdc>] (drm_client_init) from [<c0789700>]
> (drm_fb_helper_init.part.0+0x30/0x3c)
> [<c0789700>] (drm_fb_helper_init.part.0) from [<bf1740fc>]
> (msm_fbdev_init+0x4c/0xb4 [msm])
> [<bf1740fc>] (msm_fbdev_init [msm]) from [<bf169c90>]
> (msm_drm_bind+0x5d4/0x654 [msm])
> [<bf169c90>] (msm_drm_bind [msm]) from [<c07b98b0>]
> (try_to_bring_up_master+0x1f8/0x2b4)
> [<c07b98b0>] (try_to_bring_up_master) from [<c07b9a1c>]
> (__component_add+0xb0/0x174)
> [<c07b9a1c>] (__component_add) from [<c07c29e8>]
> (platform_drv_probe+0x48/0x98)
> [<c07c29e8>] (platform_drv_probe) from [<c07c0578>]
> (really_probe+0x24c/0x480)
> [<c07c0578>] (really_probe) from [<c07c09a0>]
> (driver_probe_device+0xa0/0x1f8)
> [<c07c09a0>] (driver_probe_device) from [<c07be5ec>]
> (bus_for_each_drv+0x84/0xd0)
> [<c07be5ec>] (bus_for_each_drv) from [<c07c028c>]
> (__device_attach+0xe0/0x178)
> [<c07c028c>] (__device_attach) from [<c07bf3c4>]
> (bus_probe_device+0x84/0x8c)
> [<c07bf3c4>] (bus_probe_device) from [<c07bf91c>]
> (deferred_probe_work_func+0x84/0xc4)
> [<c07bf91c>] (deferred_probe_work_func) from [<c033d2a8>]
> (process_one_work+0x1dc/0x538)
> [<c033d2a8>] (process_one_work) from [<c033ebf8>]
> (worker_thread+0x44/0x508)
> [<c033ebf8>] (worker_thread) from [<c0343370>] (kthread+0x120/0x150)
> [<c0343370>] (kthread) from [<c03010e8>] (ret_from_fork+0x14/0x2c)
I haven't checked the docs, but we see pretty much the same problem on
subsequent platforms if things are now powered properly. And there's a
OXILI gdsc (power domain) which is fed by pm8841_s4 corner, according to
the downstream DT.
[..]
> diff --git a/drivers/interconnect/qcom/msm8974.c
> b/drivers/interconnect/qcom/msm8974.c
[..]
> +DEFINE_QNODE(mas_ampss_m0, MSM8974_BIMC_MAS_AMPSS_M0, 8, 0, -1);
> +DEFINE_QNODE(mas_ampss_m1, MSM8974_BIMC_MAS_AMPSS_M1, 8, 0, -1);
> +DEFINE_QNODE(mas_mss_proc, MSM8974_BIMC_MAS_MSS_PROC, 8, 1, -1);
> +DEFINE_QNODE(bimc_to_mnoc, MSM8974_BIMC_TO_MNOC, 8, 2, -1,
> + MSM8974_BIMC_SLV_EBI_CH0);
None of these looks excessive, so please ignore the 80-char rule to
improve readability.
> +DEFINE_QNODE(bimc_to_snoc, MSM8974_BIMC_TO_SNOC, 8, 3, 2,
> + MSM8974_SNOC_TO_BIMC, MSM8974_BIMC_SLV_EBI_CH0,
> + MSM8974_BIMC_MAS_AMPSS_M0);
> +DEFINE_QNODE(slv_ebi_ch0, MSM8974_BIMC_SLV_EBI_CH0, 8, -1, 0);
> +DEFINE_QNODE(slv_ampss_l2, MSM8974_BIMC_SLV_AMPSS_L2, 8, -1, 1);
> +
[..]
> +static int qnoc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + const struct qcom_icc_desc *desc;
> + struct icc_onecell_data *data;
> + struct icc_provider *provider;
> + struct qcom_icc_node **qnodes;
> + struct qcom_icc_provider *qp;
> + struct icc_node *node;
> + size_t num_nodes, i;
> + int ret;
> +
> + /* wait for the RPM proxy */
> + if (!qcom_icc_rpm_smd_available())
> + return -EPROBE_DEFER;
> +
> + desc = of_device_get_match_data(dev);
> + if (!desc)
> + return -EINVAL;
> +
> + qnodes = desc->nodes;
> + num_nodes = desc->num_nodes;
> +
> + qp = devm_kzalloc(dev, sizeof(*qp), GFP_KERNEL);
> + if (!qp)
> + return -ENOMEM;
> +
> + data = devm_kcalloc(dev, num_nodes, sizeof(*node), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + qp->bus_clks = devm_kmemdup(dev, bus_clocks, sizeof(bus_clocks),
> + GFP_KERNEL);
> + if (!qp->bus_clks)
> + return -ENOMEM;
> +
> + qp->num_clks = ARRAY_SIZE(bus_clocks);
> + ret = devm_clk_bulk_get(dev, qp->num_clks, qp->bus_clks);
> + if (ret)
> + return ret;
> +
> + ret = clk_bulk_prepare_enable(qp->num_clks, qp->bus_clks);
We should be able to turn these off when there are no active/enabled
paths passing by this particular bus. But let's postpone that problem.
> + if (ret)
> + return ret;
> +
> + provider = &qp->provider;
> + INIT_LIST_HEAD(&provider->nodes);
> + provider->dev = dev;
> + provider->set = qcom_icc_set;
> + provider->aggregate = qcom_icc_aggregate;
> + provider->xlate = of_icc_xlate_onecell;
> + provider->data = data;
> +
> + ret = icc_provider_add(provider);
> + if (ret) {
> + dev_err(dev, "error adding interconnect provider: %d\n", ret);
> + clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
Flip the order of clk_bulk_disable_unprepare() and icc_provider_del() in
the error path, inject a new label for the clk diable and replace this
with a goto err_disable_clks;
> + return ret;
> + }
> +
> + for (i = 0; i < num_nodes; i++) {
> + size_t j;
> +
> + node = icc_node_create(qnodes[i]->id);
> + if (IS_ERR(node)) {
> + ret = PTR_ERR(node);
> + goto err;
> + }
> +
> + node->name = qnodes[i]->name;
> + node->data = qnodes[i];
> + icc_node_add(node, provider);
> +
> + dev_dbg(dev, "registered node %s\n", node->name);
> +
> + /* populate links */
> + for (j = 0; j < qnodes[i]->num_links; j++)
> + icc_link_create(node, qnodes[i]->links[j]);
> +
> + data->nodes[i] = node;
> + }
> + data->num_nodes = num_nodes;
> +
> + platform_set_drvdata(pdev, qp);
> +
> + return 0;
> +err:
> + list_for_each_entry(node, &provider->nodes, node_list) {
> + icc_node_del(node);
> + icc_node_destroy(node->id);
> + }
> + clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
> + icc_provider_del(provider);
> +
> + return ret;
> +}
> +
> +static int qnoc_remove(struct platform_device *pdev)
> +{
> + struct qcom_icc_provider *qp = platform_get_drvdata(pdev);
> + struct icc_provider *provider = &qp->provider;
> + struct icc_node *n;
> +
> + list_for_each_entry(n, &provider->nodes, node_list) {
> + icc_node_del(n);
> + icc_node_destroy(n->id);
> + }
> + clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
> +
> + return icc_provider_del(provider);
It's nice when remove unrolls probe, so please flip the order of clk
disable and provider_del.
Regards,
Bjorn
> +}
> +
> +static const struct of_device_id msm8974_noc_of_match[] = {
> + { .compatible = "qcom,msm8974-bimc", .data = &msm8974_bimc},
> + { .compatible = "qcom,msm8974-cnoc", .data = &msm8974_cnoc},
> + { .compatible = "qcom,msm8974-mmssnoc", .data = &msm8974_mnoc},
> + { .compatible = "qcom,msm8974-ocmemnoc", .data = &msm8974_onoc},
> + { .compatible = "qcom,msm8974-pnoc", .data = &msm8974_pnoc},
> + { .compatible = "qcom,msm8974-snoc", .data = &msm8974_snoc},
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, msm8974_noc_of_match);
> +
> +static struct platform_driver msm8974_noc_driver = {
> + .probe = qnoc_probe,
> + .remove = qnoc_remove,
> + .driver = {
> + .name = "qnoc-msm8974",
> + .of_match_table = msm8974_noc_of_match,
> + },
> +};
> +module_platform_driver(msm8974_noc_driver);
> +MODULE_DESCRIPTION("Qualcomm MSM8974 NoC driver");
> +MODULE_AUTHOR("Brian Masney <masn...@onstation.org>");
> +MODULE_LICENSE("GPL v2");
> --
> 2.21.0
>
