[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <be916ba8-1d2a-89dd-7374-b07618fa9b29@os.amperecomputing.com>
Date: Mon, 5 Jun 2023 00:12:23 -0700 (PDT)
From: Ilkka Koskinen <ilkka@...amperecomputing.com>
To: Robin Murphy <robin.murphy@....com>
cc: will@...nel.org, mark.rutland@....com, suzuki.poulose@....com,
bwicaksono@...dia.com, ilkka@...amperecomputing.com,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
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@....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
Powered by blists - more mailing lists