Re: [PATCH V6 08/13] interconnect: mediatek: Add interconnect provider driver

From: Georgi Djakov <georgi.djakov@linaro.org>
To: Henry Chen <henryc.chen@mediatek.com>,
	Rob Herring <robh+dt@kernel.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Stephen Boyd <swboyd@chromium.org>,
	Ryan Case <ryandcase@chromium.org>,
	Mark Brown <broonie@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Nicolas Boichat <drinkcat@google.com>,
	Fan Chen <fan.chen@mediatek.com>,
	James Liao <jamesjj.liao@mediatek.com>,
	Arvin Wang <arvin.wang@mediatek.com>,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
	linux-pm@vger.kernel.org
Subject: Re: [PATCH V6 08/13] interconnect: mediatek: Add interconnect provider driver
Date: Mon, 4 Jan 2021 20:36:04 +0200	[thread overview]
Message-ID: <c8b951b0-6412-d905-99e1-6350283b57c1@linaro.org> (raw)
In-Reply-To: <1608790134-27425-9-git-send-email-henryc.chen@mediatek.com>

On 12/24/20 08:08, Henry Chen wrote:
> Introduce Mediatek MT6873/MT8183/MT8192 specific provider driver
> using the interconnect framework.
> 
>               ICC provider         ICC Nodes
>                                ----          ----
>               ---------       |CPU |   |--- |VPU |
>      -----   |         |-----  ----    |     ----
>     |DRAM |--|DRAM     |       ----    |     ----
>     |     |--|scheduler|----- |GPU |   |--- |DISP|
>     |     |--|(EMI)    |       ----    |     ----
>     |     |--|         |       -----   |     ----
>      -----   |         |----- |MMSYS|--|--- |VDEC|
>               ---------        -----   |     ----
>                 /|\                    |     ----
>                  |change DRAM freq     |--- |VENC|
>               ----------               |     ----
>              |  DVFSR   |              |
>              |          |              |     ----
>               ----------               |--- |IMG |
>                                        |     ----
>                                        |     ----
>                                        |--- |CAM |
>                                              ----
> 
> Signed-off-by: Henry Chen <henryc.chen@mediatek.com>
> ---
>   drivers/interconnect/Kconfig            |   1 +
>   drivers/interconnect/Makefile           |   1 +
>   drivers/interconnect/mediatek/Kconfig   |  13 ++
>   drivers/interconnect/mediatek/Makefile  |   3 +
>   drivers/interconnect/mediatek/mtk-emi.c | 330 ++++++++++++++++++++++++++++++++
>   5 files changed, 348 insertions(+)
>   create mode 100644 drivers/interconnect/mediatek/Kconfig
>   create mode 100644 drivers/interconnect/mediatek/Makefile
>   create mode 100644 drivers/interconnect/mediatek/mtk-emi.c
> 
> diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
> index 5b7204e..e939f5a 100644
> --- a/drivers/interconnect/Kconfig
> +++ b/drivers/interconnect/Kconfig
> @@ -13,5 +13,6 @@ if INTERCONNECT
>   
>   source "drivers/interconnect/imx/Kconfig"
>   source "drivers/interconnect/qcom/Kconfig"
> +source "drivers/interconnect/mediatek/Kconfig"

Sort alphabetically please.

>   
>   endif
> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
> index d203520..0643a24 100644
> --- a/drivers/interconnect/Makefile
> +++ b/drivers/interconnect/Makefile
> @@ -6,3 +6,4 @@ icc-core-objs				:= core.o bulk.o
>   obj-$(CONFIG_INTERCONNECT)		+= icc-core.o
>   obj-$(CONFIG_INTERCONNECT_IMX)		+= imx/
>   obj-$(CONFIG_INTERCONNECT_QCOM)		+= qcom/
> +obj-$(CONFIG_INTERCONNECT_MTK)		+= mediatek/

Ditto.

> diff --git a/drivers/interconnect/mediatek/Kconfig b/drivers/interconnect/mediatek/Kconfig
> new file mode 100644
> index 0000000..972d3bb
> --- /dev/null
> +++ b/drivers/interconnect/mediatek/Kconfig
> @@ -0,0 +1,13 @@
> +config INTERCONNECT_MTK
> +	bool "Mediatek Network-on-Chip interconnect drivers"
> +	depends on ARCH_MEDIATEK
> +	help
> +	  Support for Mediatek's Network-on-Chip interconnect hardware.
> +
> +config INTERCONNECT_MTK_EMI
> +	tristate "Mediatek EMI interconnect driver"
> +	depends on INTERCONNECT_MTK
> +	depends on (MTK_DVFSRC && OF)

Would it be possible to enable COMPILE_TEST?

> +	help
> +	  This is a driver for the Mediatek Network-on-Chip on DVFSRC-based
> +	  platforms.
> diff --git a/drivers/interconnect/mediatek/Makefile b/drivers/interconnect/mediatek/Makefile
> new file mode 100644
> index 0000000..353842b
> --- /dev/null
> +++ b/drivers/interconnect/mediatek/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_INTERCONNECT_MTK_EMI) += mtk-emi.o
> \ No newline at end of file
> diff --git a/drivers/interconnect/mediatek/mtk-emi.c b/drivers/interconnect/mediatek/mtk-emi.c
> new file mode 100644
> index 0000000..9670077
> --- /dev/null
> +++ b/drivers/interconnect/mediatek/mtk-emi.c
> @@ -0,0 +1,330 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.

Just a reminder that the year should be updated when you re-submit.

> + *
> + */
> +
> +#include <linux/device.h>
> +#include <linux/interconnect-provider.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/soc/mediatek/mtk_dvfsrc.h>
> +#include <dt-bindings/interconnect/mtk,mt8183-emi.h>
> +#include <dt-bindings/interconnect/mtk,mt6873-emi.h>

Nit: Alphanumeric order please.

[..]
> +static int emi_icc_remove(struct platform_device *pdev);
> +static int emi_icc_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *match;
> +	const struct mtk_icc_desc *desc;
> +	struct device *dev = &pdev->dev;
> +	struct icc_node *node;
> +	struct icc_onecell_data *data;
> +	struct icc_provider *provider;
> +	struct mtk_icc_node **mnodes;
> +	size_t num_nodes, i, j;
> +	int ret;
> +
> +	match = of_match_node(emi_icc_of_match, dev->parent->of_node);
> +
> +	if (!match) {
> +		dev_err(dev, "invalid compatible string\n");
> +		return -ENODEV;
> +	}
> +
> +	desc = match->data;
> +	mnodes = desc->nodes;
> +	num_nodes = desc->num_nodes;
> +
> +	provider = devm_kzalloc(dev, sizeof(*provider), GFP_KERNEL);
> +	if (!provider)
> +		return -ENOMEM;
> +
> +	data = devm_kzalloc(dev, struct_size(data, nodes, num_nodes),
> +			    GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	provider->dev = pdev->dev.parent;
> +	provider->set = emi_icc_set;
> +	provider->aggregate = emi_icc_aggregate;
> +	provider->xlate = of_icc_xlate_onecell;
> +	INIT_LIST_HEAD(&provider->nodes);
> +	provider->data = data;
> +
> +	ret = icc_provider_add(provider);
> +	if (ret) {
> +		dev_err(&pdev->dev, "error adding interconnect provider\n");
> +		return ret;
> +	}
> +
> +	for (i = 0; i < num_nodes; i++) {
> +		node = icc_node_create(mnodes[i]->id);
> +		if (IS_ERR(node)) {
> +			ret = PTR_ERR(node);
> +			goto err;
> +		}
> +
> +		node->name = mnodes[i]->name;
> +		node->data = mnodes[i];
> +		icc_node_add(node, provider);
> +
> +		for (j = 0; j < mnodes[i]->num_links; j++)
> +			icc_link_create(node, mnodes[i]->links[j]);
> +
> +		data->nodes[i] = node;
> +	}
> +	data->num_nodes = num_nodes;
> +
> +	platform_set_drvdata(pdev, provider);
> +
> +	return 0;
> +err:
> +	icc_nodes_remove(provider);

Also call icc_provider_del() to restore the original state on error.

Thanks,
Georgi

> +	return ret;
> +}
> +
> +static int emi_icc_remove(struct platform_device *pdev)
> +{
> +	struct icc_provider *provider = platform_get_drvdata(pdev);
> +
> +	icc_nodes_remove(provider);
> +	return icc_provider_del(provider);
> +}
> +