Skip to contentSkip to conversation

perf: hisi: fix uncore PMU index ID

PATCH New Shaokun Zhang 2 participants 5 messages May 21, 2018 – May 25, 2018

Subsystems (2)
Arm PMU profiling and debugging HiSilicon PMU driver
lore
Share & tools
lore

Shaokun Zhang

author
#0 lore raw

PATCH 1/1 perf: hisi: fix uncore PMU index ID

According to ACPI SPEC about _UID (Unique ID), The _UID must be unique across all devices with either a common _HID or _CID. For HiSilion uncore PMU, SCCL_ID and INDEX_ID are used to identify the uncore PMU for the same _HID. Therefore, _UID is not equal to INDEX_ID in multi-sccl scene for the same uncore PMU device.

CCL_ID can be used as the INDEX_ID for the L3C PMU and IDX-ID is added in DSDT table for the HHA PMU.

Fixes 2940bc4("perf: hisi: Add support for HiSilicon SoC L3C PMU driver")
Fixes 2bab3cf("perf: hisi: Add support for HiSilicon SoC HHA PMU driver")
Reported-by Huiqiang Wang <[email protected]>
Signed-off-by Shaokun Zhang <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Huiqiang Wang <[email protected]>
---
 drivers/perf/hisilicon/hisi_uncore_hha_pmu.c | 18 ++++++++----------
 drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c | 11 ++---------
 2 files changed, 10 insertions(+), 19 deletions(-)
drivers/perf/hisilicon/hisi_uncore_hha_pmu.c +8 -10
index 443906e..dcd8e77 100644
@@ -238,19 +238,10 @@ MODULE_DEVICE_TABLE(acpi, hisi_hha_pmu_acpi_match);
 static int hisi_hha_pmu_init_data(struct platform_device *pdev,
 				  struct hisi_pmu *hha_pmu)
 {
-	unsigned long long id;
 	struct resource *res;
-	acpi_status status;
-
-	status = acpi_evaluate_integer(ACPI_HANDLE(&pdev->dev),
-				       "_UID", NULL, &id);
-	if (ACPI_FAILURE(status))
-		return -EINVAL;
-
-	hha_pmu->index_id = id;
 
 	/*
-	 * Use SCCL_ID and UID to identify the HHA PMU, while
+	 * Use SCCL_ID and HHA index ID to identify the HHA PMU, while
 	 * SCCL_ID is in MPIDR[aff2].
 	 */
 	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",

@@ -258,6 +249,13 @@ static int hisi_hha_pmu_init_data(struct platform_device *pdev,
 		dev_err(&pdev->dev, "Can not read hha sccl-id!\n");
 		return -EINVAL;
 	}
+
+	if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
+				     &hha_pmu->index_id)) {
+		dev_err(&pdev->dev, "Can not read hha index-id!\n");
+		return -EINVAL;
+	}
+
 	/* HHA PMUs only share the same SCCL */
 	hha_pmu->ccl_id = -1;
drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c +2 -9
index 0bde5d9..133889c 100644
@@ -237,16 +237,7 @@ MODULE_DEVICE_TABLE(acpi, hisi_l3c_pmu_acpi_match);
 static int hisi_l3c_pmu_init_data(struct platform_device *pdev,
 				  struct hisi_pmu *l3c_pmu)
 {
-	unsigned long long id;
 	struct resource *res;
-	acpi_status status;
-
-	status = acpi_evaluate_integer(ACPI_HANDLE(&pdev->dev),
-				       "_UID", NULL, &id);
-	if (ACPI_FAILURE(status))
-		return -EINVAL;
-
-	l3c_pmu->index_id = id;
 
 	/*
 	 * Use the SCCL_ID and CCL_ID to identify the L3C PMU, while

@@ -264,6 +255,8 @@ static int hisi_l3c_pmu_init_data(struct platform_device *pdev,
 		return -EINVAL;
 	}
 
+	l3c_pmu->index_id = l3c_pmu->ccl_id;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	l3c_pmu->base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(l3c_pmu->base)) {
Signature · 1 line
2.7.4
#1 lore raw

PATCH 1/1 perf: hisi: fix uncore PMU index ID

Hi Shaokun,

[+DT list]

Quoted 16 lines · Shaokun Zhang

According to ACPI SPEC about _UID (Unique ID), The _UID must be unique across all devices with either a common _HID or _CID. For HiSilion uncore PMU, SCCL_ID and INDEX_ID are used to identify the uncore PMU for the same _HID. Therefore, _UID is not equal to INDEX_ID in multi-sccl scene for the same uncore PMU device.

CCL_ID can be used as the INDEX_ID for the L3C PMU and IDX-ID is added in DSDT table for the HHA PMU.

Fixes 2940bc4("perf: hisi: Add support for HiSilicon SoC L3C PMU driver")
Fixes 2bab3cf("perf: hisi: Add support for HiSilicon SoC HHA PMU driver")
Reported-by Huiqiang Wang <[email protected]>
Signed-off-by Shaokun Zhang <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Huiqiang Wang <[email protected]>
Quoted 4 lines
---
 drivers/perf/hisilicon/hisi_uncore_hha_pmu.c | 18 ++++++++----------
 drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c | 11 ++---------
 2 files changed, 10 insertions(+), 19 deletions(-)

Whilst I'd normally just accept PMU driver submissions for vendor PMUs, this part rang my alarm bells:

Quoted 26 lines
drivers/perf/hisilicon/hisi_uncore_hha_pmu.c +1 -10
index 443906e..dcd8e77 100644
@@ -238,19 +238,10 @@ MODULE_DEVICE_TABLE(acpi, hisi_hha_pmu_acpi_match);
 static int hisi_hha_pmu_init_data(struct platform_device *pdev,
 				  struct hisi_pmu *hha_pmu)
 {
-	unsigned long long id;
 	struct resource *res;
-	acpi_status status;
-
-	status = acpi_evaluate_integer(ACPI_HANDLE(&pdev->dev),
-				       "_UID", NULL, &id);
-	if (ACPI_FAILURE(status))
-		return -EINVAL;
-
-	hha_pmu->index_id = id;
 
 	/*
-	 * Use SCCL_ID and UID to identify the HHA PMU, while
+	 * Use SCCL_ID and HHA index ID to identify the HHA PMU, while
 	 * SCCL_ID is in MPIDR[aff2].
 	 */
 	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",

drivers/perf/hisilicon/hisi_uncore_hha_pmu.c static int hisi_hha_pmu_init_data(struct platform_device *pdev, · line 257

Quoted 9 lines
drivers/perf/hisilicon/hisi_uncore_hha_pmu.c +6 -0
@@ -258,6 +249,13 @@ static int hisi_hha_pmu_init_data(struct platform_device *pdev,
 		dev_err(&pdev->dev, "Can not read hha sccl-id!\n");
 		return -EINVAL;
 	}
+
+	if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
+				     &hha_pmu->index_id)) {
+		dev_err(&pdev->dev, "Can not read hha index-id!\n");
+		return -EINVAL;
+	}

Is this a new DT property? If so, please can you update the binding documentation and get an Ack from a DT maintainer? It's not clear to me what a "hisilicon,idx-id" is, nor how I would generate on from firmware.

Thanks,

Will

author
#2 lore raw

PATCH 1/1 perf: hisi: fix uncore PMU index ID

Hi Will,

Quoted 59 lines · Will Deacon

Hi Shaokun,

[+DT list]

Quoted 16 lines · Shaokun Zhang

According to ACPI SPEC about _UID (Unique ID), The _UID must be unique across all devices with either a common _HID or _CID. For HiSilion uncore PMU, SCCL_ID and INDEX_ID are used to identify the uncore PMU for the same _HID. Therefore, _UID is not equal to INDEX_ID in multi-sccl scene for the same uncore PMU device.

CCL_ID can be used as the INDEX_ID for the L3C PMU and IDX-ID is added in DSDT table for the HHA PMU.

Fixes 2940bc4("perf: hisi: Add support for HiSilicon SoC L3C PMU driver")
Fixes 2bab3cf("perf: hisi: Add support for HiSilicon SoC HHA PMU driver")
Reported-by Huiqiang Wang <[email protected]>
Signed-off-by Shaokun Zhang <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Huiqiang Wang <[email protected]>
Quoted 4 lines
---
 drivers/perf/hisilicon/hisi_uncore_hha_pmu.c | 18 ++++++++----------
 drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c | 11 ++---------
 2 files changed, 10 insertions(+), 19 deletions(-)

Whilst I'd normally just accept PMU driver submissions for vendor PMUs, this part rang my alarm bells:

Quoted 30 lines
drivers/perf/hisilicon/hisi_uncore_hha_pmu.c +2 -10
index 443906e..dcd8e77 100644
@@ -238,19 +238,10 @@ MODULE_DEVICE_TABLE(acpi, hisi_hha_pmu_acpi_match);
 static int hisi_hha_pmu_init_data(struct platform_device *pdev,
 				  struct hisi_pmu *hha_pmu)
 {
-	unsigned long long id;
 	struct resource *res;
-	acpi_status status;
-
-	status = acpi_evaluate_integer(ACPI_HANDLE(&pdev->dev),
-				       "_UID", NULL, &id);
-	if (ACPI_FAILURE(status))
-		return -EINVAL;
-
-	hha_pmu->index_id = id;
 
 	/*
-	 * Use SCCL_ID and UID to identify the HHA PMU, while
+	 * Use SCCL_ID and HHA index ID to identify the HHA PMU, while
 	 * SCCL_ID is in MPIDR[aff2].
 	 */
 	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",

@@ -258,6 +249,13 @@ static int hisi_hha_pmu_init_data(struct platform_device *pdev,
 		dev_err(&pdev->dev, "Can not read hha sccl-id!\n");
 		return -EINVAL;
 	}
+
Quoted 8 lines

drivers/perf/hisilicon/hisi_uncore_hha_pmu.c static int hisi_hha_pmu_init_data(struct platform_device *pdev, · line 257

Quoted 5 lines
drivers/perf/hisilicon/hisi_uncore_hha_pmu.c +5 -0
+	if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
+				     &hha_pmu->index_id)) {
+		dev_err(&pdev->dev, "Can not read hha index-id!\n");
+		return -EINVAL;
+	}

Is this a new DT property? If so, please can you update the binding documentation and get an Ack from a DT maintainer? It's not clear to me

No, it is not a DT property. We don't support DT mode for this platform and only support ACPI mode.

Quoted 2 lines

what a "hisilicon,idx-id" is, nor how I would generate on from firmware.

For HiSilicon this platform, it supports multi-sccl. each sccl has more than one uncore PMUs. Like HHA uncore PMUs, each sccl has 2-HHA PMUs and idx-id is in _DSD package and used to distinguish different HHA PMUs with the same sccl, as follow:

    Name (_DSD, Package () {
      ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
      Package () {
        Package () {"hisilicon,scl-id", 0x03},
        Package () {"hisilicon,idx-id", 0x00},
      }
    })

Thanks,
Shaokun

Quoted 6 lines

Thanks,

Will

.

#3 lore raw

PATCH 1/1 perf: hisi: fix uncore PMU index ID

Quoted 43 lines · Zhangshaokun
Quoted 33 lines · Will Deacon

Whilst I'd normally just accept PMU driver submissions for vendor PMUs, this part rang my alarm bells:

Quoted 30 lines
drivers/perf/hisilicon/hisi_uncore_hha_pmu.c +2 -10
index 443906e..dcd8e77 100644
@@ -238,19 +238,10 @@ MODULE_DEVICE_TABLE(acpi, hisi_hha_pmu_acpi_match);
 static int hisi_hha_pmu_init_data(struct platform_device *pdev,
 				  struct hisi_pmu *hha_pmu)
 {
-	unsigned long long id;
 	struct resource *res;
-	acpi_status status;
-
-	status = acpi_evaluate_integer(ACPI_HANDLE(&pdev->dev),
-				       "_UID", NULL, &id);
-	if (ACPI_FAILURE(status))
-		return -EINVAL;
-
-	hha_pmu->index_id = id;
 
 	/*
-	 * Use SCCL_ID and UID to identify the HHA PMU, while
+	 * Use SCCL_ID and HHA index ID to identify the HHA PMU, while
 	 * SCCL_ID is in MPIDR[aff2].
 	 */
 	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",

@@ -258,6 +249,13 @@ static int hisi_hha_pmu_init_data(struct platform_device *pdev,
 		dev_err(&pdev->dev, "Can not read hha sccl-id!\n");
 		return -EINVAL;
 	}
+
Quoted 8 lines

drivers/perf/hisilicon/hisi_uncore_hha_pmu.c static int hisi_hha_pmu_init_data(struct platform_device *pdev, · line 257

Quoted 5 lines
drivers/perf/hisilicon/hisi_uncore_hha_pmu.c +5 -0
+	if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
+				     &hha_pmu->index_id)) {
+		dev_err(&pdev->dev, "Can not read hha index-id!\n");
+		return -EINVAL;
+	}

Is this a new DT property? If so, please can you update the binding documentation and get an Ack from a DT maintainer? It's not clear to me

Quoted 2 lines

No, it is not a DT property. We don't support DT mode for this platform and only support ACPI mode.

Hmm, but by using the firmware-agnostic "device_property_read_u32" interface, aren't you implicitly supporting it via DT as well? In fact, don't you now fail the probe if this new property isn't present? Isn't that a regression?

Quoted 6 lines
Quoted 2 lines

what a "hisilicon,idx-id" is, nor how I would generate on from firmware.

For HiSilicon this platform, it supports multi-sccl. each sccl has more than one uncore PMUs. Like HHA uncore PMUs, each sccl has 2-HHA PMUs and idx-id is in _DSD package and used to distinguish different HHA PMUs with the same sccl, as follow:

Quoted 7 lines
    Name (_DSD, Package () {
      ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
      Package () {
        Package () {"hisilicon,scl-id", 0x03},
        Package () {"hisilicon,idx-id", 0x00},
      }
    })

I'm still none the wiser about what this actually is. How is new _DSD crud supposed to be documented?

Will

author
#4 lore raw

PATCH 1/1 perf: hisi: fix uncore PMU index ID

Hi Will,

Quoted 44 lines · Will Deacon
Quoted 43 lines · Zhangshaokun
Quoted 41 lines · Will Deacon

Whilst I'd normally just accept PMU driver submissions for vendor PMUs, this part rang my alarm bells:

Quoted 26 lines
drivers/perf/hisilicon/hisi_uncore_hha_pmu.c +1 -10
index 443906e..dcd8e77 100644
@@ -238,19 +238,10 @@ MODULE_DEVICE_TABLE(acpi, hisi_hha_pmu_acpi_match);
 static int hisi_hha_pmu_init_data(struct platform_device *pdev,
 				  struct hisi_pmu *hha_pmu)
 {
-	unsigned long long id;
 	struct resource *res;
-	acpi_status status;
-
-	status = acpi_evaluate_integer(ACPI_HANDLE(&pdev->dev),
-				       "_UID", NULL, &id);
-	if (ACPI_FAILURE(status))
-		return -EINVAL;
-
-	hha_pmu->index_id = id;
 
 	/*
-	 * Use SCCL_ID and UID to identify the HHA PMU, while
+	 * Use SCCL_ID and HHA index ID to identify the HHA PMU, while
 	 * SCCL_ID is in MPIDR[aff2].
 	 */
 	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",

drivers/perf/hisilicon/hisi_uncore_hha_pmu.c static int hisi_hha_pmu_init_data(struct platform_device *pdev, · line 257

Quoted 9 lines
drivers/perf/hisilicon/hisi_uncore_hha_pmu.c +6 -0
@@ -258,6 +249,13 @@ static int hisi_hha_pmu_init_data(struct platform_device *pdev,
 		dev_err(&pdev->dev, "Can not read hha sccl-id!\n");
 		return -EINVAL;
 	}
+
+	if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
+				     &hha_pmu->index_id)) {
+		dev_err(&pdev->dev, "Can not read hha index-id!\n");
+		return -EINVAL;
+	}

Is this a new DT property? If so, please can you update the binding documentation and get an Ack from a DT maintainer? It's not clear to me

Quoted 8 lines
Quoted 2 lines

No, it is not a DT property. We don't support DT mode for this platform and only support ACPI mode.

Hmm, but by using the firmware-agnostic "device_property_read_u32" interface, aren't you implicitly supporting it via DT as well? In fact, don't you now fail the probe if this new property isn't present? Isn't that a regression?

Even though we don't support DT, using unified device interface seems a better practice in general. We could use acpi_dev_get_property(), but not much gain.
As for regression, this never worked anyway, right? And this is for a new platform with little distribution, so not much pain to upgrade the FW.

Quoted 14 lines
Quoted 6 lines
Quoted 2 lines

what a "hisilicon,idx-id" is, nor how I would generate on from firmware.

For HiSilicon this platform, it supports multi-sccl. each sccl has more than one uncore PMUs. Like HHA uncore PMUs, each sccl has 2-HHA PMUs and idx-id is in _DSD package and used to distinguish different HHA PMUs with the same sccl, as follow:

Quoted 7 lines
    Name (_DSD, Package () {
      ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
      Package () {
        Package () {"hisilicon,scl-id", 0x03},
        Package () {"hisilicon,idx-id", 0x00},
      }
    })
Quoted 3 lines

I'm still none the wiser about what this actually is. How is new _DSD crud supposed to be documented?

For instance:
when run perf list | grep hisi_sccl3_hha hisi_sccl3_hha0/rx_outer/ [kernel PMU event]
------------------------------------------
hisi_sccl3_hha1/rx_outer/ [kernel PMU event]
0 and 1 are the hha index in the same sccl that sccl-id is 3. We document as hisi_sccl{X}_<l3c{Y}/hha{Y}/ddrc{Y}> in Documentation/perf/hisi-pmu.txt

Thanks,
Shaokun

Quoted 4 lines

Will

.