Re: [PATCH 3/6] clk: sunxi-ng: h616: Add sigma-delta modulation settings for audio PLL

[Andre Przywara <andre.przywara@xxxxxxx>]

On Tue, 1 Oct 2024 14:28:50 +0100
Andre Przywara <andre.przywara@xxxxxxx> wrote:

Hi Ryan,

> On Sun, 29 Sep 2024 23:06:04 +1300
> Ryan Walklin <ryan@xxxxxxxxxxxxx> wrote:
> 
> Hi Ryan,
> 
> > Allwinner has previously released a H616 audio driver which also
> > provides sigma-delta modulation for the audio PLL clocks. This approach
> > is used in other Allwinner SoCs, including the H3 and A64.
> > 
> > One change from the vendor code is made to the PLL clocks, the 
> > vendor-specified dividers of 4/2/1 for the 1/2/4x clocks respectively result 
> > in audio playback that is too slow by 50%. Therefore the dividers are simply
> > doubled to 8/4/2 which results in correct playback rates.  
> 
> The reason for that is you force .M0 to 1 (divide by 2), in the fixup
> below (in the probe routine).
> So for instance for the 4x clock, the formula is:
> 	PLL_AUDIO(4X) = 24MHz*N/M0/M1/P
> M1 is cleared (div by 1), M0 is set (div by 2), P is exposed as .m, and N
> as .n in the ccu_nm struct. So you get that extra by-2 divider, that is
> invisible to the CCF, hence you need to compensate for that.
> 
> But with tweaking the dividers only in the fixed-factor clocks below, you
> still leave the original (_hs) clock wrong, which is a parent to other
> clocks, if I see this correctly.
> 
> Can you try to add a .fixed_post_div = 2 in the ccu_nm definition, and
> then put the real dividers in the fixed-factor clocks?

So I tested this change, and it seemed to work for me. Please don't
forget to add CCU_FEATURE_FIXED_POSTDIV - as I did initially ;-)

