Re: [PATCH] sound: soc: fsl: Do not set DAI sysclk when it is equal to system freq

[Åukasz Majewski]

On 09/05/2017 07:20 AM, Nicolin Chen wrote:
> On Sun, Sep 03, 2017 at 04:40:21PM +0200, Åukasz Majewski wrote:
> > > /*
> > > * Hardware limitation: The bclk rate must be
> > > * never greater than 1/5 IPG clock rate
> > > */
> > > if (freq * 5 > clk_get_rate(ssi_private->clk)) {
> > > dev_err(cpu_dai->dev, "bitclk > ipgclk/5\n");
> > > return -EINVAL;
> > > }
> >
> > Unfortunately not.
> >
> > This is the part of fsl_ssi_set_bclk() function which is called after
> > fsl_ssi_set_dai_sysclk() (which sets ssi_private->bitclk_freq = freq;).
> >
> > Before the aforementioned check we do have:
> >
> > 	if (ssi_private->bitclk_freq)
> > 		freq = ssi_private->bitclk_freq;
> > 	else
> > 		freq = params_channels(hw_params) * 32 * 	params_rate(hw_params);
> >
> >
> > Which assigns freq = bitclk_freq (66 MHz)
> [...]
> > And then we break on this particular check:
> > 66MHz * 5 > 66 MHz.
> [...]
>
> Does the check fail and return an error here, or not?

Yes, this check fails and return error (-EINVAL).

> > The culprit IMHO is the  ssi_private->bitclk_freq = freq; in the
> > fsl_ssi_set_dai_sysclk(), since we _should_ set SSI's IP block clock
> > (ssi_private->clk), not the bit clock (BCLK).
>
> No. We should not set the IP block clock. That's from IPG bus
> on certain IMX SoCs. Setting it might change IPG bus clocks
> which might break the system.
>
> And apparently, we shouldn't set bitclk to 66MHz either. Can
> you help to find where this 66MHz comes from?

OK.

The fsi_ssi.c file defines:

ops->set_sysclk() routine:

	.set_sysclk	= fsl_ssi_set_dai_sysclk,

This routine is called in:
int snd_soc_dai_set_sysclk() @ soc-core.c

which is called in two places for my setup:

1. asoc_simple_card_hw_params() @ simple-card.c

and

2. int asoc_simple_card_init_dai() @ simple-card-utils.c

In this function (point 2.) the

simple_dai->sysclk is set and:

snd_soc_dai_set_sysclk(dai, 0, simple_dai->sysclk, 0)

which sets frequency to 66 MHz [*].



The asoc_simple_card_init_dai() is called in

asoc_simple_card_dai_init() @ simple-card.c

which is assigned to dai_link->init

dai_link->init		= asoc_simple_card_dai_init; @ simple_card.c



And the sysclk itself is defined at:
-------------------------------------

dai_props->codec_dai->sysclk, which is used at:

asoc_simple_card_startup(), asoc_simple_card_shutdown() and others 
functions at simple-card.c


It is setup at:

asoc_simple_card_parse_clk() @ simple-card-utils.c from macro:
#define asoc_simple_card_parse_clk_cpu()


And the problem is:
-------------------

At the
asoc_simple_card_parse_clk()
we finally go to dts node:

/soc/aips-bus@02100000/i2c@021a0000/tfa9879@6C

which has clock from I2C (66 MHz).

(So the [*] hack may help here, since it is checked before checking the 
i2c node).


Note:
[*] - I could workaround this problem by setting:

system-clock-frequency = <0> in

			dailink_master: cpu {
			    sound-dai = <&ssi2>;
			};

but this is IMHO even worse hack.... than this patch.

> > This patch just quits early if it detects change, which don't need to be
> > done.
>
> I kinda see your point is to error out before hw_params(). So
> I feel it would be better to have a similar 5-times-check in
> the set_sysclk() too, if it's really necessary.
>
> Btw, I don't see simple card calling set_sysclk() function in
> any earlier stage than hw_params(). I am wondering how did you
> encounter the problem?
>
> Thanks
> Nicolin


--
Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@xxxxxxx