[RFC,v6,2/9] PM / devfreq: Add generic imx bus scaling driver
diff mbox series

Message ID f329e715898a6b9fd0cee707a93fb1e144e31bd4.1573761527.git.leonard.crestez@nxp.com
State RFC, archived
Headers show
Series
  • interconnect: Add imx support via devfreq
Related show

Commit Message

Leonard Crestez Nov. 14, 2019, 8:09 p.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 <leonard.crestez@nxp.com>
---
 drivers/devfreq/Kconfig       |   9 ++
 drivers/devfreq/Makefile      |   1 +
 drivers/devfreq/imx-devfreq.c | 150 ++++++++++++++++++++++++++++++++++
 3 files changed, 160 insertions(+)
 create mode 100644 drivers/devfreq/imx-devfreq.c

Comments

Angus Ainslie Nov. 20, 2019, 2:08 p.m. UTC | #1
Hi Leonard,

On 2019-11-14 12:09, 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.
> 

Is this working with mainline ATF or does it still need to be used with 
your modified ATF code ?

Thanks
Angus

> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/Kconfig       |   9 ++
>  drivers/devfreq/Makefile      |   1 +
>  drivers/devfreq/imx-devfreq.c | 150 ++++++++++++++++++++++++++++++++++
>  3 files changed, 160 insertions(+)
>  create mode 100644 drivers/devfreq/imx-devfreq.c
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 923a6132e741..fef5ce831e90 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -98,10 +98,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_DEVFREQ
> +	tristate "i.MX Generic 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..61d0edee16f7 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -8,10 +8,11 @@ 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_IMX8M_DDRC_DEVFREQ)	+= imx8m-ddrc.o
> +obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.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
> 
>  # DEVFREQ Event Drivers
> diff --git a/drivers/devfreq/imx-devfreq.c 
> b/drivers/devfreq/imx-devfreq.c
> new file mode 100644
> index 000000000000..620b344e87aa
> --- /dev/null
> +++ b/drivers/devfreq/imx-devfreq.c
> @@ -0,0 +1,150 @@
> +// 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_devfreq {
> +	struct devfreq_dev_profile profile;
> +	struct devfreq *devfreq;
> +	struct clk *clk;
> +	struct devfreq_passive_data passive_data;
> +};
> +
> +static int imx_devfreq_target(struct device *dev,
> +			      unsigned long *freq, u32 flags)
> +{
> +	struct imx_devfreq *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_devfreq_get_cur_freq(struct device *dev, unsigned long 
> *freq)
> +{
> +	struct imx_devfreq *priv = dev_get_drvdata(dev);
> +
> +	*freq = clk_get_rate(priv->clk);
> +
> +	return 0;
> +}
> +
> +static int imx_devfreq_get_dev_status(struct device *dev,
> +				      struct devfreq_dev_status *stat)
> +{
> +	struct imx_devfreq *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_devfreq_exit(struct device *dev)
> +{
> +	dev_pm_opp_of_remove_table(dev);
> +}
> +
> +static int imx_devfreq_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct imx_devfreq *priv;
> +	const char *gov = DEVFREQ_GOV_USERSPACE;
> +	void *govdata = NULL;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	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_devfreq_target;
> +	priv->profile.get_dev_status = imx_devfreq_get_dev_status;
> +	priv->profile.exit = imx_devfreq_exit;
> +	priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
> +	priv->profile.initial_freq = clk_get_rate(priv->clk);
> +
> +	/* Handle passive devfreq parent link */
> +	priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
> +	if (!IS_ERR(priv->passive_data.parent)) {
> +		dev_info(dev, "setup passive link to %s\n",
> +			 dev_name(priv->passive_data.parent->dev.parent));
> +		gov = DEVFREQ_GOV_PASSIVE;
> +		govdata = &priv->passive_data;
> +	} else if (priv->passive_data.parent != ERR_PTR(-ENODEV)) {
> +		// -ENODEV means no parent: not an error.
> +		ret = PTR_ERR(priv->passive_data.parent);
> +		if (ret != -EPROBE_DEFER)
> +			dev_warn(dev, "failed to get initialize passive parent: %d\n",
> +				 ret);
> +		goto err;
> +	}
> +
> +	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
> +						gov, govdata);
> +	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_devfreq_of_match[] = {
> +	{ .compatible = "fsl,imx8m-noc", },
> +	{ .compatible = "fsl,imx8m-nic", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
> +
> +static struct platform_driver imx_devfreq_platdrv = {
> +	.probe		= imx_devfreq_probe,
> +	.driver = {
> +		.name	= "imx-devfreq",
> +		.of_match_table = of_match_ptr(imx_devfreq_of_match),
> +	},
> +};
> +module_platform_driver(imx_devfreq_platdrv);
> +
> +MODULE_DESCRIPTION("Generic i.MX bus frequency driver");
> +MODULE_AUTHOR("Leonard Crestez <leonard.crestez@nxp.com>");
> +MODULE_LICENSE("GPL v2");
Leonard Crestez Nov. 20, 2019, 3:04 p.m. UTC | #2
On 20.11.2019 16:08, Angus Ainslie wrote:
> Hi Leonard,
> 
> On 2019-11-14 12:09, 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.
>>
> 
> Is this working with mainline ATF or does it still need to be used with
> your modified ATF code ?

This series doesn't perform SMC calls, that's done by the imx8m-ddrc 
driver: https://patchwork.kernel.org/cover/11244283/

This particular patch allows switching NOC frequency but that's just 
clk_set_rate.

DDRC frequency switching requires the imx branch of ATF (v2.0 + ~200 
patches) otherwise you will get probe failures. Source for imx atf is 
published here: https://source.codeaurora.org/external/imx/imx-atf/

For your particular 8mq B0 case slightly different setpoints are used 
and the fix is not in any public release yet so you need this:

https://github.com/cdleonard/arm-trusted-firmware/commits/imx_2.0.y_busfreq

Is "mainline ATF" an important criteria for Purism?

--
Regards,
Leonard
Marco Felsch Nov. 20, 2019, 3:29 p.m. UTC | #3
Hi Leonard,

On 19-11-20 15:04, Leonard Crestez wrote:
> On 20.11.2019 16:08, Angus Ainslie wrote:
> > Hi Leonard,
> > 
> > On 2019-11-14 12:09, 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.
> >>
> > 
> > Is this working with mainline ATF or does it still need to be used with
> > your modified ATF code ?
> 
> This series doesn't perform SMC calls, that's done by the imx8m-ddrc 
> driver: https://patchwork.kernel.org/cover/11244283/
> 
> This particular patch allows switching NOC frequency but that's just 
> clk_set_rate.
> 
> DDRC frequency switching requires the imx branch of ATF (v2.0 + ~200 
> patches) otherwise you will get probe failures. Source for imx atf is 
> published here: https://source.codeaurora.org/external/imx/imx-atf/
> 
> For your particular 8mq B0 case slightly different setpoints are used 
> and the fix is not in any public release yet so you need this:
> 
> https://github.com/cdleonard/arm-trusted-firmware/commits/imx_2.0.y_busfreq
> 
> Is "mainline ATF" an important criteria for Purism?

Sorry for jumping in here. Just asking myself if the nxp-atf is required
for a mainline kernel for the imx8mq devices?

Thanks,
Marco

> --
> Regards,
> Leonard
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Angus Ainslie Nov. 20, 2019, 3:41 p.m. UTC | #4
Hi Leonard,

On 2019-11-20 07:04, Leonard Crestez wrote:
> On 20.11.2019 16:08, Angus Ainslie wrote:
>> Hi Leonard,
>> 
>> On 2019-11-14 12:09, 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.
>>> 
>> 
>> Is this working with mainline ATF or does it still need to be used 
>> with
>> your modified ATF code ?
> 
> This series doesn't perform SMC calls, that's done by the imx8m-ddrc
> driver: https://patchwork.kernel.org/cover/11244283/
> 
> This particular patch allows switching NOC frequency but that's just
> clk_set_rate.
> 
> DDRC frequency switching requires the imx branch of ATF (v2.0 + ~200
> patches) otherwise you will get probe failures. Source for imx atf is
> published here: https://source.codeaurora.org/external/imx/imx-atf/

Ok I was under the impression that the imx_2.0.y_busfreq branch below 
was based on this. Shouldn't those patches be added to the imx ATF ?

> 
> For your particular 8mq B0 case slightly different setpoints are used
> and the fix is not in any public release yet so you need this:
> 
> https://github.com/cdleonard/arm-trusted-firmware/commits/imx_2.0.y_busfreq
> 

We also have 2n14w ( is that B1 ? ) imx8mq's that we are working with.

> Is "mainline ATF" an important criteria for Purism?
> 

Yes we intend to bring all of our patches to mainline and were hoping 
that NXP would be doing the same. Shouldn't a mainline kernel run on a 
mainline ATF ?

Thanks
Angus

> --
> Regards,
> Leonard
Leonard Crestez Nov. 20, 2019, 4:30 p.m. UTC | #5
On 20.11.2019 17:41, Angus Ainslie wrote:
> Hi Leonard,
> 
> On 2019-11-20 07:04, Leonard Crestez wrote:
>> On 20.11.2019 16:08, Angus Ainslie wrote:
>>> Hi Leonard,
>>>
>>> On 2019-11-14 12:09, 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.
>>>>
>>>
>>> Is this working with mainline ATF or does it still need to be used
>>> with your modified ATF code ?
>>
>> This series doesn't perform SMC calls, that's done by the imx8m-ddrc
>> driver: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11244283%2F&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C186d3c14d8bc41216e3b08d76dd0106d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637098612732915017&amp;sdata=ER10Ts4hk2Ft7%2FWBZ7r8lyFkB6un1VRwk0rSvRMm3ew%3D&amp;reserved=0
>>
>> This particular patch allows switching NOC frequency but that's just
>> clk_set_rate.
>>
>> DDRC frequency switching requires the imx branch of ATF (v2.0 + ~200
>> patches) otherwise you will get probe failures. Source for imx atf is
>> published here: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.codeaurora.org%2Fexternal%2Fimx%2Fimx-atf%2F&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C186d3c14d8bc41216e3b08d76dd0106d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637098612732915017&amp;sdata=KcdzTQaW4xxualeRUU%2B9LjBeq99wUtzDBxrHWLVbkDo%3D&amp;reserved=0
> 
> Ok I was under the impression that the imx_2.0.y_busfreq branch below
> was based on this. Shouldn't those patches be added to the imx ATF ?

Already done, it's just that CAF public releases are only made after 
internal testing. TF-A is open-source so I push patches to my personal 
github to help more adventurous developers.

>> For your particular 8mq B0 case slightly different setpoints are used
>> and the fix is not in any public release yet so you need this:
>>
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcdleonard%2Farm-trusted-firmware%2Fcommits%2Fimx_2.0.y_busfreq&amp;data=02%7C01%7Cleonard.crestez%40nxp.com%7C186d3c14d8bc41216e3b08d76dd0106d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637098612732925012&amp;sdata=Ape5T7xfiR0yfSuO1Lv9OQhmK5p3f0ROWpzJiAMw1VA%3D&amp;reserved=0
>>
> 
> We also have 2n14w ( is that B1 ? ) imx8mq's that we are working with.

Errata is e11327 and does not appear on 2N14W:

https://www.nxp.com/docs/en/errata/IMX8MDQLQ_1N14W.pdf
https://www.nxp.com/docs/en/errata/IMX8MDQLQ_2N14W.pdf

You should be able to test with a published release:
https://source.codeaurora.org/external/imx/imx-atf/log/?h=imx_4.19.35_1.1.0

>> Is "mainline ATF" an important criteria for Purism?
> 
> Yes we intend to bring all of our patches to mainline and were hoping
> that NXP would be doing the same. Shouldn't a mainline kernel run on a
> mainline ATF ?

You can still use mainline ATF (tested right now) but the imx8m-ddrc 
driver won't probe.

The ability to mix and match different branches of firmware and kernel 
is very useful for testing. There might be slight incompatibilities but 
in theory if a feature depends on both firmware and kernel support then 
it should gracefully degrade rather than crash or hang.

ATF support for this feature will be mainlined eventually, I picked the 
linux side first because review is more challenging and changes are much 
larger relative to what we have in our internal tree.

--
Regards,
Leonard
Angus Ainslie Nov. 20, 2019, 4:38 p.m. UTC | #6
On 2019-11-20 08:30, Leonard Crestez wrote:
> On 20.11.2019 17:41, Angus Ainslie wrote:
>> Hi Leonard,
>> 
>> On 2019-11-20 07:04, Leonard Crestez wrote:
>>> On 20.11.2019 16:08, Angus Ainslie wrote:
>>> Is "mainline ATF" an important criteria for Purism?
>> 
>> Yes we intend to bring all of our patches to mainline and were hoping
>> that NXP would be doing the same. Shouldn't a mainline kernel run on a
>> mainline ATF ?
> 
> You can still use mainline ATF (tested right now) but the imx8m-ddrc
> driver won't probe.
> 

Sorry I was talking about the DDR frequency scaling specifically.

> The ability to mix and match different branches of firmware and kernel
> is very useful for testing. There might be slight incompatibilities but
> in theory if a feature depends on both firmware and kernel support then
> it should gracefully degrade rather than crash or hang.

I saw the check you put in for the correct ATF version and that's very 
helpful thanks.

> 
> ATF support for this feature will be mainlined eventually, I picked the
> linux side first because review is more challenging and changes are 
> much
> larger relative to what we have in our internal tree.
> 

Do you have a patch against mainline ATF that we can test this feature 
with ?

Thanks
Angus

> --
> Regards,
> Leonard
Leonard Crestez Nov. 20, 2019, 6:02 p.m. UTC | #7
On 20.11.2019 18:38, Angus Ainslie wrote:
> On 2019-11-20 08:30, Leonard Crestez wrote:
>> On 20.11.2019 17:41, Angus Ainslie wrote:
>>> Hi Leonard,
>>>
>>> On 2019-11-20 07:04, Leonard Crestez wrote:
>>>> On 20.11.2019 16:08, Angus Ainslie wrote:
>>>> Is "mainline ATF" an important criteria for Purism?
>>>
>>> Yes we intend to bring all of our patches to mainline and were hoping
>>> that NXP would be doing the same. Shouldn't a mainline kernel run on a
>>> mainline ATF ?
>>
>> You can still use mainline ATF (tested right now) but the imx8m-ddrc
>> driver won't probe.
> 
> Sorry I was talking about the DDR frequency scaling specifically. >
>> The ability to mix and match different branches of firmware and kernel
>> is very useful for testing. There might be slight incompatibilities but
>> in theory if a feature depends on both firmware and kernel support then
>> it should gracefully degrade rather than crash or hang.
> 
> I saw the check you put in for the correct ATF version and that's very
> helpful thanks.
> 
>> ATF support for this feature will be mainlined eventually, I picked the
>> linux side first because review is more challenging and changes are
>> much larger relative to what we have in our internal tree.
> 
> Do you have a patch against mainline ATF that we can test this feature
> with ?

Not right now, and imx atf is based on a slightly older version so some 
porting effort might be required.

--
Regards,
Leonard
Chanwoo Choi Dec. 13, 2019, 1:30 a.m. UTC | #8
Hi,

On 11/15/19 5:09 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-devfreq.c | 150 ++++++++++++++++++++++++++++++++++
>  3 files changed, 160 insertions(+)
>  create mode 100644 drivers/devfreq/imx-devfreq.c
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 923a6132e741..fef5ce831e90 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -98,10 +98,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_DEVFREQ
> +	tristate "i.MX Generic 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..61d0edee16f7 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -8,10 +8,11 @@ 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_IMX8M_DDRC_DEVFREQ)	+= imx8m-ddrc.o
> +obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.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
>  
>  # DEVFREQ Event Drivers
> diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c
> new file mode 100644
> index 000000000000..620b344e87aa
> --- /dev/null
> +++ b/drivers/devfreq/imx-devfreq.c
> @@ -0,0 +1,150 @@
> +// 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_devfreq {
> +	struct devfreq_dev_profile profile;
> +	struct devfreq *devfreq;
> +	struct clk *clk;
> +	struct devfreq_passive_data passive_data;
> +};
> +
> +static int imx_devfreq_target(struct device *dev,
> +			      unsigned long *freq, u32 flags)

Don't use space for the indentation. Please use only tab.

> +{
> +	struct imx_devfreq *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_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
> +{
> +	struct imx_devfreq *priv = dev_get_drvdata(dev);
> +
> +	*freq = clk_get_rate(priv->clk);
> +
> +	return 0;
> +}
> +
> +static int imx_devfreq_get_dev_status(struct device *dev,
> +				      struct devfreq_dev_status *stat)

ditto. Please use tab for the indentation.

> +{
> +	struct imx_devfreq *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_devfreq_exit(struct device *dev)
> +{
> +	dev_pm_opp_of_remove_table(dev);
> +}
> +
> +static int imx_devfreq_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct imx_devfreq *priv;

How about changing the variable name 'priv' to 'imx' or 'imx_data'?
because it is not easy to catch the role of 'priv' from variable name.

> +	const char *gov = DEVFREQ_GOV_USERSPACE;
> +	void *govdata = NULL;

How about changing the variable name 'govdata' to 'gov_data'?
- govdata -> gov_data

> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->clk = devm_clk_get(dev, NULL);

nitpick: because the clock-name is not mandatory.
Don't need to specify the clock name to inform the role of clock
of other developer/user?

For example, "ddr", "bus" and so on.

> +	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_devfreq_target;
> +	priv->profile.get_dev_status = imx_devfreq_get_dev_status;
> +	priv->profile.exit = imx_devfreq_exit;
> +	priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
> +	priv->profile.initial_freq = clk_get_rate(priv->clk);
> +
> +	/* Handle passive devfreq parent link */
> +	priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
> +	if (!IS_ERR(priv->passive_data.parent)) {
> +		dev_info(dev, "setup passive link to %s\n",
> +			 dev_name(priv->passive_data.parent->dev.parent));
> +		gov = DEVFREQ_GOV_PASSIVE;
> +		govdata = &priv->passive_data;
> +	} else if (priv->passive_data.parent != ERR_PTR(-ENODEV)) {
> +		// -ENODEV means no parent: not an error.
> +		ret = PTR_ERR(priv->passive_data.parent);
> +		if (ret != -EPROBE_DEFER)
> +			dev_warn(dev, "failed to get initialize passive parent: %d\n",
> +				 ret);
> +		goto err;
> +	}

You better to change the exception handling as following: It is more simple.

	} else if (PTR_ERR(priv->passive_data.parent) == -EPROBE_DEFER)
			|| PTR_ERR(priv->passive_data.parent) == -ENODEV) {
		goto err;
	} else {
		ret = PTR_ERR(priv->passive_data.parent);
		dev_err(dev, "failed to get initialize passive parent: %d\n", ret);
		goto err;
	}

> +
> +	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
> +						gov, govdata);
> +	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_devfreq_of_match[] = {
> +	{ .compatible = "fsl,imx8m-noc", },
> +	{ .compatible = "fsl,imx8m-nic", },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
> +
> +static struct platform_driver imx_devfreq_platdrv = {
> +	.probe		= imx_devfreq_probe,
> +	.driver = {
> +		.name	= "imx-devfreq",
> +		.of_match_table = of_match_ptr(imx_devfreq_of_match),
> +	},
> +};
> +module_platform_driver(imx_devfreq_platdrv);
> +
> +MODULE_DESCRIPTION("Generic i.MX bus frequency driver");

If this driver is for bus frequency, you better to use 'bus' for the clock-name
for the readability.

> +MODULE_AUTHOR("Leonard Crestez <leonard.crestez@nxp.com>");
> +MODULE_LICENSE("GPL v2");
>
Chanwoo Choi Dec. 13, 2019, 1:51 a.m. UTC | #9
On 12/13/19 10:30 AM, Chanwoo Choi wrote:
> Hi,
> 
> On 11/15/19 5:09 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-devfreq.c | 150 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 160 insertions(+)
>>  create mode 100644 drivers/devfreq/imx-devfreq.c
>>
>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> index 923a6132e741..fef5ce831e90 100644
>> --- a/drivers/devfreq/Kconfig
>> +++ b/drivers/devfreq/Kconfig
>> @@ -98,10 +98,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_DEVFREQ
>> +	tristate "i.MX Generic 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..61d0edee16f7 100644
>> --- a/drivers/devfreq/Makefile
>> +++ b/drivers/devfreq/Makefile
>> @@ -8,10 +8,11 @@ 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_IMX8M_DDRC_DEVFREQ)	+= imx8m-ddrc.o
>> +obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.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
>>  
>>  # DEVFREQ Event Drivers
>> diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c
>> new file mode 100644
>> index 000000000000..620b344e87aa
>> --- /dev/null
>> +++ b/drivers/devfreq/imx-devfreq.c
>> @@ -0,0 +1,150 @@
>> +// 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_devfreq {
>> +	struct devfreq_dev_profile profile;
>> +	struct devfreq *devfreq;
>> +	struct clk *clk;
>> +	struct devfreq_passive_data passive_data;
>> +};
>> +
>> +static int imx_devfreq_target(struct device *dev,
>> +			      unsigned long *freq, u32 flags)
> 
> Don't use space for the indentation. Please use only tab.
> 
>> +{
>> +	struct imx_devfreq *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_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
>> +{
>> +	struct imx_devfreq *priv = dev_get_drvdata(dev);
>> +
>> +	*freq = clk_get_rate(priv->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static int imx_devfreq_get_dev_status(struct device *dev,
>> +				      struct devfreq_dev_status *stat)
> 
> ditto. Please use tab for the indentation.
> 
>> +{
>> +	struct imx_devfreq *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_devfreq_exit(struct device *dev)
>> +{
>> +	dev_pm_opp_of_remove_table(dev);
>> +}
>> +
>> +static int imx_devfreq_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct imx_devfreq *priv;
> 
> How about changing the variable name 'priv' to 'imx' or 'imx_data'?
> because it is not easy to catch the role of 'priv' from variable name.
> 
>> +	const char *gov = DEVFREQ_GOV_USERSPACE;
>> +	void *govdata = NULL;
> 
> How about changing the variable name 'govdata' to 'gov_data'?
> - govdata -> gov_data
> 
>> +	int ret;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	priv->clk = devm_clk_get(dev, NULL);
> 
> nitpick: because the clock-name is not mandatory.
> Don't need to specify the clock name to inform the role of clock
> of other developer/user?
> 
> For example, "ddr", "bus" and so on.

And, this driver doesn't include the 'clk_prepare_enable'.
how to enable the clock?

> 
>> +	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_devfreq_target;
>> +	priv->profile.get_dev_status = imx_devfreq_get_dev_status;
>> +	priv->profile.exit = imx_devfreq_exit;
>> +	priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
>> +	priv->profile.initial_freq = clk_get_rate(priv->clk);
>> +
>> +	/* Handle passive devfreq parent link */
>> +	priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
>> +	if (!IS_ERR(priv->passive_data.parent)) {
>> +		dev_info(dev, "setup passive link to %s\n",
>> +			 dev_name(priv->passive_data.parent->dev.parent));
>> +		gov = DEVFREQ_GOV_PASSIVE;
>> +		govdata = &priv->passive_data;
>> +	} else if (priv->passive_data.parent != ERR_PTR(-ENODEV)) {
>> +		// -ENODEV means no parent: not an error.
>> +		ret = PTR_ERR(priv->passive_data.parent);
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_warn(dev, "failed to get initialize passive parent: %d\n",
>> +				 ret);
>> +		goto err;
>> +	}
> 
> You better to change the exception handling as following: It is more simple.
> 
> 	} else if (PTR_ERR(priv->passive_data.parent) == -EPROBE_DEFER)
> 			|| PTR_ERR(priv->passive_data.parent) == -ENODEV) {
> 		goto err;
> 	} else {
> 		ret = PTR_ERR(priv->passive_data.parent);
> 		dev_err(dev, "failed to get initialize passive parent: %d\n", ret);
> 		goto err;
> 	}
> 
>> +
>> +	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
>> +						gov, govdata);
>> +	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_devfreq_of_match[] = {
>> +	{ .compatible = "fsl,imx8m-noc", },
>> +	{ .compatible = "fsl,imx8m-nic", },
>> +	{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
>> +
>> +static struct platform_driver imx_devfreq_platdrv = {
>> +	.probe		= imx_devfreq_probe,
>> +	.driver = {
>> +		.name	= "imx-devfreq",
>> +		.of_match_table = of_match_ptr(imx_devfreq_of_match),
>> +	},
>> +};
>> +module_platform_driver(imx_devfreq_platdrv);
>> +
>> +MODULE_DESCRIPTION("Generic i.MX bus frequency driver");
> 
> If this driver is for bus frequency, you better to use 'bus' for the clock-name
> for the readability.
> 
>> +MODULE_AUTHOR("Leonard Crestez <leonard.crestez@nxp.com>");
>> +MODULE_LICENSE("GPL v2");
>>
> 
>
Chanwoo Choi Dec. 16, 2019, 1:06 a.m. UTC | #10
Hi,

Also, I think that 'devfreq' word is not proper for device driver name.
imx-bus.c or imx-noc.c or others to inform the role of this driver of developer.

And, I have a question.
This driver adds the devfreq device with either passive governor
or userspace governor.

As I understood, the devfreq device with passive governor
will be operated with imx8m-ddrc.c driver.
But, when is operating with userspace governor?

I think that you better to add the explanation to description
for two scenarios how to operate with interconnect provider
on either passive governor or userspace governor usage case.

On 12/13/19 10:51 AM, Chanwoo Choi wrote:
> On 12/13/19 10:30 AM, Chanwoo Choi wrote:
>> Hi,
>>
>> On 11/15/19 5:09 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-devfreq.c | 150 ++++++++++++++++++++++++++++++++++
>>>  3 files changed, 160 insertions(+)
>>>  create mode 100644 drivers/devfreq/imx-devfreq.c
>>>
>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>> index 923a6132e741..fef5ce831e90 100644
>>> --- a/drivers/devfreq/Kconfig
>>> +++ b/drivers/devfreq/Kconfig
>>> @@ -98,10 +98,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_DEVFREQ
>>> +	tristate "i.MX Generic 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..61d0edee16f7 100644
>>> --- a/drivers/devfreq/Makefile
>>> +++ b/drivers/devfreq/Makefile
>>> @@ -8,10 +8,11 @@ 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_IMX8M_DDRC_DEVFREQ)	+= imx8m-ddrc.o
>>> +obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.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
>>>  
>>>  # DEVFREQ Event Drivers
>>> diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c
>>> new file mode 100644
>>> index 000000000000..620b344e87aa
>>> --- /dev/null
>>> +++ b/drivers/devfreq/imx-devfreq.c
>>> @@ -0,0 +1,150 @@
>>> +// 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_devfreq {
>>> +	struct devfreq_dev_profile profile;
>>> +	struct devfreq *devfreq;
>>> +	struct clk *clk;
>>> +	struct devfreq_passive_data passive_data;
>>> +};
>>> +
>>> +static int imx_devfreq_target(struct device *dev,
>>> +			      unsigned long *freq, u32 flags)
>>
>> Don't use space for the indentation. Please use only tab.
>>
>>> +{
>>> +	struct imx_devfreq *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_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
>>> +{
>>> +	struct imx_devfreq *priv = dev_get_drvdata(dev);
>>> +
>>> +	*freq = clk_get_rate(priv->clk);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int imx_devfreq_get_dev_status(struct device *dev,
>>> +				      struct devfreq_dev_status *stat)
>>
>> ditto. Please use tab for the indentation.
>>
>>> +{
>>> +	struct imx_devfreq *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_devfreq_exit(struct device *dev)
>>> +{
>>> +	dev_pm_opp_of_remove_table(dev);
>>> +}
>>> +
>>> +static int imx_devfreq_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	struct imx_devfreq *priv;
>>
>> How about changing the variable name 'priv' to 'imx' or 'imx_data'?
>> because it is not easy to catch the role of 'priv' from variable name.
>>
>>> +	const char *gov = DEVFREQ_GOV_USERSPACE;
>>> +	void *govdata = NULL;
>>
>> How about changing the variable name 'govdata' to 'gov_data'?
>> - govdata -> gov_data
>>
>>> +	int ret;
>>> +
>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> +	if (!priv)
>>> +		return -ENOMEM;
>>> +
>>> +	priv->clk = devm_clk_get(dev, NULL);
>>
>> nitpick: because the clock-name is not mandatory.
>> Don't need to specify the clock name to inform the role of clock
>> of other developer/user?
>>
>> For example, "ddr", "bus" and so on.
> 
> And, this driver doesn't include the 'clk_prepare_enable'.
> how to enable the clock?
> 
>>
>>> +	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_devfreq_target;
>>> +	priv->profile.get_dev_status = imx_devfreq_get_dev_status;
>>> +	priv->profile.exit = imx_devfreq_exit;
>>> +	priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
>>> +	priv->profile.initial_freq = clk_get_rate(priv->clk);
>>> +
>>> +	/* Handle passive devfreq parent link */
>>> +	priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
>>> +	if (!IS_ERR(priv->passive_data.parent)) {
>>> +		dev_info(dev, "setup passive link to %s\n",
>>> +			 dev_name(priv->passive_data.parent->dev.parent));
>>> +		gov = DEVFREQ_GOV_PASSIVE;
>>> +		govdata = &priv->passive_data;
>>> +	} else if (priv->passive_data.parent != ERR_PTR(-ENODEV)) {
>>> +		// -ENODEV means no parent: not an error.
>>> +		ret = PTR_ERR(priv->passive_data.parent);
>>> +		if (ret != -EPROBE_DEFER)
>>> +			dev_warn(dev, "failed to get initialize passive parent: %d\n",
>>> +				 ret);
>>> +		goto err;
>>> +	}
>>
>> You better to change the exception handling as following: It is more simple.
>>
>> 	} else if (PTR_ERR(priv->passive_data.parent) == -EPROBE_DEFER)
>> 			|| PTR_ERR(priv->passive_data.parent) == -ENODEV) {
>> 		goto err;
>> 	} else {
>> 		ret = PTR_ERR(priv->passive_data.parent);
>> 		dev_err(dev, "failed to get initialize passive parent: %d\n", ret);
>> 		goto err;
>> 	}
>>
>>> +
>>> +	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
>>> +						gov, govdata);
>>> +	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_devfreq_of_match[] = {
>>> +	{ .compatible = "fsl,imx8m-noc", },
>>> +	{ .compatible = "fsl,imx8m-nic", },
>>> +	{ /* sentinel */ },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
>>> +
>>> +static struct platform_driver imx_devfreq_platdrv = {
>>> +	.probe		= imx_devfreq_probe,
>>> +	.driver = {
>>> +		.name	= "imx-devfreq",
>>> +		.of_match_table = of_match_ptr(imx_devfreq_of_match),
>>> +	},
>>> +};
>>> +module_platform_driver(imx_devfreq_platdrv);
>>> +
>>> +MODULE_DESCRIPTION("Generic i.MX bus frequency driver");
>>
>> If this driver is for bus frequency, you better to use 'bus' for the clock-name
>> for the readability.
>>
>>> +MODULE_AUTHOR("Leonard Crestez <leonard.crestez@nxp.com>");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
>>
> 
>
Leonard Crestez Dec. 16, 2019, 2:57 p.m. UTC | #11
On 16.12.2019 03:00, Chanwoo Choi wrote:
> Hi,
> 
> Also, I think that 'devfreq' word is not proper for device driver name.
> imx-bus.c or imx-noc.c or others to inform the role of this driver of developer.

I'll rename to "imx-bus". Calling it "imx-noc" is not appropriate 
because I also want to use it for PL301 NICs.

> And, I have a question.
> This driver adds the devfreq device with either passive governor
> or userspace governor.
> 
> As I understood, the devfreq device with passive governor
> will be operated with imx8m-ddrc.c driver.
> But, when is operating with userspace governor?

There are multiple scalable buses inside the SOC, for example there's a 
NIC for display controllers and one for (pci+usb). They can use 
userspace governor for explicit frequency control.

> I think that you better to add the explanation to description
> for two scenarios how to operate with interconnect provider
> on either passive governor or userspace governor usage case.

I'll elaborate the example in bindings.

> On 12/13/19 10:51 AM, Chanwoo Choi wrote:
>> On 12/13/19 10:30 AM, Chanwoo Choi wrote:
>>> Hi,
>>>
>>> On 11/15/19 5:09 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-devfreq.c | 150 ++++++++++++++++++++++++++++++++++
>>>>   3 files changed, 160 insertions(+)
>>>>   create mode 100644 drivers/devfreq/imx-devfreq.c
>>>>
>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>>> index 923a6132e741..fef5ce831e90 100644
>>>> --- a/drivers/devfreq/Kconfig
>>>> +++ b/drivers/devfreq/Kconfig
>>>> @@ -98,10 +98,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_DEVFREQ
>>>> +	tristate "i.MX Generic 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..61d0edee16f7 100644
>>>> --- a/drivers/devfreq/Makefile
>>>> +++ b/drivers/devfreq/Makefile
>>>> @@ -8,10 +8,11 @@ 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_IMX8M_DDRC_DEVFREQ)	+= imx8m-ddrc.o
>>>> +obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.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
>>>>   
>>>>   # DEVFREQ Event Drivers
>>>> diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c
>>>> new file mode 100644
>>>> index 000000000000..620b344e87aa
>>>> --- /dev/null
>>>> +++ b/drivers/devfreq/imx-devfreq.c
>>>> @@ -0,0 +1,150 @@
>>>> +// 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_devfreq {
>>>> +	struct devfreq_dev_profile profile;
>>>> +	struct devfreq *devfreq;
>>>> +	struct clk *clk;
>>>> +	struct devfreq_passive_data passive_data;
>>>> +};
>>>> +
>>>> +static int imx_devfreq_target(struct device *dev,
>>>> +			      unsigned long *freq, u32 flags)
>>>
>>> Don't use space for the indentation. Please use only tab.

