[PATCH 0/4] arm64: SPE ACPI enablement

[PATCH 0/4] arm64: SPE ACPI enablement

[Jeremy Linton]

This patch series enables the Arm Statistical Profiling
Extension (SPE) on ACPI platforms.

This is possible because ACPI 6.3 uses a previously
reserved field in the MADT to store the SPE interrupt
number, similarly to how the normal PMU is described.
If a consistent valid interrupt exists across all the
cores in the system, a platform device is registered.
That then triggers the SPE module, which runs as normal.

This version also adds the ability to parse the PPTT for
IDENTICAL cores. We then use this to sanity check the
single SPE device we create. This creates a bit of a
problem with respect to the specification though. The
specification says that its legal for multiple tree's
to exist in the PPTT. We handle this fine, but what
happens in the case of multiple tree's is that the lack
of a common node with IDENTICAL set forces us to assume
that there are multiple non IDENTICAL cores in the
machine.

Jeremy Linton (4):
  ACPI/PPTT: Add function to return ACPI 6.3 Identical tokens
  ACPI/PPTT: Modify node flag detection to find last IDENTICAL
  arm_pmu: acpi: spe: Add initial MADT/SPE probing
  perf: arm_spe: Enable ACPI/Platform automatic module loading

 arch/arm64/include/asm/acpi.h |  3 ++
 drivers/acpi/pptt.c           | 82 ++++++++++++++++++++++++++++++-----
 drivers/perf/arm_pmu_acpi.c   | 69 +++++++++++++++++++++++++++++
 drivers/perf/arm_spe_pmu.c    | 11 ++++-
 include/linux/acpi.h          |  5 +++
 5 files changed, 157 insertions(+), 13 deletions(-)

-- 
2.20.1

[PATCH 1/4] ACPI/PPTT: Add function to return ACPI 6.3 Identical tokens

[Jeremy Linton]

ACPI 6.3 adds a flag to indicate that child nodes are all
identical cores. This is useful to authoritatively determine
if a set of (possibly offline) cores are identical or not.

Since the flag doesn't give us a unique id we can generate
one and use it to create bitmaps of sibling nodes, or simply
in a loop to determine if a subset of cores are identical.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 drivers/acpi/pptt.c  | 26 ++++++++++++++++++++++++++
 include/linux/acpi.h |  5 +++++
 2 files changed, 31 insertions(+)

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
index 065c4fc245d1..472c95ec816b 100644
--- a/drivers/acpi/pptt.c
+++ b/drivers/acpi/pptt.c
@@ -660,3 +660,29 @@ int find_acpi_cpu_topology_package(unsigned int cpu)
 	return find_acpi_cpu_topology_tag(cpu, PPTT_ABORT_PACKAGE,
 					  ACPI_PPTT_PHYSICAL_PACKAGE);
 }
+
+/**
+ * find_acpi_cpu_topology_hetero_id() - Determine a unique implementation
+ * @cpu: Kernel logical cpu number
+ *
+ * Determine a unique heterogeneous ID for the given CPU. CPUs with the same
+ * implementation should have matching IDs. Since this is a tree we can only
+ * detect implementations where the heterogeneous flag is the parent to all
+ * matching cores. AKA if a two socket machine has two different core types
+ * in each socket this will end up being represented as four unique core types
+ * rather than two.
+ *
+ * The returned ID can be used to group peers with identical implementation.
+ *
+ * The search terminates when a level is found with the identical implementation
+ * flag set or we reach a root node.
+ *
+ * Return: -ENOENT if the PPTT doesn't exist, or the cpu cannot be found.
+ * Otherwise returns a value which represents a group of identical cores
+ * similar to this cpu.
+ */
+int find_acpi_cpu_topology_hetero_id(unsigned int cpu)
+{
+	return find_acpi_cpu_topology_tag(cpu, PPTT_ABORT_PACKAGE,
+					  ACPI_PPTT_ACPI_IDENTICAL);
+}
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index d5dcebd7aad3..1444fb042898 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1309,6 +1309,7 @@ static inline int lpit_read_residency_count_address(u64 *address)
 #ifdef CONFIG_ACPI_PPTT
 int find_acpi_cpu_topology(unsigned int cpu, int level);
 int find_acpi_cpu_topology_package(unsigned int cpu);
+int find_acpi_cpu_topology_hetero_id(unsigned int cpu);
 int find_acpi_cpu_cache_topology(unsigned int cpu, int level);
 #else
 static inline int find_acpi_cpu_topology(unsigned int cpu, int level)
@@ -1319,6 +1320,10 @@ static inline int find_acpi_cpu_topology_package(unsigned int cpu)
 {
 	return -EINVAL;
 }
+static int find_acpi_cpu_topology_hetero_id(unsigned int cpu)
+{
+	return -EINVAL;
+}
 static inline int find_acpi_cpu_cache_topology(unsigned int cpu, int level)
 {
 	return -EINVAL;
-- 
2.20.1

Re: [PATCH 1/4] ACPI/PPTT: Add function to return ACPI 6.3 Identical tokens

[Rafael J. Wysocki]

On Tue, Mar 26, 2019 at 11:40 PM Jeremy Linton <jeremy.linton@arm.com> wrote:
>
> ACPI 6.3 adds a flag to indicate that child nodes are all
> identical cores. This is useful to authoritatively determine
> if a set of (possibly offline) cores are identical or not.
>
> Since the flag doesn't give us a unique id we can generate
> one and use it to create bitmaps of sibling nodes, or simply
> in a loop to determine if a subset of cores are identical.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  drivers/acpi/pptt.c  | 26 ++++++++++++++++++++++++++
>  include/linux/acpi.h |  5 +++++
>  2 files changed, 31 insertions(+)
>
> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
> index 065c4fc245d1..472c95ec816b 100644
> --- a/drivers/acpi/pptt.c
> +++ b/drivers/acpi/pptt.c
> @@ -660,3 +660,29 @@ int find_acpi_cpu_topology_package(unsigned int cpu)
>         return find_acpi_cpu_topology_tag(cpu, PPTT_ABORT_PACKAGE,
>                                           ACPI_PPTT_PHYSICAL_PACKAGE);
>  }
> +
> +/**
> + * find_acpi_cpu_topology_hetero_id() - Determine a unique implementation

Maybe "Get a core architecture tag"?

> + * @cpu: Kernel logical cpu number

s/logical cpu/logical CPU/ please.

> + *
> + * Determine a unique heterogeneous ID for the given CPU. CPUs with the same
> + * implementation should have matching IDs. Since this is a tree we can only
> + * detect implementations where the heterogeneous flag is the parent to all
> + * matching cores. AKA if a two socket machine has two different core types
> + * in each socket this will end up being represented as four unique core types
> + * rather than two.

I find it quite difficult to parse that comment, honestly.

AFAICS, the function returns a tag that will be the same for all cores
with the same architecture in one package.  That is, if the package is
heterogeneous and there are two types of cores in it, there will be
two different tags.  Is this correct?

> + *
> + * The returned ID can be used to group peers with identical implementation.
> + *
> + * The search terminates when a level is found with the identical implementation
> + * flag set or we reach a root node.
> + *
> + * Return: -ENOENT if the PPTT doesn't exist, or the cpu cannot be found.
> + * Otherwise returns a value which represents a group of identical cores
> + * similar to this cpu.
> + */
> +int find_acpi_cpu_topology_hetero_id(unsigned int cpu)
> +{
> +       return find_acpi_cpu_topology_tag(cpu, PPTT_ABORT_PACKAGE,
> +                                         ACPI_PPTT_ACPI_IDENTICAL);
> +}
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index d5dcebd7aad3..1444fb042898 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1309,6 +1309,7 @@ static inline int lpit_read_residency_count_address(u64 *address)
>  #ifdef CONFIG_ACPI_PPTT
>  int find_acpi_cpu_topology(unsigned int cpu, int level);
>  int find_acpi_cpu_topology_package(unsigned int cpu);
> +int find_acpi_cpu_topology_hetero_id(unsigned int cpu);
>  int find_acpi_cpu_cache_topology(unsigned int cpu, int level);
>  #else
>  static inline int find_acpi_cpu_topology(unsigned int cpu, int level)
> @@ -1319,6 +1320,10 @@ static inline int find_acpi_cpu_topology_package(unsigned int cpu)
>  {
>         return -EINVAL;
>  }
> +static int find_acpi_cpu_topology_hetero_id(unsigned int cpu)
> +{
> +       return -EINVAL;
> +}
>  static inline int find_acpi_cpu_cache_topology(unsigned int cpu, int level)
>  {
>         return -EINVAL;
> --
> 2.20.1
>

Re: [PATCH 1/4] ACPI/PPTT: Add function to return ACPI 6.3 Identical tokens

[Jeremy Linton]

Hi,

First, thanks for taking a look at this.

On 3/28/19 5:04 AM, Rafael J. Wysocki wrote:
> On Tue, Mar 26, 2019 at 11:40 PM Jeremy Linton <jeremy.linton@arm.com> wrote:
>>
>> ACPI 6.3 adds a flag to indicate that child nodes are all
>> identical cores. This is useful to authoritatively determine
>> if a set of (possibly offline) cores are identical or not.
>>
>> Since the flag doesn't give us a unique id we can generate
>> one and use it to create bitmaps of sibling nodes, or simply
>> in a loop to determine if a subset of cores are identical.
>>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> ---
>>   drivers/acpi/pptt.c  | 26 ++++++++++++++++++++++++++
>>   include/linux/acpi.h |  5 +++++
>>   2 files changed, 31 insertions(+)
>>
>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>> index 065c4fc245d1..472c95ec816b 100644
>> --- a/drivers/acpi/pptt.c
>> +++ b/drivers/acpi/pptt.c
>> @@ -660,3 +660,29 @@ int find_acpi_cpu_topology_package(unsigned int cpu)
>>          return find_acpi_cpu_topology_tag(cpu, PPTT_ABORT_PACKAGE,
>>                                            ACPI_PPTT_PHYSICAL_PACKAGE);
>>   }
>> +
>> +/**
>> + * find_acpi_cpu_topology_hetero_id() - Determine a unique implementation
> 
> Maybe "Get a core architecture tag"?

Sure.

> 
>> + * @cpu: Kernel logical cpu number
> 
> s/logical cpu/logical CPU/ please.

Sure.

> 
>> + *
>> + * Determine a unique heterogeneous ID for the given CPU. CPUs with the same
>> + * implementation should have matching IDs. Since this is a tree we can only
>> + * detect implementations where the heterogeneous flag is the parent to all
>> + * matching cores. AKA if a two socket machine has two different core types
>> + * in each socket this will end up being represented as four unique core types
>> + * rather than two.
> 
> I find it quite difficult to parse that comment, honestly.
> 
> AFAICS, the function returns a tag that will be the same for all cores
> with the same architecture in one package.  That is, if the package is
> heterogeneous and there are two types of cores in it, there will be
> two different tags.  Is this correct?

Yes, two ID/tags per package, but since there are two packages (in this 
example) its four different tags total. This is forced by the need to 
have a node with the PACKAGE flag set splitting up IDENTICAL cores 
within the tree. A simpler topology would hopefully be able to group all 
the identical cores in the machine together. But in the above case using 
only the PPTT tree, it may not be possible to authoritatively tell how 
many different core types are in the machine if there are more than two 
tag groupings. By itself those four tags in the above example may be 
four different core types, or only two. That is likely not a huge 
problem as a processor container, or MIDR, can be used to merge 
different tag groups together depending on the callers needs.

> 
>> + *
>> + * The returned ID can be used to group peers with identical implementation.
>> + *
>> + * The search terminates when a level is found with the identical implementation
>> + * flag set or we reach a root node.
>> + *
>> + * Return: -ENOENT if the PPTT doesn't exist, or the cpu cannot be found.
>> + * Otherwise returns a value which represents a group of identical cores
>> + * similar to this cpu.
>> + */
>> +int find_acpi_cpu_topology_hetero_id(unsigned int cpu)
>> +{
>> +       return find_acpi_cpu_topology_tag(cpu, PPTT_ABORT_PACKAGE,
>> +                                         ACPI_PPTT_ACPI_IDENTICAL);
>> +}
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index d5dcebd7aad3..1444fb042898 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -1309,6 +1309,7 @@ static inline int lpit_read_residency_count_address(u64 *address)
>>   #ifdef CONFIG_ACPI_PPTT
>>   int find_acpi_cpu_topology(unsigned int cpu, int level);
>>   int find_acpi_cpu_topology_package(unsigned int cpu);
>> +int find_acpi_cpu_topology_hetero_id(unsigned int cpu);
>>   int find_acpi_cpu_cache_topology(unsigned int cpu, int level);
>>   #else
>>   static inline int find_acpi_cpu_topology(unsigned int cpu, int level)
>> @@ -1319,6 +1320,10 @@ static inline int find_acpi_cpu_topology_package(unsigned int cpu)
>>   {
>>          return -EINVAL;
>>   }
>> +static int find_acpi_cpu_topology_hetero_id(unsigned int cpu)
>> +{
>> +       return -EINVAL;
>> +}
>>   static inline int find_acpi_cpu_cache_topology(unsigned int cpu, int level)
>>   {
>>          return -EINVAL;
>> --
>> 2.20.1
>>

[PATCH 2/4] ACPI/PPTT: Modify node flag detection to find last IDENTICAL

[Jeremy Linton]

The ACPI specification implies that the IDENTICAL flag should be
set on all non leaf nodes where the children are identical.
This means that we need to be searching for the last node with
the identical flag set rather than the first one.

To achieve this with the existing code we need to pass a
function through the tree traversal logic so we can check
the next node to assure that IDENTICAL isn't set before returning
a node with IDENTICAL set.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 drivers/acpi/pptt.c | 62 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 48 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
index 472c95ec816b..db18510346f9 100644
--- a/drivers/acpi/pptt.c
+++ b/drivers/acpi/pptt.c
@@ -432,17 +432,51 @@ static void cache_setup_acpi_cpu(struct acpi_table_header *table,
 	}
 }
 
