| Message ID | 20240929100750.860329-4-ryan@testtoast.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: add Allwinner H616 audio codec support | expand |
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
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 >
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 --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
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(-)