Re: [PATCH v3 02/23] gcc-sdm845: Add rates to the GP clocks

[Stephen Boyd <sboyd@kernel.org>]

Quoting Dzmitry Sankouski (2024-08-12 08:16:06)
> diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
> index 30b19bd39d08..44b257481556 100644
> --- a/drivers/clk/qcom/clk-rcg2.c
> +++ b/drivers/clk/qcom/clk-rcg2.c
> @@ -32,6 +32,7 @@
>  
>  #define CFG_REG                        0x4
>  #define CFG_SRC_DIV_SHIFT      0
> +#define CFG_SRC_DIV_LENGTH     8
>  #define CFG_SRC_SEL_SHIFT      8
>  #define CFG_SRC_SEL_MASK       (0x7 << CFG_SRC_SEL_SHIFT)
>  #define CFG_MODE_SHIFT         12
> @@ -393,16 +394,103 @@ static int clk_rcg2_fm_determine_rate(struct clk_hw *hw,
>         return _freq_tbl_fm_determine_rate(hw, rcg->freq_multi_tbl, req);
>  }
>  
> -static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f,
> -                               u32 *_cfg)
> +static inline u64 find_hcf(u64 a, u64 b)
> +{
> +       while (a != 0 && b != 0) {
> +               if (a > b)
> +                       a %= b;
> +               else
> +                       b %= a;
> +       }
> +       return a + b;

Is this gcd()?

> +}
> +
> +static int clk_calc_mnd(u64 parent_rate, u64 rate, struct freq_tbl *f)
> +{
> +       u64 hcf;
> +       u64 hid_div = 1, n = 1;
> +       int i = 2, count = 0;
> +
> +       hcf = find_hcf(parent_rate, rate);
> +       u64 scaled_rate = rate / hcf;
> +       u64 scaled_parent_rate = parent_rate / hcf;
> +
> +       while (scaled_parent_rate > 1) {
> +               while (scaled_parent_rate % i == 0) {
> +                       scaled_parent_rate /= i;
> +                       if (count % 2 == 0)
> +                               hid_div *= i;
> +                       else
> +                               n *= i;
> +               }
> +               i++;
> +               count++;
> +       }
> +
> +       f->m = scaled_rate;
> +       f->n = n;
> +       f->pre_div = hid_div;
> +
> +       return 0;
> +}
> +
> +static int clk_rcg2_determine_gp_rate(struct clk_hw *hw,
> +                                  struct clk_rate_request *req)
> +{
> +       struct clk_rcg2 *rcg = to_clk_rcg2(hw);
> +       struct freq_tbl *f;
> +       int src = clk_rcg2_get_parent(hw);
> +       int mnd_max = BIT(rcg->mnd_width) - 1;
> +       int hid_max = BIT(rcg->hid_width) - 1;
> +       u64 parent_rate;
> +       int ret;
> +
> +       parent_rate = rcg->freq_tbl[src].freq;
> +       f = kcalloc(MAX_PERF_LEVEL + 1, sizeof(f), GFP_KERNEL);

When is this freed? Determine rate can be called many times. Is that
supposed to be sizeof(*f)? Why so many frequency entries?


> +
> +       if (!f)
> +               return 0;
> +
> +       ret = clk_calc_mnd(parent_rate, req->rate, f);
> +       if (ret)
> +               return 0;
> +
> +
> +       while (f->n - f->m >= mnd_max) {
> +               f->m = f->m >> 1;
> +               f->n = f->n >> 1;
> +       }
> +       while (f->pre_div >= hid_max) {
> +               f->pre_div = f->pre_div >> 1;
> +               f->m = f->m >> 1;
> +       }
> +
> +       req->rate = calc_rate(parent_rate, f->m, f->n, f->n, f->pre_div);
> +
> +       return 0;
> +}
> +
> +static int __clk_rcg2_configure_parent(struct clk_rcg2 *rcg, int src, u32 *_cfg)

u8 src? Good to keep types consistent.

>  {
> -       u32 cfg, mask, d_val, not2d_val, n_minus_m;
>         struct clk_hw *hw = &rcg->clkr.hw;
> -       int ret, index = qcom_find_src_index(hw, rcg->parent_map, f->src);
> +       u32 mask = CFG_SRC_SEL_MASK;
> +       int index = qcom_find_src_index(hw, rcg->parent_map, src);
>  
>         if (index < 0)
>                 return index;
>  
> +       *_cfg &= ~mask;
> +       *_cfg |= rcg->parent_map[index].cfg << CFG_SRC_SEL_SHIFT;
> +
> +       return 0;
> +}
> +
> +static int __clk_rcg2_configure_mnd(struct clk_rcg2 *rcg, const struct freq_tbl *f,
> +                               u32 *_cfg)
> +{
> +       u32 cfg, mask, d_val, not2d_val, n_minus_m;
> +       int ret;
> +
>         if (rcg->mnd_width && f->n) {
>                 mask = BIT(rcg->mnd_width) - 1;
>                 ret = regmap_update_bits(rcg->clkr.regmap,
> @@ -431,9 +519,8 @@ static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f,
>         }
>  
>         mask = BIT(rcg->hid_width) - 1;
> -       mask |= CFG_SRC_SEL_MASK | CFG_MODE_MASK | CFG_HW_CLK_CTRL_MASK;
> +       mask |= CFG_MODE_MASK | CFG_HW_CLK_CTRL_MASK;
>         cfg = f->pre_div << CFG_SRC_DIV_SHIFT;
> -       cfg |= rcg->parent_map[index].cfg << CFG_SRC_SEL_SHIFT;
>         if (rcg->mnd_width && f->n && (f->m != f->n))
>                 cfg |= CFG_MODE_DUAL_EDGE;
>         if (rcg->hw_clk_ctrl)
> @@ -445,6 +532,22 @@ static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f,
>         return 0;
>  }
>  
> +static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f,
> +                               u32 *_cfg)
> +{
> +       int ret;
> +
> +       ret = __clk_rcg2_configure_parent(rcg, f->src, _cfg);
> +       if (ret)
> +               return ret;
> +
> +       ret = __clk_rcg2_configure_mnd(rcg, f, _cfg);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
>  static int clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f)
>  {
>         u32 cfg;
> @@ -465,6 +568,26 @@ static int clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f)
>         return update_config(rcg);
>  }
>  
> +static int clk_rcg2_configure_gp(struct clk_rcg2 *rcg, const struct freq_tbl *f)
> +{
> +       u32 cfg;
> +       int ret;
> +
> +       ret = regmap_read(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), &cfg);
> +       if (ret)
> +               return ret;
> +
> +       ret = __clk_rcg2_configure_mnd(rcg, f, &cfg);
> +       if (ret)
> +               return ret;
> +
> +       ret = regmap_write(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg), cfg);
> +       if (ret)
> +               return ret;
> +
> +       return update_config(rcg);
> +}
> +
>  static int __clk_rcg2_set_rate(struct clk_hw *hw, unsigned long rate,
>                                enum freq_policy policy)
>  {
> @@ -518,6 +641,22 @@ static int clk_rcg2_set_rate(struct clk_hw *hw, unsigned long rate,
>         return __clk_rcg2_set_rate(hw, rate, CEIL);
>  }
>  
> +static int clk_rcg2_set_gp_rate(struct clk_hw *hw, unsigned long rate,
> +                           unsigned long parent_rate)
> +{
> +       struct clk_rcg2 *rcg = to_clk_rcg2(hw);
> +       struct freq_tbl *f;
> +
> +       f = kcalloc(MAX_PERF_LEVEL + 1, sizeof(*f), GFP_KERNEL);

When is this freed?

> +
> +       if (!f)
> +               return -ENOMEM;
> +
> +       clk_calc_mnd(parent_rate, rate, f);
> +
> +       return clk_rcg2_configure_gp(rcg, f);
> +}
> +
>  static int clk_rcg2_set_floor_rate(struct clk_hw *hw, unsigned long rate,
>                                    unsigned long parent_rate)
>  {
> @@ -645,6 +784,17 @@ const struct clk_ops clk_rcg2_ops = {
>  };
>  EXPORT_SYMBOL_GPL(clk_rcg2_ops);
>  
> +const struct clk_ops clk_rcg2_gp_ops = {
> +       .is_enabled = clk_rcg2_is_enabled,
> +       .get_parent = clk_rcg2_get_parent,
> +       .set_parent = clk_rcg2_set_parent,
> +       .determine_rate = clk_rcg2_determine_gp_rate,
> +       .set_rate = clk_rcg2_set_gp_rate,
> +       .get_duty_cycle = clk_rcg2_get_duty_cycle,
> +       .set_duty_cycle = clk_rcg2_set_duty_cycle,
> +};
> +EXPORT_SYMBOL_GPL(clk_rcg2_gp_ops);
> +
>  const struct clk_ops clk_rcg2_floor_ops = {
>         .is_enabled = clk_rcg2_is_enabled,
>         .get_parent = clk_rcg2_get_parent,
> diff --git a/drivers/pwm/pwm-clk.c b/drivers/pwm/pwm-clk.c
> index c19a482d7e28..1bfc7870e3aa 100644
> --- a/drivers/pwm/pwm-clk.c
> +++ b/drivers/pwm/pwm-clk.c
> @@ -25,6 +25,7 @@
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/clk.h>
> +#include <linux/clk/clk-conf.h>
>  #include <linux/pwm.h>
>  
>  struct pwm_clk_chip {
> @@ -87,6 +88,10 @@ static int pwm_clk_probe(struct platform_device *pdev)
>         struct pwm_clk_chip *pcchip;
>         int ret;
>  
> +       ret = of_clk_set_defaults(pdev->dev.of_node, false);
> +       if (ret < 0)
> +               return -EINVAL;
> +

What is this? Debug code?

>         chip = devm_pwmchip_alloc(&pdev->dev, 1, sizeof(*pcchip));
>         if (IS_ERR(chip))
>                 return PTR_ERR(chip);
> -- 
> 2.39.2
> 
>