| Message ID | 20241203134137.2114847-2-m.wilczynski@samsung.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v1,01/14] clk: thead: Refactor TH1520 clock driver to share common code | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/vmtest-fixes-PR | fail | merge-conflict |
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? > 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@vivo.com> > */ > > -#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@kernel.org> > + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd. > + * Authors: Yangtao Li <frank.li@vivo.com> > + */ > + > +#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@kernel.org> > + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd. > + * Authors: Yangtao Li <frank.li@vivo.com> > + * > + * 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?
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@vivo.com> >> */ >> >> -#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@kernel.org> >> + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd. >> + * Authors: Yangtao Li <frank.li@vivo.com> >> + */ >> + >> +#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@kernel.org> >> + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd. >> + * Authors: Yangtao Li <frank.li@vivo.com> >> + * >> + * 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ł >
On 04/12/2024 14:54, Michal Wilczynski wrote: > > > 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 There is no such rule, no convention even. But there is a rule and convention of re-using drivers. > convention of having one driver per subsystem, similar to the You have here two drivers per subsystem, so even if there was such a rule, you just broke it. > 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@vivo.com> >>> */ >>> >>> -#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> >> ... >>> +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. It anyway in your original code cannot be exported. If you want to use syscon, then use syscon, not exporting regmaps manually. Best regards, Krzysztof
diff --git a/MAINTAINERS b/MAINTAINERS index 1e930c7a58b1..7c85abf1dd1e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -20188,7 +20188,7 @@ F: Documentation/devicetree/bindings/mailbox/thead,th1520-mbox.yaml F: Documentation/devicetree/bindings/net/thead,th1520-gmac.yaml F: Documentation/devicetree/bindings/pinctrl/thead,th1520-pinctrl.yaml F: arch/riscv/boot/dts/thead/ -F: drivers/clk/thead/clk-th1520-ap.c +F: drivers/clk/thead/ F: drivers/mailbox/mailbox-th1520.c F: drivers/net/ethernet/stmicro/stmmac/dwmac-thead.c F: drivers/pinctrl/pinctrl-th1520.c 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 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@vivo.com> */ -#include <dt-bindings/clock/thead,th1520-clk-ap.h> -#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> - -#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) - -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@kernel.org> + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd. + * Authors: Yangtao Li <frank.li@vivo.com> + */ + +#include "clk-th1520.h" + +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 = { + .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@kernel.org> + * Copyright (C) 2023 Vivo Communication Technology Co. Ltd. + * Authors: Yangtao Li <frank.li@vivo.com> + * + * 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> +#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> + +#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); + + 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; + +#endif /* CLK_TH1520_H */
The T-Head TH1520 SoC includes various clocks for different subsystems like Application Processor (AP) and Video Output (VO) [1]. Currently, the clock driver only implements AP clocks. Since the programming interface for these clocks is identical across subsystems, refactor the code to move common functions into clk-th1520.c. This prepares the driver to support VO clocks by reducing code duplication and improving maintainability. No functional changes are introduced with this refactoring. Link: https://openbeagle.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH1520%20System%20User%20Manual.pdf [1] Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com> --- MAINTAINERS | 2 +- drivers/clk/thead/Makefile | 2 +- drivers/clk/thead/clk-th1520-ap.c | 301 +----------------------------- drivers/clk/thead/clk-th1520.c | 188 +++++++++++++++++++ drivers/clk/thead/clk-th1520.h | 134 +++++++++++++ 5 files changed, 326 insertions(+), 301 deletions(-) create mode 100644 drivers/clk/thead/clk-th1520.c create mode 100644 drivers/clk/thead/clk-th1520.h