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

[Robin Murphy]

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@xxxxxxx>
> > ---
> >   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@xxxxxxx>

Thanks,
Robin.

> Mark.
>
> >   			/* Nothing to see here */
> >   			case CMN_TYPE_MPAM_S:
> >   			case CMN_TYPE_MPAM_NS:
> > --
> > 2.39.2.101.g768bb238c484.dirty