Re: [PATCH] clk: at91: avoid sleeping early - Uwe Kleine-König

From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Stephen Boyd <sboyd@kernel.org>,
	linux-kernel@vger.kernel.org,
	Michael Turquette <mturquette@baylibre.com>,
	Claudiu Beznea <claudiu.beznea@microchip.com>,
	linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] clk: at91: avoid sleeping early
Date: Tue, 24 Sep 2019 14:21:47 +0200	[thread overview]
Message-ID: <20190924122147.fojcu5u44letrele@pengutronix.de> (raw)
In-Reply-To: <20190920153906.20887-1-alexandre.belloni@bootlin.com>

On Fri, Sep 20, 2019 at 05:39:06PM +0200, Alexandre Belloni wrote:
> It is not allowed to sleep to early in the boot process and this may lead
> to kernel issues if the bootloader didn't prepare the slow clock and main
> clock.
> 
> This results in the following error and dump stack on the AriettaG25:
>    bad: scheduling from the idle thread!
> 
> Ensure it is possible to sleep, else simply have a delay.
> 
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

Tested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

See below for a comment.

> Note that this was already discussed a while ago and Arnd said this approach was
> reasonable:
>   https://lore.kernel.org/lkml/6120818.MyeJZ74hYa@wuerfel/
> 
>  drivers/clk/at91/clk-main.c |  5 ++++-
>  drivers/clk/at91/sckc.c     | 20 ++++++++++++++++----
>  2 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clk/at91/clk-main.c b/drivers/clk/at91/clk-main.c
> index f607ee702c83..ccd48e7a3d74 100644
> --- a/drivers/clk/at91/clk-main.c
> +++ b/drivers/clk/at91/clk-main.c
> @@ -293,7 +293,10 @@ static int clk_main_probe_frequency(struct regmap *regmap)
>  		regmap_read(regmap, AT91_CKGR_MCFR, &mcfr);
>  		if (mcfr & AT91_PMC_MAINRDY)
>  			return 0;
> -		usleep_range(MAINF_LOOP_MIN_WAIT, MAINF_LOOP_MAX_WAIT);
> +		if (system_state < SYSTEM_RUNNING)
> +			udelay(MAINF_LOOP_MIN_WAIT);
> +		else
> +			usleep_range(MAINF_LOOP_MIN_WAIT, MAINF_LOOP_MAX_WAIT);

Given that this construct is introduced several times, I wonder if we
want something like:

	static inline void early_usleep_range(unsigned long min, unsigned long max)
	{
		if (system_state < SYSTEM_RUNNING)
			udelay(min);
		else
			usleep_range(min, max);
	}

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel