Message-ID:
 <PAXPR04MB8459F99475C289A827987AF588BA2@PAXPR04MB8459.eurprd04.prod.outlook.com>
Date: Fri, 9 Aug 2024 07:16:10 +0000
From: Peng Fan <peng.fan@....com>
To: André Draszik <andre.draszik@...aro.org>, Michael
 Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, Peter
 Griffin <peter.griffin@...aro.org>, Krzysztof Kozlowski <krzk@...nel.org>,
	Sylwester Nawrocki <s.nawrocki@...sung.com>, Chanwoo Choi
	<cw00.choi@...sung.com>, Alim Akhtar <alim.akhtar@...sung.com>, Sam Protsenko
	<semen.protsenko@...aro.org>, Tudor Ambarus <tudor.ambarus@...aro.org>, Abel
 Vesa <abelvesa@...nel.org>, Shawn Guo <shawnguo@...nel.org>, Sascha Hauer
	<s.hauer@...gutronix.de>, Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>
CC: Will McVicker <willmcvicker@...gle.com>, "kernel-team@...roid.com"
	<kernel-team@...roid.com>, "linux-clk@...r.kernel.org"
	<linux-clk@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-samsung-soc@...r.kernel.org"
	<linux-samsung-soc@...r.kernel.org>, "imx@...ts.linux.dev"
	<imx@...ts.linux.dev>
Subject: RE: [PATCH v6 01/20] clk: bump stdout clock usage for earlycon

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

Powered by blists - more mailing lists