On 3/11/25 6:19 PM, 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@xxxxxxx>
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.
FYI I encountered a problem I mentioned below.
. . .
+/* 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)?
When doing some testing today I found that the WARN_ON_ONCE()
got called. I added some information and learned that the
values in hardware of the swcr1 and swcr3 registers were:
swcr1: 0x0050cd61
swcr3: 0x3fe00000
I'm not sure which PLL was being used.
So clearly this can happen. Somehow you need to find a way
to ensure that these registers are initialized to a sane
state (meaning one defined within pll_rate_table[]).
-Alex
+ WARN_ON_ONCE(1);
Maybe WARN_ONCE(true, "msg");. . .
