Message ID | 20240305-rk3308-audio-codec-v4-0-312acdbe628f@bootlin.com |
---|---|
Headers | show |
Series | Add support for the internal RK3308 audio codec | expand |
On 05. 03. 24 15:36, Luca Ceresoli wrote: > The sample rates set by the rockchip_i2s_tdm driver in master mode are > inaccurate up to 5% in several cases, due to the driver logic to configure > clocks and a nasty interaction with the Common Clock Framework. > > To understand what happens, here is the relevant section of the clock tree > (slightly simplified), along with the names used in the driver: > > vpll0 _OR_ vpll1 "mclk_root" > clk_i2s2_8ch_tx_src "mclk_parent" > clk_i2s2_8ch_tx_mux > clk_i2s2_8ch_tx "mclk" or "mclk_tx" > > This is what happens when playing back e.g. at 192 kHz using > audio-graph-card (when recording the same applies, only s/tx/rx/): > > 0. at probe, rockchip_i2s_tdm_set_sysclk() stores the passed frequency in > i2s_tdm->mclk_tx_freq (*) which is 50176000, and that is never modified > afterwards > > 1. when playback is started, rockchip_i2s_tdm_hw_params() is called and > does the following two calls > > 2. rockchip_i2s_tdm_calibrate_mclk(): > > 2a. selects mclk_root0 (vpll0) as a parent for mclk_parent > (mclk_tx_src), which is OK because the vpll0 rate is a good for > 192000 (and sumbultiple) rates > > 2b. sets the mclk_root frequency based on ppm calibration computations > > 2c. sets mclk_tx_src to 49152000 (= 256 * 192000), which is also OK as > it is a multiple of the required bit clock > > 3. rockchip_i2s_tdm_set_mclk() > > 3a. calls clk_set_rate() to set the rate of mclk_tx (clk_i2s2_8ch_tx) > to the value of i2s_tdm->mclk_tx_freq (*), i.e. 50176000 which is > not a multiple of the sampling frequency -- this is not OK > > 3a1. clk_set_rate() reacts by reparenting clk_i2s2_8ch_tx_src to > vpll1 -- this is not OK because the default vpll1 rate can be > divided to get 44.1 kHz and related rates, not 192 kHz > > The result is that the driver does a lot of ad-hoc decisions about clocks > and ends up in using the wrong parent at an unoptimal rate. > > Step 0 is one part of the problem: unless the card driver calls set_sysclk > at each stream start, whatever rate is set in mclk_tx_freq during boot will > be taken and used until reboot. Moreover the driver does not care if its > value is not a multiple of any audio frequency. > > Another part of the problem is that the whole reparenting and clock rate > setting logic is conflicting with the CCF algorithms to achieve largely the > same goal: selecting the best parent and setting the closest clock > rate. And it turns out that only calling once clk_set_rate() on > clk_i2s2_8ch_tx picks the correct vpll and sets the correct rate. > > The fix is based on removing the custom logic in the driver to select the > parent and set the various clocks, and just let the Clock Framework do it > all. As a side effect, the set_sysclk() op becomes useless because we now > let the CCF compute the appropriate value for the sampling rate. It also > implies that the whole calibration logic is now dead code and so it is > removed along with the "PCM Clock Compensation in PPM" kcontrol, which has > always been broken anyway. The handling of the 4 optional clocks also > becomes dead code and is removed. > > The actual rates have been tested playing 30 seconds of audio at various > sampling rates before and after this change using sox: > > time play -r <sample_rate> -n synth 30 sine 950 gain -3 > > The time reported in the table below is the 'real' value reported by the > 'time' command in the above command line. > > rate before after > --------- ------ ------ > 8000 Hz 30.60s 30.63s > 11025 Hz 30.45s 30.51s > 16000 Hz 30.47s 30.50s > 22050 Hz 30.78s 30.41s > 32000 Hz 31.02s 30.43s > 44100 Hz 30.78s 30.41s > 48000 Hz 29.81s 30.45s > 88200 Hz 30.78s 30.41s > 96000 Hz 29.79s 30.42s > 176400 Hz 27.40s 30.41s > 192000 Hz 29.79s 30.42s > > While the tests are running the clock tree confirms that: > > * without the patch, vpll1 is always used and clk_i2s2_8ch_tx always > produces 50176000 Hz, which cannot be divided for most audio rates > except the slowest ones, generating inaccurate rates > * with the patch: > - for 192000 Hz vpll0 is used > - for 176400 Hz vpll1 is used > - clk_i2s2_8ch_tx always produces (256 * <rate>) Hz > > Tested on the RK3308 using the internal audio codec. > > Fixes: 081068fd6414 ("ASoC: rockchip: add support for i2s-tdm controller") > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > I tested this patch, it works OK on 8ch I2S0 of RK3308 Radxa Pi S, frequencies checked in clock summary and clock pins with an oscilloscope. It's basically identical issue with the same cause as I reported in https://lore.kernel.org/alsa-devel/20240304134329.392c75bf@booty/T/#m19d69461aa827f15a86d6d31ed1b1520e80a909e Just a note - the patch changes mclk frequencies from fixed 256 x 192000/176400 to variable 256 x fs. While 256 x fs is more standard than the fixed mclk for all samplerates of the same family, it may cause changes for existing users if using the mclk output pins. But the existing code was broken and had to be hacked to work anyway. Perhaps this may need some considering. Also I wonder if the DTS description rockchip,i2s-tdm.yaml file may be updated too, as some clocks listed by it are removed by the patch: -const: mclk_tx_src -const: mclk_rx_src -const: mclk_root0 -const: mclk_root1 IMO it may be a bit confusing if the description lists parameters which are not actually used by the driver anymore. With regards, Pavel. > --- > > As discussed after v3, the clock manipulation logic that this patch removes > is a remnant of the downstream 4.19 driver > (https://github.com/rockchip-linux/kernel/blob/develop-4.19/sound/soc/rockchip/rockchip_i2s_tdm.c). > It might be that the CCF was not yet capable of optimally reparenting back > in 4.19, so it did make sense back then. > > Changed in v4: > - Mention the tested hardware in the commit message > - No content changes > > Changed in v3: > - this patch is new in v3, and it replaces and supersedes the patch "ASoC: > rockchip: i2s-tdm: Fix clk_id usage in .set_sysclk()" > --- > sound/soc/rockchip/rockchip_i2s_tdm.c | 352 +--------------------------------- > 1 file changed, 6 insertions(+), 346 deletions(-) > > diff --git a/sound/soc/rockchip/rockchip_i2s_tdm.c b/sound/soc/rockchip/rockchip_i2s_tdm.c > index 860e66ec85e8..9fa020ef7eab 100644 > --- a/sound/soc/rockchip/rockchip_i2s_tdm.c > +++ b/sound/soc/rockchip/rockchip_i2s_tdm.c > @@ -25,8 +25,6 @@ > #define DEFAULT_MCLK_FS 256 > #define CH_GRP_MAX 4 /* The max channel 8 / 2 */ > #define MULTIPLEX_CH_MAX 10 > -#define CLK_PPM_MIN -1000 > -#define CLK_PPM_MAX 1000 > > #define TRCM_TXRX 0 > #define TRCM_TX 1 > @@ -53,20 +51,6 @@ struct rk_i2s_tdm_dev { > struct clk *hclk; > struct clk *mclk_tx; > struct clk *mclk_rx; > - /* The mclk_tx_src is parent of mclk_tx */ > - struct clk *mclk_tx_src; > - /* The mclk_rx_src is parent of mclk_rx */ > - struct clk *mclk_rx_src; > - /* > - * The mclk_root0 and mclk_root1 are root parent and supplies for > - * the different FS. > - * > - * e.g: > - * mclk_root0 is VPLL0, used for FS=48000Hz > - * mclk_root1 is VPLL1, used for FS=44100Hz > - */ > - struct clk *mclk_root0; > - struct clk *mclk_root1; > struct regmap *regmap; > struct regmap *grf; > struct snd_dmaengine_dai_dma_data capture_dma_data; > @@ -76,19 +60,11 @@ struct rk_i2s_tdm_dev { > const struct rk_i2s_soc_data *soc_data; > bool is_master_mode; > bool io_multiplex; > - bool mclk_calibrate; > bool tdm_mode; > - unsigned int mclk_rx_freq; > - unsigned int mclk_tx_freq; > - unsigned int mclk_root0_freq; > - unsigned int mclk_root1_freq; > - unsigned int mclk_root0_initial_freq; > - unsigned int mclk_root1_initial_freq; > unsigned int frame_width; > unsigned int clk_trcm; > unsigned int i2s_sdis[CH_GRP_MAX]; > unsigned int i2s_sdos[CH_GRP_MAX]; > - int clk_ppm; > int refcount; > spinlock_t lock; /* xfer lock */ > bool has_playback; > @@ -114,12 +90,6 @@ static void i2s_tdm_disable_unprepare_mclk(struct rk_i2s_tdm_dev *i2s_tdm) > { > clk_disable_unprepare(i2s_tdm->mclk_tx); > clk_disable_unprepare(i2s_tdm->mclk_rx); > - if (i2s_tdm->mclk_calibrate) { > - clk_disable_unprepare(i2s_tdm->mclk_tx_src); > - clk_disable_unprepare(i2s_tdm->mclk_rx_src); > - clk_disable_unprepare(i2s_tdm->mclk_root0); > - clk_disable_unprepare(i2s_tdm->mclk_root1); > - } > } > > /** > @@ -142,29 +112,9 @@ static int i2s_tdm_prepare_enable_mclk(struct rk_i2s_tdm_dev *i2s_tdm) > ret = clk_prepare_enable(i2s_tdm->mclk_rx); > if (ret) > goto err_mclk_rx; > - if (i2s_tdm->mclk_calibrate) { > - ret = clk_prepare_enable(i2s_tdm->mclk_tx_src); > - if (ret) > - goto err_mclk_rx; > - ret = clk_prepare_enable(i2s_tdm->mclk_rx_src); > - if (ret) > - goto err_mclk_rx_src; > - ret = clk_prepare_enable(i2s_tdm->mclk_root0); > - if (ret) > - goto err_mclk_root0; > - ret = clk_prepare_enable(i2s_tdm->mclk_root1); > - if (ret) > - goto err_mclk_root1; > - } > > return 0; > > -err_mclk_root1: > - clk_disable_unprepare(i2s_tdm->mclk_root0); > -err_mclk_root0: > - clk_disable_unprepare(i2s_tdm->mclk_rx_src); > -err_mclk_rx_src: > - clk_disable_unprepare(i2s_tdm->mclk_tx_src); > err_mclk_rx: > clk_disable_unprepare(i2s_tdm->mclk_tx); > err_mclk_tx: > @@ -564,159 +514,6 @@ static void rockchip_i2s_tdm_xfer_resume(struct snd_pcm_substream *substream, > I2S_XFER_RXS_START); > } > > -static int rockchip_i2s_tdm_clk_set_rate(struct rk_i2s_tdm_dev *i2s_tdm, > - struct clk *clk, unsigned long rate, > - int ppm) > -{ > - unsigned long rate_target; > - int delta, ret; > - > - if (ppm == i2s_tdm->clk_ppm) > - return 0; > - > - if (ppm < 0) > - delta = -1; > - else > - delta = 1; > - > - delta *= (int)div64_u64((u64)rate * (u64)abs(ppm) + 500000, > - 1000000); > - > - rate_target = rate + delta; > - > - if (!rate_target) > - return -EINVAL; > - > - ret = clk_set_rate(clk, rate_target); > - if (ret) > - return ret; > - > - i2s_tdm->clk_ppm = ppm; > - > - return 0; > -} > - > -static int rockchip_i2s_tdm_calibrate_mclk(struct rk_i2s_tdm_dev *i2s_tdm, > - struct snd_pcm_substream *substream, > - unsigned int lrck_freq) > -{ > - struct clk *mclk_root; > - struct clk *mclk_parent; > - unsigned int mclk_root_freq; > - unsigned int mclk_root_initial_freq; > - unsigned int mclk_parent_freq; > - unsigned int div, delta; > - u64 ppm; > - int ret; > - > - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) > - mclk_parent = i2s_tdm->mclk_tx_src; > - else > - mclk_parent = i2s_tdm->mclk_rx_src; > - > - switch (lrck_freq) { > - case 8000: > - case 16000: > - case 24000: > - case 32000: > - case 48000: > - case 64000: > - case 96000: > - case 192000: > - mclk_root = i2s_tdm->mclk_root0; > - mclk_root_freq = i2s_tdm->mclk_root0_freq; > - mclk_root_initial_freq = i2s_tdm->mclk_root0_initial_freq; > - mclk_parent_freq = DEFAULT_MCLK_FS * 192000; > - break; > - case 11025: > - case 22050: > - case 44100: > - case 88200: > - case 176400: > - mclk_root = i2s_tdm->mclk_root1; > - mclk_root_freq = i2s_tdm->mclk_root1_freq; > - mclk_root_initial_freq = i2s_tdm->mclk_root1_initial_freq; > - mclk_parent_freq = DEFAULT_MCLK_FS * 176400; > - break; > - default: > - dev_err(i2s_tdm->dev, "Invalid LRCK frequency: %u Hz\n", > - lrck_freq); > - return -EINVAL; > - } > - > - ret = clk_set_parent(mclk_parent, mclk_root); > - if (ret) > - return ret; > - > - ret = rockchip_i2s_tdm_clk_set_rate(i2s_tdm, mclk_root, > - mclk_root_freq, 0); > - if (ret) > - return ret; > - > - delta = abs(mclk_root_freq % mclk_parent_freq - mclk_parent_freq); > - ppm = div64_u64((uint64_t)delta * 1000000, (uint64_t)mclk_root_freq); > - > - if (ppm) { > - div = DIV_ROUND_CLOSEST(mclk_root_initial_freq, mclk_parent_freq); > - if (!div) > - return -EINVAL; > - > - mclk_root_freq = mclk_parent_freq * round_up(div, 2); > - > - ret = clk_set_rate(mclk_root, mclk_root_freq); > - if (ret) > - return ret; > - > - i2s_tdm->mclk_root0_freq = clk_get_rate(i2s_tdm->mclk_root0); > - i2s_tdm->mclk_root1_freq = clk_get_rate(i2s_tdm->mclk_root1); > - } > - > - return clk_set_rate(mclk_parent, mclk_parent_freq); > -} > - > -static int rockchip_i2s_tdm_set_mclk(struct rk_i2s_tdm_dev *i2s_tdm, > - struct snd_pcm_substream *substream, > - struct clk **mclk) > -{ > - unsigned int mclk_freq; > - int ret; > - > - if (i2s_tdm->clk_trcm) { > - if (i2s_tdm->mclk_tx_freq != i2s_tdm->mclk_rx_freq) { > - dev_err(i2s_tdm->dev, > - "clk_trcm, tx: %d and rx: %d should be the same\n", > - i2s_tdm->mclk_tx_freq, > - i2s_tdm->mclk_rx_freq); > - return -EINVAL; > - } > - > - ret = clk_set_rate(i2s_tdm->mclk_tx, i2s_tdm->mclk_tx_freq); > - if (ret) > - return ret; > - > - ret = clk_set_rate(i2s_tdm->mclk_rx, i2s_tdm->mclk_rx_freq); > - if (ret) > - return ret; > - > - /* mclk_rx is also ok. */ > - *mclk = i2s_tdm->mclk_tx; > - } else { > - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { > - *mclk = i2s_tdm->mclk_tx; > - mclk_freq = i2s_tdm->mclk_tx_freq; > - } else { > - *mclk = i2s_tdm->mclk_rx; > - mclk_freq = i2s_tdm->mclk_rx_freq; > - } > - > - ret = clk_set_rate(*mclk, mclk_freq); > - if (ret) > - return ret; > - } > - > - return 0; > -} > - > static int rockchip_i2s_ch_to_io(unsigned int ch, bool substream_capture) > { > if (substream_capture) { > @@ -853,19 +650,17 @@ static int rockchip_i2s_tdm_hw_params(struct snd_pcm_substream *substream, > struct snd_soc_dai *dai) > { > struct rk_i2s_tdm_dev *i2s_tdm = to_info(dai); > - struct clk *mclk; > - int ret = 0; > unsigned int val = 0; > unsigned int mclk_rate, bclk_rate, div_bclk = 4, div_lrck = 64; > + int err; > > if (i2s_tdm->is_master_mode) { > - if (i2s_tdm->mclk_calibrate) > - rockchip_i2s_tdm_calibrate_mclk(i2s_tdm, substream, > - params_rate(params)); > + struct clk *mclk = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) ? > + i2s_tdm->mclk_tx : i2s_tdm->mclk_rx; > > - ret = rockchip_i2s_tdm_set_mclk(i2s_tdm, substream, &mclk); > - if (ret) > - return ret; > + err = clk_set_rate(mclk, DEFAULT_MCLK_FS * params_rate(params)); > + if (err) > + return err; > > mclk_rate = clk_get_rate(mclk); > bclk_rate = i2s_tdm->frame_width * params_rate(params); > @@ -973,96 +768,6 @@ static int rockchip_i2s_tdm_trigger(struct snd_pcm_substream *substream, > return 0; > } > > -static int rockchip_i2s_tdm_set_sysclk(struct snd_soc_dai *cpu_dai, int stream, > - unsigned int freq, int dir) > -{ > - struct rk_i2s_tdm_dev *i2s_tdm = to_info(cpu_dai); > - > - /* Put set mclk rate into rockchip_i2s_tdm_set_mclk() */ > - if (i2s_tdm->clk_trcm) { > - i2s_tdm->mclk_tx_freq = freq; > - i2s_tdm->mclk_rx_freq = freq; > - } else { > - if (stream == SNDRV_PCM_STREAM_PLAYBACK) > - i2s_tdm->mclk_tx_freq = freq; > - else > - i2s_tdm->mclk_rx_freq = freq; > - } > - > - dev_dbg(i2s_tdm->dev, "The target mclk_%s freq is: %d\n", > - stream ? "rx" : "tx", freq); > - > - return 0; > -} > - > -static int rockchip_i2s_tdm_clk_compensation_info(struct snd_kcontrol *kcontrol, > - struct snd_ctl_elem_info *uinfo) > -{ > - uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER; > - uinfo->count = 1; > - uinfo->value.integer.min = CLK_PPM_MIN; > - uinfo->value.integer.max = CLK_PPM_MAX; > - uinfo->value.integer.step = 1; > - > - return 0; > -} > - > -static int rockchip_i2s_tdm_clk_compensation_get(struct snd_kcontrol *kcontrol, > - struct snd_ctl_elem_value *ucontrol) > -{ > - struct snd_soc_dai *dai = snd_kcontrol_chip(kcontrol); > - struct rk_i2s_tdm_dev *i2s_tdm = snd_soc_dai_get_drvdata(dai); > - > - ucontrol->value.integer.value[0] = i2s_tdm->clk_ppm; > - > - return 0; > -} > - > -static int rockchip_i2s_tdm_clk_compensation_put(struct snd_kcontrol *kcontrol, > - struct snd_ctl_elem_value *ucontrol) > -{ > - struct snd_soc_dai *dai = snd_kcontrol_chip(kcontrol); > - struct rk_i2s_tdm_dev *i2s_tdm = snd_soc_dai_get_drvdata(dai); > - int ret = 0, ppm = 0; > - int changed = 0; > - unsigned long old_rate; > - > - if (ucontrol->value.integer.value[0] < CLK_PPM_MIN || > - ucontrol->value.integer.value[0] > CLK_PPM_MAX) > - return -EINVAL; > - > - ppm = ucontrol->value.integer.value[0]; > - > - old_rate = clk_get_rate(i2s_tdm->mclk_root0); > - ret = rockchip_i2s_tdm_clk_set_rate(i2s_tdm, i2s_tdm->mclk_root0, > - i2s_tdm->mclk_root0_freq, ppm); > - if (ret) > - return ret; > - if (old_rate != clk_get_rate(i2s_tdm->mclk_root0)) > - changed = 1; > - > - if (clk_is_match(i2s_tdm->mclk_root0, i2s_tdm->mclk_root1)) > - return changed; > - > - old_rate = clk_get_rate(i2s_tdm->mclk_root1); > - ret = rockchip_i2s_tdm_clk_set_rate(i2s_tdm, i2s_tdm->mclk_root1, > - i2s_tdm->mclk_root1_freq, ppm); > - if (ret) > - return ret; > - if (old_rate != clk_get_rate(i2s_tdm->mclk_root1)) > - changed = 1; > - > - return changed; > -} > - > -static struct snd_kcontrol_new rockchip_i2s_tdm_compensation_control = { > - .iface = SNDRV_CTL_ELEM_IFACE_PCM, > - .name = "PCM Clock Compensation in PPM", > - .info = rockchip_i2s_tdm_clk_compensation_info, > - .get = rockchip_i2s_tdm_clk_compensation_get, > - .put = rockchip_i2s_tdm_clk_compensation_put, > -}; > - > static int rockchip_i2s_tdm_dai_probe(struct snd_soc_dai *dai) > { > struct rk_i2s_tdm_dev *i2s_tdm = snd_soc_dai_get_drvdata(dai); > @@ -1072,9 +777,6 @@ static int rockchip_i2s_tdm_dai_probe(struct snd_soc_dai *dai) > if (i2s_tdm->has_playback) > snd_soc_dai_dma_data_set_playback(dai, &i2s_tdm->playback_dma_data); > > - if (i2s_tdm->mclk_calibrate) > - snd_soc_add_dai_controls(dai, &rockchip_i2s_tdm_compensation_control, 1); > - > return 0; > } > > @@ -1115,7 +817,6 @@ static const struct snd_soc_dai_ops rockchip_i2s_tdm_dai_ops = { > .probe = rockchip_i2s_tdm_dai_probe, > .hw_params = rockchip_i2s_tdm_hw_params, > .set_bclk_ratio = rockchip_i2s_tdm_set_bclk_ratio, > - .set_sysclk = rockchip_i2s_tdm_set_sysclk, > .set_fmt = rockchip_i2s_tdm_set_fmt, > .set_tdm_slot = rockchip_dai_tdm_slot, > .trigger = rockchip_i2s_tdm_trigger, > @@ -1444,35 +1145,6 @@ static void rockchip_i2s_tdm_path_config(struct rk_i2s_tdm_dev *i2s_tdm, > rockchip_i2s_tdm_tx_path_config(i2s_tdm, num); > } > > -static int rockchip_i2s_tdm_get_calibrate_mclks(struct rk_i2s_tdm_dev *i2s_tdm) > -{ > - int num_mclks = 0; > - > - i2s_tdm->mclk_tx_src = devm_clk_get(i2s_tdm->dev, "mclk_tx_src"); > - if (!IS_ERR(i2s_tdm->mclk_tx_src)) > - num_mclks++; > - > - i2s_tdm->mclk_rx_src = devm_clk_get(i2s_tdm->dev, "mclk_rx_src"); > - if (!IS_ERR(i2s_tdm->mclk_rx_src)) > - num_mclks++; > - > - i2s_tdm->mclk_root0 = devm_clk_get(i2s_tdm->dev, "mclk_root0"); > - if (!IS_ERR(i2s_tdm->mclk_root0)) > - num_mclks++; > - > - i2s_tdm->mclk_root1 = devm_clk_get(i2s_tdm->dev, "mclk_root1"); > - if (!IS_ERR(i2s_tdm->mclk_root1)) > - num_mclks++; > - > - if (num_mclks < 4 && num_mclks != 0) > - return -ENOENT; > - > - if (num_mclks == 4) > - i2s_tdm->mclk_calibrate = 1; > - > - return 0; > -} > - > static int rockchip_i2s_tdm_path_prepare(struct rk_i2s_tdm_dev *i2s_tdm, > struct device_node *np, > bool is_rx_path) > @@ -1610,11 +1282,6 @@ static int rockchip_i2s_tdm_probe(struct platform_device *pdev) > i2s_tdm->io_multiplex = > of_property_read_bool(node, "rockchip,io-multiplex"); > > - ret = rockchip_i2s_tdm_get_calibrate_mclks(i2s_tdm); > - if (ret) > - return dev_err_probe(i2s_tdm->dev, ret, > - "mclk-calibrate clocks missing"); > - > regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res); > if (IS_ERR(regs)) { > return dev_err_probe(i2s_tdm->dev, PTR_ERR(regs), > @@ -1667,13 +1334,6 @@ static int rockchip_i2s_tdm_probe(struct platform_device *pdev) > goto err_disable_hclk; > } > > - if (i2s_tdm->mclk_calibrate) { > - i2s_tdm->mclk_root0_initial_freq = clk_get_rate(i2s_tdm->mclk_root0); > - i2s_tdm->mclk_root1_initial_freq = clk_get_rate(i2s_tdm->mclk_root1); > - i2s_tdm->mclk_root0_freq = i2s_tdm->mclk_root0_initial_freq; > - i2s_tdm->mclk_root1_freq = i2s_tdm->mclk_root1_initial_freq; > - } > - > pm_runtime_enable(&pdev->dev); > > regmap_update_bits(i2s_tdm->regmap, I2S_DMACR, I2S_DMACR_TDL_MASK, >
Hello Pavel, DT maintainers, [a question about DT bindings below] On Wed, 6 Mar 2024 08:43:57 +0100 Pavel Hofman <pavel.hofman@ivitera.com> wrote: > On 05. 03. 24 15:36, Luca Ceresoli wrote: > > The sample rates set by the rockchip_i2s_tdm driver in master mode are > > inaccurate up to 5% in several cases, due to the driver logic to configure > > clocks and a nasty interaction with the Common Clock Framework. > > > > To understand what happens, here is the relevant section of the clock tree > > (slightly simplified), along with the names used in the driver: > > > > vpll0 _OR_ vpll1 "mclk_root" > > clk_i2s2_8ch_tx_src "mclk_parent" > > clk_i2s2_8ch_tx_mux > > clk_i2s2_8ch_tx "mclk" or "mclk_tx" > > > > This is what happens when playing back e.g. at 192 kHz using > > audio-graph-card (when recording the same applies, only s/tx/rx/): > > > > 0. at probe, rockchip_i2s_tdm_set_sysclk() stores the passed frequency in > > i2s_tdm->mclk_tx_freq (*) which is 50176000, and that is never modified > > afterwards > > > > 1. when playback is started, rockchip_i2s_tdm_hw_params() is called and > > does the following two calls > > > > 2. rockchip_i2s_tdm_calibrate_mclk(): > > > > 2a. selects mclk_root0 (vpll0) as a parent for mclk_parent > > (mclk_tx_src), which is OK because the vpll0 rate is a good for > > 192000 (and sumbultiple) rates > > > > 2b. sets the mclk_root frequency based on ppm calibration computations > > > > 2c. sets mclk_tx_src to 49152000 (= 256 * 192000), which is also OK as > > it is a multiple of the required bit clock > > > > 3. rockchip_i2s_tdm_set_mclk() > > > > 3a. calls clk_set_rate() to set the rate of mclk_tx (clk_i2s2_8ch_tx) > > to the value of i2s_tdm->mclk_tx_freq (*), i.e. 50176000 which is > > not a multiple of the sampling frequency -- this is not OK > > > > 3a1. clk_set_rate() reacts by reparenting clk_i2s2_8ch_tx_src to > > vpll1 -- this is not OK because the default vpll1 rate can be > > divided to get 44.1 kHz and related rates, not 192 kHz > > > > The result is that the driver does a lot of ad-hoc decisions about clocks > > and ends up in using the wrong parent at an unoptimal rate. > > > > Step 0 is one part of the problem: unless the card driver calls set_sysclk > > at each stream start, whatever rate is set in mclk_tx_freq during boot will > > be taken and used until reboot. Moreover the driver does not care if its > > value is not a multiple of any audio frequency. > > > > Another part of the problem is that the whole reparenting and clock rate > > setting logic is conflicting with the CCF algorithms to achieve largely the > > same goal: selecting the best parent and setting the closest clock > > rate. And it turns out that only calling once clk_set_rate() on > > clk_i2s2_8ch_tx picks the correct vpll and sets the correct rate. > > > > The fix is based on removing the custom logic in the driver to select the > > parent and set the various clocks, and just let the Clock Framework do it > > all. As a side effect, the set_sysclk() op becomes useless because we now > > let the CCF compute the appropriate value for the sampling rate. It also > > implies that the whole calibration logic is now dead code and so it is > > removed along with the "PCM Clock Compensation in PPM" kcontrol, which has > > always been broken anyway. The handling of the 4 optional clocks also > > becomes dead code and is removed. > > > > The actual rates have been tested playing 30 seconds of audio at various > > sampling rates before and after this change using sox: > > > > time play -r <sample_rate> -n synth 30 sine 950 gain -3 > > > > The time reported in the table below is the 'real' value reported by the > > 'time' command in the above command line. > > > > rate before after > > --------- ------ ------ > > 8000 Hz 30.60s 30.63s > > 11025 Hz 30.45s 30.51s > > 16000 Hz 30.47s 30.50s > > 22050 Hz 30.78s 30.41s > > 32000 Hz 31.02s 30.43s > > 44100 Hz 30.78s 30.41s > > 48000 Hz 29.81s 30.45s > > 88200 Hz 30.78s 30.41s > > 96000 Hz 29.79s 30.42s > > 176400 Hz 27.40s 30.41s > > 192000 Hz 29.79s 30.42s > > > > While the tests are running the clock tree confirms that: > > > > * without the patch, vpll1 is always used and clk_i2s2_8ch_tx always > > produces 50176000 Hz, which cannot be divided for most audio rates > > except the slowest ones, generating inaccurate rates > > * with the patch: > > - for 192000 Hz vpll0 is used > > - for 176400 Hz vpll1 is used > > - clk_i2s2_8ch_tx always produces (256 * <rate>) Hz > > > > Tested on the RK3308 using the internal audio codec. > > > > Fixes: 081068fd6414 ("ASoC: rockchip: add support for i2s-tdm controller") > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > > > I tested this patch, it works OK on 8ch I2S0 of RK3308 Radxa Pi S, > frequencies checked in clock summary and clock pins with an > oscilloscope. It's basically identical issue with the same cause as I > reported in > https://lore.kernel.org/alsa-devel/20240304134329.392c75bf@booty/T/#m19d69461aa827f15a86d6d31ed1b1520e80a909e > > > Just a note - the patch changes mclk frequencies from fixed 256 x > 192000/176400 to variable 256 x fs. While 256 x fs is more standard > than the fixed mclk for all samplerates of the same family, it may cause > changes for existing users if using the mclk output pins. But the > existing code was broken and had to be hacked to work anyway. Perhaps > this may need some considering. Thanks for clearly describing this difference. This is something I considered and in the end I found no reason to use 256 x 192000/176400 as it would use higher clocks than needed (or equal, at 192 and 176.4 kHz), deviating from the common practice and probably consuming a little more power, all apparently without any gains. And, last but not least, without a motivation in the docs I have available. Of course, should there be known reasons to keep the 256 x 192000/176400 frequencies, I'd be fine with changing my patch. > Also I wonder if the DTS description rockchip,i2s-tdm.yaml file may be > updated too, as some clocks listed by it are removed by the patch: > > -const: mclk_tx_src > -const: mclk_rx_src > -const: mclk_root0 > -const: mclk_root1 > > IMO it may be a bit confusing if the description lists parameters which > are not actually used by the driver anymore. You have a good point. I just checked the git history and indeed these four clocks have never been mentioned by any of the device trees in mainline. However removal of parts of bindings is normally never supposed to happen for backward compatibility. @DT maintainers: would removal of optional clocks from bindings allowed in this specific case? Quick facts: * bindings introduced in 2021, with the 4 optional clocks from the beginning, no changes afterwards * none of the optional clocks ever appeared in arch/.../*.dts * the 4 optional clocks actually describe a part of the clock topology, so their presence in I2S bindings is debatable at least * the I2S controller described by the bindings does not need the 4 optional clocks to work properly if the clocks are used correctly, as this patch proves If this looks like a good idea, I can send a patch to remove those optional clocks. Otherwise I can send one to add a comment to discourage people from using them. Best regards, Luca
On Tue, 05 Mar 2024 15:36:27 +0100, Luca Ceresoli wrote: > This series adds a driver for the internal audio codec of the Rockchip > RK3308 SoC, along with some related patches. This codec is internally > connected to the I2S peripherals on the same chip, and it has some > peculiarities arising from that interconnection. > > For proper bidirectional operation with the internal codec at any possible > combination of sampling rates, the I2S peripheral needs two clock sources > (tx and rx), while connection with an external codec commonly needs only > one. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/7] ASoC: rockchip: i2s-tdm: Fix inaccurate sampling rates commit: 9e2ab4b18ebd46813fc3459207335af4d368e323 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
On Tue, 05 Mar 2024 15:36:27 +0100, Luca Ceresoli wrote: > This series adds a driver for the internal audio codec of the Rockchip > RK3308 SoC, along with some related patches. This codec is internally > connected to the I2S peripherals on the same chip, and it has some > peculiarities arising from that interconnection. > > For proper bidirectional operation with the internal codec at any possible > combination of sampling rates, the I2S peripheral needs two clock sources > (tx and rx), while connection with an external codec commonly needs only > one. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [2/7] ASoC: dt-bindings: Add Rockchip RK3308 internal audio codec commit: cce4cbb157493483f03c21213753b66425a31430 [3/7] ASoC: core: add SOC_DOUBLE_RANGE_TLV() helper macro commit: d75a21611a6e723d81db3f827e131ad39b69186c [4/7] ASoC: codecs: Add RK3308 internal audio codec driver commit: 4ed0915f5bc4bcc81bca783a5b984f3d81e9764e All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
On Tue, 05 Mar 2024 15:36:27 +0100, Luca Ceresoli wrote: > This series adds a driver for the internal audio codec of the Rockchip > RK3308 SoC, along with some related patches. This codec is internally > connected to the I2S peripherals on the same chip, and it has some > peculiarities arising from that interconnection. > > For proper bidirectional operation with the internal codec at any possible > combination of sampling rates, the I2S peripheral needs two clock sources > (tx and rx), while connection with an external codec commonly needs only > one. > > [...] Applied, thanks! [5/7] arm64: defconfig: enable Rockchip RK3308 internal audio codec driver commit: 9fdd7b45da18b84d5e7d5a6b8b4b0167910f2d13 [6/7] arm64: dts: rockchip: add i2s_8ch_2 and i2s_8ch_3 commit: b5ffc424360eaced41f405f0e38bcabe61fecf39 [7/7] arm64: dts: rockchip: add the internal audio codec commit: 30d72458624bb1ba7bab1c7a1d5f4c42f512010c Best regards,
This series adds a driver for the internal audio codec of the Rockchip RK3308 SoC, along with some related patches. This codec is internally connected to the I2S peripherals on the same chip, and it has some peculiarities arising from that interconnection. For proper bidirectional operation with the internal codec at any possible combination of sampling rates, the I2S peripheral needs two clock sources (tx and rx), while connection with an external codec commonly needs only one. Since v5.16 there is a driver for the I2S in sound/soc/rockchip/rockchip_i2s_tdm.c, but in some cases it does not configure correctly the clocks, resulting in an unnecessarily inaccurate rate. Patch 1 fixes this. Patches 2-4 add the codec driver along with the bindings and a new helper macro. Patches 5-7 add to the SoC DT file two I2S controllers (those which are internally connected to the internal codec) and the codec itself and enable the driver in the ARM64 defconfig. Luca Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> --- Changes in v4: - several cleanups in the codec probe function - Link to v3: https://lore.kernel.org/r/20240221-rk3308-audio-codec-v3-0-dfa34abfcef6@bootlin.com Changes in v3: - Add the I2S clock fix patch and remove a previous fix which is now superseded - Codec driver: fix silent playback until a given amplitude of sigital value, seen at >= 96 kHz rate - various other changes, listed per-patch - Link to v2: https://lore.kernel.org/r/20231219-rk3308-audio-codec-v2-0-c70d06021946@bootlin.com Changes in v2: - largely rewrote the codec driver to use DAPM and lots of improvements and cleanups - removed the RK3308 audio card and related patches - various other changes, listed per-patch - Link to v1: https://lore.kernel.org/all/20220907142124.2532620-1-luca.ceresoli@bootlin.com/ --- Luca Ceresoli (7): ASoC: rockchip: i2s-tdm: Fix inaccurate sampling rates ASoC: dt-bindings: Add Rockchip RK3308 internal audio codec ASoC: core: add SOC_DOUBLE_RANGE_TLV() helper macro ASoC: codecs: Add RK3308 internal audio codec driver arm64: defconfig: enable Rockchip RK3308 internal audio codec driver arm64: dts: rockchip: add i2s_8ch_2 and i2s_8ch_3 arm64: dts: rockchip: add the internal audio codec .../bindings/sound/rockchip,rk3308-codec.yaml | 98 +++ MAINTAINERS | 7 + arch/arm64/boot/dts/rockchip/rk3308.dtsi | 56 ++ arch/arm64/configs/defconfig | 1 + include/sound/soc.h | 12 + sound/soc/codecs/Kconfig | 11 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/rk3308_codec.c | 974 +++++++++++++++++++++ sound/soc/codecs/rk3308_codec.h | 579 ++++++++++++ sound/soc/rockchip/rockchip_i2s_tdm.c | 352 +------- 10 files changed, 1746 insertions(+), 346 deletions(-) --- base-commit: dfda120c512b3edca1436f770924e91b14f93a98 change-id: 20231219-rk3308-audio-codec-a5558ba8949d Best regards,