[PATCH v1 03/13] clk: starfive: Add system-0 domain PLL clock driver
Changhuang Liang
changhuang.liang at starfivetech.com
Mon Apr 6 18:17:40 PDT 2026
Hi, Brian
Thanks for the review.
> Hi Changhuang,
>
> On Thu, Apr 02, 2026 at 10:49:35PM -0700, Changhuang Liang wrote:
> > Add system-0 domain PLL clock driver for StarFive JHB100 SoC.
> >
> > Signed-off-by: Changhuang Liang <changhuang.liang at starfivetech.com>
> > ---
> > drivers/clk/starfive/Kconfig | 8 +
> > drivers/clk/starfive/Makefile | 1 +
> > .../clk/starfive/clk-starfive-jhb100-pll.c | 498 ++++++++++++++++++
> > 3 files changed, 507 insertions(+)
> > create mode 100644 drivers/clk/starfive/clk-starfive-jhb100-pll.c
> >
> > diff --git a/drivers/clk/starfive/Kconfig
> > b/drivers/clk/starfive/Kconfig index c612f1ede7d7..cc712da68bd0 100644
> > --- a/drivers/clk/starfive/Kconfig
> > +++ b/drivers/clk/starfive/Kconfig
> > @@ -105,6 +105,14 @@ config CLK_STARFIVE_JHB100_PER3
> > Say yes here to support the peripheral-3 clock controller
> > on the StarFive JHB100 SoC.
> >
> > +config CLK_STARFIVE_JHB100_PLL
> > + bool "StarFive JHB100 PLL clock support"
> > + depends on ARCH_STARFIVE || COMPILE_TEST
> > + default ARCH_STARFIVE
> > + help
> > + Say yes here to support the PLL clock controller on the
> > + StarFive JHB100 SoC.
> > +
> > config CLK_STARFIVE_JHB100_SYS0
> > bool "StarFive JHB100 system-0 clock support"
> > depends on ARCH_STARFIVE || COMPILE_TEST diff --git
> > a/drivers/clk/starfive/Makefile b/drivers/clk/starfive/Makefile index
> > f00690f0cdad..547a8c170728 100644
> > --- a/drivers/clk/starfive/Makefile
> > +++ b/drivers/clk/starfive/Makefile
> > @@ -15,6 +15,7 @@ obj-$(CONFIG_CLK_STARFIVE_JHB100_PER0)
> += clk-starfive-jhb100-per0.o
> > obj-$(CONFIG_CLK_STARFIVE_JHB100_PER1) +=
> clk-starfive-jhb100-per1.o
> > obj-$(CONFIG_CLK_STARFIVE_JHB100_PER2) +=
> clk-starfive-jhb100-per2.o
> > obj-$(CONFIG_CLK_STARFIVE_JHB100_PER3) +=
> clk-starfive-jhb100-per3.o
> > +obj-$(CONFIG_CLK_STARFIVE_JHB100_PLL) +=
> clk-starfive-jhb100-pll.o
> > obj-$(CONFIG_CLK_STARFIVE_JHB100_SYS0) +=
> clk-starfive-jhb100-sys0.o
> > obj-$(CONFIG_CLK_STARFIVE_JHB100_SYS1) +=
> clk-starfive-jhb100-sys1.o
> > obj-$(CONFIG_CLK_STARFIVE_JHB100_SYS2) +=
> clk-starfive-jhb100-sys2.o
> > diff --git a/drivers/clk/starfive/clk-starfive-jhb100-pll.c
> > b/drivers/clk/starfive/clk-starfive-jhb100-pll.c
> > new file mode 100644
> > index 000000000000..1751a734ee83
> > --- /dev/null
> > +++ b/drivers/clk/starfive/clk-starfive-jhb100-pll.c
> > @@ -0,0 +1,498 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * StarFive JHB100 PLL Clock Generator Driver
> > + *
> > + * Copyright (C) 2024 StarFive Technology Co., Ltd.
> > + *
> > + * Author: Changhuang Liang <changhuang.liang at starfivetech.com>
> > + */
> > +
> > +#include <linux/bits.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +
> > +#include <dt-bindings/clock/starfive,jhb100-crg.h>
> > +
> > +/* this driver expects a 25MHz input frequency from the oscillator */
> > +#define JHB100_PLL_OSC_RATE 25000000UL
>
> You could include linux/units.h and then use: 25 * HZ_PER_MHZ
>
> > +
> > +/* System-0 domain PLL */
> > +#define JHB100_PLL2_OFFSET 0x00
> > +#define JHB100_PLL3_OFFSET 0x0c
> > +#define JHB100_PLL4_OFFSET 0x18
> > +#define JHB100_PLL5_OFFSET 0x24
> > +
> > +#define JHB100_PLL_CFG0_OFFSET 0x0
> > +#define JHB100_PLL_CFG1_OFFSET 0x4
> > +#define JHB100_PLL_CFG2_OFFSET 0x8
> > +
> > +#define JHB100_PLLX_CFG0(offset) ((offset) + JHB100_PLL_CFG0_OFFSET)
> > +/* fbdiv value should be 16 to 4095 */
> > +#define JHB100_PLL_FBDIV GENMASK(13, 2)
> > +#define JHB100_PLL_FBDIV_SHIFT 2
> > +#define JHB100_PLL_FOUTPOSTDIV_EN BIT(14)
> > +#define JHB100_PLL_FOUTPOSTDIV_EN_SHIFT 14
> > +#define JHB100_PLL_FOUTVCOP_EN BIT(16)
> > +#define JHB100_PLL_FOUTVCOP_EN_SHIFT 16
> > +
> > +#define JHB100_PLLX_CFG1(offset) ((offset) + JHB100_PLL_CFG1_OFFSET)
> > +/* frac value should be decimals multiplied by 2^24 */
> > +#define JHB100_PLL_FRAC GENMASK(23, 0)
> > +#define JHB100_PLL_FRAC_SHIFT 0
> > +#define JHB100_PLL_LOCK BIT(24)
> > +#define JHB100_PLL_LOCK_SHIFT 24
> > +
> > +#define JHB100_PLLX_CFG2(offset) ((offset) + JHB100_PLL_CFG2_OFFSET)
> > +#define JHB100_PLL_PD BIT(13)
> > +#define JHB100_PLL_PD_SHIFT 13
> > +#define JHB100_PLL_POSTDIV GENMASK(15, 14)
> > +#define JHB100_PLL_POSTDIV_SHIFT 14
> > +#define JHB100_PLL_REFDIV GENMASK(23, 18)
> > +#define JHB100_PLL_REFDIV_SHIFT 18
> > +
> > +#define JHB100_PLL_TIMEOUT_US 1000
> > +#define JHB100_PLL_INTERVAL_US 100
> > +
> > +struct jhb100_pll_preset {
> > + unsigned long freq;
> > + u32 frac; /* frac value should be decimals multiplied by 2^24
> */
> > + unsigned fbdiv : 12; /* fbdiv value should be 8 to 4095 */
> > + unsigned refdiv : 6;
> > + unsigned postdiv : 2;
> > + unsigned foutpostdiv_en : 1;
> > + unsigned foutvcop_en : 1;
> > +};
> > +
> > +struct jhb100_pll_info {
> > + char *name;
> > + const struct jhb100_pll_preset *presets;
> > + unsigned int npresets;
> > + unsigned long flag;
> > + u8 offset;
> > + bool continuous;
> > +};
> > +
> > +#define _JHB100_PLL(_idx, _name, _presets, _npresets, _offset, _flag,
> _cont) \
> > + [_idx] = { \
> > + .name = _name, \
> > + .offset = _offset, \
> > + .presets = _presets, \
> > + .npresets = _npresets, \
> > + .flag = _flag, \
> > + .continuous = _cont, \
> > + }
> > +
> > +#define JHB100_PLL(idx, name, presets, npresets, offset, cont)
> \
> > + _JHB100_PLL(idx, name, presets, npresets, offset, 0, cont)
> > +
> > +struct jhb100_pll_match_data {
> > + const struct jhb100_pll_info *pll_info;
> > + int num_pll;
> > +};
> > +
> > +struct jhb100_pll_data {
> > + struct clk_hw hw;
> > + unsigned int idx;
> > +};
> > +
> > +struct jhb100_pll_priv {
> > + struct device *dev;
> > + struct regmap *regmap;
> > + const struct jhb100_pll_match_data *match_data;
> > + struct jhb100_pll_data pll[];
> > +};
> > +
> > +struct jhb100_pll_regvals {
> > + u32 fbdiv;
> > + u32 frac;
> > + u32 postdiv;
> > + u32 refdiv;
> > + bool foutpostdiv_en;
> > + bool foutvcop_en;
> > +};
> > +
> > +static struct jhb100_pll_data *jhb100_pll_data_from(struct clk_hw
> > +*hw) {
> > + return container_of(hw, struct jhb100_pll_data, hw); }
> > +
> > +static struct jhb100_pll_priv *jhb100_pll_priv_from(struct
> > +jhb100_pll_data *pll) {
> > + return container_of(pll, struct jhb100_pll_priv, pll[pll->idx]); }
> > +
> > +static int jhb100_pll_enable(struct clk_hw *hw) {
> > + struct jhb100_pll_data *pll = jhb100_pll_data_from(hw);
> > + struct jhb100_pll_priv *priv = jhb100_pll_priv_from(pll);
> > + const struct jhb100_pll_info *info =
> > +&priv->match_data->pll_info[pll->idx];
> > +
> > + regmap_update_bits(priv->regmap, JHB100_PLLX_CFG2(info->offset),
> > + JHB100_PLL_PD, 0);
>
> Should the return value be checked here? Or just:
>
> return regumap_update_bits(...);
>
> > +
> > + return 0;
> > +}
> > +
> > +static void jhb100_pll_disable(struct clk_hw *hw) {
> > + struct jhb100_pll_data *pll = jhb100_pll_data_from(hw);
> > + struct jhb100_pll_priv *priv = jhb100_pll_priv_from(pll);
> > + const struct jhb100_pll_info *info =
> > +&priv->match_data->pll_info[pll->idx];
> > +
> > + regmap_update_bits(priv->regmap, JHB100_PLLX_CFG2(info->offset),
> > + JHB100_PLL_PD, BIT(JHB100_PLL_PD_SHIFT)); }
> > +
> > +static int jhb100_pll_is_enabled(struct clk_hw *hw) {
> > + struct jhb100_pll_data *pll = jhb100_pll_data_from(hw);
> > + struct jhb100_pll_priv *priv = jhb100_pll_priv_from(pll);
> > + const struct jhb100_pll_info *info =
> &priv->match_data->pll_info[pll->idx];
> > + u32 val;
> > +
> > + regmap_read(priv->regmap, JHB100_PLLX_CFG2(info->offset), &val);
>
> Should the return value be checked?
>
> > +
> > + return !(val & JHB100_PLL_PD);
>
> If regmap_read() returns an error, then is val uninitialized?
>
> > +}
> > +
> > +static void jhb100_pll_regvals_get(struct regmap *regmap,
> > + const struct jhb100_pll_info *info,
> > + struct jhb100_pll_regvals *ret) {
> > + u32 val;
> > +
> > + regmap_read(regmap, JHB100_PLLX_CFG0(info->offset), &val);
> > + ret->fbdiv = (val & JHB100_PLL_FBDIV) >> JHB100_PLL_FBDIV_SHIFT;
> > + ret->foutpostdiv_en = !!((val & JHB100_PLL_FOUTPOSTDIV_EN) >>
> > + JHB100_PLL_FOUTPOSTDIV_EN_SHIFT);
> > + ret->foutvcop_en = !!((val & JHB100_PLL_FOUTVCOP_EN) >>
> > + JHB100_PLL_FOUTVCOP_EN_SHIFT);
> > +
> > + regmap_read(regmap, JHB100_PLLX_CFG1(info->offset), &val);
> > + ret->frac = (val & JHB100_PLL_FRAC) >> JHB100_PLL_FRAC_SHIFT;
> > +
> > + regmap_read(regmap, JHB100_PLLX_CFG2(info->offset), &val);
> > + ret->postdiv = (val & JHB100_PLL_POSTDIV) >>
> JHB100_PLL_POSTDIV_SHIFT;
> > + ret->refdiv = (val & JHB100_PLL_REFDIV) >> JHB100_PLL_REFDIV_SHIFT;
>
> Should these regmap return values be checked, and the error code returned?
>
> > +}
> > +
> > +static unsigned long jhb100_pll_recalc_rate(struct clk_hw *hw,
> > +unsigned long parent_rate) {
> > + struct jhb100_pll_data *pll = jhb100_pll_data_from(hw);
> > + struct jhb100_pll_priv *priv = jhb100_pll_priv_from(pll);
> > + struct jhb100_pll_regvals val;
> > + unsigned long rate;
> > + u32 power = 0;
> > +
> > + jhb100_pll_regvals_get(priv->regmap,
> > +&priv->match_data->pll_info[pll->idx], &val);
> > +
> > + /*
> > + *
> > + * if (foutvcop_en)
> > + * rate = parent * (fbdiv + frac / 2^24) / refdiv
> > + *
> > + * if (foutpostdiv_en)
> > + * rate = parent * (fbdiv + frac / 2^24) / refdiv / 2^(postdiv + 1)
> > + *
> > + * parent * (fbdiv + frac / 2^24) = parent * fbdiv + parent * frac / 2^24
> > + */
> > +
> > + if (!!val.foutvcop_en == !!val.foutpostdiv_en)
> > + return 0;
> > +
> > + rate = (parent_rate * val.frac) >> 24;
> > +
> > + if (val.foutpostdiv_en)
> > + power = val.postdiv + 1;
> > +
> > + rate += parent_rate * val.fbdiv;
> > + rate /= val.refdiv << power;
>
> Could val.refdiv ever be zero?
Will check it at next version.
> > +
> > + return rate;
> > +}
> > +
> > +static int jhb100_pll_determine_rate(struct clk_hw *hw, struct
> > +clk_rate_request *req) {
> > + struct jhb100_pll_data *pll = jhb100_pll_data_from(hw);
> > + struct jhb100_pll_priv *priv = jhb100_pll_priv_from(pll);
> > + const struct jhb100_pll_info *info =
> &priv->match_data->pll_info[pll->idx];
> > + const struct jhb100_pll_preset *selected = &info->presets[0];
> > + unsigned int idx;
> > +
> > + /* if the parent rate doesn't match our expectations the presets won't
> work */
> > + if (req->best_parent_rate != JHB100_PLL_OSC_RATE) {
> > + req->rate = jhb100_pll_recalc_rate(hw, req->best_parent_rate);
> > + return 0;
> > + }
> > +
> > + /* continuous means support any rate */
> > + if (info->continuous)
> > + return 0;
> > +
> > + /* find highest rate lower or equal to the requested rate */
> > + for (idx = 1; idx < info->npresets; idx++) {
> > + const struct jhb100_pll_preset *val = &info->presets[idx];
> > +
> > + if (req->rate < val->freq)
> > + break;
> > +
> > + selected = val;
> > + }
> > +
> > + req->rate = selected->freq;
> > +
> > + return 0;
> > +}
> > +
> > +static int jhb100_pll_set_preset(struct clk_hw *hw, struct
> > +jhb100_pll_preset *val) {
> > + struct jhb100_pll_data *pll = jhb100_pll_data_from(hw);
> > + struct jhb100_pll_priv *priv = jhb100_pll_priv_from(pll);
> > + const struct jhb100_pll_info *info =
> &priv->match_data->pll_info[pll->idx];
> > + unsigned int value;
> > +
> > + regmap_update_bits(priv->regmap, JHB100_PLLX_CFG0(info->offset),
> JHB100_PLL_FBDIV,
> > + (u32)val->fbdiv << JHB100_PLL_FBDIV_SHIFT);
> > + regmap_update_bits(priv->regmap, JHB100_PLLX_CFG0(info->offset),
> JHB100_PLL_FOUTPOSTDIV_EN,
> > + (u32)val->foutpostdiv_en <<
> JHB100_PLL_FOUTPOSTDIV_EN_SHIFT);
> > + regmap_update_bits(priv->regmap, JHB100_PLLX_CFG0(info->offset),
> JHB100_PLL_FOUTVCOP_EN,
> > + (u32)val->foutvcop_en <<
> JHB100_PLL_FOUTVCOP_EN_SHIFT);
>
> These are writing to the same register. Should the values be combined into
> one, and written once to the register?
>
> > + regmap_update_bits(priv->regmap, JHB100_PLLX_CFG1(info->offset),
> JHB100_PLL_FRAC,
> > + val->frac << JHB100_PLL_FRAC_SHIFT);
> > + regmap_update_bits(priv->regmap, JHB100_PLLX_CFG2(info->offset),
> JHB100_PLL_REFDIV,
> > + (u32)val->refdiv << JHB100_PLL_REFDIV_SHIFT);
> > + regmap_update_bits(priv->regmap, JHB100_PLLX_CFG2(info->offset),
> JHB100_PLL_POSTDIV,
> > + (u32)val->postdiv << JHB100_PLL_POSTDIV_SHIFT);
>
> The last two calls to JHB100_PLLX_CFG2 also write to the same register.
> Should the two writes be combined into one?
>
> Should the return values from regmap_update_bits() be checked?
>
> > +
> > + /* waiting for PLL to lock */
> > + return regmap_read_poll_timeout_atomic(priv->regmap,
> JHB100_PLLX_CFG1(info->offset),
> > + value, value & JHB100_PLL_LOCK,
> > + JHB100_PLL_INTERVAL_US,
> > + JHB100_PLL_TIMEOUT_US);
> > +}
> > +
> > +static int jhb100_pll_rate_to_preset(struct clk_hw *hw, unsigned long rate,
> > + unsigned long parent_rate)
> > +{
> > + struct jhb100_pll_preset val = {
> > + .refdiv = 1,
> > + .postdiv = 3,
> > + .foutpostdiv_en = 1,
> > + .foutvcop_en = 0,
> > + };
> > + unsigned int power = 0;
> > + unsigned long fbdiv_24, t;
> > +
> > + if (val.foutpostdiv_en)
> > + power = val.postdiv + 1;
> > +
> > + t = val.refdiv << power;
> > + t *= rate;
> > +
> > + val.fbdiv = t / parent_rate;
>
> Should a check for parent_rate == 0 be added?
parent_rate is checked in jhb100_pll_set_rate, and it only supports 25M.
> > +
> > + fbdiv_24 = (t << 24) / parent_rate;
> > + val.frac = fbdiv_24 - (val.fbdiv << 24);
> > +
> > + return jhb100_pll_set_preset(hw, &val); }
> > +
> > +static int jhb100_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> > + unsigned long parent_rate)
> > +{
> > + struct jhb100_pll_data *pll = jhb100_pll_data_from(hw);
> > + struct jhb100_pll_priv *priv = jhb100_pll_priv_from(pll);
> > + const struct jhb100_pll_info *info =
> &priv->match_data->pll_info[pll->idx];
> > + const struct jhb100_pll_preset *val;
> > + unsigned int idx;
> > +
> > + /* if the parent rate doesn't match our expectations the presets won't
> work */
> > + if (parent_rate != JHB100_PLL_OSC_RATE)
> > + return -EINVAL;
> > +
> > + if (info->continuous)
> > + return jhb100_pll_rate_to_preset(hw, rate, parent_rate);
> > +
> > + for (idx = 0, val = &info->presets[0]; idx < info->npresets; idx++, val++) {
> > + if (val->freq == rate)
> > + return jhb100_pll_set_preset(hw, (struct jhb100_pll_preset
> *)val);
>
> The cast looks to be here because of the const in jhb100_pll_set_preset(). Can
> const be added to the declaration of jhb100_pll_set_preset()?
Will try it.
Best Regards,
Changhuang
More information about the linux-riscv
mailing list