| Message ID | 20220208124034.414635-1-wenst@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | clk: mediatek: Cleanups and Improvements - Part 1 | expand |
Hi, I couldn't find a particular patch to reply to so I'm replying cover letter to give some input on the PLL subsystem. On Tue, 8 Feb 2022 20:40:03 +0800 Chen-Yu Tsai <wenst@chromium.org> wrote: > drivers/clk/mediatek/clk-pll.c | 100 +++++- > drivers/clk/mediatek/clk-pll.h | 57 ++++ In clk-pll.c there is an mtk_clk_register_pll function which at some point executes this: > init.ops = &mtk_pll_ops; In my opinion there should be a possibility to define a custom mtk_pll_ops for a given SoC instead of using a hardcoded one because not all Mediatek SoCs share the same PLL startup/powerdown flow. For example, the existing mtk_pll_prepare implementation won't work for the entire Mediatek Cortex-A9 SoC family (this includes but not limited to mt6515, mt6517, mt6575, and mt6577). > static int mtk_pll_prepare(struct clk_hw *hw) > { > struct mtk_clk_pll *pll = to_mtk_clk_pll(hw); > u32 r; > u32 div_en_mask; > > r = readl(pll->pwr_addr) | CON0_PWR_ON; > writel(r, pll->pwr_addr); This code sets a bit to 1 to start a PLL but the SoCs I mentioned above would need to have that bit cleared (set to 0) [1] [2]. Another interesting thing in mtk_pll_prepare is > udelay(20); Is 20 ms a settle time for PLL? If yes then it would also be cool to specify an arbitrary value easily as some PLLs have longer settle time [3] [4]. Worth noting the SoCs I mentioned aren't in mainline yet, and I think there are more modern mainline-worthy Mediatek SoCs that might also need these changes in the future. Thanks. [1] MT6577 HSPA Smartphone Application Processor Datasheet, pages 1212-1227 (*_CON0 registers). [2] MT6515 GSM/EDGE Smartphone Application Processor Datasheet, pages 1202-1216 (*_CON0 registers). [3] pages 1303-1306 of [1] [4] MT6589 HSPA+ Smartphone Application Processor Datasheet, page 1344 (MDPLL1 & MDPLL2)
Hi, On Wed, Feb 9, 2022 at 3:32 AM Boris Lysov <arz65xx@gmail.com> wrote: > > Hi, I couldn't find a particular patch to reply to so I'm replying cover > letter to give some input on the PLL subsystem. > > On Tue, 8 Feb 2022 20:40:03 +0800 > Chen-Yu Tsai <wenst@chromium.org> wrote: > > drivers/clk/mediatek/clk-pll.c | 100 +++++- > > drivers/clk/mediatek/clk-pll.h | 57 ++++ > > In clk-pll.c there is an mtk_clk_register_pll function which at some point > executes this: > > > init.ops = &mtk_pll_ops; > > In my opinion there should be a possibility to define a custom mtk_pll_ops for a > given SoC instead of using a hardcoded one because not all Mediatek SoCs share > the same PLL startup/powerdown flow. For example, the existing mtk_pll_prepare > implementation won't work for the entire Mediatek Cortex-A9 SoC family (this > includes but not limited to mt6515, mt6517, mt6575, and mt6577). Ack. My scope is limited to SoCs used in Chromebooks. However Miles and Chun-Jie, who are Cc-ed on the series, should know more. That said, we can implement support for these varying parameters as we see them, not before. > > static int mtk_pll_prepare(struct clk_hw *hw) > > { > > struct mtk_clk_pll *pll = to_mtk_clk_pll(hw); > > u32 r; > > u32 div_en_mask; > > > > r = readl(pll->pwr_addr) | CON0_PWR_ON; > > writel(r, pll->pwr_addr); > > This code sets a bit to 1 to start a PLL but the SoCs I mentioned above would > need to have that bit cleared (set to 0) [1] [2]. > > Another interesting thing in mtk_pll_prepare is > > udelay(20); > Is 20 ms a settle time for PLL? If yes then it would also be cool to specify an > arbitrary value easily as some PLLs have longer settle time [3] [4]. This is a question for whomever upstreamed the driver. > Worth noting the SoCs I mentioned aren't in mainline yet, and I think there are > more modern mainline-worthy Mediatek SoCs that might also need these changes in > the future. Again, we can implement varying parameters as they appear. Thanks ChenYu > > Thanks. > > [1] MT6577 HSPA Smartphone Application Processor Datasheet, pages 1212-1227 > (*_CON0 registers). > [2] MT6515 GSM/EDGE Smartphone Application Processor Datasheet, pages > 1202-1216 (*_CON0 registers). > [3] pages 1303-1306 of [1] > [4] MT6589 HSPA+ Smartphone Application Processor Datasheet, page 1344 > (MDPLL1 & MDPLL2)
Hi everyone, This is version 3 of part 1 of planned work to cleanup, improve, and modernize the MediaTek clk drivers. The full proposal can be found here: https://lore.kernel.org/linux-clk/20220122091731.283592-1-wenst@chromium.org/ Changes since version 2: - Collected review tags - Moved platform_set_drvdata() in clk_mt8195_apusys_pll_probe() to the correct patch (patch 30) - Corrected error path for clk_mt8195_apmixed_probe() in patch "clk: mediatek: mt8195: Implement error handling in probe functions" - Also dropped review tag for this patch Changes since version 1: - Added NULL pointer check against __clk_get_hw() return value in mtk_clk_unregister_pll() - Added NULL pointer check against __clk_get_hw() return value in mtk_clk_unregister_composite() - Copied MHZ macro into clk-pll.c and restored its usage - Dropped extra semicolon in mtk_clk_register_plls() - Reported by kernel test robot <lkp@intel.com> I also wanted to ask, what would be a good way to do the "struct clk" to "struct clk_hw" API conversion? A. Add new register/unregister APIs that use "struct clk_hw" instead of "struct clk". This would result in a lot of patches and a lot of churn though. B. Replace "struct clk" with "struct clk_hw" (and "struct clk_onecell_data" with "struct clk_hw_onecell_data") and work around existing code with __clk_get_hw() or its opposite. This would result in a few huge pages, as to not break bisection, but would likely cause less churn. And it would cover all existing platforms in one go. Thanks ChenYu Original cover letter sans full proposal as follows: Part 1 involves cleaning up various parts of the MediaTek common clk driver library: - Code style cleanups in places touched by other changes - Code for a given clk type that happened to be split between different files is consolidated - Parts of the implementation, such as the internal data structures and registration/unregistration of a single clk, are internalized and no longer exported to other parts of the library - Unregister APIs for all clk types in the library are added - Remove function added for the simple driver case - Error handling is added for all clk types - MT8195 clk drivers cleaned up with error handling and removal support added Adding error handling to the clk driver library results is a change in its behavior, and might cause some breakage where the affected system was originally somewhat working, but not completely. The failure would result from a duplicated clk, either in name or ID. Old behavior was to skip over these and ignore the error, whereas the new behavior is to restore changes and error out. To detect duplicate IDs, an additional warning will now be printed when such occurrences happen. In a correct driver such things should not happen though, so maybe we could consider wrapping that in some debug Kconfig option. That's why I put it as the last patch in this series. This series was tested on MT8195 with the series applied on the downstream ChromeOS v5.10 kernel. Some other patches were applied to make this series apply cleanly, but nothing The cpumux clk type changes were not tested due to lack of related hardware. However the changes are very similar to the other types, and I am confident there are no issues. Chen-Yu Tsai (31): clk: mediatek: Use %pe to print errors clk: mediatek: gate: Consolidate gate type clk related code clk: mediatek: gate: Internalize clk implementation clk: mediatek: gate: Implement unregister API clk: mediatek: gate: Clean up included headers clk: mediatek: cpumux: Implement unregister API clk: mediatek: cpumux: Internalize struct mtk_clk_cpumux clk: mediatek: cpumux: Clean up included headers clk: mediatek: mux: Implement unregister API clk: mediatek: mux: Internalize struct mtk_clk_mux clk: mediatek: mux: Clean up included headers clk: mediatek: pll: Split definitions into separate header file clk: mediatek: pll: Implement unregister API clk: mediatek: pll: Clean up included headers clk: mediatek: Implement mtk_clk_unregister_fixed_clks() API clk: mediatek: Implement mtk_clk_unregister_factors() API clk: mediatek: Implement mtk_clk_unregister_divider_clks() API clk: mediatek: Implement mtk_clk_unregister_composites() API clk: mediatek: Add mtk_clk_simple_remove() clk: mediatek: mtk: Clean up included headers clk: mediatek: cpumux: Implement error handling in register API clk: mediatek: gate: Implement error handling in register API clk: mediatek: mux: Reverse check for existing clk to reduce nesting level clk: mediatek: mux: Implement error handling in register API clk: mediatek: pll: Implement error handling in register API clk: mediatek: mtk: Implement error handling in register APIs clk: mediatek: Unregister clks in mtk_clk_simple_probe() error path clk: mediatek: mt8195: Hook up mtk_clk_simple_remove() clk: mediatek: mt8195: Implement error handling in probe functions clk: mediatek: mt8195: Implement remove functions clk: mediatek: Warn if clk IDs are duplicated drivers/clk/mediatek/clk-apmixed.c | 2 +- drivers/clk/mediatek/clk-cpumux.c | 69 +++- drivers/clk/mediatek/clk-cpumux.h | 13 +- drivers/clk/mediatek/clk-gate.c | 148 +++++++-- drivers/clk/mediatek/clk-gate.h | 59 ++-- drivers/clk/mediatek/clk-mt2701.c | 5 +- drivers/clk/mediatek/clk-mt2712.c | 3 +- drivers/clk/mediatek/clk-mt6765.c | 3 +- drivers/clk/mediatek/clk-mt6779.c | 3 +- drivers/clk/mediatek/clk-mt6797.c | 3 +- drivers/clk/mediatek/clk-mt7622.c | 5 +- drivers/clk/mediatek/clk-mt7629.c | 5 +- drivers/clk/mediatek/clk-mt7986-apmixed.c | 4 +- drivers/clk/mediatek/clk-mt8135.c | 3 +- drivers/clk/mediatek/clk-mt8167.c | 3 +- drivers/clk/mediatek/clk-mt8173.c | 5 +- drivers/clk/mediatek/clk-mt8183.c | 3 +- drivers/clk/mediatek/clk-mt8192.c | 3 +- drivers/clk/mediatek/clk-mt8195-apmixedsys.c | 30 +- drivers/clk/mediatek/clk-mt8195-apusys_pll.c | 25 +- drivers/clk/mediatek/clk-mt8195-cam.c | 1 + drivers/clk/mediatek/clk-mt8195-ccu.c | 1 + drivers/clk/mediatek/clk-mt8195-img.c | 1 + .../clk/mediatek/clk-mt8195-imp_iic_wrap.c | 1 + drivers/clk/mediatek/clk-mt8195-infra_ao.c | 1 + drivers/clk/mediatek/clk-mt8195-ipe.c | 1 + drivers/clk/mediatek/clk-mt8195-mfg.c | 1 + drivers/clk/mediatek/clk-mt8195-peri_ao.c | 1 + drivers/clk/mediatek/clk-mt8195-scp_adsp.c | 1 + drivers/clk/mediatek/clk-mt8195-topckgen.c | 69 +++- drivers/clk/mediatek/clk-mt8195-vdec.c | 1 + drivers/clk/mediatek/clk-mt8195-vdo0.c | 20 +- drivers/clk/mediatek/clk-mt8195-vdo1.c | 20 +- drivers/clk/mediatek/clk-mt8195-venc.c | 1 + drivers/clk/mediatek/clk-mt8195-vpp0.c | 1 + drivers/clk/mediatek/clk-mt8195-vpp1.c | 1 + drivers/clk/mediatek/clk-mt8195-wpe.c | 1 + drivers/clk/mediatek/clk-mt8516.c | 3 +- drivers/clk/mediatek/clk-mtk.c | 304 +++++++++++++----- drivers/clk/mediatek/clk-mtk.h | 105 ++---- drivers/clk/mediatek/clk-mux.c | 89 ++++- drivers/clk/mediatek/clk-mux.h | 17 +- drivers/clk/mediatek/clk-pll.c | 100 +++++- drivers/clk/mediatek/clk-pll.h | 57 ++++ drivers/clk/mediatek/reset.c | 3 +- 45 files changed, 903 insertions(+), 292 deletions(-) create mode 100644 drivers/clk/mediatek/clk-pll.h