1. Patchew
  2. linux
  3. [PATCH 0/8] perf/arm-cmn: Fixes and updates
  4. View patch

[PATCH 2/8] perf/arm-cmn: Fix CCLA register offset

Robin Murphy posted 8 patches 1 year, 6 months ago
There is a newer version of this series
[PATCH 2/8] perf/arm-cmn: Fix CCLA register offset
Posted by Robin Murphy 1 year, 6 months ago 
Apparently pmu_event_sel is offset by 8 for all CCLA nodes, not just
the CCLA_RNI combination type.

Fixes: 23760a014417 ("perf/arm-cmn: Add CMN-700 support")
Signed-off-by: Robin Murphy <robin.murphy@arm.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;
 			/* Nothing to see here */
 			case CMN_TYPE_MPAM_S:
 			case CMN_TYPE_MPAM_NS:
-- 
2.39.2.101.g768bb238c484.dirty
Re: [PATCH 2/8] perf/arm-cmn: Fix CCLA register offset
Posted by Mark Rutland 1 year, 5 months ago 
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?

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@arm.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.

With that:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

>  			/* Nothing to see here */
>  			case CMN_TYPE_MPAM_S:
>  			case CMN_TYPE_MPAM_NS:
> -- 
> 2.39.2.101.g768bb238c484.dirty
>
Re: [PATCH 2/8] perf/arm-cmn: Fix CCLA register offset
Posted by Robin Murphy 1 year, 5 months ago 
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@arm.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@arm.com>

Thanks,
Robin.

> 
> Mark.
> 
>>   			/* Nothing to see here */
>>   			case CMN_TYPE_MPAM_S:
>>   			case CMN_TYPE_MPAM_NS:
>> -- 
>> 2.39.2.101.g768bb238c484.dirty
>>
Re: [PATCH 2/8] perf/arm-cmn: Fix CCLA register offset
Posted by Ilkka Koskinen 1 year, 5 months ago 
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@arm.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@arm.com>
>
> Thanks,
> Robin.


I like Mark's idea. With the change,

Reviewed-by: Ilkka Koskinen <ilkka@os.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
>>> 
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.