Messages in this thread | ![/](/images/icornerl.gif) | | Date | Mon, 5 Jun 2023 00:12:23 -0700 (PDT) | From | Ilkka Koskinen <> | 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 <robin.murphy@arm.com> > --- > 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
| ![\](/images/icornerr.gif) |