Patchwork [V2] ARM: imx: correct usecount of IPG, ARM and MMDC clk on i.mx6sl

login
register
mail settings
Submitter Anson Huang
Date Jan. 21, 2014, 10:50 a.m.
Message ID <1390301452-10143-1-git-send-email-b20788@freescale.com>
Download mbox | patch
Permalink /patch/312864/
State New
Headers show

Comments

Anson Huang - Jan. 21, 2014, 10:50 a.m.
IPG, ARM and MMDC's clock should be enabled during kernel boot up,
so we need to maintain their use count, otherwise, they may be
disabled unexpectedly if their children's clock are turned off,
which is not allowed.

Signed-off-by: Anson Huang <b20788@freescale.com>
---
 arch/arm/mach-imx/clk-imx6sl.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)
Fabio Estevam - Jan. 21, 2014, 10:56 a.m.
On Tue, Jan 21, 2014 at 8:50 AM, Anson Huang <b20788@freescale.com> wrote:
> IPG, ARM and MMDC's clock should be enabled during kernel boot up,
> so we need to maintain their use count, otherwise, they may be
> disabled unexpectedly if their children's clock are turned off,
> which is not allowed.
>
> Signed-off-by: Anson Huang <b20788@freescale.com>
> ---
>  arch/arm/mach-imx/clk-imx6sl.c |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/arch/arm/mach-imx/clk-imx6sl.c b/arch/arm/mach-imx/clk-imx6sl.c
> index 78f3bd6..047f4ff 100644
> --- a/arch/arm/mach-imx/clk-imx6sl.c
> +++ b/arch/arm/mach-imx/clk-imx6sl.c
> @@ -291,6 +291,22 @@ static void __init imx6sl_clocks_init(struct device_node *ccm_node)
>                 pr_warn("%s: failed to set AHB clock rate %d!\n",
>                         __func__, ret);
>
> +       /*
> +        * Make sure those always on clocks are enabled to maintain the correct
> +        * usecount and enabling/disabling of parent PLLs.
> +        */
> +       ret = clk_prepare_enable(clks[IMX6SL_CLK_IPG]);
> +       if (ret)
> +               pr_warn("%s: failed to enable IPG clock %d\n", __func__, ret);

What about adding WARN_ON(!ret ) ?

Regards,

Fabio Estevam
Sascha Hauer - Jan. 22, 2014, 6:47 a.m.
On Tue, Jan 21, 2014 at 06:50:52PM +0800, Anson Huang wrote:
> IPG, ARM and MMDC's clock should be enabled during kernel boot up,
> so we need to maintain their use count, otherwise, they may be
> disabled unexpectedly if their children's clock are turned off,
> which is not allowed.
> 
> Signed-off-by: Anson Huang <b20788@freescale.com>
> ---
>  arch/arm/mach-imx/clk-imx6sl.c |   16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/arch/arm/mach-imx/clk-imx6sl.c b/arch/arm/mach-imx/clk-imx6sl.c
> index 78f3bd6..047f4ff 100644
> --- a/arch/arm/mach-imx/clk-imx6sl.c
> +++ b/arch/arm/mach-imx/clk-imx6sl.c
> @@ -291,6 +291,22 @@ static void __init imx6sl_clocks_init(struct device_node *ccm_node)
>  		pr_warn("%s: failed to set AHB clock rate %d!\n",
>  			__func__, ret);
>  
> +	/*
> +	 * Make sure those always on clocks are enabled to maintain the correct
> +	 * usecount and enabling/disabling of parent PLLs.
> +	 */
> +	ret = clk_prepare_enable(clks[IMX6SL_CLK_IPG]);
> +	if (ret)
> +		pr_warn("%s: failed to enable IPG clock %d\n", __func__, ret);
> +
> +	ret = clk_prepare_enable(clks[IMX6SL_CLK_ARM]);
> +	if (ret)
> +		pr_warn("%s: failed to enable ARM clock %d\n", __func__, ret);
> +
> +	ret = clk_prepare_enable(clks[IMX6SL_CLK_MMDC_ROOT]);
> +	if (ret)
> +		pr_warn("%s: failed to enable MMDC clock %d\n", __func__, ret);
> +

