From: Mark Rutland <mark.rutland@arm.com>
To: Gowthami Thiagarajan <gthiagarajan@marvell.com>
Cc: will@kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, sgoutham@marvell.com,
bbhushan2@marvell.com, gcherian@marvell.com,
lcherian@marvell.com
Subject: Re: [PATCH 1/6] perf/marvell: Marvell PEM performance monitor support
Date: Fri, 28 Jul 2023 16:01:54 +0100 [thread overview]
Message-ID: <ZMPYYuwg7wHGXft4@FVFF77S0Q05N> (raw)
In-Reply-To: <20230630120351.1143773-2-gthiagarajan@marvell.com>
Hi,
On Fri, Jun 30, 2023 at 05:33:46PM +0530, Gowthami Thiagarajan wrote:
> PCI Express Interface PMU includes various performance counters to monitor
> the data that is transmitted over the PCIe link. The counters track various
> inbound and outbound transactions which includes separate counters for
> posted/non-posted/completion TLPs. Also, inbound and outbound memory read
> requests along with their latencies can also be monitored. Address
> Translation Services(ATS)events such as ATS Translation, ATS Page Request,
> ATS Invalidation along with their corresponding latencies are also
> supported.
>
> The performance counters are 64 bits wide.
>
> For instance,
> perf stat -e ib_tlp_pr <workload>
> tracks the inbound posted TLPs for the workload.
>
> Signed-off-by: Linu Cherian <lcherian@marvell.com>
> Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com>
This generally looks fine; I have a few comments below.
[...]
> diff --git a/drivers/perf/marvell_pem_pmu.c b/drivers/perf/marvell_pem_pmu.c
> new file mode 100644
> index 000000000000..fb27112aa7d4
> --- /dev/null
> +++ b/drivers/perf/marvell_pem_pmu.c
> @@ -0,0 +1,433 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Marvell PEM(PCIe RC) Performance Monitor Driver
> + *
> + * Copyright (C) 2023 Marvell.
> + */
Nit: please follow the preferred coding style for comments. This should have a
newline immediately after the '/*', e.g.
/*
* Marvell PEM(PCIe RC) Performance Monitor Driver
*
* Copyright (C) 2023 Marvell.
*/
Likewise for all other multi-line comments.
> +#include <linux/acpi.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/perf_event.h>
> +
> +/* Each of these events maps to a free running 64 bit counter
> + * with no event control, but can be reset.
> + *
> + */
> +enum pem_events {
> + IB_TLP_NPR,
> + IB_TLP_PR,
> + IB_TLP_CPL,
> +static u64 eventid_to_offset_table[] = {
> + 0x0,
> + 0x8,
> + 0x10,
I assume the event IDs are the values in the pem_events enum, so please use
array initalizers here to make that clear, e.g.
static u64 eventid_to_offset_table[] = {
[IB_TLP_NPR] = 0x0,
[IB_TLP_PR] = 0x8,
[IB_TLP_CPL] 0x10,
...
};
[...]
> +static int pem_perf_event_init(struct perf_event *event)
> +{
> + struct pem_pmu *pmu = to_pem_pmu(event->pmu);
> + struct hw_perf_event *hwc = &event->hw;
> +
> + if (event->attr.type != event->pmu->type)
> + return -ENOENT;
> +
> + if (is_sampling_event(event)) {
Don't we also need to check for:
event->attach_state & PERF_ATTACH_TASK
> + dev_info(pmu->dev, "Sampling not supported!\n");
> + return -EOPNOTSUPP;
> + }
Please delete this dev_info().
> +
> + if (event->cpu < 0) {
> + dev_warn(pmu->dev, "Can't provide per-task data!\n");
> + return -EOPNOTSUPP;
> + }
Likewise, please delete this dev_warn().
> +
> + /* We must NOT create groups containing mixed PMUs */
> + if (event->group_leader->pmu != event->pmu &&
> + !is_software_event(event->group_leader))
> + return -EINVAL;
> +
> + /* Set ownership of event to one CPU, same event can not be observed
> + * on multiple cpus at same time.
> + */
Please fix this comment style (or delete the comment).
> + event->cpu = pmu->cpu;
> + hwc->idx = -1;
> + return 0;
> +}
Thanks,
Mark.
next prev parent reply other threads:[~2023-07-28 15:02 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-30 12:03 [PATCH 0/6] Marvell Odyssey uncore performance monitor support Gowthami Thiagarajan
2023-06-30 12:03 ` [PATCH 1/6] perf/marvell: Marvell PEM " Gowthami Thiagarajan
2023-07-28 15:01 ` Mark Rutland [this message]
2023-08-12 3:43 ` [EXT] " Gowthami Thiagarajan
2023-06-30 12:03 ` [PATCH 2/6] dt-bindings: perf: marvell: Add YAML schemas for Marvell PEM pmu Gowthami Thiagarajan
2023-07-02 9:25 ` Krzysztof Kozlowski
2023-07-28 15:23 ` Mark Rutland
2023-06-30 12:03 ` [PATCH 3/6] perf/marvell : Odyssey LLC-TAD performance monitor support Gowthami Thiagarajan
2023-07-28 15:38 ` Mark Rutland
2023-08-12 13:51 ` Gowthami Thiagarajan
2023-08-15 9:58 ` Will Deacon
2023-08-15 13:24 ` Mark Rutland
2023-08-17 13:26 ` [EXT] " Gowthami Thiagarajan
2023-06-30 12:03 ` [PATCH 4/6] dt-bindings: perf: marvell: Add YAML schemas for Marvell Odyssey LLC-TAD pmu Gowthami Thiagarajan
2023-07-02 9:26 ` Krzysztof Kozlowski
2023-08-12 3:33 ` [EXT] " Gowthami Thiagarajan
2023-06-30 12:03 ` [PATCH 5/6] perf/marvell: Odyssey DDR Performance monitor support Gowthami Thiagarajan
2023-06-30 12:03 ` [PATCH 6/6] dt-bindings: Add YAML schemas for Marvell Odyssey DDR PMU Gowthami Thiagarajan
2023-07-02 9:27 ` Krzysztof Kozlowski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZMPYYuwg7wHGXft4@FVFF77S0Q05N \
--to=mark.rutland@arm.com \
--cc=bbhushan2@marvell.com \
--cc=gcherian@marvell.com \
--cc=gthiagarajan@marvell.com \
--cc=lcherian@marvell.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=sgoutham@marvell.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).