Re: [PATCH v2 1/4] cpufreq: mediatek: add mt8183 cpufreq support

Re: [PATCH v2 1/4] cpufreq: mediatek: add mt8183 cpufreq support

[Nicolas Boichat]

On Thu, Mar 28, 2019 at 11:46 PM Andrew-sh.Cheng
<andrew-sh.cheng@mediatek.com> wrote:
>
> For new mediatek chip mt8183,
> cci and little cluster share the same buck,
> so need to modify the attribute of regulator from exclusive to optional
>
> Intermediate clock is not always enabled by ccf in different projects,
> so cpufreq should always enable it by itself.

One comment, otherwise the changes look good. However, I feel that
this patch should be split in 3:
 1. Change to regulator_get_optional
 2. Enable inter_clk
 3. Add support for 8183

> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> ---
>  drivers/cpufreq/cpufreq-dt-platdev.c |  1 +
>  drivers/cpufreq/mediatek-cpufreq.c   | 12 ++++++++++--
>  2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
> index 47729a2..53ea52b 100644
> --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> @@ -117,6 +117,7 @@
>         { .compatible = "mediatek,mt817x", },
>         { .compatible = "mediatek,mt8173", },
>         { .compatible = "mediatek,mt8176", },
> +       { .compatible = "mediatek,mt8183", },
>
>         { .compatible = "nvidia,tegra124", },
>         { .compatible = "nvidia,tegra210", },
> diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
> index 48e9829..7cd01d3 100644
> --- a/drivers/cpufreq/mediatek-cpufreq.c
> +++ b/drivers/cpufreq/mediatek-cpufreq.c
> @@ -346,7 +346,7 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
>                 goto out_free_resources;
>         }
>
> -       proc_reg = regulator_get_exclusive(cpu_dev, "proc");
> +       proc_reg = regulator_get_optional(cpu_dev, "proc");
>         if (IS_ERR(proc_reg)) {
>                 if (PTR_ERR(proc_reg) == -EPROBE_DEFER)
>                         pr_warn("proc regulator for cpu%d not ready, retry.\n",
> @@ -376,13 +376,17 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
>                 goto out_free_resources;
>         }
>
> +       ret = clk_prepare_enable(inter_clk);

Should you disable the clock in mtk_cpu_dvfs_info_release?

> +       if (ret)
> +               goto out_free_opp_table;
> +
>         /* Search a safe voltage for intermediate frequency. */
>         rate = clk_get_rate(inter_clk);
>         opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate);
>         if (IS_ERR(opp)) {
>                 pr_err("failed to get intermediate opp for cpu%d\n", cpu);
>                 ret = PTR_ERR(opp);
> -               goto out_free_opp_table;
> +               goto out_disable_clock;
>         }
>         info->intermediate_voltage = dev_pm_opp_get_voltage(opp);
>         dev_pm_opp_put(opp);
> @@ -401,6 +405,9 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
>
>         return 0;
>
> +out_disable_clock:
> +       clk_disable_unprepare(inter_clk);
> +
>  out_free_opp_table:
>         dev_pm_opp_of_cpumask_remove_table(&info->cpus);
>
> @@ -543,6 +550,7 @@ static int mtk_cpufreq_probe(struct platform_device *pdev)
>         { .compatible = "mediatek,mt817x", },
>         { .compatible = "mediatek,mt8173", },
>         { .compatible = "mediatek,mt8176", },
> +       { .compatible = "mediatek,mt8183", },
>
>         { }
>  };
> --
> 1.8.1.1.dirty
>
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

RE: [PATCH v2 2/4] opp: add API which get max freq by voltage

[MyungJoo Ham]

>This API will get voltage as input parameter.
>Search all opp items for the item which with max frequency,
>and the voltae is smaller than provided voltage.
>
>Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
>---
> drivers/opp/core.c     | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/pm_opp.h |  8 ++++++++
> 2 files changed, 63 insertions(+)
>
>diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
>index 24c757a..57deef9 100644
>--- a/include/linux/pm_opp.h
>+++ b/include/linux/pm_opp.h
>@@ -102,6 +102,8 @@ struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
> 
> struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
> 					      unsigned long *freq);
>+struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
>+					      unsigned long u_volt);

For the symmetricity, wouldn't it be better to name it
dev_pm_opp_find_volt_ceiling(dev, u_volt); ?

Cheers,
MyungJoo

RE: [PATCH v2 4/4] devfreq: add mediatek cci devfreq

[MyungJoo Ham]

>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@mediatek.com>
>---
> drivers/devfreq/Kconfig              |  10 ++
> drivers/devfreq/Makefile             |   1 +
> drivers/devfreq/mt8183-cci-devfreq.c | 235 +++++++++++++++++++++++++++++++++++
> 3 files changed, 246 insertions(+)
> create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
>

1. It appears that proc_reg_uV might be used before initialization.
It would be appropriate to initialize it at the probe function.

2. Because you are already using OPP, why don't you rely on
OPP fully? (use OPP from CPUFreq drivers as well in order to get
OPP events automatically.) Anyway, this is a minor item that does
not need to be corrected.

Cheers
MyungJoo

Re: [v2,4/4] devfreq: add mediatek cci devfreq

[Guenter Roeck]

On Fri, Mar 29, 2019 at 02:46:12PM +0800, 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@mediatek.com>
> ---
>  drivers/devfreq/Kconfig              |  10 ++
>  drivers/devfreq/Makefile             |   1 +
>  drivers/devfreq/mt8183-cci-devfreq.c | 235 +++++++++++++++++++++++++++++++++++
>  3 files changed, 246 insertions(+)
>  create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 6a172d3..da2f8ec 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -91,6 +91,16 @@ config ARM_EXYNOS_BUS_DEVFREQ
>  	  and adjusts the operating frequencies and voltages with OPP support.
>  	  This does not yet operate with optimal voltages.
>  
> +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.
> +		Use notification to get regulator status.
> +
>  config ARM_TEGRA_DEVFREQ
>  	tristate "Tegra DEVFREQ Driver"
>  	depends on ARCH_TEGRA_124_SOC
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 32b8d4d..817dde7 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
>  
>  # DEVFREQ Drivers
>  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.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)		+= tegra-devfreq.o
>  
> diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c
> new file mode 100644
> index 0000000..af82d2e
> --- /dev/null
> +++ b/drivers/devfreq/mt8183-cci-devfreq.c
> @@ -0,0 +1,235 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 MediaTek Inc.
> + */
> +
> +#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 "governor.h"
> +
> +struct cci_devfreq {
> +	struct devfreq		*devfreq;
> +	struct regulator	*proc_reg;
> +	unsigned long           proc_reg_uV;
> +	struct clk		*cci_clk;
> +	struct notifier_block	nb;
> +};
> +
> +static int cci_devfreq_regulator_notifier(struct notifier_block *nb,
> +					  unsigned long val, void *data)
> +{
> +	struct cci_devfreq *cci_df =
> +		container_of(nb, struct cci_devfreq, nb);
> +
> +	/* deal with reduce frequency */
> +	if (val & REGULATOR_EVENT_PRE_VOLTAGE_CHANGE) {
> +		struct pre_voltage_change_data *pvc_data = data;
> +
> +		if (pvc_data->min_uV < pvc_data->old_uV) {
> +			cci_df->proc_reg_uV =
> +				(unsigned long)(pvc_data->min_uV);
> +			mutex_lock(&cci_df->devfreq->lock);
> +			update_devfreq(cci_df->devfreq);
> +			mutex_unlock(&cci_df->devfreq->lock);
> +		}
> +	}
> +
> +	if ((val & REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE) &&
> +	    ((unsigned long)data > cci_df->proc_reg_uV)) {
> +		cci_df->proc_reg_uV = (unsigned long)data;
> +		mutex_lock(&cci_df->devfreq->lock);
> +		update_devfreq(cci_df->devfreq);
> +		mutex_unlock(&cci_df->devfreq->lock);
> +	}
> +
> +	/* deal with increase frequency */
> +	if ((val & REGULATOR_EVENT_VOLTAGE_CHANGE) &&
> +	    (cci_df->proc_reg_uV < (unsigned long)data)) {
> +		cci_df->proc_reg_uV = (unsigned long)data;
> +		mutex_lock(&cci_df->devfreq->lock);
> +		update_devfreq(cci_df->devfreq);
> +		mutex_unlock(&cci_df->devfreq->lock);
> +	}
> +
> +	return 0;
> +}
> +
> +static int mtk_cci_governor_get_target(struct devfreq *devfreq,
> +				       unsigned long *freq)
> +{
> +	struct cci_devfreq *cci_df;
> +	struct dev_pm_opp *opp;
> +
> +	cci_df = dev_get_drvdata(devfreq->dev.parent);
> +
> +	/* find available frequency */
> +	opp = dev_pm_opp_find_max_freq_by_volt(devfreq->dev.parent,
> +						 cci_df->proc_reg_uV);
> +	*freq = dev_pm_opp_get_freq(opp);
> +
> +	return 0;
> +}
> +
> +static int mtk_cci_governor_event_handler(struct devfreq *devfreq,
> +					  unsigned int event, void *data)
> +{
> +	struct cci_devfreq *cci_df;
> +	struct notifier_block *nb;
> +
> +	cci_df = dev_get_drvdata(devfreq->dev.parent);
> +	nb = &cci_df->nb;
> +
> +	switch (event) {
> +	case DEVFREQ_GOV_START:
> +	case DEVFREQ_GOV_RESUME:
> +		nb->notifier_call = cci_devfreq_regulator_notifier;
> +		regulator_register_notifier(cci_df->proc_reg,
> +					    nb);
> +		break;
> +
> +	case DEVFREQ_GOV_STOP:
> +	case DEVFREQ_GOV_SUSPEND:
> +		regulator_unregister_notifier(cci_df->proc_reg,
> +					    nb);
> +
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct devfreq_governor mtk_cci_devfreq_governor = {
> +	.name = "mtk_cci_vmon",
> +	.get_target_freq = mtk_cci_governor_get_target,
> +	.event_handler = mtk_cci_governor_event_handler,
> +};
> +
> +static int mtk_cci_devfreq_target(struct device *dev, unsigned long *freq,
> +				  u32 flags)
> +{
> +	struct cci_devfreq *cci_df = dev_get_drvdata(dev);
> +
> +	if (!cci_df)
> +		return -EINVAL;
> +
> +	clk_set_rate(cci_df->cci_clk, *freq);
> +
> +	return 0;
> +}
> +
> +static struct devfreq_dev_profile cci_devfreq_profile = {
> +	.target		= mtk_cci_devfreq_target,
> +};
> +
> +static int mtk_cci_devfreq_probe(struct platform_device *pdev)
> +{
> +	struct device *cci_dev = &pdev->dev;
> +	struct cci_devfreq *cci_df;
> +	int ret;
> +
> +	cci_df = devm_kzalloc(cci_dev, sizeof(*cci_df), GFP_KERNEL);
> +	if (!cci_df)
> +		return -ENOMEM;
> +
> +	cci_df->cci_clk = clk_get(cci_dev, "cci_clock");

Should use devm_clk_get().

> +	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 = regulator_get_optional(cci_dev, "proc");

Should use devm_regulator_get_optional().

> +	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);
> +
> +		goto err_put_clk;
> +	}
> +
> +	ret = dev_pm_opp_of_add_table(&pdev->dev);
> +	if (ret) {
> +		dev_err(cci_dev, "Fail to init CCI OPP table\n");

No error code display here ? Not that it really matters, but it is
inconsistent with the other error messages.

> +		goto err_put_reg;
> +	}
> +
> +	platform_set_drvdata(pdev, cci_df);
> +
> +	cci_df->devfreq = devm_devfreq_add_device(cci_dev,
> +						       &cci_devfreq_profile,
> +						       "mtk_cci_vmon",
> +						       NULL);
> +	if (IS_ERR(cci_df->devfreq)) {

		ret = PTR_ERR(cci_df->devfreq);

> +		dev_err(cci_dev, "cannot create cci devfreq device\n", ret);

		dev_err(cci_dev, "cannot create cci devfreq device: %d\n", ret);

Something like
		dev_pm_opp_of_remove_table(...);
seems to be missing here.

> +		goto err_put_reg;
> +	}
> +
> +	return 0;
> +
> +err_put_reg:
> +	regulator_put(cci_df->proc_reg);
> +err_put_clk:
> +	clk_put(cci_df->cci_clk);

Can be dropped when using devm_ functions above.

> +
> +	return ret;
> +}
> +
> +static const struct of_device_id mediatek_cci_devfreq_of_match[] = {
> +	{ .compatible = "mediatek,mt8183-cci" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, mediatek_cci_devfreq_of_match);
> +
> +static struct platform_driver cci_devfreq_driver = {
> +	.probe	= mtk_cci_devfreq_probe,
> +	.driver = {
> +		.name = "mediatek-cci-devfreq",
> +		.of_match_table = mediatek_cci_devfreq_of_match,

If the driver depends on OF, that should be stated in the Kconfig file.
Otherwise, of_match_ptr() should be used, and mediatek_cci_devfreq_of_match
should be tagged with __maybe_unused (or made conditional with #ifdef).

> +	},
> +};
> +
> +static int __init mtk_cci_devfreq_init(void)
> +{
> +	int ret;
> +
> +	ret = devfreq_add_governor(&mtk_cci_devfreq_governor);
> +	if (ret) {
> +		pr_err("%s: failed to add governor: %d\n", __func__, ret);
> +		return ret;
> +	}
> +
> +	ret = platform_driver_register(&cci_devfreq_driver);
> +	if (ret)
> +		devfreq_remove_governor(&mtk_cci_devfreq_governor);
> +
> +	return ret;
> +}
> +module_init(mtk_cci_devfreq_init)
> +
> +static void __exit mtk_cci_devfreq_exit(void)
> +{
> +	int ret;
> +
> +	ret = devfreq_remove_governor(&mtk_cci_devfreq_governor);
> +	if (ret)
> +		pr_err("%s: failed to remove governor: %d\n", __func__, ret);
> +
> +	platform_driver_unregister(&cci_devfreq_driver);
> +}
> +module_exit(mtk_cci_devfreq_exit)
> +
> +MODULE_DESCRIPTION("Mediatek CCI devfreq driver");
> +MODULE_AUTHOR("Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH v2 4/4] devfreq: add mediatek cci devfreq

[Chanwoo Choi]

Hi Andrew-sh.Cheng,

Please add the dt-binding documentation.

On 19. 3. 29. 오후 3:46, 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@mediatek.com>
> ---
>  drivers/devfreq/Kconfig              |  10 ++
>  drivers/devfreq/Makefile             |   1 +
>  drivers/devfreq/mt8183-cci-devfreq.c | 235 +++++++++++++++++++++++++++++++++++
>  3 files changed, 246 insertions(+)
>  create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 6a172d3..da2f8ec 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -91,6 +91,16 @@ config ARM_EXYNOS_BUS_DEVFREQ
>  	  and adjusts the operating frequencies and voltages with OPP support.
>  	  This does not yet operate with optimal voltages.
>  
> +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.
> +		Use notification to get regulator status.
> +
>  config ARM_TEGRA_DEVFREQ
>  	tristate "Tegra DEVFREQ Driver"
>  	depends on ARCH_TEGRA_124_SOC
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 32b8d4d..817dde7 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
>  
>  # DEVFREQ Drivers
>  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.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)		+= tegra-devfreq.o
>  
> diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c
> new file mode 100644
> index 0000000..af82d2e
> --- /dev/null
> +++ b/drivers/devfreq/mt8183-cci-devfreq.c
> @@ -0,0 +1,235 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 MediaTek Inc.

You can add the author information under copyright information.

> + */
> +
> +#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 "governor.h"
> +
> +struct cci_devfreq {
> +	struct devfreq		*devfreq;
> +	struct regulator	*proc_reg;
> +	unsigned long           proc_reg_uV;
> +	struct clk		*cci_clk;
> +	struct notifier_block	nb;

Just use the space instead of tab as following:

	struct devfreq *devfreq;
	struct regulator *proc_reg;
	unsigned long proc_reg_uV;
	struct clk *cci_clk;
	struct notifier_block nb;

> +};
> +
> +static int cci_devfreq_regulator_notifier(struct notifier_block *nb,
> +					  unsigned long val, void *data)
> +{
> +	struct cci_devfreq *cci_df =
> +		container_of(nb, struct cci_devfreq, nb);
> +
> +	/* deal with reduce frequency */
> +	if (val & REGULATOR_EVENT_PRE_VOLTAGE_CHANGE) {
> +		struct pre_voltage_change_data *pvc_data = data;
> +
> +		if (pvc_data->min_uV < pvc_data->old_uV) {
> +			cci_df->proc_reg_uV =
> +				(unsigned long)(pvc_data->min_uV);
> +			mutex_lock(&cci_df->devfreq->lock);
> +			update_devfreq(cci_df->devfreq);

Please handle the return value of update_devfreq() for exception handling.

> +			mutex_unlock(&cci_df->devfreq->lock);
> +		}
> +	}
> +
> +	if ((val & REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE) &&

if -> else if

At the same time, receives only one notification.
It is not necessary to check the two if statement always, .


> +	    ((unsigned long)data > cci_df->proc_reg_uV)) {
> +		cci_df->proc_reg_uV = (unsigned long)data;
> +		mutex_lock(&cci_df->devfreq->lock);
> +		update_devfreq(cci_df->devfreq);

ditto. Please check the return value of update_devfreq.

> +		mutex_unlock(&cci_df->devfreq->lock);
> +	}
> +
> +	/* deal with increase frequency */
> +	if ((val & REGULATOR_EVENT_VOLTAGE_CHANGE) &&

ditto.
if -> else if

At the same time, receives only one notification.
It is not necessary to check the two if statement always, .


> +	    (cci_df->proc_reg_uV < (unsigned long)data)) {
> +		cci_df->proc_reg_uV = (unsigned long)data;
> +		mutex_lock(&cci_df->devfreq->lock);
> +		update_devfreq(cci_df->devfreq);

ditto. Please check the return value of update_devfreq.

> +		mutex_unlock(&cci_df->devfreq->lock);
> +	}
> +
> +	return 0;
> +}
> +
> +static int mtk_cci_governor_get_target(struct devfreq *devfreq,
> +				       unsigned long *freq)
> +{
> +	struct cci_devfreq *cci_df;
> +	struct dev_pm_opp *opp;
> +
> +	cci_df = dev_get_drvdata(devfreq->dev.parent);
> +
> +	/* find available frequency */
> +	opp = dev_pm_opp_find_max_freq_by_volt(devfreq->dev.parent,
> +						 cci_df->proc_reg_uV);
> +	*freq = dev_pm_opp_get_freq(opp);
> +
> +	return 0;
> +}
> +
> +static int mtk_cci_governor_event_handler(struct devfreq *devfreq,
> +					  unsigned int event, void *data)
> +{
> +	struct cci_devfreq *cci_df;
> +	struct notifier_block *nb;
> +
> +	cci_df = dev_get_drvdata(devfreq->dev.parent);
> +	nb = &cci_df->nb;
> +
> +	switch (event) {
> +	case DEVFREQ_GOV_START:
> +	case DEVFREQ_GOV_RESUME:
> +		nb->notifier_call = cci_devfreq_regulator_notifier;
> +		regulator_register_notifier(cci_df->proc_reg,
> +					    nb);

Please check the return value of regulator_register_notifier
in order to handle the exception handling.


> +		break;
> +
> +	case DEVFREQ_GOV_STOP:
> +	case DEVFREQ_GOV_SUSPEND:
> +		regulator_unregister_notifier(cci_df->proc_reg,
> +					    nb);

ditto.

> +

Remove unneeded blank line.

> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct devfreq_governor mtk_cci_devfreq_governor = {
> +	.name = "mtk_cci_vmon",

How about editing the name from mtk_cci_vmon to mediatek_cci_voltage_monitor
for the readability? Actually, 'mtk' and 'vmon' are not standard expression.

> +	.get_target_freq = mtk_cci_governor_get_target,
> +	.event_handler = mtk_cci_governor_event_handler,

Please add following code to make it as the immutable
because the governor for this driver will not be changed
through sysfs interface.

	.immutable = true

> +};
> +
> +static int mtk_cci_devfreq_target(struct device *dev, unsigned long *freq,
> +				  u32 flags)
> +{
> +	struct cci_devfreq *cci_df = dev_get_drvdata(dev);
> +
> +	if (!cci_df)
> +		return -EINVAL;
> +
> +	clk_set_rate(cci_df->cci_clk, *freq);

Please check the return value of clk_set_rate().

> +
> +	return 0;
> +}
> +
> +static struct devfreq_dev_profile cci_devfreq_profile = {
> +	.target		= mtk_cci_devfreq_target,

Just use the space instead of tab because cci_devfreq_profile
only has one record.

	.target = mtk_cci_devfreq_target,

> +};
> +
> +static int mtk_cci_devfreq_probe(struct platform_device *pdev)
> +{
> +	struct device *cci_dev = &pdev->dev;
> +	struct cci_devfreq *cci_df;
> +	int ret;
> +
> +	cci_df = devm_kzalloc(cci_dev, sizeof(*cci_df), GFP_KERNEL);
> +	if (!cci_df)
> +		return -ENOMEM;
> +
> +	cci_df->cci_clk = clk_get(cci_dev, "cci_clock");

clk_get -> devm_clk_get()

> +	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);
> +

Remove unneeded blank line.

> +		return ret;
> +	}> +
> +	cci_df->proc_reg = regulator_get_optional(cci_dev, "proc");


regulator_get_optional -> devm_regulator_get_optional

> +	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);
> +

Remove unneeded blank line.

> +		goto err_put_clk;

If you use devm_clk_get(), just return instead of goto.

> +	}
> +
> +	ret = dev_pm_opp_of_add_table(&pdev->dev);
> +	if (ret) {
> +		dev_err(cci_dev, "Fail to init CCI OPP table\n");
> +		goto err_put_reg;
> +	}
> +
> +	platform_set_drvdata(pdev, cci_df);
> +
> +	cci_df->devfreq = devm_devfreq_add_device(cci_dev,
> +						       &cci_devfreq_profile,
> +						       "mtk_cci_vmon",
> +						       NULL);
> +	if (IS_ERR(cci_df->devfreq)) {
> +		dev_err(cci_dev, "cannot create cci devfreq device\n", ret);
> +		goto err_put_reg;
> +	}
> +
> +	return 0;
> +
> +err_put_reg:
> +	regulator_put(cci_df->proc_reg);
> +err_put_clk:
> +	clk_put(cci_df->cci_clk);

If you use devm_clk_get()/devm_regulator_get(),
'err_put_reg' and 'err_put_clk' are unneeded.

> +
> +	return ret;
> +}
> +
> +static const struct of_device_id mediatek_cci_devfreq_of_match[] = {
> +	{ .compatible = "mediatek,mt8183-cci" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, mediatek_cci_devfreq_of_match);
> +
> +static struct platform_driver cci_devfreq_driver = {
> +	.probe	= mtk_cci_devfreq_probe,
> +	.driver = {
> +		.name = "mediatek-cci-devfreq",
> +		.of_match_table = mediatek_cci_devfreq_of_match,
> +	},
> +};
> +
> +static int __init mtk_cci_devfreq_init(void)
> +{
> +	int ret;
> +
> +	ret = devfreq_add_governor(&mtk_cci_devfreq_governor);
> +	if (ret) {
> +		pr_err("%s: failed to add governor: %d\n", __func__, ret);
> +		return ret;
> +	}
> +
> +	ret = platform_driver_register(&cci_devfreq_driver);
> +	if (ret)
> +		devfreq_remove_governor(&mtk_cci_devfreq_governor);
> +
> +	return ret;
> +}
> +module_init(mtk_cci_devfreq_init)
> +
> +static void __exit mtk_cci_devfreq_exit(void)
> +{
> +	int ret;
> +
> +	ret = devfreq_remove_governor(&mtk_cci_devfreq_governor);
> +	if (ret)
> +		pr_err("%s: failed to remove governor: %d\n", __func__, ret);
> +
> +	platform_driver_unregister(&cci_devfreq_driver);
> +}
> +module_exit(mtk_cci_devfreq_exit)
> +
> +MODULE_DESCRIPTION("Mediatek CCI devfreq driver");
> +MODULE_AUTHOR("Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>");
> +MODULE_LICENSE("GPL v2");
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 3/4] dt-bindings: devfreq: add compatible for mt8183 cci devfreq

