[PATCH 0/3] coresight: trbe: Enable ACPI based devices

[PATCH 0/3] coresight: trbe: Enable ACPI based devices

[Anshuman Khandual]

This series enables detection of ACPI based TRBE devices via a stand alone
purpose built representative platform device. But as a pre-requisite this
changes coresight_platform_data structure assignment for the TRBE device.

This series is based on v6.5-rc1 kernel, is also dependent on the following
EDK2 changes posted earlier by Sami.

https://edk2.groups.io/g/devel/message/107239
https://edk2.groups.io/g/devel/message/107241

Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: James Clark <james.clark@arm.com>
Cc: coresight@lists.linaro.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org

Anshuman Khandual (3):
  arm_pmu: acpi: Add a representative platform device for TRBE
  coresight: trbe: Add a representative coresight_platform_data for TRBE
  coresight: trbe: Enable ACPI based TRBE devices

 arch/arm64/include/asm/acpi.h                |  3 +
 drivers/hwtracing/coresight/coresight-trbe.c | 15 ++++-
 drivers/hwtracing/coresight/coresight-trbe.h |  1 +
 drivers/perf/arm_pmu_acpi.c                  | 63 ++++++++++++++++++++
 include/linux/perf/arm_pmu.h                 |  1 +
 5 files changed, 80 insertions(+), 3 deletions(-)

-- 
2.25.1

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

[Anshuman Khandual]

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@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/include/asm/acpi.h |  3 ++
 drivers/perf/arm_pmu_acpi.c   | 63 +++++++++++++++++++++++++++++++++++
 include/linux/perf/arm_pmu.h  |  1 +
 3 files changed, 67 insertions(+)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index bd68e1b7f29f..4d537d56eb84 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -42,6 +42,9 @@
 #define ACPI_MADT_GICC_SPE  (offsetof(struct acpi_madt_generic_interrupt, \
 	spe_interrupt) + sizeof(u16))
 
+#define ACPI_MADT_GICC_TRBE  (offsetof(struct acpi_madt_generic_interrupt, \
+	trbe_interrupt) + sizeof(u16))
+
 /* Basic configuration for ACPI */
 #ifdef	CONFIG_ACPI
 pgprot_t __acpi_get_mem_attribute(phys_addr_t addr);
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 */
+
 static int arm_pmu_acpi_parse_irqs(void)
 {
 	int irq, cpu, irq_cpu, err;
@@ -374,6 +436,7 @@ static int arm_pmu_acpi_init(void)
 		return 0;
 
 	arm_spe_acpi_register_device();
+	arm_trbe_acpi_register_device();
 
 	return 0;
 }
diff --git a/include/linux/perf/arm_pmu.h b/include/linux/perf/arm_pmu.h
index a0801f68762b..7ec26d21303d 100644
--- a/include/linux/perf/arm_pmu.h
+++ b/include/linux/perf/arm_pmu.h
@@ -187,5 +187,6 @@ void armpmu_free_irq(int irq, int cpu);
 #endif /* CONFIG_ARM_PMU */
 
 #define ARMV8_SPE_PDEV_NAME "arm,spe-v1"
+#define ARMV8_TRBE_PDEV_NAME "arm-trbe-acpi"
 
 #endif /* __ARM_PMU_H__ */
-- 
2.25.1

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

[Will Deacon]

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@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>

--->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 :)

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.

Cheers,

Will

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@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> 
> --->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.

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

[Will Deacon]

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 *))
+{
+       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)

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

[Anshuman Khandual]

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)
> 

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

[Will Deacon]

On Tue, Aug 01, 2023 at 09:05:54AM +0530, Anshuman Khandual wrote:
> 
> 
> 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 ?

__maybe_unused?

Like I said, I didn't test that thing at all, I was just trying to
illustrate the sort of refactoring I had in mind.

Will

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

[Anshuman Khandual]

On 8/1/23 13:08, Will Deacon wrote:
> On Tue, Aug 01, 2023 at 09:05:54AM +0530, Anshuman Khandual wrote:
>>
>>
>> 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 ?
> 
> __maybe_unused?
> 
> Like I said, I didn't test that thing at all, I was just trying to
> illustrate the sort of refactoring I had in mind.

Sure. If it's okay, will use your Co-developed-by/Signed-off-by tags
for this refactoring patch.

[PATCH 2/3] coresight: trbe: Add a representative coresight_platform_data for TRBE

[Anshuman Khandual]

TRBE coresight devices do not need regular connections information, as the
paths get built between all percpu source and their respective percpu sink
devices. Please refer 'commit 2cd87a7b293d ("coresight: core: Add support
for dedicated percpu sinks")' which added support for percpu sink devices.

