[OMAP3] ALSA driver 'suspend/resume' handlers

[OMAP3] ALSA driver 'suspend/resume' handlers

[hari n]

Hi,

It appears OMAP ALSA driver does not seem to disable and idle the SDMA
channel on a 'suspend' call. The problem seems to be with the
'omap_stop_dma()' call under 'SNDRV_PCM_TRIGGER_SUSPEND' in
'omap_pcm_trigger()'. ALSA audio driver uses self linking and the
function 'omap_stop_dma()', only unlinks the channels, but DOES NOT
disable an active channel for linked channels.

               memset(dma_chan_link_map, 0, sizeof(dma_chan_link_map));
               do {
                       /* The loop case: we've been here already */
                       if (dma_chan_link_map[cur_lch])
                               break;
                       /* Mark the current channel */
                       dma_chan_link_map[cur_lch] = 1;

                       disable_lnk(cur_lch);

                       next_lch = dma_chan[cur_lch].next_lch;
                       cur_lch = next_lch;
               } while (next_lch != -1);

               return; <---
       }

This means after a return from the 'omap_stop_dma()', there can still
be an active DMA transaction on the channel. Is this the intent Or a
bug? I can think of situations, where in, we may want to complete the
DMA transfer and then disable the channel. In such a case, we need to
wait until the channel is inactive.
The problem with the current implementation is that after the audio
platform trigger (suspend) is called, SOC core calls the CPU DAI
trigger (suspend) and this stops the McBSP. Depending on the current
DMA pointer, this may leave the DMA channel in active state, since DMA
is configured for DEST HW Synchronization and and the this never gets
asserted after the McBSP is stopped..

I see two ways to resolve this issue:
a) Wait after the 'omap_stop_dma()' in audio platform trigger(suspend)
until the DMA channel is inactive (i.e buffer transfer complete). But,
i believe 'trigger' is supposed to be an atomic operation, so we may
need to wait in a worker thread. And we need to flag the McBSP stop
based on DMA transfer completion.
b) Explicitly disable the DMA channel in 'omap_stop_dma().

Option 'b' above may mean losing some data on resume, if we hit 'OFF'
state during suspend. This may not be a big deal for audio (loss of
few secs audio), but for other drivers (ex: USB data file transfer?),
this may not be acceptable. 'Option 'a' means more rework in audio
driver, but no changes in DAM driver and no loss of data.
Current Audio driver also does not seem to support 'OFF' mode during
suspend. It seems to assume that all DMA and McBSP configurations are
retained in a suspend to resume transition.

Thanks
--
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

RE: [OMAP3] ALSA driver 'suspend/resume' handlers

[Pandita, Vikram]

>-----Original Message-----
>From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of hari n
>Sent: Tuesday, September 22, 2009 9:38 PM
>To: linux-omap@vger.kernel.org
>Subject: [OMAP3] ALSA driver 'suspend/resume' handlers
>
>Hi,
>
>It appears OMAP ALSA driver does not seem to disable and idle the SDMA
>channel on a 'suspend' call. The problem seems to be with the
>'omap_stop_dma()' call under 'SNDRV_PCM_TRIGGER_SUSPEND' in
>'omap_pcm_trigger()'. ALSA audio driver uses self linking and the
>function 'omap_stop_dma()', only unlinks the channels, but DOES NOT
>disable an active channel for linked channels.
>
>               memset(dma_chan_link_map, 0, sizeof(dma_chan_link_map));
>               do {
>                       /* The loop case: we've been here already */
>                       if (dma_chan_link_map[cur_lch])
>                               break;
>                       /* Mark the current channel */
>                       dma_chan_link_map[cur_lch] = 1;
>
>                       disable_lnk(cur_lch);

What if we introduce this code here? 
               		/* Stop the Channel transmission */
                		l = dma_read(CCR(cur_lch));
                		l &= ~(1 << OMAP_DMA_CCR_EN);
                		dma_write(l, CCR(cur_lch));

>
>                       next_lch = dma_chan[cur_lch].next_lch;
>                       cur_lch = next_lch;
>               } while (next_lch != -1);
>
>               return; <---
>       }
>
>This means after a return from the 'omap_stop_dma()', there can still
>be an active DMA transaction on the channel. Is this the intent Or a
>bug? I can think of situations, where in, we may want to complete the
>DMA transfer and then disable the channel. In such a case, we need to
>wait until the channel is inactive.
>The problem with the current implementation is that after the audio
>platform trigger (suspend) is called, SOC core calls the CPU DAI
>trigger (suspend) and this stops the McBSP. Depending on the current
>DMA pointer, this may leave the DMA channel in active state, since DMA
>is configured for DEST HW Synchronization and and the this never gets
>asserted after the McBSP is stopped..

If you check the chaining DMA api: omap_stop_dma_chain_transfers() has this kind of code:

               /* Stop the Channel transmission */
                l = dma_read(CCR(channels[i]));
                l &= ~(1 << 7);
                dma_write(l, CCR(channels[i]));

Should audio be using chaining api's? 
Not sure how linking and chaining are related though.
Is self-linking a case of chaining with two same elements? 


>
>I see two ways to resolve this issue:
>a) Wait after the 'omap_stop_dma()' in audio platform trigger(suspend)
>until the DMA channel is inactive (i.e buffer transfer complete). But,
>i believe 'trigger' is supposed to be an atomic operation, so we may
>need to wait in a worker thread. And we need to flag the McBSP stop
>based on DMA transfer completion.
>b) Explicitly disable the DMA channel in 'omap_stop_dma().
>
>Option 'b' above may mean losing some data on resume, if we hit 'OFF'
>state during suspend. This may not be a big deal for audio (loss of
>few secs audio), but for other drivers (ex: USB data file transfer?),
>this may not be acceptable. 'Option 'a' means more rework in audio
>driver, but no changes in DAM driver and no loss of data.
>Current Audio driver also does not seem to support 'OFF' mode during
>suspend. It seems to assume that all DMA and McBSP configurations are
>retained in a suspend to resume transition.
>
>Thanks
>--
>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

--
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

Re: [OMAP3] ALSA driver 'suspend/resume' handlers

[hari n]

On Tue, Sep 22, 2009 at 10:20 PM, Pandita, Vikram <vikram.pandita@ti.com> wrote:
>
>
>>-----Original Message-----
>>From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of hari n
>>Sent: Tuesday, September 22, 2009 9:38 PM
>>To: linux-omap@vger.kernel.org
>>Subject: [OMAP3] ALSA driver 'suspend/resume' handlers
>>
>>Hi,
>>
>>It appears OMAP ALSA driver does not seem to disable and idle the SDMA
>>channel on a 'suspend' call. The problem seems to be with the
>>'omap_stop_dma()' call under 'SNDRV_PCM_TRIGGER_SUSPEND' in
>>'omap_pcm_trigger()'. ALSA audio driver uses self linking and the
>>function 'omap_stop_dma()', only unlinks the channels, but DOES NOT
>>disable an active channel for linked channels.
>>
>>               memset(dma_chan_link_map, 0, sizeof(dma_chan_link_map));
>>               do {
>>                       /* The loop case: we've been here already */
>>                       if (dma_chan_link_map[cur_lch])
>>                               break;
>>                       /* Mark the current channel */
>>                       dma_chan_link_map[cur_lch] = 1;
>>
>>                       disable_lnk(cur_lch);
>
> What if we introduce this code here?
>                        /* Stop the Channel transmission */
>                                l = dma_read(CCR(cur_lch));
>                                l &= ~(1 << OMAP_DMA_CCR_EN);
>                                dma_write(l, CCR(cur_lch));
>
This may result in disabling an active DMA channel.. Which can lead to
un-drained FIFOs, if we do not wait to check for 'RD_ACTIVE',
'WR_ACTIVE' bits (refer to 'Disabling a channel during transfer' and
'FIFO draining mechanism'. In addition, as i mentioned below we may
also lose data..
>>
>>                       next_lch = dma_chan[cur_lch].next_lch;
>>                       cur_lch = next_lch;
>>               } while (next_lch != -1);
>>
>>               return; <---
>>       }
>>
>>This means after a return from the 'omap_stop_dma()', there can still
>>be an active DMA transaction on the channel. Is this the intent Or a
>>bug? I can think of situations, where in, we may want to complete the
>>DMA transfer and then disable the channel. In such a case, we need to
>>wait until the channel is inactive.
>>The problem with the current implementation is that after the audio
>>platform trigger (suspend) is called, SOC core calls the CPU DAI
>>trigger (suspend) and this stops the McBSP. Depending on the current
>>DMA pointer, this may leave the DMA channel in active state, since DMA
>>is configured for DEST HW Synchronization and and the this never gets
>>asserted after the McBSP is stopped..
>
> If you check the chaining DMA api: omap_stop_dma_chain_transfers() has this kind of code:
>
>               /* Stop the Channel transmission */
>                l = dma_read(CCR(channels[i]));
>                l &= ~(1 << 7);
>                dma_write(l, CCR(channels[i]));
>
> Should audio be using chaining api's?
> Not sure how linking and chaining are related though.
> Is self-linking a case of chaining with two same elements?
>
Here again, we have the same issue of disabling the channels, while
some data may be in the FIFO/flight..
>
>>
>>I see two ways to resolve this issue:
>>a) Wait after the 'omap_stop_dma()' in audio platform trigger(suspend)
>>until the DMA channel is inactive (i.e buffer transfer complete). But,
>>i believe 'trigger' is supposed to be an atomic operation, so we may
>>need to wait in a worker thread. And we need to flag the McBSP stop
>>based on DMA transfer completion.
>>b) Explicitly disable the DMA channel in 'omap_stop_dma().
>>
>>Option 'b' above may mean losing some data on resume, if we hit 'OFF'
>>state during suspend. This may not be a big deal for audio (loss of
>>few secs audio), but for other drivers (ex: USB data file transfer?),
>>this may not be acceptable. 'Option 'a' means more rework in audio
>>driver, but no changes in DAM driver and no loss of data.
>>Current Audio driver also does not seem to support 'OFF' mode during
>>suspend. It seems to assume that all DMA and McBSP configurations are
>>retained in a suspend to resume transition.
>>
>>Thanks
>>--
>>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
>
>
--
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

Re: [OMAP3] ALSA driver 'suspend/resume' handlers

[Jarkko Nikula]

On Wed, 23 Sep 2009 00:02:01 -0500
hari n <hari.zoom@gmail.com> wrote:

> >>It appears OMAP ALSA driver does not seem to disable and idle the SDMA
> >>channel on a 'suspend' call. The problem seems to be with the
> >>'omap_stop_dma()' call under 'SNDRV_PCM_TRIGGER_SUSPEND' in
> >>'omap_pcm_trigger()'. ALSA audio driver uses self linking and the
> >>function 'omap_stop_dma()', only unlinks the channels, but DOES NOT
> >>disable an active channel for linked channels.
> >>
Good finding, definitely I was expecting that it will stop the transfer.

I worder what's the background for current omap_stop_dma
implementation? I would expect that omap_stop_dma would just stop the
logical channel without touching the channel linking etc. as there is
own API for chained transfers and thus omap_stop_dma could be used for
non-chained transfers or 'hacking' with chained transfers.

> >>This means after a return from the 'omap_stop_dma()', there can still
> >>be an active DMA transaction on the channel. Is this the intent Or a
> >>bug? I can think of situations, where in, we may want to complete the
> >>DMA transfer and then disable the channel. In such a case, we need to
> >>wait until the channel is inactive.

I don't know, probably there is no driver expecting that transfer will
complete after the omap_stop_dma is called or they explicitly do some
PIO based FIFO flush for their devices?

> >>The problem with the current implementation is that after the audio
> >>platform trigger (suspend) is called, SOC core calls the CPU DAI
> >>trigger (suspend) and this stops the McBSP. Depending on the current
> >>DMA pointer, this may leave the DMA channel in active state, since DMA
> >>is configured for DEST HW Synchronization and and the this never gets
> >>asserted after the McBSP is stopped..
> >
This is true. Then the question is will the operation recover after the
stream is resumed and after the low-level dma and mcbsp contexts are
restored and operation started again.

> >>I see two ways to resolve this issue:
> >>a) Wait after the 'omap_stop_dma()' in audio platform trigger(suspend)
> >>until the DMA channel is inactive (i.e buffer transfer complete). But,
> >>i believe 'trigger' is supposed to be an atomic operation, so we may
> >>need to wait in a worker thread. And we need to flag the McBSP stop
> >>based on DMA transfer completion.
> >>b) Explicitly disable the DMA channel in 'omap_stop_dma().
> >>
> >>Option 'b' above may mean losing some data on resume, if we hit 'OFF'
> >>state during suspend. This may not be a big deal for audio (loss of
> >>few secs audio), but for other drivers (ex: USB data file transfer?),
> >>this may not be acceptable. 'Option 'a' means more rework in audio
> >>driver, but no changes in DAM driver and no loss of data.

Yeah, trigger callback is atomic and can be called from the interrupt
context as well. Option 'a' doesn't help losing of audio samples since
the soc-core is first muting the codec.

> >>Current Audio driver also does not seem to support 'OFF' mode during
> >>suspend. It seems to assume that all DMA and McBSP configurations are
> >>retained in a suspend to resume transition.
> >>
It's true, this has not got so much attention. I think I was expecting
that low-level mcbsp and dma modules will take care of their context
back-up and restore and that's enough for audio as long as audio will
inform those modules with _start/_stop calls.

I don't see any problem if suspend and resume callbacks are added into
omap-pcm.c and omap-mcbsp.c calling e.g. omap_mcbsp_config if needed.


-- 
Jarkko

RE: [OMAP3] ALSA driver 'suspend/resume' handlers

[Shilimkar, Santosh]

Hari/Jarkko,
> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Jarkko Nikula
> Sent: Thursday, September 24, 2009 12:16 PM
> To: hari n
> Cc: Pandita, Vikram; linux-omap@vger.kernel.org
> Subject: Re: [OMAP3] ALSA driver 'suspend/resume' handlers
> 
> On Wed, 23 Sep 2009 00:02:01 -0500
> hari n <hari.zoom@gmail.com> wrote:
> 
> > >>It appears OMAP ALSA driver does not seem to disable and idle the SDMA
> > >>channel on a 'suspend' call. The problem seems to be with the
> > >>'omap_stop_dma()' call under 'SNDRV_PCM_TRIGGER_SUSPEND' in
> > >>'omap_pcm_trigger()'. ALSA audio driver uses self linking and the
> > >>function 'omap_stop_dma()', only unlinks the channels, but DOES NOT
> > >>disable an active channel for linked channels.
> > >>
> Good finding, definitely I was expecting that it will stop the transfer.
> 
> I worder what's the background for current omap_stop_dma
> implementation? I would expect that omap_stop_dma would just stop the
> logical channel without touching the channel linking etc. as there is
> own API for chained transfers and thus omap_stop_dma could be used for
> non-chained transfers or 'hacking' with chained transfers.

> > >>This means after a return from the 'omap_stop_dma()', there can still
> > >>be an active DMA transaction on the channel. Is this the intent Or a
> > >>bug? I can think of situations, where in, we may want to complete the
> > >>DMA transfer and then disable the channel. In such a case, we need to
> > >>wait until the channel is inactive.
> 
> I don't know, probably there is no driver expecting that transfer will
> complete after the omap_stop_dma is called or they explicitly do some
> PIO based FIFO flush for their devices?
> 
> > >>The problem with the current implementation is that after the audio
> > >>platform trigger (suspend) is called, SOC core calls the CPU DAI
> > >>trigger (suspend) and this stops the McBSP. Depending on the current
> > >>DMA pointer, this may leave the DMA channel in active state, since DMA
> > >>is configured for DEST HW Synchronization and and the this never gets
> > >>asserted after the McBSP is stopped..
> > >
> This is true. Then the question is will the operation recover after the
> stream is resumed and after the low-level dma and mcbsp contexts are
> restored and operation started again.
> 
> > >>I see two ways to resolve this issue:
> > >>a) Wait after the 'omap_stop_dma()' in audio platform trigger(suspend)
> > >>until the DMA channel is inactive (i.e buffer transfer complete). But,
> > >>i believe 'trigger' is supposed to be an atomic operation, so we may
> > >>need to wait in a worker thread. And we need to flag the McBSP stop
> > >>based on DMA transfer completion.
> > >>b) Explicitly disable the DMA channel in 'omap_stop_dma().
> > >>
> > >>Option 'b' above may mean losing some data on resume, if we hit 'OFF'
> > >>state during suspend. This may not be a big deal for audio (loss of
> > >>few secs audio), but for other drivers (ex: USB data file transfer?),
> > >>this may not be acceptable. 'Option 'a' means more rework in audio
> > >>driver, but no changes in DAM driver and no loss of data.
> 
> Yeah, trigger callback is atomic and can be called from the interrupt
> context as well. Option 'a' doesn't help losing of audio samples since
> the soc-core is first muting the codec.
> 
> > >>Current Audio driver also does not seem to support 'OFF' mode during
> > >>suspend. It seems to assume that all DMA and McBSP configurations are
> > >>retained in a suspend to resume transition.
> > >>
> It's true, this has not got so much attention. I think I was expecting
> that low-level mcbsp and dma modules will take care of their context
> back-up and restore and that's enough for audio as long as audio will
> inform those modules with _start/_stop calls.
> 
> I don't see any problem if suspend and resume callbacks are added into
> omap-pcm.c and omap-mcbsp.c calling e.g. omap_mcbsp_config if needed.

omap_stop_dma() should be issued when we really want to stop the DMA transfer and issuing this with an outstanding transfer is a BUG in ALSA driver.

Having said that, there is also bug in the DMA driver which doesn't disable the channel in linking cases. Since we use always hardware synchronized method, hardware will take care of draining the buffer so no loss of data.

So option B should be ok and USB case also would work as mentioned above.

Regrads
Santosh
  

RE: [OMAP3] ALSA driver 'suspend/resume' handlers

[Pandita, Vikram]

>-----Original Message-----
>From: Shilimkar, Santosh
>Sent: Thursday, September 24, 2009 2:24 AM
>To: Jarkko Nikula; hari n
>Cc: Pandita, Vikram; linux-omap@vger.kernel.org
>Subject: RE: [OMAP3] ALSA driver 'suspend/resume' handlers
>
>Hari/Jarkko,
>> -----Original Message-----
>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> owner@vger.kernel.org] On Behalf Of Jarkko Nikula
>> Sent: Thursday, September 24, 2009 12:16 PM
>> To: hari n
>> Cc: Pandita, Vikram; linux-omap@vger.kernel.org
>> Subject: Re: [OMAP3] ALSA driver 'suspend/resume' handlers
>>
>> On Wed, 23 Sep 2009 00:02:01 -0500
>> hari n <hari.zoom@gmail.com> wrote:
>>
>> > >>It appears OMAP ALSA driver does not seem to disable and idle the SDMA
>> > >>channel on a 'suspend' call. The problem seems to be with the
>> > >>'omap_stop_dma()' call under 'SNDRV_PCM_TRIGGER_SUSPEND' in
>> > >>'omap_pcm_trigger()'. ALSA audio driver uses self linking and the
>> > >>function 'omap_stop_dma()', only unlinks the channels, but DOES NOT
>> > >>disable an active channel for linked channels.
>> > >>
>> Good finding, definitely I was expecting that it will stop the transfer.
>>
>> I worder what's the background for current omap_stop_dma
>> implementation? I would expect that omap_stop_dma would just stop the
>> logical channel without touching the channel linking etc. as there is
>> own API for chained transfers and thus omap_stop_dma could be used for
>> non-chained transfers or 'hacking' with chained transfers.
>
>> > >>This means after a return from the 'omap_stop_dma()', there can still
>> > >>be an active DMA transaction on the channel. Is this the intent Or a
>> > >>bug? I can think of situations, where in, we may want to complete the
>> > >>DMA transfer and then disable the channel. In such a case, we need to
>> > >>wait until the channel is inactive.
>>
>> I don't know, probably there is no driver expecting that transfer will
>> complete after the omap_stop_dma is called or they explicitly do some
>> PIO based FIFO flush for their devices?
>>
>> > >>The problem with the current implementation is that after the audio
>> > >>platform trigger (suspend) is called, SOC core calls the CPU DAI
>> > >>trigger (suspend) and this stops the McBSP. Depending on the current
>> > >>DMA pointer, this may leave the DMA channel in active state, since DMA
>> > >>is configured for DEST HW Synchronization and and the this never gets
>> > >>asserted after the McBSP is stopped..
>> > >
>> This is true. Then the question is will the operation recover after the
>> stream is resumed and after the low-level dma and mcbsp contexts are
>> restored and operation started again.
>>
>> > >>I see two ways to resolve this issue:
>> > >>a) Wait after the 'omap_stop_dma()' in audio platform trigger(suspend)
>> > >>until the DMA channel is inactive (i.e buffer transfer complete). But,
>> > >>i believe 'trigger' is supposed to be an atomic operation, so we may
>> > >>need to wait in a worker thread. And we need to flag the McBSP stop
>> > >>based on DMA transfer completion.
>> > >>b) Explicitly disable the DMA channel in 'omap_stop_dma().
>> > >>
>> > >>Option 'b' above may mean losing some data on resume, if we hit 'OFF'
>> > >>state during suspend. This may not be a big deal for audio (loss of
>> > >>few secs audio), but for other drivers (ex: USB data file transfer?),
>> > >>this may not be acceptable. 'Option 'a' means more rework in audio
>> > >>driver, but no changes in DAM driver and no loss of data.
>>
>> Yeah, trigger callback is atomic and can be called from the interrupt
>> context as well. Option 'a' doesn't help losing of audio samples since
>> the soc-core is first muting the codec.
>>
>> > >>Current Audio driver also does not seem to support 'OFF' mode during
>> > >>suspend. It seems to assume that all DMA and McBSP configurations are
>> > >>retained in a suspend to resume transition.
>> > >>
>> It's true, this has not got so much attention. I think I was expecting
>> that low-level mcbsp and dma modules will take care of their context
>> back-up and restore and that's enough for audio as long as audio will
>> inform those modules with _start/_stop calls.
>>
>> I don't see any problem if suspend and resume callbacks are added into
>> omap-pcm.c and omap-mcbsp.c calling e.g. omap_mcbsp_config if needed.
>
>omap_stop_dma() should be issued when we really want to stop the DMA transfer and issuing this with
>an outstanding transfer is a BUG in ALSA driver.
>
>Having said that, there is also bug in the DMA driver which doesn't disable the channel in linking
>cases. Since we use always hardware synchronized method, hardware will take care of draining the
>buffer so no loss of data.
>
>So option B should be ok and USB case also would work as mentioned above.

USB is not a valid use case to discuss here.
Mentor OTG controller has its internal DMA and so does EHCI/OHCI host controller.
USB on OMAP3 _does_not_ use system DMA.


>
>Regrads
>Santosh
>

Re: [OMAP3] ALSA driver 'suspend/resume' handlers

[Tony Lindgren]

* Pandita, Vikram <vikram.pandita@ti.com> [090924 05:59]:
> 
> 
> >-----Original Message-----
> >From: Shilimkar, Santosh
> >Sent: Thursday, September 24, 2009 2:24 AM
> >To: Jarkko Nikula; hari n
> >Cc: Pandita, Vikram; linux-omap@vger.kernel.org
> >Subject: RE: [OMAP3] ALSA driver 'suspend/resume' handlers
> >
> >Hari/Jarkko,
> >> -----Original Message-----
> >> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> >> owner@vger.kernel.org] On Behalf Of Jarkko Nikula
> >> Sent: Thursday, September 24, 2009 12:16 PM
> >> To: hari n
> >> Cc: Pandita, Vikram; linux-omap@vger.kernel.org
> >> Subject: Re: [OMAP3] ALSA driver 'suspend/resume' handlers
> >>
> >> On Wed, 23 Sep 2009 00:02:01 -0500
> >> hari n <hari.zoom@gmail.com> wrote:
> >>

<snip>

> >omap_stop_dma() should be issued when we really want to stop the DMA transfer and issuing this with
> >an outstanding transfer is a BUG in ALSA driver.
> >
> >Having said that, there is also bug in the DMA driver which doesn't disable the channel in linking
> >cases. Since we use always hardware synchronized method, hardware will take care of draining the
> >buffer so no loss of data.
> >
> >So option B should be ok and USB case also would work as mentioned above.
> 
> USB is not a valid use case to discuss here.
> Mentor OTG controller has its internal DMA and so does EHCI/OHCI host controller.
> USB on OMAP3 _does_not_ use system DMA.

At least tusb6010 connected to 2420 on n800 and n810 uses the system DMA.

Tony

RE: [OMAP3] ALSA driver 'suspend/resume' handlers

[Shilimkar, Santosh]

Hari,
> -----Original Message-----
> From: Shilimkar, Santosh
> Sent: Thursday, September 24, 2009 12:54 PM
> To: 'Jarkko Nikula'; hari n
> Cc: Pandita, Vikram; linux-omap@vger.kernel.org
> Subject: RE: [OMAP3] ALSA driver 'suspend/resume' handlers
> 
> Hari/Jarkko,
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > owner@vger.kernel.org] On Behalf Of Jarkko Nikula
> > Sent: Thursday, September 24, 2009 12:16 PM
> > To: hari n
> > Cc: Pandita, Vikram; linux-omap@vger.kernel.org
> > Subject: Re: [OMAP3] ALSA driver 'suspend/resume' handlers
> >
> > I don't see any problem if suspend and resume callbacks are added into
> > omap-pcm.c and omap-mcbsp.c calling e.g. omap_mcbsp_config if needed.
> 
> omap_stop_dma() should be issued when we really want to stop the DMA
> transfer and issuing this with an outstanding transfer is a BUG in ALSA
> driver.
> 
> Having said that, there is also bug in the DMA driver which doesn't
> disable the channel in linking cases. Since we use always hardware
> synchronized method, hardware will take care of draining the buffer so no
> loss of data.
> 
> So option B should be ok and USB case also would work as mentioned above.

Which option finally we converged on this issue? Shall we fix in the DMA driver or you want to do this in ALSA?


Regards,
Santosh

Re: [OMAP3] ALSA driver 'suspend/resume' handlers

[Jarkko Nikula]

On Tue, 29 Sep 2009 21:28:45 +0530
"Shilimkar, Santosh" <santosh.shilimkar@ti.com> wrote:

> > Having said that, there is also bug in the DMA driver which doesn't
> > disable the channel in linking cases. Since we use always hardware
> > synchronized method, hardware will take care of draining the buffer so no
> > loss of data.
> > 
> > So option B should be ok and USB case also would work as mentioned above.
> 
> Which option finally we converged on this issue? Shall we fix in the DMA driver or you want to do this in ALSA?
> 
For me it looks that at least the omap_stop_dma should be fixed to stop
ongoing transfer always when it's called. As the name suggest :-)

Then for offmode, if there is no clear method to save & restore
low-level McBSP & DMA context in their drivers, I don't see problem if
e.g. resume handler in sound/soc/omap/omap-mcbsp.c would call
the omap_mcbsp_config etc.


-- 
Jarkko

RE: [OMAP3] ALSA driver 'suspend/resume' handlers

[Shilimkar, Santosh]

> -----Original Message-----
> From: Jarkko Nikula [mailto:jhnikula@gmail.com]
> Sent: Wednesday, September 30, 2009 11:10 AM
> To: Shilimkar, Santosh
> Cc: hari n; Pandita, Vikram; linux-omap@vger.kernel.org
> Subject: Re: [OMAP3] ALSA driver 'suspend/resume' handlers
> 
> On Tue, 29 Sep 2009 21:28:45 +0530
> "Shilimkar, Santosh" <santosh.shilimkar@ti.com> wrote:
> 
> > > Having said that, there is also bug in the DMA driver which doesn't
> > > disable the channel in linking cases. Since we use always hardware
> > > synchronized method, hardware will take care of draining the buffer so
> no
> > > loss of data.
> > >
> > > So option B should be ok and USB case also would work as mentioned
> above.
> >
> > Which option finally we converged on this issue? Shall we fix in the DMA
> driver or you want to do this in ALSA?
> >
> For me it looks that at least the omap_stop_dma should be fixed to stop
> ongoing transfer always when it's called. As the name suggest :-)
Agree !!
Have a look at below patch and let me know if it's ok.

>From 6042ce380c36907b0740e208f243f00ca370d09e Mon Sep 17 00:00:00 2001
From: Santosh Shilimkar <santosh.shilimkar@ti.com>
Date: Wed, 30 Sep 2009 11:26:24 +0530
Subject: [PATCH] ARM: OMAP: SDMA: Stop channel in omap_stop_dma() API for linking as well.

OMAP sDMA driver API omap_stop_dma() doesn't really stop the dma when used
in linking scenario. This patch fixes the same.

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Signed-off-by: Venkatraman S <svenkatr@ti.com>
CC: Hari n <hari.zoom@gmail.com>
CC: Jarkko Nikula <jhnikula@gmail.com>
---
 arch/arm/plat-omap/dma.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
index fd3154a..633c123 100644
--- a/arch/arm/plat-omap/dma.c
+++ b/arch/arm/plat-omap/dma.c
@@ -975,6 +975,11 @@ void omap_stop_dma(int lch)
 {
 	u32 l;
 
+	/* Disable the DMA channel */
+	l = dma_read(CCR(lch));
+	l &= ~OMAP_DMA_CCR_EN;
+	dma_write(l, CCR(lch));
+
 	if (!omap_dma_in_1510_mode() && dma_chan[lch].next_lch != -1) {
 		int next_lch, cur_lch = lch;
 		char dma_chan_link_map[OMAP_DMA4_LOGICAL_DMA_CH_COUNT];
@@ -1000,10 +1005,6 @@ void omap_stop_dma(int lch)
 	if (cpu_class_is_omap1())
 		dma_write(0, CICR(lch));
 
-	l = dma_read(CCR(lch));
-	l &= ~OMAP_DMA_CCR_EN;
-	dma_write(l, CCR(lch));
-
 	dma_chan[lch].flags &= ~OMAP_DMA_ACTIVE;
 }
 EXPORT_SYMBOL(omap_stop_dma);
-- 
1.5.4.7

[OMAP3] ALSA driver 'suspend/resume' handlers

[Shilimkar, Santosh]

Tony,
Can you check below patch? If it is OK, please add this to omap-fixes
-----Original Message-----
From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Shilimkar, Santosh
Sent: Wednesday, September 30, 2009 11:38 AM
To: Jarkko Nikula
Cc: hari n; Pandita, Vikram; linux-omap@vger.kernel.org
Subject: RE: [OMAP3] ALSA driver 'suspend/resume' handlers


> -----Original Message-----
> From: Jarkko Nikula [mailto:jhnikula@gmail.com]
> Sent: Wednesday, September 30, 2009 11:10 AM
> To: Shilimkar, Santosh
> Cc: hari n; Pandita, Vikram; linux-omap@vger.kernel.org
> Subject: Re: [OMAP3] ALSA driver 'suspend/resume' handlers
> 
> On Tue, 29 Sep 2009 21:28:45 +0530
> "Shilimkar, Santosh" <santosh.shilimkar@ti.com> wrote:
> 
> > > Having said that, there is also bug in the DMA driver which doesn't
> > > disable the channel in linking cases. Since we use always hardware
> > > synchronized method, hardware will take care of draining the buffer so
> no
> > > loss of data.
> > >
> > > So option B should be ok and USB case also would work as mentioned
> above.
> >
> > Which option finally we converged on this issue? Shall we fix in the DMA
> driver or you want to do this in ALSA?
> >
> For me it looks that at least the omap_stop_dma should be fixed to stop
> ongoing transfer always when it's called. As the name suggest :-)
Agree !!
Have a look at below patch and let me know if it's ok.

>From 6042ce380c36907b0740e208f243f00ca370d09e Mon Sep 17 00:00:00 2001
From: Santosh Shilimkar <santosh.shilimkar@ti.com>
Date: Wed, 30 Sep 2009 11:26:24 +0530
Subject: [PATCH] ARM: OMAP: SDMA: Stop channel in omap_stop_dma() API for linking as well.

OMAP sDMA driver API omap_stop_dma() doesn't really stop the dma when used
in linking scenario. This patch fixes the same.

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Signed-off-by: Venkatraman S <svenkatr@ti.com>
CC: Hari n <hari.zoom@gmail.com>
CC: Jarkko Nikula <jhnikula@gmail.com>
---
 arch/arm/plat-omap/dma.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
index fd3154a..633c123 100644
--- a/arch/arm/plat-omap/dma.c
+++ b/arch/arm/plat-omap/dma.c
@@ -975,6 +975,11 @@ void omap_stop_dma(int lch)
 {
 	u32 l;
 
+	/* Disable the DMA channel */
+	l = dma_read(CCR(lch));
+	l &= ~OMAP_DMA_CCR_EN;
+	dma_write(l, CCR(lch));
+
 	if (!omap_dma_in_1510_mode() && dma_chan[lch].next_lch != -1) {
 		int next_lch, cur_lch = lch;
 		char dma_chan_link_map[OMAP_DMA4_LOGICAL_DMA_CH_COUNT];
@@ -1000,10 +1005,6 @@ void omap_stop_dma(int lch)
 	if (cpu_class_is_omap1())
 		dma_write(0, CICR(lch));
 
-	l = dma_read(CCR(lch));
-	l &= ~OMAP_DMA_CCR_EN;
-	dma_write(l, CCR(lch));
-
 	dma_chan[lch].flags &= ~OMAP_DMA_ACTIVE;
 }
 EXPORT_SYMBOL(omap_stop_dma);
-- 
1.5.4.7

--
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