Consider using a clk_init_on array like we have in clk-imx6q.c

Sascha
Anson Huang - Jan. 22, 2014, 7:16 a.m.
On Wed, Jan 22, 2014 at 07:47:40AM +0100, Sascha Hauer wrote:
> On Tue, Jan 21, 2014 at 06:50:52PM +0800, Anson Huang wrote:
> > IPG, ARM and MMDC's clock should be enabled during kernel boot up,
> > so we need to maintain their use count, otherwise, they may be
> > disabled unexpectedly if their children's clock are turned off,
> > which is not allowed.
> > 
> > Signed-off-by: Anson Huang <b20788@freescale.com>
> > ---
> >  arch/arm/mach-imx/clk-imx6sl.c |   16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/arch/arm/mach-imx/clk-imx6sl.c b/arch/arm/mach-imx/clk-imx6sl.c
> > index 78f3bd6..047f4ff 100644
> > --- a/arch/arm/mach-imx/clk-imx6sl.c
> > +++ b/arch/arm/mach-imx/clk-imx6sl.c
> > @@ -291,6 +291,22 @@ static void __init imx6sl_clocks_init(struct device_node *ccm_node)
> >  		pr_warn("%s: failed to set AHB clock rate %d!\n",
> >  			__func__, ret);
> >  
> > +	/*
> > +	 * Make sure those always on clocks are enabled to maintain the correct
> > +	 * usecount and enabling/disabling of parent PLLs.
> > +	 */
> > +	ret = clk_prepare_enable(clks[IMX6SL_CLK_IPG]);
> > +	if (ret)
> > +		pr_warn("%s: failed to enable IPG clock %d\n", __func__, ret);
> > +
> > +	ret = clk_prepare_enable(clks[IMX6SL_CLK_ARM]);
> > +	if (ret)
> > +		pr_warn("%s: failed to enable ARM clock %d\n", __func__, ret);
> > +
> > +	ret = clk_prepare_enable(clks[IMX6SL_CLK_MMDC_ROOT]);
> > +	if (ret)
> > +		pr_warn("%s: failed to enable MMDC clock %d\n", __func__, ret);
> > +
> 
> Consider using a clk_init_on array like we have in clk-imx6q.c
> 
> Sascha
> 

Right, that can make code more clean, thanks for the comment, please help review V3 patch.

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

Patch

diff --git a/arch/arm/mach-imx/clk-imx6sl.c b/arch/arm/mach-imx/clk-imx6sl.c
index 78f3bd6..047f4ff 100644
--- a/arch/arm/mach-imx/clk-imx6sl.c
+++ b/arch/arm/mach-imx/clk-imx6sl.c
@@ -291,6 +291,22 @@  static void __init imx6sl_clocks_init(struct device_node *ccm_node)
 		pr_warn("%s: failed to set AHB clock rate %d!\n",
 			__func__, ret);
 
+	/*
+	 * Make sure those always on clocks are enabled to maintain the correct
+	 * usecount and enabling/disabling of parent PLLs.
+	 */
+	ret = clk_prepare_enable(clks[IMX6SL_CLK_IPG]);
+	if (ret)
+		pr_warn("%s: failed to enable IPG clock %d\n", __func__, ret);
+
+	ret = clk_prepare_enable(clks[IMX6SL_CLK_ARM]);
+	if (ret)
+		pr_warn("%s: failed to enable ARM clock %d\n", __func__, ret);
+
+	ret = clk_prepare_enable(clks[IMX6SL_CLK_MMDC_ROOT]);
+	if (ret)
+		pr_warn("%s: failed to enable MMDC clock %d\n", __func__, ret);
+
 	if (IS_ENABLED(CONFIG_USB_MXS_PHY)) {
 		clk_prepare_enable(clks[IMX6SL_CLK_USBPHY1_GATE]);
 		clk_prepare_enable(clks[IMX6SL_CLK_USBPHY2_GATE]);