CoreSight

coresight@lists.linaro.org

5 participants
2627 discussions

Re: [PATCH v4] coresight: cti: Add Qualcomm extended CTI support

by Mike Leach

Hi, This patchset is looking a lot better now. I do have a couple of comments 1) Using the bitmaps etc is good, but this is increasing the memory footpint of all CTI data instances by 4x. Only the qualcomm 128 signal CTIs need that space - and there are many systems with 10s of standard CTIs I think it would be better to dynamically allocate the register and bitmap memory to the actual number of registers in the CTI instance - reducing the memory footprint for all CTI implementations. This should be from the config.nr_trig_max which is set from DEVID register and represents the actual number of triggers. Thus the register arrays in cti_config, ctiinen and ctiouten, plus the bitmapped fields can be allocated in the cti_set_default_config() function. I suggest that the dynamic allocation changes form the first patch in this series, with the QCOM extended changes as the second patch. 2) The issue with the claim tags is do to a bug in the core claim tag code. It does not check the number of claim tags implemented - which can be 0 as in your design. I am fixing the core code to address this issue and will post a patch shortly. This will mean that claim will "succeed" where there are no cliam tags in use. See the rest of my comments inline. On Tue, 2 Sept 2025 at 03:57, Yingchao Deng <yingchao.deng(a)oss.qualcomm.com> wrote: > > The QCOM extended CTI is a heavily parameterized version of ARM’s CSCTI. > It allows a debugger to send to trigger events to a processor or to send > a trigger event to one or more processors when a trigger event occurs > on another processor on the same SoC, or even between SoCs. Qualcomm CTI > implementation differs from the standard CTI in the following aspects: > > 1. The number of supported triggers is extended to 128. > 2. Several register offsets differ from the CoreSight specification. > 3. CLAIMSET and CLAIMCLR registers are unused. > > Signed-off-by: Jinlong Mao <jinlong.mao(a)oss.qualcomm.com> > Signed-off-by: Yingchao Deng <yingchao.deng(a)oss.qualcomm.com> > --- > The QCOM extended CTI is a heavily parameterized version of ARM’s CSCTI. > It allows a debugger to send to trigger events to a processor or to send > a trigger event to one or more processors when a trigger event occurs on > another processor on the same SoC, or even between SoCs. > > QCOM extended CTI supports up to 128 triggers. And some of the register > offsets are changed. > > The commands to configure CTI triggers are the same as ARM's CTI. > > Changes in v4: > 1. Read the DEVARCH registers to identify Qualcomm CTI. > 2. Add a reg_idx node, and refactor the coresight_cti_reg_show() and > coresight_cti_reg_store() functions accordingly. > 3. The register offsets specific to Qualcomm CTI are moved to qcom_cti.h. > Link to v3 - https://lore.kernel.org/linux-arm-msm/20250722081405.2947294-1-quic_jinlmao… > > Changes in v3: > 1. Rename is_extended_cti() to of_is_extended_cti(). > 2. Add the missing 'i' when write the CTI trigger registers. > 3. Convert the multi-line output in sysfs to single line. > 4. Initialize offset arrays using designated initializer. > Link to V2 - https://lore.kernel.org/all/20250429071841.1158315-3-quic_jinlmao@quicinc.c… > > Changes in V2: > 1. Add enum for compatible items. > 2. Move offset arrays to coresight-cti-core > --- > drivers/hwtracing/coresight/coresight-cti-core.c | 123 +++++++++++--- > .../hwtracing/coresight/coresight-cti-platform.c | 16 +- > drivers/hwtracing/coresight/coresight-cti-sysfs.c | 184 +++++++++++++++------ > drivers/hwtracing/coresight/coresight-cti.h | 22 ++- > drivers/hwtracing/coresight/qcom-cti.h | 60 +++++++ > 5 files changed, 320 insertions(+), 85 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c > index 8fb30dd73fd20ae613a45b1a467f457a046a9412..2431bc1d5ef39767d415b0df5a04da888a3104e5 100644 > --- a/drivers/hwtracing/coresight/coresight-cti-core.c > +++ b/drivers/hwtracing/coresight/coresight-cti-core.c > @@ -22,6 +22,54 @@ > #include "coresight-priv.h" > #include "coresight-cti.h" #include "qcom-cti.h" here rather than in coresight-cti.h > +static const u32 cti_normal_offset[] = { > + [INDEX_CTIINTACK] = CTIINTACK, > + [INDEX_CTIAPPSET] = CTIAPPSET, > + [INDEX_CTIAPPCLEAR] = CTIAPPCLEAR, > + [INDEX_CTIAPPPULSE] = CTIAPPPULSE, > + [INDEX_CTIINEN] = CTIINEN(0), > + [INDEX_CTIOUTEN] = CTIOUTEN(0), > + [INDEX_CTITRIGINSTATUS] = CTITRIGINSTATUS, > + [INDEX_CTITRIGOUTSTATUS] = CTITRIGOUTSTATUS, > + [INDEX_CTICHINSTATUS] = CTICHINSTATUS, > + [INDEX_CTICHOUTSTATUS] = CTICHOUTSTATUS, > + [INDEX_CTIGATE] = CTIGATE, > + [INDEX_ASICCTL] = ASICCTL, > + [INDEX_ITCHINACK] = ITCHINACK, > + [INDEX_ITTRIGINACK] = ITTRIGINACK, > + [INDEX_ITCHOUT] = ITCHOUT, > + [INDEX_ITTRIGOUT] = ITTRIGOUT, > + [INDEX_ITCHOUTACK] = ITCHOUTACK, > + [INDEX_ITTRIGOUTACK] = ITTRIGOUTACK, > + [INDEX_ITCHIN] = ITCHIN, > + [INDEX_ITTRIGIN] = ITTRIGIN, > + [INDEX_ITCTRL] = CORESIGHT_ITCTRL, > +}; > + > +static const u32 cti_extended_offset[] = { > + [INDEX_CTIINTACK] = QCOM_CTIINTACK, > + [INDEX_CTIAPPSET] = QCOM_CTIAPPSET, > + [INDEX_CTIAPPCLEAR] = QCOM_CTIAPPCLEAR, > + [INDEX_CTIAPPPULSE] = QCOM_CTIAPPPULSE, > + [INDEX_CTIINEN] = QCOM_CTIINEN, > + [INDEX_CTIOUTEN] = QCOM_CTIOUTEN, > + [INDEX_CTITRIGINSTATUS] = QCOM_CTITRIGINSTATUS, > + [INDEX_CTITRIGOUTSTATUS] = QCOM_CTITRIGOUTSTATUS, > + [INDEX_CTICHINSTATUS] = QCOM_CTICHINSTATUS, > + [INDEX_CTICHOUTSTATUS] = QCOM_CTICHOUTSTATUS, > + [INDEX_CTIGATE] = QCOM_CTIGATE, > + [INDEX_ASICCTL] = QCOM_ASICCTL, > + [INDEX_ITCHINACK] = QCOM_ITCHINACK, > + [INDEX_ITTRIGINACK] = QCOM_ITTRIGINACK, > + [INDEX_ITCHOUT] = QCOM_ITCHOUT, > + [INDEX_ITTRIGOUT] = QCOM_ITTRIGOUT, > + [INDEX_ITCHOUTACK] = QCOM_ITCHOUTACK, > + [INDEX_ITTRIGOUTACK] = QCOM_ITTRIGOUTACK, > + [INDEX_ITCHIN] = QCOM_ITCHIN, > + [INDEX_ITTRIGIN] = QCOM_ITTRIGIN, > + [INDEX_ITCTRL] = CORESIGHT_ITCTRL, > +}; > + > /* > * CTI devices can be associated with a PE, or be connected to CoreSight > * hardware. We have a list of all CTIs irrespective of CPU bound or > @@ -70,15 +118,16 @@ void cti_write_all_hw_regs(struct cti_drvdata *drvdata) > > /* write the CTI trigger registers */ > for (i = 0; i < config->nr_trig_max; i++) { > - writel_relaxed(config->ctiinen[i], drvdata->base + CTIINEN(i)); > + writel_relaxed(config->ctiinen[i], > + drvdata->base + cti_offset(drvdata, INDEX_CTIINEN, i)); > writel_relaxed(config->ctiouten[i], > - drvdata->base + CTIOUTEN(i)); > + drvdata->base + cti_offset(drvdata, INDEX_CTIOUTEN, i)); > } > > /* other regs */ > - writel_relaxed(config->ctigate, drvdata->base + CTIGATE); > - writel_relaxed(config->asicctl, drvdata->base + ASICCTL); > - writel_relaxed(config->ctiappset, drvdata->base + CTIAPPSET); > + writel_relaxed(config->ctigate, drvdata->base + cti_offset(drvdata, INDEX_CTIGATE, 0)); > + writel_relaxed(config->asicctl, drvdata->base + cti_offset(drvdata, INDEX_ASICCTL, 0)); > + writel_relaxed(config->ctiappset, drvdata->base + cti_offset(drvdata, INDEX_CTIAPPSET, 0)); > > /* re-enable CTI */ > writel_relaxed(1, drvdata->base + CTICONTROL); > @@ -99,10 +148,16 @@ static int cti_enable_hw(struct cti_drvdata *drvdata) > if (config->hw_enabled || !config->hw_powered) > goto cti_state_unchanged; > > - /* claim the device */ > - rc = coresight_claim_device(drvdata->csdev); > - if (rc) > - goto cti_err_not_enabled; As mentioned above this change will not be needed once the claimtag code is fixed. > + /* For QCOM CTI, CLAIMCLR and CLAIMSET registers do not follow the > + * CoreSight CLAIM tag protocol, there is no relationship between > + * the CLR and SET pair, it's unused for debug resource arbitration. > + */ > + if (drvdata->subtype == ARM_STD_CTI) { > + /* claim the device */ > + rc = coresight_claim_device(drvdata->csdev); > + if (rc) > + goto cti_err_not_enabled; > + } > > cti_write_all_hw_regs(drvdata); > > @@ -175,7 +230,8 @@ static int cti_disable_hw(struct cti_drvdata *drvdata) > writel_relaxed(0, drvdata->base + CTICONTROL); > config->hw_enabled = false; > > - coresight_disclaim_device_unlocked(csdev); > + if (drvdata->subtype == ARM_STD_CTI) > + coresight_disclaim_device_unlocked(csdev); > CS_LOCK(drvdata->base); > raw_spin_unlock(&drvdata->spinlock); > return ret; > @@ -214,6 +270,9 @@ void cti_write_intack(struct device *dev, u32 ackval) > /* DEVID[19:16] - number of CTM channels */ > #define CTI_DEVID_CTMCHANNELS(devid_val) ((int) BMVAL(devid_val, 16, 19)) > > +/* DEVARCH[31:21] - ARCHITECT */ > +#define CTI_DEVARCH_ARCHITECT(devarch_val) ((int)BMVAL(devarch_val, 21, 31)) > + > static void cti_set_default_config(struct device *dev, > struct cti_drvdata *drvdata) > { > @@ -270,8 +329,10 @@ int cti_add_connection_entry(struct device *dev, struct cti_drvdata *drvdata, > cti_dev->nr_trig_con++; > > /* add connection usage bit info to overall info */ > - drvdata->config.trig_in_use |= tc->con_in->used_mask; > - drvdata->config.trig_out_use |= tc->con_out->used_mask; > + bitmap_or(drvdata->config.trig_in_use, drvdata->config.trig_in_use, > + tc->con_in->used_mask, drvdata->config.nr_trig_max); > + bitmap_or(drvdata->config.trig_out_use, drvdata->config.trig_out_use, > + tc->con_out->used_mask, drvdata->config.nr_trig_max); > > return 0; > } > @@ -314,7 +375,6 @@ int cti_add_default_connection(struct device *dev, struct cti_drvdata *drvdata) > { > int ret = 0; > int n_trigs = drvdata->config.nr_trig_max; > - u32 n_trig_mask = GENMASK(n_trigs - 1, 0); > struct cti_trig_con *tc = NULL; > > /* > @@ -325,8 +385,9 @@ int cti_add_default_connection(struct device *dev, struct cti_drvdata *drvdata) > if (!tc) > return -ENOMEM; > > - tc->con_in->used_mask = n_trig_mask; > - tc->con_out->used_mask = n_trig_mask; > + bitmap_fill(tc->con_in->used_mask, n_trigs); > + bitmap_fill(tc->con_out->used_mask, n_trigs); > + > ret = cti_add_connection_entry(dev, drvdata, tc, NULL, "default"); > return ret; > } > @@ -339,7 +400,6 @@ int cti_channel_trig_op(struct device *dev, enum cti_chan_op op, > { > struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > struct cti_config *config = &drvdata->config; > - u32 trig_bitmask; > u32 chan_bitmask; > u32 reg_value; > int reg_offset; > @@ -349,25 +409,23 @@ int cti_channel_trig_op(struct device *dev, enum cti_chan_op op, > (trigger_idx >= config->nr_trig_max)) > return -EINVAL; > > - trig_bitmask = BIT(trigger_idx); > - > /* ensure registered triggers and not out filtered */ > if (direction == CTI_TRIG_IN) { > - if (!(trig_bitmask & config->trig_in_use)) > + if (!(test_bit(trigger_idx, config->trig_in_use))) > return -EINVAL; > } else { > - if (!(trig_bitmask & config->trig_out_use)) > + if (!(test_bit(trigger_idx, config->trig_out_use))) > return -EINVAL; > > if ((config->trig_filter_enable) && > - (config->trig_out_filter & trig_bitmask)) > + test_bit(trigger_idx, config->trig_out_filter)) > return -EINVAL; > } > > /* update the local register values */ > chan_bitmask = BIT(channel_idx); > - reg_offset = (direction == CTI_TRIG_IN ? CTIINEN(trigger_idx) : > - CTIOUTEN(trigger_idx)); > + reg_offset = (direction == CTI_TRIG_IN ? cti_offset(drvdata, INDEX_CTIINEN, trigger_idx) : > + cti_offset(drvdata, INDEX_CTIOUTEN, trigger_idx)); > > raw_spin_lock(&drvdata->spinlock); > > @@ -451,19 +509,19 @@ int cti_channel_setop(struct device *dev, enum cti_chan_set_op op, > case CTI_CHAN_SET: > config->ctiappset |= chan_bitmask; > reg_value = config->ctiappset; > - reg_offset = CTIAPPSET; > + reg_offset = cti_offset(drvdata, INDEX_CTIAPPSET, 0); > break; > > case CTI_CHAN_CLR: > config->ctiappset &= ~chan_bitmask; > reg_value = chan_bitmask; > - reg_offset = CTIAPPCLEAR; > + reg_offset = cti_offset(drvdata, INDEX_CTIAPPCLEAR, 0); > break; > > case CTI_CHAN_PULSE: > config->ctiappset &= ~chan_bitmask; > reg_value = chan_bitmask; > - reg_offset = CTIAPPPULSE; > + reg_offset = cti_offset(drvdata, INDEX_CTIAPPPULSE, 0); > break; > > default: > @@ -866,6 +924,7 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id) > struct coresight_desc cti_desc; > struct coresight_platform_data *pdata = NULL; > struct resource *res = &adev->res; > + u32 devarch; > > /* driver data*/ > drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL); > @@ -950,9 +1009,19 @@ static int cti_probe(struct amba_device *adev, const struct amba_id *id) > drvdata->csdev_release = drvdata->csdev->dev.release; > drvdata->csdev->dev.release = cti_device_release; > > + /* qcom_cti*/ > + devarch = readl_relaxed(drvdata->base + CORESIGHT_DEVARCH); > + if (CTI_DEVARCH_ARCHITECT(devarch) == ARCHITECT_QCOM) { > + drvdata->subtype = QCOM_CTI; > + drvdata->offsets = cti_extended_offset; > + } else { > + drvdata->subtype = ARM_STD_CTI; > + drvdata->offsets = cti_normal_offset; > + } > + > /* all done - dec pm refcount */ > pm_runtime_put(&adev->dev); > - dev_info(&drvdata->csdev->dev, "CTI initialized\n"); > + dev_info(&drvdata->csdev->dev, "CTI initialized %d\n", drvdata->subtype); > return 0; > > pm_release: > diff --git a/drivers/hwtracing/coresight/coresight-cti-platform.c b/drivers/hwtracing/coresight/coresight-cti-platform.c > index d0ae10bf6128116900acb0e65d44952996753e79..4bef860a0484d9e72c788e5e9e73ff973dac3365 100644 > --- a/drivers/hwtracing/coresight/coresight-cti-platform.c > +++ b/drivers/hwtracing/coresight/coresight-cti-platform.c > @@ -136,8 +136,8 @@ static int cti_plat_create_v8_etm_connection(struct device *dev, > goto create_v8_etm_out; > > /* build connection data */ > - tc->con_in->used_mask = 0xF0; /* sigs <4,5,6,7> */ > - tc->con_out->used_mask = 0xF0; /* sigs <4,5,6,7> */ > + bitmap_set(tc->con_in->used_mask, 4, 4); /* sigs <4,5,6,7> */ > + bitmap_set(tc->con_out->used_mask, 4, 4); /* sigs <4,5,6,7> */ > > /* > * The EXTOUT type signals from the ETM are connected to a set of input > @@ -194,10 +194,10 @@ static int cti_plat_create_v8_connections(struct device *dev, > goto of_create_v8_out; > > /* Set the v8 PE CTI connection data */ > - tc->con_in->used_mask = 0x3; /* sigs <0 1> */ > + bitmap_set(tc->con_in->used_mask, 0, 2); /* sigs <0 1> */ > tc->con_in->sig_types[0] = PE_DBGTRIGGER; > tc->con_in->sig_types[1] = PE_PMUIRQ; > - tc->con_out->used_mask = 0x7; /* sigs <0 1 2 > */ > + bitmap_set(tc->con_out->used_mask, 0, 3); /* sigs <0 1 2 > */ > tc->con_out->sig_types[0] = PE_EDBGREQ; > tc->con_out->sig_types[1] = PE_DBGRESTART; > tc->con_out->sig_types[2] = PE_CTIIRQ; > @@ -213,7 +213,7 @@ static int cti_plat_create_v8_connections(struct device *dev, > goto of_create_v8_out; > > /* filter pe_edbgreq - PE trigout sig <0> */ > - drvdata->config.trig_out_filter |= 0x1; > + set_bit(0, drvdata->config.trig_out_filter); > > of_create_v8_out: > return ret; > @@ -257,7 +257,7 @@ static int cti_plat_read_trig_group(struct cti_trig_grp *tgrp, > if (!err) { > /* set the signal usage mask */ > for (idx = 0; idx < tgrp->nr_sigs; idx++) > - tgrp->used_mask |= BIT(values[idx]); > + set_bit(values[idx], tgrp->used_mask); > } > > kfree(values); > @@ -331,7 +331,9 @@ static int cti_plat_process_filter_sigs(struct cti_drvdata *drvdata, > > err = cti_plat_read_trig_group(tg, fwnode, CTI_DT_FILTER_OUT_SIGS); > if (!err) > - drvdata->config.trig_out_filter |= tg->used_mask; > + bitmap_or(drvdata->config.trig_out_filter, > + drvdata->config.trig_out_filter, > + tg->used_mask, drvdata->config.nr_trig_max); > > kfree(tg); > return err; > diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c > index 572b80ee96fbf18ec8cf9abc30d109a676dfbc5d..d490e43810ad644be6cc076faae042771f8086d6 100644 > --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c > +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c > @@ -172,9 +172,8 @@ static struct attribute *coresight_cti_attrs[] = { > > /* register based attributes */ > > -/* Read registers with power check only (no enable check). */ > -static ssize_t coresight_cti_reg_show(struct device *dev, > - struct device_attribute *attr, char *buf) > +static ssize_t coresight_cti_mgmt_reg_show(struct device *dev, > + struct device_attribute *attr, char *buf) > { > struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > struct cs_off_attribute *cti_attr = container_of(attr, struct cs_off_attribute, attr); > @@ -189,6 +188,39 @@ static ssize_t coresight_cti_reg_show(struct device *dev, > return sysfs_emit(buf, "0x%x\n", val); > } > > +/* Read registers with power check only (no enable check). */ > +static ssize_t coresight_cti_reg_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > + struct cs_off_attribute *cti_attr = container_of(attr, struct cs_off_attribute, attr); > + u32 val = 0, idx = drvdata->config.regs_idx; > + > + pm_runtime_get_sync(dev->parent); > + raw_spin_lock(&drvdata->spinlock); > + if (drvdata->config.hw_powered) { > + switch (cti_attr->off) { > + case INDEX_CTITRIGINSTATUS: > + case INDEX_CTITRIGOUTSTATUS: > + case INDEX_ITTRIGINACK: > + case INDEX_ITTRIGOUT: > + case INDEX_ITTRIGOUTACK: > + case INDEX_ITTRIGIN: > + val = readl_relaxed(drvdata->base + > + cti_offset(drvdata, cti_attr->off, idx)); > + break; > + > + default: > + val = readl_relaxed(drvdata->base + cti_offset(drvdata, cti_attr->off, 0)); > + break; > + } > + } > + > + raw_spin_unlock(&drvdata->spinlock); > + pm_runtime_put_sync(dev->parent); > + return sysfs_emit(buf, "0x%x\n", val); > +} > + > /* Write registers with power check only (no enable check). */ > static __maybe_unused ssize_t coresight_cti_reg_store(struct device *dev, > struct device_attribute *attr, > @@ -197,19 +229,38 @@ static __maybe_unused ssize_t coresight_cti_reg_store(struct device *dev, > struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > struct cs_off_attribute *cti_attr = container_of(attr, struct cs_off_attribute, attr); > unsigned long val = 0; > + u32 idx = drvdata->config.regs_idx; > > if (kstrtoul(buf, 0, &val)) > 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); > + if (drvdata->config.hw_powered) { > + switch (cti_attr->off) { > + case INDEX_ITTRIGINACK: > + case INDEX_ITTRIGOUT: > + cti_write_single_reg(drvdata, cti_offset(drvdata, cti_attr->off, idx), val); > + break; > + > + default: > + cti_write_single_reg(drvdata, cti_offset(drvdata, cti_attr->off, 0), val); > + break; > + } > + } > raw_spin_unlock(&drvdata->spinlock); > pm_runtime_put_sync(dev->parent); > return size; > } > > +#define coresight_cti_mgmt_reg(name, offset) \ > + (&((struct cs_off_attribute[]) { \ > + { \ > + __ATTR(name, 0444, coresight_cti_mgmt_reg_show, NULL), \ > + offset \ > + } \ > + })[0].attr.attr) > + > #define coresight_cti_reg(name, offset) \ > (&((struct cs_off_attribute[]) { \ > { \ > @@ -237,17 +288,17 @@ static __maybe_unused ssize_t coresight_cti_reg_store(struct device *dev, > > /* coresight management registers */ > static struct attribute *coresight_cti_mgmt_attrs[] = { > - coresight_cti_reg(devaff0, CTIDEVAFF0), > - coresight_cti_reg(devaff1, CTIDEVAFF1), > - coresight_cti_reg(authstatus, CORESIGHT_AUTHSTATUS), > - coresight_cti_reg(devarch, CORESIGHT_DEVARCH), > - coresight_cti_reg(devid, CORESIGHT_DEVID), > - coresight_cti_reg(devtype, CORESIGHT_DEVTYPE), > - coresight_cti_reg(pidr0, CORESIGHT_PERIPHIDR0), > - coresight_cti_reg(pidr1, CORESIGHT_PERIPHIDR1), > - coresight_cti_reg(pidr2, CORESIGHT_PERIPHIDR2), > - coresight_cti_reg(pidr3, CORESIGHT_PERIPHIDR3), > - coresight_cti_reg(pidr4, CORESIGHT_PERIPHIDR4), > + coresight_cti_mgmt_reg(devaff0, CTIDEVAFF0), > + coresight_cti_mgmt_reg(devaff1, CTIDEVAFF1), > + coresight_cti_mgmt_reg(authstatus, CORESIGHT_AUTHSTATUS), > + coresight_cti_mgmt_reg(devarch, CORESIGHT_DEVARCH), > + coresight_cti_mgmt_reg(devid, CORESIGHT_DEVID), > + coresight_cti_mgmt_reg(devtype, CORESIGHT_DEVTYPE), > + coresight_cti_mgmt_reg(pidr0, CORESIGHT_PERIPHIDR0), > + coresight_cti_mgmt_reg(pidr1, CORESIGHT_PERIPHIDR1), > + coresight_cti_mgmt_reg(pidr2, CORESIGHT_PERIPHIDR2), > + coresight_cti_mgmt_reg(pidr3, CORESIGHT_PERIPHIDR3), > + coresight_cti_mgmt_reg(pidr4, CORESIGHT_PERIPHIDR4), > NULL, > }; > > @@ -258,13 +309,15 @@ static struct attribute *coresight_cti_mgmt_attrs[] = { > * If inaccessible & pcached_val not NULL then show cached value. > */ > static ssize_t cti_reg32_show(struct device *dev, char *buf, > - u32 *pcached_val, int reg_offset) > + u32 *pcached_val, int index) > { > u32 val = 0; > struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > struct cti_config *config = &drvdata->config; > + int reg_offset; > > raw_spin_lock(&drvdata->spinlock); > + reg_offset = cti_offset(drvdata, index, 0); > if ((reg_offset >= 0) && cti_active(config)) { > CS_UNLOCK(drvdata->base); > val = readl_relaxed(drvdata->base + reg_offset); > @@ -284,11 +337,12 @@ static ssize_t cti_reg32_show(struct device *dev, char *buf, > * if reg_offset >= 0 then write through if enabled. > */ > static ssize_t cti_reg32_store(struct device *dev, const char *buf, > - size_t size, u32 *pcached_val, int reg_offset) > + size_t size, u32 *pcached_val, int index) > { > unsigned long val; > struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > struct cti_config *config = &drvdata->config; > + int reg_offset; > > if (kstrtoul(buf, 0, &val)) > return -EINVAL; > @@ -298,6 +352,7 @@ static ssize_t cti_reg32_store(struct device *dev, const char *buf, > if (pcached_val) > *pcached_val = (u32)val; > > + reg_offset = cti_offset(drvdata, index, 0); > /* write through if offset and enabled */ > if ((reg_offset >= 0) && cti_active(config)) > cti_write_single_reg(drvdata, reg_offset, val); > @@ -306,14 +361,14 @@ static ssize_t cti_reg32_store(struct device *dev, const char *buf, > } > > /* Standard macro for simple rw cti config registers */ > -#define cti_config_reg32_rw(name, cfgname, offset) \ > +#define cti_config_reg32_rw(name, cfgname, index) \ > static ssize_t name##_show(struct device *dev, \ > struct device_attribute *attr, \ > char *buf) \ > { \ > struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); \ > return cti_reg32_show(dev, buf, \ > - &drvdata->config.cfgname, offset); \ > + &drvdata->config.cfgname, index); \ > } \ > \ > static ssize_t name##_store(struct device *dev, \ > @@ -322,7 +377,7 @@ static ssize_t name##_store(struct device *dev, \ > { \ > struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); \ > return cti_reg32_store(dev, buf, size, \ > - &drvdata->config.cfgname, offset); \ > + &drvdata->config.cfgname, index); \ > } \ > static DEVICE_ATTR_RW(name) > > @@ -356,6 +411,46 @@ static ssize_t inout_sel_store(struct device *dev, > } > static DEVICE_ATTR_RW(inout_sel); > > +/* > + * QCOM CTI supports up to 128 triggers, there are 6 registers need to be > + * expanded to up to 4 instances, and regs_idx can be used to indicate which > + * one is in use. > + * CTITRIGINSTATUS, CTITRIGOUTSTATUS, > + * ITTRIGIN, ITTRIGOUT, > + * ITTRIGINACK, ITTRIGOUTACK. > + */ > +static ssize_t regs_idx_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + u32 val; > + struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + raw_spin_lock(&drvdata->spinlock); > + val = drvdata->config.regs_idx; > + raw_spin_unlock(&drvdata->spinlock); > + return sprintf(buf, "%d\n", val); > +} > + > +static ssize_t regs_idx_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + unsigned long val; > + struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + if (kstrtoul(buf, 0, &val)) > + return -EINVAL; > + if (val > ((drvdata->config.nr_trig_max + 31) / 32 - 1)) > + return -EINVAL; > + > + raw_spin_lock(&drvdata->spinlock); > + drvdata->config.regs_idx = val; > + raw_spin_unlock(&drvdata->spinlock); > + return size; > +} > +static DEVICE_ATTR_RW(regs_idx); > + > static ssize_t inen_show(struct device *dev, > struct device_attribute *attr, > char *buf) > @@ -389,7 +484,7 @@ static ssize_t inen_store(struct device *dev, > > /* write through if enabled */ > if (cti_active(config)) > - cti_write_single_reg(drvdata, CTIINEN(index), val); > + cti_write_single_reg(drvdata, cti_offset(drvdata, INDEX_CTIINEN, index), val); > raw_spin_unlock(&drvdata->spinlock); > return size; > } > @@ -428,7 +523,7 @@ static ssize_t outen_store(struct device *dev, > > /* write through if enabled */ > if (cti_active(config)) > - cti_write_single_reg(drvdata, CTIOUTEN(index), val); > + cti_write_single_reg(drvdata, cti_offset(drvdata, INDEX_CTIOUTEN, index), val); > raw_spin_unlock(&drvdata->spinlock); > return size; > } > @@ -448,9 +543,9 @@ static ssize_t intack_store(struct device *dev, > } > static DEVICE_ATTR_WO(intack); > > -cti_config_reg32_rw(gate, ctigate, CTIGATE); > -cti_config_reg32_rw(asicctl, asicctl, ASICCTL); > -cti_config_reg32_rw(appset, ctiappset, CTIAPPSET); > +cti_config_reg32_rw(gate, ctigate, INDEX_CTIGATE); > +cti_config_reg32_rw(asicctl, asicctl, INDEX_ASICCTL); > +cti_config_reg32_rw(appset, ctiappset, INDEX_CTIAPPSET); > > static ssize_t appclear_store(struct device *dev, > struct device_attribute *attr, > @@ -504,6 +599,7 @@ static DEVICE_ATTR_WO(apppulse); > */ > static struct attribute *coresight_cti_regs_attrs[] = { > &dev_attr_inout_sel.attr, > + &dev_attr_regs_idx.attr, > &dev_attr_inen.attr, > &dev_attr_outen.attr, > &dev_attr_gate.attr, > @@ -512,20 +608,20 @@ static struct attribute *coresight_cti_regs_attrs[] = { > &dev_attr_appset.attr, > &dev_attr_appclear.attr, > &dev_attr_apppulse.attr, > - coresight_cti_reg(triginstatus, CTITRIGINSTATUS), > - coresight_cti_reg(trigoutstatus, CTITRIGOUTSTATUS), > - coresight_cti_reg(chinstatus, CTICHINSTATUS), > - coresight_cti_reg(choutstatus, CTICHOUTSTATUS), > + coresight_cti_reg(triginstatus, INDEX_CTITRIGINSTATUS), > + coresight_cti_reg(trigoutstatus, INDEX_CTITRIGOUTSTATUS), > + coresight_cti_reg(chinstatus, INDEX_CTICHINSTATUS), > + coresight_cti_reg(choutstatus, INDEX_CTICHOUTSTATUS), > #ifdef CONFIG_CORESIGHT_CTI_INTEGRATION_REGS > - coresight_cti_reg_rw(itctrl, CORESIGHT_ITCTRL), > - coresight_cti_reg(ittrigin, ITTRIGIN), > - coresight_cti_reg(itchin, ITCHIN), > - coresight_cti_reg_rw(ittrigout, ITTRIGOUT), > - coresight_cti_reg_rw(itchout, ITCHOUT), > - coresight_cti_reg(itchoutack, ITCHOUTACK), > - coresight_cti_reg(ittrigoutack, ITTRIGOUTACK), > - coresight_cti_reg_wo(ittriginack, ITTRIGINACK), > - coresight_cti_reg_wo(itchinack, ITCHINACK), > + coresight_cti_reg_rw(itctrl, INDEX_ITCTRL), > + coresight_cti_reg(ittrigin, INDEX_ITTRIGIN), > + coresight_cti_reg(itchin, INDEX_ITCHIN), > + coresight_cti_reg_rw(ittrigout, INDEX_ITTRIGOUT), > + coresight_cti_reg_rw(itchout, INDEX_ITCHOUT), > + coresight_cti_reg(itchoutack, INDEX_ITCHOUTACK), > + coresight_cti_reg(ittrigoutack, INDEX_ITTRIGOUTACK), > + coresight_cti_reg_wo(ittriginack, INDEX_ITTRIGINACK), > + coresight_cti_reg_wo(itchinack, INDEX_ITCHINACK), > #endif > NULL, > }; > @@ -711,10 +807,8 @@ static ssize_t trigout_filtered_show(struct device *dev, > struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > struct cti_config *cfg = &drvdata->config; > int size = 0, nr_trig_max = cfg->nr_trig_max; > - unsigned long mask = cfg->trig_out_filter; > > - if (mask) > - size = bitmap_print_to_pagebuf(true, buf, &mask, nr_trig_max); > + size = bitmap_print_to_pagebuf(true, buf, cfg->trig_out_filter, nr_trig_max); > return size; > } > static DEVICE_ATTR_RO(trigout_filtered); > @@ -926,9 +1020,8 @@ static ssize_t trigin_sig_show(struct device *dev, > struct cti_trig_con *con = (struct cti_trig_con *)ext_attr->var; > struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > struct cti_config *cfg = &drvdata->config; > - unsigned long mask = con->con_in->used_mask; > > - return bitmap_print_to_pagebuf(true, buf, &mask, cfg->nr_trig_max); > + return bitmap_print_to_pagebuf(true, buf, con->con_in->used_mask, cfg->nr_trig_max); > } > > static ssize_t trigout_sig_show(struct device *dev, > @@ -940,9 +1033,8 @@ static ssize_t trigout_sig_show(struct device *dev, > struct cti_trig_con *con = (struct cti_trig_con *)ext_attr->var; > struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent); > struct cti_config *cfg = &drvdata->config; > - unsigned long mask = con->con_out->used_mask; > > - return bitmap_print_to_pagebuf(true, buf, &mask, cfg->nr_trig_max); > + return bitmap_print_to_pagebuf(true, buf, con->con_out->used_mask, cfg->nr_trig_max); > } > > /* convert a sig type id to a name */ > diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h > index 8362a47c939c60399d7f2f3f599f72de4983a6e1..4f85986b42f0c559c7aa2033fb859cb2f6b12186 100644 > --- a/drivers/hwtracing/coresight/coresight-cti.h > +++ b/drivers/hwtracing/coresight/coresight-cti.h > @@ -15,6 +15,7 @@ > #include <linux/types.h> > > #include "coresight-priv.h" > +#include "qcom-cti.h" remove this an include in the .c file. > > struct fwnode_handle; > > @@ -57,7 +58,7 @@ struct fwnode_handle; > * Max of in and out defined in the DEVID register. > * - pick up actual number used from .dts parameters if present. > */ > -#define CTIINOUTEN_MAX 32 > +#define CTIINOUTEN_MAX 128 > > /** > * Group of related trigger signals > @@ -68,7 +69,7 @@ struct fwnode_handle; > */ > struct cti_trig_grp { > int nr_sigs; > - u32 used_mask; > + DECLARE_BITMAP(used_mask, CTIINOUTEN_MAX); > int sig_types[]; > }; > > @@ -147,9 +148,10 @@ struct cti_config { > bool hw_powered; > > /* registered triggers and filtering */ > - u32 trig_in_use; > - u32 trig_out_use; > - u32 trig_out_filter; > + DECLARE_BITMAP(trig_in_use, CTIINOUTEN_MAX); > + DECLARE_BITMAP(trig_out_use, CTIINOUTEN_MAX); > + DECLARE_BITMAP(trig_out_filter, CTIINOUTEN_MAX); > + These declare bitmaps should be changed to unsgined long * to allow dynamic allocation using bitmap_zalloc() / bitmap_free() > bool trig_filter_enable; > u8 xtrig_rchan_sel; > > @@ -160,6 +162,9 @@ struct cti_config { > u32 ctiouten[CTIINOUTEN_MAX]; Change this to a u32 * so the array can be allocated dynamically according to the actual number of registers, along with ctiinen, > u32 ctigate; > u32 asicctl; > + > + /*for qcom_cti, see regs_idx_show*/ > + u32 regs_idx; > }; > > /** > @@ -180,6 +185,8 @@ struct cti_drvdata { > struct cti_config config; > struct list_head node; > void (*csdev_release)(struct device *dev); > + enum cti_subtype subtype; > + const u32 *offsets; > }; > > /* > @@ -233,6 +240,11 @@ struct coresight_platform_data * > coresight_cti_get_platform_data(struct device *dev); > const char *cti_plat_get_node_name(struct fwnode_handle *fwnode); > > +static inline u32 cti_offset(struct cti_drvdata *drvdata, int index, int num) > +{ > + return drvdata->offsets[index] + 4 * num; > +} > + > /* cti powered and enabled */ > static inline bool cti_active(struct cti_config *cfg) > { > diff --git a/drivers/hwtracing/coresight/qcom-cti.h b/drivers/hwtracing/coresight/qcom-cti.h > new file mode 100644 > index 0000000000000000000000000000000000000000..aaf55f3e5c29023acffe96505d9de0278261d1ab > --- /dev/null > +++ b/drivers/hwtracing/coresight/qcom-cti.h > @@ -0,0 +1,60 @@ > +/* SPDX-License-Identifier: GPL-2.0-only > + * > + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries. > + */ > + > +#define ARCHITECT_QCOM 0x477 > + > +/* CTI programming registers */ > +#define QCOM_CTIINTACK 0x020 > +#define QCOM_CTIAPPSET 0x004 > +#define QCOM_CTIAPPCLEAR 0x008 > +#define QCOM_CTIAPPPULSE 0x00C > +#define QCOM_CTIINEN 0x400 > +#define QCOM_CTIOUTEN 0x800 > +#define QCOM_CTITRIGINSTATUS 0x040 > +#define QCOM_CTITRIGOUTSTATUS 0x060 > +#define QCOM_CTICHINSTATUS 0x080 > +#define QCOM_CTICHOUTSTATUS 0x084 > +#define QCOM_CTIGATE 0x088 > +#define QCOM_ASICCTL 0x08c > +/* Integration test registers */ > +#define QCOM_ITCHINACK 0xE70 > +#define QCOM_ITTRIGINACK 0xE80 > +#define QCOM_ITCHOUT 0xE74 > +#define QCOM_ITTRIGOUT 0xEA0 > +#define QCOM_ITCHOUTACK 0xE78 > +#define QCOM_ITTRIGOUTACK 0xEC0 > +#define QCOM_ITCHIN 0xE7C > +#define QCOM_ITTRIGIN 0xEE0 > + everything below here is common and should be in coresight-cti.h > +/* Qcom CTI supports up to 128 triggers*/ > +enum cti_subtype { > + ARM_STD_CTI, > + QCOM_CTI, > +}; > + > +/* These registers are remapped*/ > +enum cti_offset_index { > + INDEX_CTIINTACK, > + INDEX_CTIAPPSET, > + INDEX_CTIAPPCLEAR, > + INDEX_CTIAPPPULSE, > + INDEX_CTIINEN, > + INDEX_CTIOUTEN, > + INDEX_CTITRIGINSTATUS, > + INDEX_CTITRIGOUTSTATUS, > + INDEX_CTICHINSTATUS, > + INDEX_CTICHOUTSTATUS, > + INDEX_CTIGATE, > + INDEX_ASICCTL, > + INDEX_ITCHINACK, > + INDEX_ITTRIGINACK, > + INDEX_ITCHOUT, > + INDEX_ITTRIGOUT, > + INDEX_ITCHOUTACK, > + INDEX_ITTRIGOUTACK, > + INDEX_ITCHIN, > + INDEX_ITTRIGIN, > + INDEX_ITCTRL, > +}; > > --- > base-commit: 8cd53fb40a304576fa86ba985f3045d5c55b0ae3 > change-id: 20250828-extended_cti-d441ec991b80 > > Best regards, > -- > Yingchao Deng <yingchao.deng(a)oss.qualcomm.com> > Thanks and Regards Mike -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

4 months

Re: [PATCH] perf sample: Fix the wrong format specifier

by Mike Leach

On Mon, 22 Sept 2025 at 10:51, liujing <liujing(a)cmss.chinamobile.com> wrote: > > In the file tools/perf/util/cs-etm.c, queue_nr is of type unsigned > int and should be printed with %u. > > Signed-off-by: liujing <liujing(a)cmss.chinamobile.com> > --- > tools/perf/util/cs-etm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c > index 30f4bb3e7fa3..d13575285b19 100644 > --- a/tools/perf/util/cs-etm.c > +++ b/tools/perf/util/cs-etm.c > @@ -777,7 +777,7 @@ static void cs_etm__packet_dump(const char *pkt_string, void *data) > char queue_nr[64]; > > if (verbose) > - snprintf(queue_nr, sizeof(queue_nr), "Qnr:%d; ", etmq->queue_nr); > + snprintf(queue_nr, sizeof(queue_nr), "Qnr:%u; ", etmq->queue_nr); > else > queue_nr[0] = '\0'; > > -- > 2.27.0 > > > > Reviewed-by: Mike Leach <mike.leach(a)linaro.org> -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

4 months

[PATCH 0/6] coresight: Add format attribute for setting the timestamp interval

by James Clark

Do some cleanups then add a new format attribute to set the timestamp interval for ETMv4 in Perf mode. The current interval is too high for most use cases, and particularly on the FVP the number of timestamps generated is excessive. Although it would be good to make only SYNC timestamps the default and have counter timestamps opt-in, this would be a breaking change. We can always do that later, or disable counter timestamps from Perf. This is added as an event format attribute, rather than a Coresight config because it's something that the driver is already configuring automatically in Perf mode with any unused counter, so it's not possible to modify this with a config. Applies to coresight/next Signed-off-by: James Clark <james.clark(a)linaro.org> --- James Clark (6): coresight: Change syncfreq to be a u8 coresight: Fix holes in struct etmv4_config coresight: Repack struct etmv4_drvdata coresight: Refactor etm4_config_timestamp_event() coresight: Add format attribute for setting the timestamp interval coresight: docs: Document etm4x ts_interval Documentation/trace/coresight/coresight.rst | 14 +++ drivers/hwtracing/coresight/coresight-etm-perf.c | 6 +- drivers/hwtracing/coresight/coresight-etm4x-core.c | 110 +++++++++++++-------- drivers/hwtracing/coresight/coresight-etm4x.h | 86 ++++++++++------ 4 files changed, 144 insertions(+), 72 deletions(-) --- base-commit: a80198ba650f50d266d7fc4a6c5262df9970f9f2 change-id: 20250724-james-cs-syncfreq-7c2257a38ed3 Best regards, -- James Clark <james.clark(a)linaro.org>

4 months, 1 week

[PATCH v2 0/6] coresight: Add format attribute for setting the timestamp interval

by James Clark

Do some cleanups then add a new format attribute to set the timestamp interval for ETMv4 in Perf mode. The current interval is too high for most use cases, and particularly on the FVP the number of timestamps generated is excessive. Although it would be good to make only SYNC timestamps the default and have counter timestamps opt-in, this would be a breaking change. We can always do that later, or disable counter timestamps from Perf. This is added as an event format attribute, rather than a Coresight config because it's something that the driver is already configuring automatically in Perf mode with any unused counter, so it's not possible to modify this with a config. Applies to coresight/next Signed-off-by: James Clark <james.clark(a)linaro.org> --- Changes in v2: - Only show the attribute for ETMv4 to improve usability and fix the arm32 build error. Wrapping everything in IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM4X) isn't ideal, but the -perf.c file is shared between ETMv3 and ETMv4, and there is already precedent for doing it this way. - Link to v1: https://lore.kernel.org/r/20250811-james-cs-syncfreq-v1-0-b001cd6e3404@lina… --- James Clark (6): coresight: Change syncfreq to be a u8 coresight: Fix holes in struct etmv4_config coresight: Repack struct etmv4_drvdata coresight: Refactor etm4_config_timestamp_event() coresight: Add format attribute for setting the timestamp interval coresight: docs: Document etm4x ts_interval Documentation/trace/coresight/coresight.rst | 14 +++ drivers/hwtracing/coresight/coresight-etm-perf.c | 13 ++- drivers/hwtracing/coresight/coresight-etm4x-core.c | 110 +++++++++++++-------- drivers/hwtracing/coresight/coresight-etm4x.h | 86 ++++++++++------ 4 files changed, 151 insertions(+), 72 deletions(-) --- base-commit: a80198ba650f50d266d7fc4a6c5262df9970f9f2 change-id: 20250724-james-cs-syncfreq-7c2257a38ed3 Best regards, -- James Clark <james.clark(a)linaro.org>

4 months, 1 week

Re: [PATCH v5 2/3] coresight: Reorder coresight_device_release to match coresight_register

by Mike Leach

On Thu, 25 Sept 2025 at 16:04, Sean Anderson <sean.anderson(a)linux.dev> wrote: > > To make it easier to determine where to add new release actions, reorder > the actions in coresight_device_release to be the reverse of > coresight_register. > > Signed-off-by: Sean Anderson <sean.anderson(a)linux.dev> > --- > > Changes in v5: > - New > > drivers/hwtracing/coresight/coresight-core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > index 022c8384b98d..305b1773cfbe 100644 > --- a/drivers/hwtracing/coresight/coresight-core.c > +++ b/drivers/hwtracing/coresight/coresight-core.c > @@ -1046,8 +1046,8 @@ static void coresight_device_release(struct device *dev) > { > struct coresight_device *csdev = to_coresight_device(dev); > > - fwnode_handle_put(csdev->dev.fwnode); > free_percpu(csdev->perf_sink_id_map.cpu_map); > + fwnode_handle_put(csdev->dev.fwnode); > kfree(csdev); > } > > -- > 2.35.1.1320.gc452695387.dirty > Reviewed-by: Mike Leach <mike.leach(a)linaro.org> -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

4 months, 2 weeks

Re: [PATCH v5 1/3] coresight: Fix fwnode leak in coresight_register error path

by Mike Leach

On Thu, 25 Sept 2025 at 16:04, Sean Anderson <sean.anderson(a)linux.dev> wrote: > > If registering the CPU map fails, we need to put the fwnode. free_percpu > works when called with a NULL pointer, so just use > coresight_device_release. > > Fixes: 5ad628a76176 ("coresight: Use per-sink trace ID maps for Perf sessions") > Signed-off-by: Sean Anderson <sean.anderson(a)linux.dev> > --- > > (no changes since v4) > > Changes in v4: > - New > > drivers/hwtracing/coresight/coresight-core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > index fa758cc21827..022c8384b98d 100644 > --- a/drivers/hwtracing/coresight/coresight-core.c > +++ b/drivers/hwtracing/coresight/coresight-core.c > @@ -1352,7 +1352,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc) > raw_spin_lock_init(&csdev->perf_sink_id_map.lock); > csdev->perf_sink_id_map.cpu_map = alloc_percpu(atomic_t); > if (!csdev->perf_sink_id_map.cpu_map) { > - kfree(csdev); > + coresight_device_release(&csdev->dev); > ret = -ENOMEM; > goto err_out; > } > -- > 2.35.1.1320.gc452695387.dirty > Reviewed-by: Mike Leach <mike.leach(a)linaro.org> -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

4 months, 2 weeks

Re: [PATCH RFC 1/3] coresight: tmc: add the handle of the event to the path

by Mike Leach

Hi, On Tue, 23 Sept 2025 at 02:49, Jie Gan <jie.gan(a)oss.qualcomm.com> wrote: > > > > On 9/23/2025 1:31 AM, Carl Worth wrote: > > Jie Gan <jie.gan(a)oss.qualcomm.com> writes: > >> From: Carl Worth <carl(a)os.amperecomputing.com> > >> > >> The handle is essential for retrieving the AUX_EVENT of each CPU and is > >> required in perf mode. It has been added to the coresight_path so that > >> dependent devices can access it from the path when needed. > > > > I'd still like to have the original command I used to trigger the bug in > > the commit message. I really like having reproduction steps captured in > > commit messages when I look back at commits in the future. So, that was: > > > > perf record -e cs_etm//k -C 0-9 dd if=/dev/zero of=/dev/null > > > > Sure, I’ll include your commit message in the formal patch series, I > think it's V3 since you have submitted two versions, if you're okay with > me sending it out. > > >> /** > >> * struct coresight_path - data needed by enable/disable path > >> - * @path_list: path from source to sink. > >> - * @trace_id: trace_id of the whole path. > >> + * @path_list: path from source to sink. > >> + * @trace_id: trace_id of the whole path. > >> + * struct perf_output_handle: handle of the aux_event. > >> */ > > > > Fixing to "@handle" was mentioned in another comment already. > > > > Something about the above still feels a little off to me. It feels like > > we're throwing new data into a structure just because it happens to be > > conveniently at hand for the code paths we're needing, and not because > > it really _belongs_ there. > > > This data is perf specific - not path generic; so I agree that this structure should go elsewhere. I would suggest in the csdev (coresight_device) structure itself. We already have some sink specific data in here e.g. perf_sink_id_map. This could then be set/clear in the functions coresight-etm-perf.c file, where there is a significant amount of code dealing with the perf handle and ensuring it is valid and in scope. This can then be set only when appropriate - for source / sink devices and only when in perf mode, and avoid the need to pass the handle around as API call parameters. Regards Mike. > The core idea behind coresight_path is that it can hold all the data > potentially needed by any device along the path. > > For example with the path ETM->Link->ETR->CATU: > > All the mentioned devices operate by forming a path, for which the > system constructs a coresight_path. This 'path' is then passed to each > device along the route, allowing any device to directly access the > required data from coresight_path instead of receiving it as a separate > argument. > > Imagine a device that requires more than two or three arguments, and you > want to pass them through coresight_enable_path or similar functions... > > For certain coresight_path instances, we may not need the handle or > other parameters. Since these values are initialized, it's acceptable to > leave them as NULL or 0. > > > > Or, maybe it's the right place for it, and the cause of my concern is > > that "path" is an overly-narrow name in struct coresight_path? > > > > It defines the direction of data flow—serving as the path for trace data. > > Thanks, > Jie > > > But if a renaming of this structure would improve the code, I'd also be > > fine with that happening in a subsequent commit, so I won't try to hold > > up the current series based on that. > > > > -Carl > -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

4 months, 2 weeks

Re: [PATCH v5 3/3] coresight: Fix possible deadlock in coresight_panic_cb

by Leo Yan

On Thu, Sep 25, 2025 at 11:03:42AM -0400, Sean Anderson wrote: > Panics can occur at any time, so taking locks may cause a deadlock (such > as if the panicking CPU held the lock). coresight_panic_cb uses > bus_for_each_dev, but that calls bus_to_subsys which takes > bus_kset->list_lock. > > Instead of registering a single panic notifier and iterating over > coresight devices, register a panic notifier for each coresight device > that requires it (letting the atomic notifier list handle iteration). > atomic_notifier_chain_unregister will just return -ENOENT if a notifier > block isn't on the list, so it's safe to call when we haven't registered > a notifier. > > Fixes: 46006ceb5d02 ("coresight: core: Add provision for panic callbacks") > Signed-off-by: Sean Anderson <sean.anderson(a)linux.dev> Reviewed-by: Leo Yan <leo.yan(a)arm.com>

4 months, 2 weeks

Re: [PATCH RFC 1/3] coresight: tmc: add the handle of the event to the path

by Carl Worth

Jie Gan <jie.gan(a)oss.qualcomm.com> writes: > On 9/23/2025 1:31 AM, Carl Worth wrote: >> I'd still like to have the original command I used to trigger the bug in >> the commit message. I really like having reproduction steps captured in >> commit messages when I look back at commits in the future. So, that was: >> >> perf record -e cs_etm//k -C 0-9 dd if=/dev/zero of=/dev/null > > Sure, I’ll include your commit message in the formal patch series, I > think it's V3 since you have submitted two versions, if you're okay with > me sending it out. Yes. Please do. And thank you. > The core idea behind coresight_path is that it can hold all the data > potentially needed by any device along the path. > > For example with the path ETM->Link->ETR->CATU: OK. That makes sense to me. The name of coresight_path definitely threw me off, since I interpreted it as being a description of the path, not a container for data to be consumed along the path. -Carl

4 months, 2 weeks

Re: [UPDATED] [GIT PULL] coresight: Updates for Linux v6.18

by Suzuki K Poulose

On 24/09/2025 12:06, Greg KH wrote: > On Wed, Sep 24, 2025 at 09:22:42AM +0100, Suzuki K Poulose wrote: >> Hi Greg >> >> Please find the updated pull request to fix the invalid commit reference in >> the fixes tag, in the original pull request [0] >> >> Apologies for the inconvenience. >> >> [0] https://lkml.kernel.org/r/20250922125907.2268152-1-suzuki.poulose@arm.com >> >> Kindly pull, >> >> Suzuki >> >> --- >> >> The following changes since commit 1b237f190eb3d36f52dffe07a40b5eb210280e00: >> >> Linux 6.17-rc3 (2025-08-24 12:04:12 -0400) >> >> are available in the Git repository at: >> >> git//git.kernel.org/pub/scm/linux/kernel/git/coresight/linux.git tags/coresight-next-v6.18-v2 > > That is not a valid "url" to pull from, how did you create this? Dang! I did use the git request-pull, but have to manually replace the "ssh" URL with git:// :-( and I messed up. > > I've added the missing ':', but you might want to check your scripts to > verify you aren't doing something odd. Will take care of it, thanks. Suzuki

4 months, 2 weeks

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight