[PATCH 1/3] arm_pmu: acpi: Add a representative platform device for TRBE
Anshuman Khandual
anshuman.khandual at arm.com
Mon Jul 31 20:35:54 PDT 2023
On 7/31/23 20:29, Will Deacon wrote:
> On Mon, Jul 31, 2023 at 05:38:38PM +0530, Anshuman Khandual wrote:
>> On 7/28/23 20:10, Will Deacon wrote:
>>> On Fri, Jul 28, 2023 at 04:57:31PM +0530, Anshuman Khandual wrote:
>>>> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
>>>> index 90815ad762eb..dd3df6729808 100644
>>>> --- a/drivers/perf/arm_pmu_acpi.c
>>>> +++ b/drivers/perf/arm_pmu_acpi.c
>
> [...]
>
>>>> + ret = platform_device_register(&trbe_acpi_dev);
>>>> + if (ret < 0) {
>>>> + pr_warn("ACPI: TRBE: Unable to register device\n");
>>>> + acpi_unregister_gsi(gsi);
>>>> + }
>>>> +}
>>>> +#else
>>>> +static inline void arm_trbe_acpi_register_device(void)
>>>> +{
>>>> +
>>>> +}
>>>> +#endif /* CONFIG_CORESIGHT_TRBE */
>>>
>>> This looks like you ran s/spe/trbe/ over the SPE device registration
>>> code :)
>>
>> Yeah, almost :)
>>
>>> Please can you refactor things so we don't have all the duplication? I
>>> suspect this won't be the last device which needs the same treatement.
>>
>> Should the refactoring just accommodate SPE, and TRBE or make it more generic to
>> accommodate future devices as well. Something like the following enumeration.
>>
>> enum arm_platform_device {
>> ARM_PLATFORM_DEVICE_SPE,
>> ARM_PLATFORM_DEVICE_TRBE,
>> ARM_PLATFORM_DEVICE_MAX,
>> };
>>
>> But that would require adding some helper functions to select these following
>> elements based on the above enumeration via a common function
>>
>> - gicc->XXX_interrupt
>> - ACPI_MADT_GICC_SPE/TRBE for header length comparison
>> - static struct platform_device/resources (static objects in the file)
>>
>> Seems like will add much more code for a refactor. Did you have something else
>> in mind for the refactor.
>
> All I'm saying is that we shouldn't have identical copies of the code to
> walk the MADT, pull out the irqs and register the device.
>
> So something like the totally untested hack below. I probably broke
> something, but hopefully you see what I mean.
>
> Will
>
> --->8
>
> diff --git a/drivers/perf/arm_pmu_acpi.c b/drivers/perf/arm_pmu_acpi.c
> index 90815ad762eb..7f1cf36c6e69 100644
> --- a/drivers/perf/arm_pmu_acpi.c
> +++ b/drivers/perf/arm_pmu_acpi.c
> @@ -69,6 +69,62 @@ static void arm_pmu_acpi_unregister_irq(int cpu)
> acpi_unregister_gsi(gsi);
> }
>
> +static int
> +arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
> + u16 (*parse_gsi)(struct acpi_madt_generic_interrupt *))
This factored out helper should be wrapped inside CONFIG_ARM_SPE_PMU
and CONFIG_CORESIGHT_TRBE ? Otherwise, there will be no callers left
for this helper triggering warning.
drivers/perf/arm_pmu_acpi.c:73:1: warning: ‘arm_acpi_register_pmu_device’ defined but not used [-Wunused-function]
73 | arm_acpi_register_pmu_device(struct platform_device *pdev, u8 len,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
But in that case, we have to keep adding new configs when new devices
require platform devices to be registered. Is there a better way ?
> +{
> + int cpu, hetid, irq, ret;
> + bool matched = false;
> + u16 gsi = 0;
> +
> + if (pdev->num_resources != 1)
> + return -ENXIO;
> +
> + if (pdev->resource[0].flags != IORESOURCE_IRQ)
> + return -ENXIO;
> +
> + /*
> + * Sanity check all the GICC tables for the same interrupt number.
> + * For now, we only support homogeneous ACPI machines.
> + */
> + for_each_possible_cpu(cpu) {
> + struct acpi_madt_generic_interrupt *gicc;
> + u16 this_gsi;
> +
> + gicc = acpi_cpu_get_madt_gicc(cpu);
> + if (gicc->header.length < len)
> + return matched ? -ENXIO : 0;
> +
> + this_gsi = parse_gsi(gicc);
> + if (!matched) {
> + hetid = find_acpi_cpu_topology_hetero_id(cpu);
> + gsi = this_gsi;
> + matched = true;
> + } else if (hetid != find_acpi_cpu_topology_hetero_id(cpu) ||
> + gsi != this_gsi) {
> + pr_warn("ACPI: %s: must be homogeneous\n", pdev->name);
> + return -ENXIO;
> + }
> + }
> +
> + irq = acpi_register_gsi(NULL, gsi, ACPI_LEVEL_SENSITIVE,
> + ACPI_ACTIVE_HIGH);
> + if (irq < 0) {
> + pr_warn("ACPI: %s Unable to register interrupt: %d\n",
> + pdev->name, gsi);
> + return -ENXIO;
> + }
> +
> + pdev->resource[0].start = irq;
> + ret = platform_device_register(pdev);
> + if (ret < 0) {
> + pr_warn("ACPI: %s: Unable to register device\n", pdev->name);
> + acpi_unregister_gsi(gsi);
> + }
> +
> + return ret;
> +}
> +
> #if IS_ENABLED(CONFIG_ARM_SPE_PMU)
> static struct resource spe_resources[] = {
> {
> @@ -89,49 +145,18 @@ static struct platform_device spe_dev = {
> * and create a SPE device if we detect a recent MADT with
> * a homogeneous PPI mapping.
> */
> +static u16 arm_spe_parse_gsi(struct acpi_madt_generic_interrupt *gicc)
> +{
> + return gicc->spe_interrupt;
> +}
> +
> static void arm_spe_acpi_register_device(void)
> {
> - int cpu, hetid, irq, ret;
> - bool first = true;
> - u16 gsi = 0;
> + int err = arm_acpi_register_pmu_device(&spe_dev, ACPI_MADT_GICC_SPE,
> + arm_spe_parse_gsi);
>
> - /*
> - * Sanity check all the GICC tables for the same interrupt number.
> - * For now, we only support homogeneous ACPI/SPE machines.
> - */
> - for_each_possible_cpu(cpu) {
> - struct acpi_madt_generic_interrupt *gicc;
> -
> - gicc = acpi_cpu_get_madt_gicc(cpu);
> - if (gicc->header.length < ACPI_MADT_GICC_SPE)
> - return;
> -
> - if (first) {
> - gsi = gicc->spe_interrupt;
> - if (!gsi)
> - return;
> - hetid = find_acpi_cpu_topology_hetero_id(cpu);
> - first = false;
> - } else if ((gsi != gicc->spe_interrupt) ||
> - (hetid != find_acpi_cpu_topology_hetero_id(cpu))) {
> - pr_warn("ACPI: SPE must be homogeneous\n");
> - return;
> - }
> - }
> -
> - irq = acpi_register_gsi(NULL, gsi, ACPI_LEVEL_SENSITIVE,
> - ACPI_ACTIVE_HIGH);
> - if (irq < 0) {
> - pr_warn("ACPI: SPE Unable to register interrupt: %d\n", gsi);
> - return;
> - }
> -
> - spe_resources[0].start = irq;
> - ret = platform_device_register(&spe_dev);
> - if (ret < 0) {
> - pr_warn("ACPI: SPE: Unable to register device\n");
> - acpi_unregister_gsi(gsi);
> - }
> + if (err)
> + pr_warn("ACPI: Failed to register SPE device\n");
> }
> #else
> static inline void arm_spe_acpi_register_device(void)
>
More information about the linux-arm-kernel
mailing list