[PATCH 0/3] Add cpufreq and cci devfreq for mt8183

[PATCH 0/3] Add cpufreq and cci devfreq for mt8183

[Andrew-sh Cheng]

MT8183 supports CPU DVFS and CCI DVFS, and LITTLE cpus and CCI are in the same voltage domain.
So, this series is to add drivers to handle the voltage coupling between CPU and CCI DVFS.

Andrew-sh.Cheng (3):
  cpufreq: mediatek: add mt8183 cpufreq support
  dt-bindings: devfreq: add compatible for mt8183 cci devfreq
  devfreq: add mediatek cci devfreq

 .../bindings/devfreq/mt8183-cci-devfreq.txt        |  19 ++
 drivers/cpufreq/cpufreq-dt-platdev.c               |   1 +
 drivers/cpufreq/mediatek-cpufreq.c                 |   7 +-
 drivers/devfreq/Kconfig                            |   9 +
 drivers/devfreq/Makefile                           |   1 +
 drivers/devfreq/mt8183-cci-devfreq.c               | 224 +++++++++++++++++++++
 6 files changed, 260 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/devfreq/mt8183-cci-devfreq.txt
 create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c

-- 
1.8.1.1.dirty


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

[PATCH 1/3] cpufreq: mediatek: add mt8183 cpufreq support

[Andrew-sh Cheng]

From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>

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.

Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
---
 drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
 drivers/cpufreq/mediatek-cpufreq.c   | 7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
index b1c5468..5a1c588 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", },
 
diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index eb8920d..e956248 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -355,7 +355,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",
@@ -385,6 +385,9 @@ 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);
+	if (ret)
+		goto out_free_resources;
 	/* Search a safe voltage for intermediate frequency. */
 	rate = clk_get_rate(inter_clk);
 	opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate);
@@ -412,6 +415,7 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 
 out_free_opp_table:
 	dev_pm_opp_of_cpumask_remove_table(&info->cpus);
+	clk_disable_unprepare(inter_clk);
 
 out_free_resources:
 	if (!IS_ERR(proc_reg))
@@ -551,6 +555,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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] dt-bindings: devfreq: add compatible for mt8183 cci devfreq

[Andrew-sh Cheng]

From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>

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>;
+	};
+
-- 
1.8.1.1.dirty


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

[PATCH 3/3] devfreq: add mediatek cci devfreq

[Andrew-sh Cheng]

From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>

For big/little cpu cluster architecture,
not only CPU frequency, but CCI frequency will also affect performance.

Little cores and cci share the same buck in Mediatek mt8183 IC,
so we add a CCI devfreq which will get notification when buck voltage
is changed, then CCI devfreq can set cci frequency as high as possible.

Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
---
 drivers/devfreq/Kconfig              |   9 ++
 drivers/devfreq/Makefile             |   1 +
 drivers/devfreq/mt8183-cci-devfreq.c | 224 +++++++++++++++++++++++++++++++++++
 3 files changed, 234 insertions(+)
 create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 6a172d3..0b14aab 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -113,6 +113,15 @@ config ARM_RK3399_DMC_DEVFREQ
           It sets the frequency for the memory controller and reads the usage counts
           from hardware.
 
+config ARM_MT8183_CCI_DEVFREQ
+	tristate "MT8183 CCI DEVFREQ Driver"
+	depends on ARM_MEDIATEK_CPUFREQ
+	help
+		This adds devfreq for cci clock
+		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.
+
 source "drivers/devfreq/event/Kconfig"
 
 endif # PM_DEVFREQ
diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
index 32b8d4d..25afe8c 100644
--- a/drivers/devfreq/Makefile
+++ b/drivers/devfreq/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
 obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
 obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
 obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
+obj-$(CONFIG_ARM_MT8183_CCI_DEVFREQ)	+= mt8183-cci-devfreq.o
 
 # DEVFREQ Event Drivers
 obj-$(CONFIG_PM_DEVFREQ_EVENT)		+= event/
diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c
new file mode 100644
index 0000000..4837892
--- /dev/null
+++ b/drivers/devfreq/mt8183-cci-devfreq.c
@@ -0,0 +1,224 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (c) 2018 MediaTek Inc.
+#include <linux/cpu.h>
+#include <linux/cpufreq.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_data {
+	struct devfreq		*devfreq;
+	struct regulator	*proc_reg;
+	struct clk		*cci_clk;
+	unsigned long		buck_volt;
+	int			volt_increasing;
+	struct notifier_block	nb;
+};
+
+static int mtk_cci_governor_get_target(struct devfreq *devfreq,
+				       unsigned long *freq)
+{
+	struct cci_devfreq_data *cci_devdata;
+	int	i, opp_count;
+	struct dev_pm_opp	*opp;
+	unsigned long	opp_rate, opp_voltage;
+
+	cci_devdata = dev_get_drvdata(devfreq->dev.parent);
+
+	/* find available frequency */
+	opp_count = dev_pm_opp_get_opp_count(devfreq->dev.parent);
+	for (i = 0, opp_rate = ULONG_MAX; i < opp_count; i++, opp_rate--) {
+		opp = dev_pm_opp_find_freq_floor(devfreq->dev.parent,
+						 &opp_rate);
+		opp_voltage = dev_pm_opp_get_voltage(opp);
+		dev_pm_opp_put(opp);
+
+		if (opp_voltage <= cci_devdata->buck_volt)
+			break;
+	}
+	*freq = opp_rate;
+
+	return 0;
+}
+
+static int mtk_cci_governor_event_handler(struct devfreq *devfreq,
+					  unsigned int event, void *data)
+{
+	return 0;
+}
+
+static struct devfreq_governor mtk_cci_devfreq_governor = {
+	.name = "voltage_monitor",
+	.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_data *cci_devdata;
+
+	cci_devdata = dev_get_drvdata(dev);
+
+	clk_set_rate(cci_devdata->cci_clk, *freq);
+
+	return 0;
+}
+
+static struct devfreq_dev_profile cci_devfreq_profile = {
+	.polling_ms	= 0,
+	.target		= mtk_cci_devfreq_target,
+};
+
+static int cci_devfreq_regulator_notifier(struct notifier_block *nb,
+					  unsigned long val, void *data)
+{
+	struct cci_devfreq_data *cci_devdata =
+		container_of(nb, struct cci_devfreq_data, nb);
+
+	/* deal with reduce frequency */
+	if (val & REGULATOR_EVENT_PRE_VOLTAGE_CHANGE) {
+		struct pre_voltage_change_data *pvc_data = data;
+
+		if (pvc_data->old_uV > pvc_data->min_uV) {
+			cci_devdata->volt_increasing = 0;
+			cci_devdata->buck_volt =
+				(unsigned long)(pvc_data->min_uV);
+			mutex_lock(&cci_devdata->devfreq->lock);
+			update_devfreq(cci_devdata->devfreq);
+			mutex_unlock(&cci_devdata->devfreq->lock);
+		} else {
+			cci_devdata->volt_increasing = 1;
+		}
+	}
+	/* deal with abort reduce frequency */
+	if ((val & REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE) ||
+	    /* deal with increase frequency */
+	    ((val & REGULATOR_EVENT_VOLTAGE_CHANGE) &&
+	     cci_devdata->volt_increasing == 1)) {
+		cci_devdata->buck_volt = (unsigned long)data;
+		mutex_lock(&cci_devdata->devfreq->lock);
+		update_devfreq(cci_devdata->devfreq);
+		mutex_unlock(&cci_devdata->devfreq->lock);
+	}
+
+	return 0;
+}
+
+static int mtk_cci_devfreq_probe(struct platform_device *pdev)
+{
+	struct device *cci_dev = &pdev->dev;
+	struct cci_devfreq_data *cci_devdata;
+	struct notifier_block *nb;
+	int ret;
+
+	dev_pm_opp_of_add_table(&pdev->dev);
+
+	cci_devdata = devm_kzalloc(cci_dev, sizeof(*cci_devdata), GFP_KERNEL);
+	if (!cci_devdata)
+		return -ENOMEM;
+	nb = &cci_devdata->nb;
+	cci_devdata->cci_clk = ERR_PTR(-ENODEV);
+	cci_devdata->proc_reg = ERR_PTR(-ENODEV);
+
+	cci_devdata->cci_clk = clk_get(cci_dev, "cci_clock");
+	ret = PTR_ERR_OR_ZERO(cci_devdata->cci_clk);
+	if (ret) {
+		if (ret == -EPROBE_DEFER)
+			pr_err("cci clock not ready, retry\n");
+		else
+			pr_err("no clock for cci: %d\n", ret);
+
+		goto out_free_resources;
+	}
+
+	cci_devdata->proc_reg = regulator_get_optional(cci_dev, "proc");
+	ret = PTR_ERR_OR_ZERO(cci_devdata->proc_reg);
+	if (ret) {
+		if (ret == -EPROBE_DEFER)
+			pr_err("cci regulator not ready, retry\n");
+		else
+			pr_err("no regulator for cci: %d\n", ret);
+
+		goto out_free_resources;
+	}
+
+	platform_set_drvdata(pdev, cci_devdata);
+
+	/* create cci_devfreq and add to cci device.
+	 * governor is voltage_monitor.
+	 * governor will get platform_device data to make decision.
+	 */
+	cci_devdata->devfreq = devm_devfreq_add_device(cci_dev,
+						       &cci_devfreq_profile,
+						       "voltage_monitor",
+						       NULL);
+
+	nb->notifier_call = cci_devfreq_regulator_notifier;
+	regulator_register_notifier(cci_devdata->proc_reg,
+				    nb);
+
+	return 0;
+
+out_free_resources:
+	if (!IS_ERR(cci_devdata->cci_clk))
+		clk_put(cci_devdata->cci_clk);
+	if (!IS_ERR(cci_devdata->proc_reg))
+		regulator_put(cci_devdata->proc_reg);
+
+	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 = 0;
+
+	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 = 0;
+
+	platform_driver_unregister(&cci_devfreq_driver);
+
+	ret = devfreq_remove_governor(&mtk_cci_devfreq_governor);
+	if (ret)
+		pr_err("%s: failed to remove governor: %d\n", __func__, ret);
+}
+module_exit(mtk_cci_devfreq_exit)
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Mediatek CCI devfreq driver");
+MODULE_AUTHOR("andrew-sh.cheng");
+
-- 
1.8.1.1.dirty


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

RE: [PATCH 3/3] devfreq: add mediatek cci devfreq

[MyungJoo Ham]

