| Message ID | 1397044538-12676-1-git-send-email-festevam@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Apr 09, 2014 at 08:55:38AM -0300, Fabio Estevam wrote: > From: Fabio Estevam <fabio.estevam@freescale.com> > > Due to incorrect placement of the clock gate cell in the ldb_di[x]_clk tree, > the glitchy parent mux of ldb_di[x]_clk can cause a glitch to enter the > ldb_di_ipu_div divider. If the divider gets locked up, no ldb_di[x]_clk is > generated, and the LVDS display will hang when the ipu_di_clk is sourced from > ldb_di_clk. > > To fix the problem, both the new and current parent of the ldb_di_clk should > be disabled before the switch. This patch ensures that correct steps are > followed when ldb_di_clk parent is switched in the beginning of boot. I'm not sure this is a full/correct fix to the problem. You assume that the re-parenting only happens during boot for once? What about the re-parenting triggered by the clk_set_parent() call? Shawn > > Signed-off-by: Ranjani Vaidyanathan <Ranjani.Vaidyanathan@freescale.com> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- > arch/arm/mach-imx/clk-imx6q.c | 125 ++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 121 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c > index 20ad0d1..3ee45f4 100644 > --- a/arch/arm/mach-imx/clk-imx6q.c > +++ b/arch/arm/mach-imx/clk-imx6q.c > @@ -140,6 +140,123 @@ static struct clk_div_table video_div_table[] = { > { /* sentinel */ } > }; > > +static void init_ldb_clks(enum mx6q_clks new_parent) > +{ > + struct device_node *np; > + static void __iomem *ccm_base; > + unsigned int reg; > + > + np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-ccm"); > + ccm_base = of_iomap(np, 0); > + WARN_ON(!ccm_base); > + > + /* > + * Need to follow a strict procedure when changing the LDB > + * clock, else we can introduce a glitch. Things to keep in > + * mind: > + * 1. The current and new parent clocks must be disabled. > + * 2. The default clock for ldb_dio_clk is mmdc_ch1 which has > + * no CG bit. > + * 3. In the RTL implementation of the LDB_DI_CLK_SEL mux > + * the top four options are in one mux and the PLL3 option along > + * with another option is in the second mux. There is third mux > + * used to decide between the first and second mux. > + * The code below switches the parent to the bottom mux first > + * and then manipulates the top mux. This ensures that no glitch > + * will enter the divider. > + * > + * Need to disable MMDC_CH1 clock manually as there is no CG bit > + * for this clock. The only way to disable this clock is to move > + * it topll3_sw_clk and then to disable pll3_sw_clk > + * Make sure periph2_clk2_sel is set to pll3_sw_clk > + */ > + reg = readl_relaxed(ccm_base + 0x18); > + reg &= ~(1 << 20); > + writel_relaxed(reg, ccm_base + 0x18); > + > + /* Set MMDC_CH1 mask bit */ > + reg = readl_relaxed(ccm_base + 0x4); > + reg |= 1 << 16; > + writel_relaxed(reg, ccm_base + 0x4); > + > + /* > + * Set the periph2_clk_sel to the top mux so that > + * mmdc_ch1 is from pll3_sw_clk. > + */ > + reg = readl_relaxed(ccm_base + 0x14); > + reg |= 1 << 26; > + writel_relaxed(reg, ccm_base + 0x14); > + > + /* Wait for the clock switch */ > + while (readl_relaxed(ccm_base + 0x48)) > + ; > + > + /* Disable pll3_sw_clk by selecting the bypass clock source */ > + reg = readl_relaxed(ccm_base + 0xc); > + reg |= 1 << 0; > + writel_relaxed(reg, ccm_base + 0xc); > + > + /* Set the ldb_di0_clk and ldb_di1_clk to 111b */ > + reg = readl_relaxed(ccm_base + 0x2c); > + reg |= ((7 << 9) | (7 << 12)); > + writel_relaxed(reg, ccm_base + 0x2c); > + > + /* Set the ldb_di0_clk and ldb_di1_clk to 100b */ > + reg = readl_relaxed(ccm_base + 0x2c); > + reg &= ~((7 << 9) | (7 << 12)); > + reg |= ((4 << 9) | (4 << 12)); > + writel_relaxed(reg, ccm_base + 0x2c); > + > + /* Perform the LDB parent clock switch */ > + clk_set_parent(clk[ldb_di0_sel], clk[new_parent]); > + clk_set_parent(clk[ldb_di1_sel], clk[new_parent]); > + > + /* Unbypass pll3_sw_clk */ > + reg = readl_relaxed(ccm_base + 0xc); > + reg &= ~(1 << 0); > + writel_relaxed(reg, ccm_base + 0xc); > + > + /* > + * Set the periph2_clk_sel back to the bottom mux so that > + * mmdc_ch1 is from its original parent. > + */ > + reg = readl_relaxed(ccm_base + 0x14); > + reg &= ~(1 << 26); > + writel_relaxed(reg, ccm_base + 0x14); > + > + /* Wait for the clock switch */ > + while (readl_relaxed(ccm_base + 0x48)) > + ; > + > + /* Clear MMDC_CH1 mask bit */ > + reg = readl_relaxed(ccm_base + 0x4); > + reg &= ~(1 << 16); > + writel_relaxed(reg, ccm_base + 0x4); > +} > + > +static void disable_anatop_clocks(void __iomem *anatop_base) > +{ > + unsigned int reg; > + > + /* Make sure PFDs are disabled at boot. */ > + reg = readl_relaxed(anatop_base + 0x100); > + /* Cannot disable pll2_pfd2_396M, as it is the MMDC clock in iMX6DL */ > + if (cpu_is_imx6dl()) > + reg |= 0x80008080; > + else > + reg |= 0x80808080; > + writel_relaxed(reg, anatop_base + 0x100); > + > + reg = readl_relaxed(anatop_base + 0xf0); > + reg |= 0x80808080; > + writel_relaxed(reg, anatop_base + 0xf0); > + > + /* Make sure PLLs is disabled */ > + reg = readl_relaxed(anatop_base + 0xa0); > + reg &= ~(1 << 13); > + writel_relaxed(reg, anatop_base + 0xa0); > +} > + > static void __init imx6q_clocks_init(struct device_node *ccm_node) > { > struct device_node *np; > @@ -232,6 +349,8 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node) > clk[pll5_post_div] = clk_register_divider_table(NULL, "pll5_post_div", "pll5_video", CLK_SET_RATE_PARENT, base + 0xa0, 19, 2, 0, post_div_table, &imx_ccm_lock); > clk[pll5_video_div] = clk_register_divider_table(NULL, "pll5_video_div", "pll5_post_div", CLK_SET_RATE_PARENT, base + 0x170, 30, 2, 0, video_div_table, &imx_ccm_lock); > > + disable_anatop_clocks(base); > + > np = ccm_node; > base = of_iomap(np, 0); > WARN_ON(!base); > @@ -440,10 +559,8 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node) > clk_register_clkdev(clk[enet_ref], "enet_ref", NULL); > > if ((imx_get_soc_revision() != IMX_CHIP_REVISION_1_0) || > - cpu_is_imx6dl()) { > - clk_set_parent(clk[ldb_di0_sel], clk[pll5_video_div]); > - clk_set_parent(clk[ldb_di1_sel], clk[pll5_video_div]); > - } > + cpu_is_imx6dl()) > + init_ldb_clks(pll5_video_div); > > /* > * The gpmi needs 100MHz frequency in the EDO/Sync mode, > -- > 1.8.3.2 > > >
2014-04-09 13:55 GMT+02:00 Fabio Estevam <festevam@gmail.com>: > From: Fabio Estevam <fabio.estevam@freescale.com> > > Due to incorrect placement of the clock gate cell in the ldb_di[x]_clk tree, > the glitchy parent mux of ldb_di[x]_clk can cause a glitch to enter the > ldb_di_ipu_div divider. If the divider gets locked up, no ldb_di[x]_clk is > generated, and the LVDS display will hang when the ipu_di_clk is sourced from > ldb_di_clk. > > To fix the problem, both the new and current parent of the ldb_di_clk should > be disabled before the switch. This patch ensures that correct steps are > followed when ldb_di_clk parent is switched in the beginning of boot. > > Signed-off-by: Ranjani Vaidyanathan <Ranjani.Vaidyanathan@freescale.com> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- > arch/arm/mach-imx/clk-imx6q.c | 125 ++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 121 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c > index 20ad0d1..3ee45f4 100644 > --- a/arch/arm/mach-imx/clk-imx6q.c > +++ b/arch/arm/mach-imx/clk-imx6q.c > @@ -140,6 +140,123 @@ static struct clk_div_table video_div_table[] = { > { /* sentinel */ } > }; > > +static void init_ldb_clks(enum mx6q_clks new_parent) > +{ > + struct device_node *np; > + static void __iomem *ccm_base; > + unsigned int reg; > + > + np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-ccm"); > + ccm_base = of_iomap(np, 0); > + WARN_ON(!ccm_base); > + > + /* > + * Need to follow a strict procedure when changing the LDB > + * clock, else we can introduce a glitch. Things to keep in > + * mind: > + * 1. The current and new parent clocks must be disabled. > + * 2. The default clock for ldb_dio_clk is mmdc_ch1 which has > + * no CG bit. > + * 3. In the RTL implementation of the LDB_DI_CLK_SEL mux > + * the top four options are in one mux and the PLL3 option along > + * with another option is in the second mux. There is third mux > + * used to decide between the first and second mux. > + * The code below switches the parent to the bottom mux first > + * and then manipulates the top mux. This ensures that no glitch > + * will enter the divider. > + * > + * Need to disable MMDC_CH1 clock manually as there is no CG bit > + * for this clock. The only way to disable this clock is to move > + * it topll3_sw_clk and then to disable pll3_sw_clk > + * Make sure periph2_clk2_sel is set to pll3_sw_clk > + */ > + reg = readl_relaxed(ccm_base + 0x18); > + reg &= ~(1 << 20); > + writel_relaxed(reg, ccm_base + 0x18); > + > + /* Set MMDC_CH1 mask bit */ > + reg = readl_relaxed(ccm_base + 0x4); > + reg |= 1 << 16; > + writel_relaxed(reg, ccm_base + 0x4); > + > + /* > + * Set the periph2_clk_sel to the top mux so that > + * mmdc_ch1 is from pll3_sw_clk. > + */ > + reg = readl_relaxed(ccm_base + 0x14); > + reg |= 1 << 26; > + writel_relaxed(reg, ccm_base + 0x14); > + > + /* Wait for the clock switch */ > + while (readl_relaxed(ccm_base + 0x48)) > + ; > + > + /* Disable pll3_sw_clk by selecting the bypass clock source */ > + reg = readl_relaxed(ccm_base + 0xc); > + reg |= 1 << 0; > + writel_relaxed(reg, ccm_base + 0xc); > + > + /* Set the ldb_di0_clk and ldb_di1_clk to 111b */ > + reg = readl_relaxed(ccm_base + 0x2c); > + reg |= ((7 << 9) | (7 << 12)); > + writel_relaxed(reg, ccm_base + 0x2c); > + > + /* Set the ldb_di0_clk and ldb_di1_clk to 100b */ > + reg = readl_relaxed(ccm_base + 0x2c); > + reg &= ~((7 << 9) | (7 << 12)); > + reg |= ((4 << 9) | (4 << 12)); > + writel_relaxed(reg, ccm_base + 0x2c); > + > + /* Perform the LDB parent clock switch */ > + clk_set_parent(clk[ldb_di0_sel], clk[new_parent]); > + clk_set_parent(clk[ldb_di1_sel], clk[new_parent]); > + > + /* Unbypass pll3_sw_clk */ > + reg = readl_relaxed(ccm_base + 0xc); > + reg &= ~(1 << 0); > + writel_relaxed(reg, ccm_base + 0xc); > + > + /* > + * Set the periph2_clk_sel back to the bottom mux so that > + * mmdc_ch1 is from its original parent. > + */ > + reg = readl_relaxed(ccm_base + 0x14); > + reg &= ~(1 << 26); > + writel_relaxed(reg, ccm_base + 0x14); > + > + /* Wait for the clock switch */ > + while (readl_relaxed(ccm_base + 0x48)) > + ; > + > + /* Clear MMDC_CH1 mask bit */ > + reg = readl_relaxed(ccm_base + 0x4); > + reg &= ~(1 << 16); > + writel_relaxed(reg, ccm_base + 0x4); > +} > + > +static void disable_anatop_clocks(void __iomem *anatop_base) > +{ > + unsigned int reg; > + > + /* Make sure PFDs are disabled at boot. */ > + reg = readl_relaxed(anatop_base + 0x100); > + /* Cannot disable pll2_pfd2_396M, as it is the MMDC clock in iMX6DL */ > + if (cpu_is_imx6dl()) > + reg |= 0x80008080; > + else > + reg |= 0x80808080; > + writel_relaxed(reg, anatop_base + 0x100); > + > + reg = readl_relaxed(anatop_base + 0xf0); > + reg |= 0x80808080; > + writel_relaxed(reg, anatop_base + 0xf0); > + > + /* Make sure PLLs is disabled */ > + reg = readl_relaxed(anatop_base + 0xa0); > + reg &= ~(1 << 13); > + writel_relaxed(reg, anatop_base + 0xa0); > +} > + > static void __init imx6q_clocks_init(struct device_node *ccm_node) > { > struct device_node *np; > @@ -232,6 +349,8 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node) > clk[pll5_post_div] = clk_register_divider_table(NULL, "pll5_post_div", "pll5_video", CLK_SET_RATE_PARENT, base + 0xa0, 19, 2, 0, post_div_table, &imx_ccm_lock); > clk[pll5_video_div] = clk_register_divider_table(NULL, "pll5_video_div", "pll5_post_div", CLK_SET_RATE_PARENT, base + 0x170, 30, 2, 0, video_div_table, &imx_ccm_lock); > > + disable_anatop_clocks(base); > + > np = ccm_node; > base = of_iomap(np, 0); > WARN_ON(!base); > @@ -440,10 +559,8 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node) > clk_register_clkdev(clk[enet_ref], "enet_ref", NULL); > > if ((imx_get_soc_revision() != IMX_CHIP_REVISION_1_0) || > - cpu_is_imx6dl()) { > - clk_set_parent(clk[ldb_di0_sel], clk[pll5_video_div]); > - clk_set_parent(clk[ldb_di1_sel], clk[pll5_video_div]); > - } > + cpu_is_imx6dl()) > + init_ldb_clks(pll5_video_div); > > /* > * The gpmi needs 100MHz frequency in the EDO/Sync mode, > -- > 1.8.3.2 > My company is affected by that issue and we got this patch some days ago from Fabio - thanks. We used that time to test this patch in an automatic environment. Without this patch about one of 10 reboots (warm/cold boot does not matter) ended with a blank screen (no LVDS clock and data). With the patch applied we did 1800 cold boots without any issues. Tested-by: Christian Gmeiner <christian.gmeiner@gmail.com> -- Christian Gmeiner, MSc https://soundcloud.com/christian-gmeiner
On Wed, Apr 9, 2014 at 10:34 AM, Shawn Guo <shawn.guo@freescale.com> wrote: > I'm not sure this is a full/correct fix to the problem. You assume that > the re-parenting only happens during boot for once? What about the Yes, correct. As far as I can see LDB parent is only set once in clk-imx6q. > re-parenting triggered by the clk_set_parent() call? Where do you see the LDB clock parent to change via clk_set_parent() call? In imx_ldb.c we use clk_set_parent() to set the di parent, not ldb parent.
On Wed, Apr 09, 2014 at 11:20:28AM -0300, Fabio Estevam wrote: > On Wed, Apr 9, 2014 at 10:34 AM, Shawn Guo <shawn.guo@freescale.com> wrote: > > > I'm not sure this is a full/correct fix to the problem. You assume that > > the re-parenting only happens during boot for once? What about the > > Yes, correct. As far as I can see LDB parent is only set once in clk-imx6q. > > > re-parenting triggered by the clk_set_parent() call? > > Where do you see the LDB clock parent to change via clk_set_parent() call? > > In imx_ldb.c we use clk_set_parent() to set the di parent, not ldb parent. As long as you call clk_register_mux() to register a multiplexer, you have to ensure that clk_set_parent() call always works properly on it, no matter whether there is one actually calling into it right now. Furthermore, some re-parenting happens in a way you may not be aware of. See commit e366fdd (clk: clk-mux: implement remuxing on set_rate), for example. Shawn
Hi Shawn, On Wed, Apr 9, 2014 at 11:59 AM, Shawn Guo <shawn.guo@freescale.com> wrote: > As long as you call clk_register_mux() to register a multiplexer, you > have to ensure that clk_set_parent() call always works properly on it, > no matter whether there is one actually calling into it right now. clk_register_mux() is not called for the ldb_di_sels multiplexer: **** Clk name is lvds1_sel ********* Calling clk_register_mux **** Clk name is lvds2_sel ********* Calling clk_register_mux **** Clk name is step ********* Calling clk_register_mux **** Clk name is pll1_sw ********* Calling clk_register_mux **** Clk name is periph_pre ********* Calling clk_register_mux **** Clk name is periph2_pre ********* Calling clk_register_mux **** Clk name is periph_clk2_sel ********* Calling clk_register_mux **** Clk name is periph2_clk2_sel ********* Calling clk_register_mux **** Clk name is axi_sel ********* Calling clk_register_mux **** Clk name is esai_sel ********* Calling clk_register_mux **** Clk name is asrc_sel ********* Calling clk_register_mux **** Clk name is spdif_sel ********* Calling clk_register_mux **** Clk name is gpu2d_axi ********* Calling clk_register_mux **** Clk name is gpu3d_axi ********* Calling clk_register_mux **** Clk name is gpu2d_core_sel ********* Calling clk_register_mux **** Clk name is gpu3d_core_sel ********* Calling clk_register_mux **** Clk name is gpu3d_shader_sel ********* Calling clk_register_mux **** Clk name is ipu1_sel ********* Calling clk_register_mux **** Clk name is ipu2_sel ********* Calling clk_register_mux ********* Calling clk_register_mux ********* Calling clk_register_mux **** Clk name is ipu1_di0_pre_sel ********* Calling clk_register_mux **** Clk name is ipu1_di1_pre_sel ********* Calling clk_register_mux **** Clk name is ipu2_di0_pre_sel ********* Calling clk_register_mux **** Clk name is ipu2_di1_pre_sel ********* Calling clk_register_mux **** Clk name is ipu1_di0_sel ********* Calling clk_register_mux **** Clk name is ipu1_di1_sel ********* Calling clk_register_mux **** Clk name is ipu2_di0_sel ********* Calling clk_register_mux **** Clk name is ipu2_di1_sel ********* Calling clk_register_mux **** Clk name is hsi_tx_sel ********* Calling clk_register_mux **** Clk name is pcie_axi_sel ********* Calling clk_register_mux **** Clk name is enfc_sel ********* Calling clk_register_mux **** Clk name is vdo_axi_sel ********* Calling clk_register_mux **** Clk name is vpu_axi_sel ********* Calling clk_register_mux **** Clk name is cko1_sel ********* Calling clk_register_mux **** Clk name is cko2_sel ********* Calling clk_register_mux **** Clk name is cko ********* Calling clk_register_mux ********* doing ldb clock switch > > Furthermore, some re-parenting happens in a way you may not be aware of. > See commit e366fdd (clk: clk-mux: implement remuxing on set_rate), for > example. This commit does not affect us as we pass the CLK_SET_RATE_NO_REPARENT flag.
On Wed, Apr 09, 2014 at 12:28:50PM -0300, Fabio Estevam wrote: > Hi Shawn, > > On Wed, Apr 9, 2014 at 11:59 AM, Shawn Guo <shawn.guo@freescale.com> wrote: > > > As long as you call clk_register_mux() to register a multiplexer, you > > have to ensure that clk_set_parent() call always works properly on it, > > no matter whether there is one actually calling into it right now. > > clk_register_mux() is not called for the ldb_di_sels multiplexer: Are you sure about that? What are the following two calls in clk-imx6q.c doing then? clk[ldb_di0_sel] = imx_clk_mux_flags(...); clk[ldb_di1_sel] = imx_clk_mux_flags(...); > > **** Clk name is lvds1_sel > ********* Calling clk_register_mux > **** Clk name is lvds2_sel > ********* Calling clk_register_mux > **** Clk name is step > ********* Calling clk_register_mux > **** Clk name is pll1_sw > ********* Calling clk_register_mux > **** Clk name is periph_pre > ********* Calling clk_register_mux > **** Clk name is periph2_pre > ********* Calling clk_register_mux > **** Clk name is periph_clk2_sel > ********* Calling clk_register_mux > **** Clk name is periph2_clk2_sel > ********* Calling clk_register_mux > **** Clk name is axi_sel > ********* Calling clk_register_mux > **** Clk name is esai_sel > ********* Calling clk_register_mux > **** Clk name is asrc_sel > ********* Calling clk_register_mux > **** Clk name is spdif_sel > ********* Calling clk_register_mux > **** Clk name is gpu2d_axi > ********* Calling clk_register_mux > **** Clk name is gpu3d_axi > ********* Calling clk_register_mux > **** Clk name is gpu2d_core_sel > ********* Calling clk_register_mux > **** Clk name is gpu3d_core_sel > ********* Calling clk_register_mux > **** Clk name is gpu3d_shader_sel > ********* Calling clk_register_mux > **** Clk name is ipu1_sel > ********* Calling clk_register_mux > **** Clk name is ipu2_sel > ********* Calling clk_register_mux > ********* Calling clk_register_mux > ********* Calling clk_register_mux Oh, why clk_register_mux() is being called without a clock name in above three? > **** Clk name is ipu1_di0_pre_sel > ********* Calling clk_register_mux > **** Clk name is ipu1_di1_pre_sel > ********* Calling clk_register_mux > **** Clk name is ipu2_di0_pre_sel > ********* Calling clk_register_mux > **** Clk name is ipu2_di1_pre_sel > ********* Calling clk_register_mux > **** Clk name is ipu1_di0_sel > ********* Calling clk_register_mux > **** Clk name is ipu1_di1_sel > ********* Calling clk_register_mux > **** Clk name is ipu2_di0_sel > ********* Calling clk_register_mux > **** Clk name is ipu2_di1_sel > ********* Calling clk_register_mux > **** Clk name is hsi_tx_sel > ********* Calling clk_register_mux > **** Clk name is pcie_axi_sel > ********* Calling clk_register_mux > **** Clk name is enfc_sel > ********* Calling clk_register_mux > **** Clk name is vdo_axi_sel > ********* Calling clk_register_mux > **** Clk name is vpu_axi_sel > ********* Calling clk_register_mux > **** Clk name is cko1_sel > ********* Calling clk_register_mux > **** Clk name is cko2_sel > ********* Calling clk_register_mux > **** Clk name is cko > ********* Calling clk_register_mux > ********* doing ldb clock switch For the record, here is my printk gives. *** clk_register_mux: lvds1_sel *** clk_register_mux: lvds2_sel *** clk_register_mux: step *** clk_register_mux: pll1_sw *** clk_register_mux: periph_pre *** clk_register_mux: periph2_pre *** clk_register_mux: periph_clk2_sel *** clk_register_mux: periph2_clk2_sel *** clk_register_mux: axi_sel *** clk_register_mux: esai_sel *** clk_register_mux: asrc_sel *** clk_register_mux: spdif_sel *** clk_register_mux: gpu2d_axi *** clk_register_mux: gpu3d_axi *** clk_register_mux: gpu2d_core_sel *** clk_register_mux: gpu3d_core_sel *** clk_register_mux: gpu3d_shader_sel *** clk_register_mux: ipu1_sel *** clk_register_mux: ipu2_sel *** clk_register_mux: ldb_di0_sel *** clk_register_mux: ldb_di1_sel *** clk_register_mux: ipu1_di0_pre_sel *** clk_register_mux: ipu1_di1_pre_sel *** clk_register_mux: ipu2_di0_pre_sel *** clk_register_mux: ipu2_di1_pre_sel *** clk_register_mux: ipu1_di0_sel *** clk_register_mux: ipu1_di1_sel *** clk_register_mux: ipu2_di0_sel *** clk_register_mux: ipu2_di1_sel *** clk_register_mux: hsi_tx_sel *** clk_register_mux: pcie_axi_sel *** clk_register_mux: enfc_sel *** clk_register_mux: vdo_axi_sel *** clk_register_mux: vpu_axi_sel *** clk_register_mux: cko1_sel *** clk_register_mux: cko2_sel *** clk_register_mux: cko > > > > > Furthermore, some re-parenting happens in a way you may not be aware of. > > See commit e366fdd (clk: clk-mux: implement remuxing on set_rate), for > > example. > > This commit does not affect us as we pass the CLK_SET_RATE_NO_REPARENT flag. You did not get my point. This is just an example, and we happen to set this flag for now. My point is that as long as you register a clk to clock framework, you do not have a way to stop one from calling clk API on the clock then. This is how clk framework and API work, simple as it is. Shawn
On Wed, Apr 09, 2014 at 10:55:44PM -0300, Fabio Estevam wrote: > On Wed, Apr 9, 2014 at 10:21 PM, Shawn Guo <shawn.guo@freescale.com> wrote: > > > For the record, here is my printk gives. > ... > > *** clk_register_mux: ldb_di0_sel > > *** clk_register_mux: ldb_di1_sel > > Ok, I ran it again and yes, I can see it now. Sorry for the previous > wrong printk's. > > >> > >> > > >> > Furthermore, some re-parenting happens in a way you may not be aware of. > >> > See commit e366fdd (clk: clk-mux: implement remuxing on set_rate), for > >> > example. > >> > >> This commit does not affect us as we pass the CLK_SET_RATE_NO_REPARENT flag. > > > > You did not get my point. This is just an example, and we happen to set > > this flag for now. My point is that as long as you register a clk to > > clock framework, you do not have a way to stop one from calling clk > > API on the clock then. This is how clk framework and API work, simple > > as it is. > > The issue that this patch wants to solve is that we need to perform > this protection clock switching mechanism prior to doing the > clk_set_parent for the ldb clocks. You should fix .set_parent() of the clock then. > > Putting a printk in clk_set_parent like this: > > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1719,6 +1719,8 @@ int clk_set_parent(struct clk *clk, struct clk *parent) > if (!clk->ops) > return -EINVAL; > > + pr_info(" *** Calling clk_set_parent for %s clock\n", clk->name); > + > /* verify ops for for multi-parent clks */ > if ((clk->num_parents > 1) && (!clk->ops->set_parent)) > return -ENOSYS; > ,results in: > > *** Calling clk_set_parent for ldb_di0_sel clock > *** Calling clk_set_parent for ldb_di1_sel clock > *** Calling clk_set_parent for enfc_sel clock > *** Calling clk_set_parent for cko2_sel clock > *** Calling clk_set_parent for cko clock > *** Calling clk_set_parent for spdif_sel clock > *** Calling clk_set_parent for lvds1_sel clock > *** Calling clk_set_parent for ipu1_di0_sel clock > > Which shows that there is only one clk_set_parent being called for the > ldb_di clocks and this one is in clk-mx6q.c. > > So it is safe to perform this workaround in clk-imx6q.c. > > Don't you agree? I disagree. It's only safe for now. How do you prevent the new clk_set_parent() call on the clock in the future. This is not something that we can maintain. Shawn
Hi Fabio and Ranjani, On 09.04.2014 13:55, Fabio Estevam wrote: > From: Fabio Estevam <fabio.estevam@freescale.com> > > Due to incorrect placement of the clock gate cell in the ldb_di[x]_clk tree, > the glitchy parent mux of ldb_di[x]_clk can cause a glitch to enter the > ldb_di_ipu_div divider. If the divider gets locked up, no ldb_di[x]_clk is > generated, and the LVDS display will hang when the ipu_di_clk is sourced from > ldb_di_clk. > > To fix the problem, both the new and current parent of the ldb_di_clk should > be disabled before the switch. This patch ensures that correct steps are > followed when ldb_di_clk parent is switched in the beginning of boot. > > Signed-off-by: Ranjani Vaidyanathan <Ranjani.Vaidyanathan@freescale.com> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- > arch/arm/mach-imx/clk-imx6q.c | 125 ++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 121 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c > index 20ad0d1..3ee45f4 100644 > --- a/arch/arm/mach-imx/clk-imx6q.c > +++ b/arch/arm/mach-imx/clk-imx6q.c > @@ -140,6 +140,123 @@ static struct clk_div_table video_div_table[] = { > { /* sentinel */ } > }; > > +static void init_ldb_clks(enum mx6q_clks new_parent) > +{ > + struct device_node *np; > + static void __iomem *ccm_base; > + unsigned int reg; > + > + np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-ccm"); > + ccm_base = of_iomap(np, 0); > + WARN_ON(!ccm_base); > + > + /* > + * Need to follow a strict procedure when changing the LDB > + * clock, else we can introduce a glitch. Things to keep in > + * mind: > + * 1. The current and new parent clocks must be disabled. > + * 2. The default clock for ldb_dio_clk is mmdc_ch1 which has > + * no CG bit. > + * 3. In the RTL implementation of the LDB_DI_CLK_SEL mux > + * the top four options are in one mux and the PLL3 option along > + * with another option is in the second mux. There is third mux > + * used to decide between the first and second mux. > + * The code below switches the parent to the bottom mux first > + * and then manipulates the top mux. This ensures that no glitch > + * will enter the divider. > + * > + * Need to disable MMDC_CH1 clock manually as there is no CG bit > + * for this clock. The only way to disable this clock is to move > + * it topll3_sw_clk and then to disable pll3_sw_clk > + * Make sure periph2_clk2_sel is set to pll3_sw_clk > + */ > + reg = readl_relaxed(ccm_base + 0x18); > + reg &= ~(1 << 20); > + writel_relaxed(reg, ccm_base + 0x18); > + > + /* Set MMDC_CH1 mask bit */ > + reg = readl_relaxed(ccm_base + 0x4); > + reg |= 1 << 16; > + writel_relaxed(reg, ccm_base + 0x4); Just a hopefully simple question: Why do you mask MMDC_CH1 *after* switching to pll3_sw_clk above? Why not mask first, and then switch periph2_clk2_sel to pll3_sw_clk? Thanks Dirk
Hi, Fabio Estevam wrote: > From: Fabio Estevam <fabio.estevam@freescale.com> > > Due to incorrect placement of the clock gate cell in the ldb_di[x]_clk tree, > the glitchy parent mux of ldb_di[x]_clk can cause a glitch to enter the > ldb_di_ipu_div divider. If the divider gets locked up, no ldb_di[x]_clk is > generated, and the LVDS display will hang when the ipu_di_clk is sourced from > ldb_di_clk. > > To fix the problem, both the new and current parent of the ldb_di_clk should > be disabled before the switch. This patch ensures that correct steps are > followed when ldb_di_clk parent is switched in the beginning of boot. > > Signed-off-by: Ranjani Vaidyanathan <Ranjani.Vaidyanathan@freescale.com> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- > arch/arm/mach-imx/clk-imx6q.c | 125 ++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 121 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c > index 20ad0d1..3ee45f4 100644 > --- a/arch/arm/mach-imx/clk-imx6q.c > +++ b/arch/arm/mach-imx/clk-imx6q.c > @@ -140,6 +140,123 @@ static struct clk_div_table video_div_table[] = { > { /* sentinel */ } > }; > > +static void init_ldb_clks(enum mx6q_clks new_parent) > +{ > + struct device_node *np; > + static void __iomem *ccm_base; > + unsigned int reg; > + > + np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-ccm"); > + ccm_base = of_iomap(np, 0); > + WARN_ON(!ccm_base); > + > + /* > + * Need to follow a strict procedure when changing the LDB > + * clock, else we can introduce a glitch. Things to keep in > + * mind: > + * 1. The current and new parent clocks must be disabled. > + * 2. The default clock for ldb_dio_clk is mmdc_ch1 which has > + * no CG bit. > + * 3. In the RTL implementation of the LDB_DI_CLK_SEL mux > + * the top four options are in one mux and the PLL3 option along > + * with another option is in the second mux. There is third mux > + * used to decide between the first and second mux. > + * The code below switches the parent to the bottom mux first > + * and then manipulates the top mux. This ensures that no glitch > + * will enter the divider. > + * > + * Need to disable MMDC_CH1 clock manually as there is no CG bit > + * for this clock. The only way to disable this clock is to move > + * it topll3_sw_clk and then to disable pll3_sw_clk > + * Make sure periph2_clk2_sel is set to pll3_sw_clk > + */ > + reg = readl_relaxed(ccm_base + 0x18); > + reg &= ~(1 << 20); > + writel_relaxed(reg, ccm_base + 0x18); > + > + /* Set MMDC_CH1 mask bit */ > + reg = readl_relaxed(ccm_base + 0x4); > + reg |= 1 << 16; > + writel_relaxed(reg, ccm_base + 0x4); > + symbolic register offsets instead of magic numbers would make the code more readable and less error prone! Lothar Waßmann
Hi Dirk, There is no reason to mask the MMDC_CH1 handshake-mask after the periph2_clk2_sel bit has been set/cleared. It can be done before this step too. Only need to make sure that the MMDC handshake-mask is set before switching periph2_clk_sel (bit 26 in CCM_CBCDR). Thanks, Ranjani -----Original Message----- From: Dirk Behme [mailto:dirk.behme@de.bosch.com] Sent: Monday, May 19, 2014 2:22 AM To: Fabio Estevam; Vaidyanathan Ranjani-RA5478 Cc: Guo Shawn-R65073; Estevam Fabio-R49496; christian.gmeiner@gmail.com; linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] ARM: imx6: Fix procedure to switch the parent of LDB_DI_CLK Hi Fabio and Ranjani, On 09.04.2014 13:55, Fabio Estevam wrote: > From: Fabio Estevam <fabio.estevam@freescale.com> > > Due to incorrect placement of the clock gate cell in the ldb_di[x]_clk > tree, the glitchy parent mux of ldb_di[x]_clk can cause a glitch to > enter the ldb_di_ipu_div divider. If the divider gets locked up, no > ldb_di[x]_clk is generated, and the LVDS display will hang when the > ipu_di_clk is sourced from ldb_di_clk. > > To fix the problem, both the new and current parent of the ldb_di_clk > should be disabled before the switch. This patch ensures that correct > steps are followed when ldb_di_clk parent is switched in the beginning of boot. > > Signed-off-by: Ranjani Vaidyanathan > <Ranjani.Vaidyanathan@freescale.com> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- > arch/arm/mach-imx/clk-imx6q.c | 125 ++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 121 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-imx/clk-imx6q.c > b/arch/arm/mach-imx/clk-imx6q.c index 20ad0d1..3ee45f4 100644 > --- a/arch/arm/mach-imx/clk-imx6q.c > +++ b/arch/arm/mach-imx/clk-imx6q.c > @@ -140,6 +140,123 @@ static struct clk_div_table video_div_table[] = { > { /* sentinel */ } > }; > > +static void init_ldb_clks(enum mx6q_clks new_parent) { > + struct device_node *np; > + static void __iomem *ccm_base; > + unsigned int reg; > + > + np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-ccm"); > + ccm_base = of_iomap(np, 0); > + WARN_ON(!ccm_base); > + > + /* > + * Need to follow a strict procedure when changing the LDB > + * clock, else we can introduce a glitch. Things to keep in > + * mind: > + * 1. The current and new parent clocks must be disabled. > + * 2. The default clock for ldb_dio_clk is mmdc_ch1 which has > + * no CG bit. > + * 3. In the RTL implementation of the LDB_DI_CLK_SEL mux > + * the top four options are in one mux and the PLL3 option along > + * with another option is in the second mux. There is third mux > + * used to decide between the first and second mux. > + * The code below switches the parent to the bottom mux first > + * and then manipulates the top mux. This ensures that no glitch > + * will enter the divider. > + * > + * Need to disable MMDC_CH1 clock manually as there is no CG bit > + * for this clock. The only way to disable this clock is to move > + * it topll3_sw_clk and then to disable pll3_sw_clk > + * Make sure periph2_clk2_sel is set to pll3_sw_clk > + */ > + reg = readl_relaxed(ccm_base + 0x18); > + reg &= ~(1 << 20); > + writel_relaxed(reg, ccm_base + 0x18); > + > + /* Set MMDC_CH1 mask bit */ > + reg = readl_relaxed(ccm_base + 0x4); > + reg |= 1 << 16; > + writel_relaxed(reg, ccm_base + 0x4); Just a hopefully simple question: Why do you mask MMDC_CH1 *after* switching to pll3_sw_clk above? Why not mask first, and then switch periph2_clk2_sel to pll3_sw_clk? Thanks Dirk
On 09.04.2014 13:55, Fabio Estevam wrote: > From: Fabio Estevam <fabio.estevam@freescale.com> > > Due to incorrect placement of the clock gate cell in the ldb_di[x]_clk tree, > the glitchy parent mux of ldb_di[x]_clk can cause a glitch to enter the > ldb_di_ipu_div divider. If the divider gets locked up, no ldb_di[x]_clk is > generated, and the LVDS display will hang when the ipu_di_clk is sourced from > ldb_di_clk. > > To fix the problem, both the new and current parent of the ldb_di_clk should > be disabled before the switch. This patch ensures that correct steps are > followed when ldb_di_clk parent is switched in the beginning of boot. > > Signed-off-by: Ranjani Vaidyanathan <Ranjani.Vaidyanathan@freescale.com> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- > arch/arm/mach-imx/clk-imx6q.c | 125 ++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 121 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c > index 20ad0d1..3ee45f4 100644 > --- a/arch/arm/mach-imx/clk-imx6q.c > +++ b/arch/arm/mach-imx/clk-imx6q.c .... > +static void disable_anatop_clocks(void __iomem *anatop_base) > +{ > + unsigned int reg; > + > + /* Make sure PFDs are disabled at boot. */ > + reg = readl_relaxed(anatop_base + 0x100); > + /* Cannot disable pll2_pfd2_396M, as it is the MMDC clock in iMX6DL */ > + if (cpu_is_imx6dl()) > + reg |= 0x80008080; > + else > + reg |= 0x80808080; There are two issues with this code section: 1) You want to know here if pll2_pfd2_396M is used as MMDC clock. This is independent of IMX6DL. Even a Quad/Dual could use pll2_pfd2_396M as MMDC clock. You better should check what you are really looking for: if (MMDC clock == pll2_pfd2_396M) ... 2) Why is bit 31, i.e. the uppermost '8' in 0x80808080, set here? It looks like the TRM marks bit 31 - 24 as reserved? Best regards Dirk
Hi Dirk, Responses below. Thanks, Ranjani -----Original Message----- From: Dirk Behme [mailto:dirk.behme@gmail.com] Sent: Wednesday, June 04, 2014 11:37 AM To: Vaidyanathan Ranjani-RA5478; Estevam Fabio-R49496 Cc: Fabio Estevam; Guo Shawn-R65073; christian.gmeiner@gmail.com; linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] ARM: imx6: Fix procedure to switch the parent of LDB_DI_CLK On 09.04.2014 13:55, Fabio Estevam wrote: > From: Fabio Estevam <fabio.estevam@freescale.com> > > Due to incorrect placement of the clock gate cell in the ldb_di[x]_clk > tree, the glitchy parent mux of ldb_di[x]_clk can cause a glitch to > enter the ldb_di_ipu_div divider. If the divider gets locked up, no > ldb_di[x]_clk is generated, and the LVDS display will hang when the > ipu_di_clk is sourced from ldb_di_clk. > > To fix the problem, both the new and current parent of the ldb_di_clk > should be disabled before the switch. This patch ensures that correct > steps are followed when ldb_di_clk parent is switched in the beginning of boot. > > Signed-off-by: Ranjani Vaidyanathan > <Ranjani.Vaidyanathan@freescale.com> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- > arch/arm/mach-imx/clk-imx6q.c | 125 ++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 121 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-imx/clk-imx6q.c > b/arch/arm/mach-imx/clk-imx6q.c index 20ad0d1..3ee45f4 100644 > --- a/arch/arm/mach-imx/clk-imx6q.c > +++ b/arch/arm/mach-imx/clk-imx6q.c .... > +static void disable_anatop_clocks(void __iomem *anatop_base) { > + unsigned int reg; > + > + /* Make sure PFDs are disabled at boot. */ > + reg = readl_relaxed(anatop_base + 0x100); > + /* Cannot disable pll2_pfd2_396M, as it is the MMDC clock in iMX6DL */ > + if (cpu_is_imx6dl()) > + reg |= 0x80008080; > + else > + reg |= 0x80808080; There are two issues with this code section: 1) You want to know here if pll2_pfd2_396M is used as MMDC clock. This is independent of IMX6DL. Even a Quad/Dual could use pll2_pfd2_396M as MMDC clock. You better should check what you are really looking for: if (MMDC clock == pll2_pfd2_396M) ... [RV] Yes, you are perfectly correct. We need to check MMDC parent clock. 2) Why is bit 31, i.e. the uppermost '8' in 0x80808080, set here? It looks like the TRM marks bit 31 - 24 as reserved? [RV] Yes, this should also be fixed. Best regards Dirk
On 04.06.2014 19:29, Ranjani.Vaidyanathan@freescale.com wrote: > Hi Dirk, > > Responses below. > > Thanks, > Ranjani > > -----Original Message----- > From: Dirk Behme [mailto:dirk.behme@gmail.com] > Sent: Wednesday, June 04, 2014 11:37 AM > To: Vaidyanathan Ranjani-RA5478; Estevam Fabio-R49496 > Cc: Fabio Estevam; Guo Shawn-R65073; christian.gmeiner@gmail.com; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH] ARM: imx6: Fix procedure to switch the parent of LDB_DI_CLK > > On 09.04.2014 13:55, Fabio Estevam wrote: >> From: Fabio Estevam <fabio.estevam@freescale.com> >> >> Due to incorrect placement of the clock gate cell in the ldb_di[x]_clk >> tree, the glitchy parent mux of ldb_di[x]_clk can cause a glitch to >> enter the ldb_di_ipu_div divider. If the divider gets locked up, no >> ldb_di[x]_clk is generated, and the LVDS display will hang when the >> ipu_di_clk is sourced from ldb_di_clk. >> >> To fix the problem, both the new and current parent of the ldb_di_clk >> should be disabled before the switch. This patch ensures that correct >> steps are followed when ldb_di_clk parent is switched in the beginning of boot. >> >> Signed-off-by: Ranjani Vaidyanathan >> <Ranjani.Vaidyanathan@freescale.com> >> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> >> --- >> arch/arm/mach-imx/clk-imx6q.c | 125 ++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 121 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm/mach-imx/clk-imx6q.c >> b/arch/arm/mach-imx/clk-imx6q.c index 20ad0d1..3ee45f4 100644 >> --- a/arch/arm/mach-imx/clk-imx6q.c >> +++ b/arch/arm/mach-imx/clk-imx6q.c > .... >> +static void disable_anatop_clocks(void __iomem *anatop_base) { >> + unsigned int reg; >> + >> + /* Make sure PFDs are disabled at boot. */ >> + reg = readl_relaxed(anatop_base + 0x100); >> + /* Cannot disable pll2_pfd2_396M, as it is the MMDC clock in iMX6DL */ >> + if (cpu_is_imx6dl()) >> + reg |= 0x80008080; >> + else >> + reg |= 0x80808080; > > There are two issues with this code section: > > 1) You want to know here if pll2_pfd2_396M is used as MMDC clock. This is independent of IMX6DL. Even a Quad/Dual could use pll2_pfd2_396M as MMDC clock. You better should check what you are really looking for: > > if (MMDC clock == pll2_pfd2_396M) > ... > [RV] Yes, you are perfectly correct. We need to check MMDC parent clock. Just an additional question: Do we really need that check here? Why wouldn't it be sufficent to just do reg |= 0x00008080; ? Independent if pll2_pfd2_396M is used as MMDC clock or not? Or if we are on a DualLite or Quad/Dual? Best regards Dirk
On 04.06.2014 19:29, Ranjani.Vaidyanathan@freescale.com wrote: > Hi Dirk, > > Responses below. > > Thanks, > Ranjani > > -----Original Message----- > From: Dirk Behme [mailto:dirk.behme@gmail.com] > Sent: Wednesday, June 04, 2014 11:37 AM > To: Vaidyanathan Ranjani-RA5478; Estevam Fabio-R49496 > Cc: Fabio Estevam; Guo Shawn-R65073; christian.gmeiner@gmail.com; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH] ARM: imx6: Fix procedure to switch the parent of LDB_DI_CLK > > On 09.04.2014 13:55, Fabio Estevam wrote: >> From: Fabio Estevam <fabio.estevam@freescale.com> >> >> Due to incorrect placement of the clock gate cell in the ldb_di[x]_clk >> tree, the glitchy parent mux of ldb_di[x]_clk can cause a glitch to >> enter the ldb_di_ipu_div divider. If the divider gets locked up, no >> ldb_di[x]_clk is generated, and the LVDS display will hang when the >> ipu_di_clk is sourced from ldb_di_clk. >> >> To fix the problem, both the new and current parent of the ldb_di_clk >> should be disabled before the switch. This patch ensures that correct >> steps are followed when ldb_di_clk parent is switched in the beginning of boot. >> >> Signed-off-by: Ranjani Vaidyanathan >> <Ranjani.Vaidyanathan@freescale.com> >> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> >> --- >> arch/arm/mach-imx/clk-imx6q.c | 125 ++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 121 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm/mach-imx/clk-imx6q.c >> b/arch/arm/mach-imx/clk-imx6q.c index 20ad0d1..3ee45f4 100644 >> --- a/arch/arm/mach-imx/clk-imx6q.c >> +++ b/arch/arm/mach-imx/clk-imx6q.c > .... >> +static void disable_anatop_clocks(void __iomem *anatop_base) { >> + unsigned int reg; >> + >> + /* Make sure PFDs are disabled at boot. */ >> + reg = readl_relaxed(anatop_base + 0x100); >> + /* Cannot disable pll2_pfd2_396M, as it is the MMDC clock in iMX6DL */ >> + if (cpu_is_imx6dl()) >> + reg |= 0x80008080; >> + else >> + reg |= 0x80808080; > > There are two issues with this code section: > > 1) You want to know here if pll2_pfd2_396M is used as MMDC clock. This is independent of IMX6DL. Even a Quad/Dual could use pll2_pfd2_396M as MMDC clock. You better should check what you are really looking for: > > if (MMDC clock == pll2_pfd2_396M) > ... > [RV] Yes, you are perfectly correct. We need to check MMDC parent clock. > > 2) Why is bit 31, i.e. the uppermost '8' in 0x80808080, set here? It looks like the TRM marks bit 31 - 24 as reserved? > [RV] Yes, this should also be fixed. Ok, thanks! We'll test if (clk_get_parent(clk[periph_pre]) == clk[pll2_pfd2_396m]) reg |= 0x00008080; else reg |= 0x00808080; then. Remains the question why this check is necessary at all. Maybe just reg |= 0x00008080; is sufficient here? Thanks Dirk
Hi Dirk, Response below. Ranjani -----Original Message----- From: Dirk Behme [mailto:dirk.behme@gmail.com] Sent: Wednesday, June 04, 2014 12:49 PM To: Vaidyanathan Ranjani-RA5478; Estevam Fabio-R49496 Cc: Fabio Estevam; Guo Shawn-R65073; christian.gmeiner@gmail.com; linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] ARM: imx6: Fix procedure to switch the parent of LDB_DI_CLK On 04.06.2014 19:29, Ranjani.Vaidyanathan@freescale.com wrote: > Hi Dirk, > > Responses below. > > Thanks, > Ranjani > > -----Original Message----- > From: Dirk Behme [mailto:dirk.behme@gmail.com] > Sent: Wednesday, June 04, 2014 11:37 AM > To: Vaidyanathan Ranjani-RA5478; Estevam Fabio-R49496 > Cc: Fabio Estevam; Guo Shawn-R65073; christian.gmeiner@gmail.com; > linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH] ARM: imx6: Fix procedure to switch the parent of > LDB_DI_CLK > > On 09.04.2014 13:55, Fabio Estevam wrote: >> From: Fabio Estevam <fabio.estevam@freescale.com> >> >> Due to incorrect placement of the clock gate cell in the >> ldb_di[x]_clk tree, the glitchy parent mux of ldb_di[x]_clk can cause >> a glitch to enter the ldb_di_ipu_div divider. If the divider gets >> locked up, no ldb_di[x]_clk is generated, and the LVDS display will >> hang when the ipu_di_clk is sourced from ldb_di_clk. >> >> To fix the problem, both the new and current parent of the ldb_di_clk >> should be disabled before the switch. This patch ensures that correct >> steps are followed when ldb_di_clk parent is switched in the beginning of boot. >> >> Signed-off-by: Ranjani Vaidyanathan >> <Ranjani.Vaidyanathan@freescale.com> >> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> >> --- >> arch/arm/mach-imx/clk-imx6q.c | 125 ++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 121 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm/mach-imx/clk-imx6q.c >> b/arch/arm/mach-imx/clk-imx6q.c index 20ad0d1..3ee45f4 100644 >> --- a/arch/arm/mach-imx/clk-imx6q.c >> +++ b/arch/arm/mach-imx/clk-imx6q.c > .... >> +static void disable_anatop_clocks(void __iomem *anatop_base) { >> + unsigned int reg; >> + >> + /* Make sure PFDs are disabled at boot. */ >> + reg = readl_relaxed(anatop_base + 0x100); >> + /* Cannot disable pll2_pfd2_396M, as it is the MMDC clock in iMX6DL */ >> + if (cpu_is_imx6dl()) >> + reg |= 0x80008080; >> + else >> + reg |= 0x80808080; > > There are two issues with this code section: > > 1) You want to know here if pll2_pfd2_396M is used as MMDC clock. This is independent of IMX6DL. Even a Quad/Dual could use pll2_pfd2_396M as MMDC clock. You better should check what you are really looking for: > > if (MMDC clock == pll2_pfd2_396M) > ... > [RV] Yes, you are perfectly correct. We need to check MMDC parent clock. Just an additional question: Do we really need that check here? Why wouldn't it be sufficent to just do reg |= 0x00008080; ? Independent if pll2_pfd2_396M is used as MMDC clock or not? Or if we are on a DualLite or Quad/Dual? [RV] For mx6q, if mmdc is not from pll2_pfd2_396M, I would prefer we gate it at boot. That way the clock use count is maintained. And we don't want pll2_pfd2_396M to be an always-on clock. On mx6dl, there is no option, MMDC_CLK can only come from ppl2_pfd2. Since its possible that MX6Q can use pll2_pfd2, we should check the source of mmdc. Best regards Dirk
diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c index 20ad0d1..3ee45f4 100644 --- a/arch/arm/mach-imx/clk-imx6q.c +++ b/arch/arm/mach-imx/clk-imx6q.c @@ -140,6 +140,123 @@ static struct clk_div_table video_div_table[] = { { /* sentinel */ } }; +static void init_ldb_clks(enum mx6q_clks new_parent) +{ + struct device_node *np; + static void __iomem *ccm_base; + unsigned int reg; + + np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-ccm"); + ccm_base = of_iomap(np, 0); + WARN_ON(!ccm_base); + + /* + * Need to follow a strict procedure when changing the LDB + * clock, else we can introduce a glitch. Things to keep in + * mind: + * 1. The current and new parent clocks must be disabled. + * 2. The default clock for ldb_dio_clk is mmdc_ch1 which has + * no CG bit. + * 3. In the RTL implementation of the LDB_DI_CLK_SEL mux + * the top four options are in one mux and the PLL3 option along + * with another option is in the second mux. There is third mux + * used to decide between the first and second mux. + * The code below switches the parent to the bottom mux first + * and then manipulates the top mux. This ensures that no glitch + * will enter the divider. + * + * Need to disable MMDC_CH1 clock manually as there is no CG bit + * for this clock. The only way to disable this clock is to move + * it topll3_sw_clk and then to disable pll3_sw_clk + * Make sure periph2_clk2_sel is set to pll3_sw_clk + */ + reg = readl_relaxed(ccm_base + 0x18); + reg &= ~(1 << 20); + writel_relaxed(reg, ccm_base + 0x18); + + /* Set MMDC_CH1 mask bit */ + reg = readl_relaxed(ccm_base + 0x4); + reg |= 1 << 16; + writel_relaxed(reg, ccm_base + 0x4); + + /* + * Set the periph2_clk_sel to the top mux so that + * mmdc_ch1 is from pll3_sw_clk. + */ + reg = readl_relaxed(ccm_base + 0x14); + reg |= 1 << 26; + writel_relaxed(reg, ccm_base + 0x14); + + /* Wait for the clock switch */ + while (readl_relaxed(ccm_base + 0x48)) + ; + + /* Disable pll3_sw_clk by selecting the bypass clock source */ + reg = readl_relaxed(ccm_base + 0xc); + reg |= 1 << 0; + writel_relaxed(reg, ccm_base + 0xc); + + /* Set the ldb_di0_clk and ldb_di1_clk to 111b */ + reg = readl_relaxed(ccm_base + 0x2c); + reg |= ((7 << 9) | (7 << 12)); + writel_relaxed(reg, ccm_base + 0x2c); + + /* Set the ldb_di0_clk and ldb_di1_clk to 100b */ + reg = readl_relaxed(ccm_base + 0x2c); + reg &= ~((7 << 9) | (7 << 12)); + reg |= ((4 << 9) | (4 << 12)); + writel_relaxed(reg, ccm_base + 0x2c); + + /* Perform the LDB parent clock switch */ + clk_set_parent(clk[ldb_di0_sel], clk[new_parent]); + clk_set_parent(clk[ldb_di1_sel], clk[new_parent]); + + /* Unbypass pll3_sw_clk */ + reg = readl_relaxed(ccm_base + 0xc); + reg &= ~(1 << 0); + writel_relaxed(reg, ccm_base + 0xc); + + /* + * Set the periph2_clk_sel back to the bottom mux so that + * mmdc_ch1 is from its original parent. + */ + reg = readl_relaxed(ccm_base + 0x14); + reg &= ~(1 << 26); + writel_relaxed(reg, ccm_base + 0x14); + + /* Wait for the clock switch */ + while (readl_relaxed(ccm_base + 0x48)) + ; + + /* Clear MMDC_CH1 mask bit */ + reg = readl_relaxed(ccm_base + 0x4); + reg &= ~(1 << 16); + writel_relaxed(reg, ccm_base + 0x4); +} + +static void disable_anatop_clocks(void __iomem *anatop_base) +{ + unsigned int reg; + + /* Make sure PFDs are disabled at boot. */ + reg = readl_relaxed(anatop_base + 0x100); + /* Cannot disable pll2_pfd2_396M, as it is the MMDC clock in iMX6DL */ + if (cpu_is_imx6dl()) + reg |= 0x80008080; + else + reg |= 0x80808080; + writel_relaxed(reg, anatop_base + 0x100); + + reg = readl_relaxed(anatop_base + 0xf0); + reg |= 0x80808080; + writel_relaxed(reg, anatop_base + 0xf0); + + /* Make sure PLLs is disabled */ + reg = readl_relaxed(anatop_base + 0xa0); + reg &= ~(1 << 13); + writel_relaxed(reg, anatop_base + 0xa0); +} + static void __init imx6q_clocks_init(struct device_node *ccm_node) { struct device_node *np; @@ -232,6 +349,8 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node) clk[pll5_post_div] = clk_register_divider_table(NULL, "pll5_post_div", "pll5_video", CLK_SET_RATE_PARENT, base + 0xa0, 19, 2, 0, post_div_table, &imx_ccm_lock); clk[pll5_video_div] = clk_register_divider_table(NULL, "pll5_video_div", "pll5_post_div", CLK_SET_RATE_PARENT, base + 0x170, 30, 2, 0, video_div_table, &imx_ccm_lock); + disable_anatop_clocks(base); + np = ccm_node; base = of_iomap(np, 0); WARN_ON(!base); @@ -440,10 +559,8 @@ static void __init imx6q_clocks_init(struct device_node *ccm_node) clk_register_clkdev(clk[enet_ref], "enet_ref", NULL); if ((imx_get_soc_revision() != IMX_CHIP_REVISION_1_0) || - cpu_is_imx6dl()) { - clk_set_parent(clk[ldb_di0_sel], clk[pll5_video_div]); - clk_set_parent(clk[ldb_di1_sel], clk[pll5_video_div]); - } + cpu_is_imx6dl()) + init_ldb_clks(pll5_video_div); /* * The gpmi needs 100MHz frequency in the EDO/Sync mode,