On 2023/7/27 16:57, Jonathan Cameron wrote:
> On Tue, 6 Jun 2023 15:49:35 +0800
> Shuai Xue <xueshuai@xxxxxxxxxxxxxxxxx> wrote:
>
>> Alibaba's T-Head Yitan 710 SoC includes Synopsys' DesignWare Core PCIe
>> controller which implements which implements PMU for performance and
>> functional debugging to facilitate system maintenance.
>>
>> Document it to provide guidance on how to use it.
>>
>> Signed-off-by: Shuai Xue <xueshuai@xxxxxxxxxxxxxxxxx>
>> Reviewed-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
>
> Given this looks like it might move forwards (after Bjorn's reply)
> I'll give it a closer review :)
That's great to hear! I appreciate the effort that has been put into resuming the
review process. Thank you for your dedication and hard work in making this happen。
>
> Some editorial things in here only. What you have is easy
> to understand but nice to tidy up the odd corner or two.
> We can bikeshed this for ever so I've skipped really minor things
> where phrasing is debatable (particularly British vs US English :)
Thank you for patiently pointing out the writing issues. I appreciate your feedback
and it will make the necessary improvements.
(Comments replied inline)
Best Regards,
Shuai
>
> Jonathan
>
>
>> ---
>> .../admin-guide/perf/dwc_pcie_pmu.rst | 97 +++++++++++++++++++
>> Documentation/admin-guide/perf/index.rst | 1 +
>> 2 files changed, 98 insertions(+)
>> create mode 100644 Documentation/admin-guide/perf/dwc_pcie_pmu.rst
>>
>> diff --git a/Documentation/admin-guide/perf/dwc_pcie_pmu.rst b/Documentation/admin-guide/perf/dwc_pcie_pmu.rst
>> new file mode 100644
>> index 000000000000..c1f671cb64ec
>> --- /dev/null
>> +++ b/Documentation/admin-guide/perf/dwc_pcie_pmu.rst
>> @@ -0,0 +1,97 @@
>> +======================================================================
>> +Synopsys DesignWare Cores (DWC) PCIe Performance Monitoring Unit (PMU)
>> +======================================================================
>> +
>> +DesignWare Cores (DWC) PCIe PMU
>> +===============================
>> +
>> +The PMU is not a PCIe Root Complex integrated End Point (RCiEP) device but
>> +only PCIe configuration space register block provided by each PCIe Root
>
> I don't think you need the negative bit of description - it's not a lot of
> different things and this statement only really makes sense when compared to
> some other PCIe PMUs which the reader may never have come across.
>
> "The PMU is a PCIe configuration space register block provided by each PCIE Root
> Port in a Vendor-Specific Extended Capability ..."
Aha, you are right, I should not have made such assumptions, will adopt your
rewriting.
>
>> +Port in a Vendor-Specific Extended Capability named RAS DES (Debug, Error
>> +injection, and Statistics).
>> +
>> +As the name indicated, the RAS DES capability supports system level
>
> "As the name indicates," (present tense more appropriate here)
Will fix it.
>
>> +debugging, AER error injection, and collection of statistics. To facilitate
>> +collection of statistics, Synopsys DesignWare Cores PCIe controller
>
> "Core's"
>
> (as it belongs to the core rather than intent being that it applies to plural
> cores?)
"Synopsys DesignWare Cores PCIe controller" is from the title from Synopsys
databook, so I prefer to keep as it is here.
>
>> +provides the following two features:
>> +
>> +- Time Based Analysis (RX/TX data throughput and time spent in each
>> + low-power LTSSM state)
>> +- Lane Event counters (Error and Non-Error for lanes)
>> +
>> +Time Based Analysis
>> +-------------------
>> +
>> +Using this feature you can obtain information regarding RX/TX data
>> +throughput and time spent in each low-power LTSSM state by the controller.
>> +
>> +The counters are 64-bit width and measure data in two categories,
>> +
>> +- percentage of time does the controller stay in LTSSM state in a
>
> "percentage of time the controller stays in LTSSM "
Will fix it.
>
>> + configurable duration. The measurement range of each Event in Group#0.
>
> I'm not sure of meaning of the last sentence. Is it simply that this bullet
> refers to group#0? Perhaps make that the lead off. e.g.
>
> - Group#0: Percentage of time the controller stays in LTSSM states.
> - Group#1: Amount of data processed (Units of 16 bytes).
You are right. Will fix it.
>
>> +- amount of data processed (Units of 16 bytes). The measurement range of
>> + each Event in Group#1.
>> +
>> +Lane Event counters
>> +-------------------
>> +
>> +Using this feature you can obtain Error and Non-Error information in
>> +specific lane by the controller.
>> +
>> +The counters are 32-bit width and the measured event is select by:
>> +
>> +- Group i
>> +- Event j within the Group i
>> +- and Lane k
>> +
>> +Some of the event counters only exist for specific configurations.
>> +
>> +DesignWare Cores (DWC) PCIe PMU Driver
>> +=======================================
>> +
>> +This driver add PMU devices for each PCIe Root Port. And the PMU device is
>
> "This driver adds PMU devices for each PCIe Root Port. The PMU device is named"
>
> (Not good to start a sentence with And - an alternative form would be)
>
> "This driver adds PMU devices for each PCIe Root Port named based on the BDF of
> the Root Port."
Ok, will fix it.
>
>> +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.
>> +
>> +The DWC PCIe PMU driver registers a perf PMU driver, which provides
>> +description of available events and configuration options in sysfs, see
>> +/sys/bus/event_source/devices/dwc_rootport_{bdf}.
>> +
>> +The "format" directory describes format of the config, fields of the
>
> "config fields" (stray comma makes this confusing to read)
Will fix it.
>> +perf_event_attr structure. The "events" directory provides configuration
>> +templates for all documented events. For example,
>> +"Rx_PCIe_TLP_Data_Payload" is an equivalent of "eventid=0x22,type=0x1".
>> +
>> +The "perf list" command shall list the available events from sysfs, e.g.::
>> +
>> + $# perf list | grep dwc_rootport
>> + <...>
>> + dwc_rootport_3018/Rx_PCIe_TLP_Data_Payload/ [Kernel PMU event]
>> + <...>
>> + dwc_rootport_3018/rx_memory_read,lane=?/ [Kernel PMU event]
>> +
>> +Time Based Analysis Event Usage
>> +-------------------------------
>> +
>> +Example usage of counting PCIe RX TLP data payload (Units of 16 bytes)::
>> +
>> + $# perf stat -a -e dwc_rootport_3018/Rx_PCIe_TLP_Data_Payload/
>> +
>> +The average RX/TX bandwidth can be calculated using the following formula:
>> +
>> + PCIe RX Bandwidth = PCIE_RX_DATA * 16B / Measure_Time_Window
>> + PCIe TX Bandwidth = PCIE_TX_DATA * 16B / Measure_Time_Window
>> +
>> +Lane Event Usage
>> +-------------------------------
>> +
>> +Each lane has the same event set and to avoid generating a list of hundreds
>> +of events, the user need to specify the lane ID explicitly, e.g.::
>> +
>> + $# perf stat -a -e dwc_rootport_3018/rx_memory_read,lane=4/
>> +
>> +The driver does not support sampling, therefore "perf record" will not
>> +work. Per-task (without "-a") perf sessions are not supported.
>> diff --git a/Documentation/admin-guide/perf/index.rst b/Documentation/admin-guide/perf/index.rst
>> index 9de64a40adab..11a80cd28a2e 100644
>> --- a/Documentation/admin-guide/perf/index.rst
>> +++ b/Documentation/admin-guide/perf/index.rst
>> @@ -19,5 +19,6 @@ Performance monitor support
>> arm_dsu_pmu
>> thunderx2-pmu
>> alibaba_pmu
>> + dwc_pcie_pmu
>> nvidia-pmu
>> meson-ddr-pmu
