All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2 0/6] Introduce CoreSight STM support
@ 2016-02-03  8:15 ` Chunyan Zhang
  0 siblings, 0 replies; 68+ messages in thread
From: Chunyan Zhang @ 2016-02-03  8:15 UTC (permalink / raw)
  To: mathieu.poirier, alexander.shishkin
  Cc: robh, broonie, pratikp, nicolas.guion, corbet, mark.rutland,
	mike.leach, tor, al.grant, zhang.lyra, linux-kernel,
	linux-arm-kernel, linux-api, linux-doc

This patchset adds support for CoreSight STM IP block. It also makes
a little modification to the generic STM framework to cover the
CoreSight STM requirements. Full description follows the changelog.

Changes from v1:
 - Added a definition of coresight_simple_func() in CS-STM driver to
   avoid the kbuild test robot error for the time being.  This
   modification will be removed when merging the code in which the
   coresight_simple_func() has been moved to the header file.
 - Calculate the channel number according to the channel memory space size.

Thanks,
Chunyan

Chunyan Zhang (3):
  stm class: Add ioctl get_options interface
  stm class: adds a loop to extract the first valid STM device name
  Documentations: Add explanations of the case for non-configurable
    masters

Mathieu Poirier (2):
  stm class: provision for statically assigned masterIDs
  coresight-stm: Bindings for System Trace Macrocell

Pratik Patel (1):
  coresight-stm: adding driver for CoreSight STM component

 .../ABI/testing/sysfs-bus-coresight-devices-stm    |  53 ++
 .../devicetree/bindings/arm/coresight.txt          |  28 +
 Documentation/trace/coresight.txt                  |  37 +-
 Documentation/trace/stm.txt                        |   6 +
 drivers/hwtracing/coresight/Kconfig                |  11 +
 drivers/hwtracing/coresight/Makefile               |   1 +
 drivers/hwtracing/coresight/coresight-stm.c        | 972 +++++++++++++++++++++
 drivers/hwtracing/stm/core.c                       |  29 +-
 drivers/hwtracing/stm/policy.c                     |  46 +-
 include/linux/coresight-stm.h                      |   6 +
 include/linux/stm.h                                |  11 +
 include/uapi/linux/coresight-stm.h                 |  12 +
 include/uapi/linux/stm.h                           |   1 +
 13 files changed, 1196 insertions(+), 17 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-stm
 create mode 100644 drivers/hwtracing/coresight/coresight-stm.c
 create mode 100644 include/linux/coresight-stm.h
 create mode 100644 include/uapi/linux/coresight-stm.h

-- 
1.9.1

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH V2 0/6] Introduce CoreSight STM support
@ 2016-02-03  8:15 ` Chunyan Zhang
  0 siblings, 0 replies; 68+ messages in thread
From: Chunyan Zhang @ 2016-02-03  8:15 UTC (permalink / raw)
  To: linux-arm-kernel

This patchset adds support for CoreSight STM IP block. It also makes
a little modification to the generic STM framework to cover the
CoreSight STM requirements. Full description follows the changelog.

Changes from v1:
 - Added a definition of coresight_simple_func() in CS-STM driver to
   avoid the kbuild test robot error for the time being.  This
   modification will be removed when merging the code in which the
   coresight_simple_func() has been moved to the header file.
 - Calculate the channel number according to the channel memory space size.

Thanks,
Chunyan

Chunyan Zhang (3):
  stm class: Add ioctl get_options interface
  stm class: adds a loop to extract the first valid STM device name
  Documentations: Add explanations of the case for non-configurable
    masters

Mathieu Poirier (2):
  stm class: provision for statically assigned masterIDs
  coresight-stm: Bindings for System Trace Macrocell

Pratik Patel (1):
  coresight-stm: adding driver for CoreSight STM component

 .../ABI/testing/sysfs-bus-coresight-devices-stm    |  53 ++
 .../devicetree/bindings/arm/coresight.txt          |  28 +
 Documentation/trace/coresight.txt                  |  37 +-
 Documentation/trace/stm.txt                        |   6 +
 drivers/hwtracing/coresight/Kconfig                |  11 +
 drivers/hwtracing/coresight/Makefile               |   1 +
 drivers/hwtracing/coresight/coresight-stm.c        | 972 +++++++++++++++++++++
 drivers/hwtracing/stm/core.c                       |  29 +-
 drivers/hwtracing/stm/policy.c                     |  46 +-
 include/linux/coresight-stm.h                      |   6 +
 include/linux/stm.h                                |  11 +
 include/uapi/linux/coresight-stm.h                 |  12 +
 include/uapi/linux/stm.h                           |   1 +
 13 files changed, 1196 insertions(+), 17 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-stm
 create mode 100644 drivers/hwtracing/coresight/coresight-stm.c
 create mode 100644 include/linux/coresight-stm.h
 create mode 100644 include/uapi/linux/coresight-stm.h

-- 
1.9.1

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH V2 1/6] stm class: Add ioctl get_options interface
  2016-02-03  8:15 ` Chunyan Zhang
@ 2016-02-03  8:15   ` Chunyan Zhang
  -1 siblings, 0 replies; 68+ messages in thread
From: Chunyan Zhang @ 2016-02-03  8:15 UTC (permalink / raw)
  To: mathieu.poirier, alexander.shishkin
  Cc: robh, broonie, pratikp, nicolas.guion, corbet, mark.rutland,
	mike.leach, tor, al.grant, zhang.lyra, linux-kernel,
	linux-arm-kernel, linux-api, linux-doc

There is already an interface of set_options, but no get_options yet.
Before setting any options, one would may want to see the current
status of that option by means of get_options interface. This
interface has been used in CoreSight STM driver.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 drivers/hwtracing/stm/core.c | 11 +++++++++++
 include/linux/stm.h          |  3 +++
 include/uapi/linux/stm.h     |  1 +
 3 files changed, 15 insertions(+)

diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index b6445d9..86bb4e3 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -571,6 +571,17 @@ stm_char_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 						    options);
 
 		break;
+
+	case STP_GET_OPTIONS:
+		if (stm_data->get_options)
+			err = stm_data->get_options(stm_data,
+						    stmf->output.master,
+						    stmf->output.channel,
+						    stmf->output.nr_chans,
+						    &options);
+
+		return copy_to_user((void __user *)arg, &options, sizeof(u64));
+
 	default:
 		break;
 	}
diff --git a/include/linux/stm.h b/include/linux/stm.h
index 9d0083d..f351d62 100644
--- a/include/linux/stm.h
+++ b/include/linux/stm.h
@@ -88,6 +88,9 @@ struct stm_data {
 	long			(*set_options)(struct stm_data *, unsigned int,
 					       unsigned int, unsigned int,
 					       unsigned long);
+	long			(*get_options)(struct stm_data *, unsigned int,
+					       unsigned int, unsigned int,
+					       u64 *);
 };
 
 int stm_register_device(struct device *parent, struct stm_data *stm_data,
diff --git a/include/uapi/linux/stm.h b/include/uapi/linux/stm.h
index 626a8d3..0dab16e 100644
--- a/include/uapi/linux/stm.h
+++ b/include/uapi/linux/stm.h
@@ -46,5 +46,6 @@ struct stp_policy_id {
 #define STP_POLICY_ID_SET	_IOWR('%', 0, struct stp_policy_id)
 #define STP_POLICY_ID_GET	_IOR('%', 1, struct stp_policy_id)
 #define STP_SET_OPTIONS		_IOW('%', 2, __u64)
+#define STP_GET_OPTIONS		_IOR('%', 3, __u64)
 
 #endif /* _UAPI_LINUX_STM_H */
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 68+ messages in thread
* [PATCH V2 1/6] stm class: Add ioctl get_options interface
@ 2016-02-03  8:15   ` Chunyan Zhang
  0 siblings, 0 replies; 68+ messages in thread
From: Chunyan Zhang @ 2016-02-03  8:15 UTC (permalink / raw)
  To: linux-arm-kernel

There is already an interface of set_options, but no get_options yet.
Before setting any options, one would may want to see the current
status of that option by means of get_options interface. This
interface has been used in CoreSight STM driver.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 drivers/hwtracing/stm/core.c | 11 +++++++++++
 include/linux/stm.h          |  3 +++
 include/uapi/linux/stm.h     |  1 +
 3 files changed, 15 insertions(+)

diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index b6445d9..86bb4e3 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -571,6 +571,17 @@ stm_char_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 						    options);
 
 		break;
+
+	case STP_GET_OPTIONS:
+		if (stm_data->get_options)
+			err = stm_data->get_options(stm_data,
+						    stmf->output.master,
+						    stmf->output.channel,
+						    stmf->output.nr_chans,
+						    &options);
+
+		return copy_to_user((void __user *)arg, &options, sizeof(u64));
+
 	default:
 		break;
 	}
diff --git a/include/linux/stm.h b/include/linux/stm.h
index 9d0083d..f351d62 100644
--- a/include/linux/stm.h
+++ b/include/linux/stm.h
@@ -88,6 +88,9 @@ struct stm_data {
 	long			(*set_options)(struct stm_data *, unsigned int,
 					       unsigned int, unsigned int,
 					       unsigned long);
+	long			(*get_options)(struct stm_data *, unsigned int,
+					       unsigned int, unsigned int,
+					       u64 *);
 };
 
 int stm_register_device(struct device *parent, struct stm_data *stm_data,
diff --git a/include/uapi/linux/stm.h b/include/uapi/linux/stm.h
index 626a8d3..0dab16e 100644
--- a/include/uapi/linux/stm.h
+++ b/include/uapi/linux/stm.h
@@ -46,5 +46,6 @@ struct stp_policy_id {
 #define STP_POLICY_ID_SET	_IOWR('%', 0, struct stp_policy_id)
 #define STP_POLICY_ID_GET	_IOR('%', 1, struct stp_policy_id)
 #define STP_SET_OPTIONS		_IOW('%', 2, __u64)
+#define STP_GET_OPTIONS		_IOR('%', 3, __u64)
 
 #endif /* _UAPI_LINUX_STM_H */
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 68+ messages in thread
* [PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name
  2016-02-03  8:15 ` Chunyan Zhang
@ 2016-02-03  8:15   ` Chunyan Zhang
  -1 siblings, 0 replies; 68+ messages in thread
From: Chunyan Zhang @ 2016-02-03  8:15 UTC (permalink / raw)
  To: mathieu.poirier, alexander.shishkin
  Cc: robh, broonie, pratikp, nicolas.guion, corbet, mark.rutland,
	mike.leach, tor, al.grant, zhang.lyra, linux-kernel,
	linux-arm-kernel, linux-api, linux-doc

The node name of STM master management policy is a concatenation of an
STM device name to which this policy applies and following an arbitrary
string, these two strings are concatenated with a dot.

This patch adds a loop for extracting the STM device name when an
arbitrary number of dot(s) are found in this STM device name.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 drivers/hwtracing/stm/policy.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
index 11ab6d0..691686e 100644
--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -321,21 +321,26 @@ stp_policies_make(struct config_group *group, const char *name)
 	/*
 	 * node must look like <device_name>.<policy_name>, where
 	 * <device_name> is the name of an existing stm device and
-	 * <policy_name> is an arbitrary string
+	 * <policy_name> is an arbitrary string, when an arbitrary
+	 * number of dot(s) are found in the <device_name>, the
+	 * first matched STM device name would be extracted.
 	 */
-	p = strchr(devname, '.');
-	if (!p) {
-		kfree(devname);
-		return ERR_PTR(-EINVAL);
-	}
+	for (p = devname; ; p++) {
+		p = strchr(p, '.');
+		if (!p) {
+			kfree(devname);
+			return ERR_PTR(-EINVAL);
+		}
 
-	*p++ = '\0';
+		*p = '\0';
 
-	stm = stm_find_device(devname);
-	kfree(devname);
+		stm = stm_find_device(devname);
+		if (stm)
+			break;
+		*p = '.';
+	};
 
-	if (!stm)
-		return ERR_PTR(-ENODEV);
+	kfree(devname);
 
 	mutex_lock(&stm->policy_mutex);
 	if (stm->policy) {
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 68+ messages in thread
* [PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name
@ 2016-02-03  8:15   ` Chunyan Zhang
  0 siblings, 0 replies; 68+ messages in thread
From: Chunyan Zhang @ 2016-02-03  8:15 UTC (permalink / raw)
  To: linux-arm-kernel

The node name of STM master management policy is a concatenation of an
STM device name to which this policy applies and following an arbitrary
string, these two strings are concatenated with a dot.

This patch adds a loop for extracting the STM device name when an
arbitrary number of dot(s) are found in this STM device name.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 drivers/hwtracing/stm/policy.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
index 11ab6d0..691686e 100644
--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -321,21 +321,26 @@ stp_policies_make(struct config_group *group, const char *name)
 	/*
 	 * node must look like <device_name>.<policy_name>, where
 	 * <device_name> is the name of an existing stm device and
-	 * <policy_name> is an arbitrary string
+	 * <policy_name> is an arbitrary string, when an arbitrary
+	 * number of dot(s) are found in the <device_name>, the
+	 * first matched STM device name would be extracted.
 	 */
-	p = strchr(devname, '.');
-	if (!p) {
-		kfree(devname);
-		return ERR_PTR(-EINVAL);
-	}
+	for (p = devname; ; p++) {
+		p = strchr(p, '.');
+		if (!p) {
+			kfree(devname);
+			return ERR_PTR(-EINVAL);
+		}
 
-	*p++ = '\0';
+		*p = '\0';
 
-	stm = stm_find_device(devname);
-	kfree(devname);
+		stm = stm_find_device(devname);
+		if (stm)
+			break;
+		*p = '.';
+	};
 
-	if (!stm)
-		return ERR_PTR(-ENODEV);
+	kfree(devname);
 
 	mutex_lock(&stm->policy_mutex);
 	if (stm->policy) {
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 68+ messages in thread
* [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
  2016-02-03  8:15 ` Chunyan Zhang
@ 2016-02-03  8:15   ` Chunyan Zhang
  -1 siblings, 0 replies; 68+ messages in thread
From: Chunyan Zhang @ 2016-02-03  8:15 UTC (permalink / raw)
  To: mathieu.poirier, alexander.shishkin
  Cc: robh, broonie, pratikp, nicolas.guion, corbet, mark.rutland,
	mike.leach, tor, al.grant, zhang.lyra, linux-kernel,
	linux-arm-kernel, linux-api, linux-doc

From: Mathieu Poirier <mathieu.poirier@linaro.org>

Some architecture like ARM assign masterIDs statically at the HW design
phase, making masterID manipulation in the generic STM core irrelevant.

This patch adds a new 'mstatic' flag to struct stm_data that tells the
core that this specific STM device doesn't need explicit masterID
management.  In the core sw_start/end of masterID are set to '1',
i.e there is only one masterID to deal with.

Also this patch depends on [1], so that the number of masterID
is '1' too.

Finally the lower and upper bound for masterIDs as presented
in ($SYSFS)/class/stm/XYZ.stm/masters and
($SYSFS)/../stp-policy/XYZ.stm.my_policy/some_device/masters
are set to '-1'.  That way users can't confuse them with
architecture where masterID management is required (where any
other value would be valid).

[1] https://lkml.org/lkml/2015/12/22/348

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 drivers/hwtracing/stm/core.c   | 18 ++++++++++++++++--
 drivers/hwtracing/stm/policy.c | 19 +++++++++++++++++--
 include/linux/stm.h            |  8 ++++++++
 3 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index 86bb4e3..cd3dc19 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -44,9 +44,15 @@ static ssize_t masters_show(struct device *dev,
 			    char *buf)
 {
 	struct stm_device *stm = to_stm_device(dev);
-	int ret;
+	int ret, sw_start, sw_end;
+
+	sw_start = stm->data->sw_start;
+	sw_end = stm->data->sw_end;
 
-	ret = sprintf(buf, "%u %u\n", stm->data->sw_start, stm->data->sw_end);
+	if (stm->data->mstatic)
+		sw_start = sw_end = STM_STATIC_MASTERID;
+
+	ret = sprintf(buf, "%d %d\n", sw_start, sw_end);
 
 	return ret;
 }
@@ -629,7 +635,15 @@ int stm_register_device(struct device *parent, struct stm_data *stm_data,
 	if (!stm_data->packet || !stm_data->sw_nchannels)
 		return -EINVAL;
 
+	/*
+	 * MasterIDs are statically set in HW. As such the core is
+	 * using a single master for interaction with this device.
+	 */
+	if (stm_data->mstatic)
+		stm_data->sw_start = stm_data->sw_end = 1;
+
 	nmasters = stm_data->sw_end - stm_data->sw_start;
+
 	stm = kzalloc(sizeof(*stm) + nmasters * sizeof(void *), GFP_KERNEL);
 	if (!stm)
 		return -ENOMEM;
diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
index 691686e..7e70ca2 100644
--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -80,10 +80,17 @@ static ssize_t
 stp_policy_node_masters_show(struct config_item *item, char *page)
 {
 	struct stp_policy_node *policy_node = to_stp_policy_node(item);
+	struct stm_device *stm = policy_node->policy->stm;
+	int first_master, last_master;
 	ssize_t count;
 
-	count = sprintf(page, "%u %u\n", policy_node->first_master,
-			policy_node->last_master);
+	first_master = policy_node->first_master;
+	last_master = policy_node->last_master;
+
+	if (stm && stm->data->mstatic)
+		first_master = last_master = STM_STATIC_MASTERID;
+
+	count = sprintf(page, "%d %d\n", first_master, last_master);
 
 	return count;
 }
@@ -106,6 +113,13 @@ stp_policy_node_masters_store(struct config_item *item, const char *page,
 	if (!stm)
 		goto unlock;
 
+	/*
+	 * masterIDs are statically allocated in HW, no point in trying to
+	 * change their values.
+	 */
+	if (stm->data->mstatic)
+		goto unlock;
+
 	/* must be within [sw_start..sw_end], which is an inclusive range */
 	if (first > INT_MAX || last > INT_MAX || first > last ||
 	    first < stm->data->sw_start ||
@@ -325,6 +339,7 @@ stp_policies_make(struct config_group *group, const char *name)
 	 * number of dot(s) are found in the <device_name>, the
 	 * first matched STM device name would be extracted.
 	 */
+
 	for (p = devname; ; p++) {
 		p = strchr(p, '.');
 		if (!p) {
diff --git a/include/linux/stm.h b/include/linux/stm.h
index f351d62..c9712a7 100644
--- a/include/linux/stm.h
+++ b/include/linux/stm.h
@@ -18,6 +18,11 @@
 #include <linux/device.h>
 
 /**
+ * The masterIDs are statically set in hardware and can't be queried
+ */
+#define STM_STATIC_MASTERID -1
+
+/**
  * enum stp_packet_type - STP packets that an STM driver sends
  */
 enum stp_packet_type {
@@ -46,6 +51,8 @@ struct stm_device;
  * struct stm_data - STM device description and callbacks
  * @name:		device name
  * @stm:		internal structure, only used by stm class code
+ * @mstatic:		true if masterIDs are assigned in HW.  If so @sw_start
+ *			and @sw_end are set to '1' by the core.
  * @sw_start:		first STP master available to software
  * @sw_end:		last STP master available to software
  * @sw_nchannels:	number of STP channels per master
@@ -71,6 +78,7 @@ struct stm_device;
 struct stm_data {
 	const char		*name;
 	struct stm_device	*stm;
+	bool			mstatic;
 	unsigned int		sw_start;
 	unsigned int		sw_end;
 	unsigned int		sw_nchannels;
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 68+ messages in thread
* [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-03  8:15   ` Chunyan Zhang
  0 siblings, 0 replies; 68+ messages in thread
From: Chunyan Zhang @ 2016-02-03  8:15 UTC (permalink / raw)
  To: linux-arm-kernel

From: Mathieu Poirier <mathieu.poirier@linaro.org>

Some architecture like ARM assign masterIDs statically at the HW design
phase, making masterID manipulation in the generic STM core irrelevant.

This patch adds a new 'mstatic' flag to struct stm_data that tells the
core that this specific STM device doesn't need explicit masterID
management.  In the core sw_start/end of masterID are set to '1',
i.e there is only one masterID to deal with.

Also this patch depends on [1], so that the number of masterID
is '1' too.

Finally the lower and upper bound for masterIDs as presented
in ($SYSFS)/class/stm/XYZ.stm/masters and
($SYSFS)/../stp-policy/XYZ.stm.my_policy/some_device/masters
are set to '-1'.  That way users can't confuse them with
architecture where masterID management is required (where any
other value would be valid).

[1] https://lkml.org/lkml/2015/12/22/348

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 drivers/hwtracing/stm/core.c   | 18 ++++++++++++++++--
 drivers/hwtracing/stm/policy.c | 19 +++++++++++++++++--
 include/linux/stm.h            |  8 ++++++++
 3 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index 86bb4e3..cd3dc19 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -44,9 +44,15 @@ static ssize_t masters_show(struct device *dev,
 			    char *buf)
 {
 	struct stm_device *stm = to_stm_device(dev);
-	int ret;
+	int ret, sw_start, sw_end;
+
+	sw_start = stm->data->sw_start;
+	sw_end = stm->data->sw_end;
 
-	ret = sprintf(buf, "%u %u\n", stm->data->sw_start, stm->data->sw_end);
+	if (stm->data->mstatic)
+		sw_start = sw_end = STM_STATIC_MASTERID;
+
+	ret = sprintf(buf, "%d %d\n", sw_start, sw_end);
 
 	return ret;
 }
@@ -629,7 +635,15 @@ int stm_register_device(struct device *parent, struct stm_data *stm_data,
 	if (!stm_data->packet || !stm_data->sw_nchannels)
 		return -EINVAL;
 
+	/*
+	 * MasterIDs are statically set in HW. As such the core is
+	 * using a single master for interaction with this device.
+	 */
+	if (stm_data->mstatic)
+		stm_data->sw_start = stm_data->sw_end = 1;
+
 	nmasters = stm_data->sw_end - stm_data->sw_start;
+
 	stm = kzalloc(sizeof(*stm) + nmasters * sizeof(void *), GFP_KERNEL);
 	if (!stm)
 		return -ENOMEM;
diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
index 691686e..7e70ca2 100644
--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -80,10 +80,17 @@ static ssize_t
 stp_policy_node_masters_show(struct config_item *item, char *page)
 {
 	struct stp_policy_node *policy_node = to_stp_policy_node(item);
+	struct stm_device *stm = policy_node->policy->stm;
+	int first_master, last_master;
 	ssize_t count;
 
-	count = sprintf(page, "%u %u\n", policy_node->first_master,
-			policy_node->last_master);
+	first_master = policy_node->first_master;
+	last_master = policy_node->last_master;
+
+	if (stm && stm->data->mstatic)
+		first_master = last_master = STM_STATIC_MASTERID;
+
+	count = sprintf(page, "%d %d\n", first_master, last_master);
 
 	return count;
 }
@@ -106,6 +113,13 @@ stp_policy_node_masters_store(struct config_item *item, const char *page,
 	if (!stm)
 		goto unlock;
 
+	/*
+	 * masterIDs are statically allocated in HW, no point in trying to
+	 * change their values.
+	 */
+	if (stm->data->mstatic)
+		goto unlock;
+
 	/* must be within [sw_start..sw_end], which is an inclusive range */
 	if (first > INT_MAX || last > INT_MAX || first > last ||
 	    first < stm->data->sw_start ||
@@ -325,6 +339,7 @@ stp_policies_make(struct config_group *group, const char *name)
 	 * number of dot(s) are found in the <device_name>, the
 	 * first matched STM device name would be extracted.
 	 */
+
 	for (p = devname; ; p++) {
 		p = strchr(p, '.');
 		if (!p) {
diff --git a/include/linux/stm.h b/include/linux/stm.h
index f351d62..c9712a7 100644
--- a/include/linux/stm.h
+++ b/include/linux/stm.h
@@ -18,6 +18,11 @@
 #include <linux/device.h>
 
 /**
+ * The masterIDs are statically set in hardware and can't be queried
+ */
+#define STM_STATIC_MASTERID -1
+
+/**
  * enum stp_packet_type - STP packets that an STM driver sends
  */
 enum stp_packet_type {
@@ -46,6 +51,8 @@ struct stm_device;
  * struct stm_data - STM device description and callbacks
  * @name:		device name
  * @stm:		internal structure, only used by stm class code
+ * @mstatic:		true if masterIDs are assigned in HW.  If so @sw_start
+ *			and @sw_end are set to '1' by the core.
  * @sw_start:		first STP master available to software
  * @sw_end:		last STP master available to software
  * @sw_nchannels:	number of STP channels per master
@@ -71,6 +78,7 @@ struct stm_device;
 struct stm_data {
 	const char		*name;
 	struct stm_device	*stm;
+	bool			mstatic;
 	unsigned int		sw_start;
 	unsigned int		sw_end;
 	unsigned int		sw_nchannels;
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 68+ messages in thread
* [PATCH V2 4/6] Documentations: Add explanations of the case for non-configurable masters
  2016-02-03  8:15 ` Chunyan Zhang
@ 2016-02-03  8:15   ` Chunyan Zhang
  -1 siblings, 0 replies; 68+ messages in thread
From: Chunyan Zhang @ 2016-02-03  8:15 UTC (permalink / raw)
  To: mathieu.poirier, alexander.shishkin
  Cc: robh, broonie, pratikp, nicolas.guion, corbet, mark.rutland,
	mike.leach, tor, al.grant, zhang.lyra, linux-kernel,
	linux-arm-kernel, linux-api, linux-doc

For some STM hardware (e.g. ARM CoreSight STM), the masterID associated
to a source is set at the hardware level and not user configurable.
Since the masterID information isn't available to SW, introducing
a new value of -1 to reflect this reality.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 Documentation/trace/stm.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/trace/stm.txt b/Documentation/trace/stm.txt
index ea035f9..f03bc2b 100644
--- a/Documentation/trace/stm.txt
+++ b/Documentation/trace/stm.txt
@@ -47,6 +47,12 @@ through 127 in it. Now, any producer (trace source) identifying itself
 with "user" identification string will be allocated a master and
 channel from within these ranges.
 
+$ cat /config/stp-policy/dummy_stm.my-policy/user/masters
+-1 -1
+
+Would indicate the masters for this rule are set in hardware and
+not configurable in user space.
+
 These rules can be nested, for example, one can define a rule "dummy"
 under "user" directory from the example above and this new rule will
 be used for trace sources with the id string of "user/dummy".
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 68+ messages in thread
* [PATCH V2 4/6] Documentations: Add explanations of the case for non-configurable masters
@ 2016-02-03  8:15   ` Chunyan Zhang
  0 siblings, 0 replies; 68+ messages in thread
From: Chunyan Zhang @ 2016-02-03  8:15 UTC (permalink / raw)
  To: linux-arm-kernel

For some STM hardware (e.g. ARM CoreSight STM), the masterID associated
to a source is set at the hardware level and not user configurable.
Since the masterID information isn't available to SW, introducing
a new value of -1 to reflect this reality.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 Documentation/trace/stm.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/trace/stm.txt b/Documentation/trace/stm.txt
index ea035f9..f03bc2b 100644
--- a/Documentation/trace/stm.txt
+++ b/Documentation/trace/stm.txt
@@ -47,6 +47,12 @@ through 127 in it. Now, any producer (trace source) identifying itself
 with "user" identification string will be allocated a master and
 channel from within these ranges.
 
+$ cat /config/stp-policy/dummy_stm.my-policy/user/masters
+-1 -1
+
+Would indicate the masters for this rule are set in hardware and
+not configurable in user space.
+
 These rules can be nested, for example, one can define a rule "dummy"
 under "user" directory from the example above and this new rule will
 be used for trace sources with the id string of "user/dummy".
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 68+ messages in thread
* [PATCH V2 5/6] coresight-stm: Bindings for System Trace Macrocell
  2016-02-03  8:15 ` Chunyan Zhang
@ 2016-02-03  8:15   ` Chunyan Zhang
  -1 siblings, 0 replies; 68+ messages in thread
From: Chunyan Zhang @ 2016-02-03  8:15 UTC (permalink / raw)
  To: mathieu.poirier, alexander.shishkin
  Cc: robh, broonie, pratikp, nicolas.guion, corbet, mark.rutland,
	mike.leach, tor, al.grant, zhang.lyra, linux-kernel,
	linux-arm-kernel, linux-api, linux-doc

From: Mathieu Poirier <mathieu.poirier@linaro.org>

The System Trace Macrocell (STM) is an IP block falling under the
CoreSight umbrella.  It's main purpose it so expose stimulus channels
to any system component for the purpose of information logging.

Bindings for this IP block adds a couple of items to the current
mandatory definition for CoreSight components.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 .../devicetree/bindings/arm/coresight.txt          | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
index 62938eb..93147c0c 100644
--- a/Documentation/devicetree/bindings/arm/coresight.txt
+++ b/Documentation/devicetree/bindings/arm/coresight.txt
@@ -19,6 +19,7 @@ its hardware characteristcs.
 		- "arm,coresight-etm3x", "arm,primecell";
 		- "arm,coresight-etm4x", "arm,primecell";
 		- "qcom,coresight-replicator1x", "arm,primecell";
+		- "arm,coresight-stm", "arm,primecell"; [1]
 
 	* reg: physical base address and length of the register
 	  set(s) of the component.
@@ -36,6 +37,14 @@ its hardware characteristcs.
 	  layout using the generic DT graph presentation found in
 	  "bindings/graph.txt".
 
+* Additional required properties for System Trace Macrocells (STM):
+	* reg: along with the physical base address and length of the register
+	  set as described above, another entry is required to describe the
+	  mapping of the extended stimulus port area.
+
+	* reg-names: the only acceptable values are "stm-base" and
+	  "stm-stimulus-base", each corresponding to the areas defined in "reg".
+
 * Required properties for devices that don't show up on the AMBA bus, such as
   non-configurable replicators:
 
@@ -202,3 +211,22 @@ Example:
 			};
 		};
 	};
+
+4. STM
+	stm@20100000 {
+		compatible = "arm,coresight-stm", "arm,primecell";
+		reg = <0 0x20100000 0 0x1000>,
+		      <0 0x28000000 0 0x180000>;
+		reg-names = "stm-base", "stm-stimulus-base";
+
+		clocks = <&soc_smc50mhz>;
+		clock-names = "apb_pclk";
+		port {
+			stm_out_port: endpoint {
+				remote-endpoint = <&main_funnel_in_port2>;
+			};
+		};
+	};
+
+[1]. There is currently two version of STM: STM32 and STM500.  Both
+have the same HW interface and as such don't need an explicit binding name.
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 68+ messages in thread
* [PATCH V2 5/6] coresight-stm: Bindings for System Trace Macrocell
@ 2016-02-03  8:15   ` Chunyan Zhang
  0 siblings, 0 replies; 68+ messages in thread
From: Chunyan Zhang @ 2016-02-03  8:15 UTC (permalink / raw)
  To: linux-arm-kernel

From: Mathieu Poirier <mathieu.poirier@linaro.org>

The System Trace Macrocell (STM) is an IP block falling under the
CoreSight umbrella.  It's main purpose it so expose stimulus channels
to any system component for the purpose of information logging.

Bindings for this IP block adds a couple of items to the current
mandatory definition for CoreSight components.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 .../devicetree/bindings/arm/coresight.txt          | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
index 62938eb..93147c0c 100644
--- a/Documentation/devicetree/bindings/arm/coresight.txt
+++ b/Documentation/devicetree/bindings/arm/coresight.txt
@@ -19,6 +19,7 @@ its hardware characteristcs.
 		- "arm,coresight-etm3x", "arm,primecell";
 		- "arm,coresight-etm4x", "arm,primecell";
 		- "qcom,coresight-replicator1x", "arm,primecell";
+		- "arm,coresight-stm", "arm,primecell"; [1]
 
 	* reg: physical base address and length of the register
 	  set(s) of the component.
@@ -36,6 +37,14 @@ its hardware characteristcs.
 	  layout using the generic DT graph presentation found in
 	  "bindings/graph.txt".
 
+* Additional required properties for System Trace Macrocells (STM):
+	* reg: along with the physical base address and length of the register
+	  set as described above, another entry is required to describe the
+	  mapping of the extended stimulus port area.
+
+	* reg-names: the only acceptable values are "stm-base" and
+	  "stm-stimulus-base", each corresponding to the areas defined in "reg".
+
 * Required properties for devices that don't show up on the AMBA bus, such as
   non-configurable replicators:
 
@@ -202,3 +211,22 @@ Example:
 			};
 		};
 	};
+
+4. STM
+	stm at 20100000 {
+		compatible = "arm,coresight-stm", "arm,primecell";
+		reg = <0 0x20100000 0 0x1000>,
+		      <0 0x28000000 0 0x180000>;
+		reg-names = "stm-base", "stm-stimulus-base";
+
+		clocks = <&soc_smc50mhz>;
+		clock-names = "apb_pclk";
+		port {
+			stm_out_port: endpoint {
+				remote-endpoint = <&main_funnel_in_port2>;
+			};
+		};
+	};
+
+[1]. There is currently two version of STM: STM32 and STM500.  Both
+have the same HW interface and as such don't need an explicit binding name.
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 68+ messages in thread
* [PATCH V2 6/6] coresight-stm: adding driver for CoreSight STM component
  2016-02-03  8:15 ` Chunyan Zhang
@ 2016-02-03  8:15   ` Chunyan Zhang
  -1 siblings, 0 replies; 68+ messages in thread
From: Chunyan Zhang @ 2016-02-03  8:15 UTC (permalink / raw)
  To: mathieu.poirier, alexander.shishkin
  Cc: robh, broonie, pratikp, nicolas.guion, corbet, mark.rutland,
	mike.leach, tor, al.grant, zhang.lyra, linux-kernel,
	linux-arm-kernel, linux-api, linux-doc

From: Pratik Patel <pratikp@codeaurora.org>

This driver adds support for the STM CoreSight IP block, allowing any
system compoment (HW or SW) to log and aggregate messages via a
single entity.

The CoreSight STM exposes an application defined number of channels
called stimulus port. Configuration is done using entries in sysfs
and channels made available to userspace via configfs.

Signed-off-by: Pratik Patel <pratikp@codeaurora.org>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 .../ABI/testing/sysfs-bus-coresight-devices-stm    |  53 ++
 Documentation/trace/coresight.txt                  |  37 +-
 drivers/hwtracing/coresight/Kconfig                |  11 +
 drivers/hwtracing/coresight/Makefile               |   1 +
 drivers/hwtracing/coresight/coresight-stm.c        | 972 +++++++++++++++++++++
 include/linux/coresight-stm.h                      |   6 +
 include/uapi/linux/coresight-stm.h                 |  12 +
 7 files changed, 1090 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-stm
 create mode 100644 drivers/hwtracing/coresight/coresight-stm.c
 create mode 100644 include/linux/coresight-stm.h
 create mode 100644 include/uapi/linux/coresight-stm.h

diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-stm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-stm
new file mode 100644
index 0000000..a1d7022
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-stm
@@ -0,0 +1,53 @@
+What:		/sys/bus/coresight/devices/<memory_map>.stm/enable_source
+Date:		February 2016
+KernelVersion:	4.5
+Contact:	Mathieu Poirier <mathieu.poirier@linaro.org>
+Description:	(RW) Enable/disable tracing on this specific trace macrocell.
+		Enabling the trace macrocell implies it has been configured
+		properly and a sink has been identidifed for it.  The path
+		of coresight components linking the source to the sink is
+		configured and managed automatically by the coresight framework.
+
+What:		/sys/bus/coresight/devices/<memory_map>.stm/hwevent_enable
+Date:		February 2016
+KernelVersion:	4.5
+Contact:	Mathieu Poirier <mathieu.poirier@linaro.org>
+Description:	(RW) Provides access to the HW event enable register, used in
+		conjunction with HW event bank select register.
+
+What:		/sys/bus/coresight/devices/<memory_map>.stm/hwevent_select
+Date:		February 2016
+KernelVersion:	4.5
+Contact:	Mathieu Poirier <mathieu.poirier@linaro.org>
+Description:	(RW) Gives access to the HW event block select register
+		(STMHEBSR) in order to configure up to 256 channels.  Used in
+		conjunction with "hwevent_enable" register as described above.
+
+What:		/sys/bus/coresight/devices/<memory_map>.stm/port_enable
+Date:		February 2016
+KernelVersion:	4.5
+Contact:	Mathieu Poirier <mathieu.poirier@linaro.org>
+Description:	(RW) Provides access to the stimlus port enable register
+		(STMSPER).  Used in conjunction with "port_select" described
+		below.
+
+What:		/sys/bus/coresight/devices/<memory_map>.stm/port_select
+Date:		February 2016
+KernelVersion:	4.5
+Contact:	Mathieu Poirier <mathieu.poirier@linaro.org>
+Description:	(RW) Used to determine which bank of stimulus port bit in
+		register STMSPER (see above) apply to.
+
+What:		/sys/bus/coresight/devices/<memory_map>.stm/status
+Date:		February 2016
+KernelVersion:	4.5
+Contact:	Mathieu Poirier <mathieu.poirier@linaro.org>
+Description:	(R) List various control and status registers.  The specific
+		layout and content is driver specific.
+
+What:		/sys/bus/coresight/devices/<memory_map>.stm/traceid
+Date:		February 2016
+KernelVersion:	4.5
+Contact:	Mathieu Poirier <mathieu.poirier@linaro.org>
+Description:	(RW) Holds the trace ID that will appear in the trace stream
+		coming from this trace entity.
diff --git a/Documentation/trace/coresight.txt b/Documentation/trace/coresight.txt
index 0a5c329..a33c88c 100644
--- a/Documentation/trace/coresight.txt
+++ b/Documentation/trace/coresight.txt
@@ -190,8 +190,8 @@ expected to be accessed and controlled using those entries.
 Last but not least, "struct module *owner" is expected to be set to reflect
 the information carried in "THIS_MODULE".
 
-How to use
-----------
+How to use the tracer modules
+-----------------------------
 
 Before trace collection can start, a coresight sink needs to be identify.
 There is no limit on the amount of sinks (nor sources) that can be enabled at
@@ -297,3 +297,36 @@ Info                                    Tracing enabled
 Instruction     13570831        0x8026B584      E28DD00C        false   ADD      sp,sp,#0xc
 Instruction     0       0x8026B588      E8BD8000        true    LDM      sp!,{pc}
 Timestamp                                       Timestamp: 17107041535
+
+How to use the STM module
+-------------------------
+
+Using the System Trace Macrocell module is the same as the tracers - the only
+difference is that clients are driving the trace capture rather
+than the program flow through the code.
+
+As with any other CoreSight component, specifics about the STM tracer can be
+found in sysfs with more information on each entry being found in [1]:
+
+root@genericarmv8:~# ls /sys/bus/coresight/devices/20100000.stm
+enable_source   hwevent_select  port_enable     subsystem       uevent
+hwevent_enable  mgmt            port_select     traceid
+root@genericarmv8:~#
+
+Like any other source a sink needs to be identified and the STM enabled before
+being used:
+
+root@genericarmv8:~# echo 1 > /sys/bus/coresight/devices/20010000.etf/enable_sink
+root@genericarmv8:~# echo 1 > /sys/bus/coresight/devices/20100000.stm/enable_source
+
+From there user space applications can request and use channels using the devfs
+interface provided for that purpose by the generic STM API:
+
+root@genericarmv8:~# ls -l /dev/20100000.stm
+crw-------    1 root     root       10,  61 Jan  3 18:11 /dev/20100000.stm
+root@genericarmv8:~#
+
+Details on how to use the generic STM API can be found here [2].
+
+[1]. Documentation/ABI/testing/sysfs-bus-coresight-devices-stm
+[2]. Documentation/trace/stm.txt
diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index c85935f..f4a8bfa 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -77,4 +77,15 @@ config CORESIGHT_QCOM_REPLICATOR
 	  programmable ATB replicator sends the ATB trace stream from the
 	  ETB/ETF to the TPIUi and ETR.
 
+config CORESIGHT_STM
+	bool "CoreSight System Trace Macrocell driver"
+	depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
+	select CORESIGHT_LINKS_AND_SINKS
+	select STM
+	help
+	  This driver provides support for hardware assisted software
+	  instrumentation based tracing. This is primarily used for
+	  logging useful software events or data coming from various entities
+	  in the system, possibly running different OSs
+
 endif
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index 99f8e5f..1f6eaec 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o \
 obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o coresight-etm-cp14.o
 obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o
 obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
+obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
new file mode 100644
index 0000000..c84f57a
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-stm.c
@@ -0,0 +1,972 @@
+/* Copyright (c) 2015-2016, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only 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.
+ *
+ * Initial implementation by Pratik Patel
+ * (C) 2014-2015 Pratik Patel <pratikp@codeaurora.org>
+ *
+ * Serious refactoring, code cleanup and upgrading to the Coresight upstream
+ * framework by Mathieu Poirier
+ * (C) 2015-2016 Mathieu Poirier <mathieu.poirier@linaro.org>
+ *
+ * Guaranteed timing and support for various packet type coming from the
+ * generic STM API by Chunyan Zhang
+ * (C) 2015-2016 Chunyan Zhang <zhang.chunyan@linaro.org>
+ */
+#include <linux/amba/bus.h>
+#include <linux/bitmap.h>
+#include <linux/clk.h>
+#include <linux/coresight.h>
+#include <linux/coresight-stm.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/pm_runtime.h>
+#include <linux/stm.h>
+
+#include "coresight-priv.h"
+
+#define STMDMASTARTR			0xc04
+#define STMDMASTOPR			0xc08
+#define STMDMASTATR			0xc0c
+#define STMDMACTLR			0xc10
+#define STMDMAIDR			0xcfc
+#define STMHEER				0xd00
+#define STMHETER			0xd20
+#define STMHEBSR			0xd60
+#define STMHEMCR			0xd64
+#define STMHEMASTR			0xdf4
+#define STMHEFEAT1R			0xdf8
+#define STMHEIDR			0xdfc
+#define STMSPER				0xe00
+#define STMSPTER			0xe20
+#define STMPRIVMASKR			0xe40
+#define STMSPSCR			0xe60
+#define STMSPMSCR			0xe64
+#define STMSPOVERRIDER			0xe68
+#define STMSPMOVERRIDER			0xe6c
+#define STMSPTRIGCSR			0xe70
+#define STMTCSR				0xe80
+#define STMTSSTIMR			0xe84
+#define STMTSFREQR			0xe8c
+#define STMSYNCR			0xe90
+#define STMAUXCR			0xe94
+#define STMSPFEAT1R			0xea0
+#define STMSPFEAT2R			0xea4
+#define STMSPFEAT3R			0xea8
+#define STMITTRIGGER			0xee8
+#define STMITATBDATA0			0xeec
+#define STMITATBCTR2			0xef0
+#define STMITATBID			0xef4
+#define STMITATBCTR0			0xef8
+
+#define STM_32_CHANNEL			32
+#define BYTES_PER_CHANNEL		256
+#define STM_TRACE_BUF_SIZE		4096
+#define STM_SW_MASTER_END		127
+
+/* Register bit definition */
+#define STMTCSR_BUSY_BIT		23
+/* Reserve the first 10 channels for kernel usage */
+#define STM_CHANNEL_OFFSET		0
+
+enum stm_pkt_type {
+	STM_PKT_TYPE_DATA	= 0x98,
+	STM_PKT_TYPE_FLAG	= 0xE8,
+	STM_PKT_TYPE_TRIG	= 0xF8,
+};
+
+#define stm_channel_addr(drvdata, ch)	(drvdata->chs.base +	\
+					(ch * BYTES_PER_CHANNEL))
+#define stm_channel_off(type, opts)	(type & ~opts)
+
+#ifndef CONFIG_64BIT
+static inline void __raw_writeq(u64 val, volatile void __iomem *addr)
+{
+	asm volatile("strd %1, %0"
+		     : "+Qo" (*(volatile u64 __force *)addr)
+		     : "r" (val));
+}
+#undef writeq_relaxed
+#define writeq_relaxed(v, c)	__raw_writeq((__force u64) cpu_to_le64(v), c)
+#endif
+
+static int boot_nr_channel;
+
+module_param_named(
+	boot_nr_channel, boot_nr_channel, int, S_IRUGO
+);
+
+/**
+ * struct channel_space - central management entity for extended ports
+ * @base:		memory mapped base address where channels start.
+ * @guaraneed:		is the channel delivery guaranteed.
+ */
+struct channel_space {
+	void __iomem		*base;
+	unsigned long		*guaranteed;
+};
+
+/**
+ * struct stm_drvdata - specifics associated to an STM component
+ * @base:		memory mapped base address for this component.
+ * @dev:		the device entity associated to this component.
+ * @atclk:		optional clock for the core parts of the STM.
+ * @csdev:		component vitals needed by the framework.
+ * @spinlock:		only one at a time pls.
+ * @chs:		the channels accociated to this STM.
+ * @stm:		structure associated to the generic STM interface.
+ * @enable:		this STM is being used.
+ * @traceid:		value of the current ID for this component.
+ * @write_64bit:	whether this STM supports 64 bit access.
+ * @stmsper:		settings for register STMSPER.
+ * @stmspscr:		settings for register STMSPSCR.
+ * @numsp:		the total number of stimulus port support by this STM.
+ * @stmheer:		settings for register STMHEER.
+ * @stmheter:		settings for register STMHETER.
+ * @stmhebsr:		settings for register STMHEBSR.
+ */
+struct stm_drvdata {
+	void __iomem		*base;
+	struct device		*dev;
+	struct clk		*atclk;
+	struct coresight_device	*csdev;
+	spinlock_t		spinlock;
+	struct channel_space	chs;
+	struct stm_data		stm;
+	bool			enable;
+	u8			traceid;
+	u32			write_64bit;
+	u32			stmsper;
+	u32			stmspscr;
+	u32			numsp;
+	u32			stmheer;
+	u32			stmheter;
+	u32			stmhebsr;
+};
+
+static void stm_hwevent_enable_hw(struct stm_drvdata *drvdata)
+{
+	CS_UNLOCK(drvdata->base);
+
+	writel_relaxed(drvdata->stmhebsr, drvdata->base + STMHEBSR);
+	writel_relaxed(drvdata->stmheter, drvdata->base + STMHETER);
+	writel_relaxed(drvdata->stmheer, drvdata->base + STMHEER);
+	writel_relaxed(0x01 |	/* Enable HW event tracing */
+		       0x04,	/* Error detection on event tracing */
+		       drvdata->base + STMHEMCR);
+
+	CS_LOCK(drvdata->base);
+}
+
+static void stm_port_enable_hw(struct stm_drvdata *drvdata)
+{
+	CS_UNLOCK(drvdata->base);
+	/* ATB trigger enable on direct writes to TRIG locations */
+	writel_relaxed(0x10,
+		       drvdata->base + STMSPTRIGCSR);
+	writel_relaxed(drvdata->stmspscr, drvdata->base + STMSPSCR);
+	writel_relaxed(drvdata->stmsper, drvdata->base + STMSPER);
+
+	CS_LOCK(drvdata->base);
+}
+
+static void stm_enable_hw(struct stm_drvdata *drvdata)
+{
+	if (drvdata->stmheer)
+		stm_hwevent_enable_hw(drvdata);
+
+	stm_port_enable_hw(drvdata);
+
+	CS_UNLOCK(drvdata->base);
+
+	/* 4096 byte between synchronisation packets */
+	writel_relaxed(0xFFF, drvdata->base + STMSYNCR);
+	writel_relaxed((drvdata->traceid << 16 | /* trace id */
+			0x02 |			 /* timestamp enable */
+			0x01),			 /* global STM enable */
+			drvdata->base + STMTCSR);
+
+	CS_LOCK(drvdata->base);
+}
+
+static int stm_enable(struct coresight_device *csdev)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	pm_runtime_get_sync(drvdata->dev);
+
+	spin_lock(&drvdata->spinlock);
+	stm_enable_hw(drvdata);
+	drvdata->enable = true;
+	spin_unlock(&drvdata->spinlock);
+
+	dev_info(drvdata->dev, "STM tracing enabled\n");
+	return 0;
+}
+
+static void stm_hwevent_disable_hw(struct stm_drvdata *drvdata)
+{
+	CS_UNLOCK(drvdata->base);
+
+	writel_relaxed(0x0, drvdata->base + STMHEMCR);
+	writel_relaxed(0x0, drvdata->base + STMHEER);
+	writel_relaxed(0x0, drvdata->base + STMHETER);
+
+	CS_LOCK(drvdata->base);
+}
+
+static void stm_port_disable_hw(struct stm_drvdata *drvdata)
+{
+	CS_UNLOCK(drvdata->base);
+
+	writel_relaxed(0x0, drvdata->base + STMSPER);
+	writel_relaxed(0x0, drvdata->base + STMSPTRIGCSR);
+
+	CS_LOCK(drvdata->base);
+}
+
+static void stm_disable_hw(struct stm_drvdata *drvdata)
+{
+	u32 val;
+
+	CS_UNLOCK(drvdata->base);
+
+	val = readl_relaxed(drvdata->base + STMTCSR);
+	val &= ~0x1; /* clear global STM enable [0] */
+	writel_relaxed(val, drvdata->base + STMTCSR);
+
+	CS_LOCK(drvdata->base);
+
+	stm_port_disable_hw(drvdata);
+	if (drvdata->stmheer)
+		stm_hwevent_disable_hw(drvdata);
+}
+
+static void stm_disable(struct coresight_device *csdev)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	spin_lock(&drvdata->spinlock);
+	stm_disable_hw(drvdata);
+	drvdata->enable = false;
+	spin_unlock(&drvdata->spinlock);
+
+	/* Wait until the engine has completely stopped */
+	coresight_timeout(drvdata, STMTCSR, STMTCSR_BUSY_BIT, 0);
+
+	pm_runtime_put(drvdata->dev);
+
+	dev_info(drvdata->dev, "STM tracing disabled\n");
+}
+
+static int stm_trace_id(struct coresight_device *csdev)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	return drvdata->traceid;
+}
+
+static const struct coresight_ops_source stm_source_ops = {
+	.trace_id	= stm_trace_id,
+	.enable		= stm_enable,
+	.disable	= stm_disable,
+};
+
+static const struct coresight_ops stm_cs_ops = {
+	.source_ops	= &stm_source_ops,
+};
+
+static int stm_send_64bit(void *addr, const void *data, u32 size)
+{
+	u64 prepad = 0;
+	u64 postpad = 0;
+	char *pad;
+	u8 off, endoff;
+	u32 len = size;
+
+	off = (unsigned long)data & 0x7;
+
+	if (off) {
+		endoff = 8 - off;
+		pad = (char *)&prepad;
+		pad += off;
+
+		while (endoff && size) {
+			*pad++ = *(char *)data++;
+			endoff--;
+			size--;
+		}
+		writeq_relaxed(prepad, addr);
+	}
+
+	/* now we are 64bit aligned */
+	while (size >= 8) {
+		writeq_relaxed(*(u64 *)data, addr);
+		data += 8;
+		size -= 8;
+	}
+
+	endoff = 0;
+
+	if (size) {
+		endoff = 8 - (u8)size;
+		pad = (char *)&postpad;
+
+		while (size) {
+			*pad++ = *(char *)data++;
+			size--;
+		}
+		writeq_relaxed(postpad, addr);
+	}
+
+	return len + off + endoff;
+}
+
+static int stm_send(void *addr, const void *data, u32 size)
+{
+	u32 len = size;
+
+	if (((unsigned long)data & 0x1) && (size >= 1)) {
+		writeb_relaxed(*(u8 *)data, addr);
+		data++;
+		size--;
+	}
+	if (((unsigned long)data & 0x2) && (size >= 2)) {
+		writew_relaxed(*(u16 *)data, addr);
+		data += 2;
+		size -= 2;
+	}
+
+	/* now we are 32bit aligned */
+	while (size >= 4) {
+		writel_relaxed(*(u32 *)data, addr);
+		data += 4;
+		size -= 4;
+	}
+
+	if (size >= 2) {
+		writew_relaxed(*(u16 *)data, addr);
+		data += 2;
+		size -= 2;
+	}
+	if (size >= 1) {
+		writeb_relaxed(*(u8 *)data, addr);
+		data++;
+		size--;
+	}
+
+	return len;
+}
+
+static int stm_generic_link(struct stm_data *stm_data,
+			    unsigned int master,  unsigned int channel)
+{
+	struct stm_drvdata *drvdata = container_of(stm_data,
+						   struct stm_drvdata, stm);
+	if (!drvdata || !drvdata->csdev)
+		return -EINVAL;
+
+	return coresight_enable(drvdata->csdev);
+}
+
+static void stm_generic_unlink(struct stm_data *stm_data,
+			       unsigned int master,  unsigned int channel)
+{
+	struct stm_drvdata *drvdata = container_of(stm_data,
+						   struct stm_drvdata, stm);
+	if (!drvdata || !drvdata->csdev)
+		return;
+
+	stm_disable(drvdata->csdev);
+}
+
+static long stm_generic_set_options(struct stm_data *stm_data,
+				    unsigned int master,
+				    unsigned int channel,
+				    unsigned int nr_chans,
+				    unsigned long options)
+{
+	struct stm_drvdata *drvdata = container_of(stm_data,
+						   struct stm_drvdata, stm);
+	if (!(drvdata && drvdata->enable))
+		return -EINVAL;
+
+	if (channel >= drvdata->numsp)
+		return -EINVAL;
+
+	switch (options) {
+	case STM_OPTION_GUARANTEED:
+		set_bit(channel, drvdata->chs.guaranteed);
+		break;
+
+	case STM_OPTION_INVARIANT:
+		clear_bit(channel, drvdata->chs.guaranteed);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static long stm_generic_get_options(struct stm_data *stm_data,
+				    unsigned int master,
+				    unsigned int channel,
+				    unsigned int nr_chans,
+				    u64 *options)
+{
+	struct stm_drvdata *drvdata = container_of(stm_data,
+						   struct stm_drvdata, stm);
+	if (!(drvdata && drvdata->enable))
+		return -EINVAL;
+
+	if (channel >= drvdata->numsp)
+		return -EINVAL;
+
+	switch (*options) {
+	case STM_OPTION_GUARANTEED:
+		*options = test_bit(channel, drvdata->chs.guaranteed);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static ssize_t stm_generic_packet(struct stm_data *stm_data,
+				  unsigned int master,
+				  unsigned int channel,
+				  unsigned int packet,
+				  unsigned int flags,
+				  unsigned int size,
+				  const unsigned char *payload)
+{
+	unsigned int len = size;
+	unsigned long ch_addr;
+	struct stm_drvdata *drvdata = container_of(stm_data,
+						   struct stm_drvdata, stm);
+	if (!(drvdata && drvdata->enable))
+		return 0;
+
+	if (channel >= drvdata->numsp)
+		return 0;
+
+	ch_addr = (unsigned long)stm_channel_addr(drvdata, channel);
+
+	flags = (flags == STP_PACKET_TIMESTAMPED) ? STM_FLAG_TIMESTAMPED : 0;
+	flags |= test_bit(channel, drvdata->chs.guaranteed) ?
+			   STM_FLAG_GUARANTEED : 0;
+
+	switch (packet) {
+	case STP_PACKET_FLAG:
+		ch_addr |= stm_channel_off(STM_PKT_TYPE_FLAG, flags);
+
+		/* the generic STM API set a size of zero on flag packets. */
+		len = 1;
+		break;
+
+	case STP_PACKET_DATA:
+		ch_addr |= stm_channel_off(STM_PKT_TYPE_DATA, flags);
+		break;
+
+	default:
+		return 0;
+	}
+
+	if (drvdata->write_64bit)
+		return stm_send_64bit((void *)ch_addr, payload, len);
+
+	return stm_send((void *)ch_addr, payload, len);
+}
+
+static ssize_t hwevent_enable_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val = drvdata->stmheer;
+
+	return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
+}
+
+static ssize_t hwevent_enable_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t size)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+	int ret = 0;
+
+	ret = kstrtoul(buf, 16, &val);
+	if (ret)
+		return -EINVAL;
+
+	drvdata->stmheer = val;
+	/* HW event enable and trigger go hand in hand */
+	drvdata->stmheter = val;
+
+	return size;
+}
+static DEVICE_ATTR_RW(hwevent_enable);
+
+static ssize_t hwevent_select_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val = drvdata->stmhebsr;
+
+	return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
+}
+
+static ssize_t hwevent_select_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t size)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+	int ret = 0;
+
+	ret = kstrtoul(buf, 16, &val);
+	if (ret)
+		return -EINVAL;
+
+	drvdata->stmhebsr = val;
+
+	return size;
+}
+static DEVICE_ATTR_RW(hwevent_select);
+
+static ssize_t port_select_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+
+	if (!drvdata->enable) {
+		val = drvdata->stmspscr;
+	} else {
+		spin_lock(&drvdata->spinlock);
+		val = readl_relaxed(drvdata->base + STMSPSCR);
+		spin_unlock(&drvdata->spinlock);
+	}
+
+	return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
+}
+
+static ssize_t port_select_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t size)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val, stmsper;
+	int ret = 0;
+
+	ret = kstrtoul(buf, 16, &val);
+	if (ret)
+		return ret;
+
+	spin_lock(&drvdata->spinlock);
+	drvdata->stmspscr = val;
+
+	if (drvdata->enable) {
+		CS_UNLOCK(drvdata->base);
+		/* Process as per ARM's TRM recommendation */
+		stmsper = readl_relaxed(drvdata->base + STMSPER);
+		writel_relaxed(0x0, drvdata->base + STMSPER);
+		writel_relaxed(drvdata->stmspscr, drvdata->base + STMSPSCR);
+		writel_relaxed(stmsper, drvdata->base + STMSPER);
+		CS_LOCK(drvdata->base);
+	}
+	spin_unlock(&drvdata->spinlock);
+
+	return size;
+}
+static DEVICE_ATTR_RW(port_select);
+
+static ssize_t port_enable_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+
+	if (!drvdata->enable) {
+		val = drvdata->stmsper;
+	} else {
+		spin_lock(&drvdata->spinlock);
+		val = readl_relaxed(drvdata->base + STMSPER);
+		spin_unlock(&drvdata->spinlock);
+	}
+
+	return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
+}
+
+static ssize_t port_enable_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t size)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+	int ret = 0;
+
+	ret = kstrtoul(buf, 16, &val);
+	if (ret)
+		return ret;
+
+	spin_lock(&drvdata->spinlock);
+	drvdata->stmsper = val;
+
+	if (drvdata->enable) {
+		CS_UNLOCK(drvdata->base);
+		writel_relaxed(drvdata->stmsper, drvdata->base + STMSPER);
+		CS_LOCK(drvdata->base);
+	}
+	spin_unlock(&drvdata->spinlock);
+
+	return size;
+}
+static DEVICE_ATTR_RW(port_enable);
+
+static ssize_t traceid_show(struct device *dev,
+			    struct device_attribute *attr, char *buf)
+{
+	unsigned long val;
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	val = drvdata->traceid;
+	return sprintf(buf, "%#lx\n", val);
+}
+
+static ssize_t traceid_store(struct device *dev,
+			     struct device_attribute *attr,
+			     const char *buf, size_t size)
+{
+	int ret;
+	unsigned long val;
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	ret = kstrtoul(buf, 16, &val);
+	if (ret)
+		return ret;
+
+	/* traceid field is 7bit wide on STM32 */
+	drvdata->traceid = val & 0x7f;
+	return size;
+}
+static DEVICE_ATTR_RW(traceid);
+
+#define coresight_simple_func(type, name, offset)			\
+static ssize_t name##_show(struct device *_dev,				\
+			   struct device_attribute *attr, char *buf)	\
+{									\
+	type *drvdata = dev_get_drvdata(_dev->parent);			\
+	return scnprintf(buf, PAGE_SIZE, "0x%08x\n",			\
+			 readl_relaxed(drvdata->base + offset));	\
+}									\
+DEVICE_ATTR_RO(name)
+
+#define coresight_stm_simple_func(name, offset)	\
+	coresight_simple_func(struct stm_drvdata, name, offset)
+
+coresight_stm_simple_func(tcsr, STMTCSR);
+coresight_stm_simple_func(tsfreqr, STMTSFREQR);
+coresight_stm_simple_func(syncr, STMSYNCR);
+coresight_stm_simple_func(sper, STMSPER);
+coresight_stm_simple_func(spter, STMSPTER);
+coresight_stm_simple_func(privmaskr, STMPRIVMASKR);
+coresight_stm_simple_func(spscr, STMSPSCR);
+coresight_stm_simple_func(spmscr, STMSPMSCR);
+coresight_stm_simple_func(spfeat1r, STMSPFEAT1R);
+coresight_stm_simple_func(spfeat2r, STMSPFEAT2R);
+coresight_stm_simple_func(spfeat3r, STMSPFEAT3R);
+coresight_stm_simple_func(devid, CORESIGHT_DEVID);
+
+static struct attribute *coresight_stm_attrs[] = {
+	&dev_attr_hwevent_enable.attr,
+	&dev_attr_hwevent_select.attr,
+	&dev_attr_port_enable.attr,
+	&dev_attr_port_select.attr,
+	&dev_attr_traceid.attr,
+	NULL,
+};
+
+static struct attribute *coresight_stm_mgmt_attrs[] = {
+	&dev_attr_tcsr.attr,
+	&dev_attr_tsfreqr.attr,
+	&dev_attr_syncr.attr,
+	&dev_attr_sper.attr,
+	&dev_attr_spter.attr,
+	&dev_attr_privmaskr.attr,
+	&dev_attr_spscr.attr,
+	&dev_attr_spmscr.attr,
+	&dev_attr_spfeat1r.attr,
+	&dev_attr_spfeat2r.attr,
+	&dev_attr_spfeat3r.attr,
+	&dev_attr_devid.attr,
+	NULL,
+};
+
+static const struct attribute_group coresight_stm_group = {
+	.attrs = coresight_stm_attrs,
+};
+
+static const struct attribute_group coresight_stm_mgmt_group = {
+	.attrs = coresight_stm_mgmt_attrs,
+	.name = "mgmt",
+};
+
+static const struct attribute_group *coresight_stm_groups[] = {
+	&coresight_stm_group,
+	&coresight_stm_mgmt_group,
+	NULL,
+};
+
+static int stm_get_resource_byname(struct device_node *np,
+				   char *ch_base, struct resource *res)
+{
+	const char *name = NULL;
+	int index = 0, found = 0;
+
+	while (!of_property_read_string_index(np, "reg-names", index, &name)) {
+		if (strcmp(ch_base, name)) {
+			index++;
+			continue;
+		}
+
+		/* We have a match and @index is where it's at */
+		found = 1;
+		break;
+	}
+
+	if (!found)
+		return -EINVAL;
+
+	return of_address_to_resource(np, index, res);
+}
+
+static u32 stm_fundamental_data_size(struct stm_drvdata *drvdata)
+{
+	u32 stmspfeat2r;
+
+	stmspfeat2r = readl_relaxed(drvdata->base + STMSPFEAT2R);
+	return BMVAL(stmspfeat2r, 12, 15);
+}
+
+static u32 stm_num_stimulus_port(struct stm_drvdata *drvdata)
+{
+	u32 numsp;
+
+	numsp = readl_relaxed(drvdata->base + CORESIGHT_DEVID);
+	/*
+	 * NUMPS in STMDEVID is 17 bit long and if equal to 0x0,
+	 * 32 stimulus ports are supported.
+	 */
+	numsp &= 0x1ffff;
+	if (!numsp)
+		numsp = STM_32_CHANNEL;
+	return numsp;
+}
+
+static void stm_init_default_data(struct stm_drvdata *drvdata)
+{
+	/* Don't use port selection */
+	drvdata->stmspscr = 0x0;
+	/*
+	 * Enable all channel regardless of their number.  When port
+	 * selection isn't used (see above) STMSPER applies to all
+	 * 32 channel group available, hence setting all 32 bits to 1
+	 */
+	drvdata->stmsper = ~0x0;
+
+	/*
+	 * Select arbitrary value to start with.  If there is a conflict
+	 * with other tracers the framework will deal with it.
+	 */
+	drvdata->traceid = 0x20;
+
+	/* Set invariant transaction timing on all channels */
+	bitmap_clear(drvdata->chs.guaranteed, 0, drvdata->numsp);
+}
+
+static void stm_init_generic_data(struct stm_drvdata *drvdata)
+{
+	drvdata->stm.name = dev_name(drvdata->dev);
+	drvdata->stm.mstatic = true;
+	drvdata->stm.sw_nchannels = drvdata->numsp;
+	drvdata->stm.packet = stm_generic_packet;
+	drvdata->stm.link = stm_generic_link;
+	drvdata->stm.unlink = stm_generic_unlink;
+	drvdata->stm.set_options = stm_generic_set_options;
+	drvdata->stm.get_options = stm_generic_get_options;
+}
+
+static int stm_probe(struct amba_device *adev, const struct amba_id *id)
+{
+	int ret;
+	void __iomem *base;
+	unsigned long *guaranteed;
+	struct device *dev = &adev->dev;
+	struct coresight_platform_data *pdata = NULL;
+	struct stm_drvdata *drvdata;
+	struct resource *res = &adev->res;
+	struct resource ch_res;
+	size_t res_size, bitmap_size;
+	struct coresight_desc *desc;
+	struct device_node *np = adev->dev.of_node;
+
+	if (np) {
+		pdata = of_get_coresight_platform_data(dev, np);
+		if (IS_ERR(pdata))
+			return PTR_ERR(pdata);
+		adev->dev.platform_data = pdata;
+	}
+	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	drvdata->dev = &adev->dev;
+	drvdata->atclk = devm_clk_get(&adev->dev, "atclk"); /* optional */
+	if (!IS_ERR(drvdata->atclk)) {
+		ret = clk_prepare_enable(drvdata->atclk);
+		if (ret)
+			return ret;
+	}
+	dev_set_drvdata(dev, drvdata);
+
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+	drvdata->base = base;
+
+	ret = stm_get_resource_byname(np, "stm-stimulus-base", &ch_res);
+	if (ret)
+		return ret;
+
+	base = devm_ioremap_resource(dev, &ch_res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+	drvdata->chs.base = base;
+
+	drvdata->write_64bit = stm_fundamental_data_size(drvdata);
+
+	if (boot_nr_channel)
+		drvdata->numsp = boot_nr_channel;
+	else
+		drvdata->numsp = stm_num_stimulus_port(drvdata);
+	res_size = min((resource_size_t)(drvdata->numsp *
+			BYTES_PER_CHANNEL), resource_size(&ch_res));
+	drvdata->numsp = res_size / BYTES_PER_CHANNEL;
+
+	bitmap_size = BITS_TO_LONGS(drvdata->numsp) * sizeof(long);
+	guaranteed = devm_kzalloc(dev, bitmap_size, GFP_KERNEL);
+	if (!guaranteed)
+		return -ENOMEM;
+	drvdata->chs.guaranteed = guaranteed;
+
+	spin_lock_init(&drvdata->spinlock);
+
+	stm_init_default_data(drvdata);
+	stm_init_generic_data(drvdata);
+
+	if (stm_register_device(dev, &drvdata->stm, THIS_MODULE)) {
+		dev_info(dev, "stm_register_device failed, probing deffered\n");
+		return -EPROBE_DEFER;
+	}
+
+	desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
+	if (!desc) {
+		ret = -ENOMEM;
+		goto stm_unregister;
+	}
+
+	desc->type = CORESIGHT_DEV_TYPE_SOURCE;
+	desc->subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE;
+	desc->ops = &stm_cs_ops;
+	desc->pdata = pdata;
+	desc->dev = dev;
+	desc->groups = coresight_stm_groups;
+	drvdata->csdev = coresight_register(desc);
+	if (IS_ERR(drvdata->csdev)) {
+		ret = PTR_ERR(drvdata->csdev);
+		goto stm_unregister;
+	}
+
+	pm_runtime_put(&adev->dev);
+
+	dev_info(dev, "%s initialized\n", (char *)id->data);
+	return 0;
+
+stm_unregister:
+	stm_unregister_device(&drvdata->stm);
+	return ret;
+}
+
+static int stm_remove(struct amba_device *adev)
+{
+	struct stm_drvdata *drvdata = amba_get_drvdata(adev);
+
+	stm_unregister_device(&drvdata->stm);
+	coresight_unregister(drvdata->csdev);
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int stm_runtime_suspend(struct device *dev)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev);
+
+	if (drvdata && !IS_ERR(drvdata->atclk))
+		clk_disable_unprepare(drvdata->atclk);
+
+	return 0;
+}
+
+static int stm_runtime_resume(struct device *dev)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev);
+
+	if (drvdata && !IS_ERR(drvdata->atclk))
+		clk_prepare_enable(drvdata->atclk);
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops stm_dev_pm_ops = {
+	SET_RUNTIME_PM_OPS(stm_runtime_suspend, stm_runtime_resume, NULL)
+};
+
+static struct amba_id stm_ids[] = {
+	{
+		.id     = 0x0003b962,
+		.mask   = 0x0003ffff,
+		.data	= "STM32",
+	},
+	{ 0, 0},
+};
+
+static struct amba_driver stm_driver = {
+	.drv = {
+		.name   = "coresight-stm",
+		.owner	= THIS_MODULE,
+		.pm	= &stm_dev_pm_ops,
+	},
+	.probe          = stm_probe,
+	.remove         = stm_remove,
+	.id_table	= stm_ids,
+};
+
+module_amba_driver(stm_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("CoreSight System Trace Macrocell driver");
diff --git a/include/linux/coresight-stm.h b/include/linux/coresight-stm.h
new file mode 100644
index 0000000..a978bb8
--- /dev/null
+++ b/include/linux/coresight-stm.h
@@ -0,0 +1,6 @@
+#ifndef __LINUX_CORESIGHT_STM_H_
+#define __LINUX_CORESIGHT_STM_H_
+
+#include <uapi/linux/coresight-stm.h>
+
+#endif
diff --git a/include/uapi/linux/coresight-stm.h b/include/uapi/linux/coresight-stm.h
new file mode 100644
index 0000000..fa35f0b
--- /dev/null
+++ b/include/uapi/linux/coresight-stm.h
@@ -0,0 +1,12 @@
+#ifndef __UAPI_CORESIGHT_STM_H_
+#define __UAPI_CORESIGHT_STM_H_
+
+#define STM_FLAG_TIMESTAMPED   BIT(3)
+#define STM_FLAG_GUARANTEED    BIT(7)
+
+enum {
+	STM_OPTION_GUARANTEED = 0,
+	STM_OPTION_INVARIANT,
+};
+
+#endif
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 68+ messages in thread
* [PATCH V2 6/6] coresight-stm: adding driver for CoreSight STM component
@ 2016-02-03  8:15   ` Chunyan Zhang
  0 siblings, 0 replies; 68+ messages in thread
From: Chunyan Zhang @ 2016-02-03  8:15 UTC (permalink / raw)
  To: linux-arm-kernel

From: Pratik Patel <pratikp@codeaurora.org>

This driver adds support for the STM CoreSight IP block, allowing any
system compoment (HW or SW) to log and aggregate messages via a
single entity.

The CoreSight STM exposes an application defined number of channels
called stimulus port. Configuration is done using entries in sysfs
and channels made available to userspace via configfs.

Signed-off-by: Pratik Patel <pratikp@codeaurora.org>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 .../ABI/testing/sysfs-bus-coresight-devices-stm    |  53 ++
 Documentation/trace/coresight.txt                  |  37 +-
 drivers/hwtracing/coresight/Kconfig                |  11 +
 drivers/hwtracing/coresight/Makefile               |   1 +
 drivers/hwtracing/coresight/coresight-stm.c        | 972 +++++++++++++++++++++
 include/linux/coresight-stm.h                      |   6 +
 include/uapi/linux/coresight-stm.h                 |  12 +
 7 files changed, 1090 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-coresight-devices-stm
 create mode 100644 drivers/hwtracing/coresight/coresight-stm.c
 create mode 100644 include/linux/coresight-stm.h
 create mode 100644 include/uapi/linux/coresight-stm.h

diff --git a/Documentation/ABI/testing/sysfs-bus-coresight-devices-stm b/Documentation/ABI/testing/sysfs-bus-coresight-devices-stm
new file mode 100644
index 0000000..a1d7022
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-coresight-devices-stm
@@ -0,0 +1,53 @@
+What:		/sys/bus/coresight/devices/<memory_map>.stm/enable_source
+Date:		February 2016
+KernelVersion:	4.5
+Contact:	Mathieu Poirier <mathieu.poirier@linaro.org>
+Description:	(RW) Enable/disable tracing on this specific trace macrocell.
+		Enabling the trace macrocell implies it has been configured
+		properly and a sink has been identidifed for it.  The path
+		of coresight components linking the source to the sink is
+		configured and managed automatically by the coresight framework.
+
+What:		/sys/bus/coresight/devices/<memory_map>.stm/hwevent_enable
+Date:		February 2016
+KernelVersion:	4.5
+Contact:	Mathieu Poirier <mathieu.poirier@linaro.org>
+Description:	(RW) Provides access to the HW event enable register, used in
+		conjunction with HW event bank select register.
+
+What:		/sys/bus/coresight/devices/<memory_map>.stm/hwevent_select
+Date:		February 2016
+KernelVersion:	4.5
+Contact:	Mathieu Poirier <mathieu.poirier@linaro.org>
+Description:	(RW) Gives access to the HW event block select register
+		(STMHEBSR) in order to configure up to 256 channels.  Used in
+		conjunction with "hwevent_enable" register as described above.
+
+What:		/sys/bus/coresight/devices/<memory_map>.stm/port_enable
+Date:		February 2016
+KernelVersion:	4.5
+Contact:	Mathieu Poirier <mathieu.poirier@linaro.org>
+Description:	(RW) Provides access to the stimlus port enable register
+		(STMSPER).  Used in conjunction with "port_select" described
+		below.
+
+What:		/sys/bus/coresight/devices/<memory_map>.stm/port_select
+Date:		February 2016
+KernelVersion:	4.5
+Contact:	Mathieu Poirier <mathieu.poirier@linaro.org>
+Description:	(RW) Used to determine which bank of stimulus port bit in
+		register STMSPER (see above) apply to.
+
+What:		/sys/bus/coresight/devices/<memory_map>.stm/status
+Date:		February 2016
+KernelVersion:	4.5
+Contact:	Mathieu Poirier <mathieu.poirier@linaro.org>
+Description:	(R) List various control and status registers.  The specific
+		layout and content is driver specific.
+
+What:		/sys/bus/coresight/devices/<memory_map>.stm/traceid
+Date:		February 2016
+KernelVersion:	4.5
+Contact:	Mathieu Poirier <mathieu.poirier@linaro.org>
+Description:	(RW) Holds the trace ID that will appear in the trace stream
+		coming from this trace entity.
diff --git a/Documentation/trace/coresight.txt b/Documentation/trace/coresight.txt
index 0a5c329..a33c88c 100644
--- a/Documentation/trace/coresight.txt
+++ b/Documentation/trace/coresight.txt
@@ -190,8 +190,8 @@ expected to be accessed and controlled using those entries.
 Last but not least, "struct module *owner" is expected to be set to reflect
 the information carried in "THIS_MODULE".
 
-How to use
-----------
+How to use the tracer modules
+-----------------------------
 
 Before trace collection can start, a coresight sink needs to be identify.
 There is no limit on the amount of sinks (nor sources) that can be enabled at
@@ -297,3 +297,36 @@ Info                                    Tracing enabled
 Instruction     13570831        0x8026B584      E28DD00C        false   ADD      sp,sp,#0xc
 Instruction     0       0x8026B588      E8BD8000        true    LDM      sp!,{pc}
 Timestamp                                       Timestamp: 17107041535
+
+How to use the STM module
+-------------------------
+
+Using the System Trace Macrocell module is the same as the tracers - the only
+difference is that clients are driving the trace capture rather
+than the program flow through the code.
+
+As with any other CoreSight component, specifics about the STM tracer can be
+found in sysfs with more information on each entry being found in [1]:
+
+root at genericarmv8:~# ls /sys/bus/coresight/devices/20100000.stm
+enable_source   hwevent_select  port_enable     subsystem       uevent
+hwevent_enable  mgmt            port_select     traceid
+root at genericarmv8:~#
+
+Like any other source a sink needs to be identified and the STM enabled before
+being used:
+
+root at genericarmv8:~# echo 1 > /sys/bus/coresight/devices/20010000.etf/enable_sink
+root at genericarmv8:~# echo 1 > /sys/bus/coresight/devices/20100000.stm/enable_source
+
+From there user space applications can request and use channels using the devfs
+interface provided for that purpose by the generic STM API:
+
+root at genericarmv8:~# ls -l /dev/20100000.stm
+crw-------    1 root     root       10,  61 Jan  3 18:11 /dev/20100000.stm
+root at genericarmv8:~#
+
+Details on how to use the generic STM API can be found here [2].
+
+[1]. Documentation/ABI/testing/sysfs-bus-coresight-devices-stm
+[2]. Documentation/trace/stm.txt
diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index c85935f..f4a8bfa 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -77,4 +77,15 @@ config CORESIGHT_QCOM_REPLICATOR
 	  programmable ATB replicator sends the ATB trace stream from the
 	  ETB/ETF to the TPIUi and ETR.
 
+config CORESIGHT_STM
+	bool "CoreSight System Trace Macrocell driver"
+	depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
+	select CORESIGHT_LINKS_AND_SINKS
+	select STM
+	help
+	  This driver provides support for hardware assisted software
+	  instrumentation based tracing. This is primarily used for
+	  logging useful software events or data coming from various entities
+	  in the system, possibly running different OSs
+
 endif
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index 99f8e5f..1f6eaec 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o \
 obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o coresight-etm-cp14.o
 obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o
 obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
+obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
new file mode 100644
index 0000000..c84f57a
--- /dev/null
+++ b/drivers/hwtracing/coresight/coresight-stm.c
@@ -0,0 +1,972 @@
+/* Copyright (c) 2015-2016, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only 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.
+ *
+ * Initial implementation by Pratik Patel
+ * (C) 2014-2015 Pratik Patel <pratikp@codeaurora.org>
+ *
+ * Serious refactoring, code cleanup and upgrading to the Coresight upstream
+ * framework by Mathieu Poirier
+ * (C) 2015-2016 Mathieu Poirier <mathieu.poirier@linaro.org>
+ *
+ * Guaranteed timing and support for various packet type coming from the
+ * generic STM API by Chunyan Zhang
+ * (C) 2015-2016 Chunyan Zhang <zhang.chunyan@linaro.org>
+ */
+#include <linux/amba/bus.h>
+#include <linux/bitmap.h>
+#include <linux/clk.h>
+#include <linux/coresight.h>
+#include <linux/coresight-stm.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/pm_runtime.h>
+#include <linux/stm.h>
+
+#include "coresight-priv.h"
+
+#define STMDMASTARTR			0xc04
+#define STMDMASTOPR			0xc08
+#define STMDMASTATR			0xc0c
+#define STMDMACTLR			0xc10
+#define STMDMAIDR			0xcfc
+#define STMHEER				0xd00
+#define STMHETER			0xd20
+#define STMHEBSR			0xd60
+#define STMHEMCR			0xd64
+#define STMHEMASTR			0xdf4
+#define STMHEFEAT1R			0xdf8
+#define STMHEIDR			0xdfc
+#define STMSPER				0xe00
+#define STMSPTER			0xe20
+#define STMPRIVMASKR			0xe40
+#define STMSPSCR			0xe60
+#define STMSPMSCR			0xe64
+#define STMSPOVERRIDER			0xe68
+#define STMSPMOVERRIDER			0xe6c
+#define STMSPTRIGCSR			0xe70
+#define STMTCSR				0xe80
+#define STMTSSTIMR			0xe84
+#define STMTSFREQR			0xe8c
+#define STMSYNCR			0xe90
+#define STMAUXCR			0xe94
+#define STMSPFEAT1R			0xea0
+#define STMSPFEAT2R			0xea4
+#define STMSPFEAT3R			0xea8
+#define STMITTRIGGER			0xee8
+#define STMITATBDATA0			0xeec
+#define STMITATBCTR2			0xef0
+#define STMITATBID			0xef4
+#define STMITATBCTR0			0xef8
+
+#define STM_32_CHANNEL			32
+#define BYTES_PER_CHANNEL		256
+#define STM_TRACE_BUF_SIZE		4096
+#define STM_SW_MASTER_END		127
+
+/* Register bit definition */
+#define STMTCSR_BUSY_BIT		23
+/* Reserve the first 10 channels for kernel usage */
+#define STM_CHANNEL_OFFSET		0
+
+enum stm_pkt_type {
+	STM_PKT_TYPE_DATA	= 0x98,
+	STM_PKT_TYPE_FLAG	= 0xE8,
+	STM_PKT_TYPE_TRIG	= 0xF8,
+};
+
+#define stm_channel_addr(drvdata, ch)	(drvdata->chs.base +	\
+					(ch * BYTES_PER_CHANNEL))
+#define stm_channel_off(type, opts)	(type & ~opts)
+
+#ifndef CONFIG_64BIT
+static inline void __raw_writeq(u64 val, volatile void __iomem *addr)
+{
+	asm volatile("strd %1, %0"
+		     : "+Qo" (*(volatile u64 __force *)addr)
+		     : "r" (val));
+}
+#undef writeq_relaxed
+#define writeq_relaxed(v, c)	__raw_writeq((__force u64) cpu_to_le64(v), c)
+#endif
+
+static int boot_nr_channel;
+
+module_param_named(
+	boot_nr_channel, boot_nr_channel, int, S_IRUGO
+);
+
+/**
+ * struct channel_space - central management entity for extended ports
+ * @base:		memory mapped base address where channels start.
+ * @guaraneed:		is the channel delivery guaranteed.
+ */
+struct channel_space {
+	void __iomem		*base;
+	unsigned long		*guaranteed;
+};
+
+/**
+ * struct stm_drvdata - specifics associated to an STM component
+ * @base:		memory mapped base address for this component.
+ * @dev:		the device entity associated to this component.
+ * @atclk:		optional clock for the core parts of the STM.
+ * @csdev:		component vitals needed by the framework.
+ * @spinlock:		only one at a time pls.
+ * @chs:		the channels accociated to this STM.
+ * @stm:		structure associated to the generic STM interface.
+ * @enable:		this STM is being used.
+ * @traceid:		value of the current ID for this component.
+ * @write_64bit:	whether this STM supports 64 bit access.
+ * @stmsper:		settings for register STMSPER.
+ * @stmspscr:		settings for register STMSPSCR.
+ * @numsp:		the total number of stimulus port support by this STM.
+ * @stmheer:		settings for register STMHEER.
+ * @stmheter:		settings for register STMHETER.
+ * @stmhebsr:		settings for register STMHEBSR.
+ */
+struct stm_drvdata {
+	void __iomem		*base;
+	struct device		*dev;
+	struct clk		*atclk;
+	struct coresight_device	*csdev;
+	spinlock_t		spinlock;
+	struct channel_space	chs;
+	struct stm_data		stm;
+	bool			enable;
+	u8			traceid;
+	u32			write_64bit;
+	u32			stmsper;
+	u32			stmspscr;
+	u32			numsp;
+	u32			stmheer;
+	u32			stmheter;
+	u32			stmhebsr;
+};
+
+static void stm_hwevent_enable_hw(struct stm_drvdata *drvdata)
+{
+	CS_UNLOCK(drvdata->base);
+
+	writel_relaxed(drvdata->stmhebsr, drvdata->base + STMHEBSR);
+	writel_relaxed(drvdata->stmheter, drvdata->base + STMHETER);
+	writel_relaxed(drvdata->stmheer, drvdata->base + STMHEER);
+	writel_relaxed(0x01 |	/* Enable HW event tracing */
+		       0x04,	/* Error detection on event tracing */
+		       drvdata->base + STMHEMCR);
+
+	CS_LOCK(drvdata->base);
+}
+
+static void stm_port_enable_hw(struct stm_drvdata *drvdata)
+{
+	CS_UNLOCK(drvdata->base);
+	/* ATB trigger enable on direct writes to TRIG locations */
+	writel_relaxed(0x10,
+		       drvdata->base + STMSPTRIGCSR);
+	writel_relaxed(drvdata->stmspscr, drvdata->base + STMSPSCR);
+	writel_relaxed(drvdata->stmsper, drvdata->base + STMSPER);
+
+	CS_LOCK(drvdata->base);
+}
+
+static void stm_enable_hw(struct stm_drvdata *drvdata)
+{
+	if (drvdata->stmheer)
+		stm_hwevent_enable_hw(drvdata);
+
+	stm_port_enable_hw(drvdata);
+
+	CS_UNLOCK(drvdata->base);
+
+	/* 4096 byte between synchronisation packets */
+	writel_relaxed(0xFFF, drvdata->base + STMSYNCR);
+	writel_relaxed((drvdata->traceid << 16 | /* trace id */
+			0x02 |			 /* timestamp enable */
+			0x01),			 /* global STM enable */
+			drvdata->base + STMTCSR);
+
+	CS_LOCK(drvdata->base);
+}
+
+static int stm_enable(struct coresight_device *csdev)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	pm_runtime_get_sync(drvdata->dev);
+
+	spin_lock(&drvdata->spinlock);
+	stm_enable_hw(drvdata);
+	drvdata->enable = true;
+	spin_unlock(&drvdata->spinlock);
+
+	dev_info(drvdata->dev, "STM tracing enabled\n");
+	return 0;
+}
+
+static void stm_hwevent_disable_hw(struct stm_drvdata *drvdata)
+{
+	CS_UNLOCK(drvdata->base);
+
+	writel_relaxed(0x0, drvdata->base + STMHEMCR);
+	writel_relaxed(0x0, drvdata->base + STMHEER);
+	writel_relaxed(0x0, drvdata->base + STMHETER);
+
+	CS_LOCK(drvdata->base);
+}
+
+static void stm_port_disable_hw(struct stm_drvdata *drvdata)
+{
+	CS_UNLOCK(drvdata->base);
+
+	writel_relaxed(0x0, drvdata->base + STMSPER);
+	writel_relaxed(0x0, drvdata->base + STMSPTRIGCSR);
+
+	CS_LOCK(drvdata->base);
+}
+
+static void stm_disable_hw(struct stm_drvdata *drvdata)
+{
+	u32 val;
+
+	CS_UNLOCK(drvdata->base);
+
+	val = readl_relaxed(drvdata->base + STMTCSR);
+	val &= ~0x1; /* clear global STM enable [0] */
+	writel_relaxed(val, drvdata->base + STMTCSR);
+
+	CS_LOCK(drvdata->base);
+
+	stm_port_disable_hw(drvdata);
+	if (drvdata->stmheer)
+		stm_hwevent_disable_hw(drvdata);
+}
+
+static void stm_disable(struct coresight_device *csdev)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	spin_lock(&drvdata->spinlock);
+	stm_disable_hw(drvdata);
+	drvdata->enable = false;
+	spin_unlock(&drvdata->spinlock);
+
+	/* Wait until the engine has completely stopped */
+	coresight_timeout(drvdata, STMTCSR, STMTCSR_BUSY_BIT, 0);
+
+	pm_runtime_put(drvdata->dev);
+
+	dev_info(drvdata->dev, "STM tracing disabled\n");
+}
+
+static int stm_trace_id(struct coresight_device *csdev)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	return drvdata->traceid;
+}
+
+static const struct coresight_ops_source stm_source_ops = {
+	.trace_id	= stm_trace_id,
+	.enable		= stm_enable,
+	.disable	= stm_disable,
+};
+
+static const struct coresight_ops stm_cs_ops = {
+	.source_ops	= &stm_source_ops,
+};
+
+static int stm_send_64bit(void *addr, const void *data, u32 size)
+{
+	u64 prepad = 0;
+	u64 postpad = 0;
+	char *pad;
+	u8 off, endoff;
+	u32 len = size;
+
+	off = (unsigned long)data & 0x7;
+
+	if (off) {
+		endoff = 8 - off;
+		pad = (char *)&prepad;
+		pad += off;
+
+		while (endoff && size) {
+			*pad++ = *(char *)data++;
+			endoff--;
+			size--;
+		}
+		writeq_relaxed(prepad, addr);
+	}
+
+	/* now we are 64bit aligned */
+	while (size >= 8) {
+		writeq_relaxed(*(u64 *)data, addr);
+		data += 8;
+		size -= 8;
+	}
+
+	endoff = 0;
+
+	if (size) {
+		endoff = 8 - (u8)size;
+		pad = (char *)&postpad;
+
+		while (size) {
+			*pad++ = *(char *)data++;
+			size--;
+		}
+		writeq_relaxed(postpad, addr);
+	}
+
+	return len + off + endoff;
+}
+
+static int stm_send(void *addr, const void *data, u32 size)
+{
+	u32 len = size;
+
+	if (((unsigned long)data & 0x1) && (size >= 1)) {
+		writeb_relaxed(*(u8 *)data, addr);
+		data++;
+		size--;
+	}
+	if (((unsigned long)data & 0x2) && (size >= 2)) {
+		writew_relaxed(*(u16 *)data, addr);
+		data += 2;
+		size -= 2;
+	}
+
+	/* now we are 32bit aligned */
+	while (size >= 4) {
+		writel_relaxed(*(u32 *)data, addr);
+		data += 4;
+		size -= 4;
+	}
+
+	if (size >= 2) {
+		writew_relaxed(*(u16 *)data, addr);
+		data += 2;
+		size -= 2;
+	}
+	if (size >= 1) {
+		writeb_relaxed(*(u8 *)data, addr);
+		data++;
+		size--;
+	}
+
+	return len;
+}
+
+static int stm_generic_link(struct stm_data *stm_data,
+			    unsigned int master,  unsigned int channel)
+{
+	struct stm_drvdata *drvdata = container_of(stm_data,
+						   struct stm_drvdata, stm);
+	if (!drvdata || !drvdata->csdev)
+		return -EINVAL;
+
+	return coresight_enable(drvdata->csdev);
+}
+
+static void stm_generic_unlink(struct stm_data *stm_data,
+			       unsigned int master,  unsigned int channel)
+{
+	struct stm_drvdata *drvdata = container_of(stm_data,
+						   struct stm_drvdata, stm);
+	if (!drvdata || !drvdata->csdev)
+		return;
+
+	stm_disable(drvdata->csdev);
+}
+
+static long stm_generic_set_options(struct stm_data *stm_data,
+				    unsigned int master,
+				    unsigned int channel,
+				    unsigned int nr_chans,
+				    unsigned long options)
+{
+	struct stm_drvdata *drvdata = container_of(stm_data,
+						   struct stm_drvdata, stm);
+	if (!(drvdata && drvdata->enable))
+		return -EINVAL;
+
+	if (channel >= drvdata->numsp)
+		return -EINVAL;
+
+	switch (options) {
+	case STM_OPTION_GUARANTEED:
+		set_bit(channel, drvdata->chs.guaranteed);
+		break;
+
+	case STM_OPTION_INVARIANT:
+		clear_bit(channel, drvdata->chs.guaranteed);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static long stm_generic_get_options(struct stm_data *stm_data,
+				    unsigned int master,
+				    unsigned int channel,
+				    unsigned int nr_chans,
+				    u64 *options)
+{
+	struct stm_drvdata *drvdata = container_of(stm_data,
+						   struct stm_drvdata, stm);
+	if (!(drvdata && drvdata->enable))
+		return -EINVAL;
+
+	if (channel >= drvdata->numsp)
+		return -EINVAL;
+
+	switch (*options) {
+	case STM_OPTION_GUARANTEED:
+		*options = test_bit(channel, drvdata->chs.guaranteed);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static ssize_t stm_generic_packet(struct stm_data *stm_data,
+				  unsigned int master,
+				  unsigned int channel,
+				  unsigned int packet,
+				  unsigned int flags,
+				  unsigned int size,
+				  const unsigned char *payload)
+{
+	unsigned int len = size;
+	unsigned long ch_addr;
+	struct stm_drvdata *drvdata = container_of(stm_data,
+						   struct stm_drvdata, stm);
+	if (!(drvdata && drvdata->enable))
+		return 0;
+
+	if (channel >= drvdata->numsp)
+		return 0;
+
+	ch_addr = (unsigned long)stm_channel_addr(drvdata, channel);
+
+	flags = (flags == STP_PACKET_TIMESTAMPED) ? STM_FLAG_TIMESTAMPED : 0;
+	flags |= test_bit(channel, drvdata->chs.guaranteed) ?
+			   STM_FLAG_GUARANTEED : 0;
+
+	switch (packet) {
+	case STP_PACKET_FLAG:
+		ch_addr |= stm_channel_off(STM_PKT_TYPE_FLAG, flags);
+
+		/* the generic STM API set a size of zero on flag packets. */
+		len = 1;
+		break;
+
+	case STP_PACKET_DATA:
+		ch_addr |= stm_channel_off(STM_PKT_TYPE_DATA, flags);
+		break;
+
+	default:
+		return 0;
+	}
+
+	if (drvdata->write_64bit)
+		return stm_send_64bit((void *)ch_addr, payload, len);
+
+	return stm_send((void *)ch_addr, payload, len);
+}
+
+static ssize_t hwevent_enable_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val = drvdata->stmheer;
+
+	return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
+}
+
+static ssize_t hwevent_enable_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t size)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+	int ret = 0;
+
+	ret = kstrtoul(buf, 16, &val);
+	if (ret)
+		return -EINVAL;
+
+	drvdata->stmheer = val;
+	/* HW event enable and trigger go hand in hand */
+	drvdata->stmheter = val;
+
+	return size;
+}
+static DEVICE_ATTR_RW(hwevent_enable);
+
+static ssize_t hwevent_select_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val = drvdata->stmhebsr;
+
+	return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
+}
+
+static ssize_t hwevent_select_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t size)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+	int ret = 0;
+
+	ret = kstrtoul(buf, 16, &val);
+	if (ret)
+		return -EINVAL;
+
+	drvdata->stmhebsr = val;
+
+	return size;
+}
+static DEVICE_ATTR_RW(hwevent_select);
+
+static ssize_t port_select_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+
+	if (!drvdata->enable) {
+		val = drvdata->stmspscr;
+	} else {
+		spin_lock(&drvdata->spinlock);
+		val = readl_relaxed(drvdata->base + STMSPSCR);
+		spin_unlock(&drvdata->spinlock);
+	}
+
+	return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
+}
+
+static ssize_t port_select_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t size)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val, stmsper;
+	int ret = 0;
+
+	ret = kstrtoul(buf, 16, &val);
+	if (ret)
+		return ret;
+
+	spin_lock(&drvdata->spinlock);
+	drvdata->stmspscr = val;
+
+	if (drvdata->enable) {
+		CS_UNLOCK(drvdata->base);
+		/* Process as per ARM's TRM recommendation */
+		stmsper = readl_relaxed(drvdata->base + STMSPER);
+		writel_relaxed(0x0, drvdata->base + STMSPER);
+		writel_relaxed(drvdata->stmspscr, drvdata->base + STMSPSCR);
+		writel_relaxed(stmsper, drvdata->base + STMSPER);
+		CS_LOCK(drvdata->base);
+	}
+	spin_unlock(&drvdata->spinlock);
+
+	return size;
+}
+static DEVICE_ATTR_RW(port_select);
+
+static ssize_t port_enable_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+
+	if (!drvdata->enable) {
+		val = drvdata->stmsper;
+	} else {
+		spin_lock(&drvdata->spinlock);
+		val = readl_relaxed(drvdata->base + STMSPER);
+		spin_unlock(&drvdata->spinlock);
+	}
+
+	return scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
+}
+
+static ssize_t port_enable_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t size)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+	int ret = 0;
+
+	ret = kstrtoul(buf, 16, &val);
+	if (ret)
+		return ret;
+
+	spin_lock(&drvdata->spinlock);
+	drvdata->stmsper = val;
+
+	if (drvdata->enable) {
+		CS_UNLOCK(drvdata->base);
+		writel_relaxed(drvdata->stmsper, drvdata->base + STMSPER);
+		CS_LOCK(drvdata->base);
+	}
+	spin_unlock(&drvdata->spinlock);
+
+	return size;
+}
+static DEVICE_ATTR_RW(port_enable);
+
+static ssize_t traceid_show(struct device *dev,
+			    struct device_attribute *attr, char *buf)
+{
+	unsigned long val;
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	val = drvdata->traceid;
+	return sprintf(buf, "%#lx\n", val);
+}
+
+static ssize_t traceid_store(struct device *dev,
+			     struct device_attribute *attr,
+			     const char *buf, size_t size)
+{
+	int ret;
+	unsigned long val;
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	ret = kstrtoul(buf, 16, &val);
+	if (ret)
+		return ret;
+
+	/* traceid field is 7bit wide on STM32 */
+	drvdata->traceid = val & 0x7f;
+	return size;
+}
+static DEVICE_ATTR_RW(traceid);
+
+#define coresight_simple_func(type, name, offset)			\
+static ssize_t name##_show(struct device *_dev,				\
+			   struct device_attribute *attr, char *buf)	\
+{									\
+	type *drvdata = dev_get_drvdata(_dev->parent);			\
+	return scnprintf(buf, PAGE_SIZE, "0x%08x\n",			\
+			 readl_relaxed(drvdata->base + offset));	\
+}									\
+DEVICE_ATTR_RO(name)
+
+#define coresight_stm_simple_func(name, offset)	\
+	coresight_simple_func(struct stm_drvdata, name, offset)
+
+coresight_stm_simple_func(tcsr, STMTCSR);
+coresight_stm_simple_func(tsfreqr, STMTSFREQR);
+coresight_stm_simple_func(syncr, STMSYNCR);
+coresight_stm_simple_func(sper, STMSPER);
+coresight_stm_simple_func(spter, STMSPTER);
+coresight_stm_simple_func(privmaskr, STMPRIVMASKR);
+coresight_stm_simple_func(spscr, STMSPSCR);
+coresight_stm_simple_func(spmscr, STMSPMSCR);
+coresight_stm_simple_func(spfeat1r, STMSPFEAT1R);
+coresight_stm_simple_func(spfeat2r, STMSPFEAT2R);
+coresight_stm_simple_func(spfeat3r, STMSPFEAT3R);
+coresight_stm_simple_func(devid, CORESIGHT_DEVID);
+
+static struct attribute *coresight_stm_attrs[] = {
+	&dev_attr_hwevent_enable.attr,
+	&dev_attr_hwevent_select.attr,
+	&dev_attr_port_enable.attr,
+	&dev_attr_port_select.attr,
+	&dev_attr_traceid.attr,
+	NULL,
+};
+
+static struct attribute *coresight_stm_mgmt_attrs[] = {
+	&dev_attr_tcsr.attr,
+	&dev_attr_tsfreqr.attr,
+	&dev_attr_syncr.attr,
+	&dev_attr_sper.attr,
+	&dev_attr_spter.attr,
+	&dev_attr_privmaskr.attr,
+	&dev_attr_spscr.attr,
+	&dev_attr_spmscr.attr,
+	&dev_attr_spfeat1r.attr,
+	&dev_attr_spfeat2r.attr,
+	&dev_attr_spfeat3r.attr,
+	&dev_attr_devid.attr,
+	NULL,
+};
+
+static const struct attribute_group coresight_stm_group = {
+	.attrs = coresight_stm_attrs,
+};
+
+static const struct attribute_group coresight_stm_mgmt_group = {
+	.attrs = coresight_stm_mgmt_attrs,
+	.name = "mgmt",
+};
+
+static const struct attribute_group *coresight_stm_groups[] = {
+	&coresight_stm_group,
+	&coresight_stm_mgmt_group,
+	NULL,
+};
+
+static int stm_get_resource_byname(struct device_node *np,
+				   char *ch_base, struct resource *res)
+{
+	const char *name = NULL;
+	int index = 0, found = 0;
+
+	while (!of_property_read_string_index(np, "reg-names", index, &name)) {
+		if (strcmp(ch_base, name)) {
+			index++;
+			continue;
+		}
+
+		/* We have a match and @index is where it's at */
+		found = 1;
+		break;
+	}
+
+	if (!found)
+		return -EINVAL;
+
+	return of_address_to_resource(np, index, res);
+}
+
+static u32 stm_fundamental_data_size(struct stm_drvdata *drvdata)
+{
+	u32 stmspfeat2r;
+
+	stmspfeat2r = readl_relaxed(drvdata->base + STMSPFEAT2R);
+	return BMVAL(stmspfeat2r, 12, 15);
+}
+
+static u32 stm_num_stimulus_port(struct stm_drvdata *drvdata)
+{
+	u32 numsp;
+
+	numsp = readl_relaxed(drvdata->base + CORESIGHT_DEVID);
+	/*
+	 * NUMPS in STMDEVID is 17 bit long and if equal to 0x0,
+	 * 32 stimulus ports are supported.
+	 */
+	numsp &= 0x1ffff;
+	if (!numsp)
+		numsp = STM_32_CHANNEL;
+	return numsp;
+}
+
+static void stm_init_default_data(struct stm_drvdata *drvdata)
+{
+	/* Don't use port selection */
+	drvdata->stmspscr = 0x0;
+	/*
+	 * Enable all channel regardless of their number.  When port
+	 * selection isn't used (see above) STMSPER applies to all
+	 * 32 channel group available, hence setting all 32 bits to 1
+	 */
+	drvdata->stmsper = ~0x0;
+
+	/*
+	 * Select arbitrary value to start with.  If there is a conflict
+	 * with other tracers the framework will deal with it.
+	 */
+	drvdata->traceid = 0x20;
+
+	/* Set invariant transaction timing on all channels */
+	bitmap_clear(drvdata->chs.guaranteed, 0, drvdata->numsp);
+}
+
+static void stm_init_generic_data(struct stm_drvdata *drvdata)
+{
+	drvdata->stm.name = dev_name(drvdata->dev);
+	drvdata->stm.mstatic = true;
+	drvdata->stm.sw_nchannels = drvdata->numsp;
+	drvdata->stm.packet = stm_generic_packet;
+	drvdata->stm.link = stm_generic_link;
+	drvdata->stm.unlink = stm_generic_unlink;
+	drvdata->stm.set_options = stm_generic_set_options;
+	drvdata->stm.get_options = stm_generic_get_options;
+}
+
+static int stm_probe(struct amba_device *adev, const struct amba_id *id)
+{
+	int ret;
+	void __iomem *base;
+	unsigned long *guaranteed;
+	struct device *dev = &adev->dev;
+	struct coresight_platform_data *pdata = NULL;
+	struct stm_drvdata *drvdata;
+	struct resource *res = &adev->res;
+	struct resource ch_res;
+	size_t res_size, bitmap_size;
+	struct coresight_desc *desc;
+	struct device_node *np = adev->dev.of_node;
+
+	if (np) {
+		pdata = of_get_coresight_platform_data(dev, np);
+		if (IS_ERR(pdata))
+			return PTR_ERR(pdata);
+		adev->dev.platform_data = pdata;
+	}
+	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	drvdata->dev = &adev->dev;
+	drvdata->atclk = devm_clk_get(&adev->dev, "atclk"); /* optional */
+	if (!IS_ERR(drvdata->atclk)) {
+		ret = clk_prepare_enable(drvdata->atclk);
+		if (ret)
+			return ret;
+	}
+	dev_set_drvdata(dev, drvdata);
+
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+	drvdata->base = base;
+
+	ret = stm_get_resource_byname(np, "stm-stimulus-base", &ch_res);
+	if (ret)
+		return ret;
+
+	base = devm_ioremap_resource(dev, &ch_res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+	drvdata->chs.base = base;
+
+	drvdata->write_64bit = stm_fundamental_data_size(drvdata);
+
+	if (boot_nr_channel)
+		drvdata->numsp = boot_nr_channel;
+	else
+		drvdata->numsp = stm_num_stimulus_port(drvdata);
+	res_size = min((resource_size_t)(drvdata->numsp *
+			BYTES_PER_CHANNEL), resource_size(&ch_res));
+	drvdata->numsp = res_size / BYTES_PER_CHANNEL;
+
+	bitmap_size = BITS_TO_LONGS(drvdata->numsp) * sizeof(long);
+	guaranteed = devm_kzalloc(dev, bitmap_size, GFP_KERNEL);
+	if (!guaranteed)
+		return -ENOMEM;
+	drvdata->chs.guaranteed = guaranteed;
+
+	spin_lock_init(&drvdata->spinlock);
+
+	stm_init_default_data(drvdata);
+	stm_init_generic_data(drvdata);
+
+	if (stm_register_device(dev, &drvdata->stm, THIS_MODULE)) {
+		dev_info(dev, "stm_register_device failed, probing deffered\n");
+		return -EPROBE_DEFER;
+	}
+
+	desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
+	if (!desc) {
+		ret = -ENOMEM;
+		goto stm_unregister;
+	}
+
+	desc->type = CORESIGHT_DEV_TYPE_SOURCE;
+	desc->subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_SOFTWARE;
+	desc->ops = &stm_cs_ops;
+	desc->pdata = pdata;
+	desc->dev = dev;
+	desc->groups = coresight_stm_groups;
+	drvdata->csdev = coresight_register(desc);
+	if (IS_ERR(drvdata->csdev)) {
+		ret = PTR_ERR(drvdata->csdev);
+		goto stm_unregister;
+	}
+
+	pm_runtime_put(&adev->dev);
+
+	dev_info(dev, "%s initialized\n", (char *)id->data);
+	return 0;
+
+stm_unregister:
+	stm_unregister_device(&drvdata->stm);
+	return ret;
+}
+
+static int stm_remove(struct amba_device *adev)
+{
+	struct stm_drvdata *drvdata = amba_get_drvdata(adev);
+
+	stm_unregister_device(&drvdata->stm);
+	coresight_unregister(drvdata->csdev);
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int stm_runtime_suspend(struct device *dev)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev);
+
+	if (drvdata && !IS_ERR(drvdata->atclk))
+		clk_disable_unprepare(drvdata->atclk);
+
+	return 0;
+}
+
+static int stm_runtime_resume(struct device *dev)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(dev);
+
+	if (drvdata && !IS_ERR(drvdata->atclk))
+		clk_prepare_enable(drvdata->atclk);
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops stm_dev_pm_ops = {
+	SET_RUNTIME_PM_OPS(stm_runtime_suspend, stm_runtime_resume, NULL)
+};
+
+static struct amba_id stm_ids[] = {
+	{
+		.id     = 0x0003b962,
+		.mask   = 0x0003ffff,
+		.data	= "STM32",
+	},
+	{ 0, 0},
+};
+
+static struct amba_driver stm_driver = {
+	.drv = {
+		.name   = "coresight-stm",
+		.owner	= THIS_MODULE,
+		.pm	= &stm_dev_pm_ops,
+	},
+	.probe          = stm_probe,
+	.remove         = stm_remove,
+	.id_table	= stm_ids,
+};
+
+module_amba_driver(stm_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("CoreSight System Trace Macrocell driver");
diff --git a/include/linux/coresight-stm.h b/include/linux/coresight-stm.h
new file mode 100644
index 0000000..a978bb8
--- /dev/null
+++ b/include/linux/coresight-stm.h
@@ -0,0 +1,6 @@
+#ifndef __LINUX_CORESIGHT_STM_H_
+#define __LINUX_CORESIGHT_STM_H_
+
+#include <uapi/linux/coresight-stm.h>
+
+#endif
diff --git a/include/uapi/linux/coresight-stm.h b/include/uapi/linux/coresight-stm.h
new file mode 100644
index 0000000..fa35f0b
--- /dev/null
+++ b/include/uapi/linux/coresight-stm.h
@@ -0,0 +1,12 @@
+#ifndef __UAPI_CORESIGHT_STM_H_
+#define __UAPI_CORESIGHT_STM_H_
+
+#define STM_FLAG_TIMESTAMPED   BIT(3)
+#define STM_FLAG_GUARANTEED    BIT(7)
+
+enum {
+	STM_OPTION_GUARANTEED = 0,
+	STM_OPTION_INVARIANT,
+};
+
+#endif
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name
  2016-02-03  8:15   ` Chunyan Zhang
@ 2016-02-03 10:05     ` kbuild test robot
  -1 siblings, 0 replies; 68+ messages in thread
From: kbuild test robot @ 2016-02-03 10:05 UTC (permalink / raw)
  To: Chunyan Zhang
  Cc: kbuild-all, mathieu.poirier, alexander.shishkin, robh, broonie,
	pratikp, nicolas.guion, corbet, mark.rutland, mike.leach, tor,
	al.grant, zhang.lyra, linux-kernel, linux-arm-kernel, linux-api,
	linux-doc

Hi Chunyan,

[auto build test WARNING on robh/for-next]
[also build test WARNING on v4.5-rc2 next-20160203]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Chunyan-Zhang/Introduce-CoreSight-STM-support/20160203-161836
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux for-next


coccinelle warnings: (new ones prefixed by >>)

>> drivers/hwtracing/stm/policy.c:341:2-3: Unneeded semicolon

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH] stm class: fix semicolon.cocci warnings
  2016-02-03  8:15   ` Chunyan Zhang
@ 2016-02-03 10:05     ` kbuild test robot
  -1 siblings, 0 replies; 68+ messages in thread
From: kbuild test robot @ 2016-02-03 10:05 UTC (permalink / raw)
  To: Chunyan Zhang
  Cc: kbuild-all, mathieu.poirier, alexander.shishkin, robh, broonie,
	pratikp, nicolas.guion, corbet, mark.rutland, mike.leach, tor,
	al.grant, zhang.lyra, linux-kernel, linux-arm-kernel, linux-api,
	linux-doc

drivers/hwtracing/stm/policy.c:341:2-3: Unneeded semicolon


 Remove unneeded semicolon.

Generated by: scripts/coccinelle/misc/semicolon.cocci

CC: Chunyan Zhang <zhang.chunyan@linaro.org>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 policy.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -338,7 +338,7 @@ stp_policies_make(struct config_group *g
 		if (stm)
 			break;
 		*p = '.';
-	};
+	}
 
 	kfree(devname);
 

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name
@ 2016-02-03 10:05     ` kbuild test robot
  0 siblings, 0 replies; 68+ messages in thread
From: kbuild test robot @ 2016-02-03 10:05 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Chunyan,

[auto build test WARNING on robh/for-next]
[also build test WARNING on v4.5-rc2 next-20160203]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Chunyan-Zhang/Introduce-CoreSight-STM-support/20160203-161836
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux for-next


coccinelle warnings: (new ones prefixed by >>)

>> drivers/hwtracing/stm/policy.c:341:2-3: Unneeded semicolon

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH] stm class: fix semicolon.cocci warnings
@ 2016-02-03 10:05     ` kbuild test robot
  0 siblings, 0 replies; 68+ messages in thread
From: kbuild test robot @ 2016-02-03 10:05 UTC (permalink / raw)
  To: linux-arm-kernel

drivers/hwtracing/stm/policy.c:341:2-3: Unneeded semicolon


 Remove unneeded semicolon.

Generated by: scripts/coccinelle/misc/semicolon.cocci

CC: Chunyan Zhang <zhang.chunyan@linaro.org>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 policy.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -338,7 +338,7 @@ stp_policies_make(struct config_group *g
 		if (stm)
 			break;
 		*p = '.';
-	};
+	}
 
 	kfree(devname);
 

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name
  2016-02-03  8:15   ` Chunyan Zhang
@ 2016-02-04  8:56     ` Chunyan Zhang
  -1 siblings, 0 replies; 68+ messages in thread
From: Chunyan Zhang @ 2016-02-04  8:56 UTC (permalink / raw)
  To: mathieu.poirier, alexander.shishkin
  Cc: robh, broonie, pratikp, nicolas.guion, corbet, mark.rutland,
	mike.leach, tor, al.grant, zhang.lyra, linux-kernel,
	linux-arm-kernel, linux-api, linux-doc

The node name of STM master management policy is a concatenation of an
STM device name to which this policy applies and following an arbitrary
string, these two strings are concatenated with a dot.

This patch adds a loop for extracting the STM device name when an
arbitrary number of dot(s) are found in this STM device name.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 drivers/hwtracing/stm/policy.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
index 11ab6d0..691686e 100644
--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -321,21 +321,26 @@ stp_policies_make(struct config_group *group, const char *name)
 	/*
 	 * node must look like <device_name>.<policy_name>, where
 	 * <device_name> is the name of an existing stm device and
-	 * <policy_name> is an arbitrary string
+	 * <policy_name> is an arbitrary string, when an arbitrary
+	 * number of dot(s) are found in the <device_name>, the
+	 * first matched STM device name would be extracted.
 	 */
-	p = strchr(devname, '.');
-	if (!p) {
-		kfree(devname);
-		return ERR_PTR(-EINVAL);
-	}
+	for (p = devname; ; p++) {
+		p = strchr(p, '.');
+		if (!p) {
+			kfree(devname);
+			return ERR_PTR(-EINVAL);
+		}
 
-	*p++ = '\0';
+		*p = '\0';
 
-	stm = stm_find_device(devname);
-	kfree(devname);
+		stm = stm_find_device(devname);
+		if (stm)
+			break;
+		*p = '.';
+	}
 
-	if (!stm)
-		return ERR_PTR(-ENODEV);
+	kfree(devname);
 
 	mutex_lock(&stm->policy_mutex);
 	if (stm->policy) {
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 68+ messages in thread
* [PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name
@ 2016-02-04  8:56     ` Chunyan Zhang
  0 siblings, 0 replies; 68+ messages in thread
From: Chunyan Zhang @ 2016-02-04  8:56 UTC (permalink / raw)
  To: linux-arm-kernel

The node name of STM master management policy is a concatenation of an
STM device name to which this policy applies and following an arbitrary
string, these two strings are concatenated with a dot.

This patch adds a loop for extracting the STM device name when an
arbitrary number of dot(s) are found in this STM device name.

Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 drivers/hwtracing/stm/policy.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
index 11ab6d0..691686e 100644
--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -321,21 +321,26 @@ stp_policies_make(struct config_group *group, const char *name)
 	/*
 	 * node must look like <device_name>.<policy_name>, where
 	 * <device_name> is the name of an existing stm device and
-	 * <policy_name> is an arbitrary string
+	 * <policy_name> is an arbitrary string, when an arbitrary
+	 * number of dot(s) are found in the <device_name>, the
+	 * first matched STM device name would be extracted.
 	 */
-	p = strchr(devname, '.');
-	if (!p) {
-		kfree(devname);
-		return ERR_PTR(-EINVAL);
-	}
+	for (p = devname; ; p++) {
+		p = strchr(p, '.');
+		if (!p) {
+			kfree(devname);
+			return ERR_PTR(-EINVAL);
+		}
 
-	*p++ = '\0';
+		*p = '\0';
 
-	stm = stm_find_device(devname);
-	kfree(devname);
+		stm = stm_find_device(devname);
+		if (stm)
+			break;
+		*p = '.';
+	}
 
-	if (!stm)
-		return ERR_PTR(-ENODEV);
+	kfree(devname);
 
 	mutex_lock(&stm->policy_mutex);
 	if (stm->policy) {
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name
  2016-02-04  8:56     ` Chunyan Zhang
@ 2016-02-04 17:30       ` Alexander Shishkin
  -1 siblings, 0 replies; 68+ messages in thread
From: Alexander Shishkin @ 2016-02-04 17:30 UTC (permalink / raw)
  To: Chunyan Zhang, mathieu.poirier
  Cc: robh, broonie, pratikp, nicolas.guion, corbet, mark.rutland,
	mike.leach, tor, al.grant, zhang.lyra, linux-kernel,
	linux-arm-kernel, linux-api, linux-doc

Chunyan Zhang <zhang.chunyan@linaro.org> writes:

I few comments below:

> The node name of STM master management policy is a concatenation of an
> STM device name to which this policy applies and following an arbitrary
> string, these two strings are concatenated with a dot.

This is true.

> This patch adds a loop for extracting the STM device name when an
> arbitrary number of dot(s) are found in this STM device name.

It's not very easy to tell what's going on here from this
description. The reader be left curious as to why an arbitrary number of
dots is a reason to run a loop. When in doubt, try to imagine as if
you're seeing this patch for the first time and ask yourself, does the
message give a clear explanation of what's going on in it.

> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> ---
>  drivers/hwtracing/stm/policy.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
> index 11ab6d0..691686e 100644
> --- a/drivers/hwtracing/stm/policy.c
> +++ b/drivers/hwtracing/stm/policy.c
> @@ -321,21 +321,26 @@ stp_policies_make(struct config_group *group, const char *name)
>  	/*
>  	 * node must look like <device_name>.<policy_name>, where
>  	 * <device_name> is the name of an existing stm device and
> -	 * <policy_name> is an arbitrary string
> +	 * <policy_name> is an arbitrary string, when an arbitrary
> +	 * number of dot(s) are found in the <device_name>, the
> +	 * first matched STM device name would be extracted.
>  	 */

This leaves room for a number of suspicious situations. What if both
"xyz" and "xyz.0" are stm devices, how would you create a policy for the
latter, for example?

The rules should be such that you can tell exactly what the intended stm
device is from a policy directory name, otherwise it's just asking for
trouble.

> -	p = strchr(devname, '.');
> -	if (!p) {
> -		kfree(devname);
> -		return ERR_PTR(-EINVAL);
> -	}
> +	for (p = devname; ; p++) {
> +		p = strchr(p, '.');
> +		if (!p) {
> +			kfree(devname);
> +			return ERR_PTR(-EINVAL);
> +		}
>  
> -	*p++ = '\0';
> +		*p = '\0';
>  
> -	stm = stm_find_device(devname);
> -	kfree(devname);
> +		stm = stm_find_device(devname);
> +		if (stm)
> +			break;
> +		*p = '.';
> +	}
>  
> -	if (!stm)
> -		return ERR_PTR(-ENODEV);
> +	kfree(devname);

In the existing code there is a clear distinction between -ENODEV, which
is to say "we didn't find the device" and -EINVAL, "directory name
breaks rules/is badly formatted". After the change, it's all -EINVAL,
which also becomes "we tried everything, sorry".

So, having said all that, does the following patch solve your problem:

>From 870dc5fefa5623c39552511d31e0fa0da984d581 Mon Sep 17 00:00:00 2001
From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Date: Thu, 4 Feb 2016 18:56:34 +0200
Subject: [PATCH] stm class: Support devices with multiple instances

By convention, the name of the stm policy directory in configfs consists of
the device name to which it applies and the actual policy name, separated
by a dot. Now, some devices already have dots in their names that separate
name of the actual device from its instance identifier. Such devices will
result in two (or more, who can tell) dots in the policy directory name.

Existing policy code, however, will treat the first dot as the one that
separates device name from policy name, therefore failing the above case.

This patch makes the last dot in the directory name be the separator, thus
prohibiting dots from being used in policy names.

Suggested-by: Chunyan Zhang <zhang.chunyan@linaro.org>
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 drivers/hwtracing/stm/policy.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
index 94d3abfb73..1db189657b 100644
--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -332,10 +332,11 @@ stp_policies_make(struct config_group *group, const char *name)
 
 	/*
 	 * node must look like <device_name>.<policy_name>, where
-	 * <device_name> is the name of an existing stm device and
-	 * <policy_name> is an arbitrary string
+	 * <device_name> is the name of an existing stm device; may
+	 *               contain dots;
+	 * <policy_name> is an arbitrary string; may not contain dots
 	 */
-	p = strchr(devname, '.');
+	p = strrchr(devname, '.');
 	if (!p) {
 		kfree(devname);
 		return ERR_PTR(-EINVAL);
-- 
2.7.0

^ permalink raw reply related	[flat|nested] 68+ messages in thread
* [PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name
@ 2016-02-04 17:30       ` Alexander Shishkin
  0 siblings, 0 replies; 68+ messages in thread
From: Alexander Shishkin @ 2016-02-04 17:30 UTC (permalink / raw)
  To: linux-arm-kernel

Chunyan Zhang <zhang.chunyan@linaro.org> writes:

I few comments below:

> The node name of STM master management policy is a concatenation of an
> STM device name to which this policy applies and following an arbitrary
> string, these two strings are concatenated with a dot.

This is true.

> This patch adds a loop for extracting the STM device name when an
> arbitrary number of dot(s) are found in this STM device name.

It's not very easy to tell what's going on here from this
description. The reader be left curious as to why an arbitrary number of
dots is a reason to run a loop. When in doubt, try to imagine as if
you're seeing this patch for the first time and ask yourself, does the
message give a clear explanation of what's going on in it.

> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> ---
>  drivers/hwtracing/stm/policy.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
> index 11ab6d0..691686e 100644
> --- a/drivers/hwtracing/stm/policy.c
> +++ b/drivers/hwtracing/stm/policy.c
> @@ -321,21 +321,26 @@ stp_policies_make(struct config_group *group, const char *name)
>  	/*
>  	 * node must look like <device_name>.<policy_name>, where
>  	 * <device_name> is the name of an existing stm device and
> -	 * <policy_name> is an arbitrary string
> +	 * <policy_name> is an arbitrary string, when an arbitrary
> +	 * number of dot(s) are found in the <device_name>, the
> +	 * first matched STM device name would be extracted.
>  	 */

This leaves room for a number of suspicious situations. What if both
"xyz" and "xyz.0" are stm devices, how would you create a policy for the
latter, for example?

The rules should be such that you can tell exactly what the intended stm
device is from a policy directory name, otherwise it's just asking for
trouble.

> -	p = strchr(devname, '.');
> -	if (!p) {
> -		kfree(devname);
> -		return ERR_PTR(-EINVAL);
> -	}
> +	for (p = devname; ; p++) {
> +		p = strchr(p, '.');
> +		if (!p) {
> +			kfree(devname);
> +			return ERR_PTR(-EINVAL);
> +		}
>  
> -	*p++ = '\0';
> +		*p = '\0';
>  
> -	stm = stm_find_device(devname);
> -	kfree(devname);
> +		stm = stm_find_device(devname);
> +		if (stm)
> +			break;
> +		*p = '.';
> +	}
>  
> -	if (!stm)
> -		return ERR_PTR(-ENODEV);
> +	kfree(devname);

In the existing code there is a clear distinction between -ENODEV, which
is to say "we didn't find the device" and -EINVAL, "directory name
breaks rules/is badly formatted". After the change, it's all -EINVAL,
which also becomes "we tried everything, sorry".

So, having said all that, does the following patch solve your problem:

>From 870dc5fefa5623c39552511d31e0fa0da984d581 Mon Sep 17 00:00:00 2001
From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Date: Thu, 4 Feb 2016 18:56:34 +0200
Subject: [PATCH] stm class: Support devices with multiple instances

By convention, the name of the stm policy directory in configfs consists of
the device name to which it applies and the actual policy name, separated
by a dot. Now, some devices already have dots in their names that separate
name of the actual device from its instance identifier. Such devices will
result in two (or more, who can tell) dots in the policy directory name.

Existing policy code, however, will treat the first dot as the one that
separates device name from policy name, therefore failing the above case.

This patch makes the last dot in the directory name be the separator, thus
prohibiting dots from being used in policy names.

Suggested-by: Chunyan Zhang <zhang.chunyan@linaro.org>
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 drivers/hwtracing/stm/policy.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
index 94d3abfb73..1db189657b 100644
--- a/drivers/hwtracing/stm/policy.c
+++ b/drivers/hwtracing/stm/policy.c
@@ -332,10 +332,11 @@ stp_policies_make(struct config_group *group, const char *name)
 
 	/*
 	 * node must look like <device_name>.<policy_name>, where
-	 * <device_name> is the name of an existing stm device and
-	 * <policy_name> is an arbitrary string
+	 * <device_name> is the name of an existing stm device; may
+	 *               contain dots;
+	 * <policy_name> is an arbitrary string; may not contain dots
 	 */
-	p = strchr(devname, '.');
+	p = strrchr(devname, '.');
 	if (!p) {
 		kfree(devname);
 		return ERR_PTR(-EINVAL);
-- 
2.7.0

^ permalink raw reply related	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name
  2016-02-04 17:30       ` Alexander Shishkin
  (?)
@ 2016-02-05  3:18         ` Chunyan Zhang
  -1 siblings, 0 replies; 68+ messages in thread
From: Chunyan Zhang @ 2016-02-05  3:18 UTC (permalink / raw)
  To: Alexander Shishkin
  Cc: Mathieu Poirier, robh, Mark Brown, pratikp, nicolas.guion,
	Jon Corbet, Mark Rutland, Mike Leach, Tor Jeremiassen, Al Grant,
	Lyra Zhang, linux-kernel, linux-arm-kernel, linux-api, linux-doc

On Fri, Feb 5, 2016 at 1:30 AM, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>
> I few comments below:
>
>> The node name of STM master management policy is a concatenation of an
>> STM device name to which this policy applies and following an arbitrary
>> string, these two strings are concatenated with a dot.
>
> This is true.
>
>> This patch adds a loop for extracting the STM device name when an
>> arbitrary number of dot(s) are found in this STM device name.
>
> It's not very easy to tell what's going on here from this
> description. The reader be left curious as to why an arbitrary number of
> dots is a reason to run a loop. When in doubt, try to imagine as if
> you're seeing this patch for the first time and ask yourself, does the
> message give a clear explanation of what's going on in it.
>
>> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
>> ---
>>  drivers/hwtracing/stm/policy.c | 27 ++++++++++++++++-----------
>>  1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
>> index 11ab6d0..691686e 100644
>> --- a/drivers/hwtracing/stm/policy.c
>> +++ b/drivers/hwtracing/stm/policy.c
>> @@ -321,21 +321,26 @@ stp_policies_make(struct config_group *group, const char *name)
>>       /*
>>        * node must look like <device_name>.<policy_name>, where
>>        * <device_name> is the name of an existing stm device and
>> -      * <policy_name> is an arbitrary string
>> +      * <policy_name> is an arbitrary string, when an arbitrary
>> +      * number of dot(s) are found in the <device_name>, the
>> +      * first matched STM device name would be extracted.
>>        */
>
> This leaves room for a number of suspicious situations. What if both
> "xyz" and "xyz.0" are stm devices, how would you create a policy for the
> latter, for example?
>
> The rules should be such that you can tell exactly what the intended stm
> device is from a policy directory name, otherwise it's just asking for
> trouble.
>
>> -     p = strchr(devname, '.');
>> -     if (!p) {
>> -             kfree(devname);
>> -             return ERR_PTR(-EINVAL);
>> -     }
>> +     for (p = devname; ; p++) {
>> +             p = strchr(p, '.');
>> +             if (!p) {
>> +                     kfree(devname);
>> +                     return ERR_PTR(-EINVAL);
>> +             }
>>
>> -     *p++ = '\0';
>> +             *p = '\0';
>>
>> -     stm = stm_find_device(devname);
>> -     kfree(devname);
>> +             stm = stm_find_device(devname);
>> +             if (stm)
>> +                     break;
>> +             *p = '.';
>> +     }
>>
>> -     if (!stm)
>> -             return ERR_PTR(-ENODEV);
>> +     kfree(devname);
>
> In the existing code there is a clear distinction between -ENODEV, which
> is to say "we didn't find the device" and -EINVAL, "directory name
> breaks rules/is badly formatted". After the change, it's all -EINVAL,
> which also becomes "we tried everything, sorry".
>
> So, having said all that, does the following patch solve your problem:

Yes, I originally modified as well like your following patch, but at
that moment, I didn't get your agreement that the policy name (i.e. an
arbitrary string) cannot contain dots, so I had to consider the case.
Whatever, there isn't a panacea.  I'm very good with your patch.  Many
thanks for your review and providing the patch.


Chunyan

>
> From 870dc5fefa5623c39552511d31e0fa0da984d581 Mon Sep 17 00:00:00 2001
> From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Date: Thu, 4 Feb 2016 18:56:34 +0200
> Subject: [PATCH] stm class: Support devices with multiple instances
>
> By convention, the name of the stm policy directory in configfs consists of
> the device name to which it applies and the actual policy name, separated
> by a dot. Now, some devices already have dots in their names that separate
> name of the actual device from its instance identifier. Such devices will
> result in two (or more, who can tell) dots in the policy directory name.
>
> Existing policy code, however, will treat the first dot as the one that
> separates device name from policy name, therefore failing the above case.
>
> This patch makes the last dot in the directory name be the separator, thus
> prohibiting dots from being used in policy names.
>
> Suggested-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> ---
>  drivers/hwtracing/stm/policy.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
> index 94d3abfb73..1db189657b 100644
> --- a/drivers/hwtracing/stm/policy.c
> +++ b/drivers/hwtracing/stm/policy.c
> @@ -332,10 +332,11 @@ stp_policies_make(struct config_group *group, const char *name)
>
>         /*
>          * node must look like <device_name>.<policy_name>, where
> -        * <device_name> is the name of an existing stm device and
> -        * <policy_name> is an arbitrary string
> +        * <device_name> is the name of an existing stm device; may
> +        *               contain dots;
> +        * <policy_name> is an arbitrary string; may not contain dots
>          */
> -       p = strchr(devname, '.');
> +       p = strrchr(devname, '.');
>         if (!p) {
>                 kfree(devname);
>                 return ERR_PTR(-EINVAL);
> --
> 2.7.0
>

^ permalink raw reply	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name
@ 2016-02-05  3:18         ` Chunyan Zhang
  0 siblings, 0 replies; 68+ messages in thread
From: Chunyan Zhang @ 2016-02-05  3:18 UTC (permalink / raw)
  To: Alexander Shishkin
  Cc: Mathieu Poirier, robh, Mark Brown, pratikp, nicolas.guion,
	Jon Corbet, Mark Rutland, Mike Leach, Tor Jeremiassen, Al Grant,
	Lyra Zhang, linux-kernel, linux-arm-kernel, linux-api, linux-doc

On Fri, Feb 5, 2016 at 1:30 AM, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>
> I few comments below:
>
>> The node name of STM master management policy is a concatenation of an
>> STM device name to which this policy applies and following an arbitrary
>> string, these two strings are concatenated with a dot.
>
> This is true.
>
>> This patch adds a loop for extracting the STM device name when an
>> arbitrary number of dot(s) are found in this STM device name.
>
> It's not very easy to tell what's going on here from this
> description. The reader be left curious as to why an arbitrary number of
> dots is a reason to run a loop. When in doubt, try to imagine as if
> you're seeing this patch for the first time and ask yourself, does the
> message give a clear explanation of what's going on in it.
>
>> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
>> ---
>>  drivers/hwtracing/stm/policy.c | 27 ++++++++++++++++-----------
>>  1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
>> index 11ab6d0..691686e 100644
>> --- a/drivers/hwtracing/stm/policy.c
>> +++ b/drivers/hwtracing/stm/policy.c
>> @@ -321,21 +321,26 @@ stp_policies_make(struct config_group *group, const char *name)
>>       /*
>>        * node must look like <device_name>.<policy_name>, where
>>        * <device_name> is the name of an existing stm device and
>> -      * <policy_name> is an arbitrary string
>> +      * <policy_name> is an arbitrary string, when an arbitrary
>> +      * number of dot(s) are found in the <device_name>, the
>> +      * first matched STM device name would be extracted.
>>        */
>
> This leaves room for a number of suspicious situations. What if both
> "xyz" and "xyz.0" are stm devices, how would you create a policy for the
> latter, for example?
>
> The rules should be such that you can tell exactly what the intended stm
> device is from a policy directory name, otherwise it's just asking for
> trouble.
>
>> -     p = strchr(devname, '.');
>> -     if (!p) {
>> -             kfree(devname);
>> -             return ERR_PTR(-EINVAL);
>> -     }
>> +     for (p = devname; ; p++) {
>> +             p = strchr(p, '.');
>> +             if (!p) {
>> +                     kfree(devname);
>> +                     return ERR_PTR(-EINVAL);
>> +             }
>>
>> -     *p++ = '\0';
>> +             *p = '\0';
>>
>> -     stm = stm_find_device(devname);
>> -     kfree(devname);
>> +             stm = stm_find_device(devname);
>> +             if (stm)
>> +                     break;
>> +             *p = '.';
>> +     }
>>
>> -     if (!stm)
>> -             return ERR_PTR(-ENODEV);
>> +     kfree(devname);
>
> In the existing code there is a clear distinction between -ENODEV, which
> is to say "we didn't find the device" and -EINVAL, "directory name
> breaks rules/is badly formatted". After the change, it's all -EINVAL,
> which also becomes "we tried everything, sorry".
>
> So, having said all that, does the following patch solve your problem:

Yes, I originally modified as well like your following patch, but at
that moment, I didn't get your agreement that the policy name (i.e. an
arbitrary string) cannot contain dots, so I had to consider the case.
Whatever, there isn't a panacea.  I'm very good with your patch.  Many
thanks for your review and providing the patch.


Chunyan

>
> From 870dc5fefa5623c39552511d31e0fa0da984d581 Mon Sep 17 00:00:00 2001
> From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Date: Thu, 4 Feb 2016 18:56:34 +0200
> Subject: [PATCH] stm class: Support devices with multiple instances
>
> By convention, the name of the stm policy directory in configfs consists of
> the device name to which it applies and the actual policy name, separated
> by a dot. Now, some devices already have dots in their names that separate
> name of the actual device from its instance identifier. Such devices will
> result in two (or more, who can tell) dots in the policy directory name.
>
> Existing policy code, however, will treat the first dot as the one that
> separates device name from policy name, therefore failing the above case.
>
> This patch makes the last dot in the directory name be the separator, thus
> prohibiting dots from being used in policy names.
>
> Suggested-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> ---
>  drivers/hwtracing/stm/policy.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
> index 94d3abfb73..1db189657b 100644
> --- a/drivers/hwtracing/stm/policy.c
> +++ b/drivers/hwtracing/stm/policy.c
> @@ -332,10 +332,11 @@ stp_policies_make(struct config_group *group, const char *name)
>
>         /*
>          * node must look like <device_name>.<policy_name>, where
> -        * <device_name> is the name of an existing stm device and
> -        * <policy_name> is an arbitrary string
> +        * <device_name> is the name of an existing stm device; may
> +        *               contain dots;
> +        * <policy_name> is an arbitrary string; may not contain dots
>          */
> -       p = strchr(devname, '.');
> +       p = strrchr(devname, '.');
>         if (!p) {
>                 kfree(devname);
>                 return ERR_PTR(-EINVAL);
> --
> 2.7.0
>

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name
@ 2016-02-05  3:18         ` Chunyan Zhang
  0 siblings, 0 replies; 68+ messages in thread
From: Chunyan Zhang @ 2016-02-05  3:18 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Feb 5, 2016 at 1:30 AM, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>
> I few comments below:
>
>> The node name of STM master management policy is a concatenation of an
>> STM device name to which this policy applies and following an arbitrary
>> string, these two strings are concatenated with a dot.
>
> This is true.
>
>> This patch adds a loop for extracting the STM device name when an
>> arbitrary number of dot(s) are found in this STM device name.
>
> It's not very easy to tell what's going on here from this
> description. The reader be left curious as to why an arbitrary number of
> dots is a reason to run a loop. When in doubt, try to imagine as if
> you're seeing this patch for the first time and ask yourself, does the
> message give a clear explanation of what's going on in it.
>
>> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
>> ---
>>  drivers/hwtracing/stm/policy.c | 27 ++++++++++++++++-----------
>>  1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
>> index 11ab6d0..691686e 100644
>> --- a/drivers/hwtracing/stm/policy.c
>> +++ b/drivers/hwtracing/stm/policy.c
>> @@ -321,21 +321,26 @@ stp_policies_make(struct config_group *group, const char *name)
>>       /*
>>        * node must look like <device_name>.<policy_name>, where
>>        * <device_name> is the name of an existing stm device and
>> -      * <policy_name> is an arbitrary string
>> +      * <policy_name> is an arbitrary string, when an arbitrary
>> +      * number of dot(s) are found in the <device_name>, the
>> +      * first matched STM device name would be extracted.
>>        */
>
> This leaves room for a number of suspicious situations. What if both
> "xyz" and "xyz.0" are stm devices, how would you create a policy for the
> latter, for example?
>
> The rules should be such that you can tell exactly what the intended stm
> device is from a policy directory name, otherwise it's just asking for
> trouble.
>
>> -     p = strchr(devname, '.');
>> -     if (!p) {
>> -             kfree(devname);
>> -             return ERR_PTR(-EINVAL);
>> -     }
>> +     for (p = devname; ; p++) {
>> +             p = strchr(p, '.');
>> +             if (!p) {
>> +                     kfree(devname);
>> +                     return ERR_PTR(-EINVAL);
>> +             }
>>
>> -     *p++ = '\0';
>> +             *p = '\0';
>>
>> -     stm = stm_find_device(devname);
>> -     kfree(devname);
>> +             stm = stm_find_device(devname);
>> +             if (stm)
>> +                     break;
>> +             *p = '.';
>> +     }
>>
>> -     if (!stm)
>> -             return ERR_PTR(-ENODEV);
>> +     kfree(devname);
>
> In the existing code there is a clear distinction between -ENODEV, which
> is to say "we didn't find the device" and -EINVAL, "directory name
> breaks rules/is badly formatted". After the change, it's all -EINVAL,
> which also becomes "we tried everything, sorry".
>
> So, having said all that, does the following patch solve your problem:

Yes, I originally modified as well like your following patch, but at
that moment, I didn't get your agreement that the policy name (i.e. an
arbitrary string) cannot contain dots, so I had to consider the case.
Whatever, there isn't a panacea.  I'm very good with your patch.  Many
thanks for your review and providing the patch.


Chunyan

>
> From 870dc5fefa5623c39552511d31e0fa0da984d581 Mon Sep 17 00:00:00 2001
> From: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Date: Thu, 4 Feb 2016 18:56:34 +0200
> Subject: [PATCH] stm class: Support devices with multiple instances
>
> By convention, the name of the stm policy directory in configfs consists of
> the device name to which it applies and the actual policy name, separated
> by a dot. Now, some devices already have dots in their names that separate
> name of the actual device from its instance identifier. Such devices will
> result in two (or more, who can tell) dots in the policy directory name.
>
> Existing policy code, however, will treat the first dot as the one that
> separates device name from policy name, therefore failing the above case.
>
> This patch makes the last dot in the directory name be the separator, thus
> prohibiting dots from being used in policy names.
>
> Suggested-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> ---
>  drivers/hwtracing/stm/policy.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
> index 94d3abfb73..1db189657b 100644
> --- a/drivers/hwtracing/stm/policy.c
> +++ b/drivers/hwtracing/stm/policy.c
> @@ -332,10 +332,11 @@ stp_policies_make(struct config_group *group, const char *name)
>
>         /*
>          * node must look like <device_name>.<policy_name>, where
> -        * <device_name> is the name of an existing stm device and
> -        * <policy_name> is an arbitrary string
> +        * <device_name> is the name of an existing stm device; may
> +        *               contain dots;
> +        * <policy_name> is an arbitrary string; may not contain dots
>          */
> -       p = strchr(devname, '.');
> +       p = strrchr(devname, '.');
>         if (!p) {
>                 kfree(devname);
>                 return ERR_PTR(-EINVAL);
> --
> 2.7.0
>

^ permalink raw reply	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
  2016-02-03  8:15   ` Chunyan Zhang
@ 2016-02-05 12:52     ` Alexander Shishkin
  -1 siblings, 0 replies; 68+ messages in thread
From: Alexander Shishkin @ 2016-02-05 12:52 UTC (permalink / raw)
  To: Chunyan Zhang, mathieu.poirier
  Cc: robh, broonie, pratikp, nicolas.guion, corbet, mark.rutland,
	mike.leach, tor, al.grant, zhang.lyra, linux-kernel,
	linux-arm-kernel, linux-api, linux-doc

Chunyan Zhang <zhang.chunyan@linaro.org> writes:

> From: Mathieu Poirier <mathieu.poirier@linaro.org>
>
> Some architecture like ARM assign masterIDs statically at the HW design
> phase, making masterID manipulation in the generic STM core irrelevant.
>
> This patch adds a new 'mstatic' flag to struct stm_data that tells the
> core that this specific STM device doesn't need explicit masterID
> management.

So why do we need this patch? If your STM only has master 42 allocated
for software sources, simply set sw_start = 42, sw_end = 42 and you're
good to go, software will have exactly one channel to choose from. See
also the comment from <linux/stm.h>:

 * @sw_start:           first STP master available to software
 * @sw_end:             last STP master available to software

particularly the "available to software" part. Any other kinds of
masters the STM class code doesn't care about (yet).

> In the core sw_start/end of masterID are set to '1',
> i.e there is only one masterID to deal with.

This is also a completely arbitrary and unnecessary requirement. Again,
you can set both to 42 and it will still work.

> Also this patch depends on [1], so that the number of masterID
> is '1' too.
>
> Finally the lower and upper bound for masterIDs as presented
> in ($SYSFS)/class/stm/XYZ.stm/masters and
> ($SYSFS)/../stp-policy/XYZ.stm.my_policy/some_device/masters
> are set to '-1'.  That way users can't confuse them with
> architecture where masterID management is required (where any
> other value would be valid).

Why is this a good idea? Having the actual master there will allow
software to know what it is and also tell the decoding side what it is
(assuming you have more than one source in the STP stream, it might not
be easy to figure out, unless you know it in advance).

I don't see how one master statically assigned to software sources is
different from two masters or 32 masters. And I don't see any benefit of
hiding the master id from userspace. Am I missing something?

Regards,
--
Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-05 12:52     ` Alexander Shishkin
  0 siblings, 0 replies; 68+ messages in thread
From: Alexander Shishkin @ 2016-02-05 12:52 UTC (permalink / raw)
  To: linux-arm-kernel

Chunyan Zhang <zhang.chunyan@linaro.org> writes:

> From: Mathieu Poirier <mathieu.poirier@linaro.org>
>
> Some architecture like ARM assign masterIDs statically at the HW design
> phase, making masterID manipulation in the generic STM core irrelevant.
>
> This patch adds a new 'mstatic' flag to struct stm_data that tells the
> core that this specific STM device doesn't need explicit masterID
> management.

So why do we need this patch? If your STM only has master 42 allocated
for software sources, simply set sw_start = 42, sw_end = 42 and you're
good to go, software will have exactly one channel to choose from. See
also the comment from <linux/stm.h>:

 * @sw_start:           first STP master available to software
 * @sw_end:             last STP master available to software

particularly the "available to software" part. Any other kinds of
masters the STM class code doesn't care about (yet).

> In the core sw_start/end of masterID are set to '1',
> i.e there is only one masterID to deal with.

This is also a completely arbitrary and unnecessary requirement. Again,
you can set both to 42 and it will still work.

> Also this patch depends on [1], so that the number of masterID
> is '1' too.
>
> Finally the lower and upper bound for masterIDs as presented
> in ($SYSFS)/class/stm/XYZ.stm/masters and
> ($SYSFS)/../stp-policy/XYZ.stm.my_policy/some_device/masters
> are set to '-1'.  That way users can't confuse them with
> architecture where masterID management is required (where any
> other value would be valid).

Why is this a good idea? Having the actual master there will allow
software to know what it is and also tell the decoding side what it is
(assuming you have more than one source in the STP stream, it might not
be easy to figure out, unless you know it in advance).

I don't see how one master statically assigned to software sources is
different from two masters or 32 masters. And I don't see any benefit of
hiding the master id from userspace. Am I missing something?

Regards,
--
Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 1/6] stm class: Add ioctl get_options interface
@ 2016-02-05 12:55     ` Alexander Shishkin
  0 siblings, 0 replies; 68+ messages in thread
From: Alexander Shishkin @ 2016-02-05 12:55 UTC (permalink / raw)
  To: Chunyan Zhang, mathieu.poirier
  Cc: robh, broonie, pratikp, nicolas.guion, corbet, mark.rutland,
	mike.leach, tor, al.grant, zhang.lyra, linux-kernel,
	linux-arm-kernel, linux-api, linux-doc

Chunyan Zhang <zhang.chunyan@linaro.org> writes:

> There is already an interface of set_options, but no get_options yet.
> Before setting any options, one would may want to see the current
> status of that option by means of get_options interface. This
> interface has been used in CoreSight STM driver.
>
> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> ---
>  drivers/hwtracing/stm/core.c | 11 +++++++++++
>  include/linux/stm.h          |  3 +++
>  include/uapi/linux/stm.h     |  1 +
>  3 files changed, 15 insertions(+)
>
> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
> index b6445d9..86bb4e3 100644
> --- a/drivers/hwtracing/stm/core.c
> +++ b/drivers/hwtracing/stm/core.c
> @@ -571,6 +571,17 @@ stm_char_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  						    options);
>  
>  		break;
> +
> +	case STP_GET_OPTIONS:
> +		if (stm_data->get_options)
> +			err = stm_data->get_options(stm_data,
> +						    stmf->output.master,
> +						    stmf->output.channel,
> +						    stmf->output.nr_chans,
> +						    &options);
> +
> +		return copy_to_user((void __user *)arg, &options, sizeof(u64));

The return value of copy_to_user() is not what you assume it is.

Regards,
--
Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 1/6] stm class: Add ioctl get_options interface
@ 2016-02-05 12:55     ` Alexander Shishkin
  0 siblings, 0 replies; 68+ messages in thread
From: Alexander Shishkin @ 2016-02-05 12:55 UTC (permalink / raw)
  To: Chunyan Zhang, mathieu.poirier-QSEj5FYQhm4dnm+yROfE0A
  Cc: robh-DgEjT+Ai2ygdnm+yROfE0A, broonie-DgEjT+Ai2ygdnm+yROfE0A,
	pratikp-sgV2jX0FEOL9JmXXK+q4OQ, nicolas.guion-qxv4g6HH51o,
	corbet-T1hC0tSOHrs, mark.rutland-5wv7dgnIgG8,
	mike.leach-5wv7dgnIgG8, tor-l0cyMroinI0, al.grant-5wv7dgnIgG8,
	zhang.lyra-Re5JQEeQqe8AvxtiuMwx3w,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA

Chunyan Zhang <zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:

> There is already an interface of set_options, but no get_options yet.
> Before setting any options, one would may want to see the current
> status of that option by means of get_options interface. This
> interface has been used in CoreSight STM driver.
>
> Signed-off-by: Chunyan Zhang <zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/hwtracing/stm/core.c | 11 +++++++++++
>  include/linux/stm.h          |  3 +++
>  include/uapi/linux/stm.h     |  1 +
>  3 files changed, 15 insertions(+)
>
> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
> index b6445d9..86bb4e3 100644
> --- a/drivers/hwtracing/stm/core.c
> +++ b/drivers/hwtracing/stm/core.c
> @@ -571,6 +571,17 @@ stm_char_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  						    options);
>  
>  		break;
> +
> +	case STP_GET_OPTIONS:
> +		if (stm_data->get_options)
> +			err = stm_data->get_options(stm_data,
> +						    stmf->output.master,
> +						    stmf->output.channel,
> +						    stmf->output.nr_chans,
> +						    &options);
> +
> +		return copy_to_user((void __user *)arg, &options, sizeof(u64));

The return value of copy_to_user() is not what you assume it is.

Regards,
--
Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH V2 1/6] stm class: Add ioctl get_options interface
@ 2016-02-05 12:55     ` Alexander Shishkin
  0 siblings, 0 replies; 68+ messages in thread
From: Alexander Shishkin @ 2016-02-05 12:55 UTC (permalink / raw)
  To: linux-arm-kernel

Chunyan Zhang <zhang.chunyan@linaro.org> writes:

> There is already an interface of set_options, but no get_options yet.
> Before setting any options, one would may want to see the current
> status of that option by means of get_options interface. This
> interface has been used in CoreSight STM driver.
>
> Signed-off-by: Chunyan Zhang <zhang.chunyan@linaro.org>
> ---
>  drivers/hwtracing/stm/core.c | 11 +++++++++++
>  include/linux/stm.h          |  3 +++
>  include/uapi/linux/stm.h     |  1 +
>  3 files changed, 15 insertions(+)
>
> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
> index b6445d9..86bb4e3 100644
> --- a/drivers/hwtracing/stm/core.c
> +++ b/drivers/hwtracing/stm/core.c
> @@ -571,6 +571,17 @@ stm_char_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  						    options);
>  
>  		break;
> +
> +	case STP_GET_OPTIONS:
> +		if (stm_data->get_options)
> +			err = stm_data->get_options(stm_data,
> +						    stmf->output.master,
> +						    stmf->output.channel,
> +						    stmf->output.nr_chans,
> +						    &options);
> +
> +		return copy_to_user((void __user *)arg, &options, sizeof(u64));

The return value of copy_to_user() is not what you assume it is.

Regards,
--
Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 6/6] coresight-stm: adding driver for CoreSight STM component
  2016-02-03  8:15   ` Chunyan Zhang
@ 2016-02-05 13:06     ` Alexander Shishkin
  -1 siblings, 0 replies; 68+ messages in thread
From: Alexander Shishkin @ 2016-02-05 13:06 UTC (permalink / raw)
  To: Chunyan Zhang, mathieu.poirier
  Cc: robh, broonie, pratikp, nicolas.guion, corbet, mark.rutland,
	mike.leach, tor, al.grant, zhang.lyra, linux-kernel,
	linux-arm-kernel, linux-api, linux-doc, Russell King

Chunyan Zhang <zhang.chunyan@linaro.org> writes:

> +#ifndef CONFIG_64BIT
> +static inline void __raw_writeq(u64 val, volatile void __iomem *addr)
> +{
> +	asm volatile("strd %1, %0"
> +		     : "+Qo" (*(volatile u64 __force *)addr)
> +		     : "r" (val));
> +}

Is it really ok to do this for all !64bit arms, inside a driver, just
like that? I'm not an expert, but I'm pretty sure there's more to it.

Regards,
--
Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH V2 6/6] coresight-stm: adding driver for CoreSight STM component
@ 2016-02-05 13:06     ` Alexander Shishkin
  0 siblings, 0 replies; 68+ messages in thread
From: Alexander Shishkin @ 2016-02-05 13:06 UTC (permalink / raw)
  To: linux-arm-kernel

Chunyan Zhang <zhang.chunyan@linaro.org> writes:

> +#ifndef CONFIG_64BIT
> +static inline void __raw_writeq(u64 val, volatile void __iomem *addr)
> +{
> +	asm volatile("strd %1, %0"
> +		     : "+Qo" (*(volatile u64 __force *)addr)
> +		     : "r" (val));
> +}

Is it really ok to do this for all !64bit arms, inside a driver, just
like that? I'm not an expert, but I'm pretty sure there's more to it.

Regards,
--
Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 6/6] coresight-stm: adding driver for CoreSight STM component
@ 2016-02-05 14:30       ` Arnd Bergmann
  0 siblings, 0 replies; 68+ messages in thread
From: Arnd Bergmann @ 2016-02-05 14:30 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Alexander Shishkin, Chunyan Zhang, mathieu.poirier, mark.rutland,
	al.grant, corbet, zhang.lyra, linux-doc, linux-kernel, tor,
	broonie, mike.leach, linux-api, Russell King, pratikp,
	nicolas.guion

On Friday 05 February 2016 15:06:20 Alexander Shishkin wrote:
> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
> 
> > +#ifndef CONFIG_64BIT
> > +static inline void __raw_writeq(u64 val, volatile void __iomem *addr)
> > +{
> > +     asm volatile("strd %1, %0"
> > +                  : "+Qo" (*(volatile u64 __force *)addr)
> > +                  : "r" (val));
> > +}
> 
> Is it really ok to do this for all !64bit arms, inside a driver, just
> like that? I'm not an expert, but I'm pretty sure there's more to it.

It's normally device dependent whether this works or not, on 32-bit
architectures, a 64-bit access to an I/O bus tends to get split into
two 32 bit accesses and the order might not be the as what was
intended.

We have functions in include/linux/io-64-nonatomic-hi-lo.h
and include/linux/io-64-nonatomic-lo-hi.h that are meant to
do this right. Maybe the driver can be changed to use whichever
one is correct for it.

	Arnd

^ permalink raw reply	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 6/6] coresight-stm: adding driver for CoreSight STM component
@ 2016-02-05 14:30       ` Arnd Bergmann
  0 siblings, 0 replies; 68+ messages in thread
From: Arnd Bergmann @ 2016-02-05 14:30 UTC (permalink / raw)
  To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: Alexander Shishkin, Chunyan Zhang,
	mathieu.poirier-QSEj5FYQhm4dnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
	al.grant-5wv7dgnIgG8, corbet-T1hC0tSOHrs,
	zhang.lyra-Re5JQEeQqe8AvxtiuMwx3w,
	linux-doc-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, tor-l0cyMroinI0,
	broonie-DgEjT+Ai2ygdnm+yROfE0A, mike.leach-5wv7dgnIgG8,
	linux-api-u79uwXL29TY76Z2rM5mHXA, Russell King,
	pratikp-sgV2jX0FEOL9JmXXK+q4OQ, nicolas.guion-qxv4g6HH51o

On Friday 05 February 2016 15:06:20 Alexander Shishkin wrote:
> Chunyan Zhang <zhang.chunyan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:
> 
> > +#ifndef CONFIG_64BIT
> > +static inline void __raw_writeq(u64 val, volatile void __iomem *addr)
> > +{
> > +     asm volatile("strd %1, %0"
> > +                  : "+Qo" (*(volatile u64 __force *)addr)
> > +                  : "r" (val));
> > +}
> 
> Is it really ok to do this for all !64bit arms, inside a driver, just
> like that? I'm not an expert, but I'm pretty sure there's more to it.

It's normally device dependent whether this works or not, on 32-bit
architectures, a 64-bit access to an I/O bus tends to get split into
two 32 bit accesses and the order might not be the as what was
intended.

We have functions in include/linux/io-64-nonatomic-hi-lo.h
and include/linux/io-64-nonatomic-lo-hi.h that are meant to
do this right. Maybe the driver can be changed to use whichever
one is correct for it.

	Arnd

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH V2 6/6] coresight-stm: adding driver for CoreSight STM component
@ 2016-02-05 14:30       ` Arnd Bergmann
  0 siblings, 0 replies; 68+ messages in thread
From: Arnd Bergmann @ 2016-02-05 14:30 UTC (permalink / raw)
  To: linux-arm-kernel

On Friday 05 February 2016 15:06:20 Alexander Shishkin wrote:
> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
> 
> > +#ifndef CONFIG_64BIT
> > +static inline void __raw_writeq(u64 val, volatile void __iomem *addr)
> > +{
> > +     asm volatile("strd %1, %0"
> > +                  : "+Qo" (*(volatile u64 __force *)addr)
> > +                  : "r" (val));
> > +}
> 
> Is it really ok to do this for all !64bit arms, inside a driver, just
> like that? I'm not an expert, but I'm pretty sure there's more to it.

It's normally device dependent whether this works or not, on 32-bit
architectures, a 64-bit access to an I/O bus tends to get split into
two 32 bit accesses and the order might not be the as what was
intended.

We have functions in include/linux/io-64-nonatomic-hi-lo.h
and include/linux/io-64-nonatomic-lo-hi.h that are meant to
do this right. Maybe the driver can be changed to use whichever
one is correct for it.

	Arnd

^ permalink raw reply	[flat|nested] 68+ messages in thread
* RE: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
  2016-02-05 12:52     ` Alexander Shishkin
  (?)
@ 2016-02-05 16:31       ` Mike Leach
  -1 siblings, 0 replies; 68+ messages in thread
From: Mike Leach @ 2016-02-05 16:31 UTC (permalink / raw)
  To: Alexander Shishkin, Chunyan Zhang, mathieu.poirier
  Cc: robh, broonie, pratikp, nicolas.guion, corbet, Mark Rutland, tor,
	Al Grant, zhang.lyra, linux-kernel, linux-arm-kernel, linux-api,
	linux-doc

Hi,

I think a quick clarification of the ARM hardware STM architecture may be of value here.

The ARM hardware STM, when implemented as recommend in a hardware design, the master IDs are not under driver control, but have a hardwire association with source devices in the system. The master IDs are hardwired to individual cores and core security states, and there could be other IDs associated with hardware trace sources requiring output via the hardware STM. (an example of this is the ARM Juno development board).

Therefore in multi-core systems many masters may be associated with a single software STM stream. A user-space application running on Core 1, may have a master ID of 0x11 in the STPv2 trace stream, but if this application is context switched and later continues running on Core 2, then master ID could change to 0x21.  Masters identify a hardware source in this scheme, the precise values used will be implementation dependent.

So the number of masters "available to the software" depends on your interpretation of the phrase.
If you mean "master IDs on which software trace will appear" then that will be many - it depends on which core is running the application. However as described above, this is not predictable by any decoder, and the master set used may not be contiguous.
If you mean "master IDs selectable/allocated by the driver" then that will be 0 - the driver has no control, and possibly no knowledge of which master is associated with the current execution context. (this is of course system dependent - it may well be possible to have some configuration database associating cores with hardware IDs, but this will be implementation and board support dependant).

Therefore, there is a requirement to tell both the user-space STM client and decoder that not only is master ID management not needed - it is in fact not possible and the key to identify the software source for a given STM packet is the channel alone. In addition we have a nominal single Master ID "available to the software", to satisfy the requirements of the generic ST module API.

So on Juno for example, while we do have 128 masters, each with 65536 channels, the master allocation is not visible to system software - each core sees a single master with 65536 channels. Therefore differentiation between software sources in the STM trace is by channel number allocations only.

Best Regards

Mike.

----------------------------------------------------------------
Mike Leach                           +44 (0)1254 893911 (Direct)
Principal Engineer                   +44 (0)1254 893900 (Main)
Arm Blackburn Design Centre          +44 (0)1254 893901 (Fax)
Belthorn House
Walker Rd                            mailto:mike.leach@arm.com
Guide
Blackburn
BB1 2QE
----------------------------------------------------------------


> -----Original Message-----
> From: Alexander Shishkin [mailto:alexander.shishkin@linux.intel.com]
> Sent: 05 February 2016 12:52
> To: Chunyan Zhang; mathieu.poirier@linaro.org
> Cc: robh@kernel.org; broonie@kernel.org; pratikp@codeaurora.org;
> nicolas.guion@st.com; corbet@lwn.net; Mark Rutland; Mike Leach;
> tor@ti.com; Al Grant; zhang.lyra@gmail.com; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-api@vger.kernel.org; linux-
> doc@vger.kernel.org
> Subject: Re: [PATCH V2 3/6] stm class: provision for statically assigned
> masterIDs
>
> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>
> > From: Mathieu Poirier <mathieu.poirier@linaro.org>
> >
> > Some architecture like ARM assign masterIDs statically at the HW design
> > phase, making masterID manipulation in the generic STM core irrelevant.
> >
> > This patch adds a new 'mstatic' flag to struct stm_data that tells the
> > core that this specific STM device doesn't need explicit masterID
> > management.
>
> So why do we need this patch? If your STM only has master 42 allocated
> for software sources, simply set sw_start = 42, sw_end = 42 and you're
> good to go, software will have exactly one channel to choose from. See
> also the comment from <linux/stm.h>:
>
>  * @sw_start:           first STP master available to software
>  * @sw_end:             last STP master available to software
>
> particularly the "available to software" part. Any other kinds of
> masters the STM class code doesn't care about (yet).
>
> > In the core sw_start/end of masterID are set to '1',
> > i.e there is only one masterID to deal with.
>
> This is also a completely arbitrary and unnecessary requirement. Again,
> you can set both to 42 and it will still work.
>
> > Also this patch depends on [1], so that the number of masterID
> > is '1' too.
> >
> > Finally the lower and upper bound for masterIDs as presented
> > in ($SYSFS)/class/stm/XYZ.stm/masters and
> > ($SYSFS)/../stp-policy/XYZ.stm.my_policy/some_device/masters
> > are set to '-1'.  That way users can't confuse them with
> > architecture where masterID management is required (where any
> > other value would be valid).
>
> Why is this a good idea? Having the actual master there will allow
> software to know what it is and also tell the decoding side what it is
> (assuming you have more than one source in the STP stream, it might not
> be easy to figure out, unless you know it in advance).
>
> I don't see how one master statically assigned to software sources is
> different from two masters or 32 masters. And I don't see any benefit of
> hiding the master id from userspace. Am I missing something?
>
> Regards,
> --
> Alex

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

^ permalink raw reply	[flat|nested] 68+ messages in thread
* RE: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-05 16:31       ` Mike Leach
  0 siblings, 0 replies; 68+ messages in thread
From: Mike Leach @ 2016-02-05 16:31 UTC (permalink / raw)
  To: Alexander Shishkin, Chunyan Zhang, mathieu.poirier
  Cc: robh, broonie, pratikp, nicolas.guion, corbet, Mark Rutland, tor,
	Al Grant, zhang.lyra, linux-kernel, linux-arm-kernel, linux-api,
	linux-doc

Hi,

I think a quick clarification of the ARM hardware STM architecture may be of value here.

The ARM hardware STM, when implemented as recommend in a hardware design, the master IDs are not under driver control, but have a hardwire association with source devices in the system. The master IDs are hardwired to individual cores and core security states, and there could be other IDs associated with hardware trace sources requiring output via the hardware STM. (an example of this is the ARM Juno development board).

Therefore in multi-core systems many masters may be associated with a single software STM stream. A user-space application running on Core 1, may have a master ID of 0x11 in the STPv2 trace stream, but if this application is context switched and later continues running on Core 2, then master ID could change to 0x21.  Masters identify a hardware source in this scheme, the precise values used will be implementation dependent.

So the number of masters "available to the software" depends on your interpretation of the phrase.
If you mean "master IDs on which software trace will appear" then that will be many - it depends on which core is running the application. However as described above, this is not predictable by any decoder, and the master set used may not be contiguous.
If you mean "master IDs selectable/allocated by the driver" then that will be 0 - the driver has no control, and possibly no knowledge of which master is associated with the current execution context. (this is of course system dependent - it may well be possible to have some configuration database associating cores with hardware IDs, but this will be implementation and board support dependant).

Therefore, there is a requirement to tell both the user-space STM client and decoder that not only is master ID management not needed - it is in fact not possible and the key to identify the software source for a given STM packet is the channel alone. In addition we have a nominal single Master ID "available to the software", to satisfy the requirements of the generic ST module API.

So on Juno for example, while we do have 128 masters, each with 65536 channels, the master allocation is not visible to system software - each core sees a single master with 65536 channels. Therefore differentiation between software sources in the STM trace is by channel number allocations only.

Best Regards

Mike.

----------------------------------------------------------------
Mike Leach                           +44 (0)1254 893911 (Direct)
Principal Engineer                   +44 (0)1254 893900 (Main)
Arm Blackburn Design Centre          +44 (0)1254 893901 (Fax)
Belthorn House
Walker Rd                            mailto:mike.leach@arm.com
Guide
Blackburn
BB1 2QE
----------------------------------------------------------------


> -----Original Message-----
> From: Alexander Shishkin [mailto:alexander.shishkin@linux.intel.com]
> Sent: 05 February 2016 12:52
> To: Chunyan Zhang; mathieu.poirier@linaro.org
> Cc: robh@kernel.org; broonie@kernel.org; pratikp@codeaurora.org;
> nicolas.guion@st.com; corbet@lwn.net; Mark Rutland; Mike Leach;
> tor@ti.com; Al Grant; zhang.lyra@gmail.com; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-api@vger.kernel.org; linux-
> doc@vger.kernel.org
> Subject: Re: [PATCH V2 3/6] stm class: provision for statically assigned
> masterIDs
>
> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>
> > From: Mathieu Poirier <mathieu.poirier@linaro.org>
> >
> > Some architecture like ARM assign masterIDs statically at the HW design
> > phase, making masterID manipulation in the generic STM core irrelevant.
> >
> > This patch adds a new 'mstatic' flag to struct stm_data that tells the
> > core that this specific STM device doesn't need explicit masterID
> > management.
>
> So why do we need this patch? If your STM only has master 42 allocated
> for software sources, simply set sw_start = 42, sw_end = 42 and you're
> good to go, software will have exactly one channel to choose from. See
> also the comment from <linux/stm.h>:
>
>  * @sw_start:           first STP master available to software
>  * @sw_end:             last STP master available to software
>
> particularly the "available to software" part. Any other kinds of
> masters the STM class code doesn't care about (yet).
>
> > In the core sw_start/end of masterID are set to '1',
> > i.e there is only one masterID to deal with.
>
> This is also a completely arbitrary and unnecessary requirement. Again,
> you can set both to 42 and it will still work.
>
> > Also this patch depends on [1], so that the number of masterID
> > is '1' too.
> >
> > Finally the lower and upper bound for masterIDs as presented
> > in ($SYSFS)/class/stm/XYZ.stm/masters and
> > ($SYSFS)/../stp-policy/XYZ.stm.my_policy/some_device/masters
> > are set to '-1'.  That way users can't confuse them with
> > architecture where masterID management is required (where any
> > other value would be valid).
>
> Why is this a good idea? Having the actual master there will allow
> software to know what it is and also tell the decoding side what it is
> (assuming you have more than one source in the STP stream, it might not
> be easy to figure out, unless you know it in advance).
>
> I don't see how one master statically assigned to software sources is
> different from two masters or 32 masters. And I don't see any benefit of
> hiding the master id from userspace. Am I missing something?
>
> Regards,
> --
> Alex

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-05 16:31       ` Mike Leach
  0 siblings, 0 replies; 68+ messages in thread
From: Mike Leach @ 2016-02-05 16:31 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

I think a quick clarification of the ARM hardware STM architecture may be of value here.

The ARM hardware STM, when implemented as recommend in a hardware design, the master IDs are not under driver control, but have a hardwire association with source devices in the system. The master IDs are hardwired to individual cores and core security states, and there could be other IDs associated with hardware trace sources requiring output via the hardware STM. (an example of this is the ARM Juno development board).

Therefore in multi-core systems many masters may be associated with a single software STM stream. A user-space application running on Core 1, may have a master ID of 0x11 in the STPv2 trace stream, but if this application is context switched and later continues running on Core 2, then master ID could change to 0x21.  Masters identify a hardware source in this scheme, the precise values used will be implementation dependent.

So the number of masters "available to the software" depends on your interpretation of the phrase.
If you mean "master IDs on which software trace will appear" then that will be many - it depends on which core is running the application. However as described above, this is not predictable by any decoder, and the master set used may not be contiguous.
If you mean "master IDs selectable/allocated by the driver" then that will be 0 - the driver has no control, and possibly no knowledge of which master is associated with the current execution context. (this is of course system dependent - it may well be possible to have some configuration database associating cores with hardware IDs, but this will be implementation and board support dependant).

Therefore, there is a requirement to tell both the user-space STM client and decoder that not only is master ID management not needed - it is in fact not possible and the key to identify the software source for a given STM packet is the channel alone. In addition we have a nominal single Master ID "available to the software", to satisfy the requirements of the generic ST module API.

So on Juno for example, while we do have 128 masters, each with 65536 channels, the master allocation is not visible to system software - each core sees a single master with 65536 channels. Therefore differentiation between software sources in the STM trace is by channel number allocations only.

Best Regards

Mike.

----------------------------------------------------------------
Mike Leach                           +44 (0)1254 893911 (Direct)
Principal Engineer                   +44 (0)1254 893900 (Main)
Arm Blackburn Design Centre          +44 (0)1254 893901 (Fax)
Belthorn House
Walker Rd                            mailto:mike.leach at arm.com
Guide
Blackburn
BB1 2QE
----------------------------------------------------------------


> -----Original Message-----
> From: Alexander Shishkin [mailto:alexander.shishkin at linux.intel.com]
> Sent: 05 February 2016 12:52
> To: Chunyan Zhang; mathieu.poirier at linaro.org
> Cc: robh at kernel.org; broonie at kernel.org; pratikp at codeaurora.org;
> nicolas.guion at st.com; corbet at lwn.net; Mark Rutland; Mike Leach;
> tor at ti.com; Al Grant; zhang.lyra at gmail.com; linux-kernel at vger.kernel.org;
> linux-arm-kernel at lists.infradead.org; linux-api at vger.kernel.org; linux-
> doc at vger.kernel.org
> Subject: Re: [PATCH V2 3/6] stm class: provision for statically assigned
> masterIDs
>
> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>
> > From: Mathieu Poirier <mathieu.poirier@linaro.org>
> >
> > Some architecture like ARM assign masterIDs statically at the HW design
> > phase, making masterID manipulation in the generic STM core irrelevant.
> >
> > This patch adds a new 'mstatic' flag to struct stm_data that tells the
> > core that this specific STM device doesn't need explicit masterID
> > management.
>
> So why do we need this patch? If your STM only has master 42 allocated
> for software sources, simply set sw_start = 42, sw_end = 42 and you're
> good to go, software will have exactly one channel to choose from. See
> also the comment from <linux/stm.h>:
>
>  * @sw_start:           first STP master available to software
>  * @sw_end:             last STP master available to software
>
> particularly the "available to software" part. Any other kinds of
> masters the STM class code doesn't care about (yet).
>
> > In the core sw_start/end of masterID are set to '1',
> > i.e there is only one masterID to deal with.
>
> This is also a completely arbitrary and unnecessary requirement. Again,
> you can set both to 42 and it will still work.
>
> > Also this patch depends on [1], so that the number of masterID
> > is '1' too.
> >
> > Finally the lower and upper bound for masterIDs as presented
> > in ($SYSFS)/class/stm/XYZ.stm/masters and
> > ($SYSFS)/../stp-policy/XYZ.stm.my_policy/some_device/masters
> > are set to '-1'.  That way users can't confuse them with
> > architecture where masterID management is required (where any
> > other value would be valid).
>
> Why is this a good idea? Having the actual master there will allow
> software to know what it is and also tell the decoding side what it is
> (assuming you have more than one source in the STP stream, it might not
> be easy to figure out, unless you know it in advance).
>
> I don't see how one master statically assigned to software sources is
> different from two masters or 32 masters. And I don't see any benefit of
> hiding the master id from userspace. Am I missing something?
>
> Regards,
> --
> Alex

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

^ permalink raw reply	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
  2016-02-05 12:52     ` Alexander Shishkin
  (?)
@ 2016-02-05 18:08       ` Mathieu Poirier
  -1 siblings, 0 replies; 68+ messages in thread
From: Mathieu Poirier @ 2016-02-05 18:08 UTC (permalink / raw)
  To: Alexander Shishkin
  Cc: Chunyan Zhang, robh, Mark Brown, Pratik Patel, Nicolas GUION,
	Jon Corbet, Mark Rutland, Mike Leach, Jeremiassen, Tor, Al Grant,
	Lyra Zhang, linux-kernel, linux-arm-kernel, linux-api, linux-doc

On 5 February 2016 at 05:52, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>
>> From: Mathieu Poirier <mathieu.poirier@linaro.org>
>>
>> Some architecture like ARM assign masterIDs statically at the HW design
>> phase, making masterID manipulation in the generic STM core irrelevant.
>>
>> This patch adds a new 'mstatic' flag to struct stm_data that tells the
>> core that this specific STM device doesn't need explicit masterID
>> management.
>
> So why do we need this patch? If your STM only has master 42 allocated
> for software sources, simply set sw_start = 42, sw_end = 42 and you're
> good to go, software will have exactly one channel to choose from. See
> also the comment from <linux/stm.h>:

On ARM each source, i.e entity capable of accessing STM channels, has
a different master ID set in HW.  We can't assume the IDs are
contiguous and from a SW point of view there is no way to probe the
values.

>
>  * @sw_start:           first STP master available to software
>  * @sw_end:             last STP master available to software
>
> particularly the "available to software" part. Any other kinds of
> masters the STM class code doesn't care about (yet).
>
>> In the core sw_start/end of masterID are set to '1',
>> i.e there is only one masterID to deal with.
>
> This is also a completely arbitrary and unnecessary requirement. Again,
> you can set both to 42 and it will still work.

True - any value will do. The important thing to remember is that on
ARM, there is only one masterID channel (from an STM core point of
view).  But we could also proceed differently, see below for more
details.

>
>> Also this patch depends on [1], so that the number of masterID
>> is '1' too.
>>
>> Finally the lower and upper bound for masterIDs as presented
>> in ($SYSFS)/class/stm/XYZ.stm/masters and
>> ($SYSFS)/../stp-policy/XYZ.stm.my_policy/some_device/masters
>> are set to '-1'.  That way users can't confuse them with
>> architecture where masterID management is required (where any
>> other value would be valid).
>
> Why is this a good idea? Having the actual master there will allow
> software to know what it is and also tell the decoding side what it is
> (assuming you have more than one source in the STP stream, it might not
> be easy to figure out, unless you know it in advance).

In the ARM world masterIDs are irrelevant so why bother with them?
Writing a '-1' simply highlight this reality.

Another way of approaching the problem would be to change sw_start/end
to a bitfield that represent the valid masterIDs but in my opinion, it
is a lot of code churn for information that isn't used outside of the
decoder.

Thanks,
Mathieu

>
> I don't see how one master statically assigned to software sources is
> different from two masters or 32 masters. And I don't see any benefit of
> hiding the master id from userspace. Am I missing something?
>
> Regards,
> --
> Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-05 18:08       ` Mathieu Poirier
  0 siblings, 0 replies; 68+ messages in thread
From: Mathieu Poirier @ 2016-02-05 18:08 UTC (permalink / raw)
  To: Alexander Shishkin
  Cc: Chunyan Zhang, robh, Mark Brown, Pratik Patel, Nicolas GUION,
	Jon Corbet, Mark Rutland, Mike Leach, Jeremiassen, Tor, Al Grant,
	Lyra Zhang, linux-kernel, linux-arm-kernel, linux-api, linux-doc

On 5 February 2016 at 05:52, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>
>> From: Mathieu Poirier <mathieu.poirier@linaro.org>
>>
>> Some architecture like ARM assign masterIDs statically at the HW design
>> phase, making masterID manipulation in the generic STM core irrelevant.
>>
>> This patch adds a new 'mstatic' flag to struct stm_data that tells the
>> core that this specific STM device doesn't need explicit masterID
>> management.
>
> So why do we need this patch? If your STM only has master 42 allocated
> for software sources, simply set sw_start = 42, sw_end = 42 and you're
> good to go, software will have exactly one channel to choose from. See
> also the comment from <linux/stm.h>:

On ARM each source, i.e entity capable of accessing STM channels, has
a different master ID set in HW.  We can't assume the IDs are
contiguous and from a SW point of view there is no way to probe the
values.

>
>  * @sw_start:           first STP master available to software
>  * @sw_end:             last STP master available to software
>
> particularly the "available to software" part. Any other kinds of
> masters the STM class code doesn't care about (yet).
>
>> In the core sw_start/end of masterID are set to '1',
>> i.e there is only one masterID to deal with.
>
> This is also a completely arbitrary and unnecessary requirement. Again,
> you can set both to 42 and it will still work.

True - any value will do. The important thing to remember is that on
ARM, there is only one masterID channel (from an STM core point of
view).  But we could also proceed differently, see below for more
details.

>
>> Also this patch depends on [1], so that the number of masterID
>> is '1' too.
>>
>> Finally the lower and upper bound for masterIDs as presented
>> in ($SYSFS)/class/stm/XYZ.stm/masters and
>> ($SYSFS)/../stp-policy/XYZ.stm.my_policy/some_device/masters
>> are set to '-1'.  That way users can't confuse them with
>> architecture where masterID management is required (where any
>> other value would be valid).
>
> Why is this a good idea? Having the actual master there will allow
> software to know what it is and also tell the decoding side what it is
> (assuming you have more than one source in the STP stream, it might not
> be easy to figure out, unless you know it in advance).

In the ARM world masterIDs are irrelevant so why bother with them?
Writing a '-1' simply highlight this reality.

Another way of approaching the problem would be to change sw_start/end
to a bitfield that represent the valid masterIDs but in my opinion, it
is a lot of code churn for information that isn't used outside of the
decoder.

Thanks,
Mathieu

>
> I don't see how one master statically assigned to software sources is
> different from two masters or 32 masters. And I don't see any benefit of
> hiding the master id from userspace. Am I missing something?
>
> Regards,
> --
> Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-05 18:08       ` Mathieu Poirier
  0 siblings, 0 replies; 68+ messages in thread
From: Mathieu Poirier @ 2016-02-05 18:08 UTC (permalink / raw)
  To: linux-arm-kernel

On 5 February 2016 at 05:52, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>
>> From: Mathieu Poirier <mathieu.poirier@linaro.org>
>>
>> Some architecture like ARM assign masterIDs statically at the HW design
>> phase, making masterID manipulation in the generic STM core irrelevant.
>>
>> This patch adds a new 'mstatic' flag to struct stm_data that tells the
>> core that this specific STM device doesn't need explicit masterID
>> management.
>
> So why do we need this patch? If your STM only has master 42 allocated
> for software sources, simply set sw_start = 42, sw_end = 42 and you're
> good to go, software will have exactly one channel to choose from. See
> also the comment from <linux/stm.h>:

On ARM each source, i.e entity capable of accessing STM channels, has
a different master ID set in HW.  We can't assume the IDs are
contiguous and from a SW point of view there is no way to probe the
values.

>
>  * @sw_start:           first STP master available to software
>  * @sw_end:             last STP master available to software
>
> particularly the "available to software" part. Any other kinds of
> masters the STM class code doesn't care about (yet).
>
>> In the core sw_start/end of masterID are set to '1',
>> i.e there is only one masterID to deal with.
>
> This is also a completely arbitrary and unnecessary requirement. Again,
> you can set both to 42 and it will still work.

True - any value will do. The important thing to remember is that on
ARM, there is only one masterID channel (from an STM core point of
view).  But we could also proceed differently, see below for more
details.

>
>> Also this patch depends on [1], so that the number of masterID
>> is '1' too.
>>
>> Finally the lower and upper bound for masterIDs as presented
>> in ($SYSFS)/class/stm/XYZ.stm/masters and
>> ($SYSFS)/../stp-policy/XYZ.stm.my_policy/some_device/masters
>> are set to '-1'.  That way users can't confuse them with
>> architecture where masterID management is required (where any
>> other value would be valid).
>
> Why is this a good idea? Having the actual master there will allow
> software to know what it is and also tell the decoding side what it is
> (assuming you have more than one source in the STP stream, it might not
> be easy to figure out, unless you know it in advance).

In the ARM world masterIDs are irrelevant so why bother with them?
Writing a '-1' simply highlight this reality.

Another way of approaching the problem would be to change sw_start/end
to a bitfield that represent the valid masterIDs but in my opinion, it
is a lot of code churn for information that isn't used outside of the
decoder.

Thanks,
Mathieu

>
> I don't see how one master statically assigned to software sources is
> different from two masters or 32 masters. And I don't see any benefit of
> hiding the master id from userspace. Am I missing something?
>
> Regards,
> --
> Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* RE: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-08 10:52         ` Alexander Shishkin
  0 siblings, 0 replies; 68+ messages in thread
From: Alexander Shishkin @ 2016-02-08 10:52 UTC (permalink / raw)
  To: Mike Leach, Chunyan Zhang, mathieu.poirier
  Cc: robh, broonie, pratikp, nicolas.guion, corbet, Mark Rutland, tor,
	Al Grant, zhang.lyra, linux-kernel, linux-arm-kernel, linux-api,
	linux-doc

Mike Leach <Mike.Leach@arm.com> writes:

> Hi,
>
> I think a quick clarification of the ARM hardware STM architecture may be of value here.
>
> The ARM hardware STM, when implemented as recommend in a hardware design, the master IDs are not under driver control, but have a hardwire association with source devices in the system. The master IDs are hardwired to individual cores and core security states, and there could be other IDs associated with hardware trace sources requiring output via the hardware STM. (an example of this is the ARM Juno development board).
>
> Therefore in multi-core systems many masters may be associated with a single software STM stream. A user-space application running on Core 1, may have a master ID of 0x11 in the STPv2 trace stream, but if this application is context switched and later continues running on Core 2, then master ID could change to 0x21.  Masters identify a hardware source in this scheme, the precise values used will be implementation dependent.
>
> So the number of masters "available to the software" depends on your interpretation of the phrase.
> If you mean "master IDs on which software trace will appear" then that will be many - it depends on which core is running the application. However as described above, this is not predictable by any decoder, and the master set used may not be contiguous.
> If you mean "master IDs selectable/allocated by the driver" then that will be 0 - the driver has no control, and possibly no knowledge of which master is associated with the current execution context. (this is of course system dependent - it may well be possible to have some configuration database associating cores with hardware IDs, but this will be implementation and board support dependant).
>
> Therefore, there is a requirement to tell both the user-space STM client and decoder that not only is master ID management not needed - it is in fact not possible and the key to identify the software source for a given STM packet is the channel alone. In addition we have a nominal single Master ID "available to the software", to satisfy the requirements of the generic ST module API.
>
> So on Juno for example, while we do have 128 masters, each with 65536 channels, the master allocation is not visible to system software - each core sees a single master with 65536 channels. Therefore differentiation between software sources in the STM trace is by channel number allocations only.

Ok, thanks a lot for the detailed explanation. I was confused by the
"static" bit in the patch description. It looks like something along the
lines of this patch might indeed be in order.

Regards,
--
Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* RE: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-08 10:52         ` Alexander Shishkin
  0 siblings, 0 replies; 68+ messages in thread
From: Alexander Shishkin @ 2016-02-08 10:52 UTC (permalink / raw)
  To: Mike Leach, Chunyan Zhang, mathieu.poirier@linaro.org
  Cc: robh@kernel.org, broonie@kernel.org, pratikp@codeaurora.org,
	nicolas.guion@st.com, corbet@lwn.net, Mark Rutland, tor@ti.com,
	Al Grant, zhang.lyra@gmail.com, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-api@vger.kernel.org,
	linux-doc@vger.kernel.org

Mike Leach <Mike.Leach-5wv7dgnIgG8@public.gmane.org> writes:

> Hi,
>
> I think a quick clarification of the ARM hardware STM architecture may be of value here.
>
> The ARM hardware STM, when implemented as recommend in a hardware design, the master IDs are not under driver control, but have a hardwire association with source devices in the system. The master IDs are hardwired to individual cores and core security states, and there could be other IDs associated with hardware trace sources requiring output via the hardware STM. (an example of this is the ARM Juno development board).
>
> Therefore in multi-core systems many masters may be associated with a single software STM stream. A user-space application running on Core 1, may have a master ID of 0x11 in the STPv2 trace stream, but if this application is context switched and later continues running on Core 2, then master ID could change to 0x21.  Masters identify a hardware source in this scheme, the precise values used will be implementation dependent.
>
> So the number of masters "available to the software" depends on your interpretation of the phrase.
> If you mean "master IDs on which software trace will appear" then that will be many - it depends on which core is running the application. However as described above, this is not predictable by any decoder, and the master set used may not be contiguous.
> If you mean "master IDs selectable/allocated by the driver" then that will be 0 - the driver has no control, and possibly no knowledge of which master is associated with the current execution context. (this is of course system dependent - it may well be possible to have some configuration database associating cores with hardware IDs, but this will be implementation and board support dependant).
>
> Therefore, there is a requirement to tell both the user-space STM client and decoder that not only is master ID management not needed - it is in fact not possible and the key to identify the software source for a given STM packet is the channel alone. In addition we have a nominal single Master ID "available to the software", to satisfy the requirements of the generic ST module API.
>
> So on Juno for example, while we do have 128 masters, each with 65536 channels, the master allocation is not visible to system software - each core sees a single master with 65536 channels. Therefore differentiation between software sources in the STM trace is by channel number allocations only.

Ok, thanks a lot for the detailed explanation. I was confused by the
"static" bit in the patch description. It looks like something along the
lines of this patch might indeed be in order.

Regards,
--
Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-08 10:52         ` Alexander Shishkin
  0 siblings, 0 replies; 68+ messages in thread
From: Alexander Shishkin @ 2016-02-08 10:52 UTC (permalink / raw)
  To: linux-arm-kernel

Mike Leach <Mike.Leach@arm.com> writes:

> Hi,
>
> I think a quick clarification of the ARM hardware STM architecture may be of value here.
>
> The ARM hardware STM, when implemented as recommend in a hardware design, the master IDs are not under driver control, but have a hardwire association with source devices in the system. The master IDs are hardwired to individual cores and core security states, and there could be other IDs associated with hardware trace sources requiring output via the hardware STM. (an example of this is the ARM Juno development board).
>
> Therefore in multi-core systems many masters may be associated with a single software STM stream. A user-space application running on Core 1, may have a master ID of 0x11 in the STPv2 trace stream, but if this application is context switched and later continues running on Core 2, then master ID could change to 0x21.  Masters identify a hardware source in this scheme, the precise values used will be implementation dependent.
>
> So the number of masters "available to the software" depends on your interpretation of the phrase.
> If you mean "master IDs on which software trace will appear" then that will be many - it depends on which core is running the application. However as described above, this is not predictable by any decoder, and the master set used may not be contiguous.
> If you mean "master IDs selectable/allocated by the driver" then that will be 0 - the driver has no control, and possibly no knowledge of which master is associated with the current execution context. (this is of course system dependent - it may well be possible to have some configuration database associating cores with hardware IDs, but this will be implementation and board support dependant).
>
> Therefore, there is a requirement to tell both the user-space STM client and decoder that not only is master ID management not needed - it is in fact not possible and the key to identify the software source for a given STM packet is the channel alone. In addition we have a nominal single Master ID "available to the software", to satisfy the requirements of the generic ST module API.
>
> So on Juno for example, while we do have 128 masters, each with 65536 channels, the master allocation is not visible to system software - each core sees a single master with 65536 channels. Therefore differentiation between software sources in the STM trace is by channel number allocations only.

Ok, thanks a lot for the detailed explanation. I was confused by the
"static" bit in the patch description. It looks like something along the
lines of this patch might indeed be in order.

Regards,
--
Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
  2016-02-05 18:08       ` Mathieu Poirier
  (?)
@ 2016-02-08 13:26         ` Alexander Shishkin
  -1 siblings, 0 replies; 68+ messages in thread
From: Alexander Shishkin @ 2016-02-08 13:26 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: Chunyan Zhang, robh, Mark Brown, Pratik Patel, Nicolas GUION,
	Jon Corbet, Mark Rutland, Mike Leach, Jeremiassen, Tor, Al Grant,
	Lyra Zhang, linux-kernel, linux-arm-kernel, linux-api, linux-doc

Mathieu Poirier <mathieu.poirier@linaro.org> writes:

> On 5 February 2016 at 05:52, Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
>> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>>
>>> From: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>
>>> Some architecture like ARM assign masterIDs statically at the HW design
>>> phase, making masterID manipulation in the generic STM core irrelevant.
>>>
>>> This patch adds a new 'mstatic' flag to struct stm_data that tells the
>>> core that this specific STM device doesn't need explicit masterID
>>> management.
>>
>> So why do we need this patch? If your STM only has master 42 allocated
>> for software sources, simply set sw_start = 42, sw_end = 42 and you're
>> good to go, software will have exactly one channel to choose from. See
>> also the comment from <linux/stm.h>:
>
> On ARM each source, i.e entity capable of accessing STM channels, has
> a different master ID set in HW.  We can't assume the IDs are
> contiguous and from a SW point of view there is no way to probe the
> values.

Ok, it's the 'static' word that got me confused. From Mike's explanation
it seems to me that it's the antithesis of static; the master ID
assignment is so dynamic that it's not controllable by the software and
may or may not reflect core id, power state, phase of the moon, etc.

>>> In the core sw_start/end of masterID are set to '1',
>>> i.e there is only one masterID to deal with.
>>
>> This is also a completely arbitrary and unnecessary requirement. Again,
>> you can set both to 42 and it will still work.
>
> True - any value will do. The important thing to remember is that on
> ARM, there is only one masterID channel (from an STM core point of
> view).  But we could also proceed differently, see below for more
> details.

Well, we have the masters attribute with two numbers in it that define
the range of master IDs that the software can choose from. More
specifically to this situation:

 * the number of channel ID spaces available to the SW is
   $end - $start + 1, that is, in your case, just 1;
 * the number of master IDs for the SW to choose from is $end - $start;
 * if $end==$start, their actual numeric value doesn't really matter,
   either for the policy definition or for the actual writers.

This $end==$start situation itself may be ambiguous and can be
interpreted either as having just one *static* master ID fixed for all
SW writers (what I assumed from your commit message) or as having a
floating master ID, which changes of its own accord and is not
controllable by software.

These two situations are really the same thing from the perspective of
the system under tracing. Also, both of these situations should already
work if the driver sets both sw_start and sw_end to the same
value.

Regards,
--
Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-08 13:26         ` Alexander Shishkin
  0 siblings, 0 replies; 68+ messages in thread
From: Alexander Shishkin @ 2016-02-08 13:26 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: Chunyan Zhang, robh, Mark Brown, Pratik Patel, Nicolas GUION,
	Jon Corbet, Mark Rutland, Mike Leach, Jeremiassen, Tor, Al Grant,
	Lyra Zhang, linux-kernel, linux-arm-kernel, linux-api, linux-doc

Mathieu Poirier <mathieu.poirier@linaro.org> writes:

> On 5 February 2016 at 05:52, Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
>> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>>
>>> From: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>
>>> Some architecture like ARM assign masterIDs statically at the HW design
>>> phase, making masterID manipulation in the generic STM core irrelevant.
>>>
>>> This patch adds a new 'mstatic' flag to struct stm_data that tells the
>>> core that this specific STM device doesn't need explicit masterID
>>> management.
>>
>> So why do we need this patch? If your STM only has master 42 allocated
>> for software sources, simply set sw_start = 42, sw_end = 42 and you're
>> good to go, software will have exactly one channel to choose from. See
>> also the comment from <linux/stm.h>:
>
> On ARM each source, i.e entity capable of accessing STM channels, has
> a different master ID set in HW.  We can't assume the IDs are
> contiguous and from a SW point of view there is no way to probe the
> values.

Ok, it's the 'static' word that got me confused. From Mike's explanation
it seems to me that it's the antithesis of static; the master ID
assignment is so dynamic that it's not controllable by the software and
may or may not reflect core id, power state, phase of the moon, etc.

>>> In the core sw_start/end of masterID are set to '1',
>>> i.e there is only one masterID to deal with.
>>
>> This is also a completely arbitrary and unnecessary requirement. Again,
>> you can set both to 42 and it will still work.
>
> True - any value will do. The important thing to remember is that on
> ARM, there is only one masterID channel (from an STM core point of
> view).  But we could also proceed differently, see below for more
> details.

Well, we have the masters attribute with two numbers in it that define
the range of master IDs that the software can choose from. More
specifically to this situation:

 * the number of channel ID spaces available to the SW is
   $end - $start + 1, that is, in your case, just 1;
 * the number of master IDs for the SW to choose from is $end - $start;
 * if $end==$start, their actual numeric value doesn't really matter,
   either for the policy definition or for the actual writers.

This $end==$start situation itself may be ambiguous and can be
interpreted either as having just one *static* master ID fixed for all
SW writers (what I assumed from your commit message) or as having a
floating master ID, which changes of its own accord and is not
controllable by software.

These two situations are really the same thing from the perspective of
the system under tracing. Also, both of these situations should already
work if the driver sets both sw_start and sw_end to the same
value.

Regards,
--
Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-08 13:26         ` Alexander Shishkin
  0 siblings, 0 replies; 68+ messages in thread
From: Alexander Shishkin @ 2016-02-08 13:26 UTC (permalink / raw)
  To: linux-arm-kernel

Mathieu Poirier <mathieu.poirier@linaro.org> writes:

> On 5 February 2016 at 05:52, Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
>> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>>
>>> From: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>
>>> Some architecture like ARM assign masterIDs statically at the HW design
>>> phase, making masterID manipulation in the generic STM core irrelevant.
>>>
>>> This patch adds a new 'mstatic' flag to struct stm_data that tells the
>>> core that this specific STM device doesn't need explicit masterID
>>> management.
>>
>> So why do we need this patch? If your STM only has master 42 allocated
>> for software sources, simply set sw_start = 42, sw_end = 42 and you're
>> good to go, software will have exactly one channel to choose from. See
>> also the comment from <linux/stm.h>:
>
> On ARM each source, i.e entity capable of accessing STM channels, has
> a different master ID set in HW.  We can't assume the IDs are
> contiguous and from a SW point of view there is no way to probe the
> values.

Ok, it's the 'static' word that got me confused. From Mike's explanation
it seems to me that it's the antithesis of static; the master ID
assignment is so dynamic that it's not controllable by the software and
may or may not reflect core id, power state, phase of the moon, etc.

>>> In the core sw_start/end of masterID are set to '1',
>>> i.e there is only one masterID to deal with.
>>
>> This is also a completely arbitrary and unnecessary requirement. Again,
>> you can set both to 42 and it will still work.
>
> True - any value will do. The important thing to remember is that on
> ARM, there is only one masterID channel (from an STM core point of
> view).  But we could also proceed differently, see below for more
> details.

Well, we have the masters attribute with two numbers in it that define
the range of master IDs that the software can choose from. More
specifically to this situation:

 * the number of channel ID spaces available to the SW is
   $end - $start + 1, that is, in your case, just 1;
 * the number of master IDs for the SW to choose from is $end - $start;
 * if $end==$start, their actual numeric value doesn't really matter,
   either for the policy definition or for the actual writers.

This $end==$start situation itself may be ambiguous and can be
interpreted either as having just one *static* master ID fixed for all
SW writers (what I assumed from your commit message) or as having a
floating master ID, which changes of its own accord and is not
controllable by software.

These two situations are really the same thing from the perspective of
the system under tracing. Also, both of these situations should already
work if the driver sets both sw_start and sw_end to the same
value.

Regards,
--
Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
  2016-02-08 13:26         ` Alexander Shishkin
  (?)
@ 2016-02-08 17:05           ` Mathieu Poirier
  -1 siblings, 0 replies; 68+ messages in thread
From: Mathieu Poirier @ 2016-02-08 17:05 UTC (permalink / raw)
  To: Alexander Shishkin
  Cc: Chunyan Zhang, robh, Mark Brown, Pratik Patel, Nicolas GUION,
	Jon Corbet, Mark Rutland, Mike Leach, Jeremiassen, Tor, Al Grant,
	Lyra Zhang, linux-kernel, linux-arm-kernel, linux-api, linux-doc

On 8 February 2016 at 06:26, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Mathieu Poirier <mathieu.poirier@linaro.org> writes:
>
>> On 5 February 2016 at 05:52, Alexander Shishkin
>> <alexander.shishkin@linux.intel.com> wrote:
>>> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>>>
>>>> From: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>>
>>>> Some architecture like ARM assign masterIDs statically at the HW design
>>>> phase, making masterID manipulation in the generic STM core irrelevant.
>>>>
>>>> This patch adds a new 'mstatic' flag to struct stm_data that tells the
>>>> core that this specific STM device doesn't need explicit masterID
>>>> management.
>>>
>>> So why do we need this patch? If your STM only has master 42 allocated
>>> for software sources, simply set sw_start = 42, sw_end = 42 and you're
>>> good to go, software will have exactly one channel to choose from. See
>>> also the comment from <linux/stm.h>:
>>
>> On ARM each source, i.e entity capable of accessing STM channels, has
>> a different master ID set in HW.  We can't assume the IDs are
>> contiguous and from a SW point of view there is no way to probe the
>> values.
>
> Ok, it's the 'static' word that got me confused. From Mike's explanation
> it seems to me that it's the antithesis of static; the master ID
> assignment is so dynamic that it's not controllable by the software and
> may or may not reflect core id, power state, phase of the moon, etc.

Mike did write "master IDs are hardwired to individual cores and core
security states", which make assignment for one platform very static.
On the flip side those will change from one system to another.

>
>>>> In the core sw_start/end of masterID are set to '1',
>>>> i.e there is only one masterID to deal with.
>>>
>>> This is also a completely arbitrary and unnecessary requirement. Again,
>>> you can set both to 42 and it will still work.
>>
>> True - any value will do. The important thing to remember is that on
>> ARM, there is only one masterID channel (from an STM core point of
>> view).  But we could also proceed differently, see below for more
>> details.
>
> Well, we have the masters attribute with two numbers in it that define
> the range of master IDs that the software can choose from. More
> specifically to this situation:
>
>  * the number of channel ID spaces available to the SW is
>    $end - $start + 1, that is, in your case, just 1;
>  * the number of master IDs for the SW to choose from is $end - $start;
>  * if $end==$start, their actual numeric value doesn't really matter,
>    either for the policy definition or for the actual writers.
>
> This $end==$start situation itself may be ambiguous and can be
> interpreted either as having just one *static* master ID fixed for all
> SW writers (what I assumed from your commit message) or as having a
> floating master ID, which changes of its own accord and is not
> controllable by software.

Some clarification here.

On ARM from a SW point of view $end == $start and that doesn't change
(with regards to masterIDs) .  The ambiguity comes from the fact that
on other platforms where masterID configuration does change and is
important, the condition $end == $start could also be valid.

>From configFS, how do we tell users when masterIDs are set in HW?
That's what I wanted to highlight with the '-1'.  Regardless of the
solution we choose I think it is important that we inform, at the very
least, users when masterIDs don't matter.  We could also leave thing
as is, kill this patch and document things thoroughly.  With the
former things are obvious.

>
> These two situations are really the same thing from the perspective of
> the system under tracing. Also, both of these situations should already
> work if the driver sets both sw_start and sw_end to the same
> value.
>
> Regards,
> --
> Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-08 17:05           ` Mathieu Poirier
  0 siblings, 0 replies; 68+ messages in thread
From: Mathieu Poirier @ 2016-02-08 17:05 UTC (permalink / raw)
  To: Alexander Shishkin
  Cc: Chunyan Zhang, robh, Mark Brown, Pratik Patel, Nicolas GUION,
	Jon Corbet, Mark Rutland, Mike Leach, Jeremiassen, Tor, Al Grant,
	Lyra Zhang, linux-kernel, linux-arm-kernel, linux-api, linux-doc

On 8 February 2016 at 06:26, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Mathieu Poirier <mathieu.poirier@linaro.org> writes:
>
>> On 5 February 2016 at 05:52, Alexander Shishkin
>> <alexander.shishkin@linux.intel.com> wrote:
>>> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>>>
>>>> From: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>>
>>>> Some architecture like ARM assign masterIDs statically at the HW design
>>>> phase, making masterID manipulation in the generic STM core irrelevant.
>>>>
>>>> This patch adds a new 'mstatic' flag to struct stm_data that tells the
>>>> core that this specific STM device doesn't need explicit masterID
>>>> management.
>>>
>>> So why do we need this patch? If your STM only has master 42 allocated
>>> for software sources, simply set sw_start = 42, sw_end = 42 and you're
>>> good to go, software will have exactly one channel to choose from. See
>>> also the comment from <linux/stm.h>:
>>
>> On ARM each source, i.e entity capable of accessing STM channels, has
>> a different master ID set in HW.  We can't assume the IDs are
>> contiguous and from a SW point of view there is no way to probe the
>> values.
>
> Ok, it's the 'static' word that got me confused. From Mike's explanation
> it seems to me that it's the antithesis of static; the master ID
> assignment is so dynamic that it's not controllable by the software and
> may or may not reflect core id, power state, phase of the moon, etc.

Mike did write "master IDs are hardwired to individual cores and core
security states", which make assignment for one platform very static.
On the flip side those will change from one system to another.

>
>>>> In the core sw_start/end of masterID are set to '1',
>>>> i.e there is only one masterID to deal with.
>>>
>>> This is also a completely arbitrary and unnecessary requirement. Again,
>>> you can set both to 42 and it will still work.
>>
>> True - any value will do. The important thing to remember is that on
>> ARM, there is only one masterID channel (from an STM core point of
>> view).  But we could also proceed differently, see below for more
>> details.
>
> Well, we have the masters attribute with two numbers in it that define
> the range of master IDs that the software can choose from. More
> specifically to this situation:
>
>  * the number of channel ID spaces available to the SW is
>    $end - $start + 1, that is, in your case, just 1;
>  * the number of master IDs for the SW to choose from is $end - $start;
>  * if $end==$start, their actual numeric value doesn't really matter,
>    either for the policy definition or for the actual writers.
>
> This $end==$start situation itself may be ambiguous and can be
> interpreted either as having just one *static* master ID fixed for all
> SW writers (what I assumed from your commit message) or as having a
> floating master ID, which changes of its own accord and is not
> controllable by software.

Some clarification here.

On ARM from a SW point of view $end == $start and that doesn't change
(with regards to masterIDs) .  The ambiguity comes from the fact that
on other platforms where masterID configuration does change and is
important, the condition $end == $start could also be valid.

>From configFS, how do we tell users when masterIDs are set in HW?
That's what I wanted to highlight with the '-1'.  Regardless of the
solution we choose I think it is important that we inform, at the very
least, users when masterIDs don't matter.  We could also leave thing
as is, kill this patch and document things thoroughly.  With the
former things are obvious.

>
> These two situations are really the same thing from the perspective of
> the system under tracing. Also, both of these situations should already
> work if the driver sets both sw_start and sw_end to the same
> value.
>
> Regards,
> --
> Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-08 17:05           ` Mathieu Poirier
  0 siblings, 0 replies; 68+ messages in thread
From: Mathieu Poirier @ 2016-02-08 17:05 UTC (permalink / raw)
  To: linux-arm-kernel

On 8 February 2016 at 06:26, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Mathieu Poirier <mathieu.poirier@linaro.org> writes:
>
>> On 5 February 2016 at 05:52, Alexander Shishkin
>> <alexander.shishkin@linux.intel.com> wrote:
>>> Chunyan Zhang <zhang.chunyan@linaro.org> writes:
>>>
>>>> From: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>>
>>>> Some architecture like ARM assign masterIDs statically at the HW design
>>>> phase, making masterID manipulation in the generic STM core irrelevant.
>>>>
>>>> This patch adds a new 'mstatic' flag to struct stm_data that tells the
>>>> core that this specific STM device doesn't need explicit masterID
>>>> management.
>>>
>>> So why do we need this patch? If your STM only has master 42 allocated
>>> for software sources, simply set sw_start = 42, sw_end = 42 and you're
>>> good to go, software will have exactly one channel to choose from. See
>>> also the comment from <linux/stm.h>:
>>
>> On ARM each source, i.e entity capable of accessing STM channels, has
>> a different master ID set in HW.  We can't assume the IDs are
>> contiguous and from a SW point of view there is no way to probe the
>> values.
>
> Ok, it's the 'static' word that got me confused. From Mike's explanation
> it seems to me that it's the antithesis of static; the master ID
> assignment is so dynamic that it's not controllable by the software and
> may or may not reflect core id, power state, phase of the moon, etc.

Mike did write "master IDs are hardwired to individual cores and core
security states", which make assignment for one platform very static.
On the flip side those will change from one system to another.

>
>>>> In the core sw_start/end of masterID are set to '1',
>>>> i.e there is only one masterID to deal with.
>>>
>>> This is also a completely arbitrary and unnecessary requirement. Again,
>>> you can set both to 42 and it will still work.
>>
>> True - any value will do. The important thing to remember is that on
>> ARM, there is only one masterID channel (from an STM core point of
>> view).  But we could also proceed differently, see below for more
>> details.
>
> Well, we have the masters attribute with two numbers in it that define
> the range of master IDs that the software can choose from. More
> specifically to this situation:
>
>  * the number of channel ID spaces available to the SW is
>    $end - $start + 1, that is, in your case, just 1;
>  * the number of master IDs for the SW to choose from is $end - $start;
>  * if $end==$start, their actual numeric value doesn't really matter,
>    either for the policy definition or for the actual writers.
>
> This $end==$start situation itself may be ambiguous and can be
> interpreted either as having just one *static* master ID fixed for all
> SW writers (what I assumed from your commit message) or as having a
> floating master ID, which changes of its own accord and is not
> controllable by software.

Some clarification here.

On ARM from a SW point of view $end == $start and that doesn't change
(with regards to masterIDs) .  The ambiguity comes from the fact that
on other platforms where masterID configuration does change and is
important, the condition $end == $start could also be valid.

>From configFS, how do we tell users when masterIDs are set in HW?
That's what I wanted to highlight with the '-1'.  Regardless of the
solution we choose I think it is important that we inform, at the very
least, users when masterIDs don't matter.  We could also leave thing
as is, kill this patch and document things thoroughly.  With the
former things are obvious.

>
> These two situations are really the same thing from the perspective of
> the system under tracing. Also, both of these situations should already
> work if the driver sets both sw_start and sw_end to the same
> value.
>
> Regards,
> --
> Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* RE: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-08 17:44             ` Al Grant
  0 siblings, 0 replies; 68+ messages in thread
From: Al Grant @ 2016-02-08 17:44 UTC (permalink / raw)
  To: Mathieu Poirier, Alexander Shishkin
  Cc: Chunyan Zhang, robh, Mark Brown, Pratik Patel, Nicolas GUION,
	Jon Corbet, Mark Rutland, Mike Leach, Jeremiassen, Tor,
	Lyra Zhang, linux-kernel, linux-arm-kernel, linux-api, linux-doc

> Mike did write "master IDs are hardwired to individual cores and core security
> states", which make assignment for one platform very static.
> On the flip side those will change from one system to another.

It depends on your perspective.  From the perspective of a userspace
process not pinned to a core, the master id will appear to vary
dynamically and unpredictably as the thread migrates from one
core to another.  (That's actually useful if the decoder wants to know
where the thread is running at any given point, as it can find that out
for free, without the need to track migration events.)

On the other hand if you are pinned (e.g. you're the kernel on a
particular core, or you're a per-core worker thread in some thread
pooling system) then you have a fixed master id, and then you can
have one instance per core all using the same range of channel
numbers, with the master id indicating the core - this saves on
channel space compared to having to give each core its own
range of channel space.

Al
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

^ permalink raw reply	[flat|nested] 68+ messages in thread
* RE: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-08 17:44             ` Al Grant
  0 siblings, 0 replies; 68+ messages in thread
From: Al Grant @ 2016-02-08 17:44 UTC (permalink / raw)
  To: Mathieu Poirier, Alexander Shishkin
  Cc: Chunyan Zhang, robh-DgEjT+Ai2ygdnm+yROfE0A, Mark Brown,
	Pratik Patel, Nicolas GUION, Jon Corbet, Mark Rutland,
	Mike Leach, Jeremiassen, Tor, Lyra Zhang,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	linux-doc-u79uwXL29TY76Z2rM5mHXA

> Mike did write "master IDs are hardwired to individual cores and core security
> states", which make assignment for one platform very static.
> On the flip side those will change from one system to another.

It depends on your perspective.  From the perspective of a userspace
process not pinned to a core, the master id will appear to vary
dynamically and unpredictably as the thread migrates from one
core to another.  (That's actually useful if the decoder wants to know
where the thread is running at any given point, as it can find that out
for free, without the need to track migration events.)

On the other hand if you are pinned (e.g. you're the kernel on a
particular core, or you're a per-core worker thread in some thread
pooling system) then you have a fixed master id, and then you can
have one instance per core all using the same range of channel
numbers, with the master id indicating the core - this saves on
channel space compared to having to give each core its own
range of channel space.

Al
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-08 17:44             ` Al Grant
  0 siblings, 0 replies; 68+ messages in thread
From: Al Grant @ 2016-02-08 17:44 UTC (permalink / raw)
  To: linux-arm-kernel

> Mike did write "master IDs are hardwired to individual cores and core security
> states", which make assignment for one platform very static.
> On the flip side those will change from one system to another.

It depends on your perspective.  From the perspective of a userspace
process not pinned to a core, the master id will appear to vary
dynamically and unpredictably as the thread migrates from one
core to another.  (That's actually useful if the decoder wants to know
where the thread is running at any given point, as it can find that out
for free, without the need to track migration events.)

On the other hand if you are pinned (e.g. you're the kernel on a
particular core, or you're a per-core worker thread in some thread
pooling system) then you have a fixed master id, and then you can
have one instance per core all using the same range of channel
numbers, with the master id indicating the core - this saves on
channel space compared to having to give each core its own
range of channel space.

Al
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

^ permalink raw reply	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
  2016-02-08 17:44             ` Al Grant
  (?)
@ 2016-02-09 17:06               ` Mathieu Poirier
  -1 siblings, 0 replies; 68+ messages in thread
From: Mathieu Poirier @ 2016-02-09 17:06 UTC (permalink / raw)
  To: Al Grant
  Cc: Alexander Shishkin, Chunyan Zhang, robh, Mark Brown,
	Pratik Patel, Nicolas GUION, Jon Corbet, Mark Rutland,
	Mike Leach, Jeremiassen, Tor, Lyra Zhang, linux-kernel,
	linux-arm-kernel, linux-api, linux-doc

On 8 February 2016 at 10:44, Al Grant <Al.Grant@arm.com> wrote:
>> Mike did write "master IDs are hardwired to individual cores and core security
>> states", which make assignment for one platform very static.
>> On the flip side those will change from one system to another.
>
> It depends on your perspective.  From the perspective of a userspace
> process not pinned to a core, the master id will appear to vary
> dynamically and unpredictably as the thread migrates from one
> core to another.  (That's actually useful if the decoder wants to know
> where the thread is running at any given point, as it can find that out
> for free, without the need to track migration events.)

Right, that's the expected (and desired) behaviour.

>
> On the other hand if you are pinned (e.g. you're the kernel on a
> particular core, or you're a per-core worker thread in some thread
> pooling system) then you have a fixed master id, and then you can
> have one instance per core all using the same range of channel
> numbers, with the master id indicating the core - this saves on
> channel space compared to having to give each core its own
> range of channel space.

>From an STM core and driver point of view channel mapping works the
same way - pinning of tasks is  a kernel artefact.  The problem of
representing masterIDs in the STM core for an architecture with HW
assigned masterIDs is still unresolved.

>
> Al
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

^ permalink raw reply	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-09 17:06               ` Mathieu Poirier
  0 siblings, 0 replies; 68+ messages in thread
From: Mathieu Poirier @ 2016-02-09 17:06 UTC (permalink / raw)
  To: Al Grant
  Cc: Alexander Shishkin, Chunyan Zhang, robh, Mark Brown,
	Pratik Patel, Nicolas GUION, Jon Corbet, Mark Rutland,
	Mike Leach, Jeremiassen, Tor, Lyra Zhang, linux-kernel,
	linux-arm-kernel, linux-api, linux-doc

On 8 February 2016 at 10:44, Al Grant <Al.Grant@arm.com> wrote:
>> Mike did write "master IDs are hardwired to individual cores and core security
>> states", which make assignment for one platform very static.
>> On the flip side those will change from one system to another.
>
> It depends on your perspective.  From the perspective of a userspace
> process not pinned to a core, the master id will appear to vary
> dynamically and unpredictably as the thread migrates from one
> core to another.  (That's actually useful if the decoder wants to know
> where the thread is running at any given point, as it can find that out
> for free, without the need to track migration events.)

Right, that's the expected (and desired) behaviour.

>
> On the other hand if you are pinned (e.g. you're the kernel on a
> particular core, or you're a per-core worker thread in some thread
> pooling system) then you have a fixed master id, and then you can
> have one instance per core all using the same range of channel
> numbers, with the master id indicating the core - this saves on
> channel space compared to having to give each core its own
> range of channel space.

>From an STM core and driver point of view channel mapping works the
same way - pinning of tasks is  a kernel artefact.  The problem of
representing masterIDs in the STM core for an architecture with HW
assigned masterIDs is still unresolved.

>
> Al
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-09 17:06               ` Mathieu Poirier
  0 siblings, 0 replies; 68+ messages in thread
From: Mathieu Poirier @ 2016-02-09 17:06 UTC (permalink / raw)
  To: linux-arm-kernel

On 8 February 2016 at 10:44, Al Grant <Al.Grant@arm.com> wrote:
>> Mike did write "master IDs are hardwired to individual cores and core security
>> states", which make assignment for one platform very static.
>> On the flip side those will change from one system to another.
>
> It depends on your perspective.  From the perspective of a userspace
> process not pinned to a core, the master id will appear to vary
> dynamically and unpredictably as the thread migrates from one
> core to another.  (That's actually useful if the decoder wants to know
> where the thread is running at any given point, as it can find that out
> for free, without the need to track migration events.)

Right, that's the expected (and desired) behaviour.

>
> On the other hand if you are pinned (e.g. you're the kernel on a
> particular core, or you're a per-core worker thread in some thread
> pooling system) then you have a fixed master id, and then you can
> have one instance per core all using the same range of channel
> numbers, with the master id indicating the core - this saves on
> channel space compared to having to give each core its own
> range of channel space.

>From an STM core and driver point of view channel mapping works the
same way - pinning of tasks is  a kernel artefact.  The problem of
representing masterIDs in the STM core for an architecture with HW
assigned masterIDs is still unresolved.

>
> Al
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

^ permalink raw reply	[flat|nested] 68+ messages in thread
* RE: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
  2016-02-08 17:44             ` Al Grant
  (?)
@ 2016-02-12 15:54               ` Alexander Shishkin
  -1 siblings, 0 replies; 68+ messages in thread
From: Alexander Shishkin @ 2016-02-12 15:54 UTC (permalink / raw)
  To: Al Grant, Mathieu Poirier
  Cc: Chunyan Zhang, robh, Mark Brown, Pratik Patel, Nicolas GUION,
	Jon Corbet, Mark Rutland, Mike Leach, Jeremiassen, Tor,
	Lyra Zhang, linux-kernel, linux-arm-kernel, linux-api, linux-doc

Al Grant <Al.Grant@arm.com> writes:

>> Mike did write "master IDs are hardwired to individual cores and core security
>> states", which make assignment for one platform very static.
>> On the flip side those will change from one system to another.
>
> It depends on your perspective.  From the perspective of a userspace
> process not pinned to a core, the master id will appear to vary
> dynamically and unpredictably as the thread migrates from one
> core to another.  (That's actually useful if the decoder wants to know
> where the thread is running at any given point, as it can find that out
> for free, without the need to track migration events.)
>
> On the other hand if you are pinned (e.g. you're the kernel on a
> particular core, or you're a per-core worker thread in some thread
> pooling system) then you have a fixed master id, and then you can
> have one instance per core all using the same range of channel
> numbers, with the master id indicating the core - this saves on
> channel space compared to having to give each core its own
> range of channel space.

What I understood from Mike's explanation is that basically nothing is
fixed from the software perspective (talking about Coresight STM). One
doesn't know the master id with which one's data will be tagged,
regardless of whether one is cpu-affine or not, because:

 * even per-core master IDs will be highly implementation dependent and
   therefore not knowable by the kernel and subsequently userspace,
 * master IDs may change based on other conditions (besides security
   states), so even if we knew per-core master IDs for sure, it wouldn't
   give us the complete picture.

So even though from the HW perspective master IDs may be "hardwired" to
hardware states, the hardware states themselves are dynamic, so from the
SW perspective these master IDs are not static or hardwired at all.

If the same mmio write done by software would always result in the exact
same master ID tagged to it, that would be more like "static" to me.

Regards,
--
Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* RE: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-12 15:54               ` Alexander Shishkin
  0 siblings, 0 replies; 68+ messages in thread
From: Alexander Shishkin @ 2016-02-12 15:54 UTC (permalink / raw)
  To: Al Grant, Mathieu Poirier
  Cc: Chunyan Zhang, robh, Mark Brown, Pratik Patel, Nicolas GUION,
	Jon Corbet, Mark Rutland, Mike Leach, Jeremiassen, Tor,
	Lyra Zhang, linux-kernel, linux-arm-kernel, linux-api, linux-doc

Al Grant <Al.Grant@arm.com> writes:

>> Mike did write "master IDs are hardwired to individual cores and core security
>> states", which make assignment for one platform very static.
>> On the flip side those will change from one system to another.
>
> It depends on your perspective.  From the perspective of a userspace
> process not pinned to a core, the master id will appear to vary
> dynamically and unpredictably as the thread migrates from one
> core to another.  (That's actually useful if the decoder wants to know
> where the thread is running at any given point, as it can find that out
> for free, without the need to track migration events.)
>
> On the other hand if you are pinned (e.g. you're the kernel on a
> particular core, or you're a per-core worker thread in some thread
> pooling system) then you have a fixed master id, and then you can
> have one instance per core all using the same range of channel
> numbers, with the master id indicating the core - this saves on
> channel space compared to having to give each core its own
> range of channel space.

What I understood from Mike's explanation is that basically nothing is
fixed from the software perspective (talking about Coresight STM). One
doesn't know the master id with which one's data will be tagged,
regardless of whether one is cpu-affine or not, because:

 * even per-core master IDs will be highly implementation dependent and
   therefore not knowable by the kernel and subsequently userspace,
 * master IDs may change based on other conditions (besides security
   states), so even if we knew per-core master IDs for sure, it wouldn't
   give us the complete picture.

So even though from the HW perspective master IDs may be "hardwired" to
hardware states, the hardware states themselves are dynamic, so from the
SW perspective these master IDs are not static or hardwired at all.

If the same mmio write done by software would always result in the exact
same master ID tagged to it, that would be more like "static" to me.

Regards,

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-12 15:54               ` Alexander Shishkin
  0 siblings, 0 replies; 68+ messages in thread
From: Alexander Shishkin @ 2016-02-12 15:54 UTC (permalink / raw)
  To: linux-arm-kernel

Al Grant <Al.Grant@arm.com> writes:

>> Mike did write "master IDs are hardwired to individual cores and core security
>> states", which make assignment for one platform very static.
>> On the flip side those will change from one system to another.
>
> It depends on your perspective.  From the perspective of a userspace
> process not pinned to a core, the master id will appear to vary
> dynamically and unpredictably as the thread migrates from one
> core to another.  (That's actually useful if the decoder wants to know
> where the thread is running at any given point, as it can find that out
> for free, without the need to track migration events.)
>
> On the other hand if you are pinned (e.g. you're the kernel on a
> particular core, or you're a per-core worker thread in some thread
> pooling system) then you have a fixed master id, and then you can
> have one instance per core all using the same range of channel
> numbers, with the master id indicating the core - this saves on
> channel space compared to having to give each core its own
> range of channel space.

What I understood from Mike's explanation is that basically nothing is
fixed from the software perspective (talking about Coresight STM). One
doesn't know the master id with which one's data will be tagged,
regardless of whether one is cpu-affine or not, because:

 * even per-core master IDs will be highly implementation dependent and
   therefore not knowable by the kernel and subsequently userspace,
 * master IDs may change based on other conditions (besides security
   states), so even if we knew per-core master IDs for sure, it wouldn't
   give us the complete picture.

So even though from the HW perspective master IDs may be "hardwired" to
hardware states, the hardware states themselves are dynamic, so from the
SW perspective these master IDs are not static or hardwired at all.

If the same mmio write done by software would always result in the exact
same master ID tagged to it, that would be more like "static" to me.

Regards,
--
Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
  2016-02-08 17:05           ` Mathieu Poirier
  (?)
@ 2016-02-12 16:27             ` Alexander Shishkin
  -1 siblings, 0 replies; 68+ messages in thread
From: Alexander Shishkin @ 2016-02-12 16:27 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: Chunyan Zhang, robh, Mark Brown, Pratik Patel, Nicolas GUION,
	Jon Corbet, Mark Rutland, Mike Leach, Jeremiassen, Tor, Al Grant,
	Lyra Zhang, linux-kernel, linux-arm-kernel, linux-api, linux-doc

Mathieu Poirier <mathieu.poirier@linaro.org> writes:

> On 8 February 2016 at 06:26, Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
>> This $end==$start situation itself may be ambiguous and can be
>> interpreted either as having just one *static* master ID fixed for all
>> SW writers (what I assumed from your commit message) or as having a
>> floating master ID, which changes of its own accord and is not
>> controllable by software.
>
> Some clarification here.
>
> On ARM from a SW point of view $end == $start and that doesn't change
> (with regards to masterIDs) .  The ambiguity comes from the fact that
> on other platforms where masterID configuration does change and is
> important, the condition $end == $start could also be valid.

Yes, that's what I was saying. The thing is, on the system-under-tracing
side these two situations are not very different from one
another. Master IDs are really just numbers without any semantics
attached to them in the sense that they are not covered by the mipi spec
or any other standard (to my knowledge).

The difference is in the way we map channels to masters. One way is to
allocate a distinct set of channels for each master (the way Intel Trace
Hub does it); another way is to share the same set of channels between
multiple masters. So we can describe this as "hardware implements the
same set of channels across multiple masters" or something along those
lines.

Actually, in the latter scheme of things you can also have multiple
masters, at least theoretically. Say, you have masters [0..15], each
with distinct set of channels, but depending on hardware state these
masters actually end up as $state*16+$masterID in the STP stream.

So we might also think about differentiating between the hardware
masters (0 though 15 in the above example) and STP masters.

Regards,
--
Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-12 16:27             ` Alexander Shishkin
  0 siblings, 0 replies; 68+ messages in thread
From: Alexander Shishkin @ 2016-02-12 16:27 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: Chunyan Zhang, robh, Mark Brown, Pratik Patel, Nicolas GUION,
	Jon Corbet, Mark Rutland, Mike Leach, Jeremiassen, Tor, Al Grant,
	Lyra Zhang, linux-kernel, linux-arm-kernel, linux-api, linux-doc

Mathieu Poirier <mathieu.poirier@linaro.org> writes:

> On 8 February 2016 at 06:26, Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
>> This $end==$start situation itself may be ambiguous and can be
>> interpreted either as having just one *static* master ID fixed for all
>> SW writers (what I assumed from your commit message) or as having a
>> floating master ID, which changes of its own accord and is not
>> controllable by software.
>
> Some clarification here.
>
> On ARM from a SW point of view $end == $start and that doesn't change
> (with regards to masterIDs) .  The ambiguity comes from the fact that
> on other platforms where masterID configuration does change and is
> important, the condition $end == $start could also be valid.

Yes, that's what I was saying. The thing is, on the system-under-tracing
side these two situations are not very different from one
another. Master IDs are really just numbers without any semantics
attached to them in the sense that they are not covered by the mipi spec
or any other standard (to my knowledge).

The difference is in the way we map channels to masters. One way is to
allocate a distinct set of channels for each master (the way Intel Trace
Hub does it); another way is to share the same set of channels between
multiple masters. So we can describe this as "hardware implements the
same set of channels across multiple masters" or something along those
lines.

Actually, in the latter scheme of things you can also have multiple
masters, at least theoretically. Say, you have masters [0..15], each
with distinct set of channels, but depending on hardware state these
masters actually end up as $state*16+$masterID in the STP stream.

So we might also think about differentiating between the hardware
masters (0 though 15 in the above example) and STP masters.

Regards,
--
Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-12 16:27             ` Alexander Shishkin
  0 siblings, 0 replies; 68+ messages in thread
From: Alexander Shishkin @ 2016-02-12 16:27 UTC (permalink / raw)
  To: linux-arm-kernel

Mathieu Poirier <mathieu.poirier@linaro.org> writes:

> On 8 February 2016 at 06:26, Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
>> This $end==$start situation itself may be ambiguous and can be
>> interpreted either as having just one *static* master ID fixed for all
>> SW writers (what I assumed from your commit message) or as having a
>> floating master ID, which changes of its own accord and is not
>> controllable by software.
>
> Some clarification here.
>
> On ARM from a SW point of view $end == $start and that doesn't change
> (with regards to masterIDs) .  The ambiguity comes from the fact that
> on other platforms where masterID configuration does change and is
> important, the condition $end == $start could also be valid.

Yes, that's what I was saying. The thing is, on the system-under-tracing
side these two situations are not very different from one
another. Master IDs are really just numbers without any semantics
attached to them in the sense that they are not covered by the mipi spec
or any other standard (to my knowledge).

The difference is in the way we map channels to masters. One way is to
allocate a distinct set of channels for each master (the way Intel Trace
Hub does it); another way is to share the same set of channels between
multiple masters. So we can describe this as "hardware implements the
same set of channels across multiple masters" or something along those
lines.

Actually, in the latter scheme of things you can also have multiple
masters, at least theoretically. Say, you have masters [0..15], each
with distinct set of channels, but depending on hardware state these
masters actually end up as $state*16+$masterID in the STP stream.

So we might also think about differentiating between the hardware
masters (0 though 15 in the above example) and STP masters.

Regards,
--
Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
  2016-02-12 16:27             ` Alexander Shishkin
  (?)
@ 2016-02-12 20:33               ` Mathieu Poirier
  -1 siblings, 0 replies; 68+ messages in thread
From: Mathieu Poirier @ 2016-02-12 20:33 UTC (permalink / raw)
  To: Alexander Shishkin
  Cc: Chunyan Zhang, Rob Herring, Mark Brown, Pratik Patel,
	Nicolas GUION, Jon Corbet, Mark Rutland, Mike Leach, Jeremiassen,
	Tor, Al Grant, Lyra Zhang, linux-kernel, linux-arm-kernel,
	linux-api, linux-doc

On 12 February 2016 at 09:27, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Mathieu Poirier <mathieu.poirier@linaro.org> writes:
>
>> On 8 February 2016 at 06:26, Alexander Shishkin
>> <alexander.shishkin@linux.intel.com> wrote:
>>> This $end==$start situation itself may be ambiguous and can be
>>> interpreted either as having just one *static* master ID fixed for all
>>> SW writers (what I assumed from your commit message) or as having a
>>> floating master ID, which changes of its own accord and is not
>>> controllable by software.
>>
>> Some clarification here.
>>
>> On ARM from a SW point of view $end == $start and that doesn't change
>> (with regards to masterIDs) .  The ambiguity comes from the fact that
>> on other platforms where masterID configuration does change and is
>> important, the condition $end == $start could also be valid.
>
> Yes, that's what I was saying. The thing is, on the system-under-tracing
> side these two situations are not very different from one
> another. Master IDs are really just numbers without any semantics
> attached to them in the sense that they are not covered by the mipi spec
> or any other standard (to my knowledge).

We are definitely on the same page here, just using slightly different terms.

>
> The difference is in the way we map channels to masters. One way is to
> allocate a distinct set of channels for each master (the way Intel Trace
> Hub does it); another way is to share the same set of channels between
> multiple masters.

We are in total agreement.

> So we can describe this as "hardware implements the
> same set of channels across multiple masters" or something along those
> lines.

I suggest "Shared channels"?  In the end, that's really what it is...

The outstanding issue is still how to represent these to different way
of mapping things in the STM core.  I suggested a flag, called
"mstatic" (but that can be changed), and a representation of '-1' in
for masterIDs sysFS.  Whether we stick with that or not is irrelevant,
I'd be fine with another mechanism.  What I am keen on is that from
sysFS users can quickly tell which heuristic is enacted on that
specific architecture.

>
> Actually, in the latter scheme of things you can also have multiple
> masters, at least theoretically. Say, you have masters [0..15], each
> with distinct set of channels, but depending on hardware state these
> masters actually end up as $state*16+$masterID in the STP stream.
>
> So we might also think about differentiating between the hardware
> masters (0 though 15 in the above example) and STP masters.

I'm not sure I get what you mean here.  On ARM the masterIDs assigned
in HW, which will depend on the state, will show up in the STP stream.
But again, I might be missing your point.

Thanks,
Mathieu

>
> Regards,
> --
> Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-12 20:33               ` Mathieu Poirier
  0 siblings, 0 replies; 68+ messages in thread
From: Mathieu Poirier @ 2016-02-12 20:33 UTC (permalink / raw)
  To: Alexander Shishkin
  Cc: Chunyan Zhang, Rob Herring, Mark Brown, Pratik Patel,
	Nicolas GUION, Jon Corbet, Mark Rutland, Mike Leach, Jeremiassen,
	Tor, Al Grant, Lyra Zhang, linux-kernel, linux-arm-kernel,
	linux-api, linux-doc

On 12 February 2016 at 09:27, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Mathieu Poirier <mathieu.poirier@linaro.org> writes:
>
>> On 8 February 2016 at 06:26, Alexander Shishkin
>> <alexander.shishkin@linux.intel.com> wrote:
>>> This $end==$start situation itself may be ambiguous and can be
>>> interpreted either as having just one *static* master ID fixed for all
>>> SW writers (what I assumed from your commit message) or as having a
>>> floating master ID, which changes of its own accord and is not
>>> controllable by software.
>>
>> Some clarification here.
>>
>> On ARM from a SW point of view $end == $start and that doesn't change
>> (with regards to masterIDs) .  The ambiguity comes from the fact that
>> on other platforms where masterID configuration does change and is
>> important, the condition $end == $start could also be valid.
>
> Yes, that's what I was saying. The thing is, on the system-under-tracing
> side these two situations are not very different from one
> another. Master IDs are really just numbers without any semantics
> attached to them in the sense that they are not covered by the mipi spec
> or any other standard (to my knowledge).

We are definitely on the same page here, just using slightly different terms.

>
> The difference is in the way we map channels to masters. One way is to
> allocate a distinct set of channels for each master (the way Intel Trace
> Hub does it); another way is to share the same set of channels between
> multiple masters.

We are in total agreement.

> So we can describe this as "hardware implements the
> same set of channels across multiple masters" or something along those
> lines.

I suggest "Shared channels"?  In the end, that's really what it is...

The outstanding issue is still how to represent these to different way
of mapping things in the STM core.  I suggested a flag, called
"mstatic" (but that can be changed), and a representation of '-1' in
for masterIDs sysFS.  Whether we stick with that or not is irrelevant,
I'd be fine with another mechanism.  What I am keen on is that from
sysFS users can quickly tell which heuristic is enacted on that
specific architecture.

>
> Actually, in the latter scheme of things you can also have multiple
> masters, at least theoretically. Say, you have masters [0..15], each
> with distinct set of channels, but depending on hardware state these
> masters actually end up as $state*16+$masterID in the STP stream.
>
> So we might also think about differentiating between the hardware
> masters (0 though 15 in the above example) and STP masters.

I'm not sure I get what you mean here.  On ARM the masterIDs assigned
in HW, which will depend on the state, will show up in the STP stream.
But again, I might be missing your point.

Thanks,
Mathieu

>
> Regards,
> --
> Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-12 20:33               ` Mathieu Poirier
  0 siblings, 0 replies; 68+ messages in thread
From: Mathieu Poirier @ 2016-02-12 20:33 UTC (permalink / raw)
  To: linux-arm-kernel

On 12 February 2016 at 09:27, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Mathieu Poirier <mathieu.poirier@linaro.org> writes:
>
>> On 8 February 2016 at 06:26, Alexander Shishkin
>> <alexander.shishkin@linux.intel.com> wrote:
>>> This $end==$start situation itself may be ambiguous and can be
>>> interpreted either as having just one *static* master ID fixed for all
>>> SW writers (what I assumed from your commit message) or as having a
>>> floating master ID, which changes of its own accord and is not
>>> controllable by software.
>>
>> Some clarification here.
>>
>> On ARM from a SW point of view $end == $start and that doesn't change
>> (with regards to masterIDs) .  The ambiguity comes from the fact that
>> on other platforms where masterID configuration does change and is
>> important, the condition $end == $start could also be valid.
>
> Yes, that's what I was saying. The thing is, on the system-under-tracing
> side these two situations are not very different from one
> another. Master IDs are really just numbers without any semantics
> attached to them in the sense that they are not covered by the mipi spec
> or any other standard (to my knowledge).

We are definitely on the same page here, just using slightly different terms.

>
> The difference is in the way we map channels to masters. One way is to
> allocate a distinct set of channels for each master (the way Intel Trace
> Hub does it); another way is to share the same set of channels between
> multiple masters.

We are in total agreement.

> So we can describe this as "hardware implements the
> same set of channels across multiple masters" or something along those
> lines.

I suggest "Shared channels"?  In the end, that's really what it is...

The outstanding issue is still how to represent these to different way
of mapping things in the STM core.  I suggested a flag, called
"mstatic" (but that can be changed), and a representation of '-1' in
for masterIDs sysFS.  Whether we stick with that or not is irrelevant,
I'd be fine with another mechanism.  What I am keen on is that from
sysFS users can quickly tell which heuristic is enacted on that
specific architecture.

>
> Actually, in the latter scheme of things you can also have multiple
> masters, at least theoretically. Say, you have masters [0..15], each
> with distinct set of channels, but depending on hardware state these
> masters actually end up as $state*16+$masterID in the STP stream.
>
> So we might also think about differentiating between the hardware
> masters (0 though 15 in the above example) and STP masters.

I'm not sure I get what you mean here.  On ARM the masterIDs assigned
in HW, which will depend on the state, will show up in the STP stream.
But again, I might be missing your point.

Thanks,
Mathieu

>
> Regards,
> --
> Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
  2016-02-12 20:33               ` Mathieu Poirier
  (?)
@ 2016-02-22 18:01                 ` Mathieu Poirier
  -1 siblings, 0 replies; 68+ messages in thread
From: Mathieu Poirier @ 2016-02-22 18:01 UTC (permalink / raw)
  To: Alexander Shishkin
  Cc: Chunyan Zhang, Rob Herring, Mark Brown, Pratik Patel,
	Nicolas GUION, Jon Corbet, Mark Rutland, Mike Leach, Jeremiassen,
	Tor, Al Grant, Lyra Zhang, linux-kernel, linux-arm-kernel,
	linux-api, linux-doc

On 12 February 2016 at 13:33, Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
> On 12 February 2016 at 09:27, Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
>> Mathieu Poirier <mathieu.poirier@linaro.org> writes:
>>
>>> On 8 February 2016 at 06:26, Alexander Shishkin
>>> <alexander.shishkin@linux.intel.com> wrote:
>>>> This $end==$start situation itself may be ambiguous and can be
>>>> interpreted either as having just one *static* master ID fixed for all
>>>> SW writers (what I assumed from your commit message) or as having a
>>>> floating master ID, which changes of its own accord and is not
>>>> controllable by software.
>>>
>>> Some clarification here.
>>>
>>> On ARM from a SW point of view $end == $start and that doesn't change
>>> (with regards to masterIDs) .  The ambiguity comes from the fact that
>>> on other platforms where masterID configuration does change and is
>>> important, the condition $end == $start could also be valid.
>>
>> Yes, that's what I was saying. The thing is, on the system-under-tracing
>> side these two situations are not very different from one
>> another. Master IDs are really just numbers without any semantics
>> attached to them in the sense that they are not covered by the mipi spec
>> or any other standard (to my knowledge).
>
> We are definitely on the same page here, just using slightly different terms.
>
>>
>> The difference is in the way we map channels to masters. One way is to
>> allocate a distinct set of channels for each master (the way Intel Trace
>> Hub does it); another way is to share the same set of channels between
>> multiple masters.
>
> We are in total agreement.
>
>> So we can describe this as "hardware implements the
>> same set of channels across multiple masters" or something along those
>> lines.
>
> I suggest "Shared channels"?  In the end, that's really what it is...
>
> The outstanding issue is still how to represent these to different way
> of mapping things in the STM core.  I suggested a flag, called
> "mstatic" (but that can be changed), and a representation of '-1' in
> for masterIDs sysFS.  Whether we stick with that or not is irrelevant,
> I'd be fine with another mechanism.  What I am keen on is that from
> sysFS users can quickly tell which heuristic is enacted on that
> specific architecture.

Alex,

How do you want to proceed with the above?  Do you agree with my
current proposal or can you think of a better way?

Thanks,
Mathieu


>
>>
>> Actually, in the latter scheme of things you can also have multiple
>> masters, at least theoretically. Say, you have masters [0..15], each
>> with distinct set of channels, but depending on hardware state these
>> masters actually end up as $state*16+$masterID in the STP stream.
>>
>> So we might also think about differentiating between the hardware
>> masters (0 though 15 in the above example) and STP masters.
>
> I'm not sure I get what you mean here.  On ARM the masterIDs assigned
> in HW, which will depend on the state, will show up in the STP stream.
> But again, I might be missing your point.
>
> Thanks,
> Mathieu
>
>>
>> Regards,
>> --
>> Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* Re: [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-22 18:01                 ` Mathieu Poirier
  0 siblings, 0 replies; 68+ messages in thread
From: Mathieu Poirier @ 2016-02-22 18:01 UTC (permalink / raw)
  To: Alexander Shishkin
  Cc: Chunyan Zhang, Rob Herring, Mark Brown, Pratik Patel,
	Nicolas GUION, Jon Corbet, Mark Rutland, Mike Leach, Jeremiassen,
	Tor, Al Grant, Lyra Zhang, linux-kernel, linux-arm-kernel,
	linux-api, linux-doc

On 12 February 2016 at 13:33, Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
> On 12 February 2016 at 09:27, Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
>> Mathieu Poirier <mathieu.poirier@linaro.org> writes:
>>
>>> On 8 February 2016 at 06:26, Alexander Shishkin
>>> <alexander.shishkin@linux.intel.com> wrote:
>>>> This $end==$start situation itself may be ambiguous and can be
>>>> interpreted either as having just one *static* master ID fixed for all
>>>> SW writers (what I assumed from your commit message) or as having a
>>>> floating master ID, which changes of its own accord and is not
>>>> controllable by software.
>>>
>>> Some clarification here.
>>>
>>> On ARM from a SW point of view $end == $start and that doesn't change
>>> (with regards to masterIDs) .  The ambiguity comes from the fact that
>>> on other platforms where masterID configuration does change and is
>>> important, the condition $end == $start could also be valid.
>>
>> Yes, that's what I was saying. The thing is, on the system-under-tracing
>> side these two situations are not very different from one
>> another. Master IDs are really just numbers without any semantics
>> attached to them in the sense that they are not covered by the mipi spec
>> or any other standard (to my knowledge).
>
> We are definitely on the same page here, just using slightly different terms.
>
>>
>> The difference is in the way we map channels to masters. One way is to
>> allocate a distinct set of channels for each master (the way Intel Trace
>> Hub does it); another way is to share the same set of channels between
>> multiple masters.
>
> We are in total agreement.
>
>> So we can describe this as "hardware implements the
>> same set of channels across multiple masters" or something along those
>> lines.
>
> I suggest "Shared channels"?  In the end, that's really what it is...
>
> The outstanding issue is still how to represent these to different way
> of mapping things in the STM core.  I suggested a flag, called
> "mstatic" (but that can be changed), and a representation of '-1' in
> for masterIDs sysFS.  Whether we stick with that or not is irrelevant,
> I'd be fine with another mechanism.  What I am keen on is that from
> sysFS users can quickly tell which heuristic is enacted on that
> specific architecture.

Alex,

How do you want to proceed with the above?  Do you agree with my
current proposal or can you think of a better way?

Thanks,
Mathieu


>
>>
>> Actually, in the latter scheme of things you can also have multiple
>> masters, at least theoretically. Say, you have masters [0..15], each
>> with distinct set of channels, but depending on hardware state these
>> masters actually end up as $state*16+$masterID in the STP stream.
>>
>> So we might also think about differentiating between the hardware
>> masters (0 though 15 in the above example) and STP masters.
>
> I'm not sure I get what you mean here.  On ARM the masterIDs assigned
> in HW, which will depend on the state, will show up in the STP stream.
> But again, I might be missing your point.
>
> Thanks,
> Mathieu
>
>>
>> Regards,
>> --
>> Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
* [PATCH V2 3/6] stm class: provision for statically assigned masterIDs
@ 2016-02-22 18:01                 ` Mathieu Poirier
  0 siblings, 0 replies; 68+ messages in thread
From: Mathieu Poirier @ 2016-02-22 18:01 UTC (permalink / raw)
  To: linux-arm-kernel

On 12 February 2016 at 13:33, Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
> On 12 February 2016 at 09:27, Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
>> Mathieu Poirier <mathieu.poirier@linaro.org> writes:
>>
>>> On 8 February 2016 at 06:26, Alexander Shishkin
>>> <alexander.shishkin@linux.intel.com> wrote:
>>>> This $end==$start situation itself may be ambiguous and can be
>>>> interpreted either as having just one *static* master ID fixed for all
>>>> SW writers (what I assumed from your commit message) or as having a
>>>> floating master ID, which changes of its own accord and is not
>>>> controllable by software.
>>>
>>> Some clarification here.
>>>
>>> On ARM from a SW point of view $end == $start and that doesn't change
>>> (with regards to masterIDs) .  The ambiguity comes from the fact that
>>> on other platforms where masterID configuration does change and is
>>> important, the condition $end == $start could also be valid.
>>
>> Yes, that's what I was saying. The thing is, on the system-under-tracing
>> side these two situations are not very different from one
>> another. Master IDs are really just numbers without any semantics
>> attached to them in the sense that they are not covered by the mipi spec
>> or any other standard (to my knowledge).
>
> We are definitely on the same page here, just using slightly different terms.
>
>>
>> The difference is in the way we map channels to masters. One way is to
>> allocate a distinct set of channels for each master (the way Intel Trace
>> Hub does it); another way is to share the same set of channels between
>> multiple masters.
>
> We are in total agreement.
>
>> So we can describe this as "hardware implements the
>> same set of channels across multiple masters" or something along those
>> lines.
>
> I suggest "Shared channels"?  In the end, that's really what it is...
>
> The outstanding issue is still how to represent these to different way
> of mapping things in the STM core.  I suggested a flag, called
> "mstatic" (but that can be changed), and a representation of '-1' in
> for masterIDs sysFS.  Whether we stick with that or not is irrelevant,
> I'd be fine with another mechanism.  What I am keen on is that from
> sysFS users can quickly tell which heuristic is enacted on that
> specific architecture.

Alex,

How do you want to proceed with the above?  Do you agree with my
current proposal or can you think of a better way?

Thanks,
Mathieu


>
>>
>> Actually, in the latter scheme of things you can also have multiple
>> masters, at least theoretically. Say, you have masters [0..15], each
>> with distinct set of channels, but depending on hardware state these
>> masters actually end up as $state*16+$masterID in the STP stream.
>>
>> So we might also think about differentiating between the hardware
>> masters (0 though 15 in the above example) and STP masters.
>
> I'm not sure I get what you mean here.  On ARM the masterIDs assigned
> in HW, which will depend on the state, will show up in the STP stream.
> But again, I might be missing your point.
>
> Thanks,
> Mathieu
>
>>
>> Regards,
>> --
>> Alex

^ permalink raw reply	[flat|nested] 68+ messages in thread
end of thread, other threads:[~2016-02-22 18:01 UTC | newest]

Thread overview: 68+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-03  8:15 [PATCH V2 0/6] Introduce CoreSight STM support Chunyan Zhang
2016-02-03  8:15 ` Chunyan Zhang
2016-02-03  8:15 ` [PATCH V2 1/6] stm class: Add ioctl get_options interface Chunyan Zhang
2016-02-03  8:15   ` Chunyan Zhang
2016-02-05 12:55   ` Alexander Shishkin
2016-02-05 12:55     ` Alexander Shishkin
2016-02-05 12:55     ` Alexander Shishkin
2016-02-03  8:15 ` [PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name Chunyan Zhang
2016-02-03  8:15   ` Chunyan Zhang
2016-02-03 10:05   ` [PATCH] stm class: fix semicolon.cocci warnings kbuild test robot
2016-02-03 10:05     ` kbuild test robot
2016-02-03 10:05   ` [PATCH V2 2/6] stm class: adds a loop to extract the first valid STM device name kbuild test robot
2016-02-03 10:05     ` kbuild test robot
2016-02-04  8:56   ` Chunyan Zhang
2016-02-04  8:56     ` Chunyan Zhang
2016-02-04 17:30     ` Alexander Shishkin
2016-02-04 17:30       ` Alexander Shishkin
2016-02-05  3:18       ` Chunyan Zhang
2016-02-05  3:18         ` Chunyan Zhang
2016-02-05  3:18         ` Chunyan Zhang
2016-02-03  8:15 ` [PATCH V2 3/6] stm class: provision for statically assigned masterIDs Chunyan Zhang
2016-02-03  8:15   ` Chunyan Zhang
2016-02-05 12:52   ` Alexander Shishkin
2016-02-05 12:52     ` Alexander Shishkin
2016-02-05 16:31     ` Mike Leach
2016-02-05 16:31       ` Mike Leach
2016-02-05 16:31       ` Mike Leach
2016-02-08 10:52       ` Alexander Shishkin
2016-02-08 10:52         ` Alexander Shishkin
2016-02-08 10:52         ` Alexander Shishkin
2016-02-05 18:08     ` Mathieu Poirier
2016-02-05 18:08       ` Mathieu Poirier
2016-02-05 18:08       ` Mathieu Poirier
2016-02-08 13:26       ` Alexander Shishkin
2016-02-08 13:26         ` Alexander Shishkin
2016-02-08 13:26         ` Alexander Shishkin
2016-02-08 17:05         ` Mathieu Poirier
2016-02-08 17:05           ` Mathieu Poirier
2016-02-08 17:05           ` Mathieu Poirier
2016-02-08 17:44           ` Al Grant
2016-02-08 17:44             ` Al Grant
2016-02-08 17:44             ` Al Grant
2016-02-09 17:06             ` Mathieu Poirier
2016-02-09 17:06               ` Mathieu Poirier
2016-02-09 17:06               ` Mathieu Poirier
2016-02-12 15:54             ` Alexander Shishkin
2016-02-12 15:54               ` Alexander Shishkin
2016-02-12 15:54               ` Alexander Shishkin
2016-02-12 16:27           ` Alexander Shishkin
2016-02-12 16:27             ` Alexander Shishkin
2016-02-12 16:27             ` Alexander Shishkin
2016-02-12 20:33             ` Mathieu Poirier
2016-02-12 20:33               ` Mathieu Poirier
2016-02-12 20:33               ` Mathieu Poirier
2016-02-22 18:01               ` Mathieu Poirier
2016-02-22 18:01                 ` Mathieu Poirier
2016-02-22 18:01                 ` Mathieu Poirier
2016-02-03  8:15 ` [PATCH V2 4/6] Documentations: Add explanations of the case for non-configurable masters Chunyan Zhang
2016-02-03  8:15   ` Chunyan Zhang
2016-02-03  8:15 ` [PATCH V2 5/6] coresight-stm: Bindings for System Trace Macrocell Chunyan Zhang
2016-02-03  8:15   ` Chunyan Zhang
2016-02-03  8:15 ` [PATCH V2 6/6] coresight-stm: adding driver for CoreSight STM component Chunyan Zhang
2016-02-03  8:15   ` Chunyan Zhang
2016-02-05 13:06   ` Alexander Shishkin
2016-02-05 13:06     ` Alexander Shishkin
2016-02-05 14:30     ` Arnd Bergmann
2016-02-05 14:30       ` Arnd Bergmann
2016-02-05 14:30       ` Arnd Bergmann

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.