2023-06-01 12:11:37

by Robin Murphy

[permalink] [raw]
Subject: [PATCH 0/4] perf/arm_cspmu: Fixes and cleanups

Hi all,

I've been working on adding support for the 64-bit extension and
devicetree probing[1], which I'm not posting yet since it's still
untested and may need a bit more work. However I think these preparatory
patches stand up well enough on their own so could land sooner.

Thanks,
Robin.

[1] https://gitlab.arm.com/linux-arm/linux-rm/-/commits/cspmu-dev/


Robin Murphy (4):
perf/arm_cspmu: Fix event attribute type
ACPI/APMT: Don't register invalid resource
perf/arm_cspmu: Clean up ACPI dependency
perf/arm_cspmu: Decouple APMT dependency

drivers/acpi/arm64/apmt.c | 10 +++--
drivers/perf/arm_cspmu/Kconfig | 3 +-
drivers/perf/arm_cspmu/arm_cspmu.c | 70 +++++++++++++-----------------
drivers/perf/arm_cspmu/arm_cspmu.h | 4 +-
4 files changed, 39 insertions(+), 48 deletions(-)

--
2.39.2.101.g768bb238c484.dirty



2023-06-01 12:11:49

by Robin Murphy

[permalink] [raw]
Subject: [PATCH 4/4] perf/arm_cspmu: Decouple APMT dependency

The functional paths of the driver need not care about ACPI, so abstract
the property of atomic doubleword access as its own flag (repacking the
structure for a better fit). We also do not need to go poking directly
at the APMT for standard resources which the ACPI layer has already
dealt with, so deal with the optional MMIO page and interrupt in the
normal firmware-agnostic manner. The few remaining portions of probing
that *are* APMT-specific can still easily retrieve the APMT pointer as
needed without us having to carry a duplicate copy around everywhere.

Signed-off-by: Robin Murphy <[email protected]>
---
drivers/perf/arm_cspmu/arm_cspmu.c | 45 ++++++++----------------------
drivers/perf/arm_cspmu/arm_cspmu.h | 4 +--
2 files changed, 13 insertions(+), 36 deletions(-)

diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index 3b91115c376d..f8daf252a488 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c
@@ -100,10 +100,6 @@
#define ARM_CSPMU_ACTIVE_CPU_MASK 0x0
#define ARM_CSPMU_ASSOCIATED_CPU_MASK 0x1

-/* Check if field f in flags is set with value v */
-#define CHECK_APMT_FLAG(flags, f, v) \
- ((flags & (ACPI_APMT_FLAGS_ ## f)) == (ACPI_APMT_FLAGS_ ## f ## _ ## v))
-
/* Check and use default if implementer doesn't provide attribute callback */
#define CHECK_DEFAULT_IMPL_OPS(ops, callback) \
do { \
@@ -155,12 +151,6 @@ static u64 read_reg64_hilohi(const void __iomem *addr, u32 max_poll_count)
return val;
}

-/* Check if PMU supports 64-bit single copy atomic. */
-static inline bool supports_64bit_atomics(const struct arm_cspmu *cspmu)
-{
- return CHECK_APMT_FLAG(cspmu->apmt_node->flags, ATOMIC, SUPP);
-}
-
/* Check if cycle counter is supported. */
static inline bool supports_cycle_counter(const struct arm_cspmu *cspmu)
{
@@ -319,7 +309,7 @@ static const char *arm_cspmu_get_name(const struct arm_cspmu *cspmu)
static atomic_t pmu_idx[ACPI_APMT_NODE_TYPE_COUNT] = { 0 };

dev = cspmu->dev;
- apmt_node = cspmu->apmt_node;
+ apmt_node = dev_get_platdata(dev);
pmu_type = apmt_node->type;

if (pmu_type >= ACPI_APMT_NODE_TYPE_COUNT) {
@@ -396,8 +386,8 @@ static const struct impl_match impl_match[] = {
static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
{
int ret;
- struct acpi_apmt_node *apmt_node = cspmu->apmt_node;
struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops;
+ struct acpi_apmt_node *apmt_node = dev_get_platdata(cspmu->dev);
const struct impl_match *match = impl_match;

/*
@@ -719,7 +709,7 @@ static u64 arm_cspmu_read_counter(struct perf_event *event)
offset = counter_offset(sizeof(u64), event->hw.idx);
counter_addr = cspmu->base1 + offset;

- return supports_64bit_atomics(cspmu) ?
+ return cspmu->has_atomic_dword ?
readq(counter_addr) :
read_reg64_hilohi(counter_addr, HILOHI_MAX_POLL);
}
@@ -910,24 +900,18 @@ static struct arm_cspmu *arm_cspmu_alloc(struct platform_device *pdev)
{
struct acpi_apmt_node *apmt_node;
struct arm_cspmu *cspmu;
- struct device *dev;
-
- dev = &pdev->dev;
- apmt_node = *(struct acpi_apmt_node **)dev_get_platdata(dev);
- if (!apmt_node) {
- dev_err(dev, "failed to get APMT node\n");
- return NULL;
- }
+ struct device *dev = &pdev->dev;

cspmu = devm_kzalloc(dev, sizeof(*cspmu), GFP_KERNEL);
if (!cspmu)
return NULL;

cspmu->dev = dev;
- cspmu->apmt_node = apmt_node;
-
platform_set_drvdata(pdev, cspmu);

+ apmt_node = dev_get_platdata(dev);
+ cspmu->has_atomic_dword = apmt_node->flags & ACPI_APMT_FLAGS_ATOMIC;
+
return cspmu;
}

@@ -935,11 +919,9 @@ static int arm_cspmu_init_mmio(struct arm_cspmu *cspmu)
{
struct device *dev;
struct platform_device *pdev;
- struct acpi_apmt_node *apmt_node;

dev = cspmu->dev;
pdev = to_platform_device(dev);
- apmt_node = cspmu->apmt_node;

/* Base address for page 0. */
cspmu->base0 = devm_platform_ioremap_resource(pdev, 0);
@@ -950,7 +932,7 @@ static int arm_cspmu_init_mmio(struct arm_cspmu *cspmu)

/* Base address for page 1 if supported. Otherwise point to page 0. */
cspmu->base1 = cspmu->base0;
- if (CHECK_APMT_FLAG(apmt_node->flags, DUAL_PAGE, SUPP)) {
+ if (platform_get_resource(pdev, IORESOURCE_MEM, 1)) {
cspmu->base1 = devm_platform_ioremap_resource(pdev, 1);
if (IS_ERR(cspmu->base1)) {
dev_err(dev, "ioremap failed for page-1 resource\n");
@@ -1047,19 +1029,14 @@ static int arm_cspmu_request_irq(struct arm_cspmu *cspmu)
int irq, ret;
struct device *dev;
struct platform_device *pdev;
- struct acpi_apmt_node *apmt_node;

dev = cspmu->dev;
pdev = to_platform_device(dev);
- apmt_node = cspmu->apmt_node;

/* Skip IRQ request if the PMU does not support overflow interrupt. */
- if (apmt_node->ovflw_irq == 0)
- return 0;
-
- irq = platform_get_irq(pdev, 0);
+ irq = platform_get_irq_optional(pdev, 0);
if (irq < 0)
- return irq;
+ return irq == -ENXIO ? 0 : irq;

ret = devm_request_irq(dev, irq, arm_cspmu_handle_irq,
IRQF_NOBALANCING | IRQF_NO_THREAD, dev_name(dev),
@@ -1109,7 +1086,7 @@ static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu)
int cpu;

dev = cspmu->pmu.dev;
- apmt_node = cspmu->apmt_node;
+ apmt_node = dev_get_platdata(dev);
affinity_flag = apmt_node->flags & ACPI_APMT_FLAGS_AFFINITY;

if (affinity_flag == ACPI_APMT_FLAGS_AFFINITY_PROC) {
diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h b/drivers/perf/arm_cspmu/arm_cspmu.h
index 51323b175a4a..7892e587f606 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.h
+++ b/drivers/perf/arm_cspmu/arm_cspmu.h
@@ -118,16 +118,16 @@ struct arm_cspmu_impl {
struct arm_cspmu {
struct pmu pmu;
struct device *dev;
- struct acpi_apmt_node *apmt_node;
const char *name;
const char *identifier;
void __iomem *base0;
void __iomem *base1;
- int irq;
cpumask_t associated_cpus;
cpumask_t active_cpu;
struct hlist_node cpuhp_node;
+ int irq;

+ bool has_atomic_dword;
u32 pmcfgr;
u32 num_logical_ctrs;
u32 num_set_clr_reg;
--
2.39.2.101.g768bb238c484.dirty


2023-06-01 12:11:53

by Robin Murphy

[permalink] [raw]
Subject: [PATCH 2/4] ACPI/APMT: Don't register invalid resource

Don't register a resource for the second page unless the dual-page
extension flag is actually present to say it's valid.

CC: Lorenzo Pieralisi <[email protected]>
CC: Hanjun Guo <[email protected]>
CC: Sudeep Holla <[email protected]>
Signed-off-by: Robin Murphy <[email protected]>
---

Not a critical fix, since the driver currently works around this itself,
but patch #4 would like to clean that up.

drivers/acpi/arm64/apmt.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/arm64/apmt.c b/drivers/acpi/arm64/apmt.c
index 8cab69fa5d59..aa7d5c3c0dd8 100644
--- a/drivers/acpi/arm64/apmt.c
+++ b/drivers/acpi/arm64/apmt.c
@@ -35,11 +35,13 @@ static int __init apmt_init_resources(struct resource *res,

num_res++;

- res[num_res].start = node->base_address1;
- res[num_res].end = node->base_address1 + SZ_4K - 1;
- res[num_res].flags = IORESOURCE_MEM;
+ if (node->flags & ACPI_APMT_FLAGS_DUAL_PAGE) {
+ res[num_res].start = node->base_address1;
+ res[num_res].end = node->base_address1 + SZ_4K - 1;
+ res[num_res].flags = IORESOURCE_MEM;

- num_res++;
+ num_res++;
+ }

if (node->ovflw_irq != 0) {
trigger = (node->ovflw_irq_flags & ACPI_APMT_OVFLW_IRQ_FLAGS_MODE);
--
2.39.2.101.g768bb238c484.dirty


2023-06-02 11:05:10

by Suzuki K Poulose

[permalink] [raw]
Subject: Re: [PATCH 2/4] ACPI/APMT: Don't register invalid resource

On 01/06/2023 12:59, Robin Murphy wrote:
> Don't register a resource for the second page unless the dual-page
> extension flag is actually present to say it's valid.
>
> CC: Lorenzo Pieralisi <[email protected]>
> CC: Hanjun Guo <[email protected]>
> CC: Sudeep Holla <[email protected]>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
>
> Not a critical fix, since the driver currently works around this itself,
> but patch #4 would like to clean that up.
>
> drivers/acpi/arm64/apmt.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/arm64/apmt.c b/drivers/acpi/arm64/apmt.c
> index 8cab69fa5d59..aa7d5c3c0dd8 100644
> --- a/drivers/acpi/arm64/apmt.c
> +++ b/drivers/acpi/arm64/apmt.c
> @@ -35,11 +35,13 @@ static int __init apmt_init_resources(struct resource *res,
>
> num_res++;
>
> - res[num_res].start = node->base_address1;
> - res[num_res].end = node->base_address1 + SZ_4K - 1;
> - res[num_res].flags = IORESOURCE_MEM;
> + if (node->flags & ACPI_APMT_FLAGS_DUAL_PAGE) {
> + res[num_res].start = node->base_address1;
> + res[num_res].end = node->base_address1 + SZ_4K - 1;
> + res[num_res].flags = IORESOURCE_MEM;
>
> - num_res++;
> + num_res++;
> + }
>
> if (node->ovflw_irq != 0) {
> trigger = (node->ovflw_irq_flags & ACPI_APMT_OVFLW_IRQ_FLAGS_MODE);

Reviewed-by: Suzuki K Poulose <[email protected]>

2023-06-02 11:08:14

by Suzuki K Poulose

[permalink] [raw]
Subject: Re: [PATCH 4/4] perf/arm_cspmu: Decouple APMT dependency

On 01/06/2023 12:59, Robin Murphy wrote:
> The functional paths of the driver need not care about ACPI, so abstract
> the property of atomic doubleword access as its own flag (repacking the
> structure for a better fit). We also do not need to go poking directly
> at the APMT for standard resources which the ACPI layer has already
> dealt with, so deal with the optional MMIO page and interrupt in the
> normal firmware-agnostic manner. The few remaining portions of probing
> that *are* APMT-specific can still easily retrieve the APMT pointer as
> needed without us having to carry a duplicate copy around everywhere.
>
> Signed-off-by: Robin Murphy <[email protected]>

Reviewed-by: Suzuki K Poulose <[email protected]>



2023-06-03 08:08:06

by Hanjun Guo

[permalink] [raw]
Subject: Re: [PATCH 2/4] ACPI/APMT: Don't register invalid resource

On 2023/6/1 19:59, Robin Murphy wrote:
> Don't register a resource for the second page unless the dual-page
> extension flag is actually present to say it's valid.
>
> CC: Lorenzo Pieralisi <[email protected]>
> CC: Hanjun Guo <[email protected]>
> CC: Sudeep Holla <[email protected]>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
>
> Not a critical fix, since the driver currently works around this itself,
> but patch #4 would like to clean that up.
>
> drivers/acpi/arm64/apmt.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/arm64/apmt.c b/drivers/acpi/arm64/apmt.c
> index 8cab69fa5d59..aa7d5c3c0dd8 100644
> --- a/drivers/acpi/arm64/apmt.c
> +++ b/drivers/acpi/arm64/apmt.c
> @@ -35,11 +35,13 @@ static int __init apmt_init_resources(struct resource *res,
>
> num_res++;
>
> - res[num_res].start = node->base_address1;
> - res[num_res].end = node->base_address1 + SZ_4K - 1;
> - res[num_res].flags = IORESOURCE_MEM;
> + if (node->flags & ACPI_APMT_FLAGS_DUAL_PAGE) {
> + res[num_res].start = node->base_address1;
> + res[num_res].end = node->base_address1 + SZ_4K - 1;
> + res[num_res].flags = IORESOURCE_MEM;
>
> - num_res++;
> + num_res++;
> + }
>
> if (node->ovflw_irq != 0) {
> trigger = (node->ovflw_irq_flags & ACPI_APMT_OVFLW_IRQ_FLAGS_MODE);

Good catch,

Reviewed-by: Hanjun Guo <[email protected]>

2023-06-05 07:31:49

by Ilkka Koskinen

[permalink] [raw]
Subject: Re: [PATCH 4/4] perf/arm_cspmu: Decouple APMT dependency


Hi Robin,

I have a couple of comments below

On Thu, 1 Jun 2023, Robin Murphy wrote:
> The functional paths of the driver need not care about ACPI, so abstract
> the property of atomic doubleword access as its own flag (repacking the
> structure for a better fit). We also do not need to go poking directly
> at the APMT for standard resources which the ACPI layer has already
> dealt with, so deal with the optional MMIO page and interrupt in the
> normal firmware-agnostic manner. The few remaining portions of probing
> that *are* APMT-specific can still easily retrieve the APMT pointer as
> needed without us having to carry a duplicate copy around everywhere.
>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
> drivers/perf/arm_cspmu/arm_cspmu.c | 45 ++++++++----------------------
> drivers/perf/arm_cspmu/arm_cspmu.h | 4 +--
> 2 files changed, 13 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index 3b91115c376d..f8daf252a488 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c

...

> @@ -319,7 +309,7 @@ static const char *arm_cspmu_get_name(const struct arm_cspmu *cspmu)
> static atomic_t pmu_idx[ACPI_APMT_NODE_TYPE_COUNT] = { 0 };
>
> dev = cspmu->dev;
> - apmt_node = cspmu->apmt_node;
> + apmt_node = dev_get_platdata(dev);

Was platdata changed too? If not, I think this should be

apmt_node = *(struct acpi_apmt_node **) dev_get_platdata(dev);

> pmu_type = apmt_node->type;
>
> if (pmu_type >= ACPI_APMT_NODE_TYPE_COUNT) {
> @@ -396,8 +386,8 @@ static const struct impl_match impl_match[] = {
> static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
> {
> int ret;
> - struct acpi_apmt_node *apmt_node = cspmu->apmt_node;
> struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops;
> + struct acpi_apmt_node *apmt_node = dev_get_platdata(cspmu->dev);

Ditto

> const struct impl_match *match = impl_match;
>
> /*

...

> @@ -910,24 +900,18 @@ static struct arm_cspmu *arm_cspmu_alloc(struct platform_device *pdev)
> {
> struct acpi_apmt_node *apmt_node;
> struct arm_cspmu *cspmu;
> - struct device *dev;
> -
> - dev = &pdev->dev;
> - apmt_node = *(struct acpi_apmt_node **)dev_get_platdata(dev);
> - if (!apmt_node) {
> - dev_err(dev, "failed to get APMT node\n");
> - return NULL;
> - }
> + struct device *dev = &pdev->dev;
>
> cspmu = devm_kzalloc(dev, sizeof(*cspmu), GFP_KERNEL);
> if (!cspmu)
> return NULL;
>
> cspmu->dev = dev;
> - cspmu->apmt_node = apmt_node;
> -
> platform_set_drvdata(pdev, cspmu);
>
> + apmt_node = dev_get_platdata(dev);

Ditto

> + cspmu->has_atomic_dword = apmt_node->flags & ACPI_APMT_FLAGS_ATOMIC;
> +
> return cspmu;
> }

...

> @@ -1047,19 +1029,14 @@ static int arm_cspmu_request_irq(struct arm_cspmu *cspmu)
> int irq, ret;
> struct device *dev;
> struct platform_device *pdev;
> - struct acpi_apmt_node *apmt_node;
>
> dev = cspmu->dev;
> pdev = to_platform_device(dev);
> - apmt_node = cspmu->apmt_node;
>
> /* Skip IRQ request if the PMU does not support overflow interrupt. */
> - if (apmt_node->ovflw_irq == 0)
> - return 0;
> -
> - irq = platform_get_irq(pdev, 0);
> + irq = platform_get_irq_optional(pdev, 0);
> if (irq < 0)
> - return irq;
> + return irq == -ENXIO ? 0 : irq;
>
> ret = devm_request_irq(dev, irq, arm_cspmu_handle_irq,
> IRQF_NOBALANCING | IRQF_NO_THREAD, dev_name(dev),
> @@ -1109,7 +1086,7 @@ static int arm_cspmu_acpi_get_cpus(struct arm_cspmu *cspmu)
> int cpu;
>
> dev = cspmu->pmu.dev;

You didn't touch this one but shouldn't this be

dev = cspmu->dev;

> - apmt_node = cspmu->apmt_node;
> + apmt_node = dev_get_platdata(dev);

Ditto

> affinity_flag = apmt_node->flags & ACPI_APMT_FLAGS_AFFINITY;


Otherwise the patch looks good to me.

Cheers, Ilkka

2023-06-05 07:43:41

by Ilkka Koskinen

[permalink] [raw]
Subject: Re: [PATCH 2/4] ACPI/APMT: Don't register invalid resource



On Thu, 1 Jun 2023, Robin Murphy wrote:
> Don't register a resource for the second page unless the dual-page
> extension flag is actually present to say it's valid.
>
> CC: Lorenzo Pieralisi <[email protected]>
> CC: Hanjun Guo <[email protected]>
> CC: Sudeep Holla <[email protected]>
> Signed-off-by: Robin Murphy <[email protected]>

Reviewed-and-tested-by: Ilkka Koskinen <[email protected]>

> ---
>
> Not a critical fix, since the driver currently works around this itself,
> but patch #4 would like to clean that up.
>
> drivers/acpi/arm64/apmt.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/arm64/apmt.c b/drivers/acpi/arm64/apmt.c
> index 8cab69fa5d59..aa7d5c3c0dd8 100644
> --- a/drivers/acpi/arm64/apmt.c
> +++ b/drivers/acpi/arm64/apmt.c
> @@ -35,11 +35,13 @@ static int __init apmt_init_resources(struct resource *res,
>
> num_res++;
>
> - res[num_res].start = node->base_address1;
> - res[num_res].end = node->base_address1 + SZ_4K - 1;
> - res[num_res].flags = IORESOURCE_MEM;
> + if (node->flags & ACPI_APMT_FLAGS_DUAL_PAGE) {
> + res[num_res].start = node->base_address1;
> + res[num_res].end = node->base_address1 + SZ_4K - 1;
> + res[num_res].flags = IORESOURCE_MEM;
>
> - num_res++;
> + num_res++;
> + }
>
> if (node->ovflw_irq != 0) {
> trigger = (node->ovflw_irq_flags & ACPI_APMT_OVFLW_IRQ_FLAGS_MODE);
> --
> 2.39.2.101.g768bb238c484.dirty
>
>

2023-06-05 11:06:32

by Robin Murphy

[permalink] [raw]
Subject: Re: [PATCH 4/4] perf/arm_cspmu: Decouple APMT dependency

On 2023-06-05 08:12, Ilkka Koskinen wrote:
>
> Hi Robin,
>
> I have a couple of comments below
>
> On Thu, 1 Jun 2023, Robin Murphy wrote:
>> The functional paths of the driver need not care about ACPI, so abstract
>> the property of atomic doubleword access as its own flag (repacking the
>> structure for a better fit). We also do not need to go poking directly
>> at the APMT for standard resources which the ACPI layer has already
>> dealt with, so deal with the optional MMIO page and interrupt in the
>> normal firmware-agnostic manner. The few remaining portions of probing
>> that *are* APMT-specific can still easily retrieve the APMT pointer as
>> needed without us having to carry a duplicate copy around everywhere.
>>
>> Signed-off-by: Robin Murphy <[email protected]>
>> ---
>> drivers/perf/arm_cspmu/arm_cspmu.c | 45 ++++++++----------------------
>> drivers/perf/arm_cspmu/arm_cspmu.h |  4 +--
>> 2 files changed, 13 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c
>> b/drivers/perf/arm_cspmu/arm_cspmu.c
>> index 3b91115c376d..f8daf252a488 100644
>> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
>> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
>
> ...
>
>> @@ -319,7 +309,7 @@ static const char *arm_cspmu_get_name(const struct
>> arm_cspmu *cspmu)
>>     static atomic_t pmu_idx[ACPI_APMT_NODE_TYPE_COUNT] = { 0 };
>>
>>     dev = cspmu->dev;
>> -    apmt_node = cspmu->apmt_node;
>> +    apmt_node = dev_get_platdata(dev);
>
> Was platdata changed too? If not, I think this should be
>
>     apmt_node = *(struct acpi_apmt_node **) dev_get_platdata(dev);

Oof, indeed it looks like I got the wrong thing into my head at the
critical moment :(

Clearly this deserves a nice helpful wrapper so we only have to think
about the nasty casting once...

>>     pmu_type = apmt_node->type;
>>
>>     if (pmu_type >= ACPI_APMT_NODE_TYPE_COUNT) {
>> @@ -396,8 +386,8 @@ static const struct impl_match impl_match[] = {
>> static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
>> {
>>     int ret;
>> -    struct acpi_apmt_node *apmt_node = cspmu->apmt_node;
>>     struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops;
>> +    struct acpi_apmt_node *apmt_node = dev_get_platdata(cspmu->dev);
>
> Ditto
>
>>     const struct impl_match *match = impl_match;
>>
>>     /*
>
> ...
>
>> @@ -910,24 +900,18 @@ static struct arm_cspmu *arm_cspmu_alloc(struct
>> platform_device *pdev)
>> {
>>     struct acpi_apmt_node *apmt_node;
>>     struct arm_cspmu *cspmu;
>> -    struct device *dev;
>> -
>> -    dev = &pdev->dev;
>> -    apmt_node = *(struct acpi_apmt_node **)dev_get_platdata(dev);
>> -    if (!apmt_node) {
>> -        dev_err(dev, "failed to get APMT node\n");
>> -        return NULL;
>> -    }
>> +    struct device *dev = &pdev->dev;
>>
>>     cspmu = devm_kzalloc(dev, sizeof(*cspmu), GFP_KERNEL);
>>     if (!cspmu)
>>         return NULL;
>>
>>     cspmu->dev = dev;
>> -    cspmu->apmt_node = apmt_node;
>> -
>>     platform_set_drvdata(pdev, cspmu);
>>
>> +    apmt_node = dev_get_platdata(dev);
>
> Ditto
>
>> +    cspmu->has_atomic_dword = apmt_node->flags & ACPI_APMT_FLAGS_ATOMIC;
>> +
>>     return cspmu;
>> }
>
> ...
>
>> @@ -1047,19 +1029,14 @@ static int arm_cspmu_request_irq(struct
>> arm_cspmu *cspmu)
>>     int irq, ret;
>>     struct device *dev;
>>     struct platform_device *pdev;
>> -    struct acpi_apmt_node *apmt_node;
>>
>>     dev = cspmu->dev;
>>     pdev = to_platform_device(dev);
>> -    apmt_node = cspmu->apmt_node;
>>
>>     /* Skip IRQ request if the PMU does not support overflow
>> interrupt. */
>> -    if (apmt_node->ovflw_irq == 0)
>> -        return 0;
>> -
>> -    irq = platform_get_irq(pdev, 0);
>> +    irq = platform_get_irq_optional(pdev, 0);
>>     if (irq < 0)
>> -        return irq;
>> +        return irq == -ENXIO ? 0 : irq;
>>
>>     ret = devm_request_irq(dev, irq, arm_cspmu_handle_irq,
>>                    IRQF_NOBALANCING | IRQF_NO_THREAD, dev_name(dev),
>> @@ -1109,7 +1086,7 @@ static int arm_cspmu_acpi_get_cpus(struct
>> arm_cspmu *cspmu)
>>     int cpu;
>>
>>     dev = cspmu->pmu.dev;
>
> You didn't touch this one but shouldn't this be
>
>     dev = cspmu->dev;

Good catch - attributing the error message to the wrong device doesn't
matter too much currently, but this change does then need it to be
right. Will fix that too.

Thanks!
Robin.

>
>> -    apmt_node = cspmu->apmt_node;
>> +    apmt_node = dev_get_platdata(dev);
>
> Ditto
>
>>     affinity_flag = apmt_node->flags & ACPI_APMT_FLAGS_AFFINITY;
>
>
> Otherwise the patch looks good to me.
>
> Cheers, Ilkka