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

[Brian Masney <bmasney@redhat.com>]

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
>