| Message ID | 20220408045908.21671-1-rex-bc.chen@mediatek.com |
|---|---|
| Headers | show |
| Series | cpufreq: mediatek: Cleanup and support MT8183 and MT8186 | expand |
Il 08/04/22 06:58, Rex-BC Chen ha scritto: > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > Remove unused headers. > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 08/04/22 06:58, Rex-BC Chen ha scritto: > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > We found the buck voltage may not be exactly the same with what we set > because CPU may share the same buck with other module. > Therefore, we need to record the previous desired value instead of reading > it from regulators. > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > --- > drivers/cpufreq/mediatek-cpufreq.c | 31 +++++++++++++++++++----------- > 1 file changed, 20 insertions(+), 11 deletions(-) > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > index dc4a87e68940..472f4de29e5f 100644 > --- a/drivers/cpufreq/mediatek-cpufreq.c > +++ b/drivers/cpufreq/mediatek-cpufreq.c > @@ -40,6 +40,7 @@ struct mtk_cpu_dvfs_info { > struct list_head list_head; > int intermediate_voltage; > bool need_voltage_tracking; > + int old_vproc; > }; > > static LIST_HEAD(dvfs_info_list); > @@ -190,11 +191,17 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info, > > static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info *info, int vproc) > { > + int ret; > + > if (info->need_voltage_tracking) > - return mtk_cpufreq_voltage_tracking(info, vproc); > + ret = mtk_cpufreq_voltage_tracking(info, vproc); > else > - return regulator_set_voltage(info->proc_reg, vproc, > - vproc + VOLT_TOL); > + ret = regulator_set_voltage(info->proc_reg, vproc, > + MAX_VOLT_LIMIT); > + if (!ret) > + info->old_vproc = vproc; > + > + return ret; > } > > static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > @@ -211,15 +218,7 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > > inter_vproc = info->intermediate_voltage; > > - old_freq_hz = clk_get_rate(cpu_clk); > - old_vproc = regulator_get_voltage(info->proc_reg); > - if (old_vproc < 0) { > - pr_err("%s: invalid Vproc value: %d\n", __func__, old_vproc); > - return old_vproc; > - } > - > freq_hz = freq_table[index].frequency * 1000; > - > opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz); > if (IS_ERR(opp)) { > pr_err("cpu%d: failed to find OPP for %ld\n", > @@ -229,6 +228,16 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > vproc = dev_pm_opp_get_voltage(opp); > dev_pm_opp_put(opp); > > + old_freq_hz = clk_get_rate(cpu_clk); > + old_vproc = info->old_vproc; > + if (old_vproc == 0) > + old_vproc = regulator_get_voltage(info->proc_reg); > + if (old_vproc < 0) { > + dev_err(cpu_dev, "%s: invalid Vproc value: %d\n", > + __func__, old_vproc); > + return old_vproc; > + } From my understandment, if this fails once, it fails forever! info->old_vproc is set only if info->need_voltage_tracking is true, and only in mtk_cpufreq_set_voltage(): this function is called only after the checks that you've introduced there, and that's on previously stored values. While this was fine in the previous version, because it was always calling regulator_get_voltage(), here it's not. I think that a good option here is to: old_vproc = info->old_vproc; if (old_vproc <= 0) old_vproc = regulator_get_voltage(info->proc_reg); if (old_vproc < 0) { dev_err and return } ...or, if this is not applicable, we should still find another way to not let this driver to simply fail forever in case anything goes wrong. Regards, Angelo
Il 08/04/22 06:59, Rex-BC Chen ha scritto: > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > Voltages and shifts are defined as macros originally. > There are different requirements of these values for each MediaTek SoCs. > Therefore, we add the platform data and move these values into it. > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > --- > drivers/cpufreq/mediatek-cpufreq.c | 90 ++++++++++++++++++++---------- > 1 file changed, 61 insertions(+), 29 deletions(-) > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > index 1369da62780a..0be5609ee82e 100644 > --- a/drivers/cpufreq/mediatek-cpufreq.c > +++ b/drivers/cpufreq/mediatek-cpufreq.c ..snip.. > @@ -625,20 +649,27 @@ static struct platform_driver mtk_cpufreq_platdrv = { > .probe = mtk_cpufreq_probe, > }; > > +static const struct mtk_cpufreq_platform_data mtk_platform_data = { I would prefer if you name this after the oldest SoC that is supported in this driver, or after the IP version. Example: if the oldest one is mt7622, then this should be mt7622_platform_data; if newer SoCs have got a different cpu frequency control IP, then we should have here something like mtk_v1_platform_data, mtk_v1_3_platform_data. Please note that I didn't check release dates, so my reference to mt7622 is purely casual. Regards, Angelo
Il 08/04/22 06: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. > > - 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> > --- > drivers/cpufreq/mediatek-cpufreq.c | 69 +++++++++++++++++++++++++++++- > 1 file changed, 68 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > index b08ab7c14818..cebe5af2ef5d 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 is_ccifreq_support; bool ccifreq_supported; looks better. > }; > > /* > @@ -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; > @@ -52,6 +54,7 @@ struct mtk_cpu_dvfs_info { > int opp_cpu; > unsigned long opp_freq; > const struct mtk_cpufreq_platform_data *soc_data; > + bool is_ccifreq_bounded; bool ccifreq_bound; looks better. > }; > > static struct platform_device *cpufreq_pdev; > @@ -171,6 +174,29 @@ 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->is_ccifreq_bounded) > + 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); Please, don't break this line: 84 columns are ok. > + return false; > + } > + > + if (sup_link->supplier->links.status != DL_DEV_DRIVER_BOUND) > + return false; > + > + info->is_ccifreq_bounded = true; > + > + return true; > +} > + > static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > unsigned int index) > { > @@ -183,6 +209,9 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > long freq_hz, old_freq_hz; > int vproc, old_vproc, inter_vproc, target_vproc, ret; > > + if (info->soc_data->is_ccifreq_support && !is_ccifreq_ready(info)) > + return 0; Honestly, I think that pretending that everything is alright and faking set_target success is *not* a good idea... You should return -EAGAIN here, not zero. Regards, Angelo
Rex-BC Chen <rex-bc.chen@mediatek.com> writes: > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com> > > The Smart Voltage Scaling (SVS) is a hardware which calculates suitable > SVS bank voltages to OPP voltage table. > > When the SVS is enabled, cpufreq should listen to opp notification and do > proper actions when receiving events of disable and voltage adjustment. So listenting for OPP notifications should be done only when SVS is enabled... [...] > static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) > { > struct device *cpu_dev; > @@ -392,6 +455,17 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) > info->intermediate_voltage = dev_pm_opp_get_voltage(opp); > dev_pm_opp_put(opp); > > + info->opp_cpu = cpu; > + info->opp_nb.notifier_call = mtk_cpufreq_opp_notifier; > + ret = dev_pm_opp_register_notifier(cpu_dev, &info->opp_nb); ...but here youlisten to OPP notifications unconditionally. Seems there should be a check whether SVS is enabled before deciding to register. Kevin
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. > > - 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> The checks here are not enough, and will lead to unexpected behavior. IIUC, before doing DVFS, you're checking: 1) if the "cci" DT node is present and 2) if the driver for that device is bound If both those conditions are not met, you don't actually fail, you just silently do nothing in ->set_target(). As Angelo pointed out also, this is not a good idea, and will be rather confusing to users. The same thing would happen if the cci DT node was present, but the CCI devfreq driver was disabled. Silent failure would also be quite unexpected behavior. Similarily, if the cci DT node is not present at all (like it is in current upstream DT), this CPUfreq driver will silently do nothing. Not good. So, this patch needs to handle several scenarios: 1) CCI DT node not present In this case, the driver should still operate normally. With no CCI node, or driver there's no conflict. 2) CCI DT present/enabled but not yet bound In this case, you could return -EAGAIN as suggested by Angelo, or maybe better, it should do a deferred probe. 3) CCI DT present, but driver disabled This case is similar to (1), this driver should continue to work. Kevin
Rex-BC Chen <rex-bc.chen@mediatek.com> writes: > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > The platform data of MT8186 is different from previous MediaTek SoCs, > so we add a new compatible and platform data for it. > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> There's no upstream DT for MT8186, so I"m curious how this was tested/valiated with upstream? Kevin
On Sat, Apr 9, 2022 at 5:11 AM Kevin Hilman <khilman@baylibre.com> wrote: > > Rex-BC Chen <rex-bc.chen@mediatek.com> writes: > > > Cpufreq is a DVFS driver used for power saving to scale the clock frequency > > and supply the voltage for CPUs. This series do some cleanup for MediaTek > > cpufreq drivers and add support for MediaTek SVS[2] and MediaTek CCI > > devfreq[3] which are supported in MT8183 and MT8186. > > There's no upstream DT for MT8186 and there are no OPPs defined in the > upstream DT for MT8183. > > In order to test this on mainline, could you provide a patch for MT8183 > that adds OPPs to the DT so this can be tested with mainline? > The DT change used in the downstream kernel is from here: https://patchwork.kernel.org/project/linux-mediatek/patch/1616499241-4906-9-git-send-email-andrew-sh.cheng@mediatek.com/ Might need some update (eg. add the cci property in cpu) though. Rex, you can also include the 8183 DT change in the next version since most of the mt8183 dts are in the mainline. Thanks > Thanks, > > Kevin
On 08-04-22, 15:36, AngeloGioacchino Del Regno wrote: > Il 08/04/22 06:58, Rex-BC Chen ha scritto: > > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > > > Remove unused headers. > > > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Applied. Thanks.
On 08-04-22, 12:58, Rex-BC Chen wrote: > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > We found the buck voltage may not be exactly the same with what we set > because CPU may share the same buck with other module. > Therefore, we need to record the previous desired value instead of reading > it from regulators. > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > --- > drivers/cpufreq/mediatek-cpufreq.c | 31 +++++++++++++++++++----------- > 1 file changed, 20 insertions(+), 11 deletions(-) > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > index dc4a87e68940..472f4de29e5f 100644 > --- a/drivers/cpufreq/mediatek-cpufreq.c > +++ b/drivers/cpufreq/mediatek-cpufreq.c > @@ -40,6 +40,7 @@ struct mtk_cpu_dvfs_info { > struct list_head list_head; > int intermediate_voltage; > bool need_voltage_tracking; > + int old_vproc; I like prev_vproc better somehow, but it is up to you to name it :) > }; > > static LIST_HEAD(dvfs_info_list); > @@ -190,11 +191,17 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info, > > static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info *info, int vproc) > { > + int ret; > + > if (info->need_voltage_tracking) > - return mtk_cpufreq_voltage_tracking(info, vproc); > + ret = mtk_cpufreq_voltage_tracking(info, vproc); > else > - return regulator_set_voltage(info->proc_reg, vproc, > - vproc + VOLT_TOL); > + ret = regulator_set_voltage(info->proc_reg, vproc, > + MAX_VOLT_LIMIT); > + if (!ret) > + info->old_vproc = vproc; > + > + return ret; > } > > static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > @@ -211,15 +218,7 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > > inter_vproc = info->intermediate_voltage; > > - old_freq_hz = clk_get_rate(cpu_clk); > - old_vproc = regulator_get_voltage(info->proc_reg); > - if (old_vproc < 0) { > - pr_err("%s: invalid Vproc value: %d\n", __func__, old_vproc); > - return old_vproc; > - } > - Why did you move it down from here? I think it was fine to error out early if voltage isn't available. > freq_hz = freq_table[index].frequency * 1000; > - > opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz); > if (IS_ERR(opp)) { > pr_err("cpu%d: failed to find OPP for %ld\n", > @@ -229,6 +228,16 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > vproc = dev_pm_opp_get_voltage(opp); > dev_pm_opp_put(opp); > > + old_freq_hz = clk_get_rate(cpu_clk); > + old_vproc = info->old_vproc; > + if (old_vproc == 0) > + old_vproc = regulator_get_voltage(info->proc_reg); > + if (old_vproc < 0) { > + dev_err(cpu_dev, "%s: invalid Vproc value: %d\n", > + __func__, old_vproc); > + return old_vproc; > + } > + > /* > * If the new voltage or the intermediate voltage is higher than the > * current voltage, scale up voltage first. > -- > 2.18.0
On Fri, 2022-04-08 at 14:10 -0700, Kevin Hilman wrote: > Rex-BC Chen <rex-bc.chen@mediatek.com> writes: > > > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > > > The platform data of MT8186 is different from previous MediaTek > > SoCs, > > so we add a new compatible and platform data for it. > > > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > > There's no upstream DT for MT8186, so I"m curious how this was > tested/valiated with upstream? > > Kevin Hello Kevin, Thanks for your review. The series of MT8186 DTS is still upstreaming.
On Fri, 2022-04-08 at 15:36 +0200, AngeloGioacchino Del Regno wrote: > Il 08/04/22 06:59, Rex-BC Chen ha scritto: > > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > > > Voltages and shifts are defined as macros originally. > > There are different requirements of these values for each MediaTek > > SoCs. > > Therefore, we add the platform data and move these values into it. > > > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > > --- > > drivers/cpufreq/mediatek-cpufreq.c | 90 ++++++++++++++++++++----- > > ----- > > 1 file changed, 61 insertions(+), 29 deletions(-) > > > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c > > b/drivers/cpufreq/mediatek-cpufreq.c > > index 1369da62780a..0be5609ee82e 100644 > > --- a/drivers/cpufreq/mediatek-cpufreq.c > > +++ b/drivers/cpufreq/mediatek-cpufreq.c > > ..snip.. > > > @@ -625,20 +649,27 @@ static struct platform_driver > > mtk_cpufreq_platdrv = { > > .probe = mtk_cpufreq_probe, > > }; > > > > +static const struct mtk_cpufreq_platform_data mtk_platform_data = > > { > > I would prefer if you name this after the oldest SoC that is > supported in this > driver, or after the IP version. > > Example: if the oldest one is mt7622, then this should be > mt7622_platform_data; > if newer SoCs have got a different cpu frequency control > IP, then we > should have here something like > mtk_v1_platform_data, mtk_v1_3_platform_data. > > Please note that I didn't check release dates, so my reference to > mt7622 is > purely casual. > > Regards, > Angelo Hello Angelo, Thanks for your review. I will rename it as mt2701_platform_data. BRs, Rex
On Mon, 2022-04-11 at 08:56 +0530, Viresh Kumar wrote: > On 08-04-22, 12:58, Rex-BC Chen wrote: > > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > > > We found the buck voltage may not be exactly the same with what we > > set > > because CPU may share the same buck with other module. > > Therefore, we need to record the previous desired value instead of > > reading > > it from regulators. > > > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > --- > > drivers/cpufreq/mediatek-cpufreq.c | 31 +++++++++++++++++++------- > > ---- > > 1 file changed, 20 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c > > b/drivers/cpufreq/mediatek-cpufreq.c > > index dc4a87e68940..472f4de29e5f 100644 > > --- a/drivers/cpufreq/mediatek-cpufreq.c > > +++ b/drivers/cpufreq/mediatek-cpufreq.c > > @@ -40,6 +40,7 @@ struct mtk_cpu_dvfs_info { > > struct list_head list_head; > > int intermediate_voltage; > > bool need_voltage_tracking; > > + int old_vproc; > > I like prev_vproc better somehow, but it is up to you to name it :) Hello Viresh, Thanks for your review. I will modify this as prev_vproc in next version. > > > }; > > > > static LIST_HEAD(dvfs_info_list); > > @@ -190,11 +191,17 @@ static int > > mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info, > > > > static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info *info, > > int vproc) > > { > > + int ret; > > + > > if (info->need_voltage_tracking) > > - return mtk_cpufreq_voltage_tracking(info, vproc); > > + ret = mtk_cpufreq_voltage_tracking(info, vproc); > > else > > - return regulator_set_voltage(info->proc_reg, vproc, > > - vproc + VOLT_TOL); > > + ret = regulator_set_voltage(info->proc_reg, vproc, > > + MAX_VOLT_LIMIT); > > + if (!ret) > > + info->old_vproc = vproc; > > + > > + return ret; > > } > > > > static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > > @@ -211,15 +218,7 @@ static int mtk_cpufreq_set_target(struct > > cpufreq_policy *policy, > > > > inter_vproc = info->intermediate_voltage; > > > > - old_freq_hz = clk_get_rate(cpu_clk); > > - old_vproc = regulator_get_voltage(info->proc_reg); > > - if (old_vproc < 0) { > > - pr_err("%s: invalid Vproc value: %d\n", __func__, > > old_vproc); > > - return old_vproc; > > - } > > - > > Why did you move it down from here? I think it was fine to error out > early if voltage isn't available. I will move them to original position in next version. Thanks! BRs, Rex > > > freq_hz = freq_table[index].frequency * 1000; > > - > > opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz); > > if (IS_ERR(opp)) { > > pr_err("cpu%d: failed to find OPP for %ld\n", > > @@ -229,6 +228,16 @@ static int mtk_cpufreq_set_target(struct > > cpufreq_policy *policy, > > vproc = dev_pm_opp_get_voltage(opp); > > dev_pm_opp_put(opp); > > > > + old_freq_hz = clk_get_rate(cpu_clk); > > + old_vproc = info->old_vproc; > > + if (old_vproc == 0) > > + old_vproc = regulator_get_voltage(info->proc_reg); > > + if (old_vproc < 0) { > > + dev_err(cpu_dev, "%s: invalid Vproc value: %d\n", > > + __func__, old_vproc); > > + return old_vproc; > > + } > > + > > /* > > * If the new voltage or the intermediate voltage is higher > > than the > > * current voltage, scale up voltage first. > > -- > > 2.18.0 > >
On Fri, 2022-04-08 at 15:36 +0200, AngeloGioacchino Del Regno wrote: > Il 08/04/22 06:58, Rex-BC Chen ha scritto: > > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > > > We found the buck voltage may not be exactly the same with what we > > set > > because CPU may share the same buck with other module. > > Therefore, we need to record the previous desired value instead of > > reading > > it from regulators. > > > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > --- > > drivers/cpufreq/mediatek-cpufreq.c | 31 +++++++++++++++++++---- > > ------- > > 1 file changed, 20 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c > > b/drivers/cpufreq/mediatek-cpufreq.c > > index dc4a87e68940..472f4de29e5f 100644 > > --- a/drivers/cpufreq/mediatek-cpufreq.c > > +++ b/drivers/cpufreq/mediatek-cpufreq.c > > @@ -40,6 +40,7 @@ struct mtk_cpu_dvfs_info { > > struct list_head list_head; > > int intermediate_voltage; > > bool need_voltage_tracking; > > + int old_vproc; > > }; > > > > static LIST_HEAD(dvfs_info_list); > > @@ -190,11 +191,17 @@ static int > > mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info, > > > > static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info > > *info, int vproc) > > { > > + int ret; > > + > > if (info->need_voltage_tracking) > > - return mtk_cpufreq_voltage_tracking(info, vproc); > > + ret = mtk_cpufreq_voltage_tracking(info, vproc); > > else > > - return regulator_set_voltage(info->proc_reg, vproc, > > - vproc + VOLT_TOL); > > + ret = regulator_set_voltage(info->proc_reg, vproc, > > + MAX_VOLT_LIMIT); > > + if (!ret) > > + info->old_vproc = vproc; > > + > > + return ret; > > } > > > > static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > > @@ -211,15 +218,7 @@ static int mtk_cpufreq_set_target(struct > > cpufreq_policy *policy, > > > > inter_vproc = info->intermediate_voltage; > > > > - old_freq_hz = clk_get_rate(cpu_clk); > > - old_vproc = regulator_get_voltage(info->proc_reg); > > - if (old_vproc < 0) { > > - pr_err("%s: invalid Vproc value: %d\n", __func__, > > old_vproc); > > - return old_vproc; > > - } > > - > > freq_hz = freq_table[index].frequency * 1000; > > - > > opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz); > > if (IS_ERR(opp)) { > > pr_err("cpu%d: failed to find OPP for %ld\n", > > @@ -229,6 +228,16 @@ static int mtk_cpufreq_set_target(struct > > cpufreq_policy *policy, > > vproc = dev_pm_opp_get_voltage(opp); > > dev_pm_opp_put(opp); > > > > + old_freq_hz = clk_get_rate(cpu_clk); > > + old_vproc = info->old_vproc; > > + if (old_vproc == 0) > > + old_vproc = regulator_get_voltage(info->proc_reg); > > + if (old_vproc < 0) { > > + dev_err(cpu_dev, "%s: invalid Vproc value: %d\n", > > + __func__, old_vproc); > > + return old_vproc; > > + } > > From my understandment, if this fails once, it fails forever! > > info->old_vproc is set only if info->need_voltage_tracking is true, > and only > in mtk_cpufreq_set_voltage(): this function is called only after the > checks > that you've introduced there, and that's on previously stored values. > While this was fine in the previous version, because it was always > calling > regulator_get_voltage(), here it's not. > > I think that a good option here is to: > > old_vproc = info->old_vproc; > if (old_vproc <= 0) > old_vproc = regulator_get_voltage(info->proc_reg); > if (old_vproc < 0) { > dev_err and return > } > > ...or, if this is not applicable, we should still find another way to > not > let this driver to simply fail forever in case anything goes wrong. > > Regards, > Angelo Hello Angelo, Yes, your concern is right. I will add this in next version. if (old_vproc <= 0) old_vproc = regulator_get_voltage(info->proc_reg); if (old_vproc < 0) { dev_err and return } BRs, Rex
On Sat, 2022-04-09 at 09:05 +0800, Hsin-Yi Wang wrote: > On Sat, Apr 9, 2022 at 5:11 AM Kevin Hilman <khilman@baylibre.com> > wrote: > > > > Rex-BC Chen <rex-bc.chen@mediatek.com> writes: > > > > > Cpufreq is a DVFS driver used for power saving to scale the clock > > > frequency > > > and supply the voltage for CPUs. This series do some cleanup for > > > MediaTek > > > cpufreq drivers and add support for MediaTek SVS[2] and MediaTek > > > CCI > > > devfreq[3] which are supported in MT8183 and MT8186. > > > > There's no upstream DT for MT8186 and there are no OPPs defined in > > the > > upstream DT for MT8183. > > > > In order to test this on mainline, could you provide a patch for > > MT8183 > > that adds OPPs to the DT so this can be tested with mainline? > > > > The DT change used in the downstream kernel is from here: > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/1616499241-4906-9-git-send-email-andrew-sh.cheng@mediatek.com/__;!!CTRNKA9wMg0ARbw!ykRlVJPl8TZWMCfnAzLnqhtn3iqXeHh8f6tMBWpneZuJPPmJTGEDIiEgv-R_Q4gVunnp$ > > Might need some update (eg. add the cci property in cpu) though. > Rex, you can also include the 8183 DT change in the next version > since > most of the mt8183 dts are in the mainline. > > Thanks > > > Thanks, > > > > Kevin Hello Kevin and Hsinyi, OK I will add dts part of cpufreq for MT8183 in next version. And I think the cci part will be upstreamed in cci seriues. BRs, Rex
On Fri, 2022-04-08 at 15:37 +0200, AngeloGioacchino Del Regno wrote: > Il 08/04/22 06: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. > > > > - 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> > > --- > > drivers/cpufreq/mediatek-cpufreq.c | 69 > > +++++++++++++++++++++++++++++- > > 1 file changed, 68 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c > > b/drivers/cpufreq/mediatek-cpufreq.c > > index b08ab7c14818..cebe5af2ef5d 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 is_ccifreq_support; > > bool ccifreq_supported; looks better. Hello Angelo, Thanks for your review. OK, I will modify this in next version. > > > }; > > > > /* > > @@ -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; > > @@ -52,6 +54,7 @@ struct mtk_cpu_dvfs_info { > > int opp_cpu; > > unsigned long opp_freq; > > const struct mtk_cpufreq_platform_data *soc_data; > > + bool is_ccifreq_bounded; > > bool ccifreq_bound; looks better. > OK, I will modify this in next version. > > }; > > > > static struct platform_device *cpufreq_pdev; > > @@ -171,6 +174,29 @@ 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->is_ccifreq_bounded) > > + 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); > > Please, don't break this line: 84 columns are ok. > OK, I will modify this in next version. > > + return false; > > + } > > + > > + if (sup_link->supplier->links.status != DL_DEV_DRIVER_BOUND) > > + return false; > > + > > + info->is_ccifreq_bounded = true; > > + > > + return true; > > +} > > + > > static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > > unsigned int index) > > { > > @@ -183,6 +209,9 @@ static int mtk_cpufreq_set_target(struct > > cpufreq_policy *policy, > > long freq_hz, old_freq_hz; > > int vproc, old_vproc, inter_vproc, target_vproc, ret; > > > > + if (info->soc_data->is_ccifreq_support && > > !is_ccifreq_ready(info)) > > + return 0; > > Honestly, I think that pretending that everything is alright and > faking > set_target success is *not* a good idea... > > You should return -EAGAIN here, not zero. > > Regards, > Angelo > As metioneded by Kevin, I will review these three situations. Thanks for your suggestion. BRs, Rex
On Fri, 2022-04-08 at 13:54 -0700, Kevin Hilman wrote: > 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. > > > > - 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> > > The checks here are not enough, and will lead to unexpected behavior. > IIUC, before doing DVFS, you're checking: > > 1) if the "cci" DT node is present and > 2) if the driver for that device is bound > > If both those conditions are not met, you don't actually fail, you > just > silently do nothing in ->set_target(). As Angelo pointed out also, > this > is not a good idea, and will be rather confusing to users. > > The same thing would happen if the cci DT node was present, but the > CCI > devfreq driver was disabled. Silent failure would also be quite > unexpected behavior. Similarily, if the cci DT node is not present > at > all (like it is in current upstream DT), this CPUfreq driver will > silently do nothing. Not good. > > So, this patch needs to handle several scenarios: > > 1) CCI DT node not present > > In this case, the driver should still operate normally. With no CCI > node, or driver there's no conflict. > > 2) CCI DT present/enabled but not yet bound > > In this case, you could return -EAGAIN as suggested by Angelo, or > maybe > better, it should do a deferred probe. > > 3) CCI DT present, but driver disabled > > This case is similar to (1), this driver should continue to work. > > Kevin Hello Kevin, Thanks for your review. I will review these three situations and do some modification. BRs, Rex
On Fri, 2022-04-08 at 13:54 -0700, Kevin Hilman wrote: > 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. > > > > - 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> > > The checks here are not enough, and will lead to unexpected behavior. > IIUC, before doing DVFS, you're checking: > > 1) if the "cci" DT node is present and > 2) if the driver for that device is bound > > If both those conditions are not met, you don't actually fail, you > just > silently do nothing in ->set_target(). As Angelo pointed out also, > this > is not a good idea, and will be rather confusing to users. > > The same thing would happen if the cci DT node was present, but the > CCI > devfreq driver was disabled. Silent failure would also be quite > unexpected behavior. Similarily, if the cci DT node is not present > at > all (like it is in current upstream DT), this CPUfreq driver will > silently do nothing. Not good. > > So, this patch needs to handle several scenarios: > > 1) CCI DT node not present > > In this case, the driver should still operate normally. With no CCI > node, or driver there's no conflict. > > 2) CCI DT present/enabled but not yet bound > > In this case, you could return -EAGAIN as suggested by Angelo, or > maybe > better, it should do a deferred probe. > > 3) CCI DT present, but driver disabled > > This case is similar to (1), this driver should continue to work. > > Kevin Hello Kevin and Angelo, In my review, if we do not get the link or the link status is not correct between cci and cpufreq in target_index, I think it will never established again for this link. Because it's not checked in probe stage. So I think we just need to deal with the issue without cci device, and don't expect the link between cci and cpufreq will be connected again. If I am wrong, please correct me. Thanks. BRs, Rex
Hi Rex, Rex-BC Chen <rex-bc.chen@mediatek.com> writes: > On Fri, 2022-04-08 at 13:29 -0700, Kevin Hilman wrote: >> Rex-BC Chen <rex-bc.chen@mediatek.com> writes: >> >> > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com> >> > >> > The Smart Voltage Scaling (SVS) is a hardware which calculates >> > suitable >> > SVS bank voltages to OPP voltage table. >> > >> > When the SVS is enabled, cpufreq should listen to opp notification >> > and do >> > proper actions when receiving events of disable and voltage >> > adjustment. >> >> So listenting for OPP notifications should be done only when SVS is >> enabled... >> > > Thanks for your review. > Yes, the OPP notification is only called from MediaTek SVS. > >> [...] >> >> > static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, >> > int cpu) >> > { >> > struct device *cpu_dev; >> > @@ -392,6 +455,17 @@ static int mtk_cpu_dvfs_info_init(struct >> > mtk_cpu_dvfs_info *info, int cpu) >> > info->intermediate_voltage = dev_pm_opp_get_voltage(opp); >> > dev_pm_opp_put(opp); >> > >> > + info->opp_cpu = cpu; >> > + info->opp_nb.notifier_call = mtk_cpufreq_opp_notifier; >> > + ret = dev_pm_opp_register_notifier(cpu_dev, &info->opp_nb); >> >> ...but here youlisten to OPP notifications unconditionally. Seems >> there >> should be a check whether SVS is enabled before deciding to register. >> >> Kevin >> > Do you think it's ok that we wrap it with the SVS Kconfig define? > like > #ifdef CONFIG_MTK_SVS > mtk_cpufreq_opp_notifier() > ... > dev_pm_opp_register_notifier() > #endif Generally, we don't like to see #ifdefs in C files[1]. But more importantly, compile-time check is not enough, because SVS feature could be compiled into kernel, but not actually enabled for an SoC (e.g. DT node not enabled, etc.) so checking this at compile time is not enough. Ideally, the SVSdriver should provide a function that allows others to check if it's enabled. That function needs to know not only if it's compile in, but if it's enabled/running. If SVS is not compiled in, then that function just returns false. Kevin [1] https://www.kernel.org/doc/html/latest/process/4.Coding.html?highlight=ifdef#ifdef-and-preprocessor-use-in-general
Rex-BC Chen <rex-bc.chen@mediatek.com> writes: > On Fri, 2022-04-08 at 13:54 -0700, Kevin Hilman wrote: >> 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. >> > >> > - 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> >> >> The checks here are not enough, and will lead to unexpected behavior. >> IIUC, before doing DVFS, you're checking: >> >> 1) if the "cci" DT node is present and >> 2) if the driver for that device is bound >> >> If both those conditions are not met, you don't actually fail, you >> just >> silently do nothing in ->set_target(). As Angelo pointed out also, >> this >> is not a good idea, and will be rather confusing to users. >> >> The same thing would happen if the cci DT node was present, but the >> CCI >> devfreq driver was disabled. Silent failure would also be quite >> unexpected behavior. Similarily, if the cci DT node is not present >> at >> all (like it is in current upstream DT), this CPUfreq driver will >> silently do nothing. Not good. >> >> So, this patch needs to handle several scenarios: >> >> 1) CCI DT node not present >> >> In this case, the driver should still operate normally. With no CCI >> node, or driver there's no conflict. >> >> 2) CCI DT present/enabled but not yet bound >> >> In this case, you could return -EAGAIN as suggested by Angelo, or >> maybe >> better, it should do a deferred probe. >> >> 3) CCI DT present, but driver disabled >> >> This case is similar to (1), this driver should continue to work. >> >> Kevin > > Hello Kevin and Angelo, > > In my review, if we do not get the link or the link status is not > correct between cci and cpufreq in target_index, I think it will never > established again for this link. > Because it's not checked in probe stage. > > So I think we just need to deal with the issue without cci device, and > don't expect the link between cci and cpufreq will be connected again. > > If I am wrong, please correct me. I don't fully understand your questions, but I think what your getting at suggest that you might need to use deferred probe to handle the case where the ordering of CCI and cpufreq probing is not predictable. Kevin
On Mon, 2022-04-11 at 11:13 -0700, Kevin Hilman wrote: > Rex-BC Chen <rex-bc.chen@mediatek.com> writes: > > > On Fri, 2022-04-08 at 13:54 -0700, Kevin Hilman wrote: > > > 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. > > > > > > > > - 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> > > > > > > The checks here are not enough, and will lead to unexpected > > > behavior. > > > IIUC, before doing DVFS, you're checking: > > > > > > 1) if the "cci" DT node is present and > > > 2) if the driver for that device is bound > > > > > > If both those conditions are not met, you don't actually fail, > > > you > > > just > > > silently do nothing in ->set_target(). As Angelo pointed out > > > also, > > > this > > > is not a good idea, and will be rather confusing to users. > > > > > > The same thing would happen if the cci DT node was present, but > > > the > > > CCI > > > devfreq driver was disabled. Silent failure would also be quite > > > unexpected behavior. Similarily, if the cci DT node is not > > > present > > > at > > > all (like it is in current upstream DT), this CPUfreq driver will > > > silently do nothing. Not good. > > > > > > So, this patch needs to handle several scenarios: > > > > > > 1) CCI DT node not present > > > > > > In this case, the driver should still operate normally. With no > > > CCI > > > node, or driver there's no conflict. > > > > > > 2) CCI DT present/enabled but not yet bound > > > > > > In this case, you could return -EAGAIN as suggested by Angelo, or > > > maybe > > > better, it should do a deferred probe. > > > > > > 3) CCI DT present, but driver disabled > > > > > > This case is similar to (1), this driver should continue to work. > > > > > > Kevin > > > > Hello Kevin and Angelo, > > > > In my review, if we do not get the link or the link status is not > > correct between cci and cpufreq in target_index, I think it will > > never > > established again for this link. > > Because it's not checked in probe stage. > > > > So I think we just need to deal with the issue without cci device, > > and > > don't expect the link between cci and cpufreq will be connected > > again. > > > > If I am wrong, please correct me. > > I don't fully understand your questions, but I think what your > getting > at suggest that you might need to use deferred probe to handle the > case > where the ordering of CCI and cpufreq probing is not predictable. > > Kevin Hello Kevin and Angelo, I can summary what I got now: 1. Why we need cci for cpufreq in MT8183 and MT8186: a. CCI is a mediatek hw module. b. For mediatek SoCs before MT8183, like MT8173, the CCI hw is not introduced. c. The frequency for cci and cpufreq are determined could be configed at bootloader stage, so the frequency when entering kernel is unknown. d. Cpu little core and cci are using the same regulator. e. If we do not control CCI and just adjust the voltage in cpufreq driver. When we adjust the voltage smaller because we need to reduce the frequency, the CCI could run in high frequency which is set in bootloader. This will cause some problem, the cci could crash. Use MT8186 for a example, the bootloader set cci freq as 1.385GHz and cpufreq as 2GHz. If entering kernel and we only adjust the cpufreq voltage, if the cpufreq is under 1.618GHz, the cci will be out of spec. You can refer to the chrome project bootloader "Coreboot" patch: https://review.coreboot.org/c/coreboot/+/59569/2/src/mainboard/google/corsola/romstage.c f. If cpufreq driver wait cci ready, regulator framework will take the highest voltage requests from cci and cpufreq as output so that it prevents from high freqeuncy low voltage crash. d. Therefore, I think it's not a good idea to bypass cci device if the ccifreq_supported is true in MT8183 and MT8186. 2. Check the device link status is DL_DEV_DRIVER_BOUND is used for promising the cci is probed done. 3. About the cpufreq_driver->target_index a. When I trace the common drivers, I found if the return value is not zero, it will be BUG_ON. ref: https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1471 b. I also try to move is_ccifreq_ready() to other place, like cpufreq_driver->init and cpufreq probe function. There will be new issue. Do you have any suggetion that we can retern value of DEFER_PROBE? BRs, Rex
Rex-BC Chen <rex-bc.chen@mediatek.com> writes: > On Mon, 2022-04-11 at 11:09 -0700, Kevin Hilman wrote: >> Hi Rex, >> >> Rex-BC Chen <rex-bc.chen@mediatek.com> writes: >> >> > On Fri, 2022-04-08 at 13:29 -0700, Kevin Hilman wrote: >> > > Rex-BC Chen <rex-bc.chen@mediatek.com> writes: >> > > >> > > > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com> >> > > > >> > > > The Smart Voltage Scaling (SVS) is a hardware which calculates >> > > > suitable >> > > > SVS bank voltages to OPP voltage table. >> > > > >> > > > When the SVS is enabled, cpufreq should listen to opp >> > > > notification >> > > > and do >> > > > proper actions when receiving events of disable and voltage >> > > > adjustment. >> > > >> > > So listenting for OPP notifications should be done only when SVS >> > > is >> > > enabled... >> > > >> > >> > Thanks for your review. >> > Yes, the OPP notification is only called from MediaTek SVS. >> > >> > > [...] >> > > >> > > > static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info >> > > > *info, >> > > > int cpu) >> > > > { >> > > > struct device *cpu_dev; >> > > > @@ -392,6 +455,17 @@ static int mtk_cpu_dvfs_info_init(struct >> > > > mtk_cpu_dvfs_info *info, int cpu) >> > > > info->intermediate_voltage = >> > > > dev_pm_opp_get_voltage(opp); >> > > > dev_pm_opp_put(opp); >> > > > >> > > > + info->opp_cpu = cpu; >> > > > + info->opp_nb.notifier_call = mtk_cpufreq_opp_notifier; >> > > > + ret = dev_pm_opp_register_notifier(cpu_dev, &info- >> > > > >opp_nb); >> > > >> > > ...but here youlisten to OPP notifications >> > > unconditionally. Seems >> > > there >> > > should be a check whether SVS is enabled before deciding to >> > > register. >> > > >> > > Kevin >> > > >> > >> > Do you think it's ok that we wrap it with the SVS Kconfig define? >> > like >> > #ifdef CONFIG_MTK_SVS >> > mtk_cpufreq_opp_notifier() >> > ... >> > dev_pm_opp_register_notifier() >> > #endif >> >> Generally, we don't like to see #ifdefs in C files[1]. >> >> But more importantly, compile-time check is not enough, because SVS >> feature could be compiled into kernel, but not actually enabled for >> an >> SoC (e.g. DT node not enabled, etc.) so checking this at compile time >> is >> not enough. >> >> Ideally, the SVSdriver should provide a function that allows others >> to >> check if it's enabled. That function needs to know not only if it's >> compile in, but if it's enabled/running. If SVS is not compiled in, >> then that function just returns false. >> >> Kevin >> >> [1] >> https://urldefense.com/v3/__https://www.kernel.org/doc/html/latest/process/4.Coding.html?highlight=ifdef*ifdef-and-preprocessor-use-in-general__;Iw!!CTRNKA9wMg0ARbw!z6SrEcQOLu2u-R1OLedrRUXHYXCzuQoK3F_h9Bhzv8jNFmjV5mdNVy41eND67CuV9uIS$ >> > > Hello Kevin, > > After our internal discussion, we think the register of notifier should > not be bound for certain module. > If we provide the moethod to adjust voltage/disable opp, we think if > anyone call dev_pm_opp_adjust_voltage and dev_pm_opp_disable, it could > be used. I don't think I understand what you mean. Do you mean that this OPP notifier could be registered all the time, even if SVS is not enabled? That's fine with me. If SVS is not compiled or enabled, then the notifiers will never be called, so that's fine. > May I ask what is your concern? My concern was primarily that the changelog description did not match the code. The changelog says "when SVS is enabled, CPUfreq should listen to OPP notifications." But if I understand you correctly above, I think what you mean is that CPUfreq should always listen to OPP notifications because there are other users (e.g. SVS) that could change the OPP outside of this driver. If that's what you mean, then I think the only thing to change is the wording of the changelog. Thanks, Kevin
Rex-BC Chen <rex-bc.chen@mediatek.com> writes: [...] > I can summary what I got now: > > 1. Why we need cci for cpufreq in MT8183 and MT8186: > a. CCI is a mediatek hw module. > b. For mediatek SoCs before MT8183, like MT8173, the CCI hw > is not introduced. > c. The frequency for cci and cpufreq are determined could > be configed at bootloader stage, so the frequency when > entering kernel is unknown. > d. Cpu little core and cci are using the same regulator. > e. If we do not control CCI and just adjust the voltage in > cpufreq driver. > When we adjust the voltage smaller because we need to reduce > the frequency, the CCI could run in high frequency which is > set in bootloader. > This will cause some problem, the cci could crash. > > Use MT8186 for a example, the bootloader set cci freq as > 1.385GHz and cpufreq as 2GHz. > If entering kernel and we only adjust the cpufreq voltage, if > the cpufreq is under 1.618GHz, the cci will be out of spec. > > f. If cpufreq driver wait cci ready, regulator framework will take > the highest voltage requests from cci and cpufreq as output > so that it prevents from high freqeuncy low voltage crash. > > d. Therefore, I think it's not a good idea to bypass cci device if > the ccifreq_supported is true in MT8183 and MT8186. I do not propose to bypass CCI device. What both Angelo and I are saying is just that you need a better way to handle the cases when CCI is not (yet) enabled. The current way in the propsed patch is not good enough. I fully understand the potential problems with high frequency & low voltage when using a shared regulator. But, I think the problem we're trying to solve here is specific to the initial boot of the platform, while we are waiting for the CCI driver to be loaded. The root of the problem is that the CCI bus has constraints on the voltage regulator that are not defined anywhere until the CCI driver is loaded. So to fix that, you need to either: 1) not allow any voltage changes 2) register the CCI device constraints In the current patch, you attempt to do (1). There's nothing wrong with the idea, we just pointed out problems in your implementation, especially the fact that it does nothing, but it "succeeds" so the CPUfreq framework will think the OPPs are different from what they actually are. Just an idea, but another option could be (2). While waiting for the CCI device to be ready, the CPUfreq driver could check the current CCI freq/voltage and set min/max constraints on the regulator that prevent CCI from breaking. These constraints would stay in place until the CCI driver is ready. Once the real CCI driver is ready/registerd these contraints would be removed. Another version of this same idea would be to check the CCI freq/voltage and then limit the OPPs available to CPUfreq to only the ones that would not break CCI. Then, when CCI is ready/registered, you remove the limits. > 2. Check the device link status is DL_DEV_DRIVER_BOUND is used for > promising the cci is probed done. > > 3. About the cpufreq_driver->target_index > a. When I trace the common drivers, I found if the return value is > not zero, it will be BUG_ON. > ref: > https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1471 Right, you should not try to do deferred probe in the ->set_target() callback. Deferred probe is meant for init/probe time. > b. I also try to move is_ccifreq_ready() to other place, like > cpufreq_driver->init and cpufreq probe function. > There will be new issue. Do you have any suggetion that we can > retern value of DEFER_PROBE? The only appropriate place is in the probe function. Kevin
On Tue, 2022-04-12 at 11:50 -0700, Kevin Hilman wrote: > Rex-BC Chen <rex-bc.chen@mediatek.com> writes: > > [...] > > > I can summary what I got now: > > > > 1. Why we need cci for cpufreq in MT8183 and MT8186: > > a. CCI is a mediatek hw module. > > b. For mediatek SoCs before MT8183, like MT8173, the CCI hw > > is not introduced. > > c. The frequency for cci and cpufreq are determined could > > be configed at bootloader stage, so the frequency when > > entering kernel is unknown. > > d. Cpu little core and cci are using the same regulator. > > e. If we do not control CCI and just adjust the voltage in > > cpufreq driver. > > When we adjust the voltage smaller because we need to reduce > > the frequency, the CCI could run in high frequency which is > > set in bootloader. > > This will cause some problem, the cci could crash. > > > > Use MT8186 for a example, the bootloader set cci freq as > > 1.385GHz and cpufreq as 2GHz. > > If entering kernel and we only adjust the cpufreq voltage, if > > the cpufreq is under 1.618GHz, the cci will be out of spec. > > > > f. If cpufreq driver wait cci ready, regulator framework will > > take > > the highest voltage requests from cci and cpufreq as output > > so that it prevents from high freqeuncy low voltage crash. > > > > d. Therefore, I think it's not a good idea to bypass cci device > > if > > the ccifreq_supported is true in MT8183 and MT8186. > > I do not propose to bypass CCI device. What both Angelo and I are > saying is just that you need a better way to handle the cases when > CCI > is not (yet) enabled. The current way in the propsed patch is not > good > enough. > > I fully understand the potential problems with high frequency & low > voltage when using a shared regulator. But, I think the problem > we're > trying to solve here is specific to the initial boot of the platform, > while we are waiting for the CCI driver to be loaded. > > The root of the problem is that the CCI bus has constraints on the > voltage regulator that are not defined anywhere until the CCI driver > is > loaded. So to fix that, you need to either: > > 1) not allow any voltage changes > 2) register the CCI device constraints > > In the current patch, you attempt to do (1). There's nothing wrong > with > the idea, we just pointed out problems in your implementation, > especially the fact that it does nothing, but it "succeeds" so the > CPUfreq framework will think the OPPs are different from what they > actually are. > > Just an idea, but another option could be (2). While waiting for the > CCI device to be ready, the CPUfreq driver could check the current > CCI > freq/voltage and set min/max constraints on the regulator that > prevent > CCI from breaking. These constraints would stay in place until the > CCI > driver is ready. Once the real CCI driver is ready/registerd these > contraints would be removed. > > Another version of this same idea would be to check the CCI > freq/voltage > and then limit the OPPs available to CPUfreq to only the ones that > would > not break CCI. Then, when CCI is ready/registered, you remove the > limits. > > > 2. Check the device link status is DL_DEV_DRIVER_BOUND is used for > > promising the cci is probed done. > > > > 3. About the cpufreq_driver->target_index > > a. When I trace the common drivers, I found if the return value > > is > > not zero, it will be BUG_ON. > > ref: > > https://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c*L1471__;Iw!!CTRNKA9wMg0ARbw!wgawOs1JSuJihgxA1nxhbd2Ekoys_bPCAlIH9YJhe2N9ckG6O1mDB-7zqSf6x2MhCfXo$ > > > > Right, you should not try to do deferred probe in the ->set_target() > callback. Deferred probe is meant for init/probe time. > > > b. I also try to move is_ccifreq_ready() to other place, like > > cpufreq_driver->init and cpufreq probe function. > > There will be new issue. Do you have any suggetion that we > > can > > retern value of DEFER_PROBE? > > The only appropriate place is in the probe function. > > Kevin Hello Kevin,
Rex-BC Chen <rex-bc.chen@mediatek.com> writes: [...] > From the Chanwoo's devfreq passive govonor series, it's impossible to > let cci devreq probed done before cpufreq because the passive govonor > will search for cpufreq node and use it. > > Ref: function: cpufreq_passive_register_notifier() > > https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-testing&id=b670978ddc43eb0c60735c3af6e4a370603ab673 Well this is a problem, because CCI depends on CPUfreq, but CPUfreq depends on CCI, so one of them has to load and then wait for the other. > After I discuss with Angelo and Jia-wei, we think we are keeping the > function in target_index and if the cci is not ready we will use the > voltage which is set by bootloader to prevent high freqeuncy low > voltage crash. And then we can keep seting the target frequency. > > We assume the setting of bootloader is correct and we can do this. I'm still not crazy about this because you're lying to the CPUfreq framework. It's requesting one OPP, but you're not setting that, you're just keeping the bootloader frequency. In my earlier reply, I gave two other options for handling this. 1) set a (temporary) constraint on the voltage regulator so that it cannot change. or more clean, IMO: 2) set a CPUfreq policy that restricts available OPPs to ones that will not break CCI. Either of these solutions allow you to load the CPUfreq driver early, and then wait for the CCI driver to be ready before removing the restrictions. > For the SoCs that including ci hardware (8183 and 8186), we think it's > not ok if we don't probe cci correctly. > If we failed to get cci node, I think we sould return -ENODEV and the > probe of cpufreq failed. > > What do you think the solution? I think it would be better if CPUfreq probes sucessfully, but restricts the OPPs available until CCI is ready. If CCI fails to probe/load, you still have a working CPUfreq driver, it just has a restricted set of OPPs. Kevin
Cpufreq is a DVFS driver used for power saving to scale the clock frequency and supply the voltage for CPUs. This series do some cleanup for MediaTek cpufreq drivers and add support for MediaTek SVS[2] and MediaTek CCI devfreq[3] which are supported in MT8183 and MT8186. Changes for V2: 1. Drop the modification of transforming cpufreq-mediatek into yaml and only add the MediaTek CCI property for MediaTek cpufreq. 2. Split the original patches into several patches. Reference series: [1]: V1 of this series is present by Jia-Wei Chang. message-id:20220307122151.11666-1-jia-wei.chang@mediatek.com [2]: The MediaTek CCI devfreq driver is introduced in another series. message-id:20220307122513.11822-1-jia-wei.chang@mediatek.com [3]: The MediaTek SVS driver is introduced in another series. message-id:20220221063939.14969-1-roger.lu@mediatek.com Andrew-sh.Cheng (1): cpufreq: mediatek: Add opp notification for SVS support Jia-Wei Chang (13): dt-bindings: cpufreq: mediatek: Add MediaTek CCI property cpufreq: mediatek: Use module_init and add module_exit cpufreq: mediatek: Cleanup variables and error handling in mtk_cpu_dvfs_info_init() cpufreq: mediatek: Remove unused headers cpufreq: mediatek: Enable clocks and regulators cpufreq: mediatek: Record previous target vproc value cpufreq: mediatek: Move voltage limits to platform data cpufreq: mediatek: Add .get function cpufreq: mediatek: Make sram regulator optional cpufreq: mediatek: Update logic of voltage_tracking() cpufreq: mediatek: Use maximum voltage in init stage cpufreq: mediatek: Link CCI device to CPU cpufreq: mediatek: Add support for MT8186 Rex-BC Chen (1): cpufreq: mediatek: Use device print to show logs .../bindings/cpufreq/cpufreq-mediatek.txt | 4 + drivers/cpufreq/mediatek-cpufreq.c | 609 ++++++++++++------ 2 files changed, 401 insertions(+), 212 deletions(-)