Re: [PATCH 09/12] devfreq: add mediatek cci devfreq

From: Chanwoo Choi
Date: Thu May 28 2020 - 03:25:29 EST

Next message: kbuild test robot: "arch/x86/ia32/ia32_signal.c:350:9: sparse: sparse: incorrect type in argument 1 (different address spaces)"
Previous message: Vishal Sagar: "RE: [PATCH v14 1/2] media: dt-bindings: media: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem"
Next in thread: Chanwoo Choi: "Re: [PATCH 09/12] devfreq: add mediatek cci devfreq"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Andrew-sh.Cheng,

On 5/20/20 12:43 PM, Andrew-sh.Cheng wrote:
> This adds a devfreq driver for the Cache Coherent Interconnect (CCI)
> of the Mediatek MT8183.
>
> On the MT8183 the CCI is supplied by the same regulator as the LITTLE
> cores. The driver is notified when the regulator voltage changes
> (driven by cpufreq) and adjusts the CCI frequency to the maximum
> possible value.
>
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@xxxxxxxxxxxx>
> ---
> drivers/devfreq/Kconfig | 10 ++
> drivers/devfreq/Makefile | 1 +
> drivers/devfreq/mt8183-cci-devfreq.c | 206 +++++++++++++++++++++++++++++++++++

The mt8183-cci.c is enough for driver name.

> 3 files changed, 217 insertions(+)
> create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
>
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index d9067950af6a..4ed7116271ee 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -103,6 +103,16 @@ config ARM_IMX8M_DDRC_DEVFREQ
> This adds the DEVFREQ driver for the i.MX8M DDR Controller. It allows
> adjusting DRAM frequency.
>
> +config ARM_MT8183_CCI_DEVFREQ
> + tristate "MT8183 CCI DEVFREQ Driver"
> + depends on ARM_MEDIATEK_CPUFREQ
> + help
> + This adds a devfreq driver for Cache Coherent Interconnect
> + of Mediatek MT8183, which is shared the same regulator
> + with cpu cluster.
> + It can track buck voltage and update a proper cci frequency.

s/cci/CCI

> + Use notification to get regulator status.
> +
> config ARM_TEGRA_DEVFREQ
> tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
> depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || \
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 3eb4d5e6635c..5b1b670c954d 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -10,6 +10,7 @@ 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_MT8183_CCI_DEVFREQ) += mt8183-cci-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
> diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c
> new file mode 100644
> index 000000000000..cd7929a83bf8
> --- /dev/null
> +++ b/drivers/devfreq/mt8183-cci-devfreq.c
> @@ -0,0 +1,206 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 MediaTek Inc.

s/2019/2020

> +
> + * Author: Andrew-sh.Cheng <andrew-sh.cheng@xxxxxxxxxxxx>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/devfreq.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/time.h>
> +
> +#include "governor.h"

It is not needed. Please remove it.

