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

[hejunhao]

On 2024/2/8 18:50, Jonathan Cameron wrote:
> On Wed, 7 Feb 2024 17:42:45 +0800
> Junhao He <hejunhao3@xxxxxxxxxx> 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@xxxxxxxxxx>
> 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,