Skip to content

ASoC: SOF: Intel: hda: make sure the DMA is started/stopped by checking the RUN bit #997

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
plbossart merged 2 commits into from
Jun 11, 2019
Merged

ASoC: SOF: Intel: hda: make sure the DMA is started/stopped by checking the RUN bit #997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0864aef
ASoC: SOF: Intel: hda: make sure DMA is start/stop by read the RUN bit
zhuyingjiang May 31, 2019
16dc2ab
ASoC: SOF: Intel: hda: make sure RUN bit setting to 0 during clear st…
zhuyingjiang May 31, 2019
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
44 changes: 44 additions & 0 deletions sound/soc/sof/intel/hda-stream.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,9 @@ int hda_dsp_stream_trigger(struct snd_sof_dev *sdev,
{
struct hdac_stream *hstream = &stream->hstream;
int sd_offset = SOF_STREAM_SD_OFFSET(hstream);
u32 dma_start = SOF_HDA_SD_CTL_DMA_START;
int ret;
u32 run;

/* cmd must be for audio stream */
switch (cmd) {
Expand All @@ -234,6 +237,16 @@ int hda_dsp_stream_trigger(struct snd_sof_dev *sdev,
SOF_HDA_SD_CTL_DMA_START |
SOF_HDA_CL_DMA_SD_INT_MASK);

ret = snd_sof_dsp_read_poll_timeout(sdev,
HDA_DSP_HDA_BAR,
sd_offset, run,
((run & dma_start) == dma_start),
HDA_DSP_REG_POLL_INTERVAL_US,
HDA_DSP_STREAM_RUN_TIMEOUT);

if (ret)
return ret;

hstream->running = true;
break;
case SNDRV_PCM_TRIGGER_SUSPEND:
Expand All @@ -244,6 +257,15 @@ int hda_dsp_stream_trigger(struct snd_sof_dev *sdev,
SOF_HDA_SD_CTL_DMA_START |
SOF_HDA_CL_DMA_SD_INT_MASK, 0x0);

ret = snd_sof_dsp_read_poll_timeout(sdev, HDA_DSP_HDA_BAR,
sd_offset, run,
!(run & dma_start),
HDA_DSP_REG_POLL_INTERVAL_US,

@lgirdwood lgirdwood Jun 5, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

((run & SOF_HDA_SD_CTL_DMA_START) == 0x0), is a lot for a parameter, it's always better to define the variable on an earlier line e.g.
run_value = !(run &SOF_HDA_SD_CTL_DMA_START)
This can be fixed incrementally.

@plbossart plbossart Jun 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

agree with @lgirdwood please add an intermediate variable and simplify this.

@zhuyingjiang zhuyingjiang Jun 10, 2019 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@lgirdwood @plbossart @kv2019i
The snd_sof_dsp_read_poll_timeout is a macro #define, not a really function, if using predefined variable as @lgirdwood suggested, the result should be not the same. Because the #define need first get the value of "run", and then the run & SOF_HDA_SD_CTL_DMA_START can be calculated.
@kv2019i Do you agree?

Another solution maybe define a macro for run & SOF_HDA_SD_CTL_DMA_START, as:
1):
#define RUN_VALUE ((run) & SOF_HDA_SD_CTL_DMA_START)

and use RUN_VALUE == SOF_HDA_SD_CTL_DMA_START and RUN_VALUE == 0 as the parameter.

or
2):
#define RUN_DMA_START (((run) & SOF_HDA_SD_CTL_DMA_START) == SOF_HDA_SD_CTL_DMA_START)
#define RUN_DMA_STOP (((run) & SOF_HDA_SD_CTL_DMA_START) == 0x0)

and use as:

		ret = snd_sof_dsp_read_poll_timeout(sdev, HDA_DSP_HDA_BAR,
						sd_offset, run, RUN_DMA_START,
						HDA_DSP_REG_POLL_INTERVAL_US,
						HDA_DSP_STREAM_RUN_TIMEOUT);

What do you think?

@kv2019i kv2019i Jun 10, 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.

@zhuyingjiang I'm ok with the original as well -- this is used elsewhere in the codebase already. I'm not sure how much this helps readability to define more macros. Upto @plbossart and @lgirdwood who made the review comments.

One more option is to use a local variable to describe the mask only:

                u32 dma_start = SOF_HDA_SD_CTL_DMA_START;
		ret = snd_sof_dsp_read_poll_timeout(sdev, HDA_DSP_HDA_BAR,
						sd_offset, run, !(run & dma_start),
						HDA_DSP_REG_POLL_INTERVAL_US,

But yeah, the improvement is limited.

@plbossart plbossart Jun 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i like this update much better...

@zhuyingjiang zhuyingjiang Jun 11, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

OK, thanks @kv2019i , I will update with this solution.

HDA_DSP_STREAM_RUN_TIMEOUT);

if (ret)
return ret;

snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR, sd_offset +
SOF_HDA_ADSP_REG_CL_SD_STS,
SOF_HDA_CL_DMA_SD_INT_MASK);
Expand Down Expand Up @@ -273,7 +295,9 @@ int hda_dsp_stream_hw_params(struct snd_sof_dev *sdev,
struct hdac_stream *hstream = &stream->hstream;
int sd_offset = SOF_STREAM_SD_OFFSET(hstream);
int ret, timeout = HDA_DSP_STREAM_RESET_TIMEOUT;
u32 dma_start = SOF_HDA_SD_CTL_DMA_START;
u32 val, mask;
u32 run;

if (!stream) {
dev_err(sdev->dev, "error: no stream available\n");
Expand All @@ -294,6 +318,16 @@ int hda_dsp_stream_hw_params(struct snd_sof_dev *sdev,
snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR, sd_offset,
SOF_HDA_CL_DMA_SD_INT_MASK |
SOF_HDA_SD_CTL_DMA_START, 0);

ret = snd_sof_dsp_read_poll_timeout(sdev, HDA_DSP_HDA_BAR,
sd_offset, run,
!(run & dma_start),
HDA_DSP_REG_POLL_INTERVAL_US,
HDA_DSP_STREAM_RUN_TIMEOUT);

if (ret)
return ret;

snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR,
sd_offset + SOF_HDA_ADSP_REG_CL_SD_STS,
SOF_HDA_CL_DMA_SD_INT_MASK,
Expand Down Expand Up @@ -346,6 +380,16 @@ int hda_dsp_stream_hw_params(struct snd_sof_dev *sdev,
snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR, sd_offset,
SOF_HDA_CL_DMA_SD_INT_MASK |
SOF_HDA_SD_CTL_DMA_START, 0);

ret = snd_sof_dsp_read_poll_timeout(sdev, HDA_DSP_HDA_BAR,
sd_offset, run,
!(run & dma_start),
HDA_DSP_REG_POLL_INTERVAL_US,
HDA_DSP_STREAM_RUN_TIMEOUT);

if (ret)
return ret;

snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR,
sd_offset + SOF_HDA_ADSP_REG_CL_SD_STS,
SOF_HDA_CL_DMA_SD_INT_MASK,
Expand Down
6 changes: 6 additions & 0 deletions sound/soc/sof/intel/hda.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,12 @@
#define HDA_DSP_MBOX_UPLINK_OFFSET 0x81000

#define HDA_DSP_STREAM_RESET_TIMEOUT 300
/*
* Timeout in us, for setting the stream RUN bit, during
* start/stop the stream. The timeout expires if new RUN bit
* value cannot be read back within the specified time.
*/
#define HDA_DSP_STREAM_RUN_TIMEOUT 300

@plbossart plbossart Jun 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

still doesn't tell me where this value comes from, is this a hardware recommendation or 'same as the other drivers'

@zhuyingjiang zhuyingjiang Jun 10, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@plbossart it is "same as the other drivers"

#define HDA_DSP_CL_TRIGGER_TIMEOUT 300

#define HDA_DSP_SPIB_ENABLE 1
Expand Down