On 1/15/26 1:05 AM, Macpaul Lin (林智斌) wrote:
> On Thu, 2025-12-18 at 17:23 -0600, David Lechner wrote:
>> Fix a few missing clocks and even more clocks in the incorrect order.
>> Since the clocks are looked up by index, having them out of order or
>> skipping an ID will lead to incorrect clocks being used.
>>
>> Signed-off-by: David Lechner <[email protected]>
>> ---
>> drivers/clk/mediatek/clk-mt8365.c | 32 +++++++++++++++++------------
>> ---
>> 1 file changed, 17 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/clk/mediatek/clk-mt8365.c
>> b/drivers/clk/mediatek/clk-mt8365.c
>> index c88545fc7cf..b6332b14aea 100644
>> --- a/drivers/clk/mediatek/clk-mt8365.c
>> +++ b/drivers/clk/mediatek/clk-mt8365.c
>> @@ -80,7 +80,7 @@ static const struct mtk_fixed_clk top_fixed_clks[]
>> = {
>> FACTOR(_id, _parent, _mult, _div, CLK_PARENT_APMIXED)
>>
>> static const struct mtk_fixed_factor top_divs[] = {
>> - PLL_FACTOR(CLK_TOP_SYS_26M_D2, "sys_26m_d2", CLK_XTAL, 1, 2),
>> + PLL_FACTOR(CLK_TOP_MFGPLL, "mfgpll_ck", CLK_APMIXED_MFGPLL,
>> 1, 1),
>> PLL_FACTOR(CLK_TOP_SYSPLL_D2, "syspll_d2",
>> CLK_APMIXED_MAINPLL, 1, 2),
>> PLL_FACTOR(CLK_TOP_SYSPLL1_D2, "syspll1_d2",
>> CLK_APMIXED_MAINPLL, 1, 4),
>> PLL_FACTOR(CLK_TOP_SYSPLL1_D4, "syspll1_d4",
>> CLK_APMIXED_MAINPLL, 1, 8),
>> @@ -110,7 +110,6 @@ static const struct mtk_fixed_factor top_divs[] =
>> {
>> PLL_FACTOR(CLK_TOP_UNIVPLL3_D4, "univpll3_d4",
>> CLK_APMIXED_UNIVPLL, 1, 20),
>> PLL_FACTOR(CLK_TOP_MMPLL, "mmpll_ck", CLK_APMIXED_MMPLL, 1,
>> 1),
>> PLL_FACTOR(CLK_TOP_MMPLL_D2, "mmpll_d2", CLK_APMIXED_MMPLL,
>> 1, 2),
>> - PLL_FACTOR(CLK_TOP_MFGPLL, "mfgpll_ck", CLK_APMIXED_MFGPLL,
>> 1, 1),
>> PLL_FACTOR(CLK_TOP_LVDSPLL_D2, "lvdspll_d2",
>> CLK_APMIXED_LVDSPLL, 1, 2),
>
> I'm not familiar with clock driver, however, I've compare this
> modification with kernel's clock order then found some differences.
>
> For example,
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/mediatek/clk-mt8365.c#n34
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/mediatek/clk-mt8365.c#n64
>
> The origin order of clock CLK_TOP_SYS_26M_D2, and CLK_TOP_MFGPLL are
> the same as kernel upstream. Are the behaviors of these clocks
> different from kernel upstream?
In the Linux kernel, each clock is registered separately and clocks are
looked up by name rather than array index. So in the Linux kernel, the
order does ntt matter. In U-Boot, only the parent device is registered and
individual clocks are looked up by array index so order does matter.
>
> [snip]
>
>> PLL_FACTOR(CLK_TOP_LVDSPLL_D4, "lvdspll_d4",
>> CLK_APMIXED_LVDSPLL, 1, 4),
>> PLL_FACTOR(CLK_TOP_LVDSPLL_D8, "lvdspll_d8",
>> CLK_APMIXED_LVDSPLL, 1, 8),
>> @@ -128,6 +127,7 @@ static const struct mtk_fixed_factor top_divs[] =
>> {
>> PLL_FACTOR(CLK_TOP_APLL2_D2, "apll2_d2", CLK_APMIXED_APLL2,
>> 1, 2),
>> PLL_FACTOR(CLK_TOP_APLL2_D4, "apll2_d4", CLK_APMIXED_APLL2,
>> 1, 4),
>> PLL_FACTOR(CLK_TOP_APLL2_D8, "apll2_d8", CLK_APMIXED_APLL2,
>> 1, 8),
>> + PLL_FACTOR(CLK_TOP_SYS_26M_D2, "sys_26m_d2", CLK_XTAL, 1, 2),
>
> And so on for the other re-ordered clocks.
>
> [snip]
>
>
> Could you help to explain how to verify it?
I verified the U-Boot driver by running the `clk dump` command. This will
print `error!` if any clocks are out of order (which would break array index
lookup) or if they are missing a parent clock.
Once I fixed all of the errors, then I checked that the parent printed for
each clock matched the documentation. There were a few mistakes, so I fixed
those as well (in a separate patch).
Now that we have the `clk dump` command for Mediatek clocks, I recommend doing
this on the other boards in U-Boot as well. I expect we will find more bugs
like this.
> Did you dump clock tree and compare the orders between u-boot and Linux
> kernel?
Since the order doesn't matter in Linux, I did not do this.
> Do we need to update kernel's clock driver for MT8365?
I don't think so. All of the bugs I found in U-Boot had to do with the
array lookup implementation which does not apply to Linux.
>
> Thanks
> Macpaul Lin
>
