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

[Stephen Boyd <sboyd@kernel.org>]

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?