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