On 2023/10/21 00:49, Jonathan Cameron wrote:
> On Fri, 20 Oct 2023 21:42:29 +0800
> Shuai Xue <xueshuai@xxxxxxxxxxxxxxxxx> 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 a PCIe
>> configuration space register block provided by each PCIe Root Port in a
>> Vendor-Specific Extended Capability named RAS D.E.S (Debug, Error
>> injection, and Statistics).
>>
>> To facilitate collection of statistics the controller provides the
>> following two features for each Root Port:
>>
>> - one 64-bit counter for Time Based Analysis (RX/TX data throughput and
>> time spent in each low-power LTSSM state) and
>> - one 32-bit counter for Event Counting (error and non-error events for
>> a specified lane)
>>
>> Note: There is no interrupt for counter overflow.
>>
>> This driver adds PMU devices for each PCIe Root Port. And the PMU device is
>> named based the BDF of Root Port. For example,
>>
>> 30:03.0 PCI bridge: Device 1ded:8000 (rev 01)
>>
>> the PMU device name for this Root Port is dwc_rootport_3018.
>>
>> Example usage of counting PCIe RX TLP data payload (Units of bytes)::
>>
>> $# perf stat -a -e dwc_rootport_3018/Rx_PCIe_TLP_Data_Payload/
>>
>> average RX bandwidth can be calculated like this:
>>
>> PCIe TX Bandwidth = Rx_PCIe_TLP_Data_Payload / Measure_Time_Window
>>
>> Signed-off-by: Shuai Xue <xueshuai@xxxxxxxxxxxxxxxxx>
> LGTM other than some really trivial stuff inline if you are doing a v10
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
Hi, Jonathan,
Thank you for your prompt response.
The fact that this [3/4] patch has received a Reviewed-by tag from the
community is a momentous milestone for both the patchset itself and for me
personally. I am deeply grateful for the time and effort you have dedicated
to providing valuable comments, as it has significantly improved the code's
quality.
I will address the inline comments in the upcoming version v10. However, I
kindly request some time to wait for feedback from esteemed reviewers such
as Yicong, Bjorn, Will, or anyone else who may find this patchset
intriguing.
Best Regards and Cheers.
Shuai
>> + }
>> +
>> + /* Unwind when platform driver removes */
>> + ret = devm_add_action_or_reset(
>> + &plat_dev->dev, dwc_pcie_pmu_remove_cpuhp_instance,
>> + &pcie_pmu->cpuhp_node);
>> + if (ret)
>> + goto out;
>> +
>> + ret = perf_pmu_register(&pcie_pmu->pmu, name, -1);
>> + if (ret) {
>> + pci_err(pdev,
>> + "Error %d registering PMU @%x\n", ret, bdf);
>> + goto out;
>> + }
>> +
>> + /* Cache PMU to handle pci device hotplug */
>> + list_add(&pcie_pmu->pmu_node, &dwc_pcie_pmu_head);
>> + pcie_pmu->registered = true;
>> + notify = true;
>> +
>> + ret = devm_add_action_or_reset(
>> + &plat_dev->dev, dwc_pcie_pmu_unregister_pmu, pcie_pmu);
>
> line wrapping is a bit ugly - I would move the &plat_dev->dev to previous line.
> and I think you can get away with aligning the rest just after the (
>
Actually, I warp this with VSCode automaticlly :(
Will move the &plat_dev->dev to previous line.
>
>
>
>> +static int __init dwc_pcie_pmu_init(void)
>> +{
>> + int ret;
>> +
>> + ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN,
>> + "perf/dwc_pcie_pmu:online",
>> + dwc_pcie_pmu_online_cpu,
>> + dwc_pcie_pmu_offline_cpu);
>> + if (ret < 0)
>> + return ret;
>> +
>> + dwc_pcie_pmu_hp_state = ret;
>> +
>> + ret = platform_driver_register(&dwc_pcie_pmu_driver);
>> + if (ret)
>> + goto platform_driver_register_err;
>> +
>> + dwc_pcie_pmu_dev = platform_device_register_simple(
>> + "dwc_pcie_pmu", PLATFORM_DEVID_NONE, NULL, 0);
>> + if (IS_ERR(dwc_pcie_pmu_dev)) {
>> + ret = PTR_ERR(dwc_pcie_pmu_dev);
>> + goto platform_device_register_error;
>> + }
>> +
>> + return 0;
>> +
>> +platform_device_register_error:
>
> Trivial but I'd standardize on err or error, not mix them.
Sorry, will fix it.
>
>> + platform_driver_unregister(&dwc_pcie_pmu_driver);
>> +platform_driver_register_err:
>> + cpuhp_remove_multi_state(dwc_pcie_pmu_hp_state);
>> +
>> + return ret;
>> +}