>From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>
>
>For big/little cpu cluster architecture,
>not only CPU frequency, but CCI frequency will also affect performance.
>
>Little cores and cci share the same buck in Mediatek mt8183 IC,
>so we add a CCI devfreq which will get notification when buck voltage
>is changed, then CCI devfreq can set cci frequency as high as possible.
>
>Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>

Please correct the coding style first in the .c file.

- empty last line?
- inconsistent indentation (line26? match the first lines for all variable declarations OR don't match them at all)
- Need proper boilerplate and try to comply with C89.

Cheers,
MyungJoo


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

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

[Viresh Kumar]

On 29-01-19, 14:35, Andrew-sh Cheng wrote:
> From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>
> 
> 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.
> 
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> ---
>  drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
>  drivers/cpufreq/mediatek-cpufreq.c   | 7 ++++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
> index b1c5468..5a1c588 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", },
>  
> diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
> index eb8920d..e956248 100644
> --- a/drivers/cpufreq/mediatek-cpufreq.c
> +++ b/drivers/cpufreq/mediatek-cpufreq.c
> @@ -355,7 +355,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",
> @@ -385,6 +385,9 @@ 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);
> +	if (ret)
> +		goto out_free_resources;

Add a blank line here please.

Also out_free_resources isn't enough here and you need to free OPP table as
well.


>  	/* Search a safe voltage for intermediate frequency. */
>  	rate = clk_get_rate(inter_clk);
>  	opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate);
> @@ -412,6 +415,7 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
>  
>  out_free_opp_table:
>  	dev_pm_opp_of_cpumask_remove_table(&info->cpus);
> +	clk_disable_unprepare(inter_clk);

Clock was enabled after adding the table, and so it must be disabled before
removing the table. Just the opposite sequence.

>  
>  out_free_resources:
>  	if (!IS_ERR(proc_reg))
> @@ -551,6 +555,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

-- 
viresh

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

Re: [PATCH 3/3] devfreq: add mediatek cci devfreq

[Nicolas Boichat]

On Tue, Jan 29, 2019 at 1:36 PM Andrew-sh Cheng
<andrew-sh.cheng@mediatek.com> wrote:
>
> From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>
>
> For big/little cpu cluster architecture,
> not only CPU frequency, but CCI frequency will also affect performance.
>
> Little cores and cci share the same buck in Mediatek mt8183 IC,
> so we add a CCI devfreq which will get notification when buck voltage
> is changed, then CCI devfreq can set cci frequency as high as possible.
>
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> ---
>  drivers/devfreq/Kconfig              |   9 ++
>  drivers/devfreq/Makefile             |   1 +
>  drivers/devfreq/mt8183-cci-devfreq.c | 224 +++++++++++++++++++++++++++++++++++
>  3 files changed, 234 insertions(+)
>  create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
>
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 6a172d3..0b14aab 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -113,6 +113,15 @@ config ARM_RK3399_DMC_DEVFREQ
>            It sets the frequency for the memory controller and reads the usage counts
>            from hardware.
>
> +config ARM_MT8183_CCI_DEVFREQ
> +       tristate "MT8183 CCI DEVFREQ Driver"
> +       depends on ARM_MEDIATEK_CPUFREQ
> +       help
> +               This adds devfreq for cci clock

It might be nice to spell out that CCI stands for "Cache Coherent Interconnect".

> +               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.
> +
>  source "drivers/devfreq/event/Kconfig"
>
>  endif # PM_DEVFREQ
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 32b8d4d..25afe8c 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)     += governor_passive.o
>  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)   += exynos-bus.o
>  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)   += rk3399_dmc.o
>  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)                += tegra-devfreq.o
> +obj-$(CONFIG_ARM_MT8183_CCI_DEVFREQ)   += mt8183-cci-devfreq.o
>
>  # DEVFREQ Event Drivers
>  obj-$(CONFIG_PM_DEVFREQ_EVENT)         += event/
> diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c
> new file mode 100644
> index 0000000..4837892
> --- /dev/null
> +++ b/drivers/devfreq/mt8183-cci-devfreq.c
> @@ -0,0 +1,224 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (c) 2018 MediaTek Inc.
> +#include <linux/cpu.h>
> +#include <linux/cpufreq.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_data {
> +       struct devfreq          *devfreq;
> +       struct regulator        *proc_reg;
> +       struct clk              *cci_clk;
> +       unsigned long           buck_volt;
> +       int                     volt_increasing;
> +       struct notifier_block   nb;
> +};
> +
> +static int mtk_cci_governor_get_target(struct devfreq *devfreq,
> +                                      unsigned long *freq)
> +{
> +       struct cci_devfreq_data *cci_devdata;
> +       int     i, opp_count;

No tab here: single space between int and i.

> +       struct dev_pm_opp       *opp;
> +       unsigned long   opp_rate, opp_voltage;
> +
> +       cci_devdata = dev_get_drvdata(devfreq->dev.parent);
> +
> +       /* find available frequency */
> +       opp_count = dev_pm_opp_get_opp_count(devfreq->dev.parent);
> +       for (i = 0, opp_rate = ULONG_MAX; i < opp_count; i++, opp_rate--) {
> +               opp = dev_pm_opp_find_freq_floor(devfreq->dev.parent,
> +                                                &opp_rate);

This is quite inefficient as each call to dev_pm_opp_find_freq_floor
may go through the whole OPP table (so your loop is O(n**2), but this
can be done in O(n) by simply walking through the list).

I'd just copy what dev_pm_opp_find_freq_ceil does
(https://elixir.bootlin.com/linux/v4.19/source/drivers/opp/core.c#L425).
Or maybe we need a new function in the opp core.

> +               opp_voltage = dev_pm_opp_get_voltage(opp);
> +               dev_pm_opp_put(opp);
> +
> +               if (opp_voltage <= cci_devdata->buck_volt)
> +                       break;
> +       }
> +       *freq = opp_rate;
> +
> +       return 0;
> +}
> +
> +static int mtk_cci_governor_event_handler(struct devfreq *devfreq,
> +                                         unsigned int event, void *data)
> +{
> +       return 0;
> +}
> +
> +static struct devfreq_governor mtk_cci_devfreq_governor = {
> +       .name = "voltage_monitor",
> +       .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_data *cci_devdata;
> +
> +       cci_devdata = dev_get_drvdata(dev);
> +
> +       clk_set_rate(cci_devdata->cci_clk, *freq);
> +
> +       return 0;
> +}
> +
> +static struct devfreq_dev_profile cci_devfreq_profile = {
> +       .polling_ms     = 0,
> +       .target         = mtk_cci_devfreq_target,
> +};
> +
> +static int cci_devfreq_regulator_notifier(struct notifier_block *nb,
> +                                         unsigned long val, void *data)
> +{
> +       struct cci_devfreq_data *cci_devdata =
> +               container_of(nb, struct cci_devfreq_data, nb);
> +
> +       /* deal with reduce frequency */
> +       if (val & REGULATOR_EVENT_PRE_VOLTAGE_CHANGE) {
> +               struct pre_voltage_change_data *pvc_data = data;
> +
> +               if (pvc_data->old_uV > pvc_data->min_uV) {
> +                       cci_devdata->volt_increasing = 0;
> +                       cci_devdata->buck_volt =
> +                               (unsigned long)(pvc_data->min_uV);
> +                       mutex_lock(&cci_devdata->devfreq->lock);
> +                       update_devfreq(cci_devdata->devfreq);
> +                       mutex_unlock(&cci_devdata->devfreq->lock);
> +               } else {
> +                       cci_devdata->volt_increasing = 1;
> +               }
> +       }
> +       /* deal with abort reduce frequency */
> +       if ((val & REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE) ||
> +           /* deal with increase frequency */
> +           ((val & REGULATOR_EVENT_VOLTAGE_CHANGE) &&
> +            cci_devdata->volt_increasing == 1)) {
> +               cci_devdata->buck_volt = (unsigned long)data;
> +               mutex_lock(&cci_devdata->devfreq->lock);
> +               update_devfreq(cci_devdata->devfreq);
> +               mutex_unlock(&cci_devdata->devfreq->lock);
> +       }
> +
> +       return 0;
> +}
> +
> +static int mtk_cci_devfreq_probe(struct platform_device *pdev)
> +{
> +       struct device *cci_dev = &pdev->dev;
> +       struct cci_devfreq_data *cci_devdata;
> +       struct notifier_block *nb;
> +       int ret;
> +
> +       dev_pm_opp_of_add_table(&pdev->dev);
> +
> +       cci_devdata = devm_kzalloc(cci_dev, sizeof(*cci_devdata), GFP_KERNEL);
> +       if (!cci_devdata)
> +               return -ENOMEM;
> +       nb = &cci_devdata->nb;
> +       cci_devdata->cci_clk = ERR_PTR(-ENODEV);

This one is useless as you set cci_clk again just below.

> +       cci_devdata->proc_reg = ERR_PTR(-ENODEV);

For this one, I'd just create 2 goto targets below, instead of those
ifs in the error path.

> +
> +       cci_devdata->cci_clk = clk_get(cci_dev, "cci_clock");
> +       ret = PTR_ERR_OR_ZERO(cci_devdata->cci_clk);
> +       if (ret) {
> +               if (ret == -EPROBE_DEFER)
> +                       pr_err("cci clock not ready, retry\n");
> +               else
> +                       pr_err("no clock for cci: %d\n", ret);
> +
> +               goto out_free_resources;
> +       }
> +
> +       cci_devdata->proc_reg = regulator_get_optional(cci_dev, "proc");
> +       ret = PTR_ERR_OR_ZERO(cci_devdata->proc_reg);
> +       if (ret) {
> +               if (ret == -EPROBE_DEFER)
> +                       pr_err("cci regulator not ready, retry\n");
> +               else
> +                       pr_err("no regulator for cci: %d\n", ret);
> +
> +               goto out_free_resources;
> +       }
> +
> +       platform_set_drvdata(pdev, cci_devdata);
> +
> +       /* create cci_devfreq and add to cci device.
> +        * governor is voltage_monitor.
> +        * governor will get platform_device data to make decision.
> +        */
> +       cci_devdata->devfreq = devm_devfreq_add_device(cci_dev,
> +                                                      &cci_devfreq_profile,
> +                                                      "voltage_monitor",
> +                                                      NULL);
> +
> +       nb->notifier_call = cci_devfreq_regulator_notifier;
> +       regulator_register_notifier(cci_devdata->proc_reg,
> +                                   nb);
> +
> +       return 0;
> +
> +out_free_resources:
> +       if (!IS_ERR(cci_devdata->cci_clk))
> +               clk_put(cci_devdata->cci_clk);
> +       if (!IS_ERR(cci_devdata->proc_reg))
> +               regulator_put(cci_devdata->proc_reg);
> +
> +       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 = 0;
> +
> +       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 = 0;
> +
> +       platform_driver_unregister(&cci_devfreq_driver);
> +
> +       ret = devfreq_remove_governor(&mtk_cci_devfreq_governor);
> +       if (ret)
> +               pr_err("%s: failed to remove governor: %d\n", __func__, ret);
> +}
> +module_exit(mtk_cci_devfreq_exit)
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Mediatek CCI devfreq driver");
> +MODULE_AUTHOR("andrew-sh.cheng");
> +
> --
> 1.8.1.1.dirty
>
>
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

Re: [PATCH 3/3] devfreq: add mediatek cci devfreq

[Matthias Kaehlcke]

Hi Andrew,

On Tue, Jan 29, 2019 at 02:35:04PM +0800, Andrew-sh Cheng wrote:
> From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>

the 'From' tag is only needed when you send patches not authored by
yourself. It doesn't do any harm either, but you can drop it in future
submissions.

> For big/little cpu cluster architecture,
> not only CPU frequency, but CCI frequency will also affect performance.

This doesn't provide much information, I suggest to drop it unless
others find it valuable.

> Little cores and cci share the same buck in Mediatek mt8183 IC,
> so we add a CCI devfreq which will get notification when buck voltage
> is changed, then CCI devfreq can set cci frequency as high as
> possible.

Possible alternative:

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

Feel free to ignore or to adjust.

> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> ---
>  drivers/devfreq/Kconfig              |   9 ++
>  drivers/devfreq/Makefile             |   1 +
>  drivers/devfreq/mt8183-cci-devfreq.c | 224 +++++++++++++++++++++++++++++++++++
>  3 files changed, 234 insertions(+)
>  create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 6a172d3..0b14aab 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -113,6 +113,15 @@ config ARM_RK3399_DMC_DEVFREQ
>            It sets the frequency for the memory controller and reads the usage counts
>            from hardware.
>  
> +config ARM_MT8183_CCI_DEVFREQ
> +	tristate "MT8183 CCI DEVFREQ Driver"
> +	depends on ARM_MEDIATEK_CPUFREQ
> +	help
> +		This adds devfreq for cci clock

"This adds a devfreq driver for the Cache Coherent Interconnect (CCI)
of the 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.

not sure how important this info is here, sounds more like an
implementation detail to me.

> +
>  source "drivers/devfreq/event/Kconfig"
>  
>  endif # PM_DEVFREQ
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 32b8d4d..25afe8c 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
>  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
>  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
>  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
> +obj-$(CONFIG_ARM_MT8183_CCI_DEVFREQ)	+= mt8183-cci-devfreq.o
>  
>  # DEVFREQ Event Drivers
>  obj-$(CONFIG_PM_DEVFREQ_EVENT)		+= event/
> diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c
> new file mode 100644
> index 0000000..4837892
> --- /dev/null
> +++ b/drivers/devfreq/mt8183-cci-devfreq.c
> @@ -0,0 +1,224 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (c) 2018 MediaTek Inc.

2019 :)