+
+typedef bool (*node_check)(struct acpi_table_header *table_hdr,
+			   struct acpi_pptt_processor *cpu);
+static bool flag_package(struct acpi_table_header *table_hdr,
+			 struct acpi_pptt_processor *cpu)
+{
+	return cpu->flags & ACPI_PPTT_PHYSICAL_PACKAGE;
+}
+
+static bool flag_identical(struct acpi_table_header *table_hdr,
+			   struct acpi_pptt_processor *cpu)
+{
+	struct acpi_pptt_processor *next;
+
+	/* heterogeneous machines must use PPTT revision > 1 */
+	if (table_hdr->revision < 2)
+		return false;
+
+	/* Locate the last node in the tree with IDENTICAL set */
+	if (cpu->flags & ACPI_PPTT_ACPI_IDENTICAL) {
+		next = fetch_pptt_node(table_hdr, cpu->parent);
+		if (!(next && next->flags & ACPI_PPTT_ACPI_IDENTICAL))
+			return true;
+	}
+
+	return false;
+}
+
+static bool flag_none(struct acpi_table_header *table_hdr,
+		      struct acpi_pptt_processor *cpu)
+{
+	return false;
+}
+
 /* Passing level values greater than this will result in search termination */
 #define PPTT_ABORT_PACKAGE 0xFF
 
-static struct acpi_pptt_processor *acpi_find_processor_package_id(struct acpi_table_header *table_hdr,
-								  struct acpi_pptt_processor *cpu,
-								  int level, int flag)
+static struct acpi_pptt_processor *acpi_find_processor_tag_id(struct acpi_table_header *table_hdr,
+							      struct acpi_pptt_processor *cpu,
+							      int level, node_check chk)
 {
 	struct acpi_pptt_processor *prev_node;
 
 	while (cpu && level) {
-		if (cpu->flags & flag)
+		if (chk(table_hdr, cpu))
 			break;
 		pr_debug("level %d\n", level);
 		prev_node = fetch_pptt_node(table_hdr, cpu->parent);
@@ -473,15 +507,15 @@ static void acpi_pptt_warn_missing(void)
  * Return: Unique value, or -ENOENT if unable to locate cpu
  */
 static int topology_get_acpi_cpu_tag(struct acpi_table_header *table,
-				     unsigned int cpu, int level, int flag)
+				     unsigned int cpu, int level, node_check chk)
 {
 	struct acpi_pptt_processor *cpu_node;
 	u32 acpi_cpu_id = get_acpi_id_for_cpu(cpu);
 
 	cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
 	if (cpu_node) {
-		cpu_node = acpi_find_processor_package_id(table, cpu_node,
-							  level, flag);
+		cpu_node = acpi_find_processor_tag_id(table, cpu_node,
+							  level, chk);
 		/*
 		 * As per specification if the processor structure represents
 		 * an actual processor, then ACPI processor ID must be valid.
@@ -498,7 +532,7 @@ static int topology_get_acpi_cpu_tag(struct acpi_table_header *table,
 	return -ENOENT;
 }
 
-static int find_acpi_cpu_topology_tag(unsigned int cpu, int level, int flag)
+static int find_acpi_cpu_topology_tag(unsigned int cpu, int level, node_check chk)
 {
 	struct acpi_table_header *table;
 	acpi_status status;
@@ -509,7 +543,7 @@ static int find_acpi_cpu_topology_tag(unsigned int cpu, int level, int flag)
 		acpi_pptt_warn_missing();
 		return -ENOENT;
 	}
-	retval = topology_get_acpi_cpu_tag(table, cpu, level, flag);
+	retval = topology_get_acpi_cpu_tag(table, cpu, level, chk);
 	pr_debug("Topology Setup ACPI cpu %d, level %d ret = %d\n",
 		 cpu, level, retval);
 	acpi_put_table(table);
@@ -601,7 +635,7 @@ int cache_setup_acpi(unsigned int cpu)
  */
 int find_acpi_cpu_topology(unsigned int cpu, int level)
 {
-	return find_acpi_cpu_topology_tag(cpu, level, 0);
+	return find_acpi_cpu_topology_tag(cpu, level, flag_none);
 }
 
 /**
@@ -658,7 +692,7 @@ int find_acpi_cpu_cache_topology(unsigned int cpu, int level)
 int find_acpi_cpu_topology_package(unsigned int cpu)
 {
 	return find_acpi_cpu_topology_tag(cpu, PPTT_ABORT_PACKAGE,
-					  ACPI_PPTT_PHYSICAL_PACKAGE);
+					  flag_package);
 }
 
 /**
@@ -674,8 +708,8 @@ int find_acpi_cpu_topology_package(unsigned int cpu)
  *
  * The returned ID can be used to group peers with identical implementation.
  *
- * The search terminates when a level is found with the identical implementation
- * flag set or we reach a root node.
+ * The search terminates when a level is found without the identical
+ * implementation flag set following a node with it set, or we reach the root.
  *
  * Return: -ENOENT if the PPTT doesn't exist, or the cpu cannot be found.
  * Otherwise returns a value which represents a group of identical cores
@@ -684,5 +718,5 @@ int find_acpi_cpu_topology_package(unsigned int cpu)
 int find_acpi_cpu_topology_hetero_id(unsigned int cpu)
 {
 	return find_acpi_cpu_topology_tag(cpu, PPTT_ABORT_PACKAGE,
-					  ACPI_PPTT_ACPI_IDENTICAL);
+					  flag_identical);
 }
-- 
2.20.1

[PATCH 3/4] arm_pmu: acpi: spe: Add initial MADT/SPE probing

[Jeremy Linton]

ACPI 6.3 adds additional fields to the MADT GICC
structure to describe SPE PPI's. We pick these out
of the cached reference to the madt_gicc structure
similarly to the core PMU code. We then create a platform
device referring to the IRQ and let the user/module loader
decide whether to load the SPE driver.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/include/asm/acpi.h |  3 ++
 drivers/perf/arm_pmu_acpi.c   | 69 +++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 7628efbe6c12..d10399b9f998 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -41,6 +41,9 @@
 	(!(entry) || (entry)->header.length < ACPI_MADT_GICC_MIN_LENGTH || \
 	(unsigned long)(entry) + (entry)->header.length > (end))
 
+#define ACPI_MADT_GICC_SPE  (ACPI_OFFSET(struct acpi_madt_generic_interrupt, \
+	spe_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 0f197516d708..a2418108eab2 100644
--- a/drivers/perf/arm_pmu_acpi.c
+++ b/drivers/perf/arm_pmu_acpi.c
@@ -74,6 +74,73 @@ static void arm_pmu_acpi_unregister_irq(int cpu)
 	acpi_unregister_gsi(gsi);
 }
 
+static struct resource spe_resources[] = {
+	{
+		/* irq */
+		.flags          = IORESOURCE_IRQ,
+	}
+};
+
+static struct platform_device spe_dev = {
+	.name = "arm,spe-v1",
+	.id = -1,
+	.resource = spe_resources,
+	.num_resources = ARRAY_SIZE(spe_resources)
+};
+
+/*
+ * For lack of a better place, hook the normal PMU MADT walk
+ * and create a SPE device if we detect a recent MADT with
+ * a homogeneous PPI mapping.
+ */
+static int arm_spe_acpi_parse_irqs(void)
+{
+	int cpu, ret, irq;
+	int hetid;
+	u16 gsi = 0;
+	bool first = true;
+
+	struct acpi_madt_generic_interrupt *gicc;
+
+	/*
+	 * 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) {
+		gicc = acpi_cpu_get_madt_gicc(cpu);
+
+		if (gicc->header.length < ACPI_MADT_GICC_SPE)
+			return -ENODEV;
+		if (first) {
+			gsi = gicc->spe_interrupt;
+			if (!gsi)
+				return -ENODEV;
+			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 -EINVAL;
+		}
+	}
+
+	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 irq;
+	}
+
+	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);
+	}
+
+	return ret;
+}
+
 static int arm_pmu_acpi_parse_irqs(void)
 {
 	int irq, cpu, irq_cpu, err;
@@ -279,6 +346,8 @@ static int arm_pmu_acpi_init(void)
 	if (acpi_disabled)
 		return 0;
 
+	arm_spe_acpi_parse_irqs(); /* failures are expected */
+
 	ret = arm_pmu_acpi_parse_irqs();
 	if (ret)
 		return ret;
-- 
2.20.1

Re: [PATCH 3/4] arm_pmu: acpi: spe: Add initial MADT/SPE probing

[John Garry]

On 26/03/2019 22:39, Jeremy Linton wrote:
> ACPI 6.3 adds additional fields to the MADT GICC
> structure to describe SPE PPI's. We pick these out
> of the cached reference to the madt_gicc structure
> similarly to the core PMU code. We then create a platform
> device referring to the IRQ and let the user/module loader
> decide whether to load the SPE driver.
>
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  arch/arm64/include/asm/acpi.h |  3 ++
>  drivers/perf/arm_pmu_acpi.c   | 69 +++++++++++++++++++++++++++++++++++
>  2 files changed, 72 insertions(+)
>
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index 7628efbe6c12..d10399b9f998 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -41,6 +41,9 @@
>  	(!(entry) || (entry)->header.length < ACPI_MADT_GICC_MIN_LENGTH || \
>  	(unsigned long)(entry) + (entry)->header.length > (end))
>
> +#define ACPI_MADT_GICC_SPE  (ACPI_OFFSET(struct acpi_madt_generic_interrupt, \
> +	spe_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 0f197516d708..a2418108eab2 100644
> --- a/drivers/perf/arm_pmu_acpi.c
> +++ b/drivers/perf/arm_pmu_acpi.c
> @@ -74,6 +74,73 @@ static void arm_pmu_acpi_unregister_irq(int cpu)
>  	acpi_unregister_gsi(gsi);
>  }
>
> +static struct resource spe_resources[] = {
> +	{
> +		/* irq */
> +		.flags          = IORESOURCE_IRQ,
> +	}
> +};
> +
> +static struct platform_device spe_dev = {
> +	.name = "arm,spe-v1",
> +	.id = -1,
> +	.resource = spe_resources,
> +	.num_resources = ARRAY_SIZE(spe_resources)
> +};
> +
> +/*
> + * For lack of a better place,

It seems that the kernel Image size can now increase due to this part of 
SPE support even if ARM_SPE_PMU config is disabled.

And I don't even think that ARM_SPE_PMU depends on ARM_PMU (which 
ARM_PMU_ACPI depends on).

Thanks,
John

hook the normal PMU MADT walk
> + * and create a SPE device if we detect a recent MADT with
> + * a homogeneous PPI mapping.
> + */
> +static int arm_spe_acpi_parse_irqs(void)
> +{
> +	int cpu, ret, irq;
> +	int hetid;
> +	u16 gsi = 0;
> +	bool first = true;
> +
> +	struct acpi_madt_generic_interrupt *gicc;
> +
> +	/*
> +	 * 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) {
> +		gicc = acpi_cpu_get_madt_gicc(cpu);
> +
> +		if (gicc->header.length < ACPI_MADT_GICC_SPE)
> +			return -ENODEV;
> +		if (first) {
> +			gsi = gicc->spe_interrupt;
> +			if (!gsi)
> +				return -ENODEV;
> +			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 -EINVAL;
> +		}
> +	}
> +
> +	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 irq;
> +	}
> +
> +	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);
> +	}
> +
> +	return ret;
> +}
> +
>  static int arm_pmu_acpi_parse_irqs(void)
>  {
>  	int irq, cpu, irq_cpu, err;
> @@ -279,6 +346,8 @@ static int arm_pmu_acpi_init(void)
>  	if (acpi_disabled)
>  		return 0;
>
> +	arm_spe_acpi_parse_irqs(); /* failures are expected */
> +
>  	ret = arm_pmu_acpi_parse_irqs();
>  	if (ret)
>  		return ret;
>

Re: [PATCH 3/4] arm_pmu: acpi: spe: Add initial MADT/SPE probing

[Jeremy Linton]

Hi,

First thanks for taking a look at this, second sorry about the delay...

On 3/28/19 7:40 AM, John Garry wrote:
> On 26/03/2019 22:39, Jeremy Linton wrote:
>> ACPI 6.3 adds additional fields to the MADT GICC
>> structure to describe SPE PPI's. We pick these out
>> of the cached reference to the madt_gicc structure
>> similarly to the core PMU code. We then create a platform
>> device referring to the IRQ and let the user/module loader
>> decide whether to load the SPE driver.
>>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> ---
>>  arch/arm64/include/asm/acpi.h |  3 ++
>>  drivers/perf/arm_pmu_acpi.c   | 69 +++++++++++++++++++++++++++++++++++
>>  2 files changed, 72 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/acpi.h 
>> b/arch/arm64/include/asm/acpi.h
>> index 7628efbe6c12..d10399b9f998 100644
>> --- a/arch/arm64/include/asm/acpi.h
>> +++ b/arch/arm64/include/asm/acpi.h
>> @@ -41,6 +41,9 @@
>>      (!(entry) || (entry)->header.length < ACPI_MADT_GICC_MIN_LENGTH || \
>>      (unsigned long)(entry) + (entry)->header.length > (end))
>>
>> +#define ACPI_MADT_GICC_SPE  (ACPI_OFFSET(struct 
>> acpi_madt_generic_interrupt, \
>> +    spe_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 0f197516d708..a2418108eab2 100644
>> --- a/drivers/perf/arm_pmu_acpi.c
>> +++ b/drivers/perf/arm_pmu_acpi.c
>> @@ -74,6 +74,73 @@ static void arm_pmu_acpi_unregister_irq(int cpu)
>>      acpi_unregister_gsi(gsi);
>>  }
>>
>> +static struct resource spe_resources[] = {
>> +    {
>> +        /* irq */
>> +        .flags          = IORESOURCE_IRQ,
>> +    }
>> +};
>> +
>> +static struct platform_device spe_dev = {
>> +    .name = "arm,spe-v1",
>> +    .id = -1,
>> +    .resource = spe_resources,
>> +    .num_resources = ARRAY_SIZE(spe_resources)
>> +};
>> +
>> +/*
>> + * For lack of a better place,
> 
> It seems that the kernel Image size can now increase due to this part of 
> SPE support even if ARM_SPE_PMU config is disabled.

That is true, but it should be fairly small.

> 
> And I don't even think that ARM_SPE_PMU depends on ARM_PMU (which 
> ARM_PMU_ACPI depends on).

Well I don't think we want the generic SPE_PMU dependent on ACPI. So 
this chunk of code could be wrapped in a SPE_PMU_ACPI block, and made 
dependent on PMU_ACPI. OTOH, It seems a little trivial for that, and 
maybe just tweaking the PMU_ACPI documentation to mention that it also 
enables ACPI/SPE is a better plan.

Opinions?


> 
> Thanks,
> John
> 
> hook the normal PMU MADT walk
>> + * and create a SPE device if we detect a recent MADT with
>> + * a homogeneous PPI mapping.
>> + */
>> +static int arm_spe_acpi_parse_irqs(void)
>> +{
>> +    int cpu, ret, irq;
>> +    int hetid;
>> +    u16 gsi = 0;
>> +    bool first = true;
>> +
>> +    struct acpi_madt_generic_interrupt *gicc;
>> +
>> +    /*
>> +     * 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) {
>> +        gicc = acpi_cpu_get_madt_gicc(cpu);
>> +
>> +        if (gicc->header.length < ACPI_MADT_GICC_SPE)
>> +            return -ENODEV;
>> +        if (first) {
>> +            gsi = gicc->spe_interrupt;
>> +            if (!gsi)
>> +                return -ENODEV;
>> +            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 -EINVAL;
>> +        }
>> +    }
>> +
>> +    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 irq;
>> +    }
>> +
>> +    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);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>  static int arm_pmu_acpi_parse_irqs(void)
>>  {
>>      int irq, cpu, irq_cpu, err;
>> @@ -279,6 +346,8 @@ static int arm_pmu_acpi_init(void)
>>      if (acpi_disabled)
>>          return 0;
>>
>> +    arm_spe_acpi_parse_irqs(); /* failures are expected */
>> +
>>      ret = arm_pmu_acpi_parse_irqs();
>>      if (ret)
>>          return ret;
>>
> 
> 

Re: [PATCH 3/4] arm_pmu: acpi: spe: Add initial MADT/SPE probing

[John Garry]

On 02/04/2019 20:14, Jeremy Linton wrote:
> Hi,
>
> First thanks for taking a look at this, second sorry about the delay...
>
> On 3/28/19 7:40 AM, John Garry wrote:
>> On 26/03/2019 22:39, Jeremy Linton wrote:
>>> ACPI 6.3 adds additional fields to the MADT GICC
>>> structure to describe SPE PPI's. We pick these out
>>> of the cached reference to the madt_gicc structure
>>> similarly to the core PMU code. We then create a platform
>>> device referring to the IRQ and let the user/module loader
>>> decide whether to load the SPE driver.
>>>
>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>>> ---
>>>  arch/arm64/include/asm/acpi.h |  3 ++
>>>  drivers/perf/arm_pmu_acpi.c   | 69 +++++++++++++++++++++++++++++++++++
>>>  2 files changed, 72 insertions(+)
>>>
>>> diff --git a/arch/arm64/include/asm/acpi.h
>>> b/arch/arm64/include/asm/acpi.h
>>> index 7628efbe6c12..d10399b9f998 100644
>>> --- a/arch/arm64/include/asm/acpi.h
>>> +++ b/arch/arm64/include/asm/acpi.h
>>> @@ -41,6 +41,9 @@
>>>      (!(entry) || (entry)->header.length < ACPI_MADT_GICC_MIN_LENGTH
>>> || \
>>>      (unsigned long)(entry) + (entry)->header.length > (end))
>>>
>>> +#define ACPI_MADT_GICC_SPE  (ACPI_OFFSET(struct
>>> acpi_madt_generic_interrupt, \
>>> +    spe_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 0f197516d708..a2418108eab2 100644
>>> --- a/drivers/perf/arm_pmu_acpi.c
>>> +++ b/drivers/perf/arm_pmu_acpi.c
>>> @@ -74,6 +74,73 @@ static void arm_pmu_acpi_unregister_irq(int cpu)
>>>      acpi_unregister_gsi(gsi);
>>>  }
>>>
>>> +static struct resource spe_resources[] = {
>>> +    {
>>> +        /* irq */
>>> +        .flags          = IORESOURCE_IRQ,
>>> +    }
>>> +};
>>> +
>>> +static struct platform_device spe_dev = {
>>> +    .name = "arm,spe-v1",
>>> +    .id = -1,
>>> +    .resource = spe_resources,
>>> +    .num_resources = ARRAY_SIZE(spe_resources)
>>> +};
>>> +
>>> +/*
>>> + * For lack of a better place,
>>
>> It seems that the kernel Image size can now increase due to this part
>> of SPE support even if ARM_SPE_PMU config is disabled.
>
> That is true, but it should be fairly small.
>
>>
>> And I don't even think that ARM_SPE_PMU depends on ARM_PMU (which
>> ARM_PMU_ACPI depends on).
>
> Well I don't think we want the generic SPE_PMU dependent on ACPI.

Relevant code in the SPE driver could still be built under CONFIG_ACPI.

It seems that the real problem is that some necessary ACPI-related 
symbols used in arm_spe_acpi_parse_irqs() are not exported, while the 
SPE driver can be a loadable module.

  So
> this chunk of code could be wrapped in a SPE_PMU_ACPI block, and made
> dependent on PMU_ACPI. OTOH, It seems a little trivial for that, and
> maybe just tweaking the PMU_ACPI documentation to mention that it also
> enables ACPI/SPE is a better plan.
>
> Opinions?
>
>
>>
>>
>> hook the normal PMU MADT walk
>>> + * and create a SPE device if we detect a recent MADT with
>>> + * a homogeneous PPI mapping.
>>> + */
>>> +static int arm_spe_acpi_parse_irqs(void)

nit: it's doing a bit more than parsing IRQs

>>> +{
>>> +    int cpu, ret, irq;
>>> +    int hetid;
>>> +    u16 gsi = 0;
>>> +    bool first = true;


Thanks,
John

>>> +
>>> +    struct acpi_madt_generic_interrupt *gicc;
>>> +
>>> +    /*
>>> +     * 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) {
>>> +        gicc = acpi_cpu_get_madt_gicc(cpu);
>>> +
>>> +        if (gicc->header.length < ACPI_MADT_GICC_SPE)
>>> +            return -ENODEV;
>>> +        if (first) {
>>> +            gsi = gicc->spe_interrupt;
>>> +            if (!gsi)
>>> +                return -ENODEV;
>>> +            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 -EINVAL;
>>> +        }
>>> +    }
>>> +
>>> +    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 irq;
>>> +    }
>>> +
>>> +    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);
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>>  static int arm_pmu_acpi_parse_irqs(void)
>>>  {
>>>      int irq, cpu, irq_cpu, err;
>>> @@ -279,6 +346,8 @@ static int arm_pmu_acpi_init(void)
>>>      if (acpi_disabled)
>>>          return 0;
>>>
>>> +    arm_spe_acpi_parse_irqs(); /* failures are expected */
>>> +
>>>      ret = arm_pmu_acpi_parse_irqs();
>>>      if (ret)
>>>          return ret;
>>>
>>
>>
>
>
> .
>

Re: [PATCH 3/4] arm_pmu: acpi: spe: Add initial MADT/SPE probing

[John Garry]

On 02/04/2019 20:14, Jeremy Linton wrote:
> Hi,
>
> First thanks for taking a look at this, second sorry about the delay...
>
> On 3/28/19 7:40 AM, John Garry wrote:
>> On 26/03/2019 22:39, Jeremy Linton wrote:
>>> ACPI 6.3 adds additional fields to the MADT GICC
>>> structure to describe SPE PPI's. We pick these out
>>> of the cached reference to the madt_gicc structure
>>> similarly to the core PMU code. We then create a platform
>>> device referring to the IRQ and let the user/module loader
>>> decide whether to load the SPE driver.
>>>
>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>>> ---
>>>  arch/arm64/include/asm/acpi.h |  3 ++
>>>  drivers/perf/arm_pmu_acpi.c   | 69 +++++++++++++++++++++++++++++++++++
>>>  2 files changed, 72 insertions(+)
>>>
>>> diff --git a/arch/arm64/include/asm/acpi.h
>>> b/arch/arm64/include/asm/acpi.h
>>> index 7628efbe6c12..d10399b9f998 100644
>>> --- a/arch/arm64/include/asm/acpi.h
>>> +++ b/arch/arm64/include/asm/acpi.h
>>> @@ -41,6 +41,9 @@
>>>      (!(entry) || (entry)->header.length < ACPI_MADT_GICC_MIN_LENGTH
>>> || \
>>>      (unsigned long)(entry) + (entry)->header.length > (end))
>>>
>>> +#define ACPI_MADT_GICC_SPE  (ACPI_OFFSET(struct
>>> acpi_madt_generic_interrupt, \
>>> +    spe_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 0f197516d708..a2418108eab2 100644
>>> --- a/drivers/perf/arm_pmu_acpi.c
>>> +++ b/drivers/perf/arm_pmu_acpi.c
>>> @@ -74,6 +74,73 @@ static void arm_pmu_acpi_unregister_irq(int cpu)
>>>      acpi_unregister_gsi(gsi);
>>>  }
>>>
>>> +static struct resource spe_resources[] = {
>>> +    {
>>> +        /* irq */
>>> +        .flags          = IORESOURCE_IRQ,
>>> +    }
>>> +};
>>> +
>>> +static struct platform_device spe_dev = {
>>> +    .name = "arm,spe-v1",
>>> +    .id = -1,
>>> +    .resource = spe_resources,
>>> +    .num_resources = ARRAY_SIZE(spe_resources)
>>> +};
>>> +
>>> +/*
>>> + * For lack of a better place,
>>
>> It seems that the kernel Image size can now increase due to this part
>> of SPE support even if ARM_SPE_PMU config is disabled.
>
> That is true, but it should be fairly small.
>
>>
>> And I don't even think that ARM_SPE_PMU depends on ARM_PMU (which
>> ARM_PMU_ACPI depends on).
>
> Well I don't think we want the generic SPE_PMU dependent on ACPI.

Relevant code in the SPE driver could still be built under CONFIG_ACPI.

It seems that the real problem is that some necessary ACPI-related 
symbols used in arm_spe_acpi_parse_irqs() are not exported, while the 
SPE driver can be a loadable module.

  So
> this chunk of code could be wrapped in a SPE_PMU_ACPI block, and made
> dependent on PMU_ACPI. OTOH, It seems a little trivial for that, and
> maybe just tweaking the PMU_ACPI documentation to mention that it also
> enables ACPI/SPE is a better plan.
>
> Opinions?
>
>
>>
>>
>> hook the normal PMU MADT walk
>>> + * and create a SPE device if we detect a recent MADT with
>>> + * a homogeneous PPI mapping.
>>> + */
>>> +static int arm_spe_acpi_parse_irqs(void)

nit: it's doing a bit more than parsing IRQs

>>> +{
>>> +    int cpu, ret, irq;
>>> +    int hetid;
>>> +    u16 gsi = 0;
>>> +    bool first = true;


Thanks,
John

>>> +
>>> +    struct acpi_madt_generic_interrupt *gicc;
>>> +
>>> +    /*
>>> +     * 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) {
>>> +        gicc = acpi_cpu_get_madt_gicc(cpu);
>>> +
>>> +        if (gicc->header.length < ACPI_MADT_GICC_SPE)
>>> +            return -ENODEV;
>>> +        if (first) {
>>> +            gsi = gicc->spe_interrupt;
>>> +            if (!gsi)
>>> +                return -ENODEV;
>>> +            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 -EINVAL;
>>> +        }
>>> +    }
>>> +
>>> +    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 irq;
>>> +    }
>>> +
>>> +    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);
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>>  static int arm_pmu_acpi_parse_irqs(void)
>>>  {
>>>      int irq, cpu, irq_cpu, err;
>>> @@ -279,6 +346,8 @@ static int arm_pmu_acpi_init(void)
>>>      if (acpi_disabled)
>>>          return 0;
>>>
>>> +    arm_spe_acpi_parse_irqs(); /* failures are expected */
>>> +
>>>      ret = arm_pmu_acpi_parse_irqs();
>>>      if (ret)
>>>          return ret;
>>>
>>
>>
>
>
> .
>

[PATCH 4/4] perf: arm_spe: Enable ACPI/Platform automatic module loading

[Jeremy Linton]

Lets add the MODULE_TABLE and platform id_table entries so that
the SPE driver can attach to the ACPI platform device created by
the core pmu code.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/perf/arm_spe_pmu.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index 7cb766dafe85..ffa2c76c08bb 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c
@@ -1176,7 +1176,13 @@ static const struct of_device_id arm_spe_pmu_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, arm_spe_pmu_of_match);
 
-static int arm_spe_pmu_device_dt_probe(struct platform_device *pdev)
+static const struct platform_device_id arm_spe_match[] = {
+	{ "arm,spe-v1", 0},
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, arm_spe_match);
+
+static int arm_spe_pmu_device_probe(struct platform_device *pdev)
 {
 	int ret;
 	struct arm_spe_pmu *spe_pmu;
@@ -1236,11 +1242,12 @@ static int arm_spe_pmu_device_remove(struct platform_device *pdev)
 }
 
 static struct platform_driver arm_spe_pmu_driver = {
+	.id_table = arm_spe_match,
 	.driver	= {
 		.name		= DRVNAME,
 		.of_match_table	= of_match_ptr(arm_spe_pmu_of_match),
 	},
-	.probe	= arm_spe_pmu_device_dt_probe,
+	.probe	= arm_spe_pmu_device_probe,
 	.remove	= arm_spe_pmu_device_remove,
 };
 
-- 
2.20.1

Re: [PATCH 4/4] perf: arm_spe: Enable ACPI/Platform automatic module loading

[Will Deacon]

On Tue, Mar 26, 2019 at 05:39:38PM -0500, Jeremy Linton wrote:
> Lets add the MODULE_TABLE and platform id_table entries so that
> the SPE driver can attach to the ACPI platform device created by
> the core pmu code.
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/perf/arm_spe_pmu.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> index 7cb766dafe85..ffa2c76c08bb 100644
> --- a/drivers/perf/arm_spe_pmu.c
> +++ b/drivers/perf/arm_spe_pmu.c
> @@ -1176,7 +1176,13 @@ static const struct of_device_id arm_spe_pmu_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, arm_spe_pmu_of_match);
>  
> -static int arm_spe_pmu_device_dt_probe(struct platform_device *pdev)
> +static const struct platform_device_id arm_spe_match[] = {
> +	{ "arm,spe-v1", 0},

It would be nice if we could avoid duplicating this string from the ACPI
parsing code.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(platform, arm_spe_match);
> +
> +static int arm_spe_pmu_device_probe(struct platform_device *pdev)
>  {
>  	int ret;
>  	struct arm_spe_pmu *spe_pmu;
> @@ -1236,11 +1242,12 @@ static int arm_spe_pmu_device_remove(struct platform_device *pdev)
>  }
>  
>  static struct platform_driver arm_spe_pmu_driver = {
> +	.id_table = arm_spe_match,
>  	.driver	= {
>  		.name		= DRVNAME,
>  		.of_match_table	= of_match_ptr(arm_spe_pmu_of_match),

Hmm, so some other drivers don't hook .id_table like you do, but instead
hook .acpi_match_table in the driver structure. Is that not better?

Will

Re: [PATCH 4/4] perf: arm_spe: Enable ACPI/Platform automatic module loading

[Jeremy Linton]

Hi,

On 4/4/19 12:04 PM, Will Deacon wrote:
> On Tue, Mar 26, 2019 at 05:39:38PM -0500, Jeremy Linton wrote:
>> Lets add the MODULE_TABLE and platform id_table entries so that
>> the SPE driver can attach to the ACPI platform device created by
>> the core pmu code.
>>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>   drivers/perf/arm_spe_pmu.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
>> index 7cb766dafe85..ffa2c76c08bb 100644
>> --- a/drivers/perf/arm_spe_pmu.c
>> +++ b/drivers/perf/arm_spe_pmu.c
>> @@ -1176,7 +1176,13 @@ static const struct of_device_id arm_spe_pmu_of_match[] = {
>>   };
>>   MODULE_DEVICE_TABLE(of, arm_spe_pmu_of_match);
>>   
>> -static int arm_spe_pmu_device_dt_probe(struct platform_device *pdev)
>> +static const struct platform_device_id arm_spe_match[] = {
>> +	{ "arm,spe-v1", 0},
> 
> It would be nice if we could avoid duplicating this string from the ACPI
> parsing code.

Ok sure, I just need to find a good common place for it.

> 
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(platform, arm_spe_match);
>> +
>> +static int arm_spe_pmu_device_probe(struct platform_device *pdev)
>>   {
>>   	int ret;
>>   	struct arm_spe_pmu *spe_pmu;
>> @@ -1236,11 +1242,12 @@ static int arm_spe_pmu_device_remove(struct platform_device *pdev)
>>   }
>>   
>>   static struct platform_driver arm_spe_pmu_driver = {
>> +	.id_table = arm_spe_match,
>>   	.driver	= {
>>   		.name		= DRVNAME,
>>   		.of_match_table	= of_match_ptr(arm_spe_pmu_of_match),
> 
> Hmm, so some other drivers don't hook .id_table like you do, but instead
> hook .acpi_match_table in the driver structure. Is that not better?

This isn't actually an ACPI device, (aka not defined in the namespace), 
so its missing much of the ACPI functionality. I think that also means 
its needs to be declared this way.


> 
> Will
> 

Re: [PATCH 4/4] perf: arm_spe: Enable ACPI/Platform automatic module loading

[Will Deacon]

On Thu, Apr 04, 2019 at 12:24:38PM -0500, Jeremy Linton wrote:
> On 4/4/19 12:04 PM, Will Deacon wrote:
> > On Tue, Mar 26, 2019 at 05:39:38PM -0500, Jeremy Linton wrote:
> > > Lets add the MODULE_TABLE and platform id_table entries so that
> > > the SPE driver can attach to the ACPI platform device created by
> > > the core pmu code.
> > > 
> > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > > ---
> > >   drivers/perf/arm_spe_pmu.c | 11 +++++++++--
> > >   1 file changed, 9 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> > > index 7cb766dafe85..ffa2c76c08bb 100644
> > > --- a/drivers/perf/arm_spe_pmu.c
> > > +++ b/drivers/perf/arm_spe_pmu.c
> > > @@ -1176,7 +1176,13 @@ static const struct of_device_id arm_spe_pmu_of_match[] = {
> > >   };
> > >   MODULE_DEVICE_TABLE(of, arm_spe_pmu_of_match);
> > > -static int arm_spe_pmu_device_dt_probe(struct platform_device *pdev)
> > > +static const struct platform_device_id arm_spe_match[] = {
> > > +	{ "arm,spe-v1", 0},
> > 
> > It would be nice if we could avoid duplicating this string from the ACPI
> > parsing code.
> 
> Ok sure, I just need to find a good common place for it.
> 
> > 
> > > +	{ }
> > > +};
> > > +MODULE_DEVICE_TABLE(platform, arm_spe_match);
> > > +
> > > +static int arm_spe_pmu_device_probe(struct platform_device *pdev)
> > >   {
> > >   	int ret;
> > >   	struct arm_spe_pmu *spe_pmu;
> > > @@ -1236,11 +1242,12 @@ static int arm_spe_pmu_device_remove(struct platform_device *pdev)
> > >   }
> > >   static struct platform_driver arm_spe_pmu_driver = {
> > > +	.id_table = arm_spe_match,
> > >   	.driver	= {
> > >   		.name		= DRVNAME,
> > >   		.of_match_table	= of_match_ptr(arm_spe_pmu_of_match),
> > 
> > Hmm, so some other drivers don't hook .id_table like you do, but instead
> > hook .acpi_match_table in the driver structure. Is that not better?
> 
> This isn't actually an ACPI device, (aka not defined in the namespace), so
> its missing much of the ACPI functionality. I think that also means its
> needs to be declared this way.

Looking at platform_match(), I'd really like to avoid having both an
.id_table and an .of_match_table field.

acpi_of_match_device() will actually use the .of_match_table, but it relies
on ACPI_COMPANION returning a valid acpi_device. If we don't have one of
those, perhaps we can use the .id_table exclusively and drop the
.of_match_table instead?

Will

Re: [PATCH 4/4] perf: arm_spe: Enable ACPI/Platform automatic module loading

[Will Deacon]

On Thu, Apr 04, 2019 at 12:24:38PM -0500, Jeremy Linton wrote:
> On 4/4/19 12:04 PM, Will Deacon wrote:
> > On Tue, Mar 26, 2019 at 05:39:38PM -0500, Jeremy Linton wrote:
> > > Lets add the MODULE_TABLE and platform id_table entries so that
> > > the SPE driver can attach to the ACPI platform device created by
> > > the core pmu code.
> > > 
> > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > > ---
> > >   drivers/perf/arm_spe_pmu.c | 11 +++++++++--
> > >   1 file changed, 9 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> > > index 7cb766dafe85..ffa2c76c08bb 100644
> > > --- a/drivers/perf/arm_spe_pmu.c
> > > +++ b/drivers/perf/arm_spe_pmu.c
> > > @@ -1176,7 +1176,13 @@ static const struct of_device_id arm_spe_pmu_of_match[] = {
> > >   };
> > >   MODULE_DEVICE_TABLE(of, arm_spe_pmu_of_match);
> > > -static int arm_spe_pmu_device_dt_probe(struct platform_device *pdev)
> > > +static const struct platform_device_id arm_spe_match[] = {
> > > +	{ "arm,spe-v1", 0},
> > 
> > It would be nice if we could avoid duplicating this string from the ACPI
> > parsing code.
> 
> Ok sure, I just need to find a good common place for it.
> 
> > 
> > > +	{ }
> > > +};
> > > +MODULE_DEVICE_TABLE(platform, arm_spe_match);
> > > +
> > > +static int arm_spe_pmu_device_probe(struct platform_device *pdev)
> > >   {
> > >   	int ret;
> > >   	struct arm_spe_pmu *spe_pmu;
> > > @@ -1236,11 +1242,12 @@ static int arm_spe_pmu_device_remove(struct platform_device *pdev)
> > >   }
> > >   static struct platform_driver arm_spe_pmu_driver = {
> > > +	.id_table = arm_spe_match,
> > >   	.driver	= {
> > >   		.name		= DRVNAME,
> > >   		.of_match_table	= of_match_ptr(arm_spe_pmu_of_match),
> > 
> > Hmm, so some other drivers don't hook .id_table like you do, but instead
> > hook .acpi_match_table in the driver structure. Is that not better?
> 
> This isn't actually an ACPI device, (aka not defined in the namespace), so
> its missing much of the ACPI functionality. I think that also means its
> needs to be declared this way.

Looking at platform_match(), I'd really like to avoid having both an
.id_table and an .of_match_table field.

acpi_of_match_device() will actually use the .of_match_table, but it relies
on ACPI_COMPANION returning a valid acpi_device. If we don't have one of
those, perhaps we can use the .id_table exclusively and drop the
.of_match_table instead?

Will

Re: [PATCH 4/4] perf: arm_spe: Enable ACPI/Platform automatic module loading

[Jeremy Linton]

Hi,

On 4/16/19 8:50 AM, Will Deacon wrote:
> On Thu, Apr 04, 2019 at 12:24:38PM -0500, Jeremy Linton wrote:
>> On 4/4/19 12:04 PM, Will Deacon wrote:
>>> On Tue, Mar 26, 2019 at 05:39:38PM -0500, Jeremy Linton wrote:
>>>> Lets add the MODULE_TABLE and platform id_table entries so that
>>>> the SPE driver can attach to the ACPI platform device created by
>>>> the core pmu code.
>>>>
>>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>>>> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
>>>> ---
>>>>    drivers/perf/arm_spe_pmu.c | 11 +++++++++--
>>>>    1 file changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
>>>> index 7cb766dafe85..ffa2c76c08bb 100644
>>>> --- a/drivers/perf/arm_spe_pmu.c
>>>> +++ b/drivers/perf/arm_spe_pmu.c
>>>> @@ -1176,7 +1176,13 @@ static const struct of_device_id arm_spe_pmu_of_match[] = {
>>>>    };
>>>>    MODULE_DEVICE_TABLE(of, arm_spe_pmu_of_match);
>>>> -static int arm_spe_pmu_device_dt_probe(struct platform_device *pdev)
>>>> +static const struct platform_device_id arm_spe_match[] = {
>>>> +	{ "arm,spe-v1", 0},
>>>
>>> It would be nice if we could avoid duplicating this string from the ACPI
>>> parsing code.
>>
>> Ok sure, I just need to find a good common place for it.

There doesn't appear to be a good common place for this, so maybe 
arm_pmu.h, which can then be included in the spe driver is the right thing.


>>
>>>
>>>> +	{ }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(platform, arm_spe_match);
>>>> +
>>>> +static int arm_spe_pmu_device_probe(struct platform_device *pdev)
>>>>    {
>>>>    	int ret;
>>>>    	struct arm_spe_pmu *spe_pmu;
>>>> @@ -1236,11 +1242,12 @@ static int arm_spe_pmu_device_remove(struct platform_device *pdev)
>>>>    }
>>>>    static struct platform_driver arm_spe_pmu_driver = {
>>>> +	.id_table = arm_spe_match,
>>>>    	.driver	= {
>>>>    		.name		= DRVNAME,
>>>>    		.of_match_table	= of_match_ptr(arm_spe_pmu_of_match),
>>>
>>> Hmm, so some other drivers don't hook .id_table like you do, but instead
>>> hook .acpi_match_table in the driver structure. Is that not better?
>>
>> This isn't actually an ACPI device, (aka not defined in the namespace), so
>> its missing much of the ACPI functionality. I think that also means its
>> needs to be declared this way.
> 
> Looking at platform_match(), I'd really like to avoid having both an
> .id_table and an .of_match_table field.


> 
> acpi_of_match_device() will actually use the .of_match_table, but it relies
> on ACPI_COMPANION returning a valid acpi_device. If we don't have one of

Right, via the fwnode it can cause an acpi DSDT defined device with a 
_DSD "compatible" property to match an entry in the of_match_table 
compatible string. I don't think this is us...

> those, perhaps we can use the .id_table exclusively and drop the
> .of_match_table instead?

This definitely made me do my homework, the following is AFAIK:

Its possible to match on just a .id_table, but this requires matching 
the OF device name against the id_table name rather than against the OF 
compatible string (*). This doesn't seem like a good idea, despite 
platform_device_id entries being significantly smaller than the 
of_device_id ones. Plus, I think we end up with two duplicate tables 
because we still need the MODULE_TABLE(of,xxx) to assure that userspace 
can associate the modalias with the module.

OTOH, it seems possible to match on module name directly 
('arm_spe_pmu'), but this limits us to only a single device type for all 
ACPI device variations unless we put platform checks in the module 
itself (ick!). I suspect in the future if a spe.v2 were to come out this 
would be a problem unless a separate module were created. Then there is 
the fact this still needs a platform_device_id table, as the modalias 
will read "platform:arm_spe_pmu". Which will cause people to question 
why its not just assigned and matched against the .id_table.


*(interestingly trivia: There doesn't appear to be a single arm64 module 
which matches on a MODULE_TABLE OF name. They only match type or 
compatible. Out of the 3534 modules on my machine only three do any OF 
table type matching, ipmi_si and two drivers for freescale networking 
fsl_pq_mdio and gianfar_driver. In those cases, i'm not even sure its 
actually necessary.)

Re: [PATCH 4/4] perf: arm_spe: Enable ACPI/Platform automatic module loading

[Jeremy Linton]

Hi,

On 4/16/19 8:50 AM, Will Deacon wrote:
> On Thu, Apr 04, 2019 at 12:24:38PM -0500, Jeremy Linton wrote:
>> On 4/4/19 12:04 PM, Will Deacon wrote:
>>> On Tue, Mar 26, 2019 at 05:39:38PM -0500, Jeremy Linton wrote:
>>>> Lets add the MODULE_TABLE and platform id_table entries so that
>>>> the SPE driver can attach to the ACPI platform device created by
>>>> the core pmu code.
>>>>
>>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>>>> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
>>>> ---
>>>>    drivers/perf/arm_spe_pmu.c | 11 +++++++++--
>>>>    1 file changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
>>>> index 7cb766dafe85..ffa2c76c08bb 100644
>>>> --- a/drivers/perf/arm_spe_pmu.c
>>>> +++ b/drivers/perf/arm_spe_pmu.c
>>>> @@ -1176,7 +1176,13 @@ static const struct of_device_id arm_spe_pmu_of_match[] = {
>>>>    };
>>>>    MODULE_DEVICE_TABLE(of, arm_spe_pmu_of_match);
>>>> -static int arm_spe_pmu_device_dt_probe(struct platform_device *pdev)
>>>> +static const struct platform_device_id arm_spe_match[] = {
>>>> +	{ "arm,spe-v1", 0},
>>>
>>> It would be nice if we could avoid duplicating this string from the ACPI
>>> parsing code.
>>
>> Ok sure, I just need to find a good common place for it.

There doesn't appear to be a good common place for this, so maybe 
arm_pmu.h, which can then be included in the spe driver is the right thing.


>>
>>>
>>>> +	{ }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(platform, arm_spe_match);
>>>> +
>>>> +static int arm_spe_pmu_device_probe(struct platform_device *pdev)
>>>>    {
>>>>    	int ret;
>>>>    	struct arm_spe_pmu *spe_pmu;
>>>> @@ -1236,11 +1242,12 @@ static int arm_spe_pmu_device_remove(struct platform_device *pdev)
>>>>    }
>>>>    static struct platform_driver arm_spe_pmu_driver = {
>>>> +	.id_table = arm_spe_match,
>>>>    	.driver	= {
>>>>    		.name		= DRVNAME,
>>>>    		.of_match_table	= of_match_ptr(arm_spe_pmu_of_match),
>>>
>>> Hmm, so some other drivers don't hook .id_table like you do, but instead
>>> hook .acpi_match_table in the driver structure. Is that not better?
>>
>> This isn't actually an ACPI device, (aka not defined in the namespace), so
>> its missing much of the ACPI functionality. I think that also means its
>> needs to be declared this way.
> 
> Looking at platform_match(), I'd really like to avoid having both an
> .id_table and an .of_match_table field.


> 
> acpi_of_match_device() will actually use the .of_match_table, but it relies
> on ACPI_COMPANION returning a valid acpi_device. If we don't have one of

Right, via the fwnode it can cause an acpi DSDT defined device with a 
_DSD "compatible" property to match an entry in the of_match_table 
compatible string. I don't think this is us...

> those, perhaps we can use the .id_table exclusively and drop the
> .of_match_table instead?

This definitely made me do my homework, the following is AFAIK:

Its possible to match on just a .id_table, but this requires matching 
the OF device name against the id_table name rather than against the OF 
compatible string (*). This doesn't seem like a good idea, despite 
platform_device_id entries being significantly smaller than the 
of_device_id ones. Plus, I think we end up with two duplicate tables 
because we still need the MODULE_TABLE(of,xxx) to assure that userspace 
can associate the modalias with the module.

OTOH, it seems possible to match on module name directly 
('arm_spe_pmu'), but this limits us to only a single device type for all 
ACPI device variations unless we put platform checks in the module 
itself (ick!). I suspect in the future if a spe.v2 were to come out this 
would be a problem unless a separate module were created. Then there is 
the fact this still needs a platform_device_id table, as the modalias 
will read "platform:arm_spe_pmu". Which will cause people to question 
why its not just assigned and matched against the .id_table.


*(interestingly trivia: There doesn't appear to be a single arm64 module 
which matches on a MODULE_TABLE OF name. They only match type or 
compatible. Out of the 3534 modules on my machine only three do any OF 
table type matching, ipmi_si and two drivers for freescale networking 
fsl_pq_mdio and gianfar_driver. In those cases, i'm not even sure its 
actually necessary.)

Re: [PATCH 4/4] perf: arm_spe: Enable ACPI/Platform automatic module loading

[Will Deacon]

On Thu, Apr 25, 2019 at 07:58:28PM -0500, Jeremy Linton wrote:
> On 4/16/19 8:50 AM, Will Deacon wrote:
> > On Thu, Apr 04, 2019 at 12:24:38PM -0500, Jeremy Linton wrote:
> > > On 4/4/19 12:04 PM, Will Deacon wrote:
> > > > On Tue, Mar 26, 2019 at 05:39:38PM -0500, Jeremy Linton wrote:
> > > > > Lets add the MODULE_TABLE and platform id_table entries so that
> > > > > the SPE driver can attach to the ACPI platform device created by
> > > > > the core pmu code.
> > > > > 
> > > > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> > > > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > > ---
> > > > >    drivers/perf/arm_spe_pmu.c | 11 +++++++++--
> > > > >    1 file changed, 9 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> > > > > index 7cb766dafe85..ffa2c76c08bb 100644
> > > > > --- a/drivers/perf/arm_spe_pmu.c
> > > > > +++ b/drivers/perf/arm_spe_pmu.c
> > > > > @@ -1176,7 +1176,13 @@ static const struct of_device_id arm_spe_pmu_of_match[] = {
> > > > >    };
> > > > >    MODULE_DEVICE_TABLE(of, arm_spe_pmu_of_match);
> > > > > -static int arm_spe_pmu_device_dt_probe(struct platform_device *pdev)
> > > > > +static const struct platform_device_id arm_spe_match[] = {
> > > > > +	{ "arm,spe-v1", 0},
> > > > 
> > > > It would be nice if we could avoid duplicating this string from the ACPI
> > > > parsing code.
> > > 
> > > Ok sure, I just need to find a good common place for it.
> 
> There doesn't appear to be a good common place for this, so maybe arm_pmu.h,
> which can then be included in the spe driver is the right thing.

I'm fine with that.

> > > > > +	{ }
> > > > > +};
> > > > > +MODULE_DEVICE_TABLE(platform, arm_spe_match);
> > > > > +
> > > > > +static int arm_spe_pmu_device_probe(struct platform_device *pdev)
> > > > >    {
> > > > >    	int ret;
> > > > >    	struct arm_spe_pmu *spe_pmu;
> > > > > @@ -1236,11 +1242,12 @@ static int arm_spe_pmu_device_remove(struct platform_device *pdev)
> > > > >    }
> > > > >    static struct platform_driver arm_spe_pmu_driver = {
> > > > > +	.id_table = arm_spe_match,
> > > > >    	.driver	= {
> > > > >    		.name		= DRVNAME,
> > > > >    		.of_match_table	= of_match_ptr(arm_spe_pmu_of_match),
> > > > 
> > > > Hmm, so some other drivers don't hook .id_table like you do, but instead
> > > > hook .acpi_match_table in the driver structure. Is that not better?
> > > 
> > > This isn't actually an ACPI device, (aka not defined in the namespace), so
> > > its missing much of the ACPI functionality. I think that also means its
> > > needs to be declared this way.
> > 
> > Looking at platform_match(), I'd really like to avoid having both an
> > .id_table and an .of_match_table field.
> 
> 
> > 
> > acpi_of_match_device() will actually use the .of_match_table, but it relies
> > on ACPI_COMPANION returning a valid acpi_device. If we don't have one of
> 
> Right, via the fwnode it can cause an acpi DSDT defined device with a _DSD
> "compatible" property to match an entry in the of_match_table compatible
> string. I don't think this is us...
> 
> > those, perhaps we can use the .id_table exclusively and drop the
> > .of_match_table instead?
> 
> This definitely made me do my homework, the following is AFAIK:

FWIW: I'm also feeling my way here!

> Its possible to match on just a .id_table, but this requires matching the OF
> device name against the id_table name rather than against the OF compatible
> string (*). This doesn't seem like a good idea, despite platform_device_id
> entries being significantly smaller than the of_device_id ones. Plus, I
> think we end up with two duplicate tables because we still need the
> MODULE_TABLE(of,xxx) to assure that userspace can associate the modalias
> with the module.

Well spotted, I didn't notice that the compatible string isn't used for
matching in that case.

> OTOH, it seems possible to match on module name directly ('arm_spe_pmu'),
> but this limits us to only a single device type for all ACPI device
> variations unless we put platform checks in the module itself (ick!). I
> suspect in the future if a spe.v2 were to come out this would be a problem
> unless a separate module were created. Then there is the fact this still
> needs a platform_device_id table, as the modalias will read
> "platform:arm_spe_pmu". Which will cause people to question why its not just
> assigned and matched against the .id_table.

Ok, fair enough and sorry for the wild goose chase. Looks like we'll stick
with what you had, as the alternatives all seen considerably worse.

Will

Re: [PATCH 4/4] perf: arm_spe: Enable ACPI/Platform automatic module loading

[Will Deacon]

On Thu, Apr 25, 2019 at 07:58:28PM -0500, Jeremy Linton wrote:
> On 4/16/19 8:50 AM, Will Deacon wrote:
> > On Thu, Apr 04, 2019 at 12:24:38PM -0500, Jeremy Linton wrote:
> > > On 4/4/19 12:04 PM, Will Deacon wrote:
> > > > On Tue, Mar 26, 2019 at 05:39:38PM -0500, Jeremy Linton wrote:
> > > > > Lets add the MODULE_TABLE and platform id_table entries so that
> > > > > the SPE driver can attach to the ACPI platform device created by
> > > > > the core pmu code.
> > > > > 
> > > > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> > > > > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > > > > ---
> > > > >    drivers/perf/arm_spe_pmu.c | 11 +++++++++--
> > > > >    1 file changed, 9 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> > > > > index 7cb766dafe85..ffa2c76c08bb 100644
> > > > > --- a/drivers/perf/arm_spe_pmu.c
> > > > > +++ b/drivers/perf/arm_spe_pmu.c
> > > > > @@ -1176,7 +1176,13 @@ static const struct of_device_id arm_spe_pmu_of_match[] = {
> > > > >    };
> > > > >    MODULE_DEVICE_TABLE(of, arm_spe_pmu_of_match);
> > > > > -static int arm_spe_pmu_device_dt_probe(struct platform_device *pdev)
> > > > > +static const struct platform_device_id arm_spe_match[] = {
> > > > > +	{ "arm,spe-v1", 0},
> > > > 
> > > > It would be nice if we could avoid duplicating this string from the ACPI
> > > > parsing code.
> > > 
> > > Ok sure, I just need to find a good common place for it.
> 
> There doesn't appear to be a good common place for this, so maybe arm_pmu.h,
> which can then be included in the spe driver is the right thing.

I'm fine with that.

> > > > > +	{ }
> > > > > +};
> > > > > +MODULE_DEVICE_TABLE(platform, arm_spe_match);
> > > > > +
> > > > > +static int arm_spe_pmu_device_probe(struct platform_device *pdev)
> > > > >    {
> > > > >    	int ret;
> > > > >    	struct arm_spe_pmu *spe_pmu;
> > > > > @@ -1236,11 +1242,12 @@ static int arm_spe_pmu_device_remove(struct platform_device *pdev)
> > > > >    }
> > > > >    static struct platform_driver arm_spe_pmu_driver = {
> > > > > +	.id_table = arm_spe_match,
> > > > >    	.driver	= {
> > > > >    		.name		= DRVNAME,
> > > > >    		.of_match_table	= of_match_ptr(arm_spe_pmu_of_match),
> > > > 
> > > > Hmm, so some other drivers don't hook .id_table like you do, but instead
> > > > hook .acpi_match_table in the driver structure. Is that not better?
> > > 
> > > This isn't actually an ACPI device, (aka not defined in the namespace), so
> > > its missing much of the ACPI functionality. I think that also means its
> > > needs to be declared this way.
> > 
> > Looking at platform_match(), I'd really like to avoid having both an
> > .id_table and an .of_match_table field.
> 
> 
> > 
> > acpi_of_match_device() will actually use the .of_match_table, but it relies
> > on ACPI_COMPANION returning a valid acpi_device. If we don't have one of
> 
> Right, via the fwnode it can cause an acpi DSDT defined device with a _DSD
> "compatible" property to match an entry in the of_match_table compatible
> string. I don't think this is us...
> 
> > those, perhaps we can use the .id_table exclusively and drop the
> > .of_match_table instead?
> 
> This definitely made me do my homework, the following is AFAIK:

FWIW: I'm also feeling my way here!

> Its possible to match on just a .id_table, but this requires matching the OF
> device name against the id_table name rather than against the OF compatible
> string (*). This doesn't seem like a good idea, despite platform_device_id
> entries being significantly smaller than the of_device_id ones. Plus, I
> think we end up with two duplicate tables because we still need the
> MODULE_TABLE(of,xxx) to assure that userspace can associate the modalias
> with the module.

Well spotted, I didn't notice that the compatible string isn't used for
matching in that case.

> OTOH, it seems possible to match on module name directly ('arm_spe_pmu'),
> but this limits us to only a single device type for all ACPI device
> variations unless we put platform checks in the module itself (ick!). I
> suspect in the future if a spe.v2 were to come out this would be a problem
> unless a separate module were created. Then there is the fact this still
> needs a platform_device_id table, as the modalias will read
> "platform:arm_spe_pmu". Which will cause people to question why its not just
> assigned and matched against the .id_table.

Ok, fair enough and sorry for the wild goose chase. Looks like we'll stick
with what you had, as the alternatives all seen considerably worse.

Will

[PATCH v4 0/4] arm64: SPE ACPI enablement

[Jeremy Linton]

This patch series enables the Arm Statistical Profiling
Extension (SPE) on ACPI platforms.

This is possible because ACPI 6.3 uses a previously
reserved field in the MADT to store the SPE interrupt
number, similarly to how the normal PMU is described.
If a consistent valid interrupt exists across all the
cores in the system, a platform device is registered.
That then triggers the SPE module, which runs as normal.

We also add the ability to parse the PPTT for IDENTICAL
cores. We then use this to sanity check the single SPE
device we create. This creates a bit of a problem with
respect to the specification though. The specification
says that its legal for multiple tree's to exist in the
PPTT. We handle this fine, but what happens in the
case of multiple tree's is that the lack of a common
node with IDENTICAL set forces us to assume that there
are multiple non-IDENTICAL cores in the machine.

v3->v4: Rebase to 5.2.
	Minor formatting, patch rearrangement.
	Add missing `inline` in static header definition.
	Drop ARM_SPE_ACPI and just use ARM_SPE_PMU.

v2->v3: Previously a function pointer was being used
	  to handle the more complex node checking
	  required by the IDENTICAL flag. This version
	  simply checks for the IDENTICAL flag and calls
	  flag_identical() to preform the revision
	  and next node checks. (I think after reading
	  Raphael's comments for the Nth time, this is
	  actually what he was suggesting, which I
	  initially miss interpreted).
	Modify subject of first patch so that its clear
	  a that its a capitalization change rather,
	  than a logical C 'case' change.

v1->v2: Wrap the code which creates the SPE device in
	    a new CONFIG_ARM_SPE_ACPI ifdef.
	Move arm,spe-v1 device name into common header file
	Some comment/case sensitivity/function name changes.

Jeremy Linton (4):
  ACPI/PPTT: Modify node flag detection to find last IDENTICAL
  ACPI/PPTT: Add function to return ACPI 6.3 Identical tokens
  arm_pmu: acpi: spe: Add initial MADT/SPE probing
  perf: arm_spe: Enable ACPI/Platform automatic module loading

 arch/arm64/include/asm/acpi.h |  3 ++
 drivers/acpi/pptt.c           | 61 +++++++++++++++++++++++++---
 drivers/perf/arm_pmu_acpi.c   | 75 +++++++++++++++++++++++++++++++++++
 drivers/perf/arm_spe_pmu.c    | 12 +++++-
 include/linux/acpi.h          |  5 +++
 include/linux/perf/arm_pmu.h  |  2 +
 6 files changed, 150 insertions(+), 8 deletions(-)

-- 
2.21.0

[PATCH 4/4] perf: arm_spe: Enable ACPI/Platform automatic module loading

[Jeremy Linton]

Lets add the MODULE_TABLE and platform id_table entries so that
the SPE driver can attach to the ACPI platform device created by
the core pmu code.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/perf/arm_spe_pmu.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
index e120f933412a..4fb65c61c8ea 100644
--- a/drivers/perf/arm_spe_pmu.c
+++ b/drivers/perf/arm_spe_pmu.c
@@ -38,6 +38,7 @@
 #include <linux/of_address.h>
 #include <linux/of_device.h>
 #include <linux/perf_event.h>
+#include <linux/perf/arm_pmu.h>
 #include <linux/platform_device.h>
 #include <linux/printk.h>
 #include <linux/slab.h>
@@ -1168,7 +1169,13 @@ static const struct of_device_id arm_spe_pmu_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, arm_spe_pmu_of_match);
 
-static int arm_spe_pmu_device_dt_probe(struct platform_device *pdev)
+static const struct platform_device_id arm_spe_match[] = {
+	{ ARMV8_SPE_PDEV_NAME, 0},
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, arm_spe_match);
+
+static int arm_spe_pmu_device_probe(struct platform_device *pdev)
 {
 	int ret;
 	struct arm_spe_pmu *spe_pmu;
@@ -1228,11 +1235,12 @@ static int arm_spe_pmu_device_remove(struct platform_device *pdev)
 }
 
 static struct platform_driver arm_spe_pmu_driver = {
+	.id_table = arm_spe_match,
 	.driver	= {
 		.name		= DRVNAME,
 		.of_match_table	= of_match_ptr(arm_spe_pmu_of_match),
 	},
-	.probe	= arm_spe_pmu_device_dt_probe,
+	.probe	= arm_spe_pmu_device_probe,
 	.remove	= arm_spe_pmu_device_remove,
 };
 
-- 
2.21.0