Re: Re: [PATCH] drivers/perf: hisi: Enable HiSilicon Erratum 162700402 quirk for UC PMU

[hejunhao <hejunhao3@huawei.com>]

On 2024/2/8 18:50, Jonathan Cameron wrote:
> On Wed, 7 Feb 2024 17:42:45 +0800
> Junhao He <hejunhao3@huawei.com> wrote:
>
>> HiSilicon UC PMU v2 suffers the erratum 162700402 that the PMU counter
>> cannot be set due to the lack of clock under power saving mode. This will
>> lead to error or inaccurate counts. The clock can be enabled by the PMU
>> global enabling control.
>>
>> This patch tries to fix this by set the UC PMU enable before set event
>> period to turn on the clock, and then restore the UC PMU configuration.
>> The counter register can hold its value without a clock.
>>
>> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> Hi.
>
> Some very minor comments about the comments inline.

Hi, Jonathan


Thanks for the comments.

>
> Jonathan
>
>> ---
>>   drivers/perf/hisilicon/hisi_uncore_uc_pmu.c | 40 ++++++++++++++++++++-
>>   1 file changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
>> index 636fb79647c8..8e7a9e1f419a 100644
>> --- a/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
>> +++ b/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
>> @@ -287,10 +287,48 @@ static u64 hisi_uc_pmu_read_counter(struct hisi_pmu *uc_pmu,
>>   	return readq(uc_pmu->base + HISI_UC_CNTR_REGn(hwc->idx));
>>   }
>>   
>> -static void hisi_uc_pmu_write_counter(struct hisi_pmu *uc_pmu,
>> +static bool hisi_uc_pmu_get_glb_en_state(struct hisi_pmu *uc_pmu)
>> +{
>> +	u32 val;
>> +
>> +	val = readl(uc_pmu->base + HISI_UC_EVENT_CTRL_REG);
>> +	return !!FIELD_GET(HISI_UC_EVENT_GLB_EN, val);
>> +}
>> +
>> +static void hisi_uc_pmu_write_counter_quirk_hip09(struct hisi_pmu *uc_pmu,
>>   				      struct hw_perf_event *hwc, u64 val)
>>   {
>> +	bool enable = hisi_uc_pmu_get_glb_en_state(uc_pmu);
>> +
>> +	/* Set the UC PMU enable to turn on the clock. */
> Comment from below here but adjusted to say which path has the device
> already enabled.

This quirk function is simple. I'm going to consolidate all the comments
to annotate the quirk function.

Best regards,
Junhao.

>> +	if (!enable)
>> +		hisi_uc_pmu_start_counters(uc_pmu);
>> +
>>   	writeq(val, uc_pmu->base + HISI_UC_CNTR_REGn(hwc->idx));
>> +
>> +	/*
>> +	 * The counter register can hold its value without a clock. We need
>> +	 * restore the UC PMU configuration. The irq handler will also call
>> +	 * the function to set period. At this time, PMU is still enabled and
>> +	 * we cannot directly disable the PMU.
> I think the comment is more relevant above...

Okay, I'm going to replace the comment with the following

        /*
         * Restore PMU enable status, the counter can hold its value
         * without a clock.
         */

>> +	 */
>> +	if (!enable)
>> +		hisi_uc_pmu_stop_counters(uc_pmu);
>> +}
>> +
>> +static void hisi_uc_pmu_write_counter(struct hisi_pmu *uc_pmu,
>> +				      struct hw_perf_event *hwc, u64 val)
>> +{
>> +	/*
>> +	 * HiSilicon UC PMU v2 suffers the erratum 162700402 that the PMU
>> +	 * counter cannot be set due to the lack of clock under power saving
>> +	 * mode. This will lead to error or inaccurate counts. The clock can
>> +	 * be enabled by the PMU global enabling control.
> I'd move the comment to next to the quirk function so that people can immediately
> see what is being done.  Down here we just need to know there is a quirk.

I'd comment the quirk function.


>
>> +	 */
>> +	if (uc_pmu->identifier == HISI_PMU_V2)
>> +		hisi_uc_pmu_write_counter_quirk_hip09(uc_pmu, hwc, val);
>> +	else
>> +		writeq(val, uc_pmu->base + HISI_UC_CNTR_REGn(hwc->idx));
>>   }
>>   
>>   static void hisi_uc_pmu_enable_counter_int(struct hisi_pmu *uc_pmu,
>