Re: [PATCH v2] perf: imx9_perf: Introduce AXI filter version to refactor the driver and better extension

[Will Deacon <will@kernel.org>]

On Tue, Dec 10, 2024 at 10:02:12AM +0800, Xu Yang wrote:
> On Mon, Dec 09, 2024 at 03:44:20PM +0000, Will Deacon wrote:
> > On Mon, Nov 25, 2024 at 06:43:38PM +0800, Xu Yang wrote:
> > > The imx93 is the first supported DDR PMU that supports read transaction,
> > > write transaction and read beats events which corresponding respecitively
> > > to counter 2, 3 and 4.
> > > 
> > > However, transaction-based AXI match has low accuracy when get total bits
> > > compared to beats-based. And imx93 doesn't assign AXI_ID to each master.
> > > So axi filter is not used widely on imx93. This could be regards as AXI
> > > filter version 1.
> > > 
> > > To improve the AXI filter capability, imx95 supports 1 read beats and 3
> > > write beats event which corresponding respecitively to counter 2-5. imx95
> > > also detailed AXI_ID allocation so that most of the master could be count
> > > individually. This could be regards as AXI filter version 2.
> > > 
> > > This will introduce AXI filter version to refactor the driver and support
> > > better extension, such as coming imx943.
> > > 
> > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > > 
> > > ---
> > > Changes in v2:
> > >  - modify subject
> > >  - add comments for AXI_FILTER version
> > >  - type -> filter_ver
> > > ---
> > >  drivers/perf/fsl_imx9_ddr_perf.c | 33 ++++++++++++++++++++++++--------
> > >  1 file changed, 25 insertions(+), 8 deletions(-)

[...]

> > > @@ -624,11 +641,11 @@ static int ddr_perf_event_add(struct perf_event *event, int flags)
> > >  	hwc->idx = counter;
> > >  	hwc->state |= PERF_HES_STOPPED;
> > >  
> > > -	if (is_imx93(pmu))
> > > +	if (axi_filter_v1(pmu))
> > >  		/* read trans, write trans, read beat */
> > >  		imx93_ddr_perf_monitor_config(pmu, event_id, counter, cfg1, cfg2);
> > 
> > Hmm, doesn't this change mean we now enable this for imx91 as well? My
> > reading of the commit message is that imx93 was the first chip which
> > supports this.
> 
> Yes, it's enabled for imx91 too. In fact, imx91 is compatible with imx93.
> They use same configuration for axi filter.

Ok, but my worry is that the above code looks like userspace now _must_
provide valid values for the config1 (axi_id) and config2 (axi_mask)
fields on imx91, whereas before I think they were ignored by the driver.

In fact, without this change, how were the PMCFGn registers configured
on imx91? It looks to me like they were left uninitialised...

Will