On Tue, Sep 4, 2018 at 6:13 AM Abel Vesa [off-list ref] wrote:

On Tue, Aug 28, 2018 at 12:11:13PM -0700, Andrey Smirnov wrote:

quoted

On Tue, Aug 28, 2018 at 3:58 AM Abel Vesa [off-list ref] wrote:

quoted

On Fri, Aug 24, 2018 at 09:40:11AM +0200, Sascha Hauer wrote:

quoted

+Cc Andrey Smirnov who made me aware of this issue.

On Wed, Aug 22, 2018 at 04:48:21PM +0300, Abel Vesa wrote:

quoted

From: Lucas Stach <l.stach@pengutronix.de>

The SCCG is a new PLL type introduced on i.MX8. Add support for this.
The driver currently misses the PLL lock check, as the preliminary
documentation mentions lock configurations, but is quiet about where
to find the actual lock status signal.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Abel Vesa <redacted>
---
+static int clk_pll1_set_rate(struct clk_hw *hw, unsigned long rate,
+                       unsigned long parent_rate)
+{
+   struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
+   u32 val;
+   u32 divf;
+
+   divf = rate / (parent_rate * 2);
+
+   val = readl_relaxed(pll->base + PLL_CFG2);
+   val &= ~(PLL_DIVF_MASK << PLL_DIVF1_SHIFT);
+   val |= (divf - 1) << PLL_DIVF1_SHIFT;
+   writel_relaxed(val, pll->base + PLL_CFG2);
+
+   /* FIXME: PLL lock check */

Shouldn't be too hard to add, no?

Added to the next version which I intend to send today.

quoted

quoted

+
+   return 0;
+}
+
+static int clk_pll1_prepare(struct clk_hw *hw)
+{
+   struct clk_sccg_pll *pll = to_clk_sccg_pll(hw);
+   u32 val;
+
+   val = readl_relaxed(pll->base);
+   val &= ~(1 << PLL_PD);
+   writel_relaxed(val, pll->base);

pll->base + PLL_CFG0 please.

Same as above.

quoted

quoted

+static const struct clk_ops clk_sccg_pll1_ops = {
+   .is_prepared    = clk_pll1_is_prepared,
+   .recalc_rate    = clk_pll1_recalc_rate,
+   .round_rate     = clk_pll1_round_rate,
+   .set_rate       = clk_pll1_set_rate,
+};
+
+static const struct clk_ops clk_sccg_pll2_ops = {
+   .prepare        = clk_pll1_prepare,
+   .unprepare      = clk_pll1_unprepare,
+   .recalc_rate    = clk_pll2_recalc_rate,
+   .round_rate     = clk_pll2_round_rate,
+   .set_rate       = clk_pll2_set_rate,
+};

So these are two PLLs that share the same enable register. Doing the
prepare/unprepare for only one PLL can lead to all kinds of trouble.
Finding a good abstraction the properly handles this case with the
clock framework is probably also not easy.

I could imagine we'll need to track the enable state on both PLLs and
only if both are disabled we disable it in hardware.

With the current code we disable the PLLs when all consumers are
reparented to pll1, which probably has bad effects.

So it took me a while to understand exactly why this needs to stay like it is.

IMHO this means that, if nothing else, all of the below should be
documented in code as a comment, otherwise simpletons like me are
going to continue stumbling over it and wondering what's going on.

Will try to add proper comment to explain what's happening.

quoted

quoted

The PLL1 is never used by any device, instead it is used as a source for PLL2.

But because the interlink between the two of them is too complicated,
the PLLs 1 and 2 need to be separate clocks.

Can you go a little bit more into detail as for why PLL1 needs to be
exposed in the first place and can't just be dealt with behind the
scenes as a part of PLL2 abstraction? Are there use-cases where the
rates of the two are going to be adjusted individually in Linux?

Here is the SCCG PLL Block Diagram:

https://www.nxp.com/docs/en/reference-manual/IMX8MDQLQRM.pdf#page=834

Now lets take the System PLL 2 for example. If you follow the hierarchy,
when you reach the sys2_pll1_out you can see that one of its selectors is
actually sys1_pll1_ref_sel. Same thing applies to sys3_pll2_out with
sys2_pll1_ref_sel. This means there is an input reference clock interlinking
between different System PLLs.

Hmm, I am not sure I see how this would prevent you from treating a
PLL pair as a black box, but OTOH, doing that probably wouldn't result
cleaner or simpler code. IMHO, it's unfortunate that PD bit is shared
between the two clocks, but if PLL1 could only be ever connected to
corresponding PLL2, then my concerns were probably misguided and
current implementation is good enough.

Thanks,
Andrey Smirnov