Re: [PATCH v8 5/6] PM / devfreq: Add PM QoS support

[Leonard Crestez <leonard.crestez@nxp.com>]

On 25.09.2019 05:13, Chanwoo Choi wrote:
> On 19. 9. 25. 오전 4:22, Leonard Crestez wrote:
>> On 24.09.2019 22:11, Matthias Kaehlcke wrote:
>>> On Tue, Sep 24, 2019 at 01:11:29PM +0300, Leonard Crestez wrote:
>>>> Register notifiers with the PM QoS framework in order to respond to
>>>> requests for DEV_PM_QOS_MIN_FREQUENCY and DEV_PM_QOS_MAX_FREQUENCY.
>>>>
>>>> No notifiers are added by this patch but PM QoS constraints can be
>>>> imposed externally (for example from other devices).
>>>>
>>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>>>> ---
>>>>    drivers/devfreq/devfreq.c | 75 +++++++++++++++++++++++++++++++++++++++
>>>>    include/linux/devfreq.h   |  5 +++
>>>>    2 files changed, 80 insertions(+)
>>>>
>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>> index eee403e70c84..784f3e40536a 100644
>>>> --- a/drivers/devfreq/devfreq.c
>>>> +++ b/drivers/devfreq/devfreq.c
>>>> @@ -22,15 +22,18 @@
>>>>    #include <linux/platform_device.h>
>>>>    #include <linux/list.h>
>>>>    #include <linux/printk.h>
>>>>    #include <linux/hrtimer.h>
>>>>    #include <linux/of.h>
>>>> +#include <linux/pm_qos.h>
>>>>    #include "governor.h"
>>>>    
>>>>    #define CREATE_TRACE_POINTS
>>>>    #include <trace/events/devfreq.h>
>>>>    
>>>> +#define HZ_PER_KHZ	1000
>>>> +
>>>>    static struct class *devfreq_class;
>>>>    
>>>>    /*
>>>>     * devfreq core provides delayed work based load monitoring helper
>>>>     * functions. Governors can use these or can implement their own
>>>> @@ -109,10 +112,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>>>>    static void get_freq_range(struct devfreq *devfreq,
>>>>    			   unsigned long *min_freq,
>>>>    			   unsigned long *max_freq)
>>>>    {
>>>>    	unsigned long *freq_table = devfreq->profile->freq_table;
>>>> +	unsigned long qos_min_freq, qos_max_freq;
>>>>    
>>>>    	lockdep_assert_held(&devfreq->lock);
>>>>    
>>>>    	/*
>>>>    	 * Init min/max frequency from freq table.
>>>> @@ -125,10 +129,18 @@ static void get_freq_range(struct devfreq *devfreq,
>>>>    	} else {
>>>>    		*min_freq = freq_table[devfreq->profile->max_state - 1];
>>>>    		*max_freq = freq_table[0];
>>>>    	}
>>>>    
>>>> +	/* constraints from PM QoS */
>>>> +	qos_min_freq = dev_pm_qos_read_value(devfreq->dev.parent,
>>>> +					     DEV_PM_QOS_MIN_FREQUENCY);
>>>> +	qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent,
>>>> +					     DEV_PM_QOS_MIN_FREQUENCY); >
>>> This needs to be DEV_PM_QOS_MAX_FREQUENCY. I missed this in the earlier
>>> reviews, but stumbled across it when testing.
>>>
>>> !Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>>
>> I broke it in v8 while processing comments. Thanks for catching it.
> 
> Did you test this patchset with v8?
> Maybe it is not working with this mistake.

I ran some scripts which test that min_freq requests work as expected 
(using imx interconnect+devfreq). They don't touch max_freq.

--
Regards,
Leonard