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

[PATCH v2 0/4] 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.

1. coding style refine
2. Move regulator_register_notifier() into mtk_cci_governor_event_handler()
3. Add dev_pm_opp_find_max_freq_by_volt() API for opp framework.


Andrew-sh.Cheng (4):
  cpufreq: mediatek: add mt8183 cpufreq support
  opp: add API which get max freq by voltage
  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                 |  12 +-
 drivers/devfreq/Kconfig                            |  10 +
 drivers/devfreq/Makefile                           |   1 +
 drivers/devfreq/mt8183-cci-devfreq.c               | 235 +++++++++++++++++++++
 drivers/opp/core.c                                 |  55 +++++
 include/linux/pm_opp.h                             |   8 +
 8 files changed, 339 insertions(+), 2 deletions(-)
 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

[PATCH v2 0/4] 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.

1. coding style refine
2. Move regulator_register_notifier() into mtk_cci_governor_event_handler()
3. Add dev_pm_opp_find_max_freq_by_volt() API for opp framework.


Andrew-sh.Cheng (4):
  cpufreq: mediatek: add mt8183 cpufreq support
  opp: add API which get max freq by voltage
  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                 |  12 +-
 drivers/devfreq/Kconfig                            |  10 +
 drivers/devfreq/Makefile                           |   1 +
 drivers/devfreq/mt8183-cci-devfreq.c               | 235 +++++++++++++++++++++
 drivers/opp/core.c                                 |  55 +++++
 include/linux/pm_opp.h                             |   8 +
 8 files changed, 339 insertions(+), 2 deletions(-)
 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 v2 1/4] cpufreq: mediatek: add mt8183 cpufreq support

[Andrew-sh.Cheng]

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-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
 drivers/cpufreq/cpufreq-dt-platdev.c |  1 +
 drivers/cpufreq/mediatek-cpufreq.c   | 12 ++++++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
index 47729a2..53ea52b 100644
--- a/drivers/cpufreq/cpufreq-dt-platdev.c
+++ b/drivers/cpufreq/cpufreq-dt-platdev.c
@@ -117,6 +117,7 @@
 	{ .compatible = "mediatek,mt817x", },
 	{ .compatible = "mediatek,mt8173", },
 	{ .compatible = "mediatek,mt8176", },
+	{ .compatible = "mediatek,mt8183", },
 
 	{ .compatible = "nvidia,tegra124", },
 	{ .compatible = "nvidia,tegra210", },
diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index 48e9829..7cd01d3 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -346,7 +346,7 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 		goto out_free_resources;
 	}
 
-	proc_reg = regulator_get_exclusive(cpu_dev, "proc");
+	proc_reg = regulator_get_optional(cpu_dev, "proc");
 	if (IS_ERR(proc_reg)) {
 		if (PTR_ERR(proc_reg) == -EPROBE_DEFER)
 			pr_warn("proc regulator for cpu%d not ready, retry.\n",
@@ -376,13 +376,17 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 		goto out_free_resources;
 	}
 
+	ret = clk_prepare_enable(inter_clk);
+	if (ret)
+		goto out_free_opp_table;
+
 	/* Search a safe voltage for intermediate frequency. */
 	rate = clk_get_rate(inter_clk);
 	opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate);
 	if (IS_ERR(opp)) {
 		pr_err("failed to get intermediate opp for cpu%d\n", cpu);
 		ret = PTR_ERR(opp);
-		goto out_free_opp_table;
+		goto out_disable_clock;
 	}
 	info->intermediate_voltage = dev_pm_opp_get_voltage(opp);
 	dev_pm_opp_put(opp);
@@ -401,6 +405,9 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 
 	return 0;
 
+out_disable_clock:
+	clk_disable_unprepare(inter_clk);
+
 out_free_opp_table:
 	dev_pm_opp_of_cpumask_remove_table(&info->cpus);
 
@@ -543,6 +550,7 @@ static int mtk_cpufreq_probe(struct platform_device *pdev)
 	{ .compatible = "mediatek,mt817x", },
 	{ .compatible = "mediatek,mt8173", },
 	{ .compatible = "mediatek,mt8176", },
+	{ .compatible = "mediatek,mt8183", },
 
 	{ }
 };
-- 
1.8.1.1.dirty

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

[Andrew-sh.Cheng]

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   | 12 ++++++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
index 47729a2..53ea52b 100644
--- a/drivers/cpufreq/cpufreq-dt-platdev.c
+++ b/drivers/cpufreq/cpufreq-dt-platdev.c
@@ -117,6 +117,7 @@
 	{ .compatible = "mediatek,mt817x", },
 	{ .compatible = "mediatek,mt8173", },
 	{ .compatible = "mediatek,mt8176", },
+	{ .compatible = "mediatek,mt8183", },
 
 	{ .compatible = "nvidia,tegra124", },
 	{ .compatible = "nvidia,tegra210", },
diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index 48e9829..7cd01d3 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -346,7 +346,7 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 		goto out_free_resources;
 	}
 
-	proc_reg = regulator_get_exclusive(cpu_dev, "proc");
+	proc_reg = regulator_get_optional(cpu_dev, "proc");
 	if (IS_ERR(proc_reg)) {
 		if (PTR_ERR(proc_reg) == -EPROBE_DEFER)
 			pr_warn("proc regulator for cpu%d not ready, retry.\n",
@@ -376,13 +376,17 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 		goto out_free_resources;
 	}
 
+	ret = clk_prepare_enable(inter_clk);
+	if (ret)
+		goto out_free_opp_table;
+
 	/* Search a safe voltage for intermediate frequency. */
 	rate = clk_get_rate(inter_clk);
 	opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate);
 	if (IS_ERR(opp)) {
 		pr_err("failed to get intermediate opp for cpu%d\n", cpu);
 		ret = PTR_ERR(opp);
-		goto out_free_opp_table;
+		goto out_disable_clock;
 	}
 	info->intermediate_voltage = dev_pm_opp_get_voltage(opp);
 	dev_pm_opp_put(opp);
@@ -401,6 +405,9 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
 
 	return 0;
 
+out_disable_clock:
+	clk_disable_unprepare(inter_clk);
+
 out_free_opp_table:
 	dev_pm_opp_of_cpumask_remove_table(&info->cpus);
 
@@ -543,6 +550,7 @@ static int mtk_cpufreq_probe(struct platform_device *pdev)
 	{ .compatible = "mediatek,mt817x", },
 	{ .compatible = "mediatek,mt8173", },
 	{ .compatible = "mediatek,mt8176", },
+	{ .compatible = "mediatek,mt8183", },
 
 	{ }
 };
-- 
1.8.1.1.dirty


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

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

[Nicolas Boichat]

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

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

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

Should you disable the clock in mtk_cpu_dvfs_info_release?

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

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

[Nicolas Boichat]

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

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

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

Should you disable the clock in mtk_cpu_dvfs_info_release?

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

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

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

[Nicolas Boichat]

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

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

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

Should you disable the clock in mtk_cpu_dvfs_info_release?

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

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

[andrew-sh.cheng]

On Sat, 2019-03-30 at 17:06 -0700, Nicolas Boichat wrote:
> On Thu, Mar 28, 2019 at 11:46 PM Andrew-sh.Cheng
> <andrew-sh.cheng@mediatek.com> wrote:
> >
> > For new mediatek chip mt8183,
> > cci and little cluster share the same buck,
> > so need to modify the attribute of regulator from exclusive to optional
> >
> > Intermediate clock is not always enabled by ccf in different projects,
> > so cpufreq should always enable it by itself.
> 
> One comment, otherwise the changes look good. However, I feel that
> this patch should be split in 3:
>  1. Change to regulator_get_optional
>  2. Enable inter_clk
>  3. Add support for 8183
Okay, I will split it into 3 patches
> > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> > ---
> >  drivers/cpufreq/cpufreq-dt-platdev.c |  1 +
> >  drivers/cpufreq/mediatek-cpufreq.c   | 12 ++++++++++--
> >  2 files changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
> > index 47729a2..53ea52b 100644
> > --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> > +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> > @@ -117,6 +117,7 @@
> >         { .compatible = "mediatek,mt817x", },
> >         { .compatible = "mediatek,mt8173", },
> >         { .compatible = "mediatek,mt8176", },
> > +       { .compatible = "mediatek,mt8183", },
> >
> >         { .compatible = "nvidia,tegra124", },
> >         { .compatible = "nvidia,tegra210", },
> > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
> > index 48e9829..7cd01d3 100644
> > --- a/drivers/cpufreq/mediatek-cpufreq.c
> > +++ b/drivers/cpufreq/mediatek-cpufreq.c
> > @@ -346,7 +346,7 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
> >                 goto out_free_resources;
> >         }
> >
> > -       proc_reg = regulator_get_exclusive(cpu_dev, "proc");
> > +       proc_reg = regulator_get_optional(cpu_dev, "proc");
> >         if (IS_ERR(proc_reg)) {
> >                 if (PTR_ERR(proc_reg) == -EPROBE_DEFER)
> >                         pr_warn("proc regulator for cpu%d not ready, retry.\n",
> > @@ -376,13 +376,17 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
> >                 goto out_free_resources;
> >         }
> >
> > +       ret = clk_prepare_enable(inter_clk);
> 
> Should you disable the clock in mtk_cpu_dvfs_info_release?
Yes, I will add it.
> 
> > +       if (ret)
> > +               goto out_free_opp_table;
> > +
> >         /* Search a safe voltage for intermediate frequency. */
> >         rate = clk_get_rate(inter_clk);
> >         opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate);
> >         if (IS_ERR(opp)) {
> >                 pr_err("failed to get intermediate opp for cpu%d\n", cpu);
> >                 ret = PTR_ERR(opp);
> > -               goto out_free_opp_table;
> > +               goto out_disable_clock;
> >         }
> >         info->intermediate_voltage = dev_pm_opp_get_voltage(opp);
> >         dev_pm_opp_put(opp);
> > @@ -401,6 +405,9 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
> >
> >         return 0;
> >
> > +out_disable_clock:
> > +       clk_disable_unprepare(inter_clk);
> > +
> >  out_free_opp_table:
> >         dev_pm_opp_of_cpumask_remove_table(&info->cpus);
> >
> > @@ -543,6 +550,7 @@ static int mtk_cpufreq_probe(struct platform_device *pdev)
> >         { .compatible = "mediatek,mt817x", },
> >         { .compatible = "mediatek,mt8173", },
> >         { .compatible = "mediatek,mt8176", },
> > +       { .compatible = "mediatek,mt8183", },
> >
> >         { }
> >  };
> > --
> > 1.8.1.1.dirty
> >
> >
> > _______________________________________________
> > Linux-mediatek mailing list
> > Linux-mediatek@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-mediatek
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[andrew-sh.cheng]

On Sat, 2019-03-30 at 17:06 -0700, Nicolas Boichat wrote:
> On Thu, Mar 28, 2019 at 11:46 PM Andrew-sh.Cheng
> <andrew-sh.cheng@mediatek.com> wrote:
> >
> > For new mediatek chip mt8183,
> > cci and little cluster share the same buck,
> > so need to modify the attribute of regulator from exclusive to optional
> >
> > Intermediate clock is not always enabled by ccf in different projects,
> > so cpufreq should always enable it by itself.
> 
> One comment, otherwise the changes look good. However, I feel that
> this patch should be split in 3:
>  1. Change to regulator_get_optional
>  2. Enable inter_clk
>  3. Add support for 8183
Okay, I will split it into 3 patches
> > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> > ---
> >  drivers/cpufreq/cpufreq-dt-platdev.c |  1 +
> >  drivers/cpufreq/mediatek-cpufreq.c   | 12 ++++++++++--
> >  2 files changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
> > index 47729a2..53ea52b 100644
> > --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> > +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> > @@ -117,6 +117,7 @@
> >         { .compatible = "mediatek,mt817x", },
> >         { .compatible = "mediatek,mt8173", },
> >         { .compatible = "mediatek,mt8176", },
> > +       { .compatible = "mediatek,mt8183", },
> >
> >         { .compatible = "nvidia,tegra124", },
> >         { .compatible = "nvidia,tegra210", },
> > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
> > index 48e9829..7cd01d3 100644
> > --- a/drivers/cpufreq/mediatek-cpufreq.c
> > +++ b/drivers/cpufreq/mediatek-cpufreq.c
> > @@ -346,7 +346,7 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
> >                 goto out_free_resources;
> >         }
> >
> > -       proc_reg = regulator_get_exclusive(cpu_dev, "proc");
> > +       proc_reg = regulator_get_optional(cpu_dev, "proc");
> >         if (IS_ERR(proc_reg)) {
> >                 if (PTR_ERR(proc_reg) == -EPROBE_DEFER)
> >                         pr_warn("proc regulator for cpu%d not ready, retry.\n",
> > @@ -376,13 +376,17 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
> >                 goto out_free_resources;
> >         }
> >
> > +       ret = clk_prepare_enable(inter_clk);
> 
> Should you disable the clock in mtk_cpu_dvfs_info_release?
Yes, I will add it.
> 
> > +       if (ret)
> > +               goto out_free_opp_table;
> > +
> >         /* Search a safe voltage for intermediate frequency. */
> >         rate = clk_get_rate(inter_clk);
> >         opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate);
> >         if (IS_ERR(opp)) {
> >                 pr_err("failed to get intermediate opp for cpu%d\n", cpu);
> >                 ret = PTR_ERR(opp);
> > -               goto out_free_opp_table;
> > +               goto out_disable_clock;
> >         }
> >         info->intermediate_voltage = dev_pm_opp_get_voltage(opp);
> >         dev_pm_opp_put(opp);
> > @@ -401,6 +405,9 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu)
> >
> >         return 0;
> >
> > +out_disable_clock:
> > +       clk_disable_unprepare(inter_clk);
> > +
> >  out_free_opp_table:
> >         dev_pm_opp_of_cpumask_remove_table(&info->cpus);
> >
> > @@ -543,6 +550,7 @@ static int mtk_cpufreq_probe(struct platform_device *pdev)
> >         { .compatible = "mediatek,mt817x", },
> >         { .compatible = "mediatek,mt8173", },
> >         { .compatible = "mediatek,mt8176", },
> > +       { .compatible = "mediatek,mt8183", },
> >
> >         { }
> >  };
> > --
> > 1.8.1.1.dirty
> >
> >
> > _______________________________________________
> > Linux-mediatek mailing list
> > Linux-mediatek@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-mediatek
> 
> _______________________________________________
> 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

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

[Andrew-sh.Cheng]

