Skip to content
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.

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

[RFC] ASoC: max98373: power up amp before pipeline start #2991

Closed

Conversation

RanderWang
Copy link

@RanderWang RanderWang commented Jun 11, 2021

Since amp will be powered up 31ms after pipeline start,
overrun will happen in audio dsp fw and result to fw
error. This patch powers up amp before pipeline start.

The error is caused by : 3a27875

fixes thesofproject/sof#4066

@RanderWang RanderWang changed the title ASoC: max98373: power up amp before pipeline start [RFC] ASoC: max98373: power up amp before pipeline start Jun 11, 2021
@RanderWang RanderWang force-pushed the max98373-pre-power branch 2 times, most recently from 74cf87c to e308e37 Compare June 11, 2021 07:48
Since amp will be powered up 31ms after pipeline start,
overrun will happen in audio dsp fw and result to fw
error. This patch powers up amp before pipeline start.

Signed-off-by: Rander Wang <rander.wang@intel.com>
@@ -24,7 +24,7 @@ static int max98373_dac_event(struct snd_soc_dapm_widget *w,
struct max98373_priv *max98373 = snd_soc_component_get_drvdata(component);

switch (event) {
case SND_SOC_DAPM_POST_PMU:
case SND_SOC_DAPM_PRE_PMU:

Choose a reason for hiding this comment

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

PRE_PMU and POST_PMD looks symmetric to me, @ryans-lee any comment?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@keyonjie PRE_PMU means the event will be trigger before the power bit (MAX98373_R202B_PCM_RX_EN) is set and POST_PMU means the event will be trigger after the power bit. PMU and PMD don't have to be symmetric.

Choose a reason for hiding this comment

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

@bardliao do you know if we should set R20FF first or enable R202B first? looks we should enable the global (20FF) first and then enable the PCM_RX_EN? I don't know the codec well, but in case you power on A first and then B, for powering off, we should do with power off B first and then A? That is why symmetric I proposed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bardliao do you know if we should set R20FF first or enable R202B first? looks we should enable the global (20FF) first and then enable the PCM_RX_EN? I don't know the codec well, but in case you power on A first and then B, for powering off, we should do with power off B first and then A? That is why symmetric I proposed.

No, I don't know that. Maybe @ryans-lee can help.

Copy link
Collaborator

@bardliao bardliao left a comment

Choose a reason for hiding this comment

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

@RanderWang You have to modify the flag in line 66. ie. It should be
SND_SOC_DAPM_DAC_E("Amp Enable", "HiFi Playback",
MAX98373_R202B_PCM_RX_EN, 0, 0, max98373_dac_event,
SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),

@bardliao
Copy link
Collaborator

@RanderWang Sorry, I don't get how can codec power up sequence lead to overrun. Data will be transmitted no matter codec power is up or off, right?

@plbossart
Copy link
Member

@RanderWang I don't see the link between powering up and overflows. The Intel DSP would start sending and capturing data regardless of the state of the device. If the Amp is not powered, it'll discard data for playback and not generate anything on the capture side.

Please clarify how a power states has side effects on data streaming, thank you.

@plbossart
Copy link
Member

@RanderWang We routinely use the SSP in NOCODEC mode, so I really really don't see what your proposal might change. The SSP is the clock master, what happens outside of the DSP is irrelevant for underflows.

@keyonjie
Copy link

@RanderWang We routinely use the SSP in NOCODEC mode, so I really really don't see what your proposal might change. The SSP is the clock master, what happens outside of the DSP is irrelevant for underflows.

@plbossart the issue Rander is addressing here if for SDW codec mode, not SSP.

@plbossart
Copy link
Member

@RanderWang We routinely use the SSP in NOCODEC mode, so I really really don't see what your proposal might change. The SSP is the clock master, what happens outside of the DSP is irrelevant for underflows.

@plbossart the issue Rander is addressing here if for SDW codec mode, not SSP.

it does not matter @keyonjie, we can have SoundWire in almost-nocodec mode as well with the 'mockup' device support.

There is really no reason why power-up a device would cause underflows at the Intel DSP level.

@RanderWang
Copy link
Author

@plbossart @bardliao @keyonjie The issue is caused by delay, not power up of codec. please check #2997.

close it since we have a new PR.

@RanderWang RanderWang closed this Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TGL][SDW] XRUN causes multiple pipeline capture test to fail
4 participants