[Chanwoo Choi]

Hi,

I already reviewed this patch on v1[1].
[1] https://lkml.org/lkml/2019/2/11/2228

But, the second version patch doesn't include the anything
about review. Please check it[1].

On 19. 3. 29. 오후 3:46, Andrew-sh.Cheng wrote:
> This adds dt-binding documentation of cci devfreq
> for Mediatek MT8183 SoC platform.
> 
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> ---
>  .../bindings/devfreq/mt8183-cci-devfreq.txt           | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/devfreq/mt8183-cci-devfreq.txt
> 
> diff --git a/Documentation/devicetree/bindings/devfreq/mt8183-cci-devfreq.txt b/Documentation/devicetree/bindings/devfreq/mt8183-cci-devfreq.txt
> new file mode 100644
> index 0000000..e2b61cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/devfreq/mt8183-cci-devfreq.txt
> @@ -0,0 +1,19 @@
> +* Mediatek CCI frequency device
> +
> +Required properties:
> +- compatible: should contain "mediatek,mt8183-cci" for cci devfreq
> +
> +- clocks: for cci devfreq
> +
> +- clock-names: for cci devfreq driver to reference
> +
> +- operating-points-v2: for cci devfreq opp table
> +
> +Example:
> +	cci: cci {
> +		compatible = "mediatek,cci";
> +		clocks = <&apmixedsys CLK_APMIXED_CCIPLL>;
> +		clock-names = "cci_clock";
> +		operating-points-v2 = <&cci_opp>;
> +	};
> +
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 2/4] opp: add API which get max freq by voltage

