[PATCH v2 0/5] clk: imx6: work around LDB hang caused by ERR009219

[PATCH v2 0/5] clk: imx6: work around LDB hang caused by ERR009219

[Ahmad Fatoum]

Short version:

barebox always reparents the ldb_di[01] clocks to pll5_video_div.
Due to a hardware erratum, this can trigger a glitch that causes
LDB lock up.

Thus:

1) remove the automatic reparenting, like the kernel now does and
   mark the affected clocks read only, so assigned-clock-parents
   can't affect them
2) parse assigned-clock-parents in the clk-imx6 code as well, but
   there use a convoluted non-glitchy work around to apply them

Long version available alongside v1 here:
https://www.spinics.net/lists/u-boot-v2/msg37658.html;
and in the commit messages of the patches.
 
Changes since v2:
 - reordered/squashed commits after discussion with Sascha and Phillip
 - removed superfluous comment
 - add MUX_READ_ONLY at the correct place in the MUX_READ_ONLY arguments

Except for the last two, the total diff, is exactly the same
as v1's.


Ahmad Fatoum (2):
  clk: imx6: remove quirky clk_set_parent(LDB_diN_sel, pll5_video_div)
  clk: imx6: define an enum for ldb mux inputs

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

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

 drivers/clk/imx/clk-imx6.c | 320 +++++++++++++++++++++++++++++++++++--
 drivers/clk/imx/clk.h      |   8 +
 2 files changed, 312 insertions(+), 16 deletions(-)

-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 1/5] clk: imx6: remove quirky clk_set_parent(LDB_diN_sel, pll5_video_div)

[Ahmad Fatoum]

barebox has inherited the clk_set_parent(ldb_diN_sel, pll5_video_div)
from upstream kernel commit 32f3b8da22 ("ARM i.MX6q: set the LDB serial
clock parent to the video PLL"), where it was enabled for all i.MX6Q
revisions after 1.0. It was applied whenever CONFIG_DRIVER_VIDEO_IMX_IPUV3
was defined.

The kernel removed this reparenting again as a preventive measure
against ERR009219 in 03d576f202 ("clk: imx6: Make the LDB_DI0 and
LDB_DI1 clocks read-only").

As the kernel used the device tree compatible, which is the same
for e.g. i.MX6Solo and DualLite, but barebox queried ANATOP which
lists different CPU types for i.MX6Solo and DualLite and because
i.MX6QuadPlus wasn't supported at first, barebox grew to do
the reparenting on the odd set of:
  - Quad and Dual rev >1.0
  - DualLite
  - Solo rev >1.0
  - QuadPlus and DualPlus rev >1.0

On all of these, except for the QuadPlus and the DualPlus, this
reparenting may glitch the LDB so that it permanently locks up.

By removing the reparenting in this commit, producing this glitch is
avoided, unless the device tree[1] reinstates the old behavior:

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

Follow-up patches will explicitly check for this and do the reparenting
in a glitch-free manner.

As of v2019.03.0, following mainline boards are potentially broken
by this (i.e. they're supported by barebox, are in the list above,
had a LDB enabled and might be defining CONFIG_DRIVER_VIDEO_IMX_IPUV3):
  - imx6qdl-zii-rdu2.dtsi
  - imx6qdl-udoo.dtsi
  - imx6qdl-mba6x.dtsi
  - imx6q-var-custom.dts
  - imx6q-guf-santaro.dts
  - imx6q-embedsky-e9.dtsi

[1]: If barebox is configured to show a boot splash screen, this snippet
     should exist in the barebox device tree. If barebox acts on it, the
     kernel will show following warning:
         ccm: ldb_di0_sel already changed from reset value: 0
         ccm: ldb_di1_sel already changed from reset value: 0
     This warning is safe to ignore.

Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Raphael Poggi <poggi.raph@gmail.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/clk/imx/clk-imx6.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/clk/imx/clk-imx6.c b/drivers/clk/imx/clk-imx6.c
index 35b995dae24e..319485f8521c 100644
--- a/drivers/clk/imx/clk-imx6.c
+++ b/drivers/clk/imx/clk-imx6.c
@@ -340,13 +340,6 @@ static void imx6_add_video_clks(void __iomem *anab, void __iomem *cb)
 	clk_set_parent(clks[IMX6QDL_CLK_IPU1_DI1_PRE_SEL], clks[IMX6QDL_CLK_PLL5_VIDEO_DIV]);
 	clk_set_parent(clks[IMX6QDL_CLK_IPU2_DI0_PRE_SEL], clks[IMX6QDL_CLK_PLL5_VIDEO_DIV]);
 	clk_set_parent(clks[IMX6QDL_CLK_IPU2_DI1_PRE_SEL], clks[IMX6QDL_CLK_PLL5_VIDEO_DIV]);
-
-	if ((imx_silicon_revision() != IMX_CHIP_REV_1_0) ||
-	    cpu_is_mx6dl()) {
-		clk_set_parent(clks[IMX6QDL_CLK_LDB_DI0_SEL], clks[IMX6QDL_CLK_PLL5_VIDEO_DIV]);
-		clk_set_parent(clks[IMX6QDL_CLK_LDB_DI1_SEL], clks[IMX6QDL_CLK_PLL5_VIDEO_DIV]);
-	}
-
 }
 
 static int imx6_ccm_probe(struct device_d *dev)
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 2/5] clk: imx6: Make the LDB_DI0 and LDB_DI1 clocks read-only

[Ahmad Fatoum]

From: Philipp Zabel <p.zabel@pengutronix.de>

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>
Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
Signed-off-by: Shawn Guo <shawnguo@kernel.org>
[afa: ported from Linux kernel commit 03d576f202]
[afa: added exception for i.MX6QP, see kernel commit f4a0a6c309]
[afa: added cpu_has_err009219 helper function]
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/clk/imx/clk-imx6.c | 17 +++++++++++++++--
 drivers/clk/imx/clk.h      |  8 ++++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx6.c b/drivers/clk/imx/clk-imx6.c
index 319485f8521c..01b649ebbd26 100644
--- a/drivers/clk/imx/clk-imx6.c
+++ b/drivers/clk/imx/clk-imx6.c
@@ -64,6 +64,12 @@ static inline int cpu_is_plus(void)
 	return cpu_is_mx6qp() || cpu_is_mx6dp();
 }
 