> +
> +#define MAX_VOLT_LIMIT (1150000)
> +
> +struct cci_devfreq {
> + struct devfreq *devfreq;
> + struct regulator *proc_reg;

'proc' means the 'processor'?
Instead of 'proc_reg', you better to use 'cpu_reg'.

> + struct clk *cci_clk;
> + int old_vproc;
> + unsigned long old_freq;
> +};
> +
> +static int mtk_cci_set_voltage(struct cci_devfreq *cci_df, int vproc)
> +{
> + int ret;
> +
> + ret = regulator_set_voltage(cci_df->proc_reg, vproc,
> + MAX_VOLT_LIMIT);
> + if (!ret)
> + cci_df->old_vproc = vproc;
> + return ret;
> +}
> +
> +static int mtk_cci_devfreq_target(struct device *dev, unsigned long *freq,
> + u32 flags)
> +{
> + int ret;
> + struct cci_devfreq *cci_df = dev_get_drvdata(dev);
> + struct dev_pm_opp *opp;
> + unsigned long opp_rate, opp_voltage, old_voltage;
> +
> + if (!cci_df)
> + return -EINVAL;
> +
> + if (cci_df->old_freq == *freq)
> + return 0;
> +
> + opp_rate = *freq;
> + opp = dev_pm_opp_find_freq_floor(dev, &opp_rate);
> + opp_voltage = dev_pm_opp_get_voltage(opp);
> + dev_pm_opp_put(opp);

You can use the helper function for getting the rate
with devfreq_recommended_opp(). You can refer the following code
in drivers/devfreq/exynos-bus.c

opp = devfreq_recommended_opp(dev, freq, flags);
if (IS_ERR(opp)) {
dev_err(dev, "failed to get recommended opp instance\n");
return PTR_ERR(opp);
}
dev_pm_opp_put(opp);

> +
> + old_voltage = cci_df->old_vproc;
> + if (old_voltage == 0)
> + old_voltage = regulator_get_voltage(cci_df->proc_reg);
> +
> + // scale up: set voltage first then freq
> + if (opp_voltage > old_voltage) {
> + ret = mtk_cci_set_voltage(cci_df, opp_voltage);
> + if (ret) {
> + pr_err("cci: failed to scale up voltage\n");
> + return ret;
> + }
> + }
> +
> + ret = clk_set_rate(cci_df->cci_clk, *freq);
> + if (ret) {
> + pr_err("%s: failed cci to set rate: %d\n", __func__,
> + ret);
> + mtk_cci_set_voltage(cci_df, old_voltage);
> + return ret;
> + }
> +
> + // scale down: set freq first then voltage
> + if (opp_voltage < old_voltage) {
> + ret = mtk_cci_set_voltage(cci_df, opp_voltage);
> + if (ret) {
> + pr_err("cci: failed to scale down voltage\n");
> + clk_set_rate(cci_df->cci_clk, cci_df->old_freq);
> + return ret;
> + }
> + }

I recommend that dev_pm_opp_set_rate() and dev_pm_opp_set_regulator()
instead of 'clk_set_rate' and 'regulator_set_voltage'.
In the dev_pm_opp_set_rate(), handle the these sequence.
You can refer the merged patch[1].

[1] commit 4294a779bd8dff6c65e7e85ffe7a1ea236e92a68
- PM / devfreq: exynos-bus: Convert to use dev_pm_opp_set_rate()

> +
> + cci_df->old_freq = *freq;
> +
> + return 0;
> +}
> +
> +static struct devfreq_dev_profile cci_devfreq_profile = {
> + .target = mtk_cci_devfreq_target,

Need to add '.exit' for calling dev_pm_opp_of_remove_table().
You can refer the merged devfreq patches like exynos_bus.c, imx-bus.c.

> +};
> +
> +static int mtk_cci_devfreq_probe(struct platform_device *pdev)
> +{
> + struct device *cci_dev = &pdev->dev;
> + struct cci_devfreq *cci_df;
> + struct devfreq_passive_data *passive_data;
> + int ret;
> +
> + cci_df = devm_kzalloc(cci_dev, sizeof(*cci_df), GFP_KERNEL);
> + if (!cci_df)
> + return -ENOMEM;
> +
> + cci_df->cci_clk = devm_clk_get(cci_dev, "cci_clock");
> + ret = PTR_ERR_OR_ZERO(cci_df->cci_clk);
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(cci_dev, "failed to get clock for CCI: %d\n",
> + ret);
> + return ret;
> + }
> + cci_df->proc_reg = devm_regulator_get_optional(cci_dev, "proc");
> + ret = PTR_ERR_OR_ZERO(cci_df->proc_reg);
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(cci_dev, "failed to get regulator for CCI: %d\n",
> + ret);
> + return ret;
> + }

I recommend that use dev_pm_opp_set_regulators.
You can refer the merged patch[1].

[1] commit 4294a779bd8dff6c65e7e85ffe7a1ea236e92a68
- PM / devfreq: exynos-bus: Convert to use dev_pm_opp_set_rate()

