Message-ID: <282345e7-fa89-87b2-309d-15d0b0350900@linaro.org>
Date:   Wed, 16 Nov 2022 18:32:40 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Abel Vesa <abel.vesa@...aro.org>, Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Georgi Djakov <djakov@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Rob Herring <robh@...nel.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        devicetree@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-pm@...r.kernel.org
Subject: Re: [PATCH 2/2] interconnect: qcom: Add SM8550 interconnect provider
 driver

On 16/11/2022 12:45, Abel Vesa wrote:
> Add driver for the Qualcomm interconnect buses found in SM8550 based
> platforms. The topology consists of several NoCs that are controlled by
> a remote processor that collects the aggregated bandwidth for each
> master-slave pairs.
> 
> Signed-off-by: Abel Vesa <abel.vesa@...aro.org>

Thank you for your patch. There is something to discuss/improve.



> +static struct qcom_icc_node * const config_noc_nodes[] = {
> +	[MASTER_CNOC_CFG] = &qsm_cfg,
> +	[SLAVE_AHB2PHY_SOUTH] = &qhs_ahb2phy0,
> +	[SLAVE_AHB2PHY_NORTH] = &qhs_ahb2phy1,
> +	[SLAVE_APPSS] = &qhs_apss,
> +	[SLAVE_CAMERA_CFG] = &qhs_camera_cfg,
> +	[SLAVE_CLK_CTL] = &qhs_clk_ctl,
> +	[SLAVE_RBCPR_CX_CFG] = &qhs_cpr_cx,
> +	[SLAVE_RBCPR_MMCX_CFG] = &qhs_cpr_mmcx,
> +	[SLAVE_RBCPR_MXA_CFG] = &qhs_cpr_mxa,
> +	[SLAVE_RBCPR_MXC_CFG] = &qhs_cpr_mxc,
> +	[SLAVE_CPR_NSPCX] = &qhs_cpr_nspcx,
> +	[SLAVE_CRYPTO_0_CFG] = &qhs_crypto0_cfg,
> +	[SLAVE_CX_RDPM] = &qhs_cx_rdpm,
> +	[SLAVE_DISPLAY_CFG] = &qhs_display_cfg,
> +	[SLAVE_GFX3D_CFG] = &qhs_gpuss_cfg,
> +	[SLAVE_I2C] = &qhs_i2c,
> +	[SLAVE_IMEM_CFG] = &qhs_imem_cfg,
> +	[SLAVE_IPA_CFG] = &qhs_ipa,
> +	[SLAVE_IPC_ROUTER_CFG] = &qhs_ipc_router,
> +	[SLAVE_CNOC_MSS] = &qhs_mss_cfg,
> +	[SLAVE_MX_RDPM] = &qhs_mx_rdpm,
> +	[SLAVE_PCIE_0_CFG] = &qhs_pcie0_cfg,
> +	[SLAVE_PCIE_1_CFG] = &qhs_pcie1_cfg,
> +	[SLAVE_PDM] = &qhs_pdm,
> +	[SLAVE_PIMEM_CFG] = &qhs_pimem_cfg,
> +	[SLAVE_PRNG] = &qhs_prng,
> +	[SLAVE_QDSS_CFG] = &qhs_qdss_cfg,
> +	[SLAVE_QSPI_0] = &qhs_qspi,
> +	[SLAVE_QUP_1] = &qhs_qup1,
> +	[SLAVE_QUP_2] = &qhs_qup2,
> +	[SLAVE_SDCC_2] = &qhs_sdc2,
> +	[SLAVE_SDCC_4] = &qhs_sdc4,
> +	[SLAVE_SPSS_CFG] = &qhs_spss_cfg,
> +	[SLAVE_TCSR] = &qhs_tcsr,
> +	[SLAVE_TLMM] = &qhs_tlmm,
> +	[SLAVE_UFS_MEM_CFG] = &qhs_ufs_mem_cfg,
> +	[SLAVE_USB3_0] = &qhs_usb3_0,
> +	[SLAVE_VENUS_CFG] = &qhs_venus_cfg,
> +	[SLAVE_VSENSE_CTRL_CFG] = &qhs_vsense_ctrl_cfg,
> +	[SLAVE_LPASS_QTB_CFG] = &qss_lpass_qtb_cfg,
> +	[SLAVE_CNOC_MNOC_CFG] = &qss_mnoc_cfg,
> +	[SLAVE_NSP_QTB_CFG] = &qss_nsp_qtb_cfg,
> +	[SLAVE_PCIE_ANOC_CFG] = &qss_pcie_anoc_cfg,
> +	[SLAVE_QDSS_STM] = &xs_qdss_stm,
> +	[SLAVE_TCU] = &xs_sys_tcu_cfg,
> +};
> +
> +static struct qcom_icc_desc sm8550_config_noc = {
> +	.nodes = config_noc_nodes,
> +	.num_nodes = ARRAY_SIZE(config_noc_nodes),
> +	.bcms = config_noc_bcms,
> +	.num_bcms = ARRAY_SIZE(config_noc_bcms),
> +};
> +
> +static struct qcom_icc_bcm *cnoc_main_bcms[] = {

Const pointers. Also in several other places as well.

> +	&bcm_cn0,
> +};
> +
> +static struct qcom_icc_node *cnoc_main_nodes[] = {
> +	[MASTER_GEM_NOC_CNOC] = &qnm_gemnoc_cnoc,
> +	[MASTER_GEM_NOC_PCIE_SNOC] = &qnm_gemnoc_pcie,
> +	[SLAVE_AOSS] = &qhs_aoss,
> +	[SLAVE_TME_CFG] = &qhs_tme_cfg,
> +	[SLAVE_CNOC_CFG] = &qss_cfg,
> +	[SLAVE_DDRSS_CFG] = &qss_ddrss_cfg,
> +	[SLAVE_BOOT_IMEM] = &qxs_boot_imem,
> +	[SLAVE_IMEM] = &qxs_imem,
> +	[SLAVE_PCIE_0] = &xs_pcie_0,
> +	[SLAVE_PCIE_1] = &xs_pcie_1,
> +};
> +
> +static struct qcom_icc_desc sm8550_cnoc_main = {

This should be const.

> +	.nodes = cnoc_main_nodes,
> +	.num_nodes = ARRAY_SIZE(cnoc_main_nodes),
> +	.bcms = cnoc_main_bcms,
> +	.num_bcms = ARRAY_SIZE(cnoc_main_bcms),
> +};
> +
> +static struct qcom_icc_bcm *gem_noc_bcms[] = {
> +	&bcm_sh0,
> +	&bcm_sh1,
> +	&bcm_sh0_disp,
> +	&bcm_sh1_disp,
> +	&bcm_sh0_cam_ife_0,
> +	&bcm_sh1_cam_ife_0,
> +	&bcm_sh0_cam_ife_1,
> +	&bcm_sh1_cam_ife_1,
> +	&bcm_sh0_cam_ife_2,
> +	&bcm_sh1_cam_ife_2,
> +};
> +
> +static struct qcom_icc_node *gem_noc_nodes[] = {
> +	[MASTER_GPU_TCU] = &alm_gpu_tcu,
> +	[MASTER_SYS_TCU] = &alm_sys_tcu,
> +	[MASTER_APPSS_PROC] = &chm_apps,
> +	[MASTER_GFX3D] = &qnm_gpu,
> +	[MASTER_LPASS_GEM_NOC] = &qnm_lpass_gemnoc,
> +	[MASTER_MSS_PROC] = &qnm_mdsp,
> +	[MASTER_MNOC_HF_MEM_NOC] = &qnm_mnoc_hf,
> +	[MASTER_MNOC_SF_MEM_NOC] = &qnm_mnoc_sf,
> +	[MASTER_COMPUTE_NOC] = &qnm_nsp_gemnoc,
> +	[MASTER_ANOC_PCIE_GEM_NOC] = &qnm_pcie,
> +	[MASTER_SNOC_GC_MEM_NOC] = &qnm_snoc_gc,
> +	[MASTER_SNOC_SF_MEM_NOC] = &qnm_snoc_sf,
> +	[SLAVE_GEM_NOC_CNOC] = &qns_gem_noc_cnoc,
> +	[SLAVE_LLCC] = &qns_llcc,
> +	[SLAVE_MEM_NOC_PCIE_SNOC] = &qns_pcie,
> +	[MASTER_MNOC_HF_MEM_NOC_DISP] = &qnm_mnoc_hf_disp,
> +	[MASTER_ANOC_PCIE_GEM_NOC_DISP] = &qnm_pcie_disp,
> +	[SLAVE_LLCC_DISP] = &qns_llcc_disp,
> +	[MASTER_MNOC_HF_MEM_NOC_CAM_IFE_0] = &qnm_mnoc_hf_cam_ife_0,
> +	[MASTER_MNOC_SF_MEM_NOC_CAM_IFE_0] = &qnm_mnoc_sf_cam_ife_0,
> +	[MASTER_ANOC_PCIE_GEM_NOC_CAM_IFE_0] = &qnm_pcie_cam_ife_0,
> +	[SLAVE_LLCC_CAM_IFE_0] = &qns_llcc_cam_ife_0,
> +	[MASTER_MNOC_HF_MEM_NOC_CAM_IFE_1] = &qnm_mnoc_hf_cam_ife_1,
> +	[MASTER_MNOC_SF_MEM_NOC_CAM_IFE_1] = &qnm_mnoc_sf_cam_ife_1,
> +	[MASTER_ANOC_PCIE_GEM_NOC_CAM_IFE_1] = &qnm_pcie_cam_ife_1,
> +	[SLAVE_LLCC_CAM_IFE_1] = &qns_llcc_cam_ife_1,
> +	[MASTER_MNOC_HF_MEM_NOC_CAM_IFE_2] = &qnm_mnoc_hf_cam_ife_2,
> +	[MASTER_MNOC_SF_MEM_NOC_CAM_IFE_2] = &qnm_mnoc_sf_cam_ife_2,
> +	[MASTER_ANOC_PCIE_GEM_NOC_CAM_IFE_2] = &qnm_pcie_cam_ife_2,
> +	[SLAVE_LLCC_CAM_IFE_2] = &qns_llcc_cam_ife_2,
> +};
> +
> +static struct qcom_icc_desc sm8550_gem_noc = {

Const... and so on - multiple places.

> +	.nodes = gem_noc_nodes,
> +	.num_nodes = ARRAY_SIZE(gem_noc_nodes),
> +	.bcms = gem_noc_bcms,
> +	.num_bcms = ARRAY_SIZE(gem_noc_bcms),
> +};
> +
> +static struct qcom_icc_bcm * const lpass_ag_noc_bcms[] = {
> +};
> +
> +static struct qcom_icc_node * const lpass_ag_noc_nodes[] = {
> +	[MASTER_LPIAON_NOC] = &qnm_lpiaon_noc,
> +	[SLAVE_LPASS_GEM_NOC] = &qns_lpass_ag_noc_gemnoc,
> +};
> +
> +static const struct qcom_icc_desc sm8550_lpass_ag_noc = {
> +	.nodes = lpass_ag_noc_nodes,
> +	.num_nodes = ARRAY_SIZE(lpass_ag_noc_nodes),
> +	.bcms = lpass_ag_noc_bcms,
> +	.num_bcms = ARRAY_SIZE(lpass_ag_noc_bcms),
> +};
> +
> +static struct qcom_icc_bcm * const lpass_lpiaon_noc_bcms[] = {
> +	&bcm_lp0,
> +};
> +
> +static struct qcom_icc_node * const lpass_lpiaon_noc_nodes[] = {
> +	[MASTER_LPASS_LPINOC] = &qnm_lpass_lpinoc,
> +	[SLAVE_LPIAON_NOC_LPASS_AG_NOC] = &qns_lpass_aggnoc,
> +};
> +
> +static const struct qcom_icc_desc sm8550_lpass_lpiaon_noc = {
> +	.nodes = lpass_lpiaon_noc_nodes,
> +	.num_nodes = ARRAY_SIZE(lpass_lpiaon_noc_nodes),
> +	.bcms = lpass_lpiaon_noc_bcms,
> +	.num_bcms = ARRAY_SIZE(lpass_lpiaon_noc_bcms),
> +};
> +
> +static struct qcom_icc_bcm * const lpass_lpicx_noc_bcms[] = {
> +};
> +
> +static struct qcom_icc_node * const lpass_lpicx_noc_nodes[] = {
> +	[MASTER_LPASS_PROC] = &qxm_lpinoc_dsp_axim,
> +	[SLAVE_LPICX_NOC_LPIAON_NOC] = &qns_lpi_aon_noc,
> +};
> +
> +static const struct qcom_icc_desc sm8550_lpass_lpicx_noc = {
> +	.nodes = lpass_lpicx_noc_nodes,
> +	.num_nodes = ARRAY_SIZE(lpass_lpicx_noc_nodes),
> +	.bcms = lpass_lpicx_noc_bcms,
> +	.num_bcms = ARRAY_SIZE(lpass_lpicx_noc_bcms),
> +};
> +
> +static struct qcom_icc_bcm * const mc_virt_bcms[] = {
> +	&bcm_acv,
> +	&bcm_mc0,
> +	&bcm_acv_disp,
> +	&bcm_mc0_disp,
> +	&bcm_acv_cam_ife_0,
> +	&bcm_mc0_cam_ife_0,
> +	&bcm_acv_cam_ife_1,
> +	&bcm_mc0_cam_ife_1,
> +	&bcm_acv_cam_ife_2,
> +	&bcm_mc0_cam_ife_2,
> +};
> +
> +static struct qcom_icc_node * const mc_virt_nodes[] = {
> +	[MASTER_LLCC] = &llcc_mc,
> +	[SLAVE_EBI1] = &ebi,
> +	[MASTER_LLCC_DISP] = &llcc_mc_disp,
> +	[SLAVE_EBI1_DISP] = &ebi_disp,
> +	[MASTER_LLCC_CAM_IFE_0] = &llcc_mc_cam_ife_0,
> +	[SLAVE_EBI1_CAM_IFE_0] = &ebi_cam_ife_0,
> +	[MASTER_LLCC_CAM_IFE_1] = &llcc_mc_cam_ife_1,
> +	[SLAVE_EBI1_CAM_IFE_1] = &ebi_cam_ife_1,
> +	[MASTER_LLCC_CAM_IFE_2] = &llcc_mc_cam_ife_2,
> +	[SLAVE_EBI1_CAM_IFE_2] = &ebi_cam_ife_2,
> +};
> +
> +static const struct qcom_icc_desc sm8550_mc_virt = {
> +	.nodes = mc_virt_nodes,
> +	.num_nodes = ARRAY_SIZE(mc_virt_nodes),
> +	.bcms = mc_virt_bcms,
> +	.num_bcms = ARRAY_SIZE(mc_virt_bcms),
> +};
> +
> +static struct qcom_icc_bcm *mmss_noc_bcms[] = {
> +	&bcm_mm0,
> +	&bcm_mm1,
> +	&bcm_mm0_disp,
> +	&bcm_mm0_cam_ife_0,
> +	&bcm_mm1_cam_ife_0,
> +	&bcm_mm0_cam_ife_1,
> +	&bcm_mm1_cam_ife_1,
> +	&bcm_mm0_cam_ife_2,
> +	&bcm_mm1_cam_ife_2,
> +};
> +
> +static struct qcom_icc_node * const mmss_noc_nodes[] = {
> +	[MASTER_CAMNOC_HF] = &qnm_camnoc_hf,
> +	[MASTER_CAMNOC_ICP] = &qnm_camnoc_icp,
> +	[MASTER_CAMNOC_SF] = &qnm_camnoc_sf,
> +	[MASTER_MDP] = &qnm_mdp,
> +	[MASTER_CDSP_HCP] = &qnm_vapss_hcp,
> +	[MASTER_VIDEO] = &qnm_video,
> +	[MASTER_VIDEO_CV_PROC] = &qnm_video_cv_cpu,
> +	[MASTER_VIDEO_PROC] = &qnm_video_cvp,
> +	[MASTER_VIDEO_V_PROC] = &qnm_video_v_cpu,
> +	[MASTER_CNOC_MNOC_CFG] = &qsm_mnoc_cfg,
> +	[SLAVE_MNOC_HF_MEM_NOC] = &qns_mem_noc_hf,
> +	[SLAVE_MNOC_SF_MEM_NOC] = &qns_mem_noc_sf,
> +	[SLAVE_SERVICE_MNOC] = &srvc_mnoc,
> +	[MASTER_MDP_DISP] = &qnm_mdp_disp,
> +	[SLAVE_MNOC_HF_MEM_NOC_DISP] = &qns_mem_noc_hf_disp,
> +	[MASTER_CAMNOC_HF_CAM_IFE_0] = &qnm_camnoc_hf_cam_ife_0,
> +	[MASTER_CAMNOC_ICP_CAM_IFE_0] = &qnm_camnoc_icp_cam_ife_0,
> +	[MASTER_CAMNOC_SF_CAM_IFE_0] = &qnm_camnoc_sf_cam_ife_0,
> +	[SLAVE_MNOC_HF_MEM_NOC_CAM_IFE_0] = &qns_mem_noc_hf_cam_ife_0,
> +	[SLAVE_MNOC_SF_MEM_NOC_CAM_IFE_0] = &qns_mem_noc_sf_cam_ife_0,
> +	[MASTER_CAMNOC_HF_CAM_IFE_1] = &qnm_camnoc_hf_cam_ife_1,
> +	[MASTER_CAMNOC_ICP_CAM_IFE_1] = &qnm_camnoc_icp_cam_ife_1,
> +	[MASTER_CAMNOC_SF_CAM_IFE_1] = &qnm_camnoc_sf_cam_ife_1,
> +	[SLAVE_MNOC_HF_MEM_NOC_CAM_IFE_1] = &qns_mem_noc_hf_cam_ife_1,
> +	[SLAVE_MNOC_SF_MEM_NOC_CAM_IFE_1] = &qns_mem_noc_sf_cam_ife_1,
> +	[MASTER_CAMNOC_HF_CAM_IFE_2] = &qnm_camnoc_hf_cam_ife_2,
> +	[MASTER_CAMNOC_ICP_CAM_IFE_2] = &qnm_camnoc_icp_cam_ife_2,
> +	[MASTER_CAMNOC_SF_CAM_IFE_2] = &qnm_camnoc_sf_cam_ife_2,
> +	[SLAVE_MNOC_HF_MEM_NOC_CAM_IFE_2] = &qns_mem_noc_hf_cam_ife_2,
> +	[SLAVE_MNOC_SF_MEM_NOC_CAM_IFE_2] = &qns_mem_noc_sf_cam_ife_2,
> +};
> +
> +static const struct qcom_icc_desc sm8550_mmss_noc = {
> +	.nodes = mmss_noc_nodes,
> +	.num_nodes = ARRAY_SIZE(mmss_noc_nodes),
> +	.bcms = mmss_noc_bcms,
> +	.num_bcms = ARRAY_SIZE(mmss_noc_bcms),
> +};
> +
> +static struct qcom_icc_bcm * const nsp_noc_bcms[] = {
> +	&bcm_co0,
> +};
> +
> +static struct qcom_icc_node * const nsp_noc_nodes[] = {
> +	[MASTER_CDSP_PROC] = &qxm_nsp,
> +	[SLAVE_CDSP_MEM_NOC] = &qns_nsp_gemnoc,
> +};
> +
> +static const struct qcom_icc_desc sm8550_nsp_noc = {
> +	.nodes = nsp_noc_nodes,
> +	.num_nodes = ARRAY_SIZE(nsp_noc_nodes),
> +	.bcms = nsp_noc_bcms,
> +	.num_bcms = ARRAY_SIZE(nsp_noc_bcms),
> +};
> +
> +static struct qcom_icc_bcm * const pcie_anoc_bcms[] = {
> +	&bcm_sn7,
> +};
> +
> +static struct qcom_icc_node * const pcie_anoc_nodes[] = {
> +	[MASTER_PCIE_ANOC_CFG] = &qsm_pcie_anoc_cfg,
> +	[MASTER_PCIE_0] = &xm_pcie3_0,
> +	[MASTER_PCIE_1] = &xm_pcie3_1,
> +	[SLAVE_ANOC_PCIE_GEM_NOC] = &qns_pcie_mem_noc,
> +	[SLAVE_SERVICE_PCIE_ANOC] = &srvc_pcie_aggre_noc,
> +};
> +
> +static const struct qcom_icc_desc sm8550_pcie_anoc = {
> +	.nodes = pcie_anoc_nodes,
> +	.num_nodes = ARRAY_SIZE(pcie_anoc_nodes),
> +	.bcms = pcie_anoc_bcms,
> +	.num_bcms = ARRAY_SIZE(pcie_anoc_bcms),
> +};
> +
> +static struct qcom_icc_bcm * const system_noc_bcms[] = {
> +	&bcm_sn0,
> +	&bcm_sn1,
> +	&bcm_sn2,
> +	&bcm_sn3,
> +};
> +
> +static struct qcom_icc_node * const system_noc_nodes[] = {
> +	[MASTER_GIC_AHB] = &qhm_gic,
> +	[MASTER_A1NOC_SNOC] = &qnm_aggre1_noc,
> +	[MASTER_A2NOC_SNOC] = &qnm_aggre2_noc,
> +	[MASTER_GIC] = &xm_gic,
> +	[SLAVE_SNOC_GEM_NOC_GC] = &qns_gemnoc_gc,
> +	[SLAVE_SNOC_GEM_NOC_SF] = &qns_gemnoc_sf,
> +};
> +
> +static const struct qcom_icc_desc sm8550_system_noc = {
> +	.nodes = system_noc_nodes,
> +	.num_nodes = ARRAY_SIZE(system_noc_nodes),
> +	.bcms = system_noc_bcms,
> +	.num_bcms = ARRAY_SIZE(system_noc_bcms),
> +};
> +
> +static int qnoc_probe(struct platform_device *pdev)
> +{
> +	const struct qcom_icc_desc *desc;
> +	struct icc_onecell_data *data;
> +	struct icc_provider *provider;
> +	struct qcom_icc_node * const *qnodes;
> +	struct qcom_icc_provider *qp;
> +	struct icc_node *node;
> +	size_t num_nodes, i;
> +	int ret;
> +
> +	desc = device_get_match_data(&pdev->dev);
> +	if (!desc)
> +		return -EINVAL;
> +
> +	qnodes = desc->nodes;
> +	num_nodes = desc->num_nodes;
> +
> +	qp = devm_kzalloc(&pdev->dev, sizeof(*qp), GFP_KERNEL);
> +	if (!qp)
> +		return -ENOMEM;
> +
> +	data = devm_kcalloc(&pdev->dev, num_nodes, sizeof(*node), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	provider = &qp->provider;
> +	provider->dev = &pdev->dev;
> +	provider->set = qcom_icc_set;
> +	provider->pre_aggregate = qcom_icc_pre_aggregate;
> +	provider->aggregate = qcom_icc_aggregate;
> +	provider->xlate_extended = qcom_icc_xlate_extended;
> +	INIT_LIST_HEAD(&provider->nodes);
> +	provider->data = data;
> +
> +	qp->dev = &pdev->dev;
> +	qp->bcms = desc->bcms;
> +	qp->num_bcms = desc->num_bcms;
> +
> +	qp->voter = of_bcm_voter_get(qp->dev, NULL);
> +	if (IS_ERR(qp->voter))
> +		return PTR_ERR(qp->voter);
> +
> +	ret = icc_provider_add(provider);
> +	if (ret) {
> +		dev_err(&pdev->dev, "error adding interconnect provider\n");

return dev_err_probe().

> +		return ret;
> +	}
> +

Best regards,
Krzysztof

Powered by blists - more mailing lists