[PATCH v3 0/3] i.MX6 LDB mux/divider glitch workaround

[PATCH v3 0/3] i.MX6 LDB mux/divider glitch workaround

[Philipp Zabel]

Hi,

The previous version was broken for i.MX6S/DL since
disable_anatop_clocks() would try to check the parent of clk_periph_pre,
but it was called before clk_periph_pre was registered.
disable_anatop_clocks() then went on and failed to not disable the
memory clock.

So it appears there is a yet unreleased ERR009219, which I have not yet
seen, that supposedly describes the LDB mux glitch issue, and a draft
engineering bulletin EB821 that describes the clock switching procedure.
Shawn, do you have access to these documents?

Especially the comment explaining init_ldb_clks() should be double checked
against the erratum once available, as that contains my interpretation of
the original patch comment.

regards
Philipp

Changes since v2:
 - Fix DL/S support. disable_anatop_clocks() checks whether pll2_pfd2_396m
   is the parent of clk_periph_pre to avoid disabling the memory clock.
   For this to work, disable_anatop_clocks() must be called only after
   clk_periph_pre is registered.
 - Replace magic numbers with #defines
 - Improve the comment describing init_ldb_di_clks()
 - Improve selection checking and error messages

Fabio Estevam (1):
  ARM: imx6: Fix procedure to switch the parent of LDB_DI_CLK

Philipp Zabel (2):
  ARM: imx6: Mask mmdc_ch1 handshake for periph2_sel and
    mmdc_ch1_axi_podf
  ARM: imx6: Make the LDB_DI0 and LDB_DI1 clocks read-only.

 drivers/clk/imx/clk-imx6q.c | 294 ++++++++++++++++++++++++++++++++++++++++++--
 drivers/clk/imx/clk.h       |   8 ++
 2 files changed, 292 insertions(+), 10 deletions(-)

-- 
2.7.0.rc3

[PATCH v3 1/3] ARM: imx6: Mask mmdc_ch1 handshake for periph2_sel and mmdc_ch1_axi_podf

[Philipp Zabel]

MMDC CH1 is not used on i.MX6Q, so the handshake needed to change the
parent of periph2_sel or the divider of mmdc_ch1_axi_podf will never
succeed.
Disable the handshake mechanism to allow changing the frequency of
mmdc_ch1_axi, allowing to use it as a possible source for the LDB DI
clock.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/clk/imx/clk-imx6q.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/clk/imx/clk-imx6q.c b/drivers/clk/imx/clk-imx6q.c
index c193508..ef7afa5 100644
--- a/drivers/clk/imx/clk-imx6q.c
+++ b/drivers/clk/imx/clk-imx6q.c
@@ -137,6 +137,19 @@ static struct clk ** const uart_clks[] __initconst = {
 	NULL
 };
 
+#define CCM_CCDR		0x04
+
+#define CCDR_MMDC_CH1_MASK	BIT(16)
+
+static void __init imx6q_mmdc_ch1_mask_handshake(void __iomem *ccm_base)
+{
+	unsigned int reg;
+
+	reg = readl_relaxed(ccm_base + CCM_CCDR);
+	reg |= CCDR_MMDC_CH1_MASK;
+	writel_relaxed(reg, ccm_base + CCM_CCDR);
+}
+
 static void __init imx6q_clocks_init(struct device_node *ccm_node)
 {
 	struct device_node *np;
@@ -278,6 +291,8 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
 	base = of_iomap(np, 0);
 	WARN_ON(!base);
 
+	imx6q_mmdc_ch1_mask_handshake(base);
+
 	/*                                              name                reg       shift width parent_names     num_parents */
 	clk[IMX6QDL_CLK_STEP]             = imx_clk_mux("step",	            base + 0xc,  8,  1, step_sels,	   ARRAY_SIZE(step_sels));
 	clk[IMX6QDL_CLK_PLL1_SW]          = imx_clk_mux("pll1_sw",	    base + 0xc,  2,  1, pll1_sw_sels,      ARRAY_SIZE(pll1_sw_sels));
-- 
2.7.0.rc3

[PATCH v3 2/3] ARM: imx6: Make the LDB_DI0 and LDB_DI1 clocks read-only.

[Philipp Zabel]

Due to incorrect placement of the clock gate cell in the ldb_di[x]_clk
tree, the glitchy parent mux of ldb_di[x]_clk can cause a glitch to
enter the ldb_di_ipu_div divider. If the divider gets locked up, no
ldb_di[x]_clk is generated, and the LVDS display will hang when the
ipu_di_clk is sourced from ldb_di_clk.

To fix the problem, both the new and current parent of the ldb_di_clk
should be disabled before the switch. As this can not be guaranteed by
the clock framework during runtime, make the ldb_di[x]_sel muxes read-only.
A workaround to set the muxes once during boot could be added to the
kernel or bootloader.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/clk/imx/clk-imx6q.c | 10 ++--------
 drivers/clk/imx/clk.h       |  8 ++++++++
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/imx/clk-imx6q.c b/drivers/clk/imx/clk-imx6q.c
index ef7afa5..b4b2541 100644
--- a/drivers/clk/imx/clk-imx6q.c
+++ b/drivers/clk/imx/clk-imx6q.c
@@ -313,8 +313,8 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
 	clk[IMX6QDL_CLK_GPU3D_SHADER_SEL] = imx_clk_mux("gpu3d_shader_sel", base + 0x18, 8,  2, gpu3d_shader_sels, ARRAY_SIZE(gpu3d_shader_sels));
 	clk[IMX6QDL_CLK_IPU1_SEL]         = imx_clk_mux("ipu1_sel",         base + 0x3c, 9,  2, ipu_sels,          ARRAY_SIZE(ipu_sels));
 	clk[IMX6QDL_CLK_IPU2_SEL]         = imx_clk_mux("ipu2_sel",         base + 0x3c, 14, 2, ipu_sels,          ARRAY_SIZE(ipu_sels));
-	clk[IMX6QDL_CLK_LDB_DI0_SEL]      = imx_clk_mux_flags("ldb_di0_sel", base + 0x2c, 9,  3, ldb_di_sels,      ARRAY_SIZE(ldb_di_sels), CLK_SET_RATE_PARENT);
-	clk[IMX6QDL_CLK_LDB_DI1_SEL]      = imx_clk_mux_flags("ldb_di1_sel", base + 0x2c, 12, 3, ldb_di_sels,      ARRAY_SIZE(ldb_di_sels), CLK_SET_RATE_PARENT);
+	clk[IMX6QDL_CLK_LDB_DI0_SEL]      = imx_clk_mux_ldb("ldb_di0_sel", base + 0x2c, 9,  3, ldb_di_sels,      ARRAY_SIZE(ldb_di_sels));
+	clk[IMX6QDL_CLK_LDB_DI1_SEL]      = imx_clk_mux_ldb("ldb_di1_sel", base + 0x2c, 12, 3, ldb_di_sels,      ARRAY_SIZE(ldb_di_sels));
 	clk[IMX6QDL_CLK_IPU1_DI0_PRE_SEL] = imx_clk_mux_flags("ipu1_di0_pre_sel", base + 0x34, 6,  3, ipu_di_pre_sels,   ARRAY_SIZE(ipu_di_pre_sels), CLK_SET_RATE_PARENT);
 	clk[IMX6QDL_CLK_IPU1_DI1_PRE_SEL] = imx_clk_mux_flags("ipu1_di1_pre_sel", base + 0x34, 15, 3, ipu_di_pre_sels,   ARRAY_SIZE(ipu_di_pre_sels), CLK_SET_RATE_PARENT);
 	clk[IMX6QDL_CLK_IPU2_DI0_PRE_SEL] = imx_clk_mux_flags("ipu2_di0_pre_sel", base + 0x38, 6,  3, ipu_di_pre_sels,   ARRAY_SIZE(ipu_di_pre_sels), CLK_SET_RATE_PARENT);
@@ -514,12 +514,6 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
 
 	clk_register_clkdev(clk[IMX6QDL_CLK_ENET_REF], "enet_ref", NULL);
 
-	if ((imx_get_soc_revision() != IMX_CHIP_REVISION_1_0) ||
-	    clk_on_imx6dl()) {
-		clk_set_parent(clk[IMX6QDL_CLK_LDB_DI0_SEL], clk[IMX6QDL_CLK_PLL5_VIDEO_DIV]);
-		clk_set_parent(clk[IMX6QDL_CLK_LDB_DI1_SEL], clk[IMX6QDL_CLK_PLL5_VIDEO_DIV]);
-	}
-
 	clk_set_rate(clk[IMX6QDL_CLK_PLL3_PFD1_540M], 540000000);
 	if (clk_on_imx6dl())
 		clk_set_parent(clk[IMX6QDL_CLK_IPU1_SEL], clk[IMX6QDL_CLK_PLL3_PFD1_540M]);
diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
index c94ac5c..19a887c 100644
--- a/drivers/clk/imx/clk.h
+++ b/drivers/clk/imx/clk.h
@@ -136,6 +136,14 @@ static inline struct clk *imx_clk_mux_flags(const char *name,
 			&imx_ccm_lock);
 }
 
+static inline struct clk *imx_clk_mux_ldb(const char *name, void __iomem *reg,
+		u8 shift, u8 width, const char **parents, int num_parents)
+{
+	return clk_register_mux(NULL, name, parents, num_parents,
+			CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT, reg,
+			shift, width, CLK_MUX_READ_ONLY, &imx_ccm_lock);
+}
+
 static inline struct clk *imx_clk_fixed_factor(const char *name,
 		const char *parent, unsigned int mult, unsigned int div)
 {
-- 
2.7.0.rc3

[PATCH v3 3/3] ARM: imx6: Fix procedure to switch the parent of LDB_DI_CLK

[Philipp Zabel]

From: Fabio Estevam <fabio.estevam@freescale.com>

Due to incorrect placement of the clock gate cell in the ldb_di[x]_clk
tree, the glitchy parent mux of ldb_di[x]_clk can cause a glitch to
enter the ldb_di_ipu_div divider. If the divider gets locked up, no
ldb_di[x]_clk is generated, and the LVDS display will hang when the
ipu_di_clk is sourced from ldb_di_clk.

To fix the problem, both the new and current parent of the ldb_di_clk
should be disabled before the switch. This patch ensures that correct
steps are followed when ldb_di_clk parent is switched in the beginning
of boot. The glitchy muxes are then registered as read-only. The clock
parent can be selected using the assigned-clocks and
assigned-clock-parents properties of the ccm device tree node:

	&clks {
		assigned-clocks = <&clks IMX6QDL_CLK_LDB_DI0_SEL>,
				  <&clks IMX6QDL_CLK_LDB_DI1_SEL>;
		assigned-clock-parents = <&clks IMX6QDL_CLK_MMDC_CH1_AXI>,
					 <&clks IMX6QDL_CLK_PLL5_VIDEO_DIV>;
	};

Signed-off-by: Ranjani Vaidyanathan <Ranjani.Vaidyanathan@freescale.com>
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v2:
 - Fix DL/S support. disable_anatop_clocks() checks whether pll2_pfd2_396m
   is the parent of clk_periph_pre to avoid disabling the memory clock.
   For this to work, disable_anatop_clocks() must be called only after
   clk_periph_pre is registered.
 - Replace magic numbers with #defines
 - Improve the comment describing init_ldb_di_clks()
 - Improve selection checking and error messages
---
 drivers/clk/imx/clk-imx6q.c | 275 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 270 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/imx/clk-imx6q.c b/drivers/clk/imx/clk-imx6q.c
index b4b2541..1fdd9f6 100644
--- a/drivers/clk/imx/clk-imx6q.c
+++ b/drivers/clk/imx/clk-imx6q.c
@@ -137,9 +137,90 @@ static struct clk ** const uart_clks[] __initconst = {
 	NULL
 };
 
+static int ldb_di_sel_by_clock_id(int clock_id)
+{
+	switch (clock_id) {
+	case IMX6QDL_CLK_PLL5_VIDEO_DIV:
+		if (clk_on_imx6q() &&
+		    imx_get_soc_revision() == IMX_CHIP_REVISION_1_0)
+			return -ENOENT;
+		return 0;
+	case IMX6QDL_CLK_PLL2_PFD0_352M:
+		return 1;
+	case IMX6QDL_CLK_PLL2_PFD2_396M:
+		return 2;
+	case IMX6QDL_CLK_MMDC_CH1_AXI:
+		return 3;
+	case IMX6QDL_CLK_PLL3_USB_OTG:
+		return 4;
+	default:
+		return -ENOENT;
+	}
+}
+
+static void of_assigned_ldb_sels(struct device_node *node,
+				 unsigned int *ldb_di0_sel,
+				 unsigned int *ldb_di1_sel)
+{
+	struct of_phandle_args clkspec;
+	int index, rc, num_parents;
+	int parent, child, sel;
+
+	num_parents = of_count_phandle_with_args(node, "assigned-clock-parents",
+						 "#clock-cells");
+	for (index = 0; index < num_parents; index++) {
+		rc = of_parse_phandle_with_args(node, "assigned-clock-parents",
+					"#clock-cells", index, &clkspec);
+		if (rc < 0) {
+			/* skip empty (null) phandles */
+			if (rc == -ENOENT)
+				continue;
+			else
+				return;
+		}
+		if (clkspec.np != node || clkspec.args[0] >= IMX6QDL_CLK_END) {
+			pr_err("ccm: parent clock %d not in ccm\n", index);
+			return;
+		}
+		parent = clkspec.args[0];
+
+		rc = of_parse_phandle_with_args(node, "assigned-clocks",
+				"#clock-cells", index, &clkspec);
+		if (rc < 0)
+			return;
+		if (clkspec.np != node || clkspec.args[0] >= IMX6QDL_CLK_END) {
+			pr_err("ccm: child clock %d not in ccm\n", index);
+			return;
+		}
+		child = clkspec.args[0];
+
+		if (child != IMX6QDL_CLK_LDB_DI0_SEL &&
+		    child != IMX6QDL_CLK_LDB_DI1_SEL)
+			continue;
+
+		sel = ldb_di_sel_by_clock_id(parent);
+		if (sel < 0) {
+			pr_err("ccm: invalid ldb_di%d parent clock: %d\n",
+			       child == IMX6QDL_CLK_LDB_DI1_SEL, parent);
+			continue;
+		}
+
+		if (child == IMX6QDL_CLK_LDB_DI0_SEL)
+			*ldb_di0_sel = sel;
+		if (child == IMX6QDL_CLK_LDB_DI1_SEL)
+			*ldb_di1_sel = sel;
+	}
+}
+
 #define CCM_CCDR		0x04
+#define CCM_CCSR		0x0c
+#define CCM_CS2CDR		0x2c
 
-#define CCDR_MMDC_CH1_MASK	BIT(16)
+#define CCDR_MMDC_CH1_MASK		BIT(16)
+#define CCSR_PLL3_SW_CLK_SEL		BIT(0)
+
+#define CS2CDR_LDB_DI0_CLK_SEL_SHIFT	9
+#define CS2CDR_LDB_DI1_CLK_SEL_SHIFT	12
 
 static void __init imx6q_mmdc_ch1_mask_handshake(void __iomem *ccm_base)
 {
@@ -150,10 +231,184 @@ static void __init imx6q_mmdc_ch1_mask_handshake(void __iomem *ccm_base)
 	writel_relaxed(reg, ccm_base + CCM_CCDR);
 }
 
+/*
+ * The only way to disable the MMDC_CH1 clock is to move it to pll3_sw_clk
+ * via periph2_clk2_sel and then to disable pll3_sw_clk by selecting the
+ * bypass clock source, since there is no CG bit for mmdc_ch1.
+ */
+static void mmdc_ch1_disable(void __iomem *ccm_base)
+{
+	unsigned int reg;
+
+	clk_set_parent(clk[IMX6QDL_CLK_PERIPH2_CLK2_SEL],
+		       clk[IMX6QDL_CLK_PLL3_USB_OTG]);
+
+	/*
+	 * Handshake with mmdc_ch1 module must be masked when changing
+	 * periph2_clk_sel.
+	 */
+	clk_set_parent(clk[IMX6QDL_CLK_PERIPH2], clk[IMX6QDL_CLK_PERIPH2_CLK2]);
+
+	/* Disable pll3_sw_clk by selecting the bypass clock source */
+	reg = readl_relaxed(ccm_base + CCM_CCSR);
+	reg |= CCSR_PLL3_SW_CLK_SEL;
+	writel_relaxed(reg, ccm_base + CCM_CCSR);
+}
+
+static void mmdc_ch1_reenable(void __iomem *ccm_base)
+{
+	unsigned int reg;
+
+	/* Enable pll3_sw_clk by disabling the bypass */
+	reg = readl_relaxed(ccm_base + CCM_CCSR);
+	reg &= ~CCSR_PLL3_SW_CLK_SEL;
+	writel_relaxed(reg, ccm_base + CCM_CCSR);
+
+	clk_set_parent(clk[IMX6QDL_CLK_PERIPH2], clk[IMX6QDL_CLK_PERIPH2_PRE]);
+}
+
+/*
+ * We have to follow a strict procedure when changing the LDB clock source,
+ * otherwise we risk introducing a glitch that can lock up the LDB divider.
+ * Things to keep in mind:
+ *
+ * 1. The current and new parent clock inputs to the mux must be disabled.
+ * 2. The default clock input for ldb_di0/1_clk_sel is mmdc_ch1_axi, which
+ *    has no CG bit.
+ * 3. pll2_pfd2_396m can not be gated if it is used as memory clock.
+ * 4. In the RTL implementation of the LDB_DI_CLK_SEL muxes the top four
+ *    options are in one mux and the PLL3 option along with three unused
+ *    inputs is in a second mux. There is a third mux with two inputs used
+ *    to decide between the first and second 4-port mux:
+ *
+ *    pll5_video_div 0 --|\
+ *    pll2_pfd0_352m 1 --| |_
+ *    pll2_pfd2_396m 2 --| | `-|\
+ *    mmdc_ch1_axi   3 --|/    | |
+ *                             | |--
+ *    pll3_usb_otg   4 --|\    | |
+ *                   5 --| |_,-|/
+ *                   6 --| |
+ *                   7 --|/
+ *
+ * The ldb_di0/1_clk_sel[1:0] bits control both 4-port muxes@the same time.
+ * The ldb_di0/1_clk_sel[2] bit controls the 2-port mux. The code below
+ * switches the parent to the bottom mux first and then manipulates the top
+ * mux to ensure that no glitch will enter the divider.
+ */
+static void init_ldb_clks(struct device_node *np, void __iomem *ccm_base)
+{
+	unsigned int reg;
+	unsigned int sel[2][4];
+	int i;
+
+	reg = readl_relaxed(ccm_base + CCM_CS2CDR);
+	sel[0][0] = (reg >> CS2CDR_LDB_DI0_CLK_SEL_SHIFT) & 7;
+	sel[1][0] = (reg >> CS2CDR_LDB_DI1_CLK_SEL_SHIFT) & 7;
+
+	sel[0][3] = sel[0][2] = sel[0][1] = sel[0][0];
+	sel[1][3] = sel[1][2] = sel[1][1] = sel[1][0];
+
+	of_assigned_ldb_sels(np, &sel[0][3], &sel[1][3]);
+
+	for (i = 0; i < 2; i++) {
+		/* Warn if a glitch might have been introduced already */
+		if (sel[i][0] != 3) {
+			pr_warn("ccm: ldb_di%d_sel already changed from reset value: %d\n",
+				i, sel[i][0]);
+		}
+
+		if (sel[i][0] == sel[i][3])
+			continue;
+
+		/* Only switch to or from pll2_pfd2_396m if it is disabled */
+		if ((sel[i][0] == 2 || sel[i][3] == 2) &&
+		    (clk_get_parent(clk[IMX6QDL_CLK_PERIPH_PRE]) ==
+		     clk[IMX6QDL_CLK_PLL2_PFD2_396M])) {
+			pr_err("ccm: ldb_di%d_sel: couldn't disable pll2_pfd2_396m\n",
+			       i);
+			sel[i][3] = sel[i][2] = sel[i][1] = sel[i][0];
+			continue;
+		}
+
+		/*
+		 * It is unclear whether the procedure works for switching from
+		 * pll3_usb_otg to any other parent than pll5_video_div
+		 */
+		if (sel[i][0] > 3 && sel[i][0] != (sel[i][3] | 4)) {
+			pr_err("ccm: ldb_di%d_sel workaround only for top mux\n",
+			       i);
+			sel[i][3] = sel[i][2] = sel[i][1] = sel[i][0];
+			continue;
+		}
+
+		/* First switch to the bottom mux */
+		sel[i][1] = sel[i][0] | 4;
+
+		/* Then configure the top mux before switching back to it */
+		sel[i][2] = sel[i][3] | 4;
+
+		pr_debug("ccm: switching ldb_di0_sel: %d->%d->%d->%d\n",
+			 sel[i][0], sel[i][1], sel[i][2], sel[i][3]);
+	}
+
+	if (sel[0][0] == sel[0][3] && sel[1][0] == sel[1][3])
+		return;
+
+	mmdc_ch1_disable(ccm_base);
+
+	for (i = 1; i < 4; i++) {
+		reg = readl_relaxed(ccm_base + CCM_CS2CDR);
+		reg &= ~((7 << CS2CDR_LDB_DI0_CLK_SEL_SHIFT) |
+			 (7 << CS2CDR_LDB_DI1_CLK_SEL_SHIFT));
+		reg |= ((sel[0][i] << CS2CDR_LDB_DI0_CLK_SEL_SHIFT) |
+			(sel[1][i] << CS2CDR_LDB_DI0_CLK_SEL_SHIFT));
+		writel_relaxed(reg, ccm_base + CCM_CS2CDR);
+	}
+
+	mmdc_ch1_reenable(ccm_base);
+}
+
+#define CCM_ANALOG_PLL_VIDEO	0xa0
+#define CCM_ANALOG_PFD_480	0xf0
+#define CCM_ANALOG_PFD_528	0x100
+
+#define PLL_ENABLE		BIT(13)
+
+#define PFD0_CLKGATE		BIT(7)
+#define PFD1_CLKGATE		BIT(15)
+#define PFD2_CLKGATE		BIT(23)
+#define PFD3_CLKGATE		BIT(31)
+
+static void disable_anatop_clocks(void __iomem *anatop_base)
+{
+	unsigned int reg;
+
+	/* Make sure PLL2 PFDs 0-2 are gated */
+	reg = readl_relaxed(anatop_base + CCM_ANALOG_PFD_528);
+	/* Cannot gate PFD2 if pll2_pfd2_396m is the parent of MMDC clock */
+	if (clk_get_parent(clk[IMX6QDL_CLK_PERIPH_PRE]) ==
+	    clk[IMX6QDL_CLK_PLL2_PFD2_396M])
+		reg |= PFD0_CLKGATE | PFD1_CLKGATE;
+	else
+		reg |= PFD0_CLKGATE | PFD1_CLKGATE | PFD2_CLKGATE;
+	writel_relaxed(reg, anatop_base + CCM_ANALOG_PFD_528);
+
+	/* Make sure PLL3 PFDs 0-3 are gated */
+	reg = readl_relaxed(anatop_base + CCM_ANALOG_PFD_480);
+	reg |= PFD0_CLKGATE | PFD1_CLKGATE | PFD2_CLKGATE | PFD3_CLKGATE;
+	writel_relaxed(reg, anatop_base + CCM_ANALOG_PFD_480);
+
+	/* Make sure PLL5 is disabled */
+	reg = readl_relaxed(anatop_base + CCM_ANALOG_PLL_VIDEO);
+	reg &= ~PLL_ENABLE;
+	writel_relaxed(reg, anatop_base + CCM_ANALOG_PLL_VIDEO);
+}
+
 static void __init imx6q_clocks_init(struct device_node *ccm_node)
 {
 	struct device_node *np;
-	void __iomem *base;
+	void __iomem *anatop_base, *base;
 	int i;
 	int ret;
 
@@ -166,7 +421,7 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
 	clk[IMX6QDL_CLK_ANACLK2] = imx_obtain_fixed_clock("anaclk2", 0);
 
 	np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-anatop");
-	base = of_iomap(np, 0);
+	anatop_base = base = of_iomap(np, 0);
 	WARN_ON(!base);
 
 	/* Audio/video PLL post dividers do not work on i.MX6q revision 1.0 */
@@ -291,8 +546,6 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
 	base = of_iomap(np, 0);
 	WARN_ON(!base);
 
-	imx6q_mmdc_ch1_mask_handshake(base);
-
 	/*                                              name                reg       shift width parent_names     num_parents */
 	clk[IMX6QDL_CLK_STEP]             = imx_clk_mux("step",	            base + 0xc,  8,  1, step_sels,	   ARRAY_SIZE(step_sels));
 	clk[IMX6QDL_CLK_PLL1_SW]          = imx_clk_mux("pll1_sw",	    base + 0xc,  2,  1, pll1_sw_sels,      ARRAY_SIZE(pll1_sw_sels));
@@ -313,6 +566,18 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node)
 	clk[IMX6QDL_CLK_GPU3D_SHADER_SEL] = imx_clk_mux("gpu3d_shader_sel", base + 0x18, 8,  2, gpu3d_shader_sels, ARRAY_SIZE(gpu3d_shader_sels));
 	clk[IMX6QDL_CLK_IPU1_SEL]         = imx_clk_mux("ipu1_sel",         base + 0x3c, 9,  2, ipu_sels,          ARRAY_SIZE(ipu_sels));
 	clk[IMX6QDL_CLK_IPU2_SEL]         = imx_clk_mux("ipu2_sel",         base + 0x3c, 14, 2, ipu_sels,          ARRAY_SIZE(ipu_sels));
+
+	disable_anatop_clocks(anatop_base);
+
+	imx6q_mmdc_ch1_mask_handshake(base);
+
+	/*
+	 * The LDB_DI0/1_SEL muxes are registered read-only due to a hardware
+	 * bug. Set the muxes to the requested values before registering the
+	 * ldb_di_sel clocks.
+	 */
+	init_ldb_clks(np, base);
+
 	clk[IMX6QDL_CLK_LDB_DI0_SEL]      = imx_clk_mux_ldb("ldb_di0_sel", base + 0x2c, 9,  3, ldb_di_sels,      ARRAY_SIZE(ldb_di_sels));
 	clk[IMX6QDL_CLK_LDB_DI1_SEL]      = imx_clk_mux_ldb("ldb_di1_sel", base + 0x2c, 12, 3, ldb_di_sels,      ARRAY_SIZE(ldb_di_sels));
 	clk[IMX6QDL_CLK_IPU1_DI0_PRE_SEL] = imx_clk_mux_flags("ipu1_di0_pre_sel", base + 0x34, 6,  3, ipu_di_pre_sels,   ARRAY_SIZE(ipu_di_pre_sels), CLK_SET_RATE_PARENT);
-- 
2.7.0.rc3

[PATCH v3 0/3] i.MX6 LDB mux/divider glitch workaround

[Sean Cross]

On 22/01/2016 19:11, Philipp Zabel wrote:
> Hi,
>
> The previous version was broken for i.MX6S/DL since
> disable_anatop_clocks() would try to check the parent of clk_periph_pre,
> but it was called before clk_periph_pre was registered.
> disable_anatop_clocks() then went on and failed to not disable the
> memory clock.
>
> So it appears there is a yet unreleased ERR009219, which I have not yet
> seen, that supposedly describes the LDB mux glitch issue, and a draft
> engineering bulletin EB821 that describes the clock switching procedure.
> Shawn, do you have access to these documents?
>
> Especially the comment explaining init_ldb_clks() should be double checked
> against the erratum once available, as that contains my interpretation of
> the original patch comment.
Hi Philipp,

I have applied this series on Novena, and it results in a mostly
nonfunctional display.

We use dual-lane LVDS via an LVDS-to-eDP bridge.  Since we support HDMI
as well, and HDMI monitors are much pickier about timing than raw
panels, we clock the LDB off of PLL3 with the following entry in our
device tree:

&clks {
        assigned-clocks = <&clks IMX6QDL_CLK_LDB_DI0_SEL>,
                          <&clks IMX6QDL_CLK_LDB_DI1_SEL>;
        assigned-clock-parents = <&clks IMX6QDL_CLK_PLL3_USB_OTG>,
                                 <&clks IMX6QDL_CLK_PLL3_USB_OTG>;
};

With the patch applied, the screen looks very wrong.  The clock is
unstable, and the picture bounces around.  It is somewhat related to CPU
load.

Curiously, this is visually similar to (but not exactly the same as) the
effect we spoke of on IRC, except the result with this patch has more
banding and sparkling, and is slightly less stable.  I can very easily
reproduce the problem by writing first 0x000, then 0x660 to physical
address 0x2600000.


Sean