[v6,01/20] clk: bump stdout clock usage for earlycon

Commit Message

André Draszik Aug. 8, 2024, 2:42 p.m. UTC

On some platforms, earlycon depends on the bootloader setup stdout
clocks being retained. In some cases stdout UART clocks (or their
parents) can get disabled during loading of other drivers (e.g. i2c)
causing earlycon to stop to work sometime into the boot, halting the
whole system.

Since there are at least two platforms where that is the case, i.MX and
the Exynos-derivative gs101, this patch adds some logic to the clk core
to detect these clocks if earlycon is enabled, to bump their usage
count as part of of_clk_add_hw_provider() and of_clk_add_provider(),
and to release them again at the end of init.

This way code duplication in affected platforms can be avoided.

The general idea is based on similar code in the i.MX clock driver, but
this here is a bit more generic as in general (e.g. on gs101) clocks
can come from various different clock units (driver instances) and
therefore it can be necessary to run this code multiple times until all
required stdout clocks have probed.

Signed-off-by: André Draszik <[e-mail]>

---
v6:
* drop a stray #include from drivers/clk/samsung/clk-gs101.c
---
 drivers/clk/clk.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 129 insertions(+)

Comments

Stephen Boyd Aug. 8, 2024, 8:14 p.m. UTC | #1

Quoting André Draszik (2024-08-08 07:42:42)
> On some platforms, earlycon depends on the bootloader setup stdout
> clocks being retained. In some cases stdout UART clocks (or their
> parents) can get disabled during loading of other drivers (e.g. i2c)
> causing earlycon to stop to work sometime into the boot, halting the
> whole system.
> 
> Since there are at least two platforms where that is the case, i.MX and
> the Exynos-derivative gs101, this patch adds some logic to the clk core
> to detect these clocks if earlycon is enabled, to bump their usage
> count as part of of_clk_add_hw_provider() and of_clk_add_provider(),
> and to release them again at the end of init.
> 
> This way code duplication in affected platforms can be avoided.
> 
> The general idea is based on similar code in the i.MX clock driver, but
> this here is a bit more generic as in general (e.g. on gs101) clocks
> can come from various different clock units (driver instances) and
> therefore it can be necessary to run this code multiple times until all
> required stdout clocks have probed.
> 
> Signed-off-by: André Draszik <[e-mail]>
> 
> ---

Thanks for doing this!

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 7264cf6165ce..03c5d80e833c 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -4923,6 +4923,131 @@ static void clk_core_reparent_orphans(void)
>         clk_prepare_unlock();
>  }
>  
> +/**
> + * struct of_clk_stdout_clks - holds data that is required for handling extra
> + * references to stdout clocks during early boot.
> + *
> + * On some platforms, earlycon depends on the bootloader setup stdout clocks
> + * being retained. In some cases stdout UART clocks (or their parents) can get
> + * disabled during loading of other drivers (e.g. i2c) causing earlycon to stop
> + * to work sometime into the boot, halting the system.
> + *
> + * Having logic to detect these clocks if earlycon is enabled helps with those
> + * cases by bumping their usage count during init. The extra usage count is
> + * later dropped at the end of init.
> + *
> + * @bump_refs: whether or not to add the extra stdout clock references
> + * @lock: mutex protecting access
> + * @have_all: whether or not we have acquired all clocks, to handle cases of
> + *            clocks coming from different drivers / instances
> + * @clks: clocks associated with stdout
> + * @n_clks: number of clocks associated with stdout
> + */
> +static struct of_clk_stdout_clks {
> +       bool bump_refs;
> +
> +       struct mutex lock;
> +       bool have_all;
> +       struct clk **clks;
> +       size_t n_clks;
> +} of_clk_stdout_clks = {

This can be initdata?

> +       .lock = __MUTEX_INITIALIZER(of_clk_stdout_clks.lock),
> +};
> +
> +static int __init of_clk_bump_stdout_clocks_param(char *str)
> +{
> +       of_clk_stdout_clks.bump_refs = true;
> +       return 0;
> +}
> +__setup("earlycon", of_clk_bump_stdout_clocks_param);
> +__setup_param("earlyprintk", of_clk_keep_stdout_clocks_earlyprintk,
> +             of_clk_bump_stdout_clocks_param, 0);
> +
> +static void of_clk_bump_stdout_clocks(void)

This can be __init?