coresight_register() expect device connections via the platform_data. TRBE
devices do not have any graph connections and thus is empty. With upcoming
ACPI support for TRBE, we do not get a real acpi_device and thus
coresight_get_platform_dat() will end up in failures. Hence this allocates
a zeroed coresight_platform_data structure and assigns that back into the
device.

Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: coresight@lists.linaro.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 drivers/hwtracing/coresight/coresight-trbe.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index 7720619909d6..e1d9d06e7725 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -1494,9 +1494,9 @@ static int arm_trbe_device_probe(struct platform_device *pdev)
 	if (!drvdata)
 		return -ENOMEM;
 
-	pdata = coresight_get_platform_data(dev);
-	if (IS_ERR(pdata))
-		return PTR_ERR(pdata);
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
 
 	dev_set_drvdata(dev, drvdata);
 	dev->platform_data = pdata;
-- 
2.25.1

[PATCH 3/3] coresight: trbe: Enable ACPI based TRBE devices

[Anshuman Khandual]

This detects and enables ACPI based TRBE devices via the dummy platform
device created earlier for this purpose.

Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mike Leach <mike.leach@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: coresight@lists.linaro.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 drivers/hwtracing/coresight/coresight-trbe.c | 9 +++++++++
 drivers/hwtracing/coresight/coresight-trbe.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index e1d9d06e7725..f884883e9018 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -1537,7 +1537,16 @@ static const struct of_device_id arm_trbe_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, arm_trbe_of_match);
 
+#ifdef CONFIG_ACPI
+static const struct platform_device_id arm_trbe_acpi_match[] = {
+	{ ARMV8_TRBE_PDEV_NAME, 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, arm_trbe_acpi_match);
+#endif
+
 static struct platform_driver arm_trbe_driver = {
+	.id_table = arm_trbe_acpi_match,
 	.driver	= {
 		.name = DRVNAME,
 		.of_match_table = of_match_ptr(arm_trbe_of_match),
diff --git a/drivers/hwtracing/coresight/coresight-trbe.h b/drivers/hwtracing/coresight/coresight-trbe.h
index 77cbb5c63878..94e67009848a 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.h
+++ b/drivers/hwtracing/coresight/coresight-trbe.h
@@ -12,6 +12,7 @@
 #include <linux/irq.h>
 #include <linux/kernel.h>
 #include <linux/of.h>
+#include <linux/perf/arm_pmu.h>
 #include <linux/platform_device.h>
 #include <linux/smp.h>
 
-- 
2.25.1

Re: [PATCH 3/3] coresight: trbe: Enable ACPI based TRBE devices

[kernel test robot]

Hi Anshuman,

kernel test robot noticed the following build errors:

[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on arm/for-next soc/for-next linus/master arm/fixes v6.5-rc4 next-20230804]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Anshuman-Khandual/arm_pmu-acpi-Add-a-representative-platform-device-for-TRBE/20230728-192939
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
patch link:    https://lore.kernel.org/r/20230728112733.359620-4-anshuman.khandual%40arm.com
patch subject: [PATCH 3/3] coresight: trbe: Enable ACPI based TRBE devices
config: arm64-randconfig-r021-20230731 (https://download.01.org/0day-ci/archive/20230805/202308052123.uqR35d19-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce: (https://download.01.org/0day-ci/archive/20230805/202308052123.uqR35d19-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308052123.uqR35d19-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/hwtracing/coresight/coresight-trbe.c:1549:14: error: use of undeclared identifier 'arm_trbe_acpi_match'; did you mean 'arm_trbe_of_match'?
           .id_table = arm_trbe_acpi_match,
                       ^~~~~~~~~~~~~~~~~~~
                       arm_trbe_of_match
   drivers/hwtracing/coresight/coresight-trbe.c:1534:34: note: 'arm_trbe_of_match' declared here
   static const struct of_device_id arm_trbe_of_match[] = {
                                    ^
>> drivers/hwtracing/coresight/coresight-trbe.c:1549:14: error: incompatible pointer types initializing 'const struct platform_device_id *' with an expression of type 'const struct of_device_id[2]' [-Werror,-Wincompatible-pointer-types]
           .id_table = arm_trbe_acpi_match,
                       ^~~~~~~~~~~~~~~~~~~
   2 errors generated.


vim +1549 drivers/hwtracing/coresight/coresight-trbe.c

  1547	
  1548	static struct platform_driver arm_trbe_driver = {
> 1549		.id_table = arm_trbe_acpi_match,
  1550		.driver	= {
  1551			.name = DRVNAME,
  1552			.of_match_table = of_match_ptr(arm_trbe_of_match),
  1553			.suppress_bind_attrs = true,
  1554		},
  1555		.probe	= arm_trbe_device_probe,
  1556		.remove	= arm_trbe_device_remove,
  1557	};
  1558	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki