[V3] clk: imx: Fix reparenting of UARTs not associated with sdout

Commit Message

Adam Ford Jan. 15, 2021, 6:29 p.m. UTC

Most if not all i.MX SoC's call a function which enables all UARTS.
This is a problem for users who need to re-parent the clock source,
because any attempt to change the parent results in an busy error
due to the fact that 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
which UART clocks are associated to stdout, and only enable those
UART clocks if it's needed early.  This will move initialization of
the remaining clocks until after the parenting of the clocks.

When the clocks are shutdown, this mechanism will also disable any
clocks that were pre-initialized.

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]>
---
V3:  Return a method more closely related to upstream kernel but
     instead of passing an array of UART's, each SoC passes the max
     number of UART clocks is has.  The imx clock driver will create
     an array to enable on startup, and disable later.
V2:  Attempt to port driver from vendor kernel.

Comments

Abel Vesa Jan. 18, 2021, 12:52 p.m. UTC | #1

On 21-01-15 12:29:08, Adam Ford wrote:

...

> diff --git a/drivers/clk/imx/clk-imx25.c b/drivers/clk/imx/clk-imx25.c
> index a66cabfbf94f..66192fe0a898 100644
> --- a/drivers/clk/imx/clk-imx25.c
> +++ b/drivers/clk/imx/clk-imx25.c
> @@ -73,16 +73,6 @@ enum mx25_clks {
>  
>  static struct clk *clk[clk_max];
>  
> -static struct clk ** const uart_clks[] __initconst = {
> -	&clk[uart_ipg_per],
> -	&clk[uart1_ipg],
> -	&clk[uart2_ipg],
> -	&clk[uart3_ipg],
> -	&clk[uart4_ipg],
> -	&clk[uart5_ipg],
> -	NULL
> -};
> -

I'm assuming there is another patch that updatesthe dts files. Right ?

TBH, I'm against the idea of having to call consumer API from a clock provider driver.
I'm still investigating a way of moving the uart clock control calls in drivers/serial/imx,
where they belong.

>  static int __init __mx25_clocks_init(void __iomem *ccm_base)
>  {
>  	BUG_ON(!ccm_base);
> @@ -228,7 +218,7 @@ static int __init __mx25_clocks_init(void __iomem *ccm_base)
>  	 */
>  	clk_set_parent(clk[cko_sel], clk[ipg]);
>  
> -	imx_register_uart_clocks(uart_clks);
> +	imx_register_uart_clocks(6);

Suggestion: Maybe the number of clocks can be determined by the existing clocks in that dts node.
Hardcoding is not a good ideea here.

...

>  
> diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> index 47882c51cb85..158fe302a8b7 100644
> --- a/drivers/clk/imx/clk.c
> +++ b/drivers/clk/imx/clk.c
> @@ -147,8 +147,10 @@ void imx_cscmr1_fixup(u32 *val)
>  }
>  
>  #ifndef MODULE
> -static int imx_keep_uart_clocks;
> -static struct clk ** const *imx_uart_clocks;
> +
> +static bool imx_keep_uart_clocks;
> +static int imx_enabled_uart_clocks;
> +static struct clk **imx_uart_clocks;
>  
>  static int __init imx_keep_uart_clocks_param(char *str)
>  {
> @@ -161,24 +163,43 @@ __setup_param("earlycon", imx_keep_uart_earlycon,
>  __setup_param("earlyprintk", imx_keep_uart_earlyprintk,
>  	      imx_keep_uart_clocks_param, 0);
>  
> -void imx_register_uart_clocks(struct clk ** const clks[])
> +void imx_register_uart_clocks(unsigned int clk_count)
>  {
> +#ifdef CONFIG_OF
>  	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]);
> +		imx_uart_clocks = kcalloc(clk_count, sizeof(struct clk *), GFP_KERNEL);
> +		imx_enabled_uart_clocks = 0;
> +
> +		for (i = 0; i < clk_count; i++) {
> +			imx_uart_clocks[imx_enabled_uart_clocks] = of_clk_get(of_stdout, i);
> +
> +			/* Stop if there are no more of_stdout references */
> +			if (IS_ERR(imx_uart_clocks[imx_enabled_uart_clocks]))
> +				return;
> +
> +			/* Only enable the clock if it's not NULL */
> +			if (imx_uart_clocks[imx_enabled_uart_clocks])
> +				clk_prepare_enable(imx_uart_clocks[imx_enabled_uart_clocks++]);
> +		}
>  	}
> +#else
> +	/* i.MX boards use device trees now.  For build tests without CONFIG_OF, do nothing */
> +	imx_enabled_uart_clocks = 0;
> +#endif

Don't really see the point of this #ifdef here. Just makes the code more messy.

>  }
>  
>  static int __init imx_clk_disable_uart(void)
>  {
> -	if (imx_keep_uart_clocks && imx_uart_clocks) {
> +	if (imx_keep_uart_clocks && imx_enabled_uart_clocks) {
>  		int i;
>  
> -		for (i = 0; imx_uart_clocks[i]; i++)
> -			clk_disable_unprepare(*imx_uart_clocks[i]);
> +		for (i = 0; i < imx_enabled_uart_clocks; i++) {
> +			clk_disable_unprepare(imx_uart_clocks[i]);
> +			clk_put(imx_uart_clocks[i]);
> +		};
> +		kfree(imx_uart_clocks);
>  	}
>  
>  	return 0;
> diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
> index 4f04c8287286..7571603bee23 100644
> --- a/drivers/clk/imx/clk.h
> +++ b/drivers/clk/imx/clk.h
> @@ -11,9 +11,9 @@ extern spinlock_t imx_ccm_lock;
>  void imx_check_clocks(struct clk *clks[], unsigned int count);
>  void imx_check_clk_hws(struct clk_hw *clks[], unsigned int count);
>  #ifndef MODULE
> -void imx_register_uart_clocks(struct clk ** const clks[]);
> +void imx_register_uart_clocks(unsigned int clk_count);
>  #else
> -static inline void imx_register_uart_clocks(struct clk ** const clks[])
> +static inline void imx_register_uart_clocks(unsigned int clk_count)
>  {
>  }
>  #endif
> -- 
> 2.25.1
>

Adam Ford Jan. 18, 2021, 2 p.m. UTC | #2

On Mon, Jan 18, 2021 at 6:52 AM Abel Vesa <[e-mail]> wrote:
>
> On 21-01-15 12:29:08, Adam Ford wrote:
>
> ...
>
> > diff --git a/drivers/clk/imx/clk-imx25.c b/drivers/clk/imx/clk-imx25.c
> > index a66cabfbf94f..66192fe0a898 100644
> > --- a/drivers/clk/imx/clk-imx25.c
> > +++ b/drivers/clk/imx/clk-imx25.c
> > @@ -73,16 +73,6 @@ enum mx25_clks {
> >
> >  static struct clk *clk[clk_max];
> >
> > -static struct clk ** const uart_clks[] __initconst = {
> > -     &clk[uart_ipg_per],
> > -     &clk[uart1_ipg],
> > -     &clk[uart2_ipg],
> > -     &clk[uart3_ipg],
> > -     &clk[uart4_ipg],
> > -     &clk[uart5_ipg],
> > -     NULL
> > -};
> > -
>
> I'm assuming there is another patch that updates the dts files. Right ?

I have only been able to test this on an i.MX8M Mini.  I need to set
the parent clock of the i.MX8M Mini to an 80 MHz clock in order to run
the UART at 4Mbps.   With this patch, I can stop enabling the all the
UART clocks early and allow the clock parent configuration to occur.
From what I can tell, the remaining clocks should get activated as
they are needed, because I was able to use Bluetooth connected to
UART1 running at 4MBps using a 80MHz clock source with this patch, and
the clk_summary shows the clock is running at the proper speed.
Without this patch, the UART fails to re-parent, so I'm stuck at lower
speeds and that means choppy Bluetooth audio.

The Kernel that NXP hosts on Code Aurora that they use for Yocto
attempts scan through stdout to only enable those clocks [1].  I
attempted to push it upstream, but it was rejected [2].  Sascha
suggested creating an array which could be filled when the clocks are
enabled and that array would be used to deactivate the clocks at
shutdown.  That's what I attempted to do here.

I don't have older imx boards to know if their device trees are
configured in such a way without modifications to the device tree or
not, but since it appears to work for NXP in [2], I assumed it would
work here.

[1] - https://source.codeaurora.org/external/imx/linux-imx/commit/drivers/clk/imx/clk.c?h=imx_5.4.47_2.2.0&id=754ae82cc55b7445545fc2f092a70e0f490e9c1b
[2] - https://patchwork.kernel.org/project/linux-arm-kernel/patch/[e-mail]/

>
> TBH, I'm against the idea of having to call consumer API from a clock provider driver.
> I'm still investigating a way of moving the uart clock control calls in drivers/serial/imx,
> where they belong.

That makes sense.

>
> >  static int __init __mx25_clocks_init(void __iomem *ccm_base)
> >  {
> >       BUG_ON(!ccm_base);
> > @@ -228,7 +218,7 @@ static int __init __mx25_clocks_init(void __iomem *ccm_base)
> >        */
> >       clk_set_parent(clk[cko_sel], clk[ipg]);
> >
> > -     imx_register_uart_clocks(uart_clks);
> > +     imx_register_uart_clocks(6);
>
> Suggestion: Maybe the number of clocks can be determined by the existing clocks in that dts node.
> Hardcoding is not a good ideea here.

The tables were hard-coded before, so the idea was to pass the maximum
number of clocks instead the entire table.  The higher-level clock
code wouldn't necessarily know the maximum number of UART clocks since
the number of UARTs may change depending on the SoC.  So that
hard-coded number was simply the number of entries that were
previously used in the array that was previously passed.  When
creating a table of active clocks, it could use the number passed to
define an array, and fill the array with data grabbed from of_stdout.

If you want, I could leave the existing UART clocks alone, and create
a new function that uses the array of clocks passed to it to count the
number of available clocks.  It would limit the scope of the change to
clk/imx/clk.c.  I think that would be easier than trying to parse the
DT for a bunch of compatible flags looking for a bunch of UARTS and
their respective clocks.

If you'd rather do something in the serial imx driver, I can hold off.

adam
>
> ...
>
> >
> > diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> > index 47882c51cb85..158fe302a8b7 100644
> > --- a/drivers/clk/imx/clk.c
> > +++ b/drivers/clk/imx/clk.c
> > @@ -147,8 +147,10 @@ void imx_cscmr1_fixup(u32 *val)
> >  }
> >
> >  #ifndef MODULE
> > -static int imx_keep_uart_clocks;
> > -static struct clk ** const *imx_uart_clocks;
> > +
> > +static bool imx_keep_uart_clocks;
> > +static int imx_enabled_uart_clocks;
> > +static struct clk **imx_uart_clocks;
> >
> >  static int __init imx_keep_uart_clocks_param(char *str)
> >  {
> > @@ -161,24 +163,43 @@ __setup_param("earlycon", imx_keep_uart_earlycon,
> >  __setup_param("earlyprintk", imx_keep_uart_earlyprintk,
> >             imx_keep_uart_clocks_param, 0);
> >
> > -void imx_register_uart_clocks(struct clk ** const clks[])
> > +void imx_register_uart_clocks(unsigned int clk_count)
> >  {
> > +#ifdef CONFIG_OF
> >       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]);
> > +             imx_uart_clocks = kcalloc(clk_count, sizeof(struct clk *), GFP_KERNEL);
> > +             imx_enabled_uart_clocks = 0;
> > +
> > +             for (i = 0; i < clk_count; i++) {
> > +                     imx_uart_clocks[imx_enabled_uart_clocks] = of_clk_get(of_stdout, i);
> > +
> > +                     /* Stop if there are no more of_stdout references */
> > +                     if (IS_ERR(imx_uart_clocks[imx_enabled_uart_clocks]))
> > +                             return;
> > +
> > +                     /* Only enable the clock if it's not NULL */
> > +                     if (imx_uart_clocks[imx_enabled_uart_clocks])
> > +                             clk_prepare_enable(imx_uart_clocks[imx_enabled_uart_clocks++]);
> > +             }
> >       }
> > +#else
> > +     /* i.MX boards use device trees now.  For build tests without CONFIG_OF, do nothing */
> > +     imx_enabled_uart_clocks = 0;
> > +#endif
>
> Don't really see the point of this #ifdef here. Just makes the code more messy.
>

I added the #ifdef here because I got an e-mail from a bot from a
previous attempt.  The bot failed to build, because the build test
using a different defconfig file that doesn't enable CONFIG_OF.  For
the purposes of using this code, the CONFIG_OF is required, but
without changes to Kconfig, this seemed like an easy way to prevent
build errors for the bot.  I didn't want to break something else by
changing the Kconfig dependencies because I wasn't sure how many build
bots exist.

adam
> >  }
> >
> >  static int __init imx_clk_disable_uart(void)
> >  {
> > -     if (imx_keep_uart_clocks && imx_uart_clocks) {
> > +     if (imx_keep_uart_clocks && imx_enabled_uart_clocks) {
> >               int i;
> >
> > -             for (i = 0; imx_uart_clocks[i]; i++)
> > -                     clk_disable_unprepare(*imx_uart_clocks[i]);
> > +             for (i = 0; i < imx_enabled_uart_clocks; i++) {
> > +                     clk_disable_unprepare(imx_uart_clocks[i]);
> > +                     clk_put(imx_uart_clocks[i]);
> > +             };
> > +             kfree(imx_uart_clocks);
> >       }
> >
> >       return 0;
> > diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
> > index 4f04c8287286..7571603bee23 100644
> > --- a/drivers/clk/imx/clk.h
> > +++ b/drivers/clk/imx/clk.h
> > @@ -11,9 +11,9 @@ extern spinlock_t imx_ccm_lock;
> >  void imx_check_clocks(struct clk *clks[], unsigned int count);
> >  void imx_check_clk_hws(struct clk_hw *clks[], unsigned int count);
> >  #ifndef MODULE
> > -void imx_register_uart_clocks(struct clk ** const clks[]);
> > +void imx_register_uart_clocks(unsigned int clk_count);
> >  #else
> > -static inline void imx_register_uart_clocks(struct clk ** const clks[])
> > +static inline void imx_register_uart_clocks(unsigned int clk_count)
> >  {
> >  }
> >  #endif
> > --
> > 2.25.1
> >

Abel Vesa Jan. 20, 2021, 2:44 p.m. UTC | #3

On 21-01-18 08:00:43, Adam Ford wrote:
> On Mon, Jan 18, 2021 at 6:52 AM Abel Vesa <[e-mail]> wrote:
> >
> > On 21-01-15 12:29:08, Adam Ford wrote:
> >
> > ...
> >
> > > diff --git a/drivers/clk/imx/clk-imx25.c b/drivers/clk/imx/clk-imx25.c
> > > index a66cabfbf94f..66192fe0a898 100644
> > > --- a/drivers/clk/imx/clk-imx25.c
> > > +++ b/drivers/clk/imx/clk-imx25.c
> > > @@ -73,16 +73,6 @@ enum mx25_clks {
> > >
> > >  static struct clk *clk[clk_max];
> > >
> > > -static struct clk ** const uart_clks[] __initconst = {
> > > -     &clk[uart_ipg_per],
> > > -     &clk[uart1_ipg],
> > > -     &clk[uart2_ipg],
> > > -     &clk[uart3_ipg],
> > > -     &clk[uart4_ipg],
> > > -     &clk[uart5_ipg],
> > > -     NULL
> > > -};
> > > -
> >
> > I'm assuming there is another patch that updates the dts files. Right ?
> 
> I have only been able to test this on an i.MX8M Mini.  I need to set
> the parent clock of the i.MX8M Mini to an 80 MHz clock in order to run
> the UART at 4Mbps.   With this patch, I can stop enabling the all the
> UART clocks early and allow the clock parent configuration to occur.
> From what I can tell, the remaining clocks should get activated as
> they are needed, because I was able to use Bluetooth connected to
> UART1 running at 4MBps using a 80MHz clock source with this patch, and
> the clk_summary shows the clock is running at the proper speed.
> Without this patch, the UART fails to re-parent, so I'm stuck at lower
> speeds and that means choppy Bluetooth audio.
> 
> The Kernel that NXP hosts on Code Aurora that they use for Yocto
> attempts scan through stdout to only enable those clocks [1].  I
> attempted to push it upstream, but it was rejected [2].  Sascha
> suggested creating an array which could be filled when the clocks are
> enabled and that array would be used to deactivate the clocks at
> shutdown.  That's what I attempted to do here.
> 
> I don't have older imx boards to know if their device trees are
> configured in such a way without modifications to the device tree or
> not, but since it appears to work for NXP in [2], I assumed it would
> work here.
> 
> [1] - https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.codeaurora.org%2Fexternal%2Fimx%2Flinux-imx%2Fcommit%2Fdrivers%2Fclk%2Fimx%2Fclk.c%3Fh%3Dimx_5.4.47_2.2.0%26id%3D754ae82cc55b7445545fc2f092a70e0f490e9c1b&amp;data=04%7C01%7Cabel.vesa%40nxp.com%7Cf8922e76fa85485b86c508d8bbb97c47%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637465752633257569%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=bVmPaM1nN7Sm%2BISVvlIBoWYozfJE96fHpw431IEuggk%3D&amp;reserved=0
> [2] - https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fproject%2Flinux-arm-kernel%2Fpatch%2F20201229145130.2680442-1-aford173%40gmail.com%2F&amp;data=04%7C01%7Cabel.vesa%40nxp.com%7Cf8922e76fa85485b86c508d8bbb97c47%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637465752633257569%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=226HwbeVhZUW%2FJ3hsCSaVIxghOsPBH9EWeF8vFxaTWE%3D&amp;reserved=0
> 
> >
> > TBH, I'm against the idea of having to call consumer API from a clock provider driver.
> > I'm still investigating a way of moving the uart clock control calls in drivers/serial/imx,
> > where they belong.
> 
> That makes sense.
> 

Just a thought. The uart clock used for console remains on from u-boot,
so maybe it's enough to just add the CLK_IGNORE_UNUSED flag to all the
uart root clocks and remove the prepare/enable calls for uart clocks 
for good. I don't really have a way to test it right now, but maybe
you could give it a try.

Sascha Hauer Jan. 20, 2021, 3:13 p.m. UTC | #4

Hi Abel,

On Wed, Jan 20, 2021 at 04:44:54PM +0200, Abel Vesa wrote:
> On 21-01-18 08:00:43, Adam Ford wrote:
> > On Mon, Jan 18, 2021 at 6:52 AM Abel Vesa <[e-mail]> wrote:
> > >
> > > On 21-01-15 12:29:08, Adam Ford wrote:
> > >
> > > ...
> > >
> > > > diff --git a/drivers/clk/imx/clk-imx25.c b/drivers/clk/imx/clk-imx25.c
> > > > index a66cabfbf94f..66192fe0a898 100644
> > > > --- a/drivers/clk/imx/clk-imx25.c
> > > > +++ b/drivers/clk/imx/clk-imx25.c
> > > > @@ -73,16 +73,6 @@ enum mx25_clks {
> > > >
> > > >  static struct clk *clk[clk_max];
> > > >
> > > > -static struct clk ** const uart_clks[] __initconst = {
> > > > -     &clk[uart_ipg_per],
> > > > -     &clk[uart1_ipg],
> > > > -     &clk[uart2_ipg],
> > > > -     &clk[uart3_ipg],
> > > > -     &clk[uart4_ipg],
> > > > -     &clk[uart5_ipg],
> > > > -     NULL
> > > > -};
> > > > -
> > >
> > > I'm assuming there is another patch that updates the dts files. Right ?
> > 
> > I have only been able to test this on an i.MX8M Mini.  I need to set
> > the parent clock of the i.MX8M Mini to an 80 MHz clock in order to run
> > the UART at 4Mbps.   With this patch, I can stop enabling the all the
> > UART clocks early and allow the clock parent configuration to occur.
> > From what I can tell, the remaining clocks should get activated as
> > they are needed, because I was able to use Bluetooth connected to
> > UART1 running at 4MBps using a 80MHz clock source with this patch, and
> > the clk_summary shows the clock is running at the proper speed.
> > Without this patch, the UART fails to re-parent, so I'm stuck at lower
> > speeds and that means choppy Bluetooth audio.
> > 
> > The Kernel that NXP hosts on Code Aurora that they use for Yocto
> > attempts scan through stdout to only enable those clocks [1].  I
> > attempted to push it upstream, but it was rejected [2].  Sascha
> > suggested creating an array which could be filled when the clocks are
> > enabled and that array would be used to deactivate the clocks at
> > shutdown.  That's what I attempted to do here.
> > 
> > I don't have older imx boards to know if their device trees are
> > configured in such a way without modifications to the device tree or
> > not, but since it appears to work for NXP in [2], I assumed it would
> > work here.
> > 
> > [1] - https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsource.codeaurora.org%2Fexternal%2Fimx%2Flinux-imx%2Fcommit%2Fdrivers%2Fclk%2Fimx%2Fclk.c%3Fh%3Dimx_5.4.47_2.2.0%26id%3D754ae82cc55b7445545fc2f092a70e0f490e9c1b&amp;data=04%7C01%7Cabel.vesa%40nxp.com%7Cf8922e76fa85485b86c508d8bbb97c47%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637465752633257569%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=bVmPaM1nN7Sm%2BISVvlIBoWYozfJE96fHpw431IEuggk%3D&amp;reserved=0
> > [2] - https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fproject%2Flinux-arm-kernel%2Fpatch%2F20201229145130.2680442-1-aford173%40gmail.com%2F&amp;data=04%7C01%7Cabel.vesa%40nxp.com%7Cf8922e76fa85485b86c508d8bbb97c47%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637465752633257569%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=226HwbeVhZUW%2FJ3hsCSaVIxghOsPBH9EWeF8vFxaTWE%3D&amp;reserved=0
> > 
> > >
> > > TBH, I'm against the idea of having to call consumer API from a clock provider driver.
> > > I'm still investigating a way of moving the uart clock control calls in drivers/serial/imx,
> > > where they belong.
> > 
> > That makes sense.
> > 
> 
> Just a thought. The uart clock used for console remains on from u-boot,
> so maybe it's enough to just add the CLK_IGNORE_UNUSED flag to all the
> uart root clocks and remove the prepare/enable calls for uart clocks 
> for good. I don't really have a way to test it right now, but maybe
> you could give it a try.

That would mean that UART clocks will never be disabled, regardless of
whether they are used for console or not. That doesn't sound very
appealing.

Sascha

Abel Vesa Jan. 20, 2021, 3:28 p.m. UTC | #5

On 21-01-20 16:13:05, Sascha Hauer wrote:
> Hi Abel,
> 
> On Wed, Jan 20, 2021 at 04:44:54PM +0200, Abel Vesa wrote:
> > On 21-01-18 08:00:43, Adam Ford wrote:
> > > On Mon, Jan 18, 2021 at 6:52 AM Abel Vesa <[e-mail]> wrote:

...

> > > 
> > > >
> > > > TBH, I'm against the idea of having to call consumer API from a clock provider driver.
> > > > I'm still investigating a way of moving the uart clock control calls in drivers/serial/imx,
> > > > where they belong.
> > > 
> > > That makes sense.
> > > 
> > 
> > Just a thought. The uart clock used for console remains on from u-boot,
> > so maybe it's enough to just add the CLK_IGNORE_UNUSED flag to all the
> > uart root clocks and remove the prepare/enable calls for uart clocks 
> > for good. I don't really have a way to test it right now, but maybe
> > you could give it a try.
> 
> That would mean that UART clocks will never be disabled, regardless of
> whether they are used for console or not. That doesn't sound very
> appealing.

AFAIK, the only uart clock that is enabled by u-boot is the one used for
the console. Later on, when the serial driver probes, it will enable it itself.

Unless I'm missing something, this is exactly what we need.

Sascha Hauer Jan. 20, 2021, 3:50 p.m. UTC | #6

On Wed, Jan 20, 2021 at 05:28:13PM +0200, Abel Vesa wrote:
> On 21-01-20 16:13:05, Sascha Hauer wrote:
> > Hi Abel,
> > 
> > On Wed, Jan 20, 2021 at 04:44:54PM +0200, Abel Vesa wrote:
> > > On 21-01-18 08:00:43, Adam Ford wrote:
> > > > On Mon, Jan 18, 2021 at 6:52 AM Abel Vesa <[e-mail]> wrote:
> 
> ...
> 
> > > > 
> > > > >
> > > > > TBH, I'm against the idea of having to call consumer API from a clock provider driver.
> > > > > I'm still investigating a way of moving the uart clock control calls in drivers/serial/imx,
> > > > > where they belong.
> > > > 
> > > > That makes sense.
> > > > 
> > > 
> > > Just a thought. The uart clock used for console remains on from u-boot,
> > > so maybe it's enough to just add the CLK_IGNORE_UNUSED flag to all the
> > > uart root clocks and remove the prepare/enable calls for uart clocks 
> > > for good. I don't really have a way to test it right now, but maybe
> > > you could give it a try.
> > 
> > That would mean that UART clocks will never be disabled, regardless of
> > whether they are used for console or not. That doesn't sound very
> > appealing.
> 
> AFAIK, the only uart clock that is enabled by u-boot is the one used for
> the console. Later on, when the serial driver probes, it will enable it itself.
> 
> Unless I'm missing something, this is exactly what we need.

It might enable it, but with CLK_IGNORE_UNUSED the clock won't be
disabled again when a port is closed after usage

Sascha

Abel Vesa Jan. 20, 2021, 4:14 p.m. UTC | #7

On 21-01-20 16:50:01, Sascha Hauer wrote:
> On Wed, Jan 20, 2021 at 05:28:13PM +0200, Abel Vesa wrote:
> > On 21-01-20 16:13:05, Sascha Hauer wrote:
> > > Hi Abel,
> > > 
> > > On Wed, Jan 20, 2021 at 04:44:54PM +0200, Abel Vesa wrote:
> > > > On 21-01-18 08:00:43, Adam Ford wrote:
> > > > > On Mon, Jan 18, 2021 at 6:52 AM Abel Vesa <[e-mail]> wrote:
> > 
> > ...
> > 
> > > > > 
> > > > > >
> > > > > > TBH, I'm against the idea of having to call consumer API from a clock provider driver.
> > > > > > I'm still investigating a way of moving the uart clock control calls in drivers/serial/imx,
> > > > > > where they belong.
> > > > > 
> > > > > That makes sense.
> > > > > 
> > > > 
> > > > Just a thought. The uart clock used for console remains on from u-boot,
> > > > so maybe it's enough to just add the CLK_IGNORE_UNUSED flag to all the
> > > > uart root clocks and remove the prepare/enable calls for uart clocks 
> > > > for good. I don't really have a way to test it right now, but maybe
> > > > you could give it a try.
> > > 
> > > That would mean that UART clocks will never be disabled, regardless of
> > > whether they are used for console or not. That doesn't sound very
> > > appealing.
> > 
> > AFAIK, the only uart clock that is enabled by u-boot is the one used for
> > the console. Later on, when the serial driver probes, it will enable it itself.
> > 
> > Unless I'm missing something, this is exactly what we need.
> 
> It might enable it, but with CLK_IGNORE_UNUSED the clock won't be
> disabled again when a port is closed after usage

OK, tell me what I'm getting wrong in the following scenario:

U-boot leaves the console uart clock enabled. All the other ones are disabled.

Kernel i.MX clk driver registers the uart clocks with flag CLK_IGNORE_UNUSED.

Serial driver probes, then enables/disables the clocks when the port opens/closes.

The clk_disable_unused will ignore the uart clocks entirely due to the flag.

Sascha Hauer Jan. 21, 2021, 9:56 a.m. UTC | #8

On Wed, Jan 20, 2021 at 06:14:21PM +0200, Abel Vesa wrote:
> On 21-01-20 16:50:01, Sascha Hauer wrote:
> > On Wed, Jan 20, 2021 at 05:28:13PM +0200, Abel Vesa wrote:
> > > On 21-01-20 16:13:05, Sascha Hauer wrote:
> > > > Hi Abel,
> > > > 
> > > > On Wed, Jan 20, 2021 at 04:44:54PM +0200, Abel Vesa wrote:
> > > > > On 21-01-18 08:00:43, Adam Ford wrote:
> > > > > > On Mon, Jan 18, 2021 at 6:52 AM Abel Vesa <[e-mail]> wrote:
> > > 
> > > ...
> > > 
> > > > > > 
> > > > > > >
> > > > > > > TBH, I'm against the idea of having to call consumer API from a clock provider driver.
> > > > > > > I'm still investigating a way of moving the uart clock control calls in drivers/serial/imx,
> > > > > > > where they belong.
> > > > > > 
> > > > > > That makes sense.
> > > > > > 
> > > > > 
> > > > > Just a thought. The uart clock used for console remains on from u-boot,
> > > > > so maybe it's enough to just add the CLK_IGNORE_UNUSED flag to all the
> > > > > uart root clocks and remove the prepare/enable calls for uart clocks 
> > > > > for good. I don't really have a way to test it right now, but maybe
> > > > > you could give it a try.
> > > > 
> > > > That would mean that UART clocks will never be disabled, regardless of
> > > > whether they are used for console or not. That doesn't sound very
> > > > appealing.
> > > 
> > > AFAIK, the only uart clock that is enabled by u-boot is the one used for
> > > the console. Later on, when the serial driver probes, it will enable it itself.
> > > 
> > > Unless I'm missing something, this is exactly what we need.
> > 
> > It might enable it, but with CLK_IGNORE_UNUSED the clock won't be
> > disabled again when a port is closed after usage
> 
> OK, tell me what I'm getting wrong in the following scenario:
> 
> U-boot leaves the console uart clock enabled. All the other ones are disabled.
> 
> Kernel i.MX clk driver registers the uart clocks with flag CLK_IGNORE_UNUSED.

I was wrong at that point. I originally thought the kernel will never
disable these clocks, but in fact it only leaves them enabled during the
clk_disable_unused call.

However, when CLK_IGNORE_UNUSED becomes relevant it's too late already.
I just chatted with Lucas and he told me what the original problem was
that his patch solved.

The problem comes when an unrelated device and the earlycon UART have
the same parent clocks. The parent clock is enabled, but it's reference
count is zero. Now when the unrelated device probes and toggles its
clocks then the shared parent clock will be disabled due to the
reference count being zero. Next time earlycon prints a character the
system hangs because the UART gates are still enabled, but their parent
clocks no longer are.

Overall I think Lucas' patches are still valid and relevant and with
Adams patches we even no longer have to enable all UART clocks, but
only the ones which are actually needed.

Sascha

Abel Vesa Jan. 21, 2021, 10:24 a.m. UTC | #9

On 21-01-21 10:56:17, Sascha Hauer wrote:
> On Wed, Jan 20, 2021 at 06:14:21PM +0200, Abel Vesa wrote:
> > On 21-01-20 16:50:01, Sascha Hauer wrote:
> > > On Wed, Jan 20, 2021 at 05:28:13PM +0200, Abel Vesa wrote:
> > > > On 21-01-20 16:13:05, Sascha Hauer wrote:
> > > > > Hi Abel,
> > > > > 
> > > > > On Wed, Jan 20, 2021 at 04:44:54PM +0200, Abel Vesa wrote:
> > > > > > On 21-01-18 08:00:43, Adam Ford wrote:
> > > > > > > On Mon, Jan 18, 2021 at 6:52 AM Abel Vesa <[e-mail]> wrote:
> > > > 
> > > > ...
> > > > 
> > > > > > > 
> > > > > > > >
> > > > > > > > TBH, I'm against the idea of having to call consumer API from a clock provider driver.
> > > > > > > > I'm still investigating a way of moving the uart clock control calls in drivers/serial/imx,
> > > > > > > > where they belong.
> > > > > > > 
> > > > > > > That makes sense.
> > > > > > > 
> > > > > > 
> > > > > > Just a thought. The uart clock used for console remains on from u-boot,
> > > > > > so maybe it's enough to just add the CLK_IGNORE_UNUSED flag to all the
> > > > > > uart root clocks and remove the prepare/enable calls for uart clocks 
> > > > > > for good. I don't really have a way to test it right now, but maybe
> > > > > > you could give it a try.
> > > > > 
> > > > > That would mean that UART clocks will never be disabled, regardless of
> > > > > whether they are used for console or not. That doesn't sound very
> > > > > appealing.
> > > > 
> > > > AFAIK, the only uart clock that is enabled by u-boot is the one used for
> > > > the console. Later on, when the serial driver probes, it will enable it itself.
> > > > 
> > > > Unless I'm missing something, this is exactly what we need.
> > > 
> > > It might enable it, but with CLK_IGNORE_UNUSED the clock won't be
> > > disabled again when a port is closed after usage
> > 
> > OK, tell me what I'm getting wrong in the following scenario:
> > 
> > U-boot leaves the console uart clock enabled. All the other ones are disabled.
> > 
> > Kernel i.MX clk driver registers the uart clocks with flag CLK_IGNORE_UNUSED.
> 
> I was wrong at that point. I originally thought the kernel will never
> disable these clocks, but in fact it only leaves them enabled during the
> clk_disable_unused call.
> 
> However, when CLK_IGNORE_UNUSED becomes relevant it's too late already.
> I just chatted with Lucas and he told me what the original problem was
> that his patch solved.
> 
> The problem comes when an unrelated device and the earlycon UART have
> the same parent clocks. The parent clock is enabled, but it's reference
> count is zero. Now when the unrelated device probes and toggles its
> clocks then the shared parent clock will be disabled due to the
> reference count being zero. Next time earlycon prints a character the
> system hangs because the UART gates are still enabled, but their parent
> clocks no longer are.
> 

Hmm, that is indeed a problem. That's why I think there should be some
kind of NOCACHE flag for almost all the types of clocks. For example,
in this case, it makes sense for the core to check the bit in the register
and then disable the parent based on that instead of relying on the refcount.
Anyway, that's something that needs to be added in the CCF.

> Overall I think Lucas' patches are still valid and relevant and with
> Adams patches we even no longer have to enable all UART clocks, but
> only the ones which are actually needed.

Yeah, for now, I think we can go with Adam's patches. But longterm, I would
like to remove the special case of the uart clocks we have right now in all
the i.MX clock drivers.

> 
> Sascha
> 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pengutronix.de%2F&amp;data=04%7C01%7Cabel.vesa%40nxp.com%7Ceed68987c68f4aeaa63408d8bdf2d051%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637468197861821302%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=X1J8KgxFquNin80zKVz0Ao22vv1MuTMWf91BUTczh9Y%3D&amp;reserved=0  |
> 31137 Hildesheim, Germany                  | Phone: [phone]    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |

Adam Ford Feb. 3, 2021, 9:22 p.m. UTC | #10

On Thu, Jan 21, 2021 at 4:24 AM Abel Vesa <[e-mail]> wrote:
>
> On 21-01-21 10:56:17, Sascha Hauer wrote:
> > On Wed, Jan 20, 2021 at 06:14:21PM +0200, Abel Vesa wrote:
> > > On 21-01-20 16:50:01, Sascha Hauer wrote:
> > > > On Wed, Jan 20, 2021 at 05:28:13PM +0200, Abel Vesa wrote:
> > > > > On 21-01-20 16:13:05, Sascha Hauer wrote:
> > > > > > Hi Abel,
> > > > > >
> > > > > > On Wed, Jan 20, 2021 at 04:44:54PM +0200, Abel Vesa wrote:
> > > > > > > On 21-01-18 08:00:43, Adam Ford wrote:
> > > > > > > > On Mon, Jan 18, 2021 at 6:52 AM Abel Vesa <[e-mail]> wrote:
> > > > >
> > > > > ...
> > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > TBH, I'm against the idea of having to call consumer API from a clock provider driver.
> > > > > > > > > I'm still investigating a way of moving the uart clock control calls in drivers/serial/imx,
> > > > > > > > > where they belong.
> > > > > > > >
> > > > > > > > That makes sense.
> > > > > > > >
> > > > > > >
> > > > > > > Just a thought. The uart clock used for console remains on from u-boot,
> > > > > > > so maybe it's enough to just add the CLK_IGNORE_UNUSED flag to all the
> > > > > > > uart root clocks and remove the prepare/enable calls for uart clocks
> > > > > > > for good. I don't really have a way to test it right now, but maybe
> > > > > > > you could give it a try.
> > > > > >
> > > > > > That would mean that UART clocks will never be disabled, regardless of
> > > > > > whether they are used for console or not. That doesn't sound very
> > > > > > appealing.
> > > > >
> > > > > AFAIK, the only uart clock that is enabled by u-boot is the one used for
> > > > > the console. Later on, when the serial driver probes, it will enable it itself.
> > > > >
> > > > > Unless I'm missing something, this is exactly what we need.
> > > >
> > > > It might enable it, but with CLK_IGNORE_UNUSED the clock won't be
> > > > disabled again when a port is closed after usage
> > >
> > > OK, tell me what I'm getting wrong in the following scenario:
> > >
> > > U-boot leaves the console uart clock enabled. All the other ones are disabled.
> > >
> > > Kernel i.MX clk driver registers the uart clocks with flag CLK_IGNORE_UNUSED.
> >
> > I was wrong at that point. I originally thought the kernel will never
> > disable these clocks, but in fact it only leaves them enabled during the
> > clk_disable_unused call.
> >
> > However, when CLK_IGNORE_UNUSED becomes relevant it's too late already.
> > I just chatted with Lucas and he told me what the original problem was
> > that his patch solved.
> >
> > The problem comes when an unrelated device and the earlycon UART have
> > the same parent clocks. The parent clock is enabled, but it's reference
> > count is zero. Now when the unrelated device probes and toggles its
> > clocks then the shared parent clock will be disabled due to the
> > reference count being zero. Next time earlycon prints a character the
> > system hangs because the UART gates are still enabled, but their parent
> > clocks no longer are.
> >
>
> Hmm, that is indeed a problem. That's why I think there should be some
> kind of NOCACHE flag for almost all the types of clocks. For example,
> in this case, it makes sense for the core to check the bit in the register
> and then disable the parent based on that instead of relying on the refcount.
> Anyway, that's something that needs to be added in the CCF.
>
> > Overall I think Lucas' patches are still valid and relevant and with
> > Adams patches we even no longer have to enable all UART clocks, but
> > only the ones which are actually needed.
>
> Yeah, for now, I think we can go with Adam's patches. But longterm, I would
> like to remove the special case of the uart clocks we have right now in all
> the i.MX clock drivers.
>

Is the patch I submitted good enough for someone's acked-by or
reviewed-by, or are there changes I need to implement?

adam

> >
> > Sascha
> >
> >
> > --
> > Pengutronix e.K.                           |                             |
> > Steuerwalder Str. 21                       | https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pengutronix.de%2F&amp;data=04%7C01%7Cabel.vesa%40nxp.com%7Ceed68987c68f4aeaa63408d8bdf2d051%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637468197861821302%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=X1J8KgxFquNin80zKVz0Ao22vv1MuTMWf91BUTczh9Y%3D&amp;reserved=0  |
> > 31137 Hildesheim, Germany                  | Phone: [phone]    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |

Adam Ford Feb. 13, 2021, 2:44 p.m. UTC | #11

On Wed, Feb 3, 2021 at 3:22 PM Adam Ford <[e-mail]> wrote:
>
> On Thu, Jan 21, 2021 at 4:24 AM Abel Vesa <[e-mail]> wrote:
> >
> > On 21-01-21 10:56:17, Sascha Hauer wrote:
> > > On Wed, Jan 20, 2021 at 06:14:21PM +0200, Abel Vesa wrote:
> > > > On 21-01-20 16:50:01, Sascha Hauer wrote:
> > > > > On Wed, Jan 20, 2021 at 05:28:13PM +0200, Abel Vesa wrote:
> > > > > > On 21-01-20 16:13:05, Sascha Hauer wrote:
> > > > > > > Hi Abel,
> > > > > > >
> > > > > > > On Wed, Jan 20, 2021 at 04:44:54PM +0200, Abel Vesa wrote:
> > > > > > > > On 21-01-18 08:00:43, Adam Ford wrote:
> > > > > > > > > On Mon, Jan 18, 2021 at 6:52 AM Abel Vesa <[e-mail]> wrote:
> > > > > >
> > > > > > ...
> > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > TBH, I'm against the idea of having to call consumer API from a clock provider driver.
> > > > > > > > > > I'm still investigating a way of moving the uart clock control calls in drivers/serial/imx,
> > > > > > > > > > where they belong.
> > > > > > > > >
> > > > > > > > > That makes sense.
> > > > > > > > >
> > > > > > > >
> > > > > > > > Just a thought. The uart clock used for console remains on from u-boot,
> > > > > > > > so maybe it's enough to just add the CLK_IGNORE_UNUSED flag to all the
> > > > > > > > uart root clocks and remove the prepare/enable calls for uart clocks
> > > > > > > > for good. I don't really have a way to test it right now, but maybe
> > > > > > > > you could give it a try.
> > > > > > >
> > > > > > > That would mean that UART clocks will never be disabled, regardless of
> > > > > > > whether they are used for console or not. That doesn't sound very
> > > > > > > appealing.
> > > > > >
> > > > > > AFAIK, the only uart clock that is enabled by u-boot is the one used for
> > > > > > the console. Later on, when the serial driver probes, it will enable it itself.
> > > > > >
> > > > > > Unless I'm missing something, this is exactly what we need.
> > > > >
> > > > > It might enable it, but with CLK_IGNORE_UNUSED the clock won't be
> > > > > disabled again when a port is closed after usage
> > > >
> > > > OK, tell me what I'm getting wrong in the following scenario:
> > > >
> > > > U-boot leaves the console uart clock enabled. All the other ones are disabled.
> > > >
> > > > Kernel i.MX clk driver registers the uart clocks with flag CLK_IGNORE_UNUSED.
> > >
> > > I was wrong at that point. I originally thought the kernel will never
> > > disable these clocks, but in fact it only leaves them enabled during the
> > > clk_disable_unused call.
> > >
> > > However, when CLK_IGNORE_UNUSED becomes relevant it's too late already.
> > > I just chatted with Lucas and he told me what the original problem was
> > > that his patch solved.
> > >
> > > The problem comes when an unrelated device and the earlycon UART have
> > > the same parent clocks. The parent clock is enabled, but it's reference
> > > count is zero. Now when the unrelated device probes and toggles its
> > > clocks then the shared parent clock will be disabled due to the
> > > reference count being zero. Next time earlycon prints a character the
> > > system hangs because the UART gates are still enabled, but their parent
> > > clocks no longer are.
> > >
> >
> > Hmm, that is indeed a problem. That's why I think there should be some
> > kind of NOCACHE flag for almost all the types of clocks. For example,
> > in this case, it makes sense for the core to check the bit in the register
> > and then disable the parent based on that instead of relying on the refcount.
> > Anyway, that's something that needs to be added in the CCF.
> >
> > > Overall I think Lucas' patches are still valid and relevant and with
> > > Adams patches we even no longer have to enable all UART clocks, but
> > > only the ones which are actually needed.
> >
> > Yeah, for now, I think we can go with Adam's patches. But longterm, I would
> > like to remove the special case of the uart clocks we have right now in all
> > the i.MX clock drivers.

I looked around at other serial drivers, and I found nothing like this
function for enabling all UART clocks.  There are generic functions
for registering consoles, earlycon etc, and the serial driver fetches
the per and igp clocks from the device tree, so I attempted to simply
remove imx_register_uart_clocks().  I booted an i.MX8M Nano from a
fully-powered off state, and my serial console came up just fine.

I checked the clk_summary, and the clock parents are set correctly and
the respective clock rates appear to be correct (ie, the console is
working at the desired baud rate, and Bluetooth is happy)

Since I don't fully understand the serial driver and the clock
dependencies, I don't want to just simply remove the function without
discussing it, because I don't know the ramifications.  However, when
testing on the i.MX8M Nano, things look OK.
I also tested suspend-resume and the console UART appears to return
and the Bluetooth UART set to 4Mbps works just fine too.

I'd like to post a V4 which just removes imx_register_uart_clocks and
the corresponding calls to it.  I don't know enough about the older
32-bit i.MX SoC's, but I have build-tested it, and I can generate a
patch. Are there any objections and/or concerns?

adam
> >
>
> Is the patch I submitted good enough for someone's acked-by or
> reviewed-by, or are there changes I need to implement?
>
> adam
>
> > >
> > > Sascha
> > >
> > >
> > > --
> > > Pengutronix e.K.                           |                             |
> > > Steuerwalder Str. 21                       | https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pengutronix.de%2F&amp;data=04%7C01%7Cabel.vesa%40nxp.com%7Ceed68987c68f4aeaa63408d8bdf2d051%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637468197861821302%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=X1J8KgxFquNin80zKVz0Ao22vv1MuTMWf91BUTczh9Y%3D&amp;reserved=0  |
> > > 31137 Hildesheim, Germany                  | Phone: [phone]    |
> > > Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |

Abel Vesa Feb. 15, 2021, 1:06 p.m. UTC | #12

On 21-02-13 08:44:28, Adam Ford wrote:
> On Wed, Feb 3, 2021 at 3:22 PM Adam Ford <[e-mail]> wrote:
> >
> > On Thu, Jan 21, 2021 at 4:24 AM Abel Vesa <[e-mail]> wrote:
> > >
> > > On 21-01-21 10:56:17, Sascha Hauer wrote:
> > > > On Wed, Jan 20, 2021 at 06:14:21PM +0200, Abel Vesa wrote:
> > > > > On 21-01-20 16:50:01, Sascha Hauer wrote:
> > > > > > On Wed, Jan 20, 2021 at 05:28:13PM +0200, Abel Vesa wrote:
> > > > > > > On 21-01-20 16:13:05, Sascha Hauer wrote:
> > > > > > > > Hi Abel,
> > > > > > > >
> > > > > > > > On Wed, Jan 20, 2021 at 04:44:54PM +0200, Abel Vesa wrote:
> > > > > > > > > On 21-01-18 08:00:43, Adam Ford wrote:
> > > > > > > > > > On Mon, Jan 18, 2021 at 6:52 AM Abel Vesa <[e-mail]> wrote:
> > > > > > >
> > > > > > > ...
> > > > > > >
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > TBH, I'm against the idea of having to call consumer API from a clock provider driver.
> > > > > > > > > > > I'm still investigating a way of moving the uart clock control calls in drivers/serial/imx,
> > > > > > > > > > > where they belong.
> > > > > > > > > >
> > > > > > > > > > That makes sense.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Just a thought. The uart clock used for console remains on from u-boot,
> > > > > > > > > so maybe it's enough to just add the CLK_IGNORE_UNUSED flag to all the
> > > > > > > > > uart root clocks and remove the prepare/enable calls for uart clocks
> > > > > > > > > for good. I don't really have a way to test it right now, but maybe
> > > > > > > > > you could give it a try.
> > > > > > > >
> > > > > > > > That would mean that UART clocks will never be disabled, regardless of
> > > > > > > > whether they are used for console or not. That doesn't sound very
> > > > > > > > appealing.
> > > > > > >
> > > > > > > AFAIK, the only uart clock that is enabled by u-boot is the one used for
> > > > > > > the console. Later on, when the serial driver probes, it will enable it itself.
> > > > > > >
> > > > > > > Unless I'm missing something, this is exactly what we need.
> > > > > >
> > > > > > It might enable it, but with CLK_IGNORE_UNUSED the clock won't be
> > > > > > disabled again when a port is closed after usage
> > > > >
> > > > > OK, tell me what I'm getting wrong in the following scenario:
> > > > >
> > > > > U-boot leaves the console uart clock enabled. All the other ones are disabled.
> > > > >
> > > > > Kernel i.MX clk driver registers the uart clocks with flag CLK_IGNORE_UNUSED.
> > > >
> > > > I was wrong at that point. I originally thought the kernel will never
> > > > disable these clocks, but in fact it only leaves them enabled during the
> > > > clk_disable_unused call.
> > > >
> > > > However, when CLK_IGNORE_UNUSED becomes relevant it's too late already.
> > > > I just chatted with Lucas and he told me what the original problem was
> > > > that his patch solved.
> > > >
> > > > The problem comes when an unrelated device and the earlycon UART have
> > > > the same parent clocks. The parent clock is enabled, but it's reference
> > > > count is zero. Now when the unrelated device probes and toggles its
> > > > clocks then the shared parent clock will be disabled due to the
> > > > reference count being zero. Next time earlycon prints a character the
> > > > system hangs because the UART gates are still enabled, but their parent
> > > > clocks no longer are.
> > > >
> > >
> > > Hmm, that is indeed a problem. That's why I think there should be some
> > > kind of NOCACHE flag for almost all the types of clocks. For example,
> > > in this case, it makes sense for the core to check the bit in the register
> > > and then disable the parent based on that instead of relying on the refcount.
> > > Anyway, that's something that needs to be added in the CCF.
> > >
> > > > Overall I think Lucas' patches are still valid and relevant and with
> > > > Adams patches we even no longer have to enable all UART clocks, but
> > > > only the ones which are actually needed.
> > >
> > > Yeah, for now, I think we can go with Adam's patches. But longterm, I would
> > > like to remove the special case of the uart clocks we have right now in all
> > > the i.MX clock drivers.
> 
> I looked around at other serial drivers, and I found nothing like this
> function for enabling all UART clocks.  There are generic functions
> for registering consoles, earlycon etc, and the serial driver fetches
> the per and igp clocks from the device tree, so I attempted to simply
> remove imx_register_uart_clocks().  I booted an i.MX8M Nano from a
> fully-powered off state, and my serial console came up just fine.

Just because it works, doesn't mean it is safe. To put it simply, the
risk of some  driver disabling a clock that is parent of the uart clock
would render the earlycon broken.

> 
> I checked the clk_summary, and the clock parents are set correctly and
> the respective clock rates appear to be correct (ie, the console is
> working at the desired baud rate, and Bluetooth is happy)
> 
> Since I don't fully understand the serial driver and the clock
> dependencies, I don't want to just simply remove the function without
> discussing it, because I don't know the ramifications.  However, when
> testing on the i.MX8M Nano, things look OK.
> I also tested suspend-resume and the console UART appears to return
> and the Bluetooth UART set to 4Mbps works just fine too.
> 
> I'd like to post a V4 which just removes imx_register_uart_clocks and
> the corresponding calls to it.  I don't know enough about the older
> 32-bit i.MX SoC's, but I have build-tested it, and I can generate a
> patch. Are there any objections and/or concerns?
> 

Please don't remove the imx_register_uart_clocks for now. As much as I
would like it gone, the way the earlycon could end up broken is
so ugly that it would make it a real pain to debug it later on.

> adam
> > >
> >
> > Is the patch I submitted good enough for someone's acked-by or
> > reviewed-by, or are there changes I need to implement?
> >
> > adam
> >
> > > >
> > > > Sascha
> > > >
> > > >
> > > > --
> > > > Pengutronix e.K.                           |                             |
> > > > Steuerwalder Str. 21                       | https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pengutronix.de%2F&amp;data=04%7C01%7Cabel.vesa%40nxp.com%7C12862aeac2934a4a94f108d8d02de573%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637488242849833051%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=fKDvWYJIU7hBROH6yDM9FYUCHJDK9wMjTeJDMxRff8o%3D&amp;reserved=0  |
> > > > 31137 Hildesheim, Germany                  | Phone: [phone]    |
> > > > Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |

Adam Ford March 2, 2021, 7:03 p.m. UTC | #13

On Mon, Feb 15, 2021 at 7:06 AM Abel Vesa <[e-mail]> wrote:
>
> On 21-02-13 08:44:28, Adam Ford wrote:
> > On Wed, Feb 3, 2021 at 3:22 PM Adam Ford <[e-mail]> wrote:
> > >
> > > On Thu, Jan 21, 2021 at 4:24 AM Abel Vesa <[e-mail]> wrote:
> > > >
> > > > On 21-01-21 10:56:17, Sascha Hauer wrote:
> > > > > On Wed, Jan 20, 2021 at 06:14:21PM +0200, Abel Vesa wrote:
> > > > > > On 21-01-20 16:50:01, Sascha Hauer wrote:
> > > > > > > On Wed, Jan 20, 2021 at 05:28:13PM +0200, Abel Vesa wrote:
> > > > > > > > On 21-01-20 16:13:05, Sascha Hauer wrote:
> > > > > > > > > Hi Abel,
> > > > > > > > >
> > > > > > > > > On Wed, Jan 20, 2021 at 04:44:54PM +0200, Abel Vesa wrote:
> > > > > > > > > > On 21-01-18 08:00:43, Adam Ford wrote:
> > > > > > > > > > > On Mon, Jan 18, 2021 at 6:52 AM Abel Vesa <[e-mail]> wrote:
> > > > > > > >
> > > > > > > > ...
> > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > TBH, I'm against the idea of having to call consumer API from a clock provider driver.
> > > > > > > > > > > > I'm still investigating a way of moving the uart clock control calls in drivers/serial/imx,
> > > > > > > > > > > > where they belong.
> > > > > > > > > > >
> > > > > > > > > > > That makes sense.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Just a thought. The uart clock used for console remains on from u-boot,
> > > > > > > > > > so maybe it's enough to just add the CLK_IGNORE_UNUSED flag to all the
> > > > > > > > > > uart root clocks and remove the prepare/enable calls for uart clocks
> > > > > > > > > > for good. I don't really have a way to test it right now, but maybe
> > > > > > > > > > you could give it a try.
> > > > > > > > >
> > > > > > > > > That would mean that UART clocks will never be disabled, regardless of
> > > > > > > > > whether they are used for console or not. That doesn't sound very
> > > > > > > > > appealing.
> > > > > > > >
> > > > > > > > AFAIK, the only uart clock that is enabled by u-boot is the one used for
> > > > > > > > the console. Later on, when the serial driver probes, it will enable it itself.
> > > > > > > >
> > > > > > > > Unless I'm missing something, this is exactly what we need.
> > > > > > >
> > > > > > > It might enable it, but with CLK_IGNORE_UNUSED the clock won't be
> > > > > > > disabled again when a port is closed after usage
> > > > > >
> > > > > > OK, tell me what I'm getting wrong in the following scenario:
> > > > > >
> > > > > > U-boot leaves the console uart clock enabled. All the other ones are disabled.
> > > > > >
> > > > > > Kernel i.MX clk driver registers the uart clocks with flag CLK_IGNORE_UNUSED.
> > > > >
> > > > > I was wrong at that point. I originally thought the kernel will never
> > > > > disable these clocks, but in fact it only leaves them enabled during the
> > > > > clk_disable_unused call.
> > > > >
> > > > > However, when CLK_IGNORE_UNUSED becomes relevant it's too late already.
> > > > > I just chatted with Lucas and he told me what the original problem was
> > > > > that his patch solved.
> > > > >
> > > > > The problem comes when an unrelated device and the earlycon UART have
> > > > > the same parent clocks. The parent clock is enabled, but it's reference
> > > > > count is zero. Now when the unrelated device probes and toggles its
> > > > > clocks then the shared parent clock will be disabled due to the
> > > > > reference count being zero. Next time earlycon prints a character the
> > > > > system hangs because the UART gates are still enabled, but their parent
> > > > > clocks no longer are.
> > > > >
> > > >
> > > > Hmm, that is indeed a problem. That's why I think there should be some
> > > > kind of NOCACHE flag for almost all the types of clocks. For example,
> > > > in this case, it makes sense for the core to check the bit in the register
> > > > and then disable the parent based on that instead of relying on the refcount.
> > > > Anyway, that's something that needs to be added in the CCF.
> > > >
> > > > > Overall I think Lucas' patches are still valid and relevant and with
> > > > > Adams patches we even no longer have to enable all UART clocks, but
> > > > > only the ones which are actually needed.
> > > >
> > > > Yeah, for now, I think we can go with Adam's patches. But longterm, I would
> > > > like to remove the special case of the uart clocks we have right now in all
> > > > the i.MX clock drivers.
> >
> > I looked around at other serial drivers, and I found nothing like this
> > function for enabling all UART clocks.  There are generic functions
> > for registering consoles, earlycon etc, and the serial driver fetches
> > the per and igp clocks from the device tree, so I attempted to simply
> > remove imx_register_uart_clocks().  I booted an i.MX8M Nano from a
> > fully-powered off state, and my serial console came up just fine.
>
> Just because it works, doesn't mean it is safe. To put it simply, the
> risk of some  driver disabling a clock that is parent of the uart clock
> would render the earlycon broken.
>
> >
> > I checked the clk_summary, and the clock parents are set correctly and
> > the respective clock rates appear to be correct (ie, the console is
> > working at the desired baud rate, and Bluetooth is happy)
> >
> > Since I don't fully understand the serial driver and the clock
> > dependencies, I don't want to just simply remove the function without
> > discussing it, because I don't know the ramifications.  However, when
> > testing on the i.MX8M Nano, things look OK.
> > I also tested suspend-resume and the console UART appears to return
> > and the Bluetooth UART set to 4Mbps works just fine too.
> >
> > I'd like to post a V4 which just removes imx_register_uart_clocks and
> > the corresponding calls to it.  I don't know enough about the older
> > 32-bit i.MX SoC's, but I have build-tested it, and I can generate a
> > patch. Are there any objections and/or concerns?
> >
>
> Please don't remove the imx_register_uart_clocks for now. As much as I
> would like it gone, the way the earlycon could end up broken is
> so ugly that it would make it a real pain to debug it later on.

I won't do a V4, but where do we go from here?  I have a V3 that was
waiting for reviews, but there were some concerns.  We currently
cannot re-parent the UART's on iMX8MM or iMX8MN.  Should I resend V3,
or are there fixes/changes requested to V3?

adam
>
> > adam
> > > >
> > >
> > > Is the patch I submitted good enough for someone's acked-by or
> > > reviewed-by, or are there changes I need to implement?
> > >
> > > adam
> > >
> > > > >
> > > > > Sascha
> > > > >
> > > > >
> > > > > --
> > > > > Pengutronix e.K.                           |                             |
> > > > > Steuerwalder Str. 21                       | https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pengutronix.de%2F&amp;data=04%7C01%7Cabel.vesa%40nxp.com%7C12862aeac2934a4a94f108d8d02de573%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637488242849833051%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=fKDvWYJIU7hBROH6yDM9FYUCHJDK9wMjTeJDMxRff8o%3D&amp;reserved=0  |
> > > > > 31137 Hildesheim, Germany                  | Phone: [phone]    |
> > > > > Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |

Abel Vesa March 3, 2021, 8:31 a.m. UTC | #14

On 21-03-02 13:03:04, Adam Ford wrote:
> On Mon, Feb 15, 2021 at 7:06 AM Abel Vesa <[e-mail]> wrote:
> >
> > On 21-02-13 08:44:28, Adam Ford wrote:
> > > On Wed, Feb 3, 2021 at 3:22 PM Adam Ford <[e-mail]> wrote:
> > > >
> > > > On Thu, Jan 21, 2021 at 4:24 AM Abel Vesa <[e-mail]> wrote:
> > > > >
> > > > > On 21-01-21 10:56:17, Sascha Hauer wrote:
> > > > > > On Wed, Jan 20, 2021 at 06:14:21PM +0200, Abel Vesa wrote:
> > > > > > > On 21-01-20 16:50:01, Sascha Hauer wrote:
> > > > > > > > On Wed, Jan 20, 2021 at 05:28:13PM +0200, Abel Vesa wrote:
> > > > > > > > > On 21-01-20 16:13:05, Sascha Hauer wrote:
> > > > > > > > > > Hi Abel,
> > > > > > > > > >
> > > > > > > > > > On Wed, Jan 20, 2021 at 04:44:54PM +0200, Abel Vesa wrote:
> > > > > > > > > > > On 21-01-18 08:00:43, Adam Ford wrote:
> > > > > > > > > > > > On Mon, Jan 18, 2021 at 6:52 AM Abel Vesa <[e-mail]> wrote:
> > > > > > > > >
> > > > > > > > > ...
> > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > TBH, I'm against the idea of having to call consumer API from a clock provider driver.
> > > > > > > > > > > > > I'm still investigating a way of moving the uart clock control calls in drivers/serial/imx,
> > > > > > > > > > > > > where they belong.
> > > > > > > > > > > >
> > > > > > > > > > > > That makes sense.
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Just a thought. The uart clock used for console remains on from u-boot,
> > > > > > > > > > > so maybe it's enough to just add the CLK_IGNORE_UNUSED flag to all the
> > > > > > > > > > > uart root clocks and remove the prepare/enable calls for uart clocks
> > > > > > > > > > > for good. I don't really have a way to test it right now, but maybe
> > > > > > > > > > > you could give it a try.
> > > > > > > > > >
> > > > > > > > > > That would mean that UART clocks will never be disabled, regardless of
> > > > > > > > > > whether they are used for console or not. That doesn't sound very
> > > > > > > > > > appealing.
> > > > > > > > >
> > > > > > > > > AFAIK, the only uart clock that is enabled by u-boot is the one used for
> > > > > > > > > the console. Later on, when the serial driver probes, it will enable it itself.
> > > > > > > > >
> > > > > > > > > Unless I'm missing something, this is exactly what we need.
> > > > > > > >
> > > > > > > > It might enable it, but with CLK_IGNORE_UNUSED the clock won't be
> > > > > > > > disabled again when a port is closed after usage
> > > > > > >
> > > > > > > OK, tell me what I'm getting wrong in the following scenario:
> > > > > > >
> > > > > > > U-boot leaves the console uart clock enabled. All the other ones are disabled.
> > > > > > >
> > > > > > > Kernel i.MX clk driver registers the uart clocks with flag CLK_IGNORE_UNUSED.
> > > > > >
> > > > > > I was wrong at that point. I originally thought the kernel will never
> > > > > > disable these clocks, but in fact it only leaves them enabled during the
> > > > > > clk_disable_unused call.
> > > > > >
> > > > > > However, when CLK_IGNORE_UNUSED becomes relevant it's too late already.
> > > > > > I just chatted with Lucas and he told me what the original problem was
> > > > > > that his patch solved.
> > > > > >
> > > > > > The problem comes when an unrelated device and the earlycon UART have
> > > > > > the same parent clocks. The parent clock is enabled, but it's reference
> > > > > > count is zero. Now when the unrelated device probes and toggles its
> > > > > > clocks then the shared parent clock will be disabled due to the
> > > > > > reference count being zero. Next time earlycon prints a character the
> > > > > > system hangs because the UART gates are still enabled, but their parent
> > > > > > clocks no longer are.
> > > > > >
> > > > >
> > > > > Hmm, that is indeed a problem. That's why I think there should be some
> > > > > kind of NOCACHE flag for almost all the types of clocks. For example,
> > > > > in this case, it makes sense for the core to check the bit in the register
> > > > > and then disable the parent based on that instead of relying on the refcount.
> > > > > Anyway, that's something that needs to be added in the CCF.
> > > > >
> > > > > > Overall I think Lucas' patches are still valid and relevant and with
> > > > > > Adams patches we even no longer have to enable all UART clocks, but
> > > > > > only the ones which are actually needed.
> > > > >
> > > > > Yeah, for now, I think we can go with Adam's patches. But longterm, I would
> > > > > like to remove the special case of the uart clocks we have right now in all
> > > > > the i.MX clock drivers.
> > >
> > > I looked around at other serial drivers, and I found nothing like this
> > > function for enabling all UART clocks.  There are generic functions
> > > for registering consoles, earlycon etc, and the serial driver fetches
> > > the per and igp clocks from the device tree, so I attempted to simply
> > > remove imx_register_uart_clocks().  I booted an i.MX8M Nano from a
> > > fully-powered off state, and my serial console came up just fine.
> >
> > Just because it works, doesn't mean it is safe. To put it simply, the
> > risk of some  driver disabling a clock that is parent of the uart clock
> > would render the earlycon broken.
> >
> > >
> > > I checked the clk_summary, and the clock parents are set correctly and
> > > the respective clock rates appear to be correct (ie, the console is
> > > working at the desired baud rate, and Bluetooth is happy)
> > >
> > > Since I don't fully understand the serial driver and the clock
> > > dependencies, I don't want to just simply remove the function without
> > > discussing it, because I don't know the ramifications.  However, when
> > > testing on the i.MX8M Nano, things look OK.
> > > I also tested suspend-resume and the console UART appears to return
> > > and the Bluetooth UART set to 4Mbps works just fine too.
> > >
> > > I'd like to post a V4 which just removes imx_register_uart_clocks and
> > > the corresponding calls to it.  I don't know enough about the older
> > > 32-bit i.MX SoC's, but I have build-tested it, and I can generate a
> > > patch. Are there any objections and/or concerns?
> > >
> >
> > Please don't remove the imx_register_uart_clocks for now. As much as I
> > would like it gone, the way the earlycon could end up broken is
> > so ugly that it would make it a real pain to debug it later on.
> 
> I won't do a V4, but where do we go from here?  I have a V3 that was
> waiting for reviews, but there were some concerns.  We currently
> cannot re-parent the UART's on iMX8MM or iMX8MN.  Should I resend V3,
> or are there fixes/changes requested to V3?
> 

The v3 is fine. No need to resend.

Reviewed-by: Abel Vesa <[e-mail]>

> adam
> >
> > > adam
> > > > >
> > > >
> > > > Is the patch I submitted good enough for someone's acked-by or
> > > > reviewed-by, or are there changes I need to implement?
> > > >
> > > > adam
> > > >
> > > > > >
> > > > > > Sascha
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Pengutronix e.K.                           |                             |
> > > > > > Steuerwalder Str. 21                       | https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pengutronix.de%2F&amp;data=04%7C01%7Cabel.vesa%40nxp.com%7C900fb6f7dfdf49776eb208d8ddadd7b7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637503086023262296%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=ulZnxvilARLN03%2BEr%2BTKmVOPxRTukN%2FDg3oPY8UJ2AU%3D&amp;reserved=0  |
> > > > > > 31137 Hildesheim, Germany                  | Phone: [phone]    |
> > > > > > Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |

Ahmad Fatoum March 5, 2021, 12:21 p.m. UTC | #15

Hello Adam,

On 15.01.21 19:29, Adam Ford wrote:
>  {
> +#ifdef CONFIG_OF
>  	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]);
> +		imx_uart_clocks = kcalloc(clk_count, sizeof(struct clk *), GFP_KERNEL);
> +		imx_enabled_uart_clocks = 0;
> +
> +		for (i = 0; i < clk_count; i++) {
> +			imx_uart_clocks[imx_enabled_uart_clocks] = of_clk_get(of_stdout, i);

of_stdout may be NULL if there is no stdout-path. You should check that earlier.

With this fixed, feel free to add:

Tested-by: Ahmad Fatoum <[e-mail]>

> +
> +			/* Stop if there are no more of_stdout references */
> +			if (IS_ERR(imx_uart_clocks[imx_enabled_uart_clocks]))
> +				return;
> +
> +			/* Only enable the clock if it's not NULL */
> +			if (imx_uart_clocks[imx_enabled_uart_clocks])
> +				clk_prepare_enable(imx_uart_clocks[imx_enabled_uart_clocks++]);
> +		}
>  	}
> +#else
> +	/* i.MX boards use device trees now.  For build tests without CONFIG_OF, do nothing */
> +	imx_enabled_uart_clocks = 0;
> +#endif
>  }
>  
>  static int __init imx_clk_disable_uart(void)
>  {
> -	if (imx_keep_uart_clocks && imx_uart_clocks) {
> +	if (imx_keep_uart_clocks && imx_enabled_uart_clocks) {
>  		int i;
>  
> -		for (i = 0; imx_uart_clocks[i]; i++)
> -			clk_disable_unprepare(*imx_uart_clocks[i]);
> +		for (i = 0; i < imx_enabled_uart_clocks; i++) {
> +			clk_disable_unprepare(imx_uart_clocks[i]);
> +			clk_put(imx_uart_clocks[i]);
> +		};
> +		kfree(imx_uart_clocks);
>  	}
>  
>  	return 0;
> diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
> index 4f04c8287286..7571603bee23 100644
> --- a/drivers/clk/imx/clk.h
> +++ b/drivers/clk/imx/clk.h
> @@ -11,9 +11,9 @@ extern spinlock_t imx_ccm_lock;
>  void imx_check_clocks(struct clk *clks[], unsigned int count);
>  void imx_check_clk_hws(struct clk_hw *clks[], unsigned int count);
>  #ifndef MODULE
> -void imx_register_uart_clocks(struct clk ** const clks[]);
> +void imx_register_uart_clocks(unsigned int clk_count);
>  #else
> -static inline void imx_register_uart_clocks(struct clk ** const clks[])
> +static inline void imx_register_uart_clocks(unsigned int clk_count)
>  {
>  }
>  #endif
>

Abel Vesa March 10, 2021, 10:24 p.m. UTC | #16

On 21-03-03 10:31:19, Abel Vesa wrote:
> On 21-03-02 13:03:04, Adam Ford wrote:
> > On Mon, Feb 15, 2021 at 7:06 AM Abel Vesa <[e-mail]> wrote:
> > >
> > > On 21-02-13 08:44:28, Adam Ford wrote:
> > > > On Wed, Feb 3, 2021 at 3:22 PM Adam Ford <[e-mail]> wrote:
> > > > >
> > > > > On Thu, Jan 21, 2021 at 4:24 AM Abel Vesa <[e-mail]> wrote:
> > > > > >
> > > > > > On 21-01-21 10:56:17, Sascha Hauer wrote:
> > > > > > > On Wed, Jan 20, 2021 at 06:14:21PM +0200, Abel Vesa wrote:
> > > > > > > > On 21-01-20 16:50:01, Sascha Hauer wrote:
> > > > > > > > > On Wed, Jan 20, 2021 at 05:28:13PM +0200, Abel Vesa wrote:
> > > > > > > > > > On 21-01-20 16:13:05, Sascha Hauer wrote:
> > > > > > > > > > > Hi Abel,
> > > > > > > > > > >
> > > > > > > > > > > On Wed, Jan 20, 2021 at 04:44:54PM +0200, Abel Vesa wrote:
> > > > > > > > > > > > On 21-01-18 08:00:43, Adam Ford wrote:
> > > > > > > > > > > > > On Mon, Jan 18, 2021 at 6:52 AM Abel Vesa <[e-mail]> wrote:
> > > > > > > > > >
> > > > > > > > > > ...
> > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > TBH, I'm against the idea of having to call consumer API from a clock provider driver.
> > > > > > > > > > > > > > I'm still investigating a way of moving the uart clock control calls in drivers/serial/imx,
> > > > > > > > > > > > > > where they belong.
> > > > > > > > > > > > >
> > > > > > > > > > > > > That makes sense.
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > Just a thought. The uart clock used for console remains on from u-boot,
> > > > > > > > > > > > so maybe it's enough to just add the CLK_IGNORE_UNUSED flag to all the
> > > > > > > > > > > > uart root clocks and remove the prepare/enable calls for uart clocks
> > > > > > > > > > > > for good. I don't really have a way to test it right now, but maybe
> > > > > > > > > > > > you could give it a try.
> > > > > > > > > > >
> > > > > > > > > > > That would mean that UART clocks will never be disabled, regardless of
> > > > > > > > > > > whether they are used for console or not. That doesn't sound very
> > > > > > > > > > > appealing.
> > > > > > > > > >
> > > > > > > > > > AFAIK, the only uart clock that is enabled by u-boot is the one used for
> > > > > > > > > > the console. Later on, when the serial driver probes, it will enable it itself.
> > > > > > > > > >
> > > > > > > > > > Unless I'm missing something, this is exactly what we need.
> > > > > > > > >
> > > > > > > > > It might enable it, but with CLK_IGNORE_UNUSED the clock won't be
> > > > > > > > > disabled again when a port is closed after usage
> > > > > > > >
> > > > > > > > OK, tell me what I'm getting wrong in the following scenario:
> > > > > > > >
> > > > > > > > U-boot leaves the console uart clock enabled. All the other ones are disabled.
> > > > > > > >
> > > > > > > > Kernel i.MX clk driver registers the uart clocks with flag CLK_IGNORE_UNUSED.
> > > > > > >
> > > > > > > I was wrong at that point. I originally thought the kernel will never
> > > > > > > disable these clocks, but in fact it only leaves them enabled during the
> > > > > > > clk_disable_unused call.
> > > > > > >
> > > > > > > However, when CLK_IGNORE_UNUSED becomes relevant it's too late already.
> > > > > > > I just chatted with Lucas and he told me what the original problem was
> > > > > > > that his patch solved.
> > > > > > >
> > > > > > > The problem comes when an unrelated device and the earlycon UART have
> > > > > > > the same parent clocks. The parent clock is enabled, but it's reference
> > > > > > > count is zero. Now when the unrelated device probes and toggles its
> > > > > > > clocks then the shared parent clock will be disabled due to the
> > > > > > > reference count being zero. Next time earlycon prints a character the
> > > > > > > system hangs because the UART gates are still enabled, but their parent
> > > > > > > clocks no longer are.
> > > > > > >
> > > > > >
> > > > > > Hmm, that is indeed a problem. That's why I think there should be some
> > > > > > kind of NOCACHE flag for almost all the types of clocks. For example,
> > > > > > in this case, it makes sense for the core to check the bit in the register
> > > > > > and then disable the parent based on that instead of relying on the refcount.
> > > > > > Anyway, that's something that needs to be added in the CCF.
> > > > > >
> > > > > > > Overall I think Lucas' patches are still valid and relevant and with
> > > > > > > Adams patches we even no longer have to enable all UART clocks, but
> > > > > > > only the ones which are actually needed.
> > > > > >
> > > > > > Yeah, for now, I think we can go with Adam's patches. But longterm, I would
> > > > > > like to remove the special case of the uart clocks we have right now in all
> > > > > > the i.MX clock drivers.
> > > >
> > > > I looked around at other serial drivers, and I found nothing like this
> > > > function for enabling all UART clocks.  There are generic functions
> > > > for registering consoles, earlycon etc, and the serial driver fetches
> > > > the per and igp clocks from the device tree, so I attempted to simply
> > > > remove imx_register_uart_clocks().  I booted an i.MX8M Nano from a
> > > > fully-powered off state, and my serial console came up just fine.
> > >
> > > Just because it works, doesn't mean it is safe. To put it simply, the
> > > risk of some  driver disabling a clock that is parent of the uart clock
> > > would render the earlycon broken.
> > >
> > > >
> > > > I checked the clk_summary, and the clock parents are set correctly and
> > > > the respective clock rates appear to be correct (ie, the console is
> > > > working at the desired baud rate, and Bluetooth is happy)
> > > >
> > > > Since I don't fully understand the serial driver and the clock
> > > > dependencies, I don't want to just simply remove the function without
> > > > discussing it, because I don't know the ramifications.  However, when
> > > > testing on the i.MX8M Nano, things look OK.
> > > > I also tested suspend-resume and the console UART appears to return
> > > > and the Bluetooth UART set to 4Mbps works just fine too.
> > > >
> > > > I'd like to post a V4 which just removes imx_register_uart_clocks and
> > > > the corresponding calls to it.  I don't know enough about the older
> > > > 32-bit i.MX SoC's, but I have build-tested it, and I can generate a
> > > > patch. Are there any objections and/or concerns?
> > > >
> > >
> > > Please don't remove the imx_register_uart_clocks for now. As much as I
> > > would like it gone, the way the earlycon could end up broken is
> > > so ugly that it would make it a real pain to debug it later on.
> > 
> > I won't do a V4, but where do we go from here?  I have a V3 that was
> > waiting for reviews, but there were some concerns.  We currently
> > cannot re-parent the UART's on iMX8MM or iMX8MN.  Should I resend V3,
> > or are there fixes/changes requested to V3?
> > 
> 
> The v3 is fine. No need to resend.
> 
> Reviewed-by: Abel Vesa <[e-mail]>
> 

Please add Ahmad's suggestion and resend.

I'll pick it up then.

Thanks,
Abel

> > adam
> > >
> > > > adam
> > > > > >
> > > > >
> > > > > Is the patch I submitted good enough for someone's acked-by or
> > > > > reviewed-by, or are there changes I need to implement?
> > > > >
> > > > > adam
> > > > >
> > > > > > >
> > > > > > > Sascha
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Pengutronix e.K.                           |                             |
> > > > > > > Steuerwalder Str. 21                       | https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pengutronix.de%2F&amp;data=04%7C01%7Cabel.vesa%40nxp.com%7C900fb6f7dfdf49776eb208d8ddadd7b7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637503086023262296%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=ulZnxvilARLN03%2BEr%2BTKmVOPxRTukN%2FDg3oPY8UJ2AU%3D&amp;reserved=0  |
> > > > > > > 31137 Hildesheim, Germany                  | Phone: [phone]    |
> > > > > > > Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |

Adam Ford March 10, 2021, 10:43 p.m. UTC | #17

On Wed, Mar 10, 2021 at 4:25 PM Abel Vesa <[e-mail]> wrote:
>
> On 21-03-03 10:31:19, Abel Vesa wrote:
> > On 21-03-02 13:03:04, Adam Ford wrote:
> > > On Mon, Feb 15, 2021 at 7:06 AM Abel Vesa <[e-mail]> wrote:
> > > >
> > > > On 21-02-13 08:44:28, Adam Ford wrote:
> > > > > On Wed, Feb 3, 2021 at 3:22 PM Adam Ford <[e-mail]> wrote:
> > > > > >
> > > > > > On Thu, Jan 21, 2021 at 4:24 AM Abel Vesa <[e-mail]> wrote:
> > > > > > >
> > > > > > > On 21-01-21 10:56:17, Sascha Hauer wrote:
> > > > > > > > On Wed, Jan 20, 2021 at 06:14:21PM +0200, Abel Vesa wrote:
> > > > > > > > > On 21-01-20 16:50:01, Sascha Hauer wrote:
> > > > > > > > > > On Wed, Jan 20, 2021 at 05:28:13PM +0200, Abel Vesa wrote:
> > > > > > > > > > > On 21-01-20 16:13:05, Sascha Hauer wrote:
> > > > > > > > > > > > Hi Abel,
> > > > > > > > > > > >
> > > > > > > > > > > > On Wed, Jan 20, 2021 at 04:44:54PM +0200, Abel Vesa wrote:
> > > > > > > > > > > > > On 21-01-18 08:00:43, Adam Ford wrote:
> > > > > > > > > > > > > > On Mon, Jan 18, 2021 at 6:52 AM Abel Vesa <[e-mail]> wrote:
> > > > > > > > > > >
> > > > > > > > > > > ...
> > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > TBH, I'm against the idea of having to call consumer API from a clock provider driver.
> > > > > > > > > > > > > > > I'm still investigating a way of moving the uart clock control calls in drivers/serial/imx,
> > > > > > > > > > > > > > > where they belong.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > That makes sense.
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > Just a thought. The uart clock used for console remains on from u-boot,
> > > > > > > > > > > > > so maybe it's enough to just add the CLK_IGNORE_UNUSED flag to all the
> > > > > > > > > > > > > uart root clocks and remove the prepare/enable calls for uart clocks
> > > > > > > > > > > > > for good. I don't really have a way to test it right now, but maybe
> > > > > > > > > > > > > you could give it a try.
> > > > > > > > > > > >
> > > > > > > > > > > > That would mean that UART clocks will never be disabled, regardless of
> > > > > > > > > > > > whether they are used for console or not. That doesn't sound very
> > > > > > > > > > > > appealing.
> > > > > > > > > > >
> > > > > > > > > > > AFAIK, the only uart clock that is enabled by u-boot is the one used for
> > > > > > > > > > > the console. Later on, when the serial driver probes, it will enable it itself.
> > > > > > > > > > >
> > > > > > > > > > > Unless I'm missing something, this is exactly what we need.
> > > > > > > > > >
> > > > > > > > > > It might enable it, but with CLK_IGNORE_UNUSED the clock won't be
> > > > > > > > > > disabled again when a port is closed after usage
> > > > > > > > >
> > > > > > > > > OK, tell me what I'm getting wrong in the following scenario:
> > > > > > > > >
> > > > > > > > > U-boot leaves the console uart clock enabled. All the other ones are disabled.
> > > > > > > > >
> > > > > > > > > Kernel i.MX clk driver registers the uart clocks with flag CLK_IGNORE_UNUSED.
> > > > > > > >
> > > > > > > > I was wrong at that point. I originally thought the kernel will never
> > > > > > > > disable these clocks, but in fact it only leaves them enabled during the
> > > > > > > > clk_disable_unused call.
> > > > > > > >
> > > > > > > > However, when CLK_IGNORE_UNUSED becomes relevant it's too late already.
> > > > > > > > I just chatted with Lucas and he told me what the original problem was
> > > > > > > > that his patch solved.
> > > > > > > >
> > > > > > > > The problem comes when an unrelated device and the earlycon UART have
> > > > > > > > the same parent clocks. The parent clock is enabled, but it's reference
> > > > > > > > count is zero. Now when the unrelated device probes and toggles its
> > > > > > > > clocks then the shared parent clock will be disabled due to the
> > > > > > > > reference count being zero. Next time earlycon prints a character the
> > > > > > > > system hangs because the UART gates are still enabled, but their parent
> > > > > > > > clocks no longer are.
> > > > > > > >
> > > > > > >
> > > > > > > Hmm, that is indeed a problem. That's why I think there should be some
> > > > > > > kind of NOCACHE flag for almost all the types of clocks. For example,
> > > > > > > in this case, it makes sense for the core to check the bit in the register
> > > > > > > and then disable the parent based on that instead of relying on the refcount.
> > > > > > > Anyway, that's something that needs to be added in the CCF.
> > > > > > >
> > > > > > > > Overall I think Lucas' patches are still valid and relevant and with
> > > > > > > > Adams patches we even no longer have to enable all UART clocks, but
> > > > > > > > only the ones which are actually needed.
> > > > > > >
> > > > > > > Yeah, for now, I think we can go with Adam's patches. But longterm, I would
> > > > > > > like to remove the special case of the uart clocks we have right now in all
> > > > > > > the i.MX clock drivers.
> > > > >
> > > > > I looked around at other serial drivers, and I found nothing like this
> > > > > function for enabling all UART clocks.  There are generic functions
> > > > > for registering consoles, earlycon etc, and the serial driver fetches
> > > > > the per and igp clocks from the device tree, so I attempted to simply
> > > > > remove imx_register_uart_clocks().  I booted an i.MX8M Nano from a
> > > > > fully-powered off state, and my serial console came up just fine.
> > > >
> > > > Just because it works, doesn't mean it is safe. To put it simply, the
> > > > risk of some  driver disabling a clock that is parent of the uart clock
> > > > would render the earlycon broken.
> > > >
> > > > >
> > > > > I checked the clk_summary, and the clock parents are set correctly and
> > > > > the respective clock rates appear to be correct (ie, the console is
> > > > > working at the desired baud rate, and Bluetooth is happy)
> > > > >
> > > > > Since I don't fully understand the serial driver and the clock
> > > > > dependencies, I don't want to just simply remove the function without
> > > > > discussing it, because I don't know the ramifications.  However, when
> > > > > testing on the i.MX8M Nano, things look OK.
> > > > > I also tested suspend-resume and the console UART appears to return
> > > > > and the Bluetooth UART set to 4Mbps works just fine too.
> > > > >
> > > > > I'd like to post a V4 which just removes imx_register_uart_clocks and
> > > > > the corresponding calls to it.  I don't know enough about the older
> > > > > 32-bit i.MX SoC's, but I have build-tested it, and I can generate a
> > > > > patch. Are there any objections and/or concerns?
> > > > >
> > > >
> > > > Please don't remove the imx_register_uart_clocks for now. As much as I
> > > > would like it gone, the way the earlycon could end up broken is
> > > > so ugly that it would make it a real pain to debug it later on.
> > >
> > > I won't do a V4, but where do we go from here?  I have a V3 that was
> > > waiting for reviews, but there were some concerns.  We currently
> > > cannot re-parent the UART's on iMX8MM or iMX8MN.  Should I resend V3,
> > > or are there fixes/changes requested to V3?
> > >
> >
> > The v3 is fine. No need to resend.
> >
> > Reviewed-by: Abel Vesa <[e-mail]>
> >
>
> Please add Ahmad's suggestion and resend.

Sorry, I have a backlog. I am hoping to get to it this week.  Thanks
for the reminder.

adam
>
> I'll pick it up then.
>
> Thanks,
> Abel
>
> > > adam
> > > >
> > > > > adam
> > > > > > >
> > > > > >
> > > > > > Is the patch I submitted good enough for someone's acked-by or
> > > > > > reviewed-by, or are there changes I need to implement?
> > > > > >
> > > > > > adam
> > > > > >
> > > > > > > >
> > > > > > > > Sascha
> > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > Pengutronix e.K.                           |                             |
> > > > > > > > Steuerwalder Str. 21                       | https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.pengutronix.de%2F&amp;data=04%7C01%7Cabel.vesa%40nxp.com%7C900fb6f7dfdf49776eb208d8ddadd7b7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637503086023262296%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=ulZnxvilARLN03%2BEr%2BTKmVOPxRTukN%2FDg3oPY8UJ2AU%3D&amp;reserved=0  |
> > > > > > > > 31137 Hildesheim, Germany                  | Phone: [phone]    |
> > > > > > > > Amtsgericht Hildesheim, HRA 2686           | Fax:   [phone]-5555 |

Patch

diff --git a/drivers/clk/imx/clk-imx25.c b/drivers/clk/imx/clk-imx25.c
index a66cabfbf94f..66192fe0a898 100644
--- a/drivers/clk/imx/clk-imx25.c
+++ b/drivers/clk/imx/clk-imx25.c
@@ -73,16 +73,6 @@  enum mx25_clks {
 
 static struct clk *clk[clk_max];
 
-static struct clk ** const uart_clks[] __initconst = {
-	&clk[uart_ipg_per],
-	&clk[uart1_ipg],
-	&clk[uart2_ipg],
-	&clk[uart3_ipg],
-	&clk[uart4_ipg],
-	&clk[uart5_ipg],
-	NULL
-};
-
 static int __init __mx25_clocks_init(void __iomem *ccm_base)
 {
 	BUG_ON(!ccm_base);
@@ -228,7 +218,7 @@  static int __init __mx25_clocks_init(void __iomem *ccm_base)
 	 */
 	clk_set_parent(clk[cko_sel], clk[ipg]);
 
-	imx_register_uart_clocks(uart_clks);
+	imx_register_uart_clocks(6);
 
 	return 0;
 }
diff --git a/drivers/clk/imx/clk-imx27.c b/drivers/clk/imx/clk-imx27.c
index 5585ded8b8c6..56a5fc402b10 100644
--- a/drivers/clk/imx/clk-imx27.c
+++ b/drivers/clk/imx/clk-imx27.c
@@ -49,17 +49,6 @@  static const char *ssi_sel_clks[] = { "spll_gate", "mpll", };
 static struct clk *clk[IMX27_CLK_MAX];
 static struct clk_onecell_data clk_data;
 
-static struct clk ** const uart_clks[] __initconst = {
-	&clk[IMX27_CLK_PER1_GATE],
-	&clk[IMX27_CLK_UART1_IPG_GATE],
-	&clk[IMX27_CLK_UART2_IPG_GATE],
-	&clk[IMX27_CLK_UART3_IPG_GATE],
-	&clk[IMX27_CLK_UART4_IPG_GATE],
-	&clk[IMX27_CLK_UART5_IPG_GATE],
-	&clk[IMX27_CLK_UART6_IPG_GATE],
-	NULL
-};
-
 static void __init _mx27_clocks_init(unsigned long fref)
 {
 	BUG_ON(!ccm);
@@ -176,7 +165,7 @@  static void __init _mx27_clocks_init(unsigned long fref)
 
 	clk_prepare_enable(clk[IMX27_CLK_EMI_AHB_GATE]);
 
-	imx_register_uart_clocks(uart_clks);
+	imx_register_uart_clocks(7);
 
 	imx_print_silicon_rev("i.MX27", mx27_revision());
 }
diff --git a/drivers/clk/imx/clk-imx31.c b/drivers/clk/imx/clk-imx31.c
index 7b13fb57d842..c44e18c6f63f 100644
--- a/drivers/clk/imx/clk-imx31.c
+++ b/drivers/clk/imx/clk-imx31.c
@@ -51,16 +51,6 @@  enum mx31_clks {
 static struct clk *clk[clk_max];
 static struct clk_onecell_data clk_data;
 
-static struct clk ** const uart_clks[] __initconst = {
-	&clk[ipg],
-	&clk[uart1_gate],
-	&clk[uart2_gate],
-	&clk[uart3_gate],
-	&clk[uart4_gate],
-	&clk[uart5_gate],
-	NULL
-};
-
 static void __init _mx31_clocks_init(void __iomem *base, unsigned long fref)
 {
 	clk[dummy] = imx_clk_fixed("dummy", 0);
diff --git a/drivers/clk/imx/clk-imx35.c b/drivers/clk/imx/clk-imx35.c
index c1df03665c09..0fe5ac210156 100644
--- a/drivers/clk/imx/clk-imx35.c
+++ b/drivers/clk/imx/clk-imx35.c
@@ -82,14 +82,6 @@  enum mx35_clks {
 
 static struct clk *clk[clk_max];
 
-static struct clk ** const uart_clks[] __initconst = {
-	&clk[ipg],
-	&clk[uart1_gate],
-	&clk[uart2_gate],
-	&clk[uart3_gate],
-	NULL
-};
-
 static void __init _mx35_clocks_init(void)
 {
 	void __iomem *base;
@@ -243,7 +235,7 @@  static void __init _mx35_clocks_init(void)
 	 */
 	clk_prepare_enable(clk[scc_gate]);
 
-	imx_register_uart_clocks(uart_clks);
+	imx_register_uart_clocks(4);
 
 	imx_print_silicon_rev("i.MX35", mx35_revision());
 }
diff --git a/drivers/clk/imx/clk-imx5.c b/drivers/clk/imx/clk-imx5.c
index 01e079b81026..e4493846454d 100644
--- a/drivers/clk/imx/clk-imx5.c
+++ b/drivers/clk/imx/clk-imx5.c
@@ -128,30 +128,6 @@  static const char *ieee1588_sels[] = { "pll3_sw", "pll4_sw", "dummy" /* usbphy2_
 static struct clk *clk[IMX5_CLK_END];
 static struct clk_onecell_data clk_data;
 
-static struct clk ** const uart_clks_mx51[] __initconst = {
-	&clk[IMX5_CLK_UART1_IPG_GATE],
-	&clk[IMX5_CLK_UART1_PER_GATE],
-	&clk[IMX5_CLK_UART2_IPG_GATE],
-	&clk[IMX5_CLK_UART2_PER_GATE],
-	&clk[IMX5_CLK_UART3_IPG_GATE],
-	&clk[IMX5_CLK_UART3_PER_GATE],
-	NULL
-};
-
-static struct clk ** const uart_clks_mx50_mx53[] __initconst = {
-	&clk[IMX5_CLK_UART1_IPG_GATE],
-	&clk[IMX5_CLK_UART1_PER_GATE],
-	&clk[IMX5_CLK_UART2_IPG_GATE],
-	&clk[IMX5_CLK_UART2_PER_GATE],
-	&clk[IMX5_CLK_UART3_IPG_GATE],
-	&clk[IMX5_CLK_UART3_PER_GATE],
-	&clk[IMX5_CLK_UART4_IPG_GATE],
-	&clk[IMX5_CLK_UART4_PER_GATE],
-	&clk[IMX5_CLK_UART5_IPG_GATE],
-	&clk[IMX5_CLK_UART5_PER_GATE],
-	NULL
-};
-
 static void __init mx5_clocks_common_init(void __iomem *ccm_base)
 {
 	clk[IMX5_CLK_DUMMY]		= imx_clk_fixed("dummy", 0);
@@ -382,7 +358,7 @@  static void __init mx50_clocks_init(struct device_node *np)
 	r = clk_round_rate(clk[IMX5_CLK_USBOH3_PER_GATE], 54000000);
 	clk_set_rate(clk[IMX5_CLK_USBOH3_PER_GATE], r);
 
-	imx_register_uart_clocks(uart_clks_mx50_mx53);
+	imx_register_uart_clocks(5);
 }
 CLK_OF_DECLARE(imx50_ccm, "fsl,imx50-ccm", mx50_clocks_init);
 
@@ -488,7 +464,7 @@  static void __init mx51_clocks_init(struct device_node *np)
 	val |= 1 << 23;
 	writel(val, MXC_CCM_CLPCR);
 
-	imx_register_uart_clocks(uart_clks_mx51);
+	imx_register_uart_clocks(3);
 }
 CLK_OF_DECLARE(imx51_ccm, "fsl,imx51-ccm", mx51_clocks_init);
 
@@ -633,6 +609,6 @@  static void __init mx53_clocks_init(struct device_node *np)
 	r = clk_round_rate(clk[IMX5_CLK_USBOH3_PER_GATE], 54000000);
 	clk_set_rate(clk[IMX5_CLK_USBOH3_PER_GATE], r);
 
-	imx_register_uart_clocks(uart_clks_mx50_mx53);
+	imx_register_uart_clocks(5);
 }
 CLK_OF_DECLARE(imx53_ccm, "fsl,imx53-ccm", mx53_clocks_init);
diff --git a/drivers/clk/imx/clk-imx6q.c b/drivers/clk/imx/clk-imx6q.c
index b2ff187cedab..f444bbe8244c 100644
--- a/drivers/clk/imx/clk-imx6q.c
+++ b/drivers/clk/imx/clk-imx6q.c
@@ -140,13 +140,6 @@  static inline int clk_on_imx6dl(void)
 	return of_machine_is_compatible("fsl,imx6dl");
 }
 
-static const int uart_clk_ids[] __initconst = {
-	IMX6QDL_CLK_UART_IPG,
-	IMX6QDL_CLK_UART_SERIAL,
-};
-
-static struct clk **uart_clks[ARRAY_SIZE(uart_clk_ids) + 1] __initdata;
-
 static int ldb_di_sel_by_clock_id(int clock_id)
 {
 	switch (clock_id) {
@@ -440,7 +433,6 @@  static void __init imx6q_clocks_init(struct device_node *ccm_node)
 	struct device_node *np;
 	void __iomem *anatop_base, *base;
 	int ret;
-	int i;
 
 	clk_hw_data = kzalloc(struct_size(clk_hw_data, hws,
 					  IMX6QDL_CLK_END), GFP_KERNEL);
@@ -982,12 +974,6 @@  static void __init imx6q_clocks_init(struct device_node *ccm_node)
 			       hws[IMX6QDL_CLK_PLL3_USB_OTG]->clk);
 	}
 
-	for (i = 0; i < ARRAY_SIZE(uart_clk_ids); i++) {
-		int index = uart_clk_ids[i];
-
-		uart_clks[i] = &hws[index]->clk;
-	}
-
-	imx_register_uart_clocks(uart_clks);
+	imx_register_uart_clocks(1);
 }
 CLK_OF_DECLARE(imx6q, "fsl,imx6q-ccm", imx6q_clocks_init);
diff --git a/drivers/clk/imx/clk-imx6sl.c b/drivers/clk/imx/clk-imx6sl.c
index 2f9361946a0e..d997b5b07818 100644
--- a/drivers/clk/imx/clk-imx6sl.c
+++ b/drivers/clk/imx/clk-imx6sl.c
@@ -178,19 +178,11 @@  void imx6sl_set_wait_clk(bool enter)
 		imx6sl_enable_pll_arm(false);
 }
 
-static const int uart_clk_ids[] __initconst = {
-	IMX6SL_CLK_UART,
-	IMX6SL_CLK_UART_SERIAL,
-};
-
-static struct clk **uart_clks[ARRAY_SIZE(uart_clk_ids) + 1] __initdata;
-
 static void __init imx6sl_clocks_init(struct device_node *ccm_node)
 {
 	struct device_node *np;
 	void __iomem *base;
 	int ret;
-	int i;
 
 	clk_hw_data = kzalloc(struct_size(clk_hw_data, hws,
 					  IMX6SL_CLK_END), GFP_KERNEL);
@@ -447,12 +439,6 @@  static void __init imx6sl_clocks_init(struct device_node *ccm_node)
 	clk_set_parent(hws[IMX6SL_CLK_LCDIF_AXI_SEL]->clk,
 		       hws[IMX6SL_CLK_PLL2_PFD2]->clk);
 
-	for (i = 0; i < ARRAY_SIZE(uart_clk_ids); i++) {
-		int index = uart_clk_ids[i];
-
-		uart_clks[i] = &hws[index]->clk;
-	}
-
-	imx_register_uart_clocks(uart_clks);
+	imx_register_uart_clocks(2);
 }
 CLK_OF_DECLARE(imx6sl, "fsl,imx6sl-ccm", imx6sl_clocks_init);
diff --git a/drivers/clk/imx/clk-imx6sll.c b/drivers/clk/imx/clk-imx6sll.c
index 8e8288bda4d0..31d777f30039 100644
--- a/drivers/clk/imx/clk-imx6sll.c
+++ b/drivers/clk/imx/clk-imx6sll.c
@@ -76,26 +76,10 @@  static u32 share_count_ssi1;
 static u32 share_count_ssi2;
 static u32 share_count_ssi3;
 
-static const int uart_clk_ids[] __initconst = {
-	IMX6SLL_CLK_UART1_IPG,
-	IMX6SLL_CLK_UART1_SERIAL,
-	IMX6SLL_CLK_UART2_IPG,
-	IMX6SLL_CLK_UART2_SERIAL,
-	IMX6SLL_CLK_UART3_IPG,
-	IMX6SLL_CLK_UART3_SERIAL,
-	IMX6SLL_CLK_UART4_IPG,
-	IMX6SLL_CLK_UART4_SERIAL,
-	IMX6SLL_CLK_UART5_IPG,
-	IMX6SLL_CLK_UART5_SERIAL,
-};
-
-static struct clk **uart_clks[ARRAY_SIZE(uart_clk_ids) + 1] __initdata;
-
 static void __init imx6sll_clocks_init(struct device_node *ccm_node)
 {
 	struct device_node *np;
 	void __iomem *base;
-	int i;
 
 	clk_hw_data = kzalloc(struct_size(clk_hw_data, hws,
 					  IMX6SLL_CLK_END), GFP_KERNEL);
@@ -356,13 +340,7 @@  static void __init imx6sll_clocks_init(struct device_node *ccm_node)
 
 	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, clk_hw_data);
 
-	for (i = 0; i < ARRAY_SIZE(uart_clk_ids); i++) {
-		int index = uart_clk_ids[i];
-
-		uart_clks[i] = &hws[index]->clk;
-	}
-
-	imx_register_uart_clocks(uart_clks);
+	imx_register_uart_clocks(5);
 
 	/* Lower the AHB clock rate before changing the clock source. */
 	clk_set_rate(hws[IMX6SLL_CLK_AHB]->clk, 99000000);
diff --git a/drivers/clk/imx/clk-imx6sx.c b/drivers/clk/imx/clk-imx6sx.c
index 20dcce526d07..fc1bd23d4583 100644
--- a/drivers/clk/imx/clk-imx6sx.c
+++ b/drivers/clk/imx/clk-imx6sx.c
@@ -117,18 +117,10 @@  static u32 share_count_ssi3;
 static u32 share_count_sai1;
 static u32 share_count_sai2;
 
-static const int uart_clk_ids[] __initconst = {
-	IMX6SX_CLK_UART_IPG,
-	IMX6SX_CLK_UART_SERIAL,
-};
-
-static struct clk **uart_clks[ARRAY_SIZE(uart_clk_ids) + 1] __initdata;
-
 static void __init imx6sx_clocks_init(struct device_node *ccm_node)
 {
 	struct device_node *np;
 	void __iomem *base;
-	int i;
 
 	clk_hw_data = kzalloc(struct_size(clk_hw_data, hws,
 					  IMX6SX_CLK_CLK_END), GFP_KERNEL);
@@ -556,12 +548,6 @@  static void __init imx6sx_clocks_init(struct device_node *ccm_node)
 	clk_set_parent(hws[IMX6SX_CLK_QSPI1_SEL]->clk, hws[IMX6SX_CLK_PLL2_BUS]->clk);
 	clk_set_parent(hws[IMX6SX_CLK_QSPI2_SEL]->clk, hws[IMX6SX_CLK_PLL2_BUS]->clk);
 
-	for (i = 0; i < ARRAY_SIZE(uart_clk_ids); i++) {
-		int index = uart_clk_ids[i];
-
-		uart_clks[i] = &hws[index]->clk;
-	}
-
-	imx_register_uart_clocks(uart_clks);
+	imx_register_uart_clocks(2);
 }
 CLK_OF_DECLARE(imx6sx, "fsl,imx6sx-ccm", imx6sx_clocks_init);
diff --git a/drivers/clk/imx/clk-imx7d.c b/drivers/clk/imx/clk-imx7d.c
index 22d24a6a05e7..c4e0f1c07192 100644
--- a/drivers/clk/imx/clk-imx7d.c
+++ b/drivers/clk/imx/clk-imx7d.c
@@ -377,23 +377,10 @@  static const char *pll_video_bypass_sel[] = { "pll_video_main", "pll_video_main_
 static struct clk_hw **hws;
 static struct clk_hw_onecell_data *clk_hw_data;
 
-static const int uart_clk_ids[] __initconst = {
-	IMX7D_UART1_ROOT_CLK,
-	IMX7D_UART2_ROOT_CLK,
-	IMX7D_UART3_ROOT_CLK,
-	IMX7D_UART4_ROOT_CLK,
-	IMX7D_UART5_ROOT_CLK,
-	IMX7D_UART6_ROOT_CLK,
-	IMX7D_UART7_ROOT_CLK,
-};
-
-static struct clk **uart_clks[ARRAY_SIZE(uart_clk_ids) + 1] __initdata;
-
 static void __init imx7d_clocks_init(struct device_node *ccm_node)
 {
 	struct device_node *np;
 	void __iomem *base;
-	int i;
 
 	clk_hw_data = kzalloc(struct_size(clk_hw_data, hws,
 					  IMX7D_CLK_END), GFP_KERNEL);
@@ -897,14 +884,7 @@  static void __init imx7d_clocks_init(struct device_node *ccm_node)
 	hws[IMX7D_USB1_MAIN_480M_CLK] = imx_clk_hw_fixed_factor("pll_usb1_main_clk", "osc", 20, 1);
 	hws[IMX7D_USB_MAIN_480M_CLK] = imx_clk_hw_fixed_factor("pll_usb_main_clk", "osc", 20, 1);
 
-	for (i = 0; i < ARRAY_SIZE(uart_clk_ids); i++) {
-		int index = uart_clk_ids[i];
-
-		uart_clks[i] = &hws[index]->clk;
-	}
-
-
-	imx_register_uart_clocks(uart_clks);
+	imx_register_uart_clocks(7);
 
 }
 CLK_OF_DECLARE(imx7d, "fsl,imx7d-ccm", imx7d_clocks_init);
diff --git a/drivers/clk/imx/clk-imx7ulp.c b/drivers/clk/imx/clk-imx7ulp.c
index 634c0b6636b0..779e09105da7 100644
--- a/drivers/clk/imx/clk-imx7ulp.c
+++ b/drivers/clk/imx/clk-imx7ulp.c
@@ -43,19 +43,6 @@  static const struct clk_div_table ulp_div_table[] = {
 	{ /* sentinel */ },
 };
 
-static const int pcc2_uart_clk_ids[] __initconst = {
-	IMX7ULP_CLK_LPUART4,
-	IMX7ULP_CLK_LPUART5,
-};
-
-static const int pcc3_uart_clk_ids[] __initconst = {
-	IMX7ULP_CLK_LPUART6,
-	IMX7ULP_CLK_LPUART7,
-};
-
-static struct clk **pcc2_uart_clks[ARRAY_SIZE(pcc2_uart_clk_ids) + 1] __initdata;
-static struct clk **pcc3_uart_clks[ARRAY_SIZE(pcc3_uart_clk_ids) + 1] __initdata;
-
 static void __init imx7ulp_clk_scg1_init(struct device_node *np)
 {
 	struct clk_hw_onecell_data *clk_data;
@@ -150,7 +137,6 @@  static void __init imx7ulp_clk_pcc2_init(struct device_node *np)
 	struct clk_hw_onecell_data *clk_data;
 	struct clk_hw **hws;
 	void __iomem *base;
-	int i;
 
 	clk_data = kzalloc(struct_size(clk_data, hws, IMX7ULP_CLK_PCC2_END),
 			   GFP_KERNEL);
@@ -190,13 +176,7 @@  static void __init imx7ulp_clk_pcc2_init(struct device_node *np)
 
 	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, clk_data);
 
-	for (i = 0; i < ARRAY_SIZE(pcc2_uart_clk_ids); i++) {
-		int index = pcc2_uart_clk_ids[i];
-
-		pcc2_uart_clks[i] = &hws[index]->clk;
-	}
-
-	imx_register_uart_clocks(pcc2_uart_clks);
+	imx_register_uart_clocks(2);
 }
 CLK_OF_DECLARE(imx7ulp_clk_pcc2, "fsl,imx7ulp-pcc2", imx7ulp_clk_pcc2_init);
 
@@ -205,7 +185,6 @@  static void __init imx7ulp_clk_pcc3_init(struct device_node *np)
 	struct clk_hw_onecell_data *clk_data;
 	struct clk_hw **hws;
 	void __iomem *base;
-	int i;
 
 	clk_data = kzalloc(struct_size(clk_data, hws, IMX7ULP_CLK_PCC3_END),
 			   GFP_KERNEL);
@@ -244,13 +223,7 @@  static void __init imx7ulp_clk_pcc3_init(struct device_node *np)
 
 	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, clk_data);
 
-	for (i = 0; i < ARRAY_SIZE(pcc3_uart_clk_ids); i++) {
-		int index = pcc3_uart_clk_ids[i];
-
-		pcc3_uart_clks[i] = &hws[index]->clk;
-	}
-
-	imx_register_uart_clocks(pcc3_uart_clks);
+	imx_register_uart_clocks(7);
 }
 CLK_OF_DECLARE(imx7ulp_clk_pcc3, "fsl,imx7ulp-pcc3", imx7ulp_clk_pcc3_init);
 
diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
index 7c905861af5d..209775140fe8 100644
--- a/drivers/clk/imx/clk-imx8mm.c
+++ b/drivers/clk/imx/clk-imx8mm.c
@@ -291,20 +291,12 @@  static const char *imx8mm_clko2_sels[] = {"osc_24m", "sys_pll2_200m", "sys_pll1_
 static struct clk_hw_onecell_data *clk_hw_data;
 static struct clk_hw **hws;
 
-static const int uart_clk_ids[] = {
-	IMX8MM_CLK_UART1_ROOT,
-	IMX8MM_CLK_UART2_ROOT,
-	IMX8MM_CLK_UART3_ROOT,
-	IMX8MM_CLK_UART4_ROOT,
-};
-static struct clk **uart_hws[ARRAY_SIZE(uart_clk_ids) + 1];
-
 static int imx8mm_clocks_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	void __iomem *base;
-	int ret, i;
+	int ret;
 
 	clk_hw_data = kzalloc(struct_size(clk_hw_data, hws,
 					  IMX8MM_CLK_END), GFP_KERNEL);
@@ -622,13 +614,7 @@  static int imx8mm_clocks_probe(struct platform_device *pdev)
 		goto unregister_hws;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(uart_clk_ids); i++) {
-		int index = uart_clk_ids[i];
-
-		uart_hws[i] = &hws[index]->clk;
-	}
-
-	imx_register_uart_clocks(uart_hws);
+	imx_register_uart_clocks(4);
 
 	return 0;
 
diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
index 3c21db942d5b..43098186abeb 100644
--- a/drivers/clk/imx/clk-imx8mn.c
+++ b/drivers/clk/imx/clk-imx8mn.c
@@ -284,20 +284,12 @@  static const char * const imx8mn_clko2_sels[] = {"osc_24m", "sys_pll2_200m", "sy
 static struct clk_hw_onecell_data *clk_hw_data;
 static struct clk_hw **hws;
 
-static const int uart_clk_ids[] = {
-	IMX8MN_CLK_UART1_ROOT,
-	IMX8MN_CLK_UART2_ROOT,
-	IMX8MN_CLK_UART3_ROOT,
-	IMX8MN_CLK_UART4_ROOT,
-};
-static struct clk **uart_hws[ARRAY_SIZE(uart_clk_ids) + 1];
-
 static int imx8mn_clocks_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	void __iomem *base;
-	int ret, i;
+	int ret;
 
 	clk_hw_data = kzalloc(struct_size(clk_hw_data, hws,
 					  IMX8MN_CLK_END), GFP_KERNEL);
@@ -573,13 +565,7 @@  static int imx8mn_clocks_probe(struct platform_device *pdev)
 		goto unregister_hws;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(uart_clk_ids); i++) {
-		int index = uart_clk_ids[i];
-
-		uart_hws[i] = &hws[index]->clk;
-	}
-
-	imx_register_uart_clocks(uart_hws);
+	imx_register_uart_clocks(4);
 
 	return 0;
 
diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
index 2f4e1d674e1c..3e6557e7d559 100644
--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -414,20 +414,11 @@  static const char * const imx8mp_dram_core_sels[] = {"dram_pll_out", "dram_alt_r
 static struct clk_hw **hws;
 static struct clk_hw_onecell_data *clk_hw_data;
 
-static const int uart_clk_ids[] = {
-	IMX8MP_CLK_UART1_ROOT,
-	IMX8MP_CLK_UART2_ROOT,
-	IMX8MP_CLK_UART3_ROOT,
-	IMX8MP_CLK_UART4_ROOT,
-};
-static struct clk **uart_clks[ARRAY_SIZE(uart_clk_ids) + 1];
-
 static int imx8mp_clocks_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np;
 	void __iomem *anatop_base, *ccm_base;
-	int i;
 
 	np = of_find_compatible_node(NULL, NULL, "fsl,imx8mp-anatop");
 	anatop_base = of_iomap(np, 0);
@@ -737,13 +728,7 @@  static int imx8mp_clocks_probe(struct platform_device *pdev)
 
 	of_clk_add_hw_provider(np, of_clk_hw_onecell_get, clk_hw_data);
 
-	for (i = 0; i < ARRAY_SIZE(uart_clk_ids); i++) {
-		int index = uart_clk_ids[i];
-
-		uart_clks[i] = &hws[index]->clk;
-	}
-
-	imx_register_uart_clocks(uart_clks);
+	imx_register_uart_clocks(4);
 
 	return 0;
 }
diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
index 779ea69e639c..3d539e9f9c92 100644
--- a/drivers/clk/imx/clk-imx8mq.c
+++ b/drivers/clk/imx/clk-imx8mq.c
@@ -273,20 +273,12 @@  static const char * const imx8mq_clko2_sels[] = {"osc_25m", "sys2_pll_200m", "sy
 static struct clk_hw_onecell_data *clk_hw_data;
 static struct clk_hw **hws;
 
-static const int uart_clk_ids[] = {
-	IMX8MQ_CLK_UART1_ROOT,
-	IMX8MQ_CLK_UART2_ROOT,
-	IMX8MQ_CLK_UART3_ROOT,
-	IMX8MQ_CLK_UART4_ROOT,
-};
-static struct clk **uart_hws[ARRAY_SIZE(uart_clk_ids) + 1];
-
 static int imx8mq_clocks_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	void __iomem *base;
-	int err, i;
+	int err;
 
 	clk_hw_data = kzalloc(struct_size(clk_hw_data, hws,
 					  IMX8MQ_CLK_END), GFP_KERNEL);
@@ -607,13 +599,7 @@  static int imx8mq_clocks_probe(struct platform_device *pdev)
 		goto unregister_hws;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(uart_clk_ids); i++) {
-		int index = uart_clk_ids[i];
-
-		uart_hws[i] = &hws[index]->clk;
-	}
-
-	imx_register_uart_clocks(uart_hws);
+	imx_register_uart_clocks(4);
 
 	return 0;
 
diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
index 47882c51cb85..158fe302a8b7 100644
--- a/drivers/clk/imx/clk.c
+++ b/drivers/clk/imx/clk.c
@@ -147,8 +147,10 @@  void imx_cscmr1_fixup(u32 *val)
 }
 
 #ifndef MODULE
-static int imx_keep_uart_clocks;
-static struct clk ** const *imx_uart_clocks;
+
+static bool imx_keep_uart_clocks;
+static int imx_enabled_uart_clocks;
+static struct clk **imx_uart_clocks;
 
 static int __init imx_keep_uart_clocks_param(char *str)
 {
@@ -161,24 +163,43 @@  __setup_param("earlycon", imx_keep_uart_earlycon,
 __setup_param("earlyprintk", imx_keep_uart_earlyprintk,
 	      imx_keep_uart_clocks_param, 0);
 
-void imx_register_uart_clocks(struct clk ** const clks[])
+void imx_register_uart_clocks(unsigned int clk_count)
 {
+#ifdef CONFIG_OF
 	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]);
+		imx_uart_clocks = kcalloc(clk_count, sizeof(struct clk *), GFP_KERNEL);
+		imx_enabled_uart_clocks = 0;
+
+		for (i = 0; i < clk_count; i++) {
+			imx_uart_clocks[imx_enabled_uart_clocks] = of_clk_get(of_stdout, i);
+
+			/* Stop if there are no more of_stdout references */
+			if (IS_ERR(imx_uart_clocks[imx_enabled_uart_clocks]))
+				return;
+
+			/* Only enable the clock if it's not NULL */
+			if (imx_uart_clocks[imx_enabled_uart_clocks])
+				clk_prepare_enable(imx_uart_clocks[imx_enabled_uart_clocks++]);
+		}
 	}
+#else
+	/* i.MX boards use device trees now.  For build tests without CONFIG_OF, do nothing */
+	imx_enabled_uart_clocks = 0;
+#endif
 }
 
 static int __init imx_clk_disable_uart(void)
 {
-	if (imx_keep_uart_clocks && imx_uart_clocks) {
+	if (imx_keep_uart_clocks && imx_enabled_uart_clocks) {
 		int i;
 
-		for (i = 0; imx_uart_clocks[i]; i++)
-			clk_disable_unprepare(*imx_uart_clocks[i]);
+		for (i = 0; i < imx_enabled_uart_clocks; i++) {
+			clk_disable_unprepare(imx_uart_clocks[i]);
+			clk_put(imx_uart_clocks[i]);
+		};
+		kfree(imx_uart_clocks);
 	}
 
 	return 0;
diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
index 4f04c8287286..7571603bee23 100644
--- a/drivers/clk/imx/clk.h
+++ b/drivers/clk/imx/clk.h
@@ -11,9 +11,9 @@  extern spinlock_t imx_ccm_lock;
 void imx_check_clocks(struct clk *clks[], unsigned int count);
 void imx_check_clk_hws(struct clk_hw *clks[], unsigned int count);
 #ifndef MODULE
-void imx_register_uart_clocks(struct clk ** const clks[]);
+void imx_register_uart_clocks(unsigned int clk_count);
 #else
-static inline void imx_register_uart_clocks(struct clk ** const clks[])
+static inline void imx_register_uart_clocks(unsigned int clk_count)
 {
 }
 #endif