Re: [PATCH v3] ARM: omap: edma: add suspend suspend/resume hooks
On Friday 18 October 2013 09:27 PM, Daniel Mack wrote: On 10/09/2013 10:14 PM, Joel Fernandes wrote: On 10/09/2013 09:12 AM, Joel Fernandes wrote: On 10/09/2013 02:38 AM, Daniel Mack wrote: [..] (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. 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. 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-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: omap: edma: add suspend suspend/resume hooks
On 10/09/2013 10:14 PM, Joel Fernandes wrote: On 10/09/2013 09:12 AM, Joel Fernandes wrote: On 10/09/2013 02:38 AM, Daniel Mack wrote: [..] (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. 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. 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-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: omap: edma: add suspend suspend/resume hooks
Some temporary issues with my mua so forgive any artifacts in this email. On Oct 9, 2013, at 12:14 AM, Hebbar, Gururaja gururaja.heb...@ti.com wrote: On Wednesday 09 October 2013 09:58 AM, Joel Fernandes wrote: On 10/01/2013 10:04 AM, Daniel Mack wrote: 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 zon...@gmail.com --- arch/arm/common/edma.c | 133 + 1 file changed, 133 insertions(+) ..snip.. ..snip.. +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. 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. Also this task is one time and quick. Exactly. 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 regards Gururaja -Joel -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: omap: edma: add suspend suspend/resume hooks
On Wednesday 09 October 2013 11:33 AM, Fernandes, Joel wrote: Some temporary issues with my mua so forgive any artifacts in this email. On Oct 9, 2013, at 12:14 AM, Hebbar, Gururaja gururaja.heb...@ti.com wrote: On Wednesday 09 October 2013 09:58 AM, Joel Fernandes wrote: On 10/01/2013 10:04 AM, Daniel Mack wrote: 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 zon...@gmail.com --- arch/arm/common/edma.c | 133 + 1 file changed, 133 insertions(+) ..snip.. ..snip.. +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. 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? 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. Also this task is one time and quick. Exactly. i was referring to allocating in probe call.. 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 regards, -Joel regards Gururaja -Joel -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: omap: edma: add suspend suspend/resume hooks
Hi everyone, On 09.10.2013 08:18, Gururaja Hebbar wrote: On Wednesday 09 October 2013 11:33 AM, Fernandes, Joel wrote: Some temporary issues with my mua so forgive any artifacts in this email. On Oct 9, 2013, at 12:14 AM, Hebbar, Gururaja gururaja.heb...@ti.com wrote: On Wednesday 09 October 2013 09:58 AM, Joel Fernandes wrote: 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. 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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: omap: edma: add suspend suspend/resume hooks
On 10/09/2013 02:38 AM, Daniel Mack wrote: Hi everyone, On 09.10.2013 08:18, Gururaja Hebbar wrote: On Wednesday 09 October 2013 11:33 AM, Fernandes, Joel wrote: Some temporary issues with my mua so forgive any artifacts in this email. On Oct 9, 2013, at 12:14 AM, Hebbar, Gururaja gururaja.heb...@ti.com wrote: On Wednesday 09 October 2013 09:58 AM, Joel Fernandes wrote: 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. 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. 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. (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-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: omap: edma: add suspend suspend/resume hooks
On 10/09/2013 01:18 AM, Gururaja Hebbar wrote: On Wednesday 09 October 2013 11:33 AM, Fernandes, Joel wrote: Some temporary issues with my mua so forgive any artifacts in this email. On Oct 9, 2013, at 12:14 AM, Hebbar, Gururaja gururaja.heb...@ti.com wrote: On Wednesday 09 October 2013 09:58 AM, Joel Fernandes wrote: On 10/01/2013 10:04 AM, Daniel Mack wrote: 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 zon...@gmail.com --- arch/arm/common/edma.c | 133 + 1 file changed, 133 insertions(+) ..snip.. ..snip.. +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. 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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: omap: edma: add suspend suspend/resume hooks
On 10/09/2013 09:12 AM, Joel Fernandes wrote: On 10/09/2013 02:38 AM, Daniel Mack wrote: [..] (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. 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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: omap: edma: add suspend suspend/resume hooks
On 10/01/2013 10:04 AM, Daniel Mack wrote:
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 zon...@gmail.com
---
arch/arm/common/edma.c | 133
+
1 file changed, 133 insertions(+)
diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index 2a72169..d787f14 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -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.
+ 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.
+ unsigned int que_tc_map;
+ unsigned int que_pri;
This is OK to save.
+ } 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).
+ 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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: omap: edma: add suspend suspend/resume hooks
On Wednesday 09 October 2013 09:58 AM, Joel Fernandes wrote: On 10/01/2013 10:04 AM, Daniel Mack wrote: 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 zon...@gmail.com --- arch/arm/common/edma.c | 133 + 1 file changed, 133 insertions(+) ..snip.. ..snip.. +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 -Joel -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