OK

>>>> +{
>>>> +	struct imx_devfreq *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_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
>>>> +{
>>>> +	struct imx_devfreq *priv = dev_get_drvdata(dev);
>>>> +
>>>> +	*freq = clk_get_rate(priv->clk);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int imx_devfreq_get_dev_status(struct device *dev,
>>>> +				      struct devfreq_dev_status *stat)
>>>
>>> ditto. Please use tab for the indentation.
>>>
>>>> +{
>>>> +	struct imx_devfreq *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_devfreq_exit(struct device *dev)
>>>> +{
>>>> +	dev_pm_opp_of_remove_table(dev);
>>>> +}
>>>> +
>>>> +static int imx_devfreq_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct device *dev = &pdev->dev;
>>>> +	struct imx_devfreq *priv;
>>>
>>> How about changing the variable name 'priv' to 'imx' or 'imx_data'?
>>> because it is not easy to catch the role of 'priv' from variable name.

The name "priv" refers to private data of current device: it is short 
and not ambiguous in this context. I don't think that mentioning "imx" 
adds any additional useful information.

It doesn't seem like there's much of a convention for "local variable 
containing private data", for example exynos-bus.c uses "struct 
exynos_bus* bus" internally.

>>>
>>>> +	const char *gov = DEVFREQ_GOV_USERSPACE;
>>>> +	void *govdata = NULL;
>>>
>>> How about changing the variable name 'govdata' to 'gov_data'?
>>> - govdata -> gov_data

OK

>>>> +	int ret;
>>>> +
>>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>> +	if (!priv)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	priv->clk = devm_clk_get(dev, NULL);
>>>
>>> nitpick: because the clock-name is not mandatory.
>>> Don't need to specify the clock name to inform the role of clock
>>> of other developer/user?
>>>
>>> For example, "ddr", "bus" and so on.

I'll call this bus, but I'm not sure it's useful when a single clock is 
involved.

>> And, this driver doesn't include the 'clk_prepare_enable'.
>> how to enable the clock?

Clocks are either always on or perhaps controlled by some other 
peripheral. This driver only provides scaling.

>>>> +	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_devfreq_target;
>>>> +	priv->profile.get_dev_status = imx_devfreq_get_dev_status;
>>>> +	priv->profile.exit = imx_devfreq_exit;
>>>> +	priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
>>>> +	priv->profile.initial_freq = clk_get_rate(priv->clk);
>>>> +
>>>> +	/* Handle passive devfreq parent link */
>>>> +	priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
>>>> +	if (!IS_ERR(priv->passive_data.parent)) {
>>>> +		dev_info(dev, "setup passive link to %s\n",
>>>> +			 dev_name(priv->passive_data.parent->dev.parent));
>>>> +		gov = DEVFREQ_GOV_PASSIVE;
>>>> +		govdata = &priv->passive_data;
>>>> +	} else if (priv->passive_data.parent != ERR_PTR(-ENODEV)) {
>>>> +		// -ENODEV means no parent: not an error.
>>>> +		ret = PTR_ERR(priv->passive_data.parent);
>>>> +		if (ret != -EPROBE_DEFER)
>>>> +			dev_warn(dev, "failed to get initialize passive parent: %d\n",
>>>> +				 ret);
>>>> +		goto err;
>>>> +	}
>>>
>>> You better to change the exception handling as following: It is more simple.
>>>
>>> 	} else if (PTR_ERR(priv->passive_data.parent) == -EPROBE_DEFER)
>>> 			|| PTR_ERR(priv->passive_data.parent) == -ENODEV) {
>>> 		goto err;
>>> 	} else {
>>> 		ret = PTR_ERR(priv->passive_data.parent);
>>> 		dev_err(dev, "failed to get initialize passive parent: %d\n", ret);
>>> 		goto err;
>>> 	}

But -ENODEV is not an error, it means no passive parent was found.

>>>> +	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
>>>> +						gov, govdata);
>>>> +	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_devfreq_of_match[] = {
>>>> +	{ .compatible = "fsl,imx8m-noc", },
>>>> +	{ .compatible = "fsl,imx8m-nic", },
>>>> +	{ /* sentinel */ },
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
>>>> +
>>>> +static struct platform_driver imx_devfreq_platdrv = {
>>>> +	.probe		= imx_devfreq_probe,
>>>> +	.driver = {
>>>> +		.name	= "imx-devfreq",
>>>> +		.of_match_table = of_match_ptr(imx_devfreq_of_match),
>>>> +	},
>>>> +};
>>>> +module_platform_driver(imx_devfreq_platdrv);
>>>> +
>>>> +MODULE_DESCRIPTION("Generic i.MX bus frequency driver");
>>>
>>> If this driver is for bus frequency, you better to use 'bus' for the clock-name
>>> for the readability.

OK
Chanwoo Choi Dec. 17, 2019, 12:41 a.m. UTC | #12
On 12/16/19 11:57 PM, Leonard Crestez wrote:
> On 16.12.2019 03:00, Chanwoo Choi wrote:
>> Hi,
>>
>> Also, I think that 'devfreq' word is not proper for device driver name.
>> imx-bus.c or imx-noc.c or others to inform the role of this driver of developer.
> 
> I'll rename to "imx-bus". Calling it "imx-noc" is not appropriate 
> because I also want to use it for PL301 NICs.

OK.

> 
>> And, I have a question.
>> This driver adds the devfreq device with either passive governor
>> or userspace governor.
>>
>> As I understood, the devfreq device with passive governor
>> will be operated with imx8m-ddrc.c driver.
>> But, when is operating with userspace governor?
> 
> There are multiple scalable buses inside the SOC, for example there's a 
> NIC for display controllers and one for (pci+usb). They can use 
> userspace governor for explicit frequency control.
> 
>> I think that you better to add the explanation to description
>> for two scenarios how to operate with interconnect provider
>> on either passive governor or userspace governor usage case.
> 
> I'll elaborate the example in bindings.

OK.

> 
>> On 12/13/19 10:51 AM, Chanwoo Choi wrote:
>>> On 12/13/19 10:30 AM, Chanwoo Choi wrote:
>>>> Hi,
>>>>
>>>> On 11/15/19 5:09 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-devfreq.c | 150 ++++++++++++++++++++++++++++++++++
>>>>>   3 files changed, 160 insertions(+)
>>>>>   create mode 100644 drivers/devfreq/imx-devfreq.c
>>>>>
>>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>>>> index 923a6132e741..fef5ce831e90 100644
>>>>> --- a/drivers/devfreq/Kconfig
>>>>> +++ b/drivers/devfreq/Kconfig
>>>>> @@ -98,10 +98,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_DEVFREQ
>>>>> +	tristate "i.MX Generic 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..61d0edee16f7 100644
>>>>> --- a/drivers/devfreq/Makefile
>>>>> +++ b/drivers/devfreq/Makefile
>>>>> @@ -8,10 +8,11 @@ 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_IMX8M_DDRC_DEVFREQ)	+= imx8m-ddrc.o
>>>>> +obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.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
>>>>>   
>>>>>   # DEVFREQ Event Drivers
>>>>> diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c
>>>>> new file mode 100644
>>>>> index 000000000000..620b344e87aa
>>>>> --- /dev/null
>>>>> +++ b/drivers/devfreq/imx-devfreq.c
>>>>> @@ -0,0 +1,150 @@
>>>>> +// 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_devfreq {
>>>>> +	struct devfreq_dev_profile profile;
>>>>> +	struct devfreq *devfreq;
>>>>> +	struct clk *clk;
>>>>> +	struct devfreq_passive_data passive_data;
>>>>> +};
>>>>> +
>>>>> +static int imx_devfreq_target(struct device *dev,
>>>>> +			      unsigned long *freq, u32 flags)
>>>>
>>>> Don't use space for the indentation. Please use only tab.
> 
> OK
> 
>>>>> +{
>>>>> +	struct imx_devfreq *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_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
>>>>> +{
>>>>> +	struct imx_devfreq *priv = dev_get_drvdata(dev);
>>>>> +
>>>>> +	*freq = clk_get_rate(priv->clk);
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int imx_devfreq_get_dev_status(struct device *dev,
>>>>> +				      struct devfreq_dev_status *stat)
>>>>
>>>> ditto. Please use tab for the indentation.
>>>>
>>>>> +{
>>>>> +	struct imx_devfreq *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_devfreq_exit(struct device *dev)
>>>>> +{
>>>>> +	dev_pm_opp_of_remove_table(dev);
>>>>> +}
>>>>> +
>>>>> +static int imx_devfreq_probe(struct platform_device *pdev)
>>>>> +{
>>>>> +	struct device *dev = &pdev->dev;
>>>>> +	struct imx_devfreq *priv;
>>>>
>>>> How about changing the variable name 'priv' to 'imx' or 'imx_data'?
>>>> because it is not easy to catch the role of 'priv' from variable name.
> 
> The name "priv" refers to private data of current device: it is short 
> and not ambiguous in this context. I don't think that mentioning "imx" 
> adds any additional useful information.
> 
> It doesn't seem like there's much of a convention for "local variable 
> containing private data", for example exynos-bus.c uses "struct 
> exynos_bus* bus" internally.

OK. it is nitpick. Keep your style.

> 
>>>>
>>>>> +	const char *gov = DEVFREQ_GOV_USERSPACE;
>>>>> +	void *govdata = NULL;
>>>>
>>>> How about changing the variable name 'govdata' to 'gov_data'?
>>>> - govdata -> gov_data
> 
> OK
> 
>>>>> +	int ret;
>>>>> +
>>>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>>> +	if (!priv)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	priv->clk = devm_clk_get(dev, NULL);
>>>>
>>>> nitpick: because the clock-name is not mandatory.
>>>> Don't need to specify the clock name to inform the role of clock
>>>> of other developer/user?
>>>>
>>>> For example, "ddr", "bus" and so on.
> 
> I'll call this bus, but I'm not sure it's useful when a single clock is 
> involved.
> 
>>> And, this driver doesn't include the 'clk_prepare_enable'.
>>> how to enable the clock?
> 
> Clocks are either always on or perhaps controlled by some other 
> peripheral. This driver only provides scaling.

It is not proper use-case of clock. If device driver
want to control the clock, it have to be enabled on device driver.

Even it clock is always, the user don't know the state of clock.
Also, user can't know what kind of device driver control the clock.

It have to be controlled on this device driver
before changing the clock frequency.

> 
>>>>> +	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_devfreq_target;
>>>>> +	priv->profile.get_dev_status = imx_devfreq_get_dev_status;
>>>>> +	priv->profile.exit = imx_devfreq_exit;
>>>>> +	priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
>>>>> +	priv->profile.initial_freq = clk_get_rate(priv->clk);
>>>>> +
>>>>> +	/* Handle passive devfreq parent link */
>>>>> +	priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
>>>>> +	if (!IS_ERR(priv->passive_data.parent)) {
>>>>> +		dev_info(dev, "setup passive link to %s\n",
>>>>> +			 dev_name(priv->passive_data.parent->dev.parent));
>>>>> +		gov = DEVFREQ_GOV_PASSIVE;
>>>>> +		govdata = &priv->passive_data;
>>>>> +	} else if (priv->passive_data.parent != ERR_PTR(-ENODEV)) {
>>>>> +		// -ENODEV means no parent: not an error.
>>>>> +		ret = PTR_ERR(priv->passive_data.parent);
>>>>> +		if (ret != -EPROBE_DEFER)
>>>>> +			dev_warn(dev, "failed to get initialize passive parent: %d\n",
>>>>> +				 ret);
>>>>> +		goto err;
>>>>> +	}
>>>>
>>>> You better to change the exception handling as following: It is more simple.
>>>>
>>>> 	} else if (PTR_ERR(priv->passive_data.parent) == -EPROBE_DEFER)
>>>> 			|| PTR_ERR(priv->passive_data.parent) == -ENODEV) {
>>>> 		goto err;
>>>> 	} else {
>>>> 		ret = PTR_ERR(priv->passive_data.parent);
>>>> 		dev_err(dev, "failed to get initialize passive parent: %d\n", ret);
>>>> 		goto err;
>>>> 	}
> 
> But -ENODEV is not an error, it means no passive parent was found.

OK. just I want to make 'if statement' more simple. This style
is complicated.

> 
>>>>> +	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
>>>>> +						gov, govdata);
>>>>> +	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_devfreq_of_match[] = {
>>>>> +	{ .compatible = "fsl,imx8m-noc", },
>>>>> +	{ .compatible = "fsl,imx8m-nic", },
>>>>> +	{ /* sentinel */ },
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
>>>>> +
>>>>> +static struct platform_driver imx_devfreq_platdrv = {
>>>>> +	.probe		= imx_devfreq_probe,
>>>>> +	.driver = {
>>>>> +		.name	= "imx-devfreq",
>>>>> +		.of_match_table = of_match_ptr(imx_devfreq_of_match),
>>>>> +	},
>>>>> +};
>>>>> +module_platform_driver(imx_devfreq_platdrv);
>>>>> +
>>>>> +MODULE_DESCRIPTION("Generic i.MX bus frequency driver");
>>>>
>>>> If this driver is for bus frequency, you better to use 'bus' for the clock-name
>>>> for the readability.
> 
> OK
> 
>
Leonard Crestez Dec. 17, 2019, 9:05 p.m. UTC | #13
On 17.12.2019 02:35, Chanwoo Choi wrote:
> On 12/16/19 11:57 PM, Leonard Crestez wrote:
>> On 16.12.2019 03:00, Chanwoo Choi wrote:
>>> Hi,
>>>
>>> Also, I think that 'devfreq' word is not proper for device driver name.
>>> imx-bus.c or imx-noc.c or others to inform the role of this driver of developer.
>>
>> I'll rename to "imx-bus". Calling it "imx-noc" is not appropriate
>> because I also want to use it for PL301 NICs.
> 
> OK.
> 
>>
>>> And, I have a question.
>>> This driver adds the devfreq device with either passive governor
>>> or userspace governor.
>>>
>>> As I understood, the devfreq device with passive governor
>>> will be operated with imx8m-ddrc.c driver.
>>> But, when is operating with userspace governor?
>>
>> There are multiple scalable buses inside the SOC, for example there's a
>> NIC for display controllers and one for (pci+usb). They can use
>> userspace governor for explicit frequency control.
>>
>>> I think that you better to add the explanation to description
>>> for two scenarios how to operate with interconnect provider
>>> on either passive governor or userspace governor usage case.
>>
>> I'll elaborate the example in bindings.
> 
> OK.
> 
>>
>>> On 12/13/19 10:51 AM, Chanwoo Choi wrote:
>>>> On 12/13/19 10:30 AM, Chanwoo Choi wrote:
>>>>> Hi,
>>>>>
>>>>> On 11/15/19 5:09 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-devfreq.c | 150 ++++++++++++++++++++++++++++++++++
>>>>>>    3 files changed, 160 insertions(+)
>>>>>>    create mode 100644 drivers/devfreq/imx-devfreq.c
>>>>>>
>>>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>>>>> index 923a6132e741..fef5ce831e90 100644
>>>>>> --- a/drivers/devfreq/Kconfig
>>>>>> +++ b/drivers/devfreq/Kconfig
>>>>>> @@ -98,10 +98,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_DEVFREQ
>>>>>> +	tristate "i.MX Generic 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..61d0edee16f7 100644
>>>>>> --- a/drivers/devfreq/Makefile
>>>>>> +++ b/drivers/devfreq/Makefile
>>>>>> @@ -8,10 +8,11 @@ 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_IMX8M_DDRC_DEVFREQ)	+= imx8m-ddrc.o
>>>>>> +obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.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
>>>>>>    
>>>>>>    # DEVFREQ Event Drivers
>>>>>> diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c
>>>>>> new file mode 100644
>>>>>> index 000000000000..620b344e87aa
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/devfreq/imx-devfreq.c
>>>>>> @@ -0,0 +1,150 @@
>>>>>> +// 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_devfreq {
>>>>>> +	struct devfreq_dev_profile profile;
>>>>>> +	struct devfreq *devfreq;
>>>>>> +	struct clk *clk;
>>>>>> +	struct devfreq_passive_data passive_data;
>>>>>> +};
>>>>>> +
>>>>>> +static int imx_devfreq_target(struct device *dev,
>>>>>> +			      unsigned long *freq, u32 flags)
>>>>>
>>>>> Don't use space for the indentation. Please use only tab.
>>
>> OK

The spaces are required in order to align arguments to open paranthesis. 
Should I drop that?

It seems that check_patch.pl and process/coding-style.rst doesn't have a 
strong opinion on this; my personal preference is for long argument 
lists to just use double indentation.

>>>>>> +{
>>>>>> +	struct imx_devfreq *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_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
>>>>>> +{
>>>>>> +	struct imx_devfreq *priv = dev_get_drvdata(dev);
>>>>>> +
>>>>>> +	*freq = clk_get_rate(priv->clk);
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int imx_devfreq_get_dev_status(struct device *dev,
>>>>>> +				      struct devfreq_dev_status *stat)
>>>>>
>>>>> ditto. Please use tab for the indentation.
>>>>>
>>>>>> +{
>>>>>> +	struct imx_devfreq *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_devfreq_exit(struct device *dev)
>>>>>> +{
>>>>>> +	dev_pm_opp_of_remove_table(dev);
>>>>>> +}
>>>>>> +
>>>>>> +static int imx_devfreq_probe(struct platform_device *pdev)
>>>>>> +{
>>>>>> +	struct device *dev = &pdev->dev;
>>>>>> +	struct imx_devfreq *priv;
>>>>>
>>>>> How about changing the variable name 'priv' to 'imx' or 'imx_data'?
>>>>> because it is not easy to catch the role of 'priv' from variable name.
>>
>> The name "priv" refers to private data of current device: it is short
>> and not ambiguous in this context. I don't think that mentioning "imx"
>> adds any additional useful information.
>>
>> It doesn't seem like there's much of a convention for "local variable
>> containing private data", for example exynos-bus.c uses "struct
>> exynos_bus* bus" internally.
> 
> OK. it is nitpick. Keep your style.
> 
>>
>>>>>
>>>>>> +	const char *gov = DEVFREQ_GOV_USERSPACE;
>>>>>> +	void *govdata = NULL;
>>>>>
>>>>> How about changing the variable name 'govdata' to 'gov_data'?
>>>>> - govdata -> gov_data
>>
>> OK
>>
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>>>> +	if (!priv)
>>>>>> +		return -ENOMEM;
>>>>>> +
>>>>>> +	priv->clk = devm_clk_get(dev, NULL);
>>>>>
>>>>> nitpick: because the clock-name is not mandatory.
>>>>> Don't need to specify the clock name to inform the role of clock
>>>>> of other developer/user?
>>>>>
>>>>> For example, "ddr", "bus" and so on.
>>
>> I'll call this bus, but I'm not sure it's useful when a single clock is
>> involved.
>>
>>>> And, this driver doesn't include the 'clk_prepare_enable'.
>>>> how to enable the clock?
>>
>> Clocks are either always on or perhaps controlled by some other
>> peripheral. This driver only provides scaling.
> 
> It is not proper use-case of clock. If device driver
> want to control the clock, it have to be enabled on device driver.

> Even it clock is always, the user don't know the state of clock.
> Also, user can't know what kind of device driver control the clock.
> 
> It have to be controlled on this device driver
> before changing the clock frequency.

 From clock framework perspective prepare/enable and rate bits can be 
controlled separately.

Many peripherals are grouped with their own bus (for example a PL301 
NIC) which is normally off and only gets enabled when explicitly 
requested by drivers. If this devfreq driver always enabled bus clocks 
then it would waste power for no reason.

For example a display controller will first enable clocks to allow 
access to device registers, then configure a resolution and make a 
bandwith request which gets translated a min_freq request. Then when the 
display is blanked the entire display bus should be powered off, even if 
this makes control registers inaccessible.

This series only enables scaling for the main NOC which can't be turned 
off anyway.

>>>>>> +	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_devfreq_target;
>>>>>> +	priv->profile.get_dev_status = imx_devfreq_get_dev_status;
>>>>>> +	priv->profile.exit = imx_devfreq_exit;
>>>>>> +	priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
>>>>>> +	priv->profile.initial_freq = clk_get_rate(priv->clk);
>>>>>> +
>>>>>> +	/* Handle passive devfreq parent link */
>>>>>> +	priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
>>>>>> +	if (!IS_ERR(priv->passive_data.parent)) {
>>>>>> +		dev_info(dev, "setup passive link to %s\n",
>>>>>> +			 dev_name(priv->passive_data.parent->dev.parent));
>>>>>> +		gov = DEVFREQ_GOV_PASSIVE;
>>>>>> +		govdata = &priv->passive_data;
>>>>>> +	} else if (priv->passive_data.parent != ERR_PTR(-ENODEV)) {
>>>>>> +		// -ENODEV means no parent: not an error.
>>>>>> +		ret = PTR_ERR(priv->passive_data.parent);
>>>>>> +		if (ret != -EPROBE_DEFER)
>>>>>> +			dev_warn(dev, "failed to get initialize passive parent: %d\n",
>>>>>> +				 ret);
>>>>>> +		goto err;
>>>>>> +	}
>>>>>
>>>>> You better to change the exception handling as following: It is more simple.
>>>>>
>>>>> 	} else if (PTR_ERR(priv->passive_data.parent) == -EPROBE_DEFER)
>>>>> 			|| PTR_ERR(priv->passive_data.parent) == -ENODEV) {
>>>>> 		goto err;
>>>>> 	} else {
>>>>> 		ret = PTR_ERR(priv->passive_data.parent);
>>>>> 		dev_err(dev, "failed to get initialize passive parent: %d\n", ret);
>>>>> 		goto err;
>>>>> 	}
>>
>> But -ENODEV is not an error, it means no passive parent was found.
> 
> OK. just I want to make 'if statement' more simple. This style
> is complicated.

I can avoid handling EPROBE_DEFER in a nested if.

>>>>>> +	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
>>>>>> +						gov, govdata);
>>>>>> +	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_devfreq_of_match[] = {
>>>>>> +	{ .compatible = "fsl,imx8m-noc", },
>>>>>> +	{ .compatible = "fsl,imx8m-nic", },
>>>>>> +	{ /* sentinel */ },
>>>>>> +};
>>>>>> +MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
>>>>>> +
>>>>>> +static struct platform_driver imx_devfreq_platdrv = {
>>>>>> +	.probe		= imx_devfreq_probe,
>>>>>> +	.driver = {
>>>>>> +		.name	= "imx-devfreq",
>>>>>> +		.of_match_table = of_match_ptr(imx_devfreq_of_match),
>>>>>> +	},
>>>>>> +};
>>>>>> +module_platform_driver(imx_devfreq_platdrv);
>>>>>> +
>>>>>> +MODULE_DESCRIPTION("Generic i.MX bus frequency driver");
>>>>>
>>>>> If this driver is for bus frequency, you better to use 'bus' for the clock-name
>>>>> for the readability.
Chanwoo Choi Dec. 18, 2019, 3:15 a.m. UTC | #14
On 12/18/19 6:05 AM, Leonard Crestez wrote:
> On 17.12.2019 02:35, Chanwoo Choi wrote:
>> On 12/16/19 11:57 PM, Leonard Crestez wrote:
>>> On 16.12.2019 03:00, Chanwoo Choi wrote:
>>>> Hi,
>>>>
>>>> Also, I think that 'devfreq' word is not proper for device driver name.
>>>> imx-bus.c or imx-noc.c or others to inform the role of this driver of developer.
>>>
>>> I'll rename to "imx-bus". Calling it "imx-noc" is not appropriate
>>> because I also want to use it for PL301 NICs.
>>
>> OK.
>>
>>>
>>>> And, I have a question.
>>>> This driver adds the devfreq device with either passive governor
>>>> or userspace governor.
>>>>
>>>> As I understood, the devfreq device with passive governor
>>>> will be operated with imx8m-ddrc.c driver.
>>>> But, when is operating with userspace governor?
>>>
>>> There are multiple scalable buses inside the SOC, for example there's a
>>> NIC for display controllers and one for (pci+usb). They can use
>>> userspace governor for explicit frequency control.
>>>
>>>> I think that you better to add the explanation to description
>>>> for two scenarios how to operate with interconnect provider
>>>> on either passive governor or userspace governor usage case.
>>>
>>> I'll elaborate the example in bindings.
>>
>> OK.
>>
>>>
>>>> On 12/13/19 10:51 AM, Chanwoo Choi wrote:
>>>>> On 12/13/19 10:30 AM, Chanwoo Choi wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 11/15/19 5:09 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-devfreq.c | 150 ++++++++++++++++++++++++++++++++++
>>>>>>>    3 files changed, 160 insertions(+)
>>>>>>>    create mode 100644 drivers/devfreq/imx-devfreq.c
>>>>>>>
>>>>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>>>>>> index 923a6132e741..fef5ce831e90 100644
>>>>>>> --- a/drivers/devfreq/Kconfig
>>>>>>> +++ b/drivers/devfreq/Kconfig
>>>>>>> @@ -98,10 +98,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_DEVFREQ
>>>>>>> +	tristate "i.MX Generic 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..61d0edee16f7 100644
>>>>>>> --- a/drivers/devfreq/Makefile
>>>>>>> +++ b/drivers/devfreq/Makefile
>>>>>>> @@ -8,10 +8,11 @@ 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_IMX8M_DDRC_DEVFREQ)	+= imx8m-ddrc.o
>>>>>>> +obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.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
>>>>>>>    
>>>>>>>    # DEVFREQ Event Drivers
>>>>>>> diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..620b344e87aa
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/devfreq/imx-devfreq.c
>>>>>>> @@ -0,0 +1,150 @@
>>>>>>> +// 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_devfreq {
>>>>>>> +	struct devfreq_dev_profile profile;
>>>>>>> +	struct devfreq *devfreq;
>>>>>>> +	struct clk *clk;
>>>>>>> +	struct devfreq_passive_data passive_data;
>>>>>>> +};
>>>>>>> +
>>>>>>> +static int imx_devfreq_target(struct device *dev,
>>>>>>> +			      unsigned long *freq, u32 flags)
>>>>>>
>>>>>> Don't use space for the indentation. Please use only tab.
>>>
>>> OK
> 
> The spaces are required in order to align arguments to open paranthesis. 
> Should I drop that?
> 
> It seems that check_patch.pl and process/coding-style.rst doesn't have a 
> strong opinion on this; my personal preference is for long argument 
> lists to just use double indentation.

