[PATCH] clk: clk-imx25: Force LCDC IPG clock to be disabled

[PATCH] clk: clk-imx25: Force LCDC IPG clock to be disabled

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@nxp.com>

Currently when we boot the kernel on a mx25pdk the LCDC controller
does not show the Linux logo on boot.

This problem is well explained by Sascha Hauer:

"Unfortunately this LCD controller does not have an enable bit. The
controller starts directly when the clocks are enabled. If the clocks
are enabled when the controller is not yet programmed with proper
register values then it just goes into some undefined state. What I
suspect is that the clocks already were enabled before driver probe,
presumably by the bootloader, so the controller is already in undefined
state when entering Linux. Now by dis/enabling the ipg clock you
effectively reset the controller. Since you have programmed it with
valid register values in the mean time it starts working after this
reset."

So let's guarantee that the clock driver disables LCDC IPG clock at
the beggining, so that we don't need to rely on the state the
bootloader left this clock.

With this change the Linux logo can be seen on boot on a mx25pdk.

Cc: <stable@vger.kernel.org>
Suggested-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
 drivers/clk/imx/clk-imx25.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/clk/imx/clk-imx25.c b/drivers/clk/imx/clk-imx25.c
index c4c141c..f8619bb 100644
--- a/drivers/clk/imx/clk-imx25.c
+++ b/drivers/clk/imx/clk-imx25.c
@@ -53,6 +53,8 @@
 
 #define ccm(x)	(ccm_base + (x))
 
+#define LCDC_IPG	BIT(29)
+
 static struct clk_onecell_data clk_data;
 
 static const char *cpu_sel_clks[] = { "mpll", "mpll_cpu_3_4", };
