Re: [PATCH RFC v5 05/10] interconnect: Add imx core driver

[Georgi Djakov <georgi.djakov@linaro.org>]

Hi Leonard,

Thanks for the patch!

On 1.11.19 г. 0:52 ч., Leonard Crestez wrote:
> This adds support for i.MX SoC family to interconnect framework.
> 
> Platform drivers can describe the interconnect graph and several
> adjustment knobs where icc node bandwidth is converted to a
> DEV_PM_QOS_MIN_FREQUENCY request.
> 
> The adjustable nodes are found based on an "interconnect-node-id"
> property by scanning the entire device tree.

Are the adjustable nodes SoC specific? Can we have them here in the driver
instead of scanning the entire device tree?

> The interconnect provider doesn't need an virtual OF node, instead those
> same adjustable nodes are registered as proxies which xlate to the
> platform-level provider.
> 
> The platform device for the interconnect needs to be registered from a
> SOC driver (similar to cpufreq).
> 
> Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/interconnect/Kconfig      |   1 +
>  drivers/interconnect/Makefile     |   1 +
>  drivers/interconnect/imx/Kconfig  |   5 +
>  drivers/interconnect/imx/Makefile |   1 +
>  drivers/interconnect/imx/imx.c    | 273 ++++++++++++++++++++++++++++++
>  drivers/interconnect/imx/imx.h    |  60 +++++++
>  6 files changed, 341 insertions(+)
>  create mode 100644 drivers/interconnect/imx/Kconfig
>  create mode 100644 drivers/interconnect/imx/Makefile
>  create mode 100644 drivers/interconnect/imx/imx.c
>  create mode 100644 drivers/interconnect/imx/imx.h
> 
> diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
> index b6ea8f0a6122..f57e77b8731c 100644
> --- a/drivers/interconnect/Kconfig
> +++ b/drivers/interconnect/Kconfig
> @@ -10,7 +10,8 @@ menuconfig INTERCONNECT
>  	  If unsure, say no.
>  
>  if INTERCONNECT
>  
>  source "drivers/interconnect/qcom/Kconfig"
> +source "drivers/interconnect/imx/Kconfig"
>  
>  endif
> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
> index 28f2ab0824d5..20a13b7eb37f 100644
> --- a/drivers/interconnect/Makefile
> +++ b/drivers/interconnect/Makefile
> @@ -2,5 +2,6 @@
>  
>  icc-core-objs				:= core.o
>  
>  obj-$(CONFIG_INTERCONNECT)		+= icc-core.o
>  obj-$(CONFIG_INTERCONNECT_QCOM)		+= qcom/
> +obj-$(CONFIG_INTERCONNECT_IMX)		+= imx/
> diff --git a/drivers/interconnect/imx/Kconfig b/drivers/interconnect/imx/Kconfig
> new file mode 100644
> index 000000000000..7d81d3c83a61
> --- /dev/null
> +++ b/drivers/interconnect/imx/Kconfig
> @@ -0,0 +1,5 @@
> +config INTERCONNECT_IMX
> +	bool "i.MX interconnect drivers"
> +	depends on ARCH_MXC || COMPILE_TEST
> +	help
> +	  Generic interconnect driver for i.MX SOCs
> diff --git a/drivers/interconnect/imx/Makefile b/drivers/interconnect/imx/Makefile
> new file mode 100644
> index 000000000000..bb92fd9fe4a5
> --- /dev/null
> +++ b/drivers/interconnect/imx/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_INTERCONNECT_IMX) += imx.o
> diff --git a/drivers/interconnect/imx/imx.c b/drivers/interconnect/imx/imx.c
> new file mode 100644
> index 000000000000..7d248e01dcf0
> --- /dev/null
> +++ b/drivers/interconnect/imx/imx.c
> @@ -0,0 +1,273 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Interconnect framework driver for i.MX SoC
> + *
> + * Copyright (c) 2019, BayLibre
> + * Copyright (c) 2019, NXP
> + * Author: Alexandre Bailon <abailon@baylibre.com>
> + * Author: Leonard Crestez <leonard.crestez@nxp.com>
> + */
> +
> +#include <linux/devfreq.h>
> +#include <linux/device.h>
> +#include <linux/interconnect-provider.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_qos.h>
> +
> +#include "imx.h"
> +
> +/* private icc_provider data */
> +struct imx_icc_provider {
> +	struct device *dev;

What device is this? There is already a *dev in struct icc_provider.
Please add kernel-doc.

> +};
> +
> +/* private icc_node data */
> +struct imx_icc_node {
> +	const struct imx_icc_node_desc *desc;
> +	struct devfreq *devfreq;
> +	struct dev_pm_qos_request qos_req;
> +};
> +
> +static int imx_icc_aggregate(struct icc_node *node, u32 tag,
> +			     u32 avg_bw, u32 peak_bw,
> +			     u32 *agg_avg, u32 *agg_peak)
> +{
> +	*agg_avg += avg_bw;
> +	*agg_peak = max(*agg_peak, peak_bw);
> +
> +	return 0;
> +}
> +
> +static struct icc_node *imx_icc_xlate(struct of_phandle_args *spec, void *data)
> +{
> +	struct imx_icc_provider *desc = data;
> +	struct icc_provider *provider = dev_get_drvdata(desc->dev);
> +	unsigned int id = spec->args[0];
> +	struct icc_node *node;
> +
> +	list_for_each_entry(node, &provider->nodes, node_list)
> +		if (node->id == id)
> +			return node;
> +
> +	return ERR_PTR(-EINVAL);
> +}
> +
> +static int imx_icc_node_set(struct icc_node *node)
> +{
> +	struct device *dev = node->provider->dev;
> +	struct imx_icc_node *node_data = node->data;
> +	u64 freq;
> +
> +	if (!node_data->devfreq)
> +		return 0;
> +
> +	freq = (node->avg_bw + node->peak_bw) * node_data->desc->adj->bw_mul;

Why the sum of average and peak bandwidth?

> +	do_div(freq, node_data->desc->adj->bw_div);
> +	dev_dbg(dev, "node %s device %s avg_bw %ukBps peak_bw %ukBps min_freq %llukHz\n",
> +			node->name, dev_name(node_data->devfreq->dev.parent),
> +			node->avg_bw, node->peak_bw, freq);
> +
> +	if (freq > S32_MAX) {
> +		dev_err(dev, "%s can't request more than S32_MAX freq\n",
> +				node->name);
> +		return -ERANGE;
> +	}
> +
> +	dev_pm_qos_update_request(&node_data->qos_req, freq);
> +
> +	return 0;
> +}
> +
> +static int imx_icc_set(struct icc_node *src, struct icc_node *dst)
> +{
> +	return imx_icc_node_set(dst);
> +}
> +
> +static int imx_icc_node_init_devfreq(struct device *dev,
> +				     struct icc_node *node)
> +{
> +	struct imx_icc_node *node_data = node->data;
> +	struct device_node *dn;
> +	u32 node_id;
> +	int ret;
> +
> +	/* Find nodes based on interconnect-node-id property */
> +	for_each_node_with_property(dn, "interconnect-node-id") {
> +		ret = of_property_read_u32(dn, "interconnect-node-id",
> +					   &node_id);
> +		if (ret != 0)
> +			continue;
> +
> +		if (node_id == node->id) {
> +			of_node_get(dn);
> +			break;
> +		}
> +	}
> +
> +	if (!dn)
> +		return 0;
> +
> +	dev_info(dev, "node %s[%d] has device node %pOF\n",
> +			node->name, node->id, dn);
> +	node_data->devfreq = devfreq_get_devfreq_by_node(dn);

Ah, so you need to get the devfreq nodes? So looking at the next
patches it seems that noc and ddrc are the only adjustable nodes?

Maybe we should model them both as interconnect providers, as they
seem to be dealing with the bandwidth/frequency requirements and
changing the clock rates.

Thanks,
Georgi

> +	if (IS_ERR(node_data->devfreq)) {
> +		of_node_put(dn);
> +		ret = PTR_ERR(node_data->devfreq);
> +		dev_err(dev, "failed to fetch devfreq for %s: %d\n",
> +				node->name, ret);
> +		return ret;
> +	}
> +
> +	of_node_put(dn);
> +
> +	return dev_pm_qos_add_request(node_data->devfreq->dev.parent,
> +				      &node_data->qos_req,
> +				      DEV_PM_QOS_MIN_FREQUENCY, 0);
> +}
> +