Re: [PATCH 1/5] interconnect: qcom: Consolidate interconnect RPM support

[Georgi Djakov]

Hi Jun,

Thanks for working on this.

On 9/30/20 11:16, Jun Nie wrote:
> Add RPM based interconnect driver implements the set and aggregate
> functionalities that translates bandwidth requests into RPM messages.
> These modules provide a common set of functionalities for all
> Qualcomm RPM based interconnect providers and should help reduce code
> duplication when adding new providers.
> 
> Signed-off-by: Jun Nie <jun.nie@xxxxxxxxxx>
> ---
>  drivers/interconnect/qcom/Makefile  |   3 +-
>  drivers/interconnect/qcom/icc-rpm.c | 194 ++++++++++++++++++++++
>  drivers/interconnect/qcom/icc-rpm.h |  73 +++++++++
>  drivers/interconnect/qcom/msm8916.c | 241 ++--------------------------
>  4 files changed, 279 insertions(+), 232 deletions(-)
>  create mode 100644 drivers/interconnect/qcom/icc-rpm.c
>  create mode 100644 drivers/interconnect/qcom/icc-rpm.h
> 
> diff --git a/drivers/interconnect/qcom/Makefile b/drivers/interconnect/qcom/Makefile
> index 1702ece67dc5..f5e803489de0 100644
> --- a/drivers/interconnect/qcom/Makefile
> +++ b/drivers/interconnect/qcom/Makefile
> @@ -9,7 +9,7 @@ icc-rpmh-obj				:= icc-rpmh.o
>  qnoc-sc7180-objs			:= sc7180.o
>  qnoc-sdm845-objs			:= sdm845.o
>  qnoc-sm8150-objs			:= sm8150.o
> -icc-smd-rpm-objs			:= smd-rpm.o
> +icc-smd-rpm-objs			:= smd-rpm.o icc-rpm.o
>  
>  obj-$(CONFIG_INTERCONNECT_QCOM_BCM_VOTER) += icc-bcm-voter.o
>  obj-$(CONFIG_INTERCONNECT_QCOM_MSM8916) += qnoc-msm8916.o
> @@ -21,3 +21,4 @@ obj-$(CONFIG_INTERCONNECT_QCOM_SC7180) += qnoc-sc7180.o
>  obj-$(CONFIG_INTERCONNECT_QCOM_SDM845) += qnoc-sdm845.o
>  obj-$(CONFIG_INTERCONNECT_QCOM_SM8150) += qnoc-sm8150.o
>  obj-$(CONFIG_INTERCONNECT_QCOM_SMD_RPM) += icc-smd-rpm.o
> +obj-$(CONFIG_INTERCONNECT_QCOM_SMD_RPM) += icc-smd-rpm.o

Duplicate line.

> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
> new file mode 100644
> index 000000000000..4ddcb6cd4b84
> --- /dev/null
> +++ b/drivers/interconnect/qcom/icc-rpm.c
[..]
> +int qnoc_probe(struct platform_device *pdev, size_t cd_size, int cd_num,
> +	       const struct clk_bulk_data *cd)
> +{
> +	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_kzalloc(dev, struct_size(data, nodes, num_nodes),
> +			    GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	qp->bus_clks = devm_kmemdup(dev, cd, cd_size,
> +				    GFP_KERNEL);
> +	if (!qp->bus_clks)
> +		return -ENOMEM;
> +
> +	qp->num_clks = cd_num;
> +	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);
> +	if (ret)
> +		return ret;
> +
> +	provider = &qp->provider;
> +	INIT_LIST_HEAD(&provider->nodes);
> +	provider->dev = dev;
> +	provider->set = qcom_icc_set;
> +	provider->aggregate = icc_std_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);
> +		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);

Please drop this line.

> +
> +		/* populate links */

Please drop this comment.

> +		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:
> +	icc_nodes_remove(provider);
> +	clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
> +	icc_provider_del(provider);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(qnoc_probe);
> +
> +int qnoc_remove(struct platform_device *pdev)
> +{
> +	struct qcom_icc_provider *qp = platform_get_drvdata(pdev);
> +
> +	icc_nodes_remove(&qp->provider);
> +	clk_bulk_disable_unprepare(qp->num_clks, qp->bus_clks);
> +	return icc_provider_del(&qp->provider);
> +}
> +EXPORT_SYMBOL(qnoc_remove);
> diff --git a/drivers/interconnect/qcom/icc-rpm.h b/drivers/interconnect/qcom/icc-rpm.h
> new file mode 100644
> index 000000000000..b75bb59d79b7
> --- /dev/null
> +++ b/drivers/interconnect/qcom/icc-rpm.h
> @@ -0,0 +1,73 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2020 Linaro Ltd
> + */
> +
> +#ifndef __DRIVERS_INTERCONNECT_QCOM_SMD_RPM_COMMON_H
> +#define __DRIVERS_INTERCONNECT_QCOM_SMD_RPM_COMMON_H

Would be nice if this matches with path and filename:
__DRIVERS_INTERCONNECT_QCOM_ICC_RPM_H


All the rest looks good.

Thanks,
Georgi