Repository navigation
Fix BDW machine drivers load issue with SOF - #1484
Conversation
plbossart
left a comment
plbossart
left a comment
There was a problem hiding this comment.
Not sure I understand the issue here.
I managed to do playback in the past on Broadwell/Samus without this change?
| SND_SOC_DAILINK_REG(ssp0_pin, codec, platform), | ||
| #else | ||
| SND_SOC_DAILINK_REG(dummy, codec, dummy), | ||
| #endif |
There was a problem hiding this comment.
why is this needed?
we've managed to make things work in the past without this change.
What is the problem and what are you fixing here?
There was a problem hiding this comment.
@plbossart The old one what to map to a dummy BE, but we only have a dummy BE with nocodec support.
Now nocodec is default disabled with HDA link enabled, so the change is required.
Or we should add dummy BE to SOF BDW platform drvier.
There was a problem hiding this comment.
@xiulipan with the nocodec machine drv, we dont use the dummy CPU DAI, we actually use the ones defined in the dai_drv[]. We use dummy dai only for the codec dai.
Anyway, how does it work with the broadwell legacy driver with the dummy CPU DAI? We never register the dummy codec DAI in SOF. So why isnt the dummy CPU DAI already registered too?
There was a problem hiding this comment.
Not sure, but from the test and log, it says
[ 217.561885] broadwell-audio broadwell-audio: info: override BE DAI link Codec
[ 217.561898] broadwell-audio broadwell-audio: ASoC: CPU DAI snd-soc-dummy-dai not registered
[ 218.048635] sof-audio-acpi INT3438:00: ipc rx: 0x90020000: GLB_TRACE_MSG
And we do have something in nocodec
Lines 53 to 54 in fd30230
|
@xiulipan are you missing virtual widgets maybe? |
|
@ranj063 I tried to add virtual widgets, but it will not solved the map issue here. It will still show error "SSP0 CODEC IN/OUT" do not have sink/source. I think the map to this codec related widgets is in the SST tplg. As we have our own tplg for this mapping, so I think it maybe easier to remove the map for this. |
|
@xiulipan can you please post the full error log? |
original states/without fix for BE dai linkwith fix for BE dai link onlywith fix for BE dai link and https://github.com/thesofproject/sof/pull/2095 |
|
@xiulipan what does "with fix for BE Dai link" mean? Sorry I am not following it at all. The last log shows this: This should be fixed with adding virtual widgets for "SSP0 CODEC IN" and "SSP0 CODEC OUT" in tplg |
I tried to add the diff --git a/tools/topology/sof-bdw-codec.m4 b/tools/topology/sof-bdw-codec.m4
index 902774f87208..8639464743c1 100644
--- a/tools/topology/sof-bdw-codec.m4
+++ b/tools/topology/sof-bdw-codec.m4
@@ -96,3 +96,6 @@ DAI_CONFIG(SSP, 0, 0, Codec,
SSP_CLOCK(fsync, 48000, codec_slave),
SSP_TDM(2, 25, 3, 3),
SSP_CONFIG_DATA(SSP, 0, 24)))
+
+VIRTUAL_WIDGET(SSP0 CODEC IN, out_drv, 1)
+VIRTUAL_WIDGET(SSP0 CODEC OUT, out_drv, 1)
but still got |
|
@ranj063 I redo some test and found I was fooled by m4. It expend the |
|
@xiulipan is this PR current or abandoned? I can't figure out what this is about? |
|
@plbossart I will make add an update to this PR and the tplg PR. The basic idea here is that in the BDW machine driver, we need |
8d20a48 to
ce4ed85
Compare
?? the bdw-rt6777 does not need any change of this nature, why would we need it for this machine driver? |
|
@plbossart But I will get some error messages: It says about we do not have Could you give some advice about the fixing? @ranj063 @kv2019i @bardliao linux/sound/soc/sof/intel/bdw.c Lines 566 to 574 in 6714c4e PS: I believe in the past we will have no such issue, as sof-nocodec will have some map like this. But as we have disable nocodec support by default now. I think there is no such DAI now. |
|
@xiulipan The dummy cpu and dai should be defined on soc-utils.c. Could you check why "ASoC: CPU DAI snd-soc-dummy-dai not registered"? |
|
@plbossart Test with an BDW RT5677 with latset sof-dev with kconfig-sof-default.sh It seems we need to handle these WoV thing for BDW RT5677? linux/sound/soc/intel/boards/bdw-rt5677.c Lines 298 to 302 in c985ebd linux/sound/soc/intel/boards/bdw-rt5677.c Lines 322 to 328 in c985ebd |
|
@bardliao @plbossart I dump the names for the dai in soc. diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 062653ab03a3..97a82de767ef 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -899,9 +899,13 @@ struct snd_soc_dai *snd_soc_find_dai(
/* Find CPU DAI from registered DAIs */
for_each_component(component) {
+ dev_info(component->dev, "PXL: dlc name %s, component name %s\n",
+ dlc->name, component->name);
if (!snd_soc_is_matching_component(dlc, component))
continue;
for_each_component_dais(component, dai) {
+ dev_info(component->dev, "PXL: dlc dai_name %s, component dai_name %s\n",
+ dlc->dai_name, dai->name);
if (dlc->dai_name && strcmp(dai->name, dlc->dai_name)
&& (!dai->driver->name
|| strcmp(dai->driver->name, dlc->dai_name)))I think we will still the fix to set the right dai name for SOF in the machine drivers when work with SOF. |
ce4ed85 to
f9a8f26
Compare
|
@plbossart @bardliao It seems there is So if we want to use SOF as platform driver, we need to change the dai-name to match what we have in SOF platform driver. This PR test with thesofproject/sof#2188 and thesofproject/sof#2095 on both bdw-rt5677 and bdw-rt286. @fredoh9 can you help to check if the sound output on bdw-rt5677 is good? |
|
@xiulipan I have a suspicion there was an error in the patch 3f6c2a2. I believe the right fix for this issue might be: diff --git a/sound/soc/intel/boards/bdw-rt5677.c b/sound/soc/intel/boards/bdw-rt5677.c
index 2af8e5a62da8..322f514788ef 100644
--- a/sound/soc/intel/boards/bdw-rt5677.c
+++ b/sound/soc/intel/boards/bdw-rt5677.c
@@ -286,6 +286,9 @@ static int bdw_rt5677_init(struct snd_soc_pcm_runtime *rtd)
SND_SOC_DAILINK_DEF(dummy,
DAILINK_COMP_ARRAY(COMP_DUMMY()));
+SND_SOC_DAILINK_DEF(dummy_platform,
+ DAILINK_COMP_ARRAY(COMP_PLATFORM("snd-soc-dummy")));
+
SND_SOC_DAILINK_DEF(fe,
DAILINK_COMP_ARRAY(COMP_CPU("System Pin")));
@@ -342,7 +345,7 @@ static struct snd_soc_dai_link bdw_rt5677_dais[] = {
.dpcm_playback = 1,
.dpcm_capture = 1,
.init = bdw_rt5677_init,
- SND_SOC_DAILINK_REG(dummy, be, dummy),
+ SND_SOC_DAILINK_REG(dummy, be, dummy_platform),
},
};
|
@xiulipan hmm this isnt the right fix after all. But you know what I am not sure why the dummy comp isnt working. Take a look at this new machine drv from Curtis. This has very similar dai link definitions too: |
@xiulipan could you please give my patch above a try anyway and see what happens? Actually a better change would just be: diff --git a/sound/soc/intel/boards/bdw-rt5677.c b/sound/soc/intel/boards/bdw-rt5677.c
index 2af8e5a62da8..3ac49cf0f6a1 100644
--- a/sound/soc/intel/boards/bdw-rt5677.c
+++ b/sound/soc/intel/boards/bdw-rt5677.c
@@ -342,7 +342,7 @@ static struct snd_soc_dai_link bdw_rt5677_dais[] = {
.dpcm_playback = 1,
.dpcm_capture = 1,
.init = bdw_rt5677_init,
- SND_SOC_DAILINK_REG(dummy, be, dummy),
+ SND_SOC_DAILINK_REG(dummy, be, platform),
},
};
|
@xiulipan So the right fix for the BDW machine drv probe problem is as above and @cujomalainey has helped me verify it with the legacy SST driver as well. Here's the rationale for it: " With the introduction of snd_soc_fixup_dai_links_platform_name() to override the platform name, the CPU and platform components for the DAI links cannot be the same anymore. In the case of the bdw-rt5677 driver, using the same dummy component for the CPU and platform components results in the CPU component's name also getting overridden by the platform name set by the SOF driver. This results in DAI link binding failure because of the component name mismatch when searching for the snd-soc-dummy-dai CPU DAI. In order to prevent this, change the platform component so that it is different from the CPU component. This change has also been tested to work with the legacy driver" @plbossart let me know what you think of this. |
|
In this case it was tested on the bdw-rt5650 driver, but I can also test on the 5677 if needed |
|
@ranj063 @bardliao I have confirmed the issue here. It seems we have the same structure for the So I have some try like following: diff --git a/sound/soc/intel/boards/broadwell.c b/sound/soc/intel/boards/broadwell.c
index db7e1e87156d..d98ef549e9ad 100644
--- a/sound/soc/intel/boards/broadwell.c
+++ b/sound/soc/intel/boards/broadwell.c
@@ -158,6 +158,9 @@ SND_SOC_DAILINK_DEF(loopback,
SND_SOC_DAILINK_DEF(dummy,
DAILINK_COMP_ARRAY(COMP_DUMMY()));
+SND_SOC_DAILINK_DEF(dummyplatform,
+ DAILINK_COMP_ARRAY(COMP_DUMMY()));
+
SND_SOC_DAILINK_DEF(platform,
DAILINK_COMP_ARRAY(COMP_PLATFORM("haswell-pcm-audio")));
@@ -218,7 +221,7 @@ static struct snd_soc_dai_link broadwell_rt286_dais[] = {
.ops = &broadwell_rt286_ops,
.dpcm_playback = 1,
.dpcm_capture = 1,
- SND_SOC_DAILINK_REG(dummy, codec, dummy),
+ SND_SOC_DAILINK_REG(dummy, codec, dummyplatform),
},
};Now the fix up will not influence the CPU anymore So I think we need to add some warning to mention this issue when use @morimoto Could you take a look about our analysis about this issue to see if we need to add any patch to the or we just should not use same dai_link for cpu and platform from the beginning. |
@xiulipan like I mentioned in my comment, they do have to be different comps. Having just one macro for COMP_DUMMY() and using the same everywhere will lead to conflicts we're seeing. @morimoto could you please confirm this? |
| .ops = &bdw_rt5677_dsp_ops, | ||
| SND_SOC_DAILINK_REG(dsp), | ||
| }, | ||
| #endif |
There was a problem hiding this comment.
this is wrong I believe. @cujomalainey please correct me if I am smoking.
the WakeOnVoice is handled by the rt5677 codec IIRC, so there's no reason why it should be commented out.
However we need to check that the FE override does not prevent this from working.
There was a problem hiding this comment.
@plbossart the problem is this is a non-DPCM link and we do not override it at all with SOF. This was one of the questions @dbaluta was asking as well.
There was a problem hiding this comment.
it's good if it's not overridden!
There was a problem hiding this comment.
@xiulipan care to explain why we need to comment this out?
There was a problem hiding this comment.
The only dependency on the platform is a DAPM link to force the I2S clk. If SOF can provide that clock then WoV should work fine with SOF as the audio bypasses the DSP on a SPI link.
There was a problem hiding this comment.
@cujomalainey can you elaborate here? I don't remember what the expectation is for clocks here, and quite frankly whether we use SOF or not the hardware is the same, so there's not really any magic we can do...
There was a problem hiding this comment.
The RT5677 needs the I2S MCLK running in order to drive its DSP. Locally we have 0d2135e reverted (the same patch that causes DMA boot issues on samus and DSP crashes on buddy.) By reverting it, the kernel allows the bdw DSP to suspend and leave the MCLK running so everything is fine for us.
If we don't have that clock bad things start to happen very quickly. All I2C writes silently fail is one example and the registers read back garbage values (usually 0xBEEF).
There was a problem hiding this comment.
The MCLK is not controlled by the DSP, did you mean SCLK or BCLK?
I read rt5677 and thought CHT/BSW.
For Broadwell I don't think we play with the audio PLL and change the MCLK in the SOF driver. The code you are referring to is only used by the legacy, and no one knows what the issue might be...
There was a problem hiding this comment.
Nope I mean MCLK. It is controlled by the DSP kernel driver, at least that is where it is in SST builds.
There was a problem hiding this comment.
@cujomalainey @plbossart I will test with this FE kept and see if we can find the link there.
Then I think we can decided if we need to keep this here.
plbossart
added
BDW
|
update, blocked by new regression thesofproject/sof#2227 |
f9a8f26 to
c639a81
Compare
xiulipan
changed the title
|
@plbossart @ranj063 Update the patch. Logs for the codec dai load, no |
ranj063
left a comment
ranj063
left a comment
There was a problem hiding this comment.
typos in commit message "crete" "sperate"
|
find the rt5677-spi could not be probed. Also test with SST driver on
the device. The machine driver can also not be loaded. I think the WoV
feature may need some other effect to enable. So I disable the WoV
feature to let SOF probe.
@cujomalaney, are we missing a configuration here for the SPI?
|
|
Is CONFIG_SPI enabled? |
should be, we have all this enabled in the base-defconfig: |
|
Ah, i didn't realize there was a |
c639a81 to
747f242
Compare
The legacy driver uses dummy cpu_dai and platform, SOF requires actual values to bind. Signed-off-by: Pan Xiuli <xiuli.pan@linux.intel.com>
747f242 to
1ee2657
Compare
commented
Jan 7, 2020
|
@xiulipan can you clarify for me what the status is on BDW/rt5640? Do you get clear sound on capture and playback? Do you have a UCM file? |
commented
Jan 7, 2020
also @lgirdwood do you have a Broadwell XPS on your side to test this out? |
commented
Jan 7, 2020
|
@plbossart I do, will start charging it (version with funky BIOS where you need to enabled DSP in BIOS and boot Linux twice before DSP is really enabled by BIOS). |
commented
Jan 8, 2020
|
@plbossart I only test on a BDW rt286. |
commented
Jan 8, 2020
right, yes it's haswell that has rt5640. |
SND_SOC_DAILINK_DEF will crete structure, if reuse the same dummy structure
for cpu dai and platform. snd_soc_fixup_dai_links_platform_name will not only
overwrite the platform name but also the CPU DAI name.
Fix the issue by create a sperate dummyplatform for dummy platform.
Wake on voice is not supported for SOF on BDW, so ignore the dai.