[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <SJ0PR12MB56763459EE6CE776680E0762A0559@SJ0PR12MB5676.namprd12.prod.outlook.com>
Date: Tue, 27 Sep 2022 03:59:22 +0000
From: Besar Wicaksono <bwicaksono@...dia.com>
To: Will Deacon <will@...nel.org>
CC: "suzuki.poulose@....com" <suzuki.poulose@....com>,
"robin.murphy@....com" <robin.murphy@....com>,
"catalin.marinas@....com" <catalin.marinas@....com>,
"mark.rutland@....com" <mark.rutland@....com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
"sudeep.holla@....com" <sudeep.holla@....com>,
"thanu.rangarajan@....com" <thanu.rangarajan@....com>,
"Michael.Williams@....com" <Michael.Williams@....com>,
Thierry Reding <treding@...dia.com>,
Jonathan Hunter <jonathanh@...dia.com>,
Vikram Sethi <vsethi@...dia.com>,
"mathieu.poirier@...aro.org" <mathieu.poirier@...aro.org>,
"mike.leach@...aro.org" <mike.leach@...aro.org>,
"leo.yan@...aro.org" <leo.yan@...aro.org>
Subject: RE: [PATCH v4 1/2] perf: arm_cspmu: Add support for ARM CoreSight PMU
driver
Hi Will,
Thanks for the comment. Please see my response inline.
> -----Original Message-----
> From: Will Deacon <will@...nel.org>
> Sent: Thursday, September 22, 2022 8:53 AM
> To: Besar Wicaksono <bwicaksono@...dia.com>
> Cc: suzuki.poulose@....com; robin.murphy@....com;
> catalin.marinas@....com; mark.rutland@....com; linux-arm-
> kernel@...ts.infradead.org; linux-kernel@...r.kernel.org; linux-
> tegra@...r.kernel.org; sudeep.holla@....com;
> thanu.rangarajan@....com; Michael.Williams@....com; Thierry Reding
> <treding@...dia.com>; Jonathan Hunter <jonathanh@...dia.com>; Vikram
> Sethi <vsethi@...dia.com>; mathieu.poirier@...aro.org;
> mike.leach@...aro.org; leo.yan@...aro.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@...dia.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
Powered by blists - more mailing lists