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