Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2] cpufreq / CPPC: Add cpuinfo_cur_freq support for CPPC
To:     "Prakash, Prashanth" <pprakash@codeaurora.org>,
        George Cherian <george.cherian@cavium.com>,
        linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org
Cc:     viresh.kumar@linaro.org, rjw@rjwysocki.net
References: <1529056995-122792-1-git-send-email-george.cherian@cavium.com>
 <67822500-0c9d-ac24-71bc-2717831ab29d@codeaurora.org>
From:   George Cherian <gcherian@caviumnetworks.com>
Message-ID: <44d52166-ed9d-c199-3e19-3df1317ee78c@caviumnetworks.com>
Date:   Wed, 20 Jun 2018 14:47:02 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.8.0
MIME-Version: 1.0
In-Reply-To: <67822500-0c9d-ac24-71bc-2717831ab29d@codeaurora.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Received-SPF: None (protection.outlook.com: cavium.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Jun 2018 09:17:22.3508 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: cc4fcb03-f82e-4db6-0a05-08d5d68ea26a
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 711e4ccf-2e9b-4bcf-a551-4094005b6194
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL0PR07MB4914
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Prakash,

Thanks for the review.

On 06/19/2018 01:51 AM, Prakash, Prashanth wrote:
> External Email
> 
> Hi George,
> 
> On 6/15/2018 4:03 AM, George Cherian wrote:
>> Per Section 8.4.7.1.3 of ACPI 6.2, The platform provides performance
>> feedback via set of performance counters. To determine the actual
>> performance level delivered over time, OSPM may read a set of
>> performance counters from the Reference Performance Counter Register
>> and the Delivered Performance Counter Register.
>>
>> OSPM calculates the delivered performance over a given time period by
>> taking a beginning and ending snapshot of both the reference and
>> delivered performance counters, and calculating:
>>
>> delivered_perf = reference_perf X (delta of delivered_perf counter / delta of reference_perf counter).
>>
>> Implement the above and hook this to the cpufreq->get method.
>>
>> Signed-off-by: George Cherian <george.cherian@cavium.com>
>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>>   drivers/cpufreq/cppc_cpufreq.c | 71 ++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 71 insertions(+)
>>
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index 3464580..3fe7625 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -296,10 +296,81 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>>        return ret;
>>   }
>>
>> +static int cppc_get_rate_from_fbctrs(struct cppc_cpudata *cpu,
>> +                                  struct cppc_perf_fb_ctrs fb_ctrs_t0,
>> +                                  struct cppc_perf_fb_ctrs fb_ctrs_t1)
>> +{
>> +     u64 delta_reference, delta_delivered;
>> +     u64 reference_perf, delivered_perf;
>> +
>> +     reference_perf = fb_ctrs_t0.reference_perf;
>> +     if (fb_ctrs_t1.reference > fb_ctrs_t0.reference) {
>> +             delta_reference = fb_ctrs_t1.reference - fb_ctrs_t0.reference;
>> +     } else {
> There should be another if () here to check if the reference counters are equal.
> We cannot assume, there was a overflow when the counters are equal. As I
> mentioned on last patch, the counters *may* pause in idle states.
My Bad... I somehow, over looked that point. In case of delta_reference 
being zero there is actually a check below to avoid divide-by-zero. 
There I returned  reference perf instead of desired perf, same I will 
take care in v3. Isn't that sufficient or is there a need for an 
explicit check here for delta = zero?

Moreover the delta calculation am planning to replace with single
line comparison in v3 for both normal and overflow case.
>> +             /*
>> +              * Counters would have wrapped-around
>> +              * We also need to find whether the low level fw
>> +              * maintains 32 bit or 64 bit counters, to calculate
>> +              * the correct delta.
>> +              */
>> +             if (fb_ctrs_t0.reference > (~(u32)0))
>> +                     delta_reference  = (~((u64)0) - fb_ctrs_t0.reference) +
>> +                                     fb_ctrs_t1.reference;
>> +             else
>> +                     delta_reference  = (~((u32)0) - fb_ctrs_t0.reference) +
>> +                                     fb_ctrs_t1.reference;
>> +     }
>> +
>> +     if (fb_ctrs_t1.delivered > fb_ctrs_t0.delivered) {
>> +             delta_delivered = fb_ctrs_t1.delivered - fb_ctrs_t0.delivered;
>> +     } else {
>> +             /*
>> +              * Counters would have wrapped-around
>> +              * We also need to find whether the low level fw
>> +              * maintains 32 bit or 64 bit counters, to calculate
>> +              * the correct delta.
>> +              */
>> +             if (fb_ctrs_t0.delivered > (~(u32)0))
>> +                     delta_delivered  = (~((u64)0) - fb_ctrs_t0.delivered) +
>> +                                     fb_ctrs_t1.delivered;
>> +             else
>> +                     delta_delivered  = (~((u32)0) - fb_ctrs_t0.delivered) +
>> +                                     fb_ctrs_t1.delivered;
>> +     }
>> +
>> +     if (delta_reference)  /* Check to avoid divide-by zero */
>> +             delivered_perf = (reference_perf * delta_delivered) /
>> +                                     delta_reference;
>> +     else
>> +             delivered_perf = reference_perf;
> 
> If we cannot compute delivered performance then we should return
> desired/requested perf and not reference_perf.
> 
Noted!!
>> +
>> +     return cppc_cpufreq_perf_to_khz(cpu, delivered_perf);
>> +}
>> +
>> +static unsigned int cppc_cpufreq_get_rate(unsigned int cpunum)
>> +{
>> +     struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
>> +     struct cppc_cpudata *cpu = all_cpu_data[cpunum];
>> +     int ret;
>> +
>> +     ret = cppc_get_perf_ctrs(cpunum, &fb_ctrs_t0);
>> +     if (ret)
>> +             return ret;
>> +
>> +     udelay(2); /* 2usec delay between sampling */
>> +
>> +     ret = cppc_get_perf_ctrs(cpunum, &fb_ctrs_t1);
>> +     if (ret)
>> +             return ret;
>> +
>> +     return cppc_get_rate_from_fbctrs(cpu, fb_ctrs_t0, fb_ctrs_t1);
>> +}
>> +
>>   static struct cpufreq_driver cppc_cpufreq_driver = {
>>        .flags = CPUFREQ_CONST_LOOPS,
>>        .verify = cppc_verify_policy,
>>        .target = cppc_cpufreq_set_target,
>> +     .get = cppc_cpufreq_get_rate,
>>        .init = cppc_cpufreq_cpu_init,
>>        .stop_cpu = cppc_cpufreq_stop_cpu,
>>        .name = "cppc_cpufreq",
> 
> Thanks,
> Prashanth
> 

Thanks,
-George