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

[Stephen Boyd <sboyd@kernel.org>]

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 <andre.draszik@linaro.org>
> 
> ---

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