On 12/3/24 20:56, Stephen Boyd wrote: > Quoting Michal Wilczynski (2024-12-03 05:41:24) >> diff --git a/drivers/clk/thead/Makefile b/drivers/clk/thead/Makefile >> index 7ee0bec1f251..d7cf88390b69 100644 >> --- a/drivers/clk/thead/Makefile >> +++ b/drivers/clk/thead/Makefile >> @@ -1,2 +1,2 @@ >> # SPDX-License-Identifier: GPL-2.0 >> -obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520-ap.o >> +obj-$(CONFIG_CLK_THEAD_TH1520_AP) += clk-th1520.o clk-th1520-ap.o > > Can the -ap driver be extended instead? Or are the clks in a different > IO region? The Video Output (VO) clocks reside in a different address space as defined in the T-HEAD manual 4.4.1 [1]. Therefore, creating a separate driver made sense to maintain clarity and adhere to the existing convention of having one driver per subsystem, similar to the AP-specific driver. [1] - https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf > >> diff --git a/drivers/clk/thead/clk-th1520-ap.c b/drivers/clk/thead/clk-th1520-ap.c >> index 17e32ae08720..a6015805b859 100644 >> --- a/drivers/clk/thead/clk-th1520-ap.c >> +++ b/drivers/clk/thead/clk-th1520-ap.c >> @@ -5,297 +5,9 @@ >> * Authors: Yangtao Li <frank.li@xxxxxxxx> >> */ >> >> -#include <dt-bindings/clock/thead,th1520-clk-ap.h> > > Presumably this should stay here. > >> -#include <linux/bitfield.h> >> -#include <linux/clk-provider.h> >> -#include <linux/device.h> >> -#include <linux/module.h> >> -#include <linux/platform_device.h> >> -#include <linux/regmap.h> > > These should all stay here as well. > >> - >> -#define TH1520_PLL_POSTDIV2 GENMASK(26, 24) >> -#define TH1520_PLL_POSTDIV1 GENMASK(22, 20) >> -#define TH1520_PLL_FBDIV GENMASK(19, 8) >> -#define TH1520_PLL_REFDIV GENMASK(5, 0) >> -#define TH1520_PLL_BYPASS BIT(30) >> -#define TH1520_PLL_DSMPD BIT(24) >> -#define TH1520_PLL_FRAC GENMASK(23, 0) >> -#define TH1520_PLL_FRAC_BITS 24 >> - >> -struct ccu_internal { >> - u8 shift; >> - u8 width; >> -}; >> - >> -struct ccu_div_internal { >> - u8 shift; >> - u8 width; >> - u32 flags; >> -}; >> - >> -struct ccu_common { >> - int clkid; >> - struct regmap *map; >> - u16 cfg0; >> - u16 cfg1; >> - struct clk_hw hw; >> -}; >> - >> -struct ccu_mux { >> - struct ccu_internal mux; >> - struct ccu_common common; >> -}; >> - >> -struct ccu_gate { >> - u32 enable; >> - struct ccu_common common; >> -}; >> - >> -struct ccu_div { >> - u32 enable; >> - struct ccu_div_internal div; >> - struct ccu_internal mux; >> - struct ccu_common common; >> -}; >> - >> -struct ccu_pll { >> - struct ccu_common common; >> -}; >> - >> -#define TH_CCU_ARG(_shift, _width) \ >> - { \ >> - .shift = _shift, \ >> - .width = _width, \ >> - } >> - >> -#define TH_CCU_DIV_FLAGS(_shift, _width, _flags) \ >> - { \ >> - .shift = _shift, \ >> - .width = _width, \ >> - .flags = _flags, \ >> - } >> - >> -#define CCU_GATE(_clkid, _struct, _name, _parent, _reg, _gate, _flags) \ >> - struct ccu_gate _struct = { \ >> - .enable = _gate, \ >> - .common = { \ >> - .clkid = _clkid, \ >> - .cfg0 = _reg, \ >> - .hw.init = CLK_HW_INIT_PARENTS_DATA( \ >> - _name, \ >> - _parent, \ >> - &clk_gate_ops, \ >> - _flags), \ >> - } \ >> - } >> - >> -static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw) >> -{ >> - return container_of(hw, struct ccu_common, hw); >> -} >> - >> -static inline struct ccu_mux *hw_to_ccu_mux(struct clk_hw *hw) >> -{ >> - struct ccu_common *common = hw_to_ccu_common(hw); >> - >> - return container_of(common, struct ccu_mux, common); >> -} >> - >> -static inline struct ccu_pll *hw_to_ccu_pll(struct clk_hw *hw) >> -{ >> - struct ccu_common *common = hw_to_ccu_common(hw); >> +#include "clk-th1520.h" >> >> - return container_of(common, struct ccu_pll, common); >> -} >> - >> -static inline struct ccu_div *hw_to_ccu_div(struct clk_hw *hw) >> -{ >> - struct ccu_common *common = hw_to_ccu_common(hw); >> - >> - return container_of(common, struct ccu_div, common); >> -} >> - >> -static inline struct ccu_gate *hw_to_ccu_gate(struct clk_hw *hw) >> -{ >> - struct ccu_common *common = hw_to_ccu_common(hw); >> - >> - return container_of(common, struct ccu_gate, common); >> -} >> - >> -static u8 ccu_get_parent_helper(struct ccu_common *common, >> - struct ccu_internal *mux) >> -{ >> - unsigned int val; >> - u8 parent; >> - >> - regmap_read(common->map, common->cfg0, &val); >> - parent = val >> mux->shift; >> - parent &= GENMASK(mux->width - 1, 0); >> - >> - return parent; >> -} >> - >> -static int ccu_set_parent_helper(struct ccu_common *common, >> - struct ccu_internal *mux, >> - u8 index) >> -{ >> - return regmap_update_bits(common->map, common->cfg0, >> - GENMASK(mux->width - 1, 0) << mux->shift, >> - index << mux->shift); >> -} >> - >> -static void ccu_disable_helper(struct ccu_common *common, u32 gate) >> -{ >> - if (!gate) >> - return; >> - regmap_update_bits(common->map, common->cfg0, >> - gate, ~gate); >> -} >> - >> -static int ccu_enable_helper(struct ccu_common *common, u32 gate) >> -{ >> - unsigned int val; >> - int ret; >> - >> - if (!gate) >> - return 0; >> - >> - ret = regmap_update_bits(common->map, common->cfg0, gate, gate); >> - regmap_read(common->map, common->cfg0, &val); >> - return ret; >> -} >> - >> -static int ccu_is_enabled_helper(struct ccu_common *common, u32 gate) >> -{ >> - unsigned int val; >> - >> - if (!gate) >> - return true; >> - >> - regmap_read(common->map, common->cfg0, &val); >> - return val & gate; >> -} >> - >> -static unsigned long ccu_div_recalc_rate(struct clk_hw *hw, >> - unsigned long parent_rate) >> -{ >> - struct ccu_div *cd = hw_to_ccu_div(hw); >> - unsigned long rate; >> - unsigned int val; >> - >> - regmap_read(cd->common.map, cd->common.cfg0, &val); >> - val = val >> cd->div.shift; >> - val &= GENMASK(cd->div.width - 1, 0); >> - rate = divider_recalc_rate(hw, parent_rate, val, NULL, >> - cd->div.flags, cd->div.width); >> - >> - return rate; >> -} >> - >> -static u8 ccu_div_get_parent(struct clk_hw *hw) >> -{ >> - struct ccu_div *cd = hw_to_ccu_div(hw); >> - >> - return ccu_get_parent_helper(&cd->common, &cd->mux); >> -} >> - >> -static int ccu_div_set_parent(struct clk_hw *hw, u8 index) >> -{ >> - struct ccu_div *cd = hw_to_ccu_div(hw); >> - >> - return ccu_set_parent_helper(&cd->common, &cd->mux, index); >> -} >> - >> -static void ccu_div_disable(struct clk_hw *hw) >> -{ >> - struct ccu_div *cd = hw_to_ccu_div(hw); >> - >> - ccu_disable_helper(&cd->common, cd->enable); >> -} >> - >> -static int ccu_div_enable(struct clk_hw *hw) >> -{ >> - struct ccu_div *cd = hw_to_ccu_div(hw); >> - >> - return ccu_enable_helper(&cd->common, cd->enable); >> -} >> - >> -static int ccu_div_is_enabled(struct clk_hw *hw) >> -{ >> - struct ccu_div *cd = hw_to_ccu_div(hw); >> - >> - return ccu_is_enabled_helper(&cd->common, cd->enable); >> -} >> - >> -static const struct clk_ops ccu_div_ops = { >> - .disable = ccu_div_disable, >> - .enable = ccu_div_enable, >> - .is_enabled = ccu_div_is_enabled, >> - .get_parent = ccu_div_get_parent, >> - .set_parent = ccu_div_set_parent, >> - .recalc_rate = ccu_div_recalc_rate, >> - .determine_rate = clk_hw_determine_rate_no_reparent, >> -}; >> - >> -static unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw, >> - unsigned long parent_rate) >> -{ >> - struct ccu_pll *pll = hw_to_ccu_pll(hw); >> - unsigned long div, mul, frac; >> - unsigned int cfg0, cfg1; >> - u64 rate = parent_rate; >> - >> - regmap_read(pll->common.map, pll->common.cfg0, &cfg0); >> - regmap_read(pll->common.map, pll->common.cfg1, &cfg1); >> - >> - mul = FIELD_GET(TH1520_PLL_FBDIV, cfg0); >> - div = FIELD_GET(TH1520_PLL_REFDIV, cfg0); >> - if (!(cfg1 & TH1520_PLL_DSMPD)) { >> - mul <<= TH1520_PLL_FRAC_BITS; >> - frac = FIELD_GET(TH1520_PLL_FRAC, cfg1); >> - mul += frac; >> - div <<= TH1520_PLL_FRAC_BITS; >> - } >> - rate = parent_rate * mul; >> - rate = rate / div; >> - return rate; >> -} >> - >> -static unsigned long th1520_pll_postdiv_recalc_rate(struct clk_hw *hw, >> - unsigned long parent_rate) >> -{ >> - struct ccu_pll *pll = hw_to_ccu_pll(hw); >> - unsigned long div, rate = parent_rate; >> - unsigned int cfg0, cfg1; >> - >> - regmap_read(pll->common.map, pll->common.cfg0, &cfg0); >> - regmap_read(pll->common.map, pll->common.cfg1, &cfg1); >> - >> - if (cfg1 & TH1520_PLL_BYPASS) >> - return rate; >> - >> - div = FIELD_GET(TH1520_PLL_POSTDIV1, cfg0) * >> - FIELD_GET(TH1520_PLL_POSTDIV2, cfg0); >> - >> - rate = rate / div; >> - >> - return rate; >> -} >> - >> -static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw, >> - unsigned long parent_rate) >> -{ >> - unsigned long rate = parent_rate; >> - >> - rate = th1520_pll_vco_recalc_rate(hw, rate); >> - rate = th1520_pll_postdiv_recalc_rate(hw, rate); >> - >> - return rate; >> -} >> - >> -static const struct clk_ops clk_pll_ops = { >> - .recalc_rate = ccu_pll_recalc_rate, >> -}; >> +#define NR_CLKS (CLK_UART_SCLK + 1) >> >> static const struct clk_parent_data osc_24m_clk[] = { >> { .index = 0 } >> @@ -956,15 +668,6 @@ static struct ccu_common *th1520_gate_clks[] = { >> &sram3_clk.common, >> }; >> >> -#define NR_CLKS (CLK_UART_SCLK + 1) > > Why did this move? > >> - >> -static const struct regmap_config th1520_clk_regmap_config = { >> - .reg_bits = 32, >> - .val_bits = 32, >> - .reg_stride = 4, >> - .fast_io = true, >> -}; >> - >> static int th1520_clk_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> diff --git a/drivers/clk/thead/clk-th1520.c b/drivers/clk/thead/clk-th1520.c >> new file mode 100644 >> index 000000000000..e2bfe56de9af >> --- /dev/null >> +++ b/drivers/clk/thead/clk-th1520.c >> @@ -0,0 +1,188 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (C) 2023 Jisheng Zhang <jszhang@xxxxxxxxxx> >> + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd. >> + * Authors: Yangtao Li <frank.li@xxxxxxxx> >> + */ >> + >> +#include "clk-th1520.h" > > Explicitly include linux headers here, don't just put them all into a > header file. It helps us easily see what C files are using different > parts of the kernel infrastructure. > >> + >> +static u8 ccu_get_parent_helper(struct ccu_common *common, >> + struct ccu_internal *mux) >> +{ >> + unsigned int val; >> + u8 parent; >> + >> + regmap_read(common->map, common->cfg0, &val); >> + parent = val >> mux->shift; >> + parent &= GENMASK(mux->width - 1, 0); >> + >> + return parent; >> +} >> + >> +static int ccu_set_parent_helper(struct ccu_common *common, >> + struct ccu_internal *mux, u8 index) >> +{ >> + return regmap_update_bits(common->map, common->cfg0, >> + GENMASK(mux->width - 1, 0) << mux->shift, >> + index << mux->shift); >> +} >> + >> +static void ccu_disable_helper(struct ccu_common *common, u32 gate) >> +{ >> + if (!gate) >> + return; >> + regmap_update_bits(common->map, common->cfg0, gate, ~gate); >> +} >> + >> +static int ccu_enable_helper(struct ccu_common *common, u32 gate) >> +{ >> + unsigned int val; >> + int ret; >> + >> + if (!gate) >> + return 0; >> + >> + ret = regmap_update_bits(common->map, common->cfg0, gate, gate); >> + regmap_read(common->map, common->cfg0, &val); >> + return ret; >> +} >> + >> +static int ccu_is_enabled_helper(struct ccu_common *common, u32 gate) >> +{ >> + unsigned int val; >> + >> + if (!gate) >> + return true; >> + >> + regmap_read(common->map, common->cfg0, &val); >> + return val & gate; >> +} >> + >> +static unsigned long ccu_div_recalc_rate(struct clk_hw *hw, >> + unsigned long parent_rate) >> +{ >> + struct ccu_div *cd = hw_to_ccu_div(hw); >> + unsigned long rate; >> + unsigned int val; >> + >> + regmap_read(cd->common.map, cd->common.cfg0, &val); >> + val = val >> cd->div.shift; >> + val &= GENMASK(cd->div.width - 1, 0); >> + rate = divider_recalc_rate(hw, parent_rate, val, NULL, cd->div.flags, >> + cd->div.width); >> + >> + return rate; >> +} >> + >> +static u8 ccu_div_get_parent(struct clk_hw *hw) >> +{ >> + struct ccu_div *cd = hw_to_ccu_div(hw); >> + >> + return ccu_get_parent_helper(&cd->common, &cd->mux); >> +} >> + >> +static int ccu_div_set_parent(struct clk_hw *hw, u8 index) >> +{ >> + struct ccu_div *cd = hw_to_ccu_div(hw); >> + >> + return ccu_set_parent_helper(&cd->common, &cd->mux, index); >> +} >> + >> +static void ccu_div_disable(struct clk_hw *hw) >> +{ >> + struct ccu_div *cd = hw_to_ccu_div(hw); >> + >> + ccu_disable_helper(&cd->common, cd->enable); >> +} >> + >> +static int ccu_div_enable(struct clk_hw *hw) >> +{ >> + struct ccu_div *cd = hw_to_ccu_div(hw); >> + >> + return ccu_enable_helper(&cd->common, cd->enable); >> +} >> + >> +static int ccu_div_is_enabled(struct clk_hw *hw) >> +{ >> + struct ccu_div *cd = hw_to_ccu_div(hw); >> + >> + return ccu_is_enabled_helper(&cd->common, cd->enable); >> +} >> + >> +const struct clk_ops ccu_div_ops = { >> + .disable = ccu_div_disable, >> + .enable = ccu_div_enable, >> + .is_enabled = ccu_div_is_enabled, >> + .get_parent = ccu_div_get_parent, >> + .set_parent = ccu_div_set_parent, >> + .recalc_rate = ccu_div_recalc_rate, >> + .determine_rate = clk_hw_determine_rate_no_reparent, >> +}; >> + >> +static unsigned long th1520_pll_vco_recalc_rate(struct clk_hw *hw, >> + unsigned long parent_rate) >> +{ >> + struct ccu_pll *pll = hw_to_ccu_pll(hw); >> + unsigned long div, mul, frac; >> + unsigned int cfg0, cfg1; >> + u64 rate = parent_rate; >> + >> + regmap_read(pll->common.map, pll->common.cfg0, &cfg0); >> + regmap_read(pll->common.map, pll->common.cfg1, &cfg1); >> + >> + mul = FIELD_GET(TH1520_PLL_FBDIV, cfg0); >> + div = FIELD_GET(TH1520_PLL_REFDIV, cfg0); >> + if (!(cfg1 & TH1520_PLL_DSMPD)) { >> + mul <<= TH1520_PLL_FRAC_BITS; >> + frac = FIELD_GET(TH1520_PLL_FRAC, cfg1); >> + mul += frac; >> + div <<= TH1520_PLL_FRAC_BITS; >> + } >> + rate = parent_rate * mul; >> + rate = rate / div; >> + return rate; >> +} >> + >> +static unsigned long th1520_pll_postdiv_recalc_rate(struct clk_hw *hw, >> + unsigned long parent_rate) >> +{ >> + struct ccu_pll *pll = hw_to_ccu_pll(hw); >> + unsigned long div, rate = parent_rate; >> + unsigned int cfg0, cfg1; >> + >> + regmap_read(pll->common.map, pll->common.cfg0, &cfg0); >> + regmap_read(pll->common.map, pll->common.cfg1, &cfg1); >> + >> + if (cfg1 & TH1520_PLL_BYPASS) >> + return rate; >> + >> + div = FIELD_GET(TH1520_PLL_POSTDIV1, cfg0) * >> + FIELD_GET(TH1520_PLL_POSTDIV2, cfg0); >> + >> + rate = rate / div; >> + >> + return rate; >> +} >> + >> +static unsigned long ccu_pll_recalc_rate(struct clk_hw *hw, >> + unsigned long parent_rate) >> +{ >> + unsigned long rate = parent_rate; >> + >> + rate = th1520_pll_vco_recalc_rate(hw, rate); >> + rate = th1520_pll_postdiv_recalc_rate(hw, rate); >> + >> + return rate; >> +} >> + >> +const struct clk_ops clk_pll_ops = { >> + .recalc_rate = ccu_pll_recalc_rate, >> +}; >> + >> +const struct regmap_config th1520_clk_regmap_config = { > > I don't get why this is moved. > >> + .reg_bits = 32, >> + .val_bits = 32, >> + .reg_stride = 4, >> + .fast_io = true, >> +}; >> diff --git a/drivers/clk/thead/clk-th1520.h b/drivers/clk/thead/clk-th1520.h >> new file mode 100644 >> index 000000000000..285d41e65008 >> --- /dev/null >> +++ b/drivers/clk/thead/clk-th1520.h >> @@ -0,0 +1,134 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (C) 2023 Jisheng Zhang <jszhang@xxxxxxxxxx> >> + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd. >> + * Authors: Yangtao Li <frank.li@xxxxxxxx> >> + * >> + * clk-th1520.h - Common definitions for T-HEAD TH1520 Clock Drivers >> + */ >> + >> +#ifndef CLK_TH1520_H >> +#define CLK_TH1520_H >> + >> +#include <dt-bindings/clock/thead,th1520-clk-ap.h> > > dt-bindings comes after linux includes, but this shouldn't be here > anyway. > >> +#include <linux/bitfield.h> >> +#include <linux/clk-provider.h> > > Seems we have to have this one for clk_hw. > >> +#include <linux/device.h> >> +#include <linux/module.h> >> +#include <linux/platform_device.h> > > I don't see these includes used here, so remove them. > >> +#include <linux/regmap.h> > > Forward declare regmap and drop the include > > struct regmap; > >> + >> +#define TH1520_PLL_POSTDIV2 GENMASK(26, 24) >> +#define TH1520_PLL_POSTDIV1 GENMASK(22, 20) >> +#define TH1520_PLL_FBDIV GENMASK(19, 8) >> +#define TH1520_PLL_REFDIV GENMASK(5, 0) >> +#define TH1520_PLL_BYPASS BIT(30) >> +#define TH1520_PLL_DSMPD BIT(24) >> +#define TH1520_PLL_FRAC GENMASK(23, 0) >> +#define TH1520_PLL_FRAC_BITS 24 > > Are these going to be used in multiple drivers? > >> + >> +struct ccu_internal { >> + u8 shift; >> + u8 width; >> +}; >> + >> +struct ccu_div_internal { >> + u8 shift; >> + u8 width; >> + u32 flags; >> +}; >> + >> +struct ccu_common { >> + int clkid; >> + struct regmap *map; >> + u16 cfg0; >> + u16 cfg1; >> + struct clk_hw hw; >> +}; >> + >> +struct ccu_mux { >> + struct ccu_internal mux; >> + struct ccu_common common; >> +}; >> + >> +struct ccu_gate { >> + u32 enable; >> + struct ccu_common common; >> +}; >> + >> +struct ccu_div { >> + u32 enable; >> + struct ccu_div_internal div; >> + struct ccu_internal mux; >> + struct ccu_common common; >> +}; >> + >> +struct ccu_pll { >> + struct ccu_common common; >> +}; >> + >> +#define TH_CCU_ARG(_shift, _width) \ >> + { \ >> + .shift = _shift, \ >> + .width = _width, \ >> + } >> + >> +#define TH_CCU_DIV_FLAGS(_shift, _width, _flags) \ >> + { \ >> + .shift = _shift, \ >> + .width = _width, \ >> + .flags = _flags, \ >> + } >> + >> +#define CCU_GATE(_clkid, _struct, _name, _parent, _reg, _gate, _flags) \ >> + struct ccu_gate _struct = { \ >> + .enable = _gate, \ >> + .common = { \ >> + .clkid = _clkid, \ >> + .cfg0 = _reg, \ >> + .hw.init = CLK_HW_INIT_PARENTS_DATA( \ >> + _name, \ >> + _parent, \ >> + &clk_gate_ops, \ >> + _flags), \ >> + } \ >> + } >> + >> +static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw) >> +{ >> + return container_of(hw, struct ccu_common, hw); >> +} >> + >> +static inline struct ccu_mux *hw_to_ccu_mux(struct clk_hw *hw) >> +{ >> + struct ccu_common *common = hw_to_ccu_common(hw); >> + >> + return container_of(common, struct ccu_mux, common); >> +} >> + >> +static inline struct ccu_pll *hw_to_ccu_pll(struct clk_hw *hw) >> +{ >> + struct ccu_common *common = hw_to_ccu_common(hw); >> + >> + return container_of(common, struct ccu_pll, common); >> +} >> + >> +static inline struct ccu_div *hw_to_ccu_div(struct clk_hw *hw) >> +{ >> + struct ccu_common *common = hw_to_ccu_common(hw); >> + >> + return container_of(common, struct ccu_div, common); >> +} >> + >> +static inline struct ccu_gate *hw_to_ccu_gate(struct clk_hw *hw) >> +{ >> + struct ccu_common *common = hw_to_ccu_common(hw); >> + >> + return container_of(common, struct ccu_gate, common); >> +} >> + >> +extern const struct clk_ops ccu_div_ops; >> +extern const struct clk_ops clk_pll_ops; >> +extern const struct regmap_config th1520_clk_regmap_config; > > Why is the regmap config exported? The regmap_config is exported to allow reuse across multiple drivers. Initially, I passed the clock VOSYS address space using the reg property and created the regmap from it, enabling other drivers to utilize the same configuration. Later, I switched to a regmap-based syscon approach but haven’t moved the regmap_config back to the AP driver. Based on Krzysztof's feedback, using the reg property to pass memory-mapped registers is preferred. If needed, I can create a separate regmap_config for the VO subsystem instead of reusing the existing one. I will also address the other points you mentioned in your review. Thank you for your feedback. Michał >
