diff mbox series

[RFC,v1,01/14] clk: thead: Refactor TH1520 clock driver to share common code

Message ID 20241203134137.2114847-2-m.wilczynski@samsung.com
State New
Headers show
Series Enable drm/imagination BXM-4-64 Support for LicheePi 4A | expand

Commit Message

Michal Wilczynski Dec. 3, 2024, 1:41 p.m. UTC
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

Comments

Michal Wilczynski Dec. 4, 2024, 1:54 p.m. UTC | #1
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ł

>
Krzysztof Kozlowski Dec. 5, 2024, 7:31 a.m. UTC | #2
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 mbox series

Patch

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 */