+/* i.MX6 Quad/Dual/DualLite/Solo are all affected */
+static inline int cpu_has_err009219(void) {
+	return cpu_is_mx6d() || cpu_is_mx6q() ||
+		cpu_is_mx6dl() || cpu_is_mx6s();
+}
+
 static const char *step_sels[] = {
 	"osc",
 	"pll2_pfd2_396m",
@@ -300,8 +306,15 @@ static void imx6_add_video_clks(void __iomem *anab, void __iomem *cb)
 
 	clks[IMX6QDL_CLK_IPU1_SEL]         = imx_clk_mux("ipu1_sel",         cb + 0x3c, 9,  2, ipu_sels,          ARRAY_SIZE(ipu_sels));
 	clks[IMX6QDL_CLK_IPU2_SEL]         = imx_clk_mux("ipu2_sel",         cb + 0x3c, 14, 2, ipu_sels,          ARRAY_SIZE(ipu_sels));
-	clks[IMX6QDL_CLK_LDB_DI0_SEL]      = imx_clk_mux_p("ldb_di0_sel",      cb + 0x2c, 9,  3, ldb_di_sels,       ARRAY_SIZE(ldb_di_sels));
-	clks[IMX6QDL_CLK_LDB_DI1_SEL]      = imx_clk_mux_p("ldb_di1_sel",      cb + 0x2c, 12, 3, ldb_di_sels,       ARRAY_SIZE(ldb_di_sels));
+
+	if (cpu_has_err009219()) {
+		clks[IMX6QDL_CLK_LDB_DI0_SEL] = imx_clk_mux_ldb("ldb_di0_sel",    cb + 0x2c, 9,  3, ldb_di_sels,      ARRAY_SIZE(ldb_di_sels));
+		clks[IMX6QDL_CLK_LDB_DI1_SEL] = imx_clk_mux_ldb("ldb_di1_sel",    cb + 0x2c, 12, 3, ldb_di_sels,       ARRAY_SIZE(ldb_di_sels));
+	} else {
+		clks[IMX6QDL_CLK_LDB_DI0_SEL] = imx_clk_mux_p("ldb_di0_sel",    cb + 0x2c, 9,  3, ldb_di_sels,      ARRAY_SIZE(ldb_di_sels));
+		clks[IMX6QDL_CLK_LDB_DI1_SEL] = imx_clk_mux_p("ldb_di1_sel",    cb + 0x2c, 12, 3, ldb_di_sels,       ARRAY_SIZE(ldb_di_sels));
+	}
+
 	clks[IMX6QDL_CLK_IPU1_DI0_PRE_SEL] = imx_clk_mux_p("ipu1_di0_pre_sel", cb + 0x34, 6,  3, ipu_di_pre_sels,   ARRAY_SIZE(ipu_di_pre_sels));
 	clks[IMX6QDL_CLK_IPU1_DI1_PRE_SEL] = imx_clk_mux_p("ipu1_di1_pre_sel", cb + 0x34, 15, 3, ipu_di_pre_sels,   ARRAY_SIZE(ipu_di_pre_sels));
 	clks[IMX6QDL_CLK_IPU2_DI0_PRE_SEL] = imx_clk_mux_p("ipu2_di0_pre_sel", cb + 0x38, 6,  3, ipu_di_pre_sels,   ARRAY_SIZE(ipu_di_pre_sels));
diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
index 875c76a8b3e4..04286f03f727 100644
--- a/drivers/clk/imx/clk.h
+++ b/drivers/clk/imx/clk.h
@@ -39,6 +39,14 @@ static inline struct clk *imx_clk_divider_table(const char *name,
 				 width, table, 0);
 }
 
+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_mux(name, CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT, reg,
+		       shift, width, parents, num_parents, CLK_MUX_READ_ONLY);
+}
+
+
 static inline struct clk *imx_clk_fixed_factor(const char *name,
 		const char *parent, unsigned int mult, unsigned int div)
 {
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 3/5] clk: imx6: Mask mmdc_ch1 handshake for periph2_sel and mmdc_ch1_axi_podf

[Ahmad Fatoum]

From: Philipp Zabel <p.zabel@pengutronix.de>

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>
Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
Signed-off-by: Shawn Guo <shawnguo@kernel.org>
[afa: ported to barebox from Linux commit f13abeff2c]
[afa: moved call site to where it would've been moved in following commit]
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/clk/imx/clk-imx6.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/clk/imx/clk-imx6.c b/drivers/clk/imx/clk-imx6.c
index 01b649ebbd26..22f33aee03e9 100644
--- a/drivers/clk/imx/clk-imx6.c
+++ b/drivers/clk/imx/clk-imx6.c
@@ -299,6 +299,19 @@ static struct clk_div_table video_div_table[] = {
 	{ /* sentinel */ }
 };
 
+#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(ccm_base + CCM_CCDR);
+	reg |= CCDR_MMDC_CH1_MASK;
+	writel(reg, ccm_base + CCM_CCDR);
+}
+
 static void imx6_add_video_clks(void __iomem *anab, void __iomem *cb)
 {
 	clks[IMX6QDL_CLK_PLL5_POST_DIV] = imx_clk_divider_table("pll5_post_div", "pll5_video", anab + 0xa0, 19, 2, post_div_table);
@@ -307,6 +320,8 @@ static void imx6_add_video_clks(void __iomem *anab, void __iomem *cb)
 	clks[IMX6QDL_CLK_IPU1_SEL]         = imx_clk_mux("ipu1_sel",         cb + 0x3c, 9,  2, ipu_sels,          ARRAY_SIZE(ipu_sels));
 	clks[IMX6QDL_CLK_IPU2_SEL]         = imx_clk_mux("ipu2_sel",         cb + 0x3c, 14, 2, ipu_sels,          ARRAY_SIZE(ipu_sels));
 
+	imx6q_mmdc_ch1_mask_handshake(cb);
+
 	if (cpu_has_err009219()) {
 		clks[IMX6QDL_CLK_LDB_DI0_SEL] = imx_clk_mux_ldb("ldb_di0_sel",    cb + 0x2c, 9,  3, ldb_di_sels,      ARRAY_SIZE(ldb_di_sels));
 		clks[IMX6QDL_CLK_LDB_DI1_SEL] = imx_clk_mux_ldb("ldb_di1_sel",    cb + 0x2c, 12, 3, ldb_di_sels,       ARRAY_SIZE(ldb_di_sels));
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 4/5] clk: imx6: Fix procedure to switch the parent of LDB_DI_CLK

[Ahmad Fatoum]

From: Fabio Estevam <fabio.estevam@nxp.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>;
        };

The issue is explained in detail in EB821 ("LDB Clock Switch Procedure &
i.MX6 Asynchronous Clock Switching Guidelines") [1].

[1] http://www.nxp.com/files/32bit/doc/eng_bulletin/EB821.pdf

Signed-off-by: Ranjani Vaidyanathan <Ranjani.Vaidyanathan@nxp.com>
Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: Akshay Bhat <akshay.bhat@timesys.com>
Tested-by Joshua Clayton <stillcompiling@gmail.com>
Tested-by: Charles Kang <Charles.Kang@advantech.com.tw>
Signed-off-by: Shawn Guo <shawnguo@kernel.org>
[afa: ported to barebox from Linux commit 5d283b0838]
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/clk/imx/clk-imx6.c | 264 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 261 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/imx/clk-imx6.c b/drivers/clk/imx/clk-imx6.c
index 22f33aee03e9..69586f04a21f 100644
--- a/drivers/clk/imx/clk-imx6.c
+++ b/drivers/clk/imx/clk-imx6.c
@@ -64,6 +64,12 @@ static inline int cpu_is_plus(void)
 	return cpu_is_mx6qp() || cpu_is_mx6dp();
 }
 
