[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b54f8cf-d37d-6eb3-cbac-9852a6802b7e@os.amperecomputing.com>
Date: Fri, 23 Aug 2024 16:18:45 -0700 (PDT)
From: Ilkka Koskinen <ilkka@...amperecomputing.com>
To: Robin Murphy <robin.murphy@....com>
cc: Mark Rutland <mark.rutland@....com>, will@...nel.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
ilkka@...amperecomputing.com
Subject: Re: [PATCH 2/8] perf/arm-cmn: Fix CCLA register offset
Hi Robin,
Sorry for replying late but I just got back from my vacation
On Fri, 16 Aug 2024, Robin Murphy wrote:
> On 16/08/2024 11:00 am, Mark Rutland wrote:
>> On Fri, Aug 09, 2024 at 08:15:41PM +0100, Robin Murphy wrote:
>>> Apparently pmu_event_sel is offset by 8 for all CCLA nodes, not just
>>> the CCLA_RNI combination type.
>>
>> Was there some reason we used to think that was specific to CCLA_RNI
>> nodes, or was that just an oversight?
>
> I imagine it was just oversight/confusion helped by the original r0p0 TRM
> listing both a por_ccla_pmu_event_sel and a por_ccla_rni_pmu_event_sel as
> CCLA registers, which I could well believe I misread at a glance while
> scrolling up and down.
>
>> Looking at the CMN-700 TRM and scanning for pmu_event_sel, we have:
>>
>> 16'h2000 por_ccg_ha_pmu_event_sel
>> 16'h2000 por_ccg_ra_pmu_event_sel
>> 16'h2008 por_ccla_pmu_event_sel
>> 16'h2000 por_dn_pmu_event_sel
>> 16'h2000 cmn_hns_pmu_event_sel
>> 16'h2000 por_hni_pmu_event_sel
>> 16'h2008 por_hnp_pmu_event_sel
>> 16'h2000 por_mxp_pmu_event_sel
>> 16'h2000 por_rnd_pmu_event_sel
>> 16'h2000 por_rni_pmu_event_sel
>> 16'h2000 por_sbsx_pmu_event_sel
>>
>>> Fixes: 23760a014417 ("perf/arm-cmn: Add CMN-700 support")
>>> Signed-off-by: Robin Murphy <robin.murphy@....com>
>>> ---
>>> drivers/perf/arm-cmn.c | 5 ++++-
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
>>> index fd2122a37f22..0e2e12e2f4fb 100644
>>> --- a/drivers/perf/arm-cmn.c
>>> +++ b/drivers/perf/arm-cmn.c
>>> @@ -2393,10 +2393,13 @@ static int arm_cmn_discover(struct arm_cmn *cmn,
>>> unsigned int rgn_offset)
>>> case CMN_TYPE_CXHA:
>>> case CMN_TYPE_CCRA:
>>> case CMN_TYPE_CCHA:
>>> - case CMN_TYPE_CCLA:
>>> case CMN_TYPE_HNS:
>>> dn++;
>>> break;
>>> + case CMN_TYPE_CCLA:
>>> + dn->pmu_base += CMN_HNP_PMU_EVENT_SEL;
>>> + dn++;
>>> + break;
>>
>> When reading this for the first time, it looks like a copy-paste error
>> since CMN_HNP_PMU_EVENT_SEL doesn't have any obvious relationship with
>> CCLA nodes.
>>
>> I reckon it'd be worth adding CMN_CCLA_PMU_EVENT_SEL, and replacing the
>> existing comment above the definition of CMN_HNP_PMU_EVENT_SEL, e.g.
>>
>> /*
>> * Some nodes place common registers at different offsets from most
>> * other nodes.
>> */
>> #define CMN_HNP_PMU_EVENT_SEL 0x008
>> #define CMN_CCLA_PMU_EVENT_SEL 0x008
>>
>> That way the switch looks less suspicious, and the comment is a bit more
>> helpful to anyone trying to figure out what's going on here.
>
> Sure, that's a reasonable argument.
>
>> With that:
>>
>> Acked-by: Mark Rutland <mark.rutland@....com>
>
> Thanks,
> Robin.
I like Mark's idea. With the change,
Reviewed-by: Ilkka Koskinen <ilkka@...amperecomputing.com>
Cheers, Ilkka
>
>>
>> Mark.
>>
>>> /* Nothing to see here */
>>> case CMN_TYPE_MPAM_S:
>>> case CMN_TYPE_MPAM_NS:
>>> --
>>> 2.39.2.101.g768bb238c484.dirty
>>>
>
Powered by blists - more mailing lists