Re: [PATCH v6 04/10] clk: realtek: Add support for phase locked loops (PLLs)

Brian Masney
2026-04-03T14:34:50Z

Hi Cheng-Yu,

On Thu, Apr 02, 2026 at 03:39:51PM +0800, Yu-Chun Lin wrote:
> From: Cheng-Yu Lee <cylee12@realtek.com>
>
> Provide a full set of PLL operations for programmable PLLs and a read-only
> variant for fixed or hardware-managed PLLs.
>
> Signed-off-by: Cheng-Yu Lee <cylee12@realtek.com>
> Co-developed-by: Yu-Chun Lin <eleanor.lin@realtek.com>
> Signed-off-by: Yu-Chun Lin <eleanor.lin@realtek.com>
> ---
> Changes in v6:
> - Add the headers used in c file to follow the "Include What You Use" principle.
> - Move to_clk_pll() from clk-pll.h to clk-pll.c to limit its scope.
> ---
> drivers/clk/realtek/Makefile | 2 +
> drivers/clk/realtek/clk-pll.c | 164 +++++++++++++++++++++++++++++++
> drivers/clk/realtek/clk-pll.h | 42 ++++++++
> drivers/clk/realtek/freq_table.c | 36 +++++++
> drivers/clk/realtek/freq_table.h | 21 ++++
> 5 files changed, 265 insertions(+)
> create mode 100644 drivers/clk/realtek/clk-pll.c
> create mode 100644 drivers/clk/realtek/clk-pll.h
> create mode 100644 drivers/clk/realtek/freq_table.c
> create mode 100644 drivers/clk/realtek/freq_table.h
>
> diff --git a/drivers/clk/realtek/Makefile b/drivers/clk/realtek/Makefile
> index 377ec776ee47..a89ad77993e9 100644
> --- a/drivers/clk/realtek/Makefile
> +++ b/drivers/clk/realtek/Makefile
> @@ -2,3 +2,5 @@
> obj-$(CONFIG_RTK_CLK_COMMON) += clk-rtk.o
>
> clk-rtk-y += common.o
> +clk-rtk-y += clk-pll.o
> +clk-rtk-y += freq_table.o
> diff --git a/drivers/clk/realtek/clk-pll.c b/drivers/clk/realtek/clk-pll.c
> new file mode 100644
> index 000000000000..44730b22a94c
> --- /dev/null
> +++ b/drivers/clk/realtek/clk-pll.c
> @@ -0,0 +1,164 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2024 Realtek Semiconductor Corporation
> + * Author: Cheng-Yu Lee <cylee12@realtek.com>
> + */
> +
> +#include <linux/regmap.h>
> +#include "clk-pll.h"
> +
> +#define TIMEOUT 2000
> +
> +static inline struct clk_pll *to_clk_pll(struct clk_hw *hw)
> +{
> + struct clk_regmap *clkr = to_clk_regmap(hw);
> +
> + return container_of(clkr, struct clk_pll, clkr);
> +}
> +
> +static int wait_freq_ready(struct clk_pll *clkp)
> +{
> + u32 pollval;
> +
> + if (!clkp->freq_ready_valid)
> + return 0;
> +
> + return regmap_read_poll_timeout_atomic(clkp->clkr.regmap, clkp->freq_ready_reg, pollval,
> + (pollval & clkp->freq_ready_mask)
> + == clkp->freq_ready_val, 0, TIMEOUT);

I would put the "(pollval & clkp->freq_ready_mask) == clkp->freq_ready_val" on
the same line to improve readability. You can go out to 100 characters.

Also should the delay be greater than 0 to avoid tons of constant
retries?

> +}
> +
> +static bool is_power_on(struct clk_pll *clkp)
> +{
> + u32 val;
> +
> + if (!clkp->power_reg)
> + return true;
> +
> + if (regmap_read(clkp->clkr.regmap, clkp->power_reg, &val))
> + return true;

Is the intention if there is an error, then it marks it as success?

> +
> + return (val & clkp->power_mask) == clkp->power_val_on;
> +}
> +
> +static void clk_pll_disable(struct clk_hw *hw)
> +{
> + struct clk_pll *clkp = to_clk_pll(hw);
> +
> + if (!clkp->seq_power_off)
> + return;
> +
> + regmap_multi_reg_write(clkp->clkr.regmap, clkp->seq_power_off,
> + clkp->num_seq_power_off);
> +}
> +
> +static int clk_pll_is_enabled(struct clk_hw *hw)
> +{
> + struct clk_pll *clkp = to_clk_pll(hw);
> +
> + return is_power_on(clkp);
> +}
> +
> +static int clk_pll_determine_rate(struct clk_hw *hw,
> + struct clk_rate_request *req)
> +{
> + struct clk_pll *clkp = to_clk_pll(hw);
> + const struct freq_table *ftblv = NULL;
> +
> + ftblv = ftbl_find_by_rate(clkp->freq_tbl, req->rate);
> + if (!ftblv)
> + return -EINVAL;
> +
> + req->rate = ftblv->rate;
> + return 0;

Add newline before return.

> +}
> +
> +static unsigned long clk_pll_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct clk_pll *clkp = to_clk_pll(hw);
> + const struct freq_table *fv;
> + u32 freq_val;
> +
> + if (regmap_read(clkp->clkr.regmap, clkp->freq_reg, &freq_val))
> + return 0;
> +
> + freq_val &= clkp->freq_mask;
> +
> + fv = ftbl_find_by_val_with_mask(clkp->freq_tbl, clkp->freq_mask,
> + freq_val);
> + return fv ? fv->rate : 0;

