Re: ASoC: rockchip_i2s_tdm calibration clocking problem

[Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx>]

Hello Pavel,

On Wed, 31 Jan 2024 09:25:41 +0100
Pavel Hofman <pavel.hofman@xxxxxxxxxxx> wrote:

> Hi,
> 
> I am hitting a clock issue with rockchip_i2s_tdm.c + simple-audio-card 
> (+ RK3308).
> 
> At boot the mclk clk_i2s0_8ch_tx is (somehow) initialized to some 
> (unimportant?) value 50176000 Hz. Note that this frequency is not 
> multiple of either 48kHz or 44.1kHz.
> 
> Method asoc_simple_parse_clk() reads this value and sets it to 
> simple_dai->sysclk.
> 
> Subsequently at asoc_simple_dai_init this "random" initial value is 
> stored to i2s_tdm->mclk_tx_freq:
> 
>     17.839330]  rockchip_i2s_tdm_set_sysclk+0x50/0xbc 
> [snd_soc_rockchip_i2s_tdm]
> [   17.839367]  snd_soc_dai_set_sysclk+0x38/0xb8 [snd_soc_core]
> [   17.839596]  asoc_simple_init_dai+0x94/0xc0 [snd_soc_simple_card_utils]
> [   17.839640]  asoc_simple_dai_init+0x130/0x230 [snd_soc_simple_card_utils]
> [   17.839672]  snd_soc_link_init+0x28/0x90 [snd_soc_core]
> [   17.839843]  snd_soc_bind_card+0x60c/0xbb4 [snd_soc_core]
> 
> 
> When starting playback, called by rockchip_i2s_tdm_hw_params(), 
> rockchip_i2s_tdm_calibrate_mclk() correctly switches parent of 
> mclk_parent to correct root pll clock mclk_root0/1 for the given 
> samplerate and correctly configures mclk_parent frequency.
> 
> https://github.com/torvalds/linux/blob/master/sound/soc/rockchip/rockchip_i2s_tdm.c#L862-L864
> 
> But right after that, the next line of rockchip_i2s_tdm_hw_params() 
> calls rockchip_i2s_tdm_set_mclk()
> 
> https://github.com/torvalds/linux/blob/master/sound/soc/rockchip/rockchip_i2s_tdm.c#L866C9-L866C34
> 
> This method calls clk_set_rate(i2s_tdm->mclk_tx, i2s_tdm->mclk_tx_freq), 
> which resets the clock and its parental chain to the original incorrect 
> value stored in i2s_tdm->mclk_tx_freq from the dai initialization.
> 
> https://github.com/torvalds/linux/blob/master/sound/soc/rockchip/rockchip_i2s_tdm.c#L693
> 
> As a result, no matter what sample rate is being played, the i2s mclk 
> clock always ends up configured incorrectly.

Thanks for the detailed report. I've also run in the same issue while
working on the RK3308 internal audio codec and your analysis matches my
findings.

> DTS I2S sets all clocks, therefore the clk calibration in 
> rockchip_i2s_tdm.c should be (and is) used:
> 
> 	i2s_8ch_0: i2s@ff300000 {
> 		compatible = "rockchip,rk3308-i2s-tdm";
> 		reg = <0x0 0xff300000 0x0 0x1000>;
> 		interrupts = <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>;
> 		clocks = <&cru SCLK_I2S0_8CH_TX>, <&cru SCLK_I2S0_8CH_RX>, <&cru 
> HCLK_I2S0_8CH>,  
> 			 <&cru SCLK_I2S0_8CH_TX_SRC>,
> 			 <&cru SCLK_I2S0_8CH_RX_SRC>,
> 			 <&cru PLL_VPLL0>,
> 			 <&cru PLL_VPLL1>;
> 		clock-names = "mclk_tx", "mclk_rx", "hclk",
> 			      "mclk_tx_src", "mclk_rx_src",
> 			      "mclk_root0", "mclk_root1";
> 		.........
> 
> 
> It seems to me that the calibration code should also rewrite the 
> initially incorrect i2s_tdm->mclk_tx_freq and i2s_tdm->mclk_rx_freq with 
> correct values corresponding to the momentary hw_params rate, or maybe 
> rockchip_i2s_tdm_set_mclk() should not be called if 
> rockchip_i2s_tdm_calibrate_mclk() is called a line above (i.e. putting 
> the call to rockchip_i2s_tdm_set_mclk() into "else" branch).

In my latest v3 series I have implemented a different solution, so
you way want to review and/or test it and give your feedback in that
thread:

https://lore.kernel.org/all/20240221-rk3308-audio-codec-v3-1-dfa34abfcef6@xxxxxxxxxxx/

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com