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

[Kevin Hilman <khilman@baylibre.com>]

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