linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] clk: imx: Fix reparenting of UARTs not associated with sdout
@ 2020-12-04 18:31 Adam Ford
  2020-12-07  5:24 ` Sascha Hauer
  0 siblings, 1 reply; 4+ messages in thread
From: Adam Ford @ 2020-12-04 18:31 UTC (permalink / raw)
  To: linux-clk
  Cc: aford, charles.steves, Adam Ford, Aisheng Dong,
	Michael Turquette, Stephen Boyd, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Linus Walleij, Jerome Brunet, linux-arm-kernel, linux-kernel

The default clock source on i.MX8M Mini and Nano boards use a 24MHz clock,
but users who need to re-parent the clock source run into issues because
all the UART clocks are enabled whether or not they're needed by sdout.

Any attempt to change the parent results in an busy error because the
clocks have been enabled already.

  clk: failed to reparent uart1 to sys_pll1_80m: -16

Instead of pre-initializing all UARTS, scan the device tree to see if UART
clock is used as stdout before initializing it.  Only enable the UART clock
if it's needed in order to delay the clock initialization until after the
re-parenting of the clocks.

Fixes: 9461f7b33d11c ("clk: fix CLK_SET_RATE_GATE with clock rate protection")
Suggested-by: Aisheng Dong <aisheng.dong@nxp.com>
Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
index 47882c51cb85..6dcc5fbd8f3f 100644
--- a/drivers/clk/imx/clk.c
+++ b/drivers/clk/imx/clk.c
@@ -163,12 +163,18 @@ __setup_param("earlyprintk", imx_keep_uart_earlyprintk,
 
 void imx_register_uart_clocks(struct clk ** const clks[])
 {
+	struct clk *uart_clk;
 	if (imx_keep_uart_clocks) {
 		int i;
 
 		imx_uart_clocks = clks;
-		for (i = 0; imx_uart_clocks[i]; i++)
-			clk_prepare_enable(*imx_uart_clocks[i]);
+		for (i = 0; imx_uart_clocks[i]; i++) {
+			uart_clk = of_clk_get(of_stdout, i);
+			if (IS_ERR(uart_clk))
+				continue;
+			clk_prepare_enable(uart_clk);
+			clk_put(uart_clk);
+		}
 	}
 }
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] clk: imx: Fix reparenting of UARTs not associated with sdout
  2020-12-04 18:31 [PATCH] clk: imx: Fix reparenting of UARTs not associated with sdout Adam Ford
@ 2020-12-07  5:24 ` Sascha Hauer
  2020-12-07 15:51   ` Adam Ford
  0 siblings, 1 reply; 4+ messages in thread
From: Sascha Hauer @ 2020-12-07  5:24 UTC (permalink / raw)
  To: Adam Ford
  Cc: linux-clk, aford, charles.steves, Aisheng Dong,
	Michael Turquette, Stephen Boyd, Shawn Guo,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Linus Walleij, Jerome Brunet, linux-arm-kernel, linux-kernel

Hi Adam,

On Fri, Dec 04, 2020 at 12:31:54PM -0600, Adam Ford wrote:
> The default clock source on i.MX8M Mini and Nano boards use a 24MHz clock,
> but users who need to re-parent the clock source run into issues because
> all the UART clocks are enabled whether or not they're needed by sdout.
> 
> Any attempt to change the parent results in an busy error because the
> clocks have been enabled already.
> 
>   clk: failed to reparent uart1 to sys_pll1_80m: -16
> 
> Instead of pre-initializing all UARTS, scan the device tree to see if UART
> clock is used as stdout before initializing it.  Only enable the UART clock
> if it's needed in order to delay the clock initialization until after the
> re-parenting of the clocks.
> 
> Fixes: 9461f7b33d11c ("clk: fix CLK_SET_RATE_GATE with clock rate protection")
> Suggested-by: Aisheng Dong <aisheng.dong@nxp.com>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> 
> diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> index 47882c51cb85..6dcc5fbd8f3f 100644
> --- a/drivers/clk/imx/clk.c
> +++ b/drivers/clk/imx/clk.c
> @@ -163,12 +163,18 @@ __setup_param("earlyprintk", imx_keep_uart_earlyprintk,
>  
>  void imx_register_uart_clocks(struct clk ** const clks[])
>  {
> +	struct clk *uart_clk;
>  	if (imx_keep_uart_clocks) {
>  		int i;
>  
>  		imx_uart_clocks = clks;
> -		for (i = 0; imx_uart_clocks[i]; i++)
> -			clk_prepare_enable(*imx_uart_clocks[i]);
> +		for (i = 0; imx_uart_clocks[i]; i++) {
> +			uart_clk = of_clk_get(of_stdout, i);

This looks wrong. imx_uart_clocks is an array containing all clocks that
could possibly be used for an UART. With of_clk_get(of_stdout, i) you
get the nth clock for one specific UART.
What you have to do here is: For each of imx_uart_clocks[] you have to
iterate over all clocks of the of_stdout node.

Sascha

> +			if (IS_ERR(uart_clk))
> +				continue;
> +			clk_prepare_enable(uart_clk);
> +			clk_put(uart_clk);
> +		}
>  	}
>  }
>  
> -- 
> 2.25.1
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] clk: imx: Fix reparenting of UARTs not associated with sdout
  2020-12-07  5:24 ` Sascha Hauer
@ 2020-12-07 15:51   ` Adam Ford
  2020-12-07 16:53     ` Sascha Hauer
  0 siblings, 1 reply; 4+ messages in thread
From: Adam Ford @ 2020-12-07 15:51 UTC (permalink / raw)
  To: Sascha Hauer
  Cc: linux-clk, Adam Ford-BE, charles.steves, Aisheng Dong,
	Michael Turquette, Stephen Boyd, Shawn Guo,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Linus Walleij, Jerome Brunet, arm-soc, Linux Kernel Mailing List

On Sun, Dec 6, 2020 at 11:24 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> Hi Adam,
>
> On Fri, Dec 04, 2020 at 12:31:54PM -0600, Adam Ford wrote:
> > The default clock source on i.MX8M Mini and Nano boards use a 24MHz clock,
> > but users who need to re-parent the clock source run into issues because
> > all the UART clocks are enabled whether or not they're needed by sdout.
> >
> > Any attempt to change the parent results in an busy error because the
> > clocks have been enabled already.
> >
> >   clk: failed to reparent uart1 to sys_pll1_80m: -16
> >
> > Instead of pre-initializing all UARTS, scan the device tree to see if UART
> > clock is used as stdout before initializing it.  Only enable the UART clock
> > if it's needed in order to delay the clock initialization until after the
> > re-parenting of the clocks.
> >
> > Fixes: 9461f7b33d11c ("clk: fix CLK_SET_RATE_GATE with clock rate protection")
> > Suggested-by: Aisheng Dong <aisheng.dong@nxp.com>
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> >
> > diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> > index 47882c51cb85..6dcc5fbd8f3f 100644
> > --- a/drivers/clk/imx/clk.c
> > +++ b/drivers/clk/imx/clk.c
> > @@ -163,12 +163,18 @@ __setup_param("earlyprintk", imx_keep_uart_earlyprintk,
> >
> >  void imx_register_uart_clocks(struct clk ** const clks[])
> >  {
> > +     struct clk *uart_clk;
> >       if (imx_keep_uart_clocks) {
> >               int i;
> >
> >               imx_uart_clocks = clks;
> > -             for (i = 0; imx_uart_clocks[i]; i++)
> > -                     clk_prepare_enable(*imx_uart_clocks[i]);
> > +             for (i = 0; imx_uart_clocks[i]; i++) {
> > +                     uart_clk = of_clk_get(of_stdout, i);
>
> This looks wrong. imx_uart_clocks is an array containing all clocks that
> could possibly be used for an UART. With of_clk_get(of_stdout, i) you
> get the nth clock for one specific UART.
> What you have to do here is: For each of imx_uart_clocks[] you have to
> iterate over all clocks of the of_stdout node.

Sascha,

Thanks for the review.

I agree that I'm using the wrong index when checking of_stdout, but I
am not sure we need to loop through  imx_uart_clocks at all.
I looked at the NXP repo, and they just focus on looping through
of_stdout and don't reference the imx_uart_clocks. Can't we just loop
through the of_stdout and enable any clocks found in that?

adam


>
> Sascha
>
> > +                     if (IS_ERR(uart_clk))
> > +                             continue;
> > +                     clk_prepare_enable(uart_clk);
> > +                     clk_put(uart_clk);
> > +             }
> >       }
> >  }
> >
> > --
> > 2.25.1
> >
> >
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] clk: imx: Fix reparenting of UARTs not associated with sdout
  2020-12-07 15:51   ` Adam Ford
@ 2020-12-07 16:53     ` Sascha Hauer
  0 siblings, 0 replies; 4+ messages in thread
From: Sascha Hauer @ 2020-12-07 16:53 UTC (permalink / raw)
  To: Adam Ford
  Cc: linux-clk, Adam Ford-BE, charles.steves, Aisheng Dong,
	Michael Turquette, Stephen Boyd, Shawn Guo,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Linus Walleij, Jerome Brunet, arm-soc, Linux Kernel Mailing List

Hi Adam,

On Mon, Dec 07, 2020 at 09:51:33AM -0600, Adam Ford wrote:
> On Sun, Dec 6, 2020 at 11:24 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > Hi Adam,
> >
> > On Fri, Dec 04, 2020 at 12:31:54PM -0600, Adam Ford wrote:
> > > The default clock source on i.MX8M Mini and Nano boards use a 24MHz clock,
> > > but users who need to re-parent the clock source run into issues because
> > > all the UART clocks are enabled whether or not they're needed by sdout.
> > >
> > > Any attempt to change the parent results in an busy error because the
> > > clocks have been enabled already.
> > >
> > >   clk: failed to reparent uart1 to sys_pll1_80m: -16
> > >
> > > Instead of pre-initializing all UARTS, scan the device tree to see if UART
> > > clock is used as stdout before initializing it.  Only enable the UART clock
> > > if it's needed in order to delay the clock initialization until after the
> > > re-parenting of the clocks.
> > >
> > > Fixes: 9461f7b33d11c ("clk: fix CLK_SET_RATE_GATE with clock rate protection")
> > > Suggested-by: Aisheng Dong <aisheng.dong@nxp.com>
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > >
> > > diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> > > index 47882c51cb85..6dcc5fbd8f3f 100644
> > > --- a/drivers/clk/imx/clk.c
> > > +++ b/drivers/clk/imx/clk.c
> > > @@ -163,12 +163,18 @@ __setup_param("earlyprintk", imx_keep_uart_earlyprintk,
> > >
> > >  void imx_register_uart_clocks(struct clk ** const clks[])
> > >  {
> > > +     struct clk *uart_clk;
> > >       if (imx_keep_uart_clocks) {
> > >               int i;
> > >
> > >               imx_uart_clocks = clks;
> > > -             for (i = 0; imx_uart_clocks[i]; i++)
> > > -                     clk_prepare_enable(*imx_uart_clocks[i]);
> > > +             for (i = 0; imx_uart_clocks[i]; i++) {
> > > +                     uart_clk = of_clk_get(of_stdout, i);
> >
> > This looks wrong. imx_uart_clocks is an array containing all clocks that
> > could possibly be used for an UART. With of_clk_get(of_stdout, i) you
> > get the nth clock for one specific UART.
> > What you have to do here is: For each of imx_uart_clocks[] you have to
> > iterate over all clocks of the of_stdout node.
> 
> Sascha,
> 
> Thanks for the review.
> 
> I agree that I'm using the wrong index when checking of_stdout, but I
> am not sure we need to loop through  imx_uart_clocks at all.
> I looked at the NXP repo, and they just focus on looping through
> of_stdout and don't reference the imx_uart_clocks. Can't we just loop
> through the of_stdout and enable any clocks found in that?

This sounds good. That way we could get rid of the imx_uart_clocks array
entirely.

Note that right below imx_register_uart_clocks() there is imx_clk_disable_uart()
which disables the previously enabled clocks again later when the real
driver has taken over. You'll have to change that as well accordingly.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-12-07 16:54 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-04 18:31 [PATCH] clk: imx: Fix reparenting of UARTs not associated with sdout Adam Ford
2020-12-07  5:24 ` Sascha Hauer
2020-12-07 15:51   ` Adam Ford
2020-12-07 16:53     ` Sascha Hauer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).