Skip to content

intel: ssp: support zero mclk_rate in hw_config#7840

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.

Sign up for GitHub

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

Draft
brentlu wants to merge 2 commits into
base:main
Choose a base branch
from
Draft

intel: ssp: support zero mclk_rate in hw_config #7840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e01ea24
intel: ssp: support zero mclk_rate
brentlu Jun 21, 2023
7c2b832
topology: intel-generic-bt: set mclk_rate to zero
brentlu Jun 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 24 additions & 14 deletions src/drivers/intel/ssp/ssp.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -119,6 +119,11 @@ static int ssp_mclk_prepare_enable(struct dai *dai)
struct sof_ipc_dai_config *config = &ssp->config;
int ret;

if (!config->ssp.mclk_rate) {
dai_info(dai, "ssp_mclk_prepare_enable(): SSP%d do not need mclk", dai->index);
return 0;
}

if (ssp->clk_active & SSP_CLK_MCLK_ACTIVE)
return 0;

Expand All@@ -137,6 +142,9 @@ static void ssp_mclk_disable_unprepare(struct dai *dai)
{
struct ssp_pdata *ssp = dai_get_drvdata(dai);

if (!ssp->config.ssp.mclk_rate)
return;

if (!(ssp->clk_active & SSP_CLK_MCLK_ACTIVE))
return;

Expand DownExpand Up@@ -410,21 +418,23 @@ static int ssp_set_config_tplg(struct dai *dai, struct ipc_config_dai *common_co
sscr2 |= (ssp->params.quirks & SOF_DAI_INTEL_SSP_QUIRK_PSPSRWFDFD) ?
SSCR2_PSPSRWFDFD : 0;

if (!config->ssp.mclk_rate ||
config->ssp.mclk_rate > ssp_freq[MAX_SSP_FREQ_INDEX].freq) {
dai_err(dai, "ssp_set_config(): invalid MCLK = %d Hz (valid < %d)",
config->ssp.mclk_rate,
ssp_freq[MAX_SSP_FREQ_INDEX].freq);
ret = -EINVAL;
goto out;
}
/* not every codec reqire mclk */
if (config->ssp.mclk_rate) {

@btian1btian1Jun 26, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

config->ssp.mclk_rate
if it is zero, previous logic will goto out, current does not, is this expected?

@brentlubrentluJun 26, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yes, in previous logic, you need to specify a mclk id and rate even the codec does not need it. Here we use the zero to specify this situation.

if (config->ssp.mclk_rate > ssp_freq[MAX_SSP_FREQ_INDEX].freq) {
dai_err(dai, "ssp_set_config(): invalid MCLK = %d Hz (valid < %d)",
config->ssp.mclk_rate,
ssp_freq[MAX_SSP_FREQ_INDEX].freq);
ret = -EINVAL;
goto out;
}

if (!config->ssp.bclk_rate ||
config->ssp.bclk_rate > config->ssp.mclk_rate) {
dai_err(dai, "ssp_set_config(): BCLK %d Hz = 0 or > MCLK %d Hz",
config->ssp.bclk_rate, config->ssp.mclk_rate);
ret = -EINVAL;
goto out;
if (!config->ssp.bclk_rate ||
config->ssp.bclk_rate > config->ssp.mclk_rate) {
dai_err(dai, "ssp_set_config(): BCLK %d Hz = 0 or > MCLK %d Hz",
config->ssp.bclk_rate, config->ssp.mclk_rate);
ret = -EINVAL;
goto out;
}
}

/* calc frame width based on BCLK and rate - must be divisable */
Expand Down
7 changes: 3 additions & 4 deletions tools/topology/topology1/platform/intel/intel-generic-bt.m4
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -8,7 +8,6 @@ include(`ssp.m4')
ifdef(`SSP_INDEX',`',
`define(SSP_INDEX, `2')')

define(`BT_MCLK', 38400000)
define(`SSP_NAME', concat(concat(`SSP', SSP_INDEX),`-BT'))

# variable that need to be defined in upper m4
Expand DownExpand Up@@ -61,17 +60,17 @@ define(`data_names', DAI_DATA_NAMES(LIST(` ', "ssp_data1", "ssp_data2", "ssp

# Note WB is put as default in the list
define(`ssp_config_list_1', LIST(`',
`MULTI_SSP_CONFIG(hw_config1, HW_CONFIG_ID, DSP_A, SSP_CLOCK(mclk, BT_MCLK, codec_mclk_in),'
`MULTI_SSP_CONFIG(hw_config1, HW_CONFIG_ID, DSP_A, SSP_CLOCK(mclk, 0, codec_mclk_in),'
`SSP_CLOCK(bclk, 256000, codec_master, inverted),'
`SSP_CLOCK(fsync, 16000, codec_master),'
`SSP_TDM(1, 16, 1, 1),'
`SSP_MULTI_CONFIG_DATA(ssp_data1, 16))',
`MULTI_SSP_CONFIG(hw_config2, eval(HW_CONFIG_ID + 1), DSP_A, SSP_CLOCK(mclk, BT_MCLK, codec_mclk_in),'
`MULTI_SSP_CONFIG(hw_config2, eval(HW_CONFIG_ID + 1), DSP_A, SSP_CLOCK(mclk, 0, codec_mclk_in),'
`SSP_CLOCK(bclk, 128000, codec_master, inverted),'
`SSP_CLOCK(fsync, 8000, codec_master),'
`SSP_TDM(1, 16, 1, 1),'
`SSP_MULTI_CONFIG_DATA(ssp_data2, 16))',
`MULTI_SSP_CONFIG(hw_config3, eval(HW_CONFIG_ID + 2), DSP_A, SSP_CLOCK(mclk, BT_MCLK, codec_mclk_in),'
`MULTI_SSP_CONFIG(hw_config3, eval(HW_CONFIG_ID + 2), DSP_A, SSP_CLOCK(mclk, 0, codec_mclk_in),'

@plbossartplbossartJun 21, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Humm, can you clarify what configurations conflict?

We've seen the same type of problems recently with @kv2019i and @CBala21, it's a generic problem that the MCLK should not be tied to specific SSPs.

I am not a big fan of this zero-rate solution.

@brentlubrentluJun 21, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

For this case: #7803

For chrome books, we always choose same mclk rate for both headphone and speaker so there is no problem even most of our speaker amplifiers do not need mclk at all.

I am still struggling running bt offload on my chrome book but I believe it should work.

If we don't want to set the rate zero, we could set the mclk_id to a specific number. But it's weird that an invalid mclk id with a valid rate specified in topology. Any suggestion?

@kv2019ikv2019iJun 21, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@brentlu This is a bit problematic as ADSP is the clock consuming only in HFP BT profile. With A2DP BT offload, ADSP needs to provide BCLK, so MCLK is needed. So I think a better solution is something like this #7741 that ignores MCLK programming if the ADSP is the clock consumer.

@plbossartplbossartJun 21, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The problem is that the MCLK_IN is a valid setting since TGL. In theory the DSP can take the MCLK from the external world and generate BCLK/FSYNC from that external clock.

@brentlubrentluJun 21, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@kv2019i BCLK comes from M/N divider and MCLK comes from MCLK divider. I don't know why we need to setup MCLK just for BCLK running?

@brentlubrentluJun 21, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@plbossart yes MCLK_IN could be one source for M/N divider but I can't find the implementation in m/n driver. Did I miss something?

@kv2019ikv2019iJun 28, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@brentlu I stand corrected. It seems we can indeed omit MCLK output for BT offload (in all profiles). This needs to be double-checked on hardware, but would seem to be the ase. And indeed, this is not needed to generate BCLK.
So looks good for BT.

`SSP_CLOCK(bclk, 1536000, codec_slave),'
`SSP_CLOCK(fsync, 48000, codec_slave),'
`SSP_TDM(2, 16, 3, 0),'
Expand Down