[v14,4/4] CMDQ: save more energy in idle

Commit Message

[e-mail] Sept. 5, 2016, 1:44 a.m. UTC

Use clk_disable_unprepare instead of clk_disable to save more energy
when CMDQ is idle.

Signed-off-by: HS Liao <[e-mail]>
---
 drivers/mailbox/mtk-cmdq.c | 54 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 46 insertions(+), 8 deletions(-)

Comments

Jassi Brar Sept. 22, 2016, 7:52 a.m. UTC | #1

On Mon, Sep 5, 2016 at 7:14 AM, HS Liao <[e-mail]> wrote:
> Use clk_disable_unprepare instead of clk_disable to save more energy
> when CMDQ is idle.
>
> Signed-off-by: HS Liao <[e-mail]>
> ---
>  drivers/mailbox/mtk-cmdq.c | 54 +++++++++++++++++++++++++++++++++++++++-------

The driver is introduced by second patch of the set, so it makes sense
to merge this patch into patch 2/4.

[e-mail] Sept. 23, 2016, 9:28 a.m. UTC | #2

On Thu, 2016-09-22 at 13:22 +0530, Jassi Brar wrote:
> On Mon, Sep 5, 2016 at 7:14 AM, HS Liao <[e-mail]> wrote:
> > Use clk_disable_unprepare instead of clk_disable to save more energy
> > when CMDQ is idle.
> >
> > Signed-off-by: HS Liao <[e-mail]>
> > ---
> >  drivers/mailbox/mtk-cmdq.c | 54 +++++++++++++++++++++++++++++++++++++++-------
> 
> The driver is introduced by second patch of the set, so it makes sense
> to merge this patch into patch 2/4.

Hi Jassi,

Could you take a look at previous discussion between Matthias and me?
http://lkml.iu.edu/hypermail/linux/kernel/1606.2/05239.html
His basic idea is to simplify first working version.
Therefore, I move some code to this patch.

Thanks,
HS

[e-mail] Sept. 30, 2016, 8:56 a.m. UTC | #3

On Fri, 2016-09-23 at 17:28 +0800, Horng-Shyang Liao wrote:
> On Thu, 2016-09-22 at 13:22 +0530, Jassi Brar wrote:
> > On Mon, Sep 5, 2016 at 7:14 AM, HS Liao <[e-mail]> wrote:
> > > Use clk_disable_unprepare instead of clk_disable to save more energy
> > > when CMDQ is idle.
> > >
> > > Signed-off-by: HS Liao <[e-mail]>
> > > ---
> > >  drivers/mailbox/mtk-cmdq.c | 54 +++++++++++++++++++++++++++++++++++++++-------
> > 
> > The driver is introduced by second patch of the set, so it makes sense
> > to merge this patch into patch 2/4.
> 
> Hi Jassi,
> 
> Could you take a look at previous discussion between Matthias and me?
> http://lkml.iu.edu/hypermail/linux/kernel/1606.2/05239.html
> His basic idea is to simplify first working version.
> Therefore, I move some code to this patch.
> 
> Thanks,
> HS
> 

Hi Jassi,

What do you think about our previous discussion?

Thanks,
HS

Matthias Brugger Sept. 30, 2016, 9:01 a.m. UTC | #4

On 09/30/2016 10:56 AM, Horng-Shyang Liao wrote:
> On Fri, 2016-09-23 at 17:28 +0800, Horng-Shyang Liao wrote:
>> On Thu, 2016-09-22 at 13:22 +0530, Jassi Brar wrote:
>>> On Mon, Sep 5, 2016 at 7:14 AM, HS Liao <[e-mail]> wrote:
>>>> Use clk_disable_unprepare instead of clk_disable to save more energy
>>>> when CMDQ is idle.
>>>>
>>>> Signed-off-by: HS Liao <[e-mail]>
>>>> ---
>>>>  drivers/mailbox/mtk-cmdq.c | 54 +++++++++++++++++++++++++++++++++++++++-------
>>>
>>> The driver is introduced by second patch of the set, so it makes sense
>>> to merge this patch into patch 2/4.
>>
>> Hi Jassi,
>>
>> Could you take a look at previous discussion between Matthias and me?
>> http://lkml.iu.edu/hypermail/linux/kernel/1606.2/05239.html
>> His basic idea is to simplify first working version.
>> Therefore, I move some code to this patch.
>>

Well what I wanted to say is, that right now this driver is quite a big 
beast and this makes it difficult to review. So my idea was to just 
submit the most basic version of this driver.
Any improvements on the driver should be sent in follow-up patches after 
the basic driver got merged. That was my idea.

Regards,
Matthias

Patch

diff --git a/drivers/mailbox/mtk-cmdq.c b/drivers/mailbox/mtk-cmdq.c
index daf5561..0bf30cb 100644
--- a/drivers/mailbox/mtk-cmdq.c
+++ b/drivers/mailbox/mtk-cmdq.c
@@ -29,6 +29,7 @@ 
 #include <linux/spinlock.h>
 #include <linux/suspend.h>
 #include <linux/timer.h>
+#include <linux/workqueue.h>
 
 #define CMDQ_THR_MAX_COUNT		3 /* main, sub, general(misc) */
 #define CMDQ_INST_SIZE			8 /* instruction is 64-bit */
@@ -130,10 +131,16 @@  struct cmdq_task {
 	struct cmdq_task_cb	cb;
 };
 
+struct cmdq_clk_release {
+	struct cmdq		*cmdq;
+	struct work_struct	release_work;
+};
+
 struct cmdq {
 	struct mbox_controller	mbox;
 	void __iomem		*base;
 	u32			irq;
+	struct workqueue_struct	*clk_release_wq;
 	struct cmdq_thread	thread[CMDQ_THR_MAX_COUNT];
 	struct mutex		task_mutex;
 	struct clk		*clock;
@@ -279,11 +286,19 @@  static void cmdq_thread_wait_end(struct cmdq_thread *thread,
 static void cmdq_task_exec(struct cmdq_task *task, struct cmdq_thread *thread)
 {
 	struct cmdq *cmdq = task->cmdq;
-	unsigned long curr_pa, end_pa;
+	unsigned long curr_pa, end_pa, flags;
 
 	task->thread = thread;
 	if (list_empty(&thread->task_busy_list)) {
-		WARN_ON(clk_enable(cmdq->clock) < 0);
+		/*
+		 * Unlock for clk prepare (sleeping function).
+		 * We are safe to do that since we have task_mutex and
+		 * only flush will add task.
+		 */
+		spin_unlock_irqrestore(&thread->chan->lock, flags);
+		WARN_ON(clk_prepare_enable(cmdq->clock) < 0);
+		spin_lock_irqsave(&thread->chan->lock, flags);
+
 		WARN_ON(cmdq_thread_reset(cmdq, thread) < 0);
 
 		writel(task->pa_base, thread->base + CMDQ_THR_CURR_ADDR);
@@ -365,6 +380,26 @@  static void cmdq_task_handle_error(struct cmdq_task *task)
 	cmdq_thread_resume(thread);
 }
 
+static void cmdq_clk_release_work(struct work_struct *work_item)
+{
+	struct cmdq_clk_release *clk_release = container_of(work_item,
+			struct cmdq_clk_release, release_work);
+	struct cmdq *cmdq = clk_release->cmdq;
+
+	clk_disable_unprepare(cmdq->clock);
+	kfree(clk_release);
+}
+
+static void cmdq_clk_release_schedule(struct cmdq *cmdq)
+{
+	struct cmdq_clk_release *clk_release;
+
+	clk_release = kmalloc(sizeof(*clk_release), GFP_ATOMIC);
+	clk_release->cmdq = cmdq;
+	INIT_WORK(&clk_release->release_work, cmdq_clk_release_work);
+	queue_work(cmdq->clk_release_wq, &clk_release->release_work);
+}
+
 static void cmdq_thread_irq_handler(struct cmdq *cmdq,
 				    struct cmdq_thread *thread)
 {
@@ -414,7 +449,7 @@  static void cmdq_thread_irq_handler(struct cmdq *cmdq,
 
 	if (list_empty(&thread->task_busy_list)) {
 		cmdq_thread_disable(cmdq, thread);
-		clk_disable(cmdq->clock);
+		cmdq_clk_release_schedule(cmdq);
 	} else {
 		mod_timer(&thread->timeout,
 			  jiffies + msecs_to_jiffies(CMDQ_TIMEOUT_MS));
@@ -473,7 +508,7 @@  static void cmdq_thread_handle_timeout(unsigned long data)
 
 	cmdq_thread_resume(thread);
 	cmdq_thread_disable(cmdq, thread);
-	clk_disable(cmdq->clock);
+	cmdq_clk_release_schedule(cmdq);
 	spin_unlock_irqrestore(&thread->chan->lock, flags);
 }
 
@@ -761,7 +796,7 @@  static int cmdq_suspend(struct device *dev)
 		msleep(20);
 	}
 
-	clk_unprepare(cmdq->clock);
+	flush_workqueue(cmdq->clk_release_wq);
 	return 0;
 }
 
@@ -769,7 +804,6 @@  static int cmdq_resume(struct device *dev)
 {
 	struct cmdq *cmdq = dev_get_drvdata(dev);
 
-	WARN_ON(clk_prepare(cmdq->clock) < 0);
 	cmdq->suspended = false;
 	return 0;
 }
@@ -778,8 +812,8 @@  static int cmdq_remove(struct platform_device *pdev)
 {
 	struct cmdq *cmdq = platform_get_drvdata(pdev);
 
+	destroy_workqueue(cmdq->clk_release_wq);
 	mbox_controller_unregister(&cmdq->mbox);
-	clk_unprepare(cmdq->clock);
 	return 0;
 }
 
@@ -898,8 +932,12 @@  static int cmdq_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	cmdq->clk_release_wq = alloc_ordered_workqueue(
+			"%s", WQ_MEM_RECLAIM | WQ_HIGHPRI,
+			"cmdq_clk_release");
+
 	platform_set_drvdata(pdev, cmdq);
-	WARN_ON(clk_prepare(cmdq->clock) < 0);
+
 	return 0;
 }