From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 08849CDB47E for ; Thu, 12 Oct 2023 16:25:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:References: List-Owner; bh=5KRJmEaCzR3VvrJ94137/VwmeN36OIMQlhW8Wor/BV4=; b=nElt96WZbJhVmd B1HSdVLpnOkfKP82Mb9tQs2x5x1HpK6mWcR0cPxNlDy0MZb1+Gxsli0NWyXa8SCV/pYzyXrXcc2tb 6y9mHXWv4PTSP1VA27cttfqsL0N8O/SU8rSqnhIVNJj+N44LUWG+ZG7U1fNm9wkt1TlJk9uVpdMTz UgWrZTjzTy2gc3rAW7oYdvk4NvwN+76NTfpvbyUzJ5EKx9Wl2GFrniY6gZpKPw6u8/u2wRAGoJcUF lqrAyffQuTUlB5hcnWYw9CboOmQfK2f9VTZ2Du5zGNfsqvSyE8EvyOBKx1IvEpQrikn0YkxOVZYxo 9B8aS+B8d7zwuG60xvQw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qqyUy-001PRc-1b; Thu, 12 Oct 2023 16:25:20 +0000 Received: from ams.source.kernel.org ([145.40.68.75]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qqyUu-001PQK-2h for linux-arm-kernel@lists.infradead.org; Thu, 12 Oct 2023 16:25:19 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by ams.source.kernel.org (Postfix) with ESMTP id 41735B8253D; Thu, 12 Oct 2023 16:25:15 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 55A62C433C8; Thu, 12 Oct 2023 16:25:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1697127914; bh=jhs8ocEdk0H8R7hydf3+ylgi+ceFyylN7MYk7og3q2M=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=Oiq8SU7bWFp3/+Zz1L2IIfOEgEFdISzkAr1MLzwI+DHW3lv3JkNOn+lfHrrUngOR0 ZtWDjzLSDlrdOJeR5/qXXJIfMeWClcsQ9EPWVURatNa3/jfpXh63hJRIKEihZlP0Zc B5YFjX5wHduKITGsFrkoYE6WykCp5SJrJWWn86njNQGVRGUd8j/xEuHrGzYfOidIuD CrUPBejr3vcLbixFKzn0eVW+hycNmO//7j2mWzsaRLgRaPmwlNlPOoc1UScTqxPTJW fPXWLuk8Obn60f6KtR/VfH+Ef+hGyhwV+ydjLsFwQEsZIGlo7UUJZrDkC31NPpIcUE upgmAkgDUyPmQ== Date: Thu, 12 Oct 2023 11:25:12 -0500 From: Bjorn Helgaas To: Shuai Xue Cc: chengyou@linux.alibaba.com, kaishen@linux.alibaba.com, yangyicong@huawei.com, will@kernel.org, Jonathan.Cameron@huawei.com, baolin.wang@linux.alibaba.com, robin.murphy@arm.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-pci@vger.kernel.org, rdunlap@infradead.org, mark.rutland@arm.com, zhuo.song@linux.alibaba.com, renyu.zj@linux.alibaba.com Subject: Re: [PATCH v7 3/4] drivers/perf: add DesignWare PCIe PMU driver Message-ID: <20231012162512.GA1069387@bhelgaas> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20231012032856.2640-4-xueshuai@linux.alibaba.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231012_092517_239339_FF18AD5F X-CRM114-Status: GOOD ( 30.67 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Oct 12, 2023 at 11:28:55AM +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 and does not overflow interrupt. Not sure what "does not overflow interrupt" means. Does it mean there's no interrupt generated when the counter overflows? > +config DWC_PCIE_PMU > + tristate "Enable Synopsys DesignWare PCIe PMU Support" The DWC_PCIE_PMU symbol and the "Synopsys DesignWare PCIe PMU" text suggest that this is generic and should work on any designs based on DesignWare IP, not just the Alibaba devices. It appears that the current driver only supports Alibaba devices because it only looks at Root Ports with PCI_VENDOR_ID_ALIBABA, but I assume support for non-Alibaba devices is likely to be added in the future? If it is really generic for DesignWare-based devices the config symbol and menu entry seem fine. Maybe mention the vendor (Synopsys or Alibaba) first in the menu entry, though, since that's what most other drivers do, e.g., tristate "Synopsys DesignWare PCIe PMU" > + depends on (ARM64 && PCI) I don't see any actual ARM64 dependency in the code, so maybe omit ARM64 (as PCIE_DW_PLAT_HOST does) or add "|| COMPILE_TEST"? > + help > + Enable perf support for Synopsys DesignWare PCIe PMU Performance > + monitoring event on platform including the Yitian 710. Should this mention Alibaba or T-Head? I don't know how Alibaba/T-Head/Yitian are all related. > + * Put them togother as TRM used. s/togother/together/ Maybe "Put them together as in TRM."? > +#define _dwc_pcie_format_attr(_name, _cfg, _fld) \ > + (&((struct dwc_pcie_format_attr[]) {{ \ > + .attr = __ATTR(_name, 0444, dwc_pcie_pmu_format_show, NULL), \ > + .config = _cfg, \ > + .field = _fld, \ > + }})[0].attr.attr) Seems weird to put the \ characters in column 81 where they make everything wrap unnecessarily on a 80-column terminal. I guess it just happens to be where a tab after the longest line ends up. > +static void dwc_pcie_pmu_unregister_pmu(void *data) > +{ > + struct dwc_pcie_pmu *pcie_pmu = data; > + > + if (!pcie_pmu->registered) > + return; > + > + perf_pmu_unregister(&pcie_pmu->pmu); > + pcie_pmu->registered = false; > +} > + > +/* > + * Find the PMU of a PCI device. > + * @pdev: The PCI device. > + */ > +static struct dwc_pcie_pmu *dwc_pcie_find_dev_pmu(struct pci_dev *pdev) > +{ > + struct dwc_pcie_pmu *pcie_pmu, *tmp; > + > + list_for_each_entry_safe(pcie_pmu, tmp, &dwc_pcie_pmu_head, pmu_node) { > + if (pcie_pmu->pdev == pdev) { > + list_del(&pcie_pmu->pmu_node); Seems sort of weird to have a "find_dev" function actually *remove* the entry. The entry was added to the list near the perf_pmu_register(), so maybe consider removing it in the caller or near the perf_pmu_unregister(). Maybe also reorder the functions as: dwc_pcie_find_dev_pmu dwc_pcie_pmu_unregister_pmu dwc_pcie_pmu_notifier since dwc_pcie_find_dev_pmu() is a less interesting helper function. > + return pcie_pmu; > + } > + } > + > + return NULL; > +} > + > +static int dwc_pcie_pmu_notifier(struct notifier_block *nb, > + unsigned long action, void *data) > +{ > + struct device *dev = data; > + struct pci_dev *pdev = to_pci_dev(dev); > + struct dwc_pcie_pmu *pcie_pmu; > + > + /* Unregister the PMU when the device is going to be deleted. */ > + if (action != BUS_NOTIFY_DEL_DEVICE) > + return NOTIFY_DONE; > + > + pcie_pmu = dwc_pcie_find_dev_pmu(pdev); > + if (!pcie_pmu) > + return NOTIFY_DONE; > + > + dwc_pcie_pmu_unregister_pmu(pcie_pmu); > + > + return NOTIFY_OK; > +} > + /* Match the rootport with VSEC_RAS_DES_ID, and register a PMU for it */ > + for_each_pci_dev(pdev) { > + u16 vsec; > + u32 val; > + > + if (!(pci_is_pcie(pdev) && > + pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT)) > + continue; > + > + vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_ALIBABA, > + DWC_PCIE_VSEC_RAS_DES_ID); > + if (!vsec) > + continue; > + > + pci_read_config_dword(pdev, vsec + PCI_VNDR_HEADER, &val); > + if (PCI_VNDR_HEADER_REV(val) != 0x04 || > + PCI_VNDR_HEADER_LEN(val) != 0x100) > + continue; > + pci_dbg(pdev, > + "Detected PCIe Vendor-Specific Extended Capability RAS DES\n"); > + > + bdf = PCI_DEVID(pdev->bus->number, pdev->devfn); > + name = devm_kasprintf(&plat_dev->dev, GFP_KERNEL, "dwc_rootport_%x", > + bdf); > + if (!name) { > + ret = -ENOMEM; > + goto out; > + } > + > + /* All checks passed, go go go */ > + pcie_pmu = devm_kzalloc(&plat_dev->dev, sizeof(*pcie_pmu), GFP_KERNEL); > + if (!pcie_pmu) { > + ret = -ENOMEM; > + goto out; > + } > + > + pcie_pmu->pdev = pdev; > + pcie_pmu->ras_des = vsec; Looks like "ras_des" is the offset of a RAS DES Capability, and we only use the Vendor-specific DWC_PCIE_VSEC_RAS_DES_ID Capability to learn the RAS DES offset? That seems a little convoluted and unnecessarily Alibaba-specific. A more generic way to do this would be for the RAS DES registers to be in a Designated Vendor-Specific Capability with DVSEC Vendor ID of PCI_VENDOR_ID_SYNOPSYS and a Synopsys-defined DVSEC ID that identifies RAS DES. Then we could use pci_find_dvsec_capability() to find the RAS DES Capability directly without the Alibaba dependency. Obviously this would only work if the hardware/firmware design supports it, and I assume the Synopsis IP wasn't designed that way. Bjorn _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel