This patchset adds support for ARM CoreSight PMU device.
Specifications for ARM Performance Monitoring Unit table (APMT) and
ARM CoreSight PMU:
* APMT: https://developer.arm.com/documentation/den0117/latest
* ARM Coresight PMU:
https://developer.arm.com/documentation/ihi0091/latest
The patchset applies on top of
git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
Besar Wicaksono (2):
ACPICA: Add support for ARM Performance Monitoring Unit Table.
ACPI: ARM Performance Monitoring Unit Table (APMT) initial support
arch/arm64/Kconfig | 1 +
drivers/acpi/arm64/Kconfig | 3 +
drivers/acpi/arm64/Makefile | 1 +
drivers/acpi/arm64/apmt.c | 176 ++++++++++++++++++++++++++++++++++++
drivers/acpi/bus.c | 2 +
include/acpi/actbl2.h | 81 +++++++++++++++++
include/linux/acpi_apmt.h | 19 ++++
7 files changed, 283 insertions(+)
create mode 100644 drivers/acpi/arm64/apmt.c
create mode 100644 include/linux/acpi_apmt.h
--
2.17.1
ARM Performance Monitoring Unit Table describes the properties of PMU
support in ARM-based system. The APMT table contains a list of nodes,
each represents a PMU in the system that conforms to ARM CoreSight PMU
architecture. The properties of each node include information required
to access the PMU (e.g. MMIO base address, interrupt number) and also
identification. For more detailed information, please refer to the
specification below:
* APMT: https://developer.arm.com/documentation/den0117/latest
* ARM Coresight PMU:
https://developer.arm.com/documentation/ihi0091/latest
The initial support adds the detection of APMT table and generic
infrastructure to create platform devices for ARM CoreSight PMUs.
Similar to IORT the root pointer of APMT is preserved during runtime
and each PMU platform device is given a pointer to the corresponding
APMT node.
Signed-off-by: Besar Wicaksono <[email protected]>
---
arch/arm64/Kconfig | 1 +
drivers/acpi/arm64/Kconfig | 3 +
drivers/acpi/arm64/Makefile | 1 +
drivers/acpi/arm64/apmt.c | 176 ++++++++++++++++++++++++++++++++++++
drivers/acpi/bus.c | 2 +
include/linux/acpi_apmt.h | 19 ++++
6 files changed, 202 insertions(+)
create mode 100644 drivers/acpi/arm64/apmt.c
create mode 100644 include/linux/acpi_apmt.h
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index e80fd2372f02..49e01c4377f2 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
config ARM64
def_bool y
+ select ACPI_APMT if ACPI
select ACPI_CCA_REQUIRED if ACPI
select ACPI_GENERIC_GSI if ACPI
select ACPI_GTDT if ACPI
diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig
index d4a72835f328..b3ed6212244c 100644
--- a/drivers/acpi/arm64/Kconfig
+++ b/drivers/acpi/arm64/Kconfig
@@ -18,3 +18,6 @@ config ACPI_AGDI
reset command.
If set, the kernel parses AGDI table and listens for the command.
+
+config ACPI_APMT
+ bool
diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile
index 7b9e4045659d..e21a9e84e394 100644
--- a/drivers/acpi/arm64/Makefile
+++ b/drivers/acpi/arm64/Makefile
@@ -2,4 +2,5 @@
obj-$(CONFIG_ACPI_AGDI) += agdi.o
obj-$(CONFIG_ACPI_IORT) += iort.o
obj-$(CONFIG_ACPI_GTDT) += gtdt.o
+obj-$(CONFIG_ACPI_APMT) += apmt.o
obj-y += dma.o
diff --git a/drivers/acpi/arm64/apmt.c b/drivers/acpi/arm64/apmt.c
new file mode 100644
index 000000000000..8b8b9f480548
--- /dev/null
+++ b/drivers/acpi/arm64/apmt.c
@@ -0,0 +1,176 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ARM APMT table support.
+ * Design document number: ARM DEN0117.
+ *
+ * Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES.
+ *
+ */
+
+#define pr_fmt(fmt) "ACPI: APMT: " fmt
+
+#include <linux/acpi.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+
+#define DEV_NAME "arm-csite-pmu"
+
+/* There can be up to 3 resources: page 0 and 1 address, and interrupt. */
+#define DEV_MAX_RESOURCE_COUNT 3
+
+/* Root pointer to the mapped APMT table */
+static struct acpi_table_header *apmt_table;
+
+static int __init apmt_init_resources(struct resource *res,
+ struct acpi_apmt_node *node)
+{
+ int irq, trigger;
+ int num_res = 0;
+
+ res[num_res].start = node->base_address0;
+ res[num_res].end = node->base_address0 + SZ_4K - 1;
+ res[num_res].flags = IORESOURCE_MEM;
+
+ num_res++;
+
+ res[num_res].start = node->base_address1;
+ res[num_res].end = node->base_address1 + SZ_4K - 1;
+ res[num_res].flags = IORESOURCE_MEM;
+
+ num_res++;
+
+ if (node->ovflw_irq != 0) {
+ trigger = (node->ovflw_irq_flags & ACPI_APMT_OVFLW_IRQ_FLAGS_MODE);
+ trigger = (trigger == ACPI_APMT_OVFLW_IRQ_FLAGS_MODE_LEVEL) ?
+ ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE;
+ irq = acpi_register_gsi(NULL, node->ovflw_irq, trigger,
+ ACPI_ACTIVE_HIGH);
+
+ if (irq <= 0) {
+ pr_warn("APMT could not register gsi hwirq %d\n", irq);
+ return num_res;
+ }
+
+ res[num_res].start = irq;
+ res[num_res].end = irq;
+ res[num_res].flags = IORESOURCE_IRQ;
+
+ num_res++;
+ }
+
+ return num_res;
+}
+
+/**
+ * apmt_add_platform_device() - Allocate a platform device for APMT node
+ * @node: Pointer to device ACPI APMT node
+ *
+ * Returns: 0 on success, <0 failure
+ */
+static int __init apmt_add_platform_device(struct acpi_apmt_node *node,
+ struct fwnode_handle *fwnode)
+{
+ struct platform_device *pdev;
+ int ret, count;
+ struct resource res[DEV_MAX_RESOURCE_COUNT];
+
+ pdev = platform_device_alloc(DEV_NAME, PLATFORM_DEVID_AUTO);
+ if (!pdev)
+ return -ENOMEM;
+
+ memset(res, 0, sizeof(res));
+
+ count = apmt_init_resources(res, node);
+
+ ret = platform_device_add_resources(pdev, res, count);
+ if (ret)
+ goto dev_put;
+
+ /*
+ * Add a copy of APMT node pointer to platform_data to be used to
+ * retrieve APMT data information.
+ */
+ ret = platform_device_add_data(pdev, &node, sizeof(node));
+ if (ret)
+ goto dev_put;
+
+ pdev->dev.fwnode = fwnode;
+
+ ret = platform_device_add(pdev);
+
+ if (ret)
+ goto dev_put;
+
+ return 0;
+
+dev_put:
+ platform_device_put(pdev);
+
+ return ret;
+}
+
+static int __init apmt_init_platform_devices(void)
+{
+ struct acpi_apmt_node *apmt_node;
+ struct acpi_table_apmt *apmt;
+ struct fwnode_handle *fwnode;
+ u64 offset, end;
+ int ret;
+
+ /*
+ * apmt_table and apmt both point to the start of APMT table, but
+ * have different struct types
+ */
+ apmt = (struct acpi_table_apmt *)apmt_table;
+ offset = sizeof(*apmt);
+ end = apmt->header.length;
+
+ while (offset < end) {
+ apmt_node = ACPI_ADD_PTR(struct acpi_apmt_node, apmt,
+ offset);
+
+ fwnode = acpi_alloc_fwnode_static();
+ if (!fwnode)
+ return -ENOMEM;
+
+ ret = apmt_add_platform_device(apmt_node, fwnode);
+ if (ret) {
+ acpi_free_fwnode_static(fwnode);
+ return ret;
+ }
+
+ offset += apmt_node->length;
+ }
+
+ return 0;
+}
+
+void __init acpi_apmt_init(void)
+{
+ acpi_status status;
+ int ret;
+
+ /**
+ * APMT table nodes will be used at runtime after the apmt init,
+ * so we don't need to call acpi_put_table() to release
+ * the APMT table mapping.
+ */
+ status = acpi_get_table(ACPI_SIG_APMT, 0, &apmt_table);
+
+ if (ACPI_FAILURE(status)) {
+ if (status != AE_NOT_FOUND) {
+ const char *msg = acpi_format_exception(status);
+
+ pr_err("Failed to get APMT table, %s\n", msg);
+ }
+
+ return;
+ }
+
+ ret = apmt_init_platform_devices();
+ if (ret) {
+ pr_err("Failed to initialize APMT platform devices, ret: %d\n", ret);
+ acpi_put_table(apmt_table);
+ }
+}
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 3e58b613a2c4..25ddd9f300e7 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -27,6 +27,7 @@
#include <linux/dmi.h>
#endif
#include <linux/acpi_agdi.h>
+#include <linux/acpi_apmt.h>
#include <linux/acpi_iort.h>
#include <linux/acpi_viot.h>
#include <linux/pci.h>
@@ -1367,6 +1368,7 @@ static int __init acpi_init(void)
acpi_setup_sb_notify_handler();
acpi_viot_init();
acpi_agdi_init();
+ acpi_apmt_init();
return 0;
}
diff --git a/include/linux/acpi_apmt.h b/include/linux/acpi_apmt.h
new file mode 100644
index 000000000000..40bd634d082f
--- /dev/null
+++ b/include/linux/acpi_apmt.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * ARM CoreSight PMU driver.
+ * Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES.
+ *
+ */
+
+#ifndef __ACPI_APMT_H__
+#define __ACPI_APMT_H__
+
+#include <linux/acpi.h>
+
+#ifdef CONFIG_ACPI_APMT
+void acpi_apmt_init(void);
+#else
+static inline void acpi_apmt_init(void) { }
+#endif /* CONFIG_ACPI_APMT */
+
+#endif /* __ACPI_APMT_H__ */
--
2.17.1
ACPICA commit 002165ecc0a3dc703bb24c789aaa02fdada01675
The specification of this table is described in
"ARM Performance Monitoring Unit Architecture 1.0 Platform Design Document"
ARM DEN0117.
This patch adds the necessary types and support for
compiling/disassembling APMT.
Link: https://github.com/acpica/acpica/commit/002165ec
Signed-off-by: Besar Wicaksono <[email protected]>
---
include/acpi/actbl2.h | 81 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 81 insertions(+)
diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index 16847c8d9d5f..8fc5cf318c15 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -25,6 +25,7 @@
* the wrong signature.
*/
#define ACPI_SIG_AGDI "AGDI" /* Arm Generic Diagnostic Dump and Reset Device Interface */
+#define ACPI_SIG_APMT "APMT" /* Arm Performance Monitoring Unit table */
#define ACPI_SIG_BDAT "BDAT" /* BIOS Data ACPI Table */
#define ACPI_SIG_IORT "IORT" /* IO Remapping Table */
#define ACPI_SIG_IVRS "IVRS" /* I/O Virtualization Reporting Structure */
@@ -258,6 +259,86 @@ struct acpi_table_agdi {
#define ACPI_AGDI_SIGNALING_MODE (1)
+/*******************************************************************************
+ *
+ * APMT - ARM Performance Monitoring Unit Table
+ *
+ * Conforms to:
+ * ARM Performance Monitoring Unit Architecture 1.0 Platform Design Document
+ * ARM DEN0117 v1.0 November 25, 2021
+ *
+ ******************************************************************************/
+
+struct acpi_table_apmt {
+ struct acpi_table_header header; /* Common ACPI table header */
+};
+
+#define ACPI_APMT_NODE_ID_LENGTH 4
+
+/*
+ * APMT subtables
+ */
+struct acpi_apmt_node {
+ u16 length;
+ u8 flags;
+ u8 type;
+ u32 id;
+ u64 inst_primary;
+ u32 inst_secondary;
+ u64 base_address0;
+ u64 base_address1;
+ u32 ovflw_irq;
+ u32 reserved;
+ u32 ovflw_irq_flags;
+ u32 proc_affinity;
+ u32 impl_id;
+};
+
+/* Masks for Flags field above */
+
+#define ACPI_APMT_FLAGS_DUAL_PAGE (1<<0)
+#define ACPI_APMT_FLAGS_AFFINITY (1<<1)
+#define ACPI_APMT_FLAGS_ATOMIC (1<<2)
+
+/* Values for Flags dual page field above */
+
+#define ACPI_APMT_FLAGS_DUAL_PAGE_NSUPP (0<<0)
+#define ACPI_APMT_FLAGS_DUAL_PAGE_SUPP (1<<0)
+
+/* Values for Flags processor affinity field above */
+#define ACPI_APMT_FLAGS_AFFINITY_PROC (0<<1)
+#define ACPI_APMT_FLAGS_AFFINITY_PROC_CONTAINER (1<<1)
+
+/* Values for Flags 64-bit atomic field above */
+#define ACPI_APMT_FLAGS_ATOMIC_NSUPP (0<<2)
+#define ACPI_APMT_FLAGS_ATOMIC_SUPP (1<<2)
+
+/* Values for Type field above */
+
+enum acpi_apmt_node_type {
+ ACPI_APMT_NODE_TYPE_MC = 0x00,
+ ACPI_APMT_NODE_TYPE_SMMU = 0x01,
+ ACPI_APMT_NODE_TYPE_PCIE_ROOT = 0x02,
+ ACPI_APMT_NODE_TYPE_ACPI = 0x03,
+ ACPI_APMT_NODE_TYPE_CACHE = 0x04,
+ ACPI_APMT_NODE_TYPE_COUNT
+};
+
+/* Masks for ovflw_irq_flags field above */
+
+#define ACPI_APMT_OVFLW_IRQ_FLAGS_MODE (1<<0)
+#define ACPI_APMT_OVFLW_IRQ_FLAGS_TYPE (1<<1)
+
+/* Values for ovflw_irq_flags mode field above */
+
+#define ACPI_APMT_OVFLW_IRQ_FLAGS_MODE_LEVEL (0<<0)
+#define ACPI_APMT_OVFLW_IRQ_FLAGS_MODE_EDGE (1<<0)
+
+/* Values for ovflw_irq_flags type field above */
+
+#define ACPI_APMT_OVFLW_IRQ_FLAGS_TYPE_WIRED (0<<1)
+
+
/*******************************************************************************
*
* BDAT - BIOS Data ACPI Table
--
2.17.1
On Tue, Apr 19, 2022 at 10:55 PM Besar Wicaksono <[email protected]> wrote:
>
> ACPICA commit 002165ecc0a3dc703bb24c789aaa02fdada01675
>
> The specification of this table is described in
> "ARM Performance Monitoring Unit Architecture 1.0 Platform Design Document"
> ARM DEN0117.
>
> This patch adds the necessary types and support for
> compiling/disassembling APMT.
>
> Link: https://github.com/acpica/acpica/commit/002165ec
> Signed-off-by: Besar Wicaksono <[email protected]>
It should be equivalent to this patch:
https://patchwork.kernel.org/project/linux-acpi/patch/3370028.QJadu78ljV@kreacher/
present in linux-next.
If the other patch in the series is ACKed by the ARM64 ACPi people, I
can take it too.
Thanks!
> ---
> include/acpi/actbl2.h | 81 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 81 insertions(+)
>
> diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
> index 16847c8d9d5f..8fc5cf318c15 100644
> --- a/include/acpi/actbl2.h
> +++ b/include/acpi/actbl2.h
> @@ -25,6 +25,7 @@
> * the wrong signature.
> */
> #define ACPI_SIG_AGDI "AGDI" /* Arm Generic Diagnostic Dump and Reset Device Interface */
> +#define ACPI_SIG_APMT "APMT" /* Arm Performance Monitoring Unit table */
> #define ACPI_SIG_BDAT "BDAT" /* BIOS Data ACPI Table */
> #define ACPI_SIG_IORT "IORT" /* IO Remapping Table */
> #define ACPI_SIG_IVRS "IVRS" /* I/O Virtualization Reporting Structure */
> @@ -258,6 +259,86 @@ struct acpi_table_agdi {
>
> #define ACPI_AGDI_SIGNALING_MODE (1)
>
> +/*******************************************************************************
> + *
> + * APMT - ARM Performance Monitoring Unit Table
> + *
> + * Conforms to:
> + * ARM Performance Monitoring Unit Architecture 1.0 Platform Design Document
> + * ARM DEN0117 v1.0 November 25, 2021
> + *
> + ******************************************************************************/
> +
> +struct acpi_table_apmt {
> + struct acpi_table_header header; /* Common ACPI table header */
> +};
> +
> +#define ACPI_APMT_NODE_ID_LENGTH 4
> +
> +/*
> + * APMT subtables
> + */
> +struct acpi_apmt_node {
> + u16 length;
> + u8 flags;
> + u8 type;
> + u32 id;
> + u64 inst_primary;
> + u32 inst_secondary;
> + u64 base_address0;
> + u64 base_address1;
> + u32 ovflw_irq;
> + u32 reserved;
> + u32 ovflw_irq_flags;
> + u32 proc_affinity;
> + u32 impl_id;
> +};
> +
> +/* Masks for Flags field above */
> +
> +#define ACPI_APMT_FLAGS_DUAL_PAGE (1<<0)
> +#define ACPI_APMT_FLAGS_AFFINITY (1<<1)
> +#define ACPI_APMT_FLAGS_ATOMIC (1<<2)
> +
> +/* Values for Flags dual page field above */
> +
> +#define ACPI_APMT_FLAGS_DUAL_PAGE_NSUPP (0<<0)
> +#define ACPI_APMT_FLAGS_DUAL_PAGE_SUPP (1<<0)
> +
> +/* Values for Flags processor affinity field above */
> +#define ACPI_APMT_FLAGS_AFFINITY_PROC (0<<1)
> +#define ACPI_APMT_FLAGS_AFFINITY_PROC_CONTAINER (1<<1)
> +
> +/* Values for Flags 64-bit atomic field above */
> +#define ACPI_APMT_FLAGS_ATOMIC_NSUPP (0<<2)
> +#define ACPI_APMT_FLAGS_ATOMIC_SUPP (1<<2)
> +
> +/* Values for Type field above */
> +
> +enum acpi_apmt_node_type {
> + ACPI_APMT_NODE_TYPE_MC = 0x00,
> + ACPI_APMT_NODE_TYPE_SMMU = 0x01,
> + ACPI_APMT_NODE_TYPE_PCIE_ROOT = 0x02,
> + ACPI_APMT_NODE_TYPE_ACPI = 0x03,
> + ACPI_APMT_NODE_TYPE_CACHE = 0x04,
> + ACPI_APMT_NODE_TYPE_COUNT
> +};
> +
> +/* Masks for ovflw_irq_flags field above */
> +
> +#define ACPI_APMT_OVFLW_IRQ_FLAGS_MODE (1<<0)
> +#define ACPI_APMT_OVFLW_IRQ_FLAGS_TYPE (1<<1)
> +
> +/* Values for ovflw_irq_flags mode field above */
> +
> +#define ACPI_APMT_OVFLW_IRQ_FLAGS_MODE_LEVEL (0<<0)
> +#define ACPI_APMT_OVFLW_IRQ_FLAGS_MODE_EDGE (1<<0)
> +
> +/* Values for ovflw_irq_flags type field above */
> +
> +#define ACPI_APMT_OVFLW_IRQ_FLAGS_TYPE_WIRED (0<<1)
> +
> +
> /*******************************************************************************
> *
> * BDAT - BIOS Data ACPI Table
> --
> 2.17.1
>
Hi Sudeep,
> Any particular reason why you would like to rush and push this without
> the actual driver to probe the device being added here ?
I plan to have two patch series, one for ACPI patch (this patch) and one for driver patch.
My understanding is the driver patch will depend on this patch, but not the other way. So, I thought it would be better to get this patch approved first.
However, if it helps the review of this patch, I am hoping to post the driver patch by end of the week and will CC you on that one.
> I really don't prefer this name:
> 1. arm-coresight-pmu is much better than "csite". I see the short form
> used elsewhere in the kernel is just "cs" as in cs_etm,...etc
> 2. Since APMT is more generic than just coresight(I understand coresight
> was the initial motivation for the generic specification) and also
> the type list seem to cover memory controller, SMMU,..etc, does
> it make sense to call it "arm-generic-pmu" or something similar.
Between these two, I would prefer arm-coresight-pmu just to anticipate another standard in the future from ARM.
The APMT, to my understanding, is applicable only to CoreSight based PMUs. Using "coresight" as part of the device name is reasonable.
> Not sure if the same device name will be re-used or PMUs can be registered
> with different name under perf subsystem, but the name matters for the user
> space tools and decoders. They may use the name or type information to analyse
> the data samples.
>
> So it is better to wait for all those discussion as part of the driver
> upstreaming before you use this device name unless we are absolutely sure
> the PMUs can be registered with different names in the driver(which could
> be possible, I just don't know)
>
> Apart from this name, I am OK with the changes here and happy to ack if it
> is OK to merge this without any driver to probe this device yet.
I believe using a different name to register the PMU is possible.
In the current driver patch, we use a different name format to register the PMU: arm_csite_pmu<numeric id>. Certainly the "csite" needs to change as well ????
Another example like ARM CCI PMU uses device name "ARM-CCI PMU", but it is registered to perf subsystem as "CCI_400" or "CCI_500".
If there is no objection, I can post update to this patch and go ahead with "arm-coresight-pmu" for the device name.
Regards,
Besar
Hi Besar, Rafael,
Sorry for the delayed response.
On Tue, Apr 19, 2022 at 03:54:32PM -0500, Besar Wicaksono wrote:
> ARM Performance Monitoring Unit Table describes the properties of PMU
> support in ARM-based system. The APMT table contains a list of nodes,
> each represents a PMU in the system that conforms to ARM CoreSight PMU
> architecture. The properties of each node include information required
> to access the PMU (e.g. MMIO base address, interrupt number) and also
> identification. For more detailed information, please refer to the
> specification below:
> * APMT: https://developer.arm.com/documentation/den0117/latest
> * ARM Coresight PMU:
> https://developer.arm.com/documentation/ihi0091/latest
>
> The initial support adds the detection of APMT table and generic
> infrastructure to create platform devices for ARM CoreSight PMUs.
> Similar to IORT the root pointer of APMT is preserved during runtime
> and each PMU platform device is given a pointer to the corresponding
> APMT node.
>
Hi Besar,
This patch on its own looks fine and happy to ack. However I would like
to know general process on such changes that add platform or any bus
device but the driver itself is not upstream.
Any particular reason why you would like to rush and push this without
the actual driver to probe the device being added here ?
> Signed-off-by: Besar Wicaksono <[email protected]>
> ---
> arch/arm64/Kconfig | 1 +
> drivers/acpi/arm64/Kconfig | 3 +
> drivers/acpi/arm64/Makefile | 1 +
> drivers/acpi/arm64/apmt.c | 176 ++++++++++++++++++++++++++++++++++++
> drivers/acpi/bus.c | 2 +
> include/linux/acpi_apmt.h | 19 ++++
> 6 files changed, 202 insertions(+)
> create mode 100644 drivers/acpi/arm64/apmt.c
> create mode 100644 include/linux/acpi_apmt.h
>
[...]
> diff --git a/drivers/acpi/arm64/apmt.c b/drivers/acpi/arm64/apmt.c
> new file mode 100644
> index 000000000000..8b8b9f480548
> --- /dev/null
> +++ b/drivers/acpi/arm64/apmt.c
> @@ -0,0 +1,176 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ARM APMT table support.
> + * Design document number: ARM DEN0117.
> + *
> + * Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES.
> + *
> + */
> +
> +#define pr_fmt(fmt) "ACPI: APMT: " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +
> +#define DEV_NAME "arm-csite-pmu"
> +
I really don't prefer this name:
1. arm-coresight-pmu is much better than "csite". I see the short form
used elsewhere in the kernel is just "cs" as in cs_etm,...etc
2. Since APMT is more generic than just coresight(I understand coresight
was the initial motivation for the generic specification) and also
the type list seem to cover memory controller, SMMU,..etc, does
it make sense to call it "arm-generic-pmu" or something similar.
Anyways, it can be part of the driver discussion as people might have
opinion based on what and how the driver covers the variety of PMU
types possible as described in APMT.
Not sure if the same device name will be re-used or PMUs can be registered
with different name under perf subsystem, but the name matters for the user
space tools and decoders. They may use the name or type information to analyse
the data samples.
So it is better to wait for all those discussion as part of the driver
upstreaming before you use this device name unless we are absolutely sure
the PMUs can be registered with different names in the driver(which could
be possible, I just don't know)
Apart from this name, I am OK with the changes here and happy to ack if it
is OK to merge this without any driver to probe this device yet.
--
Regards,
Sudeep
On Wed, May 04, 2022 at 10:08:39PM +0000, Besar Wicaksono wrote:
> Hi Sudeep,
>
> > Any particular reason why you would like to rush and push this without
> > the actual driver to probe the device being added here ?
>
> I plan to have two patch series, one for ACPI patch (this patch) and one for
> driver patch. My understanding is the driver patch will depend on this
> patch, but not the other way. So, I thought it would be better to get this
> patch approved first. However, if it helps the review of this patch, I am
> hoping to post the driver patch by end of the week and will CC you on that
> one.
Sure please do that. IMO, the driver is usually upstreamed first along with
the DT bindings(in ACPI case either the spec change or the std namespace node)
The actual addition of the device happens via DT. ACPI APMT needs creation
of device here but I prefer to see the driver first.
>
> > I really don't prefer this name:
> > 1. arm-coresight-pmu is much better than "csite". I see the short form
> > used elsewhere in the kernel is just "cs" as in cs_etm,...etc
> > 2. Since APMT is more generic than just coresight(I understand coresight
> > was the initial motivation for the generic specification) and also
> > the type list seem to cover memory controller, SMMU,..etc, does
> > it make sense to call it "arm-generic-pmu" or something similar.
>
> Between these two, I would prefer arm-coresight-pmu just to anticipate
> another standard in the future from ARM. The APMT, to my understanding, is
> applicable only to CoreSight based PMUs. Using "coresight" as part of the
> device name is reasonable.
I read the APMT spec again and it has very little reference to coresight
though it is weirdly labelled as Coresight PMU for no sane reasons(Sorry I
know it's Arm to blame here and I bet something to do with marketing).
Anyways the APMT spec on its own covers all types of PMUs as I stated earlier.
So I prefer "arm-generic-pmu" or something better if you can come up with. I
am fine if you would like to retain arm-coresight-pmu when you post driver as
that's what the spec is labelled as.
Once you post the driver we can debate on that and come up with better name
with all the concerned parties involved.
>
> > Not sure if the same device name will be re-used or PMUs can be registered
> > with different name under perf subsystem, but the name matters for the user
> > space tools and decoders. They may use the name or type information to analyse
> > the data samples.
> >
> > So it is better to wait for all those discussion as part of the driver
> > upstreaming before you use this device name unless we are absolutely sure
> > the PMUs can be registered with different names in the driver(which could
> > be possible, I just don't know)
> >
> > Apart from this name, I am OK with the changes here and happy to ack if it
> > is OK to merge this without any driver to probe this device yet.
>
> I believe using a different name to register the PMU is possible.
> In the current driver patch, we use a different name format to register the
> PMU: arm_csite_pmu<numeric id>. Certainly the "csite" needs to change as
> well ????. Another example like ARM CCI PMU uses device name "ARM-CCI PMU",
> but it is registered to perf subsystem as "CCI_400" or "CCI_500".
>
Agreed, those are details we can discuss once you post with all the
maintainers involved.
> If there is no objection, I can post update to this patch and go ahead with
> "arm-coresight-pmu" for the device name.
Sure as I mentioned above that should be fine. I will then raise it with
the maintainers how we managed to labelled the spec to confuse it with other
components. I prefer whatever we add must be easy to identify and doesn't
conflict with existing PMUs(like ARM CPU PMUs, or the coresight ETM PMUs,
..etc)
In short, just post the driver the way you prefer and let us start the
discussion there.
--
Regards,
Sudeep
On 2022/4/20 4:54, Besar Wicaksono wrote:
> ARM Performance Monitoring Unit Table describes the properties of PMU
> support in ARM-based system. The APMT table contains a list of nodes,
> each represents a PMU in the system that conforms to ARM CoreSight PMU
> architecture. The properties of each node include information required
> to access the PMU (e.g. MMIO base address, interrupt number) and also
> identification. For more detailed information, please refer to the
> specification below:
> * APMT: https://developer.arm.com/documentation/den0117/latest
> * ARM Coresight PMU:
> https://developer.arm.com/documentation/ihi0091/latest
>
> The initial support adds the detection of APMT table and generic
> infrastructure to create platform devices for ARM CoreSight PMUs.
> Similar to IORT the root pointer of APMT is preserved during runtime
> and each PMU platform device is given a pointer to the corresponding
> APMT node.
>
> Signed-off-by: Besar Wicaksono <[email protected]>
> ---
> arch/arm64/Kconfig | 1 +
> drivers/acpi/arm64/Kconfig | 3 +
> drivers/acpi/arm64/Makefile | 1 +
> drivers/acpi/arm64/apmt.c | 176 ++++++++++++++++++++++++++++++++++++
> drivers/acpi/bus.c | 2 +
> include/linux/acpi_apmt.h | 19 ++++
> 6 files changed, 202 insertions(+)
> create mode 100644 drivers/acpi/arm64/apmt.c
> create mode 100644 include/linux/acpi_apmt.h
>
> +++ b/drivers/acpi/arm64/apmt.c
> @@ -0,0 +1,176 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ARM APMT table support.
> + * Design document number: ARM DEN0117.
> + *
> + * Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES.
> + *
> + */
> +
> +#define pr_fmt(fmt) "ACPI: APMT: " fmt
> +
> +#include <linux/acpi.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
Please include <linux/acpi_apmt.h>, here is the similar patch:
https://lore.kernel.org/lkml/CAJZ5v0gqr97AFuk855UZkcVpDnmj1Q6B2PE32zWmx4eKxbNvCw@mail.gmail.com/T/
Thanks
Hanjun