lkml.org 
[lkml]   [2022]   [Sep]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v4 1/2] perf: arm_cspmu: Add support for ARM CoreSight PMU driver
Date
Hi Will,

Thanks for the comment. Please see my response inline.

> -----Original Message-----
> From: Will Deacon <will@kernel.org>
> Sent: Thursday, September 22, 2022 8:53 AM
> To: Besar Wicaksono <bwicaksono@nvidia.com>
> Cc: suzuki.poulose@arm.com; robin.murphy@arm.com;
> catalin.marinas@arm.com; 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>; mathieu.poirier@linaro.org;
> mike.leach@linaro.org; leo.yan@linaro.org
> Subject: Re: [PATCH v4 1/2] perf: arm_cspmu: Add support for ARM
> CoreSight PMU driver
>
> External email: Use caution opening links or attachments
>
>
> On Sun, Aug 14, 2022 at 01:23:50PM -0500, Besar Wicaksono wrote:
> > Add support for ARM CoreSight PMU driver framework and interfaces.
> > The driver provides generic implementation to operate uncore PMU based
> > on ARM CoreSight PMU architecture. The driver also provides interface
> > to get vendor/implementation specific information, for example event
> > attributes and formating.
> >
> > The specification used in this implementation can be found below:
> > * ACPI Arm Performance Monitoring Unit table:
> > https://developer.arm.com/documentation/den0117/latest
> > * ARM Coresight PMU architecture:
> > https://developer.arm.com/documentation/ihi0091/latest
> >
> > Signed-off-by: Besar Wicaksono <bwicaksono@nvidia.com>
> > ---
> > arch/arm64/configs/defconfig | 1 +
> > drivers/perf/Kconfig | 2 +
> > drivers/perf/Makefile | 1 +
> > drivers/perf/arm_cspmu/Kconfig | 13 +
> > drivers/perf/arm_cspmu/Makefile | 6 +
> > drivers/perf/arm_cspmu/arm_cspmu.c | 1262
> ++++++++++++++++++++++++++++
> > drivers/perf/arm_cspmu/arm_cspmu.h | 151 ++++
> > 7 files changed, 1436 insertions(+)
> > create mode 100644 drivers/perf/arm_cspmu/Kconfig
> > create mode 100644 drivers/perf/arm_cspmu/Makefile
> > create mode 100644 drivers/perf/arm_cspmu/arm_cspmu.c
> > create mode 100644 drivers/perf/arm_cspmu/arm_cspmu.h
>
> [...]
>
> > diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c
> b/drivers/perf/arm_cspmu/arm_cspmu.c
> > new file mode 100644
> > index 000000000000..410876f86eb0
> > --- /dev/null
> > +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
>
> [...]
>
> > +/*
> > + * Read 64-bit register as a pair of 32-bit registers using hi-lo-hi sequence.
> > + */
> > +static u64 read_reg64_hilohi(const void __iomem *addr)
> > +{
> > + u32 val_lo, val_hi;
> > + u64 val;
> > +
> > + /* Use high-low-high sequence to avoid tearing */
> > + do {
> > + val_hi = readl(addr + 4);
> > + val_lo = readl(addr);
> > + } while (val_hi != readl(addr + 4));
>
> Hmm, we probably want a timeout or something in here so we don't lock
> up the CPU if the device goes wonky.
>

This function is used to read the counter register. The perf driver APIs
(read, stop) that use this function do not return an error code. I am
not sure if we can just break the loop and return 0. Any suggestions ?
Is triggering a panic acceptable ?

> With that, how about adding this a helper to
> include/linux/io-64-nonatomic-*o.h so other folks can reuse it?
>
> > +/* Check if PMU supports 64-bit single copy atomic. */
> > +static inline bool supports_64bit_atomics(const struct arm_cspmu
> *cspmu)
> > +{
> > + return CHECK_APMT_FLAG(cspmu->apmt_node->flags, ATOMIC,
> SUPP);
> > +}
>
> Is this just there because the architecture permits it, or are folks
> actually hanging these things off 32-bit MMIO buses on arm64 SoCs?
>

Yes, the PMU spec permits a system that needs to break 64-bit access into
a pair of 32-bit accesses.

> > +static int arm_cspmu_request_irq(struct arm_cspmu *cspmu)
> > +{
> > + int irq, ret;
> > + struct device *dev;
> > + struct platform_device *pdev;
> > + struct acpi_apmt_node *apmt_node;
> > +
> > + dev = cspmu->dev;
> > + pdev = to_platform_device(dev);
> > + apmt_node = cspmu->apmt_node;
> > +
> > + /* Skip IRQ request if the PMU does not support overflow interrupt. */
> > + if (apmt_node->ovflw_irq == 0)
> > + return 0;
>
> Set PERF_PMU_CAP_NO_INTERRUPT?
>

Thanks, I will apply it on the next version.

> Will

\
 
 \ /
  Last update: 2022-09-27 06:00    [W:0.123 / U:0.244 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site