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

hejunhao
2024-02-23T04:05:29Z

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,
>