diff mbox

[v6] ARM: omap: edma: add suspend suspend/resume hooks

Message ID 1384726754-27875-1-git-send-email-zonque@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Mack Nov. 17, 2013, 10:19 p.m. UTC 
This patch makes the edma driver resume correctly after suspend. Tested
on an AM33xx platform with cyclic audio streams and omap_hsmmc.

All information can be reconstructed by already known runtime
information.

As we now use some functions that were previously only used from __init
context, annotations had to be dropped.

[nm@ti.com: added error handling for runtime + suspend_late/early_resume]
Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Daniel Mack <zonque@gmail.com>
Tested-by: Joel Fernandes <joelf@ti.com>
Acked-by: Joel Fernandes <joelf@ti.com>
---

v6:
  amended version from Nishanth Menon, adding error handling for runtime,
  and using suspend_late/early_resume.


 arch/arm/common/edma.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 91 insertions(+), 3 deletions(-)

Comments

Joel Fernandes Nov. 25, 2013, 5:34 p.m. UTC | #1 
On 11/17/2013 04:19 PM, Daniel Mack wrote:
> This patch makes the edma driver resume correctly after suspend. Tested
> on an AM33xx platform with cyclic audio streams and omap_hsmmc.
> 
> All information can be reconstructed by already known runtime
> information.
> 
> As we now use some functions that were previously only used from __init
> context, annotations had to be dropped.
> 
> [nm@ti.com: added error handling for runtime + suspend_late/early_resume]
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> Tested-by: Joel Fernandes <joelf@ti.com>
> Acked-by: Joel Fernandes <joelf@ti.com>

Hi Sekhar,

Can you consider pulling this patch? It has been tested and Acked. Thanks.

regards,

-Joel
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sekhar Nori Nov. 27, 2013, 1:22 p.m. UTC | #2 
+ Kevin

On Monday 25 November 2013 11:04 PM, Joel Fernandes wrote:
> On 11/17/2013 04:19 PM, Daniel Mack wrote:
>> This patch makes the edma driver resume correctly after suspend. Tested
>> on an AM33xx platform with cyclic audio streams and omap_hsmmc.
>>
>> All information can be reconstructed by already known runtime
>> information.
>>
>> As we now use some functions that were previously only used from __init
>> context, annotations had to be dropped.
>>
>> [nm@ti.com: added error handling for runtime + suspend_late/early_resume]
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>> Tested-by: Joel Fernandes <joelf@ti.com>
>> Acked-by: Joel Fernandes <joelf@ti.com>
> 
> Hi Sekhar,
> 
> Can you consider pulling this patch? It has been tested and Acked. Thanks.

Kevin had some inputs on previous version of this patch. Were you able
to make sure he is okay with this version being merged?

I have some comments for which I will send another e-mail.

Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Mack Nov. 27, 2013, 1:32 p.m. UTC | #3 
On 11/27/2013 02:22 PM, Sekhar Nori wrote:
> + Kevin
> 
> On Monday 25 November 2013 11:04 PM, Joel Fernandes wrote:
>> On 11/17/2013 04:19 PM, Daniel Mack wrote:
>>> This patch makes the edma driver resume correctly after suspend. Tested
>>> on an AM33xx platform with cyclic audio streams and omap_hsmmc.
>>>
>>> All information can be reconstructed by already known runtime
>>> information.
>>>
>>> As we now use some functions that were previously only used from __init
>>> context, annotations had to be dropped.
>>>
>>> [nm@ti.com: added error handling for runtime + suspend_late/early_resume]
>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>>> Tested-by: Joel Fernandes <joelf@ti.com>
>>> Acked-by: Joel Fernandes <joelf@ti.com>
>>
>> Hi Sekhar,
>>
>> Can you consider pulling this patch? It has been tested and Acked. Thanks.
> 
> Kevin had some inputs on previous version of this patch. Were you able
> to make sure he is okay with this version being merged?

I had concerns about the feedback I got, and haven't got answers yet.

In particular, I'm not convinced that using runtime PM to suspend
channels would actually save any power during runtime, or have any other
benefit. But I might be wrong - maybe someone at TI could comment on that?


Thanks,
Daniel

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sekhar Nori Nov. 27, 2013, 1:35 p.m. UTC | #4 
+ Kevin

On Monday 18 November 2013 03:49 AM, Daniel Mack wrote:
> This patch makes the edma driver resume correctly after suspend. Tested
> on an AM33xx platform with cyclic audio streams and omap_hsmmc.
> 
> All information can be reconstructed by already known runtime
> information.
> 
> As we now use some functions that were previously only used from __init
> context, annotations had to be dropped.
> 
> [nm@ti.com: added error handling for runtime + suspend_late/early_resume]
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> Tested-by: Joel Fernandes <joelf@ti.com>
> Acked-by: Joel Fernandes <joelf@ti.com>
> ---
> 
> v6:
>   amended version from Nishanth Menon, adding error handling for runtime,
>   and using suspend_late/early_resume.
> 
> 
>  arch/arm/common/edma.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 91 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
> index 8e1a024..e2b9638 100644
> --- a/arch/arm/common/edma.c
> +++ b/arch/arm/common/edma.c
> @@ -239,6 +239,8 @@ struct edma {
>  	/* list of channels with no even trigger; terminated by "-1" */
>  	const s8	*noevent;
>  
> +	struct edma_soc_info *info;
> +
>  	/* The edma_inuse bit for each PaRAM slot is clear unless the
>  	 * channel is in use ... by ARM or DSP, for QDMA, or whatever.
>  	 */
> @@ -290,13 +292,13 @@ static void map_dmach_queue(unsigned ctlr, unsigned ch_no,
>  			~(0x7 << bit), queue_no << bit);
>  }
>  
> -static void __init map_queue_tc(unsigned ctlr, int queue_no, int tc_no)
> +static void map_queue_tc(unsigned ctlr, int queue_no, int tc_no)
>  {
>  	int bit = queue_no * 4;
>  	edma_modify(ctlr, EDMA_QUETCMAP, ~(0x7 << bit), ((tc_no & 0x7) << bit));
>  }
>  
> -static void __init assign_priority_to_queue(unsigned ctlr, int queue_no,
> +static void assign_priority_to_queue(unsigned ctlr, int queue_no,
>  		int priority)
>  {
>  	int bit = queue_no * 4;
> @@ -315,7 +317,7 @@ static void __init assign_priority_to_queue(unsigned ctlr, int queue_no,
>   * included in that particular EDMA variant (Eg : dm646x)
>   *
>   */
> -static void __init map_dmach_param(unsigned ctlr)
> +static void map_dmach_param(unsigned ctlr)
>  {
>  	int i;
>  	for (i = 0; i < EDMA_MAX_DMACH; i++)
> @@ -1785,15 +1787,101 @@ static int edma_probe(struct platform_device *pdev)
>  			edma_write_array2(j, EDMA_DRAE, i, 1, 0x0);
>  			edma_write_array(j, EDMA_QRAE, i, 0x0);
>  		}
> +		edma_cc[j]->info = info[j];
>  		arch_num_cc++;
>  	}
>  
>  	return 0;
>  }
>  
> +static int edma_pm_suspend(struct device *dev)
> +{
> +	int j, r;
> +
> +	r = pm_runtime_get_sync(dev);
> +	if (IS_ERR_VALUE(r)) {

So IS_ERR_VALUE() is only for functions which may return a negative
number outside of MAX_ERRNO as a success indication.
pm_runtime_get_sync() does not appear to be one of them so just use"

if (r < 0) { .. }

> +		dev_err(dev, "%s: get_sync returned %d\n", __func__, r);
> +		return r;
> +	}
> +
> +	for (j = 0; j < arch_num_cc; j++) {
> +		struct edma *ecc = edma_cc[j];
> +
> +		disable_irq(ecc->irq_res_start);
> +		disable_irq(ecc->irq_res_end);
> +	}
> +
> +	pm_runtime_put_sync(dev);
> +
> +	return 0;
> +}
> +
> +static int edma_pm_resume(struct device *dev)
> +{
> +	int i, j, r;
> +
> +	r = pm_runtime_get_sync(dev);
> +	if (IS_ERR_VALUE(r)) {

Same here as above.

> +		dev_err(dev, "%s: get_sync returned %d\n", __func__, r);
> +		return r;
> +	}
> +
> +	for (j = 0; j < arch_num_cc; j++) {
> +		struct edma *cc = edma_cc[j];
> +
> +		s8 (*queue_priority_mapping)[2];
> +		s8 (*queue_tc_mapping)[2];
> +
> +		queue_tc_mapping = cc->info->queue_tc_mapping;
> +		queue_priority_mapping = cc->info->queue_priority_mapping;
> +
> +		/* Event queue to TC mapping */
> +		for (i = 0; queue_tc_mapping[i][0] != -1; i++)
> +			map_queue_tc(j, queue_tc_mapping[i][0],
> +					queue_tc_mapping[i][1]);
> +
> +		/* Event queue priority mapping */
> +		for (i = 0; queue_priority_mapping[i][0] != -1; i++)
> +			assign_priority_to_queue(j,
> +						queue_priority_mapping[i][0],
> +						queue_priority_mapping[i][1]);
> +
> +		/* Map the channel to param entry if channel mapping logic
> +		 * exist
> +		 */

Please follow the multi-line commenting style.

> +		if (edma_read(j, EDMA_CCCFG) & CHMAP_EXIST)
> +			map_dmach_param(j);
> +
> +		for (i = 0; i < cc->num_channels; i++) {
> +			if (test_bit(i, cc->edma_inuse)) {
> +				/* ensure access through shadow region 0 */
> +				edma_or_array2(j, EDMA_DRAE, 0, i >> 5,
> +						BIT(i & 0x1f));

There are some checkpatch checks that result from lines like this.
Please fix these as well.

CHECK: Alignment should match open parenthesis
#179: FILE: arch/arm/common/edma.c:1841:
+			map_queue_tc(j, queue_tc_mapping[i][0],
+					queue_tc_mapping[i][1]);

CHECK: Alignment should match open parenthesis
#184: FILE: arch/arm/common/edma.c:1846:
+			assign_priority_to_queue(j,
+						queue_priority_mapping[i][0],

CHECK: Alignment should match open parenthesis
#197: FILE: arch/arm/common/edma.c:1859:
+				edma_or_array2(j, EDMA_DRAE, 0, i >> 5,
+						BIT(i & 0x1f));

total: 0 errors, 0 warnings, 3 checks, 132 lines checked

Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Mack Nov. 27, 2013, 1:47 p.m. UTC | #5 
Hi Sekhar,

On 11/27/2013 02:35 PM, Sekhar Nori wrote:
> On Monday 18 November 2013 03:49 AM, Daniel Mack wrote:

>> +static int edma_pm_suspend(struct device *dev)
>> +{
>> +	int j, r;
>> +
>> +	r = pm_runtime_get_sync(dev);
>> +	if (IS_ERR_VALUE(r)) {
> 
> So IS_ERR_VALUE() is only for functions which may return a negative
> number outside of MAX_ERRNO as a success indication.
> pm_runtime_get_sync() does not appear to be one of them so just use"
>
> if (r < 0) { .. }

That's true. Thanks for catching this, I'll fix it. However, grepping
through the tree, there are quite a lot places where the same mistake is
made.

>> +		/* Map the channel to param entry if channel mapping logic
>> +		 * exist
>> +		 */
> 
> Please follow the multi-line commenting style.

Can do. However, these lines in fact follow the style that is used
throughout the entire file ;)

> There are some checkpatch checks that result from lines like this.
> Please fix these as well.
> 
> CHECK: Alignment should match open parenthesis
> #179: FILE: arch/arm/common/edma.c:1841:
> +			map_queue_tc(j, queue_tc_mapping[i][0],
> +					queue_tc_mapping[i][1]);

If you say so, even though I disagree with checkpatch.pl here. The above
is actually more readable, right? :)


Thanks,
Daniel

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sekhar Nori Nov. 27, 2013, 1:54 p.m. UTC | #6 
On Wednesday 27 November 2013 07:17 PM, Daniel Mack wrote:
> Hi Sekhar,
> 
> On 11/27/2013 02:35 PM, Sekhar Nori wrote:
>> On Monday 18 November 2013 03:49 AM, Daniel Mack wrote:
> 
>>> +static int edma_pm_suspend(struct device *dev)
>>> +{
>>> +	int j, r;
>>> +
>>> +	r = pm_runtime_get_sync(dev);
>>> +	if (IS_ERR_VALUE(r)) {
>>
>> So IS_ERR_VALUE() is only for functions which may return a negative
>> number outside of MAX_ERRNO as a success indication.
>> pm_runtime_get_sync() does not appear to be one of them so just use"
>>
>> if (r < 0) { .. }
> 
> That's true. Thanks for catching this, I'll fix it. However, grepping
> through the tree, there are quite a lot places where the same mistake is
> made.

Yes, this is a common fallacy. Russell cleaned up a bunch of these a
while back.

> 
>>> +		/* Map the channel to param entry if channel mapping logic
>>> +		 * exist
>>> +		 */
>>
>> Please follow the multi-line commenting style.
> 
> Can do. However, these lines in fact follow the style that is used
> throughout the entire file ;)

:) I did not compare the rest of the file, but hey the bar keep rising
all the time.

> 
>> There are some checkpatch checks that result from lines like this.
>> Please fix these as well.
>>
>> CHECK: Alignment should match open parenthesis
>> #179: FILE: arch/arm/common/edma.c:1841:
>> +			map_queue_tc(j, queue_tc_mapping[i][0],
>> +					queue_tc_mapping[i][1]);
> 
> If you say so, even though I disagree with checkpatch.pl here. The above
> is actually more readable, right? :)

In this particular case, I agree so I am okay if you keep it as is. The
rest of the two reports are valid though.

Thanks,
Sekhar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kevin Hilman Dec. 3, 2013, 6:24 p.m. UTC | #7 
Daniel Mack <zonque@gmail.com> writes:

> On 11/27/2013 02:22 PM, Sekhar Nori wrote:
>> + Kevin
>> 
>> On Monday 25 November 2013 11:04 PM, Joel Fernandes wrote:
>>> On 11/17/2013 04:19 PM, Daniel Mack wrote:
>>>> This patch makes the edma driver resume correctly after suspend. Tested
>>>> on an AM33xx platform with cyclic audio streams and omap_hsmmc.
>>>>
>>>> All information can be reconstructed by already known runtime
>>>> information.
>>>>
>>>> As we now use some functions that were previously only used from __init
>>>> context, annotations had to be dropped.
>>>>
>>>> [nm@ti.com: added error handling for runtime + suspend_late/early_resume]
>>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>>>> Tested-by: Joel Fernandes <joelf@ti.com>
>>>> Acked-by: Joel Fernandes <joelf@ti.com>
>>>
>>> Hi Sekhar,
>>>
>>> Can you consider pulling this patch? It has been tested and Acked. Thanks.
>> 
>> Kevin had some inputs on previous version of this patch. Were you able
>> to make sure he is okay with this version being merged?
>
> I had concerns about the feedback I got, and haven't got answers yet.
>
> In particular, I'm not convinced that using runtime PM to suspend
> channels would actually save any power during runtime, or have any other
> benefit. 

/me returning from a week off

The amount of power to be saved depends on the activity in the system.
If DMA is unused, at least the clocks could be gated allowing the
possibility of the enclosing power domain to be gated if other devices
are also clock gated, etc. etc.

However, my comments were not really about power saving, they were about
designing things in a way that are scalable and match the longer term
goals of converting drivers to be runtime PM centric.  For example, if
someone did want to add real runtime PM to this driver later, they would
need to rework much of this.  So my suggestion was to do runtime PM the
right way from the beginning.

That being said, I'm not the maintainer of this driver so don't get to
make the final call.  I will just say that from what I've seen here, I
don't think this is the right approach.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 8e1a024..e2b9638 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -239,6 +239,8 @@  struct edma {
 	/* list of channels with no even trigger; terminated by "-1" */
 	const s8	*noevent;
 
+	struct edma_soc_info *info;
+
 	/* The edma_inuse bit for each PaRAM slot is clear unless the
 	 * channel is in use ... by ARM or DSP, for QDMA, or whatever.
 	 */
@@ -290,13 +292,13 @@  static void map_dmach_queue(unsigned ctlr, unsigned ch_no,
 			~(0x7 << bit), queue_no << bit);
 }
 
-static void __init map_queue_tc(unsigned ctlr, int queue_no, int tc_no)
+static void map_queue_tc(unsigned ctlr, int queue_no, int tc_no)
 {
 	int bit = queue_no * 4;
 	edma_modify(ctlr, EDMA_QUETCMAP, ~(0x7 << bit), ((tc_no & 0x7) << bit));
 }
 
-static void __init assign_priority_to_queue(unsigned ctlr, int queue_no,
+static void assign_priority_to_queue(unsigned ctlr, int queue_no,
 		int priority)
 {
 	int bit = queue_no * 4;
@@ -315,7 +317,7 @@  static void __init assign_priority_to_queue(unsigned ctlr, int queue_no,
  * included in that particular EDMA variant (Eg : dm646x)
  *
  */
-static void __init map_dmach_param(unsigned ctlr)
+static void map_dmach_param(unsigned ctlr)
 {
 	int i;
 	for (i = 0; i < EDMA_MAX_DMACH; i++)
@@ -1785,15 +1787,101 @@  static int edma_probe(struct platform_device *pdev)
 			edma_write_array2(j, EDMA_DRAE, i, 1, 0x0);
 			edma_write_array(j, EDMA_QRAE, i, 0x0);
 		}
+		edma_cc[j]->info = info[j];
 		arch_num_cc++;
 	}
 
 	return 0;
 }
 
+static int edma_pm_suspend(struct device *dev)
+{
+	int j, r;
+
+	r = pm_runtime_get_sync(dev);
+	if (IS_ERR_VALUE(r)) {
+		dev_err(dev, "%s: get_sync returned %d\n", __func__, r);
+		return r;
+	}
+
+	for (j = 0; j < arch_num_cc; j++) {
+		struct edma *ecc = edma_cc[j];
+
+		disable_irq(ecc->irq_res_start);
+		disable_irq(ecc->irq_res_end);
+	}
+
+	pm_runtime_put_sync(dev);
+
+	return 0;
+}
+
+static int edma_pm_resume(struct device *dev)
+{
+	int i, j, r;
+
+	r = pm_runtime_get_sync(dev);
+	if (IS_ERR_VALUE(r)) {
+		dev_err(dev, "%s: get_sync returned %d\n", __func__, r);
+		return r;
+	}
+
+	for (j = 0; j < arch_num_cc; j++) {
+		struct edma *cc = edma_cc[j];
+
+		s8 (*queue_priority_mapping)[2];
+		s8 (*queue_tc_mapping)[2];
+
+		queue_tc_mapping = cc->info->queue_tc_mapping;
+		queue_priority_mapping = cc->info->queue_priority_mapping;
+
+		/* Event queue to TC mapping */
+		for (i = 0; queue_tc_mapping[i][0] != -1; i++)
+			map_queue_tc(j, queue_tc_mapping[i][0],
+					queue_tc_mapping[i][1]);
+
+		/* Event queue priority mapping */
+		for (i = 0; queue_priority_mapping[i][0] != -1; i++)
+			assign_priority_to_queue(j,
+						queue_priority_mapping[i][0],
+						queue_priority_mapping[i][1]);
+
+		/* Map the channel to param entry if channel mapping logic
+		 * exist
+		 */
+		if (edma_read(j, EDMA_CCCFG) & CHMAP_EXIST)
+			map_dmach_param(j);
+
+		for (i = 0; i < cc->num_channels; i++) {
+			if (test_bit(i, cc->edma_inuse)) {
+				/* ensure access through shadow region 0 */
+				edma_or_array2(j, EDMA_DRAE, 0, i >> 5,
+						BIT(i & 0x1f));
+
+				setup_dma_interrupt(i,
+						    cc->intr_data[i].callback,
+						    cc->intr_data[i].data);
+			}
+		}
+
+		enable_irq(cc->irq_res_start);
+		enable_irq(cc->irq_res_end);
+	}
+
+	pm_runtime_put_sync(dev);
+
+	return 0;
+}
+
+static const struct dev_pm_ops edma_pm_ops = {
+	.suspend_late	= edma_pm_suspend,
+	.resume_early	= edma_pm_resume,
+};
+
 static struct platform_driver edma_driver = {
 	.driver = {
 		.name	= "edma",
+		.pm	= &edma_pm_ops,
 		.of_match_table = edma_of_ids,
 	},
 	.probe = edma_probe,