+/* Audio/Video PLL post dividers don't work on i.MX6q revision 1.0 */
+static inline int cpu_has_working_video_pll_post_div(void) {
+	return !((cpu_is_mx6q() || cpu_is_mx6d()) &&
+		 imx_silicon_revision() == IMX_CHIP_REV_1_0);
+}
+
 /* i.MX6 Quad/Dual/DualLite/Solo are all affected */
 static inline int cpu_has_err009219(void) {
 	return cpu_is_mx6d() || cpu_is_mx6q() ||
@@ -299,9 +305,89 @@ static struct clk_div_table video_div_table[] = {
 	{ /* sentinel */ }
 };
 
+static int ldb_di_sel_by_clock_id(int clock_id)
+{
+	switch (clock_id) {
+	case IMX6QDL_CLK_PLL5_VIDEO_DIV:
+		if (!cpu_has_working_video_pll_post_div())
+			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 CCSR_PLL3_SW_CLK_SEL		BIT(0)
 
-#define CCDR_MMDC_CH1_MASK	BIT(16)
+#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)
 {
@@ -312,7 +398,170 @@ static void __init imx6q_mmdc_ch1_mask_handshake(void __iomem *ccm_base)
 	writel(reg, ccm_base + CCM_CCDR);
 }
 
-static void imx6_add_video_clks(void __iomem *anab, void __iomem *cb)
+/*
+ * 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(clks[IMX6QDL_CLK_PERIPH2_CLK2_SEL],
+		       clks[IMX6QDL_CLK_PLL3_USB_OTG]);
+
+	/*
+	 * Handshake with mmdc_ch1 module must be masked when changing
+	 * periph2_clk_sel.
+	 */
+	clk_set_parent(clks[IMX6QDL_CLK_PERIPH2], clks[IMX6QDL_CLK_PERIPH2_CLK2]);
+
+	/* Disable pll3_sw_clk by selecting the bypass clock source */
+	reg = readl(ccm_base + CCM_CCSR);
+	reg |= CCSR_PLL3_SW_CLK_SEL;
+	writel(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(ccm_base + CCM_CCSR);
+	reg &= ~CCSR_PLL3_SW_CLK_SEL;
+	writel(reg, ccm_base + CCM_CCSR);
+
+	clk_set_parent(clks[IMX6QDL_CLK_PERIPH2], clks[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 at 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(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(clks[IMX6QDL_CLK_PERIPH_PRE]) ==
+		     clks[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;
+		}
+
+		/* 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(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(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(anatop_base + CCM_ANALOG_PFD_528);
+	/* Cannot gate PFD2 if pll2_pfd2_396m is the parent of MMDC clock */
+	if (clk_get_parent(clks[IMX6QDL_CLK_PERIPH_PRE]) ==
+	    clks[IMX6QDL_CLK_PLL2_PFD2_396M])
+		reg |= PFD0_CLKGATE | PFD1_CLKGATE;
+	else
+		reg |= PFD0_CLKGATE | PFD1_CLKGATE | PFD2_CLKGATE;
+	writel(reg, anatop_base + CCM_ANALOG_PFD_528);
+
+	/* Make sure PLL3 PFDs 0-3 are gated */
+	reg = readl(anatop_base + CCM_ANALOG_PFD_480);
+	reg |= PFD0_CLKGATE | PFD1_CLKGATE | PFD2_CLKGATE | PFD3_CLKGATE;
+	writel(reg, anatop_base + CCM_ANALOG_PFD_480);
+
+	/* Make sure PLL5 is disabled */
+	reg = readl(anatop_base + CCM_ANALOG_PLL_VIDEO);
+	reg &= ~PLL_ENABLE;
+	writel(reg, anatop_base + CCM_ANALOG_PLL_VIDEO);
+}
+
+static void imx6_add_video_clks(void __iomem *anab, void __iomem *cb, struct device_node *ccm_np)
 {
 	clks[IMX6QDL_CLK_PLL5_POST_DIV] = imx_clk_divider_table("pll5_post_div", "pll5_video", anab + 0xa0, 19, 2, post_div_table);
 	clks[IMX6QDL_CLK_PLL5_VIDEO_DIV] = imx_clk_divider_table("pll5_video_div", "pll5_post_div", anab + 0x170, 30, 2, video_div_table);
@@ -320,9 +569,18 @@ static void imx6_add_video_clks(void __iomem *anab, void __iomem *cb)
 	clks[IMX6QDL_CLK_IPU1_SEL]         = imx_clk_mux("ipu1_sel",         cb + 0x3c, 9,  2, ipu_sels,          ARRAY_SIZE(ipu_sels));
 	clks[IMX6QDL_CLK_IPU2_SEL]         = imx_clk_mux("ipu2_sel",         cb + 0x3c, 14, 2, ipu_sels,          ARRAY_SIZE(ipu_sels));
 
+	disable_anatop_clocks(anab);
+
 	imx6q_mmdc_ch1_mask_handshake(cb);
 
 	if (cpu_has_err009219()) {
+		/*
+		 * The LDB_DI0/1_SEL muxes should be read-only due to a hardware
+		 * bug. Set the muxes to the requested values before registering the
+		 * ldb_di_sel clocks.
+		 */
+		init_ldb_clks(ccm_np, cb);
+
 		clks[IMX6QDL_CLK_LDB_DI0_SEL] = imx_clk_mux_ldb("ldb_di0_sel",    cb + 0x2c, 9,  3, ldb_di_sels,      ARRAY_SIZE(ldb_di_sels));
 		clks[IMX6QDL_CLK_LDB_DI1_SEL] = imx_clk_mux_ldb("ldb_di1_sel",    cb + 0x2c, 12, 3, ldb_di_sels,       ARRAY_SIZE(ldb_di_sels));
 	} else {
@@ -527,7 +785,7 @@ static int imx6_ccm_probe(struct device_d *dev)
 	clkdev_add_physbase(clks[IMX6QDL_CLK_IPG], MX6_OCOTP_BASE_ADDR, NULL);
 
 	if (IS_ENABLED(CONFIG_DRIVER_VIDEO_IMX_IPUV3))
-		imx6_add_video_clks(anatop_base, ccm_base);
+		imx6_add_video_clks(anatop_base, ccm_base, dev->device_node);
 
 	writel(0xffffffff, ccm_base + CCGR0);
 	writel(0xf0ffffff, ccm_base + CCGR1); /* gate GPU3D, GPU2D */
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 5/5] clk: imx6: define an enum for ldb mux inputs

[Ahmad Fatoum]

For better readability should this code be reviewed
in future, replace the hardcoded input numbers
with an enum.

This is just a cosmetic change and was verified
to not affect clk-imx6.o.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/clk/imx/clk-imx6.c | 39 +++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/clk/imx/clk-imx6.c b/drivers/clk/imx/clk-imx6.c
index 69586f04a21f..fd0a51f1dcda 100644
--- a/drivers/clk/imx/clk-imx6.c
+++ b/drivers/clk/imx/clk-imx6.c
@@ -213,12 +213,20 @@ static const char *ipu_sels[] = {
 	"pll3_pfd1_540m",
 };
 
+enum ldb_di_sel { /* for use in init_ldb_clks */
+	LDB_DI_SEL_PLL5_VIDEO_DIV	= 0,
+	LDB_DI_SEL_PLL2_PFD0_352M	= 1,
+	LDB_DI_SEL_PLL2_PFD2_396M	= 2,
+	LDB_DI_SEL_MMDC_CH1_AXI		= 3,
+	LDB_DI_SEL_PLL3_USB_OTG		= 4,
+};
+
 static const char *ldb_di_sels[] = {
-	"pll5_video_div",
-	"pll2_pfd0_352m",
-	"pll2_pfd2_396m",
-	"mmdc_ch1_axi_podf",
-	"pll3_usb_otg",
+	[LDB_DI_SEL_PLL5_VIDEO_DIV]	= "pll5_video_div",
+	[LDB_DI_SEL_PLL2_PFD0_352M]	= "pll2_pfd0_352m",
+	[LDB_DI_SEL_PLL2_PFD2_396M]	= "pll2_pfd2_396m",
+	[LDB_DI_SEL_MMDC_CH1_AXI]	= "mmdc_ch1_axi_podf",
+	[LDB_DI_SEL_PLL3_USB_OTG]	= "pll3_usb_otg",
 };
 
 static const char *ipu_di_pre_sels[] = {
@@ -311,23 +319,23 @@ static int ldb_di_sel_by_clock_id(int clock_id)
 	case IMX6QDL_CLK_PLL5_VIDEO_DIV:
 		if (!cpu_has_working_video_pll_post_div())
 			return -ENOENT;
-		return 0;
+		return LDB_DI_SEL_PLL5_VIDEO_DIV;
 	case IMX6QDL_CLK_PLL2_PFD0_352M:
-		return 1;
+		return LDB_DI_SEL_PLL2_PFD0_352M;
 	case IMX6QDL_CLK_PLL2_PFD2_396M:
-		return 2;
+		return LDB_DI_SEL_PLL2_PFD2_396M;
 	case IMX6QDL_CLK_MMDC_CH1_AXI:
-		return 3;
+		return LDB_DI_SEL_MMDC_CH1_AXI;
 	case IMX6QDL_CLK_PLL3_USB_OTG:
-		return 4;
+		return LDB_DI_SEL_PLL3_USB_OTG;
 	default:
 		return -ENOENT;
 	}
 }
 
 static void of_assigned_ldb_sels(struct device_node *node,
-				 unsigned int *ldb_di0_sel,
-				 unsigned int *ldb_di1_sel)
+				 enum ldb_di_sel *ldb_di0_sel,
+				 enum ldb_di_sel *ldb_di1_sel)
 {
 	struct of_phandle_args clkspec;
 	int index, rc, num_parents;
@@ -466,7 +474,7 @@ static void mmdc_ch1_reenable(void __iomem *ccm_base)
 static void init_ldb_clks(struct device_node *np, void __iomem *ccm_base)
 {
 	unsigned int reg;
-	unsigned int sel[2][4];
+	enum ldb_di_sel sel[2][4];
 	int i;
 
 	reg = readl(ccm_base + CCM_CS2CDR);
@@ -480,7 +488,7 @@ static void init_ldb_clks(struct device_node *np, void __iomem *ccm_base)
 
 	for (i = 0; i < 2; i++) {
 		/* Warn if a glitch might have been introduced already */
-		if (sel[i][0] != 3) {
+		if (sel[i][0] != LDB_DI_SEL_MMDC_CH1_AXI) {
 			pr_warn("ccm: ldb_di%d_sel already changed from reset value: %d\n",
 				i, sel[i][0]);
 		}
@@ -489,7 +497,8 @@ static void init_ldb_clks(struct device_node *np, void __iomem *ccm_base)
 			continue;
 
 		/* Only switch to or from pll2_pfd2_396m if it is disabled */
-		if ((sel[i][0] == 2 || sel[i][3] == 2) &&
+		if ((sel[i][0] == LDB_DI_SEL_PLL2_PFD2_396M ||
+		     sel[i][3] == LDB_DI_SEL_PLL2_PFD2_396M) &&
 		    (clk_get_parent(clks[IMX6QDL_CLK_PERIPH_PRE]) ==
 		     clks[IMX6QDL_CLK_PLL2_PFD2_396M])) {
 			pr_err("ccm: ldb_di%d_sel: couldn't disable pll2_pfd2_396m\n",
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/5] clk: imx6: remove quirky clk_set_parent(LDB_diN_sel, pll5_video_div)

[Philipp Zabel]

On Tue, 2019-05-21 at 17:56 +0200, Ahmad Fatoum wrote:
> barebox has inherited the clk_set_parent(ldb_diN_sel, pll5_video_div)
> from upstream kernel commit 32f3b8da22 ("ARM i.MX6q: set the LDB serial
> clock parent to the video PLL"), where it was enabled for all i.MX6Q
> revisions after 1.0. It was applied whenever CONFIG_DRIVER_VIDEO_IMX_IPUV3
> was defined.
> 
> The kernel removed this reparenting again as a preventive measure
> against ERR009219 in 03d576f202 ("clk: imx6: Make the LDB_DI0 and
> LDB_DI1 clocks read-only").
> 
> As the kernel used the device tree compatible, which is the same
> for e.g. i.MX6Solo and DualLite, but barebox queried ANATOP which
> lists different CPU types for i.MX6Solo and DualLite and because
> i.MX6QuadPlus wasn't supported at first, barebox grew to do
> the reparenting on the odd set of:
>   - Quad and Dual rev >1.0
>   - DualLite
>   - Solo rev >1.0
>   - QuadPlus and DualPlus rev >1.0
> 
> On all of these, except for the QuadPlus and the DualPlus, this
> reparenting may glitch the LDB so that it permanently locks up.
> 
> By removing the reparenting in this commit, producing this glitch is
> avoided, unless the device tree[1] reinstates the old behavior:
> 
> 	&clks {
> 		assigned-clocks = <&clks IMX6QDL_CLK_LDB_DI0_SEL>,
> 				  <&clks IMX6QDL_CLK_LDB_DI1_SEL>;
> 		assigned-clock-parents = <&clks IMX6QDL_CLK_PLL5_VIDEO_DIV>,
> 					 <&clks IMX6QDL_CLK_PLL5_VIDEO_DIV>;
> 	};
> 
> Follow-up patches will explicitly check for this and do the reparenting
> in a glitch-free manner.
> 
> As of v2019.03.0, following mainline boards are potentially broken
        ^^^^^^^^^^
This Barebox version is out of date.

> by this (i.e. they're supported by barebox, are in the list above,
> had a LDB enabled and might be defining CONFIG_DRIVER_VIDEO_IMX_IPUV3):
>   - imx6qdl-zii-rdu2.dtsi
>   - imx6qdl-udoo.dtsi
>   - imx6qdl-mba6x.dtsi
>   - imx6q-var-custom.dts
>   - imx6q-guf-santaro.dts
>   - imx6q-embedsky-e9.dtsi

Besides RDU2 and Santaro these are all development boards.
UDOO, E9, and MBa6x have monitor connectors, so using PLL5 for LVDS may
be the wrong thing anyway, depending on the connected panel, and on
whether the HDMI/DVI connector is going to be used.

For RDU2 and Santaro I think we need the above DT change. Both don't
have HDMI connectors, which makes PLL5 the correct clock to use, and I
can't see any other place where LDB_DI clocks are switched to it.

> [1]: If barebox is configured to show a boot splash screen, this snippet
>      should exist in the barebox device tree. If barebox acts on it, the
>      kernel will show following warning:
>          ccm: ldb_di0_sel already changed from reset value: 0
>          ccm: ldb_di1_sel already changed from reset value: 0
>      This warning is safe to ignore.
> 
> Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Raphael Poggi <poggi.raph@gmail.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Apart from that,
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 2/5] clk: imx6: Make the LDB_DI0 and LDB_DI1 clocks read-only

[Philipp Zabel]

On Tue, 2019-05-21 at 17:56 +0200, Ahmad Fatoum wrote:
> From: Philipp Zabel <p.zabel@pengutronix.de>
> 
> 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>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> Signed-off-by: Shawn Guo <shawnguo@kernel.org>
> [afa: ported from Linux kernel commit 03d576f202]
> [afa: added exception for i.MX6QP, see kernel commit f4a0a6c309]
> [afa: added cpu_has_err009219 helper function]
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 3/5] clk: imx6: Mask mmdc_ch1 handshake for periph2_sel and mmdc_ch1_axi_podf

[Philipp Zabel]

On Tue, 2019-05-21 at 17:56 +0200, Ahmad Fatoum wrote:
> From: Philipp Zabel <p.zabel@pengutronix.de>
> 
> 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>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> Signed-off-by: Shawn Guo <shawnguo@kernel.org>
> [afa: ported to barebox from Linux commit f13abeff2c]
> [afa: moved call site to where it would've been moved in following commit]
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 4/5] clk: imx6: Fix procedure to switch the parent of LDB_DI_CLK

[Philipp Zabel]

On Tue, 2019-05-21 at 17:56 +0200, Ahmad Fatoum wrote:
> From: Fabio Estevam <fabio.estevam@nxp.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>;
>         };
> 
> The issue is explained in detail in EB821 ("LDB Clock Switch Procedure &
> i.MX6 Asynchronous Clock Switching Guidelines") [1].
> 
> [1] http://www.nxp.com/files/32bit/doc/eng_bulletin/EB821.pdf
> 
> Signed-off-by: Ranjani Vaidyanathan <Ranjani.Vaidyanathan@nxp.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> Reviewed-by: Akshay Bhat <akshay.bhat@timesys.com>
> Tested-by Joshua Clayton <stillcompiling@gmail.com>
> Tested-by: Charles Kang <Charles.Kang@advantech.com.tw>
> Signed-off-by: Shawn Guo <shawnguo@kernel.org>
> [afa: ported to barebox from Linux commit 5d283b0838]
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 5/5] clk: imx6: define an enum for ldb mux inputs

[Philipp Zabel]

On Tue, 2019-05-21 at 17:56 +0200, Ahmad Fatoum wrote:
> For better readability should this code be reviewed
> in future, replace the hardcoded input numbers
> with an enum.
> 
> This is just a cosmetic change and was verified
> to not affect clk-imx6.o.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Is this something that should be proposed for the Linux kernel as well?

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/5] clk: imx6: remove quirky clk_set_parent(LDB_diN_sel, pll5_video_div)

[Ahmad Fatoum]

On 23/5/19 12:01, Philipp Zabel wrote:
> On Tue, 2019-05-21 at 17:56 +0200, Ahmad Fatoum wrote:
>> barebox has inherited the clk_set_parent(ldb_diN_sel, pll5_video_div)
>> from upstream kernel commit 32f3b8da22 ("ARM i.MX6q: set the LDB serial
>> clock parent to the video PLL"), where it was enabled for all i.MX6Q
>> revisions after 1.0. It was applied whenever CONFIG_DRIVER_VIDEO_IMX_IPUV3
>> was defined.
>>
>> The kernel removed this reparenting again as a preventive measure
>> against ERR009219 in 03d576f202 ("clk: imx6: Make the LDB_DI0 and
>> LDB_DI1 clocks read-only").
>>
>> As the kernel used the device tree compatible, which is the same
>> for e.g. i.MX6Solo and DualLite, but barebox queried ANATOP which
>> lists different CPU types for i.MX6Solo and DualLite and because
>> i.MX6QuadPlus wasn't supported at first, barebox grew to do
>> the reparenting on the odd set of:
>>   - Quad and Dual rev >1.0
>>   - DualLite
>>   - Solo rev >1.0
>>   - QuadPlus and DualPlus rev >1.0
>>
>> On all of these, except for the QuadPlus and the DualPlus, this
>> reparenting may glitch the LDB so that it permanently locks up.
>>
>> By removing the reparenting in this commit, producing this glitch is
>> avoided, unless the device tree[1] reinstates the old behavior:
>>
>> 	&clks {
>> 		assigned-clocks = <&clks IMX6QDL_CLK_LDB_DI0_SEL>,
>> 				  <&clks IMX6QDL_CLK_LDB_DI1_SEL>;
>> 		assigned-clock-parents = <&clks IMX6QDL_CLK_PLL5_VIDEO_DIV>,
>> 					 <&clks IMX6QDL_CLK_PLL5_VIDEO_DIV>;
>> 	};
>>
>> Follow-up patches will explicitly check for this and do the reparenting
>> in a glitch-free manner.
>>
>> As of v2019.03.0, following mainline boards are potentially broken
>         ^^^^^^^^^^
> This Barebox version is out of date.

git diff --name-status v2019.03.0..upstream/next arch/arm/dts | grep ^A.*imx6

reports only one i.MX6UL board, so the list is still accurate,
just the header isn't.


> 
>> by this (i.e. they're supported by barebox, are in the list above,
>> had a LDB enabled and might be defining CONFIG_DRIVER_VIDEO_IMX_IPUV3):
>>   - imx6qdl-zii-rdu2.dtsi
>>   - imx6qdl-udoo.dtsi
>>   - imx6qdl-mba6x.dtsi
>>   - imx6q-var-custom.dts
>>   - imx6q-guf-santaro.dts
>>   - imx6q-embedsky-e9.dtsi
> 
> Besides RDU2 and Santaro these are all development boards.
> UDOO, E9, and MBa6x have monitor connectors, so using PLL5 for LVDS may
> be the wrong thing anyway, depending on the connected panel, and on
> whether the HDMI/DVI connector is going to be used.
> 
> For RDU2 and Santaro I think we need the above DT change. Both don't
> have HDMI connectors, which makes PLL5 the correct clock to use, and I
> can't see any other place where LDB_DI clocks are switched to it.

I see. Andrey and Sascha added those, so I'll leave it to them to decide.
 

> 
>> [1]: If barebox is configured to show a boot splash screen, this snippet
>>      should exist in the barebox device tree. If barebox acts on it, the
>>      kernel will show following warning:
>>          ccm: ldb_di0_sel already changed from reset value: 0
>>          ccm: ldb_di1_sel already changed from reset value: 0
>>      This warning is safe to ignore.
>>
>> Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
>> Cc: Raphael Poggi <poggi.raph@gmail.com>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>> Cc: Lucas Stach <l.stach@pengutronix.de>
>> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 
> Apart from that,
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

Thanks for the review!

Cheers
Ahmad

> 
> regards
> Philipp
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 5/5] clk: imx6: define an enum for ldb mux inputs

[Ahmad Fatoum]

On 23/5/19 12:03, Philipp Zabel wrote:
> On Tue, 2019-05-21 at 17:56 +0200, Ahmad Fatoum wrote:
>> For better readability should this code be reviewed
>> in future, replace the hardcoded input numbers
>> with an enum.
>>
>> This is just a cosmetic change and was verified
>> to not affect clk-imx6.o.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 
> Is this something that should be proposed for the Linux kernel as well?

I think so. Will add it to the backlog..

> 
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> regards
> Philipp
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/5] clk: imx6: remove quirky clk_set_parent(LDB_diN_sel, pll5_video_div)

[Andrey Smirnov]

On Tue, May 21, 2019 at 8:56 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> barebox has inherited the clk_set_parent(ldb_diN_sel, pll5_video_div)
> from upstream kernel commit 32f3b8da22 ("ARM i.MX6q: set the LDB serial
> clock parent to the video PLL"), where it was enabled for all i.MX6Q
> revisions after 1.0. It was applied whenever CONFIG_DRIVER_VIDEO_IMX_IPUV3
> was defined.
>
> The kernel removed this reparenting again as a preventive measure
> against ERR009219 in 03d576f202 ("clk: imx6: Make the LDB_DI0 and
> LDB_DI1 clocks read-only").
>
> As the kernel used the device tree compatible, which is the same
> for e.g. i.MX6Solo and DualLite, but barebox queried ANATOP which
> lists different CPU types for i.MX6Solo and DualLite and because
> i.MX6QuadPlus wasn't supported at first, barebox grew to do
> the reparenting on the odd set of:
>   - Quad and Dual rev >1.0
>   - DualLite
>   - Solo rev >1.0
>   - QuadPlus and DualPlus rev >1.0
>
> On all of these, except for the QuadPlus and the DualPlus, this
> reparenting may glitch the LDB so that it permanently locks up.
>
> By removing the reparenting in this commit, producing this glitch is
> avoided, unless the device tree[1] reinstates the old behavior:
>
>         &clks {
>                 assigned-clocks = <&clks IMX6QDL_CLK_LDB_DI0_SEL>,
>                                   <&clks IMX6QDL_CLK_LDB_DI1_SEL>;
>                 assigned-clock-parents = <&clks IMX6QDL_CLK_PLL5_VIDEO_DIV>,
>                                          <&clks IMX6QDL_CLK_PLL5_VIDEO_DIV>;
>         };
>
> Follow-up patches will explicitly check for this and do the reparenting
> in a glitch-free manner.
>
> As of v2019.03.0, following mainline boards are potentially broken
> by this (i.e. they're supported by barebox, are in the list above,
> had a LDB enabled and might be defining CONFIG_DRIVER_VIDEO_IMX_IPUV3):
>   - imx6qdl-zii-rdu2.dtsi

This particular patch does break a non 6Q+ version of RDU2,  but the
follow up ones in the series fix it, so it seems that no action is
really necessary on my part.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 4/5] clk: imx6: Fix procedure to switch the parent of LDB_DI_CLK

[Andrey Smirnov]

On Tue, May 21, 2019 at 8:56 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> From: Fabio Estevam <fabio.estevam@nxp.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>;
>         };
>
> The issue is explained in detail in EB821 ("LDB Clock Switch Procedure &
> i.MX6 Asynchronous Clock Switching Guidelines") [1].
>
> [1] http://www.nxp.com/files/32bit/doc/eng_bulletin/EB821.pdf
>
> Signed-off-by: Ranjani Vaidyanathan <Ranjani.Vaidyanathan@nxp.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> Reviewed-by: Akshay Bhat <akshay.bhat@timesys.com>
> Tested-by Joshua Clayton <stillcompiling@gmail.com>
> Tested-by: Charles Kang <Charles.Kang@advantech.com.tw>
> Signed-off-by: Shawn Guo <shawnguo@kernel.org>
> [afa: ported to barebox from Linux commit 5d283b0838]
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/clk/imx/clk-imx6.c | 264 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 261 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/imx/clk-imx6.c b/drivers/clk/imx/clk-imx6.c
> index 22f33aee03e9..69586f04a21f 100644
> --- a/drivers/clk/imx/clk-imx6.c
> +++ b/drivers/clk/imx/clk-imx6.c
> @@ -64,6 +64,12 @@ static inline int cpu_is_plus(void)
>         return cpu_is_mx6qp() || cpu_is_mx6dp();
>  }
>
> +/* Audio/Video PLL post dividers don't work on i.MX6q revision 1.0 */
> +static inline int cpu_has_working_video_pll_post_div(void) {
> +       return !((cpu_is_mx6q() || cpu_is_mx6d()) &&
> +                imx_silicon_revision() == IMX_CHIP_REV_1_0);
> +}
> +
>  /* i.MX6 Quad/Dual/DualLite/Solo are all affected */
>  static inline int cpu_has_err009219(void) {
>         return cpu_is_mx6d() || cpu_is_mx6q() ||
> @@ -299,9 +305,89 @@ static struct clk_div_table video_div_table[] = {
>         { /* sentinel */ }
>  };
>
> +static int ldb_di_sel_by_clock_id(int clock_id)
> +{
> +       switch (clock_id) {
> +       case IMX6QDL_CLK_PLL5_VIDEO_DIV:
> +               if (!cpu_has_working_video_pll_post_div())
> +                       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 CCSR_PLL3_SW_CLK_SEL           BIT(0)
>
> -#define CCDR_MMDC_CH1_MASK     BIT(16)
> +#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)
>  {
> @@ -312,7 +398,170 @@ static void __init imx6q_mmdc_ch1_mask_handshake(void __iomem *ccm_base)
>         writel(reg, ccm_base + CCM_CCDR);
>  }
>
> -static void imx6_add_video_clks(void __iomem *anab, void __iomem *cb)
> +/*
> + * 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(clks[IMX6QDL_CLK_PERIPH2_CLK2_SEL],
> +                      clks[IMX6QDL_CLK_PLL3_USB_OTG]);
> +
> +       /*
> +        * Handshake with mmdc_ch1 module must be masked when changing
> +        * periph2_clk_sel.
> +        */
> +       clk_set_parent(clks[IMX6QDL_CLK_PERIPH2], clks[IMX6QDL_CLK_PERIPH2_CLK2]);
> +
> +       /* Disable pll3_sw_clk by selecting the bypass clock source */
> +       reg = readl(ccm_base + CCM_CCSR);
> +       reg |= CCSR_PLL3_SW_CLK_SEL;
> +       writel(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(ccm_base + CCM_CCSR);
> +       reg &= ~CCSR_PLL3_SW_CLK_SEL;
> +       writel(reg, ccm_base + CCM_CCSR);
> +
> +       clk_set_parent(clks[IMX6QDL_CLK_PERIPH2], clks[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 at 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(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;

Nit: FIELD_GET()/GENMASK() to avoid the explicit shifting?

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 0/5] clk: imx6: work around LDB hang caused by ERR009219

[Andrey Smirnov]

On Tue, May 21, 2019 at 8:56 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> Short version:
>
> barebox always reparents the ldb_di[01] clocks to pll5_video_div.
> Due to a hardware erratum, this can trigger a glitch that causes
> LDB lock up.
>
> Thus:
>
> 1) remove the automatic reparenting, like the kernel now does and
>    mark the affected clocks read only, so assigned-clock-parents
>    can't affect them
> 2) parse assigned-clock-parents in the clk-imx6 code as well, but
>    there use a convoluted non-glitchy work around to apply them
>
> Long version available alongside v1 here:
> https://www.spinics.net/lists/u-boot-v2/msg37658.html;
> and in the commit messages of the patches.
>
> Changes since v2:
>  - reordered/squashed commits after discussion with Sascha and Phillip
>  - removed superfluous comment
>  - add MUX_READ_ONLY at the correct place in the MUX_READ_ONLY arguments
>
> Except for the last two, the total diff, is exactly the same
> as v1's.
>
>
> Ahmad Fatoum (2):
>   clk: imx6: remove quirky clk_set_parent(LDB_diN_sel, pll5_video_div)
>   clk: imx6: define an enum for ldb mux inputs
>
> Fabio Estevam (1):
>   clk: imx6: Fix procedure to switch the parent of LDB_DI_CLK
>
> Philipp Zabel (2):
>   clk: imx6: Make the LDB_DI0 and LDB_DI1 clocks read-only
>   clk: imx6: Mask mmdc_ch1 handshake for periph2_sel and
>     mmdc_ch1_axi_podf
>
>  drivers/clk/imx/clk-imx6.c | 320 +++++++++++++++++++++++++++++++++++--
>  drivers/clk/imx/clk.h      |   8 +
>  2 files changed, 312 insertions(+), 16 deletions(-)
>

Tested this on non 6Q+ RDU2, seems to work as expected when the whole
series is applied.

Tested-by: Andrey Smirnov <andrew.smirnov@gmail.com>

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/5] clk: imx6: remove quirky clk_set_parent(LDB_diN_sel, pll5_video_div)

[Ahmad Fatoum]

Hello Andrey,

On 26/5/19 23:55, Andrey Smirnov wrote:
> On Tue, May 21, 2019 at 8:56 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>>
> This particular patch does break a non 6Q+ version of RDU2,  but the
> follow up ones in the series fix it, so it seems that no action is
> really necessary on my part.

The reparenting removed in this patch isn't reinstated by the rest of the series.
They merely apply parentage expressed in the device tree in a glitch-free manner.

As both barebox and kernel imx6qdl-zii-rdu2.dtsi lack the relevant
assigned-clock-parents snippet, I am not sure what it is this patch broke that the
follow-up ones fixed?

Generally, affected boards have been broken since day 1, because the LVDS output
would've locked up every blue moon or so. If this patch breaks them, they're just
more reliably broken. :-)


Cheers
Ahmad

> 
> Thanks,
> Andrey Smirnov
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/5] clk: imx6: remove quirky clk_set_parent(LDB_diN_sel, pll5_video_div)

[Andrey Smirnov]

On Sun, May 26, 2019 at 11:29 PM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> Hello Andrey,
>
> On 26/5/19 23:55, Andrey Smirnov wrote:
> > On Tue, May 21, 2019 at 8:56 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> >>
> > This particular patch does break a non 6Q+ version of RDU2,  but the
> > follow up ones in the series fix it, so it seems that no action is
> > really necessary on my part.
>
> The reparenting removed in this patch isn't reinstated by the rest of the series.
> They merely apply parentage expressed in the device tree in a glitch-free manner.
>
> As both barebox and kernel imx6qdl-zii-rdu2.dtsi lack the relevant
> assigned-clock-parents snippet, I am not sure what it is this patch broke that the
> follow-up ones fixed?

Not sure, will investigate.

>
> Generally, affected boards have been broken since day 1, because the LVDS output
> would've locked up every blue moon or so. If this patch breaks them, they're just
> more reliably broken. :-)
>

There's a world of difference between not working every once in a blue
moon and not working from a first boot.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 2/5] clk: imx6: Make the LDB_DI0 and LDB_DI1 clocks read-only

[Andrey Smirnov]

On Tue, May 21, 2019 at 8:56 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> From: Philipp Zabel <p.zabel@pengutronix.de>
>
> 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>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> Signed-off-by: Shawn Guo <shawnguo@kernel.org>
> [afa: ported from Linux kernel commit 03d576f202]
> [afa: added exception for i.MX6QP, see kernel commit f4a0a6c309]
> [afa: added cpu_has_err009219 helper function]
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/clk/imx/clk-imx6.c | 17 +++++++++++++++--
>  drivers/clk/imx/clk.h      |  8 ++++++++
>  2 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/imx/clk-imx6.c b/drivers/clk/imx/clk-imx6.c
> index 319485f8521c..01b649ebbd26 100644
> --- a/drivers/clk/imx/clk-imx6.c
> +++ b/drivers/clk/imx/clk-imx6.c
> @@ -64,6 +64,12 @@ static inline int cpu_is_plus(void)
>         return cpu_is_mx6qp() || cpu_is_mx6dp();
>  }
>
> +/* i.MX6 Quad/Dual/DualLite/Solo are all affected */
> +static inline int cpu_has_err009219(void) {

That { should really be on a separate line.

> +       return cpu_is_mx6d() || cpu_is_mx6q() ||
> +               cpu_is_mx6dl() || cpu_is_mx6s();
> +}

Hmm, imx_init() is executed at "postcore" and this function will be
called at "core" init-level. All of the cpu_is_mx6*() functions will
call cpu_is_mx6() which won't work right until imx_init() is called.
Are you sure this is working as intended? I think cpu_mx6_is_imx6*()
functions should be used instead. That's what I had to do on my board
at least. It looks that there's other unrelated code in this file that
might have this problem as well.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 4/5] clk: imx6: Fix procedure to switch the parent of LDB_DI_CLK

[Andrey Smirnov]

On Tue, May 21, 2019 at 8:56 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> From: Fabio Estevam <fabio.estevam@nxp.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>;
>         };
>
> The issue is explained in detail in EB821 ("LDB Clock Switch Procedure &
> i.MX6 Asynchronous Clock Switching Guidelines") [1].
>
> [1] http://www.nxp.com/files/32bit/doc/eng_bulletin/EB821.pdf
>
> Signed-off-by: Ranjani Vaidyanathan <Ranjani.Vaidyanathan@nxp.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> Reviewed-by: Akshay Bhat <akshay.bhat@timesys.com>
> Tested-by Joshua Clayton <stillcompiling@gmail.com>
> Tested-by: Charles Kang <Charles.Kang@advantech.com.tw>
> Signed-off-by: Shawn Guo <shawnguo@kernel.org>
> [afa: ported to barebox from Linux commit 5d283b0838]
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/clk/imx/clk-imx6.c | 264 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 261 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/imx/clk-imx6.c b/drivers/clk/imx/clk-imx6.c
> index 22f33aee03e9..69586f04a21f 100644
> --- a/drivers/clk/imx/clk-imx6.c
> +++ b/drivers/clk/imx/clk-imx6.c
> @@ -64,6 +64,12 @@ static inline int cpu_is_plus(void)
>         return cpu_is_mx6qp() || cpu_is_mx6dp();
>  }
>
> +/* Audio/Video PLL post dividers don't work on i.MX6q revision 1.0 */
> +static inline int cpu_has_working_video_pll_post_div(void) {

Another { that should be on a new line.

> +       return !((cpu_is_mx6q() || cpu_is_mx6d()) &&
> +                imx_silicon_revision() == IMX_CHIP_REV_1_0);

Same story with cpu_is_mx6*() here. Imx_silicon_revision() depends on
imx_init() being called as well AFAIU.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/5] clk: imx6: remove quirky clk_set_parent(LDB_diN_sel, pll5_video_div)

[Ahmad Fatoum]

Hello,

On 27/5/19 08:50, Andrey Smirnov wrote:
> On Sun, May 26, 2019 at 11:29 PM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>>
>> Hello Andrey,
>>
>> On 26/5/19 23:55, Andrey Smirnov wrote:
>>> On Tue, May 21, 2019 at 8:56 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>>>>
>>> This particular patch does break a non 6Q+ version of RDU2,  but the
>>> follow up ones in the series fix it, so it seems that no action is
>>> really necessary on my part.
>>
>> The reparenting removed in this patch isn't reinstated by the rest of the series.
>> They merely apply parentage expressed in the device tree in a glitch-free manner.
>>
>> As both barebox and kernel imx6qdl-zii-rdu2.dtsi lack the relevant
>> assigned-clock-parents snippet, I am not sure what it is this patch broke that the
>> follow-up ones fixed?
> 
> Not sure, will investigate.

Ok.

> 
>>
>> Generally, affected boards have been broken since day 1, because the LVDS output
>> would've locked up every blue moon or so. If this patch breaks them, they're just
>> more reliably broken. :-)
>>
> 
> There's a world of difference between not working every once in a blue
> moon and not working from a first boot.

Ye, the latter one can be dealt with on-the-spot. The other is much more costly to
fix.

Cheers
Ahmad

> 
> Thanks,
> Andrey Smirnov
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/5] clk: imx6: remove quirky clk_set_parent(LDB_diN_sel, pll5_video_div)

[Andrey Smirnov]

On Sun, May 26, 2019 at 11:50 PM Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
>
> On Sun, May 26, 2019 at 11:29 PM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> >
> > Hello Andrey,
> >
> > On 26/5/19 23:55, Andrey Smirnov wrote:
> > > On Tue, May 21, 2019 at 8:56 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> > >>
> > > This particular patch does break a non 6Q+ version of RDU2,  but the
> > > follow up ones in the series fix it, so it seems that no action is
> > > really necessary on my part.
> >
> > The reparenting removed in this patch isn't reinstated by the rest of the series.
> > They merely apply parentage expressed in the device tree in a glitch-free manner.
> >
> > As both barebox and kernel imx6qdl-zii-rdu2.dtsi lack the relevant
> > assigned-clock-parents snippet, I am not sure what it is this patch broke that the
> > follow-up ones fixed?
>
> Not sure, will investigate.
>

Ah, I see, so with re-parenting to PLL5 removed ldb_* clocks default
to MMDC_CH1 and for that to work "clk: imx6: Mask mmdc_ch1 handshake
for periph2_sel and mmdc_ch1_axi_podf" is needed. After that commit
MMDC_CH1 seems to work well enough for frame buffer to come up and
function.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/5] clk: imx6: remove quirky clk_set_parent(LDB_diN_sel, pll5_video_div)

[Andrey Smirnov]

On Mon, May 27, 2019 at 12:23 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> Hello,
>
> On 27/5/19 08:50, Andrey Smirnov wrote:
> > On Sun, May 26, 2019 at 11:29 PM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> >>
> >> Hello Andrey,
> >>
> >> On 26/5/19 23:55, Andrey Smirnov wrote:
> >>> On Tue, May 21, 2019 at 8:56 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> >>>>
> >>> This particular patch does break a non 6Q+ version of RDU2,  but the
> >>> follow up ones in the series fix it, so it seems that no action is
> >>> really necessary on my part.
> >>
> >> The reparenting removed in this patch isn't reinstated by the rest of the series.
> >> They merely apply parentage expressed in the device tree in a glitch-free manner.
> >>
> >> As both barebox and kernel imx6qdl-zii-rdu2.dtsi lack the relevant
> >> assigned-clock-parents snippet, I am not sure what it is this patch broke that the
> >> follow-up ones fixed?
> >
> > Not sure, will investigate.
>
> Ok.
>
> >
> >>
> >> Generally, affected boards have been broken since day 1, because the LVDS output
> >> would've locked up every blue moon or so. If this patch breaks them, they're just
> >> more reliably broken. :-)
> >>
> >
> > There's a world of difference between not working every once in a blue
> > moon and not working from a first boot.
>
> Ye, the latter one can be dealt with on-the-spot. The other is much more costly to
> fix.
>

Here's a different perspective: If you needed to make an urgent phone
call, would you rather you phone didn't work every once in a blue moon
or be broken for the get go?

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 2/5] clk: imx6: Make the LDB_DI0 and LDB_DI1 clocks read-only

[Ahmad Fatoum]

Hello Andrey,

On 27/5/19 09:01, Andrey Smirnov wrote:
> On Tue, May 21, 2019 at 8:56 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>>
>> From: Philipp Zabel <p.zabel@pengutronix.de>
>>
>> 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>
>> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
>> Signed-off-by: Shawn Guo <shawnguo@kernel.org>
>> [afa: ported from Linux kernel commit 03d576f202]
>> [afa: added exception for i.MX6QP, see kernel commit f4a0a6c309]
>> [afa: added cpu_has_err009219 helper function]
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  drivers/clk/imx/clk-imx6.c | 17 +++++++++++++++--
>>  drivers/clk/imx/clk.h      |  8 ++++++++
>>  2 files changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/imx/clk-imx6.c b/drivers/clk/imx/clk-imx6.c
>> index 319485f8521c..01b649ebbd26 100644
>> --- a/drivers/clk/imx/clk-imx6.c
>> +++ b/drivers/clk/imx/clk-imx6.c
>> @@ -64,6 +64,12 @@ static inline int cpu_is_plus(void)
>>         return cpu_is_mx6qp() || cpu_is_mx6dp();
>>  }
>>
>> +/* i.MX6 Quad/Dual/DualLite/Solo are all affected */
>> +static inline int cpu_has_err009219(void) {
> 
> That { should really be on a separate line.
> 
>> +       return cpu_is_mx6d() || cpu_is_mx6q() ||
>> +               cpu_is_mx6dl() || cpu_is_mx6s();
>> +}
> 
> Hmm, imx_init() is executed at "postcore" and this function will be
> called at "core" init-level. All of the cpu_is_mx6*() functions will
> call cpu_is_mx6() which won't work right until imx_init() is called.
> Are you sure this is working as intended? I think cpu_mx6_is_imx6*()
> functions should be used instead. That's what I had to do on my board
> at least. It looks that there's other unrelated code in this file that
> might have this problem as well.

Thanks for spotting that. I will send out a v3 with this fixed.

> 
> Thanks,
> Andrey Smirnov
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/5] clk: imx6: remove quirky clk_set_parent(LDB_diN_sel, pll5_video_div)

[Ahmad Fatoum]

Hello Andrey,

On 27/5/19 09:25, Andrey Smirnov wrote:
>  After that commit
> MMDC_CH1 seems to work well enough for frame buffer to come up and
> function.

Oh, I see. I'll reorder the commits in v3 then to avoid this
temporary breakage.

Thanks
Ahmad

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/5] clk: imx6: remove quirky clk_set_parent(LDB_diN_sel, pll5_video_div)

