linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Qi Liu <liuqi115@huawei.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: <will@kernel.org>, <mark.rutland@arm.com>,
	<linux-pci@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>, <linuxarm@huawei.com>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH RFC] perf:Add driver for HiSilicon PCIe PMU
Date: Fri, 13 Mar 2020 14:51:57 +0800	[thread overview]
Message-ID: <11216b30-a198-bb85-6f88-2cdae89c5ef3@huawei.com> (raw)
In-Reply-To: <20200312203657.GA175613@google.com>

Hi Bjorn,
Thanks for reviewing the patch.

On 2020/3/13 4:36, Bjorn Helgaas wrote:
> Use "git log --oneline drivers/perf" to see the convention, and update
> subject to follow suit.  At least add a space in "perf:Add".
> 
> On Thu, Mar 12, 2020 at 08:06:56PM +0800, Qi Liu wrote:
>> From: Qi liu <liuqi115@huawei.com>
>>
>> PCIe PMU Root Complex Integrate End Point(IEP) device is
>> supported to sample bandwidth, latency, buffer occupation,
>> bandwidth utilization etc.
>> Each PMU IEP device monitors multiple root ports, and each
>> IEP is registered as a pmu in /sys/bus/event_source/devices,
>> so users can select the target IEP, and use filter to select
>> root port, function and event.
>> Filtering options are:
>> event:    - select the event.
>> subevent: - select the subevent.
>> port:     - select target root port.
>> func:     - select target EP device under the port.
> 
> The above *looks* like it's supposed to be three separate paragraphs;
> please add blank lines between them.
> 
> s/Integrate/Integrated/
> 
> I assume this is a vendor-specific device; if so, please mention the
> vendor so it doesn't look like something generic.
> 
OK, thanks, We'll correct it.
>> Example: hisi_pcie_00_14_00/event=0x08,subevent=0x04,   \
>> port=0x05,func=0x00/ -I 1000
>>
>> PMU IEP device is described by its bus, device and function,
>> target root port is 0x05 and target EP under it is function
>> 0x00. Subevent 0x04 of event 0x08 is sampled.
>>
>> Note that in this RFC:
>> 1. PCIe PMU IEP hardware is still in development.
>> 2. Perf common event list is undetermined, and name of these
>> events still need to be discussed.
>> 3. port filter could only select one port each time.
>> 4. There are two possible schemes of pmu registration, one is
>> register each root port as a pmu, it is easier for users to
>> select target port. The other one is register each IEP as pmu,
>> for counters are per IEP, not per root port, the second scheme
>> describes hardware PMC much more reasonable, but need to add
>> "port" filter option to select port. We use the second one in
>> this RFC.
>>
>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
>> ---
>>  drivers/perf/Kconfig             |  10 +
>>  drivers/perf/Makefile            |   1 +
>>  drivers/perf/pci/Makefile        |   2 +
>>  drivers/perf/pci/hisi_pcie_pmu.c | 887 +++++++++++++++++++++++++++++++++++++++
>>  include/linux/cpuhotplug.h       |   1 +
>>  5 files changed, 901 insertions(+)
>>  create mode 100644 drivers/perf/pci/Makefile
>>  create mode 100644 drivers/perf/pci/hisi_pcie_pmu.c
>>
>> diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
>> index 09ae8a9..047022b 100644
>> --- a/drivers/perf/Kconfig
>> +++ b/drivers/perf/Kconfig
>> @@ -114,6 +114,16 @@ config THUNDERX2_PMU
>>  	   The SoC has PMU support in its L3 cache controller (L3C) and
>>  	   in the DDR4 Memory Controller (DMC).
>>  
>> +config PCIE_PMU
> 
> This config symbol is too generic for a vendor-specific device.
OK, We'll use HISI_PCIE_PMU here.

> 
>> +	tristate "PCIE PERF PMU"
>> +	depends on ARM64
>> +	default m
>> +	help
>> +	   Provide support for 1630 PCIe performance monitoring unit (PMU)
>> +	   IEP devices.
>> +	   Adds the PCIe PMU into perf events system for monitoring latency,
>> +	   bandwidth etc.
> 
>> +static int hisi_pcie_pmu_irq_register(struct pci_dev *pdev,
>> +				      struct hisi_pcie_pmu *pcie_pmu)
>> +{
>> +	int irq, ret;
>> +
>> +	irq = pci_irq_vector(pdev, HISI_PCIE_EVENT_IRQ_VECTOR);
>> +	ret = request_irq(irq, hisi_pcie_pmu_irq, IRQF_SHARED,
>> +			  pcie_pmu->dev_name, pcie_pmu);
>> +	if (ret)
>> +		return ret;
>> +
>> +	pcie_pmu->irq = irq;
>> +	return ret;
> 
>   return 0;
> 
>> +}
> 
>> +static int hisi_pcie_init_pmu(struct pci_dev *pdev,
>> +			      struct hisi_pcie_pmu *pcie_pmu)
>> +{
>> +	char *name;
>> +	int ret;
>> +
>> +	hisi_get_pcie_pmu(pdev, pcie_pmu);
>> +
>> +	ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_PCIE_PMU_ONLINE,
>> +				       &pcie_pmu->node);
>> +	if (ret) {
>> +		pci_err(pdev, "Error %d registering hotplug;\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * domain_id is 0x00 means continuous domain, we use bdf num to
>> +	 * select IEP. Otherwise, use domain_id + bdf.
> 
> I don't see what this comment applies to.  Nothing in this function
> uses "domain_id".  In fact, pcie_pmu->domain_id is set but never used
> anywhere.
> 
OK ,will delete pcie_pmu->domain_id.
>> +	 */
>> +	name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
>> +			      "hisi_pcie_%x_%x_%x", pcie_pmu->bus_id,
>> +			      pcie_pmu->device_id, pcie_pmu->func_id);
> 
>> +static int hisi_pcie_pmu_probe(struct pci_dev *pdev,
>> +			       const struct pci_device_id *id)
>> +{
>> +	struct hisi_pcie_pmu *pcie_pmu;
>> +	int ret;
>> +
>> +	pcie_pmu = devm_kzalloc(&pdev->dev, sizeof(*pcie_pmu), GFP_KERNEL);
>> +	if (!pcie_pmu)
>> +		return -ENOMEM;
>> +
>> +	pci_set_drvdata(pdev, pcie_pmu);
>> +	ret = hisi_pcie_init_dev(pdev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = hisi_pcie_init_pmu(pdev, pcie_pmu);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return ret;
> 
> This is the same as:
> 
>   return hisi_pcie_init_pmu(pdev, pcie_pmu);
> 
>> +}
> 
thanks, will change it to  return hisi_pcie_init_pmu(pdev, pcie_pmu);
> .
> 


  reply	other threads:[~2020-03-13  6:52 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-12 12:06 [PATCH RFC] perf:Add driver for HiSilicon PCIe PMU Qi Liu
2020-03-12 20:36 ` Bjorn Helgaas
2020-03-13  6:51   ` Qi Liu [this message]
2020-03-13 13:23 ` Robin Murphy
2020-03-13 13:59   ` Mark Rutland
2020-03-19  1:34     ` Qi Liu
2020-03-13 14:14   ` Jonathan Cameron
2020-03-13 14:50     ` Robin Murphy

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=11216b30-a198-bb85-6f88-2cdae89c5ef3@huawei.com \
    --to=liuqi115@huawei.com \
    --cc=helgaas@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linuxarm@huawei.com \
    --cc=mark.rutland@arm.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).