Re: [PATCH v5 3/5] clk: spacemit: Add clock support for Spacemit K1 SoC

[Haylen Chu <heylenay@4d2.org>]

On Fri, Mar 28, 2025 at 09:00:40AM -0500, Alex Elder wrote:
> On 3/24/25 6:14 AM, Haylen Chu wrote:
> > On Tue, Mar 11, 2025 at 06:19:51PM -0500, Alex Elder wrote:
> > > On 3/6/25 11:57 AM, Haylen Chu wrote:
> > > > The clock tree of K1 SoC contains three main types of clock hardware
> > > > (PLL/DDN/MIX) and has control registers split into several multifunction
> > > > devices: APBS (PLLs), MPMU, APBC and APMU.
> > > > 
> > > > All register operations are done through regmap to ensure atomiciy
> > > > between concurrent operations of clock driver and reset,
> > > > power-domain driver that will be introduced in the future.
> > > > 
> > > > Signed-off-by: Haylen Chu <heylenay@4d2.org>
> > > 
> > > I'm very glad you have the DT issues resolved now.
> > > 
> > > I again have lots of comments on the code, and I think I've
> > > identified a few bugs.  Most of my comments, however, are
> > > suggesting minor changes for consistency and readability.
> > > 
> > > I'm going to skip over a lot of "ccu-k1.c" because most of what I
> > > say applies to the definitions in the header files.
> 
> Sorry I didn't respond to this earlier.

I've already started to work on the v6, so it doesn't matter. I'll also
cover some new decisions made during improving v6 in the reply.

> > > > ---
> > > >    drivers/clk/Kconfig               |    1 +
> > > >    drivers/clk/Makefile              |    1 +
> > > >    drivers/clk/spacemit/Kconfig      |   20 +
> > > >    drivers/clk/spacemit/Makefile     |    5 +
> > > >    drivers/clk/spacemit/ccu-k1.c     | 1714 +++++++++++++++++++++++++++++
> > > >    drivers/clk/spacemit/ccu_common.h |   47 +
> > > >    drivers/clk/spacemit/ccu_ddn.c    |   80 ++
> > > >    drivers/clk/spacemit/ccu_ddn.h    |   48 +
> > > >    drivers/clk/spacemit/ccu_mix.c    |  284 +++++
> > > >    drivers/clk/spacemit/ccu_mix.h    |  246 +++++
> > > >    drivers/clk/spacemit/ccu_pll.c    |  146 +++
> > > >    drivers/clk/spacemit/ccu_pll.h    |   76 ++
> > > >    12 files changed, 2668 insertions(+)
> > > >    create mode 100644 drivers/clk/spacemit/Kconfig
> > > >    create mode 100644 drivers/clk/spacemit/Makefile
> > > >    create mode 100644 drivers/clk/spacemit/ccu-k1.c
> > > >    create mode 100644 drivers/clk/spacemit/ccu_common.h
> > > >    create mode 100644 drivers/clk/spacemit/ccu_ddn.c
> > > >    create mode 100644 drivers/clk/spacemit/ccu_ddn.h
> > > >    create mode 100644 drivers/clk/spacemit/ccu_mix.c
> > > >    create mode 100644 drivers/clk/spacemit/ccu_mix.h
> > > >    create mode 100644 drivers/clk/spacemit/ccu_pll.c
> > > >    create mode 100644 drivers/clk/spacemit/ccu_pll.h
> > > > 
> > 
> > ...
> > 
> > > > diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
> > > > new file mode 100644
> > > > index 000000000000..5974a0a1b5f6
> > > > --- /dev/null
> > > > +++ b/drivers/clk/spacemit/ccu-k1.c

...

> > > > diff --git a/drivers/clk/spacemit/ccu_common.h b/drivers/clk/spacemit/ccu_common.h
> > > > new file mode 100644
> > > > index 000000000000..494cde96fe3c
> > > > --- /dev/null
> > > > +++ b/drivers/clk/spacemit/ccu_common.h
> > > > @@ -0,0 +1,47 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > > +/*
> > > > + * Copyright (c) 2024 SpacemiT Technology Co. Ltd
> > > > + * Copyright (c) 2024 Haylen Chu <heylenay@4d2.org>
> > > > + */
> > > > +
> > > > +#ifndef _CCU_COMMON_H_
> > > > +#define _CCU_COMMON_H_
> > > > +
> > > > +#include <linux/regmap.h>
> > > > +
> > > 
> > > I'm not going to suggest it at this point, but it might
> > > have worked out more nicely if you defined a top-level CCU
> > > structure that contained a union of structs, one for each
> > > type of clock (PLL, DDN, mix).
> > > 
> > > > +struct ccu_common {
> > > > +	struct regmap *regmap;
> > > > +	struct regmap *lock_regmap;
> > > 
> > > The lock_regmap is only used for PLL type clocks, right?
> > > So it could be included in the PLL struct within the union
> > > below?
> > 
> > This makes sense to me.
> 
> It might make sense but in part my suggestion makes more
> sense when taken with the comment I had above (about having
> a CCU structure with a union rather than separate per-type
> structures).  I think that would be better, but again I
> don't want you to have to do all that work if it means
> delaying getting your code accepted.
> 
> So move it to the union if that works, but for now it's
> fine the way it is.

I'll prefer to keep the regmap lock in the toplevel structure for now.
Moving the pointer into the union requires special care: we cannot
simply assign lock_regmap with NULL anymore in spacemit_ccu_register()
without checking the subtype of clk_hw, or part of the mix/ddn's struct
may be overwritten.

Keeping lock_regmap out of the union ensures the code is simple and
makes it less possible to accidentally mess the union up.

> > > > +
> > > > +	union {
> > > > +		/* For DDN and MIX */
> > > > +		struct {
> > > > +			u32 reg_ctrl;
> > > > +			u32 reg_fc;
> > > > +			u32 fc;

...

> > > 
> > > You define PLL_SWCR3_EN in "ccu_pll.c" to have value BIT(31).
> > > that's good.  But you should define its inverse, to define
> > > which bits in the reg_swcr3 field are the valid "magic" part.
> > > In both cases, I would define them here in this file, where
> > > the structure type is defined (not in "ccu_pll.c").
> > > #define SPACEMIT_PLL_SWCR3_EN	(u32)BIT(31)
> > > #define SPACEMIT_PLL_SWCR3_MASK	~(SPACEMIT_PLL_SWCR3_EN)

I changed my mind and consider it's better to keep these macros in
ccu_pll.c, since it isn't that useful outside of the file (knowing the
exact definition of swcr3 doesn't help much, as we're really defining
some magic values). It's not a strong opinion anyway.

> > > > +		};
> > > > +	};
> > > > +
> > > > +	struct clk_hw hw;
> > > > +};

...

> > > > diff --git a/drivers/clk/spacemit/ccu_ddn.c b/drivers/clk/spacemit/ccu_ddn.c
> > > > new file mode 100644
> > > > index 000000000000..ee187687d0c4
> > > > --- /dev/null
> > > > +++ b/drivers/clk/spacemit/ccu_ddn.c
> > > > @@ -0,0 +1,80 @@
> > 
> > ...
> > 
> > > > +static unsigned long clk_ddn_calc_best_rate(struct ccu_ddn *ddn,
> > > > +					    unsigned long rate, unsigned long prate,
> > > > +					    unsigned long *num, unsigned long *den)
> > > > +{
> > > > +	rational_best_approximation(rate, prate / 2,
> > > > +				    ddn->den_mask, ddn->num_mask,
> > > > +				    den, num);
> > > 
> > > Using rational_best_approximation() is excellent.  However I
> > > think you have a bug, and I don't think the exact way you're
> > > using it is clear (and might be wrong).
> > > 
> > > The bug is that the third and fourth arguments are the maximum
> > > numerator and denominator, respectively.  You are passing mask
> > > values, which in some sense represent the maximums.  However,
> > > your masks are not always in the low-order bits.  Here is one
> > > example:
> > > 
> > > static CCU_DDN_DEFINE(slow_uart1_14p74, pll1_d16_153p6,
> > >                        MPMU_SUCCR,
> > >                        GENMASK(28, 16), 16, GENMASK(12, 0), 0,
> > >                        0);
> > > 
> > > The "_num_mask" argument to this macro is 0x1fff0000, and the
> > > "_den_mask" is 0x00000fff.  The latter value (which gets passed
> > > as the max_numerator argument to rational_best_approximation())
> > > is fine, but the former is not.  So you need to shift both masks
> > > right by their corresponding shift value.
> > 
> > Thanks for finding it! I forget to change the function when redefining
> > struct ccu_ddn.
> > 
> > > Beyond that bug, rational_best_approximation() wants its first
> > > two arguments to define the desired rate (as a fraction).
> > 
> > I don't think it's the way that rational_best_approximation() works.
> > The first two arguments define the desired fraction, not the rate.
> > 
> > > So the desired rate should be the actual desired rate divided by 1
> > > (rather than being divided by the half the parent rate).  So
> > > this too might be a bug.
> 
> OK I took another look at this.  And I looked at the first commit
> that used this function to understand how to use it:
>   534fca068ec80 imx: serial: use rational library function
> 
> You want to know what are the best available numerator and
> denominator values to use (which fit into your register fields).
> These should be as close as possible to the fraction you're after.
> 
>     Fout = Fin * num / denom
> 
> Fin is the parent rate (always divided by two in this case),
> or "prate / 2".  Fout is the desired rate, or "rate".  You might
> get a better result if you express the "/ 2" in the parent rate
> as "* 2" in the desired rate.
> 
>     num_max = ddn->num_mask >> __ffs(ddn->num_mask);
>     den_max = ddn->den_mask >> __ffs(ddn->den_mask);
>     rational_best_approximation(rate * 2, prate,
> 				num_max, den_max, &num, &den)

Thanks, this should help :)

...

> > > > diff --git a/drivers/clk/spacemit/ccu_pll.c b/drivers/clk/spacemit/ccu_pll.c
> > > > new file mode 100644
> > > > index 000000000000..9df2149f6c98
> > > > --- /dev/null
> > > > +++ b/drivers/clk/spacemit/ccu_pll.c

...

> > > > +/* frequency unit Mhz, return pll vco freq */
> > > > +static unsigned long ccu_pll_get_vco_freq(struct clk_hw *hw)
> > > > +{
> > > > +	const struct ccu_pll_rate_tbl *pll_rate_table;
> > > > +	struct ccu_pll *p = hw_to_ccu_pll(hw);
> > > > +	struct ccu_common *common = &p->common;
> > > > +	u32 swcr1, swcr3, size;
> > > > +	int i;
> > > > +
> > > > +	ccu_read(swcr1, common, &swcr1);
> > > > +	ccu_read(swcr3, common, &swcr3);
> > > 
> > > You are masking off the EN bit, but you should really be
> > > using a mask defining which bits are valid instead.  As
> > > I said earlier:
> > > 
> > > #define SPACEMIT_PLL_SWCR3_MASK	~(SPACEMIT_PLL_SWCR3_EN)
> > > 
> > > > +	swcr3 &= ~PLL_SWCR3_EN;
> > > 
> > > 	swcr3 &= SPACEMIT_PLL_SWCR3_MASK;
> > > > +
> > > > +	pll_rate_table = p->pll.rate_tbl;
> > > > +	size = p->pll.tbl_size;
> > > > +
> > > > +	for (i = 0; i < size; i++) {
> > > > +		if (pll_rate_table[i].swcr1 == swcr1 &&
> > > > +		    pll_rate_table[i].swcr3 == swcr3)
> > > > +			return pll_rate_table[i].rate;
> > > > +	}
> > > > +
> > > 
> > > I have a general question here.  Once you set one of these
> > > clock rates, it will always use one of the rates defined
> > > in the table.
> > > 
> > > But what about initially?  Could the hardware start in a
> > > state that is not defined by this code?  Do you *set* the
> > > rate initially?  Should you (at least the first time the
> > > clock is prepared/enabled)?
> > 
> > Thanks, I've also seen your later report. Here we may support
> > clk_ops.init and reinitialize the PLL if it's not in a good state.
> > 
> > Could you please provide a possible reproducing scenario for me to test
> > against the PLL problem?
> 
> What I can tell you is that I see the warning, perhaps when
> I'm using the clock.  I'll try to narrow down a test case but
> right now I don't have one.
> 
> [    0.145906] WARNING: CPU: 0 PID: 1 at drivers/clk/spacemit/ccu_pll.c:51
> ccu_pll_recalc_rate+0x76/0x9a
> 
> I added code to report the swcr1 and swcr3 values but I don't
> have those right now.

I guess I've roughly located the cause, the warning is probably
triggered by PLL3,

- Comparing with vendor U-Boot, the driver is missing three or four
  entries and one of them matches the SWCR1 value (0x0050cd61)
- There's a typo when defining PLL3,

+static CCU_PLL_DEFINE(pll3, pll3_rate_tbl,
+                     APBS_PLL3_SWCR1, APBS_PLL2_SWCR3,
+                     MPMU_POSR, POSR_PLL3_LOCK, CLK_SET_RATE_GATE);

  here APBS_PLL2_SWCR3 should be APBS_PLL3_SWCR3. This explains the
  value of SWCR3 (0x3fe00000) in your case, where it's actually read
  from PLL2's register.

I'll add the missing configuration entries and fix the typo in v6.

> > 
> > > > +	WARN_ON_ONCE(1);
> > > 
> > > Maybe WARN_ONCE(true, "msg");
> > > 
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int ccu_pll_enable(struct clk_hw *hw)
> > > > +{
> > > > +	struct ccu_pll *p = hw_to_ccu_pll(hw);
> > > > +	struct ccu_common *common = &p->common;
> > > > +	unsigned int tmp;
> > > > +	int ret;
> > > > +
> > > 
> > > Get rid of ret (see below).
> > > 
> > > Will clk_ops->enable() ever be called when it's already
> > > enabled?  (If it won't, this isn't needed.  If it will,
> > > this checks the hardware, which is good.)
> > 
> > CCF holds a refcounter of clock consumers, so we could drop the check.

I didn't expect calls to ccu_pll_enable() may happen when the
previous stage bootloaders have already initialized the PLL. So the
right anwswer is yes, clk_ops->enable() may be called when the
underlying clock hardware has been enabled, but remove the check
shouldn't hurt, either: it's okay to rewrite the register as long as
we don't change rate-related settings if the PLL is enabled.

> > > > +	if (ccu_pll_is_enabled(hw))
> > > > +		return 0;
> > > > +
> > > > +	ccu_update(swcr3, common, PLL_SWCR3_EN, PLL_SWCR3_EN);
> > > > +
> > > > +	/* check lock status */
> > > > +	ret = regmap_read_poll_timeout_atomic(common->lock_regmap,
> > > > +					      p->pll.reg_lock,
> > > > +					      tmp,
> > > > +					      tmp & p->pll.lock_enable_bit,
> > > > +					      5, PLL_DELAY_TIME);
> > > 
> > > Just:
> > > 
> > > 	return regmap_read_poll_timeout_atomic(...);
> > > 
> > > I note that you call this here, but you hide the call
> > > to regmap_read_poll_timeout_atomic() behind the macro
> > > ccu_poll().  And ccu_poll() (used for the FC bit) is
> > > also only called once.
> > > 
> > > I suggest you get rid of regmap_poll() and just open-code
> > > it.
> > > 
> > > (You use ccu_read() and ccu_update() numerous times, so
> > > your "saving some characters" is justified.)
> > 
> > Makes sense to me, will take it.
> > 
> > > > +
> > > > +	return ret;
> > > > +}

Thanks,
Haylen Chu