| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJ9a7Vhh_DV1McYuLp9y4hCuzj7sdYNhd2g7vT32B_pN0L3YPw@mail.gmail.com>
Date: Tue, 5 Aug 2025 13:20:52 +0100
From: Mike Leach <mike.leach@...aro.org>
To: Mao Jinlong <quic_jinlmao@...cinc.com>
Cc: Suzuki K Poulose <suzuki.poulose@....com>, James Clark <james.clark@...aro.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Yingchao Deng <quic_yingdeng@...cinc.com>, coresight@...ts.linaro.org,
linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v3 2/2] coresight: cti: Add Qualcomm extended CTI support
Hi
On Mon, 4 Aug 2025 at 21:49, Mike Leach <mike.leach@...aro.org> wrote:
>
> Hi
>
> On Tue, 22 Jul 2025 at 09:14, Mao Jinlong <quic_jinlmao@...cinc.com> wrote:
> >
> > From: Yingchao Deng <quic_yingdeng@...cinc.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. For Qualcomm
> > extended CTI, it supports up to 128 triggers.
> >
> > Signed-off-by: Yingchao Deng <quic_yingdeng@...cinc.com>
> > Signed-off-by: Mao Jinlong <quic_jinlmao@...cinc.com>
> > ---
> > .../hwtracing/coresight/coresight-cti-core.c | 127 +++++++++++++----
> > .../coresight/coresight-cti-platform.c | 16 ++-
> > .../hwtracing/coresight/coresight-cti-sysfs.c | 128 ++++++++++++++----
> > drivers/hwtracing/coresight/coresight-cti.h | 75 +++++-----
> > 4 files changed, 245 insertions(+), 101 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c b/drivers/hwtracing/coresight/coresight-cti-core.c
> > index 8fb30dd73fd2..d6a28df48484 100644
> > --- a/drivers/hwtracing/coresight/coresight-cti-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-cti-core.c
> > @@ -15,6 +15,7 @@
> > #include <linux/kernel.h>
> > #include <linux/list.h>
> > #include <linux/mutex.h>
> > +#include <linux/of.h>
> > #include <linux/pm_runtime.h>
> > #include <linux/property.h>
> > #include <linux/spinlock.h>
> > @@ -22,6 +23,54 @@
> > #include "coresight-priv.h"
> > #include "coresight-cti.h"
> >
> > +#define CTI_EXTENDED "qcom,coresight-cti-extended"
> > +
> > +static const int cti_normal_offset[] = {
> > + [CTIINTACK] = 0x010,
> > + [CTIAPPSET] = 0x014,
> > + [CTIAPPCLEAR] = 0x018,
> > + [CTIAPPPULSE] = 0x01C,
> > + [CTIINEN] = 0x020,
> > + [CTIOUTEN] = 0x0A0,
> > + [CTITRIGINSTATUS] = 0x130,
> > + [CTITRIGOUTSTATUS] = 0x134,
> > + [CTICHINSTATUS] = 0x138,
> > + [CTICHOUTSTATUS] = 0x13C,
> > + [CTIGATE] = 0x140,
> > + [ASICCTL] = 0x144,
> > + [ITCHINACK] = 0xEDC,
> > + [ITTRIGINACK] = 0xEE0,
> > + [ITCHOUT] = 0xEE4,
> > + [ITTRIGOUT] = 0xEE8,
> > + [ITCHOUTACK] = 0xEEC,
> > + [ITTRIGOUTACK] = 0xEF0,
> > + [ITCHIN] = 0xEF4,
> > + [ITTRIGIN] = 0xEF8,
> > +};
> > +
>
> Why not all CTI registers in these arrays?
> Do not use hardcoded values in here - use the #defines from the header file.
>
>
> > +static const int cti_extended_offset[] = {
> > + [CTIINTACK] = 0x020,
> > + [CTIAPPSET] = 0x004,
> > + [CTIAPPCLEAR] = 0x008,
> > + [CTIAPPPULSE] = 0x00C,
> > + [CTIINEN] = 0x400,
> > + [CTIOUTEN] = 0x800,
> > + [CTITRIGINSTATUS] = 0x040,
> > + [CTITRIGOUTSTATUS] = 0x060,
> > + [CTICHINSTATUS] = 0x080,
> > + [CTICHOUTSTATUS] = 0x084,
> > + [CTIGATE] = 0x088,
> > + [ASICCTL] = 0x08c,
> > + [ITCHINACK] = 0xE70,
> > + [ITTRIGINACK] = 0xE80,
> > + [ITCHOUT] = 0xE74,
> > + [ITTRIGOUT] = 0xEA0,
> > + [ITCHOUTACK] = 0xE78,
> > + [ITTRIGOUTACK] = 0xEC0,
> > + [ITCHIN] = 0xE7C,
> > + [ITTRIGIN] = 0xEE0,
> > +};
> > +
>
> Again use #defines from header - in this case prehaps a new
> qcom-cti.h, #define values QCOM_CTINTACK etc...
>
> > /*
> > * 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
> > @@ -57,6 +106,12 @@ static struct cti_drvdata *cti_cpu_drvdata[NR_CPUS];
> > */
> > DEFINE_CORESIGHT_DEVLIST(cti_sys_devs, "cti_sys");
> >
> > +u32 cti_offset(struct cti_drvdata *drvdata, int index, int num)
> > +{
> > + return (drvdata->is_extended_cti ? cti_extended_offset[index]
> > + : cti_normal_offset[index]) + (4 * num);
>
> have a pointer to the offset array in drvdata and set it at probe
> time. This fn could probalb ybe inlines and then becomes
>
> return drvdata->offsets[index] + (4 * num);
>
>
> > +}
> > +
> > /* write set of regs to hardware - call with spinlock claimed */
> > void cti_write_all_hw_regs(struct cti_drvdata *drvdata)
> > {
> > @@ -70,15 +125,15 @@ 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, CTIINEN, i));
> > writel_relaxed(config->ctiouten[i],
> > - drvdata->base + CTIOUTEN(i));
> > + drvdata->base + cti_offset(drvdata, 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, CTIGATE, 0));
> > + writel_relaxed(config->asicctl, drvdata->base + cti_offset(drvdata, ASICCTL, 0));
> > + writel_relaxed(config->ctiappset, drvdata->base + cti_offset(drvdata, CTIAPPSET, 0));
> >
> > /* re-enable CTI */
> > writel_relaxed(1, drvdata->base + CTICONTROL);
> > @@ -99,10 +154,13 @@ 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;
> > + /* There is no relationship between the CLR and SET pair for extended CTI. */
>
> What does this mean? - these are CoreSight required management
> registers required by software to claim the resource. Are they not
> present of this device or has the functionality of these registers
> been dropped.
> Either way this is no longer a Coresight compatible CTI device without these.
Quick clarification - after dicussions with the CoreSight architect in
ARM, these locations must be readable, but can return 0 indicating no
claim tags present.
>
> > + if (!drvdata->is_extended_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 +233,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->is_extended_cti)
> > + coresight_disclaim_device_unlocked(csdev);
> > CS_LOCK(drvdata->base);
> > raw_spin_unlock(&drvdata->spinlock);
> > return ret;
> > @@ -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, CTIINEN, trigger_idx) :
> > + cti_offset(drvdata, 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, CTIAPPSET, 0);
> > break;
> >
> > case CTI_CHAN_CLR:
> > config->ctiappset &= ~chan_bitmask;
> > reg_value = chan_bitmask;
> > - reg_offset = CTIAPPCLEAR;
> > + reg_offset = cti_offset(drvdata, CTIAPPCLEAR, 0);
> > break;
> >
> > case CTI_CHAN_PULSE:
> > config->ctiappset &= ~chan_bitmask;
> > reg_value = chan_bitmask;
> > - reg_offset = CTIAPPPULSE;
> > + reg_offset = cti_offset(drvdata, CTIAPPPULSE, 0);
> > break;
> >
> > default:
> > @@ -857,6 +915,19 @@ static void cti_remove(struct amba_device *adev)
> > coresight_unregister(drvdata->csdev);
> > }
> >
> > +static bool of_is_extended_cti(struct device *dev)
> > +{
> > + struct fwnode_handle *fwnode = dev_fwnode(dev);
> > +
> > + if (is_of_node(fwnode)) {
> > + if (of_device_is_compatible(to_of_node(fwnode),
> > + CTI_EXTENDED))
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > static int cti_probe(struct amba_device *adev, const struct amba_id *id)
> > {
> > int ret = 0;
> > @@ -950,9 +1021,11 @@ 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;
> >
> > + drvdata->is_extended_cti = of_is_extended_cti(dev);
>
> Better as drvdata->cti_subtype - see comments below.
> > +
> > /* 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->is_extended_cti);
> > return 0;
> >
> > pm_release:
> > diff --git a/drivers/hwtracing/coresight/coresight-cti-platform.c b/drivers/hwtracing/coresight/coresight-cti-platform.c
> > index d0ae10bf6128..4bef860a0484 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 572b80ee96fb..cc680a4b900a 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,53 @@ 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);
> > + struct cti_config *cfg = &drvdata->config;
> > + u32 val = 0;
> > + int i, num;
> > + ssize_t size = 0;
> > +
> > + pm_runtime_get_sync(dev->parent);
> > + raw_spin_lock(&drvdata->spinlock);
> > + if (drvdata->config.hw_powered) {
> > + switch (cti_attr->off) {
> > + case CTICHINSTATUS:
> > + case CTICHOUTSTATUS:
> > + case ITCHINACK:
> > + case ITCHOUT:
> > + case ITCHOUTACK:
> > + case ITCHIN:
> > + val = readl_relaxed(drvdata->base + cti_offset(drvdata, cti_attr->off, 0));
> > + size += sysfs_emit(buf, "0x%x\n", val);
> > + break;
> > +
> > + case CTITRIGINSTATUS:
> > + case CTITRIGOUTSTATUS:
> > + case ITTRIGINACK:
> > + case ITTRIGOUT:
> > + case ITTRIGOUTACK:
> > + case ITTRIGIN:
> > + num = (cfg->nr_trig_max - 1) / 32;
> > + for (i = 0; i <= num; i++) {
> > + val = readl_relaxed(drvdata->base +
> > + cti_offset(drvdata, cti_attr->off, i));
> > + size += sysfs_emit_at(buf, size, "0x%x ", val);
> > + }
> > + if (size > 0)
> > + buf[size - 1] = '\n';
> > + break;
> > + }
> > + }
> > + raw_spin_unlock(&drvdata->spinlock);
> > + pm_runtime_put_sync(dev->parent);
> > + return size;
> > +}
> > +
> > /* 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 +243,45 @@ 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;
> > + int num, i;
> >
> > 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 ITCHINACK:
> > + case ITCHOUT:
> > + cti_write_single_reg(drvdata, cti_offset(drvdata, cti_attr->off, 0), val);
> > + break;
> > +
> > + case ITTRIGINACK:
> > + case ITTRIGOUT:
> > + case ITTRIGOUTACK:
> > + num = val / 32;
>
> Why?
>
> > + i = val % 32;
>
> Why? - "val" is the 32 bit value you want ot put into a 32 bit register.
>
> > + for (i = 0; i <= num; i++)
> > + cti_write_single_reg(drvdata,
> > + cti_offset(drvdata, cti_attr->off, i), BIT(i));
>
> I cannot understand what this code is trying to do. Why is this
> attempting to write to multiple registers with some BIT(i) value
> rather than the input "val"
> e.g. The complete 32biut value "val" should be written to ITTRIGOUT -
> which is what the original code did. This breaks the original
> functionality
>
> Has this code been tested?
>
> > + 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 +309,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,
> > };
> >
> > @@ -284,11 +356,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 +371,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 +380,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) \
>
> Why the pointless change of a parameter name in a macro?
>
> > 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 +396,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)
> >
> > @@ -389,7 +463,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, CTIINEN, index), val);
> > raw_spin_unlock(&drvdata->spinlock);
> > return size;
> > }
> > @@ -428,7 +502,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, CTIOUTEN, index), val);
> > raw_spin_unlock(&drvdata->spinlock);
> > return size;
> > }
> > @@ -711,10 +785,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 +998,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 +1011,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 8362a47c939c..7d052e76d116 100644
> > --- a/drivers/hwtracing/coresight/coresight-cti.h
> > +++ b/drivers/hwtracing/coresight/coresight-cti.h
> > @@ -18,46 +18,42 @@
> >
> > struct fwnode_handle;
> >
> > -/*
> > - * Device registers
> > - * 0x000 - 0x144: CTI programming and status
> > - * 0xEDC - 0xEF8: CTI integration test.
> > - * 0xF00 - 0xFFC: Coresight management registers.
> > - */
> > -/* CTI programming registers */
> > -#define CTICONTROL 0x000
> > -#define CTIINTACK 0x010
> > -#define CTIAPPSET 0x014
> > -#define CTIAPPCLEAR 0x018
> > -#define CTIAPPPULSE 0x01C
> > -#define CTIINEN(n) (0x020 + (4 * n))
> > -#define CTIOUTEN(n) (0x0A0 + (4 * n))
> > -#define CTITRIGINSTATUS 0x130
> > -#define CTITRIGOUTSTATUS 0x134
> > -#define CTICHINSTATUS 0x138
> > -#define CTICHOUTSTATUS 0x13C
> > -#define CTIGATE 0x140
> > -#define ASICCTL 0x144
> > -/* Integration test registers */
> > -#define ITCHINACK 0xEDC /* WO CTI CSSoc 400 only*/
> > -#define ITTRIGINACK 0xEE0 /* WO CTI CSSoc 400 only*/
> > -#define ITCHOUT 0xEE4 /* WO RW-600 */
> > -#define ITTRIGOUT 0xEE8 /* WO RW-600 */
> > -#define ITCHOUTACK 0xEEC /* RO CTI CSSoc 400 only*/
> > -#define ITTRIGOUTACK 0xEF0 /* RO CTI CSSoc 400 only*/
> > -#define ITCHIN 0xEF4 /* RO */
> > -#define ITTRIGIN 0xEF8 /* RO */
> > -/* management registers */
> > -#define CTIDEVAFF0 0xFA8
> > -#define CTIDEVAFF1 0xFAC
> > -
>
> Do not remove these from this file. Use them in the .c file to
> populate the offset array. See above comments
>
>
> > /*
> > * CTI CSSoc 600 has a max of 32 trigger signals per direction.
> > * CTI CSSoc 400 has 8 IO triggers - other CTIs can be impl def.
> > * 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
> > +
> > +#define CTICONTROL 0x000
> > +
> > +/* management registers */
> > +#define CTIDEVAFF0 0xFA8
> > +#define CTIDEVAFF1 0xFAC
> > +
> > +enum cti_offset_index {
> > + CTIINTACK,
> > + CTIAPPSET,
> > + CTIAPPCLEAR,
> > + CTIAPPPULSE,
> > + CTIINEN,
> > + CTIOUTEN,
> > + CTITRIGINSTATUS,
> > + CTITRIGOUTSTATUS,
> > + CTICHINSTATUS,
> > + CTICHOUTSTATUS,
> > + CTIGATE,
> > + ASICCTL,
> > + ITCHINACK,
> > + ITTRIGINACK,
> > + ITCHOUT,
> > + ITTRIGOUT,
> > + ITCHOUTACK,
> > + ITTRIGOUTACK,
> > + ITCHIN,
> > + ITTRIGIN,
> > +};
> >
> > /**
> > * Group of related trigger signals
> > @@ -68,7 +64,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 +143,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);
> > +
> > bool trig_filter_enable;
> > u8 xtrig_rchan_sel;
> >
> > @@ -180,6 +177,7 @@ struct cti_drvdata {
> > struct cti_config config;
> > struct list_head node;
> > void (*csdev_release)(struct device *dev);
> > + bool is_extended_cti;
>
> use an enum type here - e.g. enum cti_subtype { ARM_STD_CTI, QCOM_CTI };
>
>
> > };
> >
> > /*
> > @@ -232,6 +230,7 @@ int cti_create_cons_sysfs(struct device *dev, struct cti_drvdata *drvdata);
> > struct coresight_platform_data *
> > coresight_cti_get_platform_data(struct device *dev);
> > const char *cti_plat_get_node_name(struct fwnode_handle *fwnode);
> > +u32 cti_offset(struct cti_drvdata *drvdata, int index, int num);
> >
> > /* cti powered and enabled */
> > static inline bool cti_active(struct cti_config *cfg)
> > --
> > 2.25.1
> >
>
> Regards
>
> Mike
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK
Further to the ID regs issues, we also have the following clarificaiton :-
" The CIDRn registers I expect to be common across all CoreSight
components. However it's the PIDRn registers that really matter
because that's where the designer+partnum are indicated.
- CIDRn values are the same for all CoreSight compliant components
- DEVARCH needs to be different between Arm CTI and QC CTI because
the programmers model is different between the 2 CTIs.
- DEVTYPE I don't care much for (DEVARCH is more useful).
- PIDRn needs to indicate this is a QC designed part with a
QC-allocated part number."
Regards
Mike
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
Powered by blists - more mailing lists