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

Commit Message

Leonard Crestez March 26, 2020, 2:16 a.m. UTC

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 <[e-mail]>
---
 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

Comments

Chanwoo Choi March 31, 2020, 11:04 p.m. UTC | #1

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.
<[e-mail]> -> <[e-mail]>

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 <[e-mail]>
> ---
>  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 <[e-mail]>");
> +MODULE_LICENSE("GPL v2");
>

Leonard Crestez April 1, 2020, 2:20 p.m. UTC | #2

On 2020-04-01 1:55 AM, Chanwoo Choi wrote:
> Hi,
> 
> Looks good to me. I added the comments.
> But, it need to add the dt binding documentation for this device.

DT bindings were included:

https://patchwork.kernel.org/patch/11458981/

> 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.
> <[e-mail]> -> <[e-mail]>

Yeah, I received multiple bounces because of this.

> 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 <[e-mail]>
>> ---
>>   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.

dropped

>> +	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.

Reordered kconfig to match. 8M_DDRC sorts before _BUS alphabetically but 
it's pettier this way, and matches tegra.

>>   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.

dropped

>> +};
>> +
>> +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().

fixed

>> +	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.

Switched to dev_pm_opp_set_rate.

It might be interesting to add regulators control later, on some chips 
the main NOC can run at different voltages.

> 
>> +}
>> +
>> +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

fixed

>> +	 *
>> +	 * 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 <[e-mail]>");
>> +MODULE_LICENSE("GPL v2");
>>
> 
>

Chanwoo Choi April 1, 2020, 10:57 p.m. UTC | #3

On 4/1/20 11:20 PM, Leonard Crestez wrote:
> On 2020-04-01 1:55 AM, Chanwoo Choi wrote:
>> Hi,
>>
>> Looks good to me. I added the comments.
>> But, it need to add the dt binding documentation for this device.
> 
> DT bindings were included:
> 
> https://patchwork.kernel.org/patch/11458981/

The dt-binding document for this driver is required under
Documentation/devicetree/binding/devfreq.

It is difficult to catch where is the dt-binding document
for this driver for who don't know the detailed history
of this driver. I don't said that add the duplicate documentation
But, at least the some document have to point out the reference.

> 
>> 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.
>> <[e-mail]> -> <[e-mail]>
> 
> Yeah, I received multiple bounces because of this.
> 
>> 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 <[e-mail]>
>>> ---
>>>   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.
> 
> dropped
> 
>>> +	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.
> 
> Reordered kconfig to match. 8M_DDRC sorts before _BUS alphabetically but 
> it's pettier this way, and matches tegra.
> 
>>>   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.
> 
> dropped
> 
>>> +};
>>> +
>>> +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().
> 
> fixed
> 
>>> +	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.
> 
> Switched to dev_pm_opp_set_rate.
> 
> It might be interesting to add regulators control later, on some chips 
> the main NOC can run at different voltages.
> 
>>
>>> +}
>>> +
>>> +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
> 
> fixed
> 
>>> +	 *
>>> +	 * 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 <[e-mail]>");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
>>
> 
> 
> 
>

Leonard Crestez April 2, 2020, 9:53 a.m. UTC | #4

On 2020-04-02 1:48 AM, Chanwoo Choi wrote:
> On 4/1/20 11:20 PM, Leonard Crestez wrote:
>> On 2020-04-01 1:55 AM, Chanwoo Choi wrote:
>>> Hi,
>>>
>>> Looks good to me. I added the comments.
>>> But, it need to add the dt binding documentation for this device.
>>
>> DT bindings were included:
>>
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F11458981%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7C7381d117a4d1468cd2c608d7d68ecfac%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637213781167514099&sdata=Qu8x14cXnuxlwOT2SlUOf%2FLgCVWbnJRKA4TBjMIWQeA%3D&reserved=0
> 
> The dt-binding document for this driver is required under
> Documentation/devicetree/binding/devfreq.

Bindings for imx8m-ddrc were at one point posted for 
devicetree/bindings/devfreq but Rob Herring suggested to move them under 
"memory-controller" instead and I expect same logic makes sense here. 
Link to previous discussion:

https://patchwork.kernel.org/patch/11221919/

DT bindings should try to describe "hardware" rather than "drivers" and 
an "interconnect" is a class of hardware while "devfreq" isn't.

Not only that but the main noc has properties parsed by interconnect driver.

> It is difficult to catch where is the dt-binding document
> for this driver for who don't know the detailed history
> of this driver. I don't said that add the duplicate documentation
> But, at least the some document have to point out the reference.

What I usually do to find information about a device is grep for the 
compat string in the entire tree.

>>> 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.
>>> <[e-mail]> -> <[e-mail]>
>>
>> Yeah, I received multiple bounces because of this.
>>
>>> 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 <[e-mail]>
>>>> ---
>>>>    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.
>>
>> dropped
>>
>>>> +	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.
>>
>> Reordered kconfig to match. 8M_DDRC sorts before _BUS alphabetically but
>> it's pettier this way, and matches tegra.
>>
>>>>    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.
>>
>> dropped
>>
>>>> +};
>>>> +
>>>> +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().
>>
>> fixed
>>
>>>> +	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.
>>
>> Switched to dev_pm_opp_set_rate.
>>
>> It might be interesting to add regulators control later, on some chips
>> the main NOC can run at different voltages.
>>
>>>
>>>> +}
>>>> +
>>>> +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
>>
>> fixed
>>
>>>> +	 *
>>>> +	 * 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 <[e-mail]>");
>>>> +MODULE_LICENSE("GPL v2");
>>>>
>>>
>>>
>>
>>
>>
>>
> 
>

Chanwoo Choi April 3, 2020, 6:23 a.m. UTC | #5

On 4/2/20 6:53 PM, Leonard Crestez wrote:
> On 2020-04-02 1:48 AM, Chanwoo Choi wrote:
>> On 4/1/20 11:20 PM, Leonard Crestez wrote:
>>> On 2020-04-01 1:55 AM, Chanwoo Choi wrote:
>>>> Hi,
>>>>
>>>> Looks good to me. I added the comments.
>>>> But, it need to add the dt binding documentation for this device.
>>>
>>> DT bindings were included:
>>>
>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F11458981%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7C7381d117a4d1468cd2c608d7d68ecfac%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637213781167514099&sdata=Qu8x14cXnuxlwOT2SlUOf%2FLgCVWbnJRKA4TBjMIWQeA%3D&reserved=0
>>
>> The dt-binding document for this driver is required under
>> Documentation/devicetree/binding/devfreq.
> 
> Bindings for imx8m-ddrc were at one point posted for 
> devicetree/bindings/devfreq but Rob Herring suggested to move them under 
> "memory-controller" instead and I expect same logic makes sense here. 
> Link to previous discussion:
> 
> https://patchwork.kernel.org/patch/11221919/
> 
> DT bindings should try to describe "hardware" rather than "drivers" and 
> an "interconnect" is a class of hardware while "devfreq" isn't.
> 
> Not only that but the main noc has properties parsed by interconnect driver.

OK. Thanks for reply.

> 
>> It is difficult to catch where is the dt-binding document
>> for this driver for who don't know the detailed history
>> of this driver. I don't said that add the duplicate documentation
>> But, at least the some document have to point out the reference.
> 
> What I usually do to find information about a device is grep for the 
> compat string in the entire tree.
> 
>>>> 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.
>>>> <[e-mail]> -> <[e-mail]>
>>>
>>> Yeah, I received multiple bounces because of this.
>>>
>>>> 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 <[e-mail]>
>>>>> ---
>>>>>    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.
>>>
>>> dropped
>>>
>>>>> +	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.
>>>
>>> Reordered kconfig to match. 8M_DDRC sorts before _BUS alphabetically but
>>> it's pettier this way, and matches tegra.
>>>
>>>>>    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.
>>>
>>> dropped
>>>
>>>>> +};
>>>>> +
>>>>> +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().
>>>
>>> fixed
>>>
>>>>> +	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.
>>>
>>> Switched to dev_pm_opp_set_rate.
>>>
>>> It might be interesting to add regulators control later, on some chips
>>> the main NOC can run at different voltages.
>>>
>>>>
>>>>> +}
>>>>> +
>>>>> +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
>>>
>>> fixed
>>>
>>>>> +	 *
>>>>> +	 * 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 <[e-mail]>");
>>>>> +MODULE_LICENSE("GPL v2");
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>>
> 
> 
>

Patch

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
+	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
 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;
+};
+
+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);
+	dev_pm_opp_put(new_opp);
+
+	return clk_set_rate(priv->clk, new_freq);
+}
+
+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.
+	 *
+	 * 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 <[e-mail]>");
+MODULE_LICENSE("GPL v2");