On Wed, 29 Jun 2016 17:30:51 +0200
Ond?ej Jirman [off-list ref] wrote:

quoted

quoted

quoted

+	reg = readl(ccu->base + ccu->reg);
+	m_val = reg & m_mask;
+	p_val = reg & p_mask;
+
+	spin_lock(&ccu_lock);
+
+	/* increase p, then m */
+	if (ccu->p_width && p_val < (val & p_mask)) {
+		reg &= ~p_mask;
+		reg |= val & p_mask;
+		writel(reg, ccu->base + ccu->reg);
+		udelay(10);
+	}
+	if (ccu->m_width && m_val < (val & m_mask)) {
+		reg &= ~m_mask;
+		reg |= val & m_mask;
+		writel(reg, ccu->base + ccu->reg);
+		udelay(10);
+	}
+
+	/* set other factors */
+	reg &= ~(mask & ~(p_mask | m_mask));
+	reg |= val & ~(p_mask | m_mask);
+	writel(reg, ccu->base + ccu->reg);
+
+	/* decrease m */
+	if (ccu->m_width && m_val > (val & m_mask)) {
+		reg &= ~m_mask;
+		reg |= val & m_mask;
+		writel(reg, ccu->base + ccu->reg);
+		udelay(10);
+	}
+
+	/* wait for PLL stable */
+	if (ccu->lock_reg) {
+		u32 lock;
+
+		lock = BIT(ccu->lock_bit);
+		WARN_ON(readl_relaxed_poll_timeout(ccu->base + ccu->lock_reg,
+							reg, !(reg & lock),
+							100, 70000));
+	}

	[snip]

quoted

Any reason why you're waiting for the lock after decreasing m and not
before? PLL output may till run too fast before reducing the postdivider
potentially causing a crash. But I might be misunderstanding what m is
exactly.

I just followed what is done in the function
 sunxi_clk_factors_set_flat_facotrs() from Allwinnertech:

	1).try to increase factor p first
	2).try to increase factor m first
	3. write factor n & k
	4. do pair things for 2). decease factor m
	5. wait for PLL state stable
	6.do pair things for 1).  decease factor p

As this is written, it seems m is a pre-divider.

Interesting. :) I've reverse engineered their arisc code, and there the
order for setting up/changing PLL1 is:

1. try to increase factor p first
2. try to increase factor m first
3. write factor n & k
4. wait for PLL state stable
5. do pair things for 2). decease factor m
6. do pair things for 1).  decease factor p

That's why I asked.

So all this depends on the design of this clock source. It may be
different for different PLLs.

From the SDK of the sun8i/9i families, this way to change the PLL

clocks is needed only for the PLL CPU of the SoCs sun8i7w (H3) and
sun8iw8 (other name unknown).

Hard to know what to believe. Perhaps it would be possible to create a
PLL stress tester that would try both approaches and see whether one of
them is more stable/leads to creashes?

Not easy to do...

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/