clk: imx: Fix reparenting of UARTs not associated with sdout

Commit Message

Adam Ford Dec. 4, 2020, 6:31 p.m. UTC

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 <[e-mail]>
Signed-off-by: Adam Ford <[e-mail]>

Comments

Sascha Hauer Dec. 7, 2020, 5:24 a.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Adam Ford <[e-mail]>
> 
> 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
> 
>

Adam Ford Dec. 7, 2020, 3:51 p.m. UTC | #2

On Sun, Dec 6, 2020 at 11:24 PM Sascha Hauer <[e-mail]> 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 <[e-mail]>
> > Signed-off-by: Adam Ford <[e-mail]>
> >
> > 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: [phone]    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |

Sascha Hauer Dec. 7, 2020, 4:53 p.m. UTC | #3

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 <[e-mail]> 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 <[e-mail]>
> > > Signed-off-by: Adam Ford <[e-mail]>
> > >
> > > 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

Patch

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);
+		}
 	}
 }