[PATCH 5/5] perf/arm_cspmu: Add devicetree support

[Ilkka Koskinen]

On Thu, 7 Dec 2023, Robin Murphy wrote:
> On 2023-12-06 11:43 pm, Ilkka Koskinen wrote:
>> 
>> Hi Robin,
>> 
>> On Tue, 5 Dec 2023, Robin Murphy wrote:
>>> Hook up devicetree probing support. For now let's hope that people
>>> implement PMIIDR properly and we don't need an override mechanism.
>>> 
>>> Signed-off-by: Robin Murphy <robin.murphy at arm.com>
>>> ---
>>> drivers/perf/arm_cspmu/arm_cspmu.c | 71 +++++++++++++++++++++++-------
>>> 1 file changed, 56 insertions(+), 15 deletions(-)
>>> 
>>> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c 
>>> b/drivers/perf/arm_cspmu/arm_cspmu.c
>>> index b64de4d800c7..80b5fc417ee3 100644
>>> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
>>> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
>>> @@ -27,6 +27,7 @@
>>> #include <linux/io-64-nonatomic-lo-hi.h>
>>> #include <linux/module.h>
>>> #include <linux/mutex.h>
>>> +#include <linux/of.h>
>>> #include <linux/perf_event.h>
>>> #include <linux/platform_device.h>
>>> 
>>> @@ -309,6 +310,10 @@ 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;
>>> +    if (!has_acpi_companion(dev))
>> 
>> Am I missing something since this doesn't work on top of v6.7-rc4?
>> The problem I see is that has_acpi_companion() calls is_acpi_device_node(), 
>> which compares whether
>>
>>      fwnode->ops == &acpi_device_fwnode_ops;
>> 
>> However, the acpi/apmt code allocates fwnode by calling
>> acpi_alloc_fwnode_static(), which assigns &acpi_static_fwnode_ops
>> to ops.
>
> Ah, I hadn't got as far as considering that has_acpi_companion() might only 
> work for namespace devices, but it makes sense now that you point it out. I 
> should have clarified that I don't have any suitable ACPI system to test this 
> on, and have only been able to verify basic DT probing on an FPGA.

Ok, no problem. I'm always happy to give them a spin as soon as I find a 
little time.

Cheers, Ilkka


