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

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

[Philipp Zabel]

Hi,

the last version contained a bug when setting the muxes, which caused it
to OR the desired mux values for ldb_di0_sel and ldb_di1_sel and write the
result to the ldb_di0_sel bitfield. This is fixed now.
I think the mmdc_ch1 patch should be uncontroversial. The remaining two
could use some test feedback and somebody who can take a look at ERR009219
and EB821 and can confirm that this patch is in line with the suggested
workaround.

regards
Philipp

Changes since v3:
 - Fix LDB_DI1_SEL shift when setting divider and fix a debug printk,
   issues reported by Akshay Bhat.

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

[PATCH v4 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 f0efc6f..871c905 100644
--- a/drivers/clk/imx/clk-imx6q.c
+++ b/drivers/clk/imx/clk-imx6q.c
@@ -138,6 +138,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;
@@ -279,6 +292,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

[PATCH v4 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 871c905..cfcffa4 100644
--- a/drivers/clk/imx/clk-imx6q.c
+++ b/drivers/clk/imx/clk-imx6q.c
@@ -314,8 +314,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);
@@ -515,12 +515,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

[PATCH v4 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 v3:
 - Fix LDB_DI1_SEL shift when setting divider and fix a debug printk,
   issues reported by Akshay Bhat.
---
 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 cfcffa4..6ed41ae 100644
--- a/drivers/clk/imx/clk-imx6q.c
+++ b/drivers/clk/imx/clk-imx6q.c
@@ -138,9 +138,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)
 {
@@ -151,10 +232,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_di%d_sel: %d->%d->%d->%d\n", i,
+			 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_DI1_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;
 
@@ -167,7 +422,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 */
@@ -292,8 +547,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));
@@ -314,6 +567,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

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

[Akshay Bhat]

Hi Philipp,

On 02/26/2016 03:51 AM, Philipp Zabel wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
>

<snip>

> +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;
> +		}

EB821 doesn't mention the above restriction. My understanding was as 
long as the clock source you are switching from/to is disabled it should 
be ok to do so. Maybe someone from Freescale can comment?

> +
> +		/* First switch to the bottom mux */
> +		sel[i][1] = sel[i][0] | 4;
> +

Not sure if this really matters but as per EB821 Section 4.2, 6 b, the 
recommended setting for sel[i][1] is 7


> +		/* Then configure the top mux before switching back to it */
> +		sel[i][2] = sel[i][3] | 4;
> +

Same goes here, as per EB821, Section 4.2, 6 c, the recommended setting 
for sel[i][2] is 4

Thanks,
Akshay

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

[Akshay Bhat]

Hi Fabio,

On Tue, Mar 1, 2016 at 4:41 PM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
> Hi Philipp,
>
> On 02/26/2016 03:51 AM, Philipp Zabel wrote:
>>
>> From: Fabio Estevam <fabio.estevam@freescale.com>
>>
>
> <snip>
>
>
>> +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;
>> +               }
>
>
> EB821 doesn't mention the above restriction. My understanding was as long as
> the clock source you are switching from/to is disabled it should be ok to do
> so. Maybe someone from Freescale can comment?
>
>> +
>> +               /* First switch to the bottom mux */
>> +               sel[i][1] = sel[i][0] | 4;
>> +
>
>
> Not sure if this really matters but as per EB821 Section 4.2, 6 b, the
> recommended setting for sel[i][1] is 7
>
>
>> +               /* Then configure the top mux before switching back to it
>> */
>> +               sel[i][2] = sel[i][3] | 4;
>> +
>
>
> Same goes here, as per EB821, Section 4.2, 6 c, the recommended setting for
> sel[i][2] is 4
>

Any comment from NXP regarding the same? I have tested this patch as
it is and it resolves intermittent blank LVDS display issues we are
seeing on our boards. The questions is if the patch needs to be
updated to match EB821 document for the above sections?

It would be nice to get the patch accepted since it solves actual LVDS
display issues on clock switch.

Thanks,
Akshay

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

[Fabio Estevam]

Hi Akshay,

On Wed, Mar 23, 2016 at 12:48 PM, Akshay Bhat <akshay.bhat@timesys.com> wrote:

> Any comment from NXP regarding the same? I have tested this patch as
> it is and it resolves intermittent blank LVDS display issues we are
> seeing on our boards. The questions is if the patch needs to be
> updated to match EB821 document for the above sections?

I am not aware of such document.

Do you see any discrepancy with regards to the sequence from NXP 3.10 kernel?
http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/commit/?h=imx_3.10.17_1.0.1_ga&id=eecbe9a52587cf9eec30132fb9b8a6761f3a1e6d

>
> It would be nice to get the patch accepted since it solves actual LVDS
> display issues on clock switch.

Yes, agreed.

Thanks for testing the patch.

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

[Akshay Bhat]

Hi Fabio,

On 03/28/2016 02:53 PM, Fabio Estevam wrote:
> Hi Akshay,
>
> On Wed, Mar 23, 2016 at 12:48 PM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
>
>> Any comment from NXP regarding the same? I have tested this patch as
>> it is and it resolves intermittent blank LVDS display issues we are
>> seeing on our boards. The questions is if the patch needs to be
>> updated to match EB821 document for the above sections?
>
> I am not aware of such document.
>
> Do you see any discrepancy with regards to the sequence from NXP 3.10 kernel?
> http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/commit/?h=imx_3.10.17_1.0.1_ga&id=eecbe9a52587cf9eec30132fb9b8a6761f3a1e6d
>

The 2 discrepancies I see with this patch with regards to the sequence 
from NXP 3.10 kernel are:

1. NXP 3.10 kernel switches clock source as follows:
current_parent -> 7 -> 4 -> new_parent

Ref:
+	/*
+	 * Set the ldb_di0_clk and ldb_di1_clk to 111b.
+	 */
+	reg = readl_relaxed(ccm_base + 0x2c);
+	reg |= ((7 << 9) | (7 << 12));
+	writel_relaxed(reg, ccm_base + 0x2c);
+
+	/*
+	 * Set the ldb_di0_clk and ldb_di1_clk to 100b.
+	 */
+	reg = readl_relaxed(ccm_base + 0x2c);
+	reg &= ~((7 << 9) | (7 << 12));
+	reg |= ((4 << 9) | (4 << 12));
+	writel_relaxed(reg, ccm_base + 0x2c);
+
+	/*
+	 * Perform the LDB parent clock switch.
+	 */
+	clk_set_parent(clk[ldb_di0_sel], clk[new_parent]);
+	clk_set_parent(clk[ldb_di1_sel], clk[new_parent]);


If I am reading this patch correctly, then this patch does:
current_parent -> (current_parent | 4) -> (new_parent | 4) -> new_parent

Ref:
+	/* 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_di%d_sel: %d->%d->%d->%d\n", i,
+			 sel[i][0], sel[i][1], sel[i][2], sel[i][3]);


2. The NXP 3.10 kernel does not have any restriction on what the new 
parent is.

Ref:
+ /*
+ * Perform the LDB parent clock switch.
+ */
+ clk_set_parent(clk[ldb_di0_sel], clk[new_parent]);
+ clk_set_parent(clk[ldb_di1_sel], clk[new_parent]);


This patch has a restriction on the new_parent if the current_parent is 
already pll3_usb_otg

Ref:
+               /*
+                * 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;
+               }
+

Thanks,
Akshay

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

[Fabio Estevam]

Hi Akshay,

On Mon, Mar 28, 2016 at 4:26 PM, Akshay Bhat <akshay.bhat@timesys.com> wrote:

> The 2 discrepancies I see with this patch with regards to the sequence from
> NXP 3.10 kernel are:
>
> 1. NXP 3.10 kernel switches clock source as follows:
> current_parent -> 7 -> 4 -> new_parent

Yes, we need to follow this requirement.

>
> Ref:
> +       /*
> +        * Set the ldb_di0_clk and ldb_di1_clk to 111b.
> +        */
> +       reg = readl_relaxed(ccm_base + 0x2c);
> +       reg |= ((7 << 9) | (7 << 12));
> +       writel_relaxed(reg, ccm_base + 0x2c);
> +
> +       /*
> +        * Set the ldb_di0_clk and ldb_di1_clk to 100b.
> +        */
> +       reg = readl_relaxed(ccm_base + 0x2c);
> +       reg &= ~((7 << 9) | (7 << 12));
> +       reg |= ((4 << 9) | (4 << 12));
> +       writel_relaxed(reg, ccm_base + 0x2c);
> +
> +       /*
> +        * Perform the LDB parent clock switch.
> +        */
> +       clk_set_parent(clk[ldb_di0_sel], clk[new_parent]);
> +       clk_set_parent(clk[ldb_di1_sel], clk[new_parent]);
>
>
> If I am reading this patch correctly, then this patch does:
> current_parent -> (current_parent | 4) -> (new_parent | 4) -> new_parent
>
> Ref:
> +       /* 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_di%d_sel: %d->%d->%d->%d\n", i,
> +                        sel[i][0], sel[i][1], sel[i][2], sel[i][3]);
>
>
> 2. The NXP 3.10 kernel does not have any restriction on what the new parent
> is.

That's correct. There is no restriction on what the new parent will be.

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

[Philipp Zabel]

Hi Fabio,

Am Montag, den 28.03.2016, 16:33 -0300 schrieb Fabio Estevam:
> Hi Akshay,
> 
> On Mon, Mar 28, 2016 at 4:26 PM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
> 
> > The 2 discrepancies I see with this patch with regards to the sequence from
> > NXP 3.10 kernel are:
> >
> > 1. NXP 3.10 kernel switches clock source as follows:
> > current_parent -> 7 -> 4 -> new_parent
> 
> Yes, we need to follow this requirement.

Thanks. Does anybody know why?
(Does this work for current_parent == 4 and new_parent != 0)?

> > Ref:
> > +       /*
> > +        * Set the ldb_di0_clk and ldb_di1_clk to 111b.
> > +        */
> > +       reg = readl_relaxed(ccm_base + 0x2c);
> > +       reg |= ((7 << 9) | (7 << 12));
> > +       writel_relaxed(reg, ccm_base + 0x2c);
> > +
> > +       /*
> > +        * Set the ldb_di0_clk and ldb_di1_clk to 100b.
> > +        */
> > +       reg = readl_relaxed(ccm_base + 0x2c);
> > +       reg &= ~((7 << 9) | (7 << 12));
> > +       reg |= ((4 << 9) | (4 << 12));
> > +       writel_relaxed(reg, ccm_base + 0x2c);
> > +
> > +       /*
> > +        * Perform the LDB parent clock switch.
> > +        */
> > +       clk_set_parent(clk[ldb_di0_sel], clk[new_parent]);
> > +       clk_set_parent(clk[ldb_di1_sel], clk[new_parent]);
> >
> >
> > If I am reading this patch correctly, then this patch does:
> > current_parent -> (current_parent | 4) -> (new_parent | 4) -> new_parent

Yes. Switch to bottom mux, manipulate 4-port mux, and switch back to top
mux again was the intention.

> > Ref:
> > +       /* 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_di%d_sel: %d->%d->%d->%d\n", i,
> > +                        sel[i][0], sel[i][1], sel[i][2], sel[i][3]);
> >
> >
> > 2. The NXP 3.10 kernel does not have any restriction on what the new parent
> > is.
> 
> That's correct. There is no restriction on what the new parent will be.

Is this also true for starting values other than 3?

best regards
Philipp

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

[Fabio Estevam]

Hi Philipp,

On Wed, Mar 30, 2016 at 1:18 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:

>> > 1. NXP 3.10 kernel switches clock source as follows:
>> > current_parent -> 7 -> 4 -> new_parent
>>
>> Yes, we need to follow this requirement.
>
> Thanks. Does anybody know why?

The SoC folks say this needs to be done to make sure the glitch is not
propagated.

> (Does this work for current_parent == 4 and new_parent != 0)?

They say it works for all combinations.

> Is this also true for starting values other than 3?

Yes.

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

[Akshay Bhat]

On 04/04/2016 08:21 PM, Fabio Estevam wrote:
> Hi Philipp,
>
> On Wed, Mar 30, 2016 at 1:18 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
>>>> 1. NXP 3.10 kernel switches clock source as follows:
>>>> current_parent -> 7 -> 4 -> new_parent
>>>
>>> Yes, we need to follow this requirement.
>>
>> Thanks. Does anybody know why?
>
> The SoC folks say this needs to be done to make sure the glitch is not
> propagated.
>
>> (Does this work for current_parent == 4 and new_parent != 0)?
>
> They say it works for all combinations.
>
>> Is this also true for starting values other than 3?
>
> Yes.
>


Hi Philipp,

NXP has updated the EB821 document and made it public. As per the 
updated document your v4 patch matches the NXP recommendation for the 
clock switch. The only change I see is allowing to switch to any new 
parent (when starting at a value other than 3). Is it possible for you 
to resubmit this useful patch after reviewing the EB821 document?

http://www.nxp.com/files/32bit/doc/eng_bulletin/EB821.pdf?fasp=1&WT_TYPE=Engineering%20Bulletins&WT_VENDOR=FREESCALE&WT_FILE_FORMAT=pdf&WT_ASSET=Documentation&fileExt=.pdf

Thanks,
Akshay

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

[Philipp Zabel]

Am Freitag, den 08.07.2016, 17:21 -0400 schrieb Akshay Bhat:
> NXP has updated the EB821 document and made it public. As per the 
> updated document your v4 patch matches the NXP recommendation for the 
> clock switch. The only change I see is allowing to switch to any new 
> parent (when starting at a value other than 3). Is it possible for you 
> to resubmit this useful patch after reviewing the EB821 document?
> 
> http://www.nxp.com/files/32bit/doc/eng_bulletin/EB821.pdf?fasp=1&WT_TYPE=Engineering%20Bulletins&WT_VENDOR=FREESCALE&WT_FILE_FORMAT=pdf&WT_ASSET=Documentation&fileExt=.pdf

Thanks, I had a look at the document and resent the patchset with the
PLL3->x limit lifted.

regards
Philipp

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

[Fabio Estevam]

Hi Akshay,

On Tue, Mar 1, 2016 at 6:41 PM, Akshay Bhat <akshay.bhat@timesys.com> wrote:

> EB821 doesn't mention the above restriction. My understanding was as long as
> the clock source you are switching from/to is disabled it should be ok to do
> so. Maybe someone from Freescale can comment?

I don't have access to this EB821 document.

The procedure we have tested is shown here:
http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/commit/?h=imx_3.10.17_1.0.1_ga&id=eecbe9a52587cf9eec30132fb9b8a6761f3a1e6d

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

[Philipp Zabel]

Am Montag, den 28.03.2016, 15:48 -0300 schrieb Fabio Estevam:
> Hi Akshay,
> 
> On Tue, Mar 1, 2016 at 6:41 PM, Akshay Bhat <akshay.bhat@timesys.com> wrote:
> 
> > EB821 doesn't mention the above restriction. My understanding was as long as
> > the clock source you are switching from/to is disabled it should be ok to do
> > so. Maybe someone from Freescale can comment?
> 
> I don't have access to this EB821 document.
> 
> The procedure we have tested is shown here:
> http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/commit/?h=imx_3.10.17_1.0.1_ga&id=eecbe9a52587cf9eec30132fb9b8a6761f3a1e6d

This patch still describes the switching from top to bottom mux and only
then manipulating the top mux:

	 * The code below switches the parent to the bottom mux first
	 * and then manipulates the top mux. This ensures that no glitch
	 * will enter the divider.

Unfortunately it doesn't detail how this is achieved exactly. I assumed
that it's just that bit 0x4 manipulates the two-port mux that selects
between top and bottom mux and bits 0x3 manipulate the two 4-port muxes
(top and bottom) at the same time, but that was just conjecture.

regards
Philipp

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

[Philipp Zabel]

Hi Akshay,

Am Dienstag, den 01.03.2016, 16:41 -0500 schrieb Akshay Bhat:
[...]
> > +		/*
> > +		 * 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;
> > +		}
> 
> EB821 doesn't mention the above restriction. My understanding was as 
> long as the clock source you are switching from/to is disabled it should 
> be ok to do so. Maybe someone from Freescale can comment?

Maybe. If the only issue was that all input clocks have to be disabled,
I don't understand why the intermediate steps 3 -> 7 -> 4 -> 0 are
necessary though. See below for an explanation why I felt I have no idea
how to properly switch from 4 to 1, for example. Maybe this is indeed
not a problem at all and this condition can just be dropped. Do you have
a way to test this?

> > +
> > +		/* First switch to the bottom mux */
> > +		sel[i][1] = sel[i][0] | 4;
> > +
> 
> Not sure if this really matters but as per EB821 Section 4.2, 6 b, the 
> recommended setting for sel[i][1] is 7

As I understand, the EB821 does not explain why it should be 7, or why
the second step should be 4, and the section you quote is explicitly
just an example that assumes that the initial setting is the reset
default (3).
>From the original patch description it sounded like it is necessary to
first switch from the top mux to the bottom mux without changing the
4-port mux, which I interpreted as |4 (and describe in the last
paragraph of the comment above init_ldb_clocks). That information could
well be incorrect or incomplete. I'd love to see ERR009219.

regards
Philipp

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

[Fabio Estevam]

Hi Philipp,

On Wed, Mar 30, 2016 at 1:02 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:

> well be incorrect or incomplete. I'd love to see ERR009219.

ERR009219 is publicly available now:
http://cache.nxp.com/files/32bit/doc/errata/IMX6DQCE.pdf

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

[Philipp Zabel]

Am Dienstag, den 12.04.2016, 20:28 -0300 schrieb Fabio Estevam:
> Hi Philipp,
> 
> On Wed, Mar 30, 2016 at 1:02 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> 
> > well be incorrect or incomplete. I'd love to see ERR009219.
> 
> ERR009219 is publicly available now:
> http://cache.nxp.com/files/32bit/doc/errata/IMX6DQCE.pdf

Thank you for the hint, I have resent the patchset referencing the now
published EB821.

regards
Philipp

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

[Akshay Bhat]

Hi Philipp,

On 03/30/2016 12:02 PM, Philipp Zabel wrote:
> Hi Akshay,
>
> Am Dienstag, den 01.03.2016, 16:41 -0500 schrieb Akshay Bhat:
> [...]
>>> +		/*
>>> +		 * 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;
>>> +		}
>>
>> EB821 doesn't mention the above restriction. My understanding was as
>> long as the clock source you are switching from/to is disabled it should
>> be ok to do so. Maybe someone from Freescale can comment?
>
> Maybe. If the only issue was that all input clocks have to be disabled,
> I don't understand why the intermediate steps 3 -> 7 -> 4 -> 0 are
> necessary though. See below for an explanation why I felt I have no idea
> how to properly switch from 4 to 1, for example. Maybe this is indeed
> not a problem at all and this condition can just be dropped. Do you have
> a way to test this?
>

I do not have a way of reliably recreating the issue (happens only on 
few boards and even on those boards it is highly intermittent and seems 
to depend on factors like temperature, timing etc.). So I am hesitant to 
rely on any collected test data to suggest if the intermediate steps are 
not required. I understand the errata is rather lacking on why the 
intermediate steps are required but I feel at this point it is best to 
follow the workaround suggested by Freescale/NXP.

Thanks,
Akshay