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

[Robin Murphy]

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@xxxxxxxxxxxxxxxxxxxxxx>
> > Sent: Tuesday, August 19, 2025 3:16 PM
> > To: Besar Wicaksono <bwicaksono@xxxxxxxxxx>
> > Cc: will@xxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx; linux-tegra@xxxxxxxxxxxxxxx;
> > suzuki.poulose@xxxxxxx; robin.murphy@xxxxxxx;
> > ilkka@xxxxxxxxxxxxxxxxxxxxxx; mark.rutland@xxxxxxx; Thierry Reding
> > <treding@xxxxxxxxxx>; Jon Hunter <jonathanh@xxxxxxxxxx>; Vikram Sethi
> > <vsethi@xxxxxxxxxx>; Rich Wiley <rwiley@xxxxxxxxxx>; Shanker Donthineni
> > <sdonthineni@xxxxxxxxxx>
> > 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@xxxxxxxxxx>
> > > ---
> > > 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