>
>> I wonder though, if is_acpi_device_node() should check the static variant 
>> too? :/
>
> Thanks for the tip, I'll look into that and try to come up with something 
> that works for a v2 (at worst there's always the traditional assumption that 
> !dev->of_node implies ACPI)
>
> Cheers,
> Robin.
>
>> 
>> Cheers, Ilkka
>> 
>>> +        return devm_kasprintf(dev, GFP_KERNEL, PMUNAME "_%u",
>>> +                      atomic_fetch_inc(&pmu_idx[0]));
>>> +
>>>     apmt_node = arm_cspmu_apmt_node(dev);
>>>     pmu_type = apmt_node->type;
>>> 
>>> @@ -406,7 +411,6 @@ static struct arm_cspmu_impl_match 
>>> *arm_cspmu_impl_match_get(u32 pmiidr)
>>> static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
>>> {
>>>     int ret = 0;
>>> -    struct acpi_apmt_node *apmt_node = arm_cspmu_apmt_node(cspmu->dev);
>>>     struct arm_cspmu_impl_match *match;
>>> 
>>>     /* Start with a default PMU implementation */
>>> @@ -425,8 +429,12 @@ static int arm_cspmu_init_impl_ops(struct arm_cspmu 
>>> *cspmu)
>>>     };
>>> 
>>>     /* Firmware may override implementer/product ID from PMIIDR */
>>> -    if (apmt_node->impl_id)
>>> -        cspmu->impl.pmiidr = apmt_node->impl_id;
>>> +    if (has_acpi_companion(cspmu->dev)) {
>>> +        struct acpi_apmt_node *apmt_node = 
>>> arm_cspmu_apmt_node(cspmu->dev);
>>> +
>>> +        if (apmt_node->impl_id)
>>> +            cspmu->impl.pmiidr = apmt_node->impl_id;
>>> +    }
>>> 
>>>     /* Find implementer specific attribute ops. */
>>>     match = arm_cspmu_impl_match_get(cspmu->impl.pmiidr);
>>> @@ -928,7 +936,6 @@ static void arm_cspmu_read(struct perf_event *event)
>>> 
>>> static struct arm_cspmu *arm_cspmu_alloc(struct platform_device *pdev)
>>> {
>>> -    struct acpi_apmt_node *apmt_node;
>>>     struct arm_cspmu *cspmu;
>>>     struct device *dev = &pdev->dev;
>>> 
>>> @@ -939,8 +946,13 @@ static struct arm_cspmu *arm_cspmu_alloc(struct 
>>> platform_device *pdev)
>>>     cspmu->dev = dev;
>>>     platform_set_drvdata(pdev, cspmu);
>>> 
>>> -    apmt_node = arm_cspmu_apmt_node(dev);
>>> -    cspmu->has_atomic_dword = apmt_node->flags & ACPI_APMT_FLAGS_ATOMIC;
>>> +    if (has_acpi_companion(dev)) {
>>> +        struct acpi_apmt_node *apmt_node = arm_cspmu_apmt_node(dev);
>>> +
>>> +        cspmu->has_atomic_dword = apmt_node->flags & 
>>> ACPI_APMT_FLAGS_ATOMIC;
>>> +    } else {
>>> +        cspmu->has_atomic_dword = device_property_read_bool(dev, 
>>> "arm,64-bit-atomic");
>>> +    }
>>> 
>>>     return cspmu;
>>> }
>>> @@ -1133,11 +1145,6 @@ static int arm_cspmu_acpi_get_cpus(struct arm_cspmu 
>>> *cspmu)
>>>         }
>>>     }
>>> 
>>> -    if (cpumask_empty(&cspmu->associated_cpus)) {
>>> -        dev_dbg(cspmu->dev, "No cpu associated with the PMU\n");
>>> -        return -ENODEV;
>>> -    }
>>> -
>>>     return 0;
>>> }
>>> #else
>>> @@ -1147,9 +1154,36 @@ static int arm_cspmu_acpi_get_cpus(struct arm_cspmu 
>>> *cspmu)
>>> }
>>> #endif
>>> 
>>> +static int arm_cspmu_of_get_cpus(struct arm_cspmu *cspmu)
>>> +{
>>> +    struct of_phandle_iterator it;
>>> +    int ret, cpu;
>>> +
>>> +    of_for_each_phandle(&it, ret, cspmu->dev->of_node, "cpus", NULL, 0) {
>>> +        cpu = of_cpu_node_to_id(it.node);
>>> +        if (cpu < 0)
>>> +            continue;
>>> +        cpumask_set_cpu(cpu, &cspmu->associated_cpus);
>>> +    }
>>> +    return ret;
>>> +}
>>> +
>>> static int arm_cspmu_get_cpus(struct arm_cspmu *cspmu)
>>> {
>>> -    return arm_cspmu_acpi_get_cpus(cspmu);
>>> +    int ret = 0;
>>> +
>>> +    if (has_acpi_companion(cspmu->dev))
>>> +        ret = arm_cspmu_acpi_get_cpus(cspmu);
>>> +    else if (of_property_present(cspmu->dev->of_node, "cpus"))
>>> +        ret = arm_cspmu_of_get_cpus(cspmu);
>>> +    else
>>> +        cpumask_copy(&cspmu->associated_cpus, cpu_possible_mask);
>>> +
>>> +    if (!ret && cpumask_empty(&cspmu->associated_cpus)) {
>>> +        dev_dbg(cspmu->dev, "No cpu associated with the PMU\n");
>>> +        ret = -ENODEV;
>>> +    }
>>> +    return ret;
>>> }
>>> 
>>> static int arm_cspmu_register_pmu(struct arm_cspmu *cspmu)
>>> @@ -1246,11 +1280,18 @@ static const struct platform_device_id 
>>> arm_cspmu_id[] = {
>>> };
>>> MODULE_DEVICE_TABLE(platform, arm_cspmu_id);
>>> 
>>> +static const struct of_device_id arm_cspmu_of_match[] = {
>>> +    { .compatible = "arm,coresight-pmu" },
>>> +    {}
>>> +};
>>> +MODULE_DEVICE_TABLE(of, arm_cspmu_of_match);
>>> +
>>> static struct platform_driver arm_cspmu_driver = {
>>>     .driver = {
>>> -            .name = DRVNAME,
>>> -            .suppress_bind_attrs = true,
>>> -        },
>>> +        .name = DRVNAME,
>>> +        .of_match_table = arm_cspmu_of_match,
>>> +        .suppress_bind_attrs = true,
>>> +    },
>>>     .probe = arm_cspmu_device_probe,
>>>     .remove = arm_cspmu_device_remove,
>>>     .id_table = arm_cspmu_id,
>>> -- 
>>> 2.39.2.101.g768bb238c484.dirty
>>> 
>>> 
>