[Ahmad Fatoum]

Hello Andrey,

On 27/5/19 09:28, Andrey Smirnov wrote:
>>>> Generally, affected boards have been broken since day 1, because the LVDS output
>>>> would've locked up every blue moon or so. If this patch breaks them, they're just
>>>> more reliably broken. :-)
>>>>
>>>
>>> There's a world of difference between not working every once in a blue
>>> moon and not working from a first boot.
>>
>> Ye, the latter one can be dealt with on-the-spot. The other is much more costly to
>> fix.
>>
> 
> Here's a different perspective: If you needed to make an urgent phone
> call, would you rather you phone didn't work every once in a blue moon
> or be broken for the get go?

The expectation is that the phone's basic operation was verified beforehand
and a once-in-a-blue-moon kind of issue is easier missed than an always
occurring one.

Do I take this as you voicing support of v1 of the patchset?

In v1, I did some dancing to maintain the old parenting
behavior on the SoC revision, where it was happening, i.e.
  - (Quad or Dual) and rev >1.0
  - DualLite
  - Solo and rev >1.0
  - QuadPlus or DualPlus

Philipp and Sascha's opinion on that was to get rid of the reparenting as
Linux did and reorder the commits, so they are easier to follow.

Cheers
Ahmad

> 
> Thanks,
> Andrey Smirnov
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/5] clk: imx6: remove quirky clk_set_parent(LDB_diN_sel, pll5_video_div)

[Andrey Smirnov]

On Mon, May 27, 2019 at 12:49 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> Hello Andrey,
>
> On 27/5/19 09:28, Andrey Smirnov wrote:
> >>>> Generally, affected boards have been broken since day 1, because the LVDS output
> >>>> would've locked up every blue moon or so. If this patch breaks them, they're just
> >>>> more reliably broken. :-)
> >>>>
> >>>
> >>> There's a world of difference between not working every once in a blue
> >>> moon and not working from a first boot.
> >>
> >> Ye, the latter one can be dealt with on-the-spot. The other is much more costly to
> >> fix.
> >>
> >
> > Here's a different perspective: If you needed to make an urgent phone
> > call, would you rather you phone didn't work every once in a blue moon
> > or be broken for the get go?
>
> The expectation is that the phone's basic operation was verified beforehand
> and a once-in-a-blue-moon kind of issue is easier missed than an always
> occurring one.
>
> Do I take this as you voicing support of v1 of the patchset?
>

No, not at all. Both versions are fine as far as I am concerned and
should proceed on whatever course you guys decided on. This particular
conversation started with you trying to convince me that "generally"
your patch didn't break the board and it was always broken, just less
reliably so. My point is that this is just a borderline exercise in
sophistry and if a thing went from "working 99% of the time" to "not
working at all" after a change, that change broke that thing. Even if
the change itself is a good thing and definitely should be done.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/5] clk: imx6: remove quirky clk_set_parent(LDB_diN_sel, pll5_video_div)

