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

[Rex-BC Chen]

On Tue, 2022-04-12 at 11:04 -0700, Kevin Hilman wrote:
> Rex-BC Chen <rex-bc.chen at mediatek.com> writes:
> 
> > On Mon, 2022-04-11 at 11:09 -0700, Kevin Hilman wrote:
> > > Hi Rex,
> > > 
> > > Rex-BC Chen <rex-bc.chen at mediatek.com> writes:
> > > 
> > > > On Fri, 2022-04-08 at 13:29 -0700, Kevin Hilman wrote:
> > > > > Rex-BC Chen <rex-bc.chen at mediatek.com> writes:
> > > > > 
> > > > > > From: "Andrew-sh.Cheng" <andrew-sh.cheng at 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
> 
> 
> 
Hello Kevin,

Got it.
Thanks for your kindly explaination for this.
I will update the commit message in next version.

BRs,
Rex