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
>
