Hi > -----Original Message----- > From: Suzuki K Poulose <suzuki.poulose@xxxxxxx> > Sent: Tuesday, August 2, 2022 4:39 AM > To: Besar Wicaksono <bwicaksono@xxxxxxxxxx>; Mathieu Poirier > <mathieu.poirier@xxxxxxxxxx> > Cc: 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 > > > Hi > > On 01/08/2022 23:27, Besar Wicaksono wrote: > > 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. > > Unfortunately, I have been told that we have a potential problem with > "arm_syspmu" and even choosing "arm-system-pmu" for the name as it may > conflict with something that is coming soon. So we may have to go back > to something else, to avoid this exact same conversation in the near > future. Apologies for that. > > Could we use "arm_cspmu" for the code/subsystem and may be > "arm-csarch-pmu" / "arm-cs-arch-pmu" for the device name ? > In that case, including "Coresight Architecture" in the name makes more sense. I am fine with "arm_cspmu" and "arm-cs-arch-pmu". Thanks, Besar > Other suggestions ? > > Suzuki > > > > > > 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 > >>>
