[QUEUED v20160630 0/4] stm class/intel_th: Updates for char-misc-next

[QUEUED v20160630 0/4] stm class/intel_th: Updates for char-misc-next

[Alexander Shishkin]

Hi everybody,

These are the updates I have for the next merge window if we can still
make it. I'm going to request-pull these to Greg soon unless somebody
objects.

The main theme here is adding runtime pm handling to both intel_th and
stm class. I have not tested the coresight-stm with this update, but
from the code I concluded that its driver keeps it 'active' the entire
time it's enabled, so this update shouldn't make a difference for
it. However, this stm class update should allow for more fine-grained
runtime pm for coresight-stm as well in the future.

Alexander Shishkin (4):
  stm class: Add runtime power management handling
  intel_th: Add runtime power management handling
  intel_th: gth: Fix a source comment
  intel_th: Document output device callbacks

 drivers/hwtracing/intel_th/core.c     | 54 +++++++++++++++++++++++++++++------
 drivers/hwtracing/intel_th/gth.c      | 18 ++++++++++--
 drivers/hwtracing/intel_th/intel_th.h |  3 ++
 drivers/hwtracing/stm/core.c          | 47 ++++++++++++++++++++++++++++++
 4 files changed, 111 insertions(+), 11 deletions(-)

-- 
2.8.1

[QUEUED v20160630 1/4] stm class: Add runtime power management handling

[Alexander Shishkin]

Currently, there's no runtime pm in stm class devices, which makes it
harder for the underlying hardware drivers to handle their power
management.

This patch applies the following runtime pm policy to stm class devices,
which their parents can rely on for their power management tracking:

  * device is in use during character device writes,
  * delayed autosuspend is used to keep it active between adjacent
  writes,
  * device is in use while mmio regions are mapped,
  * device is is use while any stm_source devices are linked to it.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Chunyan Zhang <zhang.chunyan@linaro.org>
---
 drivers/hwtracing/stm/core.c | 47 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index ff31108b06..ff3a868c2a 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -15,6 +15,7 @@
  * as defined in MIPI STPv2 specification.
  */
 
+#include <linux/pm_runtime.h>
 #include <linux/uaccess.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -482,14 +483,40 @@ static ssize_t stm_char_write(struct file *file, const char __user *buf,
 		return -EFAULT;
 	}
 
+	pm_runtime_get_sync(&stm->dev);
+
 	count = stm_write(stm->data, stmf->output.master, stmf->output.channel,
 			  kbuf, count);
 
+	pm_runtime_mark_last_busy(&stm->dev);
+	pm_runtime_put_autosuspend(&stm->dev);
 	kfree(kbuf);
 
 	return count;
 }
 
+static void stm_mmap_open(struct vm_area_struct *vma)
+{
+	struct stm_file *stmf = vma->vm_file->private_data;
+	struct stm_device *stm = stmf->stm;
+
+	pm_runtime_get(&stm->dev);
+}
+
+static void stm_mmap_close(struct vm_area_struct *vma)
+{
+	struct stm_file *stmf = vma->vm_file->private_data;
+	struct stm_device *stm = stmf->stm;
+
+	pm_runtime_mark_last_busy(&stm->dev);
+	pm_runtime_put_autosuspend(&stm->dev);
+}
+
+static const struct vm_operations_struct stm_mmap_vmops = {
+	.open	= stm_mmap_open,
+	.close	= stm_mmap_close,
+};
+
 static int stm_char_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct stm_file *stmf = file->private_data;
@@ -514,8 +541,11 @@ static int stm_char_mmap(struct file *file, struct vm_area_struct *vma)
 	if (!phys)
 		return -EINVAL;
 
+	pm_runtime_get_sync(&stm->dev);
+
 	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
+	vma->vm_ops = &stm_mmap_vmops;
 	vm_iomap_memory(vma, phys, size);
 
 	return 0;
@@ -701,6 +731,12 @@ int stm_register_device(struct device *parent, struct stm_data *stm_data,
 	if (err)
 		goto err_device;
 
+	pm_runtime_no_callbacks(&stm->dev);
+	pm_runtime_use_autosuspend(&stm->dev);
+	pm_runtime_set_autosuspend_delay(&stm->dev, 2000);
+	pm_runtime_set_suspended(&stm->dev);
+	pm_runtime_enable(&stm->dev);
+
 	return 0;
 
 err_device:
@@ -724,6 +760,9 @@ void stm_unregister_device(struct stm_data *stm_data)
 	struct stm_source_device *src, *iter;
 	int i, ret;
 
+	pm_runtime_dont_use_autosuspend(&stm->dev);
+	pm_runtime_disable(&stm->dev);
+
 	mutex_lock(&stm->link_mutex);
 	list_for_each_entry_safe(src, iter, &stm->link_list, link_entry) {
 		ret = __stm_source_link_drop(src, stm);
@@ -878,6 +917,8 @@ static int __stm_source_link_drop(struct stm_source_device *src,
 
 	stm_output_free(link, &src->output);
 	list_del_init(&src->link_entry);
+	pm_runtime_mark_last_busy(&link->dev);
+	pm_runtime_put_autosuspend(&link->dev);
 	/* matches stm_find_device() from stm_source_link_store() */
 	stm_put_device(link);
 	rcu_assign_pointer(src->link, NULL);
@@ -971,8 +1012,11 @@ static ssize_t stm_source_link_store(struct device *dev,
 	if (!link)
 		return -EINVAL;
 
+	pm_runtime_get(&link->dev);
+
 	err = stm_source_link_add(src, link);
 	if (err) {
+		pm_runtime_put_autosuspend(&link->dev);
 		/* matches the stm_find_device() above */
 		stm_put_device(link);
 	}
@@ -1033,6 +1077,9 @@ int stm_source_register_device(struct device *parent,
 	if (err)
 		goto err;
 
+	pm_runtime_no_callbacks(&src->dev);
+	pm_runtime_forbid(&src->dev);
+
 	err = device_add(&src->dev);
 	if (err)
 		goto err;
-- 
2.8.1

[QUEUED v20160630 2/4] intel_th: Add runtime power management handling

[Alexander Shishkin]

Currently, an Intel TH (pci) device will be always active, because the
devices on the 'intel_th' bus don't implement runtime pm to track their
usage.

To address this, this patch adds runtime pm support to the 'intel_th'
bus and some additional bits for the hub. The 'output' type device is
in use while a capture is active; the 'source' type device (STH) relies
on its child stm class device for runtime pm tracking.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 drivers/hwtracing/intel_th/core.c | 54 ++++++++++++++++++++++++++++++++-------
 drivers/hwtracing/intel_th/gth.c  | 16 +++++++++++-
 2 files changed, 60 insertions(+), 10 deletions(-)

diff --git a/drivers/hwtracing/intel_th/core.c b/drivers/hwtracing/intel_th/core.c
index 0b112ae689..6f0a51a2c6 100644
--- a/drivers/hwtracing/intel_th/core.c
+++ b/drivers/hwtracing/intel_th/core.c
@@ -23,6 +23,7 @@
 #include <linux/debugfs.h>
 #include <linux/idr.h>
 #include <linux/pci.h>
+#include <linux/pm_runtime.h>
 #include <linux/dma-mapping.h>
 
 #include "intel_th.h"
@@ -67,23 +68,33 @@ static int intel_th_probe(struct device *dev)
 
 	hubdrv = to_intel_th_driver(hub->dev.driver);
 
+	pm_runtime_set_active(dev);
+	pm_runtime_no_callbacks(dev);
+	pm_runtime_enable(dev);
+
 	ret = thdrv->probe(to_intel_th_device(dev));
 	if (ret)
-		return ret;
+		goto out_pm;
 
 	if (thdrv->attr_group) {
 		ret = sysfs_create_group(&thdev->dev.kobj, thdrv->attr_group);
-		if (ret) {
-			thdrv->remove(thdev);
-
-			return ret;
-		}
+		if (ret)
+			goto out;
 	}
 
 	if (thdev->type == INTEL_TH_OUTPUT &&
 	    !intel_th_output_assigned(thdev))
+		/* does not talk to hardware */
 		ret = hubdrv->assign(hub, thdev);
 
+out:
+	if (ret)
+		thdrv->remove(thdev);
+
+out_pm:
+	if (ret)
+		pm_runtime_disable(dev);
+
 	return ret;
 }
 
@@ -103,6 +114,8 @@ static int intel_th_remove(struct device *dev)
 	if (thdrv->attr_group)
 		sysfs_remove_group(&thdev->dev.kobj, thdrv->attr_group);
 
+	pm_runtime_get_sync(dev);
+
 	thdrv->remove(thdev);
 
 	if (intel_th_output_assigned(thdev)) {
@@ -110,9 +123,14 @@ static int intel_th_remove(struct device *dev)
 			to_intel_th_driver(dev->parent->driver);
 
 		if (hub->dev.driver)
+			/* does not talk to hardware */
 			hubdrv->unassign(hub, thdev);
 	}
 
+	pm_runtime_disable(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	return 0;
 }
 
@@ -185,6 +203,7 @@ static int intel_th_output_activate(struct intel_th_device *thdev)
 {
 	struct intel_th_driver *thdrv =
 		to_intel_th_driver_or_null(thdev->dev.driver);
+	int ret = 0;
 
 	if (!thdrv)
 		return -ENODEV;
@@ -192,12 +211,17 @@ static int intel_th_output_activate(struct intel_th_device *thdev)
 	if (!try_module_get(thdrv->driver.owner))
 		return -ENODEV;
 
+	pm_runtime_get_sync(&thdev->dev);
+
 	if (thdrv->activate)
-		return thdrv->activate(thdev);
+		ret = thdrv->activate(thdev);
+	else
+		intel_th_trace_enable(thdev);
 
-	intel_th_trace_enable(thdev);
+	if (ret)
+		pm_runtime_put(&thdev->dev);
 
-	return 0;
+	return ret;
 }
 
 static void intel_th_output_deactivate(struct intel_th_device *thdev)
@@ -213,6 +237,7 @@ static void intel_th_output_deactivate(struct intel_th_device *thdev)
 	else
 		intel_th_trace_disable(thdev);
 
+	pm_runtime_put(&thdev->dev);
 	module_put(thdrv->driver.owner);
 }
 
@@ -660,6 +685,10 @@ intel_th_alloc(struct device *dev, struct resource *devres,
 
 	dev_set_drvdata(dev, th);
 
+	pm_runtime_no_callbacks(dev);
+	pm_runtime_put(dev);
+	pm_runtime_allow(dev);
+
 	err = intel_th_populate(th, devres, ndevres, irq);
 	if (err)
 		goto err_chrdev;
@@ -667,6 +696,8 @@ intel_th_alloc(struct device *dev, struct resource *devres,
 	return th;
 
 err_chrdev:
+	pm_runtime_forbid(dev);
+
 	__unregister_chrdev(th->major, 0, TH_POSSIBLE_OUTPUTS,
 			    "intel_th/output");
 
@@ -691,6 +722,9 @@ void intel_th_free(struct intel_th *th)
 
 	intel_th_device_remove(th->hub);
 
+	pm_runtime_get_sync(th->dev);
+	pm_runtime_forbid(th->dev);
+
 	__unregister_chrdev(th->major, 0, TH_POSSIBLE_OUTPUTS,
 			    "intel_th/output");
 
@@ -715,6 +749,7 @@ int intel_th_trace_enable(struct intel_th_device *thdev)
 	if (WARN_ON_ONCE(thdev->type != INTEL_TH_OUTPUT))
 		return -EINVAL;
 
+	pm_runtime_get_sync(&thdev->dev);
 	hubdrv->enable(hub, &thdev->output);
 
 	return 0;
@@ -735,6 +770,7 @@ int intel_th_trace_disable(struct intel_th_device *thdev)
 		return -EINVAL;
 
 	hubdrv->disable(hub, &thdev->output);
+	pm_runtime_put(&thdev->dev);
 
 	return 0;
 }
diff --git a/drivers/hwtracing/intel_th/gth.c b/drivers/hwtracing/intel_th/gth.c
index 9beea0b542..4106eaf131 100644
--- a/drivers/hwtracing/intel_th/gth.c
+++ b/drivers/hwtracing/intel_th/gth.c
@@ -22,6 +22,7 @@
 #include <linux/mm.h>
 #include <linux/slab.h>
 #include <linux/bitmap.h>
+#include <linux/pm_runtime.h>
 
 #include "intel_th.h"
 #include "gth.h"
@@ -190,6 +191,11 @@ static ssize_t master_attr_store(struct device *dev,
 	if (old_port >= 0) {
 		gth->master[ma->master] = -1;
 		clear_bit(ma->master, gth->output[old_port].master);
+
+		/*
+		 * if the port is active, program this setting,
+		 * implies that runtime PM is on
+		 */
 		if (gth->output[old_port].output->active)
 			gth_master_set(gth, ma->master, -1);
 	}
@@ -204,7 +210,7 @@ static ssize_t master_attr_store(struct device *dev,
 
 		set_bit(ma->master, gth->output[port].master);
 
-		/* if the port is active, program this setting */
+		/* if the port is active, program this setting, see above */
 		if (gth->output[port].output->active)
 			gth_master_set(gth, ma->master, port);
 	}
@@ -326,11 +332,15 @@ static ssize_t output_attr_show(struct device *dev,
 	struct gth_device *gth = oa->gth;
 	size_t count;
 
+	pm_runtime_get_sync(dev);
+
 	spin_lock(&gth->gth_lock);
 	count = snprintf(buf, PAGE_SIZE, "%x\n",
 			 gth_output_parm_get(gth, oa->port, oa->parm));
 	spin_unlock(&gth->gth_lock);
 
+	pm_runtime_put(dev);
+
 	return count;
 }
 
@@ -346,10 +356,14 @@ static ssize_t output_attr_store(struct device *dev,
 	if (kstrtouint(buf, 16, &config) < 0)
 		return -EINVAL;
 
+	pm_runtime_get_sync(dev);
+
 	spin_lock(&gth->gth_lock);
 	gth_output_parm_set(gth, oa->port, oa->parm, config);
 	spin_unlock(&gth->gth_lock);
 
+	pm_runtime_put(dev);
+
 	return count;
 }
 
-- 
2.8.1

[QUEUED v20160630 3/4] intel_th: gth: Fix a source comment

[Alexander Shishkin]

There's a kerneldoc comment that'd been derived from another one by way
of copying-and-pasting but hadn't been subsequently amended to replect
the purpose of the function. Fix this.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 drivers/hwtracing/intel_th/gth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwtracing/intel_th/gth.c b/drivers/hwtracing/intel_th/gth.c
index 4106eaf131..33e09369a4 100644
--- a/drivers/hwtracing/intel_th/gth.c
+++ b/drivers/hwtracing/intel_th/gth.c
@@ -465,7 +465,7 @@ static int intel_th_output_attributes(struct gth_device *gth)
 }
 
 /**
- * intel_th_gth_disable() - enable tracing to an output device
+ * intel_th_gth_disable() - disable tracing to an output device
  * @thdev:	GTH device
  * @output:	output device's descriptor
  *
-- 
2.8.1

[QUEUED v20160630 4/4] intel_th: Document output device callbacks

[Alexander Shishkin]

'output' type device callbacks are missing from the kerneldoc description
of the 'intel_th_driver' structure. Fix this.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 drivers/hwtracing/intel_th/intel_th.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hwtracing/intel_th/intel_th.h b/drivers/hwtracing/intel_th/intel_th.h
index 0482848260..4c195786bf 100644
--- a/drivers/hwtracing/intel_th/intel_th.h
+++ b/drivers/hwtracing/intel_th/intel_th.h
@@ -114,6 +114,9 @@ intel_th_output_assigned(struct intel_th_device *thdev)
  * @unassign:	deassociate an output type device from an output port
  * @enable:	enable tracing for a given output device
  * @disable:	disable tracing for a given output device
+ * @irq:	interrupt callback
+ * @activate:	enable tracing on the output's side
+ * @deactivate:	disable tracing on the output's side
  * @fops:	file operations for device nodes
  * @attr_group:	attributes provided by the driver
  *
-- 
2.8.1

Re: [QUEUED v20160630 1/4] stm class: Add runtime power management handling

[Mathieu Poirier]

On 30 June 2016 at 06:56, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Currently, there's no runtime pm in stm class devices, which makes it
> harder for the underlying hardware drivers to handle their power
> management.
>
> This patch applies the following runtime pm policy to stm class devices,
> which their parents can rely on for their power management tracking:
>
>   * device is in use during character device writes,
>   * delayed autosuspend is used to keep it active between adjacent
>   writes,
>   * device is in use while mmio regions are mapped,
>   * device is is use while any stm_source devices are linked to it.
>
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Chunyan Zhang <zhang.chunyan@linaro.org>

Coresight power management on my Juno board (the only device with an
STM I have access to) is broken and as such, can't test if this code
does what is intended.  But theoretically it looks good.

Throughout the driver, wouldn't it be better to use
pm_runtime_put_sync() rather than autosuspending with a hard coded
value?

Thanks,
Mathieu

> ---
>  drivers/hwtracing/stm/core.c | 47 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
>
> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
> index ff31108b06..ff3a868c2a 100644
> --- a/drivers/hwtracing/stm/core.c
> +++ b/drivers/hwtracing/stm/core.c
> @@ -15,6 +15,7 @@
>   * as defined in MIPI STPv2 specification.
>   */
>
> +#include <linux/pm_runtime.h>
>  #include <linux/uaccess.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> @@ -482,14 +483,40 @@ static ssize_t stm_char_write(struct file *file, const char __user *buf,
>                 return -EFAULT;
>         }
>
> +       pm_runtime_get_sync(&stm->dev);
> +
>         count = stm_write(stm->data, stmf->output.master, stmf->output.channel,
>                           kbuf, count);
>
> +       pm_runtime_mark_last_busy(&stm->dev);
> +       pm_runtime_put_autosuspend(&stm->dev);
>         kfree(kbuf);
>
>         return count;
>  }
>
> +static void stm_mmap_open(struct vm_area_struct *vma)
> +{
> +       struct stm_file *stmf = vma->vm_file->private_data;
> +       struct stm_device *stm = stmf->stm;
> +
> +       pm_runtime_get(&stm->dev);
> +}
> +
> +static void stm_mmap_close(struct vm_area_struct *vma)
> +{
> +       struct stm_file *stmf = vma->vm_file->private_data;
> +       struct stm_device *stm = stmf->stm;
> +
> +       pm_runtime_mark_last_busy(&stm->dev);
> +       pm_runtime_put_autosuspend(&stm->dev);
> +}
> +
> +static const struct vm_operations_struct stm_mmap_vmops = {
> +       .open   = stm_mmap_open,
> +       .close  = stm_mmap_close,
> +};
> +
>  static int stm_char_mmap(struct file *file, struct vm_area_struct *vma)
>  {
>         struct stm_file *stmf = file->private_data;
> @@ -514,8 +541,11 @@ static int stm_char_mmap(struct file *file, struct vm_area_struct *vma)
>         if (!phys)
>                 return -EINVAL;
>
> +       pm_runtime_get_sync(&stm->dev);
> +
>         vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
>         vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
> +       vma->vm_ops = &stm_mmap_vmops;
>         vm_iomap_memory(vma, phys, size);
>
>         return 0;
> @@ -701,6 +731,12 @@ int stm_register_device(struct device *parent, struct stm_data *stm_data,
>         if (err)
>                 goto err_device;
>
> +       pm_runtime_no_callbacks(&stm->dev);
> +       pm_runtime_use_autosuspend(&stm->dev);
> +       pm_runtime_set_autosuspend_delay(&stm->dev, 2000);
> +       pm_runtime_set_suspended(&stm->dev);
> +       pm_runtime_enable(&stm->dev);
> +
>         return 0;
>
>  err_device:
> @@ -724,6 +760,9 @@ void stm_unregister_device(struct stm_data *stm_data)
>         struct stm_source_device *src, *iter;
>         int i, ret;
>
> +       pm_runtime_dont_use_autosuspend(&stm->dev);
> +       pm_runtime_disable(&stm->dev);
> +
>         mutex_lock(&stm->link_mutex);
>         list_for_each_entry_safe(src, iter, &stm->link_list, link_entry) {
>                 ret = __stm_source_link_drop(src, stm);
> @@ -878,6 +917,8 @@ static int __stm_source_link_drop(struct stm_source_device *src,
>
>         stm_output_free(link, &src->output);
>         list_del_init(&src->link_entry);
> +       pm_runtime_mark_last_busy(&link->dev);
> +       pm_runtime_put_autosuspend(&link->dev);
>         /* matches stm_find_device() from stm_source_link_store() */
>         stm_put_device(link);
>         rcu_assign_pointer(src->link, NULL);
> @@ -971,8 +1012,11 @@ static ssize_t stm_source_link_store(struct device *dev,
>         if (!link)
>                 return -EINVAL;
>
> +       pm_runtime_get(&link->dev);
> +
>         err = stm_source_link_add(src, link);
>         if (err) {
> +               pm_runtime_put_autosuspend(&link->dev);
>                 /* matches the stm_find_device() above */
>                 stm_put_device(link);
>         }
> @@ -1033,6 +1077,9 @@ int stm_source_register_device(struct device *parent,
>         if (err)
>                 goto err;
>
> +       pm_runtime_no_callbacks(&src->dev);
> +       pm_runtime_forbid(&src->dev);
> +
>         err = device_add(&src->dev);
>         if (err)
>                 goto err;
> --
> 2.8.1
>

Re: [QUEUED v20160630 1/4] stm class: Add runtime power management handling

[Alexander Shishkin]

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

> On 30 June 2016 at 06:56, Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
>> Currently, there's no runtime pm in stm class devices, which makes it
>> harder for the underlying hardware drivers to handle their power
>> management.
>>
>> This patch applies the following runtime pm policy to stm class devices,
>> which their parents can rely on for their power management tracking:
>>
>>   * device is in use during character device writes,
>>   * delayed autosuspend is used to keep it active between adjacent
>>   writes,
>>   * device is in use while mmio regions are mapped,
>>   * device is is use while any stm_source devices are linked to it.
>>
>> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Cc: Chunyan Zhang <zhang.chunyan@linaro.org>
>
> Coresight power management on my Juno board (the only device with an
> STM I have access to) is broken and as such, can't test if this code
> does what is intended.  But theoretically it looks good.

Thanks for taking a look.

> Throughout the driver, wouldn't it be better to use
> pm_runtime_put_sync() rather than autosuspending with a hard coded
> value?

Yeah, the autosuspend is for the char write()ers that are likely to send
multiple consequent write()s, so that we don't have to go in and out of
suspend every time that happens.

Thanks,
--
Alex

Re: [QUEUED v20160630 1/4] stm class: Add runtime power management handling

[Mathieu Poirier]

On 30 June 2016 at 09:30, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Mathieu Poirier <mathieu.poirier@linaro.org> writes:
>
>> On 30 June 2016 at 06:56, Alexander Shishkin
>> <alexander.shishkin@linux.intel.com> wrote:
>>> Currently, there's no runtime pm in stm class devices, which makes it
>>> harder for the underlying hardware drivers to handle their power
>>> management.
>>>
>>> This patch applies the following runtime pm policy to stm class devices,
>>> which their parents can rely on for their power management tracking:
>>>
>>>   * device is in use during character device writes,
>>>   * delayed autosuspend is used to keep it active between adjacent
>>>   writes,
>>>   * device is in use while mmio regions are mapped,
>>>   * device is is use while any stm_source devices are linked to it.
>>>
>>> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> Cc: Chunyan Zhang <zhang.chunyan@linaro.org>
>>
>> Coresight power management on my Juno board (the only device with an
>> STM I have access to) is broken and as such, can't test if this code
>> does what is intended.  But theoretically it looks good.
>
> Thanks for taking a look.
>
>> Throughout the driver, wouldn't it be better to use
>> pm_runtime_put_sync() rather than autosuspending with a hard coded
>> value?
>
> Yeah, the autosuspend is for the char write()ers that are likely to send
> multiple consequent write()s, so that we don't have to go in and out of
> suspend every time that happens.
>

Yes, it's a trade off.  Please add a comment in stm_register_device()
that explains the usage of the autosuspend functions and the choice of
'2000' value.

Thanks,

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

> Thanks,
> --
> Alex

Re: [QUEUED v20160630 3/4] intel_th: gth: Fix a source comment

[Chunyan Zhang]

On Thu, Jun 30, 2016 at 8:56 PM, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> There's a kerneldoc comment that'd been derived from another one by way
> of copying-and-pasting but hadn't been subsequently amended to replect

                                         ^
s/replect/reflect/ ?

> the purpose of the function. Fix this.
>
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> ---
>  drivers/hwtracing/intel_th/gth.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hwtracing/intel_th/gth.c b/drivers/hwtracing/intel_th/gth.c
> index 4106eaf131..33e09369a4 100644
> --- a/drivers/hwtracing/intel_th/gth.c
> +++ b/drivers/hwtracing/intel_th/gth.c
> @@ -465,7 +465,7 @@ static int intel_th_output_attributes(struct gth_device *gth)
>  }
>
>  /**
> - * intel_th_gth_disable() - enable tracing to an output device
> + * intel_th_gth_disable() - disable tracing to an output device
>   * @thdev:     GTH device
>   * @output:    output device's descriptor
>   *
> --
> 2.8.1
>

Re: [QUEUED v20160630 1/4] stm class: Add runtime power management handling

[Alexander Shishkin]

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

> On 30 June 2016 at 09:30, Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
>> Mathieu Poirier <mathieu.poirier@linaro.org> writes:
>>
>>> On 30 June 2016 at 06:56, Alexander Shishkin
>>> <alexander.shishkin@linux.intel.com> wrote:
>>>> Currently, there's no runtime pm in stm class devices, which makes it
>>>> harder for the underlying hardware drivers to handle their power
>>>> management.
>>>>
>>>> This patch applies the following runtime pm policy to stm class devices,
>>>> which their parents can rely on for their power management tracking:
>>>>
>>>>   * device is in use during character device writes,
>>>>   * delayed autosuspend is used to keep it active between adjacent
>>>>   writes,
>>>>   * device is in use while mmio regions are mapped,
>>>>   * device is is use while any stm_source devices are linked to it.
>>>>
>>>> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>> Cc: Chunyan Zhang <zhang.chunyan@linaro.org>
>>>
>>> Coresight power management on my Juno board (the only device with an
>>> STM I have access to) is broken and as such, can't test if this code
>>> does what is intended.  But theoretically it looks good.
>>
>> Thanks for taking a look.
>>
>>> Throughout the driver, wouldn't it be better to use
>>> pm_runtime_put_sync() rather than autosuspending with a hard coded
>>> value?
>>
>> Yeah, the autosuspend is for the char write()ers that are likely to send
>> multiple consequent write()s, so that we don't have to go in and out of
>> suspend every time that happens.
>>
>
> Yes, it's a trade off.  Please add a comment in stm_register_device()
> that explains the usage of the autosuspend functions and the choice of
> '2000' value.

Will do. The 2000 is arbitrary, but it has to be configurable from a
sysfs attribute and I thought that's a start as good as any.

> Thanks,
>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

Thanks,
--
Alex

Re: [QUEUED v20160630 3/4] intel_th: gth: Fix a source comment

[Alexander Shishkin]

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

> On Thu, Jun 30, 2016 at 8:56 PM, Alexander Shishkin
> <alexander.shishkin@linux.intel.com> wrote:
>> There's a kerneldoc comment that'd been derived from another one by way
>> of copying-and-pasting but hadn't been subsequently amended to replect
>
>                                          ^
> s/replect/reflect/ ?

Thanks!

Regards,
--
Alex