ASoC:SOF:APL:set I2S slave before enabling DSP #813

zhuyingjiang · 2019-04-11T06:23:42Z

By default, the I2S ports are in master mode, during
DSP powerup sequences, this may cause distortion on
some boards for drives FS and BCLK high.
So we need to set it as slave before enabling DSP.
closes #778
Signed-off-by: Zhu Yingjiang yingjiang.zhu@linux.intel.com

ranj063 · 2019-04-11T06:51:10Z

So we need to set it as slave before enabling DSP.

@zhuyingjiang im not aware of what the sequence should be but in the commit, you set the slave mode after powering up the DSP's. It seems to contradict the commit message?

ranj063 · 2019-04-11T06:52:59Z

@zhuyingjiang also wondering why we do it only for APL and not the other cavs platforms?

zhuyingjiang · 2019-04-11T07:11:45Z

So we need to set it as slave before enabling DSP.

@zhuyingjiang im not aware of what the sequence should be but in the commit, you set the slave mode after powering up the DSP's. It seems to contradict the commit message?

That is "during power up sequence", which include:

power up core
purge FW
unset core 0
wait for IPC done
5 .power down core
....
So power up sequence including "power up core".

zhuyingjiang · 2019-04-11T07:40:52Z

@zhuyingjiang also wondering why we do it only for APL and not the other cavs platforms?

@ranj063 this is for issue #778, and from the cAVS 1.8 spec, I can't find the "I2S Force Dynamic Clock Gating" bit, but it does have "set slave" in SSC1 register.
For SOC earlier than SKL/APL(cAVS1.5) , I still have not got the spec.
@plbossart Could you explain more for @ranj063 's query? do we still need enable that on CNL?

plbossart

@zhuyingjiang the ask is to make sure valid sequences are programmed, not just to mirror what others did and hope for the best. You need to get access to the documentation and double-check all values, I did what I could to verify but you need to go deeper. In addition it's not quite right to a new operation just for a sequence that takes place inside the boot flow.

plbossart · 2019-04-11T15:44:54Z

sound/soc/sof/ops.h

this abstraction layer is not needed, this is a platform-specific operation that should be part of the initial DSP boot process. There's no point in exposing this to the core or add an intel-specific operation here.

@plbossart OK, I will get the doc of BDW/HSW and see if they are the same step of doing so, and I will still check if the clock gating step is not needed, if yes, we do not need this layer.

plbossart · 2019-04-11T15:45:06Z

sound/soc/sof/sof-priv.h

no, this is not needed

OK, thanks!

plbossart · 2019-04-11T15:56:02Z

sound/soc/sof/intel/apl.c

this is a shortcut to set the SCLKDIR (bit 25) and SFRMDIR (bit 24) fields of register SSC1 (offset 4 of SSP base). For both fields 1 means slave and 0 master.
You'd want to test that these bits are actually set since access from the host might be disabled on some platforms due to the access policy.

The registers are identical for cAVS 1.8, but you may need to check that the base SSP address is the same.

@plbossart OK, will do, but cAVS1.8 less the clock gating registers , will verify on cAVS1.5 if APL_DISABLE_ALL_SSP_CLK_GT operation can be omit.

plbossart · 2019-04-11T15:59:24Z

sound/soc/sof/intel/apl.c

that makes 6 bits, and looks awfully correlated to the number of SSPs. for other platforms you'd need to adjust this.

at the same time, I do not see anything in the IP reference on this, except for some bits called "I2S Force Dynamic Clock Gating" and "I2S Extension Force Dynamic Clock Gating" which have a different bit field in the CLKSTS register. clearly you need to make sure the registers you are using are valid, not just copy/paste from somewhere else without verifying the right bits are programmed with the right values.

@plbossart OK, will check if "I2S Force Dynamic Clock Gating" setting can be omit.

zhuyingjiang · 2019-04-13T03:17:38Z

@plbossart Do you means on all platform including BDW/HSW, APL(cAVS1.5), and CNL(cAVS1.8) need enable the feature? I will get the BDW/HSW specs and go deep with the specs then refine the PR.
Thanks!
I only changed the APL related is because that the issue #778 has the "[APL][GLK] harden" in title, so this is a misunderstanding.

plbossart · 2019-04-15T12:36:58Z

On 4/12/19 10:17 PM, Zhu, Yingjiang wrote: @plbossart <https://github.com/plbossart> Do you means on all platform including BDW/HSW, APL(cAVS1.5), and CNL(cAVS1.8) need enable the feature? I will get the BDW/HSW specs and go deep with the specs then refine the PR. Thanks! I only changed the APL related is because that the issue #778 <#778> has the "[APL][GLK] harden" in title, so this is a misunderstanding.

it's only needed on SKL+, e.g. cAVS 1.5 and 1.8.

plbossart · 2019-04-17T20:56:03Z

@zhuyingjiang The ask was to enable the SSP as slave before the firmware is loaded, not to use the SSP as slave unconditionally! The firmware should do what the topology wants. The benefit is that if the topology does want slave mode, then there is no transition to master first which drives the clocks before stopping to drive them.

Also it's possible that some platforms set the access policy in the BIOS/Coreboot and the writing from the host fails. That's ok. Those platforms would not use slave mode anyways.

plbossart

see comments below.

sound/soc/sof/intel/cnl.c

plbossart · 2019-04-17T20:57:09Z

sound/soc/sof/intel/cnl.c

certainly not 8. where did you read this?

@plbossart yes, it is 8 for max, but maybe some ports reserved.
From cAVS1.8 HAS:

I2SPC | 3 | I2S Port Count.Value of 0 – 8 is allowed

and on our current WHL RVP, the I2SPC is 3, but there have base address from 0x10000 (I2S0) to 0x17000 (I2S7). The last 5 are reserved, we can write but will not take effect.

So use 3 then...

OK, thanks!

sound/soc/sof/intel/hda-loader.c

sound/soc/sof/intel/hda.h

zhuyingjiang · 2019-04-18T05:47:47Z

@zhuyingjiang The ask was to enable the SSP as slave before the firmware is loaded, not to use the SSP as slave unconditionally! The firmware should do what the topology wants. The benefit is that if the topology does want slave mode, then there is no transition to master first which drives the clocks before stopping to drive them.

@plbossart The code I attached above to modify in Firmware side is to do debug and validate. it is not submit to FW repo.

From the spec, the host need to set the SSP to slave after poweron to avoid distortion, then when the DSP owns the access right it can set it according to the topology, by reading from the IPC message the host sent

zhuyingjiang · 2019-04-18T07:03:16Z

@plbossart updated, with separate patch.

plbossart

see comments below.
You want to use ASoC: SOF: Intel: hda: as the commit title prefix, thanks.

sound/soc/sof/intel/hda.h

plbossart · 2019-04-18T14:23:56Z

sound/soc/sof/intel/hda.h

no, I only see 3 SSPs for CNL/WHL/CML. You are confusing IP options and SOC reality, please look at the right documentation.

OK, changed.

zhuyingjiang · 2019-04-19T01:50:05Z

@plbossart @ranj063 Changed the commit message and the CNL SSP count.

keyonjie · 2019-04-19T07:23:18Z

@zhuyingjiang can you add a space after each colon between sections in your subject of each commits, e.g.
ASoC: SOF: APL: set I2S slave before enabling DSP

The DSP SSP device memory can be conditionally accessed by the host(depending on access policy). Add the SSP base memory offset of APL and CNL. Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com>

add SSP info of APL and CNL, to the sof_intel_dsp_desc structure. The max SSP count the platform support and the SSP base address. Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com>

zhuyingjiang · 2019-04-19T07:50:00Z

@keyonjie Updated, thanks!

plbossart

Thanks @zhuyingjiang for taking the feedback into account, looks almost good.

One last change to polish this PR, please fix the last commit message:

By default, the I2S ports are configured in master mode during
DSP powerup sequences, and the FS and BCLK lines will be driven on startup, even when the topology file explicitly requires the SSP to be slave.

This may be problematic for external components configured in master mode who don't expect the Intel SOC/PCH to drive. Fix by configuring the SSP as slave before the SSP outputs are enabled to avoid this transient behavior.

When the topology file configures the SSP as clock master, the initial slave configuration will be overridden.

Also please be consistent in commit titles, using Intel with a capital 'I' shows you've paid attention to details.

