Re: [PATCH 7/7] devfreq: move statistics to separate struct

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Lukaz,

On 12/16/19 10:01 PM, Lukasz Luba wrote:
> Hi Bartek,
> 
> [added Dietmar, Robin, Andrzej (for upcoming DRM drm-misc-next)]
> 
> On 11/15/19 12:40 PM, Bartlomiej Zolnierkiewicz wrote:
>>
>> [ added Zhang, Eduardo, Ørjan and Javi to Cc: ]
>>
>> On 11/15/19 7:21 AM, Chanwoo Choi wrote:
>>> Hi Bartlomiej,
>>>
>>> On 11/15/19 12:25 PM, Chanwoo Choi wrote:
>>>> Hi Bartlomiej,
>>>>
>>>> On 11/15/19 3:01 AM, Bartlomiej Zolnierkiewicz wrote:
>>>>>
>>>>> Hi Chanwoo,
>>>>>
>>>>> On 11/14/19 2:52 AM, Chanwoo Choi wrote:
>>>>>> Hi Kamil,
>>>>>>
>>>>>> The 'freq_table' and 'max_state' in the devfreq_dev_profile
>>>>>> were used in the ARM Mali device driver[1][2][3]. Although ARM Mali
>>>>>> device driver was posted to mainline kernel, they used
>>>>>> them for a long time. It means that this patch break
>>>>>> the compatibility. The ARM Mali drivers are very
>>>>>> important devfreq device driver.
>>>>>
>>>>> This argument is not a a technical one and the official upstream
>>>>> kernel policy is to not depend on out-of-tree drivers.
>>>>>
>>>>> Besides the ARM Mali drivers are full of code like:
>>>>>
>>>>> #if LINUX_VERSION_CODE >= KERNEL_VERSION(x, y, z)
>>>>> ...
>>>>> #else
>>>>> ...
>>>>> #endif
>>>>>
>>>>> so few more instances of similar code won't do any harm.. ;-)
>>>>>
>>>>>> [1] https://protect2.fireeye.com/url?k=909caa5c-cd52abe8-909d2113-000babdfecba-812f16576c3614a3&u=https://developer.arm.com/tools-and-software/graphics-and-gaming/mali-drivers/bifrost-kernel#
>>>>>> [2] https://protect2.fireeye.com/url?k=33265f96-6ee85e22-3327d4d9-000babdfecba-44c2daec328712e6&u=https://developer.arm.com/tools-and-software/graphics-and-gaming/mali-drivers/midgard-kernel
>>>>>> [3] https://protect2.fireeye.com/url?k=69bdcab0-3473cb04-69bc41ff-000babdfecba-4b576facf85e0208&u=https://developer.arm.com/tools-and-software/graphics-and-gaming/mali-drivers/utgard-kernel
>>>>>
>>>>> I took a look at ARM Mali drivers source code anyway and I fail to
>>>>> see a rationale behind their behavior of doing 'freq_table' and
>>>>> 'max_state' initialization in the driver itself (instead of leaving
>>>>> it up to the devfreq core code, like all in-kernel drivers are doing
>>>>> already).
>>>>>
>>>>> Could you please explain rationale behind ARM Mali drivers' special
>>>>> needs?
>>>>>
>>>>> [ Both ARM Mali and devfreq core code are using generic PM OPP code
>>>>>    these days to do 'freq_table' and 'max_state' initialization, the
>>>>>    only difference seems to be that ARM Mali creates the frequency
>>>>>    table in the descending order (but there also seems to be no real
>>>>>    need for it). ]
>>>>>
>>>>> Maybe this is an opportunity to simplify also the ARM Mali driver?
>>>>
>>>> OK. I agree to simplify them on this time.
>>>> For touching the 'freq_table' and 'max_state', need to fix
>>>> the descending order of freq_table.
>>>>
>>>> The partition_enable_ops() in the drivers/thermal/devfreq_cooling.c
>>>> requires the descending order of freq_table. Have to change it by using
>>>> the ascending time or support both ascending and descending order for freq_table.
>>>>
>>>> 1. Move freq_table, max_state of devfreq_dev_profile to struct devfreq
>>>> 2. Edit partition_enable_ops() in the drivers/thermal/devfreq_cooling.c
>>>>     by using ascending order instead of descending order.
>>>>
>>>
>>> After changed about 'freq_table' and 'max_state', the build error
>>> will happen on ARM mail driver because the initialization code of
>>> 'freq_table' in ARM mali driver, didn't check the kernel version.
>>>
>>> The first devfreq patch provided the 'freq_table' as optional variable
>>> in the 'struct devfreq_dev_profile'. Even if ARM mali driver is out of mainline tree,
>>> this change seems that break the usage rule of 'freq_table' in 'struct devfreq_dev_profile'.
>>>
>>> So, if there are no any beneficial reason, I just keep the current status of 'freq_table'
>>> in order to keep the previous usage rule of 'freq_table' in 'struct devfreq_dev_profile'.
>>>
>>> Frankly, I'm note sure that it is necessary. I don't want to make
>>> the side-effect without any useful reason.
>>>
>>> But,
>>> Separately, have to fix the ordering issue of partition_enable_ops()
>>> in the drivers/thermal/devfreq_cooling.c.
>>
>> Hmmm.. fixing partition_enable_opps() should be trivial but I wonder
>> why we are carrying devfreq_cooling.c code in upstream kernel at all?
> 
> Well, the devfreq_cooling.c is going to have a client in mainline:
> the GPU driver - Panfrost.
> 
> It is already in DRM branch 'drm-misc-next':
> https://protect2.fireeye.com/url?k=75a0e087-283b1ce4-75a16bc8-0cc47a31cdbc-4953aa9e0574f6dc&u=https://patchwork.freedesktop.org/patch/342848/
> 
> Regarding the devfreq_cooling.c code structure.
> I am currently working on cleaning up the devfreq cooling code and
> adding Energy Model instead for private freq, power tables. It will be
> in similar fashion as it is done in cpufreq_cooling. The model will
> be also simplified so hopefully more clients would come.
> It is under internal review and will be posted shortly.

Good news about Energy Model. When you send the patch related to Energy model,
please add me to Cc list.

> 
>>
>> It has been merged in the following commit:
>>
>> commit a76caf55e5b356ba20a5a43ac4d9f7a04b20941d
>> Author: Ørjan Eide <orjan.eide@xxxxxxx>
>> Date:   Thu Sep 10 18:09:30 2015 +0100
>>
>>      thermal: Add devfreq cooling
>>           Add a generic thermal cooling device for devfreq, that is similar to
>>      cpu_cooling.
>>           The device must use devfreq.  In order to use the power extension of the
>>      cooling device, it must have registered its OPPs using the OPP library.
>>           Cc: Zhang Rui <rui.zhang@xxxxxxxxx>
>>      Cc: Eduardo Valentin <edubezval@xxxxxxxxx>
>>      Signed-off-by: Javi Merino <javi.merino@xxxxxxx>
>>      Signed-off-by: Ørjan Eide <orjan.eide@xxxxxxx>
>>      Signed-off-by: Eduardo Valentin <edubezval@xxxxxxxxx>
>> ...
>>
>> but 4 years later there is still no single in-kernel user for this code?
> 
> There will be, via DRM tree.
> 
> Regards,
> Lukasz
> 
>>
>> Best regards,
>> -- 
>> Bartlomiej Zolnierkiewicz
>> Samsung R&D Institute Poland
>> Samsung Electronics
>>
>>>>>
>>>>>> Also, the devfreq device driver specifies their own
>>>>>> information and data into devfreq_dev_profile structure
>>>>>> before registering the devfreq device with devfreq_add_device().
>>>>>> This patch breaks the basic usage rule of devfreq_dev_profile structure.
>>>>>
>>>>> Well, 'struct devfreq_stats *stats' can be trivially moved out of
>>>>> 'struct devfreq_profile' to 'struct devfreq' if you prefer it that
>>>>> way..
>>>>>
>>>>> Best regards,
>>>>> -- 
>>>>> Bartlomiej Zolnierkiewicz
>>>>> Samsung R&D Institute Poland
>>>>> Samsung Electronics
>>>>>
>>>>>> So, I can't agree this patch. Not ack.
>>>>>>
>>>>>> Regards,
>>>>>> Chanwoo Choi
>>>>>>
>>>>>> On 11/13/19 6:13 PM, Kamil Konieczny wrote:
>>>>>>> Count time and transitions between devfreq frequencies in separate struct
>>>>>>> for improved code readability and maintenance.
>>>>>>>
>>>>>>> Signed-off-by: Kamil Konieczny <k.konieczny@xxxxxxxxxxx>
>>>>>>> ---
>>>>>>>   drivers/devfreq/devfreq.c          | 156 ++++++++++++++++-------------
>>>>>>>   drivers/devfreq/exynos-bus.c       |   6 +-
>>>>>>>   drivers/devfreq/governor_passive.c |  26 +++--
>>>>>>>   include/linux/devfreq.h            |  43 ++++----
>>>>>>>   4 files changed, 129 insertions(+), 102 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>>>>> index d79412b0de59..d85867a91230 100644
>>>>>>> --- a/drivers/devfreq/devfreq.c
>>>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>>>> @@ -105,10 +105,11 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>>>>>>>    */
>>>>>>>   static int devfreq_get_freq_level(struct devfreq *devfreq, unsigned long freq)
>>>>>>>   {
>>>>>>> +    struct devfreq_stats *stats = devfreq->profile->stats;
>>>>>>>       int lev;
>>>>>>>   -    for (lev = 0; lev < devfreq->profile->max_state; lev++)
>>>>>>> -        if (freq == devfreq->profile->freq_table[lev])
>>>>>>> +    for (lev = 0; lev < stats->max_state; lev++)
>>>>>>> +        if (freq == stats->freq_table[lev])
>>>>>>>               return lev;
>>>>>>>         return -EINVAL;
>>>>>>> @@ -117,56 +118,64 @@ static int devfreq_get_freq_level(struct devfreq *devfreq, unsigned long freq)
>>>>>>>   static int set_freq_table(struct devfreq *devfreq)
>>>>>>>   {
>>>>>>>       struct devfreq_dev_profile *profile = devfreq->profile;
>>>>>>> +    struct devfreq_stats *stats;
>>>>>>>       struct dev_pm_opp *opp;
>>>>>>>       unsigned long freq;
>>>>>>> -    int i, count;
>>>>>>> +    int i, count, err = -ENOMEM;
>>>>>>>         /* Initialize the freq_table from OPP table */
>>>>>>>       count = dev_pm_opp_get_opp_count(devfreq->dev.parent);
>>>>>>>       if (count <= 0)
>>>>>>>           return -EINVAL;
>>>>>>>   -    profile->max_state = count;
>>>>>>> -    profile->freq_table = devm_kcalloc(devfreq->dev.parent,
>>>>>>> -                    count,
>>>>>>> -                    sizeof(*profile->freq_table),
>>>>>>> -                    GFP_KERNEL);
>>>>>>> -    if (!profile->freq_table) {
>>>>>>> -        profile->max_state = 0;
>>>>>>> +    stats = devm_kzalloc(devfreq->dev.parent,
>>>>>>> +                 sizeof(struct devfreq_stats), GFP_KERNEL);
>>>>>>> +    if (!stats)
>>>>>>>           return -ENOMEM;
>>>>>>> -    }
>>>>>>>   -    for (i = 0, freq = 0; i < profile->max_state; i++, freq++) {
>>>>>>> +    profile->stats = stats;
>>>>>>> +    stats->max_state = count;
>>>>>>> +    stats->freq_table = devm_kcalloc(devfreq->dev.parent,
>>>>>>> +                     count,
>>>>>>> +                     sizeof(*stats->freq_table),
>>>>>>> +                     GFP_KERNEL);
>>>>>>> +    if (!stats->freq_table)
>>>>>>> +        goto err_no_mem;
>>>>>>> +
>>>>>>> +    for (i = 0, freq = 0; i < count; i++, freq++) {
>>>>>>>           opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, &freq);
>>>>>>>           if (IS_ERR(opp)) {
>>>>>>> -            devm_kfree(devfreq->dev.parent, profile->freq_table);
>>>>>>> -            profile->max_state = 0;
>>>>>>> -            return PTR_ERR(opp);
>>>>>>> +            devm_kfree(devfreq->dev.parent, stats->freq_table);
>>>>>>> +            stats->max_state = 0;
>>>>>>> +            err = PTR_ERR(opp);
>>>>>>> +            goto err_no_mem;
>>>>>>>           }
>>>>>>>           dev_pm_opp_put(opp);
>>>>>>> -        profile->freq_table[i] = freq;
>>>>>>> +        stats->freq_table[i] = freq;
>>>>>>>       }
>>>>>>>   -    profile->trans_table = devm_kzalloc(devfreq->dev.parent,
>>>>>>> -                        array3_size(sizeof(unsigned int),
>>>>>>> -                            count, count),
>>>>>>> -                        GFP_KERNEL);
>>>>>>> -    if (!profile->trans_table)
>>>>>>> +    stats->trans_table = devm_kzalloc(devfreq->dev.parent,
>>>>>>> +                      array3_size(sizeof(unsigned int),
>>>>>>> +                              count, count),
>>>>>>> +                      GFP_KERNEL);
>>>>>>> +    if (!stats->trans_table)
>>>>>>>           goto err_no_mem;
>>>>>>>   -    profile->time_in_state = devm_kcalloc(devfreq->dev.parent, count,
>>>>>>> -                          sizeof(*profile->time_in_state),
>>>>>>> -                          GFP_KERNEL);
>>>>>>> -    if (!profile->time_in_state)
>>>>>>> +    stats->time_in_state = devm_kcalloc(devfreq->dev.parent, count,
>>>>>>> +                        sizeof(*stats->time_in_state),
>>>>>>> +                        GFP_KERNEL);
>>>>>>> +    if (!stats->time_in_state)
>>>>>>>           goto err_no_mem;
>>>>>>>   -    profile->last_time = get_jiffies_64();
>>>>>>> -    spin_lock_init(&profile->stats_lock);
>>>>>>> +    stats->last_time = get_jiffies_64();
>>>>>>> +    spin_lock_init(&stats->stats_lock);
>>>>>>>         return 0;
>>>>>>>   err_no_mem:
>>>>>>> -    profile->max_state = 0;
>>>>>>> -    return -ENOMEM;
>>>>>>> +    stats->max_state = 0;
>>>>>>> +    devm_kfree(devfreq->dev.parent, profile->stats);
>>>>>>> +    profile->stats = NULL;
>>>>>>> +    return err;
>>>>>>>   }
>>>>>>>     /**
>>>>>>> @@ -176,7 +185,7 @@ static int set_freq_table(struct devfreq *devfreq)
>>>>>>>    */
>>>>>>>   int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>>>>>>>   {
>>>>>>> -    struct devfreq_dev_profile *profile = devfreq->profile;
>>>>>>> +    struct devfreq_stats *stats = devfreq->profile->stats;
>>>>>>>       unsigned long long cur_time;
>>>>>>>       int lev, prev_lev, ret = 0;
>>>>>>>   @@ -184,22 +193,21 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>>>>>>>         /* Immediately exit if previous_freq is not initialized yet. */
>>>>>>>       if (!devfreq->previous_freq) {
>>>>>>> -        spin_lock(&profile->stats_lock);
>>>>>>> -        profile->last_time = cur_time;
>>>>>>> -        spin_unlock(&profile->stats_lock);
>>>>>>> +        spin_lock(&stats->stats_lock);
>>>>>>> +        stats->last_time = cur_time;
>>>>>>> +        spin_unlock(&stats->stats_lock);
>>>>>>>           return 0;
>>>>>>>       }
>>>>>>>         prev_lev = devfreq_get_freq_level(devfreq, devfreq->previous_freq);
>>>>>>>   -    spin_lock(&profile->stats_lock);
>>>>>>> +    spin_lock(&stats->stats_lock);
>>>>>>>       if (prev_lev < 0) {
>>>>>>>           ret = prev_lev;
>>>>>>>           goto out;
>>>>>>>       }
>>>>>>>   -    profile->time_in_state[prev_lev] +=
>>>>>>> -             cur_time - profile->last_time;
>>>>>>> +    stats->time_in_state[prev_lev] += cur_time - stats->last_time;
>>>>>>>       lev = devfreq_get_freq_level(devfreq, freq);
>>>>>>>       if (lev < 0) {
>>>>>>>           ret = lev;
>>>>>>> @@ -207,14 +215,14 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>>>>>>>       }
>>>>>>>         if (lev != prev_lev) {
>>>>>>> -        profile->trans_table[(prev_lev *
>>>>>>> -                profile->max_state) + lev]++;
>>>>>>> -        profile->total_trans++;
>>>>>>> +        stats->trans_table[(prev_lev *
>>>>>>> +                stats->max_state) + lev]++;
>>>>>>> +        stats->total_trans++;
>>>>>>>       }
>>>>>>>     out:
>>>>>>> -    profile->last_time = cur_time;
>>>>>>> -    spin_unlock(&profile->stats_lock);
>>>>>>> +    stats->last_time = cur_time;
>>>>>>> +    spin_unlock(&stats->stats_lock);
>>>>>>>       return ret;
>>>>>>>   }
>>>>>>>   EXPORT_SYMBOL(devfreq_update_status);
>>>>>>> @@ -504,9 +512,9 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
>>>>>>>           queue_delayed_work(devfreq_wq, &devfreq->work,
>>>>>>>               msecs_to_jiffies(profile->polling_ms));
>>>>>>>   -    spin_lock(&profile->stats_lock);
>>>>>>> -    profile->last_time = get_jiffies_64();
>>>>>>> -    spin_unlock(&profile->stats_lock);
>>>>>>> +    spin_lock(&profile->stats->stats_lock);
>>>>>>> +    profile->stats->last_time = get_jiffies_64();
>>>>>>> +    spin_unlock(&profile->stats->stats_lock);
>>>>>>>       devfreq->stop_polling = false;
>>>>>>>         if (profile->get_cur_freq &&
>>>>>>> @@ -677,7 +685,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>>>>>>       devfreq->data = data;
>>>>>>>       devfreq->nb.notifier_call = devfreq_notifier_call;
>>>>>>>   -    if (!profile->max_state && !profile->freq_table) {
>>>>>>> +    if (!profile->stats) {
>>>>>>>           mutex_unlock(&devfreq->lock);
>>>>>>>           err = set_freq_table(devfreq);
>>>>>>>           if (err < 0)
>>>>>>> @@ -1282,6 +1290,7 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>>>>>>>                     const char *buf, size_t count)
>>>>>>>   {
>>>>>>>       struct devfreq *df = to_devfreq(dev);
>>>>>>> +    struct devfreq_stats *stats = df->profile->stats;
>>>>>>>       unsigned long value;
>>>>>>>       int ret;
>>>>>>>   @@ -1297,13 +1306,13 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>>>>>>>               goto unlock;
>>>>>>>           }
>>>>>>>       } else {
>>>>>>> -        unsigned long *freq_table = df->profile->freq_table;
>>>>>>> +        unsigned long *freq_table = stats->freq_table;
>>>>>>>             /* Get minimum frequency according to sorting order */
>>>>>>> -        if (freq_table[0] < freq_table[df->profile->max_state - 1])
>>>>>>> +        if (freq_table[0] < freq_table[stats->max_state - 1])
>>>>>>>               value = freq_table[0];
>>>>>>>           else
>>>>>>> -            value = freq_table[df->profile->max_state - 1];
>>>>>>> +            value = freq_table[stats->max_state - 1];
>>>>>>>       }
>>>>>>>         df->min_freq = value;
>>>>>>> @@ -1326,6 +1335,7 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>>>>>>>                     const char *buf, size_t count)
>>>>>>>   {
>>>>>>>       struct devfreq *df = to_devfreq(dev);
>>>>>>> +    struct devfreq_stats *stats = df->profile->stats;
>>>>>>>       unsigned long value;
>>>>>>>       int ret;
>>>>>>>   @@ -1341,11 +1351,11 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>>>>>>>               goto unlock;
>>>>>>>           }
>>>>>>>       } else {
>>>>>>> -        unsigned long *freq_table = df->profile->freq_table;
>>>>>>> +        unsigned long *freq_table = stats->freq_table;
>>>>>>>             /* Get maximum frequency according to sorting order */
>>>>>>> -        if (freq_table[0] < freq_table[df->profile->max_state - 1])
>>>>>>> -            value = freq_table[df->profile->max_state - 1];
>>>>>>> +        if (freq_table[0] < freq_table[stats->max_state - 1])
>>>>>>> +            value = freq_table[stats->max_state - 1];
>>>>>>>           else
>>>>>>>               value = freq_table[0];
>>>>>>>       }
>>>>>>> @@ -1373,14 +1383,15 @@ static ssize_t available_frequencies_show(struct device *d,
>>>>>>>                         char *buf)
>>>>>>>   {
>>>>>>>       struct devfreq *df = to_devfreq(d);
>>>>>>> +    struct devfreq_stats *stats = df->profile->stats;
>>>>>>>       ssize_t count = 0;
>>>>>>>       int i;
>>>>>>>         mutex_lock(&df->lock);
>>>>>>>   -    for (i = 0; i < df->profile->max_state; i++)
>>>>>>> +    for (i = 0; i < stats->max_state; i++)
>>>>>>>           count += scnprintf(&buf[count], (PAGE_SIZE - count - 2),
>>>>>>> -                "%lu ", df->profile->freq_table[i]);
>>>>>>> +                "%lu ", stats->freq_table[i]);
>>>>>>>         mutex_unlock(&df->lock);
>>>>>>>       /* Truncate the trailing space */
>>>>>>> @@ -1398,9 +1409,10 @@ static ssize_t trans_stat_show(struct device *dev,
>>>>>>>   {
>>>>>>>       struct devfreq *devfreq = to_devfreq(dev);
>>>>>>>       struct devfreq_dev_profile *profile = devfreq->profile;
>>>>>>> +    struct devfreq_stats *stats = profile->stats;
>>>>>>> +    unsigned int max_state = stats->max_state;
>>>>>>>       ssize_t len;
>>>>>>>       int i, j;
>>>>>>> -    unsigned int max_state = profile->max_state;
>>>>>>>         if (!devfreq->stop_polling &&
>>>>>>>               devfreq_update_status(devfreq, devfreq->previous_freq))
>>>>>>> @@ -1411,45 +1423,45 @@ static ssize_t trans_stat_show(struct device *dev,
>>>>>>>       len = sprintf(buf, "     From  :   To\n");
>>>>>>>       len += sprintf(buf + len, "           :");
>>>>>>>   -    spin_lock(&profile->stats_lock);
>>>>>>> +    spin_lock(&stats->stats_lock);
>>>>>>>       for (i = 0; i < max_state; i++)
>>>>>>>           len += sprintf(buf + len, "%10lu",
>>>>>>> -                profile->freq_table[i]);
>>>>>>> +                stats->freq_table[i]);
>>>>>>>         len += sprintf(buf + len, "   time(ms)\n");
>>>>>>>         for (i = 0; i < max_state; i++) {
>>>>>>> -        if (profile->freq_table[i] == devfreq->previous_freq)
>>>>>>> +        if (stats->freq_table[i] == devfreq->previous_freq)
>>>>>>>               len += sprintf(buf + len, "*");
>>>>>>>           else
>>>>>>>               len += sprintf(buf + len, " ");
>>>>>>>             len += sprintf(buf + len, "%10lu:",
>>>>>>> -                profile->freq_table[i]);
>>>>>>> +                stats->freq_table[i]);
>>>>>>>           for (j = 0; j < max_state; j++)
>>>>>>>               len += sprintf(buf + len, "%10u",
>>>>>>> -                profile->trans_table[(i * max_state) + j]);
>>>>>>> +                stats->trans_table[(i * max_state) + j]);
>>>>>>>           len += sprintf(buf + len, "%10llu\n", (u64)
>>>>>>> -            jiffies64_to_msecs(profile->time_in_state[i]));
>>>>>>> +            jiffies64_to_msecs(stats->time_in_state[i]));
>>>>>>>       }
>>>>>>>         len += sprintf(buf + len, "Total transition : %u\n",
>>>>>>> -                    profile->total_trans);
>>>>>>> -    spin_unlock(&profile->stats_lock);
>>>>>>> +                    stats->total_trans);
>>>>>>> +    spin_unlock(&stats->stats_lock);
>>>>>>>       return len;
>>>>>>>   }
>>>>>>>   static DEVICE_ATTR_RO(trans_stat);
>>>>>>>   -static void defvreq_stats_clear_table(struct devfreq_dev_profile *profile)
>>>>>>> +static void defvreq_stats_clear_table(struct devfreq_stats *stats)
>>>>>>>   {
>>>>>>> -    unsigned int count = profile->max_state;
>>>>>>> -
>>>>>>> -    spin_lock(&profile->stats_lock);
>>>>>>> -    memset(profile->time_in_state, 0, count * sizeof(u64));
>>>>>>> -    memset(profile->trans_table, 0, count * count * sizeof(int));
>>>>>>> -    profile->last_time = get_jiffies_64();
>>>>>>> -    profile->total_trans = 0;
>>>>>>> -    spin_unlock(&profile->stats_lock);
>>>>>>> +    unsigned int count = stats->max_state;
>>>>>>> +
>>>>>>> +    spin_lock(&stats->stats_lock);
>>>>>>> +    memset(stats->time_in_state, 0, count * sizeof(u64));
>>>>>>> +    memset(stats->trans_table, 0, count * count * sizeof(int));
>>>>>>> +    stats->last_time = get_jiffies_64();
>>>>>>> +    stats->total_trans = 0;
>>>>>>> +    spin_unlock(&stats->stats_lock);
>>>>>>>   }
>>>>>>>     static ssize_t trans_reset_store(struct device *dev,
>>>>>>> @@ -1459,7 +1471,7 @@ static ssize_t trans_reset_store(struct device *dev,
>>>>>>>   {
>>>>>>>       struct devfreq *devfreq = to_devfreq(dev);
>>>>>>>   -    defvreq_stats_clear_table(devfreq->profile);
>>>>>>> +    defvreq_stats_clear_table(devfreq->profile->stats);
>>>>>>>         return count;
>>>>>>>   }
>>>>>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>>>>>> index d9f377912c10..b212aae2bb3e 100644
>>>>>>> --- a/drivers/devfreq/exynos-bus.c
>>>>>>> +++ b/drivers/devfreq/exynos-bus.c
>>>>>>> @@ -496,9 +496,9 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>>>       }
>>>>>>>     out:
>>>>>>> -    max_state = bus->devfreq->profile->max_state;
>>>>>>> -    min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
>>>>>>> -    max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
>>>>>>> +    max_state = profile->stats->max_state;
>>>>>>> +    min_freq = (profile->stats->freq_table[0] / 1000);
>>>>>>> +    max_freq = (profile->stats->freq_table[max_state - 1] / 1000);
>>>>>>>       pr_info("exynos-bus: new bus device registered: %s (%6ld KHz ~ %6ld KHz)\n",
>>>>>>>               dev_name(dev), min_freq, max_freq);
>>>>>>>   diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
>>>>>>> index 58308948b863..b2d87a88335c 100644
>>>>>>> --- a/drivers/devfreq/governor_passive.c
>>>>>>> +++ b/drivers/devfreq/governor_passive.c
>>>>>>> @@ -18,6 +18,8 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>>>>>>>       struct devfreq_passive_data *p_data
>>>>>>>               = (struct devfreq_passive_data *)devfreq->data;
>>>>>>>       struct devfreq *parent_devfreq = (struct devfreq *)p_data->parent;
>>>>>>> +    struct devfreq_stats *parent_stats = parent_devfreq->profile->stats;
>>>>>>> +    struct devfreq_stats *stats;
>>>>>>>       unsigned long child_freq = ULONG_MAX;
>>>>>>>       struct dev_pm_opp *opp;
>>>>>>>       int i, count, ret = 0;
>>>>>>> @@ -47,10 +49,14 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>>>>>>>        * device. And then the index is used for getting the suitable
>>>>>>>        * new frequency for passive devfreq device.
>>>>>>>        */
>>>>>>> -    if (!devfreq->profile || !devfreq->profile->freq_table
>>>>>>> -        || devfreq->profile->max_state <= 0)
>>>>>>> +    if (!devfreq->profile || !devfreq->profile->stats ||
>>>>>>> +        devfreq->profile->stats->max_state <= 0 ||
>>>>>>> +        !parent_devfreq->profile ||    !parent_devfreq->profile->stats ||
>>>>>>> +        parent_devfreq->profile->stats->max_state <= 0)
>>>>>>>           return -EINVAL;
>>>>>>>   +    stats = devfreq->profile->stats;
>>>>>>> +    parent_stats = parent_devfreq->profile->stats;
>>>>>>>       /*
>>>>>>>        * The passive governor have to get the correct frequency from OPP
>>>>>>>        * list of parent device. Because in this case, *freq is temporary
>>>>>>> @@ -68,21 +74,21 @@ static int devfreq_passive_get_target_freq(struct devfreq *devfreq,
>>>>>>>        * Get the OPP table's index of decided freqeuncy by governor
>>>>>>>        * of parent device.
>>>>>>>        */
>>>>>>> -    for (i = 0; i < parent_devfreq->profile->max_state; i++)
>>>>>>> -        if (parent_devfreq->profile->freq_table[i] == *freq)
>>>>>>> +    for (i = 0; i < parent_stats->max_state; i++)
>>>>>>> +        if (parent_stats->freq_table[i] == *freq)
>>>>>>>               break;
>>>>>>>   -    if (i == parent_devfreq->profile->max_state) {
>>>>>>> +    if (i == parent_stats->max_state) {
>>>>>>>           ret = -EINVAL;
>>>>>>>           goto out;
>>>>>>>       }
>>>>>>>         /* Get the suitable frequency by using index of parent device. */
>>>>>>> -    if (i < devfreq->profile->max_state) {
>>>>>>> -        child_freq = devfreq->profile->freq_table[i];
>>>>>>> +    if (i < stats->max_state) {
>>>>>>> +        child_freq = stats->freq_table[i];
>>>>>>>       } else {
>>>>>>> -        count = devfreq->profile->max_state;
>>>>>>> -        child_freq = devfreq->profile->freq_table[count - 1];
>>>>>>> +        count = stats->max_state;
>>>>>>> +        child_freq = stats->freq_table[count - 1];
>>>>>>>       }
>>>>>>>         /* Return the suitable frequency for passive device. */
>>>>>>> @@ -109,7 +115,7 @@ static int update_devfreq_passive(struct devfreq *devfreq, unsigned long freq)
>>>>>>>       if (ret < 0)
>>>>>>>           goto out;
>>>>>>>   -    if (devfreq->profile->freq_table
>>>>>>> +    if (devfreq->profile->stats
>>>>>>>           && (devfreq_update_status(devfreq, freq)))
>>>>>>>           dev_err(&devfreq->dev,
>>>>>>>               "Couldn't update frequency transition information.\n");
>>>>>>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>>>>>>> index 4ceb2a517a9c..8459af1a1583 100644
>>>>>>> --- a/include/linux/devfreq.h
>>>>>>> +++ b/include/linux/devfreq.h
>>>>>>> @@ -64,6 +64,30 @@ struct devfreq_dev_status {
>>>>>>>    */
>>>>>>>   #define DEVFREQ_FLAG_LEAST_UPPER_BOUND        0x1
>>>>>>>   +/**
>>>>>>> + * struct devfreq_stats - Devfreq's transitions stats counters
>>>>>>> + * @freq_table:        Optional list of frequencies to support statistics
>>>>>>> + *            and freq_table must be generated in ascending order.
>>>>>>> + * @max_state:        The size of freq_table.
>>>>>>> + * @total_trans:    Number of devfreq transitions
>>>>>>> + * @trans_table:    Statistics of devfreq transitions
>>>>>>> + * @time_in_state:    Statistics of devfreq states
>>>>>>> + * @last_time:        The last time stats were updated
>>>>>>> + * @stats_lock:        Lock protecting trans_table, time_in_state,
>>>>>>> + *            last_time and total_trans used for statistics
>>>>>>> + */
>>>>>>> +struct devfreq_stats {
>>>>>>> +    unsigned long *freq_table;
>>>>>>> +    unsigned int max_state;
>>>>>>> +
>>>>>>> +    /* information for device frequency transition */
>>>>>>> +    unsigned int total_trans;
>>>>>>> +    unsigned int *trans_table;
>>>>>>> +    u64 *time_in_state;
>>>>>>> +    unsigned long long last_time;
>>>>>>> +    spinlock_t stats_lock;
>>>>>>> +};
>>>>>>> +
>>>>>>>   /**
>>>>>>>    * struct devfreq_dev_profile - Devfreq's user device profile
>>>>>>>    * @initial_freq:    The operating frequency when devfreq_add_device() is
>>>>>>> @@ -88,15 +112,7 @@ struct devfreq_dev_status {
>>>>>>>    *            from devfreq_remove_device() call. If the user
>>>>>>>    *            has registered devfreq->nb at a notifier-head,
>>>>>>>    *            this is the time to unregister it.
>>>>>>> - * @freq_table:        Optional list of frequencies to support statistics
>>>>>>> - *            and freq_table must be generated in ascending order.
>>>>>>> - * @max_state:        The size of freq_table.
>>>>>>> - * @total_trans:    Number of devfreq transitions
>>>>>>> - * @trans_table:    Statistics of devfreq transitions
>>>>>>> - * @time_in_state:    Statistics of devfreq states
>>>>>>> - * @last_time:        The last time stats were updated
>>>>>>> - * @stats_lock:        Lock protecting trans_table, time_in_state,
>>>>>>> - *            last_time and total_trans used for statistics
>>>>>>> + * @stats:        Statistics of devfreq states and state transitions
>>>>>>>    */
>>>>>>>   struct devfreq_dev_profile {
>>>>>>>       unsigned long initial_freq;
>>>>>>> @@ -108,14 +124,7 @@ struct devfreq_dev_profile {
>>>>>>>       int (*get_cur_freq)(struct device *dev, unsigned long *freq);
>>>>>>>       void (*exit)(struct device *dev);
>>>>>>>   -    unsigned long *freq_table;
>>>>>>> -    unsigned int max_state;
>>>>>>> -    /* information for device frequency transition */
>>>>>>> -    unsigned int total_trans;
>>>>>>> -    unsigned int *trans_table;
>>>>>>> -    u64 *time_in_state;
>>>>>>> -    unsigned long long last_time;
>>>>>>> -    spinlock_t stats_lock;
>>>>>>> +    struct devfreq_stats *stats;
>>>>>>>   };
>>>>>>>     /**
>>>>>>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> https://protect2.fireeye.com/url?k=856913bb-d8f2efd8-856898f4-0cc47a31cdbc-5cb40ce5f31ed8ed&u=http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics



[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux for Synopsys ARC Processors]    
  • [Linux on Unisoc (RDA Micro) SoCs]     [Linux Actions SoC]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  •   Powered by Linux