Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 4/8] coresight: reset "enable_sink" flag when need be
From: Mathieu Poirier @ 2016-11-10 18:18 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1478801934-26904-1-git-send-email-mathieu.poirier@linaro.org>

When using coresight from the perf interface sinks are specified
as part of the perf command line.  As such the sink needs to be
disabled once it has been acknowledged by the coresight framework.
Otherwise the sink stays enabled, which may interfere with other
sessions.

This patch removes the sink selection check from the build path
process and make it a function on it's own.  The function is
then used when operating from sysFS or perf to determine what
sink has been selected.

If operated from perf the status of the "enable_sink" flag is
reset so that concurrent session can use a different sink.  When
used from sysFS the status of the flag is left untouched since
users have full control.

The implementation doesn't handle a scenario where a sink has
been enabled from sysFS and another sink is selected from the
perf command line as both modes of operation are mutually
exclusive.

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

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 2cd7c718198a..5a346fc8ce06 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -202,6 +202,21 @@ static void *etm_setup_aux(int event_cpu, void **pages,
 	if (!event_data)
 		return NULL;
 
+	/*
+	 * In theory nothing prevent tracers in a trace session from being
+	 * associated with different sinks, nor having a sink per tracer.  But
+	 * until we have HW with this kind of topology we need to assume tracers
+	 * in a trace session are using the same sink.  Therefore go through
+	 * the coresight bus and pick the first enabled sink.
+	 *
+	 * When operated from sysFS users are responsible to enable the sink
+	 * while from perf, the perf tools will do it based on the choice made
+	 * on the cmd line.  As such the "enable_sink" flag in sysFS is reset.
+	 */
+	sink = coresight_get_enabled_sink(true);
+	if (!sink)
+		return NULL;
+
 	INIT_WORK(&event_data->work, free_event_data);
 
 	mask = &event_data->mask;
@@ -219,25 +234,11 @@ static void *etm_setup_aux(int event_cpu, void **pages,
 		 * list of devices from source to sink that can be
 		 * referenced later when the path is actually needed.
 		 */
-		event_data->path[cpu] = coresight_build_path(csdev);
+		event_data->path[cpu] = coresight_build_path(csdev, sink);
 		if (IS_ERR(event_data->path[cpu]))
 			goto err;
 	}
 
-	/*
-	 * In theory nothing prevent tracers in a trace session from being
-	 * associated with different sinks, nor having a sink per tracer.  But
-	 * until we have HW with this kind of topology and a way to convey
-	 * sink assignement from the perf cmd line we need to assume tracers
-	 * in a trace session are using the same sink.  Therefore pick the sink
-	 * found at the end of the first available path.
-	 */
-	cpu = cpumask_first(mask);
-	/* Grab the sink at the end of the path */
-	sink = coresight_get_sink(event_data->path[cpu]);
-	if (!sink)
-		goto err;
-
 	if (!sink_ops(sink)->alloc_buffer)
 		goto err;
 
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index 196a14be4b3d..ef9d8e93e3b2 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -111,7 +111,9 @@ static inline void CS_UNLOCK(void __iomem *addr)
 void coresight_disable_path(struct list_head *path);
 int coresight_enable_path(struct list_head *path, u32 mode);
 struct coresight_device *coresight_get_sink(struct list_head *path);
-struct list_head *coresight_build_path(struct coresight_device *csdev);
+struct coresight_device *coresight_get_enabled_sink(bool reset);
+struct list_head *coresight_build_path(struct coresight_device *csdev,
+				       struct coresight_device *sink);
 void coresight_release_path(struct list_head *path);
 
 #ifdef CONFIG_CORESIGHT_SOURCE_ETM3X
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 7bf00a0beb6f..0c37356e417c 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -368,6 +368,52 @@ struct coresight_device *coresight_get_sink(struct list_head *path)
 	return csdev;
 }
 
+static int coresight_enabled_sink(struct device *dev, void *data)
+{
+	bool *reset = data;
+	struct coresight_device *csdev = to_coresight_device(dev);
+
+	if ((csdev->type == CORESIGHT_DEV_TYPE_SINK ||
+	     csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) &&
+	     csdev->activated) {
+		/*
+		 * Now that we have a handle on the sink for this session,
+		 * disable the sysFS "enable_sink" flag so that possible
+		 * concurrent perf session that wish to use another sink don't
+		 * trip on it.  Doing so has no ramification for the current
+		 * session.
+		 */
+		if (*reset)
+			csdev->activated = false;
+
+		return 1;
+	}
+
+	return 0;
+}
+
+/**
+ * coresight_get_enabled_sink - returns the first enabled sink found on the bus
+ * @deactivate:	Whether the 'enable_sink' flag should be reset
+ *
+ * When operated from perf the deactivate parameter should be set to 'true'.
+ * That way the "enabled_sink" flag of the sink that was selected can be reset,
+ * allowing for other concurrent perf sessions to choose a different sink.
+ *
+ * When operated from sysFS users have full control and as such the deactivate
+ * parameter should be set to 'false', hence mandating users to explicitly
+ * clear the flag.
+ */
+struct coresight_device *coresight_get_enabled_sink(bool deactivate)
+{
+	struct device *dev = NULL;
+
+	dev = bus_find_device(&coresight_bustype, NULL, &deactivate,
+			      coresight_enabled_sink);
+
+	return dev ? to_coresight_device(dev) : NULL;
+}
+
 /**
  * _coresight_build_path - recursively build a path from a @csdev to a sink.
  * @csdev:	The device to start from.
@@ -380,6 +426,7 @@ struct coresight_device *coresight_get_sink(struct list_head *path)
  * last one.
  */
 static int _coresight_build_path(struct coresight_device *csdev,
+				 struct coresight_device *sink,
 				 struct list_head *path)
 {
 	int i;
@@ -387,15 +434,15 @@ static int _coresight_build_path(struct coresight_device *csdev,
 	struct coresight_node *node;
 
 	/* An activated sink has been found.  Enqueue the element */
-	if ((csdev->type == CORESIGHT_DEV_TYPE_SINK ||
-	     csdev->type == CORESIGHT_DEV_TYPE_LINKSINK) && csdev->activated)
+	if (csdev == sink)
 		goto out;
 
 	/* Not a sink - recursively explore each port found on this element */
 	for (i = 0; i < csdev->nr_outport; i++) {
 		struct coresight_device *child_dev = csdev->conns[i].child_dev;
 
-		if (child_dev && _coresight_build_path(child_dev, path) == 0) {
+		if (child_dev &&
+		    _coresight_build_path(child_dev, sink, path) == 0) {
 			found = true;
 			break;
 		}
@@ -422,18 +469,22 @@ static int _coresight_build_path(struct coresight_device *csdev,
 	return 0;
 }
 
-struct list_head *coresight_build_path(struct coresight_device *csdev)
+struct list_head *coresight_build_path(struct coresight_device *source,
+				       struct coresight_device *sink)
 {
 	struct list_head *path;
 	int rc;
 
+	if (!sink)
+		return ERR_PTR(-EINVAL);
+
 	path = kzalloc(sizeof(struct list_head), GFP_KERNEL);
 	if (!path)
 		return ERR_PTR(-ENOMEM);
 
 	INIT_LIST_HEAD(path);
 
-	rc = _coresight_build_path(csdev, path);
+	rc = _coresight_build_path(source, sink, path);
 	if (rc) {
 		kfree(path);
 		return ERR_PTR(rc);
@@ -497,6 +548,7 @@ static int coresight_validate_source(struct coresight_device *csdev,
 int coresight_enable(struct coresight_device *csdev)
 {
 	int cpu, ret = 0;
+	struct coresight_device *sink;
 	struct list_head *path;
 
 	mutex_lock(&coresight_mutex);
@@ -508,7 +560,17 @@ int coresight_enable(struct coresight_device *csdev)
 	if (csdev->enable)
 		goto out;
 
-	path = coresight_build_path(csdev);
+	/*
+	 * Search for a valid sink for this session but don't reset the
+	 * "enable_sink" flag in sysFS.  Users get to do that explicitly.
+	 */
+	sink = coresight_get_enabled_sink(false);
+	if (!sink) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	path = coresight_build_path(csdev, sink);
 	if (IS_ERR(path)) {
 		pr_err("building path(s) failed\n");
 		ret = PTR_ERR(path);
-- 
2.7.4

^ permalink raw reply related

* [PATCH 5/8] coresight: tmc: Cleanup operation mode handling
From: Mathieu Poirier @ 2016-11-10 18:18 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1478801934-26904-1-git-send-email-mathieu.poirier@linaro.org>

From: "Suzuki K. Poulose" <Suzuki.Poulose@arm.com>

The mode of operation of the TMC tracked in drvdata->mode is defined
as a local_t type. This is always checked and modified under the
drvdata->spinlock and hence we don't need local_t for it and the
unnecessary synchronisation instructions that comes with it. This
change makes the code a bit more cleaner.

Also fixes the order in which we update the drvdata->mode to
CS_MODE_DISABLED. i.e, in tmc_disable_etX_sink we change the
mode to CS_MODE_DISABLED before invoking tmc_disable_etX_hw()
which in turn depends on the mode to decide whether to dump the
trace to a buffer.

Applies on mathieu's coresight/next tree [1]

https://git.linaro.org/kernel/coresight.git next

Reported-by: Venkatesh Vivekanandan <venkatesh.vivekanandan@broadcom.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/hwtracing/coresight/coresight-tmc-etf.c | 32 +++++++++++--------------
 drivers/hwtracing/coresight/coresight-tmc-etr.c | 26 +++++++++-----------
 drivers/hwtracing/coresight/coresight-tmc.h     |  2 +-
 3 files changed, 26 insertions(+), 34 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index d6941ea24d8d..e80a8f4cd12e 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -70,7 +70,7 @@ static void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
 	 * When operating in sysFS mode the content of the buffer needs to be
 	 * read before the TMC is disabled.
 	 */
-	if (local_read(&drvdata->mode) == CS_MODE_SYSFS)
+	if (drvdata->mode == CS_MODE_SYSFS)
 		tmc_etb_dump_hw(drvdata);
 	tmc_disable_hw(drvdata);
 
@@ -108,7 +108,6 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev, u32 mode)
 	int ret = 0;
 	bool used = false;
 	char *buf = NULL;
-	long val;
 	unsigned long flags;
 	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 
@@ -138,13 +137,12 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev, u32 mode)
 		goto out;
 	}
 
-	val = local_xchg(&drvdata->mode, mode);
 	/*
 	 * In sysFS mode we can have multiple writers per sink.  Since this
 	 * sink is already enabled no memory is needed and the HW need not be
 	 * touched.
 	 */
-	if (val == CS_MODE_SYSFS)
+	if (drvdata->mode == CS_MODE_SYSFS)
 		goto out;
 
 	/*
@@ -163,6 +161,7 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev, u32 mode)
 		drvdata->buf = buf;
 	}
 
+	drvdata->mode = CS_MODE_SYSFS;
 	tmc_etb_enable_hw(drvdata);
 out:
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -180,7 +179,6 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev, u32 mode)
 static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, u32 mode)
 {
 	int ret = 0;
-	long val;
 	unsigned long flags;
 	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 
@@ -194,17 +192,17 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, u32 mode)
 		goto out;
 	}
 
-	val = local_xchg(&drvdata->mode, mode);
 	/*
 	 * In Perf mode there can be only one writer per sink.  There
 	 * is also no need to continue if the ETB/ETR is already operated
 	 * from sysFS.
 	 */
-	if (val != CS_MODE_DISABLED) {
+	if (drvdata->mode != CS_MODE_DISABLED) {
 		ret = -EINVAL;
 		goto out;
 	}
 
+	drvdata->mode = mode;
 	tmc_etb_enable_hw(drvdata);
 out:
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -227,7 +225,6 @@ static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
 
 static void tmc_disable_etf_sink(struct coresight_device *csdev)
 {
-	long val;
 	unsigned long flags;
 	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 
@@ -237,10 +234,11 @@ static void tmc_disable_etf_sink(struct coresight_device *csdev)
 		return;
 	}
 
-	val = local_xchg(&drvdata->mode, CS_MODE_DISABLED);
 	/* Disable the TMC only if it needs to */
-	if (val != CS_MODE_DISABLED)
+	if (drvdata->mode != CS_MODE_DISABLED) {
 		tmc_etb_disable_hw(drvdata);
+		drvdata->mode = CS_MODE_DISABLED;
+	}
 
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
@@ -260,7 +258,7 @@ static int tmc_enable_etf_link(struct coresight_device *csdev,
 	}
 
 	tmc_etf_enable_hw(drvdata);
-	local_set(&drvdata->mode, CS_MODE_SYSFS);
+	drvdata->mode = CS_MODE_SYSFS;
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
 	dev_info(drvdata->dev, "TMC-ETF enabled\n");
@@ -280,7 +278,7 @@ static void tmc_disable_etf_link(struct coresight_device *csdev,
 	}
 
 	tmc_etf_disable_hw(drvdata);
-	local_set(&drvdata->mode, CS_MODE_DISABLED);
+	drvdata->mode = CS_MODE_DISABLED;
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
 	dev_info(drvdata->dev, "TMC disabled\n");
@@ -383,7 +381,7 @@ static void tmc_update_etf_buffer(struct coresight_device *csdev,
 		return;
 
 	/* This shouldn't happen */
-	if (WARN_ON_ONCE(local_read(&drvdata->mode) != CS_MODE_PERF))
+	if (WARN_ON_ONCE(drvdata->mode != CS_MODE_PERF))
 		return;
 
 	CS_UNLOCK(drvdata->base);
@@ -504,7 +502,6 @@ const struct coresight_ops tmc_etf_cs_ops = {
 
 int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
 {
-	long val;
 	enum tmc_mode mode;
 	int ret = 0;
 	unsigned long flags;
@@ -528,9 +525,8 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
 		goto out;
 	}
 
-	val = local_read(&drvdata->mode);
 	/* Don't interfere if operated from Perf */
-	if (val == CS_MODE_PERF) {
+	if (drvdata->mode == CS_MODE_PERF) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -542,7 +538,7 @@ int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
 	}
 
 	/* Disable the TMC if need be */
-	if (val == CS_MODE_SYSFS)
+	if (drvdata->mode == CS_MODE_SYSFS)
 		tmc_etb_disable_hw(drvdata);
 
 	drvdata->reading = true;
@@ -573,7 +569,7 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
 	}
 
 	/* Re-enable the TMC if need be */
-	if (local_read(&drvdata->mode) == CS_MODE_SYSFS) {
+	if (drvdata->mode == CS_MODE_SYSFS) {
 		/*
 		 * The trace run will continue with the same allocated trace
 		 * buffer. As such zero-out the buffer so that we don't end
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 886ea83c68e0..f23ef0c23303 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -86,7 +86,7 @@ static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
 	 * When operating in sysFS mode the content of the buffer needs to be
 	 * read before the TMC is disabled.
 	 */
-	if (local_read(&drvdata->mode) == CS_MODE_SYSFS)
+	if (drvdata->mode == CS_MODE_SYSFS)
 		tmc_etr_dump_hw(drvdata);
 	tmc_disable_hw(drvdata);
 
@@ -97,7 +97,6 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev, u32 mode)
 {
 	int ret = 0;
 	bool used = false;
-	long val;
 	unsigned long flags;
 	void __iomem *vaddr = NULL;
 	dma_addr_t paddr;
@@ -134,13 +133,12 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev, u32 mode)
 		goto out;
 	}
 
-	val = local_xchg(&drvdata->mode, mode);
 	/*
 	 * In sysFS mode we can have multiple writers per sink.  Since this
 	 * sink is already enabled no memory is needed and the HW need not be
 	 * touched.
 	 */
-	if (val == CS_MODE_SYSFS)
+	if (drvdata->mode == CS_MODE_SYSFS)
 		goto out;
 
 	/*
@@ -157,6 +155,7 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev, u32 mode)
 
 	memset(drvdata->vaddr, 0, drvdata->size);
 
+	drvdata->mode = CS_MODE_SYSFS;
 	tmc_etr_enable_hw(drvdata);
 out:
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -174,7 +173,6 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev, u32 mode)
 static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, u32 mode)
 {
 	int ret = 0;
-	long val;
 	unsigned long flags;
 	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 
@@ -188,17 +186,17 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, u32 mode)
 		goto out;
 	}
 
-	val = local_xchg(&drvdata->mode, mode);
 	/*
 	 * In Perf mode there can be only one writer per sink.  There
 	 * is also no need to continue if the ETR is already operated
 	 * from sysFS.
 	 */
-	if (val != CS_MODE_DISABLED) {
+	if (drvdata->mode != CS_MODE_DISABLED) {
 		ret = -EINVAL;
 		goto out;
 	}
 
+	drvdata->mode = CS_MODE_PERF;
 	tmc_etr_enable_hw(drvdata);
 out:
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -221,7 +219,6 @@ static int tmc_enable_etr_sink(struct coresight_device *csdev, u32 mode)
 
 static void tmc_disable_etr_sink(struct coresight_device *csdev)
 {
-	long val;
 	unsigned long flags;
 	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 
@@ -231,10 +228,11 @@ static void tmc_disable_etr_sink(struct coresight_device *csdev)
 		return;
 	}
 
-	val = local_xchg(&drvdata->mode, CS_MODE_DISABLED);
 	/* Disable the TMC only if it needs to */
-	if (val != CS_MODE_DISABLED)
+	if (drvdata->mode != CS_MODE_DISABLED) {
 		tmc_etr_disable_hw(drvdata);
+		drvdata->mode = CS_MODE_DISABLED;
+	}
 
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 
@@ -253,7 +251,6 @@ const struct coresight_ops tmc_etr_cs_ops = {
 int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
 {
 	int ret = 0;
-	long val;
 	unsigned long flags;
 
 	/* config types are set a boot time and never change */
@@ -266,9 +263,8 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
 		goto out;
 	}
 
-	val = local_read(&drvdata->mode);
 	/* Don't interfere if operated from Perf */
-	if (val == CS_MODE_PERF) {
+	if (drvdata->mode == CS_MODE_PERF) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -280,7 +276,7 @@ int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
 	}
 
 	/* Disable the TMC if need be */
-	if (val == CS_MODE_SYSFS)
+	if (drvdata->mode == CS_MODE_SYSFS)
 		tmc_etr_disable_hw(drvdata);
 
 	drvdata->reading = true;
@@ -303,7 +299,7 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
 	spin_lock_irqsave(&drvdata->spinlock, flags);
 
 	/* RE-enable the TMC if need be */
-	if (local_read(&drvdata->mode) == CS_MODE_SYSFS) {
+	if (drvdata->mode == CS_MODE_SYSFS) {
 		/*
 		 * The trace run will continue with the same allocated trace
 		 * buffer. The trace buffer is cleared in tmc_etr_enable_hw(),
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index 44b3ae346118..51c01851533e 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -117,7 +117,7 @@ struct tmc_drvdata {
 	void __iomem		*vaddr;
 	u32			size;
 	u32			len;
-	local_t			mode;
+	u32			mode;
 	enum tmc_config_type	config_type;
 	enum tmc_mem_intf_width	memwidth;
 	u32			trigger_cntr;
-- 
2.7.4

^ permalink raw reply related

* [PATCH 6/8] coresight: tmc: Get rid of mode parameter for helper routines
From: Mathieu Poirier @ 2016-11-10 18:18 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1478801934-26904-1-git-send-email-mathieu.poirier@linaro.org>

From: "Suzuki K. Poulose" <Suzuki.Poulose@arm.com>

Get rid of the superfluous mode parameter and the check for
the mode in tmc_etX_enable_sink_{perf/sysfs}. While at it, also
remove the unnecessary WARN_ON() checks.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/hwtracing/coresight/coresight-tmc-etf.c | 18 +++++-------------
 drivers/hwtracing/coresight/coresight-tmc-etr.c | 15 ++++-----------
 2 files changed, 9 insertions(+), 24 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index e80a8f4cd12e..1549436e2492 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -103,7 +103,7 @@ static void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
 	CS_LOCK(drvdata->base);
 }
 
-static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev, u32 mode)
+static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev)
 {
 	int ret = 0;
 	bool used = false;
@@ -111,10 +111,6 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev, u32 mode)
 	unsigned long flags;
 	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 
-	 /* This shouldn't be happening */
-	if (WARN_ON(mode != CS_MODE_SYSFS))
-		return -EINVAL;
-
 	/*
 	 * If we don't have a buffer release the lock and allocate memory.
 	 * Otherwise keep the lock and move along.
@@ -176,16 +172,12 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev, u32 mode)
 	return ret;
 }
 
-static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, u32 mode)
+static int tmc_enable_etf_sink_perf(struct coresight_device *csdev)
 {
 	int ret = 0;
 	unsigned long flags;
 	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 
-	 /* This shouldn't be happening */
-	if (WARN_ON(mode != CS_MODE_PERF))
-		return -EINVAL;
-
 	spin_lock_irqsave(&drvdata->spinlock, flags);
 	if (drvdata->reading) {
 		ret = -EINVAL;
@@ -202,7 +194,7 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, u32 mode)
 		goto out;
 	}
 
-	drvdata->mode = mode;
+	drvdata->mode = CS_MODE_PERF;
 	tmc_etb_enable_hw(drvdata);
 out:
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -214,9 +206,9 @@ static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
 {
 	switch (mode) {
 	case CS_MODE_SYSFS:
-		return tmc_enable_etf_sink_sysfs(csdev, mode);
+		return tmc_enable_etf_sink_sysfs(csdev);
 	case CS_MODE_PERF:
-		return tmc_enable_etf_sink_perf(csdev, mode);
+		return tmc_enable_etf_sink_perf(csdev);
 	}
 
 	/* We shouldn't be here */
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index f23ef0c23303..3b84d0d38c22 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -93,7 +93,7 @@ static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
 	CS_LOCK(drvdata->base);
 }
 
-static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev, u32 mode)
+static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
 {
 	int ret = 0;
 	bool used = false;
@@ -102,9 +102,6 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev, u32 mode)
 	dma_addr_t paddr;
 	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 
-	 /* This shouldn't be happening */
-	if (WARN_ON(mode != CS_MODE_SYSFS))
-		return -EINVAL;
 
 	/*
 	 * If we don't have a buffer release the lock and allocate memory.
@@ -170,16 +167,12 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev, u32 mode)
 	return ret;
 }
 
-static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, u32 mode)
+static int tmc_enable_etr_sink_perf(struct coresight_device *csdev)
 {
 	int ret = 0;
 	unsigned long flags;
 	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
 
-	 /* This shouldn't be happening */
-	if (WARN_ON(mode != CS_MODE_PERF))
-		return -EINVAL;
-
 	spin_lock_irqsave(&drvdata->spinlock, flags);
 	if (drvdata->reading) {
 		ret = -EINVAL;
@@ -208,9 +201,9 @@ static int tmc_enable_etr_sink(struct coresight_device *csdev, u32 mode)
 {
 	switch (mode) {
 	case CS_MODE_SYSFS:
-		return tmc_enable_etr_sink_sysfs(csdev, mode);
+		return tmc_enable_etr_sink_sysfs(csdev);
 	case CS_MODE_PERF:
-		return tmc_enable_etr_sink_perf(csdev, mode);
+		return tmc_enable_etr_sink_perf(csdev);
 	}
 
 	/* We shouldn't be here */
-- 
2.7.4

^ permalink raw reply related

* [PATCH 7/8] coresight: tmc: Remove duplicate memset
From: Mathieu Poirier @ 2016-11-10 18:18 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1478801934-26904-1-git-send-email-mathieu.poirier@linaro.org>

From: "Suzuki K. Poulose" <Suzuki.Poulose@arm.com>

The tmc_etr_enable_hw() fills the buffer with 0's before enabling
the hardware. So, we don't need an explicit memset() in
tmc_enable_etr_sink_sysfs() before calling the tmc_etr_enable_hw().
This patch removes the explicit memset from tmc_enable_etr_sink_sysfs.

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

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 3b84d0d38c22..5d312699b3b9 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -150,8 +150,6 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
 		drvdata->buf = drvdata->vaddr;
 	}
 
-	memset(drvdata->vaddr, 0, drvdata->size);
-
 	drvdata->mode = CS_MODE_SYSFS;
 	tmc_etr_enable_hw(drvdata);
 out:
-- 
2.7.4

^ permalink raw reply related

* [PATCH 8/8] coresight: Add support for ARM Coresight STM-500
From: Mathieu Poirier @ 2016-11-10 18:18 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1478801934-26904-1-git-send-email-mathieu.poirier@linaro.org>

From: Suzuki K Poulose <suzuki.poulose@arm.com>

Add the PIDs for STM-500 to the known STM devices list.

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Chunyan Zhang <zhang.chunyan@linaro.org>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/hwtracing/coresight/coresight-stm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
index d397849c2c6a..944c17b48d23 100644
--- a/drivers/hwtracing/coresight/coresight-stm.c
+++ b/drivers/hwtracing/coresight/coresight-stm.c
@@ -920,6 +920,11 @@ static struct amba_id stm_ids[] = {
 		.mask   = 0x0003ffff,
 		.data	= "STM32",
 	},
+	{
+		.id	= 0x0003b963,
+		.mask	= 0x0003ffff,
+		.data	= "STM500",
+	},
 	{ 0, 0},
 };
 
-- 
2.7.4

^ permalink raw reply related

* [PATCH v2 2/3] irqchip: mtk-cirq: Add mediatek mtk-cirq implement
From: Marc Zyngier @ 2016-11-10 18:24 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1478573833.32099.11.camel@mtksdaap41>

On 08/11/16 02:57, Youlin Pei wrote:
> On Fri, 2016-11-04 at 22:21 +0000, Marc Zyngier wrote:
>> On Fri, Nov 04 2016 at 04:42:57 AM, Youlin Pei <youlin.pei@mediatek.com> wrote:
>>> On Tue, 2016-11-01 at 20:49 +0000, Marc Zyngier wrote:
>>>> On Tue, Nov 01 2016 at 11:52:01 AM, Youlin Pei <youlin.pei@mediatek.com> wrote:
>>>>> In Mediatek SOCs, the CIRQ is a low power interrupt controller
>>>>> designed to works outside MCUSYS which comprises with Cortex-Ax
>>>>> cores,CCI and GIC.
>>>>>
>>>>> The CIRQ controller is integrated in between MCUSYS( include
>>>>> Cortex-Ax, CCI and GIC ) and interrupt sources as the second
>>>>> level interrupt controller. The external interrupts which outside
>>>>> MCUSYS will feed through CIRQ then bypass to GIC. CIRQ can monitors
>>>>> all edge trigger interupts. When an edge interrupt is triggered,
>>>>> CIRQ can record the status and generate a pulse signal to GIC when
>>>>> flush command executed.
>>>>>
>>>>> When system enters sleep mode, MCUSYS will be turned off to improve
>>>>> power consumption, also GIC is power down. The edge trigger interrupts
>>>>> will be lost in this scenario without CIRQ.
>>>>>
>>>>> This commit provides the CIRQ irqchip implement.
>>>>>
>>>>> Signed-off-by: Youlin Pei <youlin.pei@mediatek.com>
>>>>> ---
>>>>>  drivers/irqchip/Makefile       |    2 +-
>>>>>  drivers/irqchip/irq-mtk-cirq.c |  262 ++++++++++++++++++++++++++++++++++++++++
>>>>>  2 files changed, 263 insertions(+), 1 deletion(-)
>>>>>  create mode 100644 drivers/irqchip/irq-mtk-cirq.c
>>>>>
>>>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>>>>> index e4dbfc8..8f33580 100644
>>>>> --- a/drivers/irqchip/Makefile
>>>>> +++ b/drivers/irqchip/Makefile
>>>>> @@ -60,7 +60,7 @@ obj-$(CONFIG_BCM7120_L2_IRQ)		+= irq-bcm7120-l2.o
>>>>>  obj-$(CONFIG_BRCMSTB_L2_IRQ)		+= irq-brcmstb-l2.o
>>>>>  obj-$(CONFIG_KEYSTONE_IRQ)		+= irq-keystone.o
>>>>>  obj-$(CONFIG_MIPS_GIC)			+= irq-mips-gic.o
>>>>> -obj-$(CONFIG_ARCH_MEDIATEK)		+= irq-mtk-sysirq.o
>>>>> +obj-$(CONFIG_ARCH_MEDIATEK)		+= irq-mtk-sysirq.o irq-mtk-cirq.o
>>>>>  obj-$(CONFIG_ARCH_DIGICOLOR)		+= irq-digicolor.o
>>>>>  obj-$(CONFIG_RENESAS_H8300H_INTC)	+= irq-renesas-h8300h.o
>>>>>  obj-$(CONFIG_RENESAS_H8S_INTC)		+= irq-renesas-h8s.o
>>>>> diff --git a/drivers/irqchip/irq-mtk-cirq.c b/drivers/irqchip/irq-mtk-cirq.c
>>>>> new file mode 100644
>>>>> index 0000000..fc43ef3
>>>>> --- /dev/null
>>>>> +++ b/drivers/irqchip/irq-mtk-cirq.c
>>>>> @@ -0,0 +1,262 @@
>>>>> +/*
>>>>> + * Copyright (c) 2016 MediaTek Inc.
>>>>> + * Author: Youlin.Pei <youlin.pei@mediatek.com>
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>> + * it under the terms of the GNU General Public License version 2 as
>>>>> + * published by the Free Software Foundation.
>>>>> + *
>>>>> + * This program is distributed in the hope that it will be useful,
>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>> + * GNU General Public License for more details.
>>>>> + */
>>>>> +
>>>>> +#include <linux/irq.h>
>>>>> +#include <linux/irqchip.h>
>>>>> +#include <linux/irqdomain.h>
>>>>> +#include <linux/of.h>
>>>>> +#include <linux/of_irq.h>
>>>>> +#include <linux/of_address.h>
>>>>> +#include <linux/io.h>
>>>>> +#include <linux/slab.h>
>>>>> +#include <linux/syscore_ops.h>
>>>>> +
>>>>> +#define CIRQ_ACK	0x40
>>>>> +#define CIRQ_MASK_SET	0xc0
>>>>> +#define CIRQ_MASK_CLR	0x100
>>>>> +#define CIRQ_SENS_SET	0x180
>>>>> +#define CIRQ_SENS_CLR	0x1c0
>>>>> +#define CIRQ_POL_SET	0x240
>>>>> +#define CIRQ_POL_CLR	0x280
>>>>> +#define CIRQ_CONTROL	0x300
>>>>> +
>>>>> +#define CIRQ_EN	0x1
>>>>> +#define CIRQ_EDGE	0x2
>>>>> +#define CIRQ_FLUSH	0x4
>>>>> +
>>>>> +#define CIRQ_IRQ_NUM    0x200
>>>>> +
>>>>> +struct mtk_cirq_chip_data {
>>>>> +	void __iomem *base;
>>>>> +	unsigned int ext_irq_start;
>>>>> +};
>>>>> +
>>>>> +static struct mtk_cirq_chip_data *cirq_data;
>>>>
>>>> Are you guaranteed that you'll only ever have a single CIRQ in any
>>>> system?
>>>
>>> In Mediatek's SOC, only hace a single CIRQ.
>>>
>>>>
>>>>> +
>>>>> +static void mtk_cirq_write_mask(struct irq_data *data, unsigned int offset)
>>>>> +{
>>>>> +	struct mtk_cirq_chip_data *chip_data = data->chip_data;
>>>>> +	unsigned int cirq_num = data->hwirq;
>>>>> +	u32 mask = 1 << (cirq_num % 32);
>>>>> +
>>>>> +	writel(mask, chip_data->base + offset + (cirq_num / 32) * 4);
>>>>
>>>> Why can't you use the relaxed accessors?
>>>
>>> It seems that i use wrong function, i will change the writel to
>>> writel_relaxed in next ve
>>>
>>>>
>>>>> +}
>>>>> +
>>>>> +static void mtk_cirq_mask(struct irq_data *data)
>>>>> +{
>>>>> +	mtk_cirq_write_mask(data, CIRQ_MASK_SET);
>>>>> +	irq_chip_mask_parent(data);
>>>>> +}
>>>>> +
>>>>> +static void mtk_cirq_unmask(struct irq_data *data)
>>>>> +{
>>>>> +	mtk_cirq_write_mask(data, CIRQ_MASK_CLR);
>>>>> +	irq_chip_unmask_parent(data);
>>>>> +}
>>>>> +
>>>>> +static void mtk_cirq_eoi(struct irq_data *data)
>>>>> +{
>>>>> +	mtk_cirq_write_mask(data, CIRQ_ACK);
>>>>
>>>> EOI and ACK have very different semantics. What is this write actually
>>>> doing? Also, you're now doing an additional MMIO write on each interrupt
>>>> EOI, doubling its cost. Do you really need to do actually signal the HW
>>>> that we've EOIed an interrupt? I would have hoped that you'd be able to
>>>> put it in "bypass" mode as long as you're not suspending...
>>>>
>>>
>>> When external interrupt happened, CIRQ status register record the status
>>> even CIRQ is not enabled. when execute the flush command, CIRQ will
>>> resend the signals according to the status. So if don't clear the
>>> status, CIRQ will resend the wrong signals. the ACK write operation will
>>> clear the status.
>>
>> But at this time, we haven't suspended yet, and there is nothing to
>> replay. Also, you only enable the edge capture when suspending. So what
>> are you ACKing here? Can't you simply clear everything right when
>> suspending and not do it at all on the fast path?
> 
> I had planned to ACK the status in cirq suspend callback, but
> encountered a problem.
> following is a piece of code from
> http://lxr.free-electrons.com/source/kernel/power/suspend.c#L361
> 
> arch_suspend_disable_irqs(); ---step1
> BUG_ON(!irqs_disabled());
> 
> error = syscore_suspend();
>            |
>            ---cirq suspend(); ---step2
> 
> if ack the status in cirq suspend, the interrupts will be lost which
> happened during step1 to step2.
> 
> So would you mind give me some suggestions about this?
> Thanks a lot!

Right. So maybe there is a way:
- On suspend you can iterate over all the cirq interrupts that have been
recorded
- For each of those, you inspect if it is pending at the GIC level
(using the irq_get_irqchip_state helper)
- if not pending, you Ack it, because it cannot be delivered anymore
- If it is pending, then you know it happened between step 1 and step 2.
- You then have the choice of either going into suspend and waking up
immediately, or failing the suspend.

Thoughts?

	M.
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply

* [PATCH v2 1/3] ARM: dts: imx6qdl-apalis: Do not rely on DDC I2C bus bitbang for HDMI
From: Vladimir Zapolskiy @ 2016-11-10 18:29 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <57268103e8fabf344723eae99b45ec7d@agner.ch>

Hi Stefan, Philipp,

On 11/09/2016 02:50 AM, Stefan Agner wrote:
> On 2016-11-08 09:33, maitysanchayan at gmail.com wrote:
>> Hello Shawn,
>>
>> On 16-10-22 15:43:04, Vladimir Zapolskiy wrote:
>>> Hi Shawn,
>>>
>>> On 10/22/2016 06:25 AM, Shawn Guo wrote:
>>>> On Mon, Sep 19, 2016 at 10:41:51AM +0530, Sanchayan Maity wrote:
>>>>> Remove the use of DDC I2C bus bitbang to support reading of EDID
>>>>> and rely on support from internal HDMI I2C master controller instead.
>>>>> As a result remove the device tree property ddc-i2c-bus.
>>>>>
>>>>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
>>>>
>>>> I think that the dw-hdmi i2c support [1] is a prerequisite of this
>>>> patch.  I do not see it lands on v4.9-rc1.  Or am I missing something?
>>>>
>>>> Shawn
>>>>
>>>> [1] https://patchwork.kernel.org/patch/9296883/
>>>>
>>>
>>> I'm adding Philipp to Cc, since he is the last one who tested the change
>>> and helped me to push the change to the mainline:
>>>
>>>   https://lists.freedesktop.org/archives/dri-devel/2016-September/118569.html
>>>
>>> The problem is that there is no official DW HDMI bridge maintainer, may be
>>> you can review the change, and if you find it satisfactory push it through
>>> ARM/iMX tree.
>>
>> Shawn, is it okay if that patch goes through your ARM/iMX tree?
>
> I don't think it makes sense that the DRM bridge changes go through
> Shawn's tree. Dave should merge Philipps pull request...
>

Philipp, do you mind to submit a rebased pull request one more time?

--
With best wishes,
Vladimir

^ permalink raw reply

* [PATCH v6 3/9] drm/hisilicon/hibmc: Add support for frame buffer
From: Sean Paul @ 2016-11-10 18:30 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1477639682-22520-4-git-send-email-zourongrong@gmail.com>

On Fri, Oct 28, 2016 at 3:27 AM, Rongrong Zou <zourongrong@gmail.com> wrote:
> Add support for fbdev and kms fb management.
>
> Signed-off-by: Rongrong Zou <zourongrong@gmail.com>
> ---
>  drivers/gpu/drm/hisilicon/hibmc/Makefile          |   2 +-
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  17 ++
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  24 ++
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c | 255 ++++++++++++++++++++++
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c       |  66 ++++++
>  5 files changed, 363 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c
>
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> index d5c40b8..810a37e 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
> @@ -1,5 +1,5 @@
>  ccflags-y := -Iinclude/drm
> -hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_power.o hibmc_ttm.o
> +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_fbdev.o hibmc_drm_power.o hibmc_ttm.o
>
>  obj-$(CONFIG_DRM_HISI_HIBMC)   +=hibmc-drm.o
>  #obj-y += hibmc-drm.o
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> index 81f4301..5ac7a7e 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
> @@ -66,11 +66,23 @@ static void hibmc_disable_vblank(struct drm_device *dev, unsigned int pipe)
>
>  static int hibmc_pm_suspend(struct device *dev)
>  {
> +       struct pci_dev *pdev = to_pci_dev(dev);
> +       struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +       struct hibmc_drm_device *hidev = drm_dev->dev_private;
> +
> +       drm_fb_helper_set_suspend_unlocked(&hidev->fbdev->helper, 1);
> +

We have atomic helpers for suspend/resume now. Please use those.

>         return 0;
>  }
>
>  static int hibmc_pm_resume(struct device *dev)
>  {
> +       struct pci_dev *pdev = to_pci_dev(dev);
> +       struct drm_device *drm_dev = pci_get_drvdata(pdev);
> +       struct hibmc_drm_device *hidev = drm_dev->dev_private;
> +
> +       drm_fb_helper_set_suspend_unlocked(&hidev->fbdev->helper, 0);
> +
>         return 0;
>  }
>
> @@ -170,6 +182,7 @@ static int hibmc_unload(struct drm_device *dev)
>  {
>         struct hibmc_drm_device *hidev = dev->dev_private;
>
> +       hibmc_fbdev_fini(hidev);
>         hibmc_mm_fini(hidev);
>         hibmc_hw_fini(hidev);
>         dev->dev_private = NULL;
> @@ -195,6 +208,10 @@ static int hibmc_load(struct drm_device *dev, unsigned long flags)
>         if (ret)
>                 goto err;
>
> +       ret = hibmc_fbdev_init(hidev);
> +       if (ret)
> +               goto err;
> +
>         return 0;
>
>  err:
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> index db8d80e..a40e9a7 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
> @@ -20,9 +20,22 @@
>  #define HIBMC_DRM_DRV_H
>
>  #include <drm/drmP.h>
> +#include <drm/drm_fb_helper.h>
>  #include <drm/ttm/ttm_bo_driver.h>
>  #include <drm/drm_gem.h>
>
> +struct hibmc_framebuffer {
> +       struct drm_framebuffer fb;
> +       struct drm_gem_object *obj;
> +       bool is_fbdev_fb;
> +};
> +
> +struct hibmc_fbdev {
> +       struct drm_fb_helper helper;
> +       struct hibmc_framebuffer *fb;
> +       int size;
> +};
> +
>  struct hibmc_drm_device {
>         /* hw */
>         void __iomem   *mmio;
> @@ -41,9 +54,13 @@ struct hibmc_drm_device {
>                 bool initialized;
>         } ttm;
>
> +       /* fbdev */
> +       struct hibmc_fbdev *fbdev;
>         bool mm_inited;
>  };
>
> +#define to_hibmc_framebuffer(x) container_of(x, struct hibmc_framebuffer, fb)
> +
>  struct hibmc_bo {
>         struct ttm_buffer_object bo;
>         struct ttm_placement placement;
> @@ -65,8 +82,15 @@ static inline struct hibmc_bo *gem_to_hibmc_bo(struct drm_gem_object *gem)
>
>  #define DRM_FILE_PAGE_OFFSET (0x100000000ULL >> PAGE_SHIFT)
>
> +int hibmc_fbdev_init(struct hibmc_drm_device *hidev);
> +void hibmc_fbdev_fini(struct hibmc_drm_device *hidev);
> +
>  int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
>                      struct drm_gem_object **obj);
> +struct hibmc_framebuffer *
> +hibmc_framebuffer_init(struct drm_device *dev,
> +                      const struct drm_mode_fb_cmd2 *mode_cmd,
> +                      struct drm_gem_object *obj);
>
>  int hibmc_mm_init(struct hibmc_drm_device *hibmc);
>  void hibmc_mm_fini(struct hibmc_drm_device *hibmc);
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c
> new file mode 100644
> index 0000000..630124b
> --- /dev/null
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c
> @@ -0,0 +1,255 @@
> +/* Hisilicon Hibmc SoC drm driver
> + *
> + * Based on the bochs drm driver.
> + *
> + * Copyright (c) 2016 Huawei Limited.
> + *
> + * Author:
> + *     Rongrong Zou <zourongrong@huawei.com>
> + *     Rongrong Zou <zourongrong@gmail.com>
> + *     Jianhua Li <lijianhua@huawei.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + */
> +
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_fb_helper.h>
> +
> +#include "hibmc_drm_drv.h"
> +
> +/* ---------------------------------------------------------------------- */

Please remove

> +
> +static int hibmcfb_create_object(
> +                               struct hibmc_drm_device *hidev,
> +                               const struct drm_mode_fb_cmd2 *mode_cmd,
> +                               struct drm_gem_object **gobj_p)
> +{
> +       struct drm_gem_object *gobj;
> +       struct drm_device *dev = hidev->dev;
> +       u32 size;
> +       int ret = 0;
> +
> +       size = mode_cmd->pitches[0] * mode_cmd->height;
> +       ret = hibmc_gem_create(dev, size, true, &gobj);
> +       if (ret)
> +               return ret;
> +
> +       *gobj_p = gobj;
> +       return ret;
> +}
> +
> +static struct fb_ops hibmc_drm_fb_ops = {
> +       .owner = THIS_MODULE,
> +       .fb_check_var = drm_fb_helper_check_var,
> +       .fb_set_par = drm_fb_helper_set_par,
> +       .fb_fillrect = drm_fb_helper_sys_fillrect,
> +       .fb_copyarea = drm_fb_helper_sys_copyarea,
> +       .fb_imageblit = drm_fb_helper_sys_imageblit,
> +       .fb_pan_display = drm_fb_helper_pan_display,
> +       .fb_blank = drm_fb_helper_blank,
> +       .fb_setcmap = drm_fb_helper_setcmap,
> +};
> +
> +static int hibmc_drm_fb_create(struct drm_fb_helper *helper,
> +                              struct drm_fb_helper_surface_size *sizes)
> +{
> +       struct hibmc_fbdev *hi_fbdev =
> +               container_of(helper, struct hibmc_fbdev, helper);
> +       struct hibmc_drm_device *hidev =
> +               (struct hibmc_drm_device *)helper->dev->dev_private;
> +       struct fb_info *info;
> +       struct hibmc_framebuffer *hibmc_fb;
> +       struct drm_framebuffer *fb;
> +       struct drm_mode_fb_cmd2 mode_cmd;
> +       struct drm_gem_object *gobj = NULL;
> +       int ret = 0;
> +       size_t size;
> +       unsigned int bytes_per_pixel;
> +       struct hibmc_bo *bo = NULL;
> +
> +       DRM_DEBUG_DRIVER("surface width(%d), height(%d) and bpp(%d)\n",
> +                        sizes->surface_width, sizes->surface_height,
> +                        sizes->surface_bpp);
> +       sizes->surface_depth = 32;
> +
> +       bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8);
> +
> +       mode_cmd.width = sizes->surface_width;
> +       mode_cmd.height = sizes->surface_height;
> +       mode_cmd.pitches[0] = mode_cmd.width * bytes_per_pixel;
> +       mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> +                                                         sizes->surface_depth);
> +
> +       size = roundup(mode_cmd.pitches[0] * mode_cmd.height, PAGE_SIZE);

It's somewhat curious that you used ALIGN in the previous patch and
roundup here. But anyways, I think PAGE_ALIGN would be the most
appropriate thing to do here.

> +
> +       ret = hibmcfb_create_object(hidev, &mode_cmd, &gobj);
> +       if (ret) {
> +               DRM_ERROR("failed to create fbcon backing object %d\r\n",
ret);

\r, yikes!!!

> +               return -ENOMEM;
> +       }
> +
> +       bo = gem_to_hibmc_bo(gobj);
> +
> +       ret = ttm_bo_reserve(&bo->bo, true, false, NULL);
> +       if (ret)

Print error here?

How about cleaning up gobj?

> +               return ret;
> +
> +       ret = hibmc_bo_pin(bo, TTM_PL_FLAG_VRAM, NULL);
> +       if (ret) {
> +               DRM_ERROR("failed to pin fbcon\n");

Print ret

ttm_bo_unreserve? It seems like you're missing clean-up in all of the
error paths in this function. Can you please make sure everything is
tidied up?

> +               return ret;
> +       }
> +
> +       ret = ttm_bo_kmap(&bo->bo, 0, bo->bo.num_pages, &bo->kmap);
> +

nit: extra space

> +       if (ret) {
> +               DRM_ERROR("failed to kmap fbcon\n");

Print ret

> +               ttm_bo_unreserve(&bo->bo);
> +               return ret;
> +       }
> +
> +       ttm_bo_unreserve(&bo->bo);

Move this between ttm_bo_kmap and if (ret), then remove it from inside
the conditional.

> +
> +       info = drm_fb_helper_alloc_fbi(helper);
> +       if (IS_ERR(info))

Print error

> +               return PTR_ERR(info);
> +
> +       info->par = hi_fbdev;
> +
> +       hibmc_fb = hibmc_framebuffer_init(hidev->dev, &mode_cmd, gobj);
> +       if (IS_ERR(hibmc_fb)) {
> +               drm_fb_helper_release_fbi(helper);
> +               return PTR_ERR(hibmc_fb);
> +       }
> +
> +       hi_fbdev->fb = hibmc_fb;
> +       hidev->fbdev->size = size;
> +       fb = &hibmc_fb->fb;

The fb variable isn't necessary, and really, the hibmc_fb doesn't
really help either, it just masks what's actually happening IMO.

> +       if (!fb) {
> +               DRM_INFO("fb is NULL\n");
> +               return -EINVAL;
> +       }
> +
> +       hi_fbdev->helper.fb = fb;
> +
> +       strcpy(info->fix.id, "hibmcdrmfb");
> +
> +       info->flags = FBINFO_DEFAULT;
> +       info->fbops = &hibmc_drm_fb_ops;
> +
> +       drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
> +       drm_fb_helper_fill_var(info, &hidev->fbdev->helper, sizes->fb_width,
> +                              sizes->fb_height);
> +
> +       info->screen_base = bo->kmap.virtual;
> +       info->screen_size = size;
> +
> +       info->fix.smem_start = bo->bo.mem.bus.offset + bo->bo.mem.bus.base;
> +       info->fix.smem_len = size;
> +
> +       return 0;
> +}
> +
> +static void hibmc_fbdev_destroy(struct hibmc_fbdev *fbdev)
> +{
> +       struct hibmc_framebuffer *gfb = fbdev->fb;
> +       struct drm_fb_helper *fbh = &fbdev->helper;
> +
> +       DRM_DEBUG_DRIVER("hibmc_fbdev_destroy\n");

Not useful

> +
> +       drm_fb_helper_unregister_fbi(fbh);
> +       drm_fb_helper_release_fbi(fbh);
> +
> +       drm_fb_helper_fini(fbh);
> +
> +       if (gfb)
> +               drm_framebuffer_unreference(&gfb->fb);
> +
> +       kfree(fbdev);
> +}
> +
> +static const struct drm_fb_helper_funcs hibmc_fbdev_helper_funcs = {
> +       .fb_probe = hibmc_drm_fb_create,
> +};
> +
> +int hibmc_fbdev_init(struct hibmc_drm_device *hidev)
> +{
> +       int ret;
> +       struct fb_var_screeninfo *var;
> +       struct fb_fix_screeninfo *fix;
> +       struct hibmc_fbdev *hifbdev;
> +
> +       hifbdev = kzalloc(sizeof(*hifbdev), GFP_KERNEL);

devm_kzalloc?

> +       if (!hifbdev)
> +               return -ENOMEM;
> +
> +       hidev->fbdev = hifbdev;
> +       drm_fb_helper_prepare(hidev->dev, &hifbdev->helper,
> +                             &hibmc_fbdev_helper_funcs);
> +
> +       /* Now just one crtc and one channel */
> +       ret = drm_fb_helper_init(hidev->dev,
> +                                &hifbdev->helper, 1, 1);
> +

nit: extra line

> +       if (ret)
> +               return ret;
> +
> +       ret = drm_fb_helper_single_add_all_connectors(&hifbdev->helper);
> +       if (ret)
> +               goto fini;
> +
> +       ret = drm_fb_helper_initial_config(&hifbdev->helper, 16);
> +       if (ret)
> +               goto fini;
> +
> +       var = &hifbdev->helper.fbdev->var;
> +       fix = &hifbdev->helper.fbdev->fix;
> +
> +       DRM_DEBUG("Member of info->var is :\n"
> +                "xres=%d\n"
> +                "yres=%d\n"
> +                "xres_virtual=%d\n"
> +                "yres_virtual=%d\n"
> +                "xoffset=%d\n"
> +                "yoffset=%d\n"
> +                "bits_per_pixel=%d\n"
> +                "...\n", var->xres, var->yres, var->xres_virtual,
> +                var->yres_virtual, var->xoffset, var->yoffset,
> +                var->bits_per_pixel);
> +       DRM_DEBUG("Member of info->fix is :\n"
> +                "smem_start=%lx\n"
> +                "smem_len=%d\n"
> +                "type=%d\n"
> +                "type_aux=%d\n"
> +                "visual=%d\n"
> +                "xpanstep=%d\n"
> +                "ypanstep=%d\n"
> +                "ywrapstep=%d\n"
> +                "line_length=%d\n"
> +                "accel=%d\n"
> +                "capabilities=%d\n"
> +                "...\n", fix->smem_start, fix->smem_len, fix->type,
> +                fix->type_aux, fix->visual, fix->xpanstep,
> +                fix->ypanstep, fix->ywrapstep, fix->line_length,
> +                fix->accel, fix->capabilities);

You've been using DRM_DEBUG_DRIVER elsewhere, you should probably use
it here, too.

> +
> +       return 0;
> +
> +fini:
> +       drm_fb_helper_fini(&hifbdev->helper);
> +       return ret;
> +}
> +
> +void hibmc_fbdev_fini(struct hibmc_drm_device *hidev)
> +{
> +       if (!hidev->fbdev)
> +               return;
> +
> +       hibmc_fbdev_destroy(hidev->fbdev);
> +       hidev->fbdev = NULL;
> +}
> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> index 0802ebd..9822f62 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
> @@ -488,3 +488,69 @@ int hibmc_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev,
>         drm_gem_object_unreference_unlocked(obj);
>         return 0;
>  }
> +
> +/* ---------------------------------------------------------------------- */
> +

Please remove

> +static void hibmc_user_framebuffer_destroy(struct drm_framebuffer *fb)
> +{
> +       struct hibmc_framebuffer *hibmc_fb = to_hibmc_framebuffer(fb);
> +
> +       drm_gem_object_unreference_unlocked(hibmc_fb->obj);
> +       drm_framebuffer_cleanup(fb);
> +       kfree(hibmc_fb);
> +}
> +
> +static const struct drm_framebuffer_funcs hibmc_fb_funcs = {
> +       .destroy = hibmc_user_framebuffer_destroy,
> +};
> +
> +struct hibmc_framebuffer *
> +hibmc_framebuffer_init(struct drm_device *dev,
> +                      const struct drm_mode_fb_cmd2 *mode_cmd,
> +                      struct drm_gem_object *obj)
> +{
> +       struct hibmc_framebuffer *hibmc_fb;
> +       int ret;
> +
> +       hibmc_fb = kzalloc(sizeof(*hibmc_fb), GFP_KERNEL);
> +       if (!hibmc_fb)

Print error

> +               return ERR_PTR(-ENOMEM);
> +
> +       drm_helper_mode_fill_fb_struct(&hibmc_fb->fb, mode_cmd);
> +       hibmc_fb->obj = obj;
> +       ret = drm_framebuffer_init(dev, &hibmc_fb->fb, &hibmc_fb_funcs);
> +       if (ret) {
> +               DRM_ERROR("drm_framebuffer_init failed: %d\n", ret);
> +               kfree(hibmc_fb);
> +               return ERR_PTR(ret);
> +       }
> +
> +       return hibmc_fb;
> +}
> +
> +static struct drm_framebuffer *
> +hibmc_user_framebuffer_create(struct drm_device *dev,
> +                             struct drm_file *filp,
> +                             const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> +       struct drm_gem_object *obj;
> +       struct hibmc_framebuffer *hibmc_fb;
> +
> +       DRM_DEBUG_DRIVER("%dx%d, format %c%c%c%c\n",
> +                        mode_cmd->width, mode_cmd->height,
> +                        (mode_cmd->pixel_format) & 0xff,
> +                        (mode_cmd->pixel_format >> 8)  & 0xff,
> +                        (mode_cmd->pixel_format >> 16) & 0xff,
> +                        (mode_cmd->pixel_format >> 24) & 0xff);
> +
> +       obj = drm_gem_object_lookup(filp, mode_cmd->handles[0]);
> +       if (!obj)
> +               return ERR_PTR(-ENOENT);
> +
> +       hibmc_fb = hibmc_framebuffer_init(dev, mode_cmd, obj);
> +       if (IS_ERR(hibmc_fb)) {
> +               drm_gem_object_unreference_unlocked(obj);
> +               return ERR_PTR((long)hibmc_fb);
> +       }
> +       return &hibmc_fb->fb;
> +}
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* [RESEND PATCH v1 05/11] dt-bindings: perf: hisi: Add Devicetree bindings for Hisilicon SoC PMU
From: Mark Rutland @ 2016-11-10 18:30 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1478151727-20250-6-git-send-email-anurup.m@huawei.com>

Hi,

On Thu, Nov 03, 2016 at 01:42:01AM -0400, Anurup M wrote:
> 	1) Device tree bindings for Hisilicon SoC PMU.
> 	2) Add example for Hisilicon L3 cache, MN and DDRC PMU.
> 
> Signed-off-by: Anurup M <anurup.m@huawei.com>
> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> ---
>  .../devicetree/bindings/arm/hisilicon/pmu.txt      | 127 +++++++++++++++++++++
>  1 file changed, 127 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/hisilicon/pmu.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/hisilicon/pmu.txt b/Documentation/devicetree/bindings/arm/hisilicon/pmu.txt
> new file mode 100644
> index 0000000..e7b35e0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/hisilicon/pmu.txt
> @@ -0,0 +1,127 @@
> +Hisilicon SoC hip05/06/07 ARMv8 PMU
> +===================================
> +
> +The Hisilicon SoC chips like hip05/06/07 etc. consist of varous independent
> +system device PMU's such as L3 cache (L3C), Miscellaneous Nodes(MN) and DDR

s/PMU's/PMUs/

> +comtroller. These PMU devices are independent and have hardware logic to

s/comtroller/controller/

> +gather statistics and performance information.
> +
> +HiSilicon SoC chip is encapsulated by multiple CPU and IO die's. The CPU die

s/die's/dies/

> +is called as Super CPU cluster (SCCL) which includes 16 cpu-cores. Every SCCL
> +is further grouped as CPU clusters (CCL) which includes 4 cpu-cores each.
> +e.g. In the case of hip05/06/07, each SCCL has 1 L3 cache and 1 MN PMU device.
> +
> +The Hisilicon SoC PMU DT node bindigs for uncore PMU devices are as below.

s/bindigs/bindings/

> +For PMU devices like L3 cache. MN etc. which are accessed using the djtag,
> +the parent node will be the djtag node of the corresponding CPU die(SCCL).
> +
> +For uncore PMU devices there are some common required properties as detailed
> +below.
> +
> +Required properties:
> +	- compatible : This field contain two values. The first value is
> +		always "hisilicon" and second value is the Module type as shown
> +		in below examples:

Just say:

 - Compatible: should contain one of:

> +		(a) "hisilicon,hisi-pmu-l3c-v1" for Hisilicon SoC L3C PMU
> +			device (Version 1)
> +		(b) "hisilicon,hisi-pmu-mn-v1" for Hisilicon SoC MN PMU
> +			device (Version 1)
> +		(c) "hisilicon,hisi-pmu-ddrc-v1" for Hisilicon SoC DDRC PMU
> +			device (Version 1)
> +		The hip05/06/07 chips have v1 hardware for L3C, MN and DDRC.
> +
> +	- scl-id : The Super Cluster ID. This can be the ID of the CPU die
> +		   or IO die in the chip.

What's this needed for?

> +	- num-events : No of events supported by this PMU device.
> +
> +	- num-counters : No of hardware counters available for counting.

This isn't probeable or well-known?

> +
> +L3 cache
> +--------
> +The L3 cache is dedicated for each SCCL and hence there are separate DT nodes
> +for L3 cache for each SCCL. For L3 cache PMU the additional required properties
> +are
> +	- counter-reg : Counter register offset.
> +
> +	- evtype-reg : Event select register offset.
> +
> +	- evctrl-reg : Event counting control(LAUCTRL) register offset.

Surely for a given revision of the chip these offsets are known? i.e.
surely the compatible string implies specific offsets?

> +	- event-en : Event enable value.

Huh?

> +	- module-id : Module ID to input for djtag. This property is an array of
> +		      module_id for each L3 cache banks.
> +
> +	- num-banks : Number of banks or instances of the device.

What's a bank? Surely they have separate instances of the PMU?

What order are these in?

> +	- cfgen-map : Config enable array to select the bank.

Huh?

> +Miscellaneous Node
> +-------------------
> +The MN is dedicated for each SCCL and hence there are separate DT nodes for MN
> +for each SCCL. For MN PMU the additional required properties are
> +	- counter-reg : Counter register offset.
> +
> +	- evtype-reg : Event select register offset.
> +
> +	- evctrl-reg : Event counting control register offset.

Likewise, surely this is well-known for a given revision of the chip?

> +
> +	- module-id : Module ID to input for djtag. As MN doesnot have multiple banks
> +		      this property is a single value.
> +
> +	- cfgen-map : Config enable to select the bank. For MN it is a single value
> +
> +	- event-en : Event enable value.

Same comments as for the L3 cache nodes


[...]

> +DDR controller
> +--------------
> +Each SCCL in Hip05/06/07 chips have 2 DDR channels and hence 2 DDR controllers.
> +There are separate DT nodes for each DDR channel.
> +For DDRC PMU the additional required properties are
> +
> +	- ch-id : DDRC Channel ID.

Why is this necessary?

Thanks,
Mark.

> +	- reg : Register base address and range for the DDRC channel.
> +
> +Example:
> +	/* DDRC for CPU die scl #2 Channel #1 for hip05 */
> +	pmu_sccl0_ddrc1: pmu_ddrc1 at 80358000 {
> +		 compatible = "hisilicon,hisi-pmu-ddrc-v1";
> +		 scl-id = <0x02>;
> +		 ch-id = <0x1>;
> +		 num-events = <0x0D>;
> +		 num-counters = <0x04>;
> +		 reg = <0x80358000 0x10000>; /* TOTEMC DDRC1 */
> +	 };
> -- 
> 2.1.4
> 

^ permalink raw reply

* [PATCH] PCI: enable extended tags support for PCIe endpoints
From: Sinan Kaya @ 2016-11-10 18:35 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1474769434-5756-1-git-send-email-okaya@codeaurora.org>


On 9/24/2016 10:10 PM, Sinan Kaya wrote:
> Each PCIe device can issue up to 32 transactions at a time by default.
> Each transaction is tracked by a tag number on the bus. 32 outstanding
> transactions is not enough for some performance critical applications
> especially when a lot of small sized frames are transmitted.
> 
> Extended tags support increases this number to 256. Devices not
> supporting extended tags tie-off this field to 0. According to ECN, it
> is safe to enable this feature for all PCIe endpoints.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/pci/probe.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 93f280d..2424f38 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1505,12 +1505,19 @@ static void program_hpp_type2(struct pci_dev *dev, struct hpp_type2 *hpp)
>  	 */
>  }
>  
> +static int pci_configure_extended_tags(struct pci_dev *dev)
> +{

I should have checked the capability here before trying to enable it. 
I'll post a follow up patch on this. 

Is there any other feedback?


> +	return pcie_capability_set_word(dev, PCI_EXP_DEVCTL,
> +					 PCI_EXP_DEVCTL_EXT_TAG);
> +}
> +
>  static void pci_configure_device(struct pci_dev *dev)
>  {
>  	struct hotplug_params hpp;
>  	int ret;
>  
>  	pci_configure_mps(dev);
> +	pci_configure_extended_tags(dev);
>  
>  	memset(&hpp, 0, sizeof(hpp));
>  	ret = pci_get_hp_params(dev, &hpp);
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

^ permalink raw reply

* [PATCH v2 1/2] iommu/dma: Implement dma_{map,unmap}_resource()
From: Catalin Marinas @ 2016-11-10 18:37 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1ae5dc0f-2e59-3e10-a61a-1f5452ad560f@arm.com>

On Thu, Nov 10, 2016 at 02:30:04PM +0000, Robin Murphy wrote:
> On 10/11/16 12:24, Joerg Roedel wrote:
> > On Wed, Oct 26, 2016 at 06:43:56PM +0100, Robin Murphy wrote:
> >> With the new dma_{map,unmap}_resource() functions added to the DMA API
> >> for the benefit of cases like slave DMA, add suitable implementations to
> >> the arsenal of our generic layer. Since cache maintenance should not be
> >> a concern, these can both be standalone callback implementations without
> >> the need for arch code wrappers.
> >>
> >> CC: Joerg Roedel <joro@8bytes.org>
> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> >> ---
> >>
> >> v2: Use iommu_dma_unmap_page for symmetry, instead of being sneaky.
> >>
> >>  drivers/iommu/dma-iommu.c | 13 +++++++++++++
> >>  include/linux/dma-iommu.h |  4 ++++
> >>  2 files changed, 17 insertions(+)
> >>
> >> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> >> index c5ab8667e6f2..a2fd90a6a782 100644
> >> --- a/drivers/iommu/dma-iommu.c
> >> +++ b/drivers/iommu/dma-iommu.c
> >> @@ -624,6 +624,19 @@ void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
> >>  	__iommu_dma_unmap(iommu_get_domain_for_dev(dev), sg_dma_address(sg));
> >>  }
> >>  
> >> +dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
> >> +		size_t size, enum dma_data_direction dir, unsigned long attrs)
> >> +{
> >> +	return iommu_dma_map_page(dev, phys_to_page(phys), offset_in_page(phys),
> >> +			size, dma_direction_to_prot(dir, false) | IOMMU_MMIO);
> >> +}
> > 
> > Isn't the whole point of map_resource that we can map regions which have
> > no 'struct page' associated? The use phys_to_page() will certainly not
> > do the right thing here.
> 
> Perhaps it's a bit cheeky, but the struct page pointer is never
> dereferenced - the only thing iommu_dma_map_page() does with it is
> immediately convert it back again. Is there ever a case where
> page_to_phys(phys_to_page(phys)) != phys ?

Without SPARSEMEM_VMEMMAP or FLATMEM, it wouldn't work. For example,
__page_to_pfn() in the SPARSEMEM case, used by page_to_phys(), even
accesses the page structure (through page_to_section()).

If the page struct is not relevant to the iommu code in question, you
could factor it out into an __iommu_dma_map_pfn(). Or maybe move the
whole logic to iommu_dma_map_resource() and call it from
iommu_dma_map_page() with the page_to_phys(page) argument.

-- 
Catalin

^ permalink raw reply

* [PATCH V2 5/5] PCI: handle CRS returned by device after FLR
From: Sinan Kaya @ 2016-11-10 18:38 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1474056395-21843-6-git-send-email-okaya@codeaurora.org>

On 9/16/2016 4:06 PM, Sinan Kaya wrote:
> An endpoint is allowed to issue CRS following an FLR request to indicate
> that it is not ready to accept new requests. Changing the polling mechanism
> in FLR wait function to go read the vendor ID instead of the command/status
> register. A CRS indication will only be given if the address to be read is
> vendor ID.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/pci/pci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e913467..1def11e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3729,7 +3729,8 @@ static void pci_flr_wait(struct pci_dev *dev)
>  
>  	do {
>  		msleep(100);
> -		pci_read_config_dword(dev, PCI_COMMAND, &id);
> +		pci_bus_read_dev_vendor_id(dev->bus, dev->devfn, &id,
> +					   60 * 1000);

We found out during Mellanox CX3 testing that the card returns the vendor
ID earlier during FLR while other cards are OK with this implementaiton. 

I think we need both of the reads to support the CRS for the endpoints
and also the command id register read to make sure endpoint init is complete.


>  	} while (i++ < 10 && id == ~0);
>  
>  	if (id == ~0)
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

^ permalink raw reply

* [PATCH V3 1/2] PCI: add CRS support to error handling path
From: Sinan Kaya @ 2016-11-10 18:39 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1475473021-14251-2-git-send-email-okaya@codeaurora.org>

On 10/3/2016 1:36 AM, Sinan Kaya wrote:
> The PCIE spec allows an endpoint device to extend the initialization time
> beyond 1 second by issuing Configuration Request Retry Status (CRS) for a
> vendor ID read request.
> 
> This basically means "I'm busy now, please call me back later".
> 
> There are two moving parts to CRS support from the SW perspective. One part
> is to determine if CRS is supported or not. The second part is to set the
> CRS visibility register.
> 
> As part of the probe, the Linux kernel sets the above two conditions in
> pci_enable_crs function. The kernel is also honoring the returned CRS in
> pci_bus_read_dev_vendor_id function if supported. The function will poll up
> to specified amount of time while endpoint is returning CRS response.
> 
> The PCIe spec also allows CRS to be issued during cold, warm, hot and FLR
> resets.
> 
> The hot reset is initiated by starting a secondary bus reset. A bus/device
> restore follows the reset.  This patch is adding vendor ID read into dev
> restore function to validate that the device is accessible before writing
> the register contents. If the device issues CRS, the code might poll up
> to 60 seconds.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/pci/pci.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index aab9d51..c8749b9 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4020,6 +4020,12 @@ static void pci_dev_save_and_disable(struct pci_dev *dev)
>  
>  static void pci_dev_restore(struct pci_dev *dev)
>  {
> +	u32 l;
> +
> +	/* see if the device is accessible first */
> +	if (!pci_bus_read_dev_vendor_id(dev->bus, dev->devfn, &l, 60 * 1000))
> +		return;
> +
>  	pci_restore_state(dev);
>  	pci_reset_notify(dev, false);
>  }
> 

Any feedback on this direction?


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

^ permalink raw reply

* [PATCH 0/8] DMA: s3c64xx: Conversion to the new channel request API
From: Krzysztof Kozlowski @ 2016-11-10 18:42 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1478791076-19528-1-git-send-email-s.nawrocki@samsung.com>

On Thu, Nov 10, 2016 at 04:17:48PM +0100, Sylwester Nawrocki wrote:
> This patch series aims to convert the s3c64xx platform to use
> the new DMA channel request API, i.e. this is only meaningful 
> for non-dt systems using s3c64xx SoCs.
> 
> Presumably the first 2 or 4 patches in this series could be queued 
> for v4.10-rc1 and the remaining patches could be left for subsequent
> release, to avoid non-trivial conflict with patches already applied 
> in the ASoC tree.

Sounds good to me. Should I put the ARM/s3c64xx commits on separate
branch in case someone would like to pull a tag with it?

Best regards,
Krzysztof

^ permalink raw reply

* [PATCH 13/13] Documentation: synopsys-dw-mshc: remove the unused properties
From: Rob Herring @ 2016-11-10 18:47 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161103062135.10697-14-jh80.chung@samsung.com>

On Thu, Nov 03, 2016 at 03:21:35PM +0900, Jaehoon Chung wrote:
> "support-highspeed" was the obsoleted property.
> And "broken-cd" is not synopsys specific property.
> It can be referred to mmc.txt binding Documentation.
> 
> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
> ---
>  Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt | 5 -----
>  1 file changed, 5 deletions(-)

Acked-by: Rob Herring <robh@kernel.org>

^ permalink raw reply

* [PATCH 1/6] dt-bindings: rockchip-dw-mshc: add RK1108 dw-mshc description
From: Rob Herring @ 2016-11-10 18:56 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1478176250-11840-1-git-send-email-andy.yan@rock-chips.com>

On Thu, Nov 03, 2016 at 08:30:50PM +0800, Andy Yan wrote:
> From: Shawn Lin <shawn.lin@rock-chips.com>
> 
> Add "rockchip,rk1108-dw-mshc", "rockchip,rk3288-dw-mshc" for
> dwmmc on rk1108 platform.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> ---
> 
>  Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt | 1 +
>  1 file changed, 1 insertion(+)

Acked-by: Rob Herring <robh@kernel.org>

^ permalink raw reply

* [PATCH 6/6] ARM: dts: rockchip: add rockchip RK1108 Evaluation board
From: Rob Herring @ 2016-11-10 18:57 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1478177039-12257-1-git-send-email-andy.yan@rock-chips.com>

On Thu, Nov 03, 2016 at 08:43:59PM +0800, Andy Yan wrote:
> RK1108EVB is designed by Rockchip for CVR field.
> This patch add basic support for it, which can boot with
> initramfs into shell.
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> ---
> 
>  Documentation/devicetree/bindings/arm/rockchip.txt |  3 +
>  arch/arm/boot/dts/Makefile                         |  1 +
>  arch/arm/boot/dts/rk1108-evb.dts                   | 69 ++++++++++++++++++++++
>  3 files changed, 73 insertions(+)
>  create mode 100644 arch/arm/boot/dts/rk1108-evb.dts

Acked-by: Rob Herring <robh@kernel.org>

^ permalink raw reply

* [PATCH v5 6/8] Documentation: bindings: add compatible specific to legacy SCPI protocol
From: Olof Johansson @ 2016-11-10 19:03 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <7ccc12bc-9a05-47e3-8ab8-d1b0ad31159e@arm.com>

On Thu, Nov 10, 2016 at 6:34 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 10/11/16 14:12, Rob Herring wrote:
>>
>> On Thu, Nov 10, 2016 at 4:26 AM, Sudeep Holla <sudeep.holla@arm.com>
>> wrote:
>>>
>>>
>>>
>>> On 10/11/16 01:22, Rob Herring wrote:
>>>>
>>>>
>>>> On Wed, Nov 02, 2016 at 10:52:09PM -0600, Sudeep Holla wrote:
>>>>>
>>>>>
>>>>> This patch adds specific compatible to support legacy SCPI protocol.
>>>>>
>>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/arm/arm,scpi.txt | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/arm/arm,scpi.txt
>>>>> b/Documentation/devicetree/bindings/arm/arm,scpi.txt
>>>>> index d1882c4540d0..ebd03fc93135 100644
>>>>> --- a/Documentation/devicetree/bindings/arm/arm,scpi.txt
>>>>> +++ b/Documentation/devicetree/bindings/arm/arm,scpi.txt
>>>>> @@ -7,7 +7,9 @@ by Linux to initiate various system control and power
>>>>> operations.
>>>>>
>>>>>  Required properties:
>>>>>
>>>>> -- compatible : should be "arm,scpi"
>>>>> +- compatible : should be
>>>>> +       * "arm,scpi" : For implementations complying to SCPI v1.0 or
>>>>> above
>>>>> +       * "arm,legacy-scpi" : For implementations complying pre SCPI
>>>>> v1.0
>>>>
>>>>
>>>>
>>>> I'd prefer that we explicitly enumerate the old versions. Are there
>>>> many?
>>>>
>>>
>>> I understand your concern, but this legacy SCPI protocol was not
>>> officially released. It was just WIP which vendors picked up from very
>>> early releases. Since they are not numbered, it's hard to have specific
>>> compatibles with different versions until v1.0. That's one of the reason
>>> to retain platform specific compatible so that we can add any quirks
>>> based on them if needed.
>>>
>>> I will probably add these information in the commit log so that it's
>>> clear why we can't do version based compatible.
>>
>>
>> This is exactly my point. By enumerate, I meant having platform
>> specific compatibles. Having "arm,legacy-scpi" is pointless because
>> who knows what version they followed and they may all be different.
>>
>
> OK, but IIUC Olof's concern wanted a generic one along with the platform
> specific compatible which kind of makes sense as so far we have seen
> some commonality between Amlogic and Rockchip.
>
> E.g. Amlogic follows most of the legacy protocol though it deviates in
> couple of things which we can handle with platform specific compatible
> (in the following patch in the series). When another user(Rockchip ?)
> make use of this legacy protocol, we can start using those platform
> specific compatible for deviations only.
>
> Is that not acceptable ?

If there's no shared legacy feature set, then it's probably less
useful to have a shared less precise compatible value.

What the main point I was trying to get across was that we shouldn't
expand the generic binding with per-vendor compatible fields, instead
we should have those as extensions on the side.

I'm also a little apprehensive of using "legacy", it goes in the same
bucket as "misc". At some point 1.0 will be legacy too, etc.


-Olof

^ permalink raw reply

* [RESEND PATCH v1 07/11] perf: hisi: Add support for Hisilicon SoC event counters
From: Mark Rutland @ 2016-11-10 19:10 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1478151727-20250-8-git-send-email-anurup.m@huawei.com>

On Thu, Nov 03, 2016 at 01:42:03AM -0400, Anurup M wrote:
> +	do {
> +		/* Get count from individual L3C banks and sum them up */
> +		for (i = 0; i < num_banks; i++) {
> +			total_raw_count += hisi_read_l3c_counter(l3c_hwmod_data,
> +									idx, i);
> +		}
> +		prev_raw_count = local64_read(&hwc->prev_count);
> +
> +		/*
> +		 * As prev_raw_count is updated with average value of
> +		 * L3 cache banks, we multiply it by no of banks and
> +		 * compute the delta
> +		 */
> +		delta = (total_raw_count - (prev_raw_count * num_banks)) &
> +								HISI_MAX_PERIOD;
> +
> +		local64_add(delta, &event->count);
> +
> +		/*
> +		 * Divide by num of banks to get average count and
> +		 * update prev_count with this value
> +		 */
> +		avg_raw_count = total_raw_count / num_banks;
> +	} while (local64_cmpxchg(
> +			 &hwc->prev_count, prev_raw_count, avg_raw_count) !=
> +							 prev_raw_count);

Please don't aggregate like this; expose separate PMUs instead.

This is racy, and by averaging and multiplying we're making up and/or
throwing away data.

[...]

> +	event_value = (val -
> +			HISI_HWEVENT_L3C_READ_ALLOCATE);
> +
> +	/* Select the appropriate Event select register */
> +	if (idx > 3)
> +		reg_offset += 4;
> +
> +	/* Value to write to event type register */
> +	val = event_value << (8 * idx);
> +

Please add helpers for these, and explain *why* the transformations are
necessary.

> +	/* Find the djtag Identifier of the Unit */
> +	client = l3c_hwmod_data->client;
> +
> +	/*
> +	 * Set the event in L3C_EVENT_TYPEx Register
> +	 * for all L3C banks
> +	 */

As above, it seems like you should expose a separate PMU per bank
instead. That applies for all the other instances where you iterate over
banks.

[...]

> +	for (i = 0; i < l3c_hwmod_data->l3c_hwcfg.num_banks; i++) {
> +		module_id = l3c_hwmod_data->l3c_hwcfg.module_id[i];
> +		cfg_en = l3c_hwmod_data->l3c_hwcfg.bank_cfgen[i];
> +		ret = hisi_djtag_writereg(module_id,
> +					cfg_en,
> +					reg_offset,
> +					value,
> +					client);
> +		if (!ret)
> +			ret = value;
> +	}

This is impossible to read. Please factor this into helpers such that
you don't need this amount of indentation.

Please do similarly elsewhere when you see this indentation pattern.

[...]

> +static int hisi_l3c_get_event_idx(struct hisi_pmu *pl3c_pmu)
> +{
> +	struct hisi_l3c_data *l3c_hwmod_data = pl3c_pmu->hwmod_data;
> +	int event_idx;
> +
> +	event_idx =
> +		find_first_zero_bit(
> +			l3c_hwmod_data->hisi_l3c_event_used_mask,
> +					 pl3c_pmu->num_counters);
> +
> +	if (event_idx == HISI_MAX_CFG_L3C_CNTR)
> +		return -EAGAIN;
> +
> +	__set_bit(event_idx,
> +		l3c_hwmod_data->hisi_l3c_event_used_mask);
> +
> +	return event_idx;
> +}

Please get rid of the weird hungarian notation (i.e. don't use 'p' as a
prefix for pointers), and use temporary variables consistently, e.g.

static int hisi_l3c_get_event_idx(struct hisi_pmu *l3c_pmu)
{
	struct hisi_l3c_data *l3c_hwmod_data = l3c_pmu->hwmod_data;
	unsigned long *used_mask = l3c_hwmod_data->hisi_l3c_event_used_mask;
	int num_counters = pl3c_pmu->num_counters
	int idx;

	idx = find_first_zero_bit(used_mask, num_counters);
	if (idx == num_counters)
		return -EAGAIN;

	set_bit(idx, used_mask);

	return idx;
}

[...]

> +	if (of_property_read_u32(node, "counter-reg",
> +				     &pl3c_hwcfg->counter_reg0_off)) {
> +		dev_err(dev, "DT:Couldnot read counter-reg!\n");
> +		return -EINVAL;
> +	}

Please use spaces in these messages.

Otherwise, my comments on the binding apply here.

[...]

> +static int init_hisi_l3c_data(struct device *dev,
> +					struct hisi_pmu *pl3c_pmu,
> +					struct hisi_djtag_client *client)
> +{
> +	struct hisi_l3c_data *l3c_hwmod_data = NULL;
> +	int ret;
> +
> +	l3c_hwmod_data = kzalloc(sizeof(struct hisi_l3c_data),
> +							GFP_KERNEL);

Use:

	l3c_hwmod_data = kzalloc(sizeof(*l3c_hwmod_data, GFP_KERNEL):

[...]

> +static int hisi_pmu_l3c_dev_probe(struct hisi_djtag_client *client)
> +{
> +	struct hisi_pmu *pl3c_pmu = NULL;
> +	struct device *dev = &client->dev;
> +	int ret;
> +
> +	pl3c_pmu = hisi_pmu_alloc(dev);
> +	if (IS_ERR(pl3c_pmu))
> +		return PTR_ERR(pl3c_pmu);

Why use error pointers for this?

hisi_pmu_alloc() only ever returns ERR_PTR(-ENOMEM) if it failed to
allocate.

It's far simpler to have it pass on NULL there, and here do:

	pl3c_pmu = hisi_pmu_alloc(dev);
	if (!pl3c_pmu)
		return -ENOMEM;

Please also s/pl3c_pmu/l3c_pmu/ here, and elsewhere throughout the
driver. The 'p' only serves to make this harder to read.

[...]

> +	/* Register with perf PMU */
> +	pl3c_pmu->pmu = (struct pmu) {
> +		.name = pl3c_pmu->name,
> +		.task_ctx_nr = perf_invalid_context,
> +		.event_init = hisi_uncore_pmu_event_init,
> +		.add = hisi_uncore_pmu_add,
> +		.del = hisi_uncore_pmu_del,
> +		.start = hisi_uncore_pmu_start,
> +		.stop = hisi_uncore_pmu_stop,
> +		.read = hisi_uncore_pmu_read,
> +	};

Please remove the comment above this.

[...]

> +int hisi_uncore_pmu_event_init(struct perf_event *event)
> +{
> +	int err;
> +	struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu);

This is undefined behaviour. This must be done *after* we check the
event->pmu->type.

> +
> +	if (event->attr.type != event->pmu->type)
> +		return -ENOENT;
> +
> +	/* we do not support sampling as the counters are all
> +	 * shared by all CPU cores in a CPU die(SCCL). Also we
> +	 * donot support attach to a task(per-process mode)
> +	 */
> +	if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK)
> +		return -EOPNOTSUPP;
> +
> +	/* counters do not have these bits */
> +	if (event->attr.exclude_user	||
> +	    event->attr.exclude_kernel	||
> +	    event->attr.exclude_host	||
> +	    event->attr.exclude_guest	||
> +	    event->attr.exclude_hv	||
> +	    event->attr.exclude_idle)
> +		return -EINVAL;
> +
> +	if (event->cpu < 0)
> +		return -EINVAL;
> +
> +	event->cpu = cpumask_first(&phisi_pmu->cpu);

You should also check the event grouping.

Take a look at what we do in arch/arm/mm/cache-l2x0-pmu.c.

[...]

> +/*
> + * Enable counter and set the counter to count
> + * the event that we're interested in.
> + */
> +void hisi_uncore_pmu_enable_event(struct perf_event *event)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +	struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu);
> +
> +	/* Disable the hardware event counting */
> +	if (phisi_pmu->ops->disable_counter)
> +		phisi_pmu->ops->disable_counter(phisi_pmu, GET_CNTR_IDX(hwc));

Why isn't the counter already disabled?

> +	/*
> +	 * Set event (if destined for Hisilicon SoC counters).
> +	 */
> +	if (phisi_pmu->ops->set_evtype)
> +		phisi_pmu->ops->set_evtype(phisi_pmu, GET_CNTR_IDX(hwc),
> +							hwc->config_base);

Why isn't this done in the pmu::event_add callback?

> +
> +	/* Enable the hardware event counting */
> +	if (phisi_pmu->ops->enable_counter)
> +		phisi_pmu->ops->enable_counter(phisi_pmu, GET_CNTR_IDX(hwc));

This should be the only necessary part of this function.

> +}
> +
> +void hisi_pmu_set_event_period(struct perf_event *event)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +	struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu);
> +
> +	/*
> +	 * The Hisilicon PMU counters have a period of 2^32. To account for the
> +	 * possiblity of extreme interrupt latency we program for a period of
> +	 * half that. Hopefully we can handle the interrupt before another 2^31
> +	 * events occur and the counter overtakes its previous value.
> +	 */
> +	u64 val = 1ULL << 31;
> +
> +	local64_set(&hwc->prev_count, val);
> +
> +	/* Write to the hardware event counter */
> +	phisi_pmu->ops->write_counter(phisi_pmu, hwc, val);
> +}
> +
> +void hisi_uncore_pmu_start(struct perf_event *event, int flags)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +	struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu);
> +	struct hisi_pmu_hw_events *hw_events;
> +
> +	hw_events = &phisi_pmu->hw_events;
> +
> +	if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED)))
> +		return;
> +
> +	WARN_ON_ONCE(!(hwc->state & PERF_HES_UPTODATE));
> +	hwc->state = 0;
> +
> +	if (phisi_pmu->ops->set_event_period)
> +		phisi_pmu->ops->set_event_period(event);

When will this differ from hisi_pmu_set_event_period() above?

> +	if (flags & PERF_EF_RELOAD) {
> +		u64 prev_raw_count =  local64_read(&hwc->prev_count);
> +
> +		phisi_pmu->ops->write_counter(phisi_pmu, hwc,
> +						(u32)prev_raw_count);
> +	}

If we always go through hisi_pmu_set_event_period(), this looks
redundant.

> +
> +	hisi_uncore_pmu_enable_event(event);

There's no matching disable_event() call in this function, so this looks
suspicious.

> +	perf_event_update_userpage(event);
> +}
> +
> +void hisi_uncore_pmu_stop(struct perf_event *event, int flags)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +	struct hisi_pmu *phisi_pmu = to_hisi_pmu(event->pmu);
> +
> +	if (hwc->state & PERF_HES_UPTODATE)
> +		return;

Why?

[...]

> +int hisi_uncore_common_fwprop_read(struct device *dev,
> +					struct hisi_pmu *phisi_pmu)
> +{
> +	if (device_property_read_u32(dev, "num-events",
> +					&phisi_pmu->num_events)) {
> +		dev_err(dev, "Cant read num-events from DT!\n");
> +		return -EINVAL;
> +	}

For consistency with the rest of the driver, and given there is no ACPI
support, please use the of_property_* API here.

Thanks,
Mark.

^ permalink raw reply

* [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced
From: Benjamin Herrenschmidt @ 2016-11-10 19:32 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161110112224.GB4418@leverpostej>

On Thu, 2016-11-10 at 11:22 +0000, Mark Rutland wrote:
> On POWER8, our PCIe doesn't do IO at all, but we have an LPC bus behind
> > firmware calls ;-) We use that infrastructure to plumb in the LPC bus.
> 
> Just to check, do you hook that in your inb/outb/etc?

Yes.

> Generally, it would seem nicer if we could have higher-level
> isa_{inb,outb,whatever} accessors that we could hook separately from
> other IO.

Maybe but generally speaking, we don't discriminate accessors per bus,
ie, readl etc... work on all memory mapped busses, inb... works on all
busses with an "IO space", at least that's been the idea. It probably
all comes from the fact that PCI IO and ISA are the same space on
x86 and most other platforms (not all).

> We don't necessarily have to move all ISA drivers over to that if we had
> a separate symbol for that interface.

What I do on ppc today is that I have a chunk of virtual address space
that is reserved for "IO space". The first 64k are "reserved" in that
they route to "the primary" ISA bus (for legacy crap that uses hard
coded addresses, though I use that for my LPC bus too). I "allocate"
space for the PCI IO spaces higher in that space. Was I to support more
LPC busses I could allocate them up there too.

The IO resource of a given device thus becomes the actual IO port plus
the offset of the base of the segment it's in.

For memory mapped IO, inb/outb will just add the virtual address of
the base of all IO space to that. The hooking mechanism will pickup
the stuff that isn't memory mapped.

It's a bit messy but then IO space performance has never been a huge
worry since IO cycles tend to be very slow to begin with.

Note: We also have the ISA memory and ISA FW spaces that we don't have
good accessors for. They somewhat exist (I think the fbdev layer uses
some for vga) but it's messy.

Cheers,
Ben.

^ permalink raw reply

* [PATCH v4 1/5] arm64: perf: Basic uncore counter support for Cavium ThunderX SOC
From: Will Deacon @ 2016-11-10 19:46 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161110165405.GH4418@leverpostej>

On Thu, Nov 10, 2016 at 04:54:06PM +0000, Mark Rutland wrote:
> On Sat, Oct 29, 2016 at 01:55:29PM +0200, Jan Glauber wrote:
> > diff --git a/drivers/perf/uncore/uncore_cavium.c b/drivers/perf/uncore/uncore_cavium.c
> > new file mode 100644
> > index 0000000..a7b4277
> > --- /dev/null
> > +++ b/drivers/perf/uncore/uncore_cavium.c
> > + * Some notes about the various counters supported by this "uncore" PMU
> > + * and the design:
> > + *
> > + * All counters are 64 bit long.
> > + * There are no overflow interrupts.
> > + * Counters are summarized per node/socket.
> > + * Most devices appear as separate PCI devices per socket with the exception
> > + * of OCX TLK which appears as one PCI device per socket and contains several
> > + * units with counters that are merged.
> 
> As a general note, as I commented on the QC L2 PMU driver [1,2], we need
> to figure out if we should be aggregating physical PMUs or not.
> 
> Judging by subsequent patches, each unit has individual counters and
> controls, and thus we cannot atomically read/write counters or controls
> across them. As such, I do not think we should aggregate them, and
> should expose them separately to userspace.

I thought each unit was registered as a separate PMU to perf? Or are you
specifically commenting on the OCX TLK? The comment there suggests that
the units cannot be individually enabled/disabled and, without docs, I
trust that's the case.

Will

^ permalink raw reply

* [PATCH v2 2/4] dt-bindings: Add TI SCI PM Domains
From: Dave Gerlach @ 2016-11-10 19:56 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <5811FE09.8000006@ti.com>

Rob, Ulf, Jon,
On 10/27/2016 08:15 AM, Dave Gerlach wrote:
> +Jon
> On 10/26/2016 04:59 PM, Rob Herring wrote:
>> On Mon, Oct 24, 2016 at 12:00 PM, Kevin Hilman <khilman@baylibre.com> wrote:
>>> Dave Gerlach <d-gerlach@ti.com> writes:
>>>
>>>> Hi,
>>>> On 10/21/2016 01:48 PM, Kevin Hilman wrote:
>>>>> Dave Gerlach <d-gerlach@ti.com> writes:
>>>>>
>>>>>> Add a generic power domain implementation, TI SCI PM Domains, that
>>>>>> will hook into the genpd framework and allow the TI SCI protocol to
>>>>>> control device power states.
>>>>>>
>>>>>> Also, provide macros representing each device index as understood
>>>>>> by TI SCI to be used in the device node power-domain references.
>>>>>> These are identifiers for the K2G devices managed by the PMMC.
>>>>>>
>>>>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>>>>> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
>>>>>> ---
>>>>>>  .../devicetree/bindings/soc/ti/sci-pm-domain.txt   | 54 +++++++++++++
>>>>>>  MAINTAINERS                                        |  2 +
>>>>>>  include/dt-bindings/genpd/k2g.h                    | 90 ++++++++++++++++++++++
>>>>>>  3 files changed, 146 insertions(+)
>>>>>>  create mode 100644 Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>>>  create mode 100644 include/dt-bindings/genpd/k2g.h
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt b/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>>> new file mode 100644
>>>>>> index 000000000000..32f38a349656
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
>>>>>> @@ -0,0 +1,54 @@
>>>>>> +Texas Instruments TI-SCI Generic Power Domain
>>>>>> +---------------------------------------------
>>>>>> +
>>>>>> +Some TI SoCs contain a system controller (like the PMMC, etc...) that is
>>>>>> +responsible for controlling the state of the IPs that are present.
>>>>>> +Communication between the host processor running an OS and the system
>>>>>> +controller happens through a protocol known as TI-SCI [1]. This pm domain
>>>>>> +implementation plugs into the generic pm domain framework and makes use of
>>>>>> +the TI SCI protocol power on and off each device when needed.
>>>>>> +
>>>>>> +[1] Documentation/devicetree/bindings/arm/keystone/ti,sci.txt
>>>>>> +
>>>>>> +PM Domain Node
>>>>>> +==============
>>>>>> +The PM domain node represents the global PM domain managed by the PMMC,
>>>>>> +which in this case is the single implementation as documented by the generic
>>>>>> +PM domain bindings in Documentation/devicetree/bindings/power/power_domain.txt.
>>>>>> +
>>>>>> +Required Properties:
>>>>>> +--------------------
>>>>>> +- compatible: should be "ti,sci-pm-domain"
>>>>>> +- #power-domain-cells: Must be 0.
>>>>>> +- ti,sci: Phandle to the TI SCI device to use for managing the devices.
>>>>>>
>>>>>> +Example:
>>>>>> +--------------------
>>>>>> +k2g_pds: k2g_pds {
>>>>>
>>>>> should use generic name like "power-contoller", e.g. k2g_pds: power-controller
>>>>
>>>> Ok, that makes more sense.
>>>>
>>>>>
>>>>>> +        compatible = "ti,sci-pm-domain";
>>>>>> +        #power-domain-cells = <0>;
>>>>>> +        ti,sci = <&pmmc>;
>>>>>> +};
>>>>>> +
>>>>>> +PM Domain Consumers
>>>>>> +===================
>>>>>> +Hardware blocks that require SCI control over their state must provide
>>>>>> +a reference to the sci-pm-domain they are part of and a unique device
>>>>>> +specific ID that identifies the device.
>>>>>> +
>>>>>> +Required Properties:
>>>>>> +--------------------
>>>>>> +- power-domains: phandle pointing to the corresponding PM domain node.
>>>>>> +- ti,sci-id: index representing the device id to be passed oevr SCI to
>>>>>> +        be used for device control.
>>>>>
>>>>> This ID doesn't look right.
>>>>>
>>>>> Why not use #power-domain-cells = <1> and pass the index in the DT? ...
>>
>> Exactly. ti,sci-id is a NAK for me.
>
> I was told not to use the onecell during v1 discussion. I agree this would be
> ideal but I cannot due to what the bindings represent, the phandle parameter is
> an index into a list of genpds, whereas we need an actual ID number we can use
> and I do not have the ability to get that from the phandle.
>
> @Ulf/Jon, is there any hope of bringing back custom xlate functions for genpd
> providers? I don't have a good background on why it was even removed. I can
> maintain a single genpd for all devices but I need a way to parse this ID,
> whether it's from a separate property or a phandle. It is locked now to indexing
> into a list of genpds but I need additional per device information for devices
> bound to a genpd and I need either a custom parameter or the ability to parse
> the phandle myself.
>

Any comments here? The meaning of the phandle onecell is fixed in the 
genpd framework so I'm not sure how we want to move forward with this, I 
need to pass a power domain ID to the genpd driver, and if this 
shouldn't be a new property I'm not sure what direction we should take.

Regards,
Dave

>>
>>>>>
>>>>>> +See dt-bindings/genpd/k2g.h for the list of valid identifiers for k2g.
>>>>>> +
>>>>>> +Example:
>>>>>> +--------------------
>>>>>> +uart0: serial at 02530c00 {
>>>>>> +   compatible = "ns16550a";
>>>>>> +   ...
>>>>>> +   power-domains = <&k2g_pds>;
>>>>>> +   ti,sci-id = <K2G_DEV_UART0>;
>>>>>
>>>>> ... like this:
>>>>>
>>>>>      power-domains = <&k2g_pds K2G_DEV_UART0>;
>>>>
>>>> That's how I did it in version one actually. I was able to define my
>>>> own xlate function to parse the phandle and get that index, but Ulf
>>>> pointed me to this series by Jon Hunter [1] that simplified genpd
>>>> providers and dropped the concept of adding your own xlate. This locks
>>>> the onecell approach to using a fixed static array of genpds that get
>>>> indexed into (without passing the index to the provider, just the
>>>> genpd that's looked up), which doesn't fit our usecase, as we don't
>>>> want a 1 to 1 genpd to device mapping based on the comments provided
>>>> in v1. Now we just use the genpd device attach/detach hooks to parse
>>>> the sci-id and then use it in the genpd device start/stop hooks.
>>
>> I have no idea what any of this means. All sounds like driver
>> architecture, not anything to do with bindings.
>
> This was a response to Kevin, not part of binding description.
>
>>
>>>
>>> Ah, right.  I remember now.  This approach allows you to use a single
>>> genpd as discussed earlier.
>>>
>>> Makes sense now, suggestion retracted.
>>
>> IIRC, the bindings in Jon's case had a node for each domain and didn't
>> need any additional property.
>
> Yes but we only have one domain and index into it, not into a list of domains,
> so the additional property is solving a different problem.
>
> Regards,
> Dave
>
>>
>> Rob
>>
>

^ permalink raw reply

* [PATCH] PM / Domains: Fix compatible for domain idle state
From: Rob Herring @ 2016-11-10 19:58 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CAPDyKFoCjf1qSBDWUY_wNx21_78fRrFqcqrFbsSmabzAZJxQAQ@mail.gmail.com>

On Mon, Nov 07, 2016 at 12:14:28PM +0100, Ulf Hansson wrote:
> On 3 November 2016 at 22:54, Lina Iyer <lina.iyer@linaro.org> wrote:
> > Re-using idle state definition provided by arm,idle-state for domain
> > idle states creates a lot of confusion and limits further evolution of
> > the domain idle definition. To keep things clear and simple, define a
> > idle states for domain using a new compatible "domain-idle-state".
> >
> > Fix existing PM domains code to look for the newly defined compatible.
> >
> > Cc: <devicetree@vger.kernel.org>
> > Cc: Rob Herring <robh@kernel.org>
> > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > ---
> >  .../bindings/power/domain-idle-state.txt           | 33 ++++++++++++++++++++++
> >  .../devicetree/bindings/power/power_domain.txt     |  8 +++---
> >  drivers/base/power/domain.c                        |  2 +-
> >  3 files changed, 38 insertions(+), 5 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/power/domain-idle-state.txt
> >
> > diff --git a/Documentation/devicetree/bindings/power/domain-idle-state.txt b/Documentation/devicetree/bindings/power/domain-idle-state.txt
> > new file mode 100644
> > index 0000000..eefc7ed
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/domain-idle-state.txt
> > @@ -0,0 +1,33 @@
> > +PM Domain Idle State Node:
> > +
> > +A domain idle state node represents the state parameters that will be used to
> > +select the state when there are no active components in the domain.
> > +
> > +The state node has the following parameters -
> > +
> > +- compatible:
> > +       Usage: Required
> > +       Value type: <string>
> > +       Definition: Must be "domain-idle-state".
> > +
> > +- entry-latency-us
> > +       Usage: Required
> > +       Value type: <prop-encoded-array>
> > +       Definition: u32 value representing worst case latency in
> > +                   microseconds required to enter the idle state.
> > +                   The exit-latency-us duration may be guaranteed
> > +                   only after entry-latency-us has passed.
> 
> As we anyway are going to change this, why not use an u64 and have the
> value in ns instead of us?

I can't imagine that you would need more resolution or range. For times 
less than 1us, s/w and register access times are going to dominate the 
time.

Unless there is a real need, I'd keep alignment with the existing 
binding.

Rob

^ permalink raw reply

* PM regression with LED changes in next-20161109
From: Pavel Machek @ 2016-11-10 20:29 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20161110175537.GF27724@atomide.com>

On Thu 2016-11-10 10:55:37, Tony Lindgren wrote:
> * Pavel Machek <pavel@ucw.cz> [161110 09:29]:
> > Hi!
> > 
> > > >>>Looks like commit 883d32ce3385 ("leds: core: Add support for poll()ing
> > > >>>the sysfs brightness attr for changes.") breaks runtime PM for me.
> > > >>>
> > > >>>On my omap dm3730 based test system, idle power consumption is over 70
> > > >>>times higher now with this patch! It goes from about 6mW for the core
> > > >>>system to over 440mW during idle meaning there's some busy timer now
> > > >>>active.
> > > >>>
> > > >>>Reverting this patch fixes the issue. Any ideas?
> > 
> > Are you using any LED that toggles with high frequency? Like perhaps
> > LED that is lit when CPU is active?
> 
> Yeah one of them seems to have cpu0 as the default trigger.

Aha. Its quite obvious we don't want to notify sysfs each time that
one is toggled, right?

IMO brightness should display max brightness for the trigger, as Hans
suggested, anything else is madness for trigger such as cpu activity.

Thanks and best regards,

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161110/db73f569/attachment.sig>

^ permalink raw reply

* PM regression with LED changes in next-20161109
From: Pavel Machek @ 2016-11-10 20:48 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <de3b4ae7-6a08-2a03-d80e-059c71c58aed@redhat.com>

Hi!

> >>It seems that we should get back to your initial approach. i.e. only
> >>brightness changes caused by hardware should be reported.
> >
> >I don't think enabling poll() here is good idea. Some hardware won't
> >be able to tell you that it changed the state. Returning maximum
> >brightness trigger is going to use seems easier/better.
> 
> The idea here is to allow userspace to poll() on the brightness
> sysfs atrribute to detect changes autonomously done by the hardware,
> such as e.g. happens on both Dell and Thinkpad laptops when pressing
> the keyboard backlight cycle hotkey. Note that these keys do not
> generate key-press events, the cycling through the brightness levels
> (including off) is done entirely in firmware.

Ok, so you can do that for keyboard backlight on thinkpad... I guess
you handle that as a special trigger on the keyboard leds? Can other
triggers, such as heartbeat, be assigned to that "led"? 

> But we do get other ACPI events for this which we can use to let
> userspace know this happens, which is something which user-
> interfaces which allow control over the kbd backlight want to know.

Yes, you can do that for keyboard backlight... but on thinkpads there
are more leds, such as battery led. That can blink on battery low, and
I don't think you can read the current status from hardware.

Getting current state of led blinking with cpu trigger is also not
quite a good idea. 
So IMO this should not be done in generic code. Instead,
kbd-backlight trigger should have special attribute, and that one
should be pollable.

> I understand that we will not always be able to do this, here is the
> Documentation/ABI/testing/sysfs-class-led text I have in mind:
> 
> 		The file supports poll() to detect changes, changes are only
> 		signalled when this file is written or when the hardware /
> 		firmware changes the brightness itself and the driver can detect
> 		this. Changes done by kernel triggers / software blinking are
> 		not signalled.
> 
> Note the "and the driver can detect this" language, that has been there
> since v1 of the poll() notification patch since I already expected not
> all hardware to be able to signal this.

Lets move it to separate attribute, for triggers that can do that,
please.

We do want a way to read maximum brightness for the heartbeat trigger,
for example..

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161110/5e875363/attachment.sig>

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox