Skip to content

dw-dma: change dw_dma_stop for byt and cht #1257

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lgirdwood merged 1 commit into from
Apr 15, 2019
Merged

dw-dma: change dw_dma_stop for byt and cht #1257

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions src/drivers/dw/dma.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -639,13 +639,24 @@ static int dw_dma_stop(struct dma *dma, int channel)

spin_lock_irq(&dma->lock, flags);

ret = poll_for_register_delay(dma_base(dma) + DW_DMA_CHAN_EN,
CHAN_MASK(channel), 0,

@lyakh lyakh Apr 11, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand it correctly, that previously this function wasn't disabling the DMA channel actively at all, it was waiting for the enable bit to go to 0, presumably, as triggered by some other process, e.g. by dw_dma_verify_block(), right? But that function only exists if DW_USE_HW_LLI is defined, and it is not defined on BYT and CHT. So, in fact I don't see what would be disabling the channel previously in this function. Does this mean, that this call was always running on a timeout? Were we getting "ewt" errors? The current timeout on BYT is defined to 1333us. Or was that bit cleared in some other way?

@tlauda tlauda Apr 11, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non cAVS platform don't have support for hardware linked list, so linked list items are manually reloaded in function dw_dma_chan_reload_lli and also this function enables the channel again (which is disabled after every lli transfer). So yes, we were just waiting for the transfer to be over with the next DMA irq, which usually took from 0.5 ms to 0.8 ms, but it really depends on when you stop the stream.

@lyakh lyakh Apr 11, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know how the channel is disabled after each transfer? Automatically or is there a write to the CHAN_EN register somewhere that I'm not seeing?

@tlauda tlauda Apr 11, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automatically.

@lyakh lyakh Apr 11, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! Unrelated to this patch, but it would be nice to add a comment explaining that to the driver.

/* channel cannot be disabled right away, so first we need to
* suspend it and drain the FIFO
*/
dw_write(dma, DW_CFG_LOW(channel), chan->cfg_lo | DW_CFG_CH_SUSPEND |
Comment thread
tlauda marked this conversation as resolved.
Outdated
DW_CFG_CH_DRAIN);

@lyakh lyakh Apr 11, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure we need to OR with .cfg_lo above, but it shouldn't hurt either.

@xiulipan xiulipan Apr 14, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyakh This means we need to keep the other bits to the original status as we only changes few bits


/* now we wait for FIFO to be empty */
ret = poll_for_register_delay(dma_base(dma) + DW_CFG_LOW(channel),
DW_CFG_CH_FIFO_EMPTY,
DW_CFG_CH_FIFO_EMPTY,
PLATFORM_DMA_TIMEOUT);
if (ret < 0)
trace_dwdma_error("dw-dma: %d channel %d timeout",
dma->plat_data.id, channel);

/* channel can be disabled */
dw_write(dma, DW_DMA_CHAN_EN, CHAN_DISABLE(channel));
Comment thread
tlauda marked this conversation as resolved.
Outdated
Comment thread
tlauda marked this conversation as resolved.
Outdated

if (!chan->irq_disabled)
dw_write(dma, DW_CLEAR_BLOCK, 0x1 << channel);

Expand Down