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
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 <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Sudeep Holla <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Tue, Mar 26, 2019 at 11:40 PM Jeremy Linton <[email protected]> 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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> ---
> 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;
>
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 <[email protected]> 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 <[email protected]>
>> ---
>> 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
>>
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 <[email protected]>
>> ---
>> ?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;
>>
>
>
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 <[email protected]>
> Reviewed-by: Sudeep Holla <[email protected]>
> ---
> 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
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 <[email protected]>
>> Reviewed-by: Sudeep Holla <[email protected]>
>> ---
>> 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
>
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 <[email protected]>
>>> ---
>>> 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;
>>>
>>
>>
>
>
> .
>
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 <[email protected]>
> > > Reviewed-by: Sudeep Holla <[email protected]>
> > > ---
> > > 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
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 <[email protected]>
>>>> Reviewed-by: Sudeep Holla <[email protected]>
>>>> ---
>>>> 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.)
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 <[email protected]>
> > > > > Reviewed-by: Sudeep Holla <[email protected]>
> > > > > ---
> > > > > 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