This API will get voltage as input parameter.
Search all opp items for the item which with max frequency,
and the voltae is smaller than provided voltage.

Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
 drivers/opp/core.c     | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h |  8 ++++++++
 2 files changed, 63 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 0420f7e..7323cd9 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -526,6 +526,61 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
+/**
+ * dev_pm_opp_find_max_freq_by_volt() - Search for a opp with max freq
+ * under provided voltage
+ * @dev:	device for which we do this operation
+ * @u_volt:	provided voltage
+ *
+ * Search for the matching available OPP which provide voltage can support.
+ *
+ * Return: matching *opp, else returns ERR_PTR in case of error
+ * and should be handled using IS_ERR.
+ * Error return values can be:
+ * EINVAL:	for bad pointer
+ * ERANGE:	no match found for search
+ * ENODEV:	if device not found in list of registered devices
+ *
+ * The callers are required to call dev_pm_opp_put() for the returned OPP after
+ * use.
+ */
+struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
+					      unsigned long u_volt)
+{
+	struct opp_table *opp_table;
+	struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
+
+	if (!dev || !u_volt) {
+		dev_err(dev, "%s: Invalid argument volt=%d\n", __func__,
+			u_volt);
+		return ERR_PTR(-EINVAL);
+	}
+
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table))
+		return ERR_CAST(opp_table);
+
+	mutex_lock(&opp_table->lock);
+
+	list_for_each_entry(temp_opp, &opp_table->opp_list, node) {
+		if (temp_opp->available) {
+			/* go to the next node, before choosing prev */
+			if (temp_opp->supplies[0].u_volt > u_volt)
+				break;
+			opp = temp_opp;
+		}
+	}
+
+	/* Increment the reference count of OPP */
+	if (!IS_ERR(opp))
+		dev_pm_opp_get(opp);
+	mutex_unlock(&opp_table->lock);
+	dev_pm_opp_put_opp_table(opp_table);
+
+	return opp;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_find_max_freq_by_volt);
+
 static int _set_opp_voltage(struct device *dev, struct regulator *reg,
 			    struct dev_pm_opp_supply *supply)
 {
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 24c757a..57deef9 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -102,6 +102,8 @@ struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
 
 struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 					      unsigned long *freq);
+struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
+					      unsigned long u_volt);
 
 struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
 					     unsigned long *freq);
@@ -207,6 +209,12 @@ static inline struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 	return ERR_PTR(-ENOTSUPP);
 }
 
+static inline struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
+					      unsigned long u_volt)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+
 static inline struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
 					unsigned long *freq)
 {
-- 
1.8.1.1.dirty

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

[Andrew-sh.Cheng]

This API will get voltage as input parameter.
Search all opp items for the item which with max frequency,
and the voltae is smaller than provided voltage.

Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
---
 drivers/opp/core.c     | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pm_opp.h |  8 ++++++++
 2 files changed, 63 insertions(+)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 0420f7e..7323cd9 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -526,6 +526,61 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
+/**
+ * dev_pm_opp_find_max_freq_by_volt() - Search for a opp with max freq
+ * under provided voltage
+ * @dev:	device for which we do this operation
+ * @u_volt:	provided voltage
+ *
+ * Search for the matching available OPP which provide voltage can support.
+ *
+ * Return: matching *opp, else returns ERR_PTR in case of error
+ * and should be handled using IS_ERR.
+ * Error return values can be:
+ * EINVAL:	for bad pointer
+ * ERANGE:	no match found for search
+ * ENODEV:	if device not found in list of registered devices
+ *
+ * The callers are required to call dev_pm_opp_put() for the returned OPP after
+ * use.
+ */
+struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
+					      unsigned long u_volt)
+{
+	struct opp_table *opp_table;
+	struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
+
+	if (!dev || !u_volt) {
+		dev_err(dev, "%s: Invalid argument volt=%d\n", __func__,
+			u_volt);
+		return ERR_PTR(-EINVAL);
+	}
+
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table))
+		return ERR_CAST(opp_table);
+
+	mutex_lock(&opp_table->lock);
+
+	list_for_each_entry(temp_opp, &opp_table->opp_list, node) {
+		if (temp_opp->available) {
+			/* go to the next node, before choosing prev */
+			if (temp_opp->supplies[0].u_volt > u_volt)
+				break;
+			opp = temp_opp;
+		}
+	}
+
+	/* Increment the reference count of OPP */
+	if (!IS_ERR(opp))
+		dev_pm_opp_get(opp);
+	mutex_unlock(&opp_table->lock);
+	dev_pm_opp_put_opp_table(opp_table);
+
+	return opp;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_find_max_freq_by_volt);
+
 static int _set_opp_voltage(struct device *dev, struct regulator *reg,
 			    struct dev_pm_opp_supply *supply)
 {
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 24c757a..57deef9 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -102,6 +102,8 @@ struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
 
 struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 					      unsigned long *freq);
+struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
+					      unsigned long u_volt);
 
 struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
 					     unsigned long *freq);
@@ -207,6 +209,12 @@ static inline struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 	return ERR_PTR(-ENOTSUPP);
 }
 
