Looping back on this, after digging a bit deeper...
On Fri, Feb 14, 2020 at 9:38 AM Nick Fan (范哲維) Nick.Fan@mediatek.com wrote:
[snip]
Another thing that I'm not implementing is the dance that Mediatek does in their kbase driver when changing the clock (described in patch 2/7): "" The binding we use with out-of-tree Mali drivers includes more clocks, this is used for devfreq: the out-of-tree driver switches clk_mux to clk_sub_parent (26Mhz), adjusts clk_main_parent, then switches clk_mux back to clk_main_parent: (see https://chromium.googlesource.com/chromiumos/third_party/kernel/+/ch romeos-4.19/drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_run time_pm.c#423) clocks = <&topckgen CLK_TOP_MFGPLL_CK>, <&topckgen CLK_TOP_MUX_MFG>, <&clk26m>, <&mfgcfg CLK_MFG_BG3D>; clock-names = "clk_main_parent", "clk_mux", "clk_sub_parent", "subsys_mfg_cg"; "" Is there a clean/simple way to implement this in the clock framework/device tree? Or should we implement something in the panfrost driver?
Putting parent clocks into 'clocks' for a device is a pretty common abuse. The 'assigned-clocks' binding is what's used for parent clock setup. Not sure that's going to help here though. Is this dance because the parent clock frequency can't be changed cleanly?
Nick/Weiyi, any idea why we do that dance in the first place? (maybe the PLL clock is unstable while it's being changed?)
Clock source may become unstable during clock frequency changes, so it is always safer to switch to a more reliable clock source. Otherwise, it may cause some problem in some corner case. I would suggest to keep it.
The Mediatek CPUfreq driver actually does a very similar dance: https://github.com/torvalds/linux/blob/master/drivers/cpufreq/mediatek-cpufr...
What they have in the device tree is the main clock, and the "intermediate" clock that is required during switching: clocks = <&mcucfg CLK_MCU_MP0_SEL>, <&topckgen CLK_TOP_ARMPLL_DIV_PLL1>; clock-names = "cpu", "intermediate";
The topology looks like this: clk26m 15 15 1 26000000 0 0 50000 armpll_ll 1 1 0 1417000000 0 0 50000 mcu_mp0_sel 0 0 0 1417000000 0 0 50000
And device tree provides mcu_mp0_sel as "cpu", and the armpll_div_pll1 as "intermediate".
The driver looks up armpll_ll by calling get_parent, then: - set_parent(mcu_mp0_sel, armpll_div_pll1) - set_rate(armpll_ll, new_rate) - set_parent(mcu_mp0_sel, armpll_ll)
On MT8183's GPU, the topology is a little bit more complicated (but I think there should be a way to merge mfg_bg3d an mfg_sel in the clock core) clk26m 15 15 1 26000000 0 0 50000 mfgpll 1 1 0 419999817 0 0 50000 mfgpll_ck 2 2 0 419999817 0 0 50000 mfg_sel 3 3 0 419999817 0 0 50000 mfg_bg3d 1 1 0 419999817 0 0 50000
We're going to need a special panfrost devfreq driver for mt8183 anyway (to handle the 2 regulators), so it would be easy to take a similar approach: - Add "intermediate" clock in the device tree (clk26m) - Find mfg_sel/mfgpll_ck using 1/2 clk_get_parent calls. - Switch mfg_sel to clk26m, set mfgpll_ck rate, switch mfg_sel back to mfgpll_ck.
(BTW, I tried to look, and couldn't find examples or reparenting during clock changes in drivers/clk, are there existing drivers doing similar things? Or this would be new?).