[PATCH 4/4] clk: add support for siflower sf21-topcrm
Chuanhong Guo
gch981213 at gmail.com
Mon May 18 06:34:17 PDT 2026
Hi!
On Mon, May 18, 2026 at 8:22 PM Yao Zi <me at ziyao.cc> wrote:
>
> On Sun, May 17, 2026 at 10:12:58PM +0800, Chuanhong Guo wrote:
> > This commit adds a driver for the Toplevel clock and reset controller
> > found on Siflower SF21A6826/SF21H8898 SoCs.
> > This block contains control for 3 PLLs, several clock mux/gate/divider
> > blocks, and a reset register for on-chip peripherals.
>
> It would be better if you could split out the reset code into
> drivers/reset, and initialize the reset controller as an auxiliary
> device, like what has been done for SpacemiT platform
> (drivers/{clock,reset}/spacemit) and AMLogic platform
> (drivers/clock/meson/axg-audio.c and
> drivers/reset/amlogic/reset-meson-aux.c).
It seems pretty common for these kinds of combined clock and reset
blocks to have a single driver. (There are 32 "select RESET_CONTROLLER"
in drivers/clk.) I deliberately chose to merge the clock and reset
together since it's named "clock and reset module" in the datasheet
and is a single block for both clock and reset on this chip.
I think syscon is usually used for those registers with a bunch
of miscellaneous control fields instead.
>
> I am neither clock nor reset maintainer, thus this only serves as
> a suggestion, with which it ends up in better code structure.
>
> > There are also two registers for enabling PCIE clock output in this
> > block. They aren't covered by this patch because I can't test those
> > without a PCIE driver. These will be added with the PCIE driver
>
> s/PCIE/PCIe/g which is the formal spelling.
Sure. I'll fix this in v2.
>
> > patchset later after I get that working.
> >
> > Signed-off-by: Chuanhong Guo <gch981213 at gmail.com>
> > ---
> > drivers/clk/Kconfig | 1 +
> > drivers/clk/Makefile | 1 +
> > drivers/clk/siflower/Kconfig | 22 +
> > drivers/clk/siflower/Makefile | 1 +
> > drivers/clk/siflower/clk-sf21-topcrm.c | 1053 ++++++++++++++++++++++++++++++++
> > 5 files changed, 1078 insertions(+)
>
> ...
>
> > diff --git a/drivers/clk/siflower/clk-sf21-topcrm.c b/drivers/clk/siflower/clk-sf21-topcrm.c
> > new file mode 100644
> > index 000000000000..7d4c5e370d6d
> > --- /dev/null
> > +++ b/drivers/clk/siflower/clk-sf21-topcrm.c
> > @@ -0,0 +1,1053 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/of.h>
> > +#include <linux/slab.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/rational.h>
> > +#include <linux/module.h>
> > +#include <linux/reset-controller.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/platform_device.h>
> > +#include <dt-bindings/clock/siflower,sf21-topcrm.h>
>
> Consider sorting the headers?
OK.
>
> ...
>
> > +static int sf21_cmnpll_vco_determine_rate(struct clk_hw *hw,
> > + struct clk_rate_request *req)
> > +{
> > + unsigned long fbdiv, refdiv;
> > +
> > + rational_best_approximation(req->rate, req->best_parent_rate,
> > + BIT(PLL_CMN_FBDIV_BITS) - 1,
> > + BIT(PLL_CMN_REFDIV_BITS) - 1, &fbdiv,
>
> FIELD_MAX(PLL_CMN_{FBDIV,REFDIV}_BITS}) would be simpler, with which it
> should be possible to get avoid of PLL_CMN_*_BITS.
Oh, I didn't know this is a thing. I'll change it in v2.
>
> > + &refdiv);
> > + if (!refdiv || !fbdiv)
> > + return -EINVAL;
> > +
> > + req->rate = (req->best_parent_rate / refdiv) * fbdiv;
> > +
> > + return 0;
> > +}
> > +
> > +static int sf21_cmnpll_vco_set_rate(struct clk_hw *hw, unsigned long rate,
> > + unsigned long parent_rate)
> > +{
> > + struct sf_clk_common *priv = hw_to_sf_clk_common(hw);
> > + unsigned long flags;
> > + unsigned long fbdiv, refdiv;
> > + u32 val;
> > + int ret;
> > +
> > + rational_best_approximation(rate, parent_rate,
> > + BIT(PLL_CMN_FBDIV_BITS) - 1,
> > + BIT(PLL_CMN_REFDIV_BITS) - 1, &fbdiv,
> > + &refdiv);
> > + if (!refdiv || !fbdiv)
> > + return -EINVAL;
> > +
> > + spin_lock_irqsave(priv->lock, flags);
>
> guard(spinlock_irqsave)(priv->lock)
>
> might simplify the code (especially the error handling path in this
> function), this applies as well for other places where
> spin_lock_irqsave() involves.
Oh, that macro is cool. I'll use it in v2.
>
> [...]
>
> Besides field/register offsets, the only difference I could tell between
> cmnpll_postdiv and pciepll_fout is that pciepll_fout clocks could be
> gated.
>
> Would it be a good idea to describe the gating function separately as a
> clock, and merge the common part of pciepll_fout and cmnpll_postdiv? In
> which way you could save a lot of mostly duplicated code.
I don't think so. Since the majority of the existing code are register
operations, and the register bit layout are completely different,
there isn't much to share between these two PLLs.
Writing a struct to describe both register layouts together seems
unnecessarily complicated and harder to read.
BTW the CMNPLL and PCIEPLL are actually different hardware.
The former is an integer PLL while the latter actually supports
fractional operations. I didn't add support for the fractional part
due to the lack of use cases and documentation. PCIE and GMAC
clocks are fed by this PLL and require exact clock frequencies
which can be achieved using only the integer mode.
>
> > + .recalc_rate = sf21_pciepll_fout_recalc_rate,
> > + .determine_rate = sf21_pciepll_fout_determine_rate,
> > + .set_rate = sf21_pciepll_fout_set_rate,
> > +};
> [...]
> > +
> > + spin_lock_irqsave(cmn_priv->lock, flags);
> > + if (index)
> > + sf_rmw(cmn_priv, mux_reg, 0, BIT(mux_offs));
> > + else
> > + sf_rmw(cmn_priv, mux_reg, BIT(mux_offs), 0);
> > +
> > + spin_unlock_irqrestore(cmn_priv->lock, flags);
> > + return 0;
> > +}
>
> I believe besides the divider reloading part, clk_mux_ops,
> clk_divider_ops, and clk_gate_ops have already provided the logic
> you implemented here. So it might be a better option to composite them
> together to implement your clocks instead of building from scratch.
The divider reloading is the exact reason I chose to not compose them
with the function you mentioned. The reloading bit need to be set on
both clock divider change and clock enabling, because the clock divider
loading only happens when the clock is running. Since I already have
to write two of the three parts you mentioned, trying to reuse
clk_mux_ops doesn't seem to reduce the code complexity here.
It's just trading get_parent and set_parent call with one more level
in the clock tree for every clock and more code to wire them together
in the probe function.
>
> > +static const struct clk_ops sf21_clk_muxdiv_ops = {
> > + .enable = sf21_muxdiv_enable,
> > + .disable = sf21_muxdiv_disable,
> > + .is_enabled = sf21_muxdiv_is_enabled,
> > + .recalc_rate = sf21_muxdiv_recalc_rate,
> > + .determine_rate = sf21_muxdiv_determine_rate,
> > + .set_rate = sf21_muxdiv_set_rate,
> > + .get_parent = sf21_muxdiv_get_parent,
> > + .set_parent = sf21_muxdiv_set_parent,
> > +};
>
> ...
>
> > +static SF21_MUXDIV(muxdiv_cpu, clk_periph_parents, 0, 1, 0, 0, 0,
> > + CLK_IGNORE_UNUSED);
>
> If it supplies the CPU core, shouldn't it be CLK_IS_CRITICAL instead?
>
> > +static SF21_MUXDIV(muxdiv_pic, clk_periph_parents, 0, 3, 3, 16, 1,
> > + CLK_IGNORE_UNUSED);
>
> Do you have any information about purpose of the clock and why it's
> marked as CLK_IGNORE_UNUSED? It's better to have a comment explaining
> this since it's not very obvious...
This one is Platform Interrupt Controller and it feeds PLIC + CLINT in the
C908 core complex.
I copied these from the vendor driver.
I'll make them CLK_IS_CRITICAL in v2.
--
Regards,
Chuanhong Guo
More information about the linux-riscv
mailing list