Re: [PATCH 02/10] PM / devfreq: Do not require devices to have OPPs

[Chanwoo Choi]

On 9/30/21 8:37 PM, Samuel Holland wrote:
> On 9/29/21 11:19 PM, Chanwoo Choi wrote:
>> Hi Samuel,
>>
>>
>> On 9/29/21 1:42 PM, Samuel Holland wrote:
>>> Since commit ea572f816032 ("PM / devfreq: Change return type of
>>> devfreq_set_freq_table()"), all devfreq devices are required to have a
>>> valid freq_table. If freq_table is not provided by the driver, it will
>>> be filled in by set_freq_table() from the OPPs; if that fails,
>>> devfreq_add_device() will return an error.
>>>
>>> However, since commit ab8f58ad72c4 ("PM / devfreq: Set min/max_freq when
>>> adding the devfreq device"), devfreq devices are _also_ required to have
>>> an OPP table, even if they provide freq_table. devfreq_add_device()
>>> requires dev_pm_opp_find_freq_ceil() and dev_pm_opp_find_freq_floor() to
>>> return successfully, specifically to initialize scaling_min/max_freq.
>>>
>>> Not all drivers need an OPP table. For example, a driver where all
>>> frequencies are determined dynamically could work by filling out only
>>> freq_table. But with the current code it must call dev_pm_opp_add() on
>>> every freq_table entry to probe successfully.
>>
>> As you commented, if device has no opp table, it should call dev_pm_opp_add().
>> The devfreq have to use OPP for controlling the frequency/regulator.
>>
>> Actually, I want that all devfreq driver uses the OPP as default way.
> 
> The current code/documentation implies that an OPP table is intended to
> be optional. For example:
> 
>  * struct devfreq - Device devfreq structure
> ...
>  * @opp_table:  Reference to OPP table of dev.parent, if one exists.
> 
> So this should be updated if an OPP table is no longer optional.

Right. Need to update it.

> 
>> Are there any reason why don't use the OPP table?
> 
> dev_pm_opp_add() takes a voltage, and assumes the existence of some
> voltage regulator, but there is none involved here. The only way to have
> an OPP table without regulators is to use a static table in the
> devicetree. But that also doesn't make much sense, because the OPPs
> aren't actually customizable; they are integer dividers from a fixed
> base clock.

You can use OPP for only clock control without regulator. OPP already
provides them. OPP already provides the helpful function which
implement the functions to handle the clock/regulator or power doamin.
It is useful framework to control clock/regulator. 

If the standard framework in Linux kernel, it is best to use
this framework in order to remove the duplicate codes on multiple
device drivers. It is one of advantage of Linux kernel. 

Also, if OPP doesn't support the some requirement of you,
you can contribute and update the OPP.

 And adding a fixed OPP table to each board would be a lot of
> work to replace a trivial loop in the driver. So it seems to be the
> wrong abstraction.

I don't understand. As I commented for patch 10, you can add
the OPP entry of the clock without the fixed OPP table in devicetree.

> 
> Using an OPP table adds extra complexity (memory allocations, error
> cases), just to duplicate the list of frequencies that already has to
> exist in freq_table. And the driver works fine without any of that.

'freq_table' of devfreq was developed before of adding OPP interface to Linux kernel as I knew. Actually, I prefer to use the OPP interface
instead of initializing the freq_table directly by device driver.
I just keep the 'freq_table' for preventing the build/working issue
for older device driver. I think OPP is enough to control frequency/voltage
and it provides the various helper funcitons for user of OPP.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics