[PATCH V3 00/18] coresight: tmc: make driver usable by Perf

[PATCH V3 00/18] coresight: tmc: make driver usable by Perf

[Mathieu Poirier]

This patchset makes the TMC driver usable from Perf by way of the
recently added AUX area functionality.  Note that only the kernel
side of the operation is presented here.  The user space perf tool
enhancement are kept here[1].  Upstreaming of that portion will 
follow when the kernel side has been addressed.

The first 15 patches remodel the driver so that the functionality
is compartmented enough to be used as building blocks when interfacing
with the Perf subsystem.

Patch 16 and 17 implement the AUX area API, taking heavily on the existing
ETB10 implementation.

Finally patch 18 allows the TMC to be used as a link rather than a sink,
something that is required when dealing with an ETF component.

Best regards,
Mathieu

[1]. https://git.linaro.org/people/mathieu.poirier/coresight.git/shortlog/refs/heads/perf-opencsd-4.6-rc2

Changes for V3:
- Reworked drvdata::memwidth assignment.
- cleaned up coresight-tmc.h header file.
- Added WARN_ON[_ONCE]() on error paths.
- Added missing spinlock release.
- Using a type 'long' when dealing with local_[cmp]xchg()
- Added Suzuki K Poulose's Reviewed-by.

Changes for V2:
- Renamed functions tmc_read_prepare/unprepare_etf() to 
  tmc_read_prepare/unprepare_etb().
- Consolidated return points in functions tmc_enable_etf/etr_sink().
- Re-wrote allocation schemed in functions tmc_enable_etf/etr_sink().
- Fixed description typos in patch 7/15.
- Fixed problem with linksink devices when disabling a path in patch 15/15.

Mathieu Poirier (18):
  coresight: tmc: modifying naming convention
  coresight: tmc: waiting for TMCReady bit before programming
  coresight: tmc: re-implementing tmc_read_prepare/unprepare() functions
  coresight: tmc: clearly define number of transfers per burst
  coresight: tmc: introducing new header file
  coresight: tmc: cleaning up header file
  coresight: tmc: splitting driver in ETB/ETF and ETR components
  coresight: tmc: making prepare/unprepare functions generic
  coresight: tmc: allocating memory when needed
  coresight: tmc: getting the right read_count on tmc_open()
  coresight: tmc: adding mode of operation for link/sinks
  coresight: tmc: dump system memory content only when needed
  coresight: tmc: make sysFS and Perf mode mutually exclusive
  coresight: tmc: keep track of memory width
  coresight: moving struct cs_buffers to header file
  coresight: tmc: implementing TMC-ETF AUX space API
  coresight: tmc: implementing TMC-ETR AUX space API
  coresight: configuring ETF in FIFO mode when acting as link

 drivers/hwtracing/coresight/Makefile            |   4 +-
 drivers/hwtracing/coresight/coresight-etb10.c   |  20 -
 drivers/hwtracing/coresight/coresight-priv.h    |  20 +
 drivers/hwtracing/coresight/coresight-tmc-etf.c | 603 ++++++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tmc-etr.c | 469 ++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tmc.c     | 485 +++----------------
 drivers/hwtracing/coresight/coresight-tmc.h     | 145 ++++++
 drivers/hwtracing/coresight/coresight.c         |  32 +-
 8 files changed, 1339 insertions(+), 439 deletions(-)
 create mode 100644 drivers/hwtracing/coresight/coresight-tmc-etf.c
 create mode 100644 drivers/hwtracing/coresight/coresight-tmc-etr.c
 create mode 100644 drivers/hwtracing/coresight/coresight-tmc.h

-- 
2.5.0

[PATCH V3 01/18] coresight: tmc: modifying naming convention

[Mathieu Poirier]

According to the TMC architectural state machine, the 'stopped'
state is reached when bit 2 (TMCReady) of the TMC Status register
turns to '1'.  The code is correct but the naming convention isn't.

The 'Triggered' bit occupies position '1' of the TMC Status register
and has nothing to do with the indication of the TMC entering the
stopped state. As such renaming function "tmc_wait_for_triggered()"
and changing the #define to reflect what the code is really doing.

This patch has no effect other than clarifying the semantic.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-tmc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
index 2b42ecbd8831..3f646e29a99b 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.c
+++ b/drivers/hwtracing/coresight/coresight-tmc.c
@@ -77,7 +77,7 @@
 #define TMC_FFCR_TRIGON_TRIGIN	BIT(8)
 #define TMC_FFCR_STOP_ON_FLUSH	BIT(12)
 
-#define TMC_STS_TRIGGERED_BIT	2
+#define TMC_STS_TMCREADY_BIT	2
 #define TMC_FFCR_FLUSHMAN_BIT	6
 
 enum tmc_config_type {
@@ -132,11 +132,11 @@ struct tmc_drvdata {
 	u32			trigger_cntr;
 };
 
-static void tmc_wait_for_ready(struct tmc_drvdata *drvdata)
+static void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
 {
 	/* Ensure formatter, unformatter and hardware fifo are empty */
 	if (coresight_timeout(drvdata->base,
-			      TMC_STS, TMC_STS_TRIGGERED_BIT, 1)) {
+			      TMC_STS, TMC_STS_TMCREADY_BIT, 1)) {
 		dev_err(drvdata->dev,
 			"timeout observed when probing at offset %#x\n",
 			TMC_STS);
@@ -160,7 +160,7 @@ static void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
 			TMC_FFCR);
 	}
 
-	tmc_wait_for_ready(drvdata);
+	tmc_wait_for_tmcready(drvdata);
 }
 
 static void tmc_enable_hw(struct tmc_drvdata *drvdata)
-- 
2.5.0

[PATCH V3 02/18] coresight: tmc: waiting for TMCReady bit before programming

[Mathieu Poirier]

According to the TRM before programming the TMC in circular
buffer mode (and that for any configuration, ETB, ETR, ETF),
the TMCReady bit in the status register has to be set.

This patch adds a check to make sure the state machine is in
a state where it can be configured, and complains otherwise.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-tmc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
index 3f646e29a99b..66fa7736d12f 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.c
+++ b/drivers/hwtracing/coresight/coresight-tmc.c
@@ -180,6 +180,9 @@ static void tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
 
 	CS_UNLOCK(drvdata->base);
 
+	/* Wait for TMCSReady bit to be set */
+	tmc_wait_for_tmcready(drvdata);
+
 	writel_relaxed(TMC_MODE_CIRCULAR_BUFFER, drvdata->base + TMC_MODE);
 	writel_relaxed(TMC_FFCR_EN_FMT | TMC_FFCR_EN_TI |
 		       TMC_FFCR_FON_FLIN | TMC_FFCR_FON_TRIG_EVT |
@@ -201,6 +204,9 @@ static void tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
 
 	CS_UNLOCK(drvdata->base);
 
+	/* Wait for TMCSReady bit to be set */
+	tmc_wait_for_tmcready(drvdata);
+
 	writel_relaxed(drvdata->size / 4, drvdata->base + TMC_RSZ);
 	writel_relaxed(TMC_MODE_CIRCULAR_BUFFER, drvdata->base + TMC_MODE);
 
@@ -230,6 +236,9 @@ static void tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
 {
 	CS_UNLOCK(drvdata->base);
 
+	/* Wait for TMCSReady bit to be set */
+	tmc_wait_for_tmcready(drvdata);
+
 	writel_relaxed(TMC_MODE_HARDWARE_FIFO, drvdata->base + TMC_MODE);
 	writel_relaxed(TMC_FFCR_EN_FMT | TMC_FFCR_EN_TI,
 		       drvdata->base + TMC_FFCR);
-- 
2.5.0

[PATCH V3 03/18] coresight: tmc: re-implementing tmc_read_prepare/unprepare() functions

[Mathieu Poirier]

In their current implementation the tmc_read_prepare/unprepare()
are a lump of if/else that is difficult to read.  This patch is
alleviating that by using a switch statement.  The latter also
allows for a better control on the error path.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-tmc.c | 56 ++++++++++++++++++-----------
 1 file changed, 36 insertions(+), 20 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
index 66fa7736d12f..d211aeec49f8 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.c
+++ b/drivers/hwtracing/coresight/coresight-tmc.c
@@ -431,7 +431,7 @@ static const struct coresight_ops tmc_etf_cs_ops = {
 
 static int tmc_read_prepare(struct tmc_drvdata *drvdata)
 {
-	int ret;
+	int ret = 0;
 	unsigned long flags;
 	enum tmc_mode mode;
 
@@ -439,25 +439,31 @@ static int tmc_read_prepare(struct tmc_drvdata *drvdata)
 	if (!drvdata->enable)
 		goto out;
 
-	if (drvdata->config_type == TMC_CONFIG_TYPE_ETB) {
+	switch (drvdata->config_type) {
+	case TMC_CONFIG_TYPE_ETB:
 		tmc_etb_disable_hw(drvdata);
-	} else if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
-		tmc_etr_disable_hw(drvdata);
-	} else {
+		break;
+	case TMC_CONFIG_TYPE_ETF:
+		/* There is no point in reading a TMC in HW FIFO mode */
 		mode = readl_relaxed(drvdata->base + TMC_MODE);
-		if (mode == TMC_MODE_CIRCULAR_BUFFER) {
-			tmc_etb_disable_hw(drvdata);
-		} else {
-			ret = -ENODEV;
+		if (mode != TMC_MODE_CIRCULAR_BUFFER) {
+			ret = -EINVAL;
 			goto err;
 		}
+
+		tmc_etb_disable_hw(drvdata);
+		break;
+	case TMC_CONFIG_TYPE_ETR:
+		tmc_etr_disable_hw(drvdata);
+		break;
+	default:
+		ret = -EINVAL;
+		goto err;
 	}
+
 out:
 	drvdata->reading = true;
-	spin_unlock_irqrestore(&drvdata->spinlock, flags);
-
 	dev_info(drvdata->dev, "TMC read start\n");
-	return 0;
 err:
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 	return ret;
@@ -472,20 +478,30 @@ static void tmc_read_unprepare(struct tmc_drvdata *drvdata)
 	if (!drvdata->enable)
 		goto out;
 
-	if (drvdata->config_type == TMC_CONFIG_TYPE_ETB) {
+	switch (drvdata->config_type) {
+	case TMC_CONFIG_TYPE_ETB:
 		tmc_etb_enable_hw(drvdata);
-	} else if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
-		tmc_etr_enable_hw(drvdata);
-	} else {
+		break;
+	case TMC_CONFIG_TYPE_ETF:
+		/* Make sure we don't re-enable a TMC in HW FIFO mode */
 		mode = readl_relaxed(drvdata->base + TMC_MODE);
-		if (mode == TMC_MODE_CIRCULAR_BUFFER)
-			tmc_etb_enable_hw(drvdata);
+		if (mode != TMC_MODE_CIRCULAR_BUFFER)
+			goto err;
+
+		tmc_etb_enable_hw(drvdata);
+		break;
+	case TMC_CONFIG_TYPE_ETR:
+		tmc_etr_disable_hw(drvdata);
+		break;
+	default:
+		goto err;
 	}
+
 out:
 	drvdata->reading = false;
-	spin_unlock_irqrestore(&drvdata->spinlock, flags);
-
 	dev_info(drvdata->dev, "TMC read end\n");
+err:
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 }
 
 static int tmc_open(struct inode *inode, struct file *file)
-- 
2.5.0

[PATCH V3 04/18] coresight: tmc: clearly define number of transfers per burst

[Mathieu Poirier]

This patch makes the name of the define reflect the amount of
data tranfers per burst, in this case 16.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-tmc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
index d211aeec49f8..8751d53fa078 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.c
+++ b/drivers/hwtracing/coresight/coresight-tmc.c
@@ -67,7 +67,7 @@
 #define TMC_AXICTL_PROT_CTL_B0	BIT(0)
 #define TMC_AXICTL_PROT_CTL_B1	BIT(1)
 #define TMC_AXICTL_SCT_GAT_MODE	BIT(7)
-#define TMC_AXICTL_WR_BURST_LEN 0xF00
+#define TMC_AXICTL_WR_BURST_16	0xF00
 /* TMC_FFCR - 0x304 */
 #define TMC_FFCR_EN_FMT		BIT(0)
 #define TMC_FFCR_EN_TI		BIT(1)
@@ -211,7 +211,7 @@ static void tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
 	writel_relaxed(TMC_MODE_CIRCULAR_BUFFER, drvdata->base + TMC_MODE);
 
 	axictl = readl_relaxed(drvdata->base + TMC_AXICTL);
-	axictl |= TMC_AXICTL_WR_BURST_LEN;
+	axictl |= TMC_AXICTL_WR_BURST_16;
 	writel_relaxed(axictl, drvdata->base + TMC_AXICTL);
 	axictl &= ~TMC_AXICTL_SCT_GAT_MODE;
 	writel_relaxed(axictl, drvdata->base + TMC_AXICTL);
-- 
2.5.0

[PATCH V3 05/18] coresight: tmc: introducing new header file

[Mathieu Poirier]

The amount of #define, enumeration and structure definition
is big enough to justify moving them to a new header file.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-tmc.c | 102 +----------------------
 drivers/hwtracing/coresight/coresight-tmc.h | 122 ++++++++++++++++++++++++++++
 2 files changed, 123 insertions(+), 101 deletions(-)
 create mode 100644 drivers/hwtracing/coresight/coresight-tmc.h

diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
index 8751d53fa078..f9a6624ab531 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.c
+++ b/drivers/hwtracing/coresight/coresight-tmc.c
@@ -30,107 +30,7 @@
 #include <linux/amba/bus.h>
 
 #include "coresight-priv.h"
-
-#define TMC_RSZ			0x004
-#define TMC_STS			0x00c
-#define TMC_RRD			0x010
-#define TMC_RRP			0x014
-#define TMC_RWP			0x018
-#define TMC_TRG			0x01c
-#define TMC_CTL			0x020
-#define TMC_RWD			0x024
-#define TMC_MODE		0x028
-#define TMC_LBUFLEVEL		0x02c
-#define TMC_CBUFLEVEL		0x030
-#define TMC_BUFWM		0x034
-#define TMC_RRPHI		0x038
-#define TMC_RWPHI		0x03c
-#define TMC_AXICTL		0x110
-#define TMC_DBALO		0x118
-#define TMC_DBAHI		0x11c
-#define TMC_FFSR		0x300
-#define TMC_FFCR		0x304
-#define TMC_PSCR		0x308
-#define TMC_ITMISCOP0		0xee0
-#define TMC_ITTRFLIN		0xee8
-#define TMC_ITATBDATA0		0xeec
-#define TMC_ITATBCTR2		0xef0
-#define TMC_ITATBCTR1		0xef4
-#define TMC_ITATBCTR0		0xef8
-
-/* register description */
-/* TMC_CTL - 0x020 */
-#define TMC_CTL_CAPT_EN		BIT(0)
-/* TMC_STS - 0x00C */
-#define TMC_STS_TRIGGERED	BIT(1)
-/* TMC_AXICTL - 0x110 */
-#define TMC_AXICTL_PROT_CTL_B0	BIT(0)
-#define TMC_AXICTL_PROT_CTL_B1	BIT(1)
-#define TMC_AXICTL_SCT_GAT_MODE	BIT(7)
-#define TMC_AXICTL_WR_BURST_16	0xF00
-/* TMC_FFCR - 0x304 */
-#define TMC_FFCR_EN_FMT		BIT(0)
-#define TMC_FFCR_EN_TI		BIT(1)
-#define TMC_FFCR_FON_FLIN	BIT(4)
-#define TMC_FFCR_FON_TRIG_EVT	BIT(5)
-#define TMC_FFCR_FLUSHMAN	BIT(6)
-#define TMC_FFCR_TRIGON_TRIGIN	BIT(8)
-#define TMC_FFCR_STOP_ON_FLUSH	BIT(12)
-
-#define TMC_STS_TMCREADY_BIT	2
-#define TMC_FFCR_FLUSHMAN_BIT	6
-
-enum tmc_config_type {
-	TMC_CONFIG_TYPE_ETB,
-	TMC_CONFIG_TYPE_ETR,
-	TMC_CONFIG_TYPE_ETF,
-};
-
-enum tmc_mode {
-	TMC_MODE_CIRCULAR_BUFFER,
-	TMC_MODE_SOFTWARE_FIFO,
-	TMC_MODE_HARDWARE_FIFO,
-};
-
-enum tmc_mem_intf_width {
-	TMC_MEM_INTF_WIDTH_32BITS	= 0x2,
-	TMC_MEM_INTF_WIDTH_64BITS	= 0x3,
-	TMC_MEM_INTF_WIDTH_128BITS	= 0x4,
-	TMC_MEM_INTF_WIDTH_256BITS	= 0x5,
-};
-
-/**
- * struct tmc_drvdata - specifics associated to an TMC component
- * @base:	memory mapped base address for this component.
- * @dev:	the device entity associated to this component.
- * @csdev:	component vitals needed by the framework.
- * @miscdev:	specifics to handle "/dev/xyz.tmc" entry.
- * @spinlock:	only one at a time pls.
- * @read_count:	manages preparation of buffer for reading.
- * @buf:	area of memory where trace data get sent.
- * @paddr:	DMA start location in RAM.
- * @vaddr:	virtual representation of @paddr.
- * @size:	@buf size.
- * @enable:	this TMC is being used.
- * @config_type: TMC variant, must be of type @tmc_config_type.
- * @trigger_cntr: amount of words to store after a trigger.
- */
-struct tmc_drvdata {
-	void __iomem		*base;
-	struct device		*dev;
-	struct coresight_device	*csdev;
-	struct miscdevice	miscdev;
-	spinlock_t		spinlock;
-	int			read_count;
-	bool			reading;
-	char			*buf;
-	dma_addr_t		paddr;
-	void			*vaddr;
-	u32			size;
-	bool			enable;
-	enum tmc_config_type	config_type;
-	u32			trigger_cntr;
-};
+#include "coresight-tmc.h"
 
 static void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
 {
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
new file mode 100644
index 000000000000..49718b4a9788
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -0,0 +1,122 @@
+/*
+ * Copyright(C) 2015 Linaro Limited. All rights reserved.
+ * Author: Mathieu Poirier <mathieu.poirier@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef _CORESIGHT_TMC_H
+#define _CORESIGHT_TMC_H
+
+#define TMC_RSZ			0x004
+#define TMC_STS			0x00c
+#define TMC_RRD			0x010
+#define TMC_RRP			0x014
+#define TMC_RWP			0x018
+#define TMC_TRG			0x01c
+#define TMC_CTL			0x020
+#define TMC_RWD			0x024
+#define TMC_MODE		0x028
+#define TMC_LBUFLEVEL		0x02c
+#define TMC_CBUFLEVEL		0x030
+#define TMC_BUFWM		0x034
+#define TMC_RRPHI		0x038
+#define TMC_RWPHI		0x03c
+#define TMC_AXICTL		0x110
+#define TMC_DBALO		0x118
+#define TMC_DBAHI		0x11c
+#define TMC_FFSR		0x300
+#define TMC_FFCR		0x304
+#define TMC_PSCR		0x308
+#define TMC_ITMISCOP0		0xee0
+#define TMC_ITTRFLIN		0xee8
+#define TMC_ITATBDATA0		0xeec
+#define TMC_ITATBCTR2		0xef0
+#define TMC_ITATBCTR1		0xef4
+#define TMC_ITATBCTR0		0xef8
+
+/* register description */
+/* TMC_CTL - 0x020 */
+#define TMC_CTL_CAPT_EN		BIT(0)
+/* TMC_STS - 0x00C */
+#define TMC_STS_TRIGGERED	BIT(1)
+/* TMC_AXICTL - 0x110 */
+#define TMC_AXICTL_PROT_CTL_B0	BIT(0)
+#define TMC_AXICTL_PROT_CTL_B1	BIT(1)
+#define TMC_AXICTL_SCT_GAT_MODE	BIT(7)
+#define TMC_AXICTL_WR_BURST_16	0xF00
+/* TMC_FFCR - 0x304 */
+#define TMC_FFCR_EN_FMT		BIT(0)
+#define TMC_FFCR_EN_TI		BIT(1)
+#define TMC_FFCR_FON_FLIN	BIT(4)
+#define TMC_FFCR_FON_TRIG_EVT	BIT(5)
+#define TMC_FFCR_FLUSHMAN	BIT(6)
+#define TMC_FFCR_TRIGON_TRIGIN	BIT(8)
+#define TMC_FFCR_STOP_ON_FLUSH	BIT(12)
+
+#define TMC_STS_TMCREADY_BIT	2
+#define TMC_FFCR_FLUSHMAN_BIT	6
+
+enum tmc_config_type {
+	TMC_CONFIG_TYPE_ETB,
+	TMC_CONFIG_TYPE_ETR,
+	TMC_CONFIG_TYPE_ETF,
+};
+
+enum tmc_mode {
+	TMC_MODE_CIRCULAR_BUFFER,
+	TMC_MODE_SOFTWARE_FIFO,
+	TMC_MODE_HARDWARE_FIFO,
+};
+
+enum tmc_mem_intf_width {
+	TMC_MEM_INTF_WIDTH_32BITS	= 0x2,
+	TMC_MEM_INTF_WIDTH_64BITS	= 0x3,
+	TMC_MEM_INTF_WIDTH_128BITS	= 0x4,
+	TMC_MEM_INTF_WIDTH_256BITS	= 0x5,
+};
+
+/**
+ * struct tmc_drvdata - specifics associated to an TMC component
+ * @base:	memory mapped base address for this component.
+ * @dev:	the device entity associated to this component.
+ * @csdev:	component vitals needed by the framework.
+ * @miscdev:	specifics to handle "/dev/xyz.tmc" entry.
+ * @spinlock:	only one at a time pls.
+ * @read_count:	manages preparation of buffer for reading.
+ * @buf:	area of memory where trace data get sent.
+ * @paddr:	DMA start location in RAM.
+ * @vaddr:	virtual representation of @paddr.
+ * @size:	@buf size.
+ * @enable:	this TMC is being used.
+ * @config_type: TMC variant, must be of type @tmc_config_type.
+ * @trigger_cntr: amount of words to store after a trigger.
+ */
+struct tmc_drvdata {
+	void __iomem		*base;
+	struct device		*dev;
+	struct coresight_device	*csdev;
+	struct miscdevice	miscdev;
+	spinlock_t		spinlock;
+	int			read_count;
+	bool			reading;
+	char			*buf;
+	dma_addr_t		paddr;
+	void __iomem		*vaddr;
+	u32			size;
+	bool			enable;
+	enum tmc_config_type	config_type;
+	u32			trigger_cntr;
+};
+
+#endif
-- 
2.5.0

[PATCH V3 06/18] coresight: tmc: cleaning up header file

[Mathieu Poirier]

This patch first move the TMC_STS_TMCREADY_BIT and
TMC_FFCR_FLUSHMAN_BIT defines to their respective section.
It also removes TMC_FFCR_FLUSHMAN, since the same result
can easily be obtained using the BIT() macro.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-tmc.c | 2 +-
 drivers/hwtracing/coresight/coresight-tmc.h | 5 ++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
index f9a6624ab531..07e2809d832b 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.c
+++ b/drivers/hwtracing/coresight/coresight-tmc.c
@@ -50,7 +50,7 @@ static void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
 	ffcr = readl_relaxed(drvdata->base + TMC_FFCR);
 	ffcr |= TMC_FFCR_STOP_ON_FLUSH;
 	writel_relaxed(ffcr, drvdata->base + TMC_FFCR);
-	ffcr |= TMC_FFCR_FLUSHMAN;
+	ffcr |= BIT(TMC_FFCR_FLUSHMAN_BIT);
 	writel_relaxed(ffcr, drvdata->base + TMC_FFCR);
 	/* Ensure flush completes */
 	if (coresight_timeout(drvdata->base,
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index 49718b4a9788..5a60830c8db5 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -49,6 +49,7 @@
 /* TMC_CTL - 0x020 */
 #define TMC_CTL_CAPT_EN		BIT(0)
 /* TMC_STS - 0x00C */
+#define TMC_STS_TMCREADY_BIT	2
 #define TMC_STS_TRIGGERED	BIT(1)
 /* TMC_AXICTL - 0x110 */
 #define TMC_AXICTL_PROT_CTL_B0	BIT(0)
@@ -56,16 +57,14 @@
 #define TMC_AXICTL_SCT_GAT_MODE	BIT(7)
 #define TMC_AXICTL_WR_BURST_16	0xF00
 /* TMC_FFCR - 0x304 */
+#define TMC_FFCR_FLUSHMAN_BIT	6
 #define TMC_FFCR_EN_FMT		BIT(0)
 #define TMC_FFCR_EN_TI		BIT(1)
 #define TMC_FFCR_FON_FLIN	BIT(4)
 #define TMC_FFCR_FON_TRIG_EVT	BIT(5)
-#define TMC_FFCR_FLUSHMAN	BIT(6)
 #define TMC_FFCR_TRIGON_TRIGIN	BIT(8)
 #define TMC_FFCR_STOP_ON_FLUSH	BIT(12)
 
-#define TMC_STS_TMCREADY_BIT	2
-#define TMC_FFCR_FLUSHMAN_BIT	6
 
 enum tmc_config_type {
 	TMC_CONFIG_TYPE_ETB,
-- 
2.5.0

[PATCH V3 07/18] coresight: tmc: splitting driver in ETB/ETF and ETR components

[Mathieu Poirier]

The TMC block can operate in 3 modes (ETB, ETF and ETR) and accessed
via two interfaces (sysFS and Perf).  That makes 6 mode to cover, which
is way too much coupling for a single file.

This patch splits the original TMC driver in 2 halves, one for ETB/ETF
and another one for ETR mode.  A common core is kept for functionality
common to all 3 modes.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/Makefile            |   4 +-
 drivers/hwtracing/coresight/coresight-tmc-etf.c | 204 ++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tmc-etr.c | 128 ++++++++++++
 drivers/hwtracing/coresight/coresight-tmc.c     | 264 +-----------------------
 drivers/hwtracing/coresight/coresight-tmc.h     |  18 ++
 5 files changed, 357 insertions(+), 261 deletions(-)
 create mode 100644 drivers/hwtracing/coresight/coresight-tmc-etf.c
 create mode 100644 drivers/hwtracing/coresight/coresight-tmc-etr.c

diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index 1d0e32c7dbe4..0cf842ea3fe2 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -3,7 +3,9 @@
 #
 obj-$(CONFIG_CORESIGHT) += coresight.o coresight-etm-perf.o
 obj-$(CONFIG_OF) += of_coresight.o
-obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o
+obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o \
+					     coresight-tmc-etf.o \
+					     coresight-tmc-etr.o
 obj-$(CONFIG_CORESIGHT_SINK_TPIU) += coresight-tpiu.o
 obj-$(CONFIG_CORESIGHT_SINK_ETBV10) += coresight-etb10.o
 obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o \
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
new file mode 100644
index 000000000000..467d19221f7b
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -0,0 +1,204 @@
+/*
+ * Copyright(C) 2016 Linaro Limited. All rights reserved.
+ * Author: Mathieu Poirier <mathieu.poirier@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/coresight.h>
+#include "coresight-priv.h"
+#include "coresight-tmc.h"
+
+void tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
+{
+	/* Zero out the memory to help with debug */
+	memset(drvdata->buf, 0, drvdata->size);
+
+	CS_UNLOCK(drvdata->base);
+
+	/* Wait for TMCSReady bit to be set */
+	tmc_wait_for_tmcready(drvdata);
+
+	writel_relaxed(TMC_MODE_CIRCULAR_BUFFER, drvdata->base + TMC_MODE);
+	writel_relaxed(TMC_FFCR_EN_FMT | TMC_FFCR_EN_TI |
+		       TMC_FFCR_FON_FLIN | TMC_FFCR_FON_TRIG_EVT |
+		       TMC_FFCR_TRIGON_TRIGIN,
+		       drvdata->base + TMC_FFCR);
+
+	writel_relaxed(drvdata->trigger_cntr, drvdata->base + TMC_TRG);
+	tmc_enable_hw(drvdata);
+
+	CS_LOCK(drvdata->base);
+}
+
+static void tmc_etb_dump_hw(struct tmc_drvdata *drvdata)
+{
+	enum tmc_mem_intf_width memwidth;
+	u8 memwords;
+	char *bufp;
+	u32 read_data;
+	int i;
+
+	memwidth = BMVAL(readl_relaxed(drvdata->base + CORESIGHT_DEVID), 8, 10);
+	if (memwidth == TMC_MEM_INTF_WIDTH_32BITS)
+		memwords = 1;
+	else if (memwidth == TMC_MEM_INTF_WIDTH_64BITS)
+		memwords = 2;
+	else if (memwidth == TMC_MEM_INTF_WIDTH_128BITS)
+		memwords = 4;
+	else
+		memwords = 8;
+
+	bufp = drvdata->buf;
+	while (1) {
+		for (i = 0; i < memwords; i++) {
+			read_data = readl_relaxed(drvdata->base + TMC_RRD);
+			if (read_data == 0xFFFFFFFF)
+				return;
+			memcpy(bufp, &read_data, 4);
+			bufp += 4;
+		}
+	}
+}
+
+void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
+{
+	CS_UNLOCK(drvdata->base);
+
+	tmc_flush_and_stop(drvdata);
+	tmc_etb_dump_hw(drvdata);
+	tmc_disable_hw(drvdata);
+
+	CS_LOCK(drvdata->base);
+}
+
+static void tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
+{
+	CS_UNLOCK(drvdata->base);
+
+	/* Wait for TMCSReady bit to be set */
+	tmc_wait_for_tmcready(drvdata);
+
+	writel_relaxed(TMC_MODE_HARDWARE_FIFO, drvdata->base + TMC_MODE);
+	writel_relaxed(TMC_FFCR_EN_FMT | TMC_FFCR_EN_TI,
+		       drvdata->base + TMC_FFCR);
+	writel_relaxed(0x0, drvdata->base + TMC_BUFWM);
+	tmc_enable_hw(drvdata);
+
+	CS_LOCK(drvdata->base);
+}
+
+static void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
+{
+	CS_UNLOCK(drvdata->base);
+
+	tmc_flush_and_stop(drvdata);
+	tmc_disable_hw(drvdata);
+
+	CS_LOCK(drvdata->base);
+}
+
+static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
+{
+	unsigned long flags;
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+	if (drvdata->reading) {
+		spin_unlock_irqrestore(&drvdata->spinlock, flags);
+		return -EBUSY;
+	}
+
+	tmc_etb_enable_hw(drvdata);
+	drvdata->enable = true;
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+	dev_info(drvdata->dev, "TMC-ETB/ETF enabled\n");
+	return 0;
+}
+
+static void tmc_disable_etf_sink(struct coresight_device *csdev)
+{
+	unsigned long flags;
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+	if (drvdata->reading) {
+		spin_unlock_irqrestore(&drvdata->spinlock, flags);
+		return;
+	}
+
+	tmc_etb_disable_hw(drvdata);
+	drvdata->enable = false;
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+	dev_info(drvdata->dev, "TMC-ETB/ETF disabled\n");
+}
+
+static int tmc_enable_etf_link(struct coresight_device *csdev,
+			       int inport, int outport)
+{
+	unsigned long flags;
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+	if (drvdata->reading) {
+		spin_unlock_irqrestore(&drvdata->spinlock, flags);
+		return -EBUSY;
+	}
+
+	tmc_etf_enable_hw(drvdata);
+	drvdata->enable = true;
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+	dev_info(drvdata->dev, "TMC-ETF enabled\n");
+	return 0;
+}
+
+static void tmc_disable_etf_link(struct coresight_device *csdev,
+				 int inport, int outport)
+{
+	unsigned long flags;
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+	if (drvdata->reading) {
+		spin_unlock_irqrestore(&drvdata->spinlock, flags);
+		return;
+	}
+
+	tmc_etf_disable_hw(drvdata);
+	drvdata->enable = false;
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+	dev_info(drvdata->dev, "TMC disabled\n");
+}
+
+static const struct coresight_ops_sink tmc_etf_sink_ops = {
+	.enable		= tmc_enable_etf_sink,
+	.disable	= tmc_disable_etf_sink,
+};
+
+static const struct coresight_ops_link tmc_etf_link_ops = {
+	.enable		= tmc_enable_etf_link,
+	.disable	= tmc_disable_etf_link,
+};
+
+const struct coresight_ops tmc_etb_cs_ops = {
+	.sink_ops	= &tmc_etf_sink_ops,
+};
+
+const struct coresight_ops tmc_etf_cs_ops = {
+	.sink_ops	= &tmc_etf_sink_ops,
+	.link_ops	= &tmc_etf_link_ops,
+};
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
new file mode 100644
index 000000000000..5d9333ec49ae
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -0,0 +1,128 @@
+/*
+ * Copyright(C) 2016 Linaro Limited. All rights reserved.
+ * Author: Mathieu Poirier <mathieu.poirier@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/coresight.h>
+#include "coresight-priv.h"
+#include "coresight-tmc.h"
+
+void tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
+{
+	u32 axictl;
+
+	/* Zero out the memory to help with debug */
+	memset(drvdata->vaddr, 0, drvdata->size);
+
+	CS_UNLOCK(drvdata->base);
+
+	/* Wait for TMCSReady bit to be set */
+	tmc_wait_for_tmcready(drvdata);
+
+	writel_relaxed(drvdata->size / 4, drvdata->base + TMC_RSZ);
+	writel_relaxed(TMC_MODE_CIRCULAR_BUFFER, drvdata->base + TMC_MODE);
+
+	axictl = readl_relaxed(drvdata->base + TMC_AXICTL);
+	axictl |= TMC_AXICTL_WR_BURST_16;
+	writel_relaxed(axictl, drvdata->base + TMC_AXICTL);
+	axictl &= ~TMC_AXICTL_SCT_GAT_MODE;
+	writel_relaxed(axictl, drvdata->base + TMC_AXICTL);
+	axictl = (axictl &
+		  ~(TMC_AXICTL_PROT_CTL_B0 | TMC_AXICTL_PROT_CTL_B1)) |
+		  TMC_AXICTL_PROT_CTL_B1;
+	writel_relaxed(axictl, drvdata->base + TMC_AXICTL);
+
+	writel_relaxed(drvdata->paddr, drvdata->base + TMC_DBALO);
+	writel_relaxed(0x0, drvdata->base + TMC_DBAHI);
+	writel_relaxed(TMC_FFCR_EN_FMT | TMC_FFCR_EN_TI |
+		       TMC_FFCR_FON_FLIN | TMC_FFCR_FON_TRIG_EVT |
+		       TMC_FFCR_TRIGON_TRIGIN,
+		       drvdata->base + TMC_FFCR);
+	writel_relaxed(drvdata->trigger_cntr, drvdata->base + TMC_TRG);
+	tmc_enable_hw(drvdata);
+
+	CS_LOCK(drvdata->base);
+}
+
+static void tmc_etr_dump_hw(struct tmc_drvdata *drvdata)
+{
+	u32 rwp, val;
+
+	rwp = readl_relaxed(drvdata->base + TMC_RWP);
+	val = readl_relaxed(drvdata->base + TMC_STS);
+
+	/* How much memory do we still have */
+	if (val & BIT(0))
+		drvdata->buf = drvdata->vaddr + rwp - drvdata->paddr;
+	else
+		drvdata->buf = drvdata->vaddr;
+}
+
+void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
+{
+	CS_UNLOCK(drvdata->base);
+
+	tmc_flush_and_stop(drvdata);
+	tmc_etr_dump_hw(drvdata);
+	tmc_disable_hw(drvdata);
+
+	CS_LOCK(drvdata->base);
+}
+
+static int tmc_enable_etr_sink(struct coresight_device *csdev, u32 mode)
+{
+	unsigned long flags;
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+	if (drvdata->reading) {
+		spin_unlock_irqrestore(&drvdata->spinlock, flags);
+		return -EBUSY;
+	}
+
+	tmc_etr_enable_hw(drvdata);
+	drvdata->enable = true;
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+	dev_info(drvdata->dev, "TMC-ETR enabled\n");
+	return 0;
+}
+
+static void tmc_disable_etr_sink(struct coresight_device *csdev)
+{
+	unsigned long flags;
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+	if (drvdata->reading) {
+		spin_unlock_irqrestore(&drvdata->spinlock, flags);
+		return;
+	}
+
+	tmc_etr_disable_hw(drvdata);
+	drvdata->enable = false;
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+	dev_info(drvdata->dev, "TMC-ETR disabled\n");
+}
+
+static const struct coresight_ops_sink tmc_etr_sink_ops = {
+	.enable		= tmc_enable_etr_sink,
+	.disable	= tmc_disable_etr_sink,
+};
+
+const struct coresight_ops tmc_etr_cs_ops = {
+	.sink_ops	= &tmc_etr_sink_ops,
+};
diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
index 07e2809d832b..8d7f6d54c9b0 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.c
+++ b/drivers/hwtracing/coresight/coresight-tmc.c
@@ -32,7 +32,7 @@
 #include "coresight-priv.h"
 #include "coresight-tmc.h"
 
-static void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
+void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
 {
 	/* Ensure formatter, unformatter and hardware fifo are empty */
 	if (coresight_timeout(drvdata->base,
@@ -43,7 +43,7 @@ static void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
 	}
 }
 
-static void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
+void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
 {
 	u32 ffcr;
 
@@ -63,272 +63,16 @@ static void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
 	tmc_wait_for_tmcready(drvdata);
 }
 
-static void tmc_enable_hw(struct tmc_drvdata *drvdata)
+void tmc_enable_hw(struct tmc_drvdata *drvdata)
 {
 	writel_relaxed(TMC_CTL_CAPT_EN, drvdata->base + TMC_CTL);
 }
 
-static void tmc_disable_hw(struct tmc_drvdata *drvdata)
+void tmc_disable_hw(struct tmc_drvdata *drvdata)
 {
 	writel_relaxed(0x0, drvdata->base + TMC_CTL);
 }
 
-static void tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
-{
-	/* Zero out the memory to help with debug */
-	memset(drvdata->buf, 0, drvdata->size);
-
-	CS_UNLOCK(drvdata->base);
-
-	/* Wait for TMCSReady bit to be set */
-	tmc_wait_for_tmcready(drvdata);
-
-	writel_relaxed(TMC_MODE_CIRCULAR_BUFFER, drvdata->base + TMC_MODE);
-	writel_relaxed(TMC_FFCR_EN_FMT | TMC_FFCR_EN_TI |
-		       TMC_FFCR_FON_FLIN | TMC_FFCR_FON_TRIG_EVT |
-		       TMC_FFCR_TRIGON_TRIGIN,
-		       drvdata->base + TMC_FFCR);
-
-	writel_relaxed(drvdata->trigger_cntr, drvdata->base + TMC_TRG);
-	tmc_enable_hw(drvdata);
-
-	CS_LOCK(drvdata->base);
-}
-
-static void tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
-{
-	u32 axictl;
-
-	/* Zero out the memory to help with debug */
-	memset(drvdata->vaddr, 0, drvdata->size);
-
-	CS_UNLOCK(drvdata->base);
-
-	/* Wait for TMCSReady bit to be set */
-	tmc_wait_for_tmcready(drvdata);
-
-	writel_relaxed(drvdata->size / 4, drvdata->base + TMC_RSZ);
-	writel_relaxed(TMC_MODE_CIRCULAR_BUFFER, drvdata->base + TMC_MODE);
-
-	axictl = readl_relaxed(drvdata->base + TMC_AXICTL);
-	axictl |= TMC_AXICTL_WR_BURST_16;
-	writel_relaxed(axictl, drvdata->base + TMC_AXICTL);
-	axictl &= ~TMC_AXICTL_SCT_GAT_MODE;
-	writel_relaxed(axictl, drvdata->base + TMC_AXICTL);
-	axictl = (axictl &
-		  ~(TMC_AXICTL_PROT_CTL_B0 | TMC_AXICTL_PROT_CTL_B1)) |
-		  TMC_AXICTL_PROT_CTL_B1;
-	writel_relaxed(axictl, drvdata->base + TMC_AXICTL);
-
-	writel_relaxed(drvdata->paddr, drvdata->base + TMC_DBALO);
-	writel_relaxed(0x0, drvdata->base + TMC_DBAHI);
-	writel_relaxed(TMC_FFCR_EN_FMT | TMC_FFCR_EN_TI |
-		       TMC_FFCR_FON_FLIN | TMC_FFCR_FON_TRIG_EVT |
-		       TMC_FFCR_TRIGON_TRIGIN,
-		       drvdata->base + TMC_FFCR);
-	writel_relaxed(drvdata->trigger_cntr, drvdata->base + TMC_TRG);
-	tmc_enable_hw(drvdata);
-
-	CS_LOCK(drvdata->base);
-}
-
-static void tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
-{
-	CS_UNLOCK(drvdata->base);
-
-	/* Wait for TMCSReady bit to be set */
-	tmc_wait_for_tmcready(drvdata);
-
-	writel_relaxed(TMC_MODE_HARDWARE_FIFO, drvdata->base + TMC_MODE);
-	writel_relaxed(TMC_FFCR_EN_FMT | TMC_FFCR_EN_TI,
-		       drvdata->base + TMC_FFCR);
-	writel_relaxed(0x0, drvdata->base + TMC_BUFWM);
-	tmc_enable_hw(drvdata);
-
-	CS_LOCK(drvdata->base);
-}
-
-static int tmc_enable(struct tmc_drvdata *drvdata, enum tmc_mode mode)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&drvdata->spinlock, flags);
-	if (drvdata->reading) {
-		spin_unlock_irqrestore(&drvdata->spinlock, flags);
-		return -EBUSY;
-	}
-
-	if (drvdata->config_type == TMC_CONFIG_TYPE_ETB) {
-		tmc_etb_enable_hw(drvdata);
-	} else if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
-		tmc_etr_enable_hw(drvdata);
-	} else {
-		if (mode == TMC_MODE_CIRCULAR_BUFFER)
-			tmc_etb_enable_hw(drvdata);
-		else
-			tmc_etf_enable_hw(drvdata);
-	}
-	drvdata->enable = true;
-	spin_unlock_irqrestore(&drvdata->spinlock, flags);
-
-	dev_info(drvdata->dev, "TMC enabled\n");
-	return 0;
-}
-
-static int tmc_enable_sink(struct coresight_device *csdev, u32 mode)
-{
-	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
-
-	return tmc_enable(drvdata, TMC_MODE_CIRCULAR_BUFFER);
-}
-
-static int tmc_enable_link(struct coresight_device *csdev, int inport,
-			   int outport)
-{
-	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
-
-	return tmc_enable(drvdata, TMC_MODE_HARDWARE_FIFO);
-}
-
-static void tmc_etb_dump_hw(struct tmc_drvdata *drvdata)
-{
-	enum tmc_mem_intf_width memwidth;
-	u8 memwords;
-	char *bufp;
-	u32 read_data;
-	int i;
-
-	memwidth = BMVAL(readl_relaxed(drvdata->base + CORESIGHT_DEVID), 8, 10);
-	if (memwidth == TMC_MEM_INTF_WIDTH_32BITS)
-		memwords = 1;
-	else if (memwidth == TMC_MEM_INTF_WIDTH_64BITS)
-		memwords = 2;
-	else if (memwidth == TMC_MEM_INTF_WIDTH_128BITS)
-		memwords = 4;
-	else
-		memwords = 8;
-
-	bufp = drvdata->buf;
-	while (1) {
-		for (i = 0; i < memwords; i++) {
-			read_data = readl_relaxed(drvdata->base + TMC_RRD);
-			if (read_data == 0xFFFFFFFF)
-				return;
-			memcpy(bufp, &read_data, 4);
-			bufp += 4;
-		}
-	}
-}
-
-static void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
-{
-	CS_UNLOCK(drvdata->base);
-
-	tmc_flush_and_stop(drvdata);
-	tmc_etb_dump_hw(drvdata);
-	tmc_disable_hw(drvdata);
-
-	CS_LOCK(drvdata->base);
-}
-
-static void tmc_etr_dump_hw(struct tmc_drvdata *drvdata)
-{
-	u32 rwp, val;
-
-	rwp = readl_relaxed(drvdata->base + TMC_RWP);
-	val = readl_relaxed(drvdata->base + TMC_STS);
-
-	/* How much memory do we still have */
-	if (val & BIT(0))
-		drvdata->buf = drvdata->vaddr + rwp - drvdata->paddr;
-	else
-		drvdata->buf = drvdata->vaddr;
-}
-
-static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
-{
-	CS_UNLOCK(drvdata->base);
-
-	tmc_flush_and_stop(drvdata);
-	tmc_etr_dump_hw(drvdata);
-	tmc_disable_hw(drvdata);
-
-	CS_LOCK(drvdata->base);
-}
-
-static void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
-{
-	CS_UNLOCK(drvdata->base);
-
-	tmc_flush_and_stop(drvdata);
-	tmc_disable_hw(drvdata);
-
-	CS_LOCK(drvdata->base);
-}
-
-static void tmc_disable(struct tmc_drvdata *drvdata, enum tmc_mode mode)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&drvdata->spinlock, flags);
-	if (drvdata->reading)
-		goto out;
-
-	if (drvdata->config_type == TMC_CONFIG_TYPE_ETB) {
-		tmc_etb_disable_hw(drvdata);
-	} else if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
-		tmc_etr_disable_hw(drvdata);
-	} else {
-		if (mode == TMC_MODE_CIRCULAR_BUFFER)
-			tmc_etb_disable_hw(drvdata);
-		else
-			tmc_etf_disable_hw(drvdata);
-	}
-out:
-	drvdata->enable = false;
-	spin_unlock_irqrestore(&drvdata->spinlock, flags);
-
-	dev_info(drvdata->dev, "TMC disabled\n");
-}
-
-static void tmc_disable_sink(struct coresight_device *csdev)
-{
-	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
-
-	tmc_disable(drvdata, TMC_MODE_CIRCULAR_BUFFER);
-}
-
-static void tmc_disable_link(struct coresight_device *csdev, int inport,
-			     int outport)
-{
-	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
-
-	tmc_disable(drvdata, TMC_MODE_HARDWARE_FIFO);
-}
-
-static const struct coresight_ops_sink tmc_sink_ops = {
-	.enable		= tmc_enable_sink,
-	.disable	= tmc_disable_sink,
-};
-
-static const struct coresight_ops_link tmc_link_ops = {
-	.enable		= tmc_enable_link,
-	.disable	= tmc_disable_link,
-};
-
-static const struct coresight_ops tmc_etb_cs_ops = {
-	.sink_ops	= &tmc_sink_ops,
-};
-
-static const struct coresight_ops tmc_etr_cs_ops = {
-	.sink_ops	= &tmc_sink_ops,
-};
-
-static const struct coresight_ops tmc_etf_cs_ops = {
-	.sink_ops	= &tmc_sink_ops,
-	.link_ops	= &tmc_link_ops,
-};
-
 static int tmc_read_prepare(struct tmc_drvdata *drvdata)
 {
 	int ret = 0;
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index 5a60830c8db5..b3017e115c4d 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -18,6 +18,8 @@
 #ifndef _CORESIGHT_TMC_H
 #define _CORESIGHT_TMC_H
 
+#include <linux/miscdevice.h>
+
 #define TMC_RSZ			0x004
 #define TMC_STS			0x00c
 #define TMC_RRD			0x010
@@ -118,4 +120,20 @@ struct tmc_drvdata {
 	u32			trigger_cntr;
 };
 
+/* Generic functions */
+void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata);
+void tmc_flush_and_stop(struct tmc_drvdata *drvdata);
+void tmc_enable_hw(struct tmc_drvdata *drvdata);
+void tmc_disable_hw(struct tmc_drvdata *drvdata);
+
+/* ETB/ETF functions */
+void tmc_etb_enable_hw(struct tmc_drvdata *drvdata);
+void tmc_etb_disable_hw(struct tmc_drvdata *drvdata);
+extern const struct coresight_ops tmc_etb_cs_ops;
+extern const struct coresight_ops tmc_etf_cs_ops;
+
+/* ETR functions */
+void tmc_etr_enable_hw(struct tmc_drvdata *drvdata);
+void tmc_etr_disable_hw(struct tmc_drvdata *drvdata);
+extern const struct coresight_ops tmc_etr_cs_ops;
 #endif
-- 
2.5.0

[PATCH V3 08/18] coresight: tmc: making prepare/unprepare functions generic

[Mathieu Poirier]

Dealing with HW related matters in tmc_read_prepare/unprepare
becomes convoluted when many cases need to be handled distinctively.

As such moving processing related to HW setup to individual driver
files and keep the core driver generic.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-tmc-etf.c | 62 ++++++++++++++++++++++++-
 drivers/hwtracing/coresight/coresight-tmc-etr.c | 42 ++++++++++++++++-
 drivers/hwtracing/coresight/coresight-tmc.c     | 55 +++++-----------------
 drivers/hwtracing/coresight/coresight-tmc.h     |  8 ++--
 4 files changed, 117 insertions(+), 50 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index 467d19221f7b..91e43572ce9f 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -71,7 +71,7 @@ static void tmc_etb_dump_hw(struct tmc_drvdata *drvdata)
 	}
 }
 
-void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
+static void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
 {
 	CS_UNLOCK(drvdata->base);
 
@@ -202,3 +202,63 @@ const struct coresight_ops tmc_etf_cs_ops = {
 	.sink_ops	= &tmc_etf_sink_ops,
 	.link_ops	= &tmc_etf_link_ops,
 };
+
+int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
+{
+	enum tmc_mode mode;
+	int ret = 0;
+	unsigned long flags;
+
+	/* config types are set a boot time and never change */
+	if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETB &&
+			 drvdata->config_type != TMC_CONFIG_TYPE_ETF))
+		return -EINVAL;
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+
+	/* There is no point in reading a TMC in HW FIFO mode */
+	mode = readl_relaxed(drvdata->base + TMC_MODE);
+	if (mode != TMC_MODE_CIRCULAR_BUFFER) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Disable the TMC if need be */
+	if (drvdata->enable)
+		tmc_etb_disable_hw(drvdata);
+
+	drvdata->reading = true;
+out:
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+	return ret;
+}
+
+int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
+{
+	enum tmc_mode mode;
+	unsigned long flags;
+
+	/* config types are set a boot time and never change */
+	if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETB &&
+			 drvdata->config_type != TMC_CONFIG_TYPE_ETF))
+		return -EINVAL;
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+
+	/* There is no point in reading a TMC in HW FIFO mode */
+	mode = readl_relaxed(drvdata->base + TMC_MODE);
+	if (mode != TMC_MODE_CIRCULAR_BUFFER) {
+		spin_unlock_irqrestore(&drvdata->spinlock, flags);
+		return -EINVAL;
+	}
+
+	/* Re-enable the TMC if need be */
+	if (drvdata->enable)
+		tmc_etb_enable_hw(drvdata);
+
+	drvdata->reading = false;
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+	return 0;
+}
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 5d9333ec49ae..3483d139a4ac 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -70,7 +70,7 @@ static void tmc_etr_dump_hw(struct tmc_drvdata *drvdata)
 		drvdata->buf = drvdata->vaddr;
 }
 
-void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
+static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
 {
 	CS_UNLOCK(drvdata->base);
 
@@ -126,3 +126,43 @@ static const struct coresight_ops_sink tmc_etr_sink_ops = {
 const struct coresight_ops tmc_etr_cs_ops = {
 	.sink_ops	= &tmc_etr_sink_ops,
 };
+
+int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
+{
+	unsigned long flags;
+
+	/* config types are set a boot time and never change */
+	if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETR))
+		return -EINVAL;
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+
+	/* Disable the TMC if need be */
+	if (drvdata->enable)
+		tmc_etr_disable_hw(drvdata);
+
+	drvdata->reading = true;
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+	return 0;
+}
+
+int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
+{
+	unsigned long flags;
+
+	/* config types are set a boot time and never change */
+	if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETR))
+		return -EINVAL;
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+
+	/* RE-enable the TMC if need be */
+	if (drvdata->enable)
+		tmc_etr_enable_hw(drvdata);
+
+	drvdata->reading = false;
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+	return 0;
+}
diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
index 8d7f6d54c9b0..63f8e55116a6 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.c
+++ b/drivers/hwtracing/coresight/coresight-tmc.c
@@ -76,76 +76,43 @@ void tmc_disable_hw(struct tmc_drvdata *drvdata)
 static int tmc_read_prepare(struct tmc_drvdata *drvdata)
 {
 	int ret = 0;
-	unsigned long flags;
-	enum tmc_mode mode;
-
-	spin_lock_irqsave(&drvdata->spinlock, flags);
-	if (!drvdata->enable)
-		goto out;
 
 	switch (drvdata->config_type) {
 	case TMC_CONFIG_TYPE_ETB:
-		tmc_etb_disable_hw(drvdata);
-		break;
 	case TMC_CONFIG_TYPE_ETF:
-		/* There is no point in reading a TMC in HW FIFO mode */
-		mode = readl_relaxed(drvdata->base + TMC_MODE);
-		if (mode != TMC_MODE_CIRCULAR_BUFFER) {
-			ret = -EINVAL;
-			goto err;
-		}
-
-		tmc_etb_disable_hw(drvdata);
+		ret = tmc_read_prepare_etb(drvdata);
 		break;
 	case TMC_CONFIG_TYPE_ETR:
-		tmc_etr_disable_hw(drvdata);
+		ret = tmc_read_prepare_etr(drvdata);
 		break;
 	default:
 		ret = -EINVAL;
-		goto err;
 	}
 
-out:
-	drvdata->reading = true;
-	dev_info(drvdata->dev, "TMC read start\n");
-err:
-	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+	if (!ret)
+		dev_info(drvdata->dev, "TMC read start\n");
+
 	return ret;
 }
 
 static void tmc_read_unprepare(struct tmc_drvdata *drvdata)
 {
-	unsigned long flags;
-	enum tmc_mode mode;
-
-	spin_lock_irqsave(&drvdata->spinlock, flags);
-	if (!drvdata->enable)
-		goto out;
+	int ret = 0;
 
 	switch (drvdata->config_type) {
 	case TMC_CONFIG_TYPE_ETB:
-		tmc_etb_enable_hw(drvdata);
-		break;
 	case TMC_CONFIG_TYPE_ETF:
-		/* Make sure we don't re-enable a TMC in HW FIFO mode */
-		mode = readl_relaxed(drvdata->base + TMC_MODE);
-		if (mode != TMC_MODE_CIRCULAR_BUFFER)
-			goto err;
-
-		tmc_etb_enable_hw(drvdata);
+		ret = tmc_read_unprepare_etb(drvdata);
 		break;
 	case TMC_CONFIG_TYPE_ETR:
-		tmc_etr_disable_hw(drvdata);
+		ret = tmc_read_unprepare_etr(drvdata);
 		break;
 	default:
-		goto err;
+		ret = -EINVAL;
 	}
 
-out:
-	drvdata->reading = false;
-	dev_info(drvdata->dev, "TMC read end\n");
-err:
-	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+	if (!ret)
+		dev_info(drvdata->dev, "TMC read end\n");
 }
 
 static int tmc_open(struct inode *inode, struct file *file)
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index b3017e115c4d..df661903f83c 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -127,13 +127,13 @@ void tmc_enable_hw(struct tmc_drvdata *drvdata);
 void tmc_disable_hw(struct tmc_drvdata *drvdata);
 
 /* ETB/ETF functions */
-void tmc_etb_enable_hw(struct tmc_drvdata *drvdata);
-void tmc_etb_disable_hw(struct tmc_drvdata *drvdata);
+int tmc_read_prepare_etb(struct tmc_drvdata *drvdata);
+int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata);
 extern const struct coresight_ops tmc_etb_cs_ops;
 extern const struct coresight_ops tmc_etf_cs_ops;
 
 /* ETR functions */
-void tmc_etr_enable_hw(struct tmc_drvdata *drvdata);
-void tmc_etr_disable_hw(struct tmc_drvdata *drvdata);
+int tmc_read_prepare_etr(struct tmc_drvdata *drvdata);
+int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata);
 extern const struct coresight_ops tmc_etr_cs_ops;
 #endif
-- 
2.5.0

[PATCH V3 09/18] coresight: tmc: allocating memory when needed

[Mathieu Poirier]

In it's current form the TMC probe() function allocates
trace buffer memory at boot time, event if coresight isn't
used.  This is highly inefficient since trace buffers can
occupy a lot of memory that could be used otherwised.

This patch allocates trace buffers on the fly, when the
coresight subsystem is solicited.  Allocated buffers are
released when traces are read using the device descriptors
under /dev.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/hwtracing/coresight/coresight-tmc-etf.c | 86 +++++++++++++++++++++++--
 drivers/hwtracing/coresight/coresight-tmc-etr.c | 84 +++++++++++++++++++++++-
 drivers/hwtracing/coresight/coresight-tmc.c     | 14 ----
 3 files changed, 165 insertions(+), 19 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index 91e43572ce9f..bcfa40e8cd1c 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -16,14 +16,12 @@
  */
 
 #include <linux/coresight.h>
+#include <linux/slab.h>
 #include "coresight-priv.h"
 #include "coresight-tmc.h"
 
 void tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
 {
-	/* Zero out the memory to help with debug */
-	memset(drvdata->buf, 0, drvdata->size);
-
 	CS_UNLOCK(drvdata->base);
 
 	/* Wait for TMCSReady bit to be set */
@@ -110,19 +108,69 @@ static void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
 
 static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
 {
+	bool used = false;
+	char *buf = NULL;
 	unsigned long flags;
 	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 
+	 /* This shouldn't be happening */
+	if (WARN_ON(mode != CS_MODE_SYSFS))
+		return -EINVAL;
+
+	/*
+	 * If a buffer is already allocated *keep holding* the lock and
+	 * jump to the fast path.  Otherwise release the lock and allocate
+	 * memory to work with.
+	 */
 	spin_lock_irqsave(&drvdata->spinlock, flags);
+	if (drvdata->buf)
+		goto fast_path;
+
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+	/* Allocating the memory here while outside of the spinlock */
+	buf = kzalloc(drvdata->size, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	/* Let's try again */
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+fast_path:
 	if (drvdata->reading) {
 		spin_unlock_irqrestore(&drvdata->spinlock, flags);
+		/*
+		 * Free allocated memory outside of the spinlock.  There is
+		 * no need to assert the validity of 'buf' since calling
+		 * kfree(NULL) is safe.
+		 */
+		kfree(buf);
 		return -EBUSY;
 	}
 
+	/*
+	 * If drvdata::buf isn't NULL, memory was allocated for a previous
+	 * trace run but wasn't read.  If so simply zero-out the memory.
+	 * Otherwise use the memory allocated above.
+	 *
+	 * The memory is freed when users read the buffer using the
+	 * /dev/xyz.{etf|etb} interface.  See tmc_read_unprepare_etf() for
+	 * details.
+	 */
+	if (drvdata->buf) {
+		memset(drvdata->buf, 0, drvdata->size);
+	} else {
+		used = true;
+		drvdata->buf = buf;
+	}
+
 	tmc_etb_enable_hw(drvdata);
 	drvdata->enable = true;
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
+	/* Free memory outside the spinlock if need be */
+	if (!used && buf)
+		kfree(buf);
+
 	dev_info(drvdata->dev, "TMC-ETB/ETF enabled\n");
 	return 0;
 }
@@ -223,6 +271,12 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
 		goto out;
 	}
 
+	/* If drvdata::buf is NULL the trace data has been read already */
+	if (drvdata->buf == NULL) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	/* Disable the TMC if need be */
 	if (drvdata->enable)
 		tmc_etb_disable_hw(drvdata);
@@ -236,6 +290,7 @@ out:
 
 int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
 {
+	char *buf = NULL;
 	enum tmc_mode mode;
 	unsigned long flags;
 
@@ -254,11 +309,34 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
 	}
 
 	/* Re-enable the TMC if need be */
-	if (drvdata->enable)
+	if (drvdata->enable) {
+		/*
+		 * The trace run will continue with the same allocated trace
+		 * buffer. As such zero-out the buffer so that we don't end
+		 * up with stale data.
+		 *
+		 * Since the tracer is still enabled drvdata::buf
+		 * can't be NULL.
+		 */
+		memset(drvdata->buf, 0, drvdata->size);
 		tmc_etb_enable_hw(drvdata);
+	} else {
+		/*
+		 * The ETB/ETF is not tracing and the buffer was just read.
+		 * As such prepare to free the trace buffer.
+		 */
+		buf = drvdata->buf;
+		drvdata->buf = NULL;
+	}
 
 	drvdata->reading = false;
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
+	/*
+	 * Free allocated memory outside of the spinlock.  There is no need
+	 * to assert the validity of 'buf' since calling kfree(NULL) is safe.
+	 */
+	kfree(buf);
+
 	return 0;
 }
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 3483d139a4ac..474c70c089f3 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -16,6 +16,7 @@
  */
 
 #include <linux/coresight.h>
+#include <linux/dma-mapping.h>
 #include "coresight-priv.h"
 #include "coresight-tmc.h"
 
@@ -83,19 +84,70 @@ static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
 
 static int tmc_enable_etr_sink(struct coresight_device *csdev, u32 mode)
 {
+	bool used = false;
 	unsigned long flags;
+	void __iomem *vaddr = NULL;
+	dma_addr_t paddr;
 	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 
+	 /* This shouldn't be happening */
+	if (WARN_ON(mode != CS_MODE_SYSFS))
+		return -EINVAL;
+
+	/*
+	 * If a buffer is already allocated *keep holding* the lock and
+	 * jump to the fast path.  Otherwise release the lock and allocate
+	 * memory to work with.
+	 */
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+	if (drvdata->vaddr)
+		goto fast_path;
+
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+	/*
+	 * Contiguous  memory can't be allocated while a spinlock is held.
+	 * As such allocate memory here and free it if a buffer has already
+	 * been allocated (from a previous session).
+	 */
+	vaddr = dma_alloc_coherent(drvdata->dev, drvdata->size,
+				   &paddr, GFP_KERNEL);
+	if (!vaddr)
+		return -ENOMEM;
+
+	/* Let's try again */
 	spin_lock_irqsave(&drvdata->spinlock, flags);
+fast_path:
 	if (drvdata->reading) {
 		spin_unlock_irqrestore(&drvdata->spinlock, flags);
+		if (vaddr)
+			dma_free_coherent(drvdata->dev, drvdata->size,
+					  vaddr, paddr);
 		return -EBUSY;
 	}
 
+	/*
+	 * If drvdata::buf == NULL, use the memory allocated above.
+	 * Otherwise a buffer still exists from a previous session, so
+	 * simply use that.
+	 */
+	if (drvdata->buf == NULL) {
+		used = true;
+		drvdata->vaddr = vaddr;
+		drvdata->paddr = paddr;
+		drvdata->buf = drvdata->vaddr;
+	}
+
+	memset(drvdata->vaddr, 0, drvdata->size);
+
 	tmc_etr_enable_hw(drvdata);
 	drvdata->enable = true;
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
+	/* Free memory outside the spinlock if need be */
+	if (!used && vaddr)
+		dma_free_coherent(drvdata->dev, drvdata->size, vaddr, paddr);
+
 	dev_info(drvdata->dev, "TMC-ETR enabled\n");
 	return 0;
 }
@@ -137,6 +189,12 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
 
 	spin_lock_irqsave(&drvdata->spinlock, flags);
 
+	/* If drvdata::buf is NULL the trace data has been read already */
+	if (drvdata->buf == NULL) {
+		spin_unlock_irqrestore(&drvdata->spinlock, flags);
+		return -EINVAL;
+	}
+
 	/* Disable the TMC if need be */
 	if (drvdata->enable)
 		tmc_etr_disable_hw(drvdata);
@@ -150,6 +208,8 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
 int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
 {
 	unsigned long flags;
+	dma_addr_t paddr;
+	void __iomem *vaddr = NULL;
 
 	/* config types are set a boot time and never change */
 	if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETR))
@@ -158,11 +218,33 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
 	spin_lock_irqsave(&drvdata->spinlock, flags);
 
 	/* RE-enable the TMC if need be */
-	if (drvdata->enable)
+	if (drvdata->enable) {
+		/*
+		 * The trace run will continue with the same allocated trace
+		 * buffer. As such zero-out the buffer so that we don't end
+		 * up with stale data.
+		 *
+		 * Since the tracer is still enabled drvdata::buf
+		 * can't be NULL.
+		 */
+		memset(drvdata->buf, 0, drvdata->size);
 		tmc_etr_enable_hw(drvdata);
+	} else {
+		/*
+		 * The ETR is not tracing and the buffer was just read.
+		 * As such prepare to free the trace buffer.
+		 */
+		vaddr = drvdata->vaddr;
+		paddr = drvdata->paddr;
+		drvdata->buf = NULL;
+	}
 
 	drvdata->reading = false;
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
+	/* Free allocated memory out side of the spinlock */
+	if (vaddr)
+		dma_free_coherent(drvdata->dev, drvdata->size, vaddr, paddr);
+
 	return 0;
 }
diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
index 63f8e55116a6..e8e12a9b917a 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.c
+++ b/drivers/hwtracing/coresight/coresight-tmc.c
@@ -319,20 +319,6 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
 
 	pm_runtime_put(&adev->dev);
 
-	if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
-		drvdata->vaddr = dma_alloc_coherent(dev, drvdata->size,
-						&drvdata->paddr, GFP_KERNEL);
-		if (!drvdata->vaddr)
-			return -ENOMEM;
-
-		memset(drvdata->vaddr, 0, drvdata->size);
-		drvdata->buf = drvdata->vaddr;
-	} else {
-		drvdata->buf = devm_kzalloc(dev, drvdata->size, GFP_KERNEL);
-		if (!drvdata->buf)
-			return -ENOMEM;
-	}
-
 	desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
 	if (!desc) {
 		ret = -ENOMEM;
-- 
2.5.0

[PATCH V3 10/18] coresight: tmc: getting the right read_count on tmc_open()

[Mathieu Poirier]

In function tmc_open(), if tmc_read_prepare() fails variable
drvdata->read_count is not decremented, causing unwanted
access to drvdata->buf and very likely, a crash dump.

By moving the incrementation to a place where we know things
are stable this kind of situation is avoided.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Suzuki K Poulose <Suzuki.Poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-tmc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
index e8e12a9b917a..55806352b1f1 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.c
+++ b/drivers/hwtracing/coresight/coresight-tmc.c
@@ -121,13 +121,14 @@ static int tmc_open(struct inode *inode, struct file *file)
 						   struct tmc_drvdata, miscdev);
 	int ret = 0;
 
-	if (drvdata->read_count++)
+	if (drvdata->read_count)
 		goto out;
 
 	ret = tmc_read_prepare(drvdata);
 	if (ret)
 		return ret;
 out:
+	drvdata->read_count++;
 	nonseekable_open(inode, file);
 
 	dev_dbg(drvdata->dev, "%s: successfully opened\n", __func__);
-- 
2.5.0

[PATCH V3 11/18] coresight: tmc: adding mode of operation for link/sinks

[Mathieu Poirier]

Moving tmc_drvdata::enable to a local_t mode.  That way the
sink interface is aware of it's orgin and the foundation for
mutual exclusion between the sysFS and Perf interface can be
laid out.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/hwtracing/coresight/coresight-tmc-etf.c | 28 ++++++++++++++++++-------
 drivers/hwtracing/coresight/coresight-tmc-etr.c | 24 ++++++++++++++++-----
 drivers/hwtracing/coresight/coresight-tmc.h     |  4 ++--
 3 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index bcfa40e8cd1c..d090a9745c73 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -110,6 +110,7 @@ static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
 {
 	bool used = false;
 	char *buf = NULL;
+	long val;
 	unsigned long flags;
 	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 
@@ -147,6 +148,15 @@ fast_path:
 		return -EBUSY;
 	}
 
+	val = local_xchg(&drvdata->mode, mode);
+	/*
+	 * In sysFS mode we can have multiple writers per sink.  Since this
+	 * sink is already enabled no memory is needed and the HW need not be
+	 * touched.
+	 */
+	if (val == CS_MODE_SYSFS)
+		goto spin_unlock;
+
 	/*
 	 * If drvdata::buf isn't NULL, memory was allocated for a previous
 	 * trace run but wasn't read.  If so simply zero-out the memory.
@@ -164,7 +174,7 @@ fast_path:
 	}
 
 	tmc_etb_enable_hw(drvdata);
-	drvdata->enable = true;
+spin_unlock:
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
 	/* Free memory outside the spinlock if need be */
@@ -177,6 +187,7 @@ fast_path:
 
 static void tmc_disable_etf_sink(struct coresight_device *csdev)
 {
+	long val;
 	unsigned long flags;
 	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 
@@ -186,8 +197,11 @@ static void tmc_disable_etf_sink(struct coresight_device *csdev)
 		return;
 	}
 
-	tmc_etb_disable_hw(drvdata);
-	drvdata->enable = false;
+	val = local_xchg(&drvdata->mode, CS_MODE_DISABLED);
+	/* Disable the TMC only if it needs to */
+	if (val != CS_MODE_DISABLED)
+		tmc_etb_disable_hw(drvdata);
+
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
 	dev_info(drvdata->dev, "TMC-ETB/ETF disabled\n");
@@ -206,7 +220,7 @@ static int tmc_enable_etf_link(struct coresight_device *csdev,
 	}
 
 	tmc_etf_enable_hw(drvdata);
-	drvdata->enable = true;
+	local_set(&drvdata->mode, CS_MODE_SYSFS);
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
 	dev_info(drvdata->dev, "TMC-ETF enabled\n");
@@ -226,7 +240,7 @@ static void tmc_disable_etf_link(struct coresight_device *csdev,
 	}
 
 	tmc_etf_disable_hw(drvdata);
-	drvdata->enable = false;
+	local_set(&drvdata->mode, CS_MODE_DISABLED);
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
 	dev_info(drvdata->dev, "TMC disabled\n");
@@ -278,7 +292,7 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
 	}
 
 	/* Disable the TMC if need be */
-	if (drvdata->enable)
+	if (local_read(&drvdata->mode) == CS_MODE_SYSFS)
 		tmc_etb_disable_hw(drvdata);
 
 	drvdata->reading = true;
@@ -309,7 +323,7 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
 	}
 
 	/* Re-enable the TMC if need be */
-	if (drvdata->enable) {
+	if (local_read(&drvdata->mode) == CS_MODE_SYSFS) {
 		/*
 		 * The trace run will continue with the same allocated trace
 		 * buffer. As such zero-out the buffer so that we don't end
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 474c70c089f3..8bbbf3ab1387 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -85,6 +85,7 @@ static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
 static int tmc_enable_etr_sink(struct coresight_device *csdev, u32 mode)
 {
 	bool used = false;
+	long val;
 	unsigned long flags;
 	void __iomem *vaddr = NULL;
 	dma_addr_t paddr;
@@ -126,6 +127,15 @@ fast_path:
 		return -EBUSY;
 	}
 
+	val = local_xchg(&drvdata->mode, mode);
+	/*
+	 * In sysFS mode we can have multiple writers per sink.  Since this
+	 * sink is already enabled no memory is needed and the HW need not be
+	 * touched.
+	 */
+	if (val == CS_MODE_SYSFS)
+		goto spin_unlock;
+
 	/*
 	 * If drvdata::buf == NULL, use the memory allocated above.
 	 * Otherwise a buffer still exists from a previous session, so
@@ -141,7 +151,7 @@ fast_path:
 	memset(drvdata->vaddr, 0, drvdata->size);
 
 	tmc_etr_enable_hw(drvdata);
-	drvdata->enable = true;
+spin_unlock:
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
 	/* Free memory outside the spinlock if need be */
@@ -154,6 +164,7 @@ fast_path:
 
 static void tmc_disable_etr_sink(struct coresight_device *csdev)
 {
+	long val;
 	unsigned long flags;
 	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 
@@ -163,8 +174,11 @@ static void tmc_disable_etr_sink(struct coresight_device *csdev)
 		return;
 	}
 
-	tmc_etr_disable_hw(drvdata);
-	drvdata->enable = false;
+	val = local_xchg(&drvdata->mode, CS_MODE_DISABLED);
+	/* Disable the TMC only if it needs to */
+	if (val != CS_MODE_DISABLED)
+		tmc_etr_disable_hw(drvdata);
+
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
 	dev_info(drvdata->dev, "TMC-ETR disabled\n");
@@ -196,7 +210,7 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
 	}
 
 	/* Disable the TMC if need be */
-	if (drvdata->enable)
+	if (local_read(&drvdata->mode) == CS_MODE_SYSFS)
 		tmc_etr_disable_hw(drvdata);
 
 	drvdata->reading = true;
@@ -218,7 +232,7 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
 	spin_lock_irqsave(&drvdata->spinlock, flags);
 
 	/* RE-enable the TMC if need be */
-	if (drvdata->enable) {
+	if (local_read(&drvdata->mode) == CS_MODE_SYSFS) {
 		/*
 		 * The trace run will continue with the same allocated trace
 		 * buffer. As such zero-out the buffer so that we don't end
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index df661903f83c..9b4c215d2b6b 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -99,7 +99,7 @@ enum tmc_mem_intf_width {
  * @paddr:	DMA start location in RAM.
  * @vaddr:	virtual representation of @paddr.
  * @size:	@buf size.
- * @enable:	this TMC is being used.
+ * @mode:	how this TMC is being used.
  * @config_type: TMC variant, must be of type @tmc_config_type.
  * @trigger_cntr: amount of words to store after a trigger.
  */
@@ -115,7 +115,7 @@ struct tmc_drvdata {
 	dma_addr_t		paddr;
 	void __iomem		*vaddr;
 	u32			size;
-	bool			enable;
+	local_t			mode;
 	enum tmc_config_type	config_type;
 	u32			trigger_cntr;
 };
-- 
2.5.0

[PATCH V3 12/18] coresight: tmc: dump system memory content only when needed

[Mathieu Poirier]

Calling tmc_etf/etr_dump_hw() is required only when operating from
sysFS.  When working from Perf, the system memory is harvested
from the AUX trace API.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/hwtracing/coresight/coresight-tmc-etf.c | 7 ++++++-
 drivers/hwtracing/coresight/coresight-tmc-etr.c | 7 ++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index d090a9745c73..25fad93b68d4 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -74,7 +74,12 @@ static void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
 	CS_UNLOCK(drvdata->base);
 
 	tmc_flush_and_stop(drvdata);
-	tmc_etb_dump_hw(drvdata);
+	/*
+	 * When operating in sysFS mode the content of the buffer needs to be
+	 * read before the TMC is disabled.
+	 */
+	if (local_read(&drvdata->mode) == CS_MODE_SYSFS)
+		tmc_etb_dump_hw(drvdata);
 	tmc_disable_hw(drvdata);
 
 	CS_LOCK(drvdata->base);
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 8bbbf3ab1387..4b000f4003a2 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -76,7 +76,12 @@ static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
 	CS_UNLOCK(drvdata->base);
 
 	tmc_flush_and_stop(drvdata);
-	tmc_etr_dump_hw(drvdata);
+	/*
+	 * When operating in sysFS mode the content of the buffer needs to be
+	 * read before the TMC is disabled.
+	 */
+	if (local_read(&drvdata->mode) == CS_MODE_SYSFS)
+		tmc_etr_dump_hw(drvdata);
 	tmc_disable_hw(drvdata);
 
 	CS_LOCK(drvdata->base);
-- 
2.5.0

[PATCH V3 13/18] coresight: tmc: make sysFS and Perf mode mutually exclusive

[Mathieu Poirier]

The sysFS and Perf access methods can't be allowed to interfere
with one another.  As such introducing guards to access
functions that prevents moving forward if a TMC is already
being used.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/hwtracing/coresight/coresight-tmc-etf.c | 60 +++++++++++++++++++++-
 drivers/hwtracing/coresight/coresight-tmc-etr.c | 68 +++++++++++++++++++++++--
 2 files changed, 121 insertions(+), 7 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index 25fad93b68d4..cc88c15ba45c 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -111,7 +111,7 @@ static void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
 	CS_LOCK(drvdata->base);
 }
 
-static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
+static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev, u32 mode)
 {
 	bool used = false;
 	char *buf = NULL;
@@ -190,6 +190,54 @@ spin_unlock:
 	return 0;
 }
 
+static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, u32 mode)
+{
+	int ret = 0;
+	long val;
+	unsigned long flags;
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	 /* This shouldn't be happening */
+	if (WARN_ON(mode != CS_MODE_PERF))
+		return -EINVAL;
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+	if (drvdata->reading) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	val = local_xchg(&drvdata->mode, mode);
+	/*
+	 * In Perf mode there can be only one writer per sink.  There
+	 * is also no need to continue if the ETB/ETR is already operated
+	 * from sysFS.
+	 */
+	if (val != CS_MODE_DISABLED) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	tmc_etb_enable_hw(drvdata);
+out:
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+	return ret;
+}
+
+static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
+{
+	switch (mode) {
+	case CS_MODE_SYSFS:
+		return tmc_enable_etf_sink_sysfs(csdev, mode);
+	case CS_MODE_PERF:
+		return tmc_enable_etf_sink_perf(csdev, mode);
+	}
+
+	/* We shouldn't be here */
+	return -EINVAL;
+}
+
 static void tmc_disable_etf_sink(struct coresight_device *csdev)
 {
 	long val;
@@ -272,6 +320,7 @@ const struct coresight_ops tmc_etf_cs_ops = {
 
 int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
 {
+	long val;
 	enum tmc_mode mode;
 	int ret = 0;
 	unsigned long flags;
@@ -290,6 +339,13 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
 		goto out;
 	}
 
+	val = local_read(&drvdata->mode);
+	/* Don't interfere if operated from Perf */
+	if (val == CS_MODE_PERF) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	/* If drvdata::buf is NULL the trace data has been read already */
 	if (drvdata->buf == NULL) {
 		ret = -EINVAL;
@@ -297,7 +353,7 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
 	}
 
 	/* Disable the TMC if need be */
-	if (local_read(&drvdata->mode) == CS_MODE_SYSFS)
+	if (val == CS_MODE_SYSFS)
 		tmc_etb_disable_hw(drvdata);
 
 	drvdata->reading = true;
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 4b000f4003a2..a9a94a09186a 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -87,7 +87,7 @@ static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
 	CS_LOCK(drvdata->base);
 }
 
-static int tmc_enable_etr_sink(struct coresight_device *csdev, u32 mode)
+static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev, u32 mode)
 {
 	bool used = false;
 	long val;
@@ -167,6 +167,54 @@ spin_unlock:
 	return 0;
 }
 
+static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, u32 mode)
+{
+	int ret = 0;
+	long val;
+	unsigned long flags;
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	 /* This shouldn't be happening */
+	if (WARN_ON(mode != CS_MODE_PERF))
+		return -EINVAL;
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+	if (drvdata->reading) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	val = local_xchg(&drvdata->mode, mode);
+	/*
+	 * In Perf mode there can be only one writer per sink.  There
+	 * is also no need to continue if the ETR is already operated
+	 * from sysFS.
+	 */
+	if (val != CS_MODE_DISABLED) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	tmc_etr_enable_hw(drvdata);
+out:
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+	return ret;
+}
+
+static int tmc_enable_etr_sink(struct coresight_device *csdev, u32 mode)
+{
+	switch (mode) {
+	case CS_MODE_SYSFS:
+		return tmc_enable_etr_sink_sysfs(csdev, mode);
+	case CS_MODE_PERF:
+		return tmc_enable_etr_sink_perf(csdev, mode);
+	}
+
+	/* We shouldn't be here */
+	return -EINVAL;
+}
+
 static void tmc_disable_etr_sink(struct coresight_device *csdev)
 {
 	long val;
@@ -200,6 +248,8 @@ const struct coresight_ops tmc_etr_cs_ops = {
 
 int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
 {
+	int ret = 0;
+	long val;
 	unsigned long flags;
 
 	/* config types are set a boot time and never change */
@@ -208,20 +258,28 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
 
 	spin_lock_irqsave(&drvdata->spinlock, flags);
 
+	val = local_read(&drvdata->mode);
+	/* Don't interfere if operated from Perf */
+	if (val == CS_MODE_PERF) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	/* If drvdata::buf is NULL the trace data has been read already */
 	if (drvdata->buf == NULL) {
-		spin_unlock_irqrestore(&drvdata->spinlock, flags);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out;
 	}
 
 	/* Disable the TMC if need be */
-	if (local_read(&drvdata->mode) == CS_MODE_SYSFS)
+	if (val == CS_MODE_SYSFS)
 		tmc_etr_disable_hw(drvdata);
 
 	drvdata->reading = true;
+out:
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
-	return 0;
+	return ret;
 }
 
 int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
-- 
2.5.0

[PATCH V3 14/18] coresight: tmc: keep track of memory width

[Mathieu Poirier]

Accessing the HW configuration register each time the memory
width is needed simply doesn't make sense.  It is much more
efficient to read the value once and keep a reference for
later use.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/hwtracing/coresight/coresight-tmc-etf.c | 14 +---------
 drivers/hwtracing/coresight/coresight-tmc.c     | 34 +++++++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tmc.h     | 10 +++++---
 3 files changed, 41 insertions(+), 17 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index cc88c15ba45c..981c5ca75e36 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -41,25 +41,13 @@ void tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
 
 static void tmc_etb_dump_hw(struct tmc_drvdata *drvdata)
 {
-	enum tmc_mem_intf_width memwidth;
-	u8 memwords;
 	char *bufp;
 	u32 read_data;
 	int i;
 
-	memwidth = BMVAL(readl_relaxed(drvdata->base + CORESIGHT_DEVID), 8, 10);
-	if (memwidth == TMC_MEM_INTF_WIDTH_32BITS)
-		memwords = 1;
-	else if (memwidth == TMC_MEM_INTF_WIDTH_64BITS)
-		memwords = 2;
-	else if (memwidth == TMC_MEM_INTF_WIDTH_128BITS)
-		memwords = 4;
-	else
-		memwords = 8;
-
 	bufp = drvdata->buf;
 	while (1) {
-		for (i = 0; i < memwords; i++) {
+		for (i = 0; i < drvdata->memwidth; i++) {
 			read_data = readl_relaxed(drvdata->base + TMC_RRD);
 			if (read_data == 0xFFFFFFFF)
 				return;
diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
index 55806352b1f1..cb030a09659d 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.c
+++ b/drivers/hwtracing/coresight/coresight-tmc.c
@@ -193,6 +193,39 @@ static const struct file_operations tmc_fops = {
 	.llseek		= no_llseek,
 };
 
+static enum tmc_mem_intf_width tmc_get_memwidth(u32 devid)
+{
+	enum tmc_mem_intf_width memwidth;
+
+	/*
+	 * Excerpt from the TRM:
+	 *
+	 * DEVID::MEMWIDTH[10:8]
+	 * 0x2 Memory interface databus is 32 bits wide.
+	 * 0x3 Memory interface databus is 64 bits wide.
+	 * 0x4 Memory interface databus is 128 bits wide.
+	 * 0x5 Memory interface databus is 256 bits wide.
+	 */
+	switch (BMVAL(devid, 8, 10)) {
+	case 0x2:
+		memwidth = TMC_MEM_INTF_WIDTH_32BITS;
+		break;
+	case 0x3:
+		memwidth = TMC_MEM_INTF_WIDTH_64BITS;
+		break;
+	case 0x4:
+		memwidth = TMC_MEM_INTF_WIDTH_128BITS;
+		break;
+	case 0x5:
+		memwidth = TMC_MEM_INTF_WIDTH_256BITS;
+		break;
+	default:
+		memwidth = 0;
+	}
+
+	return memwidth;
+}
+
 #define coresight_tmc_simple_func(name, offset)			\
 	coresight_simple_func(struct tmc_drvdata, name, offset)
 
@@ -306,6 +339,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
 
 	devid = readl_relaxed(drvdata->base + CORESIGHT_DEVID);
 	drvdata->config_type = BMVAL(devid, 6, 7);
+	drvdata->memwidth = tmc_get_memwidth(devid);
 
 	if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
 		if (np)
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index 9b4c215d2b6b..12a097f3d06c 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -81,10 +81,10 @@ enum tmc_mode {
 };
 
 enum tmc_mem_intf_width {
-	TMC_MEM_INTF_WIDTH_32BITS	= 0x2,
-	TMC_MEM_INTF_WIDTH_64BITS	= 0x3,
-	TMC_MEM_INTF_WIDTH_128BITS	= 0x4,
-	TMC_MEM_INTF_WIDTH_256BITS	= 0x5,
+	TMC_MEM_INTF_WIDTH_32BITS	= 1,
+	TMC_MEM_INTF_WIDTH_64BITS	= 2,
+	TMC_MEM_INTF_WIDTH_128BITS	= 4,
+	TMC_MEM_INTF_WIDTH_256BITS	= 8,
 };
 
 /**
@@ -101,6 +101,7 @@ enum tmc_mem_intf_width {
  * @size:	@buf size.
  * @mode:	how this TMC is being used.
  * @config_type: TMC variant, must be of type @tmc_config_type.
+ * @memwidth:	width of the memory interface databus, in bytes.
  * @trigger_cntr: amount of words to store after a trigger.
  */
 struct tmc_drvdata {
@@ -117,6 +118,7 @@ struct tmc_drvdata {
 	u32			size;
 	local_t			mode;
 	enum tmc_config_type	config_type;
+	enum tmc_mem_intf_width	memwidth;
 	u32			trigger_cntr;
 };
 
-- 
2.5.0

[PATCH V3 15/18] coresight: moving struct cs_buffers to header file

[Mathieu Poirier]

That way we can re-use the structure in other drivers.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etb10.c | 20 --------------------
 drivers/hwtracing/coresight/coresight-priv.h  | 20 ++++++++++++++++++++
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
index a974c39171a9..239800aa5e24 100644
--- a/drivers/hwtracing/coresight/coresight-etb10.c
+++ b/drivers/hwtracing/coresight/coresight-etb10.c
@@ -71,26 +71,6 @@
 #define ETB_FRAME_SIZE_WORDS	4
 
 /**
- * struct cs_buffer - keep track of a recording session' specifics
- * @cur:	index of the current buffer
- * @nr_pages:	max number of pages granted to us
- * @offset:	offset within the current buffer
- * @data_size:	how much we collected in this run
- * @lost:	other than zero if we had a HW buffer wrap around
- * @snapshot:	is this run in snapshot mode
- * @data_pages:	a handle the ring buffer
- */
-struct cs_buffers {
-	unsigned int		cur;
-	unsigned int		nr_pages;
-	unsigned long		offset;
-	local_t			data_size;
-	local_t			lost;
-	bool			snapshot;
-	void			**data_pages;
-};
-
-/**
  * struct etb_drvdata - specifics associated to an ETB component
  * @base:	memory mapped base address for this component.
  * @dev:	the device entity associated to this component.
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index 3b5dd95a3588..ad975c58080d 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -53,6 +53,26 @@ enum cs_mode {
 	CS_MODE_PERF,
 };
 
+/**
+ * struct cs_buffer - keep track of a recording session' specifics
+ * @cur:	index of the current buffer
+ * @nr_pages:	max number of pages granted to us
+ * @offset:	offset within the current buffer
+ * @data_size:	how much we collected in this run
+ * @lost:	other than zero if we had a HW buffer wrap around
+ * @snapshot:	is this run in snapshot mode
+ * @data_pages:	a handle the ring buffer
+ */
+struct cs_buffers {
+	unsigned int		cur;
+	unsigned int		nr_pages;
+	unsigned long		offset;
+	local_t			data_size;
+	local_t			lost;
+	bool			snapshot;
+	void			**data_pages;
+};
+
 static inline void CS_LOCK(void __iomem *addr)
 {
 	do {
-- 
2.5.0

[PATCH V3 16/18] coresight: tmc: implementing TMC-ETF AUX space API

[Mathieu Poirier]

This patch implement the AUX area interfaces required to
use the TMC (configured as an ETF) from the Perf sub-system.

The heuristic is heavily borrowed from the ETB10 implementation.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/hwtracing/coresight/coresight-tmc-etf.c | 198 ++++++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tmc.h     |   1 +
 2 files changed, 199 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index 981c5ca75e36..4f38e8d185bc 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -15,7 +15,9 @@
  * this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <linux/circ_buf.h>
 #include <linux/coresight.h>
+#include <linux/perf_event.h>
 #include <linux/slab.h>
 #include "coresight-priv.h"
 #include "coresight-tmc.h"
@@ -287,9 +289,205 @@ static void tmc_disable_etf_link(struct coresight_device *csdev,
 	dev_info(drvdata->dev, "TMC disabled\n");
 }
 
+static void *tmc_alloc_etf_buffer(struct coresight_device *csdev, int cpu,
+				  void **pages, int nr_pages, bool overwrite)
+{
+	int node;
+	struct cs_buffers *buf;
+
+	if (cpu == -1)
+		cpu = smp_processor_id();
+	node = cpu_to_node(cpu);
+
+	/* Allocate memory structure for interaction with Perf */
+	buf = kzalloc_node(sizeof(struct cs_buffers), GFP_KERNEL, node);
+	if (!buf)
+		return NULL;
+
+	buf->snapshot = overwrite;
+	buf->nr_pages = nr_pages;
+	buf->data_pages = pages;
+
+	return buf;
+}
+
+static void tmc_free_etf_buffer(void *config)
+{
+	struct cs_buffers *buf = config;
+
+	kfree(buf);
+}
+
+static int tmc_set_etf_buffer(struct coresight_device *csdev,
+			      struct perf_output_handle *handle,
+			      void *sink_config)
+{
+	int ret = 0;
+	unsigned long head;
+	struct cs_buffers *buf = sink_config;
+
+	/* wrap head around to the amount of space we have */
+	head = handle->head & ((buf->nr_pages << PAGE_SHIFT) - 1);
+
+	/* find the page to write to */
+	buf->cur = head / PAGE_SIZE;
+
+	/* and offset within that page */
+	buf->offset = head % PAGE_SIZE;
+
+	local_set(&buf->data_size, 0);
+
+	return ret;
+}
+
+static unsigned long tmc_reset_etf_buffer(struct coresight_device *csdev,
+					  struct perf_output_handle *handle,
+					  void *sink_config, bool *lost)
+{
+	long size = 0;
+	struct cs_buffers *buf = sink_config;
+
+	if (buf) {
+		/*
+		 * In snapshot mode ->data_size holds the new address of the
+		 * ring buffer's head.  The size itself is the whole address
+		 * range since we want the latest information.
+		 */
+		if (buf->snapshot)
+			handle->head = local_xchg(&buf->data_size,
+						  buf->nr_pages << PAGE_SHIFT);
+		/*
+		 * Tell the tracer PMU how much we got in this run and if
+		 * something went wrong along the way.  Nobody else can use
+		 * this cs_buffers instance until we are done.  As such
+		 * resetting parameters here and squaring off with the ring
+		 * buffer API in the tracer PMU is fine.
+		 */
+		*lost = !!local_xchg(&buf->lost, 0);
+		size = local_xchg(&buf->data_size, 0);
+	}
+
+	return size;
+}
+
+static void tmc_update_etf_buffer(struct coresight_device *csdev,
+				  struct perf_output_handle *handle,
+				  void *sink_config)
+{
+	int i, cur;
+	u32 *buf_ptr;
+	u32 read_ptr, write_ptr;
+	u32 status, to_read;
+	unsigned long offset;
+	struct cs_buffers *buf = sink_config;
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	if (!buf)
+		return;
+
+	/* This shouldn't happen */
+	if (WARN_ON_ONCE(local_read(&drvdata->mode) != CS_MODE_PERF))
+		return;
+
+	CS_UNLOCK(drvdata->base);
+
+	tmc_flush_and_stop(drvdata);
+
+	read_ptr = readl_relaxed(drvdata->base + TMC_RRP);
+	write_ptr = readl_relaxed(drvdata->base + TMC_RWP);
+
+	/*
+	 * Get a hold of the status register and see if a wrap around
+	 * has occurred.  If so adjust things accordingly.
+	 */
+	status = readl_relaxed(drvdata->base + TMC_STS);
+	if (status & TMC_STS_FULL) {
+		local_inc(&buf->lost);
+		to_read = drvdata->size;
+	} else {
+		to_read = CIRC_CNT(write_ptr, read_ptr, drvdata->size);
+	}
+
+	/*
+	 * The TMC RAM buffer may be bigger than the space available in the
+	 * perf ring buffer (handle->size).  If so advance the RRP so that we
+	 * get the latest trace data.
+	 */
+	if (to_read > handle->size) {
+		u32 mask = 0;
+
+		/*
+		 * The value written to RRP must be byte-address aligned to
+		 * the width of the trace memory databus _and_ to a frame
+		 * boundary (16 byte), whichever is the biggest. For example,
+		 * for 32-bit, 64-bit and 128-bit wide trace memory, the four
+		 * LSBs must be 0s. For 256-bit wide trace memory, the five
+		 * LSBs must be 0s.
+		 */
+		switch (drvdata->memwidth) {
+		case TMC_MEM_INTF_WIDTH_32BITS:
+		case TMC_MEM_INTF_WIDTH_64BITS:
+		case TMC_MEM_INTF_WIDTH_128BITS:
+			mask = GENMASK(31, 5);
+			break;
+		case TMC_MEM_INTF_WIDTH_256BITS:
+			mask = GENMASK(31, 6);
+			break;
+		}
+
+		/*
+		 * Make sure the new size is aligned in accordance with the
+		 * requirement explained above.
+		 */
+		to_read = handle->size & mask;
+		/* Move the RAM read pointer up */
+		read_ptr = (write_ptr + drvdata->size) - to_read;
+		/* Make sure we are still within our limits */
+		read_ptr &= ~(drvdata->size - 1);
+		/* Tell the HW */
+		writel_relaxed(read_ptr, drvdata->base + TMC_RRP);
+		local_inc(&buf->lost);
+	}
+
+	cur = buf->cur;
+	offset = buf->offset;
+
+	/* for every byte to read */
+	for (i = 0; i < to_read; i += 4) {
+		buf_ptr = buf->data_pages[cur] + offset;
+		*buf_ptr = readl_relaxed(drvdata->base + TMC_RRD);
+
+		offset += 4;
+		if (offset >= PAGE_SIZE) {
+			offset = 0;
+			cur++;
+			/* wrap around at the end of the buffer */
+			cur &= buf->nr_pages - 1;
+		}
+	}
+
+	/*
+	 * In snapshot mode all we have to do is communicate to
+	 * perf_aux_output_end() the address of the current head.  In full
+	 * trace mode the same function expects a size to move rb->aux_head
+	 * forward.
+	 */
+	if (buf->snapshot)
+		local_set(&buf->data_size, (cur * PAGE_SIZE) + offset);
+	else
+		local_add(to_read, &buf->data_size);
+
+	CS_LOCK(drvdata->base);
+}
+
 static const struct coresight_ops_sink tmc_etf_sink_ops = {
 	.enable		= tmc_enable_etf_sink,
 	.disable	= tmc_disable_etf_sink,
+	.alloc_buffer	= tmc_alloc_etf_buffer,
+	.free_buffer	= tmc_free_etf_buffer,
+	.set_buffer	= tmc_set_etf_buffer,
+	.reset_buffer	= tmc_reset_etf_buffer,
+	.update_buffer	= tmc_update_etf_buffer,
 };
 
 static const struct coresight_ops_link tmc_etf_link_ops = {
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index 12a097f3d06c..bb88e1a84d00 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -52,6 +52,7 @@
 #define TMC_CTL_CAPT_EN		BIT(0)
 /* TMC_STS - 0x00C */
 #define TMC_STS_TMCREADY_BIT	2
+#define TMC_STS_FULL		BIT(0)
 #define TMC_STS_TRIGGERED	BIT(1)
 /* TMC_AXICTL - 0x110 */
 #define TMC_AXICTL_PROT_CTL_B0	BIT(0)
-- 
2.5.0

[PATCH V3 17/18] coresight: tmc: implementing TMC-ETR AUX space API

[Mathieu Poirier]

This patch implement the AUX area interfaces required to
use the TMC (configured as an ETR) from the Perf sub-system.

The ETR is configured to work with contiguous memory only.
Although not optimal, it allows the IP block to be used
while the scatter-gather mode of operation is being worked
on.

The heuristic is heavily borrowed from the ETB10 and TMC-ETF
implementation.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/hwtracing/coresight/coresight-tmc-etf.c |   6 +-
 drivers/hwtracing/coresight/coresight-tmc-etr.c | 142 ++++++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tmc.h     |   3 +
 3 files changed, 148 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index 4f38e8d185bc..78d08d76e945 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -370,9 +370,9 @@ static unsigned long tmc_reset_etf_buffer(struct coresight_device *csdev,
 	return size;
 }
 
-static void tmc_update_etf_buffer(struct coresight_device *csdev,
-				  struct perf_output_handle *handle,
-				  void *sink_config)
+void tmc_update_etf_buffer(struct coresight_device *csdev,
+			   struct perf_output_handle *handle,
+			   void *sink_config)
 {
 	int i, cur;
 	u32 *buf_ptr;
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index a9a94a09186a..ffce04b92d88 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -15,11 +15,30 @@
  * this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <linux/circ_buf.h>
 #include <linux/coresight.h>
 #include <linux/dma-mapping.h>
+#include <linux/slab.h>
+
 #include "coresight-priv.h"
 #include "coresight-tmc.h"
 
+/**
+ * struct cs_etr_buffer - keep track of a recording session' specifics
+ * @tmc:	generic portion of the TMC buffers
+ * @paddr:	the physical address of a DMA'able contiguous memory area
+ * @vaddr:	the virtual address associated to @paddr
+ * @size:	how much memory we have, starting at @paddr
+ * @dev:	the device @vaddr has been tied to
+ */
+struct cs_etr_buffers {
+	struct cs_buffers	tmc;
+	dma_addr_t		paddr;
+	void __iomem		*vaddr;
+	u32			size;
+	struct device		*dev;
+};
+
 void tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
 {
 	u32 axictl;
@@ -237,9 +256,132 @@ static void tmc_disable_etr_sink(struct coresight_device *csdev)
 	dev_info(drvdata->dev, "TMC-ETR disabled\n");
 }
 
+static void *tmc_alloc_etr_buffer(struct coresight_device *csdev, int cpu,
+				  void **pages, int nr_pages, bool overwrite)
+{
+	int node;
+	struct cs_etr_buffers *buf;
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	if (cpu == -1)
+		cpu = smp_processor_id();
+	node = cpu_to_node(cpu);
+
+	/* Allocate memory structure for interaction with Perf */
+	buf = kzalloc_node(sizeof(struct cs_etr_buffers), GFP_KERNEL, node);
+	if (!buf)
+		return NULL;
+
+	buf->dev = drvdata->dev;
+	buf->size = drvdata->size;
+	buf->vaddr = dma_alloc_coherent(buf->dev, buf->size,
+					&buf->paddr, GFP_KERNEL);
+	if (!buf->vaddr) {
+		kfree(buf);
+		return NULL;
+	}
+
+	buf->tmc.snapshot = overwrite;
+	buf->tmc.nr_pages = nr_pages;
+	buf->tmc.data_pages = pages;
+
+	return buf;
+}
+
+static void tmc_free_etr_buffer(void *config)
+{
+	struct cs_etr_buffers *buf = config;
+
+	dma_free_coherent(buf->dev, buf->size, buf->vaddr, buf->paddr);
+	kfree(buf);
+}
+
+static int tmc_set_etr_buffer(struct coresight_device *csdev,
+			      struct perf_output_handle *handle,
+			      void *sink_config)
+{
+	int ret = 0;
+	unsigned long head;
+	struct cs_etr_buffers *buf = sink_config;
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	/* wrap head around to the amount of space we have */
+	head = handle->head & ((buf->tmc.nr_pages << PAGE_SHIFT) - 1);
+
+	/* find the page to write to */
+	buf->tmc.cur = head / PAGE_SIZE;
+
+	/* and offset within that page */
+	buf->tmc.offset = head % PAGE_SIZE;
+
+	local_set(&buf->tmc.data_size, 0);
+
+	/* Tell the HW where to put the trace data */
+	drvdata->vaddr = buf->vaddr;
+	drvdata->paddr = buf->paddr;
+	memset(drvdata->vaddr, 0, drvdata->size);
+
+	return ret;
+}
+
+static unsigned long tmc_reset_etr_buffer(struct coresight_device *csdev,
+					  struct perf_output_handle *handle,
+					  void *sink_config, bool *lost)
+{
+	long size = 0;
+	struct cs_etr_buffers *buf = sink_config;
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	if (buf) {
+		/*
+		 * In snapshot mode ->data_size holds the new address of the
+		 * ring buffer's head.  The size itself is the whole address
+		 * range since we want the latest information.
+		 */
+		if (buf->tmc.snapshot) {
+			size = buf->tmc.nr_pages << PAGE_SHIFT;
+			handle->head = local_xchg(&buf->tmc.data_size, size);
+		}
+
+		/*
+		 * Tell the tracer PMU how much we got in this run and if
+		 * something went wrong along the way.  Nobody else can use
+		 * this cs_etr_buffers instance until we are done.  As such
+		 * resetting parameters here and squaring off with the ring
+		 * buffer API in the tracer PMU is fine.
+		 */
+		*lost = !!local_xchg(&buf->tmc.lost, 0);
+		size = local_xchg(&buf->tmc.data_size, 0);
+	}
+
+	/* Get ready for another run */
+	drvdata->vaddr = NULL;
+	drvdata->paddr = 0;
+
+	return size;
+}
+
+static void tmc_update_etr_buffer(struct coresight_device *csdev,
+				  struct perf_output_handle *handle,
+				  void *sink_config)
+{
+	struct cs_etr_buffers *buf = sink_config;
+
+	/*
+	 * An ETR configured to work in contiguous memory mode works the same
+	 * was as an ETB or ETF.
+	 */
+	tmc_update_etf_buffer(csdev, handle, &buf->tmc);
+}
+
 static const struct coresight_ops_sink tmc_etr_sink_ops = {
 	.enable		= tmc_enable_etr_sink,
 	.disable	= tmc_disable_etr_sink,
+	.alloc_buffer	= tmc_alloc_etr_buffer,
+	.free_buffer	= tmc_free_etr_buffer,
+	.set_buffer	= tmc_set_etr_buffer,
+	.reset_buffer	= tmc_reset_etr_buffer,
+	.update_buffer	= tmc_update_etr_buffer,
 };
 
 const struct coresight_ops tmc_etr_cs_ops = {
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index bb88e1a84d00..cb0e55105192 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -132,6 +132,9 @@ void tmc_disable_hw(struct tmc_drvdata *drvdata);
 /* ETB/ETF functions */
 int tmc_read_prepare_etb(struct tmc_drvdata *drvdata);
 int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata);
+void tmc_update_etf_buffer(struct coresight_device *csdev,
+			   struct perf_output_handle *handle,
+			   void *sink_config);
 extern const struct coresight_ops tmc_etb_cs_ops;
 extern const struct coresight_ops tmc_etf_cs_ops;
 
-- 
2.5.0

[PATCH V3 18/18] coresight: configuring ETF in FIFO mode when acting as link

[Mathieu Poirier]

When part of a path but not identified as a sink, the EFT has to
be configured as a link and placed in HW FIFO mode.  As such when
enabling a path, call the right configuration function based on
the role the ETF if playing in this trace run.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 2ea5961092c1..bba9f3d653c9 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -257,15 +257,27 @@ static void coresight_disable_source(struct coresight_device *csdev)
 
 void coresight_disable_path(struct list_head *path)
 {
+	u32 type;
 	struct coresight_node *nd;
 	struct coresight_device *csdev, *parent, *child;
 
 	list_for_each_entry(nd, path, link) {
 		csdev = nd->csdev;
+		type = csdev->type;
 
-		switch (csdev->type) {
+		/*
+		 * ETF devices are tricky... They can be a link or a sink,
+		 * depending on how they are configured.  If an ETF has been
+		 * "activated" it will be configured as a sink, otherwise
+		 * go ahead with the link configuration.
+		 */
+		if (type == CORESIGHT_DEV_TYPE_LINKSINK)
+			type = (csdev == coresight_get_sink(path)) ?
+						CORESIGHT_DEV_TYPE_SINK :
+						CORESIGHT_DEV_TYPE_LINK;
+
+		switch (type) {
 		case CORESIGHT_DEV_TYPE_SINK:
-		case CORESIGHT_DEV_TYPE_LINKSINK:
 			coresight_disable_sink(csdev);
 			break;
 		case CORESIGHT_DEV_TYPE_SOURCE:
@@ -286,15 +298,27 @@ int coresight_enable_path(struct list_head *path, u32 mode)
 {
 
 	int ret = 0;
+	u32 type;
 	struct coresight_node *nd;
 	struct coresight_device *csdev, *parent, *child;
 
 	list_for_each_entry_reverse(nd, path, link) {
 		csdev = nd->csdev;
+		type = csdev->type;
+
+		/*
+		 * ETF devices are tricky... They can be a link or a sink,
+		 * depending on how they are configured.  If an ETF has been
+		 * "activated" it will be configured as a sink, otherwise
+		 * go ahead with the link configuration.
+		 */
+		if (type == CORESIGHT_DEV_TYPE_LINKSINK)
+			type = (csdev == coresight_get_sink(path)) ?
+						CORESIGHT_DEV_TYPE_SINK :
+						CORESIGHT_DEV_TYPE_LINK;
 
-		switch (csdev->type) {
+		switch (type) {
 		case CORESIGHT_DEV_TYPE_SINK:
-		case CORESIGHT_DEV_TYPE_LINKSINK:
 			ret = coresight_enable_sink(csdev, mode);
 			if (ret)
 				goto err;
-- 
2.5.0

Re: [PATCH V3 09/18] coresight: tmc: allocating memory when needed

[Suzuki K Poulose]

On 22/04/16 18:14, Mathieu Poirier wrote:
>   static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
>   {
> +	bool used = false;
> +	char *buf = NULL;
>   	unsigned long flags;
>   	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> +	 /* This shouldn't be happening */
> +	if (WARN_ON(mode != CS_MODE_SYSFS))
> +		return -EINVAL;
> +
> +	/*
> +	 * If a buffer is already allocated *keep holding* the lock and
> +	 * jump to the fast path.  Otherwise release the lock and allocate
> +	 * memory to work with.
> +	 */
>   	spin_lock_irqsave(&drvdata->spinlock, flags);
> +	if (drvdata->buf)
> +		goto fast_path;
> +
> +	spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +
> +	/* Allocating the memory here while outside of the spinlock */
> +	buf = kzalloc(drvdata->size, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	/* Let's try again */
> +	spin_lock_irqsave(&drvdata->spinlock, flags);
> +fast_path:

nit: As I mentioned in the previous series, could we not avoid the goto in the middle of the
function,by doing :

	if (!drvdata->buf) {
		unlock();
		buf = alloc();
		if (!buf) return -ENOMEM;
		lock();
	}


>   	if (drvdata->reading) {
>   		spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +		/*
> +		 * Free allocated memory outside of the spinlock.  There is
> +		 * no need to assert the validity of 'buf' since calling
> +		 * kfree(NULL) is safe.
> +		 */
> +		kfree(buf);
>   		return -EBUSY;
>   	}

It would be good to unify the exit points as we do similar steps.
	if (drvdata->reading) {
		rc = -EBUSY;
		goto out;
	}
>
> +	/*
> +	 * If drvdata::buf isn't NULL, memory was allocated for a previous
> +	 * trace run but wasn't read.  If so simply zero-out the memory.
> +	 * Otherwise use the memory allocated above.
> +	 *
> +	 * The memory is freed when users read the buffer using the
> +	 * /dev/xyz.{etf|etb} interface.  See tmc_read_unprepare_etf() for
> +	 * details.
> +	 */
> +	if (drvdata->buf) {
> +		memset(drvdata->buf, 0, drvdata->size);
> +	} else {
> +		used = true;
> +		drvdata->buf = buf;
> +	}
> +
>   	tmc_etb_enable_hw(drvdata);
>   	drvdata->enable = true;

out:

>   	spin_unlock_irqrestore(&drvdata->spinlock, flags);
>
> +	/* Free memory outside the spinlock if need be */
> +	if (!used && buf)
> +		kfree(buf);
> +
	if (!rc)
	   	dev_info(drvdata->dev, "TMC-ETB/ETF enabled\n");
    	return rc


> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 3483d139a4ac..474c70c089f3 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c

>   static int tmc_enable_etr_sink(struct coresight_device *csdev, u32 mode)
>   {
> +	bool used = false;
>   	unsigned long flags;
> +	void __iomem *vaddr = NULL;
> +	dma_addr_t paddr;
>   	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> +	 /* This shouldn't be happening */
> +	if (WARN_ON(mode != CS_MODE_SYSFS))
> +		return -EINVAL;
> +
> +	/*
> +	 * If a buffer is already allocated *keep holding* the lock and
> +	 * jump to the fast path.  Otherwise release the lock and allocate
> +	 * memory to work with.
> +	 */
> +	spin_lock_irqsave(&drvdata->spinlock, flags);
> +	if (drvdata->vaddr)
> +		goto fast_path;
> +
> +	spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +
> +	/*
> +	 * Contiguous  memory can't be allocated while a spinlock is held.
> +	 * As such allocate memory here and free it if a buffer has already
> +	 * been allocated (from a previous session).
> +	 */
> +	vaddr = dma_alloc_coherent(drvdata->dev, drvdata->size,
> +				   &paddr, GFP_KERNEL);
> +	if (!vaddr)
> +		return -ENOMEM;
> +
> +	/* Let's try again */
>   	spin_lock_irqsave(&drvdata->spinlock, flags);
> +fast_path:

nit: Same as above.

I am not too particular about the above changes, but would be good to have them
reader friendly.

Either way,

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Suzuki

Re: [PATCH V3 10/18] coresight: tmc: getting the right read_count on tmc_open()

[Suzuki K Poulose]

On 22/04/16 18:14, Mathieu Poirier wrote:
> In function tmc_open(), if tmc_read_prepare() fails variable
> drvdata->read_count is not decremented, causing unwanted
> access to drvdata->buf and very likely, a crash dump.
>
> By moving the incrementation to a place where we know things
> are stable this kind of situation is avoided.
>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Reviewed-by: Suzuki K Poulose <Suzuki.Poulose@arm.com>
> ---
>   drivers/hwtracing/coresight/coresight-tmc.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
> index e8e12a9b917a..55806352b1f1 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc.c
> @@ -121,13 +121,14 @@ static int tmc_open(struct inode *inode, struct file *file)
>   						   struct tmc_drvdata, miscdev);
>   	int ret = 0;
>

On a second thought, I think there could be a race here.


> -	if (drvdata->read_count++)
> +	if (drvdata->read_count)
>   		goto out;
>
>   	ret = tmc_read_prepare(drvdata);
>   	if (ret)
>   		return ret;
>   out:

What prevents someone else doing a release() on the file when we get here, without
incrementing the read_count ? Also, read_count accesses are not protected. Either should
be covered by the drvdata->spinlock or convert it to atomic.



> +	drvdata->read_count++;
>   	nonseekable_open(inode, file);


Cheers
Suzuki

Re: [PATCH V3 12/18] coresight: tmc: dump system memory content only when needed

[Suzuki K Poulose]

On 22/04/16 18:14, Mathieu Poirier wrote:
> Calling tmc_etf/etr_dump_hw() is required only when operating from
> sysFS.  When working from Perf, the system memory is harvested
> from the AUX trace API.
>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>   drivers/hwtracing/coresight/coresight-tmc-etf.c | 7 ++++++-
>   drivers/hwtracing/coresight/coresight-tmc-etr.c | 7 ++++++-
>   2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> index d090a9745c73..25fad93b68d4 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> @@ -74,7 +74,12 @@ static void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
>   	CS_UNLOCK(drvdata->base);
>
>   	tmc_flush_and_stop(drvdata);
> -	tmc_etb_dump_hw(drvdata);
> +	/*
> +	 * When operating in sysFS mode the content of the buffer needs to be
> +	 * read before the TMC is disabled.
> +	 */
> +	if (local_read(&drvdata->mode) == CS_MODE_SYSFS)
> +		tmc_etb_dump_hw(drvdata);
>   	tmc_disable_hw(drvdata);
>
>   	CS_LOCK(drvdata->base);
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 8bbbf3ab1387..4b000f4003a2 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -76,7 +76,12 @@ static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
>   	CS_UNLOCK(drvdata->base);
>
>   	tmc_flush_and_stop(drvdata);
> -	tmc_etr_dump_hw(drvdata);
> +	/*
> +	 * When operating in sysFS mode the content of the buffer needs to be
> +	 * read before the TMC is disabled.
> +	 */
> +	if (local_read(&drvdata->mode) == CS_MODE_SYSFS)
> +		tmc_etr_dump_hw(drvdata);
>   	tmc_disable_hw(drvdata);

Nit: It would be cleaner if tmc_etX_disable_hw accepted a bool parameter to decide
whether to dump the hw data or not. That way, the callers can make the decision,
leaving the disable_hw code unaware of the mode checks.

Suzuki

Re: [PATCH V3 09/18] coresight: tmc: allocating memory when needed

[Mathieu Poirier]

On 25 April 2016 at 04:20, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
> On 22/04/16 18:14, Mathieu Poirier wrote:
>>
>>   static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
>>   {
>> +       bool used = false;
>> +       char *buf = NULL;
>>         unsigned long flags;
>>         struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>>
>> +        /* This shouldn't be happening */
>> +       if (WARN_ON(mode != CS_MODE_SYSFS))
>> +               return -EINVAL;
>> +
>> +       /*
>> +        * If a buffer is already allocated *keep holding* the lock and
>> +        * jump to the fast path.  Otherwise release the lock and allocate
>> +        * memory to work with.
>> +        */
>>         spin_lock_irqsave(&drvdata->spinlock, flags);
>> +       if (drvdata->buf)
>> +               goto fast_path;
>> +
>> +       spin_unlock_irqrestore(&drvdata->spinlock, flags);
>> +
>> +       /* Allocating the memory here while outside of the spinlock */
>> +       buf = kzalloc(drvdata->size, GFP_KERNEL);
>> +       if (!buf)
>> +               return -ENOMEM;
>> +
>> +       /* Let's try again */
>> +       spin_lock_irqsave(&drvdata->spinlock, flags);
>> +fast_path:
>
>
> nit: As I mentioned in the previous series, could we not avoid the goto in
> the middle of the
> function,by doing :
>
>         if (!drvdata->buf) {
>                 unlock();
>                 buf = alloc();
>                 if (!buf) return -ENOMEM;
>                 lock();
>         }
>

If we do not move the condition below (as per your comment in the
previous set), this will indeed make the code easier to
read/understand.

I will make the change.

>
>>         if (drvdata->reading) {
>>                 spin_unlock_irqrestore(&drvdata->spinlock, flags);
>> +               /*
>> +                * Free allocated memory outside of the spinlock.  There
>> is
>> +                * no need to assert the validity of 'buf' since calling
>> +                * kfree(NULL) is safe.
>> +                */
>> +               kfree(buf);
>>                 return -EBUSY;
>>         }
>
>
> It would be good to unify the exit points as we do similar steps.
>         if (drvdata->reading) {
>                 rc = -EBUSY;
>                 goto out;
>         }
>>
>>
>> +       /*
>> +        * If drvdata::buf isn't NULL, memory was allocated for a previous
>> +        * trace run but wasn't read.  If so simply zero-out the memory.
>> +        * Otherwise use the memory allocated above.
>> +        *
>> +        * The memory is freed when users read the buffer using the
>> +        * /dev/xyz.{etf|etb} interface.  See tmc_read_unprepare_etf() for
>> +        * details.
>> +        */
>> +       if (drvdata->buf) {
>> +               memset(drvdata->buf, 0, drvdata->size);
>> +       } else {
>> +               used = true;
>> +               drvdata->buf = buf;
>> +       }
>> +
>>         tmc_etb_enable_hw(drvdata);
>>         drvdata->enable = true;
>
>
> out:
>
>>         spin_unlock_irqrestore(&drvdata->spinlock, flags);
>>
>> +       /* Free memory outside the spinlock if need be */
>> +       if (!used && buf)
>> +               kfree(buf);
>> +
>
>         if (!rc)
>                 dev_info(drvdata->dev, "TMC-ETB/ETF enabled\n");
>         return rc
>
>
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> index 3483d139a4ac..474c70c089f3 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>
>
>>   static int tmc_enable_etr_sink(struct coresight_device *csdev, u32 mode)
>>   {
>> +       bool used = false;
>>         unsigned long flags;
>> +       void __iomem *vaddr = NULL;
>> +       dma_addr_t paddr;
>>         struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>>
>> +        /* This shouldn't be happening */
>> +       if (WARN_ON(mode != CS_MODE_SYSFS))
>> +               return -EINVAL;
>> +
>> +       /*
>> +        * If a buffer is already allocated *keep holding* the lock and
>> +        * jump to the fast path.  Otherwise release the lock and allocate
>> +        * memory to work with.
>> +        */
>> +       spin_lock_irqsave(&drvdata->spinlock, flags);
>> +       if (drvdata->vaddr)
>> +               goto fast_path;
>> +
>> +       spin_unlock_irqrestore(&drvdata->spinlock, flags);
>> +
>> +       /*
>> +        * Contiguous  memory can't be allocated while a spinlock is held.
>> +        * As such allocate memory here and free it if a buffer has
>> already
>> +        * been allocated (from a previous session).
>> +        */
>> +       vaddr = dma_alloc_coherent(drvdata->dev, drvdata->size,
>> +                                  &paddr, GFP_KERNEL);
>> +       if (!vaddr)
>> +               return -ENOMEM;
>> +
>> +       /* Let's try again */
>>         spin_lock_irqsave(&drvdata->spinlock, flags);
>> +fast_path:
>
>
> nit: Same as above.
>
> I am not too particular about the above changes, but would be good to have
> them
> reader friendly.
>
> Either way,
>
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>
> Suzuki

Re: [PATCH V3 10/18] coresight: tmc: getting the right read_count on tmc_open()

[Mathieu Poirier]

On 25 April 2016 at 04:47, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
> On 22/04/16 18:14, Mathieu Poirier wrote:
>>
>> In function tmc_open(), if tmc_read_prepare() fails variable
>> drvdata->read_count is not decremented, causing unwanted
>> access to drvdata->buf and very likely, a crash dump.
>>
>> By moving the incrementation to a place where we know things
>> are stable this kind of situation is avoided.
>>
>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Reviewed-by: Suzuki K Poulose <Suzuki.Poulose@arm.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-tmc.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc.c
>> b/drivers/hwtracing/coresight/coresight-tmc.c
>> index e8e12a9b917a..55806352b1f1 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc.c
>> @@ -121,13 +121,14 @@ static int tmc_open(struct inode *inode, struct file
>> *file)
>>                                                    struct tmc_drvdata,
>> miscdev);
>>         int ret = 0;
>>
>
> On a second thought, I think there could be a race here.
>
>
>> -       if (drvdata->read_count++)
>> +       if (drvdata->read_count)
>>                 goto out;
>>
>>         ret = tmc_read_prepare(drvdata);
>>         if (ret)
>>                 return ret;
>>   out:
>
>
> What prevents someone else doing a release() on the file when we get here,
> without
> incrementing the read_count ? Also, read_count accesses are not protected.
> Either should
> be covered by the drvdata->spinlock or convert it to atomic.

I agree - I'll move it to an atomic type.

Thanks,
Mathieu

>
>
>
>> +       drvdata->read_count++;
>>         nonseekable_open(inode, file);
>
>
>
> Cheers
> Suzuki

Re: [PATCH V3 13/18] coresight: tmc: make sysFS and Perf mode mutually exclusive

[Suzuki K Poulose]

On 22/04/16 18:14, Mathieu Poirier wrote:
> The sysFS and Perf access methods can't be allowed to interfere
> with one another.  As such introducing guards to access
> functions that prevents moving forward if a TMC is already
> being used.
>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>   drivers/hwtracing/coresight/coresight-tmc-etf.c | 60 +++++++++++++++++++++-
>   drivers/hwtracing/coresight/coresight-tmc-etr.c | 68 +++++++++++++++++++++++--
>   2 files changed, 121 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> index 25fad93b68d4..cc88c15ba45c 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> @@ -111,7 +111,7 @@ static void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
>   	CS_LOCK(drvdata->base);
>   }
>
> -static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
> +static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev, u32 mode)
>   {
>   	bool used = false;
>   	char *buf = NULL;
> @@ -190,6 +190,54 @@ spin_unlock:
>   	return 0;
>   }
>
> +static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, u32 mode)
> +{
> +	int ret = 0;
> +	long val;
> +	unsigned long flags;
> +	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> +	 /* This shouldn't be happening */
> +	if (WARN_ON(mode != CS_MODE_PERF))
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&drvdata->spinlock, flags);
> +	if (drvdata->reading) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	val = local_xchg(&drvdata->mode, mode);
> +	/*
> +	 * In Perf mode there can be only one writer per sink.  There
> +	 * is also no need to continue if the ETB/ETR is already operated
> +	 * from sysFS.
> +	 */
> +	if (val != CS_MODE_DISABLED) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	tmc_etb_enable_hw(drvdata);
> +out:
> +	spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +
> +	return ret;
> +}
> +
> +static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
> +{
> +	switch (mode) {
> +	case CS_MODE_SYSFS:
> +		return tmc_enable_etf_sink_sysfs(csdev, mode);
> +	case CS_MODE_PERF:
> +		return tmc_enable_etf_sink_perf(csdev, mode);
> +	}
> +
> +	/* We shouldn't be here */
> +	return -EINVAL;
> +}
> +
>   static void tmc_disable_etf_sink(struct coresight_device *csdev)
>   {
>   	long val;
> @@ -272,6 +320,7 @@ const struct coresight_ops tmc_etf_cs_ops = {
>
>   int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
>   {
> +	long val;
>   	enum tmc_mode mode;
>   	int ret = 0;
>   	unsigned long flags;
> @@ -290,6 +339,13 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
>   		goto out;
>   	}
>
> +	val = local_read(&drvdata->mode);
> +	/* Don't interfere if operated from Perf */
> +	if (val == CS_MODE_PERF) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
>   	/* If drvdata::buf is NULL the trace data has been read already */
>   	if (drvdata->buf == NULL) {
>   		ret = -EINVAL;
> @@ -297,7 +353,7 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
>   	}
>
>   	/* Disable the TMC if need be */
> -	if (local_read(&drvdata->mode) == CS_MODE_SYSFS)
> +	if (val == CS_MODE_SYSFS)
>   		tmc_etb_disable_hw(drvdata);
>
>   	drvdata->reading = true;
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 4b000f4003a2..a9a94a09186a 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -87,7 +87,7 @@ static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
>   	CS_LOCK(drvdata->base);
>   }
>
> -static int tmc_enable_etr_sink(struct coresight_device *csdev, u32 mode)
> +static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev, u32 mode)
>   {
>   	bool used = false;
>   	long val;
> @@ -167,6 +167,54 @@ spin_unlock:
>   	return 0;
>   }
>
> +static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, u32 mode)
> +{
> +	int ret = 0;
> +	long val;
> +	unsigned long flags;
> +	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> +	 /* This shouldn't be happening */
> +	if (WARN_ON(mode != CS_MODE_PERF))
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&drvdata->spinlock, flags);
> +	if (drvdata->reading) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	val = local_xchg(&drvdata->mode, mode);
> +	/*
> +	 * In Perf mode there can be only one writer per sink.  There
> +	 * is also no need to continue if the ETR is already operated
> +	 * from sysFS.
> +	 */
> +	if (val != CS_MODE_DISABLED) {

Could val be CS_MODE_PERF ? In other words, should we be checking :
	if (val == CS_MODE_SYSFS)  instead ?

Suzuki

Re: [PATCH V3 12/18] coresight: tmc: dump system memory content only when needed

[Mathieu Poirier]

On 25 April 2016 at 05:16, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
> On 22/04/16 18:14, Mathieu Poirier wrote:
>>
>> Calling tmc_etf/etr_dump_hw() is required only when operating from
>> sysFS.  When working from Perf, the system memory is harvested
>> from the AUX trace API.
>>
>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> ---
>>   drivers/hwtracing/coresight/coresight-tmc-etf.c | 7 ++++++-
>>   drivers/hwtracing/coresight/coresight-tmc-etr.c | 7 ++++++-
>>   2 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c
>> b/drivers/hwtracing/coresight/coresight-tmc-etf.c
>> index d090a9745c73..25fad93b68d4 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
>> @@ -74,7 +74,12 @@ static void tmc_etb_disable_hw(struct tmc_drvdata
>> *drvdata)
>>         CS_UNLOCK(drvdata->base);
>>
>>         tmc_flush_and_stop(drvdata);
>> -       tmc_etb_dump_hw(drvdata);
>> +       /*
>> +        * When operating in sysFS mode the content of the buffer needs to
>> be
>> +        * read before the TMC is disabled.
>> +        */
>> +       if (local_read(&drvdata->mode) == CS_MODE_SYSFS)
>> +               tmc_etb_dump_hw(drvdata);
>>         tmc_disable_hw(drvdata);
>>
>>         CS_LOCK(drvdata->base);
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> index 8bbbf3ab1387..4b000f4003a2 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> @@ -76,7 +76,12 @@ static void tmc_etr_disable_hw(struct tmc_drvdata
>> *drvdata)
>>         CS_UNLOCK(drvdata->base);
>>
>>         tmc_flush_and_stop(drvdata);
>> -       tmc_etr_dump_hw(drvdata);
>> +       /*
>> +        * When operating in sysFS mode the content of the buffer needs to
>> be
>> +        * read before the TMC is disabled.
>> +        */
>> +       if (local_read(&drvdata->mode) == CS_MODE_SYSFS)
>> +               tmc_etr_dump_hw(drvdata);
>>         tmc_disable_hw(drvdata);
>
>
> Nit: It would be cleaner if tmc_etX_disable_hw accepted a bool parameter to
> decide
> whether to dump the hw data or not. That way, the callers can make the
> decision,
> leaving the disable_hw code unaware of the mode checks.

My goal in pushing the functionality to tmc_etX_disable_hw() was to
especially keep the the higher layers unaware of sink buffer
management specifics.  There is two ways to look at this and we simply
landed on different sides of the fence.  I'm not strongly opinionated
on this, I'll modify if you're really keen on this.

Thanks,
Mathieu

>
> Suzuki

Re: [PATCH V3 14/18] coresight: tmc: keep track of memory width

[Suzuki K Poulose]

On 22/04/16 18:14, Mathieu Poirier wrote:
> Accessing the HW configuration register each time the memory
> width is needed simply doesn't make sense.  It is much more
> efficient to read the value once and keep a reference for
> later use.
>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>   drivers/hwtracing/coresight/coresight-tmc-etf.c | 14 +---------
>   drivers/hwtracing/coresight/coresight-tmc.c     | 34 +++++++++++++++++++++++++
>   drivers/hwtracing/coresight/coresight-tmc.h     | 10 +++++---
>   3 files changed, 41 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> index cc88c15ba45c..981c5ca75e36 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> @@ -41,25 +41,13 @@ void tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
>
>   static void tmc_etb_dump_hw(struct tmc_drvdata *drvdata)
>   {
> -	enum tmc_mem_intf_width memwidth;
> -	u8 memwords;
>   	char *bufp;
>   	u32 read_data;
>   	int i;
>
> -	memwidth = BMVAL(readl_relaxed(drvdata->base + CORESIGHT_DEVID), 8, 10);
> -	if (memwidth == TMC_MEM_INTF_WIDTH_32BITS)
> -		memwords = 1;
> -	else if (memwidth == TMC_MEM_INTF_WIDTH_64BITS)
> -		memwords = 2;
> -	else if (memwidth == TMC_MEM_INTF_WIDTH_128BITS)
> -		memwords = 4;
> -	else
> -		memwords = 8;
> -
>   	bufp = drvdata->buf;
>   	while (1) {
> -		for (i = 0; i < memwords; i++) {
> +		for (i = 0; i < drvdata->memwidth; i++) {
>   			read_data = readl_relaxed(drvdata->base + TMC_RRD);
>   			if (read_data == 0xFFFFFFFF)
>   				return;
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c
> index 55806352b1f1..cb030a09659d 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc.c
> @@ -193,6 +193,39 @@ static const struct file_operations tmc_fops = {
>   	.llseek		= no_llseek,
>   };
>
> +static enum tmc_mem_intf_width tmc_get_memwidth(u32 devid)
> +{
> +	enum tmc_mem_intf_width memwidth;
> +
> +	/*
> +	 * Excerpt from the TRM:
> +	 *
> +	 * DEVID::MEMWIDTH[10:8]
> +	 * 0x2 Memory interface databus is 32 bits wide.
> +	 * 0x3 Memory interface databus is 64 bits wide.
> +	 * 0x4 Memory interface databus is 128 bits wide.
> +	 * 0x5 Memory interface databus is 256 bits wide.
> +	 */
> +	switch (BMVAL(devid, 8, 10)) {
> +	case 0x2:
> +		memwidth = TMC_MEM_INTF_WIDTH_32BITS;
> +		break;
> +	case 0x3:
> +		memwidth = TMC_MEM_INTF_WIDTH_64BITS;
> +		break;
> +	case 0x4:
> +		memwidth = TMC_MEM_INTF_WIDTH_128BITS;
> +		break;
> +	case 0x5:
> +		memwidth = TMC_MEM_INTF_WIDTH_256BITS;
> +		break;
> +	default:
> +		memwidth = 0;
> +	}
> +
> +	return memwidth;
> +}
> +
>   #define coresight_tmc_simple_func(name, offset)			\
>   	coresight_simple_func(struct tmc_drvdata, name, offset)
>
> @@ -306,6 +339,7 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
>
>   	devid = readl_relaxed(drvdata->base + CORESIGHT_DEVID);
>   	drvdata->config_type = BMVAL(devid, 6, 7);
> +	drvdata->memwidth = tmc_get_memwidth(devid);
>
>   	if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
>   		if (np)
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> index 9b4c215d2b6b..12a097f3d06c 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.h
> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> @@ -81,10 +81,10 @@ enum tmc_mode {
>   };
>
>   enum tmc_mem_intf_width {
> -	TMC_MEM_INTF_WIDTH_32BITS	= 0x2,
> -	TMC_MEM_INTF_WIDTH_64BITS	= 0x3,
> -	TMC_MEM_INTF_WIDTH_128BITS	= 0x4,
> -	TMC_MEM_INTF_WIDTH_256BITS	= 0x5,
> +	TMC_MEM_INTF_WIDTH_32BITS	= 1,
> +	TMC_MEM_INTF_WIDTH_64BITS	= 2,
> +	TMC_MEM_INTF_WIDTH_128BITS	= 4,
> +	TMC_MEM_INTF_WIDTH_256BITS	= 8,
>   };

I think this would cause confusion for the reader. It would be good to
leave the definitions above as before and tmc_get_memwidth() doing:

i.e,
	case TMC_MEM_INTF_WIDTH_32BITS:
		memwidth = 1;
		break;

But we still store the memwidth in bytes.

Suzuki

Re: [PATCH V3 13/18] coresight: tmc: make sysFS and Perf mode mutually exclusive

[Mathieu Poirier]

On 25 April 2016 at 08:32, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
> On 22/04/16 18:14, Mathieu Poirier wrote:
>>
>> The sysFS and Perf access methods can't be allowed to interfere
>> with one another.  As such introducing guards to access
>> functions that prevents moving forward if a TMC is already
>> being used.
>>
>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> ---
>>   drivers/hwtracing/coresight/coresight-tmc-etf.c | 60
>> +++++++++++++++++++++-
>>   drivers/hwtracing/coresight/coresight-tmc-etr.c | 68
>> +++++++++++++++++++++++--
>>   2 files changed, 121 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c
>> b/drivers/hwtracing/coresight/coresight-tmc-etf.c
>> index 25fad93b68d4..cc88c15ba45c 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
>> @@ -111,7 +111,7 @@ static void tmc_etf_disable_hw(struct tmc_drvdata
>> *drvdata)
>>         CS_LOCK(drvdata->base);
>>   }
>>
>> -static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
>> +static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev, u32
>> mode)
>>   {
>>         bool used = false;
>>         char *buf = NULL;
>> @@ -190,6 +190,54 @@ spin_unlock:
>>         return 0;
>>   }
>>
>> +static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, u32
>> mode)
>> +{
>> +       int ret = 0;
>> +       long val;
>> +       unsigned long flags;
>> +       struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +
>> +        /* This shouldn't be happening */
>> +       if (WARN_ON(mode != CS_MODE_PERF))
>> +               return -EINVAL;
>> +
>> +       spin_lock_irqsave(&drvdata->spinlock, flags);
>> +       if (drvdata->reading) {
>> +               ret = -EINVAL;
>> +               goto out;
>> +       }
>> +
>> +       val = local_xchg(&drvdata->mode, mode);
>> +       /*
>> +        * In Perf mode there can be only one writer per sink.  There
>> +        * is also no need to continue if the ETB/ETR is already operated
>> +        * from sysFS.
>> +        */
>> +       if (val != CS_MODE_DISABLED) {
>> +               ret = -EINVAL;
>> +               goto out;
>> +       }
>> +
>> +       tmc_etb_enable_hw(drvdata);
>> +out:
>> +       spin_unlock_irqrestore(&drvdata->spinlock, flags);
>> +
>> +       return ret;
>> +}
>> +
>> +static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
>> +{
>> +       switch (mode) {
>> +       case CS_MODE_SYSFS:
>> +               return tmc_enable_etf_sink_sysfs(csdev, mode);
>> +       case CS_MODE_PERF:
>> +               return tmc_enable_etf_sink_perf(csdev, mode);
>> +       }
>> +
>> +       /* We shouldn't be here */
>> +       return -EINVAL;
>> +}
>> +
>>   static void tmc_disable_etf_sink(struct coresight_device *csdev)
>>   {
>>         long val;
>> @@ -272,6 +320,7 @@ const struct coresight_ops tmc_etf_cs_ops = {
>>
>>   int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
>>   {
>> +       long val;
>>         enum tmc_mode mode;
>>         int ret = 0;
>>         unsigned long flags;
>> @@ -290,6 +339,13 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
>>                 goto out;
>>         }
>>
>> +       val = local_read(&drvdata->mode);
>> +       /* Don't interfere if operated from Perf */
>> +       if (val == CS_MODE_PERF) {
>> +               ret = -EINVAL;
>> +               goto out;
>> +       }
>> +
>>         /* If drvdata::buf is NULL the trace data has been read already */
>>         if (drvdata->buf == NULL) {
>>                 ret = -EINVAL;
>> @@ -297,7 +353,7 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
>>         }
>>
>>         /* Disable the TMC if need be */
>> -       if (local_read(&drvdata->mode) == CS_MODE_SYSFS)
>> +       if (val == CS_MODE_SYSFS)
>>                 tmc_etb_disable_hw(drvdata);
>>
>>         drvdata->reading = true;
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> index 4b000f4003a2..a9a94a09186a 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> @@ -87,7 +87,7 @@ static void tmc_etr_disable_hw(struct tmc_drvdata
>> *drvdata)
>>         CS_LOCK(drvdata->base);
>>   }
>>
>> -static int tmc_enable_etr_sink(struct coresight_device *csdev, u32 mode)
>> +static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev, u32
>> mode)
>>   {
>>         bool used = false;
>>         long val;
>> @@ -167,6 +167,54 @@ spin_unlock:
>>         return 0;
>>   }
>>
>> +static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, u32
>> mode)
>> +{
>> +       int ret = 0;
>> +       long val;
>> +       unsigned long flags;
>> +       struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +
>> +        /* This shouldn't be happening */
>> +       if (WARN_ON(mode != CS_MODE_PERF))
>> +               return -EINVAL;
>> +
>> +       spin_lock_irqsave(&drvdata->spinlock, flags);
>> +       if (drvdata->reading) {
>> +               ret = -EINVAL;
>> +               goto out;
>> +       }
>> +
>> +       val = local_xchg(&drvdata->mode, mode);
>> +       /*
>> +        * In Perf mode there can be only one writer per sink.  There
>> +        * is also no need to continue if the ETR is already operated
>> +        * from sysFS.
>> +        */
>> +       if (val != CS_MODE_DISABLED) {
>
>
> Could val be CS_MODE_PERF ? In other words, should we be checking :
>         if (val == CS_MODE_SYSFS)  instead ?

If we check for CS_MODE_SYSFS we also have to check for CS_MODE_PERF,
which is two checks rather than a single one with the current
solution.

Mathieu

>
> Suzuki

Re: [PATCH V3 12/18] coresight: tmc: dump system memory content only when needed

[Suzuki K Poulose]

On 25/04/16 15:38, Mathieu Poirier wrote:
> On 25 April 2016 at 05:16, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
>> On 22/04/16 18:14, Mathieu Poirier wrote:
>>>
>>> Calling tmc_etf/etr_dump_hw() is required only when operating from
>>> sysFS.  When working from Perf, the system memory is harvested
>>> from the AUX trace API.
>>>
>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> ---
>>>    drivers/hwtracing/coresight/coresight-tmc-etf.c | 7 ++++++-
>>>    drivers/hwtracing/coresight/coresight-tmc-etr.c | 7 ++++++-
>>>    2 files changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c
>>> b/drivers/hwtracing/coresight/coresight-tmc-etf.c
>>> index d090a9745c73..25fad93b68d4 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
>>> @@ -74,7 +74,12 @@ static void tmc_etb_disable_hw(struct tmc_drvdata
>>> *drvdata)
>>>          CS_UNLOCK(drvdata->base);
>>>
>>>          tmc_flush_and_stop(drvdata);
>>> -       tmc_etb_dump_hw(drvdata);
>>> +       /*
>>> +        * When operating in sysFS mode the content of the buffer needs to
>>> be
>>> +        * read before the TMC is disabled.
>>> +        */
>>> +       if (local_read(&drvdata->mode) == CS_MODE_SYSFS)
>>> +               tmc_etb_dump_hw(drvdata);
>>>          tmc_disable_hw(drvdata);
>>>
>>>          CS_LOCK(drvdata->base);
>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>>> b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>>> index 8bbbf3ab1387..4b000f4003a2 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>>> @@ -76,7 +76,12 @@ static void tmc_etr_disable_hw(struct tmc_drvdata
>>> *drvdata)
>>>          CS_UNLOCK(drvdata->base);
>>>
>>>          tmc_flush_and_stop(drvdata);
>>> -       tmc_etr_dump_hw(drvdata);
>>> +       /*
>>> +        * When operating in sysFS mode the content of the buffer needs to
>>> be
>>> +        * read before the TMC is disabled.
>>> +        */
>>> +       if (local_read(&drvdata->mode) == CS_MODE_SYSFS)
>>> +               tmc_etr_dump_hw(drvdata);
>>>          tmc_disable_hw(drvdata);
>>
>>
>> Nit: It would be cleaner if tmc_etX_disable_hw accepted a bool parameter to
>> decide
>> whether to dump the hw data or not. That way, the callers can make the
>> decision,
>> leaving the disable_hw code unaware of the mode checks.
>
> My goal in pushing the functionality to tmc_etX_disable_hw() was to
> especially keep the the higher layers unaware of sink buffer
> management specifics.  There is two ways to look at this and we simply
> landed on different sides of the fence.  I'm not strongly opinionated
> on this, I'll modify if you're really keen on this.

I am not, you can retain as it is.

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Suzuki

Re: [PATCH V3 13/18] coresight: tmc: make sysFS and Perf mode mutually exclusive

[Suzuki K Poulose]

On 25/04/16 15:48, Mathieu Poirier wrote:
> On 25 April 2016 at 08:32, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
>> On 22/04/16 18:14, Mathieu Poirier wrote:

>>> +       spin_lock_irqsave(&drvdata->spinlock, flags);
>>> +       if (drvdata->reading) {
>>> +               ret = -EINVAL;
>>> +               goto out;
>>> +       }
>>> +
>>> +       val = local_xchg(&drvdata->mode, mode);
>>> +       /*
>>> +        * In Perf mode there can be only one writer per sink.  There
>>> +        * is also no need to continue if the ETR is already operated
>>> +        * from sysFS.
>>> +        */
>>> +       if (val != CS_MODE_DISABLED) {
>>
>>
>> Could val be CS_MODE_PERF ? In other words, should we be checking :
>>          if (val == CS_MODE_SYSFS)  instead ?
>
> If we check for CS_MODE_SYSFS we also have to check for CS_MODE_PERF,
> which is two checks rather than a single one with the current
> solution.

I am confused now. The comment says, we want to check for sysfs mode and
don't continue in that case. So, we shouldn't be worried about PERF mode.

Suzuki

Re: [PATCH V3 14/18] coresight: tmc: keep track of memory width

[Mathieu Poirier]

On 25 April 2016 at 08:41, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
> On 22/04/16 18:14, Mathieu Poirier wrote:
>>
>> Accessing the HW configuration register each time the memory
>> width is needed simply doesn't make sense.  It is much more
>> efficient to read the value once and keep a reference for
>> later use.
>>
>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> ---
>>   drivers/hwtracing/coresight/coresight-tmc-etf.c | 14 +---------
>>   drivers/hwtracing/coresight/coresight-tmc.c     | 34
>> +++++++++++++++++++++++++
>>   drivers/hwtracing/coresight/coresight-tmc.h     | 10 +++++---
>>   3 files changed, 41 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c
>> b/drivers/hwtracing/coresight/coresight-tmc-etf.c
>> index cc88c15ba45c..981c5ca75e36 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
>> @@ -41,25 +41,13 @@ void tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
>>
>>   static void tmc_etb_dump_hw(struct tmc_drvdata *drvdata)
>>   {
>> -       enum tmc_mem_intf_width memwidth;
>> -       u8 memwords;
>>         char *bufp;
>>         u32 read_data;
>>         int i;
>>
>> -       memwidth = BMVAL(readl_relaxed(drvdata->base + CORESIGHT_DEVID),
>> 8, 10);
>> -       if (memwidth == TMC_MEM_INTF_WIDTH_32BITS)
>> -               memwords = 1;
>> -       else if (memwidth == TMC_MEM_INTF_WIDTH_64BITS)
>> -               memwords = 2;
>> -       else if (memwidth == TMC_MEM_INTF_WIDTH_128BITS)
>> -               memwords = 4;
>> -       else
>> -               memwords = 8;
>> -
>>         bufp = drvdata->buf;
>>         while (1) {
>> -               for (i = 0; i < memwords; i++) {
>> +               for (i = 0; i < drvdata->memwidth; i++) {
>>                         read_data = readl_relaxed(drvdata->base +
>> TMC_RRD);
>>                         if (read_data == 0xFFFFFFFF)
>>                                 return;
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc.c
>> b/drivers/hwtracing/coresight/coresight-tmc.c
>> index 55806352b1f1..cb030a09659d 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc.c
>> @@ -193,6 +193,39 @@ static const struct file_operations tmc_fops = {
>>         .llseek         = no_llseek,
>>   };
>>
>> +static enum tmc_mem_intf_width tmc_get_memwidth(u32 devid)
>> +{
>> +       enum tmc_mem_intf_width memwidth;
>> +
>> +       /*
>> +        * Excerpt from the TRM:
>> +        *
>> +        * DEVID::MEMWIDTH[10:8]
>> +        * 0x2 Memory interface databus is 32 bits wide.
>> +        * 0x3 Memory interface databus is 64 bits wide.
>> +        * 0x4 Memory interface databus is 128 bits wide.
>> +        * 0x5 Memory interface databus is 256 bits wide.
>> +        */
>> +       switch (BMVAL(devid, 8, 10)) {
>> +       case 0x2:
>> +               memwidth = TMC_MEM_INTF_WIDTH_32BITS;
>> +               break;
>> +       case 0x3:
>> +               memwidth = TMC_MEM_INTF_WIDTH_64BITS;
>> +               break;
>> +       case 0x4:
>> +               memwidth = TMC_MEM_INTF_WIDTH_128BITS;
>> +               break;
>> +       case 0x5:
>> +               memwidth = TMC_MEM_INTF_WIDTH_256BITS;
>> +               break;
>> +       default:
>> +               memwidth = 0;
>> +       }
>> +
>> +       return memwidth;
>> +}
>> +
>>   #define coresight_tmc_simple_func(name, offset)                       \
>>         coresight_simple_func(struct tmc_drvdata, name, offset)
>>
>> @@ -306,6 +339,7 @@ static int tmc_probe(struct amba_device *adev, const
>> struct amba_id *id)
>>
>>         devid = readl_relaxed(drvdata->base + CORESIGHT_DEVID);
>>         drvdata->config_type = BMVAL(devid, 6, 7);
>> +       drvdata->memwidth = tmc_get_memwidth(devid);
>>
>>         if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
>>                 if (np)
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h
>> b/drivers/hwtracing/coresight/coresight-tmc.h
>> index 9b4c215d2b6b..12a097f3d06c 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc.h
>> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
>> @@ -81,10 +81,10 @@ enum tmc_mode {
>>   };
>>
>>   enum tmc_mem_intf_width {
>> -       TMC_MEM_INTF_WIDTH_32BITS       = 0x2,
>> -       TMC_MEM_INTF_WIDTH_64BITS       = 0x3,
>> -       TMC_MEM_INTF_WIDTH_128BITS      = 0x4,
>> -       TMC_MEM_INTF_WIDTH_256BITS      = 0x5,
>> +       TMC_MEM_INTF_WIDTH_32BITS       = 1,
>> +       TMC_MEM_INTF_WIDTH_64BITS       = 2,
>> +       TMC_MEM_INTF_WIDTH_128BITS      = 4,
>> +       TMC_MEM_INTF_WIDTH_256BITS      = 8,
>>   };
>
>
> I think this would cause confusion for the reader. It would be good to
> leave the definitions above as before and tmc_get_memwidth() doing:
>
> i.e,
>         case TMC_MEM_INTF_WIDTH_32BITS:
>                 memwidth = 1;
>                 break;
>
> But we still store the memwidth in bytes.

If we proceed this way we have to do a case statement on hard coded
values (or introduce a new enumeration) in tmc_update_etf_buffer(). In
my opinion the current solution scales better.

Mathieu


>
> Suzuki

Re: [PATCH V3 13/18] coresight: tmc: make sysFS and Perf mode mutually exclusive

[Mathieu Poirier]

On 25 April 2016 at 08:52, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
> On 25/04/16 15:48, Mathieu Poirier wrote:
>>
>> On 25 April 2016 at 08:32, Suzuki K Poulose <Suzuki.Poulose@arm.com>
>> wrote:
>>>
>>> On 22/04/16 18:14, Mathieu Poirier wrote:
>
>
>>>> +       spin_lock_irqsave(&drvdata->spinlock, flags);
>>>> +       if (drvdata->reading) {
>>>> +               ret = -EINVAL;
>>>> +               goto out;
>>>> +       }
>>>> +
>>>> +       val = local_xchg(&drvdata->mode, mode);
>>>> +       /*
>>>> +        * In Perf mode there can be only one writer per sink.  There
>>>> +        * is also no need to continue if the ETR is already operated
>>>> +        * from sysFS.
>>>> +        */
>>>> +       if (val != CS_MODE_DISABLED) {
>>>
>>>
>>>
>>> Could val be CS_MODE_PERF ? In other words, should we be checking :
>>>          if (val == CS_MODE_SYSFS)  instead ?
>>
>>
>> If we check for CS_MODE_SYSFS we also have to check for CS_MODE_PERF,
>> which is two checks rather than a single one with the current
>> solution.
>
>
> I am confused now. The comment says, we want to check for sysfs mode and
> don't continue in that case. So, we shouldn't be worried about PERF mode.

You are correct about the sysFS part, but the first sentence of the
comment also mention that in perf mode there can only be one writer
per sink.  Otherwise ring buffers for one session would end up with
traces from other ongoing sessions, and that is not taking into
account the buffer management nightmares it would cause.

Mathieu

>
> Suzuki
>
>

Re: [PATCH V3 14/18] coresight: tmc: keep track of memory width

[Suzuki K Poulose]

On 25/04/16 15:55, Mathieu Poirier wrote:
> On 25 April 2016 at 08:41, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
>> On 22/04/16 18:14, Mathieu Poirier wrote:
>>>
>>> Accessing the HW configuration register each time the memory
>>> width is needed simply doesn't make sense.  It is much more
>>> efficient to read the value once and keep a reference for
>>> later use.
>>>
>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> ---
>>>    drivers/hwtracing/coresight/coresight-tmc-etf.c | 14 +---------
>>>    drivers/hwtracing/coresight/coresight-tmc.c     | 34
>>> +++++++++++++++++++++++++
>>>    drivers/hwtracing/coresight/coresight-tmc.h     | 10 +++++---
>>>    3 files changed, 41 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c
>>> b/drivers/hwtracing/coresight/coresight-tmc-etf.c
>>> index cc88c15ba45c..981c5ca75e36 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
>>> @@ -41,25 +41,13 @@ void tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
>>>
>>>    static void tmc_etb_dump_hw(struct tmc_drvdata *drvdata)
>>>    {
>>> -       enum tmc_mem_intf_width memwidth;
>>> -       u8 memwords;
>>>          char *bufp;
>>>          u32 read_data;
>>>          int i;
>>>
>>> -       memwidth = BMVAL(readl_relaxed(drvdata->base + CORESIGHT_DEVID),
>>> 8, 10);
>>> -       if (memwidth == TMC_MEM_INTF_WIDTH_32BITS)
>>> -               memwords = 1;
>>> -       else if (memwidth == TMC_MEM_INTF_WIDTH_64BITS)
>>> -               memwords = 2;
>>> -       else if (memwidth == TMC_MEM_INTF_WIDTH_128BITS)
>>> -               memwords = 4;
>>> -       else
>>> -               memwords = 8;
>>> -
>>>          bufp = drvdata->buf;
>>>          while (1) {
>>> -               for (i = 0; i < memwords; i++) {
>>> +               for (i = 0; i < drvdata->memwidth; i++) {
>>>                          read_data = readl_relaxed(drvdata->base +
>>> TMC_RRD);
>>>                          if (read_data == 0xFFFFFFFF)
>>>                                  return;
>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc.c
>>> b/drivers/hwtracing/coresight/coresight-tmc.c
>>> index 55806352b1f1..cb030a09659d 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tmc.c
>>> +++ b/drivers/hwtracing/coresight/coresight-tmc.c
>>> @@ -193,6 +193,39 @@ static const struct file_operations tmc_fops = {
>>>          .llseek         = no_llseek,
>>>    };
>>>
>>> +static enum tmc_mem_intf_width tmc_get_memwidth(u32 devid)
>>> +{
>>> +       enum tmc_mem_intf_width memwidth;
>>> +
>>> +       /*
>>> +        * Excerpt from the TRM:
>>> +        *
>>> +        * DEVID::MEMWIDTH[10:8]
>>> +        * 0x2 Memory interface databus is 32 bits wide.
>>> +        * 0x3 Memory interface databus is 64 bits wide.
>>> +        * 0x4 Memory interface databus is 128 bits wide.
>>> +        * 0x5 Memory interface databus is 256 bits wide.
>>> +        */
>>> +       switch (BMVAL(devid, 8, 10)) {
>>> +       case 0x2:
>>> +               memwidth = TMC_MEM_INTF_WIDTH_32BITS;
>>> +               break;
>>> +       case 0x3:
>>> +               memwidth = TMC_MEM_INTF_WIDTH_64BITS;
>>> +               break;
>>> +       case 0x4:
>>> +               memwidth = TMC_MEM_INTF_WIDTH_128BITS;
>>> +               break;
>>> +       case 0x5:
>>> +               memwidth = TMC_MEM_INTF_WIDTH_256BITS;
>>> +               break;
>>> +       default:
>>> +               memwidth = 0;
>>> +       }
>>> +
>>> +       return memwidth;
>>> +}
>>> +
>>>    #define coresight_tmc_simple_func(name, offset)                       \
>>>          coresight_simple_func(struct tmc_drvdata, name, offset)
>>>
>>> @@ -306,6 +339,7 @@ static int tmc_probe(struct amba_device *adev, const
>>> struct amba_id *id)
>>>
>>>          devid = readl_relaxed(drvdata->base + CORESIGHT_DEVID);
>>>          drvdata->config_type = BMVAL(devid, 6, 7);
>>> +       drvdata->memwidth = tmc_get_memwidth(devid);
>>>
>>>          if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
>>>                  if (np)
>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h
>>> b/drivers/hwtracing/coresight/coresight-tmc.h
>>> index 9b4c215d2b6b..12a097f3d06c 100644
>>> --- a/drivers/hwtracing/coresight/coresight-tmc.h
>>> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
>>> @@ -81,10 +81,10 @@ enum tmc_mode {
>>>    };
>>>
>>>    enum tmc_mem_intf_width {
>>> -       TMC_MEM_INTF_WIDTH_32BITS       = 0x2,
>>> -       TMC_MEM_INTF_WIDTH_64BITS       = 0x3,
>>> -       TMC_MEM_INTF_WIDTH_128BITS      = 0x4,
>>> -       TMC_MEM_INTF_WIDTH_256BITS      = 0x5,
>>> +       TMC_MEM_INTF_WIDTH_32BITS       = 1,
>>> +       TMC_MEM_INTF_WIDTH_64BITS       = 2,
>>> +       TMC_MEM_INTF_WIDTH_128BITS      = 4,
>>> +       TMC_MEM_INTF_WIDTH_256BITS      = 8,
>>>    };
>>
>>
>> I think this would cause confusion for the reader. It would be good to
>> leave the definitions above as before and tmc_get_memwidth() doing:
>>
>> i.e,
>>          case TMC_MEM_INTF_WIDTH_32BITS:
>>                  memwidth = 1;
>>                  break;
>>
>> But we still store the memwidth in bytes.
>
> If we proceed this way we have to do a case statement on hard coded
> values (or introduce a new enumeration) in tmc_update_etf_buffer(). In

But then we are doing that already with this patch in tmc_get_memwidth already.
My point is, we expect named symbols for values defined in the standards, so that
you don't make a mistake when dealing with them. It is really difficult to track
down a bug if we do make a mistake.

> my opinion the current solution scales better.

I would prefer doing a hardcoded check in tmc_update_etf_buffer() like :

+		/*
+		 * The value written to RRP must be byte-address aligned to
+		 * the width of the trace memory databus _and_ to a frame
+		 * boundary (16 byte), whichever is the biggest. For example,
+		 * for 32-bit, 64-bit and 128-bit wide trace memory, the four
+		 * LSBs must be 0s. For 256-bit wide trace memory, the five
+		 * LSBs must be 0s.
+		 */
+		if (drvdata->memwidth == 8)
+			mask = GENMASK(31, 6);
+		else
+			mask = GENMASK(31, 5);


Suzuki

Re: [PATCH V3 13/18] coresight: tmc: make sysFS and Perf mode mutually exclusive

[Suzuki K Poulose]

On 25/04/16 16:05, Mathieu Poirier wrote:
> On 25 April 2016 at 08:52, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
>> On 25/04/16 15:48, Mathieu Poirier wrote:
>>>
>>> On 25 April 2016 at 08:32, Suzuki K Poulose <Suzuki.Poulose@arm.com>
>>> wrote:
>>>>
>>>> On 22/04/16 18:14, Mathieu Poirier wrote:
>>
>>
>>>>> +       spin_lock_irqsave(&drvdata->spinlock, flags);
>>>>> +       if (drvdata->reading) {
>>>>> +               ret = -EINVAL;
>>>>> +               goto out;
>>>>> +       }
>>>>> +
>>>>> +       val = local_xchg(&drvdata->mode, mode);
>>>>> +       /*
>>>>> +        * In Perf mode there can be only one writer per sink.  There
>>>>> +        * is also no need to continue if the ETR is already operated
>>>>> +        * from sysFS.
>>>>> +        */
>>>>> +       if (val != CS_MODE_DISABLED) {
>>>>
>>>>
>>>>
>>>> Could val be CS_MODE_PERF ? In other words, should we be checking :
>>>>           if (val == CS_MODE_SYSFS)  instead ?
>>>
>>>
>>> If we check for CS_MODE_SYSFS we also have to check for CS_MODE_PERF,
>>> which is two checks rather than a single one with the current
>>> solution.
>>
>>
>> I am confused now. The comment says, we want to check for sysfs mode and
>> don't continue in that case. So, we shouldn't be worried about PERF mode.
>
> You are correct about the sysFS part, but the first sentence of the
> comment also mention that in perf mode there can only be one writer
> per sink.  Otherwise ring buffers for one session would end up with
> traces from other ongoing sessions, and that is not taking into
> account the buffer management nightmares it would cause.

OK, in either case, val == CS_MODE_SYSFS is much better check there, to
what we want to do

Suzuki

Re: [PATCH V3 13/18] coresight: tmc: make sysFS and Perf mode mutually exclusive

[Mathieu Poirier]

On 25 April 2016 at 09:11, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
> On 25/04/16 16:05, Mathieu Poirier wrote:
>>
>> On 25 April 2016 at 08:52, Suzuki K Poulose <Suzuki.Poulose@arm.com>
>> wrote:
>>>
>>> On 25/04/16 15:48, Mathieu Poirier wrote:
>>>>
>>>>
>>>> On 25 April 2016 at 08:32, Suzuki K Poulose <Suzuki.Poulose@arm.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> On 22/04/16 18:14, Mathieu Poirier wrote:
>>>
>>>
>>>
>>>>>> +       spin_lock_irqsave(&drvdata->spinlock, flags);
>>>>>> +       if (drvdata->reading) {
>>>>>> +               ret = -EINVAL;
>>>>>> +               goto out;
>>>>>> +       }
>>>>>> +
>>>>>> +       val = local_xchg(&drvdata->mode, mode);
>>>>>> +       /*
>>>>>> +        * In Perf mode there can be only one writer per sink.  There
>>>>>> +        * is also no need to continue if the ETR is already operated
>>>>>> +        * from sysFS.
>>>>>> +        */
>>>>>> +       if (val != CS_MODE_DISABLED) {
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Could val be CS_MODE_PERF ? In other words, should we be checking :
>>>>>           if (val == CS_MODE_SYSFS)  instead ?
>>>>
>>>>
>>>>
>>>> If we check for CS_MODE_SYSFS we also have to check for CS_MODE_PERF,
>>>> which is two checks rather than a single one with the current
>>>> solution.
>>>
>>>
>>>
>>> I am confused now. The comment says, we want to check for sysfs mode and
>>> don't continue in that case. So, we shouldn't be worried about PERF mode.
>>
>>
>> You are correct about the sysFS part, but the first sentence of the
>> comment also mention that in perf mode there can only be one writer
>> per sink.  Otherwise ring buffers for one session would end up with
>> traces from other ongoing sessions, and that is not taking into
>> account the buffer management nightmares it would cause.
>
>
> OK, in either case, val == CS_MODE_SYSFS is much better check there, to
> what we want to do

If we check for SYSFS we also need to check for PERF.  Otherwise
nothing prevents another session from using the sink buffer, which is
not supported.

>
> Suzuki

Re: [PATCH V3 14/18] coresight: tmc: keep track of memory width

[Mathieu Poirier]

On 25 April 2016 at 09:09, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
> On 25/04/16 15:55, Mathieu Poirier wrote:
>>
>> On 25 April 2016 at 08:41, Suzuki K Poulose <Suzuki.Poulose@arm.com>
>> wrote:
>>>
>>> On 22/04/16 18:14, Mathieu Poirier wrote:
>>>>
>>>>
>>>> Accessing the HW configuration register each time the memory
>>>> width is needed simply doesn't make sense.  It is much more
>>>> efficient to read the value once and keep a reference for
>>>> later use.
>>>>
>>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>> ---
>>>>    drivers/hwtracing/coresight/coresight-tmc-etf.c | 14 +---------
>>>>    drivers/hwtracing/coresight/coresight-tmc.c     | 34
>>>> +++++++++++++++++++++++++
>>>>    drivers/hwtracing/coresight/coresight-tmc.h     | 10 +++++---
>>>>    3 files changed, 41 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c
>>>> b/drivers/hwtracing/coresight/coresight-tmc-etf.c
>>>> index cc88c15ba45c..981c5ca75e36 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
>>>> @@ -41,25 +41,13 @@ void tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
>>>>
>>>>    static void tmc_etb_dump_hw(struct tmc_drvdata *drvdata)
>>>>    {
>>>> -       enum tmc_mem_intf_width memwidth;
>>>> -       u8 memwords;
>>>>          char *bufp;
>>>>          u32 read_data;
>>>>          int i;
>>>>
>>>> -       memwidth = BMVAL(readl_relaxed(drvdata->base + CORESIGHT_DEVID),
>>>> 8, 10);
>>>> -       if (memwidth == TMC_MEM_INTF_WIDTH_32BITS)
>>>> -               memwords = 1;
>>>> -       else if (memwidth == TMC_MEM_INTF_WIDTH_64BITS)
>>>> -               memwords = 2;
>>>> -       else if (memwidth == TMC_MEM_INTF_WIDTH_128BITS)
>>>> -               memwords = 4;
>>>> -       else
>>>> -               memwords = 8;
>>>> -
>>>>          bufp = drvdata->buf;
>>>>          while (1) {
>>>> -               for (i = 0; i < memwords; i++) {
>>>> +               for (i = 0; i < drvdata->memwidth; i++) {
>>>>                          read_data = readl_relaxed(drvdata->base +
>>>> TMC_RRD);
>>>>                          if (read_data == 0xFFFFFFFF)
>>>>                                  return;
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc.c
>>>> b/drivers/hwtracing/coresight/coresight-tmc.c
>>>> index 55806352b1f1..cb030a09659d 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tmc.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc.c
>>>> @@ -193,6 +193,39 @@ static const struct file_operations tmc_fops = {
>>>>          .llseek         = no_llseek,
>>>>    };
>>>>
>>>> +static enum tmc_mem_intf_width tmc_get_memwidth(u32 devid)
>>>> +{
>>>> +       enum tmc_mem_intf_width memwidth;
>>>> +
>>>> +       /*
>>>> +        * Excerpt from the TRM:
>>>> +        *
>>>> +        * DEVID::MEMWIDTH[10:8]
>>>> +        * 0x2 Memory interface databus is 32 bits wide.
>>>> +        * 0x3 Memory interface databus is 64 bits wide.
>>>> +        * 0x4 Memory interface databus is 128 bits wide.
>>>> +        * 0x5 Memory interface databus is 256 bits wide.
>>>> +        */
>>>> +       switch (BMVAL(devid, 8, 10)) {
>>>> +       case 0x2:
>>>> +               memwidth = TMC_MEM_INTF_WIDTH_32BITS;
>>>> +               break;
>>>> +       case 0x3:
>>>> +               memwidth = TMC_MEM_INTF_WIDTH_64BITS;
>>>> +               break;
>>>> +       case 0x4:
>>>> +               memwidth = TMC_MEM_INTF_WIDTH_128BITS;
>>>> +               break;
>>>> +       case 0x5:
>>>> +               memwidth = TMC_MEM_INTF_WIDTH_256BITS;
>>>> +               break;
>>>> +       default:
>>>> +               memwidth = 0;
>>>> +       }
>>>> +
>>>> +       return memwidth;
>>>> +}
>>>> +
>>>>    #define coresight_tmc_simple_func(name, offset)
>>>> \
>>>>          coresight_simple_func(struct tmc_drvdata, name, offset)
>>>>
>>>> @@ -306,6 +339,7 @@ static int tmc_probe(struct amba_device *adev, const
>>>> struct amba_id *id)
>>>>
>>>>          devid = readl_relaxed(drvdata->base + CORESIGHT_DEVID);
>>>>          drvdata->config_type = BMVAL(devid, 6, 7);
>>>> +       drvdata->memwidth = tmc_get_memwidth(devid);
>>>>
>>>>          if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
>>>>                  if (np)
>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h
>>>> b/drivers/hwtracing/coresight/coresight-tmc.h
>>>> index 9b4c215d2b6b..12a097f3d06c 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-tmc.h
>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
>>>> @@ -81,10 +81,10 @@ enum tmc_mode {
>>>>    };
>>>>
>>>>    enum tmc_mem_intf_width {
>>>> -       TMC_MEM_INTF_WIDTH_32BITS       = 0x2,
>>>> -       TMC_MEM_INTF_WIDTH_64BITS       = 0x3,
>>>> -       TMC_MEM_INTF_WIDTH_128BITS      = 0x4,
>>>> -       TMC_MEM_INTF_WIDTH_256BITS      = 0x5,
>>>> +       TMC_MEM_INTF_WIDTH_32BITS       = 1,
>>>> +       TMC_MEM_INTF_WIDTH_64BITS       = 2,
>>>> +       TMC_MEM_INTF_WIDTH_128BITS      = 4,
>>>> +       TMC_MEM_INTF_WIDTH_256BITS      = 8,
>>>>    };
>>>
>>>
>>>
>>> I think this would cause confusion for the reader. It would be good to
>>> leave the definitions above as before and tmc_get_memwidth() doing:
>>>
>>> i.e,
>>>          case TMC_MEM_INTF_WIDTH_32BITS:
>>>                  memwidth = 1;
>>>                  break;
>>>
>>> But we still store the memwidth in bytes.
>>
>>
>> If we proceed this way we have to do a case statement on hard coded
>> values (or introduce a new enumeration) in tmc_update_etf_buffer(). In
>
>
> But then we are doing that already with this patch in tmc_get_memwidth
> already.

Right.  With my approach we use hard coded values once and named
symbols every time we need access to memwidth.  With your approach we
use name symbols once and hard coded values (along with a lengthy
comment) every time memwidth is accessed.

> My point is, we expect named symbols for values defined in the standards, so
> that
> you don't make a mistake when dealing with them. It is really difficult to
> track
> down a bug if we do make a mistake.
>
>> my opinion the current solution scales better.
>
>
> I would prefer doing a hardcoded check in tmc_update_etf_buffer() like :
>
> +               /*
> +                * The value written to RRP must be byte-address aligned to
> +                * the width of the trace memory databus _and_ to a frame
> +                * boundary (16 byte), whichever is the biggest. For
> example,
> +                * for 32-bit, 64-bit and 128-bit wide trace memory, the
> four
> +                * LSBs must be 0s. For 256-bit wide trace memory, the five
> +                * LSBs must be 0s.
> +                */
> +               if (drvdata->memwidth == 8)
> +                       mask = GENMASK(31, 6);
> +               else
> +                       mask = GENMASK(31, 5);
>
>
> Suzuki

Re: [PATCH V3 14/18] coresight: tmc: keep track of memory width

[Suzuki K Poulose]

On 25/04/16 16:25, Mathieu Poirier wrote:
> On 25 April 2016 at 09:09, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
>> On 25/04/16 15:55, Mathieu Poirier wrote:
>>>
>>> On 25 April 2016 at 08:41, Suzuki K Poulose <Suzuki.Poulose@arm.com>
>>> wrote:
>>>>
>>>> On 22/04/16 18:14, Mathieu Poirier wrote:
>>>>>
>>>>>
>>>>> Accessing the HW configuration register each time the memory
>>>>> width is needed simply doesn't make sense.  It is much more
>>>>> efficient to read the value once and keep a reference for
>>>>> later use.
>>>>>
>>>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>>> ---
>>>>>     drivers/hwtracing/coresight/coresight-tmc-etf.c | 14 +---------
>>>>>     drivers/hwtracing/coresight/coresight-tmc.c     | 34
>>>>> +++++++++++++++++++++++++
>>>>>     drivers/hwtracing/coresight/coresight-tmc.h     | 10 +++++---
>>>>>     3 files changed, 41 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c
>>>>> b/drivers/hwtracing/coresight/coresight-tmc-etf.c
>>>>> index cc88c15ba45c..981c5ca75e36 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
>>>>> @@ -41,25 +41,13 @@ void tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
>>>>>
>>>>>     static void tmc_etb_dump_hw(struct tmc_drvdata *drvdata)
>>>>>     {
>>>>> -       enum tmc_mem_intf_width memwidth;
>>>>> -       u8 memwords;
>>>>>           char *bufp;
>>>>>           u32 read_data;
>>>>>           int i;
>>>>>
>>>>> -       memwidth = BMVAL(readl_relaxed(drvdata->base + CORESIGHT_DEVID),
>>>>> 8, 10);
>>>>> -       if (memwidth == TMC_MEM_INTF_WIDTH_32BITS)
>>>>> -               memwords = 1;
>>>>> -       else if (memwidth == TMC_MEM_INTF_WIDTH_64BITS)
>>>>> -               memwords = 2;
>>>>> -       else if (memwidth == TMC_MEM_INTF_WIDTH_128BITS)
>>>>> -               memwords = 4;
>>>>> -       else
>>>>> -               memwords = 8;
>>>>> -
>>>>>           bufp = drvdata->buf;
>>>>>           while (1) {
>>>>> -               for (i = 0; i < memwords; i++) {
>>>>> +               for (i = 0; i < drvdata->memwidth; i++) {
>>>>>                           read_data = readl_relaxed(drvdata->base +
>>>>> TMC_RRD);
>>>>>                           if (read_data == 0xFFFFFFFF)
>>>>>                                   return;
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc.c
>>>>> b/drivers/hwtracing/coresight/coresight-tmc.c
>>>>> index 55806352b1f1..cb030a09659d 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-tmc.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc.c
>>>>> @@ -193,6 +193,39 @@ static const struct file_operations tmc_fops = {
>>>>>           .llseek         = no_llseek,
>>>>>     };
>>>>>
>>>>> +static enum tmc_mem_intf_width tmc_get_memwidth(u32 devid)
>>>>> +{
>>>>> +       enum tmc_mem_intf_width memwidth;
>>>>> +
>>>>> +       /*
>>>>> +        * Excerpt from the TRM:
>>>>> +        *
>>>>> +        * DEVID::MEMWIDTH[10:8]
>>>>> +        * 0x2 Memory interface databus is 32 bits wide.
>>>>> +        * 0x3 Memory interface databus is 64 bits wide.
>>>>> +        * 0x4 Memory interface databus is 128 bits wide.
>>>>> +        * 0x5 Memory interface databus is 256 bits wide.
>>>>> +        */
>>>>> +       switch (BMVAL(devid, 8, 10)) {
>>>>> +       case 0x2:
>>>>> +               memwidth = TMC_MEM_INTF_WIDTH_32BITS;
>>>>> +               break;
>>>>> +       case 0x3:
>>>>> +               memwidth = TMC_MEM_INTF_WIDTH_64BITS;
>>>>> +               break;
>>>>> +       case 0x4:
>>>>> +               memwidth = TMC_MEM_INTF_WIDTH_128BITS;
>>>>> +               break;
>>>>> +       case 0x5:
>>>>> +               memwidth = TMC_MEM_INTF_WIDTH_256BITS;
>>>>> +               break;
>>>>> +       default:
>>>>> +               memwidth = 0;
>>>>> +       }
>>>>> +
>>>>> +       return memwidth;
>>>>> +}
>>>>> +
>>>>>     #define coresight_tmc_simple_func(name, offset)
>>>>> \
>>>>>           coresight_simple_func(struct tmc_drvdata, name, offset)
>>>>>
>>>>> @@ -306,6 +339,7 @@ static int tmc_probe(struct amba_device *adev, const
>>>>> struct amba_id *id)
>>>>>
>>>>>           devid = readl_relaxed(drvdata->base + CORESIGHT_DEVID);
>>>>>           drvdata->config_type = BMVAL(devid, 6, 7);
>>>>> +       drvdata->memwidth = tmc_get_memwidth(devid);
>>>>>
>>>>>           if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
>>>>>                   if (np)
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h
>>>>> b/drivers/hwtracing/coresight/coresight-tmc.h
>>>>> index 9b4c215d2b6b..12a097f3d06c 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-tmc.h
>>>>> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
>>>>> @@ -81,10 +81,10 @@ enum tmc_mode {
>>>>>     };
>>>>>
>>>>>     enum tmc_mem_intf_width {
>>>>> -       TMC_MEM_INTF_WIDTH_32BITS       = 0x2,
>>>>> -       TMC_MEM_INTF_WIDTH_64BITS       = 0x3,
>>>>> -       TMC_MEM_INTF_WIDTH_128BITS      = 0x4,
>>>>> -       TMC_MEM_INTF_WIDTH_256BITS      = 0x5,
>>>>> +       TMC_MEM_INTF_WIDTH_32BITS       = 1,
>>>>> +       TMC_MEM_INTF_WIDTH_64BITS       = 2,
>>>>> +       TMC_MEM_INTF_WIDTH_128BITS      = 4,
>>>>> +       TMC_MEM_INTF_WIDTH_256BITS      = 8,
>>>>>     };
>>>>
>>>>
>>>>
>>>> I think this would cause confusion for the reader. It would be good to
>>>> leave the definitions above as before and tmc_get_memwidth() doing:
>>>>
>>>> i.e,
>>>>           case TMC_MEM_INTF_WIDTH_32BITS:
>>>>                   memwidth = 1;
>>>>                   break;
>>>>
>>>> But we still store the memwidth in bytes.
>>>
>>>
>>> If we proceed this way we have to do a case statement on hard coded
>>> values (or introduce a new enumeration) in tmc_update_etf_buffer(). In
>>
>>
>> But then we are doing that already with this patch in tmc_get_memwidth
>> already.
>
> Right.  With my approach we use hard coded values once and named
> symbols every time we need access to memwidth.  With your approach we
> use name symbols once and hard coded values (along with a lengthy
> comment) every time memwidth is accessed.

That lengthy comment already exists as part of your patch. All I was saying
is you could reduce the switch..case for width to an if () for hard coded
value check, which is only done for mask generation.

Suzuki

Re: [PATCH V3 13/18] coresight: tmc: make sysFS and Perf mode mutually exclusive

[Suzuki K Poulose]

On 25/04/16 16:18, Mathieu Poirier wrote:
> On 25 April 2016 at 09:11, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote:
>> On 25/04/16 16:05, Mathieu Poirier wrote:
>>>
>>> On 25 April 2016 at 08:52, Suzuki K Poulose <Suzuki.Poulose@arm.com>
>>> wrote:
>>>>
>>>> On 25/04/16 15:48, Mathieu Poirier wrote:
>>>>>
>>>>>
>>>>> On 25 April 2016 at 08:32, Suzuki K Poulose <Suzuki.Poulose@arm.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> On 22/04/16 18:14, Mathieu Poirier wrote:
>>>>
>>>>
>>>>
>>>>>>> +       spin_lock_irqsave(&drvdata->spinlock, flags);
>>>>>>> +       if (drvdata->reading) {
>>>>>>> +               ret = -EINVAL;
>>>>>>> +               goto out;
>>>>>>> +       }
>>>>>>> +
>>>>>>> +       val = local_xchg(&drvdata->mode, mode);
>>>>>>> +       /*
>>>>>>> +        * In Perf mode there can be only one writer per sink.  There
>>>>>>> +        * is also no need to continue if the ETR is already operated
>>>>>>> +        * from sysFS.
>>>>>>> +        */
>>>>>>> +       if (val != CS_MODE_DISABLED) {
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Could val be CS_MODE_PERF ? In other words, should we be checking :
>>>>>>            if (val == CS_MODE_SYSFS)  instead ?
>>>>>
>>>>>
>>>>>
>>>>> If we check for CS_MODE_SYSFS we also have to check for CS_MODE_PERF,
>>>>> which is two checks rather than a single one with the current
>>>>> solution.
>>>>
>>>>
>>>>
>>>> I am confused now. The comment says, we want to check for sysfs mode and
>>>> don't continue in that case. So, we shouldn't be worried about PERF mode.
>>>
>>>
>>> You are correct about the sysFS part, but the first sentence of the
>>> comment also mention that in perf mode there can only be one writer
>>> per sink.  Otherwise ring buffers for one session would end up with
>>> traces from other ongoing sessions, and that is not taking into
>>> account the buffer management nightmares it would cause.
>>
>>
>> OK, in either case, val == CS_MODE_SYSFS is much better check there, to
>> what we want to do
>
> If we check for SYSFS we also need to check for PERF.  Otherwise
> nothing prevents another session from using the sink buffer, which is
> not supported.

Ah, I got wrong. Sorry for the noise. The current check makes sense.

Suzuki