The CPU power management issue in the CTI driver was first observed in series [1]; this series resolves that issue. It fixes bugs and removes CPU PM operations from the CoreSight CTI driver, the goal is to use the CoreSight core layer as the central place for CPU power management. Removing CPU PM from CTI driver can avoid conflicts with the core layer.
This series can be divided into:
Patches 01 ~ 02: Fix spinlock with irqsave and register read with CS lock. Patches 03 ~ 08: Access ASICCTL condintioanlly, remove CPU PM code, and refactor register access in sysfs knob.
This series is based on coresight-next branch and has been validated on Juno r2 platforms, pass normal sysfs and perf test, as well as CPU PM stress testing.
[1] https://lore.kernel.org/all/20250915-arm_coresight_power_management_fix-v3-0...
Signed-off-by: Leo Yan leo.yan@arm.com --- Changes in v2: - Rebased on coresight-next branch (v7.1). - Kept read/write cache value in sysfs knob (Mike). - Link to v1: https://lore.kernel.org/r/20260209-arm_coresight_cti_refactor_v1-v1-0-db71ab...
--- Leo Yan (8): coresight: cti: Make spinlock usage consistent coresight: cti: Fix register reads coresight: cti: Access ASICCTL only when implemented coresight: cti: Remove CPU power management code coresight: cti: Rename cti_active() to cti_is_active() coresight: cti: Remove hw_powered flag coresight: cti: Remove hw_enabled flag coresight: cti: Properly handle negative offsets in cti_reg32_{show|store}()
drivers/hwtracing/coresight/coresight-cti-core.c | 278 ++++------------------ drivers/hwtracing/coresight/coresight-cti-sysfs.c | 171 ++++++------- drivers/hwtracing/coresight/coresight-cti.h | 13 +- 3 files changed, 137 insertions(+), 325 deletions(-) --- base-commit: eef33a7cce239783d0422526a4d786289a936f1b change-id: 20251223-arm_coresight_cti_refactor_v1-76e1bda8b716
Best regards,
The spinlock is acquired sometimes with IRQs disabled and sometimes without. This leads to inconsistent semantics: the lock can be either HARDIRQ-safe or HARDIRQ-unsafe, which may trigger lockdep complaints.
Make spinlock usage consistent by acquiring it with disabling IRQs. It is possible for sysfs knobs to acquire the spinlock for accessing a CTI device, while at the same time a perf session sends an IPI to enable the same CTI device. In this case, the spinlock must be IRQ-safe, which is why all lock acquisitions are changed to disable IRQs.
Use guard() and scoped_guard() for spinlock to tidy up the code.
Fixes: 984f37efa385 ("coresight: cti: Write regsiters directly in cti_enable_hw()") Tested-by: James Clark james.clark@linaro.org Reviewed-by: Mike Leach mike.leach@arm.com Signed-off-by: Leo Yan leo.yan@arm.com --- drivers/hwtracing/coresight/coresight-cti-core.c | 74 ++++------- drivers/hwtracing/coresight/coresight-cti-sysfs.c | 145 ++++++++++++---------- 2 files changed, 103 insertions(+), 116 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c index fddc8f31b91dccba1506aae8c2fd5f348240b98b..e3c98d89c987c10faafa02d1dbd721cfd4187edb 100644 --- a/drivers/hwtracing/coresight/coresight-cti-core.c +++ b/drivers/hwtracing/coresight/coresight-cti-core.c @@ -81,10 +81,9 @@ void cti_write_all_hw_regs(struct cti_drvdata *drvdata) static int cti_enable_hw(struct cti_drvdata *drvdata) { struct cti_config *config = &drvdata->config; - unsigned long flags; - int rc = 0; + int rc;
- raw_spin_lock_irqsave(&drvdata->spinlock, flags); + guard(raw_spinlock_irqsave)(&drvdata->spinlock);
/* no need to do anything if enabled or unpowered*/ if (config->hw_enabled || !config->hw_powered) @@ -93,22 +92,15 @@ static int cti_enable_hw(struct cti_drvdata *drvdata) /* claim the device */ rc = coresight_claim_device(drvdata->csdev); if (rc) - goto cti_err_not_enabled; + return rc;
cti_write_all_hw_regs(drvdata);
config->hw_enabled = true; - drvdata->config.enable_req_count++; - raw_spin_unlock_irqrestore(&drvdata->spinlock, flags); - return rc;
cti_state_unchanged: drvdata->config.enable_req_count++; - - /* cannot enable due to error */ -cti_err_not_enabled: - raw_spin_unlock_irqrestore(&drvdata->spinlock, flags); - return rc; + return 0; }
/* re-enable CTI on CPU when using CPU hotplug */ @@ -116,25 +108,21 @@ static void cti_cpuhp_enable_hw(struct cti_drvdata *drvdata) { struct cti_config *config = &drvdata->config;
- raw_spin_lock(&drvdata->spinlock); + guard(raw_spinlock_irqsave)(&drvdata->spinlock); + config->hw_powered = true;
/* no need to do anything if no enable request */ if (!drvdata->config.enable_req_count) - goto cti_hp_not_enabled; + return;
/* try to claim the device */ if (coresight_claim_device(drvdata->csdev)) - goto cti_hp_not_enabled; + return;
cti_write_all_hw_regs(drvdata); config->hw_enabled = true; - raw_spin_unlock(&drvdata->spinlock); return; - - /* did not re-enable due to no claim / no request */ -cti_hp_not_enabled: - raw_spin_unlock(&drvdata->spinlock); }
/* disable hardware */ @@ -142,23 +130,20 @@ static int cti_disable_hw(struct cti_drvdata *drvdata) { struct cti_config *config = &drvdata->config; struct coresight_device *csdev = drvdata->csdev; - int ret = 0;
- raw_spin_lock(&drvdata->spinlock); + guard(raw_spinlock_irqsave)(&drvdata->spinlock);
/* don't allow negative refcounts, return an error */ - if (!drvdata->config.enable_req_count) { - ret = -EINVAL; - goto cti_not_disabled; - } + if (!drvdata->config.enable_req_count) + return -EINVAL;
/* check refcount - disable on 0 */ if (--drvdata->config.enable_req_count > 0) - goto cti_not_disabled; + return 0;
/* no need to do anything if disabled or cpu unpowered */ if (!config->hw_enabled || !config->hw_powered) - goto cti_not_disabled; + return 0;
CS_UNLOCK(drvdata->base);
@@ -168,13 +153,7 @@ static int cti_disable_hw(struct cti_drvdata *drvdata)
coresight_disclaim_device_unlocked(csdev); CS_LOCK(drvdata->base); - raw_spin_unlock(&drvdata->spinlock); - return ret; - - /* not disabled this call */ -cti_not_disabled: - raw_spin_unlock(&drvdata->spinlock); - return ret; + return 0; }
void cti_write_single_reg(struct cti_drvdata *drvdata, int offset, u32 value) @@ -189,11 +168,11 @@ void cti_write_intack(struct device *dev, u32 ackval) struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); struct cti_config *config = &drvdata->config;
- raw_spin_lock(&drvdata->spinlock); + guard(raw_spinlock_irqsave)(&drvdata->spinlock); + /* write if enabled */ if (cti_active(config)) cti_write_single_reg(drvdata, CTIINTACK, ackval); - raw_spin_unlock(&drvdata->spinlock); }
/* @@ -360,7 +339,7 @@ int cti_channel_trig_op(struct device *dev, enum cti_chan_op op, reg_offset = (direction == CTI_TRIG_IN ? CTIINEN(trigger_idx) : CTIOUTEN(trigger_idx));
- raw_spin_lock(&drvdata->spinlock); + guard(raw_spinlock_irqsave)(&drvdata->spinlock);
/* read - modify write - the trigger / channel enable value */ reg_value = direction == CTI_TRIG_IN ? config->ctiinen[trigger_idx] : @@ -379,7 +358,7 @@ int cti_channel_trig_op(struct device *dev, enum cti_chan_op op, /* write through if enabled */ if (cti_active(config)) cti_write_single_reg(drvdata, reg_offset, reg_value); - raw_spin_unlock(&drvdata->spinlock); + return 0; }
@@ -397,7 +376,8 @@ int cti_channel_gate_op(struct device *dev, enum cti_chan_gate_op op,
chan_bitmask = BIT(channel_idx);
- raw_spin_lock(&drvdata->spinlock); + guard(raw_spinlock_irqsave)(&drvdata->spinlock); + reg_value = config->ctigate; switch (op) { case CTI_GATE_CHAN_ENABLE: @@ -417,7 +397,7 @@ int cti_channel_gate_op(struct device *dev, enum cti_chan_gate_op op, if (cti_active(config)) cti_write_single_reg(drvdata, CTIGATE, reg_value); } - raw_spin_unlock(&drvdata->spinlock); + return err; }
@@ -436,7 +416,8 @@ int cti_channel_setop(struct device *dev, enum cti_chan_set_op op,
chan_bitmask = BIT(channel_idx);
- raw_spin_lock(&drvdata->spinlock); + guard(raw_spinlock_irqsave)(&drvdata->spinlock); + reg_value = config->ctiappset; switch (op) { case CTI_CHAN_SET: @@ -464,7 +445,6 @@ int cti_channel_setop(struct device *dev, enum cti_chan_set_op op,
if ((err == 0) && cti_active(config)) cti_write_single_reg(drvdata, reg_offset, reg_value); - raw_spin_unlock(&drvdata->spinlock);
return err; } @@ -667,7 +647,7 @@ static int cti_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd, if (WARN_ON_ONCE(drvdata->ctidev.cpu != cpu)) return NOTIFY_BAD;
- raw_spin_lock(&drvdata->spinlock); + guard(raw_spinlock_irqsave)(&drvdata->spinlock);
switch (cmd) { case CPU_PM_ENTER: @@ -707,7 +687,6 @@ static int cti_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd, }
cti_notify_exit: - raw_spin_unlock(&drvdata->spinlock); return notify_res; }
@@ -734,11 +713,12 @@ static int cti_dying_cpu(unsigned int cpu) if (!drvdata) return 0;
- raw_spin_lock(&drvdata->spinlock); + guard(raw_spinlock_irqsave)(&drvdata->spinlock); + drvdata->config.hw_powered = false; if (drvdata->config.hw_enabled) coresight_disclaim_device(drvdata->csdev); - raw_spin_unlock(&drvdata->spinlock); + return 0; }
diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c index 572b80ee96fbf18ec8cf9abc30d109a676dfbc5d..455d08bcccd49a3f1eac8abd8246806ef73a9ab6 100644 --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c @@ -84,11 +84,11 @@ static ssize_t enable_show(struct device *dev, bool enabled, powered; struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
- raw_spin_lock(&drvdata->spinlock); - enable_req = drvdata->config.enable_req_count; - powered = drvdata->config.hw_powered; - enabled = drvdata->config.hw_enabled; - raw_spin_unlock(&drvdata->spinlock); + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) { + enable_req = drvdata->config.enable_req_count; + powered = drvdata->config.hw_powered; + enabled = drvdata->config.hw_enabled; + }
if (powered) return sprintf(buf, "%d\n", enabled); @@ -134,9 +134,8 @@ static ssize_t powered_show(struct device *dev, bool powered; struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
- raw_spin_lock(&drvdata->spinlock); - powered = drvdata->config.hw_powered; - raw_spin_unlock(&drvdata->spinlock); + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) + powered = drvdata->config.hw_powered;
return sprintf(buf, "%d\n", powered); } @@ -181,10 +180,12 @@ static ssize_t coresight_cti_reg_show(struct device *dev, u32 val = 0;
pm_runtime_get_sync(dev->parent); - raw_spin_lock(&drvdata->spinlock); - if (drvdata->config.hw_powered) - val = readl_relaxed(drvdata->base + cti_attr->off); - raw_spin_unlock(&drvdata->spinlock); + + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) { + if (drvdata->config.hw_powered) + val = readl_relaxed(drvdata->base + cti_attr->off); + } + pm_runtime_put_sync(dev->parent); return sysfs_emit(buf, "0x%x\n", val); } @@ -202,10 +203,12 @@ static __maybe_unused ssize_t coresight_cti_reg_store(struct device *dev, return -EINVAL;
pm_runtime_get_sync(dev->parent); - raw_spin_lock(&drvdata->spinlock); - if (drvdata->config.hw_powered) - cti_write_single_reg(drvdata, cti_attr->off, val); - raw_spin_unlock(&drvdata->spinlock); + + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) { + if (drvdata->config.hw_powered) + cti_write_single_reg(drvdata, cti_attr->off, val); + } + pm_runtime_put_sync(dev->parent); return size; } @@ -264,17 +267,18 @@ static ssize_t cti_reg32_show(struct device *dev, char *buf, struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); struct cti_config *config = &drvdata->config;
- raw_spin_lock(&drvdata->spinlock); - if ((reg_offset >= 0) && cti_active(config)) { - CS_UNLOCK(drvdata->base); - val = readl_relaxed(drvdata->base + reg_offset); - if (pcached_val) - *pcached_val = val; - CS_LOCK(drvdata->base); - } else if (pcached_val) { - val = *pcached_val; + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) { + if ((reg_offset >= 0) && cti_active(config)) { + CS_UNLOCK(drvdata->base); + val = readl_relaxed(drvdata->base + reg_offset); + if (pcached_val) + *pcached_val = val; + CS_LOCK(drvdata->base); + } else if (pcached_val) { + val = *pcached_val; + } } - raw_spin_unlock(&drvdata->spinlock); + return sprintf(buf, "%#x\n", val); }
@@ -293,15 +297,16 @@ static ssize_t cti_reg32_store(struct device *dev, const char *buf, if (kstrtoul(buf, 0, &val)) return -EINVAL;
- raw_spin_lock(&drvdata->spinlock); - /* local store */ - if (pcached_val) - *pcached_val = (u32)val; + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) { + /* local store */ + if (pcached_val) + *pcached_val = (u32)val; + + /* write through if offset and enabled */ + if ((reg_offset >= 0) && cti_active(config)) + cti_write_single_reg(drvdata, reg_offset, val); + }
- /* write through if offset and enabled */ - if ((reg_offset >= 0) && cti_active(config)) - cti_write_single_reg(drvdata, reg_offset, val); - raw_spin_unlock(&drvdata->spinlock); return size; }
@@ -349,9 +354,9 @@ static ssize_t inout_sel_store(struct device *dev, if (val > (CTIINOUTEN_MAX - 1)) return -EINVAL;
- raw_spin_lock(&drvdata->spinlock); + guard(raw_spinlock_irqsave)(&drvdata->spinlock); + drvdata->config.ctiinout_sel = val; - raw_spin_unlock(&drvdata->spinlock); return size; } static DEVICE_ATTR_RW(inout_sel); @@ -364,10 +369,11 @@ static ssize_t inen_show(struct device *dev, int index; struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
- raw_spin_lock(&drvdata->spinlock); - index = drvdata->config.ctiinout_sel; - val = drvdata->config.ctiinen[index]; - raw_spin_unlock(&drvdata->spinlock); + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) { + index = drvdata->config.ctiinout_sel; + val = drvdata->config.ctiinen[index]; + } + return sprintf(buf, "%#lx\n", val); }
@@ -383,14 +389,15 @@ static ssize_t inen_store(struct device *dev, if (kstrtoul(buf, 0, &val)) return -EINVAL;
- raw_spin_lock(&drvdata->spinlock); + guard(raw_spinlock_irqsave)(&drvdata->spinlock); + index = config->ctiinout_sel; config->ctiinen[index] = val;
/* write through if enabled */ if (cti_active(config)) cti_write_single_reg(drvdata, CTIINEN(index), val); - raw_spin_unlock(&drvdata->spinlock); + return size; } static DEVICE_ATTR_RW(inen); @@ -403,10 +410,11 @@ static ssize_t outen_show(struct device *dev, int index; struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
- raw_spin_lock(&drvdata->spinlock); - index = drvdata->config.ctiinout_sel; - val = drvdata->config.ctiouten[index]; - raw_spin_unlock(&drvdata->spinlock); + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) { + index = drvdata->config.ctiinout_sel; + val = drvdata->config.ctiouten[index]; + } + return sprintf(buf, "%#lx\n", val); }
@@ -422,14 +430,15 @@ static ssize_t outen_store(struct device *dev, if (kstrtoul(buf, 0, &val)) return -EINVAL;
- raw_spin_lock(&drvdata->spinlock); + guard(raw_spinlock_irqsave)(&drvdata->spinlock); + index = config->ctiinout_sel; config->ctiouten[index] = val;
/* write through if enabled */ if (cti_active(config)) cti_write_single_reg(drvdata, CTIOUTEN(index), val); - raw_spin_unlock(&drvdata->spinlock); + return size; } static DEVICE_ATTR_RW(outen); @@ -463,7 +472,7 @@ static ssize_t appclear_store(struct device *dev, if (kstrtoul(buf, 0, &val)) return -EINVAL;
- raw_spin_lock(&drvdata->spinlock); + guard(raw_spinlock_irqsave)(&drvdata->spinlock);
/* a 1'b1 in appclr clears down the same bit in appset*/ config->ctiappset &= ~val; @@ -471,7 +480,7 @@ static ssize_t appclear_store(struct device *dev, /* write through if enabled */ if (cti_active(config)) cti_write_single_reg(drvdata, CTIAPPCLEAR, val); - raw_spin_unlock(&drvdata->spinlock); + return size; } static DEVICE_ATTR_WO(appclear); @@ -487,12 +496,12 @@ static ssize_t apppulse_store(struct device *dev, if (kstrtoul(buf, 0, &val)) return -EINVAL;
- raw_spin_lock(&drvdata->spinlock); + guard(raw_spinlock_irqsave)(&drvdata->spinlock);
/* write through if enabled */ if (cti_active(config)) cti_write_single_reg(drvdata, CTIAPPPULSE, val); - raw_spin_unlock(&drvdata->spinlock); + return size; } static DEVICE_ATTR_WO(apppulse); @@ -681,9 +690,9 @@ static ssize_t trig_filter_enable_show(struct device *dev, u32 val; struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
- raw_spin_lock(&drvdata->spinlock); - val = drvdata->config.trig_filter_enable; - raw_spin_unlock(&drvdata->spinlock); + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) + val = drvdata->config.trig_filter_enable; + return sprintf(buf, "%d\n", val); }
@@ -697,9 +706,9 @@ static ssize_t trig_filter_enable_store(struct device *dev, if (kstrtoul(buf, 0, &val)) return -EINVAL;
- raw_spin_lock(&drvdata->spinlock); + guard(raw_spinlock_irqsave)(&drvdata->spinlock); + drvdata->config.trig_filter_enable = !!val; - raw_spin_unlock(&drvdata->spinlock); return size; } static DEVICE_ATTR_RW(trig_filter_enable); @@ -728,7 +737,7 @@ static ssize_t chan_xtrigs_reset_store(struct device *dev, struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); struct cti_config *config = &drvdata->config;
- raw_spin_lock(&drvdata->spinlock); + guard(raw_spinlock_irqsave)(&drvdata->spinlock);
/* clear the CTI trigger / channel programming registers */ for (i = 0; i < config->nr_trig_max; i++) { @@ -747,7 +756,6 @@ static ssize_t chan_xtrigs_reset_store(struct device *dev, if (cti_active(config)) cti_write_all_hw_regs(drvdata);
- raw_spin_unlock(&drvdata->spinlock); return size; } static DEVICE_ATTR_WO(chan_xtrigs_reset); @@ -768,9 +776,9 @@ static ssize_t chan_xtrigs_sel_store(struct device *dev, if (val > (drvdata->config.nr_ctm_channels - 1)) return -EINVAL;
- raw_spin_lock(&drvdata->spinlock); + guard(raw_spinlock_irqsave)(&drvdata->spinlock); + drvdata->config.xtrig_rchan_sel = val; - raw_spin_unlock(&drvdata->spinlock); return size; }
@@ -781,9 +789,8 @@ static ssize_t chan_xtrigs_sel_show(struct device *dev, unsigned long val; struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
- raw_spin_lock(&drvdata->spinlock); - val = drvdata->config.xtrig_rchan_sel; - raw_spin_unlock(&drvdata->spinlock); + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) + val = drvdata->config.xtrig_rchan_sel;
return sprintf(buf, "%ld\n", val); } @@ -838,12 +845,12 @@ static ssize_t print_chan_list(struct device *dev, unsigned long inuse_bits = 0, chan_mask;
/* scan regs to get bitmap of channels in use. */ - raw_spin_lock(&drvdata->spinlock); - for (i = 0; i < config->nr_trig_max; i++) { - inuse_bits |= config->ctiinen[i]; - inuse_bits |= config->ctiouten[i]; + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) { + for (i = 0; i < config->nr_trig_max; i++) { + inuse_bits |= config->ctiinen[i]; + inuse_bits |= config->ctiouten[i]; + } } - raw_spin_unlock(&drvdata->spinlock);
/* inverse bits if printing free channels */ if (!inuse)
Introduce cti_read_single_reg() as an interface for reading registers with unlocking the CS lock. Consolidate register read in sysfs interfaces using this new helper.
Fixes: b5213376c240 ("coresight: cti: Add sysfs access to program function registers") Fixes: 1a556ca6cc24 ("coresight: cti: Add sysfs coresight mgmt register access") Reviewed-by: Mike Leach mike.leach@arm.com Signed-off-by: Leo Yan leo.yan@arm.com --- drivers/hwtracing/coresight/coresight-cti-core.c | 11 +++++++++++ drivers/hwtracing/coresight/coresight-cti-sysfs.c | 6 ++---- drivers/hwtracing/coresight/coresight-cti.h | 1 + 3 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c index e3c98d89c987c10faafa02d1dbd721cfd4187edb..6a53c3ceebf41645e599017e076d12b8b158b10f 100644 --- a/drivers/hwtracing/coresight/coresight-cti-core.c +++ b/drivers/hwtracing/coresight/coresight-cti-core.c @@ -156,6 +156,17 @@ static int cti_disable_hw(struct cti_drvdata *drvdata) return 0; }
+u32 cti_read_single_reg(struct cti_drvdata *drvdata, int offset) +{ + int val; + + CS_UNLOCK(drvdata->base); + val = readl_relaxed(drvdata->base + offset); + CS_LOCK(drvdata->base); + + return val; +} + void cti_write_single_reg(struct cti_drvdata *drvdata, int offset, u32 value) { CS_UNLOCK(drvdata->base); diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c index 455d08bcccd49a3f1eac8abd8246806ef73a9ab6..9a997b2f090472761e9734fffc534663df8b06c6 100644 --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c @@ -183,7 +183,7 @@ static ssize_t coresight_cti_reg_show(struct device *dev,
scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) { if (drvdata->config.hw_powered) - val = readl_relaxed(drvdata->base + cti_attr->off); + val = cti_read_single_reg(drvdata, cti_attr->off); }
pm_runtime_put_sync(dev->parent); @@ -269,11 +269,9 @@ static ssize_t cti_reg32_show(struct device *dev, char *buf,
scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) { if ((reg_offset >= 0) && cti_active(config)) { - CS_UNLOCK(drvdata->base); - val = readl_relaxed(drvdata->base + reg_offset); + val = cti_read_single_reg(drvdata, reg_offset); if (pcached_val) *pcached_val = val; - CS_LOCK(drvdata->base); } else if (pcached_val) { val = *pcached_val; } diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h index daff9e32a6daca90f4d5f4726f163fdd9106191c..45735526fe55b0e4746ba856443f0fcc807d82bb 100644 --- a/drivers/hwtracing/coresight/coresight-cti.h +++ b/drivers/hwtracing/coresight/coresight-cti.h @@ -220,6 +220,7 @@ int cti_disable(struct coresight_device *csdev, struct coresight_path *path); void cti_write_all_hw_regs(struct cti_drvdata *drvdata); void cti_write_intack(struct device *dev, u32 ackval); void cti_write_single_reg(struct cti_drvdata *drvdata, int offset, u32 value); +u32 cti_read_single_reg(struct cti_drvdata *drvdata, int offset); int cti_channel_trig_op(struct device *dev, enum cti_chan_op op, enum cti_trig_dir direction, u32 channel_idx, u32 trigger_idx);
According to the Arm ARM (DDI 0487 L.b), ASICCTL is implemented only when CTIDEVID.EXTMUXNUM is non-zero.
Based on CTIDEVID.EXTMUXNUM, add a flag 'asicctl_impl' to indicate whether the register is implemented, and access ASICCTL conditionally based on the flag.
Allow the sysfs node to be visible only when the register is implemented.
Reviewed-by: Mike Leach mike.leach@arm.com Signed-off-by: Leo Yan leo.yan@arm.com --- drivers/hwtracing/coresight/coresight-cti-core.c | 5 ++++- drivers/hwtracing/coresight/coresight-cti-sysfs.c | 13 +++++++++++++ drivers/hwtracing/coresight/coresight-cti.h | 2 ++ 3 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c index 6a53c3ceebf41645e599017e076d12b8b158b10f..726970d852dee3b962e5147311dfadd0b741cfe0 100644 --- a/drivers/hwtracing/coresight/coresight-cti-core.c +++ b/drivers/hwtracing/coresight/coresight-cti-core.c @@ -68,7 +68,8 @@ void cti_write_all_hw_regs(struct cti_drvdata *drvdata)
/* other regs */ writel_relaxed(config->ctigate, drvdata->base + CTIGATE); - writel_relaxed(config->asicctl, drvdata->base + ASICCTL); + if (config->asicctl_impl) + writel_relaxed(config->asicctl, drvdata->base + ASICCTL); writel_relaxed(config->ctiappset, drvdata->base + CTIAPPSET);
/* re-enable CTI */ @@ -221,6 +222,8 @@ static void cti_set_default_config(struct device *dev, config->trig_filter_enable = true; config->ctigate = GENMASK(config->nr_ctm_channels - 1, 0); config->enable_req_count = 0; + + config->asicctl_impl = !!FIELD_GET(GENMASK(4, 0), devid); }
/* diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c index 9a997b2f090472761e9734fffc534663df8b06c6..c15a580f6e90f57b1376e0b883a27700966feb1a 100644 --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c @@ -537,6 +537,18 @@ static struct attribute *coresight_cti_regs_attrs[] = { NULL, };
+static umode_t coresight_cti_regs_is_visible(struct kobject *kobj, + struct attribute *attr, int idx) +{ + struct device *dev = kobj_to_dev(kobj); + struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); + + if (attr == &dev_attr_asicctl.attr && !drvdata->config.asicctl_impl) + return 0; + + return attr->mode; +} + /* CTI channel x-trigger programming */ static int cti_trig_op_parse(struct device *dev, enum cti_chan_op op, @@ -1174,6 +1186,7 @@ static const struct attribute_group coresight_cti_mgmt_group = {
static const struct attribute_group coresight_cti_regs_group = { .attrs = coresight_cti_regs_attrs, + .is_visible = coresight_cti_regs_is_visible, .name = "regs", };
diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h index 45735526fe55b0e4746ba856443f0fcc807d82bb..a4b3968d8d3d19a20604fb09db14cf41962f5a79 100644 --- a/drivers/hwtracing/coresight/coresight-cti.h +++ b/drivers/hwtracing/coresight/coresight-cti.h @@ -119,6 +119,7 @@ struct cti_device { * @nr_trig_max: Max number of trigger signals implemented on device. * (max of trig_in or trig_out) - from ID register. * @nr_ctm_channels: number of available CTM channels - from ID register. + * @asicctl_impl: true if asicctl is implemented. * @enable_req_count: CTI is enabled alongside >=1 associated devices. * @hw_enabled: true if hw is currently enabled. * @hw_powered: true if associated cpu powered on, or no cpu. @@ -140,6 +141,7 @@ struct cti_config { /* hardware description */ int nr_ctm_channels; int nr_trig_max; + bool asicctl_impl;
/* cti enable control */ int enable_req_count;
According Arm ARM, the CTI ASICCTL register:
"It is IMPLEMENTATION DEFINED whether ASICCTL is implemented in the Core power domain or in the Debug power domain."
This is the only CTI register that may reside in the core power domain. However, it has been confirmed that Arm designed CTIs place ASICCTL in the debug power domain. Furthermore, ASICCTL is implemented only when CTIDEVID.EXTMUXNUM is non-zero, which is a rare case for CPU CTIs.
For these reasons, it is safe to conclude that all CTI registers are not located in the CPU power domain. Therefore, the CTI driver does not need CPU power management.
This commit removes the CPU power management from CTI driver.
Reviewed-by: Mike Leach mike.leach@arm.com Signed-off-by: Leo Yan leo.yan@arm.com --- drivers/hwtracing/coresight/coresight-cti-core.c | 186 +---------------------- 1 file changed, 3 insertions(+), 183 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c index 726970d852dee3b962e5147311dfadd0b741cfe0..3aa081f28a18dc4987feeb57244aa384e80702ac 100644 --- a/drivers/hwtracing/coresight/coresight-cti-core.c +++ b/drivers/hwtracing/coresight/coresight-cti-core.c @@ -42,12 +42,6 @@ static DEFINE_MUTEX(ect_mutex); #define csdev_to_cti_drvdata(csdev) \ dev_get_drvdata(csdev->dev.parent)
-/* power management handling */ -static int nr_cti_cpu; - -/* quick lookup list for CPU bound CTIs when power handling */ -static struct cti_drvdata *cti_cpu_drvdata[NR_CPUS]; - /* write set of regs to hardware - call with spinlock claimed */ void cti_write_all_hw_regs(struct cti_drvdata *drvdata) { @@ -104,28 +98,6 @@ static int cti_enable_hw(struct cti_drvdata *drvdata) return 0; }
-/* re-enable CTI on CPU when using CPU hotplug */ -static void cti_cpuhp_enable_hw(struct cti_drvdata *drvdata) -{ - struct cti_config *config = &drvdata->config; - - guard(raw_spinlock_irqsave)(&drvdata->spinlock); - - config->hw_powered = true; - - /* no need to do anything if no enable request */ - if (!drvdata->config.enable_req_count) - return; - - /* try to claim the device */ - if (coresight_claim_device(drvdata->csdev)) - return; - - cti_write_all_hw_regs(drvdata); - config->hw_enabled = true; - return; -} - /* disable hardware */ static int cti_disable_hw(struct cti_drvdata *drvdata) { @@ -643,146 +615,6 @@ static void cti_remove_conn_xrefs(struct cti_drvdata *drvdata) } }
-/** cti PM callbacks **/ -static int cti_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd, - void *v) -{ - struct cti_drvdata *drvdata; - struct coresight_device *csdev; - unsigned int cpu = smp_processor_id(); - int notify_res = NOTIFY_OK; - - if (!cti_cpu_drvdata[cpu]) - return NOTIFY_OK; - - drvdata = cti_cpu_drvdata[cpu]; - csdev = drvdata->csdev; - - if (WARN_ON_ONCE(drvdata->ctidev.cpu != cpu)) - return NOTIFY_BAD; - - guard(raw_spinlock_irqsave)(&drvdata->spinlock); - - switch (cmd) { - case CPU_PM_ENTER: - /* CTI regs all static - we have a copy & nothing to save */ - drvdata->config.hw_powered = false; - if (drvdata->config.hw_enabled) - coresight_disclaim_device(csdev); - break; - - case CPU_PM_ENTER_FAILED: - drvdata->config.hw_powered = true; - if (drvdata->config.hw_enabled) { - if (coresight_claim_device(csdev)) - drvdata->config.hw_enabled = false; - } - break; - - case CPU_PM_EXIT: - /* write hardware registers to re-enable. */ - drvdata->config.hw_powered = true; - drvdata->config.hw_enabled = false; - - /* check enable reference count to enable HW */ - if (drvdata->config.enable_req_count) { - /* check we can claim the device as we re-power */ - if (coresight_claim_device(csdev)) - goto cti_notify_exit; - - drvdata->config.hw_enabled = true; - cti_write_all_hw_regs(drvdata); - } - break; - - default: - notify_res = NOTIFY_DONE; - break; - } - -cti_notify_exit: - return notify_res; -} - -static struct notifier_block cti_cpu_pm_nb = { - .notifier_call = cti_cpu_pm_notify, -}; - -/* CPU HP handlers */ -static int cti_starting_cpu(unsigned int cpu) -{ - struct cti_drvdata *drvdata = cti_cpu_drvdata[cpu]; - - if (!drvdata) - return 0; - - cti_cpuhp_enable_hw(drvdata); - return 0; -} - -static int cti_dying_cpu(unsigned int cpu) -{ - struct cti_drvdata *drvdata = cti_cpu_drvdata[cpu]; - - if (!drvdata) - return 0; - - guard(raw_spinlock_irqsave)(&drvdata->spinlock); - - drvdata->config.hw_powered = false; - if (drvdata->config.hw_enabled) - coresight_disclaim_device(drvdata->csdev); - - return 0; -} - -static int cti_pm_setup(struct cti_drvdata *drvdata) -{ - int ret; - - if (drvdata->ctidev.cpu == -1) - return 0; - - if (nr_cti_cpu) - goto done; - - cpus_read_lock(); - ret = cpuhp_setup_state_nocalls_cpuslocked( - CPUHP_AP_ARM_CORESIGHT_CTI_STARTING, - "arm/coresight_cti:starting", - cti_starting_cpu, cti_dying_cpu); - if (ret) { - cpus_read_unlock(); - return ret; - } - - ret = cpu_pm_register_notifier(&cti_cpu_pm_nb); - cpus_read_unlock(); - if (ret) { - cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_CTI_STARTING); - return ret; - } - -done: - nr_cti_cpu++; - cti_cpu_drvdata[drvdata->ctidev.cpu] = drvdata; - - return 0; -} - -/* release PM registrations */ -static void cti_pm_release(struct cti_drvdata *drvdata) -{ - if (drvdata->ctidev.cpu == -1) - return; - - cti_cpu_drvdata[drvdata->ctidev.cpu] = NULL; - if (--nr_cti_cpu == 0) { - cpu_pm_unregister_notifier(&cti_cpu_pm_nb); - cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_CTI_STARTING); - } -} - /** cti ect operations **/ int cti_enable(struct coresight_device *csdev, enum cs_mode mode, struct coresight_path *path) @@ -815,7 +647,6 @@ static void cti_remove(struct amba_device *adev)
mutex_lock(&ect_mutex); cti_remove_conn_xrefs(drvdata); - cti_pm_release(drvdata);
/* remove from the list */ list_for_each_entry_safe(ect_item, ect_tmp, &ect_net, node) { @@ -889,17 +720,12 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id) if (!cti_desc.name) return -ENOMEM;
- /* setup CPU power management handling for CPU bound CTI devices. */ - ret = cti_pm_setup(drvdata); - if (ret) - return ret; - /* create dynamic attributes for connections */ ret = cti_create_cons_sysfs(dev, drvdata); if (ret) { dev_err(dev, "%s: create dynamic sysfs entries failed\n", cti_desc.name); - goto pm_release; + return ret; }
/* set up coresight component description */ @@ -912,10 +738,8 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id)
coresight_clear_self_claim_tag(&cti_desc.access); drvdata->csdev = coresight_register(&cti_desc); - if (IS_ERR(drvdata->csdev)) { - ret = PTR_ERR(drvdata->csdev); - goto pm_release; - } + if (IS_ERR(drvdata->csdev)) + return PTR_ERR(drvdata->csdev);
/* add to list of CTI devices */ mutex_lock(&ect_mutex); @@ -928,10 +752,6 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id) pm_runtime_put(&adev->dev); dev_info(&drvdata->csdev->dev, "CTI initialized\n"); return 0; - -pm_release: - cti_pm_release(drvdata); - return ret; }
static struct amba_cs_uci_id uci_id_cti[] = {
Rename cti_active() to cti_is_active() to clarify that it checks whether the CTI device is active.
Reviewed-by: Mike Leach mike.leach@arm.com Signed-off-by: Leo Yan leo.yan@arm.com --- drivers/hwtracing/coresight/coresight-cti-core.c | 8 ++++---- drivers/hwtracing/coresight/coresight-cti-sysfs.c | 14 +++++++------- drivers/hwtracing/coresight/coresight-cti.h | 2 +- 3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c index 3aa081f28a18dc4987feeb57244aa384e80702ac..28f995263433c8727a7fdeff94fbe0a4286767e8 100644 --- a/drivers/hwtracing/coresight/coresight-cti-core.c +++ b/drivers/hwtracing/coresight/coresight-cti-core.c @@ -155,7 +155,7 @@ void cti_write_intack(struct device *dev, u32 ackval) guard(raw_spinlock_irqsave)(&drvdata->spinlock);
/* write if enabled */ - if (cti_active(config)) + if (cti_is_active(config)) cti_write_single_reg(drvdata, CTIINTACK, ackval); }
@@ -342,7 +342,7 @@ int cti_channel_trig_op(struct device *dev, enum cti_chan_op op, config->ctiouten[trigger_idx] = reg_value;
/* write through if enabled */ - if (cti_active(config)) + if (cti_is_active(config)) cti_write_single_reg(drvdata, reg_offset, reg_value);
return 0; @@ -380,7 +380,7 @@ int cti_channel_gate_op(struct device *dev, enum cti_chan_gate_op op, } if (err == 0) { config->ctigate = reg_value; - if (cti_active(config)) + if (cti_is_active(config)) cti_write_single_reg(drvdata, CTIGATE, reg_value); }
@@ -429,7 +429,7 @@ int cti_channel_setop(struct device *dev, enum cti_chan_set_op op, break; }
- if ((err == 0) && cti_active(config)) + if ((err == 0) && cti_is_active(config)) cti_write_single_reg(drvdata, reg_offset, reg_value);
return err; diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c index c15a580f6e90f57b1376e0b883a27700966feb1a..a22cc9a2bee24eb6115e7adb61880cc86d03e12e 100644 --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c @@ -268,7 +268,7 @@ static ssize_t cti_reg32_show(struct device *dev, char *buf, struct cti_config *config = &drvdata->config;
scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) { - if ((reg_offset >= 0) && cti_active(config)) { + if ((reg_offset >= 0) && cti_is_active(config)) { val = cti_read_single_reg(drvdata, reg_offset); if (pcached_val) *pcached_val = val; @@ -301,7 +301,7 @@ static ssize_t cti_reg32_store(struct device *dev, const char *buf, *pcached_val = (u32)val;
/* write through if offset and enabled */ - if ((reg_offset >= 0) && cti_active(config)) + if ((reg_offset >= 0) && cti_is_active(config)) cti_write_single_reg(drvdata, reg_offset, val); }
@@ -393,7 +393,7 @@ static ssize_t inen_store(struct device *dev, config->ctiinen[index] = val;
/* write through if enabled */ - if (cti_active(config)) + if (cti_is_active(config)) cti_write_single_reg(drvdata, CTIINEN(index), val);
return size; @@ -434,7 +434,7 @@ static ssize_t outen_store(struct device *dev, config->ctiouten[index] = val;
/* write through if enabled */ - if (cti_active(config)) + if (cti_is_active(config)) cti_write_single_reg(drvdata, CTIOUTEN(index), val);
return size; @@ -476,7 +476,7 @@ static ssize_t appclear_store(struct device *dev, config->ctiappset &= ~val;
/* write through if enabled */ - if (cti_active(config)) + if (cti_is_active(config)) cti_write_single_reg(drvdata, CTIAPPCLEAR, val);
return size; @@ -497,7 +497,7 @@ static ssize_t apppulse_store(struct device *dev, guard(raw_spinlock_irqsave)(&drvdata->spinlock);
/* write through if enabled */ - if (cti_active(config)) + if (cti_is_active(config)) cti_write_single_reg(drvdata, CTIAPPPULSE, val);
return size; @@ -763,7 +763,7 @@ static ssize_t chan_xtrigs_reset_store(struct device *dev, config->xtrig_rchan_sel = 0;
/* if enabled then write through */ - if (cti_active(config)) + if (cti_is_active(config)) cti_write_all_hw_regs(drvdata);
return size; diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h index a4b3968d8d3d19a20604fb09db14cf41962f5a79..2edc0d01812c93fe5a817da366a2862b8490c0a3 100644 --- a/drivers/hwtracing/coresight/coresight-cti.h +++ b/drivers/hwtracing/coresight/coresight-cti.h @@ -236,7 +236,7 @@ coresight_cti_get_platform_data(struct device *dev); const char *cti_plat_get_node_name(struct fwnode_handle *fwnode);
/* cti powered and enabled */ -static inline bool cti_active(struct cti_config *cfg) +static inline bool cti_is_active(struct cti_config *cfg) { return cfg->hw_powered && cfg->hw_enabled; }
Since the CPU PM code has been removed from the CTI driver and the device is enabled via runtime PM, pm_runtime_active() can be used to check whether the device is powered.
As a result, the hw_powered flag is redundant, remove it.
Reviewed-by: Mike Leach mike.leach@arm.com Signed-off-by: Leo Yan leo.yan@arm.com --- drivers/hwtracing/coresight/coresight-cti-core.c | 11 ++++----- drivers/hwtracing/coresight/coresight-cti-sysfs.c | 29 ++++++----------------- drivers/hwtracing/coresight/coresight-cti.h | 6 ++--- 3 files changed, 13 insertions(+), 33 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c index 28f995263433c8727a7fdeff94fbe0a4286767e8..5ac36f0776181559a87b3ee37d6c9076882576b5 100644 --- a/drivers/hwtracing/coresight/coresight-cti-core.c +++ b/drivers/hwtracing/coresight/coresight-cti-core.c @@ -80,8 +80,8 @@ static int cti_enable_hw(struct cti_drvdata *drvdata)
guard(raw_spinlock_irqsave)(&drvdata->spinlock);
- /* no need to do anything if enabled or unpowered*/ - if (config->hw_enabled || !config->hw_powered) + /* no need to do anything if enabled */ + if (config->hw_enabled) goto cti_state_unchanged;
/* claim the device */ @@ -114,8 +114,8 @@ static int cti_disable_hw(struct cti_drvdata *drvdata) if (--drvdata->config.enable_req_count > 0) return 0;
- /* no need to do anything if disabled or cpu unpowered */ - if (!config->hw_enabled || !config->hw_powered) + /* no need to do anything if disabled */ + if (!config->hw_enabled) return 0;
CS_UNLOCK(drvdata->base); @@ -702,9 +702,6 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id) return PTR_ERR(pdata); }
- /* default to powered - could change on PM notifications */ - drvdata->config.hw_powered = true; - /* * Set up device name - will depend if cpu bound or otherwise. * diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c index a22cc9a2bee24eb6115e7adb61880cc86d03e12e..9ab586a5c9a4fd2a64c542aaaaa625e2299edd62 100644 --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c @@ -81,19 +81,12 @@ static ssize_t enable_show(struct device *dev, char *buf) { int enable_req; - bool enabled, powered; struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
- scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) { + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) enable_req = drvdata->config.enable_req_count; - powered = drvdata->config.hw_powered; - enabled = drvdata->config.hw_enabled; - }
- if (powered) - return sprintf(buf, "%d\n", enabled); - else - return sprintf(buf, "%d\n", !!enable_req); + return sprintf(buf, "%d\n", !!enable_req); }
static ssize_t enable_store(struct device *dev, @@ -131,11 +124,7 @@ static ssize_t powered_show(struct device *dev, struct device_attribute *attr, char *buf) { - bool powered; - struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); - - scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) - powered = drvdata->config.hw_powered; + bool powered = pm_runtime_active(dev->parent);
return sprintf(buf, "%d\n", powered); } @@ -181,10 +170,8 @@ static ssize_t coresight_cti_reg_show(struct device *dev,
pm_runtime_get_sync(dev->parent);
- scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) { - if (drvdata->config.hw_powered) - val = cti_read_single_reg(drvdata, cti_attr->off); - } + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) + val = cti_read_single_reg(drvdata, cti_attr->off);
pm_runtime_put_sync(dev->parent); return sysfs_emit(buf, "0x%x\n", val); @@ -204,10 +191,8 @@ static __maybe_unused ssize_t coresight_cti_reg_store(struct device *dev,
pm_runtime_get_sync(dev->parent);
- scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) { - if (drvdata->config.hw_powered) - cti_write_single_reg(drvdata, cti_attr->off, val); - } + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) + cti_write_single_reg(drvdata, cti_attr->off, val);
pm_runtime_put_sync(dev->parent); return size; diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h index 2edc0d01812c93fe5a817da366a2862b8490c0a3..8754cb5def7918f03235343844467af8410b75a8 100644 --- a/drivers/hwtracing/coresight/coresight-cti.h +++ b/drivers/hwtracing/coresight/coresight-cti.h @@ -122,7 +122,6 @@ struct cti_device { * @asicctl_impl: true if asicctl is implemented. * @enable_req_count: CTI is enabled alongside >=1 associated devices. * @hw_enabled: true if hw is currently enabled. - * @hw_powered: true if associated cpu powered on, or no cpu. * @trig_in_use: bitfield of in triggers registered as in use. * @trig_out_use: bitfield of out triggers registered as in use. * @trig_out_filter: bitfield of out triggers that are blocked if filter @@ -146,7 +145,6 @@ struct cti_config { /* cti enable control */ int enable_req_count; bool hw_enabled; - bool hw_powered;
/* registered triggers and filtering */ u32 trig_in_use; @@ -235,10 +233,10 @@ struct coresight_platform_data * coresight_cti_get_platform_data(struct device *dev); const char *cti_plat_get_node_name(struct fwnode_handle *fwnode);
-/* cti powered and enabled */ +/* Check if a cti device is enabled */ static inline bool cti_is_active(struct cti_config *cfg) { - return cfg->hw_powered && cfg->hw_enabled; + return cfg->hw_enabled; }
#endif /* _CORESIGHT_CORESIGHT_CTI_H */
The enable_req_count field already tracks whether the CTI device is enabled. A non-zero value indicates that the device is active, the hw_enabled flag is redundant if so.
Remove hw_enabled and update cti_is_active() to check enable_req_count. Replace open-coded enable_req_count checks with cti_is_active().
Reviewed-by: Mike Leach mike.leach@arm.com Signed-off-by: Leo Yan leo.yan@arm.com --- drivers/hwtracing/coresight/coresight-cti-core.c | 11 ++--------- drivers/hwtracing/coresight/coresight-cti-sysfs.c | 2 +- drivers/hwtracing/coresight/coresight-cti.h | 4 +--- 3 files changed, 4 insertions(+), 13 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c index 5ac36f0776181559a87b3ee37d6c9076882576b5..2f4c9362709a90b12a1aeb5016905b7d4474b912 100644 --- a/drivers/hwtracing/coresight/coresight-cti-core.c +++ b/drivers/hwtracing/coresight/coresight-cti-core.c @@ -81,7 +81,7 @@ static int cti_enable_hw(struct cti_drvdata *drvdata) guard(raw_spinlock_irqsave)(&drvdata->spinlock);
/* no need to do anything if enabled */ - if (config->hw_enabled) + if (cti_is_active(config)) goto cti_state_unchanged;
/* claim the device */ @@ -91,8 +91,6 @@ static int cti_enable_hw(struct cti_drvdata *drvdata)
cti_write_all_hw_regs(drvdata);
- config->hw_enabled = true; - cti_state_unchanged: drvdata->config.enable_req_count++; return 0; @@ -107,22 +105,17 @@ static int cti_disable_hw(struct cti_drvdata *drvdata) guard(raw_spinlock_irqsave)(&drvdata->spinlock);
/* don't allow negative refcounts, return an error */ - if (!drvdata->config.enable_req_count) + if (!cti_is_active(config)) return -EINVAL;
/* check refcount - disable on 0 */ if (--drvdata->config.enable_req_count > 0) return 0;
- /* no need to do anything if disabled */ - if (!config->hw_enabled) - return 0; - CS_UNLOCK(drvdata->base);
/* disable CTI */ writel_relaxed(0, drvdata->base + CTICONTROL); - config->hw_enabled = false;
coresight_disclaim_device_unlocked(csdev); CS_LOCK(drvdata->base); diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c index 9ab586a5c9a4fd2a64c542aaaaa625e2299edd62..9ef44956ebdc7781717d773fa014165989df2048 100644 --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c @@ -84,7 +84,7 @@ static ssize_t enable_show(struct device *dev, struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) - enable_req = drvdata->config.enable_req_count; + enable_req = cti_is_active(&drvdata->config);
return sprintf(buf, "%d\n", !!enable_req); } diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h index 8754cb5def7918f03235343844467af8410b75a8..c5f9e79fabc608f5a95e7e4d8b0cbe1c853d584a 100644 --- a/drivers/hwtracing/coresight/coresight-cti.h +++ b/drivers/hwtracing/coresight/coresight-cti.h @@ -121,7 +121,6 @@ struct cti_device { * @nr_ctm_channels: number of available CTM channels - from ID register. * @asicctl_impl: true if asicctl is implemented. * @enable_req_count: CTI is enabled alongside >=1 associated devices. - * @hw_enabled: true if hw is currently enabled. * @trig_in_use: bitfield of in triggers registered as in use. * @trig_out_use: bitfield of out triggers registered as in use. * @trig_out_filter: bitfield of out triggers that are blocked if filter @@ -144,7 +143,6 @@ struct cti_config {
/* cti enable control */ int enable_req_count; - bool hw_enabled;
/* registered triggers and filtering */ u32 trig_in_use; @@ -236,7 +234,7 @@ const char *cti_plat_get_node_name(struct fwnode_handle *fwnode); /* Check if a cti device is enabled */ static inline bool cti_is_active(struct cti_config *cfg) { - return cfg->hw_enabled; + return !!cfg->enable_req_count; }
#endif /* _CORESIGHT_CORESIGHT_CTI_H */
Return an error when the offset is negative.
Signed-off-by: Leo Yan leo.yan@arm.com --- drivers/hwtracing/coresight/coresight-cti-sysfs.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c index 9ef44956ebdc7781717d773fa014165989df2048..4c0a60840efb9ecc47850c1eb5a7abb41b49c5cc 100644 --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c @@ -252,8 +252,11 @@ static ssize_t cti_reg32_show(struct device *dev, char *buf, struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); struct cti_config *config = &drvdata->config;
+ if (reg_offset < 0) + return -EINVAL; + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) { - if ((reg_offset >= 0) && cti_is_active(config)) { + if (cti_is_active(config)) { val = cti_read_single_reg(drvdata, reg_offset); if (pcached_val) *pcached_val = val; @@ -280,13 +283,16 @@ static ssize_t cti_reg32_store(struct device *dev, const char *buf, if (kstrtoul(buf, 0, &val)) return -EINVAL;
+ if (reg_offset < 0) + return -EINVAL; + scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) { /* local store */ if (pcached_val) *pcached_val = (u32)val;
/* write through if offset and enabled */ - if ((reg_offset >= 0) && cti_is_active(config)) + if (cti_is_active(config)) cti_write_single_reg(drvdata, reg_offset, val); }
Hi Leo,
On 2/26/26 09:23, Leo Yan wrote:
Return an error when the offset is negative.
Signed-off-by: Leo Yan leo.yan@arm.com
drivers/hwtracing/coresight/coresight-cti-sysfs.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c index 9ef44956ebdc7781717d773fa014165989df2048..4c0a60840efb9ecc47850c1eb5a7abb41b49c5cc 100644 --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c @@ -252,8 +252,11 @@ static ssize_t cti_reg32_show(struct device *dev, char *buf, struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); struct cti_config *config = &drvdata->config;
- if (reg_offset < 0)
return -EINVAL;- scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) {
if ((reg_offset >= 0) && cti_is_active(config)) {
if (cti_is_active(config)) { val = cti_read_single_reg(drvdata, reg_offset); if (pcached_val) *pcached_val = val;@@ -280,13 +283,16 @@ static ssize_t cti_reg32_store(struct device *dev, const char *buf, if (kstrtoul(buf, 0, &val)) return -EINVAL;
- if (reg_offset < 0)
return -EINVAL;- scoped_guard(raw_spinlock_irqsave, &drvdata->spinlock) { /* local store */ if (pcached_val) *pcached_val = (u32)val;
/* write through if offset and enabled */
if ((reg_offset >= 0) && cti_is_active(config))
if (cti_is_active(config)) cti_write_single_reg(drvdata, reg_offset, val);
Given that at this point the code is only called via macro, which hardcodes the offsets. don't think the check will ever return the error.
But in case usage changes in future this is safe. so..
Reviewed-by: Mike Leach mike.leach@Arm.com
On Thu, 26 Feb 2026 09:23:48 +0000, Leo Yan wrote:
The CPU power management issue in the CTI driver was first observed in series [1]; this series resolves that issue. It fixes bugs and removes CPU PM operations from the CoreSight CTI driver, the goal is to use the CoreSight core layer as the central place for CPU power management. Removing CPU PM from CTI driver can avoid conflicts with the core layer.
This series can be divided into:
[...]
Applied, thanks!
[1/8] coresight: cti: Make spinlock usage consistent https://git.kernel.org/coresight/c/ef7d4aaf6867 [2/8] coresight: cti: Fix register reads https://git.kernel.org/coresight/c/6582fe69ac4b [3/8] coresight: cti: Access ASICCTL only when implemented https://git.kernel.org/coresight/c/b4d9ef475ec7 [4/8] coresight: cti: Remove CPU power management code https://git.kernel.org/coresight/c/59213b4be5c1 [5/8] coresight: cti: Rename cti_active() to cti_is_active() https://git.kernel.org/coresight/c/b727e7bba3ff [6/8] coresight: cti: Remove hw_powered flag https://git.kernel.org/coresight/c/daedb30fd6ac [7/8] coresight: cti: Remove hw_enabled flag https://git.kernel.org/coresight/c/d5e57babdffb [8/8] coresight: cti: Properly handle negative offsets in cti_reg32_{show|store}() https://git.kernel.org/coresight/c/9c5ef7a30d90
Best regards,
-
Leo Yan -
Mike Leach -
Suzuki K Poulose