Re: [PATCH V2 07/15] cpufreq: mediatek: Add opp notification for SVS support

[Rex-BC Chen <rex-bc.chen@mediatek.com>]

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.

May I ask what is your concern?

BRs,
Rex



_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek