[PATCH v3 0/6] clk: imx6: work around LDB hang caused by ERR009219

[PATCH v3 0/6] 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.
v2 is available here:
https://www.spinics.net/lists/u-boot-v2/msg37970.html

Changes since v2:
 - reordered commits to avoid intermittent breakage, final
   diff unaffected
 - prepended a commit fixing premature use of cpu_is_mx6
 - fixed brace placement of cpu_(mx6_)?has_err009219
 - changed commits to use cpu_mx6_is_mx6*, all suggested by Andrey
 - updated commit messages, collected Andrey's Tested-By and
   Philipp's Reviewed-By

Changes since v1:
 - 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 (3):
  clk: imx6: fix use of cpu_is_mx6* before they are initialized
  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: Mask mmdc_ch1 handshake for periph2_sel and
    mmdc_ch1_axi_podf
  clk: imx6: Make the LDB_DI0 and LDB_DI1 clocks read-only

 drivers/clk/imx/clk-imx6.c | 329 ++++++++++++++++++++++++++++++++++---
 drivers/clk/imx/clk.h      |   8 +
 2 files changed, 317 insertions(+), 20 deletions(-)

-- 
2.20.1


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

[PATCH v3 1/6] clk: imx6: fix use of cpu_is_mx6* before they are initialized

[Ahmad Fatoum]

The cpu_is_mx6* macros rely on __imx_cpu_type, which is initialized
dependent on device tree compatible at "postcore" init-level imx_init.

The imx6q-ccm driver, which uses the cpu_is_mx6* macros, is registered
at "core" init-level, however. This results on the macro always returning
false.

Fix this by using the cpu_mx6_is_mx6* family of macros instead.
These already require that that CPU is a MX6, which is safe because the
the driver only matches against "fsl,imx6q-ccm".

Only exception is the video5 pll reparenting. Here we will just
maintain the old behavior as we will drop the if clause in a following
commit anyway.

Reported-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/clk/imx/clk-imx6.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/imx/clk-imx6.c b/drivers/clk/imx/clk-imx6.c
index 35b995dae24e..0b71a1322466 100644
--- a/drivers/clk/imx/clk-imx6.c
+++ b/drivers/clk/imx/clk-imx6.c
@@ -59,9 +59,9 @@
 static struct clk *clks[IMX6QDL_CLK_END];
 static struct clk_onecell_data clk_data;
 
-static inline int cpu_is_plus(void)
+static inline int cpu_mx6_is_plus(void)
 {
-	return cpu_is_mx6qp() || cpu_is_mx6dp();
+	return cpu_mx6_is_mx6qp() || cpu_mx6_is_mx6dp();
 }
 
 static const char *step_sels[] = {
@@ -341,8 +341,7 @@ static void imx6_add_video_clks(void __iomem *anab, void __iomem *cb)
 	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()) {
+	if (imx_silicon_revision() != IMX_CHIP_REV_1_0) {
 		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]);
 	}
@@ -418,7 +417,7 @@ static int imx6_ccm_probe(struct device_d *dev)
 	clks[IMX6QDL_CLK_USDHC2_SEL]       = imx_clk_mux("usdhc2_sel",       base + 0x1c, 17, 1, usdhc_sels,        ARRAY_SIZE(usdhc_sels));
 	clks[IMX6QDL_CLK_USDHC3_SEL]       = imx_clk_mux("usdhc3_sel",       base + 0x1c, 18, 1, usdhc_sels,        ARRAY_SIZE(usdhc_sels));
 	clks[IMX6QDL_CLK_USDHC4_SEL]       = imx_clk_mux("usdhc4_sel",       base + 0x1c, 19, 1, usdhc_sels,        ARRAY_SIZE(usdhc_sels));
-	if (cpu_is_plus())
+	if (cpu_mx6_is_plus())
 		clks[IMX6QDL_CLK_ENFC_SEL]         = imx_clk_mux("enfc_sel",         base + 0x2c, 16, 2, enfc_sels_plus,    ARRAY_SIZE(enfc_sels_plus));
 	else
 		clks[IMX6QDL_CLK_ENFC_SEL]         = imx_clk_mux("enfc_sel",         base + 0x2c, 16, 2, enfc_sels,         ARRAY_SIZE(enfc_sels));
@@ -469,7 +468,7 @@ static int imx6_ccm_probe(struct device_d *dev)
 	clks[IMX6QDL_CLK_ECSPI2]       = imx_clk_gate2("ecspi2",        "ecspi_root",        base + 0x6c, 2);
 	clks[IMX6QDL_CLK_ECSPI3]       = imx_clk_gate2("ecspi3",        "ecspi_root",        base + 0x6c, 4);
 	clks[IMX6QDL_CLK_ECSPI4]       = imx_clk_gate2("ecspi4",        "ecspi_root",        base + 0x6c, 6);
-	if (cpu_is_mx6dl())
+	if (cpu_mx6_is_mx6dl())
 		clks[IMX6DL_CLK_I2C4]  = imx_clk_gate2("i2c4",          "ipg_per",           base + 0x6c, 8);
 	else
 		clks[IMX6Q_CLK_ECSPI5] = imx_clk_gate2("ecspi5",        "ecspi_root",        base + 0x6c, 8);
-- 
2.20.1


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

[PATCH v3 2/6] 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: reviewed for barebox]
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Tested-by: Andrey Smirnov <andrew.smirnov@gmail.com>
[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 | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/clk/imx/clk-imx6.c b/drivers/clk/imx/clk-imx6.c
index 0b71a1322466..d23b7e277bdf 100644
--- a/drivers/clk/imx/clk-imx6.c
+++ b/drivers/clk/imx/clk-imx6.c
@@ -293,6 +293,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);
@@ -300,6 +313,9 @@ 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);
+
 	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));
-- 
2.20.1


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

[PATCH v3 3/6] 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").

In barebox, the reparenting remains and is done for all i.MX6 SoCs
with a revision > 1.0. On all of them, 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. The device tree[1] can elect to reinstate the reparenting:

	&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 reparenting
defined this way in a glitch-free manner.

As of v2019.07.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>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Tested-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/clk/imx/clk-imx6.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/clk/imx/clk-imx6.c b/drivers/clk/imx/clk-imx6.c
index d23b7e277bdf..6f19535be7ba 100644
--- a/drivers/clk/imx/clk-imx6.c
+++ b/drivers/clk/imx/clk-imx6.c
@@ -356,12 +356,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) {
-		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 v3 4/6] 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: reviewed for barebox]
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Tested-by: Andrey Smirnov <andrew.smirnov@gmail.com>
[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 6f19535be7ba..61819ff9744d 100644
--- a/drivers/clk/imx/clk-imx6.c
+++ b/drivers/clk/imx/clk-imx6.c
@@ -64,6 +64,13 @@ static inline int cpu_mx6_is_plus(void)
 	return cpu_mx6_is_mx6qp() || cpu_mx6_is_mx6dp();
 }
 
+/* i.MX6 Quad/Dual/DualLite/Solo are all affected */
+static inline int cpu_mx6_has_err009219(void)
+{
+	return cpu_mx6_is_mx6d() || cpu_mx6_is_mx6q() ||
+		cpu_mx6_is_mx6dl() || cpu_mx6_is_mx6s();
+}
+
 static const char *step_sels[] = {
 	"osc",
 	"pll2_pfd2_396m",
@@ -316,8 +323,14 @@ static void imx6_add_video_clks(void __iomem *anab, void __iomem *cb)
 
 	imx6q_mmdc_ch1_mask_handshake(cb);
 
-	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_mx6_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 v3 5/6] 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: reviewed for barebox]
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Tested-by: Andrey Smirnov <andrew.smirnov@gmail.com>
[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 61819ff9744d..ffeecf02f9b5 100644
--- a/drivers/clk/imx/clk-imx6.c
+++ b/drivers/clk/imx/clk-imx6.c
@@ -64,6 +64,12 @@ static inline int cpu_mx6_is_plus(void)
 	return cpu_mx6_is_mx6qp() || cpu_mx6_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_mx6_has_err009219(void)
 {
@@ -300,9 +306,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)
 {
@@ -313,7 +399,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);
@@ -321,9 +570,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_mx6_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 {
@@ -528,7 +786,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 v3 6/6] 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.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Tested-by: Andrey Smirnov <andrew.smirnov@gmail.com>
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 ffeecf02f9b5..c47281b16e68 100644
--- a/drivers/clk/imx/clk-imx6.c
+++ b/drivers/clk/imx/clk-imx6.c
@@ -214,12 +214,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[] = {
@@ -312,23 +320,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;
@@ -467,7 +475,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);
@@ -481,7 +489,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]);
 		}
@@ -490,7 +498,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 v3 1/6] clk: imx6: fix use of cpu_is_mx6* before they are initialized

[Ahmad Fatoum]

On 17/7/19 19:05, Ahmad Fatoum wrote:
> The cpu_is_mx6* macros rely on __imx_cpu_type, which is initialized
> dependent on device tree compatible at "postcore" init-level imx_init.
> 
> The imx6q-ccm driver, which uses the cpu_is_mx6* macros, is registered
> at "core" init-level, however. This results on the macro always returning
> false.
> 
> Fix this by using the cpu_mx6_is_mx6* family of macros instead.
> These already require that that CPU is a MX6, which is safe because the
> the driver only matches against "fsl,imx6q-ccm".
> 
> Only exception is the video5 pll reparenting. Here we will just
> maintain the old behavior as we will drop the if clause in a following
> commit anyway.
> 
> Reported-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

While it's clearly the right thing to do, I have no affected boards
(Dual or Plus) sitting on my desk, so the change is untested.

A Tested-by would be appreciated, thanks!

> ---
>  drivers/clk/imx/clk-imx6.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-imx6.c b/drivers/clk/imx/clk-imx6.c
> index 35b995dae24e..0b71a1322466 100644
> --- a/drivers/clk/imx/clk-imx6.c
> +++ b/drivers/clk/imx/clk-imx6.c
> @@ -59,9 +59,9 @@
>  static struct clk *clks[IMX6QDL_CLK_END];
>  static struct clk_onecell_data clk_data;
>  
> -static inline int cpu_is_plus(void)
> +static inline int cpu_mx6_is_plus(void)
>  {
> -	return cpu_is_mx6qp() || cpu_is_mx6dp();
> +	return cpu_mx6_is_mx6qp() || cpu_mx6_is_mx6dp();
>  }
>  
>  static const char *step_sels[] = {
> @@ -341,8 +341,7 @@ static void imx6_add_video_clks(void __iomem *anab, void __iomem *cb)
>  	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()) {
> +	if (imx_silicon_revision() != IMX_CHIP_REV_1_0) {
>  		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]);
>  	}
> @@ -418,7 +417,7 @@ static int imx6_ccm_probe(struct device_d *dev)
>  	clks[IMX6QDL_CLK_USDHC2_SEL]       = imx_clk_mux("usdhc2_sel",       base + 0x1c, 17, 1, usdhc_sels,        ARRAY_SIZE(usdhc_sels));
>  	clks[IMX6QDL_CLK_USDHC3_SEL]       = imx_clk_mux("usdhc3_sel",       base + 0x1c, 18, 1, usdhc_sels,        ARRAY_SIZE(usdhc_sels));
>  	clks[IMX6QDL_CLK_USDHC4_SEL]       = imx_clk_mux("usdhc4_sel",       base + 0x1c, 19, 1, usdhc_sels,        ARRAY_SIZE(usdhc_sels));
> -	if (cpu_is_plus())
> +	if (cpu_mx6_is_plus())
>  		clks[IMX6QDL_CLK_ENFC_SEL]         = imx_clk_mux("enfc_sel",         base + 0x2c, 16, 2, enfc_sels_plus,    ARRAY_SIZE(enfc_sels_plus));
>  	else
>  		clks[IMX6QDL_CLK_ENFC_SEL]         = imx_clk_mux("enfc_sel",         base + 0x2c, 16, 2, enfc_sels,         ARRAY_SIZE(enfc_sels));
> @@ -469,7 +468,7 @@ static int imx6_ccm_probe(struct device_d *dev)
>  	clks[IMX6QDL_CLK_ECSPI2]       = imx_clk_gate2("ecspi2",        "ecspi_root",        base + 0x6c, 2);
>  	clks[IMX6QDL_CLK_ECSPI3]       = imx_clk_gate2("ecspi3",        "ecspi_root",        base + 0x6c, 4);
>  	clks[IMX6QDL_CLK_ECSPI4]       = imx_clk_gate2("ecspi4",        "ecspi_root",        base + 0x6c, 6);
> -	if (cpu_is_mx6dl())
> +	if (cpu_mx6_is_mx6dl())
>  		clks[IMX6DL_CLK_I2C4]  = imx_clk_gate2("i2c4",          "ipg_per",           base + 0x6c, 8);
>  	else
>  		clks[IMX6Q_CLK_ECSPI5] = imx_clk_gate2("ecspi5",        "ecspi_root",        base + 0x6c, 8);
> 

-- 
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 v3 4/6] clk: imx6: Make the LDB_DI0 and LDB_DI1 clocks read-only

[Roland Hieber]

On Wed, Jul 17, 2019 at 07:06:02PM +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: reviewed for barebox]
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> Tested-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> [afa: ported from Linux kernel commit 03d576f202]

Should this line be further above, or were the lines above all part of
the Linux commit?

 - Roland

> [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 6f19535be7ba..61819ff9744d 100644
> --- a/drivers/clk/imx/clk-imx6.c
> +++ b/drivers/clk/imx/clk-imx6.c
> @@ -64,6 +64,13 @@ static inline int cpu_mx6_is_plus(void)
>  	return cpu_mx6_is_mx6qp() || cpu_mx6_is_mx6dp();
>  }
>  
> +/* i.MX6 Quad/Dual/DualLite/Solo are all affected */
> +static inline int cpu_mx6_has_err009219(void)
> +{
> +	return cpu_mx6_is_mx6d() || cpu_mx6_is_mx6q() ||
> +		cpu_mx6_is_mx6dl() || cpu_mx6_is_mx6s();
> +}
> +
>  static const char *step_sels[] = {
>  	"osc",
>  	"pll2_pfd2_396m",
> @@ -316,8 +323,14 @@ static void imx6_add_video_clks(void __iomem *anab, void __iomem *cb)
>  
>  	imx6q_mmdc_ch1_mask_handshake(cb);
>  
> -	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_mx6_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
> 

-- 
Roland Hieber                     | r.hieber@pengutronix.de     |
Pengutronix e.K.                  | https://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim | Phone: +49-5121-206917-5086 |
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 v3 4/6] clk: imx6: Make the LDB_DI0 and LDB_DI1 clocks read-only

[Ahmad Fatoum]

Hello Roland,

On 18/7/19 10:19, Roland Hieber wrote:
> On Wed, Jul 17, 2019 at 07:06:02PM +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: reviewed for barebox]
>> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
>> Tested-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> [afa: ported from Linux kernel commit 03d576f202]
> 
> Should this line be further above, or were the lines above all part of
> the Linux commit?

