Linux-Clk Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] clk: at91: avoid sleeping early
@ 2019-09-20 15:39 Alexandre Belloni
       [not found] ` <20190923165848.3108A20882@mail.kernel.org>
  2019-09-24 12:21 ` Uwe Kleine-König
  0 siblings, 2 replies; 5+ messages in thread
From: Alexandre Belloni @ 2019-09-20 15:39 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Nicolas Ferre, Claudiu Beznea, Michael Turquette, linux-clk,
	linux-arm-kernel, linux-kernel, Alexandre Belloni,
	Uwe Kleine-König

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

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);
 	} while (time_before(prep_time, timeout));
 
 	return -ETIMEDOUT;
diff --git a/drivers/clk/at91/sckc.c b/drivers/clk/at91/sckc.c
index 9bfe9a28294a..fac0ca56d42d 100644
--- a/drivers/clk/at91/sckc.c
+++ b/drivers/clk/at91/sckc.c
@@ -76,7 +76,10 @@ static int clk_slow_osc_prepare(struct clk_hw *hw)
 
 	writel(tmp | osc->bits->cr_osc32en, sckcr);
 
-	usleep_range(osc->startup_usec, osc->startup_usec + 1);
+	if (system_state < SYSTEM_RUNNING)
+		udelay(osc->startup_usec);
+	else
+		usleep_range(osc->startup_usec, osc->startup_usec + 1);
 
 	return 0;
 }
@@ -187,7 +190,10 @@ static int clk_slow_rc_osc_prepare(struct clk_hw *hw)
 
 	writel(readl(sckcr) | osc->bits->cr_rcen, sckcr);
 
-	usleep_range(osc->startup_usec, osc->startup_usec + 1);
+	if (system_state < SYSTEM_RUNNING)
+		udelay(osc->startup_usec);
+	else
+		usleep_range(osc->startup_usec, osc->startup_usec + 1);
 
 	return 0;
 }
@@ -288,7 +294,10 @@ static int clk_sam9x5_slow_set_parent(struct clk_hw *hw, u8 index)
 
 	writel(tmp, sckcr);
 
-	usleep_range(SLOWCK_SW_TIME_USEC, SLOWCK_SW_TIME_USEC + 1);
+	if (system_state < SYSTEM_RUNNING)
+		udelay(SLOWCK_SW_TIME_USEC);
+	else
+		usleep_range(SLOWCK_SW_TIME_USEC, SLOWCK_SW_TIME_USEC + 1);
 
 	return 0;
 }
@@ -533,7 +542,10 @@ static int clk_sama5d4_slow_osc_prepare(struct clk_hw *hw)
 		return 0;
 	}
 
-	usleep_range(osc->startup_usec, osc->startup_usec + 1);
+	if (system_state < SYSTEM_RUNNING)
+		udelay(osc->startup_usec);
+	else
+		usleep_range(osc->startup_usec, osc->startup_usec + 1);
 	osc->prepared = true;
 
 	return 0;
-- 
2.21.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] clk: at91: avoid sleeping early
       [not found] ` <20190923165848.3108A20882@mail.kernel.org>
@ 2019-09-23 21:34   ` Alexandre Belloni
  0 siblings, 0 replies; 5+ messages in thread
From: Alexandre Belloni @ 2019-09-23 21:34 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Nicolas Ferre, Claudiu Beznea, Michael Turquette, linux-clk,
	linux-arm-kernel, linux-kernel, Uwe Kleine-König

On 23/09/2019 09:58:47-0700, Stephen Boyd wrote:
> Quoting Alexandre Belloni (2019-09-20 08:39:06)
> > 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>
> > ---
> > 
> > Note that this was already discussed a while ago and Arnd said this approach was
> > reasonable:
> >   https://lore.kernel.org/lkml/6120818.MyeJZ74hYa@wuerfel/
> 
> Does this need a Fixes: tag?
> 

I'm not sure how far this can get backported

Fixes: 80eded6ce8bb ("clk: at91: add slow clks driver")


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] clk: at91: avoid sleeping early
  2019-09-20 15:39 [PATCH] clk: at91: avoid sleeping early Alexandre Belloni
       [not found] ` <20190923165848.3108A20882@mail.kernel.org>
@ 2019-09-24 12:21 ` Uwe Kleine-König
  2019-09-24 20:20   ` Stephen Boyd
  1 sibling, 1 reply; 5+ messages in thread
From: Uwe Kleine-König @ 2019-09-24 12:21 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Stephen Boyd, Nicolas Ferre, Claudiu Beznea, Michael Turquette,
	linux-clk, linux-arm-kernel, linux-kernel

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/  |

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] clk: at91: avoid sleeping early
  2019-09-24 12:21 ` Uwe Kleine-König
@ 2019-09-24 20:20   ` Stephen Boyd
  2019-10-05 20:05     ` Alexandre Belloni
  0 siblings, 1 reply; 5+ messages in thread
From: Stephen Boyd @ 2019-09-24 20:20 UTC (permalink / raw)
  To: Alexandre Belloni, u.kleine-koenig
  Cc: Nicolas Ferre, Claudiu Beznea, Michael Turquette, linux-clk,
	linux-arm-kernel, linux-kernel

Quoting Uwe  (2019-09-24 05:21:47)
> On Fri, Sep 20, 2019 at 05:39:06PM +0200, Alexandre Belloni wrote:
> > 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);
>         }
> 

Maybe, but I think the intent is to not encourage this behavior? So
providing a wrapper will make it "easy" and then we'll have to tell
users to stop calling it. Another idea would be to make usleep_range()
"do the right thing" and call udelay if the system isn't running. And
another idea from tlgx[1] is to pull the delay logic into another clk op
that we can call to see when the enable or prepare is done. That may be
possible by introducing another clk_ops callback that when present
indicates we should sleep or delay for so much time while waiting for
the prepare or enable to complete.

[1] https://lkml.kernel.org/r/alpine.DEB.2.11.1606061448010.28031@nanos


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] clk: at91: avoid sleeping early
  2019-09-24 20:20   ` Stephen Boyd
@ 2019-10-05 20:05     ` Alexandre Belloni
  0 siblings, 0 replies; 5+ messages in thread
From: Alexandre Belloni @ 2019-10-05 20:05 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: u.kleine-koenig, Nicolas Ferre, Claudiu Beznea,
	Michael Turquette, linux-clk, linux-arm-kernel, linux-kernel

On 24/09/2019 13:20:15-0700, Stephen Boyd wrote:
> Quoting Uwe  (2019-09-24 05:21:47)
> > On Fri, Sep 20, 2019 at 05:39:06PM +0200, Alexandre Belloni wrote:
> > > 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);
> >         }
> > 
> 
> Maybe, but I think the intent is to not encourage this behavior? So
> providing a wrapper will make it "easy" and then we'll have to tell
> users to stop calling it. Another idea would be to make usleep_range()
> "do the right thing" and call udelay if the system isn't running. And
> another idea from tlgx[1] is to pull the delay logic into another clk op
> that we can call to see when the enable or prepare is done. That may be
> possible by introducing another clk_ops callback that when present
> indicates we should sleep or delay for so much time while waiting for
> the prepare or enable to complete.
> 
> [1] https://lkml.kernel.org/r/alpine.DEB.2.11.1606061448010.28031@nanos
> 

Do you want me to implement that now or are you planning to apply the
patch in the meantime ?


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-20 15:39 [PATCH] clk: at91: avoid sleeping early Alexandre Belloni
     [not found] ` <20190923165848.3108A20882@mail.kernel.org>
2019-09-23 21:34   ` Alexandre Belloni
2019-09-24 12:21 ` Uwe Kleine-König
2019-09-24 20:20   ` Stephen Boyd
2019-10-05 20:05     ` Alexandre Belloni

Linux-Clk Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-clk/0 linux-clk/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-clk linux-clk/ https://lore.kernel.org/linux-clk \
		linux-clk@vger.kernel.org linux-clk@archiver.kernel.org
	public-inbox-index linux-clk

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-clk


AGPL code for this site: git clone https://public-inbox.org/ public-inbox