@@ -99,6 +101,7 @@ static struct clk ** const uart_clks[] __initconst = {
 static int __init __mx25_clocks_init(unsigned long osc_rate,
 				     void __iomem *ccm_base)
 {
+	unsigned int reg;
 	BUG_ON(!ccm_base);
 
 	clk[dummy] = imx_clk_fixed("dummy", 0);
@@ -232,6 +235,20 @@ static int __init __mx25_clocks_init(unsigned long osc_rate,
 
 	imx_check_clocks(clk, ARRAY_SIZE(clk));
 
+	/*
+	 * The LCDC controller does not have an enable bit. The
+	 * controller starts directly when the clocks are enabled.
+	 * If the clocks are enabled when the controller is not yet
+	 * programmed with proper register values (enabled at the
+	 * bootloader, for example) then it just goes into some undefined
+	 * state.
+	 * To avoid this issue, let's directly access CGCR1 register
+	 * and disable the LCDC IPG clock.
+	 */
+	reg = readl(ccm_base + CCM_CGCR1);
+	reg &= ~LCDC_IPG;
+	writel(reg, ccm_base + CCM_CGCR1);
+
 	clk_prepare_enable(clk[emi_ahb]);
 
 	/* Clock source for gpt must be derived from AHB */
-- 
1.9.1

Re: [PATCH] clk: clk-imx25: Force LCDC IPG clock to be disabled

[Carlos Soto]

2016-01-11 18:01 GMT+01:00 Fabio Estevam <festevam@gmail.com>:
> From: Fabio Estevam <fabio.estevam@nxp.com>
>
> Currently when we boot the kernel on a mx25pdk the LCDC controller
> does not show the Linux logo on boot.
>
> This problem is well explained by Sascha Hauer:
>
> "Unfortunately this LCD controller does not have an enable bit. The
> controller starts directly when the clocks are enabled. If the clocks
> are enabled when the controller is not yet programmed with proper
> register values then it just goes into some undefined state. What I
> suspect is that the clocks already were enabled before driver probe,
> presumably by the bootloader, so the controller is already in undefined
> state when entering Linux. Now by dis/enabling the ipg clock you
> effectively reset the controller. Since you have programmed it with
> valid register values in the mean time it starts working after this
> reset."
>
> So let's guarantee that the clock driver disables LCDC IPG clock at
> the beggining, so that we don't need to rely on the state the
> bootloader left this clock.
>
> With this change the Linux logo can be seen on boot on a mx25pdk.
>
> Cc: <stable@vger.kernel.org>
> Suggested-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> ---
>  drivers/clk/imx/clk-imx25.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/drivers/clk/imx/clk-imx25.c b/drivers/clk/imx/clk-imx25.c
> index c4c141c..f8619bb 100644
> --- a/drivers/clk/imx/clk-imx25.c
> +++ b/drivers/clk/imx/clk-imx25.c
> @@ -53,6 +53,8 @@
>
>  #define ccm(x) (ccm_base + (x))
>
> +#define LCDC_IPG       BIT(29)
> +
>  static struct clk_onecell_data clk_data;
>
>  static const char *cpu_sel_clks[] = { "mpll", "mpll_cpu_3_4", };
> @@ -99,6 +101,7 @@ static struct clk ** const uart_clks[] __initconst = {
>  static int __init __mx25_clocks_init(unsigned long osc_rate,
>                                      void __iomem *ccm_base)
>  {
> +       unsigned int reg;
>         BUG_ON(!ccm_base);
>
>         clk[dummy] = imx_clk_fixed("dummy", 0);
> @@ -232,6 +235,20 @@ static int __init __mx25_clocks_init(unsigned long osc_rate,
>
>         imx_check_clocks(clk, ARRAY_SIZE(clk));
>
> +       /*
> +        * The LCDC controller does not have an enable bit. The
> +        * controller starts directly when the clocks are enabled.
> +        * If the clocks are enabled when the controller is not yet
> +        * programmed with proper register values (enabled at the
> +        * bootloader, for example) then it just goes into some undefined
> +        * state.
> +        * To avoid this issue, let's directly access CGCR1 register
> +        * and disable the LCDC IPG clock.
> +        */
> +       reg = readl(ccm_base + CCM_CGCR1);
> +       reg &= ~LCDC_IPG;
> +       writel(reg, ccm_base + CCM_CGCR1);
> +
>         clk_prepare_enable(clk[emi_ahb]);
>
>         /* Clock source for gpt must be derived from AHB */
> --
> 1.9.1
>

Works OK on my TX25. Thanks Fabio and Sasha!

Tested-by: Carlos Soto <csotoalonso@gmail.com>

Regards,
Carlos

Re: [PATCH] clk: clk-imx25: Force LCDC IPG clock to be disabled

[Michael Turquette]

Hi Fabio & company,

Quoting Fabio Estevam (2016-01-11 09:01:56)
> From: Fabio Estevam <fabio.estevam@nxp.com>
> =

> Currently when we boot the kernel on a mx25pdk the LCDC controller
> does not show the Linux logo on boot.
> =

> This problem is well explained by Sascha Hauer:
> =

> "Unfortunately this LCD controller does not have an enable bit. The
> controller starts directly when the clocks are enabled. If the clocks
> are enabled when the controller is not yet programmed with proper
> register values then it just goes into some undefined state. What I
> suspect is that the clocks already were enabled before driver probe,
> presumably by the bootloader, so the controller is already in undefined
> state when entering Linux. Now by dis/enabling the ipg clock you
> effectively reset the controller. Since you have programmed it with
> valid register values in the mean time it starts working after this
> reset."
> =

> So let's guarantee that the clock driver disables LCDC IPG clock at
> the beggining, so that we don't need to rely on the state the
> bootloader left this clock.
> =

> With this change the Linux logo can be seen on boot on a mx25pdk.
> =

> Cc: <stable@vger.kernel.org>
> Suggested-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>

A couple of thoughts on this:

1) should you have a reset controller thingy for the LCDC? That might be
a better way to model what's going on.

2) is "resetting" the clock signal something generic that people need?
We could introduce a flag like CLK_RESET_AT_BOOT that calls uses the
top-level API to enable and then disable the clock at registration-time.

Of the two ideas, I prefer #1, so that you can reset the device at
boot-time using the reset framework.

Regards,
Mike

> ---
>  drivers/clk/imx/clk-imx25.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> =

> diff --git a/drivers/clk/imx/clk-imx25.c b/drivers/clk/imx/clk-imx25.c
> index c4c141c..f8619bb 100644
> --- a/drivers/clk/imx/clk-imx25.c
> +++ b/drivers/clk/imx/clk-imx25.c
> @@ -53,6 +53,8 @@
>  =

>  #define ccm(x) (ccm_base + (x))
>  =

> +#define LCDC_IPG       BIT(29)
> +
>  static struct clk_onecell_data clk_data;
>  =

>  static const char *cpu_sel_clks[] =3D { "mpll", "mpll_cpu_3_4", };
> @@ -99,6 +101,7 @@ static struct clk ** const uart_clks[] __initconst =3D=
 {
>  static int __init __mx25_clocks_init(unsigned long osc_rate,
>                                      void __iomem *ccm_base)
>  {
> +       unsigned int reg;
>         BUG_ON(!ccm_base);
>  =

>         clk[dummy] =3D imx_clk_fixed("dummy", 0);
> @@ -232,6 +235,20 @@ static int __init __mx25_clocks_init(unsigned long o=
sc_rate,
>  =

>         imx_check_clocks(clk, ARRAY_SIZE(clk));
>  =

> +       /*
> +        * The LCDC controller does not have an enable bit. The
> +        * controller starts directly when the clocks are enabled.
> +        * If the clocks are enabled when the controller is not yet
> +        * programmed with proper register values (enabled at the
> +        * bootloader, for example) then it just goes into some undefined
> +        * state.
> +        * To avoid this issue, let's directly access CGCR1 register
> +        * and disable the LCDC IPG clock.
> +        */
> +       reg =3D readl(ccm_base + CCM_CGCR1);
> +       reg &=3D ~LCDC_IPG;
> +       writel(reg, ccm_base + CCM_CGCR1);
> +
>         clk_prepare_enable(clk[emi_ahb]);
>  =

>         /* Clock source for gpt must be derived from AHB */
> -- =

> 1.9.1
>=20

Re: [PATCH] clk: clk-imx25: Force LCDC IPG clock to be disabled

[Fabio Estevam]

Hi Mike,

On Wed, Jan 13, 2016 at 6:17 PM, Michael Turquette
<mturquette@baylibre.com> wrote:

> A couple of thoughts on this:
>
> 1) should you have a reset controller thingy for the LCDC? That might be
> a better way to model what's going on.

Ok, just sent a patch forcing a reset to the controller by
enabling/disabling the IPG clock.

Thanks