2019-03-31 00:08:06

by Nicolas Boichat

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

On Thu, Mar 28, 2019 at 11:46 PM Andrew-sh.Cheng
<[email protected]> 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 <[email protected]>
> ---
> 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-mediatek