Re: [PATCH 1/5] perf/arm_cspmu: Export arm_cspmu_apmt_node

[Robin Murphy <robin.murphy@arm.com>]

On 2025-08-20 8:07 pm, Besar Wicaksono wrote:
> Hi Ilkka,
> 
> Thanks for the feedback. Please see my comment inline.
> 
>> -----Original Message-----
>> From: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>> Sent: Tuesday, August 19, 2025 3:16 PM
>> To: Besar Wicaksono <bwicaksono@nvidia.com>
>> Cc: will@kernel.org; linux-arm-kernel@lists.infradead.org; linux-
>> kernel@vger.kernel.org; linux-tegra@vger.kernel.org;
>> suzuki.poulose@arm.com; robin.murphy@arm.com;
>> ilkka@os.amperecomputing.com; mark.rutland@arm.com; Thierry Reding
>> <treding@nvidia.com>; Jon Hunter <jonathanh@nvidia.com>; Vikram Sethi
>> <vsethi@nvidia.com>; Rich Wiley <rwiley@nvidia.com>; Shanker Donthineni
>> <sdonthineni@nvidia.com>
>> Subject: Re: [PATCH 1/5] perf/arm_cspmu: Export arm_cspmu_apmt_node
>>
>> External email: Use caution opening links or attachments
>>
>>
>> Hi Ben,
>>
>> On Tue, 12 Aug 2025, Besar Wicaksono wrote:
>>> Make arm_cspmu_apmt_node API accessible to vendor driver.
>>
>> I think I haven't seen the latest version of the spec. So, I'm curious,
>> what kind of information the table has that the vendor drivers needs to
>> have access to it?
>>
> 
> The vendor driver may need the node instance primary and secondary
> fields to get additional properties of the PMU that is not covered
> by the standard properties. For example, the PMU device entry in
> APMT can be defined as ACPI node type. The node instance primary
> and secondary will contain the HID and UID of an ACPI device object
> that is associated with the PMU. This ACPI object can have more info
> to supplement the standard props.

Rather than exposing the raw APMT data, maybe then cspmu should just 
encapsulate a method for retrieving the associated ACPI device, if any? 
I guess it could be a generalised "firmware device" notion - even though 
for DT that should mostly be cspmu->dev already - since that could then 
work neatly for generic device properties, but perhaps we don't expect 
many sub-drivers to support both ACPI and DT...

Thanks,
Robin.

>>>
>>> Signed-off-by: Besar Wicaksono <bwicaksono@nvidia.com>
>>> ---
>>> drivers/perf/arm_cspmu/arm_cspmu.c | 3 ++-
>>> drivers/perf/arm_cspmu/arm_cspmu.h | 4 ++++
>>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c
>> b/drivers/perf/arm_cspmu/arm_cspmu.c
>>> index efa9b229e701..e4b98cfa606c 100644
>>> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
>>> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
>>> @@ -70,12 +70,13 @@ static void arm_cspmu_set_ev_filter(struct
>> arm_cspmu *cspmu,
>>> static void arm_cspmu_set_cc_filter(struct arm_cspmu *cspmu,
>>>                                    const struct perf_event *event);
>>>
>>> -static struct acpi_apmt_node *arm_cspmu_apmt_node(struct device *dev)
>>> +struct acpi_apmt_node *arm_cspmu_apmt_node(struct device *dev)
>>> {
>>>        struct acpi_apmt_node **ptr = dev_get_platdata(dev);
>>>
>>>        return ptr ? *ptr : NULL;
>>> }
>>> +EXPORT_SYMBOL_GPL(arm_cspmu_apmt_node);
>>
>> Rather than exporting the function, wouldn't it be better to move it to
>> arm_cspmu.h instead?
> 
> Sounds good to me. I will make the change on the next revision.
> 
> Thanks,
> Besar
> 
>>
>> Cheers, Ilkka
>>
>>>
>>> /*
>>>   * In CoreSight PMU architecture, all of the MMIO registers are 32-bit except
>>> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h
>> b/drivers/perf/arm_cspmu/arm_cspmu.h
>>> index 19684b76bd96..36c1dcce33d6 100644
>>> --- a/drivers/perf/arm_cspmu/arm_cspmu.h
>>> +++ b/drivers/perf/arm_cspmu/arm_cspmu.h
>>> @@ -8,6 +8,7 @@
>>> #ifndef __ARM_CSPMU_H__
>>> #define __ARM_CSPMU_H__
>>>
>>> +#include <linux/acpi.h>
>>> #include <linux/bitfield.h>
>>> #include <linux/cpumask.h>
>>> #include <linux/device.h>
>>> @@ -222,4 +223,7 @@ int arm_cspmu_impl_register(const struct
>> arm_cspmu_impl_match *impl_match);
>>> /* Unregister vendor backend. */
>>> void arm_cspmu_impl_unregister(const struct arm_cspmu_impl_match
>> *impl_match);
>>>
>>> +/* Get ACPI APMT node. */
>>> +struct acpi_apmt_node *arm_cspmu_apmt_node(struct device *dev);
>>> +
>>> #endif /* __ARM_CSPMU_H__ */
>>> --
>>> 2.47.0
>>>
>>>