* [PATCH] perf: hisi: fix uncore PMU index ID
@ 2018-05-21 11:15 Shaokun Zhang
2018-05-21 17:05 ` Will Deacon
0 siblings, 1 reply; 9+ messages in thread
From: Shaokun Zhang @ 2018-05-21 11:15 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH] perf: hisi: fix uncore PMU index ID 2018-05-21 11:15 [PATCH] perf: hisi: fix uncore PMU index ID Shaokun Zhang @ 2018-05-21 17:05 ` Will Deacon 0 siblings, 0 replies; 9+ messages in thread From: Will Deacon @ 2018-05-21 17:05 UTC (permalink / raw) To: 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] 9+ messages in thread
* Re: [PATCH] perf: hisi: fix uncore PMU index ID @ 2018-05-21 17:05 ` Will Deacon 0 siblings, 0 replies; 9+ 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] 9+ messages in thread
* [PATCH] perf: hisi: fix uncore PMU index ID 2018-05-21 17:05 ` Will Deacon @ 2018-05-22 9:18 ` Zhangshaokun -1 siblings, 0 replies; 9+ messages in thread From: Zhangshaokun @ 2018-05-22 9:18 UTC (permalink / raw) To: linux-arm-kernel 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 > > . > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] perf: hisi: fix uncore PMU index ID @ 2018-05-22 9:18 ` Zhangshaokun 0 siblings, 0 replies; 9+ messages in thread From: Zhangshaokun @ 2018-05-22 9:18 UTC (permalink / raw) To: Will Deacon Cc: Mark Rutland, devicetree, Huiqiang Wang, linux-arm-kernel, Linuxarm 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 > > . > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] perf: hisi: fix uncore PMU index ID 2018-05-22 9:18 ` Zhangshaokun @ 2018-05-23 16:42 ` Will Deacon -1 siblings, 0 replies; 9+ messages in thread From: Will Deacon @ 2018-05-23 16:42 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] perf: hisi: fix uncore PMU index ID @ 2018-05-23 16:42 ` Will Deacon 0 siblings, 0 replies; 9+ messages in thread From: Will Deacon @ 2018-05-23 16:42 UTC (permalink / raw) To: Zhangshaokun Cc: Mark Rutland, devicetree, Huiqiang Wang, linux-arm-kernel, Linuxarm 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] perf: hisi: fix uncore PMU index ID 2018-05-23 16:42 ` Will Deacon @ 2018-05-25 8:34 ` Zhangshaokun -1 siblings, 0 replies; 9+ messages in thread From: Zhangshaokun @ 2018-05-25 8:34 UTC (permalink / raw) To: linux-arm-kernel 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 > > . > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] perf: hisi: fix uncore PMU index ID @ 2018-05-25 8:34 ` Zhangshaokun 0 siblings, 0 replies; 9+ messages in thread From: Zhangshaokun @ 2018-05-25 8:34 UTC (permalink / raw) To: Will Deacon Cc: Mark Rutland, devicetree, Huiqiang Wang, linux-arm-kernel, Linuxarm 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 > > . > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-05-25 8:34 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-05-21 11:15 [PATCH] perf: hisi: fix uncore PMU index ID Shaokun Zhang 2018-05-21 17:05 ` Will Deacon 2018-05-21 17:05 ` Will Deacon 2018-05-22 9:18 ` Zhangshaokun 2018-05-22 9:18 ` Zhangshaokun 2018-05-23 16:42 ` Will Deacon 2018-05-23 16:42 ` Will Deacon 2018-05-25 8:34 ` Zhangshaokun 2018-05-25 8:34 ` Zhangshaokun
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.