| Message ID | 20220408045908.21671-1-rex-bc.chen@mediatek.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | cpufreq: mediatek: Cleanup and support MT8183 and MT8186 | expand |
Il 08/04/22 06:59, Rex-BC Chen ha scritto: > 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. > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > 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, 82 insertions(+), 8 deletions(-) > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > index 472f4de29e5f..1369da62780a 100644 > --- a/drivers/cpufreq/mediatek-cpufreq.c > +++ b/drivers/cpufreq/mediatek-cpufreq.c ..snip.. > + > 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); You are registering this notifier, but never unregistering it. Please unregister it in error conditions and also upon runtime removal of the mediatek-cpufreq module. -> dev_pm_opp_unregister_notifier() Regards, Angelo > + if (ret) { > + dev_warn(cpu_dev, "cpu%d: failed to register opp notifier\n", cpu); > + goto out_disable_inter_clock; > + } > + > + mutex_init(&info->reg_lock); > + info->opp_freq = clk_get_rate(info->cpu_clk); > + > /* > * If SRAM regulator is present, software "voltage tracking" is needed > * for this CPU power domain.
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: > 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? Thanks, 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 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
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 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
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(-)