> + ret = regulator_enable(cci_df->proc_reg);
> + if (ret) {
> + pr_warn("enable buck for cci fail\n");

Use dev_err instead of 'pr_warn'.

> + return ret;
> + }
> +
> + ret = dev_pm_opp_of_add_table(cci_dev);
> + if (ret) {
> + dev_err(cci_dev, "Fail to init CCI OPP table: %d\n", ret);

How about changing the error log as following
because in this driver, use the 'failed to' sentence for error handling?

failed to get OPP table for CCI:L %d

> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, cci_df);
> +
> + passive_data = devm_kzalloc(cci_dev, sizeof(*passive_data), GFP_KERNEL);
> + if (!passive_data)
> + return -ENOMEM;

On this error case, you have to call dev_pm_opp_of_remove_table().
You better to make the 'err_opp' jump lable and then add 'goto err_opp'.

> +
> + passive_data->parent_type = CPUFREQ_PARENT_DEV;
> +
> + cci_df->devfreq = devm_devfreq_add_device(cci_dev,
> + &cci_devfreq_profile,
> + DEVFREQ_GOV_PASSIVE,
> + passive_data);
> + if (IS_ERR(cci_df->devfreq)) {
> + ret = PTR_ERR(cci_df->devfreq);
> + dev_err(cci_dev, "cannot create cci devfreq device:%d\n", ret);
> + dev_pm_opp_of_remove_table(cci_dev);

Instead of direct call, use 'goto err_opp'.

> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int mtk_cci_devfreq_remove(struct platform_device *pdev)
> +{
> + struct device *cci_dev = &pdev->dev;
> + struct cci_devfreq *cci_df;
> + struct notifier_block *opp_nb;
> +
> + cci_df = platform_get_drvdata(pdev);
> + opp_nb = &cci_df->opp_nb;
> +
> + dev_pm_opp_unregister_notifier(cci_dev, opp_nb);

This patch doesn't call the dev_pm_opp_register_notifier.
Please remove it.

> + devm_devfreq_remove_device(cci_dev, cci_df->devfreq);

Don't need to call this function because you used devm_devfreq_add_device().

> + dev_pm_opp_of_remove_table(cci_dev)> + regulator_disable(cci_df->proc_reg);
> +
> + return 0;
> +}
> +
> +static const __maybe_unused struct of_device_id
> + mediatek_cci_devfreq_of_match[] = {

Make it on one line and remove '__maybe_unused' keyword.
- mediatek_cci_devfreq_of_match-> mediatek_cci_of_match

> + { .compatible = "mediatek,mt8183-cci" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, mediatek_cci_devfreq_of_match);

ditto.

> +
> +static struct platform_driver cci_devfreq_driver = {
> + .probe = mtk_cci_devfreq_probe,
> + .remove = mtk_cci_devfreq_remove,
> + .driver = {
> + .name = "mediatek-cci-devfreq",
> + .of_match_table = of_match_ptr(mediatek_cci_devfreq_of_match),

ditto.

> + },
> +};
> +
> +static int __init mtk_cci_devfreq_init(void)
> +{
> + return platform_driver_register(&cci_devfreq_driver);
> +}
> +module_init(mtk_cci_devfreq_init)
> +
> +static void __exit mtk_cci_devfreq_exit(void)
> +{
> + platform_driver_unregister(&cci_devfreq_driver);
> +}
> +module_exit(mtk_cci_devfreq_exit)

Use 'module_platform_driver' instead of module_init and module_exit.

> +
> +MODULE_DESCRIPTION("Mediatek CCI devfreq driver");
> +MODULE_AUTHOR("Andrew-sh.Cheng <andrew-sh.cheng@xxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL v2");
>

--
Best Regards,
Chanwoo Choi
Samsung Electronics

Next message: kbuild test robot: "arch/x86/ia32/ia32_signal.c:350:9: sparse: sparse: incorrect type in argument 1 (different address spaces)"
Previous message: Vishal Sagar: "RE: [PATCH v14 1/2] media: dt-bindings: media: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem"
Next in thread: Chanwoo Choi: "Re: [PATCH 09/12] devfreq: add mediatek cci devfreq"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]