> +#include <linux/cpu.h>
> +#include <linux/cpufreq.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_data {

nit: consider dropping the '_data' suffix, it doesn't add much/any
value.

> +	struct devfreq		*devfreq;
> +	struct regulator	*proc_reg;
> +	struct clk		*cci_clk;
> +	unsigned long		buck_volt;

proc_reg_uV?

place after proc_reg

> +	int			volt_increasing;

should be bool

actually, is it really needed? See comment in cci_devfreq_regulator_notifier()

> +	struct notifier_block	nb;
> +};
> +
> +static int mtk_cci_governor_get_target(struct devfreq *devfreq,
> +				       unsigned long *freq)
> +{
> +	struct cci_devfreq_data *cci_devdata;

nit: consider calling it 'cci_df' (here and elsewhere). In the context
the abbreviation is clear and there is less clutter when accessing
fields in the structure.

> +	int	i, opp_count;
> +	struct dev_pm_opp	*opp;
> +	unsigned long	opp_rate, opp_voltage;
> +
> +	cci_devdata = dev_get_drvdata(devfreq->dev.parent);
> +
> +	/* find available frequency */
> +	opp_count = dev_pm_opp_get_opp_count(devfreq->dev.parent);
> +	for (i = 0, opp_rate = ULONG_MAX; i < opp_count; i++, opp_rate--) {
> +		opp = dev_pm_opp_find_freq_floor(devfreq->dev.parent,
> +						 &opp_rate);
> +		opp_voltage = dev_pm_opp_get_voltage(opp);
> +		dev_pm_opp_put(opp);
> +
> +		if (opp_voltage <= cci_devdata->buck_volt)
> +			break;
> +	}
> +	*freq = opp_rate;
> +
> +	return 0;
> +}
> +
> +static int mtk_cci_governor_event_handler(struct devfreq *devfreq,
> +					  unsigned int event, void *data)
> +{
> +	return 0;
> +}
> +
> +static struct devfreq_governor mtk_cci_devfreq_governor = {
> +	.name = "voltage_monitor",

"mtk_cci_vmon" or similar?

> +	.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_data *cci_devdata;
> +
> +	cci_devdata = dev_get_drvdata(dev);
> +
> +	clk_set_rate(cci_devdata->cci_clk, *freq);
> +
> +	return 0;
> +}
> +
> +static struct devfreq_dev_profile cci_devfreq_profile = {
> +	.polling_ms	= 0,
> +	.target		= mtk_cci_devfreq_target,
> +};
> +
> +static int cci_devfreq_regulator_notifier(struct notifier_block *nb,
> +					  unsigned long val, void *data)
> +{
> +	struct cci_devfreq_data *cci_devdata =
> +		container_of(nb, struct cci_devfreq_data, nb);
> +
> +	/* deal with reduce frequency */
> +	if (val & REGULATOR_EVENT_PRE_VOLTAGE_CHANGE) {
> +		struct pre_voltage_change_data *pvc_data = data;
> +
> +		if (pvc_data->old_uV > pvc_data->min_uV) {

I would suggest to invert the condition. For humans it is easier to
parse "is the new voltage smaller than the previous one", than
viceversa.

> +			cci_devdata->volt_increasing = 0;
> +			cci_devdata->buck_volt =
> +				(unsigned long)(pvc_data->min_uV);
> +			mutex_lock(&cci_devdata->devfreq->lock);
> +			update_devfreq(cci_devdata->devfreq);
> +			mutex_unlock(&cci_devdata->devfreq->lock);
> +		} else {
> +			cci_devdata->volt_increasing = 1;
> +		}
> +	}

nit: add empty line

> +	/* deal with abort reduce frequency */

Shouldn't this be "abort voltage change?". If so I suggest to drop the
comment, REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE in the condition makes
it clear.

> +	if ((val & REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE) ||
> +	    /* deal with increase frequency */
> +	    ((val & REGULATOR_EVENT_VOLTAGE_CHANGE) &&
> +	     cci_devdata->volt_increasing == 1)) {

to check for a voltage increase you could use 'cci_devdata->buck_volt
< (unsigned long)data' instead of ->volt_increasing.

> +		cci_devdata->buck_volt = (unsigned long)data;
> +		mutex_lock(&cci_devdata->devfreq->lock);
> +		update_devfreq(cci_devdata->devfreq);
> +		mutex_unlock(&cci_devdata->devfreq->lock);
> +	}
> +
> +	return 0;
> +}
> +
> +static int mtk_cci_devfreq_probe(struct platform_device *pdev)
> +{
> +	struct device *cci_dev = &pdev->dev;
> +	struct cci_devfreq_data *cci_devdata;
> +	struct notifier_block *nb;
> +	int ret;
> +
> +	dev_pm_opp_of_add_table(&pdev->dev);
> +
> +	cci_devdata = devm_kzalloc(cci_dev, sizeof(*cci_devdata), GFP_KERNEL);
> +	if (!cci_devdata)
> +		return -ENOMEM;
> +	nb = &cci_devdata->nb;
> +	cci_devdata->cci_clk = ERR_PTR(-ENODEV);

initialization not needed, the field is assigned a few lines below.

> +	cci_devdata->proc_reg = ERR_PTR(-ENODEV);

also not really needed with the right gotos in place

> +
> +	cci_devdata->cci_clk = clk_get(cci_dev, "cci_clock");
> +	ret = PTR_ERR_OR_ZERO(cci_devdata->cci_clk);
> +	if (ret) {
> +		if (ret == -EPROBE_DEFER)
> +			pr_err("cci clock not ready, retry\n");
> +		else
> +			pr_err("no clock for cci: %d\n", ret);
> +
> +		goto out_free_resources;

nothing to be freed here, just return ret.

> +	}
> +
> +	cci_devdata->proc_reg = regulator_get_optional(cci_dev, "proc");
> +	ret = PTR_ERR_OR_ZERO(cci_devdata->proc_reg);
> +	if (ret) {
> +		if (ret == -EPROBE_DEFER)
> +			pr_err("cci regulator not ready, retry\n");
> +		else
> +			pr_err("no regulator for cci: %d\n", ret);
> +
> +		goto out_free_resources;
> +	}
> +
> +	platform_set_drvdata(pdev, cci_devdata);
> +
> +	/* create cci_devfreq and add to cci device.
> +	 * governor is voltage_monitor.
> +	 * governor will get platform_device data to make decision.
> +	 */
> +	cci_devdata->devfreq = devm_devfreq_add_device(cci_dev,
> +						       &cci_devfreq_profile,
> +						       "voltage_monitor",

adjust name in case you follow my suggestion above.

check cci_devdata->devfreq for errors.

> +						       NULL);
> +
> +	nb->notifier_call = cci_devfreq_regulator_notifier;
> +	regulator_register_notifier(cci_devdata->proc_reg,
> +				    nb);
> +
> +	return 0;
> +
> +out_free_resources:
> +	if (!IS_ERR(cci_devdata->cci_clk))
> +		clk_put(cci_devdata->cci_clk);
> +	if (!IS_ERR(cci_devdata->proc_reg))
> +		regulator_put(cci_devdata->proc_reg);

use individual labels and get rid of the IS_ERRs.

e.g.

err_put_reg:
	regulator_put(cci_devdata->proc_reg);

err_put_clk:
	clk_put(cci_devdata->cci_clk);


and jump to the corresponding lable 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,
> +	},
> +};
> +
> +static int __init mtk_cci_devfreq_init(void)
> +{
> +	int ret = 0;

initialization not needed

> +
> +	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 = 0;

initialization not needed

> +	platform_driver_unregister(&cci_devfreq_driver);
> +
> +	ret = devfreq_remove_governor(&mtk_cci_devfreq_governor);
> +	if (ret)
> +		pr_err("%s: failed to remove governor: %d\n", __func__, ret);

typically you reverse the order of initialization, i.e. remove the
governor, then unregister the driver.

> +}
> +module_exit(mtk_cci_devfreq_exit)
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Mediatek CCI devfreq driver");
> +MODULE_AUTHOR("andrew-sh.cheng");

should be either "Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>" or
"Andrew-sh.Cheng"

Cheers

Matthias

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

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

[andrew-sh.cheng]

On Tue, 2019-01-29 at 15:43 +0530, Viresh Kumar wrote:
> On 29-01-19, 14:35, Andrew-sh Cheng wrote:
> > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>
> > 
> > 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.
> > 
> > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> > ---
> >  drivers/cpufreq/cpufreq-dt-platdev.c | 1 +
> >  drivers/cpufreq/mediatek-cpufreq.c   | 7 ++++++-
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
> > index b1c5468..5a1c588 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", },
> >  
> > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
> > index eb8920d..e956248 100644
> > --- a/drivers/cpufreq/mediatek-cpufreq.c
> > +++ b/drivers/cpufreq/mediatek-cpufreq.c
> > @@ -355,7 +355,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",
> > @@ -385,6 +385,9 @@ 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);
> > +	if (ret)
> > +		goto out_free_resources;
> 
> Add a blank line here please.
> 
> Also out_free_resources isn't enough here and you need to free OPP table as
> well.
I will modify these in next patch.
> 
> 
> >  	/* Search a safe voltage for intermediate frequency. */
> >  	rate = clk_get_rate(inter_clk);
> >  	opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate);
> > @@ -412,6 +415,7 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
> >  
> >  out_free_opp_table:
> >  	dev_pm_opp_of_cpumask_remove_table(&info->cpus);
> > +	clk_disable_unprepare(inter_clk);
> 
> Clock was enabled after adding the table, and so it must be disabled before
> removing the table. Just the opposite sequence.
I will modify this in next patch.
> 
> >  
> >  out_free_resources:
> >  	if (!IS_ERR(proc_reg))
> > @@ -551,6 +555,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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] devfreq: add mediatek cci devfreq

[andrew-sh.cheng]

On Tue, 2019-01-29 at 17:17 +0900, MyungJoo Ham wrote:
> >From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>
> >
> >For big/little cpu cluster architecture,
> >not only CPU frequency, but CCI frequency will also affect performance.
> >
> >Little cores and cci share the same buck in Mediatek mt8183 IC,
> >so we add a CCI devfreq which will get notification when buck voltage
> >is changed, then CCI devfreq can set cci frequency as high as possible.
> >
> >Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> 
> Please correct the coding style first in the .c file.
> 
> - empty last line?
I will modify this in next patch
> - inconsistent indentation (line26? match the first lines for all variable declarations OR don't match them at all)
I will modify this in next patch
> - Need proper boilerplate and try to comply with C89.
I have checked my compile is gnu89 and without build warning.
May you advice where I should modify, or script I can use to check?
> 
> Cheers,
> MyungJoo
> 



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

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

[Chanwoo Choi]

Hi Andrew-sh.Cheng,

On 19. 1. 29. 오후 3:35, Andrew-sh Cheng wrote:
> From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>
> 
> 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

You have to add more detailed what to do on this driver.
Also, please add the full alphabet for CCI.

> +
> +Required properties:
> +- compatible: should contain "mediatek,mt8183-cci" for cci devfreq

Usually, dt-binding document only specifies h/w information
and how to bind device. 'devfreq' is the word of linux-side.
It means that 'devfreq' cannot indicate the any h/w device.

You should use the h/w device information and word of CCI for writing
the dt-binding document without linux-side word like devfreq.

For example, 
- cci devfreq -> frequency scaling of CCI

> +

Remove unneeded blank line.

> +- clocks: for cci devfreq

ditto of 'devfreq' word.

> +

ditto of blank line

> +- clock-names: for cci devfreq driver to reference

ditto of 'devfreq' word.

> +

ditto of blank line

> +- operating-points-v2: for cci devfreq opp table

ditto of 'devfreq' word.

> +
> +Example:
> +	cci: cci {
> +		compatible = "mediatek,cci";
> +		clocks = <&apmixedsys CLK_APMIXED_CCIPLL>;
> +		clock-names = "cci_clock";
> +		operating-points-v2 = <&cci_opp>;
> +	};
> +
> 

This document is missing the 'regulator' property. Please add it

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

Re: [PATCH 3/3] devfreq: add mediatek cci devfreq

[Chanwoo Choi]

Hi Andrew-sh.Cheng,

On 19. 1. 29. 오후 3:35, Andrew-sh Cheng wrote:
> From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>
> 
> For big/little cpu cluster architecture,
> not only CPU frequency, but CCI frequency will also affect performance.
> 
> Little cores and cci share the same buck in Mediatek mt8183 IC,
> so we add a CCI devfreq which will get notification when buck voltage
> is changed, then CCI devfreq can set cci frequency as high as possible.
> 
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> ---
>  drivers/devfreq/Kconfig              |   9 ++
>  drivers/devfreq/Makefile             |   1 +
>  drivers/devfreq/mt8183-cci-devfreq.c | 224 +++++++++++++++++++++++++++++++++++
>  3 files changed, 234 insertions(+)
>  create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 6a172d3..0b14aab 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -113,6 +113,15 @@ config ARM_RK3399_DMC_DEVFREQ
>            It sets the frequency for the memory controller and reads the usage counts
>            from hardware.
>  
> +config ARM_MT8183_CCI_DEVFREQ
> +	tristate "MT8183 CCI DEVFREQ Driver"
> +	depends on ARM_MEDIATEK_CPUFREQ
> +	help
> +		This adds devfreq for cci clock
> +		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.
> +

Move ARM_MT8183_CCI_DEVFREQ under ARM_EXYNOS_BUS_DEVFREQ.
On later, I'll change the order between ARM_TEGRA_DEVFREQ and ARM_RK3399_DMC_DEVFREQ
alphabetically. So, you better to keep the order of name.


>  source "drivers/devfreq/event/Kconfig"
>  
>  endif # PM_DEVFREQ
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 32b8d4d..25afe8c 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
>  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
>  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
>  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
> +obj-$(CONFIG_ARM_MT8183_CCI_DEVFREQ)	+= mt8183-cci-devfreq.o

Move it under obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ).

>  
>  # DEVFREQ Event Drivers
>  obj-$(CONFIG_PM_DEVFREQ_EVENT)		+= event/
> diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c
> new file mode 100644
> index 0000000..4837892
> --- /dev/null
> +++ b/drivers/devfreq/mt8183-cci-devfreq.c
> @@ -0,0 +1,224 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (c) 2018 MediaTek Inc.

Add blank line and change year 2018 -> 2019.

> +#include <linux/cpu.h>
> +#include <linux/cpufreq.h>

This driver doesn't use any helper function of cpufreq.h/cpu.h.
It depends on devfreq/regulator framework. Please remove them.

> +#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_data {
> +	struct devfreq		*devfreq;
> +	struct regulator	*proc_reg;
> +	struct clk		*cci_clk;
> +	unsigned long		buck_volt;
> +	int			volt_increasing;
> +	struct notifier_block	nb;
> +};
> +
> +static int mtk_cci_governor_get_target(struct devfreq *devfreq,
> +				       unsigned long *freq)
> +{
> +	struct cci_devfreq_data *cci_devdata;
> +	int	i, opp_count;
> +	struct dev_pm_opp	*opp;
> +	unsigned long	opp_rate, opp_voltage;

Above three definitions use the tab for indentation. Please use space.

> +
> +	cci_devdata = dev_get_drvdata(devfreq->dev.parent);
> +
> +	/* find available frequency */
> +	opp_count = dev_pm_opp_get_opp_count(devfreq->dev.parent);
> +	for (i = 0, opp_rate = ULONG_MAX; i < opp_count; i++, opp_rate--) {
> +		opp = dev_pm_opp_find_freq_floor(devfreq->dev.parent,
> +						 &opp_rate);
> +		opp_voltage = dev_pm_opp_get_voltage(opp);
> +		dev_pm_opp_put(opp);
> +
> +		if (opp_voltage <= cci_devdata->buck_volt)
> +			break;
> +	}
> +	*freq = opp_rate;
> +
> +	return 0;
> +}
> +
> +static int mtk_cci_governor_event_handler(struct devfreq *devfreq,
> +					  unsigned int event, void *data)
> +{
> +	return 0;
> +}

The devfreq core enables the governor with using DEVFREQ_GOV_START.
But, this driver just start after proving the device driver. It break
the previous sequence of governor.

So, if you register/unregister the regulator as following,
it can keep the existing sequence for governor starting.

	case DEVFREQ_GOV_START:
	case DEVFREQ_GOV_RESUME:
		regulator_register_notifier(...);

	case DEVFREQ_GOV_STOP:
	case DEVFREQ_GOV_SUSPEND:
		regulator_unregister_notifier(...);
		break;

> +
> +static struct devfreq_governor mtk_cci_devfreq_governor = {
> +	.name = "voltage_monitor",
> +	.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_data *cci_devdata;
> +
> +	cci_devdata = dev_get_drvdata(dev);

You better to modify it as following:

	struct cci_devfreq_data *cci_devdata = dev_get_drvdata(dev);
	
	if (!cci_devdata)
		return -EINVAL;

> +
> +	clk_set_rate(cci_devdata->cci_clk, *freq);
> +
> +	return 0;
> +}
> +
> +static struct devfreq_dev_profile cci_devfreq_profile = {
> +	.polling_ms	= 0,

Dont' need to initialize it as zero. just remove this line.

> +	.target		= mtk_cci_devfreq_target,
> +};
> +
> +static int cci_devfreq_regulator_notifier(struct notifier_block *nb,
> +					  unsigned long val, void *data)
> +{
> +	struct cci_devfreq_data *cci_devdata =
> +		container_of(nb, struct cci_devfreq_data, nb);
> +
> +	/* deal with reduce frequency */
> +	if (val & REGULATOR_EVENT_PRE_VOLTAGE_CHANGE) {
> +		struct pre_voltage_change_data *pvc_data = data;
> +
> +		if (pvc_data->old_uV > pvc_data->min_uV) {
> +			cci_devdata->volt_increasing = 0;

IMO, 'volt_increasing' is not clear. Instead, cci_devdata
could have the 'previous_voltage' variable instead of volt_increasing.
And you could use 'previous_voltage' for comparision.

> +			cci_devdata->buck_volt =
> +				(unsigned long)(pvc_data->min_uV);
> +			mutex_lock(&cci_devdata->devfreq->lock);
> +			update_devfreq(cci_devdata->devfreq);
> +			mutex_unlock(&cci_devdata->devfreq->lock);
> +		} else {
> +			cci_devdata->volt_increasing = 1;
> +		}
> +	}
> +	/* deal with abort reduce frequency */
> +	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, .



> +	    /* deal with increase frequency */
> +	    ((val & REGULATOR_EVENT_VOLTAGE_CHANGE) &&
> +	     cci_devdata->volt_increasing == 1)) {
> +		cci_devdata->buck_volt = (unsigned long)data;
> +		mutex_lock(&cci_devdata->devfreq->lock);
> +		update_devfreq(cci_devdata->devfreq);
> +		mutex_unlock(&cci_devdata->devfreq->lock);
> +	}
> +
> +	return 0;
> +}
> +
> +static int mtk_cci_devfreq_probe(struct platform_device *pdev)
> +{
> +	struct device *cci_dev = &pdev->dev;
> +	struct cci_devfreq_data *cci_devdata;
> +	struct notifier_block *nb;
> +	int ret;
> +
> +	dev_pm_opp_of_add_table(&pdev->dev);

Move it after getting clock/regulator. After checking that all resourcess are available,
and then you better to get the opp table. And check the return value  of dev_pm_opp...()

> +
> +	cci_devdata = devm_kzalloc(cci_dev, sizeof(*cci_devdata), GFP_KERNEL);
> +	if (!cci_devdata)
> +		return -ENOMEM;
> +	nb = &cci_devdata->nb;
> +	cci_devdata->cci_clk = ERR_PTR(-ENODEV);

It should be handled when error happen. It is not good for readability.

> +	cci_devdata->proc_reg = ERR_PTR(-ENODEV);

ditto.

> +
> +	cci_devdata->cci_clk = clk_get(cci_dev, "cci_clock");
> +	ret = PTR_ERR_OR_ZERO(cci_devdata->cci_clk);
> +	if (ret) {
> +		if (ret == -EPROBE_DEFER)
> +			pr_err("cci clock not ready, retry\n");

You should use 'dev_err' instead of pr_err on this driver.

I think that don't show the error when EPROBE_DEFER. you better to just return.

> +		else
> +			pr_err("no clock for cci: %d\n", ret);

How about changing the log like below?
"failed to get clock for CCI"

pr_err -> dev_err

> +
> +		goto out_free_resources;
> +	}
> +
> +	cci_devdata->proc_reg = regulator_get_optional(cci_dev, "proc");
> +	ret = PTR_ERR_OR_ZERO(cci_devdata->proc_reg);
> +	if (ret) {
> +		if (ret == -EPROBE_DEFER)
> +			pr_err("cci regulator not ready, retry\n");

ditto.

> +		else
> +			pr_err("no regulator for cci: %d\n", ret);

How about changing the log like below?
"failed to get regulator for CCI"

> +
> +		goto out_free_resources;
> +	}
> +
> +	platform_set_drvdata(pdev, cci_devdata);
> +
> +	/* create cci_devfreq and add to cci device.
> +	 * governor is voltage_monitor.
> +	 * governor will get platform_device data to make decision.
> +	 */

I think that you don't need to explain the kind of governor and how to
decide the next frequency by governor. It is enough to call the devm_devfreq_add_device.

> +	cci_devdata->devfreq = devm_devfreq_add_device(cci_dev,
> +						       &cci_devfreq_profile,
> +						       "voltage_monitor",
> +						       NULL);
> +
> +	nb->notifier_call = cci_devfreq_regulator_notifier;
> +	regulator_register_notifier(cci_devdata->proc_reg,
> +				    nb);

As I commented, regulator_register_notifier() should be handled in event_handler function.

> +
> +	return 0;
> +
> +out_free_resources:
> +	if (!IS_ERR(cci_devdata->cci_clk))
> +		clk_put(cci_devdata->cci_clk);
> +	if (!IS_ERR(cci_devdata->proc_reg))
> +		regulator_put(cci_devdata->proc_reg);
> +
> +	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 = 0;
> +
> +	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 = 0;
> +
> +	platform_driver_unregister(&cci_devfreq_driver);
> +
> +	ret = devfreq_remove_governor(&mtk_cci_devfreq_governor);
> +	if (ret)
> +		pr_err("%s: failed to remove governor: %d\n", __func__, ret);
> +}
> +module_exit(mtk_cci_devfreq_exit)
> +
> +MODULE_LICENSE("GPL v2");

nitpick. better to move it under MODULE_AUTHOR("")

> +MODULE_DESCRIPTION("Mediatek CCI devfreq driver");
> +MODULE_AUTHOR("andrew-sh.cheng");

Add full name and email as following:
- MODULE_AUTHOR("Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>"


> +
> 

Remove unneeded blank line.


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

Re: [PATCH 3/3] devfreq: add mediatek cci devfreq

[andrew-sh.cheng]

On Fri, 2019-02-01 at 10:43 +0700, Nicolas Boichat wrote:
> On Tue, Jan 29, 2019 at 1:36 PM Andrew-sh Cheng
> <andrew-sh.cheng@mediatek.com> wrote:
> >
> > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>
> >
> > For big/little cpu cluster architecture,
> > not only CPU frequency, but CCI frequency will also affect performance.
> >
> > Little cores and cci share the same buck in Mediatek mt8183 IC,
> > so we add a CCI devfreq which will get notification when buck voltage
> > is changed, then CCI devfreq can set cci frequency as high as possible.
> >
> > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> > ---
> >  drivers/devfreq/Kconfig              |   9 ++
> >  drivers/devfreq/Makefile             |   1 +
> >  drivers/devfreq/mt8183-cci-devfreq.c | 224 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 234 insertions(+)
> >  create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
> >
> > diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> > index 6a172d3..0b14aab 100644
> > --- a/drivers/devfreq/Kconfig
> > +++ b/drivers/devfreq/Kconfig
> > @@ -113,6 +113,15 @@ config ARM_RK3399_DMC_DEVFREQ
> >            It sets the frequency for the memory controller and reads the usage counts
> >            from hardware.
> >
> > +config ARM_MT8183_CCI_DEVFREQ
> > +       tristate "MT8183 CCI DEVFREQ Driver"
> > +       depends on ARM_MEDIATEK_CPUFREQ
> > +       help
> > +               This adds devfreq for cci clock
> 
> It might be nice to spell out that CCI stands for "Cache Coherent Interconnect".
I will modify in next patch.
> 
> > +               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.
> > +
> >  source "drivers/devfreq/event/Kconfig"
> >
> >  endif # PM_DEVFREQ
> > diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> > index 32b8d4d..25afe8c 100644
> > --- a/drivers/devfreq/Makefile
> > +++ b/drivers/devfreq/Makefile
> > @@ -11,6 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)     += governor_passive.o
> >  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)   += exynos-bus.o
> >  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)   += rk3399_dmc.o
> >  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)                += tegra-devfreq.o
> > +obj-$(CONFIG_ARM_MT8183_CCI_DEVFREQ)   += mt8183-cci-devfreq.o
> >
> >  # DEVFREQ Event Drivers
> >  obj-$(CONFIG_PM_DEVFREQ_EVENT)         += event/
> > diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c
> > new file mode 100644
> > index 0000000..4837892
> > --- /dev/null
> > +++ b/drivers/devfreq/mt8183-cci-devfreq.c
> > @@ -0,0 +1,224 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Copyright (c) 2018 MediaTek Inc.
> > +#include <linux/cpu.h>
> > +#include <linux/cpufreq.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_data {
> > +       struct devfreq          *devfreq;
> > +       struct regulator        *proc_reg;
> > +       struct clk              *cci_clk;
> > +       unsigned long           buck_volt;
> > +       int                     volt_increasing;
> > +       struct notifier_block   nb;
> > +};
> > +
> > +static int mtk_cci_governor_get_target(struct devfreq *devfreq,
> > +                                      unsigned long *freq)
> > +{
> > +       struct cci_devfreq_data *cci_devdata;
> > +       int     i, opp_count;
> 
> No tab here: single space between int and i.
I will modify in next patch.
> 
> > +       struct dev_pm_opp       *opp;
> > +       unsigned long   opp_rate, opp_voltage;
> > +
> > +       cci_devdata = dev_get_drvdata(devfreq->dev.parent);
> > +
> > +       /* find available frequency */
> > +       opp_count = dev_pm_opp_get_opp_count(devfreq->dev.parent);
> > +       for (i = 0, opp_rate = ULONG_MAX; i < opp_count; i++, opp_rate--) {
> > +               opp = dev_pm_opp_find_freq_floor(devfreq->dev.parent,
> > +                                                &opp_rate);
> 
> This is quite inefficient as each call to dev_pm_opp_find_freq_floor
> may go through the whole OPP table (so your loop is O(n**2), but this
> can be done in O(n) by simply walking through the list).
> 
> I'd just copy what dev_pm_opp_find_freq_ceil does
> (https://elixir.bootlin.com/linux/v4.19/source/drivers/opp/core.c#L425).
> Or maybe we need a new function in the opp core.
I will add an API for opp framework for this in next patch
> 
> > +               opp_voltage = dev_pm_opp_get_voltage(opp);
> > +               dev_pm_opp_put(opp);
> > +
> > +               if (opp_voltage <= cci_devdata->buck_volt)
> > +                       break;
> > +       }
> > +       *freq = opp_rate;
> > +
> > +       return 0;
> > +}
> > +
> > +static int mtk_cci_governor_event_handler(struct devfreq *devfreq,
> > +                                         unsigned int event, void *data)
> > +{
> > +       return 0;
> > +}
> > +
> > +static struct devfreq_governor mtk_cci_devfreq_governor = {
> > +       .name = "voltage_monitor",
> > +       .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_data *cci_devdata;
> > +
> > +       cci_devdata = dev_get_drvdata(dev);
> > +
> > +       clk_set_rate(cci_devdata->cci_clk, *freq);
> > +
> > +       return 0;
> > +}
> > +
> > +static struct devfreq_dev_profile cci_devfreq_profile = {
> > +       .polling_ms     = 0,
> > +       .target         = mtk_cci_devfreq_target,
> > +};
> > +
> > +static int cci_devfreq_regulator_notifier(struct notifier_block *nb,
> > +                                         unsigned long val, void *data)
> > +{
> > +       struct cci_devfreq_data *cci_devdata =
> > +               container_of(nb, struct cci_devfreq_data, nb);
> > +
> > +       /* deal with reduce frequency */
> > +       if (val & REGULATOR_EVENT_PRE_VOLTAGE_CHANGE) {
> > +               struct pre_voltage_change_data *pvc_data = data;
> > +
> > +               if (pvc_data->old_uV > pvc_data->min_uV) {
> > +                       cci_devdata->volt_increasing = 0;
> > +                       cci_devdata->buck_volt =
> > +                               (unsigned long)(pvc_data->min_uV);
> > +                       mutex_lock(&cci_devdata->devfreq->lock);
> > +                       update_devfreq(cci_devdata->devfreq);
> > +                       mutex_unlock(&cci_devdata->devfreq->lock);
> > +               } else {
> > +                       cci_devdata->volt_increasing = 1;
> > +               }
> > +       }
> > +       /* deal with abort reduce frequency */
> > +       if ((val & REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE) ||
> > +           /* deal with increase frequency */
> > +           ((val & REGULATOR_EVENT_VOLTAGE_CHANGE) &&
> > +            cci_devdata->volt_increasing == 1)) {
> > +               cci_devdata->buck_volt = (unsigned long)data;
> > +               mutex_lock(&cci_devdata->devfreq->lock);
> > +               update_devfreq(cci_devdata->devfreq);
> > +               mutex_unlock(&cci_devdata->devfreq->lock);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int mtk_cci_devfreq_probe(struct platform_device *pdev)
> > +{
> > +       struct device *cci_dev = &pdev->dev;
> > +       struct cci_devfreq_data *cci_devdata;
> > +       struct notifier_block *nb;
> > +       int ret;
> > +
> > +       dev_pm_opp_of_add_table(&pdev->dev);
> > +
> > +       cci_devdata = devm_kzalloc(cci_dev, sizeof(*cci_devdata), GFP_KERNEL);
> > +       if (!cci_devdata)
> > +               return -ENOMEM;
> > +       nb = &cci_devdata->nb;
> > +       cci_devdata->cci_clk = ERR_PTR(-ENODEV);
> 
> This one is useless as you set cci_clk again just below.
> 
> > +       cci_devdata->proc_reg = ERR_PTR(-ENODEV);
> 
> For this one, I'd just create 2 goto targets below, instead of those
> ifs in the error path.
I will modify and change to 2 goto in next patch.
> 
> > +
> > +       cci_devdata->cci_clk = clk_get(cci_dev, "cci_clock");
> > +       ret = PTR_ERR_OR_ZERO(cci_devdata->cci_clk);
> > +       if (ret) {
> > +               if (ret == -EPROBE_DEFER)
> > +                       pr_err("cci clock not ready, retry\n");
> > +               else
> > +                       pr_err("no clock for cci: %d\n", ret);
> > +
> > +               goto out_free_resources;
> > +       }
> > +
> > +       cci_devdata->proc_reg = regulator_get_optional(cci_dev, "proc");
> > +       ret = PTR_ERR_OR_ZERO(cci_devdata->proc_reg);
> > +       if (ret) {
> > +               if (ret == -EPROBE_DEFER)
> > +                       pr_err("cci regulator not ready, retry\n");
> > +               else
> > +                       pr_err("no regulator for cci: %d\n", ret);
> > +
> > +               goto out_free_resources;
> > +       }
> > +
> > +       platform_set_drvdata(pdev, cci_devdata);
> > +
> > +       /* create cci_devfreq and add to cci device.
> > +        * governor is voltage_monitor.
> > +        * governor will get platform_device data to make decision.
> > +        */
> > +       cci_devdata->devfreq = devm_devfreq_add_device(cci_dev,
> > +                                                      &cci_devfreq_profile,
> > +                                                      "voltage_monitor",
> > +                                                      NULL);
> > +
> > +       nb->notifier_call = cci_devfreq_regulator_notifier;
> > +       regulator_register_notifier(cci_devdata->proc_reg,
> > +                                   nb);
> > +
> > +       return 0;
> > +
> > +out_free_resources:
> > +       if (!IS_ERR(cci_devdata->cci_clk))
> > +               clk_put(cci_devdata->cci_clk);
> > +       if (!IS_ERR(cci_devdata->proc_reg))
> > +               regulator_put(cci_devdata->proc_reg);
> > +
> > +       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 = 0;
> > +
> > +       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 = 0;
> > +
> > +       platform_driver_unregister(&cci_devfreq_driver);
> > +
> > +       ret = devfreq_remove_governor(&mtk_cci_devfreq_governor);
> > +       if (ret)
> > +               pr_err("%s: failed to remove governor: %d\n", __func__, ret);
> > +}
> > +module_exit(mtk_cci_devfreq_exit)
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("Mediatek CCI devfreq driver");
> > +MODULE_AUTHOR("andrew-sh.cheng");
> > +
> > --
> > 1.8.1.1.dirty
> >
> >
> > _______________________________________________
> > Linux-mediatek mailing list
> > Linux-mediatek@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-mediatek
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek



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

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

[andrew-sh.cheng]

On Tue, 2019-02-12 at 10:00 +0900, Chanwoo Choi wrote:
> Hi Andrew-sh.Cheng,
> 
> On 19. 1. 29. 오후 3:35, Andrew-sh Cheng wrote:
> > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>
> > 
> > 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
> 
> You have to add more detailed what to do on this driver.
> Also, please add the full alphabet for CCI.
I will modify in next patch.
> 
> > +
> > +Required properties:
> > +- compatible: should contain "mediatek,mt8183-cci" for cci devfreq
> 
> Usually, dt-binding document only specifies h/w information
> and how to bind device. 'devfreq' is the word of linux-side.
> It means that 'devfreq' cannot indicate the any h/w device.
> 
> You should use the h/w device information and word of CCI for writing
> the dt-binding document without linux-side word like devfreq.
> 
> For example, 
> - cci devfreq -> frequency scaling of CCI
Thank you for advice.
I will modify in next patch.
> 
> > +
> 
> Remove unneeded blank line.
I will modify in next patch.
> 
> > +- clocks: for cci devfreq
> 
> ditto of 'devfreq' word.
I will modify in next patch.
> 
> > +
> 
> ditto of blank line
I will modify in next patch.
> 
> > +- clock-names: for cci devfreq driver to reference
> 
> ditto of 'devfreq' word.
I will modify in next patch.
> 
> > +
> 
> ditto of blank line
I will modify in next patch.
> 
> > +- operating-points-v2: for cci devfreq opp table
> 
> ditto of 'devfreq' word.
I will modify in next patch.
> 
> > +
> > +Example:
> > +	cci: cci {
> > +		compatible = "mediatek,cci";
> > +		clocks = <&apmixedsys CLK_APMIXED_CCIPLL>;
> > +		clock-names = "cci_clock";
> > +		operating-points-v2 = <&cci_opp>;
> > +	};
> > +
> > 
> 
> This document is missing the 'regulator' property. Please add it
I will add it in next patch
> 



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

Re: [PATCH 3/3] devfreq: add mediatek cci devfreq

[andrew-sh.cheng]

On Fri, 2019-02-01 at 12:13 -0800, Matthias Kaehlcke wrote:
> Hi Andrew,
> 
> On Tue, Jan 29, 2019 at 02:35:04PM +0800, Andrew-sh Cheng wrote:
> > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>
> 
> the 'From' tag is only needed when you send patches not authored by
> yourself. It doesn't do any harm either, but you can drop it in future
> submissions.
Got it~
> 
> > For big/little cpu cluster architecture,
> > not only CPU frequency, but CCI frequency will also affect performance.
> 
> This doesn't provide much information, I suggest to drop it unless
> others find it valuable.
I will remove it in next patch.
> 
> > Little cores and cci share the same buck in Mediatek mt8183 IC,
> > so we add a CCI devfreq which will get notification when buck voltage
> > is changed, then CCI devfreq can set cci frequency as high as
> > possible.
> 
> Possible alternative:
> 
> "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."
> 
> Feel free to ignore or to adjust.
Thank you. This description provide suitable information.
> 
> > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> > ---
> >  drivers/devfreq/Kconfig              |   9 ++
> >  drivers/devfreq/Makefile             |   1 +
> >  drivers/devfreq/mt8183-cci-devfreq.c | 224 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 234 insertions(+)
> >  create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
> > 
> > diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> > index 6a172d3..0b14aab 100644
> > --- a/drivers/devfreq/Kconfig
> > +++ b/drivers/devfreq/Kconfig
> > @@ -113,6 +113,15 @@ config ARM_RK3399_DMC_DEVFREQ
> >            It sets the frequency for the memory controller and reads the usage counts
> >            from hardware.
> >  
> > +config ARM_MT8183_CCI_DEVFREQ
> > +	tristate "MT8183 CCI DEVFREQ Driver"
> > +	depends on ARM_MEDIATEK_CPUFREQ
> > +	help
> > +		This adds devfreq for cci clock
> 
> "This adds a devfreq driver for the Cache Coherent Interconnect (CCI)
> of the Mediatek MT8183"?
Yes, I describe more detail in next patch.
> 
> > +		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.
> 
> not sure how important this info is here, sounds more like an
> implementation detail to me.
Yes, just describe the situation.
> 
> > +
> >  source "drivers/devfreq/event/Kconfig"
> >  
> >  endif # PM_DEVFREQ
> > diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> > index 32b8d4d..25afe8c 100644
> > --- a/drivers/devfreq/Makefile
> > +++ b/drivers/devfreq/Makefile
> > @@ -11,6 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
> >  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
> >  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
> >  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
> > +obj-$(CONFIG_ARM_MT8183_CCI_DEVFREQ)	+= mt8183-cci-devfreq.o
> >  
> >  # DEVFREQ Event Drivers
> >  obj-$(CONFIG_PM_DEVFREQ_EVENT)		+= event/
> > diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c
> > new file mode 100644
> > index 0000000..4837892
> > --- /dev/null
> > +++ b/drivers/devfreq/mt8183-cci-devfreq.c
> > @@ -0,0 +1,224 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Copyright (c) 2018 MediaTek Inc.
> 
> 2019 :)
Got it  :)
> 
> > +#include <linux/cpu.h>
> > +#include <linux/cpufreq.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_data {
> 
> nit: consider dropping the '_data' suffix, it doesn't add much/any
> value.
I will change to struct cci_devfreq in next patch.
> 
> > +	struct devfreq		*devfreq;
> > +	struct regulator	*proc_reg;
> > +	struct clk		*cci_clk;
> > +	unsigned long		buck_volt;
> 
> proc_reg_uV?
> 
> place after proc_reg
OK~
> 
> > +	int			volt_increasing;
> 
> should be bool
> 
> actually, is it really needed? See comment in cci_devfreq_regulator_notifier()
> 
You are right. I will remove this and use notification data to judge the
condition in next patch.

> > +	struct notifier_block	nb;
> > +};
> > +
> > +static int mtk_cci_governor_get_target(struct devfreq *devfreq,
> > +				       unsigned long *freq)
> > +{
> > +	struct cci_devfreq_data *cci_devdata;
> 
> nit: consider calling it 'cci_df' (here and elsewhere). In the context
> the abbreviation is clear and there is less clutter when accessing
> fields in the structure.
OK, I will change it in next patch. 
> 
> > +	int	i, opp_count;
> > +	struct dev_pm_opp	*opp;
> > +	unsigned long	opp_rate, opp_voltage;
> > +
> > +	cci_devdata = dev_get_drvdata(devfreq->dev.parent);
> > +
> > +	/* find available frequency */
> > +	opp_count = dev_pm_opp_get_opp_count(devfreq->dev.parent);
> > +	for (i = 0, opp_rate = ULONG_MAX; i < opp_count; i++, opp_rate--) {
> > +		opp = dev_pm_opp_find_freq_floor(devfreq->dev.parent,
> > +						 &opp_rate);
> > +		opp_voltage = dev_pm_opp_get_voltage(opp);
> > +		dev_pm_opp_put(opp);
> > +
> > +		if (opp_voltage <= cci_devdata->buck_volt)
> > +			break;
> > +	}
> > +	*freq = opp_rate;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_cci_governor_event_handler(struct devfreq *devfreq,
> > +					  unsigned int event, void *data)
> > +{
> > +	return 0;
> > +}
> > +
> > +static struct devfreq_governor mtk_cci_devfreq_governor = {
> > +	.name = "voltage_monitor",
> 
> "mtk_cci_vmon" or similar?
OK. I will change it in next patch. 

> > +	.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_data *cci_devdata;
> > +
> > +	cci_devdata = dev_get_drvdata(dev);
> > +
> > +	clk_set_rate(cci_devdata->cci_clk, *freq);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct devfreq_dev_profile cci_devfreq_profile = {
> > +	.polling_ms	= 0,
> > +	.target		= mtk_cci_devfreq_target,
> > +};
> > +
> > +static int cci_devfreq_regulator_notifier(struct notifier_block *nb,
> > +					  unsigned long val, void *data)
> > +{
> > +	struct cci_devfreq_data *cci_devdata =
> > +		container_of(nb, struct cci_devfreq_data, nb);
> > +
> > +	/* deal with reduce frequency */
> > +	if (val & REGULATOR_EVENT_PRE_VOLTAGE_CHANGE) {
> > +		struct pre_voltage_change_data *pvc_data = data;
> > +
> > +		if (pvc_data->old_uV > pvc_data->min_uV) {
> 
> I would suggest to invert the condition. For humans it is easier to
> parse "is the new voltage smaller than the previous one", than
> viceversa.
Got it.
> 
> > +			cci_devdata->volt_increasing = 0;
> > +			cci_devdata->buck_volt =
> > +				(unsigned long)(pvc_data->min_uV);
> > +			mutex_lock(&cci_devdata->devfreq->lock);
> > +			update_devfreq(cci_devdata->devfreq);
> > +			mutex_unlock(&cci_devdata->devfreq->lock);
> > +		} else {
> > +			cci_devdata->volt_increasing = 1;
> > +		}
> > +	}
> 
> nit: add empty line
I will modify in next patch.
> 
> > +	/* deal with abort reduce frequency */
> 
> Shouldn't this be "abort voltage change?". If so I suggest to drop the
> comment, REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE in the condition makes
> it clear.
OK
> 
> > +	if ((val & REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE) ||
> > +	    /* deal with increase frequency */
> > +	    ((val & REGULATOR_EVENT_VOLTAGE_CHANGE) &&
> > +	     cci_devdata->volt_increasing == 1)) {
> 
> to check for a voltage increase you could use 'cci_devdata->buck_volt
> < (unsigned long)data' instead of ->volt_increasing.
Thank you for suggestion.
I will modify in next patch.
> 
> > +		cci_devdata->buck_volt = (unsigned long)data;
> > +		mutex_lock(&cci_devdata->devfreq->lock);
> > +		update_devfreq(cci_devdata->devfreq);
> > +		mutex_unlock(&cci_devdata->devfreq->lock);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_cci_devfreq_probe(struct platform_device *pdev)
> > +{
> > +	struct device *cci_dev = &pdev->dev;
> > +	struct cci_devfreq_data *cci_devdata;
> > +	struct notifier_block *nb;
> > +	int ret;
> > +
> > +	dev_pm_opp_of_add_table(&pdev->dev);
> > +
> > +	cci_devdata = devm_kzalloc(cci_dev, sizeof(*cci_devdata), GFP_KERNEL);
> > +	if (!cci_devdata)
> > +		return -ENOMEM;
> > +	nb = &cci_devdata->nb;
> > +	cci_devdata->cci_clk = ERR_PTR(-ENODEV);
> 
> initialization not needed, the field is assigned a few lines below.
> 
> > +	cci_devdata->proc_reg = ERR_PTR(-ENODEV);
> 
> also not really needed with the right gotos in place
OK. I will remove unnecessary initialization. 
> 
> > +
> > +	cci_devdata->cci_clk = clk_get(cci_dev, "cci_clock");
> > +	ret = PTR_ERR_OR_ZERO(cci_devdata->cci_clk);
> > +	if (ret) {
> > +		if (ret == -EPROBE_DEFER)
> > +			pr_err("cci clock not ready, retry\n");
> > +		else
> > +			pr_err("no clock for cci: %d\n", ret);
> > +
> > +		goto out_free_resources;
> 
> nothing to be freed here, just return ret.
OK.
> 
> > +	}
> > +
> > +	cci_devdata->proc_reg = regulator_get_optional(cci_dev, "proc");
> > +	ret = PTR_ERR_OR_ZERO(cci_devdata->proc_reg);
> > +	if (ret) {
> > +		if (ret == -EPROBE_DEFER)
> > +			pr_err("cci regulator not ready, retry\n");
> > +		else
> > +			pr_err("no regulator for cci: %d\n", ret);
> > +
> > +		goto out_free_resources;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, cci_devdata);
> > +
> > +	/* create cci_devfreq and add to cci device.
> > +	 * governor is voltage_monitor.
> > +	 * governor will get platform_device data to make decision.
> > +	 */
> > +	cci_devdata->devfreq = devm_devfreq_add_device(cci_dev,
> > +						       &cci_devfreq_profile,
> > +						       "voltage_monitor",
> 
> adjust name in case you follow my suggestion above.
> 
> check cci_devdata->devfreq for errors.
Will add error check in next patch.
> 
> > +						       NULL);
> > +
> > +	nb->notifier_call = cci_devfreq_regulator_notifier;
> > +	regulator_register_notifier(cci_devdata->proc_reg,
> > +				    nb);
> > +
> > +	return 0;
> > +
> > +out_free_resources:
> > +	if (!IS_ERR(cci_devdata->cci_clk))
> > +		clk_put(cci_devdata->cci_clk);
> > +	if (!IS_ERR(cci_devdata->proc_reg))
> > +		regulator_put(cci_devdata->proc_reg);
> 
> use individual labels and get rid of the IS_ERRs.
> 
> e.g.
> 
> err_put_reg:
> 	regulator_put(cci_devdata->proc_reg);
> 
> err_put_clk:
> 	clk_put(cci_devdata->cci_clk);
> 
> 
> and jump to the corresponding lable above.
Got it. I will use individual label and remove the IS_ERR checking.
> 
> > +
> > +	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 = 0;
> 
> initialization not needed
OK.
> 
> > +
> > +	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 = 0;
> 
> initialization not needed
OK.
> 
> > +	platform_driver_unregister(&cci_devfreq_driver);
> > +
> > +	ret = devfreq_remove_governor(&mtk_cci_devfreq_governor);
> > +	if (ret)
> > +		pr_err("%s: failed to remove governor: %d\n", __func__, ret);
> 
> typically you reverse the order of initialization, i.e. remove the
> governor, then unregister the driver.
OK. I will change it in next patch.

> 
> > +}
> > +module_exit(mtk_cci_devfreq_exit)
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("Mediatek CCI devfreq driver");
> > +MODULE_AUTHOR("andrew-sh.cheng");
> 
> should be either "Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>" or
> "Andrew-sh.Cheng"
I will change to "Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>"  :)
> 
> Cheers
> 
> Matthias
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek



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

Re: [PATCH 3/3] devfreq: add mediatek cci devfreq

[andrew-sh.cheng]

On Tue, 2019-02-12 at 13:06 +0900, Chanwoo Choi wrote:
> Hi Andrew-sh.Cheng,
> 
> On 19. 1. 29. 오후 3:35, Andrew-sh Cheng wrote:
> > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>
> > 
> > For big/little cpu cluster architecture,
> > not only CPU frequency, but CCI frequency will also affect performance.
> > 
> > Little cores and cci share the same buck in Mediatek mt8183 IC,
> > so we add a CCI devfreq which will get notification when buck voltage
> > is changed, then CCI devfreq can set cci frequency as high as possible.
> > 
> > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> > ---
> >  drivers/devfreq/Kconfig              |   9 ++
> >  drivers/devfreq/Makefile             |   1 +
> >  drivers/devfreq/mt8183-cci-devfreq.c | 224 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 234 insertions(+)
> >  create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
> > 
> > diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> > index 6a172d3..0b14aab 100644
> > --- a/drivers/devfreq/Kconfig
> > +++ b/drivers/devfreq/Kconfig
> > @@ -113,6 +113,15 @@ config ARM_RK3399_DMC_DEVFREQ
> >            It sets the frequency for the memory controller and reads the usage counts
> >            from hardware.
> >  
> > +config ARM_MT8183_CCI_DEVFREQ
> > +	tristate "MT8183 CCI DEVFREQ Driver"
> > +	depends on ARM_MEDIATEK_CPUFREQ
> > +	help
> > +		This adds devfreq for cci clock
> > +		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.
> > +
> 
> Move ARM_MT8183_CCI_DEVFREQ under ARM_EXYNOS_BUS_DEVFREQ.
> On later, I'll change the order between ARM_TEGRA_DEVFREQ and ARM_RK3399_DMC_DEVFREQ
> alphabetically. So, you better to keep the order of name.
OK.
> 
> 
> >  source "drivers/devfreq/event/Kconfig"
> >  
> >  endif # PM_DEVFREQ
> > diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> > index 32b8d4d..25afe8c 100644
> > --- a/drivers/devfreq/Makefile
> > +++ b/drivers/devfreq/Makefile
> > @@ -11,6 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
> >  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
> >  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
> >  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
> > +obj-$(CONFIG_ARM_MT8183_CCI_DEVFREQ)	+= mt8183-cci-devfreq.o
> 
> Move it under obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ).
OK.
> 
> >  
> >  # DEVFREQ Event Drivers
> >  obj-$(CONFIG_PM_DEVFREQ_EVENT)		+= event/
> > diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c
> > new file mode 100644
> > index 0000000..4837892
> > --- /dev/null
> > +++ b/drivers/devfreq/mt8183-cci-devfreq.c
> > @@ -0,0 +1,224 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Copyright (c) 2018 MediaTek Inc.
> 
> Add blank line and change year 2018 -> 2019.
OK.
> 
> > +#include <linux/cpu.h>
> > +#include <linux/cpufreq.h>
> 
> This driver doesn't use any helper function of cpufreq.h/cpu.h.
> It depends on devfreq/regulator framework. Please remove them.
I will remove it in next patch.
> 
> > +#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_data {
> > +	struct devfreq		*devfreq;
> > +	struct regulator	*proc_reg;
> > +	struct clk		*cci_clk;
> > +	unsigned long		buck_volt;
> > +	int			volt_increasing;
> > +	struct notifier_block	nb;
> > +};
> > +
> > +static int mtk_cci_governor_get_target(struct devfreq *devfreq,
> > +				       unsigned long *freq)
> > +{
> > +	struct cci_devfreq_data *cci_devdata;
> > +	int	i, opp_count;
> > +	struct dev_pm_opp	*opp;
> > +	unsigned long	opp_rate, opp_voltage;
> 
> Above three definitions use the tab for indentation. Please use space.
OK.
> 
> > +
> > +	cci_devdata = dev_get_drvdata(devfreq->dev.parent);
> > +
> > +	/* find available frequency */
> > +	opp_count = dev_pm_opp_get_opp_count(devfreq->dev.parent);
> > +	for (i = 0, opp_rate = ULONG_MAX; i < opp_count; i++, opp_rate--) {
> > +		opp = dev_pm_opp_find_freq_floor(devfreq->dev.parent,
> > +						 &opp_rate);
> > +		opp_voltage = dev_pm_opp_get_voltage(opp);
> > +		dev_pm_opp_put(opp);
> > +
> > +		if (opp_voltage <= cci_devdata->buck_volt)
> > +			break;
> > +	}
> > +	*freq = opp_rate;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_cci_governor_event_handler(struct devfreq *devfreq,
> > +					  unsigned int event, void *data)
> > +{
> > +	return 0;
> > +}
> 
> The devfreq core enables the governor with using DEVFREQ_GOV_START.
> But, this driver just start after proving the device driver. It break
> the previous sequence of governor.
> 
> So, if you register/unregister the regulator as following,
> it can keep the existing sequence for governor starting.
> 
> 	case DEVFREQ_GOV_START:
> 	case DEVFREQ_GOV_RESUME:
> 		regulator_register_notifier(...);
> 
> 	case DEVFREQ_GOV_STOP:
> 	case DEVFREQ_GOV_SUSPEND:
> 		regulator_unregister_notifier(...);
> 		break;
I will move regulator register/unregister notifier to
governor event handler as above in next patch.
> 
> > +
> > +static struct devfreq_governor mtk_cci_devfreq_governor = {
> > +	.name = "voltage_monitor",
> > +	.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_data *cci_devdata;
> > +
> > +	cci_devdata = dev_get_drvdata(dev);
> 
> You better to modify it as following:
> 
> 	struct cci_devfreq_data *cci_devdata = dev_get_drvdata(dev);
> 	
> 	if (!cci_devdata)
> 		return -EINVAL;
I will modify it in next patch
> 
> > +
> > +	clk_set_rate(cci_devdata->cci_clk, *freq);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct devfreq_dev_profile cci_devfreq_profile = {
> > +	.polling_ms	= 0,
> 
> Dont' need to initialize it as zero. just remove this line.
OK.
> 
> > +	.target		= mtk_cci_devfreq_target,
> > +};
> > +
> > +static int cci_devfreq_regulator_notifier(struct notifier_block *nb,
> > +					  unsigned long val, void *data)
> > +{
> > +	struct cci_devfreq_data *cci_devdata =
> > +		container_of(nb, struct cci_devfreq_data, nb);
> > +
> > +	/* deal with reduce frequency */
> > +	if (val & REGULATOR_EVENT_PRE_VOLTAGE_CHANGE) {
> > +		struct pre_voltage_change_data *pvc_data = data;
> > +
> > +		if (pvc_data->old_uV > pvc_data->min_uV) {
> > +			cci_devdata->volt_increasing = 0;
> 
> IMO, 'volt_increasing' is not clear. Instead, cci_devdata
> could have the 'previous_voltage' variable instead of volt_increasing.
> And you could use 'previous_voltage' for comparision.
OK, I will remove volt_increasing in next patch.
> 
> > +			cci_devdata->buck_volt =
> > +				(unsigned long)(pvc_data->min_uV);
> > +			mutex_lock(&cci_devdata->devfreq->lock);
> > +			update_devfreq(cci_devdata->devfreq);
> > +			mutex_unlock(&cci_devdata->devfreq->lock);
> > +		} else {
> > +			cci_devdata->volt_increasing = 1;
> > +		}
> > +	}
> > +	/* deal with abort reduce frequency */
> > +	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, .
OK, I will separate it in next patch.
> 
> 
> 
> > +	    /* deal with increase frequency */
> > +	    ((val & REGULATOR_EVENT_VOLTAGE_CHANGE) &&
> > +	     cci_devdata->volt_increasing == 1)) {
> > +		cci_devdata->buck_volt = (unsigned long)data;
> > +		mutex_lock(&cci_devdata->devfreq->lock);
> > +		update_devfreq(cci_devdata->devfreq);
> > +		mutex_unlock(&cci_devdata->devfreq->lock);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_cci_devfreq_probe(struct platform_device *pdev)
> > +{
> > +	struct device *cci_dev = &pdev->dev;
> > +	struct cci_devfreq_data *cci_devdata;
> > +	struct notifier_block *nb;
> > +	int ret;
> > +
> > +	dev_pm_opp_of_add_table(&pdev->dev);
> 
> Move it after getting clock/regulator. After checking that all resourcess are available,
> and then you better to get the opp table. And check the return value  of dev_pm_opp...()
> 
OK. I will move it after checking clock/regulator are available, and add
return value checking. 
> > +
> > +	cci_devdata = devm_kzalloc(cci_dev, sizeof(*cci_devdata), GFP_KERNEL);
> > +	if (!cci_devdata)
> > +		return -ENOMEM;
> > +	nb = &cci_devdata->nb;
> > +	cci_devdata->cci_clk = ERR_PTR(-ENODEV);
> 
> It should be handled when error happen. It is not good for readability.
I will remove it in next patch.
> 
> > +	cci_devdata->proc_reg = ERR_PTR(-ENODEV);
> 
> ditto.
I will remove it in next patch.
> 
> > +
> > +	cci_devdata->cci_clk = clk_get(cci_dev, "cci_clock");
> > +	ret = PTR_ERR_OR_ZERO(cci_devdata->cci_clk);
> > +	if (ret) {
> > +		if (ret == -EPROBE_DEFER)
> > +			pr_err("cci clock not ready, retry\n");
> 
> You should use 'dev_err' instead of pr_err on this driver.
> 
> I think that don't show the error when EPROBE_DEFER. you better to just return.
I will change pr_err to dev_err, and remove the log with EPROBE_DEFER in
next patch.
> 
> > +		else
> > +			pr_err("no clock for cci: %d\n", ret);
> 
> How about changing the log like below?
> "failed to get clock for CCI"
OK.
> 
> pr_err -> dev_err
> 
> > +
> > +		goto out_free_resources;
> > +	}
> > +
> > +	cci_devdata->proc_reg = regulator_get_optional(cci_dev, "proc");
> > +	ret = PTR_ERR_OR_ZERO(cci_devdata->proc_reg);
> > +	if (ret) {
> > +		if (ret == -EPROBE_DEFER)
> > +			pr_err("cci regulator not ready, retry\n");
> 
> ditto.
> 
> > +		else
> > +			pr_err("no regulator for cci: %d\n", ret);
> 
> How about changing the log like below?
> "failed to get regulator for CCI"
OK.
> 
> > +
> > +		goto out_free_resources;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, cci_devdata);
> > +
> > +	/* create cci_devfreq and add to cci device.
> > +	 * governor is voltage_monitor.
> > +	 * governor will get platform_device data to make decision.
> > +	 */
> 
> I think that you don't need to explain the kind of governor and how to
> decide the next frequency by governor. It is enough to call the devm_devfreq_add_device.
I will remove these explanation in next patch.
> 
> > +	cci_devdata->devfreq = devm_devfreq_add_device(cci_dev,
> > +						       &cci_devfreq_profile,
> > +						       "voltage_monitor",
> > +						       NULL);
> > +
> > +	nb->notifier_call = cci_devfreq_regulator_notifier;
> > +	regulator_register_notifier(cci_devdata->proc_reg,
> > +				    nb);
> 
> As I commented, regulator_register_notifier() should be handled in event_handler function.
OK. I will move it into governor event handler
> 
> > +
> > +	return 0;
> > +
> > +out_free_resources:
> > +	if (!IS_ERR(cci_devdata->cci_clk))
> > +		clk_put(cci_devdata->cci_clk);
> > +	if (!IS_ERR(cci_devdata->proc_reg))
> > +		regulator_put(cci_devdata->proc_reg);
> > +
> > +	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 = 0;
> > +
> > +	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 = 0;
> > +
> > +	platform_driver_unregister(&cci_devfreq_driver);
> > +
> > +	ret = devfreq_remove_governor(&mtk_cci_devfreq_governor);
> > +	if (ret)
> > +		pr_err("%s: failed to remove governor: %d\n", __func__, ret);
> > +}
> > +module_exit(mtk_cci_devfreq_exit)
> > +
> > +MODULE_LICENSE("GPL v2");
> 
> nitpick. better to move it under MODULE_AUTHOR("")
I will move it after MODULE_AUTHOR :)
> 
> > +MODULE_DESCRIPTION("Mediatek CCI devfreq driver");
> > +MODULE_AUTHOR("andrew-sh.cheng");
> 
> Add full name and email as following:
> - MODULE_AUTHOR("Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>"
I will change to full name and email in next patch.
> 
> 
> > +
> > 
> 
> Remove unneeded blank line.
OK.
> 
> 



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