diff mbox series

[v5,03/14] coresight: cti: Add sysfs access to program function regs

Message ID 20191119231912.12768-4-mike.leach@linaro.org (mailing list archive)
State New, archived
Headers show
Series CoreSight CTI Driver | expand

Commit Message

Mike Leach Nov. 19, 2019, 11:19 p.m. UTC 
Adds in sysfs programming support for the CTI function register sets.
Allows direct manipulation of channel / trigger association registers.

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Mike Leach <mike.leach@linaro.org>
---
 .../hwtracing/coresight/coresight-cti-sysfs.c | 362 ++++++++++++++++++
 drivers/hwtracing/coresight/coresight-cti.c   |  19 +
 drivers/hwtracing/coresight/coresight-cti.h   |   5 +
 3 files changed, 386 insertions(+)

Comments

Suzuki K Poulose Nov. 27, 2019, 6:26 p.m. UTC | #1 
On 19/11/2019 23:19, Mike Leach wrote:
> Adds in sysfs programming support for the CTI function register sets.
> Allows direct manipulation of channel / trigger association registers.
> 
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Mike Leach <mike.leach@linaro.org>
> ---
>   .../hwtracing/coresight/coresight-cti-sysfs.c | 362 ++++++++++++++++++
>   drivers/hwtracing/coresight/coresight-cti.c   |  19 +
>   drivers/hwtracing/coresight/coresight-cti.h   |   5 +
>   3 files changed, 386 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> index 507f8eb487fe..02d3ee0c1278 100644
> --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> @@ -109,6 +109,362 @@ static struct attribute *coresight_cti_mgmt_attrs[] = {
>   	NULL,
>   };
>   
> +/* CTI low level programming registers */
> +
> +/*
> + * Show a simple 32 bit value if enabled and powered.
> + * If inaccessible & pcached_val not NULL then show cached value.
> + */

Also I am not sure if it makes sense to mention that the value is cached.

> +static ssize_t cti_reg32_show(struct device *dev, char *buf,
> +			      u32 *pcached_val, int reg_offset)
> +{
> +	u32 val = 0;
   +	char *state = "";

> +	struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	struct cti_config *config = &drvdata->config;
> +
> +	spin_lock(&drvdata->spinlock);
> +	if ((reg_offset >= 0) && CTI_PWR_ENA(config)) {

minor nit: Personally I don't like the naming here. This could simply 
be: cti_accessible(config) , may be defined as a static inline function
instead of a macro:

static inline bool cti_accessible(struct cti_drvdata *drvdata)
{
	struct cti_config *cfg = &drvdata->config;

	return cfg->hw_powered && cfg->hw_enabled;
}


> +		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;

   +		state = " (cached)";
> +	}
> +	spin_unlock(&drvdata->spinlock);
> +	return scnprintf(buf, PAGE_SIZE, "%#x\n", val);
   +	return scnprintf(buf, PAGE_SIZE, "%#x%s\n", val, state);

> +}
> +
> +/*
> + * Store a simple 32 bit value.
> + * If pcached_val not NULL, then copy to here too,
> + * 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)


> +static ssize_t appclear_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);
> +	struct cti_config *config = &drvdata->config;
> +
> +	if (kstrtoul(buf, 0, &val))
> +		return -EINVAL;
> +
> +	spin_lock(&drvdata->spinlock);
> +
> +	/* a 1'b1 in appclr clears down the same bit in appset*/

nit: a 0b1 ?

> +	config->ctiappset &= ~val;
> +
> +	/* write through if enabled */
> +	if (CTI_PWR_ENA(config))
> +		cti_write_single_reg(drvdata, CTIAPPCLEAR, val);
> +	spin_unlock(&drvdata->spinlock);
> +	return size;
> +}
> +static DEVICE_ATTR_WO(appclear);
> +

Otherwise looks good to me.

Suzuki
Suzuki K Poulose Nov. 28, 2019, 10:54 a.m. UTC | #2 
On 19/11/2019 23:19, Mike Leach wrote:
> Adds in sysfs programming support for the CTI function register sets.
> Allows direct manipulation of channel / trigger association registers.
> 
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Mike Leach <mike.leach@linaro.org>


