Re: [PATCH 2/8] PM / devfreq: Add generic imx bus scaling driver

From: Chanwoo Choi <cw00.choi@samsung.com>
To: Leonard Crestez <leonard.crestez@nxp.com>,
	Georgi Djakov <georgi.djakov@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Martin Kepplinger <martink@posteo.de>
Cc: "Artur Świgoń" <a.swigon@samsung.com>,
	"Alexandre Bailon" <abailon@baylibre.com>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	"Jacky Bai" <ping.bai@nxp.com>,
	"Anson Huang" <Anson.Huang@nxp.com>,
	"Abel Vesa" <abel.vesa@nxp.com>,
	"Krzysztof Kozlowski" <krzk@kernel.org>,
	"MyungJoo Ham" <myungjoo.ham@samsung.com>,
	"Kyungmin Park" <kyungmin.park@samsung.com>,
	"Saravana Kannan" <saravanak@google.com>,
	"Mark Rutland" <mark.rutland@arm.com>,
	"Viresh Kumar" <viresh.kumar@linaro.org>,
	"Shawn Guo" <shawnguo@kernel.org>,
	"Dong Aisheng" <aisheng.dong@nxp.com>,
	"Fabio Estevam" <fabio.estevam@nxp.com>,
	"Stephen Boyd" <sboyd@kernel.org>,
	"Michael Turquette" <mturquette@baylibre.com>,
	"Matthias Kaehlcke" <mka@chromium.org>,
	"Angus Ainslie" <angus@akkea.ca>,
	"Silvano di Ninno" <silvano.dininno@nxp.com>,
	linux-pm@vger.kernel.org, kernel@pengutronix.de,
	linux-imx@nxp.com, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 2/8] PM / devfreq: Add generic imx bus scaling driver
Date: Wed, 1 Apr 2020 08:04:37 +0900	[thread overview]
Message-ID: <781911a0-6139-8e41-cd96-da40fad685fc@samsung.com> (raw)
In-Reply-To: <e32290a36b31fbe922cc8ed48c33e89a5eb08804.1585188174.git.leonard.crestez@nxp.com>

Hi,

Looks good to me. I added the comments.
But, it need to add the dt binding documentation for this device.

The old email of Artur Świgoń is not used. On next time,
use following the new email address Because when I reply the mail,
always show the fail message from thunderbird due to the Artur's old email.
<a.swigon@partnet.samsung.com> -> <a.swigon@samsung.com>

On 3/26/20 11:16 AM, Leonard Crestez wrote:
> Add initial support for dynamic frequency switching on pieces of the imx
> interconnect fabric.
> 
> All this driver does is set a clk rate based on an opp table, it does
> not map register areas.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/Kconfig   |   9 +++
>  drivers/devfreq/Makefile  |   1 +
>  drivers/devfreq/imx-bus.c | 142 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 152 insertions(+)
>  create mode 100644 drivers/devfreq/imx-bus.c
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 0b1df12e0f21..44d26192ddc4 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -99,10 +99,19 @@ config ARM_IMX8M_DDRC_DEVFREQ
>  	select DEVFREQ_GOV_USERSPACE
>  	help
>  	  This adds the DEVFREQ driver for the i.MX8M DDR Controller. It allows
>  	  adjusting DRAM frequency.
>  
> +config ARM_IMX_BUS_DEVFREQ
> +	tristate "i.MX Generic Bus DEVFREQ Driver"
> +	depends on ARCH_MXC || COMPILE_TEST
> +	select DEVFREQ_GOV_PASSIVE
> +	select DEVFREQ_GOV_USERSPACE

Maybe, you would update it by using passive governor?
But, in this version, it doesn't handle the any passive governor.

> +	help
> +	  This adds the generic DEVFREQ driver for i.MX interconnects. It
> +	  allows adjusting NIC/NOC frequency.
> +
>  config ARM_TEGRA_DEVFREQ
>  	tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
>  	depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || \
>  		ARCH_TEGRA_132_SOC || ARCH_TEGRA_124_SOC || \
>  		ARCH_TEGRA_210_SOC || \
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 3eb4d5e6635c..3ca1ad0ecb97 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -7,10 +7,11 @@ obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE)	+= governor_powersave.o
>  obj-$(CONFIG_DEVFREQ_GOV_USERSPACE)	+= governor_userspace.o
>  obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
>  
>  # DEVFREQ Drivers
>  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
> +obj-$(CONFIG_ARM_IMX_BUS_DEVFREQ)	+= imx-bus.o

The ARM_IMX_BUS_DEVFREQ config is under ARM_IMX8M_DDRC_DEVFREQ
and imx-bus.o is over imx8m-ddrc.o. Need to edit the sequence.

>  obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ)	+= imx8m-ddrc.o
>  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
>  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra30-devfreq.o
>  obj-$(CONFIG_ARM_TEGRA20_DEVFREQ)	+= tegra20-devfreq.o
>  
> diff --git a/drivers/devfreq/imx-bus.c b/drivers/devfreq/imx-bus.c
> new file mode 100644
> index 000000000000..285e0f1ae6b1
> --- /dev/null
> +++ b/drivers/devfreq/imx-bus.c
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2019 NXP
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/devfreq.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/pm_opp.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +struct imx_bus {
> +	struct devfreq_dev_profile profile;
> +	struct devfreq *devfreq;
> +	struct clk *clk;
> +	struct devfreq_passive_data passive_data;

This patch doesn't touch the passive_data.

> +};
> +
> +static int imx_bus_target(struct device *dev,
> +		unsigned long *freq, u32 flags)
> +{
> +	struct imx_bus *priv = dev_get_drvdata(dev);
> +	struct dev_pm_opp *new_opp;
> +	unsigned long new_freq;
> +	int ret;
> +
> +	new_opp = devfreq_recommended_opp(dev, freq, flags);
> +	if (IS_ERR(new_opp)) {
> +		ret = PTR_ERR(new_opp);
> +		dev_err(dev, "failed to get recommended opp: %d\n", ret);
> +		return ret;
> +	}
> +	new_freq = dev_pm_opp_get_freq(new_opp);

It doesn't need. Because the new frequency is stored to 'freq'
by calling devfreq_recommended_opp().

> +	dev_pm_opp_put(new_opp);
> +
> +	return clk_set_rate(priv->clk, new_freq);

nitpick. you can use dev_pm_opp_set_rate(). But, I'm not forcing to use it.

> +}
> +
> +static int imx_bus_get_cur_freq(struct device *dev, unsigned long *freq)
> +{
> +	struct imx_bus *priv = dev_get_drvdata(dev);
> +
> +	*freq = clk_get_rate(priv->clk);
> +
> +	return 0;
> +}
> +
> +static int imx_bus_get_dev_status(struct device *dev,
> +		struct devfreq_dev_status *stat)
> +{
> +	struct imx_bus *priv = dev_get_drvdata(dev);
> +
> +	stat->busy_time = 0;
> +	stat->total_time = 0;
> +	stat->current_frequency = clk_get_rate(priv->clk);
> +
> +	return 0;
> +}
> +
> +static void imx_bus_exit(struct device *dev)
> +{
> +	dev_pm_opp_of_remove_table(dev);
> +}
> +
> +static int imx_bus_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct imx_bus *priv;
> +	const char *gov = DEVFREQ_GOV_USERSPACE;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Fetch the clock to adjust but don't explictly enable.

Need to fix typo.
s/explictly/explicitly

> +	 *
> +	 * For imx bus clock clk_set_rate is safe no matter if the clock is on
> +	 * or off and some peripheral side-buses might be off unless enabled by
> +	 * drivers for devices on those specific buses.
> +	 *
> +	 * Rate adjustment on a disabled bus clock just takes effect later.
> +	 */
> +	priv->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->clk)) {
> +		ret = PTR_ERR(priv->clk);
> +		dev_err(dev, "failed to fetch clk: %d\n", ret);
> +		return ret;
> +	}
> +	platform_set_drvdata(pdev, priv);
> +
> +	ret = dev_pm_opp_of_add_table(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to get OPP table\n");
> +		return ret;
> +	}
> +
> +	priv->profile.polling_ms = 1000;
> +	priv->profile.target = imx_bus_target;
> +	priv->profile.get_dev_status = imx_bus_get_dev_status;
> +	priv->profile.exit = imx_bus_exit;
> +	priv->profile.get_cur_freq = imx_bus_get_cur_freq;
> +	priv->profile.initial_freq = clk_get_rate(priv->clk);
> +
> +	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
> +						gov, NULL);
> +	if (IS_ERR(priv->devfreq)) {
> +		ret = PTR_ERR(priv->devfreq);
> +		dev_err(dev, "failed to add devfreq device: %d\n", ret);
> +		goto err;
> +	}
> +
> +	return 0;
> +
> +err:
> +	dev_pm_opp_of_remove_table(dev);
> +	return ret;
> +}
> +
> +static const struct of_device_id imx_bus_of_match[] = {
> +	{ .compatible = "fsl,imx8m-noc", },
> +	{ .compatible = "fsl,imx8m-nic", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, imx_bus_of_match);
> +
> +static struct platform_driver imx_bus_platdrv = {
> +	.probe		= imx_bus_probe,
> +	.driver = {
> +		.name	= "imx-bus-devfreq",
> +		.of_match_table = of_match_ptr(imx_bus_of_match),
> +	},
> +};
> +module_platform_driver(imx_bus_platdrv);
> +
> +MODULE_DESCRIPTION("Generic i.MX bus frequency scaling driver");
> +MODULE_AUTHOR("Leonard Crestez <leonard.crestez@nxp.com>");
> +MODULE_LICENSE("GPL v2");
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics