Re: [PATCH 1/3] arm_pmu: acpi: Add a representative platform device for TRBE

[Anshuman Khandual]

On 7/28/23 20:10, Will Deacon wrote:
> On Fri, Jul 28, 2023 at 04:57:31PM +0530, Anshuman Khandual wrote:
>> ACPI TRBE does not have a HID for identification which could create and add
>> a platform device into the platform bus. Also without a platform device, it
>> cannot be probed and bound to a platform driver.
>>
>> This creates a dummy platform device for TRBE after ascertaining that ACPI
>> provides required interrupts uniformly across all cpus on the system. This
>> device gets created inside drivers/perf/arm_pmu_acpi.c to accommodate TRBE
>> being built as a module.
>>
>> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>> Cc: Will Deacon <will@xxxxxxxxxx>
>> Cc: Mark Rutland <mark.rutland@xxxxxxx>
>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
> 
> --->8
> 
>> 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
>> @@ -139,6 +139,68 @@ static inline void arm_spe_acpi_register_device(void)
>>  }
>>  #endif /* CONFIG_ARM_SPE_PMU */
>>  
>> +#ifdef CONFIG_CORESIGHT_TRBE
>> +static struct resource trbe_acpi_resources[] = {
>> +	{
>> +		/* irq */
>> +		.flags          = IORESOURCE_IRQ,
>> +	}
>> +};
>> +
>> +static struct platform_device trbe_acpi_dev = {
>> +	.name = ARMV8_TRBE_PDEV_NAME,
>> +	.id = -1,
>> +	.resource = trbe_acpi_resources,
>> +	.num_resources = ARRAY_SIZE(trbe_acpi_resources)
>> +};
>> +
>> +static void arm_trbe_acpi_register_device(void)
>> +{
>> +	int cpu, hetid, irq, ret;
>> +	bool first = true;
>> +	u16 gsi = 0;
>> +
>> +	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_TRBE)
>> +			return;
>> +
>> +		if (first) {
>> +			gsi = gicc->trbe_interrupt;
>> +			if (!gsi)
>> +				return;
>> +
>> +			hetid = find_acpi_cpu_topology_hetero_id(cpu);
>> +			first = false;
>> +		} else if ((gsi != gicc->trbe_interrupt) ||
>> +			   (hetid != find_acpi_cpu_topology_hetero_id(cpu))) {
>> +			pr_warn("ACPI: TRBE must be homogeneous\n");
>> +			return;
>> +		}
>> +	}
>> +
>> +	irq = acpi_register_gsi(NULL, gsi, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_HIGH);
>> +	if (irq < 0) {
>> +		pr_warn("ACPI: TRBE Unable to register interrupt: %d\n", gsi);
>> +		return;
>> +	}
>> +	trbe_acpi_resources[0].start = irq;
>> +
>> +	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.