ARM: omap: edma: add suspend suspend/resume hooks
PATCH v3 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.
The code was shamelessly taken from an ancient BSP tree.
--- arch/arm/common/edma.c | 133 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 133 insertions(+)
arch/arm/common/edma.c +133 -0
@@ -258,6 +258,20 @@ struct edma { void *data);void *data; } intr_data[EDMA_MAX_DMACH ]; + +struct { +struct edmacc_param *prm_set; +unsigned int *ch_map;/* 64 registers */ +unsigned int *que_num;/* 8 registers */ +unsigned int sh_esr; +unsigned int sh_esrh; +unsigned int sh_eesr; +unsigned int sh_eesrh; +unsigned int sh_iesr; +unsigned int sh_iesrh; +unsigned int que_tc_map; +unsigned int que_pri; + } context; };static struct edma *edma_cc[EDMA_MAX_CC ];
@@ -1655,6 +1669,16 @@ static int edma_probe(struct platform_device *pdev)memcpy_toio (edmacc_regs_base[j] +PARM_OFFSET (i), &dummy_paramset,PARM_SIZE ); +/* resume context */ + edma_cc[j]->context.prm_set = +kzalloc ((sizeof (struct edmacc_param ) * + edma_cc[j]->num_slots),GFP_KERNEL ); + edma_cc[j]->context.ch_map = +kzalloc ((sizeof (unsigned int ) * + edma_cc[j]->num_channels),GFP_KERNEL ); + edma_cc[j]->context.que_num = +kzalloc ((sizeof (unsigned int ) *8 ),GFP_KERNEL ); +/* Mark all channels as unused */ memset (edma_cc[j]->edma_unused,0xff ,sizeof (edma_cc[j]->edma_unused));
@@ -1762,6 +1786,114 @@ static int edma_probe(struct platform_device *pdev)return 0 ; } +static int edma_pm_suspend (struct device *dev) +{ +int i, j; + +pm_runtime_get_sync (dev); + +for (i =0 ; i < arch_num_cc; i++) { +struct edma *ecc = edma_cc[i]; + +/* backup channel data */ +for (j =0 ; j < ecc->num_channels; j++) + ecc->context.ch_map[j] = +edma_read_array (i,EDMA_DCHMAP , j); + +/* backup DMA Queue Number */ +for (j =0 ; j <8 ; j++) + ecc->context.que_num[j] = +edma_read_array (i,EDMA_DMAQNUM , j); + +/* backup DMA shadow Event Set data */ + ecc->context.sh_esr =edma_shadow0_read_array (i,SH_ESR ,0 ); + ecc->context.sh_esrh =edma_shadow0_read_array (i,SH_ESR ,1 ); + +/* backup DMA Shadow Event Enable Set data */ + ecc->context.sh_eesr = +edma_shadow0_read_array (i,SH_EER ,0 ); + ecc->context.sh_eesrh = +edma_shadow0_read_array (i,SH_EER ,1 ); + +/* backup DMA Shadow Interrupt Enable Set data */ + ecc->context.sh_iesr = +edma_shadow0_read_array (i,SH_IER ,0 ); + ecc->context.sh_iesrh = +edma_shadow0_read_array (i,SH_IER ,1 ); + + ecc->context.que_tc_map =edma_read (i,EDMA_QUETCMAP ); + +/* backup DMA Queue Priority data */ + ecc->context.que_pri =edma_read (i,EDMA_QUEPRI ); + +/* backup paramset */ +for (j =0 ; j < ecc->num_slots; j++) +memcpy_fromio (&ecc->context.prm_set[j], + edmacc_regs_base[i] +PARM_OFFSET (j), +PARM_SIZE ); + } + +pm_runtime_put_sync (dev); + +return 0 ; +} + +static int edma_pm_resume (struct device *dev) +{ +int i, j; + +pm_runtime_get_sync (dev); + +for (i =0 ; i < arch_num_cc; i++) { +struct edma *ecc = edma_cc[i]; + +/* restore channel data */ +for (j =0 ; j < ecc->num_channels; j++) { +edma_write_array (i,EDMA_DCHMAP , j, + ecc->context.ch_map[j]); + } + +/* restore DMA Queue Number */ +for (j =0 ; j <8 ; j++) { +edma_write_array (i,EDMA_DMAQNUM , j, + ecc->context.que_num[j]); + } + +/* restore DMA shadow Event Set data */ +edma_shadow0_write_array (i,SH_ESR ,0 , ecc->context.sh_esr); +edma_shadow0_write_array (i,SH_ESR ,1 , ecc->context.sh_esrh); + +/* restore DMA Shadow Event Enable Set data */ +edma_shadow0_write_array (i,SH_EESR ,0 , + ecc->context.sh_eesr); +edma_shadow0_write_array (i,SH_EESR ,1 , + ecc->context.sh_eesrh); + +/* restore DMA Shadow Interrupt Enable Set data */ +edma_shadow0_write_array (i,SH_IESR ,0 , + ecc->context.sh_iesr); +edma_shadow0_write_array (i,SH_IESR ,1 , + ecc->context.sh_iesrh); + +edma_write (i,EDMA_QUETCMAP , ecc->context.que_tc_map); + +/* restore DMA Queue Priority data */ +edma_write (i,EDMA_QUEPRI , ecc->context.que_pri); + +/* restore paramset */ +for (j =0 ; j < ecc->num_slots; j++) { +memcpy_toio (edmacc_regs_base[i] +PARM_OFFSET (j), + &ecc->context.prm_set[j],PARM_SIZE ); + } + } + +pm_runtime_put_sync (dev); + +return 0 ; +} + +static SIMPLE_DEV_PM_OPS(edma_pm_ops, edma_pm_suspend, edma_pm_resume); +static const struct of_device_id edma_of_ids[] = { { .compatible ="ti,edma3" , }, {}
@@ -1770,6 +1902,7 @@ static const struct of_device_id edma_of_ids[] = {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
Quoted 15 lines · Daniel Mack
This patch makes the edma driver resume correctly after suspend. Tested on an AM33xx platform with cyclic audio streams.
The code was shamelessly taken from an ancient BSP tree.
Signed-off-by Daniel Mack <[email protected]>--- arch/arm/common/edma.c | 133 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 133 insertions(+)arch/arm/common/edma.c +0
-0
arch/arm/common/edma.c struct edma { · line 265
Quoted 8 lines
arch/arm/common/edma.c +5
-0@@ -258,6 +258,20 @@ struct edma { void *data);void *data; } intr_data[EDMA_MAX_DMACH ]; + +struct { +struct edmacc_param *prm_set; +unsigned int *ch_map;/* 64 registers */ +unsigned int *que_num;/* 8 registers */
This is OK to save.
arch/arm/common/edma.c struct edma { · line 271
Quoted 6 lines
arch/arm/common/edma.c +6
-0+unsigned int sh_esr; +unsigned int sh_esrh; +unsigned int sh_eesr; +unsigned int sh_eesrh; +unsigned int sh_iesr; +unsigned int sh_iesrh;
Are all these really necessary?
esr and eer- No one should really be setting the esr and should not depend on it when going to a low power state. I wouldn't expect any driver to suspend between edma_start and edma_stop. In DMA Engine, this would correspond to the driver getting a successful callback.
Before suspend, drivers using DMA should anyway be done with using the channel before they can goto suspend, correct me if I'm wrong. So this seems unnecessary to do.
Only thing that makes sense to me to save here is the iesr registers.
arch/arm/common/edma.c struct edma { · line 273
Quoted 2 lines
arch/arm/common/edma.c +2
-0+unsigned int que_tc_map; +unsigned int que_pri;
This is OK to save.
arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1675
Quoted 12 lines
arch/arm/common/edma.c +5
-0+ } context; };static struct edma *edma_cc[EDMA_MAX_CC ];@@ -1655,6 +1669,16 @@ static int edma_probe(struct platform_device *pdev)memcpy_toio (edmacc_regs_base[j] +PARM_OFFSET (i), &dummy_paramset,PARM_SIZE ); +/* resume context */ + edma_cc[j]->context.prm_set = +kzalloc ((sizeof (struct edmacc_param ) * + edma_cc[j]->num_slots),GFP_KERNEL );
Why should you back-up PaRAM set? I feel this is not necessary. PaRAM set will be setup again for the transfer after the resume, and there shouldn't be any pending transfers before the suspend.
Looks like in your audio driver you need to make sure any pending DMA transfers are completed before suspending (unless you're already doing so).
arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1680
Quoted 5 lines
arch/arm/common/edma.c +5
-0+ edma_cc[j]->context.ch_map = +kzalloc ((sizeof (unsigned int ) * + edma_cc[j]->num_channels),GFP_KERNEL ); + edma_cc[j]->context.que_num = +kzalloc ((sizeof (unsigned int ) *8 ),GFP_KERNEL );
Can these allocations be moved to the suspend path? For systems that don't suspend/resume even once, I feel we shouldn't allocate memory that we don't use. These allocations are better to do there.
-Joel
Quoted 11 lines · Joel Fernandes
Quoted 10 lines · Daniel Mack
This patch makes the edma driver resume correctly after suspend. Tested on an AM33xx platform with cyclic audio streams.
The code was shamelessly taken from an ancient BSP tree.
Signed-off-by Daniel Mack <[email protected]>--- arch/arm/common/edma.c | 133 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 133 insertions(+)
..snip..
..snip..
Quoted 10 lines
arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1680
Quoted 6 lines
arch/arm/common/edma.c +5
-0+ edma_cc[j]->context.ch_map = +kzalloc ((sizeof (unsigned int ) * + edma_cc[j]->num_channels),GFP_KERNEL ); + edma_cc[j]->context.que_num = +kzalloc ((sizeof (unsigned int ) *8 ),GFP_KERNEL );Can these allocations be moved to the suspend path? For systems that don't suspend/resume even once, I feel we shouldn't allocate memory that we don't use. These allocations are better to do there.
AFAIK, Suspend/resume should be quick. Allocating and deallocating on every iterating would be useless and time consuming.
Also this task is one time and quick.
Are there any systems (Linux based for now) which doesn't suspend/resume? I believe the probability is very less.
regards
Gururaja
Quoted 8 lines
-Joel
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Some temporary issues with my mua so forgive any artifacts in this email.
Quoted 21 lines · Gururaja
Quoted 10 lines · Joel Fernandes
Quoted 9 lines · Daniel Mack
This patch makes the edma driver resume correctly after suspend. Tested on an AM33xx platform with cyclic audio streams.
The code was shamelessly taken from an ancient BSP tree.
Signed-off-by Daniel Mack <[email protected]>---
arch/arm/common/edma.c| 133 +++++++++++++++++++++++++++++++++++++++++++++++++1 file changed, 133 insertions(+)..snip..
..snip..Quoted 7 lines
Quoted 6 lines
arch/arm/common/edma.c +5
-0+ edma_cc[j]->context.ch_map = +kzalloc ((sizeof (unsigned int ) * + edma_cc[j]->num_channels),GFP_KERNEL ); + edma_cc[j]->context.que_num = +kzalloc ((sizeof (unsigned int ) *8 ),GFP_KERNEL );
Quoted 6 lines
Quoted 3 lines
Can these allocations be moved to the suspend path? For systems that don't suspend/resume even once, I feel we shouldn't allocate memory that we don't use. These allocations are better to do there.
AFAIK, Suspend/resume should be quick. Allocating and deallocating on every iterating would be useless and time consuming.
Nobody said allocate and deallocate on every iteration. Allocate once during the first suspend call and then don't have to allocate on subsequent calls.
As for suspend resume being quick, that argument can flipped the other way too, booting should be quick which is far more frequent than suspend/resume. Apart from the fact that we're not allocating useless memory we would never use.
Quoted 2 lines
Also this task is one time and quick.
Exactly.
Quoted 3 lines
Are there any systems (Linux based for now) which doesn't suspend/resume? I believe the probability is very less.
Nobody talked about suspend/resume not being supported in Linux so not sure what your argument is here.
regards,
-Joel
Quoted 12 lines
regards
GururajaQuoted 7 lines
-Joel
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 25 lines · Joel
Some temporary issues with my mua so forgive any artifacts in this email.
Quoted 21 lines · Gururaja
Quoted 10 lines · Joel Fernandes
Quoted 9 lines · Daniel Mack
This patch makes the edma driver resume correctly after suspend. Tested on an AM33xx platform with cyclic audio streams.
The code was shamelessly taken from an ancient BSP tree.
Signed-off-by Daniel Mack <[email protected]>---
arch/arm/common/edma.c| 133 +++++++++++++++++++++++++++++++++++++++++++++++++1 file changed, 133 insertions(+)..snip..
..snip..Quoted 7 lines
Quoted 6 lines
arch/arm/common/edma.c +5
-0+ edma_cc[j]->context.ch_map = +kzalloc ((sizeof (unsigned int ) * + edma_cc[j]->num_channels),GFP_KERNEL ); + edma_cc[j]->context.que_num = +kzalloc ((sizeof (unsigned int ) *8 ),GFP_KERNEL );
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Can these allocations be moved to the suspend path? For systems that don't suspend/resume even once, I feel we shouldn't allocate memory that we don't use. These allocations are better to do there.
AFAIK, Suspend/resume should be quick. Allocating and deallocating on every iterating would be useless and time consuming.
Nobody said allocate and deallocate on every iteration. Allocate once during the first suspend call and then don't have to allocate on subsequent calls.
I couldn't find any code which allocates parameters inside suspend. Could you show me some code which does this?
Quoted 7 lines
As for suspend resume being quick, that argument can flipped the other way too, booting should be quick which is far more frequent than suspend/resume. Apart from the fact that we're not allocating useless memory we would never use.
Quoted 2 lines
Also this task is one time and quick.
Exactly.
i was referring to allocating in probe call..
Quoted 6 lines
Quoted 4 lines
Are there any systems (Linux based for now) which doesn't suspend/resume? I believe the probability is very less.
Nobody talked about suspend/resume not being supported in Linux so not sure what your argument is here.
I meant linux systems which doesn't go to suspend and resume. Not suspend/resume feature.
Also, I was referring to your 1st comment "... For systems that don't suspend/resume even once, ...."
regards
Gururaja
Quoted 17 lines
regards,
-Joel
Quoted 12 lines
regards
GururajaQuoted 7 lines
-Joel
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi everyone,
Quoted 14 lines · Gururaja Hebbar
Quoted 13 lines · Joel
Some temporary issues with my mua so forgive any artifacts in this email.
Quoted 6 lines · Gururaja
Quoted 1 line · Joel Fernandes
On 10/01/2013 10:04 AM, Daniel Mack wrote:
AFAIK, Suspend/resume should be quick. Allocating and deallocating on every iterating would be useless and time consuming.
Quoted 5 lines
Quoted 3 lines
Nobody said allocate and deallocate on every iteration. Allocate once during the first suspend call and then don't have to allocate on subsequent calls.
I couldn't find any code which allocates parameters inside suspend.
Me neighter :)
But on a general note, I wonder whether it's really worth discussing and merging this patch. As I wrote in the cover letter, it's just a quick and dirty solution that I copied from a very old BSP tree, and I know that the file I'm patching here is going to be removed soon anyway. Actually, the sooner the better.
(And the 'v3' in the subject is really my bad, sorry - I only sent one version of this patch ever).
I can respin the patch on top of the proper driver once all the edma bits have eventually been moved to drivers/dma. Is anyone continuing Matt Porter's work on this?
Thanks,
Daniel
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 23 lines · Daniel Mack
Hi everyone,
Quoted 14 lines · Gururaja Hebbar
Quoted 13 lines · Joel
Some temporary issues with my mua so forgive any artifacts in this email.
Quoted 6 lines · Gururaja
Quoted 1 line · Joel Fernandes
On 10/01/2013 10:04 AM, Daniel Mack wrote:
AFAIK, Suspend/resume should be quick. Allocating and deallocating on every iterating would be useless and time consuming.
Quoted 5 lines
Quoted 3 lines
Nobody said allocate and deallocate on every iteration. Allocate once during the first suspend call and then don't have to allocate on subsequent calls.
I couldn't find any code which allocates parameters inside suspend.
Quoted 7 lines
Me neighter :)
But on a general note, I wonder whether it's really worth discussing and merging this patch. As I wrote in the cover letter, it's just a quick and dirty solution that I copied from a very old BSP tree, and I know that the file I'm patching here is going to be removed soon anyway. Actually, the sooner the better.
OK, I realize its also unnecessary to allocate save anything. In your new version when you respin, please just recreate the state based on the in-memory kernel data structures already there. Ex, you know which channels were allocated so you just set those bits in the IESR.
Quoted 6 lines
(And the 'v3' in the subject is really my bad, sorry - I only sent one version of this patch ever).
I can respin the patch on top of the proper driver once all the edma bits have eventually been moved to
drivers/dma. Is anyone continuing Matt Porter's work on this?
The work is waiting on conversion of the davinci-pcm ASoC driver to DMA Engine, which once done can make exposing the private EDMA API in arch/arm/common/edma.c obsolete and we can take it to drivers/dma. Some more work to be done in edma in unifying the probe etc.
thanks,
-Joel
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 1 line · Joel Fernandes
On 10/09/2013 02:38 AM, Daniel Mack wrote:
[..]
Quoted 11 lines
Quoted 6 lines
(And the 'v3' in the subject is really my bad, sorry - I only sent one version of this patch ever).
I can respin the patch on top of the proper driver once all the edma bits have eventually been moved to
drivers/dma. Is anyone continuing Matt Porter's work on this?The work is waiting on conversion of the davinci-pcm ASoC driver to DMA Engine, which once done can make exposing the private EDMA API in
arch/arm/common/edma.cobsolete and we can take it todrivers/dma. Some more work to be done in edma in unifying the probe etc.
Forgot to mention, since ASoC DMAengine conversion is not going to happen any time soon considering the amount of work involved, I suggest you respin this patch on the common/edma code itself. That way we can keep suspend/resume working on these platforms that use EDMA till the actual conversion takes place.
I suggest though to recreate the state based on existing datastructures instead of allocating/saving additional memory.
Hope you're also in agreement with my comments on your original patch on what needs to / need not be saved.
thanks,
-Joel
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 15 lines · Joel Fernandes
Quoted 1 line · Joel Fernandes
On 10/09/2013 02:38 AM, Daniel Mack wrote:
[..]
Quoted 11 lines
Quoted 6 lines
(And the 'v3' in the subject is really my bad, sorry - I only sent one version of this patch ever).
I can respin the patch on top of the proper driver once all the edma bits have eventually been moved to
drivers/dma. Is anyone continuing Matt Porter's work on this?The work is waiting on conversion of the davinci-pcm ASoC driver to DMA Engine, which once done can make exposing the private EDMA API in
arch/arm/common/edma.cobsolete and we can take it todrivers/dma. Some more work to be done in edma in unifying the probe etc.
Quoted 4 lines
Forgot to mention, since ASoC DMAengine conversion is not going to happen any time soon considering the amount of work involved, I suggest you respin this patch on the common/edma code itself. That way we can keep suspend/resume working on these platforms that use EDMA till the actual conversion takes place.
Ok, I also figured that the current patch does not really suffice. There seems to be an issue with omap_hsmmc DMA channels being disfunct after resume, so I'll respin it.
Quoted 5 lines
I suggest though to recreate the state based on existing datastructures instead of allocating/saving additional memory.
Hope you're also in agreement with my comments on your original patch on what needs to / need not be saved.
I'm still not sure whether the parameters block has to be preserved manually. Let see what we're left with eventually :)
Thanks,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 16 lines · Daniel Mack
Quoted 15 lines · Joel Fernandes
Quoted 1 line · Joel Fernandes
On 10/09/2013 02:38 AM, Daniel Mack wrote:
[..]
Quoted 11 lines
Quoted 6 lines
(And the 'v3' in the subject is really my bad, sorry - I only sent one version of this patch ever).
I can respin the patch on top of the proper driver once all the edma bits have eventually been moved to
drivers/dma. Is anyone continuing Matt Porter's work on this?The work is waiting on conversion of the davinci-pcm ASoC driver to DMA Engine, which once done can make exposing the private EDMA API in
arch/arm/common/edma.cobsolete and we can take it todrivers/dma. Some more work to be done in edma in unifying the probe etc.
Quoted 9 lines
Quoted 4 lines
Forgot to mention, since ASoC DMAengine conversion is not going to happen any time soon considering the amount of work involved, I suggest you respin this patch on the common/edma code itself. That way we can keep suspend/resume working on these platforms that use EDMA till the actual conversion takes place.
Ok, I also figured that the current patch does not really suffice. There seems to be an issue with
omap_hsmmcDMA channels being disfunct after resume, so I'll respin it.
Just for the record, context save/restore of DRAE0[1] is needed for omap_hsmmc DMA to work after resuming from low power mode.
[1]
0340h DRAE0 DMA Region Access Enable Register for Region 0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 30 lines · Gururaja Hebbar
Quoted 29 lines · Joel
Some temporary issues with my mua so forgive any artifacts in this email.
Quoted 25 lines · Gururaja
Quoted 10 lines · Joel Fernandes
Quoted 9 lines · Daniel Mack
This patch makes the edma driver resume correctly after suspend. Tested on an AM33xx platform with cyclic audio streams.
The code was shamelessly taken from an ancient BSP tree.
Signed-off-by Daniel Mack <[email protected]>---
arch/arm/common/edma.c| 133 +++++++++++++++++++++++++++++++++++++++++++++++++1 file changed, 133 insertions(+)..snip..
..snip..Quoted 10 lines
arch/arm/common/edma.c static int edma_probe(struct platform_device *pdev) · line 1680
Quoted 6 lines
arch/arm/common/edma.c +5
-0+ edma_cc[j]->context.ch_map = +kzalloc ((sizeof (unsigned int ) * + edma_cc[j]->num_channels),GFP_KERNEL ); + edma_cc[j]->context.que_num = +kzalloc ((sizeof (unsigned int ) *8 ),GFP_KERNEL );Can these allocations be moved to the suspend path? For systems that don't suspend/resume even once, I feel we shouldn't allocate memory that we don't use. These allocations are better to do there.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
AFAIK, Suspend/resume should be quick. Allocating and deallocating on every iterating would be useless and time consuming.
Nobody said allocate and deallocate on every iteration. Allocate once during the first suspend call and then don't have to allocate on subsequent calls.
I couldn't find any code which allocates parameters inside suspend. Could you show me some code which does this?
Based on my last reply to Daniel, inorder to restore- there's no need to allocate/save anything.
thanks,
-Joel