Skip to content

Conversation

@brentlu
Copy link
Contributor

@brentlu brentlu commented May 28, 2021

This PR is for discussion in #4219 to solver the hw_free issue, see comment #4219 (comment)

bardliao and others added 3 commits May 29, 2021 02:06
So that we can stop ssp clock in dai_reset.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
(cherry picked from commit 7feb456)
There is an STREAM_PCM_FREE IPC command which is sent to a SOF
pipeline in trigger stop. Here we add an new DAI_HW_FREE command which
will be sent to SOF DAI component. The command is sent from BE DAI
link so codec driver has chance to run code in the mute_stream()
callback before the DAI_HW_FREE IPC command is sent.

Signed-off-by: Brent Lu <brent.lu@intel.com>
We will enable/disable ssp clock in the ops.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
(cherry picked from commit 3ffe417)
@brentlu brentlu marked this pull request as draft May 28, 2021 19:19
@brentlu brentlu changed the title Dai hw free ipc: add DAI_HW_FREE IPC command May 29, 2021
@kv2019i kv2019i added the ABI ABI change involved label May 31, 2021
@kv2019i
Copy link
Collaborator

kv2019i commented May 31, 2021

Although this is backwards compatible, this needs to follow ABI change process:
https://thesofproject.github.io/latest/contribute/process/abiprocess.html

//return ipc_comp_set_value(header, COMP_CMD_LOOPBACK);
return -EINVAL;
case SOF_IPC_DAI_HW_FREE:
return ipc_msg_dai_hw_free(header);
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have SOF_IPC_STREAM_PCM_FREE which is sent at ALSA PCM .hw_free(), why we need a new IPC, just call dai_hw_free() in dai_reset() as @bardliao do in PR #4219 is good enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For two reasons, first the PCM_FREE is 'usually' sent in trigger stop now. Second, we need to send something in BE DAI's hw_free instead of FE DAI or the SOF will still stop the bclk too early.

DROP:
sof's trigger()
=> send STREAM_TRIG_STOP
=> send STREAM_PCM_FREE
HW_FREE:
sof's hw_free() (FE)
=> too early, codec could do nothing
codec's mute_stream()
=> codec could handle it's PLL here
sof's hw_free() (BE)
=> send DAI_HW_FREE to SOF to stop bclk
codec's hw_free()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, this PR is meant to be merged to #4219 if agreed. To fix the issue found by codec vendor when validating #4219.

Copy link
Contributor

Choose a reason for hiding this comment

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

got the issue now @brentlu
IMO we can still use the existed _IPC_STREAM_PCM_FREE but change the point of sending it, e.g. send it later, at the point where we want the clocks to be freed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems weird for me to move it from FE to BE DAI Link since this command deals with PCM pipe...

@lgirdwood
Copy link
Member

@brentlu lets get input from @plbossart here as this is probably more complex for the kernel than the FW.

@plbossart
Copy link
Member

plbossart commented Jun 1, 2021

I don't understand any of this @brentlu @lgirdwood @keyonjie

a) "the PCM_FREE is 'usually' sent in trigger stop now." Why is this the case? This seems like a major bug to me.
b) I am not hot on having parts of the DAI management done at the DSP level (e.g. some calls done using the dai_driver callback) and some directly sent by the kernel. This is a recipe for state machines going sideways on both sides.

@brentlu
Copy link
Contributor Author

brentlu commented Jun 1, 2021

@brentlu lets get input from @plbossart here as this is probably more complex for the kernel than the FW.

Sorry I forgot to mention kernel PR is this one:
thesofproject/linux#2951

@brentlu
Copy link
Contributor Author

brentlu commented Jun 6, 2021

we are not adding extra ipc command now.

@brentlu brentlu closed this Jun 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ABI ABI change involved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants