diff mbox series

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

Message ID 20240929100750.860329-4-ryan@testtoast.com (mailing list archive)
State Changes Requested, archived
Headers show
Series ASoC: add Allwinner H616 audio codec support | expand

Commit Message

Ryan Walklin Sept. 29, 2024, 10:06 a.m. UTC
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.

Add SDM to the H616 clock control unit driver.

Signed-off-by: Ryan Walklin <ryan@testtoast.com>
---
 drivers/clk/sunxi-ng/ccu-sun50i-h616.c | 36 +++++++++++++-------------
 1 file changed, 18 insertions(+), 18 deletions(-)

Comments

Andre Przywara Oct. 1, 2024, 1:28 p.m. UTC | #1
On Sun, 29 Sep 2024 23:06:04 +1300
Ryan Walklin <ryan@testtoast.com> 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?

And please explain all this in comments ...

> Add SDM to the H616 clock control unit driver.
> 
> Signed-off-by: Ryan Walklin <ryan@testtoast.com>
> ---
>  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[] = {
>   */
>  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
Andre Przywara Oct. 18, 2024, 9:29 a.m. UTC | #2
On Tue, 1 Oct 2024 14:28:50 +0100
Andre Przywara <andre.przywara@arm.com> wrote:

Hi Ryan,

> On Sun, 29 Sep 2024 23:06:04 +1300
> Ryan Walklin <ryan@testtoast.com> 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@testtoast.com>
> > ---
> >  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  
>
Ryan Walklin Oct. 20, 2024, 6:38 a.m. UTC | #3
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
diff mbox series

Patch

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
+
+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),
+	.sdm		= _SUNXI_CCU_SDM(pll_audio_sdm_table,
+				  BIT(24), 0x178, BIT(31)),
+
 	.common		= {
+		.features = CCU_FEATURE_SIGMA_DELTA_MOD,
 		.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[] = {
  */
 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);
 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.
-	 */
 	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