diff mbox

ARM: imx6: Fix procedure to switch the parent of LDB_DI_CLK

Message ID 1397044538-12676-1-git-send-email-festevam@gmail.com
State New
Headers show

Commit Message

Fabio Estevam April 9, 2014, 11:55 a.m. UTC
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(-)

Comments

Shawn Guo April 9, 2014, 1:34 p.m. UTC | #1
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
> 
> 
>
Christian Gmeiner April 9, 2014, 1:35 p.m. UTC | #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
Fabio Estevam April 9, 2014, 2:20 p.m. UTC | #3
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.
Shawn Guo April 9, 2014, 2:59 p.m. UTC | #4
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
Fabio Estevam April 9, 2014, 3:28 p.m. UTC | #5
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.
Shawn Guo April 10, 2014, 1:21 a.m. UTC | #6
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
Shawn Guo April 10, 2014, 2:44 a.m. UTC | #7
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
Dirk Behme May 19, 2014, 7:22 a.m. UTC | #8
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
Lothar Waßmann May 19, 2014, 9:25 a.m. UTC | #9
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
Ranjani.Vaidyanathan@freescale.com May 19, 2014, 5:07 p.m. UTC | #10
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
Dirk Behme June 4, 2014, 4:37 p.m. UTC | #11
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
Ranjani.Vaidyanathan@freescale.com June 4, 2014, 5:29 p.m. UTC | #12
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
Dirk Behme June 4, 2014, 5:49 p.m. UTC | #13
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
Dirk Behme June 5, 2014, 3:56 p.m. UTC | #14
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
Ranjani.Vaidyanathan@freescale.com June 5, 2014, 4:26 p.m. UTC | #15
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 mbox

Patch

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,