Skip to content

Conversation

@brentlu
Copy link
Contributor

@brentlu brentlu commented May 26, 2021

This PR is for ODM/codec vendor validation and review

The series of patches are originated from #4219 which is working on master branch. Since GLK branch is ancient, the implementation of bclk clock control is much simpler here but the idea is the same.

@brentlu brentlu marked this pull request as draft May 26, 2021 10:09
@brentlu brentlu requested a review from bardliao May 26, 2021 10:11
@lgirdwood
Copy link
Member

@slawblauciak @mwasko fyi.

@plbossart
Copy link
Member

let's wait that the code is agreed on with the main branch, this should be a backport and aligned with main.

I would also like the topology changes to be applied on the main branch so that we can test all this with CI. @bardliao can you take the topology patches and apply them to your PR #4219 ?

@brentlu
Copy link
Contributor Author

brentlu commented May 26, 2021

@plbossart yes, we will wait main branch finalized and see what's need to be ported to here. I've validated this PR on customer device and this is the result from oscilliscope:

  1. both bclk and fsync start toggling in hw_param with correct frequency
  2. both bclk and fsync disabled in hw_free.

As a side effect, this PR also fixes da7219 which PLL locks on fsync...

@plbossart
Copy link
Member

As a side effect, this PR also fixes da7219 which PLL locks on fsync...

That's why I'd like this change to become the default for all I2S codecs ;-)

@bardliao
Copy link
Collaborator

let's wait that the code is agreed on with the main branch, this should be a backport and aligned with main.

I would also like the topology changes to be applied on the main branch so that we can test all this with CI. @bardliao can you take the topology patches and apply them to your PR #4219 ?

Yes, I took the topology patches with slight modification to PR #4219

@brentlu brentlu force-pushed the glk-ssp-clock branch 2 times, most recently from 557dabc to 4328afc Compare June 6, 2021 16:31
@brentlu
Copy link
Contributor Author

brentlu commented Jun 7, 2021

update to sync with #4288

@brentlu
Copy link
Contributor Author

brentlu commented Jun 7, 2021

measure the signal of this backport:

bclk starts long before fsync in prepare(), then fsync starts with data with one zero sample ahead

tek00000

fsync stops toggling first, then bclk stops 5.84us later

tek00001

@plbossart
Copy link
Member

plbossart commented Jun 7, 2021

Thanks @brentlu just to be clear.
Are you saying the results are now OK and all problems with CS42L42 are solved with this PR?


spin_lock(&dai->lock);

trace_ssp("ssp_hw_free()");
Copy link
Collaborator

Choose a reason for hiding this comment

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

this doesn't have to be under the lock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think we need the lock to protect the ssp->state

Copy link
Collaborator

Choose a reason for hiding this comment

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

@brentlu I meant the trace_ssp("ssp_hw_free()"); line

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's done. thanks for review

@brentlu
Copy link
Contributor Author

brentlu commented Jun 9, 2021

@plbossart both ODM and codec vendor could not find problem in this backport. Codec vendor still need to check the performance since bclk frequency is changed but basic function is ok.

@plbossart
Copy link
Member

@plbossart both ODM and codec vendor could not find problem in this backport. Codec vendor still need to check the performance since bclk frequency is changed but basic function is ok.

Good to know, thanks @brentlu
I'll try to finish up the main branch this week, too many things to do at the moment.

plbossart and others added 3 commits June 21, 2021 09:41
The DAI_CONFIG IPC is currently used both for hw_params and
hw_free. For some DAIs, there are hacky ways with e.g. invalid DMA
channels to indicate a hw_free. Rather than adding a new IPC for
hw_params and hw_free, let's add a flag that indicates if the
DAI_CONFIG is really applied during a hw_params or hw_free stage.

This is tagged as a ABI 3.19 change.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

(cherry picked from commit 0792297)
Signed-off-by: Brent Lu <brent.lu@intel.com>
Add two clks_control bits. MCLK and/or BCLK will start during hw_params
and stop during hw_free if the corresponding bit is set.

This is tagged as a ABI 3.19 change.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

(cherry picked from commit f46c144)
Signed-off-by: Brent Lu <brent.lu@intel.com>
This will help identify support for SSP BCLK/MCLK changes.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

(cherry picked from commit 2c06163)
Signed-off-by: Brent Lu <brent.lu@intel.com>
bardliao and others added 2 commits June 24, 2021 22:01
Some codecs need the SSP bit clock to start before data is provided,
and conversely the bit clock to remain active until the hw_free stage.

For backwards-compatibility with older kernels, the
SOF_DAI_INTEL_SSP_CLKCTRL_MCLK_ES and
SOF_DAI_INTEL_SSP_CLKCTRL_BCLK_ES bitfields are used to set an
internal state in the ssp->clk_active field. This helps deals with the
case where a topology sets these bits but the older kernel does not
make use of the modified IPC.

While we are at it, add clearer info traces for SSP configurations.

Note that the FSYNC only starts when DMA transfers are enabled in the
.trigger stage. This is by-design, the FSYNC will only start if the
FIFO is not empty. During the prepare stages the DMA transfers are not
enabled so the FIFOs are empty.

To enable the FSYNC at an earlier stage, we would need a major surgery
in the SOF architecture, or we would need to start zero-based DMA
transfers.

Co-developed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

(cherry picked from commit a0c1758)
Signed-off-by: Brent Lu <brent.lu@intel.com>
The current code does not follow recommended programming sequences:
TSRE and RSRE should be set before SSE and conversely cleared before
SSE.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

(cherry picked from commit 99193ab)
Signed-off-by: Brent Lu <brent.lu@intel.com>
brentlu added 5 commits June 24, 2021 22:02
Define the SSP_CC_MCLK/BCLK_ES bit to be used in SSP_CONFIG_DATA macro
to enable mclk/bclk on hw_params and disable malk/bclk on hw_free.

Signed-off-by: Brent Lu <brent.lu@intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

(cherry picked from commit 61e4cd7)
Signed-off-by: Brent Lu <brent.lu@intel.com>
Add support for cs42l42 running on GLK boards. There is a 10 ms BCLK
delay in the master branch version to avoid the noise on TX path.
However, this feature is not supported on GLK production branch yet so
we remove it for now.

Signed-off-by: Brent Lu <brent.lu@intel.com>

(cherry picked from commit 45ce11022b40fe0fe8d998ee3f93a74b53fb7fd0)
Signed-off-by: Brent Lu <brent.lu@intel.com>
Enable the bclk clock control for SSP2.

Note that this impacts existing GLK-based chromebooks as well as newer
hardware.

Signed-off-by: Brent Lu <brent.lu@intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>

(cherry picked from commit 7199c68)
Signed-off-by: Brent Lu <brent.lu@intel.com>
By changing bclk to 2.4MHz, we can use XTAL as clock source and reduce
power consumption.

Signed-off-by: Brent Lu <brent.lu@intel.com>

(cherry picked from commit 5abd8a9)
Signed-off-by: Brent Lu <brent.lu@intel.com>
Changing SSP2 configuration to use 24-bit sample bitwidth.

Signed-off-by: Brent Lu <brent.lu@intel.com>

(cherry picked from commit 44cc184dee0da1bcb9e1edd54d301b838b54c856)
Signed-off-by: Brent Lu <brent.lu@intel.com>
@brentlu brentlu added GLK Applies to Gemini Lake platform chrome Chromebooks or ChromeOS labels Jun 24, 2021
@brentlu
Copy link
Contributor Author

brentlu commented Jun 24, 2021

based on the #4288 with topology changes to use 2.4MHz bclk and 24-bit sample width

@brentlu brentlu changed the title bclk clock control for GLK branch Backport bclk clock control to GLK branch Jun 24, 2021
@brentlu brentlu marked this pull request as ready for review June 24, 2021 16:14
@lgirdwood
Copy link
Member

@mwasko @slawblauciak this one is for you.

@mwasko
Copy link
Contributor

mwasko commented Jun 30, 2021

@brentlu I would recommend to create a separate branch for this change to avoid mess on release code that took place quite a long time ago. If you want to apply this fix on top of glk-012-stable-branch then I would like to propose to create glk-012-hot-fix-1 branch for that.

@brentlu
Copy link
Contributor Author

brentlu commented Jul 5, 2021

Hi @mwasko,
Sure, no problem. I found that I could create a branch myself. Thanks for advise.

@brentlu brentlu changed the base branch from glk-012-stable-branch to glk-012-hot-fix-1 July 7, 2021 02:05
@brentlu brentlu merged commit 2a13eac into thesofproject:glk-012-hot-fix-1 Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chrome Chromebooks or ChromeOS GLK Applies to Gemini Lake platform

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants