[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <2a6dde28-a74d-0992-3f9d-7346e36a0b9c@huawei.com>
Date: Fri, 23 Feb 2024 12:05:24 +0800
From: hejunhao <hejunhao3@...wei.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
CC: <will@...nel.org>, <linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>, <linuxarm@...wei.com>,
<yangyicong@...wei.com>, <prime.zeng@...ilicon.com>
Subject: Re: Re: [PATCH] drivers/perf: hisi: Enable HiSilicon Erratum
162700402 quirk for UC PMU
On 2024/2/8 18:50, Jonathan Cameron wrote:
> On Wed, 7 Feb 2024 17:42:45 +0800
> Junhao He <hejunhao3@...wei.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@...wei.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,
>
Powered by blists - more mailing lists