[bkokoszx]

We should clear MNDSS and MCDSS bits before setting
new mclk and bclks source clocks. This commit is
going to fix case when driver want to reconfigure
the same SSP (e.g. index = 0).

1. Driver sends ssp_config() and FW configures ssp
   (index = 0) with MCDSS bits set to e.g. 1.
2. Driver wants to reconfigure the same ssp (index = 0)
   by sending new ssp_config() and FW tries to configure
   ssp 0 once again, but firstly it reads MDIVCTRL
   register:
   mdivc = mn_reg_read(MN_MDIVCTRL, mclk_id)
   and then tries to set MCDSS(0):
   mdivc |= MCDSS(0);
   , but there is "or" function here - after it MCDSS bits
   still will be set to 1. That's why we should clear this
   bits earlier.

Signed-off-by: Bartosz Kokoszko bartoszx.kokoszko@linux.intel.com

[lbetlej]

@bkokoszx don't you think that we may need to log that clock source is overwritten (esp. if you clear non-default value) ? if we have multiple SSP ports configured change of clock source affects all I2S ports.

[bkokoszx]

@lbetlej
In that case, IIUC, we will never "release" (overwrite) different clk used by other SSP, because earlier in mn_set_mclk/bclk() we have following part of code:

if (is_mclk_source_in_use())
		ret = check_current_mclk_source(mclk_rate);
	else
		ret = setup_initial_mclk_source(mclk_id, mclk_rate);

We check in is_mclk_source_in_use() whether we use another source clock, if not we go to setup_initial_mclk_source. We release mclk_sources_used in mn_release_mclk(), but we do not clear register there:

void mn_release_mclk(uint32_t mclk_id)
{
	struct mn *mn = mn_get();

	spin_lock(&mn->lock);
	mn->mclk_sources_used[mclk_id] = false;
	platform_shared_commit(mn, sizeof(*mn));
	spin_unlock(&mn->lock);
}

[lgirdwood]

@bkokoszx as @lbetlej says do we log this change anywhere - we should. We should also log any attempt to change the M/N when in use and return an error.

[bkokoszx]

@lgirdwood @lbetlej
We return an error in check_current_mclk_source() function:

if (ssp_freq[mn->mclk_source_clock].freq % mclk_rate != 0) {
		tr_err(&mn_tr, "MCLK %d, no valid configuration for already selected source = %d",
		       mclk_rate, mn->mclk_source_clock);
		ret = -EINVAL;
	}

The case this PR is going to fix is when we want to reconfigure the same SSP (eg. index = 0).

1. Driver sends ssp_config and FW configures ssp (index 0) with MCDSS bits set to e.g. 1
2. We want to reconfigure ssp (index 0), driver sends ssp_config and FW tries to configure ssp (index 0) once again, but firstly we read MDICVTRL register:

mdivc = mn_reg_read(MN_MDIVCTRL, mclk_id);

and the we try to set MCDSS(0)

mdivc |= MCDSS(0);

, but we have "or" function here - so after that we still will have MCDSS bits set to 1.

[lgirdwood]

@bkokoszx ok, can you detail the "why" above in the commit message as it's now clear what you are doing and why.

[bkokoszx]

@lgirdwood
Updated

[lgirdwood]

Travis looks like a timeout and Jenkins is usual issues

[lgirdwood]

SOFCI TEST

[lgirdwood]

@bkokoszx any idea why we are seeing the issue on CNL qemu boot ? is M/N config in the boot path ?

[bkokoszx]

SOFCI TEST

[bkokoszx]

@lgirdwood
Basically, I have no idea why this is failing...

[lgirdwood]

@bkokoszx it's failing on some others too now. @marc-hb has a PR that may shed light on this.

[marc-hb]

The same test failed in unrelated https://travis-ci.org/github/thesofproject/sof/jobs/702557005 / #3064 too and with even less error messages.

I restored in #3101 some error messages I accidentally dropped earlier, so yes rebasing might help. BTW I tried this test a couple days ago and it passed locally.

The timeout message is expected every time, see thesofproject/qemu#44