> And please explain all this in comments ...
> 
> > Add SDM to the H616 clock control unit driver.
> > 
> > Signed-off-by: Ryan Walklin <ryan@xxxxxxxxxxxxx>
> > ---
> >  drivers/clk/sunxi-ng/ccu-sun50i-h616.c | 36 +++++++++++++-------------
> >  1 file changed, 18 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h616.c b/drivers/clk/sunxi-ng/ccu-sun50i-h616.c
> > index 84e406ddf9d12..be272947b0fee 100644
> > --- a/drivers/clk/sunxi-ng/ccu-sun50i-h616.c
> > +++ b/drivers/clk/sunxi-ng/ccu-sun50i-h616.c
> > @@ -215,20 +215,23 @@ static struct ccu_nkmp pll_de_clk = {
> >  	},
> >  };
> >  
> > -/*
> > - * TODO: Determine SDM settings for the audio PLL. The manual suggests
> > - * PLL_FACTOR_N=16, PLL_POST_DIV_P=2, OUTPUT_DIV=2, pattern=0xe000c49b
> > - * for 24.576 MHz, and PLL_FACTOR_N=22, PLL_POST_DIV_P=3, OUTPUT_DIV=2,
> > - * pattern=0xe001288c for 22.5792 MHz.
> > - * This clashes with our fixed PLL_POST_DIV_P.
> > - */  
> 
> >  #define SUN50I_H616_PLL_AUDIO_REG	0x078
> > +  
> 
> Can you please (re-)add a comment here explaining the sources of these
> parameters? Because the values deviate from the ones in the manual.
> And also please mention here that there are two more divider bits (named
> m0 and m1 in the manual), that we cannot model in our ccu_nm struct, and
> thus force them to fixed values in the probe routine below?
> 
> > +static struct ccu_sdm_setting pll_audio_sdm_table[] = {
> > +	{ .rate = 90316800, .pattern = 0xc001288d, .m = 3, .n = 22 },
> > +	{ .rate = 98304000, .pattern = 0xc001eb85, .m = 5, .n = 40 },
> > +};
> > +
> >  static struct ccu_nm pll_audio_hs_clk = {
> >  	.enable		= BIT(31),
> >  	.lock		= BIT(28),
> > -	.n		= _SUNXI_CCU_MULT_MIN(8, 8, 12),
> > -	.m		= _SUNXI_CCU_DIV(1, 1), /* input divider */
> > +	.n			= _SUNXI_CCU_MULT_MIN(8, 8, 12),
> > +	.m			= _SUNXI_CCU_DIV(16, 6),  
> 
> Can you please keep the original indentation? You could add a "post-div"
> comment after the .m parameter, to map to the manual.
> 
> And add that ".fixed_post_div = 2," here.
> 
> > +	.sdm		= _SUNXI_CCU_SDM(pll_audio_sdm_table,
> > +				  BIT(24), 0x178, BIT(31)),
> > +
> >  	.common		= {
> > +		.features = CCU_FEATURE_SIGMA_DELTA_MOD,  
> 
> Please indent like the other parameters below.
> 
> >  		.reg		= 0x078,
> >  		.hw.init	= CLK_HW_INIT("pll-audio-hs", "osc24M",
> >  					      &ccu_nm_ops,
> > @@ -690,13 +693,13 @@ static const struct clk_hw *clk_parent_pll_audio[] = {
> >   */
                     ^^^^
There is a comment up here, not shown in this diff, which doesn't apply
anymore. Please update it.

Cheers,
Andre

> >  static CLK_FIXED_FACTOR_HWS(pll_audio_1x_clk, "pll-audio-1x",
> >  			    clk_parent_pll_audio,
> > -			    96, 1, CLK_SET_RATE_PARENT);
> > +			    8, 1, CLK_SET_RATE_PARENT);  
> 
> As mentioned, with the fixed_post_div, you should be able to put the real
> divider values in here.
> 
> >  static CLK_FIXED_FACTOR_HWS(pll_audio_2x_clk, "pll-audio-2x",
> >  			    clk_parent_pll_audio,
> > -			    48, 1, CLK_SET_RATE_PARENT);
> > +			    4, 1, CLK_SET_RATE_PARENT);
> >  static CLK_FIXED_FACTOR_HWS(pll_audio_4x_clk, "pll-audio-4x",
> >  			    clk_parent_pll_audio,
> > -			    24, 1, CLK_SET_RATE_PARENT);
> > +			    2, 1, CLK_SET_RATE_PARENT);
> >  
> >  static const struct clk_hw *pll_periph0_parents[] = {
> >  	&pll_periph0_clk.common.hw
> > @@ -1135,13 +1138,10 @@ static int sun50i_h616_ccu_probe(struct platform_device *pdev)
> >  		writel(val, reg + usb2_clk_regs[i]);
> >  	}
> >  
> > -	/*
> > -	 * Force the post-divider of pll-audio to 12 and the output divider
> > -	 * of it to 2, so 24576000 and 22579200 rates can be set exactly.
> > -	 */  
> 
> Can you please keep the comment, and adjust it accordingly? Saying that
> the recommended BSP parameters for the PLL audio recommend M0 to be 1, and
> M1 to be 0, and that we enforce this here?
> 
> Cheers,
> Andre
> 
> >  	val = readl(reg + SUN50I_H616_PLL_AUDIO_REG);
> > -	val &= ~(GENMASK(21, 16) | BIT(0));
> > -	writel(val | (11 << 16) | BIT(0), reg + SUN50I_H616_PLL_AUDIO_REG);
> > +	val &= ~BIT(1);
> > +	val |= BIT(0);
> > +	writel(val, reg + SUN50I_H616_PLL_AUDIO_REG);
> >  
> >  	/*
> >  	 * First clock parent (osc32K) is unusable for CEC. But since there  
>