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@xxxxxxx> 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