> +/*
> + * #define CTI_DEBUG_INTEGRATION_CTRL to enable the access to the integration
> + * control registers. Normally only used to investigate connection data.
> + */

On a second thought, I have some comments on this symbol.

Given that the integration control registers may be useful for people to
find the device connections, I strongly feel that this is provided
via a CONFIG symbol rather than a  debug symbol within the code.

i.e, CONFIG_CTI_DEBUG_INTEGRATION_CTRL, to help the people better.
Codewise this doesn't make much difference, but it certainly makes
it more easier for people to use it.

We have used debug symbols elsewhere in the drivers for pure functional
debugging purposes. However I feel this is case is superior.


Cheers
Suzuki
Mathieu Poirier Nov. 28, 2019, 5:20 p.m. UTC | #3 
On Thu, 28 Nov 2019 at 03:54, Suzuki Kuruppassery Poulose
<suzuki.poulose@arm.com> wrote:
>
> On 19/11/2019 23:19, Mike Leach wrote:
> > Adds in sysfs programming support for the CTI function register sets.
> > Allows direct manipulation of channel / trigger association registers.
> >
> > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > Signed-off-by: Mike Leach <mike.leach@linaro.org>
>
>
> > +/*
> > + * #define CTI_DEBUG_INTEGRATION_CTRL to enable the access to the integration
> > + * control registers. Normally only used to investigate connection data.
> > + */
>
> On a second thought, I have some comments on this symbol.
>
> Given that the integration control registers may be useful for people to
> find the device connections, I strongly feel that this is provided
> via a CONFIG symbol rather than a  debug symbol within the code.

Device connections can be discovered with the dynamic sysfs connection
entries added as part of patch 09.  In cases where that is not
sufficient and people really need to use the integration control
registers they are probably instrumenting the code anyway.

>
> i.e, CONFIG_CTI_DEBUG_INTEGRATION_CTRL, to help the people better.
> Codewise this doesn't make much difference, but it certainly makes
> it more easier for people to use it.

I agree that code-wise it doesn't make much difference but I'm really
not convinced it makes the driver easier to use, and one needs to
recompile their kernel for production systems anyway.

Thanks,
Mathieu

>
> We have used debug symbols elsewhere in the drivers for pure functional
> debugging purposes. However I feel this is case is superior.
>
>
> Cheers
> Suzuki
Suzuki K Poulose Nov. 28, 2019, 6 p.m. UTC | #4 
Hi Mathieu,

On 28/11/2019 17:20, Mathieu Poirier wrote:
> On Thu, 28 Nov 2019 at 03:54, Suzuki Kuruppassery Poulose
> <suzuki.poulose@arm.com> wrote:
>>
>> On 19/11/2019 23:19, Mike Leach wrote:
>>> Adds in sysfs programming support for the CTI function register sets.
>>> Allows direct manipulation of channel / trigger association registers.
>>>
>>> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> Signed-off-by: Mike Leach <mike.leach@linaro.org>
>>
>>
>>> +/*
>>> + * #define CTI_DEBUG_INTEGRATION_CTRL to enable the access to the integration
>>> + * control registers. Normally only used to investigate connection data.
>>> + */
>>
>> On a second thought, I have some comments on this symbol.
>>
>> Given that the integration control registers may be useful for people to
>> find the device connections, I strongly feel that this is provided
>> via a CONFIG symbol rather than a  debug symbol within the code.
> 
> Device connections can be discovered with the dynamic sysfs connection
> entries added as part of patch 09.  In cases where that is not

Yes, that correct. That happens only if the DT/ACPI describes the
connections.

> sufficient and people really need to use the integration control
> registers they are probably instrumenting the code anyway.

In this case given the CTI number of triggers and connections, this
step is to identify the connections in the first place, so that they
can be described in the DT/ACPI. Of course this is not a common
activity, but more of a board bring up activity. Thus, we can't expect
the board bringup engineer to necessarily know how to modify
the driver to get this exposed. Having a Kconfig entry, with
a help text makes this easier for them to avoid fiddling with
the code. Hope this is clearer now.

Cheers
Suzuki


> 
>>
>> i.e, CONFIG_CTI_DEBUG_INTEGRATION_CTRL, to help the people better.
>> Codewise this doesn't make much difference, but it certainly makes
>> it more easier for people to use it.
> 
> I agree that code-wise it doesn't make much difference but I'm really
> not convinced it makes the driver easier to use, and one needs to
> recompile their kernel for production systems anyway.
> 
> Thanks,
> Mathieu
> 
>>
>> We have used debug symbols elsewhere in the drivers for pure functional
>> debugging purposes. However I feel this is case is superior.
>>
>>
>> Cheers
>> Suzuki
Mike Leach Nov. 29, 2019, 12:47 p.m. UTC | #5 
Hi Suzuki,


On Wed, 27 Nov 2019 at 18:26, Suzuki Kuruppassery Poulose
<suzuki.poulose@arm.com> wrote:
>
> On 19/11/2019 23:19, Mike Leach wrote:
> > Adds in sysfs programming support for the CTI function register sets.
> > Allows direct manipulation of channel / trigger association registers.
> >
> > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > Signed-off-by: Mike Leach <mike.leach@linaro.org>
> > ---
> >   .../hwtracing/coresight/coresight-cti-sysfs.c | 362 ++++++++++++++++++
> >   drivers/hwtracing/coresight/coresight-cti.c   |  19 +
> >   drivers/hwtracing/coresight/coresight-cti.h   |   5 +
> >   3 files changed, 386 insertions(+)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> > index 507f8eb487fe..02d3ee0c1278 100644
> > --- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> > +++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
> > @@ -109,6 +109,362 @@ static struct attribute *coresight_cti_mgmt_attrs[] = {
> >       NULL,
> >   };
> >
> > +/* CTI low level programming registers */
> > +
> > +/*
> > + * Show a simple 32 bit value if enabled and powered.
> > + * If inaccessible & pcached_val not NULL then show cached value.
> > + */
>
> Also I am not sure if it makes sense to mention that the value is cached.
>
> > +static ssize_t cti_reg32_show(struct device *dev, char *buf,
> > +                           u32 *pcached_val, int reg_offset)
> > +{
> > +     u32 val = 0;
>    +    char *state = "";
>
> > +     struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
> > +     struct cti_config *config = &drvdata->config;
> > +
> > +     spin_lock(&drvdata->spinlock);
> > +     if ((reg_offset >= 0) && CTI_PWR_ENA(config)) {
>
> minor nit: Personally I don't like the naming here. This could simply
> be: cti_accessible(config) , may be defined as a static inline function
> instead of a macro:
>
> static inline bool cti_accessible(struct cti_drvdata *drvdata)
> {
>         struct cti_config *cfg = &drvdata->config;
>
>         return cfg->hw_powered && cfg->hw_enabled;
> }
>
>

Since this is a generic access function used throughout the file - the
cached pointer is an indicator used by the callee that there is a
value available if the CTI is unpowered  / disabled - so the function
can show an appropriate value which will be taken from the config
structure.

So I don't think it is relevant to show that a "cached" value is being
used to show the user. If you look at similar functions in the ETM
drivers for example, quite often a show function simple shows that
stored value from a config structure without ever looking at the
register in the device.

As to naming - the name is chosen to represent a specific state - both
powered and enabled. The sysfs interface is accessible in any state  -
powered / unpowered  , enabled /disabled - so I am being specific.
Unlike the ETM, this hardware can have registers programmed while
enabled - and for some such as apppulse this is the only time it makes
sense to use them.

I don't mind either way between macro / inline function - though it
still has to be declared in the header as it is used in multiple .c
files.
I'd be inclined to call it cti_active() if preferred to cti_pwr_ena -
active implies that the CTI is in operation.

Thanks

Mike


> > +             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;
>
>    +            state = " (cached)";
> > +     }
> > +     spin_unlock(&drvdata->spinlock);
> > +     return scnprintf(buf, PAGE_SIZE, "%#x\n", val);
>    +    return scnprintf(buf, PAGE_SIZE, "%#x%s\n", val, state);
>
> > +}
> > +
> > +/*
> > + * Store a simple 32 bit value.
> > + * If pcached_val not NULL, then copy to here too,
> > + * 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)
>
>
> > +static ssize_t appclear_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);
> > +     struct cti_config *config = &drvdata->config;
> > +
> > +     if (kstrtoul(buf, 0, &val))
> > +             return -EINVAL;
> > +
> > +     spin_lock(&drvdata->spinlock);
> > +
> > +     /* a 1'b1 in appclr clears down the same bit in appset*/
>
> nit: a 0b1 ?
>
Syntax is <bitwidth>'<radix><value> - a habit picked up from verilog.

