Re: [PATCH 2/3] soc: sifive: Add SiFive private L2 cache PMU driver

[Conor Dooley]

Arnd, Perf people,

On Tue, Jun 20, 2023 at 11:14:32AM +0800, Eric Lin wrote:
> On Fri, Jun 16, 2023 at 6:13 PM Conor Dooley <conor.dooley@xxxxxxxxxxxxx> wrote:
> >
> > On Fri, Jun 16, 2023 at 02:32:09PM +0800, Eric Lin wrote:
> > > From: Greentime Hu <greentime.hu@xxxxxxxxxx>
> > >
> > > This adds SiFive private L2 cache PMU driver. User
> > > can use perf tool to profile by event name and event id.
> > >
> > > Example:
> > > $ perf stat -C 0 -e /sifive_pl2_pmu/inner_acquire_block_btot/
> > >                 -e /sifive_pl2_pmu/inner_acquire_block_ntob/
> > >                 -e /sifive_pl2_pmu/inner_acquire_block_ntot/ ls
> > >
> > >  Performance counter stats for 'CPU(s) 0':
> > >
> > >                300      sifive_pl2_pmu/inner_acquire_block_btot/
> > >              17801      sifive_pl2_pmu/inner_acquire_block_ntob/
> > >               5253      sifive_pl2_pmu/inner_acquire_block_ntot/
> > >
> > >        0.088917326 seconds time elapsed
> > >
> > > $ perf stat -C 0 -e /sifive_pl2_pmu/event=0x10001/
> > >                 -e /sifive_pl2_pmu/event=0x4001/
> > >                 -e /sifive_pl2_pmu/event=0x8001/ ls
> > >
> > >  Performance counter stats for 'CPU(s) 0':
> > >
> > >                251      sifive_pl2_pmu/event=0x10001/
> > >               2620      sifive_pl2_pmu/event=0x4001/
> > >                644      sifive_pl2_pmu/event=0x8001/
> > >
> > >        0.092827110 seconds time elapsed
> > >
> > > Signed-off-by: Greentime Hu <greentime.hu@xxxxxxxxxx>
> > > Signed-off-by: Eric Lin <eric.lin@xxxxxxxxxx>
> > > Reviewed-by: Zong Li <zong.li@xxxxxxxxxx>
> > > Reviewed-by: Nick Hu <nick.hu@xxxxxxxxxx>
> > > ---
> > >  drivers/soc/sifive/Kconfig            |   9 +
> > >  drivers/soc/sifive/Makefile           |   1 +
> > >  drivers/soc/sifive/sifive_pl2.h       |  20 +
> > >  drivers/soc/sifive/sifive_pl2_cache.c |  16 +
> > >  drivers/soc/sifive/sifive_pl2_pmu.c   | 669 ++++++++++++++++++++++++++
> >
> > Perf drivers should be in drivers/perf, no?
> >
> 
> Hi Conor,
> 
> Yes, I see most of the drivers are in the drivers/perf.
> 
> But I grep perf_pmu_register(), it seems not all the pmu drivers are
> in drivers/perf as below:
> 
> arch/arm/mach-imx/mmdc.c:517:   ret =
> perf_pmu_register(&(pmu_mmdc->pmu), name, -1);
> arch/arm/mm/cache-l2x0-pmu.c:552:       ret =
> perf_pmu_register(l2x0_pmu, l2x0_name, -1);
> ...
> drivers/dma/idxd/perfmon.c:627: rc = perf_pmu_register(&idxd_pmu->pmu,
> idxd_pmu->name, -1);
> drivers/fpga/dfl-fme-perf.c:904:static int
> fme_perf_pmu_register(struct platform_device *pdev,
> drivers/fpga/dfl-fme-perf.c:929:        ret = perf_pmu_register(pmu, name, -1);
> ...
> drivers/gpu/drm/amd/amdgpu/amdgpu_pmu.c:549:    ret =
> perf_pmu_register(&pmu_entry->pmu, pmu_name, -1);
> drivers/gpu/drm/i915/i915_pmu.c:1190:   ret =
> perf_pmu_register(&pmu->base, pmu->name, -1);
> drivers/hwtracing/coresight/coresight-etm-perf.c:907:   ret =
> perf_pmu_register(&etm_pmu, CORESIGHT_ETM_PMU_NAME, -1);
> drivers/hwtracing/ptt/hisi_ptt.c:895:   ret =
> perf_pmu_register(&hisi_ptt->hisi_ptt_pmu, pmu_name, -1);
> drivers/iommu/intel/perfmon.c:570:      return
> perf_pmu_register(&iommu_pmu->pmu, iommu_pmu->pmu.name, -1);
> drivers/nvdimm/nd_perf.c:309:   rc = perf_pmu_register(&nd_pmu->pmu,
> nd_pmu->pmu.name, -1);
> ...
> 
> I just wondering what kind of pmu drivers should be in drivers/perf
> and what kind of pmu drivers should not be in drivers/perf.
> Thanks.

To be quite honest, I have no idea.
I'm just a wee bit wary of taking anything that appears to have another
home via drivers/soc. I'd rather break drivers out, using the aux bus or
similar if need be, so that people who are knowledgeable in an area are
CCed on patches.
Hopefully Arnd or the Perf people can offer some guidance here. If it
does go into drivers/soc, it'll need a review from someone knowledgeable
of perf anyway.

Cheers,
Conor.

Attachment: signature.asc
Description: PGP signature