[Nicolas Boichat]

On Fri, Mar 29, 2019 at 2:46 PM Andrew-sh.Cheng
<andrew-sh.cheng@mediatek.com> wrote:
>
> This API will get voltage as input parameter.
> Search all opp items for the item which with max frequency,
> and the voltae is smaller than provided voltage.
>
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> ---
>  drivers/opp/core.c     | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_opp.h |  8 ++++++++
>  2 files changed, 63 insertions(+)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 0420f7e..7323cd9 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -526,6 +526,61 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
>
> +/**
> + * dev_pm_opp_find_max_freq_by_volt() - Search for a opp with max freq
> + * under provided voltage
> + * @dev:       device for which we do this operation
> + * @u_volt:    provided voltage
> + *
> + * Search for the matching available OPP which provide voltage can support.
> + *
> + * Return: matching *opp, else returns ERR_PTR in case of error
> + * and should be handled using IS_ERR.
> + * Error return values can be:
> + * EINVAL:     for bad pointer
> + * ERANGE:     no match found for search
> + * ENODEV:     if device not found in list of registered devices
> + *
> + * The callers are required to call dev_pm_opp_put() for the returned OPP after
> + * use.
> + */
> +struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
> +                                             unsigned long u_volt)
> +{
> +       struct opp_table *opp_table;
> +       struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
> +
> +       if (!dev || !u_volt) {
> +               dev_err(dev, "%s: Invalid argument volt=%d\n", __func__,
> +                       u_volt);

u_volt is an unsigned long, so you should use %lu.

drivers/opp/core.c:582:3: error: format '%d' expects argument of type
'int', but argument 4 has type 'long unsigned int' [-Werror=format=]
  chromeos-kernel-4_19-4.19.32-r271:    dev_err(dev, "%s: Invalid
argument volt=%d\n", __func__,
  chromeos-kernel-4_19-4.19.32-r271:    ^

> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       opp_table = _find_opp_table(dev);
> +       if (IS_ERR(opp_table))
> +               return ERR_CAST(opp_table);
> +
> +       mutex_lock(&opp_table->lock);
> +
> +       list_for_each_entry(temp_opp, &opp_table->opp_list, node) {
> +               if (temp_opp->available) {
> +                       /* go to the next node, before choosing prev */
> +                       if (temp_opp->supplies[0].u_volt > u_volt)
> +                               break;
> +                       opp = temp_opp;
> +               }
> +       }
> +
> +       /* Increment the reference count of OPP */
> +       if (!IS_ERR(opp))
> +               dev_pm_opp_get(opp);
> +       mutex_unlock(&opp_table->lock);
> +       dev_pm_opp_put_opp_table(opp_table);
> +
> +       return opp;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_opp_find_max_freq_by_volt);
> +
>  static int _set_opp_voltage(struct device *dev, struct regulator *reg,
>                             struct dev_pm_opp_supply *supply)
>  {
> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> index 24c757a..57deef9 100644
> --- a/include/linux/pm_opp.h
> +++ b/include/linux/pm_opp.h
> @@ -102,6 +102,8 @@ struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
>
>  struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
>                                               unsigned long *freq);
> +struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
> +                                             unsigned long u_volt);
>
>  struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
>                                              unsigned long *freq);
> @@ -207,6 +209,12 @@ static inline struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
>         return ERR_PTR(-ENOTSUPP);
>  }
>
> +static inline struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
> +                                             unsigned long u_volt)
> +{
> +       return ERR_PTR(-ENOTSUPP);
> +}
> +
>  static inline struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
>                                         unsigned long *freq)
>  {
> --
> 1.8.1.1.dirty
>
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v2 2/4] opp: add API which get max freq by voltage

