| Message ID | 20220415055916.28350-12-rex-bc.chen@mediatek.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | cpufreq: mediatek: Cleanup and support MT8183 and MT8186 | expand |
Il 15/04/22 07:59, Rex-BC Chen ha scritto: > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > In some MediaTek SoCs, like MT8183, CPU and CCI share the same power > supplies. Cpufreq needs to check if CCI devfreq exists and wait until > CCI devfreq ready before scaling frequency. > > Before CCI devfreq is ready, we record the voltage when booting to > kernel and use the max(cpu target voltage, booting voltage) to > prevent cpufreq adjust to the lower voltage which will cause the CCI > crash because of high frequency and low voltage. > > - Add is_ccifreq_ready() to link CCI device to CPI, and CPU will start > DVFS when CCI is ready. > - Add platform data for MT8183. > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> I am enthusiast to see that the solution that I've proposed was welcome! I only have one nit on this patch, check below: > --- > drivers/cpufreq/mediatek-cpufreq.c | 80 +++++++++++++++++++++++++++++- > 1 file changed, 79 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > index d4c00237e862..dd3f739fede1 100644 > --- a/drivers/cpufreq/mediatek-cpufreq.c > +++ b/drivers/cpufreq/mediatek-cpufreq.c ..snip.. > @@ -225,6 +251,14 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > vproc = dev_pm_opp_get_voltage(opp); > dev_pm_opp_put(opp); > > + /* > + * If MediaTek cci is supported but is not ready, we will use the value > + * of max(target cpu voltage, booting voltage) to prevent high freqeuncy > + * low voltage crash. > + */ > + if (info->soc_data->ccifreq_supported && !is_ccifreq_ready(info)) > + vproc = max(vproc, info->vproc_on_boot); > + > /* > * If the new voltage or the intermediate voltage is higher than the > * current voltage, scale up voltage first. ..snip.. > @@ -423,6 +484,13 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) > if (ret) > goto out_disable_mux_clock; > > + info->vproc_on_boot = regulator_get_voltage(info->proc_reg); This result is used only if we use ccifreq, so this should be enclosed in an if condition: this will spare us some (yes, small) time on devices that don't use it. if (info->soc_data->ccifreq_supported) { info->vproc_on_boot = regulator_get_voltage(info->proc_reg); if (info->vproc_on_boot < 0) { dev_err(.... goto .. } } P.S.: While at it, since the maximum width is 100 columns, the dev_err() call fits, so don't break that line! After the requested change: Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
On Fri, 2022-04-15 at 14:24 +0200, AngeloGioacchino Del Regno wrote: > Il 15/04/22 07:59, Rex-BC Chen ha scritto: > > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > > > In some MediaTek SoCs, like MT8183, CPU and CCI share the same > > power > > supplies. Cpufreq needs to check if CCI devfreq exists and wait > > until > > CCI devfreq ready before scaling frequency. > > > > Before CCI devfreq is ready, we record the voltage when booting to > > kernel and use the max(cpu target voltage, booting voltage) to > > prevent cpufreq adjust to the lower voltage which will cause the > > CCI > > crash because of high frequency and low voltage. > > > > - Add is_ccifreq_ready() to link CCI device to CPI, and CPU will > > start > > DVFS when CCI is ready. > > - Add platform data for MT8183. > > > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > > I am enthusiast to see that the solution that I've proposed was > welcome! > > I only have one nit on this patch, check below: > Hello Angelo, Thanks for your advice for this modification. I also reply to Kevin and describe this solution. Let's wait for Kevin's feedback and other suggestion. > > --- > > drivers/cpufreq/mediatek-cpufreq.c | 80 > > +++++++++++++++++++++++++++++- > > 1 file changed, 79 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c > > b/drivers/cpufreq/mediatek-cpufreq.c > > index d4c00237e862..dd3f739fede1 100644 > > --- a/drivers/cpufreq/mediatek-cpufreq.c > > +++ b/drivers/cpufreq/mediatek-cpufreq.c > > ..snip.. > > > @@ -225,6 +251,14 @@ static int mtk_cpufreq_set_target(struct > > cpufreq_policy *policy, > > vproc = dev_pm_opp_get_voltage(opp); > > dev_pm_opp_put(opp); > > > > + /* > > + * If MediaTek cci is supported but is not ready, we will use > > the value > > + * of max(target cpu voltage, booting voltage) to prevent high > > freqeuncy > > + * low voltage crash. > > + */ > > + if (info->soc_data->ccifreq_supported && > > !is_ccifreq_ready(info)) > > + vproc = max(vproc, info->vproc_on_boot); > > + > > /* > > * If the new voltage or the intermediate voltage is higher > > than the > > * current voltage, scale up voltage first. > > ..snip.. > > > @@ -423,6 +484,13 @@ static int mtk_cpu_dvfs_info_init(struct > > mtk_cpu_dvfs_info *info, int cpu) > > if (ret) > > goto out_disable_mux_clock; > > > > + info->vproc_on_boot = regulator_get_voltage(info->proc_reg); > > This result is used only if we use ccifreq, so this should be > enclosed in an if > condition: this will spare us some (yes, small) time on devices that > don't use it. > > if (info->soc_data->ccifreq_supported) { > info->vproc_on_boot = regulator_get_voltage(info- > >proc_reg); > if (info->vproc_on_boot < 0) { > dev_err(.... > goto .. > } > } > > P.S.: While at it, since the maximum width is 100 columns, the > dev_err() call fits, > so don't break that line! > > After the requested change: > > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> I will add this in next version if there is no any suggestion for this patch. Thanks! BRs, Rex
Hi Rex, Rex-BC Chen <rex-bc.chen@mediatek.com> writes: > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > In some MediaTek SoCs, like MT8183, CPU and CCI share the same power > supplies. Cpufreq needs to check if CCI devfreq exists and wait until > CCI devfreq ready before scaling frequency. > > Before CCI devfreq is ready, we record the voltage when booting to > kernel and use the max(cpu target voltage, booting voltage) to > prevent cpufreq adjust to the lower voltage which will cause the CCI > crash because of high frequency and low voltage. > > - Add is_ccifreq_ready() to link CCI device to CPI, and CPU will start > DVFS when CCI is ready. > - Add platform data for MT8183. > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> The solution of keeping the max of the CPU voltage from OPP and boot-up voltage makes sense until CCI is ready. Thank you for the rework and the detailed technical explanations. Reviewed-by: Kevin Hilman <khilman@baylibre.com> Kevin
diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c index d4c00237e862..dd3f739fede1 100644 --- a/drivers/cpufreq/mediatek-cpufreq.c +++ b/drivers/cpufreq/mediatek-cpufreq.c @@ -22,6 +22,7 @@ struct mtk_cpufreq_platform_data { int proc_max_volt; int sram_min_volt; int sram_max_volt; + bool ccifreq_supported; }; /* @@ -38,6 +39,7 @@ struct mtk_cpufreq_platform_data { struct mtk_cpu_dvfs_info { struct cpumask cpus; struct device *cpu_dev; + struct device *cci_dev; struct regulator *proc_reg; struct regulator *sram_reg; struct clk *cpu_clk; @@ -45,6 +47,7 @@ struct mtk_cpu_dvfs_info { struct list_head list_head; int intermediate_voltage; bool need_voltage_tracking; + int vproc_on_boot; int pre_vproc; /* Avoid race condition for regulators between notify and policy */ struct mutex reg_lock; @@ -52,6 +55,7 @@ struct mtk_cpu_dvfs_info { int opp_cpu; unsigned long opp_freq; const struct mtk_cpufreq_platform_data *soc_data; + bool ccifreq_bound; }; static struct platform_device *cpufreq_pdev; @@ -188,6 +192,28 @@ static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info *info, int vproc) return ret; } +static bool is_ccifreq_ready(struct mtk_cpu_dvfs_info *info) +{ + struct device_link *sup_link; + + if (info->ccifreq_bound) + return true; + + sup_link = device_link_add(info->cpu_dev, info->cci_dev, + DL_FLAG_AUTOREMOVE_CONSUMER); + if (!sup_link) { + dev_err(info->cpu_dev, "cpu%d: sup_link is NULL\n", info->opp_cpu); + return false; + } + + if (sup_link->supplier->links.status != DL_DEV_DRIVER_BOUND) + return false; + + info->ccifreq_bound = true; + + return true; +} + static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index) { @@ -225,6 +251,14 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, vproc = dev_pm_opp_get_voltage(opp); dev_pm_opp_put(opp); + /* + * If MediaTek cci is supported but is not ready, we will use the value + * of max(target cpu voltage, booting voltage) to prevent high freqeuncy + * low voltage crash. + */ + if (info->soc_data->ccifreq_supported && !is_ccifreq_ready(info)) + vproc = max(vproc, info->vproc_on_boot); + /* * If the new voltage or the intermediate voltage is higher than the * current voltage, scale up voltage first. @@ -346,6 +380,23 @@ static int mtk_cpufreq_opp_notifier(struct notifier_block *nb, return notifier_from_errno(ret); } +static struct device *of_get_cci(struct device *cpu_dev) +{ + struct device_node *np; + struct platform_device *pdev; + + np = of_parse_phandle(cpu_dev->of_node, "mediatek,cci", 0); + if (IS_ERR_OR_NULL(np)) + return NULL; + + pdev = of_find_device_by_node(np); + of_node_put(np); + if (IS_ERR_OR_NULL(pdev)) + return NULL; + + return &pdev->dev; +} + static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) { struct device *cpu_dev; @@ -360,6 +411,16 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) } info->cpu_dev = cpu_dev; + info->ccifreq_bound = false; + if (info->soc_data->ccifreq_supported) { + info->cci_dev = of_get_cci(info->cpu_dev); + if (IS_ERR_OR_NULL(info->cci_dev)) { + ret = PTR_ERR(info->cci_dev); + dev_err(cpu_dev, "cpu%d: failed to get cci device\n", cpu); + return -ENODEV; + } + } + info->cpu_clk = clk_get(cpu_dev, "cpu"); if (IS_ERR(info->cpu_clk)) { ret = PTR_ERR(info->cpu_clk); @@ -423,6 +484,13 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) if (ret) goto out_disable_mux_clock; + info->vproc_on_boot = regulator_get_voltage(info->proc_reg); + if (info->vproc_on_boot < 0) { + dev_err(info->cpu_dev, + "invalid Vproc value: %d\n", info->vproc_on_boot); + goto out_disable_inter_clock; + } + /* Search a safe voltage for intermediate frequency. */ rate = clk_get_rate(info->inter_clk); opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate); @@ -623,6 +691,16 @@ static const struct mtk_cpufreq_platform_data mt2701_platform_data = { .proc_max_volt = 1150000, .sram_min_volt = 0, .sram_max_volt = 1150000, + .ccifreq_supported = false, +}; + +static const struct mtk_cpufreq_platform_data mt8183_platform_data = { + .min_volt_shift = 100000, + .max_volt_shift = 200000, + .proc_max_volt = 1150000, + .sram_min_volt = 0, + .sram_max_volt = 1150000, + .ccifreq_supported = true, }; /* List of machines supported by this driver */ @@ -635,7 +713,7 @@ static const struct of_device_id mtk_cpufreq_machines[] __initconst = { { .compatible = "mediatek,mt817x", .data = &mt2701_platform_data }, { .compatible = "mediatek,mt8173", .data = &mt2701_platform_data }, { .compatible = "mediatek,mt8176", .data = &mt2701_platform_data }, - { .compatible = "mediatek,mt8183", .data = &mt2701_platform_data }, + { .compatible = "mediatek,mt8183", .data = &mt8183_platform_data }, { .compatible = "mediatek,mt8365", .data = &mt2701_platform_data }, { .compatible = "mediatek,mt8516", .data = &mt2701_platform_data }, { }