Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 02/10] PM / devfreq: Do not require devices to have OPPs
To:     Samuel Holland <samuel@sholland.org>,
        MyungJoo Ham <myungjoo.ham@samsung.com>,
        Kyungmin Park <kyungmin.park@samsung.com>
Cc:     Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-pm@vger.kernel.org,
        linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org,
        Maxime Ripard <mripard@kernel.org>,
        Chen-Yu Tsai <wens@csie.org>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        Rob Herring <robh+dt@kernel.org>
From:   Chanwoo Choi <cw00.choi@samsung.com>
Organization: Samsung Electronics
Message-ID: <afde619a-6816-0a93-807a-01e3718a38c1@samsung.com>
Date:   Fri, 1 Oct 2021 11:14:13 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101
        Thunderbird/59.0
MIME-Version: 1.0
In-Reply-To: <7107eea6-f2ca-fb7c-5975-569066ba21a7@sholland.org>
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <20210929044254.38301-1-samuel@sholland.org>
        <CGME20210929044301epcas1p4d69083b46ca38d610981db6f01cfe9e4@epcas1p4.samsung.com>
        <20210929044254.38301-3-samuel@sholland.org>
        <114afa7e-6218-6b1f-f87e-84690f10029c@samsung.com>
        <d0a2c36b-4019-2f52-13f0-be76db5a48ec@sholland.org>
        <7d404f89-6567-61e6-7964-d2ca578ed652@samsung.com>
        <7107eea6-f2ca-fb7c-5975-569066ba21a7@sholland.org>
Precedence: bulk

On 10/1/21 10:45 AM, Samuel Holland wrote:
> On 9/30/21 8:59 PM, Chanwoo Choi wrote:
>> 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.
> 
> Thanks for the explanation. I will convert the driver to use
> dev_pm_opp_add(), and I will drop patches 2 and 4. I think patch 3 is
> still worth considering.

Thanks. Previously, there was some devfreq driver to use freq_table
with descending order. I'll check the devfreq driver in linux kernel.
If there are no any use-case of freq_table with descending order,
I'll accept the patch3.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics