Re: [PATCH RFC v6 2/9] PM / devfreq: Add generic imx bus scaling driver

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

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel