On 29.6.2016 08:51, Jean-Francois Moine wrote:

On Tue, 28 Jun 2016 21:31:06 +0200
Ond?ej Jirman [off-list ref] wrote:

quoted

On 28.6.2016 17:37, Jean-Francois Moine wrote:

quoted

Most of the clocks in the Allwinner's SoCs are configured in the CCU
(Clock Configuration Unit).

The PLL clocks are driven from the main clock. Their rates are controlled
by a set of multiply and divide factors, named from the Allwinner's
documentation:
- multipliers: 'n' and 'k'
- dividers: 'd1', 'd2', 'm' and 'p'

The peripheral clocks may receive their inputs from one or more parents,
thanks to a mux. Their rates are controlled by a set of divide factors
only, named 'm' and 'p'.

This driver also handles:
- fixed clocks,
- the phase delays for the MMCs,
- the clock gates,
- the bus gates,
- and the resets.

Signed-off-by: Jean-Francois Moine <redacted>
---
 drivers/clk/sunxi/Makefile |   2 +
 drivers/clk/sunxi/ccu.c    | 980 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/sunxi/ccu.h    | 153 +++++++
 3 files changed, 1135 insertions(+)
 create mode 100644 drivers/clk/sunxi/ccu.c
 create mode 100644 drivers/clk/sunxi/ccu.h

	[snip]

quoted

quoted

diff --git a/drivers/clk/sunxi/ccu.c b/drivers/clk/sunxi/ccu.c
new file mode 100644
index 0000000..5749f9c
--- /dev/null
+++ b/drivers/clk/sunxi/ccu.c

	[snip]

quoted

quoted

+static void ccu_pll_set_flat_factors(struct ccu *ccu, u32 mask, u32 val)
+{
+	u32 reg, m_val, p_val;
+	u32 m_mask = (1 << ccu->m_width) - 1;
+	u32 p_mask = (1 << ccu->p_width) - 1;

I see you have shifted the factor values to be set in the caller of this
function, but then you have to shift masks too, to match. It might be
clearer to do both shifts in this function.

Oops, yes. Thanks. (I did not test yet your patch about the CPU
frequency)

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.

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?

regards,
  Ondrej

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160629/24e0a893/attachment-0001.sig>