RE: [PATCH v4 1/2] perf: arm_cspmu: Add support for ARM CoreSight PMU driver

[Besar Wicaksono <bwicaksono@nvidia.com>]

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