Add newline before return.

> +}
> +
> +static int clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct clk_pll *clkp = to_clk_pll(hw);
> + const struct freq_table *fv;
> + int ret;
> +
> + fv = ftbl_find_by_rate(clkp->freq_tbl, rate);
> + if (!fv || fv->rate != rate)
> + return -EINVAL;
> +
> + if (clkp->seq_pre_set_freq) {
> + ret = regmap_multi_reg_write(clkp->clkr.regmap, clkp->seq_pre_set_freq,
> + clkp->num_seq_pre_set_freq);
> + if (ret)
> + return ret;
> + }
> +
> + ret = regmap_update_bits(clkp->clkr.regmap, clkp->freq_reg,
> + clkp->freq_mask, fv->val);
> + if (ret)
> + return ret;
> +
> + if (clkp->seq_post_set_freq) {
> + ret = regmap_multi_reg_write(clkp->clkr.regmap, clkp->seq_post_set_freq,
> + clkp->num_seq_post_set_freq);
> + if (ret)
> + return ret;
> + }
> +
> + if (is_power_on(clkp)) {
> + ret = wait_freq_ready(clkp);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int clk_pll_enable(struct clk_hw *hw)
> +{
> + struct clk_pll *clkp = to_clk_pll(hw);
> + int ret;
> +
> + if (!clkp->seq_power_on)
> + return 0;
> +
> + if (is_power_on(clkp))
> + return 0;
> +
> + ret = regmap_multi_reg_write(clkp->clkr.regmap, clkp->seq_power_on,
> + clkp->num_seq_power_on);
> + if (ret)
> + return ret;
> +
> + return wait_freq_ready(clkp);
> +}
> +
> +const struct clk_ops rtk_clk_pll_ops = {
> + .enable = clk_pll_enable,
> + .disable = clk_pll_disable,
> + .is_enabled = clk_pll_is_enabled,
> + .recalc_rate = clk_pll_recalc_rate,
> + .determine_rate = clk_pll_determine_rate,
> + .set_rate = clk_pll_set_rate,
> +};
> +EXPORT_SYMBOL_NS_GPL(rtk_clk_pll_ops, "REALTEK_CLK");
> +
> +const struct clk_ops rtk_clk_pll_ro_ops = {
> + .recalc_rate = clk_pll_recalc_rate,
> +};
> +EXPORT_SYMBOL_NS_GPL(rtk_clk_pll_ro_ops, "REALTEK_CLK");
> diff --git a/drivers/clk/realtek/clk-pll.h b/drivers/clk/realtek/clk-pll.h
> new file mode 100644
> index 000000000000..00884585a242
> --- /dev/null
> +++ b/drivers/clk/realtek/clk-pll.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2017-2019 Realtek Semiconductor Corporation
> + * Author: Cheng-Yu Lee <cylee12@realtek.com>
> + */
> +
> +#ifndef __CLK_REALTEK_CLK_PLL_H
> +#define __CLK_REALTEK_CLK_PLL_H
> +
> +#include "common.h"
> +#include "freq_table.h"
> +
> +struct reg_sequence;
> +
> +struct clk_pll {
> + struct clk_regmap clkr;
> + const struct reg_sequence *seq_power_on;
> + u32 num_seq_power_on;
> + const struct reg_sequence *seq_power_off;
> + u32 num_seq_power_off;
> + const struct reg_sequence *seq_pre_set_freq;
> + u32 num_seq_pre_set_freq;
> + const struct reg_sequence *seq_post_set_freq;
> + u32 num_seq_post_set_freq;
> + const struct freq_table *freq_tbl;
> + u32 freq_reg;
> + u32 freq_mask;
> + u32 freq_ready_valid;
> + u32 freq_ready_mask;
> + u32 freq_ready_reg;
> + u32 freq_ready_val;
> + u32 power_reg;
> + u32 power_mask;
> + u32 power_val_on;
> +};
> +
> +#define __clk_pll_hw(_ptr) __clk_regmap_hw(&(_ptr)->clkr)
> +
> +extern const struct clk_ops rtk_clk_pll_ops;
> +extern const struct clk_ops rtk_clk_pll_ro_ops;
> +
> +#endif /* __CLK_REALTEK_CLK_PLL_H */
> diff --git a/drivers/clk/realtek/freq_table.c b/drivers/clk/realtek/freq_table.c
> new file mode 100644
> index 000000000000..272a10e75a54
> --- /dev/null
> +++ b/drivers/clk/realtek/freq_table.c
> @@ -0,0 +1,36 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/bitops.h>
> +#include "freq_table.h"
> +
> +const struct freq_table *ftbl_find_by_rate(const struct freq_table *ftbl,
> + unsigned long rate)
> +{
> + unsigned long best_rate = 0;
> + const struct freq_table *best = NULL;

Put variables in reverse Christmas tree order.

> +
> + for (; !IS_FREQ_TABLE_END(ftbl); ftbl++) {
> + if (ftbl->rate == rate)
> + return ftbl;
> +
> + if (ftbl->rate > rate)
> + continue;
> +
> + if (ftbl->rate > best_rate) {
> + best_rate = ftbl->rate;
> + best = ftbl;
> + }
> + }
> +
> + return best;
> +}
> +
> +const struct freq_table *
> +ftbl_find_by_val_with_mask(const struct freq_table *ftbl, u32 mask, u32 value)
> +{
> + for (; !IS_FREQ_TABLE_END(ftbl); ftbl++) {
> + if ((ftbl->val & mask) == (value & mask))
> + return ftbl;
> + }
> + return NULL;
> +};
> diff --git a/drivers/clk/realtek/freq_table.h b/drivers/clk/realtek/freq_table.h
> new file mode 100644
> index 000000000000..6d9116651105
> --- /dev/null
> +++ b/drivers/clk/realtek/freq_table.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +struct freq_table {
> + u32 val;
> + unsigned long rate;
> +};
> +
> +/* ofs check */
> +#define CLK_OFS_INVALID -1
> +#define CLK_OFS_IS_VALID(_ofs) ((_ofs) != CLK_OFS_INVALID)

Is this used anywhere?

Brian

> +
> +#define FREQ_TABLE_END \
> + { \
> + .rate = 0 \
> + }
> +#define IS_FREQ_TABLE_END(_f) ((_f)->rate == 0)
> +
> +const struct freq_table *ftbl_find_by_rate(const struct freq_table *ftbl,
> + unsigned long rate);
> +const struct freq_table *
> +ftbl_find_by_val_with_mask(const struct freq_table *ftbl, u32 mask, u32 value);
> --
> 2.34.1
>