lkml.org 
[lkml]   [2022]   [Apr]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH V3 11/15] cpufreq: mediatek: Link CCI device to CPU
    From
    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>

    \
     
     \ /
      Last update: 2022-04-15 14:26    [W:6.823 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site