From: Bjorn Helgaas <helgaas@kernel.org>
To: Shuai Xue <xueshuai@linux.alibaba.com>
Cc: will@kernel.org, Jonathan.Cameron@Huawei.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, rdunlap@infradead.org,
robin.murphy@arm.com, mark.rutland@arm.com,
baolin.wang@linux.alibaba.com, zhuo.song@linux.alibaba.com,
linux-pci@vger.kernel.org
Subject: Re: [PATCH v1 2/3] drivers/perf: add DesignWare PCIe PMU driver
Date: Thu, 22 Sep 2022 12:36:07 -0500 [thread overview]
Message-ID: <20220922173607.GA1318619@bhelgaas> (raw)
In-Reply-To: <20220917121036.14864-3-xueshuai@linux.alibaba.com>
[+cc linux-pci]
On Sat, Sep 17, 2022 at 08:10:35PM +0800, Shuai Xue wrote:
> This commit adds the PCIe Performance Monitoring Unit (PMU) driver support
> for T-Head Yitian SoC chip. Yitian is based on the Synopsys PCI Express
> Core controller IP which provides statistics feature. The PMU is not a PCIe
> Root Complex integrated End Point(RCiEP) device but only register counters
> provided by each PCIe Root Port.
>
> To facilitate collection of statistics the controller provides the
> following two features for each Root Port:
>
> - Time Based Analysis (RX/TX data throughput and time spent in each
> low-power LTSSM state)
> - Event counters (Error and Non-Error for lanes)
>
> Note, only one counter for each type.
>
> This driver add PMU devices for each PCIe Root Port. And the PMU device is
> named based the BDF of Root Port. For example,
>
> 10:00.0 PCI bridge: Device 1ded:8000 (rev 01)
>
> the PMU device name for this Root Port is pcie_bdf_100000.
>
> Example usage of counting PCIe RX TLP data payload (Units of 16 bytes)::
>
> $# perf stat -a -e pcie_bdf_200/Rx_PCIe_TLP_Data_Payload/
>
> average RX bandwidth can be calculated like this:
>
> PCIe TX Bandwidth = PCIE_TX_DATA * 16B / Measure_Time_Window
>
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> +++ b/drivers/perf/dwc_pcie_pmu.c
> ...
> +#define DWC_PCIE_VSEC_ID 0x02
I don't think DWC_PCIE_VSEC_ID is a very good name because it doesn't
tell us anything about the purpose of the capability. Something like
DWC_PCIE_RAS_DES_VSEC_ID would be more useful to readers.
> +#define DWC_PCIE_LINK_CAPABILITIES_REG 0xC
> +#define DWC_PCIE_LANE_SHIFT 4
> +#define DWC_PCIE_LANE_MASK GENMASK(9, 4)
Shouldn't need these at all; see below.
> +struct dwc_pcie_info_table {
> + u32 bdf;
> + u32 cap_pos;
Would be useful to name this "ras_des" or similar so we have a hint
about what we're reading/writing when using "pcie_info->cap_pos" below.
> +static struct device_attribute dwc_pcie_pmu_cpumask_attr =
> +__ATTR(cpumask, 0444, dwc_pcie_pmu_cpumask_show, NULL);
DEVICE_ATTR_RO()?
> +#define _dwc_pcie_format_attr(_name, _cfg, _fld) \
> + (&((struct dwc_pcie_format_attr[]) {{ \
> + .attr = __ATTR(_name, 0444, dwc_pcie_pmu_format_show, NULL), \
Ditto.
> +#define DWC_PCIE_EVENT_ATTR(_name, _type, _eventid, _lane) \
> + (&((struct dwc_pcie_event_attr[]) {{ \
> + .attr = __ATTR(_name, 0444, dwc_pcie_event_show, NULL), \
Ditto.
> +static int dwc_pcie_pmu_discover(struct dwc_pcie_pmu_priv *priv)
> +{
> + int val, where, index = 0;
> + struct pci_dev *pdev = NULL;
> + struct dwc_pcie_info_table *pcie_info;
> +
> + priv->pcie_table =
> + devm_kcalloc(priv->dev, RP_NUM_MAX, sizeof(*pcie_info), GFP_KERNEL);
> + if (!priv->pcie_table)
> + return -EINVAL;
> +
> + pcie_info = priv->pcie_table;
> + while ((pdev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, pdev)) != NULL &&
> + index < RP_NUM_MAX) {
> + if (!pci_dev_is_rootport(pdev))
> + continue;
> +
> + pcie_info[index].bdf = dwc_pcie_get_bdf(pdev);
> + pcie_info[index].pdev = pdev;
> +
> + if (dwc_pcie_find_ras_des_cap_position(pdev, &where))
> + continue;
> +
> + pcie_info[index].cap_pos = where;
> +
> + pci_read_config_dword(pdev,
> + pdev->pcie_cap + DWC_PCIE_LINK_CAPABILITIES_REG,
> + &val);
> + pcie_info[index].num_lanes =
> + (val & DWC_PCIE_LANE_MASK) >> DWC_PCIE_LANE_SHIFT;
I think you can use pcie_get_width_cap() here.
> +static int dwc_pcie_pmu_set_event_id(struct dwc_pcie_info_table *pcie_info,
> + int event_id)
> +{
> + int ret;
> + u32 val;
> +
> + ret = dwc_pcie_pmu_read_dword(pcie_info, DWC_PCIE_EVENT_CNT_CTRL, &val);
> + if (ret) {
> + pci_err(pcie_info->pdev, "PCIe read fail\n");
Maybe #define dev_fmt above to add a prefix to these messages?
Otherwise I think they will look like:
pcieport 0000:00:1c.0: PCIe read fail
which suggests it's related to pcieport, but that's the wrong place to
look.
I think every caller of dwc_pcie_pmu_read_dword() makes the same check
and prints the same message; maybe the message should be moved inside
dwc_pcie_pmu_read_dword()?
Same with dwc_pcie_pmu_write_dword(); moving the message there would
simplify all callers.
> +static int dwc_pcie_pmu_event_enable(struct dwc_pcie_info_table *pcie_info,
> + u32 enable)
> +{
> + u32 ret;
> + u32 val;
> +
> + ret = dwc_pcie_pmu_read_dword(pcie_info, DWC_PCIE_EVENT_CNT_CTRL, &val);
> + if (ret) {
> + pci_err(pcie_info->pdev, "PCIe read fail\n");
> + return ret;
> + }
> +
> + val &= ~(DWC_PCIE__CNT_ENABLE_MASK);
Superfluous parens.
> +static int dwc_pcie_pmu_base_time_add_prepare(struct dwc_pcie_info_table
> + *pcie_info, u32 event_id)
> +{
> + u32 ret;
> + u32 val;
> +
> + ret = dwc_pcie_pmu_read_dword(pcie_info,
> + DWC_PCIE_TIME_BASED_ANALYSIS_CTRL, &val);
> + if (ret) {
> + pci_err(pcie_info->pdev, "PCIe read fail\n");
> + return ret;
> + }
> +
> + val &= ~DWC_PCIE__TIME_BASED_REPORT_SELECT_MASK;
> + val |= event_id << DWC_PCIE__TIME_BASED_REPORT_SELECT_SHIFT;
> + val &= ~DWC_PCIE__TIME_BASED_DURATION_SELECT;
> +
> + /*
> + * TIME_BASED_ANALYSIS_DATA_REG is a 64 bit register, we can safely
> + * use it with any manually controllered duration.
s/controllered/controlled/ ? Not sure what this means. Maybe that
64 bits is wide enough you don't need to worry about rollover?
> +static struct dwc_pcie_info_table *pmu_to_pcie_info(struct pmu *pmu)
> +{
> + struct dwc_pcie_info_table *pcie_info;
> + struct dwc_pcie_pmu *pcie_pmu = to_pcie_pmu(pmu);
> +
> + pcie_info = container_of(pcie_pmu, struct dwc_pcie_info_table, pcie_pmu);
> + if (pcie_info == NULL)
> + pci_err(pcie_info->pdev, "Can't get pcie info\n");
It shouldn't be possible to get here for a pmu with no pcie_info, and
callers don't check for a NULL pointer return value before
dereferencing it, so I guess all this adds is an error message before
a NULL pointer oops? Not sure the code clutter is worth it.
> + return pcie_info;
> +}
> +static int dwc_pcie_pmu_event_init(struct perf_event *event)
> +{
> + struct hw_perf_event *hwc = &event->hw;
> + struct dwc_pcie_pmu *pcie_pmu = to_pcie_pmu(event->pmu);
> + struct perf_event *sibling;
> +
> + if (event->attr.type != event->pmu->type)
> + return -ENOENT;
> +
> + if (hwc->sample_period) {
> + dev_dbg(pcie_pmu->dev, "Sampling not supported\n");
> + return -EOPNOTSUPP;
> + }
> +
> + if (event->cpu < 0) {
> + dev_dbg(pcie_pmu->dev, "Per-task mode not supported\n");
> + return -EOPNOTSUPP;
> + }
> +
> + event->cpu = pcie_pmu->on_cpu;
> +
> + if (event->group_leader != event &&
> + !is_software_event(event->group_leader)) {
> + dev_dbg(pcie_pmu->dev, "Drive way only allow one event!\n");
"Drive way"? -ENOPARSE for me :)
> + return -EINVAL;
> + }
> +
> + for_each_sibling_event(sibling, event->group_leader) {
> + if (sibling != event && !is_software_event(sibling)) {
> + dev_dbg(pcie_pmu->dev, "Drive way event not allowed!\n");
> + return -EINVAL;
> + }
> + }
> +static void dwc_pcie_pmu_set_period(struct hw_perf_event *hwc)
> +{
> + u64 new = 0;
Superfluous variable.
> + local64_set(&hwc->prev_count, new);
> +}
> +static int __dwc_pcie_pmu_probe(struct dwc_pcie_pmu_priv *priv,
> + struct dwc_pcie_info_table *pcie_info)
> +{
> + int ret;
> + char *name;
> + struct dwc_pcie_pmu *pcie_pmu;
> + struct device *dev;
> +
> + if (!pcie_info || !pcie_info->pdev) {
> + pci_err(pcie_info->pdev, "Input parameter is invalid\n");
There are a lot of "Input parameter is invalid" messages. If somebody
sees that, there's no hint about which one to look at. Messages that
are constant strings are usually a hint that they could include more
information.
> +static int dwc_pcie_pmu_probe(struct platform_device *pdev)
> +{
> + int ret = 0;
> + int pcie_index;
> + struct dwc_pcie_pmu_priv *priv;
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> + priv->dev = &pdev->dev;
> + platform_set_drvdata(pdev, priv);
> +
> + /* If PMU is not support on current platform, keep slient */
s/not support/not supported/
s/slient/silent/
Bjorn
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-09-22 17:37 UTC|newest]
Thread overview: 78+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-17 12:10 [PATCH v1 0/3] drivers/perf: add Synopsys DesignWare PCIe PMU driver support Shuai Xue
2022-09-17 12:10 ` [PATCH v1 1/3] docs: perf: Add description for Synopsys DesignWare PCIe PMU driver Shuai Xue
2022-09-22 13:25 ` Will Deacon
2022-09-23 13:51 ` Shuai Xue
2022-11-07 15:28 ` Will Deacon
2022-09-23 1:27 ` Yicong Yang
2022-09-23 14:47 ` Shuai Xue
2022-09-17 12:10 ` [PATCH v1 2/3] drivers/perf: add " Shuai Xue
2022-09-22 15:58 ` Jonathan Cameron
2022-09-22 17:32 ` Bjorn Helgaas
2022-09-23 3:35 ` Yicong Yang
2022-09-23 10:56 ` Jonathan Cameron
2022-09-23 13:45 ` Shuai Xue
2022-09-23 15:54 ` Jonathan Cameron
2022-09-26 13:31 ` Shuai Xue
2022-09-26 14:32 ` Robin Murphy
2022-09-26 17:18 ` Bjorn Helgaas
2022-09-27 5:13 ` Shuai Xue
2022-09-27 10:04 ` Jonathan Cameron
2022-09-27 10:14 ` Robin Murphy
2022-09-27 12:49 ` Shuai Xue
2022-09-27 13:39 ` Jonathan Cameron
2022-09-27 12:29 ` Shuai Xue
2022-09-27 10:03 ` Jonathan Cameron
2022-09-22 17:36 ` Bjorn Helgaas [this message]
2022-09-23 14:46 ` Shuai Xue
2022-09-23 18:51 ` Bjorn Helgaas
2022-09-27 6:01 ` Shuai Xue
2022-09-23 3:30 ` Yicong Yang
2022-09-23 15:43 ` Shuai Xue
2022-09-24 8:00 ` Yicong Yang
2022-09-26 11:39 ` Shuai Xue
2022-09-17 12:10 ` [PATCH v1 3/3] MAINTAINERS: add maintainers for " Shuai Xue
2023-04-10 3:16 ` [PATCH v2 0/3] drivers/perf: add Synopsys DesignWare PCIe PMU driver support Shuai Xue
2023-04-10 3:17 ` [PATCH v2 1/3] docs: perf: Add description for Synopsys DesignWare PCIe PMU driver Shuai Xue
2023-04-10 3:17 ` [PATCH v2 2/3] drivers/perf: add " Shuai Xue
2023-04-10 7:25 ` kernel test robot
2023-04-11 3:17 ` Baolin Wang
2023-04-17 1:16 ` Shuai Xue
2023-04-18 1:51 ` Baolin Wang
2023-04-19 1:39 ` Shuai Xue
2023-04-10 3:17 ` [PATCH v2 3/3] MAINTAINERS: add maintainers for " Shuai Xue
2023-04-17 6:17 ` [PATCH v3 0/3] drivers/perf: add Synopsys DesignWare PCIe PMU driver support Shuai Xue
2023-04-17 6:17 ` [PATCH v3 1/3] docs: perf: Add description for Synopsys DesignWare PCIe PMU driver Shuai Xue
2023-05-16 14:32 ` Jonathan Cameron
2023-05-17 1:27 ` Shuai Xue
2023-04-17 6:17 ` [PATCH v3 2/3] drivers/perf: add " Shuai Xue
2023-04-18 23:30 ` Robin Murphy
2023-04-27 6:33 ` Shuai Xue
2023-05-16 15:03 ` Jonathan Cameron
2023-05-16 19:17 ` Bjorn Helgaas
2023-05-17 9:54 ` Jonathan Cameron
2023-05-17 16:27 ` Bjorn Helgaas
2023-05-19 10:08 ` Shuai Xue
2023-04-17 6:17 ` [PATCH v3 3/3] MAINTAINERS: add maintainers for " Shuai Xue
2023-05-16 13:01 ` [PATCH v4 0/4] drivers/perf: add Synopsys DesignWare PCIe PMU driver support Shuai Xue
2023-05-16 13:01 ` [PATCH v4 1/4] docs: perf: Add description for Synopsys DesignWare PCIe PMU driver Shuai Xue
2023-05-16 13:01 ` [PATCH v4 2/4] PCI: move Alibaba Vendor ID linux/pci_ids.h Shuai Xue
2023-05-16 13:01 ` [PATCH v4 3/4] drivers/perf: add DesignWare PCIe PMU driver Shuai Xue
2023-05-16 19:19 ` Bjorn Helgaas
2023-05-17 2:35 ` Shuai Xue
[not found] ` <202305170639.XU3djFZX-lkp@intel.com>
2023-05-17 3:37 ` Shuai Xue
2023-05-16 13:01 ` [PATCH v4 4/4] MAINTAINERS: add maintainers for " Shuai Xue
2023-05-22 3:54 ` [PATCH v5 0/4] drivers/perf: add Synopsys DesignWare PCIe PMU driver support Shuai Xue
2023-05-22 14:28 ` Jonathan Cameron
2023-05-23 2:57 ` Shuai Xue
2023-05-22 3:54 ` [PATCH v5 1/4] docs: perf: Add description for Synopsys DesignWare PCIe PMU driver Shuai Xue
2023-05-29 3:45 ` Baolin Wang
2023-05-29 6:31 ` Shuai Xue
2023-05-22 3:54 ` [PATCH v5 2/4] PCI: move Alibaba Vendor ID linux/pci_ids.h Shuai Xue
2023-05-22 16:04 ` Bjorn Helgaas
2023-05-23 3:22 ` Shuai Xue
2023-05-23 11:54 ` Bjorn Helgaas
2023-05-23 12:49 ` Shuai Xue
2023-05-22 3:54 ` [PATCH v5 3/4] drivers/perf: add DesignWare PCIe PMU driver Shuai Xue
2023-05-29 6:13 ` Baolin Wang
2023-05-29 6:33 ` Shuai Xue
2023-05-22 3:54 ` [PATCH v5 4/4] MAINTAINERS: add maintainers for " Shuai Xue
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=20220922173607.GA1318619@bhelgaas \
--to=helgaas@kernel.org \
--cc=Jonathan.Cameron@Huawei.com \
--cc=baolin.wang@linux.alibaba.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=rdunlap@infradead.org \
--cc=robin.murphy@arm.com \
--cc=will@kernel.org \
--cc=xueshuai@linux.alibaba.com \
--cc=zhuo.song@linux.alibaba.com \
/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).