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

["Ryan Walklin" <ryan@testtoast.com>]

On Fri, 18 Oct 2024, at 10:29 PM, Andre Przywara wrote:
> On Tue, 1 Oct 2024 14:28:50 +0100
> Andre Przywara <andre.przywara@arm.com> wrote:
>
> Hi Ryan,

>> 
>> 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 ;-)

Thanks, have updated the patch as above and the manual-described multipliers are working.
>
>> And please explain all this in comments ...

Will do.

>> 
>> >  #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?

Thanks, noted.

>> 
>> > +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.

Corrected for v2.
>> 
>> > +	.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.

Fixed, ta.

>> >  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?

Thanks, have updated.

Regards,

Ryan