> +{
> +       size_t n_clks;
> +
> +       /*
> +        * We only need to run this code if required to do so and only ever
> +        * before late initcalls have run. Otherwise it'd be impossible to know
> +        * when to drop the extra clock references again.
> +        *
> +        * This generally means that this only works if on affected platforms
> +        * the clock drivers have been built-in (as opposed to being modules).
> +        */
> +       if (!of_clk_stdout_clks.bump_refs)
> +               return;
> +
> +       n_clks = of_clk_get_parent_count(of_stdout);
> +       if (!n_clks || !of_stdout)
> +               return;
> +
> +       mutex_lock(&of_clk_stdout_clks.lock);
> +
> +       /*
> +        * We only need to keep trying if we have not succeeded previously,
> +        * i.e. if not all required clocks were ready during previous attempts.
> +        */
> +       if (of_clk_stdout_clks.have_all)
> +               goto out_unlock;
> +
> +       if (!of_clk_stdout_clks.clks) {
> +               of_clk_stdout_clks.n_clks = n_clks;
> +
> +               of_clk_stdout_clks.clks = kcalloc(of_clk_stdout_clks.n_clks,
> +                                             sizeof(*of_clk_stdout_clks.clks),
> +                                             GFP_KERNEL);
> +               if (!of_clk_stdout_clks.clks)
> +                       goto out_unlock;
> +       }
> +
> +       /* assume that this time we'll be able to grab all required clocks */
> +       of_clk_stdout_clks.have_all = true;
> +       for (size_t i = 0; i < n_clks; ++i) {
> +               struct clk *clk;
> +
> +               /* we might have grabbed this clock in a previous attempt */
> +               if (of_clk_stdout_clks.clks[i])
> +                       continue;
> +
> +               clk = of_clk_get(of_stdout, i);
> +               if (IS_ERR(clk)) {
> +                       /* retry next time if clock has not probed yet */
> +                       of_clk_stdout_clks.have_all = false;
> +                       continue;
> +               }
> +
> +               if (clk_prepare_enable(clk)) {
> +                       clk_put(clk);
> +                       continue;
> +               }
> +               of_clk_stdout_clks.clks[i] = clk;
> +       }
> +
> +out_unlock:
> +       mutex_unlock(&of_clk_stdout_clks.lock);
> +}
> +
> +static int __init of_clk_drop_stdout_clocks(void)
> +{
> +       for (size_t i = 0; i < of_clk_stdout_clks.n_clks; ++i) {
> +               clk_disable_unprepare(of_clk_stdout_clks.clks[i]);
> +               clk_put(of_clk_stdout_clks.clks[i]);
> +       }
> +
> +       kfree(of_clk_stdout_clks.clks);
> +
> +       /*
> +        * Do not try to acquire stdout clocks after late initcalls, e.g.
> +        * during further module loading, as we then wouldn't have a way to
> +        * drop the references (and associated allocations) ever again.
> +        */
> +       of_clk_stdout_clks.bump_refs = false;
> +
> +       return 0;
> +}
> +late_initcall_sync(of_clk_drop_stdout_clocks);
> +
>  /**
>   * struct of_clk_provider - Clock provider registration structure
>   * @link: Entry in global list of clock providers
> @@ -5031,6 +5156,8 @@ int of_clk_add_provider(struct device_node *np,
>  
>         fwnode_dev_initialized(&np->fwnode, true);
>  
> +       of_clk_bump_stdout_clocks();

This can be a wrapper function that isn't marked __init but which calls
the init function with __ref. That lets us free up as much code as
possible. We need to set a bool in of_clk_drop_stdout_clocks() that when
false doesn't call the __init functions that are wrapped though, i.e.
'bump_refs'. Here's the structure:

	static bool bump_stdout_clks __ro_after_init = true;

	static int __init _of_clk_bump_stdout_clks(void)
	{
		...
	}

	static int __ref of_clk_bump_stdout_clks(void)
	{
		if (bump_stdout_clks)
			return _of_clk_bump_stdout_clks();

		return 0;
	}

	static int __init of_clk_drop_stdout_clks(void)
	{
		bump_stdout_clks = false;
		...
	}
	late_initcall_sync(of_clk_drop_stdout_clks);

> +
>         return ret;
>  }
>  EXPORT_SYMBOL_GPL(of_clk_add_provider);

Peng Fan Aug. 9, 2024, 7:16 a.m. UTC | #2

> Subject: [PATCH v6 01/20] clk: bump stdout clock usage for earlycon
> 
> On some platforms, earlycon depends on the bootloader setup stdout
> clocks being retained. In some cases stdout UART clocks (or their
> parents) can get disabled during loading of other drivers (e.g. i2c)
> causing earlycon to stop to work sometime into the boot, halting the
> whole system.
> 
> Since there are at least two platforms where that is the case, i.MX and
> the Exynos-derivative gs101, this patch adds some logic to the clk core
> to detect these clocks if earlycon is enabled, to bump their usage count
> as part of of_clk_add_hw_provider() and of_clk_add_provider(), and to
> release them again at the end of init.
> 
> This way code duplication in affected platforms can be avoided.
> 
> The general idea is based on similar code in the i.MX clock driver, but
> this here is a bit more generic as in general (e.g. on gs101) clocks can
> come from various different clock units (driver instances) and therefore
> it can be necessary to run this code multiple times until all required
> stdout clocks have probed.
> 
> Signed-off-by: André Draszik <[e-mail]>
> 
> ---
> v6:
> * drop a stray #include from drivers/clk/samsung/clk-gs101.c
> ---
>  drivers/clk/clk.c | 129
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 129 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index
> 7264cf6165ce..03c5d80e833c 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -4923,6 +4923,131 @@ static void
> clk_core_reparent_orphans(void)
>  	clk_prepare_unlock();
>  }
> 
> +/**
> + * struct of_clk_stdout_clks - holds data that is required for handling
> +extra
> + * references to stdout clocks during early boot.
> + *
> + * On some platforms, earlycon depends on the bootloader setup
> stdout
> +clocks
> + * being retained. In some cases stdout UART clocks (or their parents)
> +can get
> + * disabled during loading of other drivers (e.g. i2c) causing earlycon
> +to stop
> + * to work sometime into the boot, halting the system.
> + *
> + * Having logic to detect these clocks if earlycon is enabled helps
> +with those
> + * cases by bumping their usage count during init. The extra usage
> +count is
> + * later dropped at the end of init.
> + *
> + * @bump_refs: whether or not to add the extra stdout clock
> references
> + * @lock: mutex protecting access
> + * @have_all: whether or not we have acquired all clocks, to handle
> cases of
> + *            clocks coming from different drivers / instances
> + * @clks: clocks associated with stdout
> + * @n_clks: number of clocks associated with stdout  */ static struct
> +of_clk_stdout_clks {
> +	bool bump_refs;
> +
> +	struct mutex lock;
> +	bool have_all;
> +	struct clk **clks;
> +	size_t n_clks;
> +} of_clk_stdout_clks = {
> +	.lock = __MUTEX_INITIALIZER(of_clk_stdout_clks.lock),
> +};
> +
> +static int __init of_clk_bump_stdout_clocks_param(char *str) {
> +	of_clk_stdout_clks.bump_refs = true;
> +	return 0;
> +}
> +__setup("earlycon", of_clk_bump_stdout_clocks_param);
> +__setup_param("earlyprintk", of_clk_keep_stdout_clocks_earlyprintk,
> +	      of_clk_bump_stdout_clocks_param, 0);
> +
> +static void of_clk_bump_stdout_clocks(void) {
> +	size_t n_clks;
> +
> +	/*
> +	 * We only need to run this code if required to do so and only
> ever
> +	 * before late initcalls have run. Otherwise it'd be impossible to
> know
> +	 * when to drop the extra clock references again.
> +	 *
> +	 * This generally means that this only works if on affected
> platforms
> +	 * the clock drivers have been built-in (as opposed to being
> modules).
> +	 */
> +	if (!of_clk_stdout_clks.bump_refs)
> +		return;
> +
> +	n_clks = of_clk_get_parent_count(of_stdout);
> +	if (!n_clks || !of_stdout)
> +		return;
> +
> +	mutex_lock(&of_clk_stdout_clks.lock);
> +
> +	/*
> +	 * We only need to keep trying if we have not succeeded
> previously,
> +	 * i.e. if not all required clocks were ready during previous
> attempts.
> +	 */
> +	if (of_clk_stdout_clks.have_all)
> +		goto out_unlock;
> +
> +	if (!of_clk_stdout_clks.clks) {
> +		of_clk_stdout_clks.n_clks = n_clks;
> +
> +		of_clk_stdout_clks.clks =
> kcalloc(of_clk_stdout_clks.n_clks,
> +
> sizeof(*of_clk_stdout_clks.clks),
> +					      GFP_KERNEL);
> +		if (!of_clk_stdout_clks.clks)
> +			goto out_unlock;
> +	}
> +
> +	/* assume that this time we'll be able to grab all required
> clocks */
> +	of_clk_stdout_clks.have_all = true;
> +	for (size_t i = 0; i < n_clks; ++i) {
> +		struct clk *clk;
> +
> +		/* we might have grabbed this clock in a previous
> attempt */
> +		if (of_clk_stdout_clks.clks[i])
> +			continue;
> +
> +		clk = of_clk_get(of_stdout, i);
> +		if (IS_ERR(clk)) {
> +			/* retry next time if clock has not probed yet
> */
> +			of_clk_stdout_clks.have_all = false;
> +			continue;
> +		}
> +
> +		if (clk_prepare_enable(clk)) {
> +			clk_put(clk);
> +			continue;
> +		}
> +		of_clk_stdout_clks.clks[i] = clk;
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&of_clk_stdout_clks.lock);
> +}
> +
> +static int __init of_clk_drop_stdout_clocks(void) {
> +	for (size_t i = 0; i < of_clk_stdout_clks.n_clks; ++i) {
> +		clk_disable_unprepare(of_clk_stdout_clks.clks[i]);
> +		clk_put(of_clk_stdout_clks.clks[i]);
> +	}
> +
> +	kfree(of_clk_stdout_clks.clks);
> +
> +	/*
> +	 * Do not try to acquire stdout clocks after late initcalls, e.g.
> +	 * during further module loading, as we then wouldn't have a
> way to
> +	 * drop the references (and associated allocations) ever again.
> +	 */
> +	of_clk_stdout_clks.bump_refs = false;
> +
> +	return 0;
> +}
> +late_initcall_sync(of_clk_drop_stdout_clocks);

If the uart driver is built as module, this might break earlycon.
Before uart driver loaded, clk disabled per my understanding.

> +
>  /**
>   * struct of_clk_provider - Clock provider registration structure
>   * @link: Entry in global list of clock providers @@ -5031,6 +5156,8
> @@ int of_clk_add_provider(struct device_node *np,
> 
>  	fwnode_dev_initialized(&np->fwnode, true);
> 
> +	of_clk_bump_stdout_clocks();
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(of_clk_add_provider);
> @@ -5073,6 +5200,8 @@ int of_clk_add_hw_provider(struct
> device_node *np,
> 
>  	fwnode_dev_initialized(&np->fwnode, true);
> 
> +	of_clk_bump_stdout_clocks();

If clock driver is built as module,  the will make the
clocks will be always enabled, if my understanding is correct.


Regards,
Peng.

> +
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(of_clk_add_hw_provider);
> 
> --
> 2.46.0.rc2.264.g509ed76dc8-goog

André Draszik Aug. 9, 2024, 8:50 a.m. UTC | #3

Hi Stephen,

On Thu, 2024-08-08 at 13:14 -0700, Stephen Boyd wrote:
> > +static struct of_clk_stdout_clks {
> > +       bool bump_refs;
> > +
> > +       struct mutex lock;
> > +       bool have_all;
> > +       struct clk **clks;
> > +       size_t n_clks;
> > +} of_clk_stdout_clks = {
> 
> This can be initdata?

With the __ref wrapper you suggested below that's indeed possible. Without,
it's not, and I wasn't aware of __ref. Thanks for the pointer!

> > +       .lock = __MUTEX_INITIALIZER(of_clk_stdout_clks.lock),
> > +};
> > +
> > +static int __init of_clk_bump_stdout_clocks_param(char *str)
> > +{
> > +       of_clk_stdout_clks.bump_refs = true;
> > +       return 0;
> > +}
> > +__setup("earlycon", of_clk_bump_stdout_clocks_param);
> > +__setup_param("earlyprintk", of_clk_keep_stdout_clocks_earlyprintk,
> > +             of_clk_bump_stdout_clocks_param, 0);
> > +
> > +static void of_clk_bump_stdout_clocks(void)
> 
> This can be __init?

dito.

Cheers,
Andre'

> 
> > +{
> > +       size_t n_clks;
> > +
> > +       /*
> > +        * We only need to run this code if required to do so and only ever
> > +        * before late initcalls have run. Otherwise it'd be impossible to know
> > +        * when to drop the extra clock references again.
> > +        *
> > +        * This generally means that this only works if on affected platforms
> > +        * the clock drivers have been built-in (as opposed to being modules).
> > +        */
> > +       if (!of_clk_stdout_clks.bump_refs)
> > +               return;
> > +
> > +       n_clks = of_clk_get_parent_count(of_stdout);
> > +       if (!n_clks || !of_stdout)
> > +               return;
> > +
> > +       mutex_lock(&of_clk_stdout_clks.lock);
> > +
> > +       /*
> > +        * We only need to keep trying if we have not succeeded previously,
> > +        * i.e. if not all required clocks were ready during previous attempts.
> > +        */
> > +       if (of_clk_stdout_clks.have_all)
> > +               goto out_unlock;
> > +
> > +       if (!of_clk_stdout_clks.clks) {
> > +               of_clk_stdout_clks.n_clks = n_clks;
> > +
> > +               of_clk_stdout_clks.clks = kcalloc(of_clk_stdout_clks.n_clks,
> > +                                             sizeof(*of_clk_stdout_clks.clks),
> > +                                             GFP_KERNEL);
> > +               if (!of_clk_stdout_clks.clks)
> > +                       goto out_unlock;
> > +       }
> > +
> > +       /* assume that this time we'll be able to grab all required clocks */
> > +       of_clk_stdout_clks.have_all = true;
> > +       for (size_t i = 0; i < n_clks; ++i) {
> > +               struct clk *clk;
> > +
> > +               /* we might have grabbed this clock in a previous attempt */
> > +               if (of_clk_stdout_clks.clks[i])
> > +                       continue;
> > +
> > +               clk = of_clk_get(of_stdout, i);
> > +               if (IS_ERR(clk)) {
> > +                       /* retry next time if clock has not probed yet */
> > +                       of_clk_stdout_clks.have_all = false;
> > +                       continue;
> > +               }
> > +
> > +               if (clk_prepare_enable(clk)) {
> > +                       clk_put(clk);
> > +                       continue;
> > +               }
> > +               of_clk_stdout_clks.clks[i] = clk;
> > +       }
> > +
> > +out_unlock:
> > +       mutex_unlock(&of_clk_stdout_clks.lock);
> > +}
> > +
> > +static int __init of_clk_drop_stdout_clocks(void)
> > +{
> > +       for (size_t i = 0; i < of_clk_stdout_clks.n_clks; ++i) {
> > +               clk_disable_unprepare(of_clk_stdout_clks.clks[i]);
> > +               clk_put(of_clk_stdout_clks.clks[i]);
> > +       }
> > +
> > +       kfree(of_clk_stdout_clks.clks);
> > +
> > +       /*
> > +        * Do not try to acquire stdout clocks after late initcalls, e.g.
> > +        * during further module loading, as we then wouldn't have a way to
> > +        * drop the references (and associated allocations) ever again.
> > +        */
> > +       of_clk_stdout_clks.bump_refs = false;
> > +
> > +       return 0;
> > +}
> > +late_initcall_sync(of_clk_drop_stdout_clocks);
> > +
> >  /**
> >   * struct of_clk_provider - Clock provider registration structure
> >   * @link: Entry in global list of clock providers
> > @@ -5031,6 +5156,8 @@ int of_clk_add_provider(struct device_node *np,
> >  
> >         fwnode_dev_initialized(&np->fwnode, true);
> >  
> > +       of_clk_bump_stdout_clocks();
> 
> This can be a wrapper function that isn't marked __init but which calls
> the init function with __ref. That lets us free up as much code as
> possible. We need to set a bool in of_clk_drop_stdout_clocks() that when
> false doesn't call the __init functions that are wrapped though, i.e.
> 'bump_refs'. Here's the structure:
> 
> 	static bool bump_stdout_clks __ro_after_init = true;
> 
> 	static int __init _of_clk_bump_stdout_clks(void)
> 	{
> 		...
> 	}
> 
> 	static int __ref of_clk_bump_stdout_clks(void)
> 	{
> 		if (bump_stdout_clks)
> 			return _of_clk_bump_stdout_clks();
> 
> 		return 0;
> 	}
> 
> 	static int __init of_clk_drop_stdout_clks(void)
> 	{
> 		bump_stdout_clks = false;
> 		...
> 	}
> 	late_initcall_sync(of_clk_drop_stdout_clks);
> 
> > +
> >         return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(of_clk_add_provider);

André Draszik Aug. 9, 2024, 9:02 a.m. UTC | #4

Hi Peng,

On Fri, 2024-08-09 at 07:16 +0000, Peng Fan wrote:
> > +static int __init of_clk_drop_stdout_clocks(void) {
> > +	for (size_t i = 0; i < of_clk_stdout_clks.n_clks; ++i) {
> > +		clk_disable_unprepare(of_clk_stdout_clks.clks[i]);
> > +		clk_put(of_clk_stdout_clks.clks[i]);
> > +	}
> > +
> > +	kfree(of_clk_stdout_clks.clks);
> > +
> > +	/*
> > +	 * Do not try to acquire stdout clocks after late initcalls, e.g.
> > +	 * during further module loading, as we then wouldn't have a
> > way to
> > +	 * drop the references (and associated allocations) ever again.
> > +	 */
> > +	of_clk_stdout_clks.bump_refs = false;
> > +
> > +	return 0;
> > +}
> > +late_initcall_sync(of_clk_drop_stdout_clocks);
> 
> If the uart driver is built as module, this might break earlycon.
> Before uart driver loaded, clk disabled per my understanding.

You're right.

With this in mind, I'm not sure then if a generic solution is possible...

I guess it has to be duplicated into the platforms after all and platforms
can enable this if they opt to disallow uart as module?

Any other suggestions?

> > +
> >  /**
> >   * struct of_clk_provider - Clock provider registration structure
> >   * @link: Entry in global list of clock providers @@ -5031,6 +5156,8
> > @@ int of_clk_add_provider(struct device_node *np,
> > 
> >  	fwnode_dev_initialized(&np->fwnode, true);
> > 
> > +	of_clk_bump_stdout_clocks();
> > +
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(of_clk_add_provider);
> > @@ -5073,6 +5200,8 @@ int of_clk_add_hw_provider(struct
> > device_node *np,
> > 
> >  	fwnode_dev_initialized(&np->fwnode, true);
> > 
> > +	of_clk_bump_stdout_clocks();
> 
> If clock driver is built as module,  the will make the
> clocks will be always enabled, if my understanding is correct.

until late_initcall_sync(), at which point it'll be disabled before the uart
driver has probed, yes :-(

Cheers,
Andre'

André Draszik Aug. 9, 2024, 9:07 a.m. UTC | #5

On Fri, 2024-08-09 at 10:02 +0100, André Draszik wrote:
> Hi Peng,
> 
> On Fri, 2024-08-09 at 07:16 +0000, Peng Fan wrote:
> > > +static int __init of_clk_drop_stdout_clocks(void) {
> > > +	for (size_t i = 0; i < of_clk_stdout_clks.n_clks; ++i) {
> > > +		clk_disable_unprepare(of_clk_stdout_clks.clks[i]);
> > > +		clk_put(of_clk_stdout_clks.clks[i]);
> > > +	}
> > > +
> > > +	kfree(of_clk_stdout_clks.clks);
> > > +
> > > +	/*
> > > +	 * Do not try to acquire stdout clocks after late initcalls, e.g.
> > > +	 * during further module loading, as we then wouldn't have a
> > > way to
> > > +	 * drop the references (and associated allocations) ever again.
> > > +	 */
> > > +	of_clk_stdout_clks.bump_refs = false;
> > > +
> > > +	return 0;
> > > +}
> > > +late_initcall_sync(of_clk_drop_stdout_clocks);
> > 
> > If the uart driver is built as module, this might break earlycon.
> > Before uart driver loaded, clk disabled per my understanding.
> 
> You're right.
> 
> With this in mind, I'm not sure then if a generic solution is possible...
> 
> I guess it has to be duplicated into the platforms after all and platforms
> can enable this if they opt to disallow uart as module?
> 
> Any other suggestions?
> 
> > > +
> > >  /**
> > >   * struct of_clk_provider - Clock provider registration structure
> > >   * @link: Entry in global list of clock providers @@ -5031,6 +5156,8
> > > @@ int of_clk_add_provider(struct device_node *np,
> > > 
> > >  	fwnode_dev_initialized(&np->fwnode, true);
> > > 
> > > +	of_clk_bump_stdout_clocks();
> > > +
> > >  	return ret;
> > >  }
> > >  EXPORT_SYMBOL_GPL(of_clk_add_provider);
> > > @@ -5073,6 +5200,8 @@ int of_clk_add_hw_provider(struct
> > > device_node *np,
> > > 
> > >  	fwnode_dev_initialized(&np->fwnode, true);
> > > 
> > > +	of_clk_bump_stdout_clocks();
> > 
> > If clock driver is built as module,  the will make the
> > clocks will be always enabled, if my understanding is correct.
> 
> until late_initcall_sync(), at which point it'll be disabled before the uart
> driver has probed, yes :-(

Sorry, ignore that. If clock driver is built as module, the code to bump
the clocks is disabled by the time this code runs (due to setting the flag
as part of late_initcall_sync(of_clk_drop_stdout_clocks)), in other words
it will not bump the clocks at all in that case and behaviour is as before.

Did I miss something?

Cheers,
Andre'

Peng Fan Aug. 10, 2024, 9:43 a.m. UTC | #6

> Subject: Re: [PATCH v6 01/20] clk: bump stdout clock usage for
> earlycon
> 
> On Fri, 2024-08-09 at 10:02 +0100, André Draszik wrote:
> > Hi Peng,
> >
> > On Fri, 2024-08-09 at 07:16 +0000, Peng Fan wrote:
> > > > +static int __init of_clk_drop_stdout_clocks(void) {
> > > > +	for (size_t i = 0; i < of_clk_stdout_clks.n_clks; ++i) {
> > > > +		clk_disable_unprepare(of_clk_stdout_clks.clks[i]);
> > > > +		clk_put(of_clk_stdout_clks.clks[i]);
> > > > +	}
> > > > +
> > > > +	kfree(of_clk_stdout_clks.clks);
> > > > +
> > > > +	/*
> > > > +	 * Do not try to acquire stdout clocks after late initcalls, e.g.
> > > > +	 * during further module loading, as we then wouldn't have a
> > > > way to
> > > > +	 * drop the references (and associated allocations) ever again.
> > > > +	 */
> > > > +	of_clk_stdout_clks.bump_refs = false;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +late_initcall_sync(of_clk_drop_stdout_clocks);
> > >
> > > If the uart driver is built as module, this might break earlycon.
> > > Before uart driver loaded, clk disabled per my understanding.
> >
> > You're right.
> >
> > With this in mind, I'm not sure then if a generic solution is possible...
> >
> > I guess it has to be duplicated into the platforms after all and
> > platforms can enable this if they opt to disallow uart as module?
> >
> > Any other suggestions?
> >
> > > > +
> > > >  /**
> > > >   * struct of_clk_provider - Clock provider registration structure
> > > >   * @link: Entry in global list of clock providers @@ -5031,6
> > > > +5156,8 @@ int of_clk_add_provider(struct device_node *np,
> > > >
> > > >  	fwnode_dev_initialized(&np->fwnode, true);
> > > >
> > > > +	of_clk_bump_stdout_clocks();
> > > > +
> > > >  	return ret;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(of_clk_add_provider);
> > > > @@ -5073,6 +5200,8 @@ int of_clk_add_hw_provider(struct
> > > > device_node *np,
> > > >
> > > >  	fwnode_dev_initialized(&np->fwnode, true);
> > > >
> > > > +	of_clk_bump_stdout_clocks();
> > >
> > > If clock driver is built as module,  the will make the clocks will
> > > be always enabled, if my understanding is correct.
> >
> > until late_initcall_sync(), at which point it'll be disabled before
> > the uart driver has probed, yes :-(
> 
> Sorry, ignore that. If clock driver is built as module, the code to bump
> the clocks is disabled by the time this code runs (due to setting the flag
> as part of late_initcall_sync(of_clk_drop_stdout_clocks)), in other
> words it will not bump the clocks at all in that case and behaviour is as
> before.

Just checked again, you are right.
If the clock driver is built at module and "earlycon" is set in bootargs,
of_clk_stdout_clks.bump_refs will be true. late_initcall_sync will
set it back to false.

Regards,
Peng.

> 
> Did I miss something?
> 
> Cheers,
> Andre'

Peng Fan Aug. 10, 2024, 9:48 a.m. UTC | #7

> Subject: Re: [PATCH v6 01/20] clk: bump stdout clock usage for
> earlycon
> 
> Hi Peng,
> 
> On Fri, 2024-08-09 at 07:16 +0000, Peng Fan wrote:
> > > +static int __init of_clk_drop_stdout_clocks(void) {
> > > +	for (size_t i = 0; i < of_clk_stdout_clks.n_clks; ++i) {
> > > +		clk_disable_unprepare(of_clk_stdout_clks.clks[i]);
> > > +		clk_put(of_clk_stdout_clks.clks[i]);
> > > +	}
> > > +
> > > +	kfree(of_clk_stdout_clks.clks);
> > > +
> > > +	/*
> > > +	 * Do not try to acquire stdout clocks after late initcalls, e.g.
> > > +	 * during further module loading, as we then wouldn't have a
> > > way to
> > > +	 * drop the references (and associated allocations) ever again.
> > > +	 */
> > > +	of_clk_stdout_clks.bump_refs = false;
> > > +
> > > +	return 0;
> > > +}
> > > +late_initcall_sync(of_clk_drop_stdout_clocks);
> >
> > If the uart driver is built as module, this might break earlycon.
> > Before uart driver loaded, clk disabled per my understanding.
> 
> You're right.
> 
> With this in mind, I'm not sure then if a generic solution is possible...
> 
> I guess it has to be duplicated into the platforms after all and platforms
> can enable this if they opt to disallow uart as module?
> 
> Any other suggestions?

I not have good idea here. Put of_clk_drop_stdout_clocks after
bootconsole disabled might be ok.

Regards,
Peng.

> 
> > > +
> > >  /**
> > >   * struct of_clk_provider - Clock provider registration structure
> > >   * @link: Entry in global list of clock providers @@ -5031,6
> > > +5156,8 @@ int of_clk_add_provider(struct device_node *np,
> > >
> > >  	fwnode_dev_initialized(&np->fwnode, true);
> > >
> > > +	of_clk_bump_stdout_clocks();
> > > +
> > >  	return ret;
> > >  }
> > >  EXPORT_SYMBOL_GPL(of_clk_add_provider);
> > > @@ -5073,6 +5200,8 @@ int of_clk_add_hw_provider(struct
> device_node
> > > *np,
> > >
> > >  	fwnode_dev_initialized(&np->fwnode, true);
> > >
> > > +	of_clk_bump_stdout_clocks();
> >
> > If clock driver is built as module,  the will make the clocks will be
> > always enabled, if my understanding is correct.
> 
> until late_initcall_sync(), at which point it'll be disabled before the uart
> driver has probed, yes :-(
> 
> Cheers,
> Andre'

Patch

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 7264cf6165ce..03c5d80e833c 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4923,6 +4923,131 @@  static void clk_core_reparent_orphans(void)
 	clk_prepare_unlock();
 }
 
+/**
+ * struct of_clk_stdout_clks - holds data that is required for handling extra
+ * references to stdout clocks during early boot.
+ *
+ * On some platforms, earlycon depends on the bootloader setup stdout clocks
+ * being retained. In some cases stdout UART clocks (or their parents) can get
+ * disabled during loading of other drivers (e.g. i2c) causing earlycon to stop
+ * to work sometime into the boot, halting the system.
+ *
+ * Having logic to detect these clocks if earlycon is enabled helps with those
+ * cases by bumping their usage count during init. The extra usage count is
+ * later dropped at the end of init.
+ *
+ * @bump_refs: whether or not to add the extra stdout clock references
+ * @lock: mutex protecting access
+ * @have_all: whether or not we have acquired all clocks, to handle cases of
+ *            clocks coming from different drivers / instances
+ * @clks: clocks associated with stdout
+ * @n_clks: number of clocks associated with stdout
+ */
+static struct of_clk_stdout_clks {
+	bool bump_refs;
+
+	struct mutex lock;
+	bool have_all;
+	struct clk **clks;
+	size_t n_clks;
+} of_clk_stdout_clks = {
+	.lock = __MUTEX_INITIALIZER(of_clk_stdout_clks.lock),
+};
+
+static int __init of_clk_bump_stdout_clocks_param(char *str)
+{
+	of_clk_stdout_clks.bump_refs = true;
+	return 0;
+}
+__setup("earlycon", of_clk_bump_stdout_clocks_param);
+__setup_param("earlyprintk", of_clk_keep_stdout_clocks_earlyprintk,
+	      of_clk_bump_stdout_clocks_param, 0);
+
+static void of_clk_bump_stdout_clocks(void)
+{
+	size_t n_clks;
+
+	/*
+	 * We only need to run this code if required to do so and only ever
+	 * before late initcalls have run. Otherwise it'd be impossible to know
+	 * when to drop the extra clock references again.
+	 *
+	 * This generally means that this only works if on affected platforms
+	 * the clock drivers have been built-in (as opposed to being modules).
+	 */
+	if (!of_clk_stdout_clks.bump_refs)
+		return;
+
+	n_clks = of_clk_get_parent_count(of_stdout);
+	if (!n_clks || !of_stdout)
+		return;
+
+	mutex_lock(&of_clk_stdout_clks.lock);
+
+	/*
+	 * We only need to keep trying if we have not succeeded previously,
+	 * i.e. if not all required clocks were ready during previous attempts.
+	 */
+	if (of_clk_stdout_clks.have_all)
+		goto out_unlock;
+
+	if (!of_clk_stdout_clks.clks) {
+		of_clk_stdout_clks.n_clks = n_clks;
+
+		of_clk_stdout_clks.clks = kcalloc(of_clk_stdout_clks.n_clks,
+					      sizeof(*of_clk_stdout_clks.clks),
+					      GFP_KERNEL);
+		if (!of_clk_stdout_clks.clks)
+			goto out_unlock;
+	}
+
+	/* assume that this time we'll be able to grab all required clocks */
+	of_clk_stdout_clks.have_all = true;
+	for (size_t i = 0; i < n_clks; ++i) {
+		struct clk *clk;
+
+		/* we might have grabbed this clock in a previous attempt */
+		if (of_clk_stdout_clks.clks[i])
+			continue;
+
+		clk = of_clk_get(of_stdout, i);
+		if (IS_ERR(clk)) {
+			/* retry next time if clock has not probed yet */
+			of_clk_stdout_clks.have_all = false;
+			continue;
+		}
+
+		if (clk_prepare_enable(clk)) {
+			clk_put(clk);
+			continue;
+		}
+		of_clk_stdout_clks.clks[i] = clk;
+	}
+
+out_unlock:
+	mutex_unlock(&of_clk_stdout_clks.lock);
+}
+
+static int __init of_clk_drop_stdout_clocks(void)
+{
+	for (size_t i = 0; i < of_clk_stdout_clks.n_clks; ++i) {
+		clk_disable_unprepare(of_clk_stdout_clks.clks[i]);
+		clk_put(of_clk_stdout_clks.clks[i]);
+	}
+
+	kfree(of_clk_stdout_clks.clks);
+
+	/*
+	 * Do not try to acquire stdout clocks after late initcalls, e.g.
+	 * during further module loading, as we then wouldn't have a way to
+	 * drop the references (and associated allocations) ever again.
+	 */
+	of_clk_stdout_clks.bump_refs = false;
+
+	return 0;
+}
+late_initcall_sync(of_clk_drop_stdout_clocks);
+
 /**
  * struct of_clk_provider - Clock provider registration structure
  * @link: Entry in global list of clock providers
@@ -5031,6 +5156,8 @@  int of_clk_add_provider(struct device_node *np,
 
 	fwnode_dev_initialized(&np->fwnode, true);
 
+	of_clk_bump_stdout_clocks();
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(of_clk_add_provider);
@@ -5073,6 +5200,8 @@  int of_clk_add_hw_provider(struct device_node *np,
 
 	fwnode_dev_initialized(&np->fwnode, true);
 
+	of_clk_bump_stdout_clocks();
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(of_clk_add_hw_provider);