The first [afa: is the Demarcation line between kernel and barebox
commit message. If I moved the second [afa: further up it wouldn't be
that clear who did what..

In retrospect, I should have used some ASCII box for the original
commit and indented it a bit. Should there be a v4, I can do this.
(Unless this is completely unworkable..) 

> 
>  - Roland
> 
>> [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 6f19535be7ba..61819ff9744d 100644
>> --- a/drivers/clk/imx/clk-imx6.c
>> +++ b/drivers/clk/imx/clk-imx6.c
>> @@ -64,6 +64,13 @@ static inline int cpu_mx6_is_plus(void)
>>  	return cpu_mx6_is_mx6qp() || cpu_mx6_is_mx6dp();
>>  }
>>  
>> +/* i.MX6 Quad/Dual/DualLite/Solo are all affected */
>> +static inline int cpu_mx6_has_err009219(void)
>> +{
>> +	return cpu_mx6_is_mx6d() || cpu_mx6_is_mx6q() ||
>> +		cpu_mx6_is_mx6dl() || cpu_mx6_is_mx6s();
>> +}
>> +
>>  static const char *step_sels[] = {
>>  	"osc",
>>  	"pll2_pfd2_396m",
>> @@ -316,8 +323,14 @@ static void imx6_add_video_clks(void __iomem *anab, void __iomem *cb)
>>  
>>  	imx6q_mmdc_ch1_mask_handshake(cb);
>>  
>> -	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_mx6_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
>>
> 

-- 
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 v3 4/6] clk: imx6: Make the LDB_DI0 and LDB_DI1 clocks read-only

[Roland Hieber]

On Thu, Jul 18, 2019 at 10:25:05AM +0200, Ahmad Fatoum wrote:
> Hello Roland,
> 
> On 18/7/19 10:19, Roland Hieber wrote:
> > On Wed, Jul 17, 2019 at 07:06:02PM +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: reviewed for barebox]
> >> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> >> Tested-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> >> [afa: ported from Linux kernel commit 03d576f202]
> > 
> > Should this line be further above, or were the lines above all part of
> > the Linux commit?
> 
> The first [afa: is the Demarcation line between kernel and barebox
> commit message. If I moved the second [afa: further up it wouldn't be
> that clear who did what..

Okay, until now I always read the footers like they were appended from
top to bottom chronologically, so first reviewing it for barebox and
only afterwards porting it from Linux doesn't make sense to me :-)
I just haven't met your style in the wild yet.

 - Roland

-- 
Roland Hieber                     | r.hieber@pengutronix.de     |
Pengutronix e.K.                  | https://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim | Phone: +49-5121-206917-5086 |
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 v3 1/6] clk: imx6: fix use of cpu_is_mx6* before they are initialized

[Ahmad Fatoum]

On 17/7/19 19:10, Ahmad Fatoum wrote:
> On 17/7/19 19:05, Ahmad Fatoum wrote:
>> The cpu_is_mx6* macros rely on __imx_cpu_type, which is initialized
>> dependent on device tree compatible at "postcore" init-level imx_init.
>>
>> The imx6q-ccm driver, which uses the cpu_is_mx6* macros, is registered
>> at "core" init-level, however. This results on the macro always returning
>> false.
>>
>> Fix this by using the cpu_mx6_is_mx6* family of macros instead.
>> These already require that that CPU is a MX6, which is safe because the
>> the driver only matches against "fsl,imx6q-ccm".
>>
>> Only exception is the video5 pll reparenting. Here we will just
>> maintain the old behavior as we will drop the if clause in a following
>> commit anyway.
>>
>> Reported-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 
> While it's clearly the right thing to do, I have no affected boards
> (Dual or Plus) sitting on my desk, so the change is untested.

s/Dual/DualLite/ (or Solo).

> 
> A Tested-by would be appreciated, thanks!
> 
>> ---
>>  drivers/clk/imx/clk-imx6.c | 11 +++++------
>>  1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/clk/imx/clk-imx6.c b/drivers/clk/imx/clk-imx6.c
>> index 35b995dae24e..0b71a1322466 100644
>> --- a/drivers/clk/imx/clk-imx6.c
>> +++ b/drivers/clk/imx/clk-imx6.c
>> @@ -59,9 +59,9 @@
>>  static struct clk *clks[IMX6QDL_CLK_END];
>>  static struct clk_onecell_data clk_data;
>>  
>> -static inline int cpu_is_plus(void)
>> +static inline int cpu_mx6_is_plus(void)
>>  {
>> -	return cpu_is_mx6qp() || cpu_is_mx6dp();
>> +	return cpu_mx6_is_mx6qp() || cpu_mx6_is_mx6dp();
>>  }
>>  
>>  static const char *step_sels[] = {
>> @@ -341,8 +341,7 @@ static void imx6_add_video_clks(void __iomem *anab, void __iomem *cb)
>>  	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()) {
>> +	if (imx_silicon_revision() != IMX_CHIP_REV_1_0) {
>>  		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]);
>>  	}
>> @@ -418,7 +417,7 @@ static int imx6_ccm_probe(struct device_d *dev)
>>  	clks[IMX6QDL_CLK_USDHC2_SEL]       = imx_clk_mux("usdhc2_sel",       base + 0x1c, 17, 1, usdhc_sels,        ARRAY_SIZE(usdhc_sels));
>>  	clks[IMX6QDL_CLK_USDHC3_SEL]       = imx_clk_mux("usdhc3_sel",       base + 0x1c, 18, 1, usdhc_sels,        ARRAY_SIZE(usdhc_sels));
>>  	clks[IMX6QDL_CLK_USDHC4_SEL]       = imx_clk_mux("usdhc4_sel",       base + 0x1c, 19, 1, usdhc_sels,        ARRAY_SIZE(usdhc_sels));
>> -	if (cpu_is_plus())
>> +	if (cpu_mx6_is_plus())
>>  		clks[IMX6QDL_CLK_ENFC_SEL]         = imx_clk_mux("enfc_sel",         base + 0x2c, 16, 2, enfc_sels_plus,    ARRAY_SIZE(enfc_sels_plus));
>>  	else
>>  		clks[IMX6QDL_CLK_ENFC_SEL]         = imx_clk_mux("enfc_sel",         base + 0x2c, 16, 2, enfc_sels,         ARRAY_SIZE(enfc_sels));
>> @@ -469,7 +468,7 @@ static int imx6_ccm_probe(struct device_d *dev)
>>  	clks[IMX6QDL_CLK_ECSPI2]       = imx_clk_gate2("ecspi2",        "ecspi_root",        base + 0x6c, 2);
>>  	clks[IMX6QDL_CLK_ECSPI3]       = imx_clk_gate2("ecspi3",        "ecspi_root",        base + 0x6c, 4);
>>  	clks[IMX6QDL_CLK_ECSPI4]       = imx_clk_gate2("ecspi4",        "ecspi_root",        base + 0x6c, 6);
>> -	if (cpu_is_mx6dl())
>> +	if (cpu_mx6_is_mx6dl())
>>  		clks[IMX6DL_CLK_I2C4]  = imx_clk_gate2("i2c4",          "ipg_per",           base + 0x6c, 8);
>>  	else
>>  		clks[IMX6Q_CLK_ECSPI5] = imx_clk_gate2("ecspi5",        "ecspi_root",        base + 0x6c, 8);
>>
> 

-- 
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 v3 4/6] clk: imx6: Make the LDB_DI0 and LDB_DI1 clocks read-only

[Sascha Hauer]

On Thu, Jul 18, 2019 at 10:25:05AM +0200, Ahmad Fatoum wrote:
> Hello Roland,
> 
> On 18/7/19 10:19, Roland Hieber wrote:
> > On Wed, Jul 17, 2019 at 07:06:02PM +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: reviewed for barebox]
> >> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> >> Tested-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> >> [afa: ported from Linux kernel commit 03d576f202]
> > 
> > Should this line be further above, or were the lines above all part of
> > the Linux commit?
> 
> The first [afa: is the Demarcation line between kernel and barebox
> commit message. If I moved the second [afa: further up it wouldn't be
> that clear who did what..
> 
> In retrospect, I should have used some ASCII box for the original
> commit and indented it a bit. 

Added that and applied, thanks

Sascha

-- 
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 v3 5/6] clk: imx6: Fix procedure to switch the parent of LDB_DI_CLK

[Sascha Hauer]

Hi Ahmad,

On Wed, Jul 17, 2019 at 07:06:03PM +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.

I haven't investigates any further, but this one breaks NAND support at
least on my phyCORE i.MX6 board. With this patch applied we get:

MXS: Timeout resetting block via register 0x00112000
mxs_nand 112000.gpmi-nand@112000.of: probe failed: Connection timed out

Does this patch disable any clocks necessary for NAND?

Sascha

-- 
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 v3 5/6] clk: imx6: Fix procedure to switch the parent of LDB_DI_CLK

[Ahmad Fatoum]

On 9/10/19 3:15 PM, Sascha Hauer wrote:
> Hi Ahmad,
> 
> On Wed, Jul 17, 2019 at 07:06:03PM +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.
> 
> I haven't investigates any further, but this one breaks NAND support at
> least on my phyCORE i.MX6 board. With this patch applied we get:
> 
> MXS: Timeout resetting block via register 0x00112000
> mxs_nand 112000.gpmi-nand@112000.of: probe failed: Connection timed out
> 
> Does this patch disable any clocks necessary for NAND?

Running clk_dump on an i.MX6Q shows the gpmi clocks being sourced from
pll2_pfd2_396m. PFD outputs come out of reset ungated, but since this commit
disable_anatop_clocks() gates them.
Apparently, nothing in barebox ungates them again? The commit is a straight
port from the kernel, so I think there's some other upstream change that is
missing. Any idea?

Cheers
Ahmad
 

> 
> Sascha
> 

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