[Ahmad Fatoum]

On 27/5/19 10:02, Andrey Smirnov wrote:
> On Mon, May 27, 2019 at 12:49 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>>
>> Hello Andrey,
>>
>> On 27/5/19 09:28, Andrey Smirnov wrote:
>>>>>> Generally, affected boards have been broken since day 1, because the LVDS output
>>>>>> would've locked up every blue moon or so. If this patch breaks them, they're just
>>>>>> more reliably broken. :-)
>>>>>>
>>>>>
>>>>> There's a world of difference between not working every once in a blue
>>>>> moon and not working from a first boot.
>>>>
>>>> Ye, the latter one can be dealt with on-the-spot. The other is much more costly to
>>>> fix.
>>>>
>>>
>>> Here's a different perspective: If you needed to make an urgent phone
>>> call, would you rather you phone didn't work every once in a blue moon
>>> or be broken for the get go?
>>
>> The expectation is that the phone's basic operation was verified beforehand
>> and a once-in-a-blue-moon kind of issue is easier missed than an always
>> occurring one.
>>
>> Do I take this as you voicing support of v1 of the patchset?
>>
> 
> No, not at all. Both versions are fine as far as I am concerned and
> should proceed on whatever course you guys decided on. This particular
> conversation started with you trying to convince me that "generally"

s/generally/strictly speaking/, would maybe have conveyed the intention
better.

> your patch didn't break the board and it was always broken, just less
> reliably so. My point is that this is just a borderline exercise in
> sophistry and if a thing went from "working 99% of the time" to "not
> working at all" after a change, that change broke that thing. Even if
> the change itself is a good thing and definitely should be done.

I didn't mean to downplay the patch's breakage. It's just that arguing
in favor of it is easier when the aspect was partially broken before.

Thanks again,
Ahmad

> 
> Thanks,
> Andrey Smirnov
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox