i.MX6 LDB mux/divider glitch workaround

PATCH Superseded Philipp Zabel 3 participants 19 messages Feb 26, 2016 – Jul 11, 2016

Subsystems (3)

Arm i.MX and Layerscape Common clk NXP i.MX clock drivers

lore

Share & tools

lore

Philipp Zabel

author

2016-02-26 08:51 UTC #0 lore raw

v4 cover i.MX6 LDB mux/divider glitch workaround

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(-)

Signature · 1 line

2.7.0

Philipp Zabel

author

2016-02-26 08:51 UTC #1 lore raw

PATCH v4 1/3 ARM: imx6: Mask mmdc_ch1 handshake for periph2_sel and mmdc_ch1_axi_podf

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 <[email protected]>

---
 drivers/clk/imx/clk-imx6q.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

drivers/clk/imx/clk-imx6q.c +15 -0

index f0efc6f..871c905 100644
@@ -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));

Signature · 1 line

2.7.0

Philipp Zabel

author

2016-02-26 08:51 UTC #2 lore raw

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

In reply to 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 <[email protected]>

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

drivers/clk/imx/clk-imx6q.c +2 -8

index 871c905..cfcffa4 100644
@@ -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]);

drivers/clk/imx/clk.h +8 -0

index c94ac5c..19a887c 100644
@@ -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)
 {

Signature · 1 line

2.7.0

Philipp Zabel

author

2016-02-26 08:51 UTC #3 lore raw

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

In reply to Philipp Zabel

From: Fabio Estevam <[email protected]>

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 <[email protected]>

Signed-off-by Fabio Estevam <[email protected]>

Signed-off-by Philipp Zabel <[email protected]>

---
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(-)

drivers/clk/imx/clk-imx6q.c +270 -5

index cfcffa4..6ed41ae 100644
@@ -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);

Signature · 1 line

2.7.0

Akshay Bhat

2016-03-01 21:41 UTC #4 lore raw

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

Hi Philipp,

Quoted 2 lines · Philipp Zabel

From: Fabio Estevam <[email protected]>

<snip>

Quoted 35 lines

drivers/clk/imx/clk-imx6q.c +35 -0

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

drivers/clk/imx/clk-imx6q.c static void __init imx6q_mmdc_ch1_mask_handshake(void __iomem *ccm_base) · line 344

Quoted 10 lines

drivers/clk/imx/clk-imx6q.c +10 -0

+		/*
+		 * 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?

drivers/clk/imx/clk-imx6q.c static void __init imx6q_mmdc_ch1_mask_handshake(void __iomem *ccm_base) · line 348

Quoted 4 lines

drivers/clk/imx/clk-imx6q.c +4 -0

+
+		/* 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

drivers/clk/imx/clk-imx6q.c static void __init imx6q_mmdc_ch1_mask_handshake(void __iomem *ccm_base) · line 351

Quoted 3 lines

drivers/clk/imx/clk-imx6q.c +3 -0

+		/* 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

Akshay Bhat

2016-03-23 15:48 UTC #5 lore raw

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

Hi Fabio,

Quoted 76 lines · Akshay Bhat

Hi Philipp,

Quoted 3 lines · Philipp Zabel

From: Fabio Estevam <[email protected]>

<snip>

Quoted 46 lines

drivers/clk/imx/clk-imx6q.c +19 -0

+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",

drivers/clk/imx/clk-imx6q.c +7 -0

+                               i, sel[i][0]);
+               }
+
+               if (sel[i][0] == sel[i][3])
+                       continue;
+
+               /* Only switch to or from pll2_pfd2_396m if it is disabled

drivers/clk/imx/clk-imx6q.c +4 -0

+               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",

drivers/clk/imx/clk-imx6q.c +7 -0

+                              i);
+                       sel[i][3] = sel[i][2] = sel[i][1] = sel[i][0];
+                       continue;
+               }
+
+               /*
+                * It is unclear whether the procedure works for switching

from

drivers/clk/imx/clk-imx6q.c +4 -0

+                * 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",

drivers/clk/imx/clk-imx6q.c static void __init imx6q_mmdc_ch1_mask_handshake(void __iomem *ccm_base) · line 333

Quoted 4 lines

drivers/clk/imx/clk-imx6q.c +4 -0

+                              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?

drivers/clk/imx/clk-imx6q.c static void __init imx6q_mmdc_ch1_mask_handshake(void __iomem *ccm_base) · line 348

Quoted 4 lines

drivers/clk/imx/clk-imx6q.c +4 -0

+
+               /* 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

Quoted 9 lines

Review
Quoted 4 lines
+ /* Then configure the top mux before switching back to it
*/
(quoted diff) +2 -0
+               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

Fabio Estevam

reviewer

2016-03-28 18:53 UTC #6 lore raw

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

Hi Akshay,

Quoted 4 lines · Akshay Bhat

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

Quoted 3 lines

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.

Akshay Bhat

2016-03-28 19:26 UTC #7 lore raw

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

Hi Fabio,

Quoted 9 lines · Fabio Estevam

Hi Akshay,
Quoted 4 lines · Akshay Bhat
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?

Quoted 5 lines

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

Fabio Estevam

reviewer

2016-03-28 19:33 UTC #8 lore raw

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

Hi Akshay,

Quoted 5 lines · Akshay Bhat

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.

Quoted 38 lines

Ref:

(quoted diff) +20 -0

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

drivers/clk/imx/clk-imx6q.c +8 -0

+       /* 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]);

Quoted 2 lines

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.

Philipp Zabel

author

2016-03-30 16:18 UTC #9 lore raw

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

Hi Fabio,

Am Montag, den 28.03.2016, 16:33 -0300 schrieb Fabio Estevam:

Quoted 11 lines

Hi Akshay,
Quoted 5 lines · Akshay Bhat
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)?

Quoted 23 lines

Ref:

(quoted diff) +20 -0

+       /*
+        * 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]);

Quoted 2 lines

Quoted 2 lines
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.

Quoted 11 lines

Ref:

drivers/clk/imx/clk-imx6q.c +8 -0

+       /* 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]);

Quoted 4 lines

Quoted 2 lines
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

Fabio Estevam

reviewer

2016-04-05 00:21 UTC #10 lore raw

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

Hi Philipp,

Quoted 6 lines · Philipp Zabel

Quoted 4 lines
Quoted 2 lines
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.

Quoted 1 line

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

They say it works for all combinations.

Quoted 1 line

Is this also true for starting values other than 3?

Yes.

Akshay Bhat

2016-07-08 21:21 UTC #11 lore raw

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

Quoted 14 lines · Fabio Estevam

Hi Philipp,
Quoted 6 lines · Philipp Zabel
Quoted 4 lines
Quoted 2 lines
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.

Quoted 8 lines

Quoted 1 line
(Does this work for current_parent == 4 and new_parent != 0)?
They say it works for all combinations.
Quoted 1 line
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

Philipp Zabel

author

2016-07-11 11:14 UTC #12 lore raw

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

Am Freitag, den 08.07.2016, 17:21 -0400 schrieb Akshay Bhat:

Quoted 7 lines

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

Fabio Estevam

reviewer

2016-03-28 18:48 UTC #13 lore raw

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

In reply to Akshay Bhat

Hi Akshay,

Quoted 3 lines · Akshay Bhat

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

Philipp Zabel

author

2016-03-30 16:12 UTC #14 lore raw

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

Am Montag, den 28.03.2016, 15:48 -0300 schrieb Fabio Estevam:

Quoted 4 lines

Hi Akshay,
On Tue, Mar 1, 2016 at 6:41 PM, Akshay Bhat <[email protected]> wrote:

Quoted 8 lines

Quoted 3 lines
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

Philipp Zabel

author

2016-03-30 16:02 UTC #15 lore raw

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

In reply to Akshay Bhat

Hi Akshay,

Am Dienstag, den 01.03.2016, 16:41 -0500 schrieb Akshay Bhat:
[...]

Quoted 6 lines

drivers/clk/imx/clk-imx6q.c +6 -0

+		/*
+		 * 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",

Quoted 8 lines

drivers/clk/imx/clk-imx6q.c static void __init imx6q_mmdc_ch1_mask_handshake(void __iomem *ccm_base) · line 333
Quoted 4 lines
drivers/clk/imx/clk-imx6q.c +4 -0
+			       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?

Quoted 7 lines

drivers/clk/imx/clk-imx6q.c static void __init imx6q_mmdc_ch1_mask_handshake(void __iomem *ccm_base) · line 348
Quoted 4 lines
drivers/clk/imx/clk-imx6q.c +4 -0
+
+		/* 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).

Quoted 1 line

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

Fabio Estevam

reviewer

2016-04-12 23:28 UTC #16 lore raw

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

Hi Philipp,

Quoted 1 line · Philipp Zabel

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

Philipp Zabel

author

2016-07-11 11:13 UTC #17 lore raw

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

Am Dienstag, den 12.04.2016, 20:28 -0300 schrieb Fabio Estevam:

Quoted 8 lines

Hi Philipp,
Quoted 1 line · Philipp Zabel
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

Akshay Bhat

2016-04-13 14:48 UTC #18 lore raw

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

In reply to Philipp Zabel

Hi Philipp,

Quoted 19 lines · Philipp Zabel

Hi Akshay,
Am Dienstag, den 01.03.2016, 16:41 -0500 schrieb Akshay Bhat:
[...]
Quoted 6 lines
Quoted 6 lines
drivers/clk/imx/clk-imx6q.c +6 -0
+		/*
+		 * 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",
Quoted 8 lines
drivers/clk/imx/clk-imx6q.c static void __init imx6q_mmdc_ch1_mask_handshake(void __iomem *ccm_base) · line 333
Quoted 4 lines
drivers/clk/imx/clk-imx6q.c +4 -0
+			       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?

Quoted 7 lines

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