Generally, almost patches use the tab for the indentation.
I don't use space for the indentation. If use the tab
for the indentation, it is not harmful for the readability.

If use the space for the pretty to make the alignment between parameter,
I think it it not good.

> 
>>>>>>> +{
>>>>>>> +	struct imx_devfreq *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_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
>>>>>>> +{
>>>>>>> +	struct imx_devfreq *priv = dev_get_drvdata(dev);
>>>>>>> +
>>>>>>> +	*freq = clk_get_rate(priv->clk);
>>>>>>> +
>>>>>>> +	return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int imx_devfreq_get_dev_status(struct device *dev,
>>>>>>> +				      struct devfreq_dev_status *stat)
>>>>>>
>>>>>> ditto. Please use tab for the indentation.
>>>>>>
>>>>>>> +{
>>>>>>> +	struct imx_devfreq *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_devfreq_exit(struct device *dev)
>>>>>>> +{
>>>>>>> +	dev_pm_opp_of_remove_table(dev);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int imx_devfreq_probe(struct platform_device *pdev)
>>>>>>> +{
>>>>>>> +	struct device *dev = &pdev->dev;
>>>>>>> +	struct imx_devfreq *priv;
>>>>>>
>>>>>> How about changing the variable name 'priv' to 'imx' or 'imx_data'?
>>>>>> because it is not easy to catch the role of 'priv' from variable name.
>>>
>>> The name "priv" refers to private data of current device: it is short
>>> and not ambiguous in this context. I don't think that mentioning "imx"
>>> adds any additional useful information.
>>>
>>> It doesn't seem like there's much of a convention for "local variable
>>> containing private data", for example exynos-bus.c uses "struct
>>> exynos_bus* bus" internally.
>>
>> OK. it is nitpick. Keep your style.
>>
>>>
>>>>>>
>>>>>>> +	const char *gov = DEVFREQ_GOV_USERSPACE;
>>>>>>> +	void *govdata = NULL;
>>>>>>
>>>>>> How about changing the variable name 'govdata' to 'gov_data'?
>>>>>> - govdata -> gov_data
>>>
>>> OK
>>>
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>>>>> +	if (!priv)
>>>>>>> +		return -ENOMEM;
>>>>>>> +
>>>>>>> +	priv->clk = devm_clk_get(dev, NULL);
>>>>>>
>>>>>> nitpick: because the clock-name is not mandatory.
>>>>>> Don't need to specify the clock name to inform the role of clock
>>>>>> of other developer/user?
>>>>>>
>>>>>> For example, "ddr", "bus" and so on.
>>>
>>> I'll call this bus, but I'm not sure it's useful when a single clock is
>>> involved.
>>>
>>>>> And, this driver doesn't include the 'clk_prepare_enable'.
>>>>> how to enable the clock?
>>>
>>> Clocks are either always on or perhaps controlled by some other
>>> peripheral. This driver only provides scaling.
>>
>> It is not proper use-case of clock. If device driver
>> want to control the clock, it have to be enabled on device driver.
> 
>> Even it clock is always, the user don't know the state of clock.
>> Also, user can't know what kind of device driver control the clock.
>>
>> It have to be controlled on this device driver
>> before changing the clock frequency.
> 
>  From clock framework perspective prepare/enable and rate bits can be 
> controlled separately.
> 
> Many peripherals are grouped with their own bus (for example a PL301 
> NIC) which is normally off and only gets enabled when explicitly 
> requested by drivers. If this devfreq driver always enabled bus clocks 
> then it would waste power for no reason.

You can save the power with following sequence.
You are keeping the following sequence without any problem.
	clk_prepare_enable()
	clk_set_rate()
	clk_disable_unprepare()

clk API support the reference count for the clock user.
It doesn't affect the any behavior of other device sharing the bus clock
and waste any power-consumption because it will always restore
the reference count after changing the clock rate. 

> 
> For example a display controller will first enable clocks to allow 
> access to device registers, then configure a resolution and make a 
> bandwith request which gets translated a min_freq request. Then when the 
> display is blanked the entire display bus should be powered off, even if 
> this makes control registers inaccessible.
> 
> This series only enables scaling for the main NOC which can't be turned 
> off anyway.
> 
>>>>>>> +	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_devfreq_target;
>>>>>>> +	priv->profile.get_dev_status = imx_devfreq_get_dev_status;
>>>>>>> +	priv->profile.exit = imx_devfreq_exit;
>>>>>>> +	priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
>>>>>>> +	priv->profile.initial_freq = clk_get_rate(priv->clk);
>>>>>>> +
>>>>>>> +	/* Handle passive devfreq parent link */
>>>>>>> +	priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
>>>>>>> +	if (!IS_ERR(priv->passive_data.parent)) {
>>>>>>> +		dev_info(dev, "setup passive link to %s\n",
>>>>>>> +			 dev_name(priv->passive_data.parent->dev.parent));
>>>>>>> +		gov = DEVFREQ_GOV_PASSIVE;
>>>>>>> +		govdata = &priv->passive_data;
>>>>>>> +	} else if (priv->passive_data.parent != ERR_PTR(-ENODEV)) {
>>>>>>> +		// -ENODEV means no parent: not an error.
>>>>>>> +		ret = PTR_ERR(priv->passive_data.parent);
>>>>>>> +		if (ret != -EPROBE_DEFER)
>>>>>>> +			dev_warn(dev, "failed to get initialize passive parent: %d\n",
>>>>>>> +				 ret);
>>>>>>> +		goto err;
>>>>>>> +	}
>>>>>>
>>>>>> You better to change the exception handling as following: It is more simple.
>>>>>>
>>>>>> 	} else if (PTR_ERR(priv->passive_data.parent) == -EPROBE_DEFER)
>>>>>> 			|| PTR_ERR(priv->passive_data.parent) == -ENODEV) {
>>>>>> 		goto err;
>>>>>> 	} else {
>>>>>> 		ret = PTR_ERR(priv->passive_data.parent);
>>>>>> 		dev_err(dev, "failed to get initialize passive parent: %d\n", ret);
>>>>>> 		goto err;
>>>>>> 	}
>>>
>>> But -ENODEV is not an error, it means no passive parent was found.
>>
>> OK. just I want to make 'if statement' more simple. This style
>> is complicated.
> 
> I can avoid handling EPROBE_DEFER in a nested if.

Anyway, if you make the exception more simple, I'm ok. 

> 
>>>>>>> +	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
>>>>>>> +						gov, govdata);
>>>>>>> +	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_devfreq_of_match[] = {
>>>>>>> +	{ .compatible = "fsl,imx8m-noc", },
>>>>>>> +	{ .compatible = "fsl,imx8m-nic", },
>>>>>>> +	{ /* sentinel */ },
>>>>>>> +};
>>>>>>> +MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
>>>>>>> +
>>>>>>> +static struct platform_driver imx_devfreq_platdrv = {
>>>>>>> +	.probe		= imx_devfreq_probe,
>>>>>>> +	.driver = {
>>>>>>> +		.name	= "imx-devfreq",
>>>>>>> +		.of_match_table = of_match_ptr(imx_devfreq_of_match),
>>>>>>> +	},
>>>>>>> +};
>>>>>>> +module_platform_driver(imx_devfreq_platdrv);
>>>>>>> +
>>>>>>> +MODULE_DESCRIPTION("Generic i.MX bus frequency driver");
>>>>>>
>>>>>> If this driver is for bus frequency, you better to use 'bus' for the clock-name
>>>>>> for the readability.
> 
> 
> 
>
Leonard Crestez Dec. 18, 2019, 10:10 a.m. UTC | #15
On 18.12.2019 05:08, Chanwoo Choi wrote:
> On 12/18/19 6:05 AM, Leonard Crestez wrote:
>> On 17.12.2019 02:35, Chanwoo Choi wrote:
>>> On 12/16/19 11:57 PM, Leonard Crestez wrote:
>>>> On 16.12.2019 03:00, Chanwoo Choi wrote:
>>>>> Hi,
>>>>>
>>>>> Also, I think that 'devfreq' word is not proper for device driver name.
>>>>> imx-bus.c or imx-noc.c or others to inform the role of this driver of developer.
>>>>
>>>> I'll rename to "imx-bus". Calling it "imx-noc" is not appropriate
>>>> because I also want to use it for PL301 NICs.
>>>
>>> OK.
>>>
>>>>
>>>>> And, I have a question.
>>>>> This driver adds the devfreq device with either passive governor
>>>>> or userspace governor.
>>>>>
>>>>> As I understood, the devfreq device with passive governor
>>>>> will be operated with imx8m-ddrc.c driver.
>>>>> But, when is operating with userspace governor?
>>>>
>>>> There are multiple scalable buses inside the SOC, for example there's a
>>>> NIC for display controllers and one for (pci+usb). They can use
>>>> userspace governor for explicit frequency control.
>>>>
>>>>> I think that you better to add the explanation to description
>>>>> for two scenarios how to operate with interconnect provider
>>>>> on either passive governor or userspace governor usage case.
>>>>
>>>> I'll elaborate the example in bindings.
>>>
>>> OK.
>>>
>>>>
>>>>> On 12/13/19 10:51 AM, Chanwoo Choi wrote:
>>>>>> On 12/13/19 10:30 AM, Chanwoo Choi wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 11/15/19 5:09 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-devfreq.c | 150 ++++++++++++++++++++++++++++++++++
>>>>>>>>     3 files changed, 160 insertions(+)
>>>>>>>>     create mode 100644 drivers/devfreq/imx-devfreq.c
>>>>>>>>
>>>>>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>>>>>>>> index 923a6132e741..fef5ce831e90 100644
>>>>>>>> --- a/drivers/devfreq/Kconfig
>>>>>>>> +++ b/drivers/devfreq/Kconfig
>>>>>>>> @@ -98,10 +98,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_DEVFREQ
>>>>>>>> +	tristate "i.MX Generic 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..61d0edee16f7 100644
>>>>>>>> --- a/drivers/devfreq/Makefile
>>>>>>>> +++ b/drivers/devfreq/Makefile
>>>>>>>> @@ -8,10 +8,11 @@ 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_IMX8M_DDRC_DEVFREQ)	+= imx8m-ddrc.o
>>>>>>>> +obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.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
>>>>>>>>     
>>>>>>>>     # DEVFREQ Event Drivers
>>>>>>>> diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c
>>>>>>>> new file mode 100644
>>>>>>>> index 000000000000..620b344e87aa
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/drivers/devfreq/imx-devfreq.c
>>>>>>>> @@ -0,0 +1,150 @@
>>>>>>>> +// 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_devfreq {
>>>>>>>> +	struct devfreq_dev_profile profile;
>>>>>>>> +	struct devfreq *devfreq;
>>>>>>>> +	struct clk *clk;
>>>>>>>> +	struct devfreq_passive_data passive_data;
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +static int imx_devfreq_target(struct device *dev,
>>>>>>>> +			      unsigned long *freq, u32 flags)
>>>>>>>
>>>>>>> Don't use space for the indentation. Please use only tab.
>>>>
>>>> OK
>>
>> The spaces are required in order to align arguments to open paranthesis.
>> Should I drop that?
>>
>> It seems that check_patch.pl and process/coding-style.rst doesn't have a
>> strong opinion on this; my personal preference is for long argument
>> lists to just use double indentation.
> 
> Generally, almost patches use the tab for the indentation.
> I don't use space for the indentation. If use the tab
> for the indentation, it is not harmful for the readability.
> 
> If use the space for the pretty to make the alignment between parameter,
> I think it it not good.

OK, I'll just use two tabs. This also matches my personal preference.

>>>>>>>> +{
>>>>>>>> +	struct imx_devfreq *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_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
>>>>>>>> +{
>>>>>>>> +	struct imx_devfreq *priv = dev_get_drvdata(dev);
>>>>>>>> +
>>>>>>>> +	*freq = clk_get_rate(priv->clk);
>>>>>>>> +
>>>>>>>> +	return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int imx_devfreq_get_dev_status(struct device *dev,
>>>>>>>> +				      struct devfreq_dev_status *stat)
>>>>>>>
>>>>>>> ditto. Please use tab for the indentation.
>>>>>>>
>>>>>>>> +{
>>>>>>>> +	struct imx_devfreq *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_devfreq_exit(struct device *dev)
>>>>>>>> +{
>>>>>>>> +	dev_pm_opp_of_remove_table(dev);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int imx_devfreq_probe(struct platform_device *pdev)
>>>>>>>> +{
>>>>>>>> +	struct device *dev = &pdev->dev;
>>>>>>>> +	struct imx_devfreq *priv;
>>>>>>>
>>>>>>> How about changing the variable name 'priv' to 'imx' or 'imx_data'?
>>>>>>> because it is not easy to catch the role of 'priv' from variable name.
>>>>
>>>> The name "priv" refers to private data of current device: it is short
>>>> and not ambiguous in this context. I don't think that mentioning "imx"
>>>> adds any additional useful information.
>>>>
>>>> It doesn't seem like there's much of a convention for "local variable
>>>> containing private data", for example exynos-bus.c uses "struct
>>>> exynos_bus* bus" internally.
>>>
>>> OK. it is nitpick. Keep your style.
>>>
>>>>
>>>>>>>
>>>>>>>> +	const char *gov = DEVFREQ_GOV_USERSPACE;
>>>>>>>> +	void *govdata = NULL;
>>>>>>>
>>>>>>> How about changing the variable name 'govdata' to 'gov_data'?
>>>>>>> - govdata -> gov_data
>>>>
>>>> OK
>>>>
>>>>>>>> +	int ret;
>>>>>>>> +
>>>>>>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>>>>>> +	if (!priv)
>>>>>>>> +		return -ENOMEM;
>>>>>>>> +
>>>>>>>> +	priv->clk = devm_clk_get(dev, NULL);
>>>>>>>
>>>>>>> nitpick: because the clock-name is not mandatory.
>>>>>>> Don't need to specify the clock name to inform the role of clock
>>>>>>> of other developer/user?
>>>>>>>
>>>>>>> For example, "ddr", "bus" and so on.
>>>>
>>>> I'll call this bus, but I'm not sure it's useful when a single clock is
>>>> involved.
>>>>
>>>>>> And, this driver doesn't include the 'clk_prepare_enable'.
>>>>>> how to enable the clock?
>>>>
>>>> Clocks are either always on or perhaps controlled by some other
>>>> peripheral. This driver only provides scaling.
>>>
>>> It is not proper use-case of clock. If device driver
>>> want to control the clock, it have to be enabled on device driver.
>>
>>> Even it clock is always, the user don't know the state of clock.
>>> Also, user can't know what kind of device driver control the clock.
>>>
>>> It have to be controlled on this device driver
>>> before changing the clock frequency.
>>
>>   From clock framework perspective prepare/enable and rate bits can be
>> controlled separately.
>>
>> Many peripherals are grouped with their own bus (for example a PL301
>> NIC) which is normally off and only gets enabled when explicitly
>> requested by drivers. If this devfreq driver always enabled bus clocks
>> then it would waste power for no reason.
> 
> You can save the power with following sequence.
> You are keeping the following sequence without any problem.
> 	clk_prepare_enable()
> 	clk_set_rate()
> 	clk_disable_unprepare()
> 
> clk API support the reference count for the clock user.
> It doesn't affect the any behavior of other device sharing the bus clock
> and waste any power-consumption because it will always restore
> the reference count after changing the clock rate.

But this doesn't serve any purpose?

In some cases (depending on clock flags like CLK_SET_RATE_GATE or 
CLK_SET_RATE_UNGATE flags) the clk_set_rate function can require than 
clocks are either on or off and otherwise return an error.

For imx bus clocks there is no such requirement.

>> For example a display controller will first enable clocks to allow
>> access to device registers, then configure a resolution and make a
>> bandwith request which gets translated a min_freq request. Then when the
>> display is blanked the entire display bus should be powered off, even if
>> this makes control registers inaccessible.
>>
>> This series only enables scaling for the main NOC which can't be turned
>> off anyway.
>>
>>>>>>>> +	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_devfreq_target;
>>>>>>>> +	priv->profile.get_dev_status = imx_devfreq_get_dev_status;
>>>>>>>> +	priv->profile.exit = imx_devfreq_exit;
>>>>>>>> +	priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
>>>>>>>> +	priv->profile.initial_freq = clk_get_rate(priv->clk);
>>>>>>>> +
>>>>>>>> +	/* Handle passive devfreq parent link */
>>>>>>>> +	priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
>>>>>>>> +	if (!IS_ERR(priv->passive_data.parent)) {
>>>>>>>> +		dev_info(dev, "setup passive link to %s\n",
>>>>>>>> +			 dev_name(priv->passive_data.parent->dev.parent));
>>>>>>>> +		gov = DEVFREQ_GOV_PASSIVE;
>>>>>>>> +		govdata = &priv->passive_data;
>>>>>>>> +	} else if (priv->passive_data.parent != ERR_PTR(-ENODEV)) {
>>>>>>>> +		// -ENODEV means no parent: not an error.
>>>>>>>> +		ret = PTR_ERR(priv->passive_data.parent);
>>>>>>>> +		if (ret != -EPROBE_DEFER)
>>>>>>>> +			dev_warn(dev, "failed to get initialize passive parent: %d\n",
>>>>>>>> +				 ret);
>>>>>>>> +		goto err;
>>>>>>>> +	}
>>>>>>>
>>>>>>> You better to change the exception handling as following: It is more simple.
>>>>>>>
>>>>>>> 	} else if (PTR_ERR(priv->passive_data.parent) == -EPROBE_DEFER)
>>>>>>> 			|| PTR_ERR(priv->passive_data.parent) == -ENODEV) {
>>>>>>> 		goto err;
>>>>>>> 	} else {
>>>>>>> 		ret = PTR_ERR(priv->passive_data.parent);
>>>>>>> 		dev_err(dev, "failed to get initialize passive parent: %d\n", ret);
>>>>>>> 		goto err;
>>>>>>> 	}
>>>>
>>>> But -ENODEV is not an error, it means no passive parent was found.
>>>
>>> OK. just I want to make 'if statement' more simple. This style
>>> is complicated.
>>
>> I can avoid handling EPROBE_DEFER in a nested if.
> 
> Anyway, if you make the exception more simple, I'm ok.
> 
>>
>>>>>>>> +	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
>>>>>>>> +						gov, govdata);
>>>>>>>> +	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_devfreq_of_match[] = {
>>>>>>>> +	{ .compatible = "fsl,imx8m-noc", },
>>>>>>>> +	{ .compatible = "fsl,imx8m-nic", },
>>>>>>>> +	{ /* sentinel */ },
>>>>>>>> +};
>>>>>>>> +MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
>>>>>>>> +
>>>>>>>> +static struct platform_driver imx_devfreq_platdrv = {
>>>>>>>> +	.probe		= imx_devfreq_probe,
>>>>>>>> +	.driver = {
>>>>>>>> +		.name	= "imx-devfreq",
>>>>>>>> +		.of_match_table = of_match_ptr(imx_devfreq_of_match),
>>>>>>>> +	},
>>>>>>>> +};
>>>>>>>> +module_platform_driver(imx_devfreq_platdrv);
>>>>>>>> +
>>>>>>>> +MODULE_DESCRIPTION("Generic i.MX bus frequency driver");
>>>>>>>
>>>>>>> If this driver is for bus frequency, you better to use 'bus' for the clock-name
>>>>>>> for the readability.
>>
>>
>>
>>
> 
>
Chanwoo Choi Dec. 18, 2019, 10:46 a.m. UTC | #16
Hi,

2019년 12월 18일 (수) 오후 7:11, Leonard Crestez <leonard.crestez@nxp.com>님이 작성:
>
> On 18.12.2019 05:08, Chanwoo Choi wrote:
> > On 12/18/19 6:05 AM, Leonard Crestez wrote:
> >> On 17.12.2019 02:35, Chanwoo Choi wrote:
> >>> On 12/16/19 11:57 PM, Leonard Crestez wrote:
> >>>> On 16.12.2019 03:00, Chanwoo Choi wrote:
> >>>>> Hi,
> >>>>>
> >>>>> Also, I think that 'devfreq' word is not proper for device driver name.
> >>>>> imx-bus.c or imx-noc.c or others to inform the role of this driver of developer.
> >>>>
> >>>> I'll rename to "imx-bus". Calling it "imx-noc" is not appropriate
> >>>> because I also want to use it for PL301 NICs.
> >>>
> >>> OK.
> >>>
> >>>>
> >>>>> And, I have a question.
> >>>>> This driver adds the devfreq device with either passive governor
> >>>>> or userspace governor.
> >>>>>
> >>>>> As I understood, the devfreq device with passive governor
> >>>>> will be operated with imx8m-ddrc.c driver.
> >>>>> But, when is operating with userspace governor?
> >>>>
> >>>> There are multiple scalable buses inside the SOC, for example there's a
> >>>> NIC for display controllers and one for (pci+usb). They can use
> >>>> userspace governor for explicit frequency control.
> >>>>
> >>>>> I think that you better to add the explanation to description
> >>>>> for two scenarios how to operate with interconnect provider
> >>>>> on either passive governor or userspace governor usage case.
> >>>>
> >>>> I'll elaborate the example in bindings.
> >>>
> >>> OK.
> >>>
> >>>>
> >>>>> On 12/13/19 10:51 AM, Chanwoo Choi wrote:
> >>>>>> On 12/13/19 10:30 AM, Chanwoo Choi wrote:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> On 11/15/19 5:09 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-devfreq.c | 150 ++++++++++++++++++++++++++++++++++
> >>>>>>>>     3 files changed, 160 insertions(+)
> >>>>>>>>     create mode 100644 drivers/devfreq/imx-devfreq.c
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> >>>>>>>> index 923a6132e741..fef5ce831e90 100644
> >>>>>>>> --- a/drivers/devfreq/Kconfig
> >>>>>>>> +++ b/drivers/devfreq/Kconfig
> >>>>>>>> @@ -98,10 +98,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_DEVFREQ
> >>>>>>>> +      tristate "i.MX Generic 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..61d0edee16f7 100644
> >>>>>>>> --- a/drivers/devfreq/Makefile
> >>>>>>>> +++ b/drivers/devfreq/Makefile
> >>>>>>>> @@ -8,10 +8,11 @@ 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_IMX8M_DDRC_DEVFREQ)       += imx8m-ddrc.o
> >>>>>>>> +obj-$(CONFIG_ARM_IMX_DEVFREQ)         += imx-devfreq.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
> >>>>>>>>
> >>>>>>>>     # DEVFREQ Event Drivers
> >>>>>>>> diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c
> >>>>>>>> new file mode 100644
> >>>>>>>> index 000000000000..620b344e87aa
> >>>>>>>> --- /dev/null
> >>>>>>>> +++ b/drivers/devfreq/imx-devfreq.c
> >>>>>>>> @@ -0,0 +1,150 @@
> >>>>>>>> +// 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_devfreq {
> >>>>>>>> +      struct devfreq_dev_profile profile;
> >>>>>>>> +      struct devfreq *devfreq;
> >>>>>>>> +      struct clk *clk;
> >>>>>>>> +      struct devfreq_passive_data passive_data;
> >>>>>>>> +};
> >>>>>>>> +
> >>>>>>>> +static int imx_devfreq_target(struct device *dev,
> >>>>>>>> +                            unsigned long *freq, u32 flags)
> >>>>>>>
> >>>>>>> Don't use space for the indentation. Please use only tab.
> >>>>
> >>>> OK
> >>
> >> The spaces are required in order to align arguments to open paranthesis.
> >> Should I drop that?
> >>
> >> It seems that check_patch.pl and process/coding-style.rst doesn't have a
> >> strong opinion on this; my personal preference is for long argument
> >> lists to just use double indentation.
> >
> > Generally, almost patches use the tab for the indentation.
> > I don't use space for the indentation. If use the tab
> > for the indentation, it is not harmful for the readability.
> >
> > If use the space for the pretty to make the alignment between parameter,
> > I think it it not good.
>
> OK, I'll just use two tabs. This also matches my personal preference.
>
> >>>>>>>> +{
> >>>>>>>> +      struct imx_devfreq *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_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
> >>>>>>>> +{
> >>>>>>>> +      struct imx_devfreq *priv = dev_get_drvdata(dev);
> >>>>>>>> +
> >>>>>>>> +      *freq = clk_get_rate(priv->clk);
> >>>>>>>> +
> >>>>>>>> +      return 0;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +static int imx_devfreq_get_dev_status(struct device *dev,
> >>>>>>>> +                                    struct devfreq_dev_status *stat)
> >>>>>>>
> >>>>>>> ditto. Please use tab for the indentation.
> >>>>>>>
> >>>>>>>> +{
> >>>>>>>> +      struct imx_devfreq *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_devfreq_exit(struct device *dev)
> >>>>>>>> +{
> >>>>>>>> +      dev_pm_opp_of_remove_table(dev);
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +static int imx_devfreq_probe(struct platform_device *pdev)
> >>>>>>>> +{
> >>>>>>>> +      struct device *dev = &pdev->dev;
> >>>>>>>> +      struct imx_devfreq *priv;
> >>>>>>>
> >>>>>>> How about changing the variable name 'priv' to 'imx' or 'imx_data'?
> >>>>>>> because it is not easy to catch the role of 'priv' from variable name.
> >>>>
> >>>> The name "priv" refers to private data of current device: it is short
> >>>> and not ambiguous in this context. I don't think that mentioning "imx"
> >>>> adds any additional useful information.
> >>>>
> >>>> It doesn't seem like there's much of a convention for "local variable
> >>>> containing private data", for example exynos-bus.c uses "struct
> >>>> exynos_bus* bus" internally.
> >>>
> >>> OK. it is nitpick. Keep your style.
> >>>
> >>>>
> >>>>>>>
> >>>>>>>> +      const char *gov = DEVFREQ_GOV_USERSPACE;
> >>>>>>>> +      void *govdata = NULL;
> >>>>>>>
> >>>>>>> How about changing the variable name 'govdata' to 'gov_data'?
> >>>>>>> - govdata -> gov_data
> >>>>
> >>>> OK
> >>>>
> >>>>>>>> +      int ret;
> >>>>>>>> +
> >>>>>>>> +      priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >>>>>>>> +      if (!priv)
> >>>>>>>> +              return -ENOMEM;
> >>>>>>>> +
> >>>>>>>> +      priv->clk = devm_clk_get(dev, NULL);
> >>>>>>>
> >>>>>>> nitpick: because the clock-name is not mandatory.
> >>>>>>> Don't need to specify the clock name to inform the role of clock
> >>>>>>> of other developer/user?
> >>>>>>>
> >>>>>>> For example, "ddr", "bus" and so on.
> >>>>
> >>>> I'll call this bus, but I'm not sure it's useful when a single clock is
> >>>> involved.
> >>>>
> >>>>>> And, this driver doesn't include the 'clk_prepare_enable'.
> >>>>>> how to enable the clock?
> >>>>
> >>>> Clocks are either always on or perhaps controlled by some other
> >>>> peripheral. This driver only provides scaling.
> >>>
> >>> It is not proper use-case of clock. If device driver
> >>> want to control the clock, it have to be enabled on device driver.
> >>
> >>> Even it clock is always, the user don't know the state of clock.
> >>> Also, user can't know what kind of device driver control the clock.
> >>>
> >>> It have to be controlled on this device driver
> >>> before changing the clock frequency.
> >>
> >>   From clock framework perspective prepare/enable and rate bits can be
> >> controlled separately.
> >>
> >> Many peripherals are grouped with their own bus (for example a PL301
> >> NIC) which is normally off and only gets enabled when explicitly
> >> requested by drivers. If this devfreq driver always enabled bus clocks
> >> then it would waste power for no reason.
> >
> > You can save the power with following sequence.
> > You are keeping the following sequence without any problem.
> >       clk_prepare_enable()
> >       clk_set_rate()
> >       clk_disable_unprepare()
> >
> > clk API support the reference count for the clock user.
> > It doesn't affect the any behavior of other device sharing the bus clock
> > and waste any power-consumption because it will always restore
> > the reference count after changing the clock rate.
>
> But this doesn't serve any purpose?
>
> In some cases (depending on clock flags like CLK_SET_RATE_GATE or
> CLK_SET_RATE_UNGATE flags) the clk_set_rate function can require than
> clocks are either on or off and otherwise return an error.
>
> For imx bus clocks there is no such requirement.

If you guarantee that it is not required on all board related to imx,
please add this comment when calling clk_set_rate().
Because the user who don't know the history and character,
don't understand why don't enable or disable for clock.

I'll agree if you add the comment why don't need to
enable/disable control on this driver.


>
> >> For example a display controller will first enable clocks to allow
> >> access to device registers, then configure a resolution and make a
> >> bandwith request which gets translated a min_freq request. Then when the
> >> display is blanked the entire display bus should be powered off, even if
> >> this makes control registers inaccessible.
> >>
> >> This series only enables scaling for the main NOC which can't be turned
> >> off anyway.
> >>
> >>>>>>>> +      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_devfreq_target;
> >>>>>>>> +      priv->profile.get_dev_status = imx_devfreq_get_dev_status;
> >>>>>>>> +      priv->profile.exit = imx_devfreq_exit;
> >>>>>>>> +      priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
> >>>>>>>> +      priv->profile.initial_freq = clk_get_rate(priv->clk);
> >>>>>>>> +
> >>>>>>>> +      /* Handle passive devfreq parent link */
> >>>>>>>> +      priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
> >>>>>>>> +      if (!IS_ERR(priv->passive_data.parent)) {
> >>>>>>>> +              dev_info(dev, "setup passive link to %s\n",
> >>>>>>>> +                       dev_name(priv->passive_data.parent->dev.parent));
> >>>>>>>> +              gov = DEVFREQ_GOV_PASSIVE;
> >>>>>>>> +              govdata = &priv->passive_data;
> >>>>>>>> +      } else if (priv->passive_data.parent != ERR_PTR(-ENODEV)) {
> >>>>>>>> +              // -ENODEV means no parent: not an error.
> >>>>>>>> +              ret = PTR_ERR(priv->passive_data.parent);
> >>>>>>>> +              if (ret != -EPROBE_DEFER)
> >>>>>>>> +                      dev_warn(dev, "failed to get initialize passive parent: %d\n",
> >>>>>>>> +                               ret);
> >>>>>>>> +              goto err;
> >>>>>>>> +      }
> >>>>>>>
> >>>>>>> You better to change the exception handling as following: It is more simple.
> >>>>>>>
> >>>>>>>         } else if (PTR_ERR(priv->passive_data.parent) == -EPROBE_DEFER)
> >>>>>>>                         || PTR_ERR(priv->passive_data.parent) == -ENODEV) {
> >>>>>>>                 goto err;
> >>>>>>>         } else {
> >>>>>>>                 ret = PTR_ERR(priv->passive_data.parent);
> >>>>>>>                 dev_err(dev, "failed to get initialize passive parent: %d\n", ret);
> >>>>>>>                 goto err;
> >>>>>>>         }
> >>>>
> >>>> But -ENODEV is not an error, it means no passive parent was found.
> >>>
> >>> OK. just I want to make 'if statement' more simple. This style
> >>> is complicated.
> >>
> >> I can avoid handling EPROBE_DEFER in a nested if.
> >
> > Anyway, if you make the exception more simple, I'm ok.
> >
> >>
> >>>>>>>> +      priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
> >>>>>>>> +                                              gov, govdata);
> >>>>>>>> +      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_devfreq_of_match[] = {
> >>>>>>>> +      { .compatible = "fsl,imx8m-noc", },
> >>>>>>>> +      { .compatible = "fsl,imx8m-nic", },
> >>>>>>>> +      { /* sentinel */ },
> >>>>>>>> +};
> >>>>>>>> +MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
> >>>>>>>> +
> >>>>>>>> +static struct platform_driver imx_devfreq_platdrv = {
> >>>>>>>> +      .probe          = imx_devfreq_probe,
> >>>>>>>> +      .driver = {
> >>>>>>>> +              .name   = "imx-devfreq",
> >>>>>>>> +              .of_match_table = of_match_ptr(imx_devfreq_of_match),
> >>>>>>>> +      },
> >>>>>>>> +};
> >>>>>>>> +module_platform_driver(imx_devfreq_platdrv);
> >>>>>>>> +
> >>>>>>>> +MODULE_DESCRIPTION("Generic i.MX bus frequency driver");
> >>>>>>>
> >>>>>>> If this driver is for bus frequency, you better to use 'bus' for the clock-name
> >>>>>>> for the readability.
> >>
> >>
> >>
> >>
> >
> >
>
Chanwoo Choi Dec. 18, 2019, 5:06 p.m. UTC | #17
2019년 12월 18일 (수) 오후 7:46, Chanwoo Choi <chanwoo@kernel.org>님이 작성:
>
> Hi,
>
> 2019년 12월 18일 (수) 오후 7:11, Leonard Crestez <leonard.crestez@nxp.com>님이 작성:
> >
> > On 18.12.2019 05:08, Chanwoo Choi wrote:
> > > On 12/18/19 6:05 AM, Leonard Crestez wrote:
> > >> On 17.12.2019 02:35, Chanwoo Choi wrote:
> > >>> On 12/16/19 11:57 PM, Leonard Crestez wrote:
> > >>>> On 16.12.2019 03:00, Chanwoo Choi wrote:
> > >>>>> Hi,
> > >>>>>
> > >>>>> Also, I think that 'devfreq' word is not proper for device driver name.
> > >>>>> imx-bus.c or imx-noc.c or others to inform the role of this driver of developer.
> > >>>>
> > >>>> I'll rename to "imx-bus". Calling it "imx-noc" is not appropriate
> > >>>> because I also want to use it for PL301 NICs.
> > >>>
> > >>> OK.
> > >>>
> > >>>>
> > >>>>> And, I have a question.
> > >>>>> This driver adds the devfreq device with either passive governor
> > >>>>> or userspace governor.
> > >>>>>
> > >>>>> As I understood, the devfreq device with passive governor
> > >>>>> will be operated with imx8m-ddrc.c driver.
> > >>>>> But, when is operating with userspace governor?
> > >>>>
> > >>>> There are multiple scalable buses inside the SOC, for example there's a
> > >>>> NIC for display controllers and one for (pci+usb). They can use
> > >>>> userspace governor for explicit frequency control.
> > >>>>
> > >>>>> I think that you better to add the explanation to description
> > >>>>> for two scenarios how to operate with interconnect provider
> > >>>>> on either passive governor or userspace governor usage case.
> > >>>>
> > >>>> I'll elaborate the example in bindings.
> > >>>
> > >>> OK.
> > >>>
> > >>>>
> > >>>>> On 12/13/19 10:51 AM, Chanwoo Choi wrote:
> > >>>>>> On 12/13/19 10:30 AM, Chanwoo Choi wrote:
> > >>>>>>> Hi,
> > >>>>>>>
> > >>>>>>> On 11/15/19 5:09 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-devfreq.c | 150 ++++++++++++++++++++++++++++++++++
> > >>>>>>>>     3 files changed, 160 insertions(+)
> > >>>>>>>>     create mode 100644 drivers/devfreq/imx-devfreq.c
> > >>>>>>>>
> > >>>>>>>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> > >>>>>>>> index 923a6132e741..fef5ce831e90 100644
> > >>>>>>>> --- a/drivers/devfreq/Kconfig
> > >>>>>>>> +++ b/drivers/devfreq/Kconfig
> > >>>>>>>> @@ -98,10 +98,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_DEVFREQ
> > >>>>>>>> +      tristate "i.MX Generic 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..61d0edee16f7 100644
> > >>>>>>>> --- a/drivers/devfreq/Makefile
> > >>>>>>>> +++ b/drivers/devfreq/Makefile
> > >>>>>>>> @@ -8,10 +8,11 @@ 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_IMX8M_DDRC_DEVFREQ)       += imx8m-ddrc.o
> > >>>>>>>> +obj-$(CONFIG_ARM_IMX_DEVFREQ)         += imx-devfreq.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
> > >>>>>>>>
> > >>>>>>>>     # DEVFREQ Event Drivers
> > >>>>>>>> diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c
> > >>>>>>>> new file mode 100644
> > >>>>>>>> index 000000000000..620b344e87aa
> > >>>>>>>> --- /dev/null
> > >>>>>>>> +++ b/drivers/devfreq/imx-devfreq.c
> > >>>>>>>> @@ -0,0 +1,150 @@
> > >>>>>>>> +// 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_devfreq {
> > >>>>>>>> +      struct devfreq_dev_profile profile;
> > >>>>>>>> +      struct devfreq *devfreq;
> > >>>>>>>> +      struct clk *clk;
> > >>>>>>>> +      struct devfreq_passive_data passive_data;
> > >>>>>>>> +};
> > >>>>>>>> +
> > >>>>>>>> +static int imx_devfreq_target(struct device *dev,
> > >>>>>>>> +                            unsigned long *freq, u32 flags)
> > >>>>>>>
> > >>>>>>> Don't use space for the indentation. Please use only tab.
> > >>>>
> > >>>> OK
> > >>
> > >> The spaces are required in order to align arguments to open paranthesis.
> > >> Should I drop that?
> > >>
> > >> It seems that check_patch.pl and process/coding-style.rst doesn't have a
> > >> strong opinion on this; my personal preference is for long argument
> > >> lists to just use double indentation.
> > >
> > > Generally, almost patches use the tab for the indentation.
> > > I don't use space for the indentation. If use the tab
> > > for the indentation, it is not harmful for the readability.
> > >
> > > If use the space for the pretty to make the alignment between parameter,
> > > I think it it not good.
> >
> > OK, I'll just use two tabs. This also matches my personal preference.
> >
> > >>>>>>>> +{
> > >>>>>>>> +      struct imx_devfreq *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_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
> > >>>>>>>> +{
> > >>>>>>>> +      struct imx_devfreq *priv = dev_get_drvdata(dev);
> > >>>>>>>> +
> > >>>>>>>> +      *freq = clk_get_rate(priv->clk);
> > >>>>>>>> +
> > >>>>>>>> +      return 0;
> > >>>>>>>> +}
> > >>>>>>>> +
> > >>>>>>>> +static int imx_devfreq_get_dev_status(struct device *dev,
> > >>>>>>>> +                                    struct devfreq_dev_status *stat)
> > >>>>>>>
> > >>>>>>> ditto. Please use tab for the indentation.
> > >>>>>>>
> > >>>>>>>> +{
> > >>>>>>>> +      struct imx_devfreq *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_devfreq_exit(struct device *dev)
> > >>>>>>>> +{
> > >>>>>>>> +      dev_pm_opp_of_remove_table(dev);
> > >>>>>>>> +}
> > >>>>>>>> +
> > >>>>>>>> +static int imx_devfreq_probe(struct platform_device *pdev)
> > >>>>>>>> +{
> > >>>>>>>> +      struct device *dev = &pdev->dev;
> > >>>>>>>> +      struct imx_devfreq *priv;
> > >>>>>>>
> > >>>>>>> How about changing the variable name 'priv' to 'imx' or 'imx_data'?
> > >>>>>>> because it is not easy to catch the role of 'priv' from variable name.
> > >>>>
> > >>>> The name "priv" refers to private data of current device: it is short
> > >>>> and not ambiguous in this context. I don't think that mentioning "imx"
> > >>>> adds any additional useful information.
> > >>>>
> > >>>> It doesn't seem like there's much of a convention for "local variable
> > >>>> containing private data", for example exynos-bus.c uses "struct
> > >>>> exynos_bus* bus" internally.
> > >>>
> > >>> OK. it is nitpick. Keep your style.
> > >>>
> > >>>>
> > >>>>>>>
> > >>>>>>>> +      const char *gov = DEVFREQ_GOV_USERSPACE;
> > >>>>>>>> +      void *govdata = NULL;
> > >>>>>>>
> > >>>>>>> How about changing the variable name 'govdata' to 'gov_data'?
> > >>>>>>> - govdata -> gov_data
> > >>>>
> > >>>> OK
> > >>>>
> > >>>>>>>> +      int ret;
> > >>>>>>>> +
> > >>>>>>>> +      priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > >>>>>>>> +      if (!priv)
> > >>>>>>>> +              return -ENOMEM;
> > >>>>>>>> +
> > >>>>>>>> +      priv->clk = devm_clk_get(dev, NULL);
> > >>>>>>>
> > >>>>>>> nitpick: because the clock-name is not mandatory.
> > >>>>>>> Don't need to specify the clock name to inform the role of clock
> > >>>>>>> of other developer/user?
> > >>>>>>>
> > >>>>>>> For example, "ddr", "bus" and so on.
> > >>>>
> > >>>> I'll call this bus, but I'm not sure it's useful when a single clock is
> > >>>> involved.
> > >>>>
> > >>>>>> And, this driver doesn't include the 'clk_prepare_enable'.
> > >>>>>> how to enable the clock?
> > >>>>
> > >>>> Clocks are either always on or perhaps controlled by some other
> > >>>> peripheral. This driver only provides scaling.
> > >>>
> > >>> It is not proper use-case of clock. If device driver
> > >>> want to control the clock, it have to be enabled on device driver.
> > >>
> > >>> Even it clock is always, the user don't know the state of clock.
> > >>> Also, user can't know what kind of device driver control the clock.
> > >>>
> > >>> It have to be controlled on this device driver
> > >>> before changing the clock frequency.
> > >>
> > >>   From clock framework perspective prepare/enable and rate bits can be
> > >> controlled separately.
> > >>
> > >> Many peripherals are grouped with their own bus (for example a PL301
> > >> NIC) which is normally off and only gets enabled when explicitly
> > >> requested by drivers. If this devfreq driver always enabled bus clocks
> > >> then it would waste power for no reason.
> > >
> > > You can save the power with following sequence.
> > > You are keeping the following sequence without any problem.
> > >       clk_prepare_enable()
> > >       clk_set_rate()
> > >       clk_disable_unprepare()
> > >
> > > clk API support the reference count for the clock user.
> > > It doesn't affect the any behavior of other device sharing the bus clock
> > > and waste any power-consumption because it will always restore
> > > the reference count after changing the clock rate.
> >
> > But this doesn't serve any purpose?

Again thinking, it show the at least sequence to control the clock.
To do this sequence, it doesn't affect the behavior of any device driver
which share the clock. And it show the clock is well-controlled by
this device driver.

> >
> > In some cases (depending on clock flags like CLK_SET_RATE_GATE or
> > CLK_SET_RATE_UNGATE flags) the clk_set_rate function can require than
> > clocks are either on or off and otherwise return an error.
> >
> > For imx bus clocks there is no such requirement.

Usually, the clock is enabled before using it. It doesn't depend on
the any arch or SoC.
CCF define the this rule for the clock control.

>
> If you guarantee that it is not required on all board related to imx,
> please add this comment when calling clk_set_rate().
> Because the user who don't know the history and character,
> don't understand why don't enable or disable for clock.
>
> I'll agree if you add the comment why don't need to
> enable/disable control on this driver.
>
>
> >
> > >> For example a display controller will first enable clocks to allow
> > >> access to device registers, then configure a resolution and make a
> > >> bandwith request which gets translated a min_freq request. Then when the
> > >> display is blanked the entire display bus should be powered off, even if
> > >> this makes control registers inaccessible.
> > >>
> > >> This series only enables scaling for the main NOC which can't be turned
> > >> off anyway.
> > >>
> > >>>>>>>> +      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_devfreq_target;
> > >>>>>>>> +      priv->profile.get_dev_status = imx_devfreq_get_dev_status;
> > >>>>>>>> +      priv->profile.exit = imx_devfreq_exit;
> > >>>>>>>> +      priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
> > >>>>>>>> +      priv->profile.initial_freq = clk_get_rate(priv->clk);
> > >>>>>>>> +
> > >>>>>>>> +      /* Handle passive devfreq parent link */
> > >>>>>>>> +      priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
> > >>>>>>>> +      if (!IS_ERR(priv->passive_data.parent)) {
> > >>>>>>>> +              dev_info(dev, "setup passive link to %s\n",
> > >>>>>>>> +                       dev_name(priv->passive_data.parent->dev.parent));
> > >>>>>>>> +              gov = DEVFREQ_GOV_PASSIVE;
> > >>>>>>>> +              govdata = &priv->passive_data;
> > >>>>>>>> +      } else if (priv->passive_data.parent != ERR_PTR(-ENODEV)) {
> > >>>>>>>> +              // -ENODEV means no parent: not an error.
> > >>>>>>>> +              ret = PTR_ERR(priv->passive_data.parent);
> > >>>>>>>> +              if (ret != -EPROBE_DEFER)
> > >>>>>>>> +                      dev_warn(dev, "failed to get initialize passive parent: %d\n",
> > >>>>>>>> +                               ret);
> > >>>>>>>> +              goto err;
> > >>>>>>>> +      }
> > >>>>>>>
> > >>>>>>> You better to change the exception handling as following: It is more simple.
> > >>>>>>>
> > >>>>>>>         } else if (PTR_ERR(priv->passive_data.parent) == -EPROBE_DEFER)
> > >>>>>>>                         || PTR_ERR(priv->passive_data.parent) == -ENODEV) {
> > >>>>>>>                 goto err;
> > >>>>>>>         } else {
> > >>>>>>>                 ret = PTR_ERR(priv->passive_data.parent);
> > >>>>>>>                 dev_err(dev, "failed to get initialize passive parent: %d\n", ret);
> > >>>>>>>                 goto err;
> > >>>>>>>         }
> > >>>>
> > >>>> But -ENODEV is not an error, it means no passive parent was found.
> > >>>
> > >>> OK. just I want to make 'if statement' more simple. This style
> > >>> is complicated.
> > >>
> > >> I can avoid handling EPROBE_DEFER in a nested if.
> > >
> > > Anyway, if you make the exception more simple, I'm ok.
> > >
> > >>
> > >>>>>>>> +      priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
> > >>>>>>>> +                                              gov, govdata);
> > >>>>>>>> +      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_devfreq_of_match[] = {
> > >>>>>>>> +      { .compatible = "fsl,imx8m-noc", },
> > >>>>>>>> +      { .compatible = "fsl,imx8m-nic", },
> > >>>>>>>> +      { /* sentinel */ },
> > >>>>>>>> +};
> > >>>>>>>> +MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
> > >>>>>>>> +
> > >>>>>>>> +static struct platform_driver imx_devfreq_platdrv = {
> > >>>>>>>> +      .probe          = imx_devfreq_probe,
> > >>>>>>>> +      .driver = {
> > >>>>>>>> +              .name   = "imx-devfreq",
> > >>>>>>>> +              .of_match_table = of_match_ptr(imx_devfreq_of_match),
> > >>>>>>>> +      },
> > >>>>>>>> +};
> > >>>>>>>> +module_platform_driver(imx_devfreq_platdrv);
> > >>>>>>>> +
> > >>>>>>>> +MODULE_DESCRIPTION("Generic i.MX bus frequency driver");
> > >>>>>>>
> > >>>>>>> If this driver is for bus frequency, you better to use 'bus' for the clock-name
> > >>>>>>> for the readability.
> > >>
> > >>
> > >>
> > >>
> > >
> > >
> >
>
>
> --
> Best Regards,
> Chanwoo Choi
Martin Kepplinger Feb. 4, 2020, 9:45 a.m. UTC | #18
On 20.11.19 19:02, Leonard Crestez wrote:
> On 20.11.2019 18:38, Angus Ainslie wrote:
>> On 2019-11-20 08:30, Leonard Crestez wrote:
>>> On 20.11.2019 17:41, Angus Ainslie wrote:
>>>> Hi Leonard,
>>>>
>>>> On 2019-11-20 07:04, Leonard Crestez wrote:
>>>>> On 20.11.2019 16:08, Angus Ainslie wrote:
>>>>> Is "mainline ATF" an important criteria for Purism?
>>>>
>>>> Yes we intend to bring all of our patches to mainline and were hoping
>>>> that NXP would be doing the same. Shouldn't a mainline kernel run on a
>>>> mainline ATF ?
>>>
>>> You can still use mainline ATF (tested right now) but the imx8m-ddrc
>>> driver won't probe.
>>
>> Sorry I was talking about the DDR frequency scaling specifically. >
>>> The ability to mix and match different branches of firmware and kernel
>>> is very useful for testing. There might be slight incompatibilities but
>>> in theory if a feature depends on both firmware and kernel support then
>>> it should gracefully degrade rather than crash or hang.
>>
>> I saw the check you put in for the correct ATF version and that's very
>> helpful thanks.
>>
>>> ATF support for this feature will be mainlined eventually, I picked the
>>> linux side first because review is more challenging and changes are
>>> much larger relative to what we have in our internal tree.
>>
>> Do you have a patch against mainline ATF that we can test this feature
>> with ?
> 
> Not right now, and imx atf is based on a slightly older version so some 
> porting effort might be required.
> 

Hi Leonard,

Have you, by chance, looked at the mainline ATF side for your devfreq
work in the meantime? I'd happily test. (btw. the linux driver fails
"nicely" there.)

thanks,

                                  martin
Martin Kepplinger Feb. 13, 2020, 10:53 a.m. UTC | #19
On 04.02.20 10:45, Martin Kepplinger wrote:
> On 20.11.19 19:02, Leonard Crestez wrote:
>> On 20.11.2019 18:38, Angus Ainslie wrote:
>>> On 2019-11-20 08:30, Leonard Crestez wrote:
>>>> On 20.11.2019 17:41, Angus Ainslie wrote:
>>>>> Hi Leonard,
>>>>>
>>>>> On 2019-11-20 07:04, Leonard Crestez wrote:
>>>>>> On 20.11.2019 16:08, Angus Ainslie wrote:
>>>>>> Is "mainline ATF" an important criteria for Purism?
>>>>>
>>>>> Yes we intend to bring all of our patches to mainline and were hoping
>>>>> that NXP would be doing the same. Shouldn't a mainline kernel run on a
>>>>> mainline ATF ?
>>>>
>>>> You can still use mainline ATF (tested right now) but the imx8m-ddrc
>>>> driver won't probe.
>>>
>>> Sorry I was talking about the DDR frequency scaling specifically. >
>>>> The ability to mix and match different branches of firmware and kernel
>>>> is very useful for testing. There might be slight incompatibilities but
>>>> in theory if a feature depends on both firmware and kernel support then
>>>> it should gracefully degrade rather than crash or hang.
>>>
>>> I saw the check you put in for the correct ATF version and that's very
>>> helpful thanks.
>>>
>>>> ATF support for this feature will be mainlined eventually, I picked the
>>>> linux side first because review is more challenging and changes are
>>>> much larger relative to what we have in our internal tree.
>>>
>>> Do you have a patch against mainline ATF that we can test this feature
>>> with ?
>>
>> Not right now, and imx atf is based on a slightly older version so some 
>> porting effort might be required.
>>
> 
> Hi Leonard,
> 
> Have you, by chance, looked at the mainline ATF side for your devfreq
> work in the meantime? I'd happily test. (btw. the linux driver fails
> "nicely" there.)
> 

hi again. Do you have any plans regarding the ATF implementation of
IMX_SIP_DDR_DVFS in mainline yet? The kernel is only one half of the
devfreq solution after all. I'm asking just to schedule my work a bit :)

thanks,
                            martin

Patch
diff mbox series

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 923a6132e741..fef5ce831e90 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -98,10 +98,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_DEVFREQ
+	tristate "i.MX Generic 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..61d0edee16f7 100644
--- a/drivers/devfreq/Makefile
+++ b/drivers/devfreq/Makefile
@@ -8,10 +8,11 @@  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_IMX8M_DDRC_DEVFREQ)	+= imx8m-ddrc.o
+obj-$(CONFIG_ARM_IMX_DEVFREQ)		+= imx-devfreq.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
 
 # DEVFREQ Event Drivers
diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c
new file mode 100644
index 000000000000..620b344e87aa
--- /dev/null
+++ b/drivers/devfreq/imx-devfreq.c
@@ -0,0 +1,150 @@ 
+// 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_devfreq {
+	struct devfreq_dev_profile profile;
+	struct devfreq *devfreq;
+	struct clk *clk;
+	struct devfreq_passive_data passive_data;
+};
+
+static int imx_devfreq_target(struct device *dev,
+			      unsigned long *freq, u32 flags)
+{
+	struct imx_devfreq *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_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
+{
+	struct imx_devfreq *priv = dev_get_drvdata(dev);
+
+	*freq = clk_get_rate(priv->clk);
+
+	return 0;
+}
+
+static int imx_devfreq_get_dev_status(struct device *dev,
+				      struct devfreq_dev_status *stat)
+{
+	struct imx_devfreq *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_devfreq_exit(struct device *dev)
+{
+	dev_pm_opp_of_remove_table(dev);
+}
+
+static int imx_devfreq_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct imx_devfreq *priv;
+	const char *gov = DEVFREQ_GOV_USERSPACE;
+	void *govdata = NULL;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	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_devfreq_target;
+	priv->profile.get_dev_status = imx_devfreq_get_dev_status;
+	priv->profile.exit = imx_devfreq_exit;
+	priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
+	priv->profile.initial_freq = clk_get_rate(priv->clk);
+
+	/* Handle passive devfreq parent link */
+	priv->passive_data.parent = devfreq_get_devfreq_by_phandle(dev, 0);
+	if (!IS_ERR(priv->passive_data.parent)) {
+		dev_info(dev, "setup passive link to %s\n",
+			 dev_name(priv->passive_data.parent->dev.parent));
+		gov = DEVFREQ_GOV_PASSIVE;
+		govdata = &priv->passive_data;
+	} else if (priv->passive_data.parent != ERR_PTR(-ENODEV)) {
+		// -ENODEV means no parent: not an error.
+		ret = PTR_ERR(priv->passive_data.parent);
+		if (ret != -EPROBE_DEFER)
+			dev_warn(dev, "failed to get initialize passive parent: %d\n",
+				 ret);
+		goto err;
+	}
+
+	priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
+						gov, govdata);
+	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_devfreq_of_match[] = {
+	{ .compatible = "fsl,imx8m-noc", },
+	{ .compatible = "fsl,imx8m-nic", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
+
+static struct platform_driver imx_devfreq_platdrv = {
+	.probe		= imx_devfreq_probe,
+	.driver = {
+		.name	= "imx-devfreq",
+		.of_match_table = of_match_ptr(imx_devfreq_of_match),
+	},
+};
+module_platform_driver(imx_devfreq_platdrv);
+
+MODULE_DESCRIPTION("Generic i.MX bus frequency driver");
+MODULE_AUTHOR("Leonard Crestez <leonard.crestez@nxp.com>");
+MODULE_LICENSE("GPL v2");