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

[Yu-Chun Lin [林祐君]]

Hi Brian,

> Hi Cheng-Yu,
> 
> On Thu, Apr 02, 2026 at 03:39:51PM +0800, Yu-Chun Lin wrote:
> > From: Cheng-Yu Lee <cylee12@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > Co-developed-by: Yu-Chun Lin <eleanor.lin@xxxxxxxxxxx>
> > Signed-off-by: Yu-Chun Lin <eleanor.lin@xxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxx>  */
> > +
> > +#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?
> 

Since aligning with the open parenthesis would exceed the 100-character limit,
I will use an extra tab for the continuation line.

Also, I have increased the delay_us parameter to 1 to avoid constant retries.

The code will look like this:

	return regmap_read_poll_timeout_atomic(clkp->clkr.regmap, clkp->freq_ready_reg, pollval,
		(pollval & clkp->freq_ready_mask) == clkp->freq_ready_val, 1, TIMEOUT);

> > +}
> > +
> > +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?
> 

Returning true here is a conservative design choice. When reading the power
status fails, returning true prevents unconditionally turning off the clock.
> > +
> > +     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.
> 

Ack.

> > +}
> > +
> > +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.
> 

Ack.

> > +}
> > +

(snip)

> > 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.

Ack

> 
> > +
> > +     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?

I will drop them.

Best Regards,
Yu-Chun

> 
> Brian