Re: [PATCH v6 2/4] interconnect: add clk-based icc provider support

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 12.05.2023 02:13, Dmitry Baryshkov wrote:
> For some devices it is useful to export clocks as interconnect providers,
> if the clock corresponds to bus bandwidth.
> 
> For example, on MSM8996 the cluster interconnect clock should be scaled
> according to the cluster frequencies. Exporting it as an interconnect
> allows one to properly describe this as the cluster bandwidth
> requirements.
> 
> Tested-by: Yassine Oudjana <y.oudjana@protonmail.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
[...]

> +
> +	for (i = 0, j = 0; i < num_clocks; i++) {
> +		qp->clocks[i].clk = data[i].clk;
> +
> +		node = icc_node_create(first_id + j);
> +		if (IS_ERR(node)) {
> +			ret = PTR_ERR(node);
> +			goto err;
> +		}
> +
> +		node->name = devm_kasprintf(dev, GFP_KERNEL, "%s_master", data[i].name);
> +		node->data = &qp->clocks[i];
> +		icc_node_add(node, provider);
> +		/* link to the next node, slave */
> +		icc_link_create(node, first_id + j + 1);
> +		onecell->nodes[j++] = node;
> +
> +		node = icc_node_create(first_id + j);
> +		if (IS_ERR(node)) {
> +			ret = PTR_ERR(node);
> +			goto err;
> +		}
> +
> +		node->name = devm_kasprintf(dev, GFP_KERNEL, "%s_slave", data[i].name);
> +		/* no data for slave node */
> +		icc_node_add(node, provider);
> +		onecell->nodes[j++] = node;
I'm still not very into using 2 iterators and modifying one
on the flight, but I don't think I have any other issues with
this driver..

Some sort of a Mostly-Acked-by tag would be helpful here!

Konrad
> +	}
> +
> +	onecell->num_nodes = j;
> +
> +	ret = icc_provider_register(provider);
> +	if (ret)
> +		goto err;
> +
> +	return provider;
> +
> +err:
> +	icc_nodes_remove(provider);
> +
> +	return ERR_PTR(ret);
> +}
> +
> +/**
> + * icc_clk_unregister() - unregister a previously registered clk interconnect provider
> + * @provider: provider returned by icc_clk_register()
> + */
> +void icc_clk_unregister(struct icc_provider *provider)
> +{
> +	struct icc_clk_provider *qp = container_of(provider, struct icc_clk_provider, provider);
> +	int i;
> +
> +	icc_provider_deregister(&qp->provider);
> +	icc_nodes_remove(&qp->provider);
> +
> +	for (i = 0; i < qp->num_clocks; i++) {
> +		struct icc_clk_node *qn = &qp->clocks[i];
> +
> +		if (qn->enabled)
> +			clk_disable_unprepare(qn->clk);
> +	}
> +}
> diff --git a/include/linux/interconnect-clk.h b/include/linux/interconnect-clk.h
> new file mode 100644
> index 000000000000..0cd80112bea5
> --- /dev/null
> +++ b/include/linux/interconnect-clk.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2023, Linaro Ltd.
> + */
> +
> +#ifndef __LINUX_INTERCONNECT_CLK_H
> +#define __LINUX_INTERCONNECT_CLK_H
> +
> +struct device;
> +
> +struct icc_clk_data {
> +	struct clk *clk;
> +	const char *name;
> +};
> +
> +struct icc_provider *icc_clk_register(struct device *dev,
> +				      unsigned int first_id,
> +				      unsigned int num_clocks,
> +				      const struct icc_clk_data *data);
> +void icc_clk_unregister(struct icc_provider *provider);
> +
> +#endif