+static inline struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
+					      unsigned long u_volt)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+
 static inline struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
 					unsigned long *freq)
 {
-- 
1.8.1.1.dirty


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

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

[Nicolas Boichat]

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

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

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

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

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

[Nicolas Boichat]

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

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

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

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

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

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

[Nicolas Boichat]

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

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

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

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

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

[andrew-sh.cheng]

On Wed, 2019-04-03 at 12:32 +0800, Nicolas Boichat wrote:
> On Fri, Mar 29, 2019 at 2:46 PM Andrew-sh.Cheng
> <andrew-sh.cheng@mediatek.com> wrote:
> >
> > This API will get voltage as input parameter.
> > Search all opp items for the item which with max frequency,
> > and the voltae is smaller than provided voltage.
> >
> > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> > ---
> >  drivers/opp/core.c     | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/pm_opp.h |  8 ++++++++
> >  2 files changed, 63 insertions(+)
> >
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index 0420f7e..7323cd9 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -526,6 +526,61 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
> >  }
> >  EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
> >
> > +/**
> > + * dev_pm_opp_find_max_freq_by_volt() - Search for a opp with max freq
> > + * under provided voltage
> > + * @dev:       device for which we do this operation
> > + * @u_volt:    provided voltage
> > + *
> > + * Search for the matching available OPP which provide voltage can support.
> > + *
> > + * Return: matching *opp, else returns ERR_PTR in case of error
> > + * and should be handled using IS_ERR.
> > + * Error return values can be:
> > + * EINVAL:     for bad pointer
> > + * ERANGE:     no match found for search
> > + * ENODEV:     if device not found in list of registered devices
> > + *
> > + * The callers are required to call dev_pm_opp_put() for the returned OPP after
> > + * use.
> > + */
> > +struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
> > +                                             unsigned long u_volt)
> > +{
> > +       struct opp_table *opp_table;
> > +       struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
> > +
> > +       if (!dev || !u_volt) {
> > +               dev_err(dev, "%s: Invalid argument volt=%d\n", __func__,
> > +                       u_volt);
> 
> u_volt is an unsigned long, so you should use %lu.
> 
> drivers/opp/core.c:582:3: error: format '%d' expects argument of type
> 'int', but argument 4 has type 'long unsigned int' [-Werror=format=]
>   chromeos-kernel-4_19-4.19.32-r271:    dev_err(dev, "%s: Invalid
> argument volt=%d\n", __func__,
>   chromeos-kernel-4_19-4.19.32-r271:    ^
> 
Got it. I will fix this on next patch
> > +               return ERR_PTR(-EINVAL);
> > +       }
> > +
> > +       opp_table = _find_opp_table(dev);
> > +       if (IS_ERR(opp_table))
> > +               return ERR_CAST(opp_table);
> > +
> > +       mutex_lock(&opp_table->lock);
> > +
> > +       list_for_each_entry(temp_opp, &opp_table->opp_list, node) {
> > +               if (temp_opp->available) {
> > +                       /* go to the next node, before choosing prev */
> > +                       if (temp_opp->supplies[0].u_volt > u_volt)
> > +                               break;
> > +                       opp = temp_opp;
> > +               }
> > +       }
> > +
> > +       /* Increment the reference count of OPP */
> > +       if (!IS_ERR(opp))
> > +               dev_pm_opp_get(opp);
> > +       mutex_unlock(&opp_table->lock);
> > +       dev_pm_opp_put_opp_table(opp_table);
> > +
> > +       return opp;
> > +}
> > +EXPORT_SYMBOL_GPL(dev_pm_opp_find_max_freq_by_volt);
> > +
> >  static int _set_opp_voltage(struct device *dev, struct regulator *reg,
> >                             struct dev_pm_opp_supply *supply)
> >  {
> > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> > index 24c757a..57deef9 100644
> > --- a/include/linux/pm_opp.h
> > +++ b/include/linux/pm_opp.h
> > @@ -102,6 +102,8 @@ struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
> >
> >  struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
> >                                               unsigned long *freq);
> > +struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
> > +                                             unsigned long u_volt);
> >
> >  struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
> >                                              unsigned long *freq);
> > @@ -207,6 +209,12 @@ static inline struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
> >         return ERR_PTR(-ENOTSUPP);
> >  }
> >
> > +static inline struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
> > +                                             unsigned long u_volt)
> > +{
> > +       return ERR_PTR(-ENOTSUPP);
> > +}
> > +
> >  static inline struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
> >                                         unsigned long *freq)
> >  {
> > --
> > 1.8.1.1.dirty
> >
> >
> > _______________________________________________
> > Linux-mediatek mailing list
> > Linux-mediatek@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[andrew-sh.cheng]

On Wed, 2019-04-03 at 12:32 +0800, Nicolas Boichat wrote:
> On Fri, Mar 29, 2019 at 2:46 PM Andrew-sh.Cheng
> <andrew-sh.cheng@mediatek.com> wrote:
> >
> > This API will get voltage as input parameter.
> > Search all opp items for the item which with max frequency,
> > and the voltae is smaller than provided voltage.
> >
> > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> > ---
> >  drivers/opp/core.c     | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/pm_opp.h |  8 ++++++++
> >  2 files changed, 63 insertions(+)
> >
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index 0420f7e..7323cd9 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -526,6 +526,61 @@ struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
> >  }
> >  EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
> >
> > +/**
> > + * dev_pm_opp_find_max_freq_by_volt() - Search for a opp with max freq
> > + * under provided voltage
> > + * @dev:       device for which we do this operation
> > + * @u_volt:    provided voltage
> > + *
> > + * Search for the matching available OPP which provide voltage can support.
> > + *
> > + * Return: matching *opp, else returns ERR_PTR in case of error
> > + * and should be handled using IS_ERR.
> > + * Error return values can be:
> > + * EINVAL:     for bad pointer
> > + * ERANGE:     no match found for search
> > + * ENODEV:     if device not found in list of registered devices
> > + *
> > + * The callers are required to call dev_pm_opp_put() for the returned OPP after
> > + * use.
> > + */
> > +struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
> > +                                             unsigned long u_volt)
> > +{
> > +       struct opp_table *opp_table;
> > +       struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
> > +
> > +       if (!dev || !u_volt) {
> > +               dev_err(dev, "%s: Invalid argument volt=%d\n", __func__,
> > +                       u_volt);
> 
> u_volt is an unsigned long, so you should use %lu.
> 
> drivers/opp/core.c:582:3: error: format '%d' expects argument of type
> 'int', but argument 4 has type 'long unsigned int' [-Werror=format=]
>   chromeos-kernel-4_19-4.19.32-r271:    dev_err(dev, "%s: Invalid
> argument volt=%d\n", __func__,
>   chromeos-kernel-4_19-4.19.32-r271:    ^
> 
Got it. I will fix this on next patch
> > +               return ERR_PTR(-EINVAL);
> > +       }
> > +
> > +       opp_table = _find_opp_table(dev);
> > +       if (IS_ERR(opp_table))
> > +               return ERR_CAST(opp_table);
> > +
> > +       mutex_lock(&opp_table->lock);
> > +
> > +       list_for_each_entry(temp_opp, &opp_table->opp_list, node) {
> > +               if (temp_opp->available) {
> > +                       /* go to the next node, before choosing prev */
> > +                       if (temp_opp->supplies[0].u_volt > u_volt)
> > +                               break;
> > +                       opp = temp_opp;
> > +               }
> > +       }
> > +
> > +       /* Increment the reference count of OPP */
> > +       if (!IS_ERR(opp))
> > +               dev_pm_opp_get(opp);
> > +       mutex_unlock(&opp_table->lock);
> > +       dev_pm_opp_put_opp_table(opp_table);
> > +
> > +       return opp;
> > +}
> > +EXPORT_SYMBOL_GPL(dev_pm_opp_find_max_freq_by_volt);
> > +
> >  static int _set_opp_voltage(struct device *dev, struct regulator *reg,
> >                             struct dev_pm_opp_supply *supply)
> >  {
> > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> > index 24c757a..57deef9 100644
> > --- a/include/linux/pm_opp.h
> > +++ b/include/linux/pm_opp.h
> > @@ -102,6 +102,8 @@ struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
> >
> >  struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
> >                                               unsigned long *freq);
> > +struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
> > +                                             unsigned long u_volt);
> >
> >  struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
> >                                              unsigned long *freq);
> > @@ -207,6 +209,12 @@ static inline struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
> >         return ERR_PTR(-ENOTSUPP);
> >  }
> >
> > +static inline struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
> > +                                             unsigned long u_volt)
> > +{
> > +       return ERR_PTR(-ENOTSUPP);
> > +}
> > +
> >  static inline struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
> >                                         unsigned long *freq)
> >  {
> > --
> > 1.8.1.1.dirty
> >
> >
> > _______________________________________________
> > Linux-mediatek mailing list
> > Linux-mediatek@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-mediatek



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

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

[Viresh Kumar]

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

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

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

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

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

[Viresh Kumar]

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

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

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

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

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

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

[Viresh Kumar]

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

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

I will remove the API now.

--
Viresh

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

[Viresh Kumar]

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

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

I will remove the API now.

--
Viresh

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

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

[Viresh Kumar]

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

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

I will remove the API now.

--
Viresh

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[Andrew-sh.Cheng]

This adds a devfreq driver for the Cache Coherent Interconnect (CCI)
of the Mediatek MT8183.

On the MT8183 the CCI is supplied by the same regulator as the LITTLE
cores. The driver is notified when the regulator voltage changes
(driven by cpufreq) and adjusts the CCI frequency to the maximum
possible value.

Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
 drivers/devfreq/Kconfig              |  10 ++
 drivers/devfreq/Makefile             |   1 +
 drivers/devfreq/mt8183-cci-devfreq.c | 235 +++++++++++++++++++++++++++++++++++
 3 files changed, 246 insertions(+)
 create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 6a172d3..da2f8ec 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -91,6 +91,16 @@ config ARM_EXYNOS_BUS_DEVFREQ
 	  and adjusts the operating frequencies and voltages with OPP support.
 	  This does not yet operate with optimal voltages.
 
+config ARM_MT8183_CCI_DEVFREQ
+	tristate "MT8183 CCI DEVFREQ Driver"
+	depends on ARM_MEDIATEK_CPUFREQ
+	help
+		This adds a devfreq driver for Cache Coherent Interconnect
+		of Mediatek MT8183, which is shared the same regulator
+		with cpu cluster.
+		It can track buck voltage and update a proper cci frequency.
+		Use notification to get regulator status.
+
 config ARM_TEGRA_DEVFREQ
 	tristate "Tegra DEVFREQ Driver"
 	depends on ARCH_TEGRA_124_SOC
diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
index 32b8d4d..817dde7 100644
--- a/drivers/devfreq/Makefile
+++ b/drivers/devfreq/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
 
 # DEVFREQ Drivers
 obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
+obj-$(CONFIG_ARM_MT8183_CCI_DEVFREQ)    += mt8183-cci-devfreq.o
 obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
 obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
 
diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c
new file mode 100644
index 0000000..af82d2e
--- /dev/null
+++ b/drivers/devfreq/mt8183-cci-devfreq.c
@@ -0,0 +1,235 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 MediaTek Inc.
+ */
+
+#include <linux/clk.h>
+#include <linux/devfreq.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+
+#include "governor.h"
+
+struct cci_devfreq {
+	struct devfreq		*devfreq;
+	struct regulator	*proc_reg;
+	unsigned long           proc_reg_uV;
+	struct clk		*cci_clk;
+	struct notifier_block	nb;
+};
+
+static int cci_devfreq_regulator_notifier(struct notifier_block *nb,
+					  unsigned long val, void *data)
+{
+	struct cci_devfreq *cci_df =
+		container_of(nb, struct cci_devfreq, nb);
+
+	/* deal with reduce frequency */
+	if (val & REGULATOR_EVENT_PRE_VOLTAGE_CHANGE) {
+		struct pre_voltage_change_data *pvc_data = data;
+
+		if (pvc_data->min_uV < pvc_data->old_uV) {
+			cci_df->proc_reg_uV =
+				(unsigned long)(pvc_data->min_uV);
+			mutex_lock(&cci_df->devfreq->lock);
+			update_devfreq(cci_df->devfreq);
+			mutex_unlock(&cci_df->devfreq->lock);
+		}
+	}
+
+	if ((val & REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE) &&
+	    ((unsigned long)data > cci_df->proc_reg_uV)) {
+		cci_df->proc_reg_uV = (unsigned long)data;
+		mutex_lock(&cci_df->devfreq->lock);
+		update_devfreq(cci_df->devfreq);
+		mutex_unlock(&cci_df->devfreq->lock);
+	}
+
+	/* deal with increase frequency */
+	if ((val & REGULATOR_EVENT_VOLTAGE_CHANGE) &&
+	    (cci_df->proc_reg_uV < (unsigned long)data)) {
+		cci_df->proc_reg_uV = (unsigned long)data;
+		mutex_lock(&cci_df->devfreq->lock);
+		update_devfreq(cci_df->devfreq);
+		mutex_unlock(&cci_df->devfreq->lock);
+	}
+
+	return 0;
+}
+
+static int mtk_cci_governor_get_target(struct devfreq *devfreq,
+				       unsigned long *freq)
+{
+	struct cci_devfreq *cci_df;
+	struct dev_pm_opp *opp;
+
+	cci_df = dev_get_drvdata(devfreq->dev.parent);
+
+	/* find available frequency */
+	opp = dev_pm_opp_find_max_freq_by_volt(devfreq->dev.parent,
+						 cci_df->proc_reg_uV);
+	*freq = dev_pm_opp_get_freq(opp);
+
+	return 0;
+}
+
+static int mtk_cci_governor_event_handler(struct devfreq *devfreq,
+					  unsigned int event, void *data)
+{
+	struct cci_devfreq *cci_df;
+	struct notifier_block *nb;
+
+	cci_df = dev_get_drvdata(devfreq->dev.parent);
+	nb = &cci_df->nb;
+
+	switch (event) {
+	case DEVFREQ_GOV_START:
+	case DEVFREQ_GOV_RESUME:
+		nb->notifier_call = cci_devfreq_regulator_notifier;
+		regulator_register_notifier(cci_df->proc_reg,
+					    nb);
+		break;
+
+	case DEVFREQ_GOV_STOP:
+	case DEVFREQ_GOV_SUSPEND:
+		regulator_unregister_notifier(cci_df->proc_reg,
+					    nb);
+
+		break;
+
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static struct devfreq_governor mtk_cci_devfreq_governor = {
+	.name = "mtk_cci_vmon",
+	.get_target_freq = mtk_cci_governor_get_target,
+	.event_handler = mtk_cci_governor_event_handler,
+};
+
+static int mtk_cci_devfreq_target(struct device *dev, unsigned long *freq,
+				  u32 flags)
+{
+	struct cci_devfreq *cci_df = dev_get_drvdata(dev);
+
+	if (!cci_df)
+		return -EINVAL;
+
+	clk_set_rate(cci_df->cci_clk, *freq);
+
+	return 0;
+}
+
+static struct devfreq_dev_profile cci_devfreq_profile = {
+	.target		= mtk_cci_devfreq_target,
+};
+
+static int mtk_cci_devfreq_probe(struct platform_device *pdev)
+{
+	struct device *cci_dev = &pdev->dev;
+	struct cci_devfreq *cci_df;
+	int ret;
+
+	cci_df = devm_kzalloc(cci_dev, sizeof(*cci_df), GFP_KERNEL);
+	if (!cci_df)
+		return -ENOMEM;
+
+	cci_df->cci_clk = clk_get(cci_dev, "cci_clock");
+	ret = PTR_ERR_OR_ZERO(cci_df->cci_clk);
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(cci_dev, "failed to get clock for CCI: %d\n",
+				ret);
+
+		return ret;
+	}
+
+	cci_df->proc_reg = regulator_get_optional(cci_dev, "proc");
+	ret = PTR_ERR_OR_ZERO(cci_df->proc_reg);
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(cci_dev, "failed to get regulator for CCI: %d\n",
+				ret);
+
+		goto err_put_clk;
+	}
+
+	ret = dev_pm_opp_of_add_table(&pdev->dev);
+	if (ret) {
+		dev_err(cci_dev, "Fail to init CCI OPP table\n");
+		goto err_put_reg;
+	}
+
+	platform_set_drvdata(pdev, cci_df);
+
+	cci_df->devfreq = devm_devfreq_add_device(cci_dev,
+						       &cci_devfreq_profile,
+						       "mtk_cci_vmon",
+						       NULL);
+	if (IS_ERR(cci_df->devfreq)) {
+		dev_err(cci_dev, "cannot create cci devfreq device\n", ret);
+		goto err_put_reg;
+	}
+
+	return 0;
+
+err_put_reg:
+	regulator_put(cci_df->proc_reg);
+err_put_clk:
+	clk_put(cci_df->cci_clk);
+
+	return ret;
+}
+
+static const struct of_device_id mediatek_cci_devfreq_of_match[] = {
+	{ .compatible = "mediatek,mt8183-cci" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, mediatek_cci_devfreq_of_match);
+
+static struct platform_driver cci_devfreq_driver = {
+	.probe	= mtk_cci_devfreq_probe,
+	.driver = {
+		.name = "mediatek-cci-devfreq",
+		.of_match_table = mediatek_cci_devfreq_of_match,
+	},
+};
+
+static int __init mtk_cci_devfreq_init(void)
+{
+	int ret;
+
+	ret = devfreq_add_governor(&mtk_cci_devfreq_governor);
+	if (ret) {
+		pr_err("%s: failed to add governor: %d\n", __func__, ret);
+		return ret;
+	}
+
+	ret = platform_driver_register(&cci_devfreq_driver);
+	if (ret)
+		devfreq_remove_governor(&mtk_cci_devfreq_governor);
+
+	return ret;
+}
+module_init(mtk_cci_devfreq_init)
+
+static void __exit mtk_cci_devfreq_exit(void)
+{
+	int ret;
+
+	ret = devfreq_remove_governor(&mtk_cci_devfreq_governor);
+	if (ret)
+		pr_err("%s: failed to remove governor: %d\n", __func__, ret);
+
+	platform_driver_unregister(&cci_devfreq_driver);
+}
+module_exit(mtk_cci_devfreq_exit)
+
+MODULE_DESCRIPTION("Mediatek CCI devfreq driver");
+MODULE_AUTHOR("Andrew-sh.Cheng <andrew-sh.cheng-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>");
+MODULE_LICENSE("GPL v2");
-- 
1.8.1.1.dirty

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

[Andrew-sh.Cheng]

This adds a devfreq driver for the Cache Coherent Interconnect (CCI)
of the Mediatek MT8183.

On the MT8183 the CCI is supplied by the same regulator as the LITTLE
cores. The driver is notified when the regulator voltage changes
(driven by cpufreq) and adjusts the CCI frequency to the maximum
possible value.

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

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 6a172d3..da2f8ec 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -91,6 +91,16 @@ config ARM_EXYNOS_BUS_DEVFREQ
 	  and adjusts the operating frequencies and voltages with OPP support.
 	  This does not yet operate with optimal voltages.
 
+config ARM_MT8183_CCI_DEVFREQ
+	tristate "MT8183 CCI DEVFREQ Driver"
+	depends on ARM_MEDIATEK_CPUFREQ
+	help
+		This adds a devfreq driver for Cache Coherent Interconnect
+		of Mediatek MT8183, which is shared the same regulator
+		with cpu cluster.
+		It can track buck voltage and update a proper cci frequency.
+		Use notification to get regulator status.
+
 config ARM_TEGRA_DEVFREQ
 	tristate "Tegra DEVFREQ Driver"
 	depends on ARCH_TEGRA_124_SOC
diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
index 32b8d4d..817dde7 100644
--- a/drivers/devfreq/Makefile
+++ b/drivers/devfreq/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
 
 # DEVFREQ Drivers
 obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
+obj-$(CONFIG_ARM_MT8183_CCI_DEVFREQ)    += mt8183-cci-devfreq.o
 obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
 obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
 
diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c
new file mode 100644
index 0000000..af82d2e
--- /dev/null
+++ b/drivers/devfreq/mt8183-cci-devfreq.c
@@ -0,0 +1,235 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 MediaTek Inc.
+ */
+
+#include <linux/clk.h>
+#include <linux/devfreq.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+
+#include "governor.h"
+
+struct cci_devfreq {
+	struct devfreq		*devfreq;
+	struct regulator	*proc_reg;
+	unsigned long           proc_reg_uV;
+	struct clk		*cci_clk;
+	struct notifier_block	nb;
+};
+
+static int cci_devfreq_regulator_notifier(struct notifier_block *nb,
+					  unsigned long val, void *data)
+{
+	struct cci_devfreq *cci_df =
+		container_of(nb, struct cci_devfreq, nb);
+
+	/* deal with reduce frequency */
+	if (val & REGULATOR_EVENT_PRE_VOLTAGE_CHANGE) {
+		struct pre_voltage_change_data *pvc_data = data;
+
+		if (pvc_data->min_uV < pvc_data->old_uV) {
+			cci_df->proc_reg_uV =
+				(unsigned long)(pvc_data->min_uV);
+			mutex_lock(&cci_df->devfreq->lock);
+			update_devfreq(cci_df->devfreq);
+			mutex_unlock(&cci_df->devfreq->lock);
+		}
+	}
+
+	if ((val & REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE) &&
+	    ((unsigned long)data > cci_df->proc_reg_uV)) {
+		cci_df->proc_reg_uV = (unsigned long)data;
+		mutex_lock(&cci_df->devfreq->lock);
+		update_devfreq(cci_df->devfreq);
+		mutex_unlock(&cci_df->devfreq->lock);
+	}
+
+	/* deal with increase frequency */
+	if ((val & REGULATOR_EVENT_VOLTAGE_CHANGE) &&
+	    (cci_df->proc_reg_uV < (unsigned long)data)) {
+		cci_df->proc_reg_uV = (unsigned long)data;
+		mutex_lock(&cci_df->devfreq->lock);
+		update_devfreq(cci_df->devfreq);
+		mutex_unlock(&cci_df->devfreq->lock);
+	}
+
+	return 0;
+}
+
+static int mtk_cci_governor_get_target(struct devfreq *devfreq,
+				       unsigned long *freq)
+{
+	struct cci_devfreq *cci_df;
+	struct dev_pm_opp *opp;
+
+	cci_df = dev_get_drvdata(devfreq->dev.parent);
+
+	/* find available frequency */
+	opp = dev_pm_opp_find_max_freq_by_volt(devfreq->dev.parent,
+						 cci_df->proc_reg_uV);
+	*freq = dev_pm_opp_get_freq(opp);
+
+	return 0;
+}
+
+static int mtk_cci_governor_event_handler(struct devfreq *devfreq,
+					  unsigned int event, void *data)
+{
+	struct cci_devfreq *cci_df;
+	struct notifier_block *nb;
+
+	cci_df = dev_get_drvdata(devfreq->dev.parent);
+	nb = &cci_df->nb;
+
+	switch (event) {
+	case DEVFREQ_GOV_START:
+	case DEVFREQ_GOV_RESUME:
+		nb->notifier_call = cci_devfreq_regulator_notifier;
+		regulator_register_notifier(cci_df->proc_reg,
+					    nb);
+		break;
+
+	case DEVFREQ_GOV_STOP:
+	case DEVFREQ_GOV_SUSPEND:
+		regulator_unregister_notifier(cci_df->proc_reg,
+					    nb);
+
+		break;
+
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static struct devfreq_governor mtk_cci_devfreq_governor = {
+	.name = "mtk_cci_vmon",
+	.get_target_freq = mtk_cci_governor_get_target,
+	.event_handler = mtk_cci_governor_event_handler,
+};
+
+static int mtk_cci_devfreq_target(struct device *dev, unsigned long *freq,
+				  u32 flags)
+{
+	struct cci_devfreq *cci_df = dev_get_drvdata(dev);
+
+	if (!cci_df)
+		return -EINVAL;
+
+	clk_set_rate(cci_df->cci_clk, *freq);
+
+	return 0;
+}
+
+static struct devfreq_dev_profile cci_devfreq_profile = {
+	.target		= mtk_cci_devfreq_target,
+};
+
+static int mtk_cci_devfreq_probe(struct platform_device *pdev)
+{
+	struct device *cci_dev = &pdev->dev;
+	struct cci_devfreq *cci_df;
+	int ret;
+
+	cci_df = devm_kzalloc(cci_dev, sizeof(*cci_df), GFP_KERNEL);
+	if (!cci_df)
+		return -ENOMEM;
+
+	cci_df->cci_clk = clk_get(cci_dev, "cci_clock");
+	ret = PTR_ERR_OR_ZERO(cci_df->cci_clk);
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(cci_dev, "failed to get clock for CCI: %d\n",
+				ret);
+
+		return ret;
+	}
+
+	cci_df->proc_reg = regulator_get_optional(cci_dev, "proc");
+	ret = PTR_ERR_OR_ZERO(cci_df->proc_reg);
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(cci_dev, "failed to get regulator for CCI: %d\n",
+				ret);
+
+		goto err_put_clk;
+	}
+
+	ret = dev_pm_opp_of_add_table(&pdev->dev);
+	if (ret) {
+		dev_err(cci_dev, "Fail to init CCI OPP table\n");
+		goto err_put_reg;
+	}
+
+	platform_set_drvdata(pdev, cci_df);
+
+	cci_df->devfreq = devm_devfreq_add_device(cci_dev,
+						       &cci_devfreq_profile,
+						       "mtk_cci_vmon",
+						       NULL);
+	if (IS_ERR(cci_df->devfreq)) {
+		dev_err(cci_dev, "cannot create cci devfreq device\n", ret);
+		goto err_put_reg;
+	}
+
+	return 0;
+
+err_put_reg:
+	regulator_put(cci_df->proc_reg);
+err_put_clk:
+	clk_put(cci_df->cci_clk);
+
+	return ret;
+}
+
+static const struct of_device_id mediatek_cci_devfreq_of_match[] = {
+	{ .compatible = "mediatek,mt8183-cci" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, mediatek_cci_devfreq_of_match);
+
+static struct platform_driver cci_devfreq_driver = {
+	.probe	= mtk_cci_devfreq_probe,
+	.driver = {
+		.name = "mediatek-cci-devfreq",
+		.of_match_table = mediatek_cci_devfreq_of_match,
+	},
+};
+
+static int __init mtk_cci_devfreq_init(void)
+{
+	int ret;
+
+	ret = devfreq_add_governor(&mtk_cci_devfreq_governor);
+	if (ret) {
+		pr_err("%s: failed to add governor: %d\n", __func__, ret);
+		return ret;
+	}
+
+	ret = platform_driver_register(&cci_devfreq_driver);
+	if (ret)
+		devfreq_remove_governor(&mtk_cci_devfreq_governor);
+
+	return ret;
+}
+module_init(mtk_cci_devfreq_init)
+
+static void __exit mtk_cci_devfreq_exit(void)
+{
+	int ret;
+
+	ret = devfreq_remove_governor(&mtk_cci_devfreq_governor);
+	if (ret)
+		pr_err("%s: failed to remove governor: %d\n", __func__, ret);
+
+	platform_driver_unregister(&cci_devfreq_driver);
+}
+module_exit(mtk_cci_devfreq_exit)
+
+MODULE_DESCRIPTION("Mediatek CCI devfreq driver");
+MODULE_AUTHOR("Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>");
+MODULE_LICENSE("GPL v2");
-- 
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: [v2,4/4] devfreq: add mediatek cci devfreq

[Guenter Roeck]

On Fri, Mar 29, 2019 at 02:46:12PM +0800, Andrew-sh Cheng wrote:
> This adds a devfreq driver for the Cache Coherent Interconnect (CCI)
> of the Mediatek MT8183.
> 
> On the MT8183 the CCI is supplied by the same regulator as the LITTLE
> cores. The driver is notified when the regulator voltage changes
> (driven by cpufreq) and adjusts the CCI frequency to the maximum
> possible value.
> 
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> ---
>  drivers/devfreq/Kconfig              |  10 ++
>  drivers/devfreq/Makefile             |   1 +
>  drivers/devfreq/mt8183-cci-devfreq.c | 235 +++++++++++++++++++++++++++++++++++
>  3 files changed, 246 insertions(+)
>  create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 6a172d3..da2f8ec 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -91,6 +91,16 @@ config ARM_EXYNOS_BUS_DEVFREQ
>  	  and adjusts the operating frequencies and voltages with OPP support.
>  	  This does not yet operate with optimal voltages.
>  
> +config ARM_MT8183_CCI_DEVFREQ
> +	tristate "MT8183 CCI DEVFREQ Driver"
> +	depends on ARM_MEDIATEK_CPUFREQ
> +	help
> +		This adds a devfreq driver for Cache Coherent Interconnect
> +		of Mediatek MT8183, which is shared the same regulator
> +		with cpu cluster.
> +		It can track buck voltage and update a proper cci frequency.
> +		Use notification to get regulator status.
> +
>  config ARM_TEGRA_DEVFREQ
>  	tristate "Tegra DEVFREQ Driver"
>  	depends on ARCH_TEGRA_124_SOC
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 32b8d4d..817dde7 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
>  
>  # DEVFREQ Drivers
>  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
> +obj-$(CONFIG_ARM_MT8183_CCI_DEVFREQ)    += mt8183-cci-devfreq.o
>  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
>  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
>  
> diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c
> new file mode 100644
> index 0000000..af82d2e
> --- /dev/null
> +++ b/drivers/devfreq/mt8183-cci-devfreq.c
> @@ -0,0 +1,235 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 MediaTek Inc.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/devfreq.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include "governor.h"
> +
> +struct cci_devfreq {
> +	struct devfreq		*devfreq;
> +	struct regulator	*proc_reg;
> +	unsigned long           proc_reg_uV;
> +	struct clk		*cci_clk;
> +	struct notifier_block	nb;
> +};
> +
> +static int cci_devfreq_regulator_notifier(struct notifier_block *nb,
> +					  unsigned long val, void *data)
> +{
> +	struct cci_devfreq *cci_df =
> +		container_of(nb, struct cci_devfreq, nb);
> +
> +	/* deal with reduce frequency */
> +	if (val & REGULATOR_EVENT_PRE_VOLTAGE_CHANGE) {
> +		struct pre_voltage_change_data *pvc_data = data;
> +
> +		if (pvc_data->min_uV < pvc_data->old_uV) {
> +			cci_df->proc_reg_uV =
> +				(unsigned long)(pvc_data->min_uV);
> +			mutex_lock(&cci_df->devfreq->lock);
> +			update_devfreq(cci_df->devfreq);
> +			mutex_unlock(&cci_df->devfreq->lock);
> +		}
> +	}
> +
> +	if ((val & REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE) &&
> +	    ((unsigned long)data > cci_df->proc_reg_uV)) {
> +		cci_df->proc_reg_uV = (unsigned long)data;
> +		mutex_lock(&cci_df->devfreq->lock);
> +		update_devfreq(cci_df->devfreq);
> +		mutex_unlock(&cci_df->devfreq->lock);
> +	}
> +
> +	/* deal with increase frequency */
> +	if ((val & REGULATOR_EVENT_VOLTAGE_CHANGE) &&
> +	    (cci_df->proc_reg_uV < (unsigned long)data)) {
> +		cci_df->proc_reg_uV = (unsigned long)data;
> +		mutex_lock(&cci_df->devfreq->lock);
> +		update_devfreq(cci_df->devfreq);
> +		mutex_unlock(&cci_df->devfreq->lock);
> +	}
> +
> +	return 0;
> +}
> +
> +static int mtk_cci_governor_get_target(struct devfreq *devfreq,
> +				       unsigned long *freq)
> +{
> +	struct cci_devfreq *cci_df;
> +	struct dev_pm_opp *opp;
> +
> +	cci_df = dev_get_drvdata(devfreq->dev.parent);
> +
> +	/* find available frequency */
> +	opp = dev_pm_opp_find_max_freq_by_volt(devfreq->dev.parent,
> +						 cci_df->proc_reg_uV);
> +	*freq = dev_pm_opp_get_freq(opp);
> +
> +	return 0;
> +}
> +
> +static int mtk_cci_governor_event_handler(struct devfreq *devfreq,
> +					  unsigned int event, void *data)
> +{
> +	struct cci_devfreq *cci_df;
> +	struct notifier_block *nb;
> +
> +	cci_df = dev_get_drvdata(devfreq->dev.parent);
> +	nb = &cci_df->nb;
> +
> +	switch (event) {
> +	case DEVFREQ_GOV_START:
> +	case DEVFREQ_GOV_RESUME:
> +		nb->notifier_call = cci_devfreq_regulator_notifier;
> +		regulator_register_notifier(cci_df->proc_reg,
> +					    nb);
> +		break;
> +
> +	case DEVFREQ_GOV_STOP:
> +	case DEVFREQ_GOV_SUSPEND:
> +		regulator_unregister_notifier(cci_df->proc_reg,
> +					    nb);
> +
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct devfreq_governor mtk_cci_devfreq_governor = {
> +	.name = "mtk_cci_vmon",
> +	.get_target_freq = mtk_cci_governor_get_target,
> +	.event_handler = mtk_cci_governor_event_handler,
> +};
> +
> +static int mtk_cci_devfreq_target(struct device *dev, unsigned long *freq,
> +				  u32 flags)
> +{
> +	struct cci_devfreq *cci_df = dev_get_drvdata(dev);
> +
> +	if (!cci_df)
> +		return -EINVAL;
> +
> +	clk_set_rate(cci_df->cci_clk, *freq);
> +
> +	return 0;
> +}
> +
> +static struct devfreq_dev_profile cci_devfreq_profile = {
> +	.target		= mtk_cci_devfreq_target,
> +};
> +
> +static int mtk_cci_devfreq_probe(struct platform_device *pdev)
> +{
> +	struct device *cci_dev = &pdev->dev;
> +	struct cci_devfreq *cci_df;
> +	int ret;
> +
> +	cci_df = devm_kzalloc(cci_dev, sizeof(*cci_df), GFP_KERNEL);
> +	if (!cci_df)
> +		return -ENOMEM;
> +
> +	cci_df->cci_clk = clk_get(cci_dev, "cci_clock");

Should use devm_clk_get().

> +	ret = PTR_ERR_OR_ZERO(cci_df->cci_clk);
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(cci_dev, "failed to get clock for CCI: %d\n",
> +				ret);
> +
> +		return ret;
> +	}
> +
> +	cci_df->proc_reg = regulator_get_optional(cci_dev, "proc");

Should use devm_regulator_get_optional().

> +	ret = PTR_ERR_OR_ZERO(cci_df->proc_reg);
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(cci_dev, "failed to get regulator for CCI: %d\n",
> +				ret);
> +
> +		goto err_put_clk;
> +	}
> +
> +	ret = dev_pm_opp_of_add_table(&pdev->dev);
> +	if (ret) {
> +		dev_err(cci_dev, "Fail to init CCI OPP table\n");

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

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

		ret = PTR_ERR(cci_df->devfreq);

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

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

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

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

Can be dropped when using devm_ functions above.

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

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

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

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

[Guenter Roeck]

On Fri, Mar 29, 2019 at 02:46:12PM +0800, Andrew-sh Cheng wrote:
> This adds a devfreq driver for the Cache Coherent Interconnect (CCI)
> of the Mediatek MT8183.
> 
> On the MT8183 the CCI is supplied by the same regulator as the LITTLE
> cores. The driver is notified when the regulator voltage changes
> (driven by cpufreq) and adjusts the CCI frequency to the maximum
> possible value.
> 
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> ---
>  drivers/devfreq/Kconfig              |  10 ++
>  drivers/devfreq/Makefile             |   1 +
>  drivers/devfreq/mt8183-cci-devfreq.c | 235 +++++++++++++++++++++++++++++++++++
>  3 files changed, 246 insertions(+)
>  create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 6a172d3..da2f8ec 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -91,6 +91,16 @@ config ARM_EXYNOS_BUS_DEVFREQ
>  	  and adjusts the operating frequencies and voltages with OPP support.
>  	  This does not yet operate with optimal voltages.
>  
> +config ARM_MT8183_CCI_DEVFREQ
> +	tristate "MT8183 CCI DEVFREQ Driver"
> +	depends on ARM_MEDIATEK_CPUFREQ
> +	help
> +		This adds a devfreq driver for Cache Coherent Interconnect
> +		of Mediatek MT8183, which is shared the same regulator
> +		with cpu cluster.
> +		It can track buck voltage and update a proper cci frequency.
> +		Use notification to get regulator status.
> +
>  config ARM_TEGRA_DEVFREQ
>  	tristate "Tegra DEVFREQ Driver"
>  	depends on ARCH_TEGRA_124_SOC
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 32b8d4d..817dde7 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
>  
>  # DEVFREQ Drivers
>  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
> +obj-$(CONFIG_ARM_MT8183_CCI_DEVFREQ)    += mt8183-cci-devfreq.o
>  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
>  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
>  
> diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c
> new file mode 100644
> index 0000000..af82d2e
> --- /dev/null
> +++ b/drivers/devfreq/mt8183-cci-devfreq.c
> @@ -0,0 +1,235 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 MediaTek Inc.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/devfreq.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include "governor.h"
> +
> +struct cci_devfreq {
> +	struct devfreq		*devfreq;
> +	struct regulator	*proc_reg;
> +	unsigned long           proc_reg_uV;
> +	struct clk		*cci_clk;
> +	struct notifier_block	nb;
> +};
> +
> +static int cci_devfreq_regulator_notifier(struct notifier_block *nb,
> +					  unsigned long val, void *data)
> +{
> +	struct cci_devfreq *cci_df =
> +		container_of(nb, struct cci_devfreq, nb);
> +
> +	/* deal with reduce frequency */
> +	if (val & REGULATOR_EVENT_PRE_VOLTAGE_CHANGE) {
> +		struct pre_voltage_change_data *pvc_data = data;
> +
> +		if (pvc_data->min_uV < pvc_data->old_uV) {
> +			cci_df->proc_reg_uV =
> +				(unsigned long)(pvc_data->min_uV);
> +			mutex_lock(&cci_df->devfreq->lock);
> +			update_devfreq(cci_df->devfreq);
> +			mutex_unlock(&cci_df->devfreq->lock);
> +		}
> +	}
> +
> +	if ((val & REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE) &&
> +	    ((unsigned long)data > cci_df->proc_reg_uV)) {
> +		cci_df->proc_reg_uV = (unsigned long)data;
> +		mutex_lock(&cci_df->devfreq->lock);
> +		update_devfreq(cci_df->devfreq);
> +		mutex_unlock(&cci_df->devfreq->lock);
> +	}
> +
> +	/* deal with increase frequency */
> +	if ((val & REGULATOR_EVENT_VOLTAGE_CHANGE) &&
> +	    (cci_df->proc_reg_uV < (unsigned long)data)) {
> +		cci_df->proc_reg_uV = (unsigned long)data;
> +		mutex_lock(&cci_df->devfreq->lock);
> +		update_devfreq(cci_df->devfreq);
> +		mutex_unlock(&cci_df->devfreq->lock);
> +	}
> +
> +	return 0;
> +}
> +
> +static int mtk_cci_governor_get_target(struct devfreq *devfreq,
> +				       unsigned long *freq)
> +{
> +	struct cci_devfreq *cci_df;
> +	struct dev_pm_opp *opp;
> +
> +	cci_df = dev_get_drvdata(devfreq->dev.parent);
> +
> +	/* find available frequency */
> +	opp = dev_pm_opp_find_max_freq_by_volt(devfreq->dev.parent,
> +						 cci_df->proc_reg_uV);
> +	*freq = dev_pm_opp_get_freq(opp);
> +
> +	return 0;
> +}
> +
> +static int mtk_cci_governor_event_handler(struct devfreq *devfreq,
> +					  unsigned int event, void *data)
> +{
> +	struct cci_devfreq *cci_df;
> +	struct notifier_block *nb;
> +
> +	cci_df = dev_get_drvdata(devfreq->dev.parent);
> +	nb = &cci_df->nb;
> +
> +	switch (event) {
> +	case DEVFREQ_GOV_START:
> +	case DEVFREQ_GOV_RESUME:
> +		nb->notifier_call = cci_devfreq_regulator_notifier;
> +		regulator_register_notifier(cci_df->proc_reg,
> +					    nb);
> +		break;
> +
> +	case DEVFREQ_GOV_STOP:
> +	case DEVFREQ_GOV_SUSPEND:
> +		regulator_unregister_notifier(cci_df->proc_reg,
> +					    nb);
> +
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct devfreq_governor mtk_cci_devfreq_governor = {
> +	.name = "mtk_cci_vmon",
> +	.get_target_freq = mtk_cci_governor_get_target,
> +	.event_handler = mtk_cci_governor_event_handler,
> +};
> +
> +static int mtk_cci_devfreq_target(struct device *dev, unsigned long *freq,
> +				  u32 flags)
> +{
> +	struct cci_devfreq *cci_df = dev_get_drvdata(dev);
> +
> +	if (!cci_df)
> +		return -EINVAL;
> +
> +	clk_set_rate(cci_df->cci_clk, *freq);
> +
> +	return 0;
> +}
> +
> +static struct devfreq_dev_profile cci_devfreq_profile = {
> +	.target		= mtk_cci_devfreq_target,
> +};
> +
> +static int mtk_cci_devfreq_probe(struct platform_device *pdev)
> +{
> +	struct device *cci_dev = &pdev->dev;
> +	struct cci_devfreq *cci_df;
> +	int ret;
> +
> +	cci_df = devm_kzalloc(cci_dev, sizeof(*cci_df), GFP_KERNEL);
> +	if (!cci_df)
> +		return -ENOMEM;
> +
> +	cci_df->cci_clk = clk_get(cci_dev, "cci_clock");

Should use devm_clk_get().

> +	ret = PTR_ERR_OR_ZERO(cci_df->cci_clk);
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(cci_dev, "failed to get clock for CCI: %d\n",
> +				ret);
> +
> +		return ret;
> +	}
> +
> +	cci_df->proc_reg = regulator_get_optional(cci_dev, "proc");

Should use devm_regulator_get_optional().

> +	ret = PTR_ERR_OR_ZERO(cci_df->proc_reg);
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(cci_dev, "failed to get regulator for CCI: %d\n",
> +				ret);
> +
> +		goto err_put_clk;
> +	}
> +
> +	ret = dev_pm_opp_of_add_table(&pdev->dev);
> +	if (ret) {
> +		dev_err(cci_dev, "Fail to init CCI OPP table\n");

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

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

		ret = PTR_ERR(cci_df->devfreq);

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

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

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

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

Can be dropped when using devm_ functions above.

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

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

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

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

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

[andrew-sh.cheng]

On Mon, 2019-04-08 at 10:22 -0700, Guenter Roeck wrote:
> On Fri, Mar 29, 2019 at 02:46:12PM +0800, Andrew-sh Cheng wrote:
> > This adds a devfreq driver for the Cache Coherent Interconnect (CCI)
> > of the Mediatek MT8183.
> > 
> > On the MT8183 the CCI is supplied by the same regulator as the LITTLE
> > cores. The driver is notified when the regulator voltage changes
> > (driven by cpufreq) and adjusts the CCI frequency to the maximum
> > possible value.
> > 
> > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> > ---
> >  drivers/devfreq/Kconfig              |  10 ++
> >  drivers/devfreq/Makefile             |   1 +
> >  drivers/devfreq/mt8183-cci-devfreq.c | 235 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 246 insertions(+)
> >  create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
> > 
> > diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> > index 6a172d3..da2f8ec 100644
> > --- a/drivers/devfreq/Kconfig
> > +++ b/drivers/devfreq/Kconfig
> > @@ -91,6 +91,16 @@ config ARM_EXYNOS_BUS_DEVFREQ
> >  	  and adjusts the operating frequencies and voltages with OPP support.
> >  	  This does not yet operate with optimal voltages.
> >  
> > +config ARM_MT8183_CCI_DEVFREQ
> > +	tristate "MT8183 CCI DEVFREQ Driver"
> > +	depends on ARM_MEDIATEK_CPUFREQ
> > +	help
> > +		This adds a devfreq driver for Cache Coherent Interconnect
> > +		of Mediatek MT8183, which is shared the same regulator
> > +		with cpu cluster.
> > +		It can track buck voltage and update a proper cci frequency.
> > +		Use notification to get regulator status.
> > +
> >  config ARM_TEGRA_DEVFREQ
> >  	tristate "Tegra DEVFREQ Driver"
> >  	depends on ARCH_TEGRA_124_SOC
> > diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> > index 32b8d4d..817dde7 100644
> > --- a/drivers/devfreq/Makefile
> > +++ b/drivers/devfreq/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
> >  
> >  # DEVFREQ Drivers
> >  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
> > +obj-$(CONFIG_ARM_MT8183_CCI_DEVFREQ)    += mt8183-cci-devfreq.o
> >  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
> >  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
> >  
> > diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c
> > new file mode 100644
> > index 0000000..af82d2e
> > --- /dev/null
> > +++ b/drivers/devfreq/mt8183-cci-devfreq.c
> > @@ -0,0 +1,235 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2019 MediaTek Inc.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/devfreq.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#include "governor.h"
> > +
> > +struct cci_devfreq {
> > +	struct devfreq		*devfreq;
> > +	struct regulator	*proc_reg;
> > +	unsigned long           proc_reg_uV;
> > +	struct clk		*cci_clk;
> > +	struct notifier_block	nb;
> > +};
> > +
> > +static int cci_devfreq_regulator_notifier(struct notifier_block *nb,
> > +					  unsigned long val, void *data)
> > +{
> > +	struct cci_devfreq *cci_df =
> > +		container_of(nb, struct cci_devfreq, nb);
> > +
> > +	/* deal with reduce frequency */
> > +	if (val & REGULATOR_EVENT_PRE_VOLTAGE_CHANGE) {
> > +		struct pre_voltage_change_data *pvc_data = data;
> > +
> > +		if (pvc_data->min_uV < pvc_data->old_uV) {
> > +			cci_df->proc_reg_uV =
> > +				(unsigned long)(pvc_data->min_uV);
> > +			mutex_lock(&cci_df->devfreq->lock);
> > +			update_devfreq(cci_df->devfreq);
> > +			mutex_unlock(&cci_df->devfreq->lock);
> > +		}
> > +	}
> > +
> > +	if ((val & REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE) &&
> > +	    ((unsigned long)data > cci_df->proc_reg_uV)) {
> > +		cci_df->proc_reg_uV = (unsigned long)data;
> > +		mutex_lock(&cci_df->devfreq->lock);
> > +		update_devfreq(cci_df->devfreq);
> > +		mutex_unlock(&cci_df->devfreq->lock);
> > +	}
> > +
> > +	/* deal with increase frequency */
> > +	if ((val & REGULATOR_EVENT_VOLTAGE_CHANGE) &&
> > +	    (cci_df->proc_reg_uV < (unsigned long)data)) {
> > +		cci_df->proc_reg_uV = (unsigned long)data;
> > +		mutex_lock(&cci_df->devfreq->lock);
> > +		update_devfreq(cci_df->devfreq);
> > +		mutex_unlock(&cci_df->devfreq->lock);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_cci_governor_get_target(struct devfreq *devfreq,
> > +				       unsigned long *freq)
> > +{
> > +	struct cci_devfreq *cci_df;
> > +	struct dev_pm_opp *opp;
> > +
> > +	cci_df = dev_get_drvdata(devfreq->dev.parent);
> > +
> > +	/* find available frequency */
> > +	opp = dev_pm_opp_find_max_freq_by_volt(devfreq->dev.parent,
> > +						 cci_df->proc_reg_uV);
> > +	*freq = dev_pm_opp_get_freq(opp);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_cci_governor_event_handler(struct devfreq *devfreq,
> > +					  unsigned int event, void *data)
> > +{
> > +	struct cci_devfreq *cci_df;
> > +	struct notifier_block *nb;
> > +
> > +	cci_df = dev_get_drvdata(devfreq->dev.parent);
> > +	nb = &cci_df->nb;
> > +
> > +	switch (event) {
> > +	case DEVFREQ_GOV_START:
> > +	case DEVFREQ_GOV_RESUME:
> > +		nb->notifier_call = cci_devfreq_regulator_notifier;
> > +		regulator_register_notifier(cci_df->proc_reg,
> > +					    nb);
> > +		break;
> > +
> > +	case DEVFREQ_GOV_STOP:
> > +	case DEVFREQ_GOV_SUSPEND:
> > +		regulator_unregister_notifier(cci_df->proc_reg,
> > +					    nb);
> > +
> > +		break;
> > +
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct devfreq_governor mtk_cci_devfreq_governor = {
> > +	.name = "mtk_cci_vmon",
> > +	.get_target_freq = mtk_cci_governor_get_target,
> > +	.event_handler = mtk_cci_governor_event_handler,
> > +};
> > +
> > +static int mtk_cci_devfreq_target(struct device *dev, unsigned long *freq,
> > +				  u32 flags)
> > +{
> > +	struct cci_devfreq *cci_df = dev_get_drvdata(dev);
> > +
> > +	if (!cci_df)
> > +		return -EINVAL;
> > +
> > +	clk_set_rate(cci_df->cci_clk, *freq);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct devfreq_dev_profile cci_devfreq_profile = {
> > +	.target		= mtk_cci_devfreq_target,
> > +};
> > +
> > +static int mtk_cci_devfreq_probe(struct platform_device *pdev)
> > +{
> > +	struct device *cci_dev = &pdev->dev;
> > +	struct cci_devfreq *cci_df;
> > +	int ret;
> > +
> > +	cci_df = devm_kzalloc(cci_dev, sizeof(*cci_df), GFP_KERNEL);
> > +	if (!cci_df)
> > +		return -ENOMEM;
> > +
> > +	cci_df->cci_clk = clk_get(cci_dev, "cci_clock");
> 
> Should use devm_clk_get().
I will change it on next patch.
> 
> > +	ret = PTR_ERR_OR_ZERO(cci_df->cci_clk);
> > +	if (ret) {
> > +		if (ret != -EPROBE_DEFER)
> > +			dev_err(cci_dev, "failed to get clock for CCI: %d\n",
> > +				ret);
> > +
> > +		return ret;
> > +	}
> > +
> > +	cci_df->proc_reg = regulator_get_optional(cci_dev, "proc");
> 
> Should use devm_regulator_get_optional().
I will change it on next patch.
> 
> > +	ret = PTR_ERR_OR_ZERO(cci_df->proc_reg);
> > +	if (ret) {
> > +		if (ret != -EPROBE_DEFER)
> > +			dev_err(cci_dev, "failed to get regulator for CCI: %d\n",
> > +				ret);
> > +
> > +		goto err_put_clk;
> > +	}
> > +
> > +	ret = dev_pm_opp_of_add_table(&pdev->dev);
> > +	if (ret) {
> > +		dev_err(cci_dev, "Fail to init CCI OPP table\n");
> 
> No error code display here ? Not that it really matters, but it is
> inconsistent with the other error messages.
I will add it on next patch.
> 
> > +		goto err_put_reg;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, cci_df);
> > +
> > +	cci_df->devfreq = devm_devfreq_add_device(cci_dev,
> > +						       &cci_devfreq_profile,
> > +						       "mtk_cci_vmon",
> > +						       NULL);
> > +	if (IS_ERR(cci_df->devfreq)) {
> 
> 		ret = PTR_ERR(cci_df->devfreq);
> 
> > +		dev_err(cci_dev, "cannot create cci devfreq device\n", ret);
> 
> 		dev_err(cci_dev, "cannot create cci devfreq device: %d\n", ret);
> 
> Something like
> 		dev_pm_opp_of_remove_table(...);
> seems to be missing here.
I will fix this, and add dev_pm_opp_of_remove_table(...) when error
occur.
> 
> > +		goto err_put_reg;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_put_reg:
> > +	regulator_put(cci_df->proc_reg);
> > +err_put_clk:
> > +	clk_put(cci_df->cci_clk);
> 
> Can be dropped when using devm_ functions above.
I will remove these code.
> 
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct of_device_id mediatek_cci_devfreq_of_match[] = {
> > +	{ .compatible = "mediatek,mt8183-cci" },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, mediatek_cci_devfreq_of_match);
> > +
> > +static struct platform_driver cci_devfreq_driver = {
> > +	.probe	= mtk_cci_devfreq_probe,
> > +	.driver = {
> > +		.name = "mediatek-cci-devfreq",
> > +		.of_match_table = mediatek_cci_devfreq_of_match,
> 
> If the driver depends on OF, that should be stated in the Kconfig file.
> Otherwise, of_match_ptr() should be used, and mediatek_cci_devfreq_of_match
> should be tagged with __maybe_unused (or made conditional with #ifdef).
I will fix this on next patch.
> 
> > +	},
> > +};
> > +
> > +static int __init mtk_cci_devfreq_init(void)
> > +{
> > +	int ret;
> > +
> > +	ret = devfreq_add_governor(&mtk_cci_devfreq_governor);
> > +	if (ret) {
> > +		pr_err("%s: failed to add governor: %d\n", __func__, ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = platform_driver_register(&cci_devfreq_driver);
> > +	if (ret)
> > +		devfreq_remove_governor(&mtk_cci_devfreq_governor);
> > +
> > +	return ret;
> > +}
> > +module_init(mtk_cci_devfreq_init)
> > +
> > +static void __exit mtk_cci_devfreq_exit(void)
> > +{
> > +	int ret;
> > +
> > +	ret = devfreq_remove_governor(&mtk_cci_devfreq_governor);
> > +	if (ret)
> > +		pr_err("%s: failed to remove governor: %d\n", __func__, ret);
> > +
> > +	platform_driver_unregister(&cci_devfreq_driver);
> > +}
> > +module_exit(mtk_cci_devfreq_exit)
> > +
> > +MODULE_DESCRIPTION("Mediatek CCI devfreq driver");
> > +MODULE_AUTHOR("Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>");
> > +MODULE_LICENSE("GPL v2");
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[andrew-sh.cheng]

On Mon, 2019-04-08 at 10:22 -0700, Guenter Roeck wrote:
> On Fri, Mar 29, 2019 at 02:46:12PM +0800, Andrew-sh Cheng wrote:
> > This adds a devfreq driver for the Cache Coherent Interconnect (CCI)
> > of the Mediatek MT8183.
> > 
> > On the MT8183 the CCI is supplied by the same regulator as the LITTLE
> > cores. The driver is notified when the regulator voltage changes
> > (driven by cpufreq) and adjusts the CCI frequency to the maximum
> > possible value.
> > 
> > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> > ---
> >  drivers/devfreq/Kconfig              |  10 ++
> >  drivers/devfreq/Makefile             |   1 +
> >  drivers/devfreq/mt8183-cci-devfreq.c | 235 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 246 insertions(+)
> >  create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
> > 
> > diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> > index 6a172d3..da2f8ec 100644
> > --- a/drivers/devfreq/Kconfig
> > +++ b/drivers/devfreq/Kconfig
> > @@ -91,6 +91,16 @@ config ARM_EXYNOS_BUS_DEVFREQ
> >  	  and adjusts the operating frequencies and voltages with OPP support.
> >  	  This does not yet operate with optimal voltages.
> >  
> > +config ARM_MT8183_CCI_DEVFREQ
> > +	tristate "MT8183 CCI DEVFREQ Driver"
> > +	depends on ARM_MEDIATEK_CPUFREQ
> > +	help
> > +		This adds a devfreq driver for Cache Coherent Interconnect
> > +		of Mediatek MT8183, which is shared the same regulator
> > +		with cpu cluster.
> > +		It can track buck voltage and update a proper cci frequency.
> > +		Use notification to get regulator status.
> > +
> >  config ARM_TEGRA_DEVFREQ
> >  	tristate "Tegra DEVFREQ Driver"
> >  	depends on ARCH_TEGRA_124_SOC
> > diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> > index 32b8d4d..817dde7 100644
> > --- a/drivers/devfreq/Makefile
> > +++ b/drivers/devfreq/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
> >  
> >  # DEVFREQ Drivers
> >  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
> > +obj-$(CONFIG_ARM_MT8183_CCI_DEVFREQ)    += mt8183-cci-devfreq.o
> >  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
> >  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
> >  
> > diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c
> > new file mode 100644
> > index 0000000..af82d2e
> > --- /dev/null
> > +++ b/drivers/devfreq/mt8183-cci-devfreq.c
> > @@ -0,0 +1,235 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2019 MediaTek Inc.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/devfreq.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#include "governor.h"
> > +
> > +struct cci_devfreq {
> > +	struct devfreq		*devfreq;
> > +	struct regulator	*proc_reg;
> > +	unsigned long           proc_reg_uV;
> > +	struct clk		*cci_clk;
> > +	struct notifier_block	nb;
> > +};
> > +
> > +static int cci_devfreq_regulator_notifier(struct notifier_block *nb,
> > +					  unsigned long val, void *data)
> > +{
> > +	struct cci_devfreq *cci_df =
> > +		container_of(nb, struct cci_devfreq, nb);
> > +
> > +	/* deal with reduce frequency */
> > +	if (val & REGULATOR_EVENT_PRE_VOLTAGE_CHANGE) {
> > +		struct pre_voltage_change_data *pvc_data = data;
> > +
> > +		if (pvc_data->min_uV < pvc_data->old_uV) {
> > +			cci_df->proc_reg_uV =
> > +				(unsigned long)(pvc_data->min_uV);
> > +			mutex_lock(&cci_df->devfreq->lock);
> > +			update_devfreq(cci_df->devfreq);
> > +			mutex_unlock(&cci_df->devfreq->lock);
> > +		}
> > +	}
> > +
> > +	if ((val & REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE) &&
> > +	    ((unsigned long)data > cci_df->proc_reg_uV)) {
> > +		cci_df->proc_reg_uV = (unsigned long)data;
> > +		mutex_lock(&cci_df->devfreq->lock);
> > +		update_devfreq(cci_df->devfreq);
> > +		mutex_unlock(&cci_df->devfreq->lock);
> > +	}
> > +
> > +	/* deal with increase frequency */
> > +	if ((val & REGULATOR_EVENT_VOLTAGE_CHANGE) &&
> > +	    (cci_df->proc_reg_uV < (unsigned long)data)) {
> > +		cci_df->proc_reg_uV = (unsigned long)data;
> > +		mutex_lock(&cci_df->devfreq->lock);
> > +		update_devfreq(cci_df->devfreq);
> > +		mutex_unlock(&cci_df->devfreq->lock);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_cci_governor_get_target(struct devfreq *devfreq,
> > +				       unsigned long *freq)
> > +{
> > +	struct cci_devfreq *cci_df;
> > +	struct dev_pm_opp *opp;
> > +
> > +	cci_df = dev_get_drvdata(devfreq->dev.parent);
> > +
> > +	/* find available frequency */
> > +	opp = dev_pm_opp_find_max_freq_by_volt(devfreq->dev.parent,
> > +						 cci_df->proc_reg_uV);
> > +	*freq = dev_pm_opp_get_freq(opp);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_cci_governor_event_handler(struct devfreq *devfreq,
> > +					  unsigned int event, void *data)
> > +{
> > +	struct cci_devfreq *cci_df;
> > +	struct notifier_block *nb;
> > +
> > +	cci_df = dev_get_drvdata(devfreq->dev.parent);
> > +	nb = &cci_df->nb;
> > +
> > +	switch (event) {
> > +	case DEVFREQ_GOV_START:
> > +	case DEVFREQ_GOV_RESUME:
> > +		nb->notifier_call = cci_devfreq_regulator_notifier;
> > +		regulator_register_notifier(cci_df->proc_reg,
> > +					    nb);
> > +		break;
> > +
> > +	case DEVFREQ_GOV_STOP:
> > +	case DEVFREQ_GOV_SUSPEND:
> > +		regulator_unregister_notifier(cci_df->proc_reg,
> > +					    nb);
> > +
> > +		break;
> > +
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct devfreq_governor mtk_cci_devfreq_governor = {
> > +	.name = "mtk_cci_vmon",
> > +	.get_target_freq = mtk_cci_governor_get_target,
> > +	.event_handler = mtk_cci_governor_event_handler,
> > +};
> > +
> > +static int mtk_cci_devfreq_target(struct device *dev, unsigned long *freq,
> > +				  u32 flags)
> > +{
> > +	struct cci_devfreq *cci_df = dev_get_drvdata(dev);
> > +
> > +	if (!cci_df)
> > +		return -EINVAL;
> > +
> > +	clk_set_rate(cci_df->cci_clk, *freq);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct devfreq_dev_profile cci_devfreq_profile = {
> > +	.target		= mtk_cci_devfreq_target,
> > +};
> > +
> > +static int mtk_cci_devfreq_probe(struct platform_device *pdev)
> > +{
> > +	struct device *cci_dev = &pdev->dev;
> > +	struct cci_devfreq *cci_df;
> > +	int ret;
> > +
> > +	cci_df = devm_kzalloc(cci_dev, sizeof(*cci_df), GFP_KERNEL);
> > +	if (!cci_df)
> > +		return -ENOMEM;
> > +
> > +	cci_df->cci_clk = clk_get(cci_dev, "cci_clock");
> 
> Should use devm_clk_get().
I will change it on next patch.
> 
> > +	ret = PTR_ERR_OR_ZERO(cci_df->cci_clk);
> > +	if (ret) {
> > +		if (ret != -EPROBE_DEFER)
> > +			dev_err(cci_dev, "failed to get clock for CCI: %d\n",
> > +				ret);
> > +
> > +		return ret;
> > +	}
> > +
> > +	cci_df->proc_reg = regulator_get_optional(cci_dev, "proc");
> 
> Should use devm_regulator_get_optional().
I will change it on next patch.
> 
> > +	ret = PTR_ERR_OR_ZERO(cci_df->proc_reg);
> > +	if (ret) {
> > +		if (ret != -EPROBE_DEFER)
> > +			dev_err(cci_dev, "failed to get regulator for CCI: %d\n",
> > +				ret);
> > +
> > +		goto err_put_clk;
> > +	}
> > +
> > +	ret = dev_pm_opp_of_add_table(&pdev->dev);
> > +	if (ret) {
> > +		dev_err(cci_dev, "Fail to init CCI OPP table\n");
> 
> No error code display here ? Not that it really matters, but it is
> inconsistent with the other error messages.
I will add it on next patch.
> 
> > +		goto err_put_reg;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, cci_df);
> > +
> > +	cci_df->devfreq = devm_devfreq_add_device(cci_dev,
> > +						       &cci_devfreq_profile,
> > +						       "mtk_cci_vmon",
> > +						       NULL);
> > +	if (IS_ERR(cci_df->devfreq)) {
> 
> 		ret = PTR_ERR(cci_df->devfreq);
> 
> > +		dev_err(cci_dev, "cannot create cci devfreq device\n", ret);
> 
> 		dev_err(cci_dev, "cannot create cci devfreq device: %d\n", ret);
> 
> Something like
> 		dev_pm_opp_of_remove_table(...);
> seems to be missing here.
I will fix this, and add dev_pm_opp_of_remove_table(...) when error
occur.
> 
> > +		goto err_put_reg;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_put_reg:
> > +	regulator_put(cci_df->proc_reg);
> > +err_put_clk:
> > +	clk_put(cci_df->cci_clk);
> 
> Can be dropped when using devm_ functions above.
I will remove these code.
> 
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct of_device_id mediatek_cci_devfreq_of_match[] = {
> > +	{ .compatible = "mediatek,mt8183-cci" },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, mediatek_cci_devfreq_of_match);
> > +
> > +static struct platform_driver cci_devfreq_driver = {
> > +	.probe	= mtk_cci_devfreq_probe,
> > +	.driver = {
> > +		.name = "mediatek-cci-devfreq",
> > +		.of_match_table = mediatek_cci_devfreq_of_match,
> 
> If the driver depends on OF, that should be stated in the Kconfig file.
> Otherwise, of_match_ptr() should be used, and mediatek_cci_devfreq_of_match
> should be tagged with __maybe_unused (or made conditional with #ifdef).
I will fix this on next patch.
> 
> > +	},
> > +};
> > +
> > +static int __init mtk_cci_devfreq_init(void)
> > +{
> > +	int ret;
> > +
> > +	ret = devfreq_add_governor(&mtk_cci_devfreq_governor);
> > +	if (ret) {
> > +		pr_err("%s: failed to add governor: %d\n", __func__, ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = platform_driver_register(&cci_devfreq_driver);
> > +	if (ret)
> > +		devfreq_remove_governor(&mtk_cci_devfreq_governor);
> > +
> > +	return ret;
> > +}
> > +module_init(mtk_cci_devfreq_init)
> > +
> > +static void __exit mtk_cci_devfreq_exit(void)
> > +{
> > +	int ret;
> > +
> > +	ret = devfreq_remove_governor(&mtk_cci_devfreq_governor);
> > +	if (ret)
> > +		pr_err("%s: failed to remove governor: %d\n", __func__, ret);
> > +
> > +	platform_driver_unregister(&cci_devfreq_driver);
> > +}
> > +module_exit(mtk_cci_devfreq_exit)
> > +
> > +MODULE_DESCRIPTION("Mediatek CCI devfreq driver");
> > +MODULE_AUTHOR("Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>");
> > +MODULE_LICENSE("GPL v2");
> 
> _______________________________________________
> 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 v2 4/4] devfreq: add mediatek cci devfreq

[Chanwoo Choi]

Hi Andrew-sh.Cheng,

Please add the dt-binding documentation.

On 19. 3. 29. 오후 3:46, Andrew-sh.Cheng wrote:
> This adds a devfreq driver for the Cache Coherent Interconnect (CCI)
> of the Mediatek MT8183.
> 
> On the MT8183 the CCI is supplied by the same regulator as the LITTLE
> cores. The driver is notified when the regulator voltage changes
> (driven by cpufreq) and adjusts the CCI frequency to the maximum
> possible value.
> 
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> ---
>  drivers/devfreq/Kconfig              |  10 ++
>  drivers/devfreq/Makefile             |   1 +
>  drivers/devfreq/mt8183-cci-devfreq.c | 235 +++++++++++++++++++++++++++++++++++
>  3 files changed, 246 insertions(+)
>  create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 6a172d3..da2f8ec 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -91,6 +91,16 @@ config ARM_EXYNOS_BUS_DEVFREQ
>  	  and adjusts the operating frequencies and voltages with OPP support.
>  	  This does not yet operate with optimal voltages.
>  
> +config ARM_MT8183_CCI_DEVFREQ
> +	tristate "MT8183 CCI DEVFREQ Driver"
> +	depends on ARM_MEDIATEK_CPUFREQ
> +	help
> +		This adds a devfreq driver for Cache Coherent Interconnect
> +		of Mediatek MT8183, which is shared the same regulator
> +		with cpu cluster.
> +		It can track buck voltage and update a proper cci frequency.
> +		Use notification to get regulator status.
> +
>  config ARM_TEGRA_DEVFREQ
>  	tristate "Tegra DEVFREQ Driver"
>  	depends on ARCH_TEGRA_124_SOC
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 32b8d4d..817dde7 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
>  
>  # DEVFREQ Drivers
>  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
> +obj-$(CONFIG_ARM_MT8183_CCI_DEVFREQ)    += mt8183-cci-devfreq.o
>  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
>  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
>  
> diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c
> new file mode 100644
> index 0000000..af82d2e
> --- /dev/null
> +++ b/drivers/devfreq/mt8183-cci-devfreq.c
> @@ -0,0 +1,235 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 MediaTek Inc.

You can add the author information under copyright information.

> + */
> +
> +#include <linux/clk.h>
> +#include <linux/devfreq.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include "governor.h"
> +
> +struct cci_devfreq {
> +	struct devfreq		*devfreq;
> +	struct regulator	*proc_reg;
> +	unsigned long           proc_reg_uV;
> +	struct clk		*cci_clk;
> +	struct notifier_block	nb;

Just use the space instead of tab as following:

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

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

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

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

if -> else if

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


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

ditto. Please check the return value of update_devfreq.

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

ditto.
if -> else if

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


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

ditto. Please check the return value of update_devfreq.

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

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


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

ditto.

> +

Remove unneeded blank line.

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

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

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

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

	.immutable = true

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

Please check the return value of clk_set_rate().

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

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

	.target = mtk_cci_devfreq_target,

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

clk_get -> devm_clk_get()

> +	ret = PTR_ERR_OR_ZERO(cci_df->cci_clk);
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(cci_dev, "failed to get clock for CCI: %d\n",
> +				ret);
> +

Remove unneeded blank line.

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


regulator_get_optional -> devm_regulator_get_optional

> +	ret = PTR_ERR_OR_ZERO(cci_df->proc_reg);
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(cci_dev, "failed to get regulator for CCI: %d\n",
> +				ret);
> +

Remove unneeded blank line.

> +		goto err_put_clk;

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

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

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

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


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Chanwoo Choi]

Hi Andrew-sh.Cheng,

Please add the dt-binding documentation.

On 19. 3. 29. 오후 3:46, Andrew-sh.Cheng wrote:
> This adds a devfreq driver for the Cache Coherent Interconnect (CCI)
> of the Mediatek MT8183.
> 
> On the MT8183 the CCI is supplied by the same regulator as the LITTLE
> cores. The driver is notified when the regulator voltage changes
> (driven by cpufreq) and adjusts the CCI frequency to the maximum
> possible value.
> 
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> ---
>  drivers/devfreq/Kconfig              |  10 ++
>  drivers/devfreq/Makefile             |   1 +
>  drivers/devfreq/mt8183-cci-devfreq.c | 235 +++++++++++++++++++++++++++++++++++
>  3 files changed, 246 insertions(+)
>  create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 6a172d3..da2f8ec 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -91,6 +91,16 @@ config ARM_EXYNOS_BUS_DEVFREQ
>  	  and adjusts the operating frequencies and voltages with OPP support.
>  	  This does not yet operate with optimal voltages.
>  
> +config ARM_MT8183_CCI_DEVFREQ
> +	tristate "MT8183 CCI DEVFREQ Driver"
> +	depends on ARM_MEDIATEK_CPUFREQ
> +	help
> +		This adds a devfreq driver for Cache Coherent Interconnect
> +		of Mediatek MT8183, which is shared the same regulator
> +		with cpu cluster.
> +		It can track buck voltage and update a proper cci frequency.
> +		Use notification to get regulator status.
> +
>  config ARM_TEGRA_DEVFREQ
>  	tristate "Tegra DEVFREQ Driver"
>  	depends on ARCH_TEGRA_124_SOC
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 32b8d4d..817dde7 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
>  
>  # DEVFREQ Drivers
>  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
> +obj-$(CONFIG_ARM_MT8183_CCI_DEVFREQ)    += mt8183-cci-devfreq.o
>  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
>  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
>  
> diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c
> new file mode 100644
> index 0000000..af82d2e
> --- /dev/null
> +++ b/drivers/devfreq/mt8183-cci-devfreq.c
> @@ -0,0 +1,235 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 MediaTek Inc.

You can add the author information under copyright information.

> + */
> +
> +#include <linux/clk.h>
> +#include <linux/devfreq.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include "governor.h"
> +
> +struct cci_devfreq {
> +	struct devfreq		*devfreq;
> +	struct regulator	*proc_reg;
> +	unsigned long           proc_reg_uV;
> +	struct clk		*cci_clk;
> +	struct notifier_block	nb;

Just use the space instead of tab as following:

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

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

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

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

if -> else if

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


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

ditto. Please check the return value of update_devfreq.

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

ditto.
if -> else if

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


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

ditto. Please check the return value of update_devfreq.

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

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


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

ditto.

> +

Remove unneeded blank line.

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

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

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

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

	.immutable = true

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

Please check the return value of clk_set_rate().

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

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

	.target = mtk_cci_devfreq_target,

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

clk_get -> devm_clk_get()

> +	ret = PTR_ERR_OR_ZERO(cci_df->cci_clk);
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(cci_dev, "failed to get clock for CCI: %d\n",
> +				ret);
> +

Remove unneeded blank line.

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


regulator_get_optional -> devm_regulator_get_optional

> +	ret = PTR_ERR_OR_ZERO(cci_df->proc_reg);
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(cci_dev, "failed to get regulator for CCI: %d\n",
> +				ret);
> +

Remove unneeded blank line.

> +		goto err_put_clk;

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

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

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

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


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

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

[andrew-sh.cheng]

On Tue, 2019-04-16 at 18:05 +0900, Chanwoo Choi wrote:
> Hi Andrew-sh.Cheng,
> 
> Please add the dt-binding documentation.
> 
> On 19. 3. 29. 오후 3:46, Andrew-sh.Cheng wrote:
> > This adds a devfreq driver for the Cache Coherent Interconnect (CCI)
> > of the Mediatek MT8183.
> > 
> > On the MT8183 the CCI is supplied by the same regulator as the LITTLE
> > cores. The driver is notified when the regulator voltage changes
> > (driven by cpufreq) and adjusts the CCI frequency to the maximum
> > possible value.
> > 
> > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> > ---
> >  drivers/devfreq/Kconfig              |  10 ++
> >  drivers/devfreq/Makefile             |   1 +
> >  drivers/devfreq/mt8183-cci-devfreq.c | 235 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 246 insertions(+)
> >  create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
> > 
> > diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> > index 6a172d3..da2f8ec 100644
> > --- a/drivers/devfreq/Kconfig
> > +++ b/drivers/devfreq/Kconfig
> > @@ -91,6 +91,16 @@ config ARM_EXYNOS_BUS_DEVFREQ
> >  	  and adjusts the operating frequencies and voltages with OPP support.
> >  	  This does not yet operate with optimal voltages.
> >  
> > +config ARM_MT8183_CCI_DEVFREQ
> > +	tristate "MT8183 CCI DEVFREQ Driver"
> > +	depends on ARM_MEDIATEK_CPUFREQ
> > +	help
> > +		This adds a devfreq driver for Cache Coherent Interconnect
> > +		of Mediatek MT8183, which is shared the same regulator
> > +		with cpu cluster.
> > +		It can track buck voltage and update a proper cci frequency.
> > +		Use notification to get regulator status.
> > +
> >  config ARM_TEGRA_DEVFREQ
> >  	tristate "Tegra DEVFREQ Driver"
> >  	depends on ARCH_TEGRA_124_SOC
> > diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> > index 32b8d4d..817dde7 100644
> > --- a/drivers/devfreq/Makefile
> > +++ b/drivers/devfreq/Makefile
> > @@ -9,6 +9,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
> >  
> >  # DEVFREQ Drivers
> >  obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
> > +obj-$(CONFIG_ARM_MT8183_CCI_DEVFREQ)    += mt8183-cci-devfreq.o
> >  obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
> >  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
> >  
> > diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c
> > new file mode 100644
> > index 0000000..af82d2e
> > --- /dev/null
> > +++ b/drivers/devfreq/mt8183-cci-devfreq.c
> > @@ -0,0 +1,235 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2019 MediaTek Inc.
> 
> You can add the author information under copyright information.
ok~
> 
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/devfreq.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#include "governor.h"
> > +
> > +struct cci_devfreq {
> > +	struct devfreq		*devfreq;
> > +	struct regulator	*proc_reg;
> > +	unsigned long           proc_reg_uV;
> > +	struct clk		*cci_clk;
> > +	struct notifier_block	nb;
> 
> Just use the space instead of tab as following:
> 
> 	struct devfreq *devfreq;
> 	struct regulator *proc_reg;
> 	unsigned long proc_reg_uV;
> 	struct clk *cci_clk;
> 	struct notifier_block nb;
> 
OK, I will modify at next patch.
> > +};
> > +
> > +static int cci_devfreq_regulator_notifier(struct notifier_block *nb,
> > +					  unsigned long val, void *data)
> > +{
> > +	struct cci_devfreq *cci_df =
> > +		container_of(nb, struct cci_devfreq, nb);
> > +
> > +	/* deal with reduce frequency */
> > +	if (val & REGULATOR_EVENT_PRE_VOLTAGE_CHANGE) {
> > +		struct pre_voltage_change_data *pvc_data = data;
> > +
> > +		if (pvc_data->min_uV < pvc_data->old_uV) {
> > +			cci_df->proc_reg_uV =
> > +				(unsigned long)(pvc_data->min_uV);
> > +			mutex_lock(&cci_df->devfreq->lock);
> > +			update_devfreq(cci_df->devfreq);
> 
> Please handle the return value of update_devfreq() for exception handling.
OK, I will modify at next patch.
> 
> > +			mutex_unlock(&cci_df->devfreq->lock);
> > +		}
> > +	}
> > +
> > +	if ((val & REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE) &&
> 
> if -> else if
> 
> At the same time, receives only one notification.
> It is not necessary to check the two if statement always, .
OK, I will modify at next patch.
> 
> 
> > +	    ((unsigned long)data > cci_df->proc_reg_uV)) {
> > +		cci_df->proc_reg_uV = (unsigned long)data;
> > +		mutex_lock(&cci_df->devfreq->lock);
> > +		update_devfreq(cci_df->devfreq);
> 
> ditto. Please check the return value of update_devfreq.
OK, I will modify at next patch.
> 
> > +		mutex_unlock(&cci_df->devfreq->lock);
> > +	}
> > +
> > +	/* deal with increase frequency */
> > +	if ((val & REGULATOR_EVENT_VOLTAGE_CHANGE) &&
> 
> ditto.
> if -> else if
> 
> At the same time, receives only one notification.
> It is not necessary to check the two if statement always, .
OK, I will modify at next patch.
> 
> 
> > +	    (cci_df->proc_reg_uV < (unsigned long)data)) {
> > +		cci_df->proc_reg_uV = (unsigned long)data;
> > +		mutex_lock(&cci_df->devfreq->lock);
> > +		update_devfreq(cci_df->devfreq);
> 
> ditto. Please check the return value of update_devfreq.
OK, I will modify at next patch.
> 
> > +		mutex_unlock(&cci_df->devfreq->lock);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_cci_governor_get_target(struct devfreq *devfreq,
> > +				       unsigned long *freq)
> > +{
> > +	struct cci_devfreq *cci_df;
> > +	struct dev_pm_opp *opp;
> > +
> > +	cci_df = dev_get_drvdata(devfreq->dev.parent);
> > +
> > +	/* find available frequency */
> > +	opp = dev_pm_opp_find_max_freq_by_volt(devfreq->dev.parent,
> > +						 cci_df->proc_reg_uV);
> > +	*freq = dev_pm_opp_get_freq(opp);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_cci_governor_event_handler(struct devfreq *devfreq,
> > +					  unsigned int event, void *data)
> > +{
> > +	struct cci_devfreq *cci_df;
> > +	struct notifier_block *nb;
> > +
> > +	cci_df = dev_get_drvdata(devfreq->dev.parent);
> > +	nb = &cci_df->nb;
> > +
> > +	switch (event) {
> > +	case DEVFREQ_GOV_START:
> > +	case DEVFREQ_GOV_RESUME:
> > +		nb->notifier_call = cci_devfreq_regulator_notifier;
> > +		regulator_register_notifier(cci_df->proc_reg,
> > +					    nb);
> 
> Please check the return value of regulator_register_notifier
> in order to handle the exception handling.
OK, I will modify at next patch.
> 
> 
> > +		break;
> > +
> > +	case DEVFREQ_GOV_STOP:
> > +	case DEVFREQ_GOV_SUSPEND:
> > +		regulator_unregister_notifier(cci_df->proc_reg,
> > +					    nb);
> 
> ditto.
> 
> > +
> 
> Remove unneeded blank line.
OK, I will modify at next patch.
> 
> > +		break;
> > +
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct devfreq_governor mtk_cci_devfreq_governor = {
> > +	.name = "mtk_cci_vmon",
> 
> How about editing the name from mtk_cci_vmon to mediatek_cci_voltage_monitor
> for the readability? Actually, 'mtk' and 'vmon' are not standard expression.
The srting of name is only [16], I'll leave it as mtk_cci_vmon.
> 
> > +	.get_target_freq = mtk_cci_governor_get_target,
> > +	.event_handler = mtk_cci_governor_event_handler,
> 
> Please add following code to make it as the immutable
> because the governor for this driver will not be changed
> through sysfs interface.
> 
> 	.immutable = true
OK, I will modify at next patch.
> 
> > +};
> > +
> > +static int mtk_cci_devfreq_target(struct device *dev, unsigned long *freq,
> > +				  u32 flags)
> > +{
> > +	struct cci_devfreq *cci_df = dev_get_drvdata(dev);
> > +
> > +	if (!cci_df)
> > +		return -EINVAL;
> > +
> > +	clk_set_rate(cci_df->cci_clk, *freq);
> 
> Please check the return value of clk_set_rate().
OK, I will modify at next patch.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static struct devfreq_dev_profile cci_devfreq_profile = {
> > +	.target		= mtk_cci_devfreq_target,
> 
> Just use the space instead of tab because cci_devfreq_profile
> only has one record.
> 
> 	.target = mtk_cci_devfreq_target,
OK, I will modify at next patch.
> 
> > +};
> > +
> > +static int mtk_cci_devfreq_probe(struct platform_device *pdev)
> > +{
> > +	struct device *cci_dev = &pdev->dev;
> > +	struct cci_devfreq *cci_df;
> > +	int ret;
> > +
> > +	cci_df = devm_kzalloc(cci_dev, sizeof(*cci_df), GFP_KERNEL);
> > +	if (!cci_df)
> > +		return -ENOMEM;
> > +
> > +	cci_df->cci_clk = clk_get(cci_dev, "cci_clock");
> 
> clk_get -> devm_clk_get()
OK, I will modify at next patch.
> 
> > +	ret = PTR_ERR_OR_ZERO(cci_df->cci_clk);
> > +	if (ret) {
> > +		if (ret != -EPROBE_DEFER)
> > +			dev_err(cci_dev, "failed to get clock for CCI: %d\n",
> > +				ret);
> > +
> 
> Remove unneeded blank line.
> 
> > +		return ret;
> > +	}> +
> > +	cci_df->proc_reg = regulator_get_optional(cci_dev, "proc");
> 
> 
> regulator_get_optional -> devm_regulator_get_optional
OK, I will modify at next patch.
> 
> > +	ret = PTR_ERR_OR_ZERO(cci_df->proc_reg);
> > +	if (ret) {
> > +		if (ret != -EPROBE_DEFER)
> > +			dev_err(cci_dev, "failed to get regulator for CCI: %d\n",
> > +				ret);
> > +
> 
> Remove unneeded blank line.
> 
> > +		goto err_put_clk;
> 
> If you use devm_clk_get(), just return instead of goto.
OK, I will rewrite these code.
> 
> > +	}
> > +
> > +	ret = dev_pm_opp_of_add_table(&pdev->dev);
> > +	if (ret) {
> > +		dev_err(cci_dev, "Fail to init CCI OPP table\n");
> > +		goto err_put_reg;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, cci_df);
> > +
> > +	cci_df->devfreq = devm_devfreq_add_device(cci_dev,
> > +						       &cci_devfreq_profile,
> > +						       "mtk_cci_vmon",
> > +						       NULL);
> > +	if (IS_ERR(cci_df->devfreq)) {
> > +		dev_err(cci_dev, "cannot create cci devfreq device\n", ret);
> > +		goto err_put_reg;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_put_reg:
> > +	regulator_put(cci_df->proc_reg);
> > +err_put_clk:
> > +	clk_put(cci_df->cci_clk);
> 
> If you use devm_clk_get()/devm_regulator_get(),
> 'err_put_reg' and 'err_put_clk' are unneeded.
OK, I will rewrite these code.
> 
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct of_device_id mediatek_cci_devfreq_of_match[] = {
> > +	{ .compatible = "mediatek,mt8183-cci" },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, mediatek_cci_devfreq_of_match);
> > +
> > +static struct platform_driver cci_devfreq_driver = {
> > +	.probe	= mtk_cci_devfreq_probe,
> > +	.driver = {
> > +		.name = "mediatek-cci-devfreq",
> > +		.of_match_table = mediatek_cci_devfreq_of_match,
> > +	},
> > +};
> > +
> > +static int __init mtk_cci_devfreq_init(void)
> > +{
> > +	int ret;
> > +
> > +	ret = devfreq_add_governor(&mtk_cci_devfreq_governor);
> > +	if (ret) {
> > +		pr_err("%s: failed to add governor: %d\n", __func__, ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = platform_driver_register(&cci_devfreq_driver);
> > +	if (ret)
> > +		devfreq_remove_governor(&mtk_cci_devfreq_governor);
> > +
> > +	return ret;
> > +}
> > +module_init(mtk_cci_devfreq_init)
> > +
> > +static void __exit mtk_cci_devfreq_exit(void)
> > +{
> > +	int ret;
> > +
> > +	ret = devfreq_remove_governor(&mtk_cci_devfreq_governor);
> > +	if (ret)
> > +		pr_err("%s: failed to remove governor: %d\n", __func__, ret);
> > +
> > +	platform_driver_unregister(&cci_devfreq_driver);
> > +}
> > +module_exit(mtk_cci_devfreq_exit)
> > +
> > +MODULE_DESCRIPTION("Mediatek CCI devfreq driver");
> > +MODULE_AUTHOR("Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>");
> > +MODULE_LICENSE("GPL v2");
> > 
> 
> 



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

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

[Andrew-sh.Cheng]

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

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

[Andrew-sh.Cheng]

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

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

[Chanwoo Choi]

Hi,

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

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

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


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Chanwoo Choi]

Hi,

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

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

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


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

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

[andrew-sh.cheng]

On Tue, 2019-04-16 at 18:08 +0900, Chanwoo Choi wrote:
> Hi,
> 
> I already reviewed this patch on v1[1].
> [1] https://lkml.org/lkml/2019/2/11/2228
> 
> But, the second version patch doesn't include the anything
> about review. Please check it[1].
HI Chanwoo,
Sorry for this clumsy mistake.
I will update at patch v3
> 
> On 19. 3. 29. 오후 3:46, Andrew-sh.Cheng wrote:
> > This adds dt-binding documentation of cci devfreq
> > for Mediatek MT8183 SoC platform.
> > 
> > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> > ---
> >  .../bindings/devfreq/mt8183-cci-devfreq.txt           | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/devfreq/mt8183-cci-devfreq.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/devfreq/mt8183-cci-devfreq.txt b/Documentation/devicetree/bindings/devfreq/mt8183-cci-devfreq.txt
> > new file mode 100644
> > index 0000000..e2b61cf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/devfreq/mt8183-cci-devfreq.txt
> > @@ -0,0 +1,19 @@
> > +* Mediatek CCI frequency device
> > +
> > +Required properties:
> > +- compatible: should contain "mediatek,mt8183-cci" for cci devfreq
> > +
> > +- clocks: for cci devfreq
> > +
> > +- clock-names: for cci devfreq driver to reference
> > +
> > +- operating-points-v2: for cci devfreq opp table
> > +
> > +Example:
> > +	cci: cci {
> > +		compatible = "mediatek,cci";
> > +		clocks = <&apmixedsys CLK_APMIXED_CCIPLL>;
> > +		clock-names = "cci_clock";
> > +		operating-points-v2 = <&cci_opp>;
> > +	};
> > +
> > 
> 
> 



_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[andrew-sh.cheng]

On Tue, 2019-04-16 at 18:08 +0900, Chanwoo Choi wrote:
> Hi,
> 
> I already reviewed this patch on v1[1].
> [1] https://lkml.org/lkml/2019/2/11/2228
> 
> But, the second version patch doesn't include the anything
> about review. Please check it[1].
HI Chanwoo,
Sorry for this clumsy mistake.
I will update at patch v3
> 
> On 19. 3. 29. 오후 3:46, Andrew-sh.Cheng wrote:
> > This adds dt-binding documentation of cci devfreq
> > for Mediatek MT8183 SoC platform.
> > 
> > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> > ---
> >  .../bindings/devfreq/mt8183-cci-devfreq.txt           | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/devfreq/mt8183-cci-devfreq.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/devfreq/mt8183-cci-devfreq.txt b/Documentation/devicetree/bindings/devfreq/mt8183-cci-devfreq.txt
> > new file mode 100644
> > index 0000000..e2b61cf
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/devfreq/mt8183-cci-devfreq.txt
> > @@ -0,0 +1,19 @@
> > +* Mediatek CCI frequency device
> > +
> > +Required properties:
> > +- compatible: should contain "mediatek,mt8183-cci" for cci devfreq
> > +
> > +- clocks: for cci devfreq
> > +
> > +- clock-names: for cci devfreq driver to reference
> > +
> > +- operating-points-v2: for cci devfreq opp table
> > +
> > +Example:
> > +	cci: cci {
> > +		compatible = "mediatek,cci";
> > +		clocks = <&apmixedsys CLK_APMIXED_CCIPLL>;
> > +		clock-names = "cci_clock";
> > +		operating-points-v2 = <&cci_opp>;
> > +	};
> > +
> > 
> 
> 



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

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

[MyungJoo Ham]

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

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

Cheers,
MyungJoo

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

[MyungJoo Ham]

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

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

Cheers,
MyungJoo



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

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

[MyungJoo Ham]

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

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

Cheers,
MyungJoo

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

[andrew-sh.cheng]

On Mon, 2019-04-01 at 11:30 +0900, MyungJoo Ham wrote:
> >This API will get voltage as input parameter.
> >Search all opp items for the item which with max frequency,
> >and the voltae is smaller than provided voltage.
> >
> >Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> >---
> > drivers/opp/core.c     | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/pm_opp.h |  8 ++++++++
> > 2 files changed, 63 insertions(+)
> >
> >diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> >index 24c757a..57deef9 100644
> >--- a/include/linux/pm_opp.h
> >+++ b/include/linux/pm_opp.h
> >@@ -102,6 +102,8 @@ struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
> > 
> > struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
> > 					      unsigned long *freq);
> >+struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
> >+					      unsigned long u_volt);
> 
> For the symmetricity, wouldn't it be better to name it
> dev_pm_opp_find_volt_ceiling(dev, u_volt); ?
Yes.
Viresh has comment on this, too.
I will use dev_pm_opp_find_freq_ceil_by_volt() in next patch.

> 
> Cheers,
> MyungJoo
> 
> 
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[andrew-sh.cheng]

On Mon, 2019-04-01 at 11:30 +0900, MyungJoo Ham wrote:
> >This API will get voltage as input parameter.
> >Search all opp items for the item which with max frequency,
> >and the voltae is smaller than provided voltage.
> >
> >Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> >---
> > drivers/opp/core.c     | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/pm_opp.h |  8 ++++++++
> > 2 files changed, 63 insertions(+)
> >
> >diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> >index 24c757a..57deef9 100644
> >--- a/include/linux/pm_opp.h
> >+++ b/include/linux/pm_opp.h
> >@@ -102,6 +102,8 @@ struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
> > 
> > struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
> > 					      unsigned long *freq);
> >+struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
> >+					      unsigned long u_volt);
> 
> For the symmetricity, wouldn't it be better to name it
> dev_pm_opp_find_volt_ceiling(dev, u_volt); ?
Yes.
Viresh has comment on this, too.
I will use dev_pm_opp_find_freq_ceil_by_volt() in next patch.

> 
> Cheers,
> MyungJoo
> 
> 
> 
> _______________________________________________
> 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 v2 4/4] devfreq: add mediatek cci devfreq

[MyungJoo Ham]

>This adds a devfreq driver for the Cache Coherent Interconnect (CCI)
>of the Mediatek MT8183.
>
>On the MT8183 the CCI is supplied by the same regulator as the LITTLE
>cores. The driver is notified when the regulator voltage changes
>(driven by cpufreq) and adjusts the CCI frequency to the maximum
>possible value.
>
>Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
>---
> drivers/devfreq/Kconfig              |  10 ++
> drivers/devfreq/Makefile             |   1 +
> drivers/devfreq/mt8183-cci-devfreq.c | 235 +++++++++++++++++++++++++++++++++++
> 3 files changed, 246 insertions(+)
> create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
>

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

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

Cheers
MyungJoo

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

[MyungJoo Ham]

>This adds a devfreq driver for the Cache Coherent Interconnect (CCI)
>of the Mediatek MT8183.
>
>On the MT8183 the CCI is supplied by the same regulator as the LITTLE
>cores. The driver is notified when the regulator voltage changes
>(driven by cpufreq) and adjusts the CCI frequency to the maximum
>possible value.
>
>Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
>---
> drivers/devfreq/Kconfig              |  10 ++
> drivers/devfreq/Makefile             |   1 +
> drivers/devfreq/mt8183-cci-devfreq.c | 235 +++++++++++++++++++++++++++++++++++
> 3 files changed, 246 insertions(+)
> create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
>

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

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

Cheers
MyungJoo

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

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

[MyungJoo Ham]

>This adds a devfreq driver for the Cache Coherent Interconnect (CCI)
>of the Mediatek MT8183.
>
>On the MT8183 the CCI is supplied by the same regulator as the LITTLE
>cores. The driver is notified when the regulator voltage changes
>(driven by cpufreq) and adjusts the CCI frequency to the maximum
>possible value.
>
>Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
>---
> drivers/devfreq/Kconfig              |  10 ++
> drivers/devfreq/Makefile             |   1 +
> drivers/devfreq/mt8183-cci-devfreq.c | 235 +++++++++++++++++++++++++++++++++++
> 3 files changed, 246 insertions(+)
> create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
>

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

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

Cheers
MyungJoo

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

[andrew-sh.cheng]

On Mon, 2019-04-01 at 13:18 +0900, MyungJoo Ham wrote:
> >This adds a devfreq driver for the Cache Coherent Interconnect (CCI)
> >of the Mediatek MT8183.
> >
> >On the MT8183 the CCI is supplied by the same regulator as the LITTLE
> >cores. The driver is notified when the regulator voltage changes
> >(driven by cpufreq) and adjusts the CCI frequency to the maximum
> >possible value.
> >
> >Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> >---
> > drivers/devfreq/Kconfig              |  10 ++
> > drivers/devfreq/Makefile             |   1 +
> > drivers/devfreq/mt8183-cci-devfreq.c | 235 +++++++++++++++++++++++++++++++++++
> > 3 files changed, 246 insertions(+)
> > create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
> >
> 
> 1. It appears that proc_reg_uV might be used before initialization.
> It would be appropriate to initialize it at the probe function.
In this governor, cci only change frequency after get notification.
So it must set proc_reg_uV first, and then call update_devfreq() which
call to mtk_cci_governor_get_target() and use proc_reg_uV.
> 
> 2. Because you are already using OPP, why don't you rely on
> OPP fully? (use OPP from CPUFreq drivers as well in order to get
> OPP events automatically.) Anyway, this is a minor item that does
> not need to be corrected.
For some discuss about this in Mediatek before, we decide to put the
operation of CCI in CCI related driver and separated from CPUFreq.
> 
> Cheers
> MyungJoo
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

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

[andrew-sh.cheng]

On Mon, 2019-04-01 at 13:18 +0900, MyungJoo Ham wrote:
> >This adds a devfreq driver for the Cache Coherent Interconnect (CCI)
> >of the Mediatek MT8183.
> >
> >On the MT8183 the CCI is supplied by the same regulator as the LITTLE
> >cores. The driver is notified when the regulator voltage changes
> >(driven by cpufreq) and adjusts the CCI frequency to the maximum
> >possible value.
> >
> >Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> >---
> > drivers/devfreq/Kconfig              |  10 ++
> > drivers/devfreq/Makefile             |   1 +
> > drivers/devfreq/mt8183-cci-devfreq.c | 235 +++++++++++++++++++++++++++++++++++
> > 3 files changed, 246 insertions(+)
> > create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
> >
> 
> 1. It appears that proc_reg_uV might be used before initialization.
> It would be appropriate to initialize it at the probe function.
In this governor, cci only change frequency after get notification.
So it must set proc_reg_uV first, and then call update_devfreq() which
call to mtk_cci_governor_get_target() and use proc_reg_uV.
> 
> 2. Because you are already using OPP, why don't you rely on
> OPP fully? (use OPP from CPUFreq drivers as well in order to get
> OPP events automatically.) Anyway, this is a minor item that does
> not need to be corrected.
For some discuss about this in Mediatek before, we decide to put the
operation of CCI in CCI related driver and separated from CPUFreq.
> 
> Cheers
> MyungJoo
> 
> _______________________________________________
> 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