Re: [PATCH v2] cpufreq: airoha: Add EN7581 Cpufreq SMC driver

[Christian Marangi <ansuelsmth@gmail.com>]

On Fri, Nov 29, 2024 at 09:32:34AM +0530, Viresh Kumar wrote:
> On 25-11-24, 13:43, Christian Marangi wrote:
> > sorry for the delay... I checked the example and other cpufreq driver
> > that register a simple cpufreq-dt. None of the current driver implements
> > a full clk provider internally
> 
> Yeah, that may be done from platform code for those drivers instead of the
> cpufreq driver.
> 
> > and I have some fear it might be
> > problematic to have mixed stuff, eventually I feel I should implement a
> > small clk driver that implements determine_rate, set_rate, a compatible
> > and all sort. And still we would have the double reference of OPP
> > Index->Freq Clock->OPP index.
> 
> One way to avoid that would be to use performance-state stuff and model this as
> a genpd. In that case, opp->level field (which you can set as index only in your
> case) will be programmed as is by the genpd core. That's why I cc'd Ulf earlier
> as it looked more suited to your case.
>

Hi I just sent v4 that exactly implement that. It was hard but at the
end I manage to make use of it :D

> > I wonder if a much easier and better solution for this is (similar to
> > how we do with suspend and resume) add entry in the struct
> > cpufreq_dt_platform_data, to permit to define simple .target_index and
> > .get and overwrite the default one cpufreq-dt.
> 
> Easier, sure. Better, I doubt that :(
> 
> The OPP core currently configures a lot of stuff from set-opp API, like clk,
> regulators, genpd (performance state), bandwidth, etc and I really want to use
> that infrastructure for everyone instead of starting to open code that in
> drivers. The suspend/resume callbacks are special as the OPP core doesn't know
> what to do in that case and so it was left for the drivers to handle that.
> 

Yep, totally understandable and i can see the problem in open coding it.
Easier fort devs but leaves space for random implementation that can be
implemented with OPP and PD

> > This permits to both reduce greatly the airoha-cpufreq driver, register
> > a simple cpufreq-dt and prevent any kind of overhead. After all the
> > .target_index and .get doesn't do anything fancy, they just call the OPP
> > set and clk get rate.
> 
> Yeah, clk-get is pretty simple but opp-set isn't and then there is scope of
> further enhancements / improvements in the future which can be best handled if
> we keep that code common for everyone.
> 
> > What do you think? Changes are really trivial since this is already
> > implemented for suspend and resume.
> 
> I think you can model it as a performance state (which lot of platforms are
> doing as well), and then you won't have the index->clk->index issue anymore.
>

Hope you can check v4 if everything is OK. A dedicated node was required
for the clk provider and the PD provider but that is O.K. if that means
transparent handling for cpufreq-dt.

Thanks a lot on the hint on how to correctly implement this!

-- 
	Ansuel