lkml.org 
[lkml]   [2022]   [Aug]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [RESEND PATCH v3 1/2] perf: coresight_pmu: Add support for ARM CoreSight PMU driver
Date
Hi

> -----Original Message-----
> From: Mathieu Poirier <mathieu.poirier@linaro.org>
> Sent: Thursday, July 21, 2022 10:36 AM
> To: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Besar Wicaksono <bwicaksono@nvidia.com>; Robin Murphy
> <robin.murphy@arm.com>; catalin.marinas@arm.com; will@kernel.org;
> mark.rutland@arm.com; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; linux-tegra@vger.kernel.org;
> sudeep.holla@arm.com; thanu.rangarajan@arm.com;
> Michael.Williams@arm.com; Thierry Reding <treding@nvidia.com>; Jonathan
> Hunter <jonathanh@nvidia.com>; Vikram Sethi <vsethi@nvidia.com>;
> mike.leach@linaro.org; leo.yan@linaro.org
> 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@arm.com>
> wrote:
> >
> > Hi
> >
> > On 14/07/2022 05:47, Besar Wicaksono wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Robin Murphy <robin.murphy@arm.com>
> > >> Sent: Wednesday, July 13, 2022 3:13 AM
> > >> To: Mathieu Poirier <mathieu.poirier@linaro.org>; Besar Wicaksono
> > >> <bwicaksono@nvidia.com>
> > >> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>;
> catalin.marinas@arm.com;
> > >> will@kernel.org; mark.rutland@arm.com; linux-arm-
> > >> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> > >> tegra@vger.kernel.org; sudeep.holla@arm.com;
> > >> thanu.rangarajan@arm.com; Michael.Williams@arm.com; Thierry
> Reding
> > >> <treding@nvidia.com>; Jonathan Hunter <jonathanh@nvidia.com>;
> Vikram
> > >> Sethi <vsethi@nvidia.com>; mike.leach@linaro.org; leo.yan@linaro.org
> > >> 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
> >
\
 
 \ /
  Last update: 2022-08-02 00:29    [W:0.244 / U:0.316 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site