Hi > -----Original Message----- > From: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx> > Sent: Thursday, July 21, 2022 10:36 AM > To: Suzuki K Poulose <suzuki.poulose@xxxxxxx> > Cc: Besar Wicaksono <bwicaksono@xxxxxxxxxx>; Robin Murphy > <robin.murphy@xxxxxxx>; catalin.marinas@xxxxxxx; will@xxxxxxxxxx; > mark.rutland@xxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; linux-tegra@xxxxxxxxxxxxxxx; > sudeep.holla@xxxxxxx; thanu.rangarajan@xxxxxxx; > Michael.Williams@xxxxxxx; Thierry Reding <treding@xxxxxxxxxx>; Jonathan > Hunter <jonathanh@xxxxxxxxxx>; Vikram Sethi <vsethi@xxxxxxxxxx>; > mike.leach@xxxxxxxxxx; leo.yan@xxxxxxxxxx > Subject: Re: [RESEND PATCH v3 1/2] perf: coresight_pmu: Add support for > ARM CoreSight PMU driver > > External email: Use caution opening links or attachments > > > On Thu, 21 Jul 2022 at 03:19, Suzuki K Poulose <suzuki.poulose@xxxxxxx> > wrote: > > > > Hi > > > > On 14/07/2022 05:47, Besar Wicaksono wrote: > > > > > > > > >> -----Original Message----- > > >> From: Robin Murphy <robin.murphy@xxxxxxx> > > >> Sent: Wednesday, July 13, 2022 3:13 AM > > >> To: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>; Besar Wicaksono > > >> <bwicaksono@xxxxxxxxxx> > > >> Cc: Suzuki K Poulose <suzuki.poulose@xxxxxxx>; > catalin.marinas@xxxxxxx; > > >> will@xxxxxxxxxx; mark.rutland@xxxxxxx; linux-arm- > > >> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux- > > >> tegra@xxxxxxxxxxxxxxx; sudeep.holla@xxxxxxx; > > >> thanu.rangarajan@xxxxxxx; Michael.Williams@xxxxxxx; Thierry > Reding > > >> <treding@xxxxxxxxxx>; Jonathan Hunter <jonathanh@xxxxxxxxxx>; > Vikram > > >> Sethi <vsethi@xxxxxxxxxx>; mike.leach@xxxxxxxxxx; leo.yan@xxxxxxxxxx > > >> Subject: Re: [RESEND PATCH v3 1/2] perf: coresight_pmu: Add support > for > > >> ARM CoreSight PMU driver > > >> > > >> External email: Use caution opening links or attachments > > >> > > >> > > >> On 2022-07-12 17:36, Mathieu Poirier wrote: > > >> [...] > > >>>>> If we have decied to call this arm_system_pmu, (which I am > perfectly > > >>>>> happy with), could we please stick to that name for functions that > we > > >>>>> export ? > > >>>>> > > >>>>> e.g, > > >>>>> > > >> > s/coresight_pmu_sysfs_event_show/arm_system_pmu_event_show()/ > > >>>>> > > >>>> > > >>>> Just want to confirm, is it just the public functions or do we need to > > >> replace > > >>>> all that has "coresight" naming ? Including the static functions, structs, > > >> filename. > > >>> > > >>> I think all references to "coresight" should be changed to > > >> "arm_system_pmu", > > >>> including filenames. That way there is no doubt this IP block is not > > >>> related, and does not interoperate, with the any of the "coresight" IP > > >> blocks > > >>> already supported[1] in the kernel. > > >>> > > >>> I have looked at the documentation[2] in the cover letter and I agree > > >>> with an earlier comment from Sudeep that this IP has very little to do > with > > >> any > > >>> of the other CoreSight IP blocks found in the CoreSight framework[1]. > > >> Using the > > >>> "coresight" naming convention in this driver would be _extremely_ > > >> confusing, > > >>> especially when it comes to exported functions. > > >> > > >> But conversely, how is it not confusing to make up completely different > > >> names for things than what they're actually called? The CoreSight > > >> Performance Monitoring Unit is a part of the Arm CoreSight > architecture, > > >> it says it right there on page 1. What if I instinctively associate the > > >> name Mathieu with someone more familiar to me, so to avoid confusion > I'd > > >> prefer to call you Steve? Is that OK? > > >> > > > > > > What is the naming convention for modules under drivers/perf ? > > > In my observation, the names there correspond to the part monitored by > > > the PMU. The confusion on using "coresight_pmu" naming could be that > > > people may think the PMU monitors coresight system, i.e the trace > system under hwtracing. > > > However, the driver in this patch is for a new PMU standard that > monitors uncore > > > parts. Uncore was considered as terminology from Intel, so "system" was > picked instead. > > > Please see this thread for reference: > > > https://lore.kernel.org/linux-arm- > kernel/20220510111318.GD27557@willie-the-truck/ > > > > I think we all understand the state of affairs. > > > > - We have an architecutre specification for PMUs, Arm CoreSight PMU > > Architecutre, which has absolutely no relationship with : > > > > either CoreSight Self-Hosted Tracing (handled by "coresight" > > subsystem in the kernel under drivers/hwtracing/coresight/, with a user > > visible pmu as "cs_etm") > > > > or the CoreSight Architecture (except for the name). This is of less > > significance in general. But has a significant impact on the "name" > > users might expect for the driver/Kconfig etc. > > > > - We want to be able to make it easier for the users/developers to > > choose what they want without causing confusion. > > > > For an end-user: Having the PMU instance named after the "System IP" > > (as implememented in the driver solves the problem and falling back to > > arm_system_pmu is a good enough choice. So let us stick with that) > > > > Kconfig: May be we can choose > > CONFIG_ARM_CORESIGHT_PMU_ARCH_PMU > > or even > > CONFIG_ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU > > > > with appropriate help text to ensure there is enough stress about what > > this is and what this is not would be sufficient. > > CONFIG_ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU sounds good to me. > > Now the remaining contention is about the name of the "subsystem" and > > also the dir/files. This may sound insignificant. But it is also > > important to get this right. e.g., helps the reviewers unambiguously > > identify the change or maintainers accepting pull requests (remember > > these two PMUs (cs_etm and this one) go via different trees.). Not > > everyone who deals with this in the community may be aware of how > > these are different. > > > > We could choose arm_cspmu_ or simply cspmu. Given that only the > > "normal" users care about the "association" with the "architecture" > > and more advanced users (e.g, developers) can easily map "Kconfig" > > to driver files, may be we could even stick to the "arm_syspmu" > > (from "arm system pmu") ? > > > > +1 on "arm_syspmu" > I am fine too with arm_syspmu. If there is no objection, I am going to post new update by end of this week or early next week. Thanks, Besar > > Suzuki > > > > > > > > > >> As it happens, Steve, I do actually agree with you that "coresight_" is > > >> a bad prefix here, but only for the reason that it's too general. TBH I > > >> think that's true of the existing Linux subsystem too, but that damage > > >> is already done, and I'd concur that there's little value in trying to > > >> unpick that now, despite the clear existence of products like CoreSight > > >> DAP and CoreSight ELA which don't have all that much to do with > program > > >> trace either. > > >> > > >> However, hindsight and inertia are hardly good reasons to double down > on > > >> poor decisions, so if I was going to vote for anything here it would be > > >> "cspmu_", which is about as > > >> obviously-related-to-the-thing-it-actually-is as we can get while also > > >> being pleasantly concise. > > >> > > >> [ And no, this isn't bikeshedding. Naming things right is *important* ] > > >> > > > > > > I agree having the correct name is important, especially at this early stage. > > > A direction of what the naming should describe would be very helpful > here. > > > > > >> Cheers, > > >> Robin. > > >> > > >>> > > >>> Thanks, > > >>> Steve > > >>> > > >>> [1]. drivers/hwtracing/coresight/ > > >>> [2]. https://developer.arm.com/documentation/ihi0091/latest > >
