CoreSight ETM4x devices could be accessed either via MMIO (handled via
amba_driver) or CPU system instructions (handled via platform driver). But
this has the following issues :
- Each new CPU comes up with its own PID and thus we need to keep on
adding the "known" PIDs to get it working with AMBA driver. While
the ETM4 architecture (and CoreSight architecture) defines way to
identify a device as ETM4. Thus older kernels won't be able to
"discover" a newer CPU, unless we add the PIDs.
- With ACPI, the ETM4x devices have the same HID to identify the device
irrespective of the mode of access. This creates a problem where two
different drivers (both AMBA based driver and platform driver) would
hook into the "HID" and could conflict. e.g., if AMBA driver gets
hold of a non-MMIO device, the probe fails. If we have single driver
hooked into the given "HID", we could handle them seamlessly,
irrespective of the mode of access.
- CoreSight is heavily dependent on the runtime power management. With
ACPI, amba_driver doesn't get us anywhere with handling the power
and thus one need to always turn the power ON to use them. Moving to
platform driver gives us the power management for free.
Due to all of the above, we are moving the MMIO based etm4x devices to be
supported via platform driver. The series makes the existing platform
driver generic to handle both type of the access modes. With that we can
also remove the etm4x amba driver.
Finally, we need a way to make sure the new driver gets control of the
ETM4x device on a DT based system. CoreSight devices have always had the
"arm,primecell" in the compatible list. But the way this is handled
currently in OF code is a bit messy. The ETM4x devices are identified by
"arm,coresight-etm4x". The platform driver can never get a chance to probe
these devices, since the "arm,primecell" takes priority and is hard-coded
in the OF code. We have two options here :
1) Remove the arm,primecell from all DTS. This is fine for "new" kernels
with this change. But, for existing boards, using an older kernel will
break. Thus, is not preferred.
2) Add a white list of "compatibles" where the "priority" of the
"arm,primecell" can be ignored.
The series implements (2) above and applies on 6.3-rc2.
Cc: Steve Clevenger <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Frank Rowand <[email protected]>
Cc: Russell King (Oracle) <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Sudeep Holla <[email protected]>
Cc: Lorenzo Pieralisi <[email protected]>
Cc: Mathieu Poirier <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Mike Leach <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Anshuman Khandual (6):
coresight: etm4x: Allocate and device assign 'struct etmv4_drvdata' earlier
coresight: etm4x: Drop iomem 'base' argument from etm4_probe()
coresight: etm4x: Drop pid argument from etm4_probe()
coresight: etm4x: Change etm4_platform_driver driver for MMIO devices
of/platform: Skip coresight etm4x devices from AMBA bus
coresight: etm4x: Drop the AMBA driver
Suzuki Poulose (1):
coresight: etm4x: Add ACPI support in platform driver
drivers/acpi/acpi_amba.c | 1 -
.../coresight/coresight-etm4x-core.c | 171 ++++++++----------
drivers/hwtracing/coresight/coresight-etm4x.h | 3 +
drivers/of/platform.c | 10 +-
include/linux/coresight.h | 56 ++++++
5 files changed, 143 insertions(+), 98 deletions(-)
--
2.25.1
Allocate and device assign 'struct etmv4_drvdata' earlier during the driver
probe, ensuring that it can be retrieved in power management based runtime
callbacks if required. This will also help in dropping iomem base address
argument from the function etm4_probe() later.
Cc: Mathieu Poirier <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Mike Leach <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Anshuman Khandual <[email protected]>
---
.../coresight/coresight-etm4x-core.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 1ea8f173cca0..10119c223fbe 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -2050,17 +2050,14 @@ static int etm4_add_coresight_dev(struct etm4_init_arg *init_arg)
static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
{
- struct etmv4_drvdata *drvdata;
+ struct etmv4_drvdata *drvdata = dev_get_drvdata(dev);
struct csdev_access access = { 0 };
struct etm4_init_arg init_arg = { 0 };
struct etm4_init_arg *delayed;
- drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
- if (!drvdata)
+ if (WARN_ON(!drvdata))
return -ENOMEM;
- dev_set_drvdata(dev, drvdata);
-
if (pm_save_enable == PARAM_PM_SAVE_FIRMWARE)
pm_save_enable = coresight_loses_context_with_cpu(dev) ?
PARAM_PM_SAVE_SELF_HOSTED : PARAM_PM_SAVE_NEVER;
@@ -2112,6 +2109,7 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
static int etm4_probe_amba(struct amba_device *adev, const struct amba_id *id)
{
+ struct etmv4_drvdata *drvdata;
void __iomem *base;
struct device *dev = &adev->dev;
struct resource *res = &adev->res;
@@ -2122,6 +2120,11 @@ static int etm4_probe_amba(struct amba_device *adev, const struct amba_id *id)
if (IS_ERR(base))
return PTR_ERR(base);
+ drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+ if (!drvdata)
+ return -ENOMEM;
+
+ dev_set_drvdata(dev, drvdata);
ret = etm4_probe(dev, base, id->id);
if (!ret)
pm_runtime_put(&adev->dev);
@@ -2131,8 +2134,14 @@ static int etm4_probe_amba(struct amba_device *adev, const struct amba_id *id)
static int etm4_probe_platform_dev(struct platform_device *pdev)
{
+ struct etmv4_drvdata *drvdata;
int ret;
+ drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
+ if (!drvdata)
+ return -ENOMEM;
+
+ dev_set_drvdata(&pdev->dev, drvdata);
pm_runtime_get_noresume(&pdev->dev);
pm_runtime_set_active(&pdev->dev);
pm_runtime_enable(&pdev->dev);
--
2.25.1
'struct etm4_drvdata' itself can carry the base address before etm4_probe()
gets called. Just drop that redundant argument from etm4_probe().
Cc: Mathieu Poirier <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Mike Leach <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Anshuman Khandual <[email protected]>
---
drivers/hwtracing/coresight/coresight-etm4x-core.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 10119c223fbe..5d77571a8df9 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -2048,7 +2048,7 @@ static int etm4_add_coresight_dev(struct etm4_init_arg *init_arg)
return 0;
}
-static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
+static int etm4_probe(struct device *dev, u32 etm_pid)
{
struct etmv4_drvdata *drvdata = dev_get_drvdata(dev);
struct csdev_access access = { 0 };
@@ -2069,8 +2069,6 @@ static int etm4_probe(struct device *dev, void __iomem *base, u32 etm_pid)
return -ENOMEM;
}
- drvdata->base = base;
-
spin_lock_init(&drvdata->spinlock);
drvdata->cpu = coresight_get_cpu(dev);
@@ -2124,8 +2122,9 @@ static int etm4_probe_amba(struct amba_device *adev, const struct amba_id *id)
if (!drvdata)
return -ENOMEM;
+ drvdata->base = base;
dev_set_drvdata(dev, drvdata);
- ret = etm4_probe(dev, base, id->id);
+ ret = etm4_probe(dev, id->id);
if (!ret)
pm_runtime_put(&adev->dev);
@@ -2141,6 +2140,7 @@ static int etm4_probe_platform_dev(struct platform_device *pdev)
if (!drvdata)
return -ENOMEM;
+ drvdata->base = NULL;
dev_set_drvdata(&pdev->dev, drvdata);
pm_runtime_get_noresume(&pdev->dev);
pm_runtime_set_active(&pdev->dev);
@@ -2151,7 +2151,7 @@ static int etm4_probe_platform_dev(struct platform_device *pdev)
* HW by reading appropriate registers on the HW
* and thus we could skip the PID.
*/
- ret = etm4_probe(&pdev->dev, NULL, 0);
+ ret = etm4_probe(&pdev->dev, 0);
pm_runtime_put(&pdev->dev);
return ret;
--
2.25.1
Coresight device pid can be retrieved from its iomem base address, which is
stored in 'struct etm4x_drvdata'. This drops pid argument from etm4_probe()
and 'struct etm4_init_arg'. Instead etm4_check_arch_features() derives the
coresight device pid with a new helper coresight_get_pid(), right before it
is consumed in etm4_hisi_match_pid().
Cc: Mathieu Poirier <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Mike Leach <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Anshuman Khandual <[email protected]>
---
.../coresight/coresight-etm4x-core.c | 19 +++++++------------
include/linux/coresight.h | 12 ++++++++++++
2 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 5d77571a8df9..a4c138e67920 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -66,7 +66,6 @@ static u64 etm4_get_access_type(struct etmv4_config *config);
static enum cpuhp_state hp_online;
struct etm4_init_arg {
- unsigned int pid;
struct device *dev;
struct csdev_access *csa;
};
@@ -370,8 +369,10 @@ static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
}
static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
- unsigned int id)
+ struct csdev_access *csa)
{
+ unsigned int id = coresight_get_pid(csa);
+
if (etm4_hisi_match_pid(id))
set_bit(ETM4_IMPDEF_HISI_CORE_COMMIT, drvdata->arch_features);
}
@@ -1165,7 +1166,7 @@ static void etm4_init_arch_data(void *info)
etm4_os_unlock_csa(drvdata, csa);
etm4_cs_unlock(drvdata, csa);
- etm4_check_arch_features(drvdata, init_arg->pid);
+ etm4_check_arch_features(drvdata, csa);
/* find all capabilities of the tracing unit */
etmidr0 = etm4x_relaxed_read32(csa, TRCIDR0);
@@ -2048,7 +2049,7 @@ static int etm4_add_coresight_dev(struct etm4_init_arg *init_arg)
return 0;
}
-static int etm4_probe(struct device *dev, u32 etm_pid)
+static int etm4_probe(struct device *dev)
{
struct etmv4_drvdata *drvdata = dev_get_drvdata(dev);
struct csdev_access access = { 0 };
@@ -2077,7 +2078,6 @@ static int etm4_probe(struct device *dev, u32 etm_pid)
init_arg.dev = dev;
init_arg.csa = &access;
- init_arg.pid = etm_pid;
/*
* Serialize against CPUHP callbacks to avoid race condition
@@ -2124,7 +2124,7 @@ static int etm4_probe_amba(struct amba_device *adev, const struct amba_id *id)
drvdata->base = base;
dev_set_drvdata(dev, drvdata);
- ret = etm4_probe(dev, id->id);
+ ret = etm4_probe(dev);
if (!ret)
pm_runtime_put(&adev->dev);
@@ -2146,12 +2146,7 @@ static int etm4_probe_platform_dev(struct platform_device *pdev)
pm_runtime_set_active(&pdev->dev);
pm_runtime_enable(&pdev->dev);
- /*
- * System register based devices could match the
- * HW by reading appropriate registers on the HW
- * and thus we could skip the PID.
- */
- ret = etm4_probe(&pdev->dev, 0);
+ ret = etm4_probe(&pdev->dev);
pm_runtime_put(&pdev->dev);
return ret;
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index f19a47b9bb5a..f85b041ea475 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -370,6 +370,18 @@ static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa,
return csa->read(offset, true, false);
}
+#define CORESIGHT_PIDRn(i) (0xFE0 + ((i) * 4))
+
+static inline u32 coresight_get_pid(struct csdev_access *csa)
+{
+ u32 i, pid = 0;
+
+ for (i = 0; i < 4; i++)
+ pid |= csdev_access_relaxed_read32(csa, CORESIGHT_PIDRn(i)) << (i * 8);
+
+ return pid;
+}
+
static inline u64 csdev_access_relaxed_read_pair(struct csdev_access *csa,
u32 lo_offset, u32 hi_offset)
{
--
2.25.1
This updates existing etm4_platform_driver to accommodate MMIO based device
tree represented coresight etm4x devices along with current sysreg ones. It
first looks for 'apb_clk' clock and tries to enable it via a new helper i.e
coresight_get_enable_apb_pclk(). If 'apb_clock' is not found on the system
as indicated by a return value 'NULL', ignore and proceed further assuming
that platform already has got required clocks enabled. But if the clock is
but could not be enabled, device probe fails with -ENODEV. Similarly iomem
base address is fetched via devm_ioremap_resource() onyl when the platform
has valid 'struct resource'. The probed device is ensured to be a coresight
etm4x, via two new helpers in etm4_init_iomem_access(). This also registers
runtime power management callbacks i.e for suspend and resume operations.
Cc: Mathieu Poirier <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Mike Leach <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Anshuman Khandual <[email protected]>
---
.../coresight/coresight-etm4x-core.c | 62 +++++++++++++++++--
drivers/hwtracing/coresight/coresight-etm4x.h | 3 +
include/linux/coresight.h | 44 +++++++++++++
3 files changed, 105 insertions(+), 4 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index a4c138e67920..60f027e33aa0 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -30,6 +30,7 @@
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
#include <linux/property.h>
+#include <linux/clk/clk-conf.h>
#include <asm/barrier.h>
#include <asm/sections.h>
@@ -1067,12 +1068,24 @@ static bool etm4_init_sysreg_access(struct etmv4_drvdata *drvdata,
return true;
}
+static bool is_etm4x_device(void __iomem *base)
+{
+ u32 devarch = readl(base + TRCDEVARCH);
+ u32 devtype = readl(base + TRCDEVTYPE);
+
+ return (((devarch & ETM_DEVARCH_ID_MASK) == ETM_DEVARCH_ETMv4x_ARCH) &&
+ (devtype == ETM_DEVTYPE_ETMv4x_ARCH));
+}
+
static bool etm4_init_iomem_access(struct etmv4_drvdata *drvdata,
struct csdev_access *csa)
{
u32 devarch = readl_relaxed(drvdata->base + TRCDEVARCH);
u32 idr1 = readl_relaxed(drvdata->base + TRCIDR1);
+ if (!is_coresight_device(drvdata->base) || !is_etm4x_device(drvdata->base))
+ return false;
+
/*
* All ETMs must implement TRCDEVARCH to indicate that
* the component is an ETMv4. To support any broken
@@ -2133,6 +2146,7 @@ static int etm4_probe_amba(struct amba_device *adev, const struct amba_id *id)
static int etm4_probe_platform_dev(struct platform_device *pdev)
{
+ struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
struct etmv4_drvdata *drvdata;
int ret;
@@ -2140,7 +2154,16 @@ static int etm4_probe_platform_dev(struct platform_device *pdev)
if (!drvdata)
return -ENOMEM;
- drvdata->base = NULL;
+ drvdata->pclk = coresight_get_enable_apb_pclk(&pdev->dev);
+ if (IS_ERR(drvdata->pclk))
+ return -ENODEV;
+
+ if (res) {
+ drvdata->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(drvdata->base))
+ return PTR_ERR(drvdata->base);
+ }
+
dev_set_drvdata(&pdev->dev, drvdata);
pm_runtime_get_noresume(&pdev->dev);
pm_runtime_set_active(&pdev->dev);
@@ -2186,7 +2209,7 @@ static struct amba_cs_uci_id uci_id_etm4[] = {
/* ETMv4 UCI data */
.devarch = ETM_DEVARCH_ETMv4x_ARCH,
.devarch_mask = ETM_DEVARCH_ID_MASK,
- .devtype = 0x00000013,
+ .devtype = ETM_DEVTYPE_ETMv4x_ARCH,
}
};
@@ -2244,6 +2267,10 @@ static int __exit etm4_remove_platform_dev(struct platform_device *pdev)
if (drvdata)
ret = etm4_remove_dev(drvdata);
+
+ if (!IS_ERR(drvdata->pclk))
+ clk_put(drvdata->pclk);
+
pm_runtime_disable(&pdev->dev);
return ret;
}
@@ -2284,7 +2311,34 @@ static struct amba_driver etm4x_amba_driver = {
.id_table = etm4_ids,
};
-static const struct of_device_id etm4_sysreg_match[] = {
+#ifdef CONFIG_PM
+static int etm4_runtime_suspend(struct device *dev)
+{
+ struct etmv4_drvdata *drvdata = dev_get_drvdata(dev);
+
+ if (!IS_ERR(drvdata->pclk))
+ clk_disable_unprepare(drvdata->pclk);
+
+ return 0;
+}
+
+static int etm4_runtime_resume(struct device *dev)
+{
+ struct etmv4_drvdata *drvdata = dev_get_drvdata(dev);
+
+ if (!IS_ERR(drvdata->pclk))
+ clk_prepare_enable(drvdata->pclk);
+
+ return 0;
+}
+#endif
+
+static const struct dev_pm_ops etm4_dev_pm_ops = {
+ SET_RUNTIME_PM_OPS(etm4_runtime_suspend, etm4_runtime_resume, NULL)
+};
+
+static const struct of_device_id etm4_match[] = {
+ { .compatible = "arm,coresight-etm4x" },
{ .compatible = "arm,coresight-etm4x-sysreg" },
{ .compatible = "arm,embedded-trace-extension" },
{}
@@ -2295,7 +2349,7 @@ static struct platform_driver etm4_platform_driver = {
.remove = etm4_remove_platform_dev,
.driver = {
.name = "coresight-etm4x",
- .of_match_table = etm4_sysreg_match,
+ .of_match_table = etm4_match,
.suppress_bind_attrs = true,
},
};
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index 434f4e95ee17..5a37df4a02e9 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -701,6 +701,8 @@
#define ETM_DEVARCH_ETE_ARCH \
(ETM_DEVARCH_ARCHITECT_ARM | ETM_DEVARCH_ARCHID_ETE | ETM_DEVARCH_PRESENT)
+#define ETM_DEVTYPE_ETMv4x_ARCH 0x00000013
+
#define TRCSTATR_IDLE_BIT 0
#define TRCSTATR_PMSTABLE_BIT 1
#define ETM_DEFAULT_ADDR_COMP 0
@@ -1017,6 +1019,7 @@ struct etmv4_save_state {
* @arch_features: Bitmap of arch features of etmv4 devices.
*/
struct etmv4_drvdata {
+ struct clk *pclk;
void __iomem *base;
struct coresight_device *csdev;
spinlock_t spinlock;
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index f85b041ea475..75a7aa6d7444 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -6,6 +6,8 @@
#ifndef _LINUX_CORESIGHT_H
#define _LINUX_CORESIGHT_H
+#include <linux/amba/bus.h>
+#include <linux/clk.h>
#include <linux/device.h>
#include <linux/io.h>
#include <linux/perf_event.h>
@@ -370,6 +372,48 @@ static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa,
return csa->read(offset, true, false);
}
+#define CORESIGHT_CIDRn(i) (0xFF0 + ((i) * 4))
+
+static inline u32 coresight_get_cid(void __iomem *base)
+{
+ u32 i, cid = 0;
+
+ for (i = 0; i < 4; i++)
+ cid |= readl(base + CORESIGHT_CIDRn(i)) << (i * 8);
+
+ return cid;
+}
+
+static inline bool is_coresight_device(void __iomem *base)
+{
+ u32 cid = coresight_get_cid(base);
+
+ return cid == CORESIGHT_CID;
+}
+
+/*
+ * This function attempts to find a 'apb_pclk' clock on the system and
+ * if found, enables it. This returns NULL if 'apb_pclk' clock is not
+ * and return error pointer from clk_prepare_enable(), if it fails to
+ * enable the discovered clock.
+ */
+static inline struct clk *coresight_get_enable_apb_pclk(struct device *dev)
+{
+ struct clk *pclk;
+ int ret;
+
+ pclk = clk_get(dev, "apb_pclk");
+ if (IS_ERR(pclk))
+ return NULL;
+
+ ret = clk_prepare_enable(pclk);
+ if (ret) {
+ clk_put(pclk);
+ return ERR_PTR(ret);
+ }
+ return pclk;
+}
+
#define CORESIGHT_PIDRn(i) (0xFE0 + ((i) * 4))
static inline u32 coresight_get_pid(struct csdev_access *csa)
--
2.25.1
From: Suzuki Poulose <[email protected]>
Drop ETM4X ACPI ID from the AMBA ACPI device list, and instead just move it
inside the new ACPI devices list detected and used via platform driver.
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Mathieu Poirier <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Mike Leach <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: Sudeep Holla <[email protected]>
Cc: Lorenzo Pieralisi <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Suzuki Poulose <[email protected]>
Signed-off-by: Anshuman Khandual <[email protected]>
---
drivers/acpi/acpi_amba.c | 1 -
drivers/hwtracing/coresight/coresight-etm4x-core.c | 10 ++++++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/acpi_amba.c b/drivers/acpi/acpi_amba.c
index f5b443ab01c2..099966cbac5a 100644
--- a/drivers/acpi/acpi_amba.c
+++ b/drivers/acpi/acpi_amba.c
@@ -22,7 +22,6 @@
static const struct acpi_device_id amba_id_list[] = {
{"ARMH0061", 0}, /* PL061 GPIO Device */
{"ARMH0330", 0}, /* ARM DMA Controller DMA-330 */
- {"ARMHC500", 0}, /* ARM CoreSight ETM4x */
{"ARMHC501", 0}, /* ARM CoreSight ETR */
{"ARMHC502", 0}, /* ARM CoreSight STM */
{"ARMHC503", 0}, /* ARM CoreSight Debug */
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 60f027e33aa0..fe494c9c6bad 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -3,6 +3,7 @@
* Copyright (c) 2014, The Linux Foundation. All rights reserved.
*/
+#include <linux/acpi.h>
#include <linux/bitops.h>
#include <linux/kernel.h>
#include <linux/moduleparam.h>
@@ -2344,12 +2345,21 @@ static const struct of_device_id etm4_match[] = {
{}
};
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id etm4x_acpi_ids[] = {
+ {"ARMHC500", 0}, /* ARM CoreSight ETM4x */
+ {}
+};
+MODULE_DEVICE_TABLE(acpi, etm4x_acpi_ids);
+#endif
+
static struct platform_driver etm4_platform_driver = {
.probe = etm4_probe_platform_dev,
.remove = etm4_remove_platform_dev,
.driver = {
.name = "coresight-etm4x",
.of_match_table = etm4_match,
+ .acpi_match_table = ACPI_PTR(etm4x_acpi_ids),
.suppress_bind_attrs = true,
},
};
--
2.25.1
Allow other drivers to claim a device, disregarding the "priority" of
"arm,primecell". e.g., CoreSight ETM4x devices could be accessed via MMIO
(AMBA Bus) or via CPU system instructions. The CoreSight ETM4x platform
driver can now handle both types of devices. In order to make sure the
driver gets to handle the "MMIO based" devices, which always had the
"arm,primecell" compatible, we have two options :
1) Remove the "arm,primecell" from the DTS. But this may be problematic
for an older kernel without the support.
2) The other option is to allow OF code to "ignore" the arm,primecell
priority for a selected list of compatibles. This would make sure that
both older kernels and the new kernels work fine without breaking
the functionality. The new DTS could always have the "arm,primecell"
removed.
This patch implements Option (2).
Cc: Rob Herring <[email protected]>
Cc: Frank Rowand <[email protected]>
Cc: Russell King (Oracle) <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
Cc: [email protected]
Co-developed-by: Suzuki Poulose <[email protected]>
Signed-off-by: Suzuki Poulose <[email protected]>
Signed-off-by: Anshuman Khandual <[email protected]>
---
drivers/of/platform.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index b2bd2e783445..59ff1a38ccaa 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -325,6 +325,13 @@ static const struct of_dev_auxdata *of_dev_lookup(const struct of_dev_auxdata *l
return NULL;
}
+static const struct of_device_id of_ignore_amba_table[] = {
+#ifdef CONFIG_CORESIGHT_SOURCE_ETM4X
+ { .compatible = "arm,coresight-etm4x" },
+#endif
+ {} /* NULL terminated */
+};
+
/**
* of_platform_bus_create() - Create a device for a node and its children.
* @bus: device node of the bus to instantiate
@@ -373,7 +380,8 @@ static int of_platform_bus_create(struct device_node *bus,
platform_data = auxdata->platform_data;
}
- if (of_device_is_compatible(bus, "arm,primecell")) {
+ if (of_device_is_compatible(bus, "arm,primecell") &&
+ unlikely(!of_match_node(of_ignore_amba_table, bus))) {
/*
* Don't return an error here to keep compatibility with older
* device tree files.
--
2.25.1
MMIO based etm4x devices, represented with AMBA device tree entries are now
detected and operated via the common platform driver. An explicit dedicated
AMBA driver is no longer required, this just drops that off completely.
Cc: Mathieu Poirier <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Mike Leach <[email protected]>
Cc: Leo Yan <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Anshuman Khandual <[email protected]>
---
.../coresight/coresight-etm4x-core.c | 89 -------------------
1 file changed, 89 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index fe494c9c6bad..e15551355975 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -2119,32 +2119,6 @@ static int etm4_probe(struct device *dev)
return etm4_add_coresight_dev(&init_arg);
}
-static int etm4_probe_amba(struct amba_device *adev, const struct amba_id *id)
-{
- struct etmv4_drvdata *drvdata;
- void __iomem *base;
- struct device *dev = &adev->dev;
- struct resource *res = &adev->res;
- int ret;
-
- /* Validity for the resource is already checked by the AMBA core */
- base = devm_ioremap_resource(dev, res);
- if (IS_ERR(base))
- return PTR_ERR(base);
-
- drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
- if (!drvdata)
- return -ENOMEM;
-
- drvdata->base = base;
- dev_set_drvdata(dev, drvdata);
- ret = etm4_probe(dev);
- if (!ret)
- pm_runtime_put(&adev->dev);
-
- return ret;
-}
-
static int etm4_probe_platform_dev(struct platform_device *pdev)
{
struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -2205,15 +2179,6 @@ static int etm4_probe_cpu(unsigned int cpu)
return 0;
}
-static struct amba_cs_uci_id uci_id_etm4[] = {
- {
- /* ETMv4 UCI data */
- .devarch = ETM_DEVARCH_ETMv4x_ARCH,
- .devarch_mask = ETM_DEVARCH_ID_MASK,
- .devtype = ETM_DEVTYPE_ETMv4x_ARCH,
- }
-};
-
static void clear_etmdrvdata(void *info)
{
int cpu = *(int *)info;
@@ -2253,14 +2218,6 @@ static int __exit etm4_remove_dev(struct etmv4_drvdata *drvdata)
return 0;
}
-static void __exit etm4_remove_amba(struct amba_device *adev)
-{
- struct etmv4_drvdata *drvdata = dev_get_drvdata(&adev->dev);
-
- if (drvdata)
- etm4_remove_dev(drvdata);
-}
-
static int __exit etm4_remove_platform_dev(struct platform_device *pdev)
{
int ret = 0;
@@ -2276,42 +2233,6 @@ static int __exit etm4_remove_platform_dev(struct platform_device *pdev)
return ret;
}
-static const struct amba_id etm4_ids[] = {
- CS_AMBA_ID(0x000bb95d), /* Cortex-A53 */
- CS_AMBA_ID(0x000bb95e), /* Cortex-A57 */
- CS_AMBA_ID(0x000bb95a), /* Cortex-A72 */
- CS_AMBA_ID(0x000bb959), /* Cortex-A73 */
- CS_AMBA_UCI_ID(0x000bb9da, uci_id_etm4),/* Cortex-A35 */
- CS_AMBA_UCI_ID(0x000bbd05, uci_id_etm4),/* Cortex-A55 */
- CS_AMBA_UCI_ID(0x000bbd0a, uci_id_etm4),/* Cortex-A75 */
- CS_AMBA_UCI_ID(0x000bbd0c, uci_id_etm4),/* Neoverse N1 */
- CS_AMBA_UCI_ID(0x000bbd41, uci_id_etm4),/* Cortex-A78 */
- CS_AMBA_UCI_ID(0x000f0205, uci_id_etm4),/* Qualcomm Kryo */
- CS_AMBA_UCI_ID(0x000f0211, uci_id_etm4),/* Qualcomm Kryo */
- CS_AMBA_UCI_ID(0x000bb802, uci_id_etm4),/* Qualcomm Kryo 385 Cortex-A55 */
- CS_AMBA_UCI_ID(0x000bb803, uci_id_etm4),/* Qualcomm Kryo 385 Cortex-A75 */
- CS_AMBA_UCI_ID(0x000bb805, uci_id_etm4),/* Qualcomm Kryo 4XX Cortex-A55 */
- CS_AMBA_UCI_ID(0x000bb804, uci_id_etm4),/* Qualcomm Kryo 4XX Cortex-A76 */
- CS_AMBA_UCI_ID(0x000bbd0d, uci_id_etm4),/* Qualcomm Kryo 5XX Cortex-A77 */
- CS_AMBA_UCI_ID(0x000cc0af, uci_id_etm4),/* Marvell ThunderX2 */
- CS_AMBA_UCI_ID(0x000b6d01, uci_id_etm4),/* HiSilicon-Hip08 */
- CS_AMBA_UCI_ID(0x000b6d02, uci_id_etm4),/* HiSilicon-Hip09 */
- {},
-};
-
-MODULE_DEVICE_TABLE(amba, etm4_ids);
-
-static struct amba_driver etm4x_amba_driver = {
- .drv = {
- .name = "coresight-etm4x",
- .owner = THIS_MODULE,
- .suppress_bind_attrs = true,
- },
- .probe = etm4_probe_amba,
- .remove = etm4_remove_amba,
- .id_table = etm4_ids,
-};
-
#ifdef CONFIG_PM
static int etm4_runtime_suspend(struct device *dev)
{
@@ -2374,27 +2295,17 @@ static int __init etm4x_init(void)
if (ret)
return ret;
- ret = amba_driver_register(&etm4x_amba_driver);
- if (ret) {
- pr_err("Error registering etm4x AMBA driver\n");
- goto clear_pm;
- }
-
ret = platform_driver_register(&etm4_platform_driver);
if (!ret)
return 0;
pr_err("Error registering etm4x platform driver\n");
- amba_driver_unregister(&etm4x_amba_driver);
-
-clear_pm:
etm4_pm_clear();
return ret;
}
static void __exit etm4x_exit(void)
{
- amba_driver_unregister(&etm4x_amba_driver);
platform_driver_unregister(&etm4_platform_driver);
etm4_pm_clear();
}
--
2.25.1
On 17/03/2023 03:04, Anshuman Khandual wrote:
> This updates existing etm4_platform_driver to accommodate MMIO based device
> tree represented coresight etm4x devices along with current sysreg ones. It
> first looks for 'apb_clk' clock and tries to enable it via a new helper i.e
> coresight_get_enable_apb_pclk(). If 'apb_clock' is not found on the system
> as indicated by a return value 'NULL', ignore and proceed further assuming
> that platform already has got required clocks enabled. But if the clock is
> but could not be enabled, device probe fails with -ENODEV. Similarly iomem
> base address is fetched via devm_ioremap_resource() onyl when the platform
> has valid 'struct resource'. The probed device is ensured to be a coresight
> etm4x, via two new helpers in etm4_init_iomem_access(). This also registers
> runtime power management callbacks i.e for suspend and resume operations.
This looks too much detail about the actual implementation of HOW rather
than WHAT.
Could it be something like :
"Add support for handling MMIO based devices via platform driver. We
need to make sure that :
1) The APB clock, if present is enabled at probe and via runtime_pm ops.
2) Use the ETM4x architecture/CoreSight Architecture registers to
identify a device as CoreSight ETM4x, instead of relying a white
list of "Peripheral IDs"
The driver doesn't get to handle the devices yet, until we wire the ACPI
and DT changes to move the devices to be handled via platform driver
than the etm4_amba driver.
"
>
> Cc: Mathieu Poirier <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Mike Leach <[email protected]>
> Cc: Leo Yan <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Anshuman Khandual <[email protected]>
> ---
> .../coresight/coresight-etm4x-core.c | 62 +++++++++++++++++--
> drivers/hwtracing/coresight/coresight-etm4x.h | 3 +
> include/linux/coresight.h | 44 +++++++++++++
> 3 files changed, 105 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index a4c138e67920..60f027e33aa0 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -30,6 +30,7 @@
> #include <linux/platform_device.h>
> #include <linux/pm_runtime.h>
> #include <linux/property.h>
> +#include <linux/clk/clk-conf.h>
>
> #include <asm/barrier.h>
> #include <asm/sections.h>
> @@ -1067,12 +1068,24 @@ static bool etm4_init_sysreg_access(struct etmv4_drvdata *drvdata,
> return true;
> }
>
> +static bool is_etm4x_device(void __iomem *base)
> +{
> + u32 devarch = readl(base + TRCDEVARCH);
> + u32 devtype = readl(base + TRCDEVTYPE);
> +
> + return (((devarch & ETM_DEVARCH_ID_MASK) == ETM_DEVARCH_ETMv4x_ARCH) &&
> + (devtype == ETM_DEVTYPE_ETMv4x_ARCH));
> +}
> +
> static bool etm4_init_iomem_access(struct etmv4_drvdata *drvdata,
> struct csdev_access *csa)
> {
> u32 devarch = readl_relaxed(drvdata->base + TRCDEVARCH);
> u32 idr1 = readl_relaxed(drvdata->base + TRCIDR1);
>
> + if (!is_coresight_device(drvdata->base) || !is_etm4x_device(drvdata->base))
> + return false;
> +
> /*
> * All ETMs must implement TRCDEVARCH to indicate that
> * the component is an ETMv4. To support any broken
Now that we do the is_etm4x_device(), we could the following ^^
TRCDEVARCH check.
> @@ -2133,6 +2146,7 @@ static int etm4_probe_amba(struct amba_device *adev, const struct amba_id *id)
>
> static int etm4_probe_platform_dev(struct platform_device *pdev)
> {
> + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> struct etmv4_drvdata *drvdata;
> int ret;
>
> @@ -2140,7 +2154,16 @@ static int etm4_probe_platform_dev(struct platform_device *pdev)
> if (!drvdata)
> return -ENOMEM;
>
> - drvdata->base = NULL;
> + drvdata->pclk = coresight_get_enable_apb_pclk(&pdev->dev);
> + if (IS_ERR(drvdata->pclk))
> + return -ENODEV;
> +
> + if (res) {
> + drvdata->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(drvdata->base))
> + return PTR_ERR(drvdata->base);
Drop the clock reference ?
> + }
> +
> dev_set_drvdata(&pdev->dev, drvdata);
> pm_runtime_get_noresume(&pdev->dev);
> pm_runtime_set_active(&pdev->dev);
> @@ -2186,7 +2209,7 @@ static struct amba_cs_uci_id uci_id_etm4[] = {
> /* ETMv4 UCI data */
> .devarch = ETM_DEVARCH_ETMv4x_ARCH,
> .devarch_mask = ETM_DEVARCH_ID_MASK,
> - .devtype = 0x00000013,
> + .devtype = ETM_DEVTYPE_ETMv4x_ARCH,
> }
> };
>
> @@ -2244,6 +2267,10 @@ static int __exit etm4_remove_platform_dev(struct platform_device *pdev)
>
> if (drvdata)
> ret = etm4_remove_dev(drvdata);
> +
> + if (!IS_ERR(drvdata->pclk))
> + clk_put(drvdata->pclk);
> +
> pm_runtime_disable(&pdev->dev);
If we re-order the clk_put() after pm_runtime_disable(), we could use a
label to jump here from the ioremap_resource failure.
> return ret;
> }
> @@ -2284,7 +2311,34 @@ static struct amba_driver etm4x_amba_driver = {
> .id_table = etm4_ids,
> };
>
> -static const struct of_device_id etm4_sysreg_match[] = {
> +#ifdef CONFIG_PM
> +static int etm4_runtime_suspend(struct device *dev)
> +{
> + struct etmv4_drvdata *drvdata = dev_get_drvdata(dev);
> +
> + if (!IS_ERR(drvdata->pclk))
> + clk_disable_unprepare(drvdata->pclk);
> +
> + return 0;
> +}
> +
> +static int etm4_runtime_resume(struct device *dev)
> +{
> + struct etmv4_drvdata *drvdata = dev_get_drvdata(dev);
> +
> + if (!IS_ERR(drvdata->pclk))
> + clk_prepare_enable(drvdata->pclk);
> +
> + return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops etm4_dev_pm_ops = {
> + SET_RUNTIME_PM_OPS(etm4_runtime_suspend, etm4_runtime_resume, NULL)
> +};
Where is this hooked in ?
> +
> +static const struct of_device_id etm4_match[] = {
> + { .compatible = "arm,coresight-etm4x" },
> { .compatible = "arm,coresight-etm4x-sysreg" },
> { .compatible = "arm,embedded-trace-extension" },
> {}
> @@ -2295,7 +2349,7 @@ static struct platform_driver etm4_platform_driver = {
> .remove = etm4_remove_platform_dev,
> .driver = {
> .name = "coresight-etm4x",
> - .of_match_table = etm4_sysreg_match,
> + .of_match_table = etm4_match,
> .suppress_bind_attrs = true,
> },
> };
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> index 434f4e95ee17..5a37df4a02e9 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> @@ -701,6 +701,8 @@
> #define ETM_DEVARCH_ETE_ARCH \
> (ETM_DEVARCH_ARCHITECT_ARM | ETM_DEVARCH_ARCHID_ETE | ETM_DEVARCH_PRESENT)
>
> +#define ETM_DEVTYPE_ETMv4x_ARCH 0x00000013
> +
> #define TRCSTATR_IDLE_BIT 0
> #define TRCSTATR_PMSTABLE_BIT 1
> #define ETM_DEFAULT_ADDR_COMP 0
> @@ -1017,6 +1019,7 @@ struct etmv4_save_state {
> * @arch_features: Bitmap of arch features of etmv4 devices.
> */
> struct etmv4_drvdata {
> + struct clk *pclk;
Please document the field, above the structure.
> void __iomem *base;
> struct coresight_device *csdev;
> spinlock_t spinlock;
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index f85b041ea475..75a7aa6d7444 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -6,6 +6,8 @@
> #ifndef _LINUX_CORESIGHT_H
> #define _LINUX_CORESIGHT_H
>
> +#include <linux/amba/bus.h>
> +#include <linux/clk.h>
> #include <linux/device.h>
> #include <linux/io.h>
> #include <linux/perf_event.h>
> @@ -370,6 +372,48 @@ static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa,
> return csa->read(offset, true, false);
> }
>
> +#define CORESIGHT_CIDRn(i) (0xFF0 + ((i) * 4))
> +
> +static inline u32 coresight_get_cid(void __iomem *base)
> +{
> + u32 i, cid = 0;
> +
> + for (i = 0; i < 4; i++)
> + cid |= readl(base + CORESIGHT_CIDRn(i)) << (i * 8);
> +
> + return cid;
> +}
> +
> +static inline bool is_coresight_device(void __iomem *base)
> +{
> + u32 cid = coresight_get_cid(base);
> +
> + return cid == CORESIGHT_CID;
> +}
> +
> +/*
> + * This function attempts to find a 'apb_pclk' clock on the system and
> + * if found, enables it. This returns NULL if 'apb_pclk' clock is not
> + * and return error pointer from clk_prepare_enable(), if it fails to
> + * enable the discovered clock.
minor nit: Easier if it is something like:
Attempt to find and enable "APB clock" for the given device.
Returns:
NULL - No clock found
clk - Clock is found and enabled.
ERROR - Failed to enable the clock.
Suzuki
On 17/03/2023 03:04, Anshuman Khandual wrote:
> From: Suzuki Poulose <[email protected]>
>
> Drop ETM4X ACPI ID from the AMBA ACPI device list, and instead just move it
> inside the new ACPI devices list detected and used via platform driver.
It may be a good idea to add a short summary of why we are doing this ?
Though it is part of the cover letter, it would benefit people looking
at the commit (e.g., maintainers or even in the future).
Suzuki
>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: Mathieu Poirier <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Mike Leach <[email protected]>
> Cc: Leo Yan <[email protected]>
> Cc: Sudeep Holla <[email protected]>
> Cc: Lorenzo Pieralisi <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Suzuki Poulose <[email protected]>
> Signed-off-by: Anshuman Khandual <[email protected]>
> ---
> drivers/acpi/acpi_amba.c | 1 -
> drivers/hwtracing/coresight/coresight-etm4x-core.c | 10 ++++++++++
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/acpi_amba.c b/drivers/acpi/acpi_amba.c
> index f5b443ab01c2..099966cbac5a 100644
> --- a/drivers/acpi/acpi_amba.c
> +++ b/drivers/acpi/acpi_amba.c
> @@ -22,7 +22,6 @@
> static const struct acpi_device_id amba_id_list[] = {
> {"ARMH0061", 0}, /* PL061 GPIO Device */
> {"ARMH0330", 0}, /* ARM DMA Controller DMA-330 */
> - {"ARMHC500", 0}, /* ARM CoreSight ETM4x */
> {"ARMHC501", 0}, /* ARM CoreSight ETR */
> {"ARMHC502", 0}, /* ARM CoreSight STM */
> {"ARMHC503", 0}, /* ARM CoreSight Debug */
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index 60f027e33aa0..fe494c9c6bad 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -3,6 +3,7 @@
> * Copyright (c) 2014, The Linux Foundation. All rights reserved.
> */
>
> +#include <linux/acpi.h>
> #include <linux/bitops.h>
> #include <linux/kernel.h>
> #include <linux/moduleparam.h>
> @@ -2344,12 +2345,21 @@ static const struct of_device_id etm4_match[] = {
> {}
> };
>
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id etm4x_acpi_ids[] = {
> + {"ARMHC500", 0}, /* ARM CoreSight ETM4x */
> + {}
> +};
> +MODULE_DEVICE_TABLE(acpi, etm4x_acpi_ids);
> +#endif
> +
> static struct platform_driver etm4_platform_driver = {
> .probe = etm4_probe_platform_dev,
> .remove = etm4_remove_platform_dev,
> .driver = {
> .name = "coresight-etm4x",
> .of_match_table = etm4_match,
> + .acpi_match_table = ACPI_PTR(etm4x_acpi_ids),
> .suppress_bind_attrs = true,
> },
> };
On Thu, Mar 16, 2023 at 10:06 PM Anshuman Khandual
<[email protected]> wrote:
>
> Allow other drivers to claim a device, disregarding the "priority" of
> "arm,primecell". e.g., CoreSight ETM4x devices could be accessed via MMIO
> (AMBA Bus) or via CPU system instructions.
The OS can pick which one, use both, or this is a system integration
time decision?
> The CoreSight ETM4x platform
> driver can now handle both types of devices. In order to make sure the
> driver gets to handle the "MMIO based" devices, which always had the
> "arm,primecell" compatible, we have two options :
>
> 1) Remove the "arm,primecell" from the DTS. But this may be problematic
> for an older kernel without the support.
>
> 2) The other option is to allow OF code to "ignore" the arm,primecell
> priority for a selected list of compatibles. This would make sure that
> both older kernels and the new kernels work fine without breaking
> the functionality. The new DTS could always have the "arm,primecell"
> removed.
3) Drop patches 6 and 7 and just register as both AMBA and platform
drivers. It's just some extra boilerplate. I would also do different
compatible strings for CPU system instruction version (assuming this
is an integration time decision).
>
> This patch implements Option (2).
>
> Cc: Rob Herring <[email protected]>
> Cc: Frank Rowand <[email protected]>
> Cc: Russell King (Oracle) <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Co-developed-by: Suzuki Poulose <[email protected]>
> Signed-off-by: Suzuki Poulose <[email protected]>
> Signed-off-by: Anshuman Khandual <[email protected]>
> ---
> drivers/of/platform.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index b2bd2e783445..59ff1a38ccaa 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -325,6 +325,13 @@ static const struct of_dev_auxdata *of_dev_lookup(const struct of_dev_auxdata *l
> return NULL;
> }
>
> +static const struct of_device_id of_ignore_amba_table[] = {
> +#ifdef CONFIG_CORESIGHT_SOURCE_ETM4X
> + { .compatible = "arm,coresight-etm4x" },
> +#endif
> + {} /* NULL terminated */
> +};
> +
> /**
> * of_platform_bus_create() - Create a device for a node and its children.
> * @bus: device node of the bus to instantiate
> @@ -373,7 +380,8 @@ static int of_platform_bus_create(struct device_node *bus,
> platform_data = auxdata->platform_data;
> }
>
> - if (of_device_is_compatible(bus, "arm,primecell")) {
> + if (of_device_is_compatible(bus, "arm,primecell") &&
> + unlikely(!of_match_node(of_ignore_amba_table, bus))) {
of_match_node is going to take orders of magnitude longer than any
difference unlikely() would make. Drop it.
> /*
> * Don't return an error here to keep compatibility with older
> * device tree files.
> --
> 2.25.1
>
Hi Rob
Thanks for your response.
On 17/03/2023 14:52, Rob Herring wrote:
> On Thu, Mar 16, 2023 at 10:06 PM Anshuman Khandual
> <[email protected]> wrote:
>>
>> Allow other drivers to claim a device, disregarding the "priority" of
>> "arm,primecell". e.g., CoreSight ETM4x devices could be accessed via MMIO
>> (AMBA Bus) or via CPU system instructions.
>
> The OS can pick which one, use both, or this is a system integration
> time decision?
Not an OS choice. Historically, this has always been MMIO accessed but
with v8.4 TraceFiltering support, CPUs are encouraged to use system
instructions and obsolete MMIO. So, yes, MMIO is still possible but
something that is discouraged and have to be decided at system
integration time.
>
>> The CoreSight ETM4x platform
>> driver can now handle both types of devices. In order to make sure the
>> driver gets to handle the "MMIO based" devices, which always had the
>> "arm,primecell" compatible, we have two options :
>>
>> 1) Remove the "arm,primecell" from the DTS. But this may be problematic
>> for an older kernel without the support.
>>
>> 2) The other option is to allow OF code to "ignore" the arm,primecell
>> priority for a selected list of compatibles. This would make sure that
>> both older kernels and the new kernels work fine without breaking
>> the functionality. The new DTS could always have the "arm,primecell"
>> removed.
>
> 3) Drop patches 6 and 7 and just register as both AMBA and platform
> drivers. It's just some extra boilerplate. I would also do different
> compatible strings for CPU system instruction version (assuming this
> is an integration time decision).
The system instruction (and the reigster layouts) are all part of the
ETMv4/ETE architecture and specific capabilities/features are
discoverable, just like the Arm CPUs. Thus we don't need special
versions within the ETMv4x or ETE minor versions. As of now, we have
one for etm4x and another for ete.
One problem with the AMBA driver in place is having to keep on adding
new PIDs for the CPUs. The other option is to have a blanket mask
for matching the PIDs with AMBA_UCI_ID checks.
>
>>
>> This patch implements Option (2).
>>
>> Cc: Rob Herring <[email protected]>
>> Cc: Frank Rowand <[email protected]>
>> Cc: Russell King (Oracle) <[email protected]>
>> Cc: Greg Kroah-Hartman <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Co-developed-by: Suzuki Poulose <[email protected]>
>> Signed-off-by: Suzuki Poulose <[email protected]>
>> Signed-off-by: Anshuman Khandual <[email protected]>
>> ---
>> drivers/of/platform.c | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index b2bd2e783445..59ff1a38ccaa 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -325,6 +325,13 @@ static const struct of_dev_auxdata *of_dev_lookup(const struct of_dev_auxdata *l
>> return NULL;
>> }
>>
>> +static const struct of_device_id of_ignore_amba_table[] = {
>> +#ifdef CONFIG_CORESIGHT_SOURCE_ETM4X
>> + { .compatible = "arm,coresight-etm4x" },
>> +#endif
>> + {} /* NULL terminated */
>> +};
>> +
>> /**
>> * of_platform_bus_create() - Create a device for a node and its children.
>> * @bus: device node of the bus to instantiate
>> @@ -373,7 +380,8 @@ static int of_platform_bus_create(struct device_node *bus,
>> platform_data = auxdata->platform_data;
>> }
>>
>> - if (of_device_is_compatible(bus, "arm,primecell")) {
>> + if (of_device_is_compatible(bus, "arm,primecell") &&
>> + unlikely(!of_match_node(of_ignore_amba_table, bus))) {
>
> of_match_node is going to take orders of magnitude longer than any
> difference unlikely() would make. Drop it.
Agreed.
Suzuki
>
>> /*
>> * Don't return an error here to keep compatibility with older
>> * device tree files.
>> --
>> 2.25.1
>>
On Fri, Mar 17, 2023 at 11:03 AM Suzuki K Poulose
<[email protected]> wrote:
>
> Hi Rob
>
> Thanks for your response.
>
> On 17/03/2023 14:52, Rob Herring wrote:
> > On Thu, Mar 16, 2023 at 10:06 PM Anshuman Khandual
> > <[email protected]> wrote:
> >>
> >> Allow other drivers to claim a device, disregarding the "priority" of
> >> "arm,primecell". e.g., CoreSight ETM4x devices could be accessed via MMIO
> >> (AMBA Bus) or via CPU system instructions.
> >
> > The OS can pick which one, use both, or this is a system integration
> > time decision?
>
> Not an OS choice. Historically, this has always been MMIO accessed but
> with v8.4 TraceFiltering support, CPUs are encouraged to use system
> instructions and obsolete MMIO. So, yes, MMIO is still possible but
> something that is discouraged and have to be decided at system
> integration time.
>
> >
> >> The CoreSight ETM4x platform
> >> driver can now handle both types of devices. In order to make sure the
> >> driver gets to handle the "MMIO based" devices, which always had the
> >> "arm,primecell" compatible, we have two options :
> >>
> >> 1) Remove the "arm,primecell" from the DTS. But this may be problematic
> >> for an older kernel without the support.
> >>
> >> 2) The other option is to allow OF code to "ignore" the arm,primecell
> >> priority for a selected list of compatibles. This would make sure that
> >> both older kernels and the new kernels work fine without breaking
> >> the functionality. The new DTS could always have the "arm,primecell"
> >> removed.
> >
> > 3) Drop patches 6 and 7 and just register as both AMBA and platform
> > drivers. It's just some extra boilerplate. I would also do different
> > compatible strings for CPU system instruction version (assuming this
> > is an integration time decision).
>
> The system instruction (and the reigster layouts) are all part of the
> ETMv4/ETE architecture and specific capabilities/features are
> discoverable, just like the Arm CPUs. Thus we don't need special
> versions within the ETMv4x or ETE minor versions. As of now, we have
> one for etm4x and another for ete.
I just meant 2 new compatible strings. One each for ETMv4x and ETE,
but different from the 2 existing ones. It is different h/w presented
to the OS, so different compatible.
> One problem with the AMBA driver in place is having to keep on adding
> new PIDs for the CPUs. The other option is to have a blanket mask
> for matching the PIDs with AMBA_UCI_ID checks.
But if MMIO access is discouraged, then new h/w would use the platform
driver(s), not the amba driver, and you won't have to add PIDs.
Rob
Hi Anshuman,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on soc/for-next rafael-pm/linux-next linus/master v6.3-rc2 next-20230317]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Anshuman-Khandual/coresight-etm4x-Allocate-and-device-assign-struct-etmv4_drvdata-earlier/20230317-110755
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230317030501.1811905-4-anshuman.khandual%40arm.com
patch subject: [PATCH 3/7] coresight: etm4x: Drop pid argument from etm4_probe()
config: arm64-randconfig-r021-20230312 (https://download.01.org/0day-ci/archive/20230318/[email protected]/config)
compiler: aarch64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/39e224e4248f0f7b2c59b97c12a12f8343ab900e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Anshuman-Khandual/coresight-etm4x-Allocate-and-device-assign-struct-etmv4_drvdata-earlier/20230317-110755
git checkout 39e224e4248f0f7b2c59b97c12a12f8343ab900e
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/hwtracing/coresight/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
drivers/hwtracing/coresight/coresight-etm4x-core.c: In function 'etm4_init_arch_data':
>> drivers/hwtracing/coresight/coresight-etm4x-core.c:1169:43: warning: passing argument 2 of 'etm4_check_arch_features' makes integer from pointer without a cast [-Wint-conversion]
1169 | etm4_check_arch_features(drvdata, csa);
| ^~~
| |
| struct csdev_access *
drivers/hwtracing/coresight/coresight-etm4x-core.c:389:51: note: expected 'unsigned int' but argument is of type 'struct csdev_access *'
389 | unsigned int id)
| ~~~~~~~~~~~~~^~
vim +/etm4_check_arch_features +1169 drivers/hwtracing/coresight/coresight-etm4x-core.c
1138
1139 static void etm4_init_arch_data(void *info)
1140 {
1141 u32 etmidr0;
1142 u32 etmidr2;
1143 u32 etmidr3;
1144 u32 etmidr4;
1145 u32 etmidr5;
1146 struct etm4_init_arg *init_arg = info;
1147 struct etmv4_drvdata *drvdata;
1148 struct csdev_access *csa;
1149 int i;
1150
1151 drvdata = dev_get_drvdata(init_arg->dev);
1152 csa = init_arg->csa;
1153
1154 /*
1155 * If we are unable to detect the access mechanism,
1156 * or unable to detect the trace unit type, fail
1157 * early.
1158 */
1159 if (!etm4_init_csdev_access(drvdata, csa))
1160 return;
1161
1162 /* Detect the support for OS Lock before we actually use it */
1163 etm_detect_os_lock(drvdata, csa);
1164
1165 /* Make sure all registers are accessible */
1166 etm4_os_unlock_csa(drvdata, csa);
1167 etm4_cs_unlock(drvdata, csa);
1168
> 1169 etm4_check_arch_features(drvdata, csa);
1170
1171 /* find all capabilities of the tracing unit */
1172 etmidr0 = etm4x_relaxed_read32(csa, TRCIDR0);
1173
1174 /* INSTP0, bits[2:1] P0 tracing support field */
1175 drvdata->instrp0 = !!(FIELD_GET(TRCIDR0_INSTP0_MASK, etmidr0) == 0b11);
1176 /* TRCBB, bit[5] Branch broadcast tracing support bit */
1177 drvdata->trcbb = !!(etmidr0 & TRCIDR0_TRCBB);
1178 /* TRCCOND, bit[6] Conditional instruction tracing support bit */
1179 drvdata->trccond = !!(etmidr0 & TRCIDR0_TRCCOND);
1180 /* TRCCCI, bit[7] Cycle counting instruction bit */
1181 drvdata->trccci = !!(etmidr0 & TRCIDR0_TRCCCI);
1182 /* RETSTACK, bit[9] Return stack bit */
1183 drvdata->retstack = !!(etmidr0 & TRCIDR0_RETSTACK);
1184 /* NUMEVENT, bits[11:10] Number of events field */
1185 drvdata->nr_event = FIELD_GET(TRCIDR0_NUMEVENT_MASK, etmidr0);
1186 /* QSUPP, bits[16:15] Q element support field */
1187 drvdata->q_support = FIELD_GET(TRCIDR0_QSUPP_MASK, etmidr0);
1188 /* TSSIZE, bits[28:24] Global timestamp size field */
1189 drvdata->ts_size = FIELD_GET(TRCIDR0_TSSIZE_MASK, etmidr0);
1190
1191 /* maximum size of resources */
1192 etmidr2 = etm4x_relaxed_read32(csa, TRCIDR2);
1193 /* CIDSIZE, bits[9:5] Indicates the Context ID size */
1194 drvdata->ctxid_size = FIELD_GET(TRCIDR2_CIDSIZE_MASK, etmidr2);
1195 /* VMIDSIZE, bits[14:10] Indicates the VMID size */
1196 drvdata->vmid_size = FIELD_GET(TRCIDR2_VMIDSIZE_MASK, etmidr2);
1197 /* CCSIZE, bits[28:25] size of the cycle counter in bits minus 12 */
1198 drvdata->ccsize = FIELD_GET(TRCIDR2_CCSIZE_MASK, etmidr2);
1199
1200 etmidr3 = etm4x_relaxed_read32(csa, TRCIDR3);
1201 /* CCITMIN, bits[11:0] minimum threshold value that can be programmed */
1202 drvdata->ccitmin = FIELD_GET(TRCIDR3_CCITMIN_MASK, etmidr3);
1203 /* EXLEVEL_S, bits[19:16] Secure state instruction tracing */
1204 drvdata->s_ex_level = FIELD_GET(TRCIDR3_EXLEVEL_S_MASK, etmidr3);
1205 drvdata->config.s_ex_level = drvdata->s_ex_level;
1206 /* EXLEVEL_NS, bits[23:20] Non-secure state instruction tracing */
1207 drvdata->ns_ex_level = FIELD_GET(TRCIDR3_EXLEVEL_NS_MASK, etmidr3);
1208 /*
1209 * TRCERR, bit[24] whether a trace unit can trace a
1210 * system error exception.
1211 */
1212 drvdata->trc_error = !!(etmidr3 & TRCIDR3_TRCERR);
1213 /* SYNCPR, bit[25] implementation has a fixed synchronization period? */
1214 drvdata->syncpr = !!(etmidr3 & TRCIDR3_SYNCPR);
1215 /* STALLCTL, bit[26] is stall control implemented? */
1216 drvdata->stallctl = !!(etmidr3 & TRCIDR3_STALLCTL);
1217 /* SYSSTALL, bit[27] implementation can support stall control? */
1218 drvdata->sysstall = !!(etmidr3 & TRCIDR3_SYSSTALL);
1219 /*
1220 * NUMPROC - the number of PEs available for tracing, 5bits
1221 * = TRCIDR3.bits[13:12]bits[30:28]
1222 * bits[4:3] = TRCIDR3.bits[13:12] (since etm-v4.2, otherwise RES0)
1223 * bits[3:0] = TRCIDR3.bits[30:28]
1224 */
1225 drvdata->nr_pe = (FIELD_GET(TRCIDR3_NUMPROC_HI_MASK, etmidr3) << 3) |
1226 FIELD_GET(TRCIDR3_NUMPROC_LO_MASK, etmidr3);
1227 /* NOOVERFLOW, bit[31] is trace overflow prevention supported */
1228 drvdata->nooverflow = !!(etmidr3 & TRCIDR3_NOOVERFLOW);
1229
1230 /* number of resources trace unit supports */
1231 etmidr4 = etm4x_relaxed_read32(csa, TRCIDR4);
1232 /* NUMACPAIRS, bits[0:3] number of addr comparator pairs for tracing */
1233 drvdata->nr_addr_cmp = FIELD_GET(TRCIDR4_NUMACPAIRS_MASK, etmidr4);
1234 /* NUMPC, bits[15:12] number of PE comparator inputs for tracing */
1235 drvdata->nr_pe_cmp = FIELD_GET(TRCIDR4_NUMPC_MASK, etmidr4);
1236 /*
1237 * NUMRSPAIR, bits[19:16]
1238 * The number of resource pairs conveyed by the HW starts at 0, i.e a
1239 * value of 0x0 indicate 1 resource pair, 0x1 indicate two and so on.
1240 * As such add 1 to the value of NUMRSPAIR for a better representation.
1241 *
1242 * For ETM v4.3 and later, 0x0 means 0, and no pairs are available -
1243 * the default TRUE and FALSE resource selectors are omitted.
1244 * Otherwise for values 0x1 and above the number is N + 1 as per v4.2.
1245 */
1246 drvdata->nr_resource = FIELD_GET(TRCIDR4_NUMRSPAIR_MASK, etmidr4);
1247 if ((drvdata->arch < ETM_ARCH_V4_3) || (drvdata->nr_resource > 0))
1248 drvdata->nr_resource += 1;
1249 /*
1250 * NUMSSCC, bits[23:20] the number of single-shot
1251 * comparator control for tracing. Read any status regs as these
1252 * also contain RO capability data.
1253 */
1254 drvdata->nr_ss_cmp = FIELD_GET(TRCIDR4_NUMSSCC_MASK, etmidr4);
1255 for (i = 0; i < drvdata->nr_ss_cmp; i++) {
1256 drvdata->config.ss_status[i] =
1257 etm4x_relaxed_read32(csa, TRCSSCSRn(i));
1258 }
1259 /* NUMCIDC, bits[27:24] number of Context ID comparators for tracing */
1260 drvdata->numcidc = FIELD_GET(TRCIDR4_NUMCIDC_MASK, etmidr4);
1261 /* NUMVMIDC, bits[31:28] number of VMID comparators for tracing */
1262 drvdata->numvmidc = FIELD_GET(TRCIDR4_NUMVMIDC_MASK, etmidr4);
1263
1264 etmidr5 = etm4x_relaxed_read32(csa, TRCIDR5);
1265 /* NUMEXTIN, bits[8:0] number of external inputs implemented */
1266 drvdata->nr_ext_inp = FIELD_GET(TRCIDR5_NUMEXTIN_MASK, etmidr5);
1267 /* TRACEIDSIZE, bits[21:16] indicates the trace ID width */
1268 drvdata->trcid_size = FIELD_GET(TRCIDR5_TRACEIDSIZE_MASK, etmidr5);
1269 /* ATBTRIG, bit[22] implementation can support ATB triggers? */
1270 drvdata->atbtrig = !!(etmidr5 & TRCIDR5_ATBTRIG);
1271 /*
1272 * LPOVERRIDE, bit[23] implementation supports
1273 * low-power state override
1274 */
1275 drvdata->lpoverride = (etmidr5 & TRCIDR5_LPOVERRIDE) && (!drvdata->skip_power_up);
1276 /* NUMSEQSTATE, bits[27:25] number of sequencer states implemented */
1277 drvdata->nrseqstate = FIELD_GET(TRCIDR5_NUMSEQSTATE_MASK, etmidr5);
1278 /* NUMCNTR, bits[30:28] number of counters available for tracing */
1279 drvdata->nr_cntr = FIELD_GET(TRCIDR5_NUMCNTR_MASK, etmidr5);
1280 etm4_cs_lock(drvdata, csa);
1281 cpu_detect_trace_filtering(drvdata);
1282 }
1283
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Anshuman,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on soc/for-next rafael-pm/linux-next linus/master v6.3-rc2 next-20230317]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Anshuman-Khandual/coresight-etm4x-Allocate-and-device-assign-struct-etmv4_drvdata-earlier/20230317-110755
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230317030501.1811905-5-anshuman.khandual%40arm.com
patch subject: [PATCH 4/7] coresight: etm4x: Change etm4_platform_driver driver for MMIO devices
config: arm64-buildonly-randconfig-r002-20230312 (https://download.01.org/0day-ci/archive/20230318/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/f02ad9e7f97ab4fc1f90c7e6399004e9ec89ef26
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Anshuman-Khandual/coresight-etm4x-Allocate-and-device-assign-struct-etmv4_drvdata-earlier/20230317-110755
git checkout f02ad9e7f97ab4fc1f90c7e6399004e9ec89ef26
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/hwtracing/coresight/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/hwtracing/coresight/coresight-etm4x-core.c:2336:32: warning: unused variable 'etm4_dev_pm_ops' [-Wunused-const-variable]
static const struct dev_pm_ops etm4_dev_pm_ops = {
^
1 warning generated.
vim +/etm4_dev_pm_ops +2336 drivers/hwtracing/coresight/coresight-etm4x-core.c
2335
> 2336 static const struct dev_pm_ops etm4_dev_pm_ops = {
2337 SET_RUNTIME_PM_OPS(etm4_runtime_suspend, etm4_runtime_resume, NULL)
2338 };
2339
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On 3/18/23 13:51, kernel test robot wrote:
> Hi Anshuman,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on robh/for-next]
> [also build test WARNING on soc/for-next rafael-pm/linux-next linus/master v6.3-rc2 next-20230317]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Anshuman-Khandual/coresight-etm4x-Allocate-and-device-assign-struct-etmv4_drvdata-earlier/20230317-110755
> base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
> patch link: https://lore.kernel.org/r/20230317030501.1811905-4-anshuman.khandual%40arm.com
> patch subject: [PATCH 3/7] coresight: etm4x: Drop pid argument from etm4_probe()
> config: arm64-randconfig-r021-20230312 (https://download.01.org/0day-ci/archive/20230318/[email protected]/config)
> compiler: aarch64-linux-gcc (GCC) 12.1.0
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # https://github.com/intel-lab-lkp/linux/commit/39e224e4248f0f7b2c59b97c12a12f8343ab900e
> git remote add linux-review https://github.com/intel-lab-lkp/linux
> git fetch --no-tags linux-review Anshuman-Khandual/coresight-etm4x-Allocate-and-device-assign-struct-etmv4_drvdata-earlier/20230317-110755
> git checkout 39e224e4248f0f7b2c59b97c12a12f8343ab900e
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm64 olddefconfig
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/hwtracing/coresight/
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <[email protected]>
> | Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> All warnings (new ones prefixed by >>):
>
> drivers/hwtracing/coresight/coresight-etm4x-core.c: In function 'etm4_init_arch_data':
>>> drivers/hwtracing/coresight/coresight-etm4x-core.c:1169:43: warning: passing argument 2 of 'etm4_check_arch_features' makes integer from pointer without a cast [-Wint-conversion]
> 1169 | etm4_check_arch_features(drvdata, csa);
> | ^~~
> | |
> | struct csdev_access *
> drivers/hwtracing/coresight/coresight-etm4x-core.c:389:51: note: expected 'unsigned int' but argument is of type 'struct csdev_access *'
> 389 | unsigned int id)
> | ~~~~~~~~~~~~~^~
Fallback stub's signature also needs an update. I will fold this change in next series.
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -388,7 +388,7 @@ static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
}
static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
- unsigned int id)
+ struct csdev_access *csa)
{
}
#endif /* CONFIG_ETM4X_IMPDEF_FEATURE */
On 3/18/23 15:54, kernel test robot wrote:
> Hi Anshuman,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on robh/for-next]
> [also build test WARNING on soc/for-next rafael-pm/linux-next linus/master v6.3-rc2 next-20230317]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Anshuman-Khandual/coresight-etm4x-Allocate-and-device-assign-struct-etmv4_drvdata-earlier/20230317-110755
> base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
> patch link: https://lore.kernel.org/r/20230317030501.1811905-5-anshuman.khandual%40arm.com
> patch subject: [PATCH 4/7] coresight: etm4x: Change etm4_platform_driver driver for MMIO devices
> config: arm64-buildonly-randconfig-r002-20230312 (https://download.01.org/0day-ci/archive/20230318/[email protected]/config)
> compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # install arm64 cross compiling tool for clang build
> # apt-get install binutils-aarch64-linux-gnu
> # https://github.com/intel-lab-lkp/linux/commit/f02ad9e7f97ab4fc1f90c7e6399004e9ec89ef26
> git remote add linux-review https://github.com/intel-lab-lkp/linux
> git fetch --no-tags linux-review Anshuman-Khandual/coresight-etm4x-Allocate-and-device-assign-struct-etmv4_drvdata-earlier/20230317-110755
> git checkout f02ad9e7f97ab4fc1f90c7e6399004e9ec89ef26
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 olddefconfig
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/hwtracing/coresight/
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <[email protected]>
> | Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> All warnings (new ones prefixed by >>):
>
>>> drivers/hwtracing/coresight/coresight-etm4x-core.c:2336:32: warning: unused variable 'etm4_dev_pm_ops' [-Wunused-const-variable]
> static const struct dev_pm_ops etm4_dev_pm_ops = {
These pm_ops needs to tagged along with the platform driver.
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 3ce2b4911a49..fe10dd91183e 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -2280,6 +2280,7 @@ static struct platform_driver etm4_platform_driver = {
.of_match_table = etm4_match,
.acpi_match_table = ACPI_PTR(etm4x_acpi_ids),
.suppress_bind_attrs = true,
+ .pm = &etm4_dev_pm_ops,
},
};
On 3/17/23 15:02, Suzuki K Poulose wrote:
> On 17/03/2023 03:04, Anshuman Khandual wrote:
>> This updates existing etm4_platform_driver to accommodate MMIO based device
>> tree represented coresight etm4x devices along with current sysreg ones. It
>> first looks for 'apb_clk' clock and tries to enable it via a new helper i.e
>> coresight_get_enable_apb_pclk(). If 'apb_clock' is not found on the system
>> as indicated by a return value 'NULL', ignore and proceed further assuming
>> that platform already has got required clocks enabled. But if the clock is
>> but could not be enabled, device probe fails with -ENODEV. Similarly iomem
>> base address is fetched via devm_ioremap_resource() onyl when the platform
>> has valid 'struct resource'. The probed device is ensured to be a coresight
>> etm4x, via two new helpers in etm4_init_iomem_access(). This also registers
>> runtime power management callbacks i.e for suspend and resume operations.
>
> This looks too much detail about the actual implementation of HOW rather
> than WHAT.
>
> Could it be something like :
>
> "Add support for handling MMIO based devices via platform driver. We
> need to make sure that :
> 1) The APB clock, if present is enabled at probe and via runtime_pm ops.
> 2) Use the ETM4x architecture/CoreSight Architecture registers to
> identify a device as CoreSight ETM4x, instead of relying a white
> list of "Peripheral IDs"
> The driver doesn't get to handle the devices yet, until we wire the ACPI
> and DT changes to move the devices to be handled via platform driver
> than the etm4_amba driver.> "
Sure, will update the commit message as above.
>
>>
>> Cc: Mathieu Poirier <[email protected]>
>> Cc: Suzuki K Poulose <[email protected]>
>> Cc: Mike Leach <[email protected]>
>> Cc: Leo Yan <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Signed-off-by: Anshuman Khandual <[email protected]>
>> ---
>> .../coresight/coresight-etm4x-core.c | 62 +++++++++++++++++--
>> drivers/hwtracing/coresight/coresight-etm4x.h | 3 +
>> include/linux/coresight.h | 44 +++++++++++++
>> 3 files changed, 105 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index a4c138e67920..60f027e33aa0 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -30,6 +30,7 @@
>> #include <linux/platform_device.h>
>> #include <linux/pm_runtime.h>
>> #include <linux/property.h>
>> +#include <linux/clk/clk-conf.h>
>> #include <asm/barrier.h>
>> #include <asm/sections.h>
>> @@ -1067,12 +1068,24 @@ static bool etm4_init_sysreg_access(struct etmv4_drvdata *drvdata,
>> return true;
>> }
>> +static bool is_etm4x_device(void __iomem *base)
>> +{
>> + u32 devarch = readl(base + TRCDEVARCH);
>> + u32 devtype = readl(base + TRCDEVTYPE);
>> +
>> + return (((devarch & ETM_DEVARCH_ID_MASK) == ETM_DEVARCH_ETMv4x_ARCH) &&
>> + (devtype == ETM_DEVTYPE_ETMv4x_ARCH));
>> +}
>> +
>> static bool etm4_init_iomem_access(struct etmv4_drvdata *drvdata,
>> struct csdev_access *csa)
>> {
>> u32 devarch = readl_relaxed(drvdata->base + TRCDEVARCH);
>> u32 idr1 = readl_relaxed(drvdata->base + TRCIDR1);
>> + if (!is_coresight_device(drvdata->base) || !is_etm4x_device(drvdata->base))
>> + return false;
>> +
>> /*
>> * All ETMs must implement TRCDEVARCH to indicate that
>> * the component is an ETMv4. To support any broken
>
> Now that we do the is_etm4x_device(), we could the following ^^
> TRCDEVARCH check.
In order to keep the change at minimum, is_etm4x_device() has been
changed to assert device type but not the devarch itself, which is
being checked in the existing code.
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1069,13 +1069,11 @@ static bool etm4_init_sysreg_access(struct etmv4_drvdata *drvdata,
return true;
}
-static bool is_etm4x_device(void __iomem *base)
+static bool is_etm4x_devtype(void __iomem *base)
{
- u32 devarch = readl(base + TRCDEVARCH);
u32 devtype = readl(base + TRCDEVTYPE);
- return (((devarch & ETM_DEVARCH_ID_MASK) == ETM_DEVARCH_ETMv4x_ARCH) &&
- (devtype == ETM_DEVTYPE_ETMv4x_ARCH));
+ return (devtype == ETM_DEVTYPE_ETMv4x_ARCH);
}
static bool etm4_init_iomem_access(struct etmv4_drvdata *drvdata,
@@ -1084,7 +1082,7 @@ static bool etm4_init_iomem_access(struct etmv4_drvdata *drvdata,
u32 devarch = readl_relaxed(drvdata->base + TRCDEVARCH);
u32 idr1 = readl_relaxed(drvdata->base + TRCIDR1);
- if (!is_coresight_device(drvdata->base) || !is_etm4x_device(drvdata->base))
+ if (!is_coresight_device(drvdata->base) || !is_etm4x_devtype(drvdata->base))
return false;
/*
>
>> @@ -2133,6 +2146,7 @@ static int etm4_probe_amba(struct amba_device *adev, const struct amba_id *id)
>> static int etm4_probe_platform_dev(struct platform_device *pdev)
>> {
>> + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> struct etmv4_drvdata *drvdata;
>> int ret;
>> @@ -2140,7 +2154,16 @@ static int etm4_probe_platform_dev(struct platform_device *pdev)
>> if (!drvdata)
>> return -ENOMEM;
>> - drvdata->base = NULL;
>> + drvdata->pclk = coresight_get_enable_apb_pclk(&pdev->dev);
>> + if (IS_ERR(drvdata->pclk))
>> + return -ENODEV;
>> +
>> + if (res) {
>> + drvdata->base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(drvdata->base))
>> + return PTR_ERR(drvdata->base);
>
> Drop the clock reference ?
Right, will drop the clock here.
if (res) {
drvdata->base = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(drvdata->base)) {
clk_put(drvdata->pclk);
return PTR_ERR(drvdata->base);
}
}
>
>> + }
>> +
>> dev_set_drvdata(&pdev->dev, drvdata);
>> pm_runtime_get_noresume(&pdev->dev);
>> pm_runtime_set_active(&pdev->dev);
>> @@ -2186,7 +2209,7 @@ static struct amba_cs_uci_id uci_id_etm4[] = {
>> /* ETMv4 UCI data */
>> .devarch = ETM_DEVARCH_ETMv4x_ARCH,
>> .devarch_mask = ETM_DEVARCH_ID_MASK,
>> - .devtype = 0x00000013,
>> + .devtype = ETM_DEVTYPE_ETMv4x_ARCH,
>> }
>> };
>> @@ -2244,6 +2267,10 @@ static int __exit etm4_remove_platform_dev(struct platform_device *pdev)
>> if (drvdata)
>> ret = etm4_remove_dev(drvdata);
>> +
>> + if (!IS_ERR(drvdata->pclk))
>> + clk_put(drvdata->pclk);
>> +
>> pm_runtime_disable(&pdev->dev);
>
> If we re-order the clk_put() after pm_runtime_disable(), we could use a label to jump here from the ioremap_resource failure.
This is in etm4_remove_platform_dev() and there is no ioremap_resource() failure ?
>
>> return ret;
>> }
>> @@ -2284,7 +2311,34 @@ static struct amba_driver etm4x_amba_driver = {
>> .id_table = etm4_ids,
>> };
>> -static const struct of_device_id etm4_sysreg_match[] = {
>> +#ifdef CONFIG_PM
>> +static int etm4_runtime_suspend(struct device *dev)
>> +{
>> + struct etmv4_drvdata *drvdata = dev_get_drvdata(dev);
>> +
>> + if (!IS_ERR(drvdata->pclk))
>> + clk_disable_unprepare(drvdata->pclk);
>> +
>> + return 0;
>> +}
>> +
>> +static int etm4_runtime_resume(struct device *dev)
>> +{
>> + struct etmv4_drvdata *drvdata = dev_get_drvdata(dev);
>> +
>> + if (!IS_ERR(drvdata->pclk))
>> + clk_prepare_enable(drvdata->pclk);
>> +
>> + return 0;
>> +}
>> +#endif
>> +
>> +static const struct dev_pm_ops etm4_dev_pm_ops = {
>> + SET_RUNTIME_PM_OPS(etm4_runtime_suspend, etm4_runtime_resume, NULL)
>> +};
>
> Where is this hooked in ?
These pm_ops needs to tagged with the platform driver.
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index 3ce2b4911a49..fe10dd91183e 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -2280,6 +2280,7 @@ static struct platform_driver etm4_platform_driver = {
.of_match_table = etm4_match,
.acpi_match_table = ACPI_PTR(etm4x_acpi_ids),
.suppress_bind_attrs = true,
+ .pm = &etm4_dev_pm_ops,
},
};
>
>> +
>> +static const struct of_device_id etm4_match[] = {
>> + { .compatible = "arm,coresight-etm4x" },
>> { .compatible = "arm,coresight-etm4x-sysreg" },
>> { .compatible = "arm,embedded-trace-extension" },
>> {}
>> @@ -2295,7 +2349,7 @@ static struct platform_driver etm4_platform_driver = {
>> .remove = etm4_remove_platform_dev,
>> .driver = {
>> .name = "coresight-etm4x",
>> - .of_match_table = etm4_sysreg_match,
>> + .of_match_table = etm4_match,
>> .suppress_bind_attrs = true,
>> },
>> };
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
>> index 434f4e95ee17..5a37df4a02e9 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
>> @@ -701,6 +701,8 @@
>> #define ETM_DEVARCH_ETE_ARCH \
>> (ETM_DEVARCH_ARCHITECT_ARM | ETM_DEVARCH_ARCHID_ETE | ETM_DEVARCH_PRESENT)
>> +#define ETM_DEVTYPE_ETMv4x_ARCH 0x00000013
>> +
>> #define TRCSTATR_IDLE_BIT 0
>> #define TRCSTATR_PMSTABLE_BIT 1
>> #define ETM_DEFAULT_ADDR_COMP 0
>> @@ -1017,6 +1019,7 @@ struct etmv4_save_state {
>> * @arch_features: Bitmap of arch features of etmv4 devices.
>> */
>> struct etmv4_drvdata {
>> + struct clk *pclk;
>
> Please document the field, above the structure.
Will add the following document for the field.
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -954,6 +954,7 @@ struct etmv4_save_state {
/**
* struct etm4_drvdata - specifics associated to an ETM component
+ * @pclk APB clock for this component
* @base: Memory mapped base address for this component.
* @csdev: Component vitals needed by the framework.
* @spinlock: Only one at a time pls.
>
>> void __iomem *base;
>> struct coresight_device *csdev;
>> spinlock_t spinlock;
>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>> index f85b041ea475..75a7aa6d7444 100644
>> --- a/include/linux/coresight.h
>> +++ b/include/linux/coresight.h
>> @@ -6,6 +6,8 @@
>> #ifndef _LINUX_CORESIGHT_H
>> #define _LINUX_CORESIGHT_H
>> +#include <linux/amba/bus.h>
>> +#include <linux/clk.h>
>> #include <linux/device.h>
>> #include <linux/io.h>
>> #include <linux/perf_event.h>
>> @@ -370,6 +372,48 @@ static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa,
>> return csa->read(offset, true, false);
>> }
>> +#define CORESIGHT_CIDRn(i) (0xFF0 + ((i) * 4))
>> +
>> +static inline u32 coresight_get_cid(void __iomem *base)
>> +{
>> + u32 i, cid = 0;
>> +
>> + for (i = 0; i < 4; i++)
>> + cid |= readl(base + CORESIGHT_CIDRn(i)) << (i * 8);
>> +
>> + return cid;
>> +}
>> +
>> +static inline bool is_coresight_device(void __iomem *base)
>> +{
>> + u32 cid = coresight_get_cid(base);
>> +
>> + return cid == CORESIGHT_CID;
>> +}
>> +
>> +/*
>> + * This function attempts to find a 'apb_pclk' clock on the system and
>> + * if found, enables it. This returns NULL if 'apb_pclk' clock is not
>> + * and return error pointer from clk_prepare_enable(), if it fails to
>> + * enable the discovered clock.
>
> minor nit: Easier if it is something like:
>
> Attempt to find and enable "APB clock" for the given device.
>
> Returns:
> NULL - No clock found
> clk - Clock is found and enabled.
> ERROR - Failed to enable the clock.
Sure, will change as follows (slightly reorganized)
/*
* Attempt to find and enable "APB clock" for the given device
*
* Returns:
*
* clk - Clock is found and enabled
* NULL - clock is not found
* ERROR - Clock is found but failed to enable
*/
On 3/17/23 21:33, Suzuki K Poulose wrote:
>>> drivers/of/platform.c | 10 +++++++++-
>>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>> index b2bd2e783445..59ff1a38ccaa 100644
>>> --- a/drivers/of/platform.c
>>> +++ b/drivers/of/platform.c
>>> @@ -325,6 +325,13 @@ static const struct of_dev_auxdata *of_dev_lookup(const struct of_dev_auxdata *l
>>> return NULL;
>>> }
>>>
>>> +static const struct of_device_id of_ignore_amba_table[] = {
>>> +#ifdef CONFIG_CORESIGHT_SOURCE_ETM4X
>>> + { .compatible = "arm,coresight-etm4x" },
>>> +#endif
>>> + {} /* NULL terminated */
>>> +};
>>> +
>>> /**
>>> * of_platform_bus_create() - Create a device for a node and its children.
>>> * @bus: device node of the bus to instantiate
>>> @@ -373,7 +380,8 @@ static int of_platform_bus_create(struct device_node *bus,
>>> platform_data = auxdata->platform_data;
>>> }
>>>
>>> - if (of_device_is_compatible(bus, "arm,primecell")) {
>>> + if (of_device_is_compatible(bus, "arm,primecell") &&
>>> + unlikely(!of_match_node(of_ignore_amba_table, bus))) {
>>
>> of_match_node is going to take orders of magnitude longer than any
>> difference unlikely() would make. Drop it.
>
> Agreed.
Sure, will drop the unlikely() here.
On 17/03/2023 20:06, Rob Herring wrote:
> On Fri, Mar 17, 2023 at 11:03 AM Suzuki K Poulose
> <[email protected]> wrote:
>>
>> Hi Rob
>>
>> Thanks for your response.
>>
>> On 17/03/2023 14:52, Rob Herring wrote:
>>> On Thu, Mar 16, 2023 at 10:06 PM Anshuman Khandual
>>> <[email protected]> wrote:
>>>>
>>>> Allow other drivers to claim a device, disregarding the "priority" of
>>>> "arm,primecell". e.g., CoreSight ETM4x devices could be accessed via MMIO
>>>> (AMBA Bus) or via CPU system instructions.
>>>
>>> The OS can pick which one, use both, or this is a system integration
>>> time decision?
>>
>> Not an OS choice. Historically, this has always been MMIO accessed but
>> with v8.4 TraceFiltering support, CPUs are encouraged to use system
>> instructions and obsolete MMIO. So, yes, MMIO is still possible but
>> something that is discouraged and have to be decided at system
>> integration time.
>>
>>>
>>>> The CoreSight ETM4x platform
>>>> driver can now handle both types of devices. In order to make sure the
>>>> driver gets to handle the "MMIO based" devices, which always had the
>>>> "arm,primecell" compatible, we have two options :
>>>>
>>>> 1) Remove the "arm,primecell" from the DTS. But this may be problematic
>>>> for an older kernel without the support.
>>>>
>>>> 2) The other option is to allow OF code to "ignore" the arm,primecell
>>>> priority for a selected list of compatibles. This would make sure that
>>>> both older kernels and the new kernels work fine without breaking
>>>> the functionality. The new DTS could always have the "arm,primecell"
>>>> removed.
>>>
>>> 3) Drop patches 6 and 7 and just register as both AMBA and platform
>>> drivers. It's just some extra boilerplate. I would also do different
>>> compatible strings for CPU system instruction version (assuming this
>>> is an integration time decision).
>>
>> The system instruction (and the reigster layouts) are all part of the
>> ETMv4/ETE architecture and specific capabilities/features are
>> discoverable, just like the Arm CPUs. Thus we don't need special
>> versions within the ETMv4x or ETE minor versions. As of now, we have
>> one for etm4x and another for ete.
>
> I just meant 2 new compatible strings. One each for ETMv4x and ETE,
> but different from the 2 existing ones. It is different h/w presented
> to the OS, so different compatible.
>
Sorry, was not very clear here.
Right now, we have :
1) arm,coresight-etm4x && arm,primecell - For AMBA based devices
2) arm,coresight-etm4x-sysreg - For system instruction based
3) arm,embedded-trace-extension - For ETE
>> One problem with the AMBA driver in place is having to keep on adding
>> new PIDs for the CPUs. The other option is to have a blanket mask
>> for matching the PIDs with AMBA_UCI_ID checks.
>
> But if MMIO access is discouraged, then new h/w would use the platform
> driver(s), not the amba driver, and you won't have to add PIDs.
Yes for v8.4 onwards. Alternatively, the newer DTS could skip
arm,primecell in the entry and that would kick the platform driver
in. So, that should be fine I guess.
Kind regards
Suzuki
>
> Rob
On Mon, Mar 20, 2023 at 5:37 AM Suzuki K Poulose <[email protected]> wrote:
>
>
> On 17/03/2023 20:06, Rob Herring wrote:
> > On Fri, Mar 17, 2023 at 11:03 AM Suzuki K Poulose
> > <[email protected]> wrote:
> >>
> >> Hi Rob
> >>
> >> Thanks for your response.
> >>
> >> On 17/03/2023 14:52, Rob Herring wrote:
> >>> On Thu, Mar 16, 2023 at 10:06 PM Anshuman Khandual
> >>> <[email protected]> wrote:
> >>>>
> >>>> Allow other drivers to claim a device, disregarding the "priority" of
> >>>> "arm,primecell". e.g., CoreSight ETM4x devices could be accessed via MMIO
> >>>> (AMBA Bus) or via CPU system instructions.
> >>>
> >>> The OS can pick which one, use both, or this is a system integration
> >>> time decision?
> >>
> >> Not an OS choice. Historically, this has always been MMIO accessed but
> >> with v8.4 TraceFiltering support, CPUs are encouraged to use system
> >> instructions and obsolete MMIO. So, yes, MMIO is still possible but
> >> something that is discouraged and have to be decided at system
> >> integration time.
> >>
> >>>
> >>>> The CoreSight ETM4x platform
> >>>> driver can now handle both types of devices. In order to make sure the
> >>>> driver gets to handle the "MMIO based" devices, which always had the
> >>>> "arm,primecell" compatible, we have two options :
> >>>>
> >>>> 1) Remove the "arm,primecell" from the DTS. But this may be problematic
> >>>> for an older kernel without the support.
> >>>>
> >>>> 2) The other option is to allow OF code to "ignore" the arm,primecell
> >>>> priority for a selected list of compatibles. This would make sure that
> >>>> both older kernels and the new kernels work fine without breaking
> >>>> the functionality. The new DTS could always have the "arm,primecell"
> >>>> removed.
> >>>
> >>> 3) Drop patches 6 and 7 and just register as both AMBA and platform
> >>> drivers. It's just some extra boilerplate. I would also do different
> >>> compatible strings for CPU system instruction version (assuming this
> >>> is an integration time decision).
> >>
> >> The system instruction (and the reigster layouts) are all part of the
> >> ETMv4/ETE architecture and specific capabilities/features are
> >> discoverable, just like the Arm CPUs. Thus we don't need special
> >> versions within the ETMv4x or ETE minor versions. As of now, we have
> >> one for etm4x and another for ete.
> >
> > I just meant 2 new compatible strings. One each for ETMv4x and ETE,
> > but different from the 2 existing ones. It is different h/w presented
> > to the OS, so different compatible.
> >
>
> Sorry, was not very clear here.
>
> Right now, we have :
>
> 1) arm,coresight-etm4x && arm,primecell - For AMBA based devices
> 2) arm,coresight-etm4x-sysreg - For system instruction based
> 3) arm,embedded-trace-extension - For ETE
Ah, so we already have that...
>
>
> >> One problem with the AMBA driver in place is having to keep on adding
> >> new PIDs for the CPUs. The other option is to have a blanket mask
> >> for matching the PIDs with AMBA_UCI_ID checks.
> >
> > But if MMIO access is discouraged, then new h/w would use the platform
> > driver(s), not the amba driver, and you won't have to add PIDs.
>
> Yes for v8.4 onwards. Alternatively, the newer DTS could skip
> arm,primecell in the entry and that would kick the platform driver
> in. So, that should be fine I guess.
Except that the new dts would not work with older kernels.
I'm now lost as to what problem this series is trying to solve. Will
reply further on cover letter...
Rob
On Thu, Mar 16, 2023 at 10:05 PM Anshuman Khandual
<[email protected]> wrote:
>
> CoreSight ETM4x devices could be accessed either via MMIO (handled via
> amba_driver) or CPU system instructions (handled via platform driver). But
> this has the following issues :
>
> - Each new CPU comes up with its own PID and thus we need to keep on
> adding the "known" PIDs to get it working with AMBA driver. While
> the ETM4 architecture (and CoreSight architecture) defines way to
> identify a device as ETM4. Thus older kernels won't be able to
> "discover" a newer CPU, unless we add the PIDs.
But v8.4 discourages MMIO access, so this problem will go away on its
own. Even if not, adding IDs to stable kernels is standard practice
whether it is PCI VID/PID, compatible string or AMBA PID.
> - With ACPI, the ETM4x devices have the same HID to identify the device
> irrespective of the mode of access. This creates a problem where two
> different drivers (both AMBA based driver and platform driver) would
> hook into the "HID" and could conflict. e.g., if AMBA driver gets
> hold of a non-MMIO device, the probe fails. If we have single driver
> hooked into the given "HID", we could handle them seamlessly,
> irrespective of the mode of access.
Why are we changing DT for ACPI? Just always use the platform driver
for ACPI and leave DT systems alone.
> - CoreSight is heavily dependent on the runtime power management. With
> ACPI, amba_driver doesn't get us anywhere with handling the power
> and thus one need to always turn the power ON to use them. Moving to
> platform driver gives us the power management for free.
This sounds like an issue for any amba driver. If this is an issue,
solve it for everyone, not just work around it in one driver.
When someone puts another primecell device into an ACPI system, are we
going to go do the same one-off change in that driver too? (We kind of
already did with SBSA UART...)
Rob
On 20/03/2023 14:17, Rob Herring wrote:
> On Thu, Mar 16, 2023 at 10:05 PM Anshuman Khandual
> <[email protected]> wrote:
>>
>> CoreSight ETM4x devices could be accessed either via MMIO (handled via
>> amba_driver) or CPU system instructions (handled via platform driver). But
>> this has the following issues :
>>
>> - Each new CPU comes up with its own PID and thus we need to keep on
>> adding the "known" PIDs to get it working with AMBA driver. While
>> the ETM4 architecture (and CoreSight architecture) defines way to
>> identify a device as ETM4. Thus older kernels won't be able to
>> "discover" a newer CPU, unless we add the PIDs.
>
> But v8.4 discourages MMIO access, so this problem will go away on its
> own. Even if not, adding IDs to stable kernels is standard practice
> whether it is PCI VID/PID, compatible string or AMBA PID.
Yes, it would eventually go away. As for adding the PIDs, the
fundamental issue is, unlike other drivers, except for the "PIDs"
everything else is architected and each CPU has this PID alone
different and we have plenty of CPUs implementaions out there.
But all that said, since we added this as an AMBA driver in the first
place (all for simply getting the apb_clk management), I am happy to
choose the "Add PIDs to stable kernel approach" for this problem.
>
>> - With ACPI, the ETM4x devices have the same HID to identify the device
>> irrespective of the mode of access. This creates a problem where two
>> different drivers (both AMBA based driver and platform driver) would
>> hook into the "HID" and could conflict. e.g., if AMBA driver gets
>> hold of a non-MMIO device, the probe fails. If we have single driver
>> hooked into the given "HID", we could handle them seamlessly,
>> irrespective of the mode of access.
>
> Why are we changing DT for ACPI? Just always use the platform driver
> for ACPI and leave DT systems alone.
This was mainly due to (1), given we have a platform driver anyway for
ACPI. As mentioned above, we could leave the DT alone.
>
>> - CoreSight is heavily dependent on the runtime power management. With
>> ACPI, amba_driver doesn't get us anywhere with handling the power
>> and thus one need to always turn the power ON to use them. Moving to
>> platform driver gives us the power management for free.
>
> This sounds like an issue for any amba driver. If this is an issue,
> solve it for everyone, not just work around it in one driver.
This alone wouldn't be sufficient. We need a platform driver anyway to
handle the two different modes in ACPI for ETMs. But this will be a
an option for the other CoreSight components which are always MMIO.
Thanks
Suzuki
>
> When someone puts another primecell device into an ACPI system, are we
> going to go do the same one-off change in that driver too? (We kind of
> already did with SBSA UART...)
>
> Rob
On Mon, Mar 20, 2023 at 09:17:16AM -0500, Rob Herring wrote:
>
> This sounds like an issue for any amba driver. If this is an issue,
> solve it for everyone, not just work around it in one driver.
>
Well it is an issue in general for power management. ACPI has specific
methods that can be executed for entering specific states.
The way AMBA was glue into ACPI bus scan IMO was a hack and PM wasn't
considered at the time. It was just hack to get AMBA drivers to work
with ACPI without any consideration about runtime PM or any methods that
comes as part of ACPI device. There is even some dummy clock handler to
deal with AMBA requesting APB clocks. AMBA device is added as companion
to the ACPI device created as part of the normal bus scan in ACPI which
adds its own PM callbacks and rely on clocks and power domains independent
of the ACPI standard methods(_ON/_OFF).
The default enumeration adds platform devices which adds no extra PM
callbacks and allows normal acpi_device probe flow.
> When someone puts another primecell device into an ACPI system, are we
> going to go do the same one-off change in that driver too? (We kind of
> already did with SBSA UART...)
>
I would prefer to move all the existing users of ACPI + AMBA to move away
from it and just use platform device. This list is not big today, bunch
of coresight, PL061/GPIO and PL330/DMA. And all these are assumed to be
working or actually working if there is no need for any power management.
E.g. on juno coresight needs PM to turn on before probing and AMBA fails
as dummy clocks are added but no power domains attached as ACPI doesn't
need deal with power domains in the OSPM if it is all well abstracted in
methods like _ON/_OFF. They are dealt with explicit power domain in the
DT which needs to be turned on and AMBA relies on that.
One possible further hacky solution is to add dummy genpd to satisfy AMBA
but not sure if we can guarantee ordering between ACPI device calling ON
and its companion AMBA device probing so that the power domain is ON before
AMBA uses the dummy clock and power domains in its pm callback hooks.
Even the UART would fail if it needed any PM methods, we just don't happen
to need that for SBSA and may be we could have made it work as amba device
(can't recollect the exact reason for not doing so now).
--
Regards,
Sudeep
On Tue, Mar 21, 2023 at 9:34 AM Sudeep Holla <[email protected]> wrote:
>
> On Mon, Mar 20, 2023 at 09:17:16AM -0500, Rob Herring wrote:
> >
> > This sounds like an issue for any amba driver. If this is an issue,
> > solve it for everyone, not just work around it in one driver.
> >
>
> Well it is an issue in general for power management. ACPI has specific
> methods that can be executed for entering specific states.
>
> The way AMBA was glue into ACPI bus scan IMO was a hack and PM wasn't
> considered at the time. It was just hack to get AMBA drivers to work
> with ACPI without any consideration about runtime PM or any methods that
> comes as part of ACPI device. There is even some dummy clock handler to
> deal with AMBA requesting APB clocks. AMBA device is added as companion
> to the ACPI device created as part of the normal bus scan in ACPI which
> adds its own PM callbacks and rely on clocks and power domains independent
> of the ACPI standard methods(_ON/_OFF).
I thought only DT had hacks... ;)
> The default enumeration adds platform devices which adds no extra PM
> callbacks and allows normal acpi_device probe flow.
>
> > When someone puts another primecell device into an ACPI system, are we
> > going to go do the same one-off change in that driver too? (We kind of
> > already did with SBSA UART...)
> >
>
> I would prefer to move all the existing users of ACPI + AMBA to move away
> from it and just use platform device. This list is not big today, bunch
> of coresight, PL061/GPIO and PL330/DMA. And all these are assumed to be
> working or actually working if there is no need for any power management.
> E.g. on juno coresight needs PM to turn on before probing and AMBA fails
> as dummy clocks are added but no power domains attached as ACPI doesn't
> need deal with power domains in the OSPM if it is all well abstracted in
> methods like _ON/_OFF. They are dealt with explicit power domain in the
> DT which needs to be turned on and AMBA relies on that.
>
> One possible further hacky solution is to add dummy genpd to satisfy AMBA
> but not sure if we can guarantee ordering between ACPI device calling ON
> and its companion AMBA device probing so that the power domain is ON before
> AMBA uses the dummy clock and power domains in its pm callback hooks.
What if we made AMBA skip its usual matching by ID and only use
DT/ACPI style matching? We have specific compatibles, but they have
never been used by the kernel. The only reason the bus code needs to
do PM is reading the IDs which could be pushed into the drivers that
need to match on specific IDs (I suspect we have some where the
compatible is not specific enough (old ST stuff)).
Looks like we only have 2 platforms left not using DT:
arch/arm/mach-ep93xx/core.c: amba_device_register(&uart1_device,
&iomem_resource);
arch/arm/mach-ep93xx/core.c: amba_device_register(&uart2_device,
&iomem_resource);
arch/arm/mach-ep93xx/core.c: amba_device_register(&uart3_device,
&iomem_resource);
arch/arm/mach-s3c/pl080.c:
amba_device_register(&s3c64xx_dma0_device, &iomem_resource);
arch/arm/mach-s3c/pl080.c:
amba_device_register(&s3c64xx_dma1_device, &iomem_resource);
Get rid of these cases and we don't have to worry about non-DT or ACPI matching.
> Even the UART would fail if it needed any PM methods, we just don't happen
> to need that for SBSA and may be we could have made it work as amba device
> (can't recollect the exact reason for not doing so now).
SBSA doesn't require ID registers. SBSA UART is a "great" example of
none of the existing 2 standards work, so let's create a 3rd.
Rob