By default, the I2S ports are configured in master mode during DSP powerup sequences, the FS and BCLK lines will be driven on startup, even when the topology file explicitly requires the SSP to be slave. This may be problematic for external components configured in master mode who don't expect the Intel SOC/PCH to drive. Fix by configuring the SSP as slave before the SSP outputs are enabled to avoid this transient behavior. When the topology file configures the SSP as clock master, the initial slave configuration will be overridden. Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com>

zhuyingjiang · 2019-04-20T02:17:14Z

@plbossart Updated, thanks!

plbossart · 2019-04-22T15:38:58Z

one additional reviewer needed.

ranj063

@plbossart the change looks good to me. I havent read the documentation to confirm the APL/CNL SSP counts but if you are good with it, it should be fine.

plbossart · 2019-04-22T15:57:13Z

@plbossart the change looks good to me. I havent read the documentation to confirm the APL/CNL SSP counts but if you are good with it, it should be fine.

thanks @ranj063

plbossart requested changes Apr 11, 2019

View reviewed changes

zhuyingjiang force-pushed the topic/apl-set-I2S-as-slave branch from 3ebe44a to 25bf592 Compare April 17, 2019 10:58

zhuyingjiang changed the title ~~ASoC:SOF:APL:set I2S slave before enabling DSP~~ [RFC]ASoC:SOF:APL:set I2S slave before enabling DSP Apr 17, 2019

zhuyingjiang requested review from plbossart and ranj063 April 17, 2019 11:21

plbossart requested changes Apr 17, 2019

View reviewed changes

zhuyingjiang force-pushed the topic/apl-set-I2S-as-slave branch from 25bf592 to 72ee42f Compare April 18, 2019 06:54

zhuyingjiang requested a review from plbossart April 18, 2019 07:03

plbossart requested changes Apr 18, 2019

View reviewed changes

zhuyingjiang force-pushed the topic/apl-set-I2S-as-slave branch from 72ee42f to 3a75b1e Compare April 19, 2019 01:48

zhuyingjiang requested review from keyonjie and plbossart April 19, 2019 01:50

zhuyingjiang changed the title ~~[RFC]ASoC:SOF:APL:set I2S slave before enabling DSP~~ ASoC:SOF:APL:set I2S slave before enabling DSP Apr 19, 2019

zhuyingjiang added 2 commits April 19, 2019 15:42

ASoC: SOF: Intel: hda: add the SSP Host Device memory space

2fbac53

The DSP SSP device memory can be conditionally accessed by the host(depending on access policy). Add the SSP base memory offset of APL and CNL. Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com>

ASoC: SOF: Intel: hda: add SSP info to the chip info struct

1ebc619

add SSP info of APL and CNL, to the sof_intel_dsp_desc structure. The max SSP count the platform support and the SSP base address. Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com>

zhuyingjiang force-pushed the topic/apl-set-I2S-as-slave branch from 3a75b1e to ecb2123 Compare April 19, 2019 07:48

plbossart requested changes Apr 19, 2019

View reviewed changes

zhuyingjiang force-pushed the topic/apl-set-I2S-as-slave branch from ecb2123 to c77ad74 Compare April 20, 2019 02:14

plbossart approved these changes Apr 22, 2019

View reviewed changes

ranj063 approved these changes Apr 22, 2019

View reviewed changes

plbossart merged commit 596d392 into thesofproject:topic/sof-dev Apr 22, 2019

ASoC:SOF:APL:set I2S slave before enabling DSP #813

ASoC:SOF:APL:set I2S slave before enabling DSP #813

Uh oh!

Conversation

zhuyingjiang commented Apr 11, 2019

Uh oh!

ranj063 commented Apr 11, 2019

Uh oh!

ranj063 commented Apr 11, 2019

Uh oh!

zhuyingjiang commented Apr 11, 2019

Uh oh!

zhuyingjiang commented Apr 11, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhuyingjiang commented Apr 13, 2019

Uh oh!

plbossart commented Apr 15, 2019 via email

Uh oh!

plbossart commented Apr 17, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhuyingjiang Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zhuyingjiang commented Apr 18, 2019

Uh oh!

zhuyingjiang commented Apr 18, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhuyingjiang commented Apr 19, 2019

Uh oh!

keyonjie commented Apr 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhuyingjiang commented Apr 19, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

zhuyingjiang commented Apr 20, 2019

zhuyingjiang Apr 18, 2019 •

edited

Loading

keyonjie commented Apr 19, 2019 •

edited

Loading