Why does RP1_PLL_SYS_SEC need to be critical?

[6by9]

Describe the bug

From #6459, RP1_PLL_SYS_SEC has to be defined as critical to avoid it being disabled and losing the post-divider setup in (ctrl reg bits 12:8).

Setting a clock rate should reset those anyway, and we have an assigned-clock-rate in DT for that clock, so why does it not get set?

Steps to reproduce the behaviour

See #6459. Remove CLK_IS_CRITICAL from RP1_PLL_SYS_SEC. Notice that ethernet doesn't negotiate a DHCP address.
/sys/kernel/debug/clk/pll_sys_sec/regdump reports 0x80000000, not 0x80000800.

Device (s)

Raspberry Pi 5

System

Pi5 using 6.6.62

Logs

No response

Additional context

No response

[pelwell]

Having an assigned clock rate and having a consumer are not the same thing - who is the consumer?

[6by9]

RP1_CLK_ETH is the consumer.

[pelwell]

Likely culprit:

static void rp1_pll_divider_off(struct clk_hw *hw)
{
	struct rp1_pll *divider = container_of(hw, struct rp1_pll, div.hw);
	struct rp1_clockman *clockman = divider->clockman;
	const struct rp1_pll_data *data = divider->data;

	spin_lock(&clockman->regs_lock);
	clockman_write(clockman, data->ctrl_reg, PLL_SEC_RST);
	spin_unlock(&clockman->regs_lock);
}

[6by9]

Yes P33M has pointed to that in #6459 (comment), however those bits are also set via

static int rp1_pll_divider_set_rate(struct clk_hw *hw,
				    unsigned long rate,
				    unsigned long parent_rate)
{
	struct rp1_pll *divider = container_of(hw, struct rp1_pll, div.hw);
	struct rp1_clockman *clockman = divider->clockman;
	const struct rp1_pll_data *data = divider->data;
	u32 div, sec;

	div = DIV_ROUND_UP_ULL(parent_rate, rate);
	div = clamp(div, 8u, 19u);

	spin_lock(&clockman->regs_lock);
	sec = clockman_read(clockman, data->ctrl_reg);
	sec = set_register_field(sec, div, PLL_SEC_DIV_MASK, PLL_SEC_DIV_SHIFT);

	/* Must keep the divider in reset to change the value. */
	sec |= PLL_SEC_RST;
	clockman_write(clockman, data->ctrl_reg, sec);

So if something sets the rate (which I sort of expected an assigned rate to do), then they should have been set.

[pelwell]

That assumes that the set_rate happens after the divider has been turned off.

[pelwell]

Wouldn't it make sense to make rp1_pll_divider_off less destructive before looking for other explanations?

[6by9]

Wouldn't it make sense to make rp1_pll_divider_off less destructive before looking for other explanations?

I'm happy to go with that change. P33M had described it as noddy and making assumptions. I don't know the clock frameworks well enough to comment.

[pelwell]

Unprepare is the opposite of prepare. Prepare doesn't set the clock rate, therefore unprepare shouldn't unset it.

[6by9]

Unprepare is the opposite of prepare. Prepare doesn't set the clock rate, therefore unprepare shouldn't unset it.

Ack. Will update the PR. Thanks for the answers.