• Luca Ceresoli's avatar
    ASoC: rockchip: i2s-tdm: Fix inaccurate sampling rates · 9e2ab4b1
    Luca Ceresoli authored
    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: 081068fd ("ASoC: rockchip: add support for i2s-tdm controller")
    Signed-off-by: default avatarLuca Ceresoli <luca.ceresoli@bootlin.com>
    Link: https://msgid.link/r/20240305-rk3308-audio-codec-v4-1-312acdbe628f@bootlin.comSigned-off-by: default avatarMark Brown <broonie@kernel.org>
    9e2ab4b1
rockchip_i2s_tdm.c 35.9 KB