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

ASoC: SOF: ipc3-topology: subtract ALH dai_index by 2 #5217

Open
wants to merge 1 commit into
base: topic/sof-dev
Choose a base branch
from

Conversation

bardliao
Copy link
Collaborator

We use SDW pin 2 as for ALH dai 0. However, IPC3 ALH dai index should start from 0.

Fixes: thesofproject/sof#9571

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

@bardliao do we need to state this is a fix in our commit message as some users SDW wont work without this patch ?

Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

I think this is way to go. A few comments on the commit message and inline commets. I wonder if we could somehow also highlight that we have to support existing topology files, so renumbering (in Linux code) is not an option.

if (flags & SOF_DAI_CONFIG_FLAGS_HW_PARAMS) {
/*
* DAI 2 is used as the first DAI for SDW in topology, normalize
* it to 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe refer to sof_sdw.c (something like "see SOC_SDW_INTEL_BIDIR_PDI_BASE in sof_sdw.c") defitinion to explain where the value "2" comes from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe refer to sof_sdw.c (something like "see SOC_SDW_INTEL_BIDIR_PDI_BASE in sof_sdw.c") defitinion to explain where the value "2" comes from?

Added. But on the second thought, this is Intel specific. Should we do it here?

@@ -1594,6 +1594,17 @@ static int sof_ipc3_widget_setup_comp_dai(struct snd_sof_widget *swidget)
if (ret < 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain in commit this is needed to support 3rd and 4th pins of SDW with IPC3 and Intel platforms. Older topology files have not used these pins, so this has not been an issue before (people will want to understand why this was not needed before and why this is needed now).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Commit message updated.

comp_dai->dai_index -= 2;
if (comp_dai->dai_index < 0) {
dev_err(sdev->dev,
"Invalid ALH dai index %d, Please use SDW Pin 2 and above\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we can say that only Pin numbers > 1 are supported instead of saying please use Pin 2

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe we can say that only Pin numbers > 1 are supported instead of saying please use Pin 2

Updated

Intel SoundWire machine driver always use bidirectional DAIs. And the
DAI index starts from 2. However, SOF IPC3 firmware assume all DAIs
are bidirectional and the index starts from 0. As a result, FW DAI 0
and 1 are never used. That worked fine because we use up to 2 DAIs in
a SDW link. But, a multi-function SDW codec needs to use 4 DAIs and
some platforms like Tiger Lake supports 4 bidirectional DAIs in each
SDW link. That means all bidirectional DAIs include DAI 0 and 1 will
be used. Therefore, we need to subtract ALH dai_index by 2 which is the
base of bidirectional DAIs.
IOW, currently, SDW0 Pin 2 which is the first bidirectional SDW Pin
will use FW DAI 2. But FW DAI 4 is invalid. With this commit SDW0 Pin 2
will use FW DAI 0 and SDW0 Pin 5 will use FW DAI 3 which is valid.

The issue exists since beginning. And the Fixes tag is the first commit
that this commit can be applied.

Fixes: b66bfc3 ("ASoC: SOF: sof-audio: Fix broken early bclk feature for SSP")
Signed-off-by: Bard Liao <[email protected]>
@bardliao
Copy link
Collaborator Author

@bardliao do we need to state this is a fix in our commit message as some users SDW wont work without this patch ?

Yes, Fixes tag added.

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.

[BUG] [IPC3] ALH Pin5 ipc tx error for 0x30010000.
4 participants