[PATCH] perf: hisi: fix uncore PMU index ID

[PATCH] perf: hisi: fix uncore PMU index ID

[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 <wanghuiqiang@huawei.com>
Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Huiqiang Wang <wanghuiqiang@huawei.com>
---
 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(-)

diff --git a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
index 443906e..dcd8e77 100644
--- a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
@@ -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;
 
diff --git a/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
index 0bde5d9..133889c 100644
--- a/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
@@ -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)) {
-- 
2.7.4

[PATCH] perf: hisi: fix uncore PMU index ID

[Will Deacon]

Hi Shaokun,

[+DT list]

On Mon, May 21, 2018 at 07:15:17PM +0800, Shaokun Zhang wrote:
> 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 <wanghuiqiang@huawei.com>
> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Huiqiang Wang <wanghuiqiang@huawei.com>
> ---
>  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:

> diff --git a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> index 443906e..dcd8e77 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> @@ -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;
> +	}

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

[PATCH] perf: hisi: fix uncore PMU index ID

[Zhangshaokun]

Hi Will,

On 2018/5/22 1:05, Will Deacon wrote:
> Hi Shaokun,
> 
> [+DT list]
> 
> On Mon, May 21, 2018 at 07:15:17PM +0800, Shaokun Zhang wrote:
>> 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 <wanghuiqiang@huawei.com>
>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Huiqiang Wang <wanghuiqiang@huawei.com>
>> ---
>>  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:
> 
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
>> index 443906e..dcd8e77 100644
>> --- a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
>> +++ b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
>> @@ -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;
>> +	}
> 
> 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.

> 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

> Thanks,
> 
> Will
> 
> .
> 

[PATCH] perf: hisi: fix uncore PMU index ID

[Will Deacon]

On Tue, May 22, 2018 at 05:18:51PM +0800, Zhangshaokun wrote:
> On 2018/5/22 1:05, Will Deacon wrote:
> > Whilst I'd normally just accept PMU driver submissions for vendor PMUs,
> > this part rang my alarm bells:
> > 
> >> diff --git a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> >> index 443906e..dcd8e77 100644
> >> --- a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> >> +++ b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> >> @@ -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;
> >> +	}
> > 
> > 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.

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?

> > 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},
>       }
>     })

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

Will

[PATCH] perf: hisi: fix uncore PMU index ID

[Zhangshaokun]

Hi Will,

On 2018/5/24 0:42, Will Deacon wrote:
> On Tue, May 22, 2018 at 05:18:51PM +0800, Zhangshaokun wrote:
>> On 2018/5/22 1:05, Will Deacon wrote:
>>> Whilst I'd normally just accept PMU driver submissions for vendor PMUs,
>>> this part rang my alarm bells:
>>>
>>>> diff --git a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
>>>> index 443906e..dcd8e77 100644
>>>> --- a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
>>>> +++ b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
>>>> @@ -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;
>>>> +	}
>>>
>>> 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.
> 
> 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.

>>> 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},
>>       }
>>     })
> 
> 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

> Will
> 
> .
>