Re: [PATCH v2 3/4] perf/arm_cspmu: Clean up ACPI dependency

Re: [PATCH v2 3/4] perf/arm_cspmu: Clean up ACPI dependency

[Geert Uytterhoeven]

Hi Robin,

On Mon, Jun 5, 2023 at 7:05 PM Robin Murphy <robin.murphy@arm.com> wrote:
> Build-wise, the ACPI dependency consists of only a couple of things
> which could probably stand being factored out into ACPI helpers anyway.
> However for the immediate concern of working towards Devicetree support
> here, it's easy enough to make a few tweaks to contain the affected code
> locally, such that we can relax the Kconfig dependency.
>
> Reviewed-and-Tested-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Reviewed-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Thanks for your patch, which is now commit f9bd34e3753ea8f1
("perf/arm_cspmu: Clean up ACPI dependency") upstream.

> --- a/drivers/perf/arm_cspmu/Kconfig
> +++ b/drivers/perf/arm_cspmu/Kconfig
> @@ -4,8 +4,7 @@
>
>  config ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU
>         tristate "ARM Coresight Architecture PMU"
> -       depends on ARM64 && ACPI
> -       depends on ACPI_APMT || COMPILE_TEST
> +       depends on ARM64 || COMPILE_TEST

From looking at the code, the "arm-cs-arch-pmu" platform device can
be instantiated only through ACPI.  So I think it is a bit premature to
relax the dependency, and expose this question to people configuring
an ARM64 kernel without ACPI/APMT support.

Am I missing something?
Thanks!

>         help
>           Provides support for performance monitoring unit (PMU) devices
>           based on ARM CoreSight PMU architecture. Note that this PMU

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 3/4] perf/arm_cspmu: Clean up ACPI dependency

[Robin Murphy]

Hi Geert,

On 2023-07-03 10:21, Geert Uytterhoeven wrote:
> Hi Robin,
> 
> On Mon, Jun 5, 2023 at 7:05 PM Robin Murphy <robin.murphy@arm.com> wrote:
>> Build-wise, the ACPI dependency consists of only a couple of things
>> which could probably stand being factored out into ACPI helpers anyway.
>> However for the immediate concern of working towards Devicetree support
>> here, it's easy enough to make a few tweaks to contain the affected code
>> locally, such that we can relax the Kconfig dependency.
>>
>> Reviewed-and-Tested-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Reviewed-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> 
> Thanks for your patch, which is now commit f9bd34e3753ea8f1
> ("perf/arm_cspmu: Clean up ACPI dependency") upstream.
> 
>> --- a/drivers/perf/arm_cspmu/Kconfig
>> +++ b/drivers/perf/arm_cspmu/Kconfig
>> @@ -4,8 +4,7 @@
>>
>>   config ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU
>>          tristate "ARM Coresight Architecture PMU"
>> -       depends on ARM64 && ACPI
>> -       depends on ACPI_APMT || COMPILE_TEST
>> +       depends on ARM64 || COMPILE_TEST
> 
>  From looking at the code, the "arm-cs-arch-pmu" platform device can
> be instantiated only through ACPI.  So I think it is a bit premature to
> relax the dependency, and expose this question to people configuring
> an ARM64 kernel without ACPI/APMT support.
> 
> Am I missing something?

As was mentioned in the original cover letter on v1, these patches were 
actually the bottom half of a branch adding DT support - the DT parts 
are still untested and not quite complete (there's a property I don't 
need for the thing I'm looking at, but still deserves to be hooked up in 
general), but it seemed worth landing these prep patches since they 
impact what Besar and Ilkka are also working on in parallel.

At this point, the kconfig could indeed be "depends on (ARM64 && 
ACPI_APMT) || COMPILE_TEST". I can't recall why I didn't change that 
when splitting these patches out for posting - I may have decided the 
impact was negligible (i.e. even with DT support, it's still going to be 
a driver most people won't care about anyway), or the visibility vs. 
functional dependency aspect may have just slipped my mind entirely. 
You're welcome to make that change for now if you'd like to.

(I'm not sure how soon I'll be posting the follow-up DT patches, since 
I'm dependent on other people to provide testing and feedback, and 
haven't heard any news yet)

Thanks,
Robin.


> Thanks!
> 
>>          help
>>            Provides support for performance monitoring unit (PMU) devices
>>            based on ARM CoreSight PMU architecture. Note that this PMU
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
>