[Viresh Kumar]

On 29-03-19, 14:46, Andrew-sh.Cheng wrote:
> This API will get voltage as input parameter.
> Search all opp items for the item which with max frequency,
> and the voltae is smaller than provided voltage.
> 
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> ---
>  drivers/opp/core.c     | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_opp.h |  8 ++++++++
>  2 files changed, 63 insertions(+)

I have applied this patch with some modifications, here is the diff:

---
 drivers/opp/core.c     | 29 ++++++++++++++---------------
 include/linux/pm_opp.h |  8 ++++----
 2 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 7323cd9aabf9..0e7703fe733f 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -527,31 +527,30 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
 /**
- * dev_pm_opp_find_max_freq_by_volt() - Search for a opp with max freq
- * under provided voltage
- * @dev:	device for which we do this operation
- * @u_volt:	provided voltage
+ * dev_pm_opp_find_freq_ceil_by_volt() - Find OPP with highest frequency for
+ *					 target voltage.
+ * @dev:	Device for which we do this operation.
+ * @u_volt:	Target voltage.
+ *
+ * Search for OPP with highest (ceil) frequency and has voltage <= u_volt.
  *
- * Search for the matching available OPP which provide voltage can support.
+ * Return: matching *opp, else returns ERR_PTR in case of error which should be
+ * handled using IS_ERR.
  *
- * Return: matching *opp, else returns ERR_PTR in case of error
- * and should be handled using IS_ERR.
  * Error return values can be:
- * EINVAL:	for bad pointer
- * ERANGE:	no match found for search
- * ENODEV:	if device not found in list of registered devices
+ * EINVAL:	bad parameters
  *
  * The callers are required to call dev_pm_opp_put() for the returned OPP after
  * use.
  */
-struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
-					      unsigned long u_volt)
+struct dev_pm_opp *dev_pm_opp_find_freq_ceil_by_volt(struct device *dev,
+						     unsigned long u_volt)
 {
 	struct opp_table *opp_table;
 	struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
 
 	if (!dev || !u_volt) {
-		dev_err(dev, "%s: Invalid argument volt=%d\n", __func__,
+		dev_err(dev, "%s: Invalid argument volt=%lu\n", __func__,
 			u_volt);
 		return ERR_PTR(-EINVAL);
 	}
@@ -564,7 +563,6 @@ struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
 
 	list_for_each_entry(temp_opp, &opp_table->opp_list, node) {
 		if (temp_opp->available) {
-			/* go to the next node, before choosing prev */
 			if (temp_opp->supplies[0].u_volt > u_volt)
 				break;
 			opp = temp_opp;
@@ -574,12 +572,13 @@ struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
 	/* Increment the reference count of OPP */
 	if (!IS_ERR(opp))
 		dev_pm_opp_get(opp);
+
 	mutex_unlock(&opp_table->lock);
 	dev_pm_opp_put_opp_table(opp_table);
 
 	return opp;
 }
-EXPORT_SYMBOL_GPL(dev_pm_opp_find_max_freq_by_volt);
+EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_ceil_by_volt);
 
 static int _set_opp_voltage(struct device *dev, struct regulator *reg,
 			    struct dev_pm_opp_supply *supply)
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 57deef9cf5d3..b150fe97ce5a 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -102,8 +102,8 @@ struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
 
 struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 					      unsigned long *freq);
-struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
-					      unsigned long u_volt);
+struct dev_pm_opp *dev_pm_opp_find_freq_ceil_by_volt(struct device *dev,
+						     unsigned long u_volt);
 
 struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
 					     unsigned long *freq);
@@ -209,8 +209,8 @@ static inline struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 	return ERR_PTR(-ENOTSUPP);
 }
 
-static inline struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
-					      unsigned long u_volt)
+static inline struct dev_pm_opp *dev_pm_opp_find_freq_ceil_by_volt(struct device *dev,
+					unsigned long u_volt)
 {
 	return ERR_PTR(-ENOTSUPP);
 }

Re: [PATCH v2 2/4] opp: add API which get max freq by voltage

[Viresh Kumar]

On Fri, 29 Mar 2019 at 12:16, Andrew-sh.Cheng
<andrew-sh.cheng@mediatek.com> wrote:
>
> This API will get voltage as input parameter.
> Search all opp items for the item which with max frequency,
> and the voltae is smaller than provided voltage.
>
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> ---
>  drivers/opp/core.c     | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_opp.h |  8 ++++++++
>  2 files changed, 63 insertions(+)

This patch added dev_pm_opp_find_freq_ceil_by_volt() but I don't find anything
in the kernel which uses it? The patchset for CCI never got merged ?

I will remove the API now.

--
Viresh