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

* Re: [PATCH] perf: hisi: fix uncore PMU index ID
       [not found] <1526901317-52248-1-git-send-email-zhangshaokun@hisilicon.com>
@ 2018-05-21 17:05 ` Will Deacon
  2018-05-22  9:18   ` Zhangshaokun
  0 siblings, 1 reply; 4+ messages in thread
From: Will Deacon @ 2018-05-21 17:05 UTC (permalink / raw)
  To: Shaokun Zhang; +Cc: Mark Rutland, devicetree, Huiqiang Wang, linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 4+ messages in thread