> > +     config->ctiappset &= ~val;
> > +
> > +     /* write through if enabled */
> > +     if (CTI_PWR_ENA(config))
> > +             cti_write_single_reg(drvdata, CTIAPPCLEAR, val);
> > +     spin_unlock(&drvdata->spinlock);
> > +     return size;
> > +}
> > +static DEVICE_ATTR_WO(appclear);
> > +
>
> Otherwise looks good to me.
>
> Suzuki
Mike Leach Nov. 29, 2019, 12:50 p.m. UTC | #6 
Hi Suzuki, Mathieu,

On Thu, 28 Nov 2019 at 10:54, Suzuki Kuruppassery Poulose
<suzuki.poulose@arm.com> wrote:
>
> On 19/11/2019 23:19, Mike Leach wrote:
> > Adds in sysfs programming support for the CTI function register sets.
> > Allows direct manipulation of channel / trigger association registers.
> >
> > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > Signed-off-by: Mike Leach <mike.leach@linaro.org>
>
>
> > +/*
> > + * #define CTI_DEBUG_INTEGRATION_CTRL to enable the access to the integration
> > + * control registers. Normally only used to investigate connection data.
> > + */
>
> On a second thought, I have some comments on this symbol.
>
> Given that the integration control registers may be useful for people to
> find the device connections, I strongly feel that this is provided
> via a CONFIG symbol rather than a  debug symbol within the code.
>
> i.e, CONFIG_CTI_DEBUG_INTEGRATION_CTRL, to help the people better.
> Codewise this doesn't make much difference, but it certainly makes
> it more easier for people to use it.
>
> We have used debug symbols elsewhere in the drivers for pure functional
> debugging purposes. However I feel this is case is superior.
>
>
> Cheers
> Suzuki

Per the comment above, and the discussions following, I would agree
that using a config symbol makes it easier for users to select the
feature and gives us an opportunity to put in some explanation as to
what it does.

Thanks

Mike
diff mbox series

Patch

diff --git a/drivers/hwtracing/coresight/coresight-cti-sysfs.c b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
index 507f8eb487fe..02d3ee0c1278 100644
--- a/drivers/hwtracing/coresight/coresight-cti-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-cti-sysfs.c
@@ -109,6 +109,362 @@  static struct attribute *coresight_cti_mgmt_attrs[] = {
 	NULL,
 };
 
+/* CTI low level programming registers */
+
+/*
+ * Show a simple 32 bit value if enabled and powered.
+ * 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 val = 0;
+	struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	struct cti_config *config = &drvdata->config;
+
+	spin_lock(&drvdata->spinlock);
+	if ((reg_offset >= 0) && CTI_PWR_ENA(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;
+	}
+	spin_unlock(&drvdata->spinlock);
+	return scnprintf(buf, PAGE_SIZE, "%#x\n", val);
+}
+
+/*
+ * Store a simple 32 bit value.
+ * If pcached_val not NULL, then copy to here too,
+ * 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)
+{
+	unsigned long val;
+	struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	struct cti_config *config = &drvdata->config;
+
+	if (kstrtoul(buf, 0, &val))
+		return -EINVAL;
+
+	spin_lock(&drvdata->spinlock);
+	/* local store */
+	if (pcached_val)
+		*pcached_val = (u32)val;
+
+	/* write through if offset and enabled */
+	if ((reg_offset >= 0) && CTI_PWR_ENA(config))
+		cti_write_single_reg(drvdata, reg_offset, val);
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+
+/* Standard macro for simple rw cti config registers */
+#define cti_config_reg32_rw(name, cfgname, offset)			\
+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);	\
+}									\
+									\
+static ssize_t name##_store(struct device *dev,				\
+			    struct device_attribute *attr,		\
+			    const char *buf, size_t size)		\
+{									\
+	struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);	\
+	return cti_reg32_store(dev, buf, size,				\
+			       &drvdata->config.cfgname, offset);	\
+}									\
+static DEVICE_ATTR_RW(name)
+
+static ssize_t inout_sel_show(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	u32 val;
+	struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	val = (u32)drvdata->config.ctiinout_sel;
+	return scnprintf(buf, PAGE_SIZE, "%d\n", val);
+}
+
+static ssize_t inout_sel_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 > (CTIINOUTEN_MAX - 1))
+		return -EINVAL;
+
+	spin_lock(&drvdata->spinlock);
+	drvdata->config.ctiinout_sel = val;
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_RW(inout_sel);
+
+static ssize_t inen_show(struct device *dev,
+			 struct device_attribute *attr,
+			 char *buf)
+{
+	unsigned long val;
+	int index;
+	struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	spin_lock(&drvdata->spinlock);
+	index = drvdata->config.ctiinout_sel;
+	val = drvdata->config.ctiinen[index];
+	spin_unlock(&drvdata->spinlock);
+	return scnprintf(buf, PAGE_SIZE, "INEN%d %#lx\n", index, val);
+}
+
+static ssize_t inen_store(struct device *dev,
+			  struct device_attribute *attr,
+			  const char *buf, size_t size)
+{
+	unsigned long val;
+	int index;
+	struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	struct cti_config *config = &drvdata->config;
+
+	if (kstrtoul(buf, 0, &val))
+		return -EINVAL;
+
+	spin_lock(&drvdata->spinlock);
+	index = config->ctiinout_sel;
+	config->ctiinen[index] = val;
+
+	/* write through if enabled */
+	if (CTI_PWR_ENA(config))
+		cti_write_single_reg(drvdata, CTIINEN(index), val);
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_RW(inen);
+
+static ssize_t outen_show(struct device *dev,
+			  struct device_attribute *attr,
+			  char *buf)
+{
+	unsigned long val;
+	int index;
+	struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	spin_lock(&drvdata->spinlock);
+	index = drvdata->config.ctiinout_sel;
+	val = drvdata->config.ctiouten[index];
+	spin_unlock(&drvdata->spinlock);
+	return scnprintf(buf, PAGE_SIZE, "OUTEN%d %#lx\n", index, val);
+}
+
+static ssize_t outen_store(struct device *dev,
+			   struct device_attribute *attr,
+			   const char *buf, size_t size)
+{
+	unsigned long val;
+	int index;
+	struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	struct cti_config *config = &drvdata->config;
+
+	if (kstrtoul(buf, 0, &val))
+		return -EINVAL;
+
+	spin_lock(&drvdata->spinlock);
+	index = config->ctiinout_sel;
+	config->ctiouten[index] = val;
+
+	/* write through if enabled */
+	if (CTI_PWR_ENA(config))
+		cti_write_single_reg(drvdata, CTIOUTEN(index), val);
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_RW(outen);
+
+static ssize_t intack_store(struct device *dev,
+			    struct device_attribute *attr,
+			    const char *buf, size_t size)
+{
+	unsigned long val;
+
+	if (kstrtoul(buf, 0, &val))
+		return -EINVAL;
+
+	cti_write_intack(dev, val);
+	return size;
+}
+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);
+
+static ssize_t appclear_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);
+	struct cti_config *config = &drvdata->config;
+
+	if (kstrtoul(buf, 0, &val))
+		return -EINVAL;
+
+	spin_lock(&drvdata->spinlock);
+
+	/* a 1'b1 in appclr clears down the same bit in appset*/
+	config->ctiappset &= ~val;
+
+	/* write through if enabled */
+	if (CTI_PWR_ENA(config))
+		cti_write_single_reg(drvdata, CTIAPPCLEAR, val);
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_WO(appclear);
+
+static ssize_t apppulse_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);
+	struct cti_config *config = &drvdata->config;
+
+	if (kstrtoul(buf, 0, &val))
+		return -EINVAL;
+
+	spin_lock(&drvdata->spinlock);
+
+	/* write through if enabled */
+	if (CTI_PWR_ENA(config))
+		cti_write_single_reg(drvdata, CTIAPPPULSE, val);
+	spin_unlock(&drvdata->spinlock);
+	return size;
+}
+static DEVICE_ATTR_WO(apppulse);
+
+coresight_cti_reg(triginstatus, CTITRIGINSTATUS);
+coresight_cti_reg(trigoutstatus, CTITRIGOUTSTATUS);
+coresight_cti_reg(chinstatus, CTICHINSTATUS);
+coresight_cti_reg(choutstatus, CTICHOUTSTATUS);
+
+/*
+ * #define CTI_DEBUG_INTEGRATION_CTRL to enable the access to the integration
+ * control registers. Normally only used to investigate connection data.
+ */
+/* #define CTI_DEBUG_INTEGRATION_CTRL */
+
+#ifdef CTI_DEBUG_INTEGRATION_CTRL
+
+/* macro to access RW registers with power check only (no enable check). */
+#define coresight_cti_reg_rw(name, offset)				\
+static ssize_t name##_show(struct device *dev,				\
+			   struct device_attribute *attr, char *buf)	\
+{									\
+	struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);	\
+	u32 val = 0;							\
+	pm_runtime_get_sync(dev->parent);				\
+	spin_lock(&drvdata->spinlock);					\
+	if (drvdata->config.hw_powered)					\
+		val = readl_relaxed(drvdata->base + offset);		\
+	spin_unlock(&drvdata->spinlock);				\
+	pm_runtime_put_sync(dev->parent);				\
+	return scnprintf(buf, PAGE_SIZE, "0x%x\n", val);		\
+}									\
+									\
+static ssize_t name##_store(struct device *dev,				\
+			    struct device_attribute *attr,		\
+			    const char *buf, size_t size)		\
+{									\
+	struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);	\
+	unsigned long val = 0;						\
+	if (kstrtoul(buf, 0, &val))					\
+		return -EINVAL;						\
+									\
+	pm_runtime_get_sync(dev->parent);				\
+	spin_lock(&drvdata->spinlock);					\
+	if (drvdata->config.hw_powered)					\
+		cti_write_single_reg(drvdata, reg_offset, val);		\
+	spin_unlock(&drvdata->spinlock);				\
+	pm_runtime_put_sync(dev->parent);				\
+	return size;							\
+}									\
+static DEVICE_ATTR_RW(name)
+
+/* macro to access WO registers with power check only (no enable check). */
+#define coresight_cti_reg_wo(name, offset)				\
+static ssize_t name##_store(struct device *dev,				\
+			    struct device_attribute *attr,		\
+			    const char *buf, size_t size)		\
+{									\
+	struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);	\
+	unsigned long val = 0;						\
+	if (kstrtoul(buf, 0, &val))					\
+		return -EINVAL;						\
+									\
+	pm_runtime_get_sync(dev->parent);				\
+	spin_lock(&drvdata->spinlock);					\
+	if (drvdata->config.hw_powered)					\
+		cti_write_single_reg(drvdata, reg_offset, val);		\
+	spin_unlock(&drvdata->spinlock);				\
+	pm_runtime_put_sync(dev->parent);				\
+	return size;							\
+}									\
+static DEVICE_ATTR_WO(name)
+
+coresight_cti_reg_rw(itchout, ITCHOUT);
+coresight_cti_reg_rw(ittrigout, ITTRIGOUT);
+coresight_cti_reg_rw(itctrl, CORESIGHT_ITCTRL);
+coresight_cti_reg_wo(itchinack, ITCHINACK);
+coresight_cti_reg_wo(ittriginack, ITTRIGINACK);
+coresight_cti_reg(ittrigin, ITTRIGIN);
+coresight_cti_reg(itchin, ITCHIN);
+coresight_cti_reg(itchoutack, ITCHOUTACK);
+coresight_cti_reg(ittrigoutack, ITTRIGOUTACK);
+
+#endif /* CTI_DEBUG_INTEGRATION_CTRL */
+
+static struct attribute *coresight_cti_regs_attrs[] = {
+	&dev_attr_inout_sel.attr,
+	&dev_attr_inen.attr,
+	&dev_attr_outen.attr,
+	&dev_attr_gate.attr,
+	&dev_attr_asicctl.attr,
+	&dev_attr_intack.attr,
+	&dev_attr_appset.attr,
+	&dev_attr_appclear.attr,
+	&dev_attr_apppulse.attr,
+	&dev_attr_triginstatus.attr,
+	&dev_attr_trigoutstatus.attr,
+	&dev_attr_chinstatus.attr,
+	&dev_attr_choutstatus.attr,
+#ifdef CTI_DEBUG_INTEGRATION_CTRL
+	&dev_attr_itctrl.attr,
+	&dev_attr_ittrigin.attr,
+	&dev_attr_itchin.attr,
+	&dev_attr_ittrigout.attr,
+	&dev_attr_itchout.attr,
+	&dev_attr_itchoutack.attr,
+	&dev_attr_ittrigoutack.attr,
+	&dev_attr_ittriginack.attr,
+	&dev_attr_itchinack.attr,
+#endif
+	NULL,
+};
+
+/* sysfs groups */
 static const struct attribute_group coresight_cti_group = {
 	.attrs = coresight_cti_attrs,
 };
@@ -118,8 +474,14 @@  static const struct attribute_group coresight_cti_mgmt_group = {
 	.name = "mgmt",
 };
 
+static const struct attribute_group coresight_cti_regs_group = {
+	.attrs = coresight_cti_regs_attrs,
+	.name = "regs",
+};
+
 const struct attribute_group *coresight_cti_groups[] = {
 	&coresight_cti_group,
 	&coresight_cti_mgmt_group,
+	&coresight_cti_regs_group,
 	NULL,
 };
diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c
index 7ae48bf62d17..b016b1e67fb1 100644
--- a/drivers/hwtracing/coresight/coresight-cti.c
+++ b/drivers/hwtracing/coresight/coresight-cti.c
@@ -149,6 +149,25 @@  static int cti_disable_hw(struct cti_drvdata *drvdata)
 	return 0;
 }
 
+void cti_write_single_reg(struct cti_drvdata *drvdata, int offset, u32 value)
+{
+	CS_UNLOCK(drvdata->base);
+	writel_relaxed(value, drvdata->base + offset);
+	CS_LOCK(drvdata->base);
+}
+
+void cti_write_intack(struct device *dev, u32 ackval)
+{
+	struct cti_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	struct cti_config *config = &drvdata->config;
+
+	spin_lock(&drvdata->spinlock);
+	/* write if enabled */
+	if (CTI_PWR_ENA(config))
+		cti_write_single_reg(drvdata, CTIINTACK, ackval);
+	spin_unlock(&drvdata->spinlock);
+}
+
 /*
  * Look at the HW DEVID register for some of the HW settings.
  * DEVID[15:8] - max number of in / out triggers.
diff --git a/drivers/hwtracing/coresight/coresight-cti.h b/drivers/hwtracing/coresight/coresight-cti.h
index e0d476533a82..73869fa8b313 100644
--- a/drivers/hwtracing/coresight/coresight-cti.h
+++ b/drivers/hwtracing/coresight/coresight-cti.h
@@ -180,7 +180,12 @@  struct cti_trig_con *cti_allocate_trig_con(struct device *dev, int in_sigs,
 					   int out_sigs);
 int cti_enable(struct coresight_device *csdev);
 int cti_disable(struct coresight_device *csdev);
+void cti_write_intack(struct device *dev, u32 ackval);
+void cti_write_single_reg(struct cti_drvdata *drvdata, int offset, u32 value);
 struct coresight_platform_data *
 coresight_cti_get_platform_data(struct device *dev);
 
+/* cti powered and enabled */
+#define CTI_PWR_ENA(p_cfg) (p_cfg->hw_enabled && p_cfg->hw_powered)
+
 #endif  /* _CORESIGHT_CORESIGHT_CTI_H */