Re: [PATCH v3 2/2] perf/dwc_pcie: Add support for Rockchip SoCs

[Shuai Xue <xueshuai@xxxxxxxxxxxxxxxxx>]

在 2024/12/13 12:24, Shawn Lin 写道:
> Add support for Rockchip SoCs by adding vendor ID to the vendor list.
> And fix the lane-event based enable/disable/read process which is slightly
> different on Rockchip SoCs, by checking vendor ID.
>
> Signed-off-by: Shawn Lin <shawn.lin@xxxxxxxxxxxxxx>
>
> ---
>
> Changes in v3: None
> Changes in v2:
> - rebase on Bejorn's new patch about Qualifing VSEC Capability by Vendor, Revision
>
>   drivers/perf/dwc_pcie_pmu.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
>
> diff --git a/drivers/perf/dwc_pcie_pmu.c b/drivers/perf/dwc_pcie_pmu.c
> index d022f49..ba6d5116 100644
> --- a/drivers/perf/dwc_pcie_pmu.c
> +++ b/drivers/perf/dwc_pcie_pmu.c
> @@ -116,6 +116,8 @@ static const struct dwc_pcie_pmu_vsec_id dwc_pcie_pmu_vsec_ids[] = {
>   	  .vsec_id = 0x02, .vsec_rev = 0x4 },
>   	{ .vendor_id = PCI_VENDOR_ID_QCOM,
>   	  .vsec_id = 0x02, .vsec_rev = 0x4 },
> +	{ .vendor_id = PCI_VENDOR_ID_ROCKCHIP,
> +	  .vsec_id = 0x02, .vsec_rev = 0x4 },
>   	{} /* terminator */
>   };
>   
> @@ -264,12 +266,27 @@ static const struct attribute_group *dwc_pcie_attr_groups[] = {
>   	NULL
>   };
>   
> +static void dwc_pcie_pmu_lane_event_enable_for_rk(struct pci_dev *pdev,
> +						  u16 ras_des_offset,
> +						  bool enable)
> +{
> +	if (enable)
> +		pci_write_config_dword(pdev, ras_des_offset + DWC_PCIE_EVENT_CNT_CTL,
> +				DWC_PCIE_CNT_ENABLE | DWC_PCIE_PER_EVENT_ON);

DWC_PCIE_CNT_ENABLE is a bit mask,

    #define DWC_PCIE_CNT_ENABLE			GENMASK(4, 2)
    #define DWC_PCIE_PER_EVENT_ON			0x3

so, DWC_PCIE_CNT_ENABLE | DWC_PCIE_PER_EVENT_ON == DWC_PCIE_CNT_ENABLE.

what value do you really intend to set here?


> +	else
> +		pci_clear_and_set_config_dword(pdev, ras_des_offset + DWC_PCIE_EVENT_CNT_CTL,
> +				DWC_PCIE_CNT_ENABLE, DWC_PCIE_PER_EVENT_ON);

It is really weird that DWC_PCIE_PER_EVENT_ON mean disable.
Does 0x3 in the register data book for ROCKCHIP means "per event off"?

The register data book from my hand:

    - 0x1 (PER_EVENT_OFF): per event off
    - 0x3 (PER_EVENT_ON): per event on

> +}
> +
>   static void dwc_pcie_pmu_lane_event_enable(struct dwc_pcie_pmu *pcie_pmu,
>   					   bool enable)
>   {
>   	struct pci_dev *pdev = pcie_pmu->pdev;
>   	u16 ras_des_offset = pcie_pmu->ras_des_offset;
>   
> +	if (pdev->vendor == PCI_VENDOR_ID_ROCKCHIP)
> +		return dwc_pcie_pmu_lane_event_enable_for_rk(pdev, ras_des_offset, enable);
> +
>   	if (enable)
>   		pci_clear_and_set_config_dword(pdev,
>   					ras_des_offset + DWC_PCIE_EVENT_CNT_CTL,
> @@ -295,9 +312,14 @@ static u64 dwc_pcie_pmu_read_lane_event_counter(struct perf_event *event)
>   {
>   	struct dwc_pcie_pmu *pcie_pmu = to_dwc_pcie_pmu(event->pmu);
>   	struct pci_dev *pdev = pcie_pmu->pdev;
> +	int event_id = DWC_PCIE_EVENT_ID(event);
>   	u16 ras_des_offset = pcie_pmu->ras_des_offset;
>   	u32 val;
>   
> +	if (pdev->vendor == PCI_VENDOR_ID_ROCKCHIP)
> +		pci_write_config_dword(pdev, ras_des_offset + DWC_PCIE_EVENT_CNT_CTL,
> +				       event_id << 16);

I think dwc_pcie_pmu_event_add() has done the same work to set the event id.

Best Regards,
Shuai