Skip to contentSkip to conversation

ARM: omap: edma: add suspend suspend/resume hooks

PATCH Superseded Daniel Mack 6 participants 21 messages Oct 30, 2013 – Nov 17, 2013 Arm (32-bit)

v4 of 9 Superseded Latest is v9 →

lore
Share & tools
lore
#0 lore raw

PATCH v4 1/1 ARM: omap: edma: add suspend suspend/resume hooks

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.

---
There was actually only a v3 ever, I made a mistake when formating the first version of this patch. To prevent confusion though, I named this one v4.

v3 -> v4:
* dropped extra allocations, and reconstruct register values from already known driver states.

Hi Joel, Gururaja, Balaji,

thanks a lot for your feedback. I successfully tested this version with davinci mcasp as well as omap_hsmmc. I'd appreciate another round of reviews :)

Thanks,
Daniel

 arch/arm/common/edma.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 79 insertions(+), 3 deletions(-)
arch/arm/common/edma.c +79 -3
index 8e1a024..f15cdb9 100644
@@ -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,89 @@ 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;
+
+	pm_runtime_get_sync(dev);
+
+	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;
+
+	pm_runtime_get_sync(dev);
+
+	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 SIMPLE_DEV_PM_OPS(edma_pm_ops, edma_pm_suspend, 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,
Signature · 1 line
1.8.3.1
#1 lore raw

PATCH v4 1/1 ARM: omap: edma: add suspend suspend/resume hooks

Hi Daniel,

On Wed, Oct 30, 2013 at 4:21 PM, Daniel Mack <[email protected]> wrote:
[...]

arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1874

Quoted 9 lines
arch/arm/common/edma.c +4 -0
+
+static SIMPLE_DEV_PM_OPS(edma_pm_ops, edma_pm_suspend, edma_pm_resume);
+
 static struct platform_driver edma_driver = {
        .driver = {
                .name   = "edma",
+               .pm     = &edma_pm_ops,
                .of_match_table = edma_of_ids,
        },

A while back we discovered a nasty race condition here that had us move the EDMA PM callbacks to the noirq phase. IIRC the MMC driver was resuming before the EDMA
driver had a chance to run and that was leading to a deadlock. I am not sure how to force
this scenario but i do remember spending time debugging this on a random codebase.
Maybe some else has some better ideas on how to force this race condition...

I think logically it makes sense to have the DMA controllers in any system resume before
any of the client drivers. Maybe a set of syscore_ops similar to the timekeeping stuff needs
to be added (in the dmaengine framework?) to get this right.

Regards,
Vaibhav

#2 lore raw

PATCH v4 1/1 ARM: omap: edma: add suspend suspend/resume hooks

Hi Vaibhav,

Quoted 14 lines · Vaibhav Bedia

Hi Daniel,

On Wed, Oct 30, 2013 at 4:21 PM, Daniel Mack <[email protected]> wrote:
[...]

arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1874

Quoted 9 lines
arch/arm/common/edma.c +4 -0
+
+static SIMPLE_DEV_PM_OPS(edma_pm_ops, edma_pm_suspend, edma_pm_resume);
+
 static struct platform_driver edma_driver = {
        .driver = {
                .name   = "edma",
+               .pm     = &edma_pm_ops,
                .of_match_table = edma_of_ids,
        },
Quoted 8 lines

A while back we discovered a nasty race condition here that had us move the EDMA PM callbacks to the noirq phase. IIRC the MMC driver was resuming before the EDMA
driver had a chance to run and that was leading to a deadlock. I am not sure how to force
this scenario but i do remember spending time debugging this on a random codebase.
Maybe some else has some better ideas on how to force this race condition...

I think you're talking about the patch at [1] which is not upstream. A quick question with my limited knowledge of suspend/resume- How can there be pending I/O operations between suspend/resume cycles? The sync is done before suspend, so I don't understand how one is receiving a response from the card after resume before EDMA can resume? I'd imagine that until all devices are resumed, there will be no I/O operation issued. Let me know your thoughts.

thanks,

-Joel

[1]
https://www.gitorious.org/x0148406-public/linux-kernel/commit/b81bf04091986fa3893f31955564594567be3b61

#3 lore raw

PATCH v4 1/1 ARM: omap: edma: add suspend suspend/resume hooks

Quoted 26 lines · Joel Fernandes

Hi Vaibhav,

Quoted 14 lines · Vaibhav Bedia

Hi Daniel,

On Wed, Oct 30, 2013 at 4:21 PM, Daniel Mack <[email protected]> wrote:
[...]

arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1874

Quoted 9 lines
arch/arm/common/edma.c +4 -0
+
+static SIMPLE_DEV_PM_OPS(edma_pm_ops, edma_pm_suspend, edma_pm_resume);
+
 static struct platform_driver edma_driver = {
        .driver = {
                .name   = "edma",
+               .pm     = &edma_pm_ops,
                .of_match_table = edma_of_ids,
        },
Quoted 8 lines

A while back we discovered a nasty race condition here that had us move the EDMA PM callbacks to the noirq phase. IIRC the MMC driver was resuming before the EDMA
driver had a chance to run and that was leading to a deadlock. I am not sure how to force
this scenario but i do remember spending time debugging this on a random codebase.
Maybe some else has some better ideas on how to force this race condition...

Quoted 3 lines

I think you're talking about the patch at [1] which is not upstream. A quick question with my limited knowledge of suspend/resume- How can there be pending I/O operations between suspend/resume cycles?

AFAIK, MMC framework started talking to cards immediately after resume. Due to race condition, EDMA resume callback had not yet completed and HSMMC driver had initiated a DMA operation. This resulted in Deadlock.

regards
Gururaja

Quoted 12 lines

The sync is done before suspend,
so I don't understand how one is receiving a response from the card after resume before EDMA can resume? I'd imagine that until all devices are resumed, there will be no I/O operation issued. Let me know your thoughts.

thanks,

-Joel

[1]
https://www.gitorious.org/x0148406-public/linux-kernel/commit/b81bf04091986fa3893f31955564594567be3b61

#4 lore raw

PATCH v4 1/1 ARM: omap: edma: add suspend suspend/resume hooks

Quoted 27 lines · Gururaja Hebbar
Quoted 26 lines · Joel Fernandes

Hi Vaibhav,

Quoted 22 lines · Vaibhav Bedia

Hi Daniel,

On Wed, Oct 30, 2013 at 4:21 PM, Daniel Mack <[email protected]> wrote:
[...]

arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1874

Quoted 9 lines
arch/arm/common/edma.c +4 -0
+
+static SIMPLE_DEV_PM_OPS(edma_pm_ops, edma_pm_suspend, edma_pm_resume);
+
 static struct platform_driver edma_driver = {
        .driver = {
                .name   = "edma",
+               .pm     = &edma_pm_ops,
                .of_match_table = edma_of_ids,
        },

A while back we discovered a nasty race condition here that had us move the EDMA PM callbacks to the noirq phase. IIRC the MMC driver was resuming before the EDMA
driver had a chance to run and that was leading to a deadlock. I am not sure how to force
this scenario but i do remember spending time debugging this on a random codebase.
Maybe some else has some better ideas on how to force this race condition...

Quoted 7 lines
Quoted 3 lines

I think you're talking about the patch at [1] which is not upstream. A quick question with my limited knowledge of suspend/resume- How can there be pending I/O operations between suspend/resume cycles?

AFAIK, MMC framework started talking to cards immediately after resume. Due to race condition, EDMA resume callback had not yet completed and HSMMC driver had initiated a DMA operation. This resulted in Deadlock.

Hmm. At least in my case, that doesn't happen. And my debug logs also show that the calls are in expected order. Which tree were you on when you saw this?

Thanks,
Daniel

#5 lore raw

PATCH v4 1/1 ARM: omap: edma: add suspend suspend/resume hooks

Quoted 32 lines · Daniel Mack
Quoted 31 lines · Gururaja Hebbar
Quoted 29 lines · Joel Fernandes

Hi Vaibhav,

Quoted 14 lines · Vaibhav Bedia

Hi Daniel,

On Wed, Oct 30, 2013 at 4:21 PM, Daniel Mack <[email protected]> wrote:
[...]

arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1874

Quoted 9 lines
arch/arm/common/edma.c +4 -0
+
+static SIMPLE_DEV_PM_OPS(edma_pm_ops, edma_pm_suspend, edma_pm_resume);
+
 static struct platform_driver edma_driver = {
        .driver = {
                .name   = "edma",
+               .pm     = &edma_pm_ops,
                .of_match_table = edma_of_ids,
        },
Quoted 8 lines

A while back we discovered a nasty race condition here that had us move the EDMA PM callbacks to the noirq phase. IIRC the MMC driver was resuming before the EDMA
driver had a chance to run and that was leading to a deadlock. I am not sure how to force
this scenario but i do remember spending time debugging this on a random codebase.
Maybe some else has some better ideas on how to force this race condition...

I think you're talking about the patch at [1] which is not upstream. A quick question with my limited knowledge of suspend/resume- How can there be pending I/O operations between suspend/resume cycles?

Quoted 7 lines
Quoted 3 lines

AFAIK, MMC framework started talking to cards immediately after resume. Due to race condition, EDMA resume callback had not yet completed and HSMMC driver had initiated a DMA operation. This resulted in Deadlock.

Hmm. At least in my case, that doesn't happen. And my debug logs also show that the calls are in expected order. Which tree were you on when you saw this?

Tested this on a vendor V3.12 tag based kernel:

Test patch: http://pastebin.com/AmnktQ7B
test: echo -n "1">/sys/power/pm_print_times; rtcwake -d /dev/rtc0 -m mem -s 5

with the current patch: http://pastebin.com/RujarRLV
suspend_late and resume_early: http://pastebin.com/RujarRLV
suspend_noirq and resume_noirq: http://pastebin.com/nKfbm7Mj

one needs to be careful of the sequence - donot forget that omap_device also does stuff in the background to every SoC device in noirq - sequence is paramount. you would want to ensure edma is saving after every single dependent device is done with it's stuff and guarenteed to never request any further transaction, and resume is done before any of the dependent devices need edma. but edma is also a peripheral that omap_device and generic runtime pm framework deals with - so ensure sequences consider that as well.

Signature · 2 lines
Regards,
Nishanth Menon
#6 lore raw

PATCH v4 1/1 ARM: omap: edma: add suspend suspend/resume hooks

Quoted 9 lines · Nishanth Menon

Tested this on a vendor V3.12 tag based kernel:

Test patch: http://pastebin.com/AmnktQ7B
test: echo -n "1">/sys/power/pm_print_times; rtcwake -d /dev/rtc0 -m mem -s 5

with the current patch: http://pastebin.com/RujarRLV
suspend_late and resume_early: http://pastebin.com/RujarRLV

These two are identical.

Quoted 1 line

suspend_noirq and resume_noirq: http://pastebin.com/nKfbm7Mj

And I can't see any difference between this one and the first one, except for slightly different timings. Am I missing anything?

Quoted 8 lines

one needs to be careful of the sequence - donot forget that omap_device also does stuff in the background to every SoC device in noirq - sequence is paramount. you would want to ensure edma is saving after every single dependent device is done with it's stuff and guarenteed to never request any further transaction, and resume is done before any of the dependent devices need edma. but edma is also a peripheral that omap_device and generic runtime pm framework deals with - so ensure sequences consider that as well.

So, what would you say which sequence is correct then? :)

Thanks,
Daniel

#7 lore raw

PATCH v4 1/1 ARM: omap: edma: add suspend suspend/resume hooks

Quoted 11 lines · Daniel Mack
Quoted 9 lines · Nishanth Menon

Tested this on a vendor V3.12 tag based kernel:

Test patch: http://pastebin.com/AmnktQ7B
test: echo -n "1">/sys/power/pm_print_times; rtcwake -d /dev/rtc0 -m mem -s 5

with the current patch: http://pastebin.com/RujarRLV
suspend_late and resume_early: http://pastebin.com/RujarRLV

Quoted 6 lines

These two are identical.

Quoted 1 line

suspend_noirq and resume_noirq: http://pastebin.com/nKfbm7Mj

And I can't see any difference between this one and the first one, except for slightly different timings. Am I missing anything?

aah, that happens to be a little key :)
if you see the current patch, it happens around line 417, with suspend_late, it moves deeper(or later) into suspend around 738 with noirq - it is as late as it can get - around line 823 just before the last set of arch suspend calls are done.

Quoted 10 lines
Quoted 8 lines

one needs to be careful of the sequence - donot forget that omap_device also does stuff in the background to every SoC device in noirq - sequence is paramount. you would want to ensure edma is saving after every single dependent device is done with it's stuff and guarenteed to never request any further transaction, and resume is done before any of the dependent devices need edma. but edma is also a peripheral that omap_device and generic runtime pm framework deals with - so ensure sequences consider that as well.

So, what would you say which sequence is correct then? :)

Disclaimer: I have not dug deeper, other than a cursory look. With proper error handling, proper split between suspend and suspend_late seems appropriate to me(at noirq, runtime_get could fail as pmruntime is disabled as part of suspend sequence and could race with omap_device handling in a future cleanup there) - with the assumption that all drivers that are using things have cleaned up prior to that. edma is a generic engine that many drivers may choose to use - example of MMC used in this discussion is just one of other potential users - for a generic driver like dma, you'd want to stay as deep in the suspend as possible. you may also want to ensure that further calls will not succeed until resume is invoked.

Signature · 2 lines
Regards,
Nishanth Menon
#8 lore raw

PATCH v4 1/1 ARM: omap: edma: add suspend suspend/resume hooks

Hi Nishanth :)

Quoted 16 lines · Nishanth Menon
Quoted 15 lines · Daniel Mack
Quoted 9 lines · Nishanth Menon

Tested this on a vendor V3.12 tag based kernel:

Test patch: http://pastebin.com/AmnktQ7B
test: echo -n "1">/sys/power/pm_print_times; rtcwake -d /dev/rtc0 -m mem -s 5

with the current patch: http://pastebin.com/RujarRLV
suspend_late and resume_early: http://pastebin.com/RujarRLV

These two are identical.

Quoted 1 line

suspend_noirq and resume_noirq: http://pastebin.com/nKfbm7Mj

Quoted 9 lines
Quoted 2 lines

And I can't see any difference between this one and the first one, except for slightly different timings. Am I missing anything?

aah, that happens to be a little key :)
if you see the current patch, it happens around line 417, with suspend_late, it moves deeper(or later) into suspend around 738 with noirq - it is as late as it can get - around line 823 just before the last set of arch suspend calls are done.

That's some nifty analysis overnight ;)

Yeah, the intention was to move the EDMA ops as late as possible. I am not sure if noirq thing takes care of the late i2c stuff to talk to the PMICs that happens on some OMAPs. Maybe the generic PM code handles that already but i am a bit rusty on the details right now so that would just mention that scenario to be considered.

Quoted 11 lines
Quoted 10 lines
Quoted 8 lines

one needs to be careful of the sequence - donot forget that omap_device also does stuff in the background to every SoC device in noirq - sequence is paramount. you would want to ensure edma is saving after every single dependent device is done with it's stuff and guarenteed to never request any further transaction, and resume is done before any of the dependent devices need edma. but edma is also a peripheral that omap_device and generic runtime pm framework deals with - so ensure sequences consider that as well.

So, what would you say which sequence is correct then? :)

Quoted 12 lines

Disclaimer: I have not dug deeper, other than a cursory look. With proper error handling, proper split between suspend and suspend_late seems appropriate to me(at noirq, runtime_get could fail as pmruntime is disabled as part of suspend sequence and could race with omap_device handling in a future cleanup there) - with the assumption that all drivers that are using things have cleaned up prior to that. edma is a generic engine that many drivers may choose to use - example of MMC used in this discussion is just one of other potential users - for a generic driver like dma, you'd want to stay as deep in the suspend as possible. you may also want to ensure that further calls will not succeed until resume is invoked.

Hmm doesn't this highlight the problem that issues like this could happen on any platform. If so, shouldn't we have a generic callback (dmaengine layer?) to do this right? For timers we have syscore_ops, is something like that for a DMA controller an overkill?

Regards,
Vaibhav

#9 lore raw

PATCH v4 1/1 ARM: omap: edma: add suspend suspend/resume hooks

Quoted 28 lines · Vaibhav Bedia

Hi Nishanth :)

Quoted 16 lines · Nishanth Menon
Quoted 15 lines · Daniel Mack
Quoted 9 lines · Nishanth Menon

Tested this on a vendor V3.12 tag based kernel:

Test patch: http://pastebin.com/AmnktQ7B
test: echo -n "1">/sys/power/pm_print_times; rtcwake -d /dev/rtc0 -m mem -s 5

with the current patch: http://pastebin.com/RujarRLV
suspend_late and resume_early: http://pastebin.com/RujarRLV

These two are identical.

Quoted 1 line

suspend_noirq and resume_noirq: http://pastebin.com/nKfbm7Mj

Quoted 9 lines
Quoted 2 lines

And I can't see any difference between this one and the first one, except for slightly different timings. Am I missing anything?

aah, that happens to be a little key :)
if you see the current patch, it happens around line 417, with suspend_late, it moves deeper(or later) into suspend around 738 with noirq - it is as late as it can get - around line 823 just before the last set of arch suspend calls are done.

Quoted 8 lines

That's some nifty analysis overnight ;)

Yeah, the intention was to move the EDMA ops as late as possible. I am not sure if noirq thing takes care of the late i2c stuff to talk to the PMICs that happens on some OMAPs. Maybe the generic PM code handles that already but i am a bit rusty on the details right now so that would just mention that scenario to be considered.

To trigger the fail, I created a custom Test case on TI vendor kernel based on v3.12 tag:
On beagleBone-Black
test scenario (BBB):

Boot from SD card
ensure firmware is loaded (rev 0x182)
run LTP fsstress [1] in background on EMMC
mkdir -p /tmp/testing
mke2fs /dev/mmcblk1p1
mount /dev/mmcblk1p1 /tmp/testing
fsstress -d /tmp/testing p 4 -n 1000 -l0 -v >/dev/null &
run ping in the background (to add yet another interface) run memtester[2] (70% of free memory)
memtester 343M >/dev/null &
sleep 10 (to allow memtester to start up properly)
start=date;i=0; while [ 1 ]; do rtcwake -d /dev/rtc0 -m mem -s
2; i=$((i + 1)); echo "$i: start =$start, now="date; sleep 2; done

Eventual hang log (using the regular suspend-resume): [3] - took close to two days of testing to trigger this.

Moving to a suspend_late and resume_early such as in [4], it passed the test for over 4 days now.

Daniel,
will be good if you could post [4] for comments if you think that is the right thing to do and helps solve the issue you saw as well.

[1]
https://github.com/linux-test-project/ltp/tree/master/testcases/kernel/fs/fsstress

[2] http://pyropus.ca/software/memtester
[3] http://pastebin.pandaboard.org/index.php/view/18307529
[4]
https://git.ti.com/ti-linux-kernel/ti-linux-kernel/commit/f8f9a8c38644d27dc8671009209922531b072110

Signature · 2 lines
Regards,
Nishanth Menon
#10 lore raw

PATCH v4 1/1 ARM: omap: edma: add suspend suspend/resume hooks

Hi Nishanth,

Quoted 19 lines · Nishanth Menon

To trigger the fail, I created a custom Test case on TI vendor kernel based on v3.12 tag:
On beagleBone-Black
test scenario (BBB):

Boot from SD card
ensure firmware is loaded (rev 0x182)
run LTP fsstress [1] in background on EMMC
mkdir -p /tmp/testing
mke2fs /dev/mmcblk1p1
mount /dev/mmcblk1p1 /tmp/testing
fsstress -d /tmp/testing p 4 -n 1000 -l0 -v >/dev/null &
run ping in the background (to add yet another interface) run memtester[2] (70% of free memory)
memtester 343M >/dev/null &
sleep 10 (to allow memtester to start up properly)
start=date;i=0; while [ 1 ]; do rtcwake -d /dev/rtc0 -m mem -s
2; i=$((i + 1)); echo "$i: start =$start, now="date; sleep 2; done

Quoted 5 lines

Eventual hang log (using the regular suspend-resume): [3] - took close to two days of testing to trigger this.

Moving to a suspend_late and resume_early such as in [4], it passed the test for over 4 days now.

Wow, that's an intense test that you have there :)

Quoted 3 lines

Daniel,
will be good if you could post [4] for comments if you think that is the right thing to do and helps solve the issue you saw as well.

Alright, will do!

Thanks a lot,
Daniel

#11 lore raw

PATCH v4 1/1 ARM: omap: edma: add suspend suspend/resume hooks

In reply to Joel Fernandes

Hi Joel,

Quoted 26 lines · Joel Fernandes

Hi Vaibhav,

Quoted 14 lines · Vaibhav Bedia

Hi Daniel,

On Wed, Oct 30, 2013 at 4:21 PM, Daniel Mack <[email protected]> wrote:
[...]

arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1874

Quoted 9 lines
arch/arm/common/edma.c +4 -0
+
+static SIMPLE_DEV_PM_OPS(edma_pm_ops, edma_pm_suspend, edma_pm_resume);
+
 static struct platform_driver edma_driver = {
        .driver = {
                .name   = "edma",
+               .pm     = &edma_pm_ops,
                .of_match_table = edma_of_ids,
        },
Quoted 8 lines

A while back we discovered a nasty race condition here that had us move the EDMA PM callbacks to the noirq phase. IIRC the MMC driver was resuming before the EDMA
driver had a chance to run and that was leading to a deadlock. I am not sure how to force
this scenario but i do remember spending time debugging this on a random codebase.
Maybe some else has some better ideas on how to force this race condition...

Quoted 1 line

I think you're talking about the patch at [1] which is not upstream. A quick

Yeah that's the one.

Quoted 6 lines

question with my limited knowledge of suspend/resume- How can there be pending I/O operations between suspend/resume cycles? The sync is done before suspend, so I don't understand how one is receiving a response from the card after resume before EDMA can resume? I'd imagine that until all devices are resumed, there will be no I/O operation issued. Let me know your thoughts.

Sadly the commit message does not capture the details to the level it should have.

Quoted 1 line

From what i remember, during the resume operation the driver was waiting for the

response of the first cmd that it sends and that never happened since the driver was setup to use DMA mode and the EDMA driver hadn't resumed. Ideally such issues should have been noticed a long time back. One more thing to note is that the MMC controller on AMx series does not have the internal DMA like OMAPx. However i am not sure how much that, if at all, plays a role here.

Maybe someone internally still has a copy of the analysis done and you could try to hunt that down for more details. In case you do it would be good to have it
mentioned here.

Regards,
Vaibhav

#12 lore raw

PATCH v4 1/1 ARM: omap: edma: add suspend suspend/resume hooks

In reply to Daniel Mack

On 10/30/2013 03:21 PM, Daniel Mack wrote:
[...]

Quoted 4 lines
arch/arm/common/edma.c +0 -0
index 8e1a024..f15cdb9 100644

[...]

arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1802

Quoted 6 lines
arch/arm/common/edma.c +6 -0
+static int edma_pm_suspend(struct device *dev)
+{
+	int j;
+
+	pm_runtime_get_sync(dev);
+

error checks?

Quoted 7 lines
arch/arm/common/edma.c +7 -0
+	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);
+	}
+

arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1819

Quoted 10 lines
arch/arm/common/edma.c +10 -0
+	pm_runtime_put_sync(dev);
+
+	return 0;
+}
+
+static int edma_pm_resume(struct device *dev)
+{
+	int i, j;
+
+	pm_runtime_get_sync(dev);

error checks?

[...]

Signature · 2 lines
Regards,
Nishanth Menon
#13 lore raw

PATCH v4 1/1 ARM: omap: edma: add suspend suspend/resume hooks

Quoted 16 lines · Nishanth Menon

On 10/30/2013 03:21 PM, Daniel Mack wrote:
[...]

Quoted 4 lines
arch/arm/common/edma.c +0 -0
index 8e1a024..f15cdb9 100644

[...]

arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1802

Quoted 6 lines
arch/arm/common/edma.c +6 -0
+static int edma_pm_suspend(struct device *dev)
+{
+	int j;
+
+	pm_runtime_get_sync(dev);
+

error checks?

Quoted 8 lines

arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1810

Quoted 8 lines
arch/arm/common/edma.c +8 -0
+	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);

Also, it make no sense to call pm_runtime_putXXX() in suspend handlers
- it will not work because PM core calls pm_runtime_get_noresume() from device_prepare().

Now, for OMAP the edma Device will be finally disabled by OMAP device core at suspend_noirq stage (if it's connected to HW_mode). But, in other cases it need to be handled somehow :)

Quoted 14 lines

arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1819

Quoted 9 lines
arch/arm/common/edma.c +9 -0
+
+	return 0;
+}
+
+static int edma_pm_resume(struct device *dev)
+{
+	int i, j;
+
+	pm_runtime_get_sync(dev);

error checks?

[...]

#14 lore raw

PATCH v4 1/1 ARM: omap: edma: add suspend suspend/resume hooks

Hi Grygorii :)

Quoted 36 lines · Grygorii Strashko
Quoted 21 lines · Nishanth Menon

On 10/30/2013 03:21 PM, Daniel Mack wrote:
[...]

Quoted 5 lines
arch/arm/common/edma.c +0 -0
index 8e1a024..f15cdb9 100644

[...]

arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1802

Quoted 6 lines
arch/arm/common/edma.c +6 -0
+static int edma_pm_suspend(struct device *dev)
+{
+       int j;
+
+       pm_runtime_get_sync(dev);
+

error checks?

Quoted 1 line
Quoted 8 lines

arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1810

Quoted 8 lines
arch/arm/common/edma.c +8 -0
+       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);

Also, it make no sense to call pm_runtime_putXXX() in suspend handlers
- it will not work because PM core calls pm_runtime_get_noresume() from device_prepare().

Quoted 4 lines

Now, for OMAP the edma Device will be finally disabled by OMAP device core at suspend_noirq stage (if it's connected to HW_mode).
But, in other cases it need to be handled somehow :)

This PM stuff just gets more interesting ;)

#15 lore raw

PATCH v4 1/1 ARM: omap: edma: add suspend suspend/resume hooks

In reply to Daniel Mack

Hi Daniel,

Thanks for your followup patch on this. It looks much better now using existing functions to save/restore the state.

Quoted 35 lines · Daniel Mack

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.

---
There was actually only a v3 ever, I made a mistake when formating the first version of this patch. To prevent confusion though, I named this one v4.

v3 -> v4:
* dropped extra allocations, and reconstruct register values from already known driver states.

Hi Joel, Gururaja, Balaji,

thanks a lot for your feedback. I successfully tested this version with davinci mcasp as well as omap_hsmmc. I'd appreciate another round of reviews :)

Thanks,
Daniel

 arch/arm/common/edma.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 79 insertions(+), 3 deletions(-)
arch/arm/common/edma.c +0 -0

[..]

Quoted 15 lines
arch/arm/common/edma.c +15 -0
+static int edma_pm_resume(struct device *dev)
+{
+	int i, j;
+
+	pm_runtime_get_sync(dev);
+
+	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;
+

arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1839

Quoted 10 lines
arch/arm/common/edma.c +10 -0
+		/* 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]);

I know ti,edma-regions property is not currently being used, but we should future proof this by setting up DRAE for like done in probe:

                for (i = 0; i < info[j]->n_region; i++) {
                        edma_write_array2(j, EDMA_DRAE, i, 0, 0x0);
                        edma_write_array2(j, EDMA_DRAE, i, 1, 0x0);
                        edma_write_array(j, EDMA_QRAE, i, 0x0);
                }
Quoted 18 lines
arch/arm/common/edma.c +18 -0
+
+		/* 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);
+			}
+

arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1867

Quoted 10 lines
arch/arm/common/edma.c +10 -0
+		enable_irq(cc->irq_res_start);
+		enable_irq(cc->irq_res_end);
+	}
+
+	pm_runtime_put_sync(dev);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(edma_pm_ops, edma_pm_suspend, edma_pm_resume);

I agree with Nishanth here, it is better to do this in .suspend/resume _noirq stage to rule out any ordering bugs that may show up in the future, since such an issue already showed up in earlier testing.

I would appreciate it if you can make these 2 changes and post a v5. Thanks for a lot for all the hardwork.

regards,

-Joel

#16 lore raw

PATCH v4 1/1 ARM: omap: edma: add suspend suspend/resume hooks

Quoted 71 lines · Joel Fernandes

Hi Daniel,

Thanks for your followup patch on this. It looks much better now using existing functions to save/restore the state.

Quoted 35 lines · Daniel Mack

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.

---
There was actually only a v3 ever, I made a mistake when formating the first version of this patch. To prevent confusion though, I named this one v4.

v3 -> v4:
* dropped extra allocations, and reconstruct register values from already known driver states.

Hi Joel, Gururaja, Balaji,

thanks a lot for your feedback. I successfully tested this version with davinci mcasp as well as omap_hsmmc. I'd appreciate another round of reviews :)

Thanks,
Daniel

 arch/arm/common/edma.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 79 insertions(+), 3 deletions(-)
arch/arm/common/edma.c +0 -0

[..]

Quoted 15 lines
arch/arm/common/edma.c +15 -0
+static int edma_pm_resume(struct device *dev)
+{
+	int i, j;
+
+	pm_runtime_get_sync(dev);
+
+	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;
+

arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1839

Quoted 10 lines
arch/arm/common/edma.c +10 -0
+		/* 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]);

I know ti,edma-regions property is not currently being used, but we should future proof this by setting up DRAE for like done in probe:

Quoted 5 lines
                for (i = 0; i < info[j]->n_region; i++) {
                        edma_write_array2(j, EDMA_DRAE, i, 0, 0x0);
                        edma_write_array2(j, EDMA_DRAE, i, 1, 0x0);
                        edma_write_array(j, EDMA_QRAE, i, 0x0);
                }

Please ignore this comment I posted earlier. That is not all required to do. Should've looked closer. Sorry about it.

cheers,

-Joel

#17 lore raw

PATCH v4 1/1 ARM: omap: edma: add suspend suspend/resume hooks

In reply to Joel Fernandes

Hi Joel,

Quoted 2 lines · Joel Fernandes

Thanks for your followup patch on this. It looks much better now using existing functions to save/restore the state.

Yes, thanks for the suggesting it in the first place.

Quoted 32 lines · Daniel Mack
Quoted 1 line
arch/arm/common/edma.c +0 -0

[..]

Quoted 15 lines
arch/arm/common/edma.c +15 -0
+static int edma_pm_resume(struct device *dev)
+{
+	int i, j;
+
+	pm_runtime_get_sync(dev);
+
+	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;
+

arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1839

Quoted 10 lines
arch/arm/common/edma.c +10 -0
+		/* 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]);

I know ti,edma-regions property is not currently being used, but we should future proof this by setting up DRAE for like done in probe:

Quoted 5 lines
                for (i = 0; i < info[j]->n_region; i++) {
                        edma_write_array2(j, EDMA_DRAE, i, 0, 0x0);
                        edma_write_array2(j, EDMA_DRAE, i, 1, 0x0);
                        edma_write_array(j, EDMA_QRAE, i, 0x0);
                }

That doesn't work for me. I'm running long-time tests here on a device which has a mwifiex connected to omap_hsmmc. The test procedure includes:

a) a script on the device that puts the device to sleep some seconds after it has been woken up

b) a script on a host that wakes up the device with wake-on-lan every 10 seconds

c) a flood ping that checks whether the device is responding

That precedure is running since a couple of hourse here, and it works well with both by v3 and v4 patches. Moving the functions to .suspend/resume _noirq doesn't seem to break anything.

Setting QRAE to 0 as you mentioned above, however, makes the device fail at resume.

Quoted 5 lines
Quoted 1 line

+static SIMPLE_DEV_PM_OPS(edma_pm_ops, edma_pm_suspend, edma_pm_resume);

I agree with Nishanth here, it is better to do this in .suspend/resume _noirq stage to rule out any ordering bugs that may show up in the future, since such an issue already showed up in earlier testing.

Alright, I already did that.

Quoted 2 lines

I would appreciate it if you can make these 2 changes and post a v5. Thanks for a lot for all the hardwork.

No problem at all :)

Quoted 1 line

Still sure about that? What about your follow-up to your own reply?

Many thanks for all the feedback!

Daniel

#18 lore raw

PATCH v4 1/1 ARM: omap: edma: add suspend suspend/resume hooks

On 11/07/2013 11:37 AM, Daniel Mack wrote:
[..]

Quoted 1 line
Quoted 1 line

I know ti,edma-regions property is not currently being used, but we should>>

future proof this by setting up DRAE for like done in probe:

Quoted 19 lines
Quoted 6 lines
                for (i = 0; i < info[j]->n_region; i++) {
                        edma_write_array2(j, EDMA_DRAE, i, 0, 0x0);
                        edma_write_array2(j, EDMA_DRAE, i, 1, 0x0);
                        edma_write_array(j, EDMA_QRAE, i, 0x0);
                }

That doesn't work for me. I'm running long-time tests here on a device which has a mwifiex connected to omap_hsmmc. The test procedure includes:

a) a script on the device that puts the device to sleep some seconds after it has been woken up

b) a script on a host that wakes up the device with wake-on-lan every 10 seconds

c) a flood ping that checks whether the device is responding

Quoted 6 lines

That precedure is running since a couple of hourse here, and it works well with both by v3 and v4 patches. Moving the functions to .suspend/resume _noirq doesn't seem to break anything.

Setting QRAE to 0 as you mentioned above, however, makes the device fail at resume.

Yes, I recall that. It definitely shouldn't be done.

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

+static SIMPLE_DEV_PM_OPS(edma_pm_ops, edma_pm_suspend, edma_pm_resume);

I agree with Nishanth here, it is better to do this in .suspend/resume _noirq stage to rule out any ordering bugs that may show up in the future, since such an issue already showed up in earlier testing.

Alright, I already did that.

Cool, can you post the updated patch? I'll run some more tests too..

Quoted 9 lines
Quoted 2 lines

I would appreciate it if you can make these 2 changes and post a v5. Thanks for a lot for all the hardwork.

No problem at all :)

Quoted 1 line

Still sure about that? What about your follow-up to your own reply?

Yes, the Ack definitely stands for the changes you made since the last revision. Only thing left is to fix the ordering, Let's do that (you seem to have already done it) and test this a bit more.

Glad you're working on this, thanks!

-Joel

#19 lore raw

PATCH v4 1/1 ARM: omap: edma: add suspend suspend/resume hooks

In reply to Daniel Mack
Quoted 55 lines · Daniel Mack

Hi Joel,

Quoted 2 lines · Joel Fernandes

Thanks for your followup patch on this. It looks much better now using existing functions to save/restore the state.

Yes, thanks for the suggesting it in the first place.

Quoted 32 lines · Daniel Mack
Quoted 1 line
arch/arm/common/edma.c +0 -0

[..]

Quoted 15 lines
arch/arm/common/edma.c +15 -0
+static int edma_pm_resume(struct device *dev)
+{
+	int i, j;
+
+	pm_runtime_get_sync(dev);
+
+	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;
+

arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1839

Quoted 10 lines
arch/arm/common/edma.c +10 -0
+		/* 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]);

I know ti,edma-regions property is not currently being used, but we should future proof this by setting up DRAE for like done in probe:

Quoted 5 lines
                for (i = 0; i < info[j]->n_region; i++) {
                        edma_write_array2(j, EDMA_DRAE, i, 0, 0x0);
                        edma_write_array2(j, EDMA_DRAE, i, 1, 0x0);
                        edma_write_array(j, EDMA_QRAE, i, 0x0);
                }

That doesn't work for me. I'm running long-time tests here on a device which has a mwifiex connected to omap_hsmmc. The test procedure includes:

a) a script on the device that puts the device to sleep some seconds after it has been woken up

Quoted 4 lines

b) a script on a host that wakes up the device with wake-on-lan every 10 seconds

c) a flood ping that checks whether the device is responding

can you share above 2 (b & C) test scripts? (pastebin or inline if small)

thanks in advance

regards
Gururaja

Quoted 34 lines

That precedure is running since a couple of hourse here, and it works well with both by v3 and v4 patches. Moving the functions to .suspend/resume _noirq doesn't seem to break anything.

Setting QRAE to 0 as you mentioned above, however, makes the device fail at resume.

Quoted 5 lines
Quoted 1 line

+static SIMPLE_DEV_PM_OPS(edma_pm_ops, edma_pm_suspend, edma_pm_resume);

I agree with Nishanth here, it is better to do this in .suspend/resume _noirq stage to rule out any ordering bugs that may show up in the future, since such an issue already showed up in earlier testing.

Alright, I already did that.

Quoted 2 lines

I would appreciate it if you can make these 2 changes and post a v5. Thanks for a lot for all the hardwork.

No problem at all :)

Quoted 1 line

Still sure about that? What about your follow-up to your own reply?

Many thanks for all the feedback!

Daniel

#20 lore raw

PATCH v4 1/1 ARM: omap: edma: add suspend suspend/resume hooks

Quoted 8 lines · Gururaja Hebbar
Quoted 6 lines · Daniel Mack

I'm running long-time tests here on a device
which has a mwifiex connected to omap_hsmmc. The test procedure includes:

a) a script on the device that puts the device to sleep some seconds after it has been woken up

Quoted 6 lines
Quoted 4 lines

b) a script on a host that wakes up the device with wake-on-lan every 10 seconds

c) a flood ping that checks whether the device is responding

can you share above 2 (b & C) test scripts? (pastebin or inline if small)

Of course. It's very hand-baked and simple:

a)

# wpa_supplicant is started and and the ip address ist set

while [ true ]; do
ethtool -s eth0 wol g
echo mem >/sys/power/state
ethtool -s eth0 wol d
sleep 7
done

b)

while [ true ]; do

	sudo etherwake -i em1 BC:6A:29:84:0A:62
	sleep 5

done

c)

ping -s 32768 -i 0.1 192.168.178.222

Daniel