Hi Will, Thanks for the comment. Please see my response inline. > -----Original Message----- > From: Will Deacon <will@xxxxxxxxxx> > Sent: Thursday, September 22, 2022 8:53 AM > To: Besar Wicaksono <bwicaksono@xxxxxxxxxx> > Cc: suzuki.poulose@xxxxxxx; robin.murphy@xxxxxxx; > catalin.marinas@xxxxxxx; 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>; mathieu.poirier@xxxxxxxxxx; > mike.leach@xxxxxxxxxx; leo.yan@xxxxxxxxxx > 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@xxxxxxxxxx> > > --- > > 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
