| Message ID | 1526901317-52248-1-git-send-email-zhangshaokun@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 > > . >
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
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 > > . >
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)) {
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(-)