linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] clk: expand clk_ignore_unused mechanism to keep only a few clks on
@ 2022-10-26 15:18 Uwe Kleine-König
  2023-03-13 18:27 ` Uwe Kleine-König
  2023-03-29 19:49 ` Stephen Boyd
  0 siblings, 2 replies; 8+ messages in thread
From: Uwe Kleine-König @ 2022-10-26 15:18 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: linux-doc, linux-clk, linux-kernel, kernel, Jonathan Corbet

Allow to pass an integer n that results in only keeping n unused clocks
enabled.

This helps to debug the problem if you only know that clk_ignore_unused
helps but you have no clue yet which clock is the culprit.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

compared to v2 sent in August 2021 this is a trivial rebase on top of
v6.1-rc1. I pinged that one repeatedly, I'm now trying with resending
and calling the rebased patch v3 to maybe get some feedback. :-\

Best regards
Uwe

 Documentation/driver-api/clk.rst |  4 +++-
 drivers/clk/clk.c                | 33 ++++++++++++++++++++++++--------
 2 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/Documentation/driver-api/clk.rst b/Documentation/driver-api/clk.rst
index 3cad45d14187..65ae7c3e2b33 100644
--- a/Documentation/driver-api/clk.rst
+++ b/Documentation/driver-api/clk.rst
@@ -259,7 +259,9 @@ the disabling means that the driver will remain functional while the issues
 are sorted out.
 
 To bypass this disabling, include "clk_ignore_unused" in the bootargs to the
-kernel.
+kernel. If you pass "clk_ignore_unused=n" (where n is an integer) the first n
+found clocks are not disabled which can be useful for bisecting over the unused
+clks if you don't know yet which of them is reponsible for your problem.
 
 Locking
 =======
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index c3c3f8c07258..356119a7e5fe 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1322,6 +1322,8 @@ static void __init clk_unprepare_unused_subtree(struct clk_core *core)
 	clk_pm_runtime_put(core);
 }
 
+static unsigned clk_unused_keep_on __initdata;
+
 static void __init clk_disable_unused_subtree(struct clk_core *core)
 {
 	struct clk_core *child;
@@ -1352,12 +1354,17 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
 	 * back to .disable
 	 */
 	if (clk_core_is_enabled(core)) {
-		trace_clk_disable(core);
-		if (core->ops->disable_unused)
-			core->ops->disable_unused(core->hw);
-		else if (core->ops->disable)
-			core->ops->disable(core->hw);
-		trace_clk_disable_complete(core);
+		if (clk_unused_keep_on) {
+			pr_warn("Keep unused clk \"%s\" on\n", core->name);
+			clk_unused_keep_on -= 1;
+		} else {
+			trace_clk_disable(core);
+			if (core->ops->disable_unused)
+				core->ops->disable_unused(core->hw);
+			else if (core->ops->disable)
+				core->ops->disable(core->hw);
+			trace_clk_disable_complete(core);
+		}
 	}
 
 unlock_out:
@@ -1369,9 +1376,17 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
 }
 
 static bool clk_ignore_unused __initdata;
-static int __init clk_ignore_unused_setup(char *__unused)
+static int __init clk_ignore_unused_setup(char *keep)
 {
-	clk_ignore_unused = true;
+	if (*keep == '=') {
+		int ret;
+
+		ret = kstrtouint(keep + 1, 0, &clk_unused_keep_on);
+		if (ret < 0)
+			pr_err("Warning: failed to parse clk_ignore_unused parameter, ignoring");
+	} else {
+		clk_ignore_unused = true;
+	}
 	return 1;
 }
 __setup("clk_ignore_unused", clk_ignore_unused_setup);
@@ -1383,6 +1398,8 @@ static int __init clk_disable_unused(void)
 	if (clk_ignore_unused) {
 		pr_warn("clk: Not disabling unused clocks\n");
 		return 0;
+	} else if (clk_unused_keep_on) {
+		pr_warn("clk: Not disabling %u unused clocks\n", clk_unused_keep_on);
 	}
 
 	clk_prepare_lock();
-- 
2.37.2


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

* Re: [PATCH v3] clk: expand clk_ignore_unused mechanism to keep only a few clks on
  2022-10-26 15:18 [PATCH v3] clk: expand clk_ignore_unused mechanism to keep only a few clks on Uwe Kleine-König
@ 2023-03-13 18:27 ` Uwe Kleine-König
  2023-03-29 19:49 ` Stephen Boyd
  1 sibling, 0 replies; 8+ messages in thread
From: Uwe Kleine-König @ 2023-03-13 18:27 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: linux-kernel, Jonathan Corbet, linux-clk, kernel, linux-doc

[-- Attachment #1: Type: text/plain, Size: 897 bytes --]

Hello,

On Wed, Oct 26, 2022 at 05:18:12PM +0200, Uwe Kleine-König wrote:
> Allow to pass an integer n that results in only keeping n unused clocks
> enabled.
> 
> This helps to debug the problem if you only know that clk_ignore_unused
> helps but you have no clue yet which clock is the culprit.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> compared to v2 sent in August 2021 this is a trivial rebase on top of
> v6.1-rc1. I pinged that one repeatedly, I'm now trying with resending
> and calling the rebased patch v3 to maybe get some feedback. :-\

I didn't get any feedback on this patch and still consider it useful.

Any insights from your side?

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH v3] clk: expand clk_ignore_unused mechanism to keep only a few clks on
  2022-10-26 15:18 [PATCH v3] clk: expand clk_ignore_unused mechanism to keep only a few clks on Uwe Kleine-König
  2023-03-13 18:27 ` Uwe Kleine-König
@ 2023-03-29 19:49 ` Stephen Boyd
  2023-03-29 20:46   ` [PATCH v4] " Uwe Kleine-König
  1 sibling, 1 reply; 8+ messages in thread
From: Stephen Boyd @ 2023-03-29 19:49 UTC (permalink / raw)
  To: Michael Turquette, Uwe Kleine-König
  Cc: linux-doc, linux-clk, linux-kernel, kernel, Jonathan Corbet

Quoting Uwe Kleine-König (2022-10-26 08:18:12)
> Allow to pass an integer n that results in only keeping n unused clocks
> enabled.
> 
> This helps to debug the problem if you only know that clk_ignore_unused
> helps but you have no clue yet which clock is the culprit.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> compared to v2 sent in August 2021 this is a trivial rebase on top of
> v6.1-rc1. I pinged that one repeatedly, I'm now trying with resending
> and calling the rebased patch v3 to maybe get some feedback. :-\
> 
> Best regards
> Uwe
> 
>  Documentation/driver-api/clk.rst |  4 +++-

No update to Documentation/admin-guide/kernel-parameters.txt?

>  drivers/clk/clk.c                | 33 ++++++++++++++++++++++++--------
>  2 files changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index c3c3f8c07258..356119a7e5fe 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1322,6 +1322,8 @@ static void __init clk_unprepare_unused_subtree(struct clk_core *core)
>         clk_pm_runtime_put(core);
>  }
>  
> +static unsigned clk_unused_keep_on __initdata;
> +
>  static void __init clk_disable_unused_subtree(struct clk_core *core)
>  {
>         struct clk_core *child;
> @@ -1352,12 +1354,17 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
>          * back to .disable
>          */
>         if (clk_core_is_enabled(core)) {
> -               trace_clk_disable(core);
> -               if (core->ops->disable_unused)
> -                       core->ops->disable_unused(core->hw);
> -               else if (core->ops->disable)
> -                       core->ops->disable(core->hw);
> -               trace_clk_disable_complete(core);
> +               if (clk_unused_keep_on) {
> +                       pr_warn("Keep unused clk \"%s\" on\n", core->name);
> +                       clk_unused_keep_on -= 1;
> +               } else {
> +                       trace_clk_disable(core);

We have trace_clk_disable() here. Can you have this tracepoint print to
the kernel log and watch over serial console? That would be faster than
bisecting.

> +                       if (core->ops->disable_unused)
> +                               core->ops->disable_unused(core->hw);
> +                       else if (core->ops->disable)
> +                               core->ops->disable(core->hw);
> +                       trace_clk_disable_complete(core);
> +               }
>         }
>  
>  unlock_out:
> @@ -1369,9 +1376,17 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
>  }
>  
>  static bool clk_ignore_unused __initdata;
> -static int __init clk_ignore_unused_setup(char *__unused)
> +static int __init clk_ignore_unused_setup(char *keep)
>  {
> -       clk_ignore_unused = true;
> +       if (*keep == '=') {
> +               int ret;
> +
> +               ret = kstrtouint(keep + 1, 0, &clk_unused_keep_on);
> +               if (ret < 0)

Could omit 'ret' and just have if (kstrtouint(..))

> +                       pr_err("Warning: failed to parse clk_ignore_unused parameter, ignoring");

Missing newline on printk.

> +       } else {
> +               clk_ignore_unused = true;
> +       }
>         return 1;
>  }
>  __setup("clk_ignore_unused", clk_ignore_unused_setup);

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

* [PATCH v4] clk: expand clk_ignore_unused mechanism to keep only a few clks on
  2023-03-29 19:49 ` Stephen Boyd
@ 2023-03-29 20:46   ` Uwe Kleine-König
  2023-03-29 21:27     ` Stephen Boyd
  0 siblings, 1 reply; 8+ messages in thread
From: Uwe Kleine-König @ 2023-03-29 20:46 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Michael Turquette, linux-kernel, Jonathan Corbet, linux-clk,
	kernel, linux-doc

[-- Attachment #1: Type: text/plain, Size: 7489 bytes --]

Allow to pass an integer n that results in only keeping n unused clocks
enabled.

This helps to debug the problem if you only know that clk_ignore_unused
helps but you have no clue yet which clock is the culprit.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello Stephen,

On Wed, Mar 29, 2023 at 12:49:19PM -0700, Stephen Boyd wrote:
> Quoting Uwe Kleine-König (2022-10-26 08:18:12)
> >  Documentation/driver-api/clk.rst |  4 +++-
> 
> No update to Documentation/admin-guide/kernel-parameters.txt?

Fair request. I mentioned it there that you can assign an integer but
refer to Documentation/driver-api/clk.rst for the details. 

> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index c3c3f8c07258..356119a7e5fe 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > [...]
> > @@ -1352,12 +1354,17 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
> >          * back to .disable
> >          */
> >         if (clk_core_is_enabled(core)) {
> > -               trace_clk_disable(core);
> > -               if (core->ops->disable_unused)
> > -                       core->ops->disable_unused(core->hw);
> > -               else if (core->ops->disable)
> > -                       core->ops->disable(core->hw);
> > -               trace_clk_disable_complete(core);
> > +               if (clk_unused_keep_on) {
> > +                       pr_warn("Keep unused clk \"%s\" on\n", core->name);
> > +                       clk_unused_keep_on -= 1;
> > +               } else {
> > +                       trace_clk_disable(core);
> 
> We have trace_clk_disable() here. Can you have this tracepoint print to
> the kernel log and watch over serial console? That would be faster than
> bisecting.

Well no, that doesn't work for all the problems where
clk_ignore_unused=7 could be useful. Consider that e.g. you know that
eth0 is broken, but with clk_ignore_unused is works. So one of the (say)
25 nominally unused clks are required for eth0. But it's not possible to
test the network after each of the 25 clk_disable()s. Unless I'm missing
something and you can hook a userspace action on a trace line?!

> > +                       if (core->ops->disable_unused)
> > +                               core->ops->disable_unused(core->hw);
> > +                       else if (core->ops->disable)
> > +                               core->ops->disable(core->hw);
> > +                       trace_clk_disable_complete(core);
> > +               }
> >         }
> >  
> >  unlock_out:
> > @@ -1369,9 +1376,17 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
> >  }
> >  
> >  static bool clk_ignore_unused __initdata;
> > -static int __init clk_ignore_unused_setup(char *__unused)
> > +static int __init clk_ignore_unused_setup(char *keep)
> >  {
> > -       clk_ignore_unused = true;
> > +       if (*keep == '=') {
> > +               int ret;
> > +
> > +               ret = kstrtouint(keep + 1, 0, &clk_unused_keep_on);
> > +               if (ret < 0)
> 
> Could omit 'ret' and just have if (kstrtouint(..))

I don't feel strong, but I think having that on two lines is easier to
read. So I kept it as is, but if you insist, I can change.

> > +                       pr_err("Warning: failed to parse clk_ignore_unused parameter, ignoring");
> 
> Missing newline on printk.

fixed.

Best regards
Uwe

 .../admin-guide/kernel-parameters.txt         |  6 ++--
 Documentation/driver-api/clk.rst              |  4 ++-
 drivers/clk/clk.c                             | 33 ++++++++++++++-----
 3 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 6221a1d057dd..1a378fe94e48 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -600,8 +600,10 @@
 			force such clocks to be always-on nor does it reserve
 			those clocks in any way. This parameter is useful for
 			debug and development, but should not be needed on a
-			platform with proper driver support.  For more
-			information, see Documentation/driver-api/clk.rst.
+			platform with proper driver support.
+			It can take a value (e.g. clk_ignore_unused=7), to only
+			disable some clks. For more information, see
+			Documentation/driver-api/clk.rst.
 
 	clock=		[BUGS=X86-32, HW] gettimeofday clocksource override.
 			[Deprecated]
diff --git a/Documentation/driver-api/clk.rst b/Documentation/driver-api/clk.rst
index 3cad45d14187..65ae7c3e2b33 100644
--- a/Documentation/driver-api/clk.rst
+++ b/Documentation/driver-api/clk.rst
@@ -259,7 +259,9 @@ the disabling means that the driver will remain functional while the issues
 are sorted out.
 
 To bypass this disabling, include "clk_ignore_unused" in the bootargs to the
-kernel.
+kernel. If you pass "clk_ignore_unused=n" (where n is an integer) the first n
+found clocks are not disabled which can be useful for bisecting over the unused
+clks if you don't know yet which of them is reponsible for your problem.
 
 Locking
 =======
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ae07685c7588..87f605a4f791 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1343,6 +1343,8 @@ static void __init clk_unprepare_unused_subtree(struct clk_core *core)
 	clk_pm_runtime_put(core);
 }
 
+static unsigned clk_unused_keep_on __initdata;
+
 static void __init clk_disable_unused_subtree(struct clk_core *core)
 {
 	struct clk_core *child;
@@ -1373,12 +1375,17 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
 	 * back to .disable
 	 */
 	if (clk_core_is_enabled(core)) {
-		trace_clk_disable(core);
-		if (core->ops->disable_unused)
-			core->ops->disable_unused(core->hw);
-		else if (core->ops->disable)
-			core->ops->disable(core->hw);
-		trace_clk_disable_complete(core);
+		if (clk_unused_keep_on) {
+			pr_warn("Keep unused clk \"%s\" on\n", core->name);
+			clk_unused_keep_on -= 1;
+		} else {
+			trace_clk_disable(core);
+			if (core->ops->disable_unused)
+				core->ops->disable_unused(core->hw);
+			else if (core->ops->disable)
+				core->ops->disable(core->hw);
+			trace_clk_disable_complete(core);
+		}
 	}
 
 unlock_out:
@@ -1390,9 +1397,17 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
 }
 
 static bool clk_ignore_unused __initdata;
-static int __init clk_ignore_unused_setup(char *__unused)
+static int __init clk_ignore_unused_setup(char *keep)
 {
-	clk_ignore_unused = true;
+	if (*keep == '=') {
+		int ret;
+
+		ret = kstrtouint(keep + 1, 0, &clk_unused_keep_on);
+		if (ret < 0)
+			pr_err("Warning: failed to parse clk_ignore_unused parameter, ignoring\n");
+	} else {
+		clk_ignore_unused = true;
+	}
 	return 1;
 }
 __setup("clk_ignore_unused", clk_ignore_unused_setup);
@@ -1404,6 +1419,8 @@ static int __init clk_disable_unused(void)
 	if (clk_ignore_unused) {
 		pr_warn("clk: Not disabling unused clocks\n");
 		return 0;
+	} else if (clk_unused_keep_on) {
+		pr_warn("clk: Not disabling %u unused clocks\n", clk_unused_keep_on);
 	}
 
 	clk_prepare_lock();
-- 
2.39.2

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH v4] clk: expand clk_ignore_unused mechanism to keep only a few clks on
  2023-03-29 20:46   ` [PATCH v4] " Uwe Kleine-König
@ 2023-03-29 21:27     ` Stephen Boyd
  2023-03-30  6:06       ` Uwe Kleine-König
  0 siblings, 1 reply; 8+ messages in thread
From: Stephen Boyd @ 2023-03-29 21:27 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Michael Turquette, linux-kernel, Jonathan Corbet, linux-clk,
	kernel, linux-doc

Quoting Uwe Kleine-König (2023-03-29 13:46:32)
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index c3c3f8c07258..356119a7e5fe 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > [...]
> > > @@ -1352,12 +1354,17 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
> > >          * back to .disable
> > >          */
> > >         if (clk_core_is_enabled(core)) {
> > > -               trace_clk_disable(core);
> > > -               if (core->ops->disable_unused)
> > > -                       core->ops->disable_unused(core->hw);
> > > -               else if (core->ops->disable)
> > > -                       core->ops->disable(core->hw);
> > > -               trace_clk_disable_complete(core);
> > > +               if (clk_unused_keep_on) {
> > > +                       pr_warn("Keep unused clk \"%s\" on\n", core->name);
> > > +                       clk_unused_keep_on -= 1;
> > > +               } else {
> > > +                       trace_clk_disable(core);
> > 
> > We have trace_clk_disable() here. Can you have this tracepoint print to
> > the kernel log and watch over serial console? That would be faster than
> > bisecting.
> 
> Well no, that doesn't work for all the problems where
> clk_ignore_unused=7 could be useful. Consider that e.g. you know that
> eth0 is broken, but with clk_ignore_unused is works. So one of the (say)
> 25 nominally unused clks are required for eth0. But it's not possible to
> test the network after each of the 25 clk_disable()s. Unless I'm missing
> something and you can hook a userspace action on a trace line?!

In that case it sounds like you want to compile the kernel with the
support for enabling clks from debugfs. Can you use that?

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

* Re: [PATCH v4] clk: expand clk_ignore_unused mechanism to keep only a few clks on
  2023-03-29 21:27     ` Stephen Boyd
@ 2023-03-30  6:06       ` Uwe Kleine-König
  2023-03-30 15:26         ` Sebastian Reichel
  0 siblings, 1 reply; 8+ messages in thread
From: Uwe Kleine-König @ 2023-03-30  6:06 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: linux-doc, Michael Turquette, Jonathan Corbet, linux-kernel,
	kernel, linux-clk

[-- Attachment #1: Type: text/plain, Size: 2276 bytes --]

On Wed, Mar 29, 2023 at 02:27:08PM -0700, Stephen Boyd wrote:
> Quoting Uwe Kleine-König (2023-03-29 13:46:32)
> > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > index c3c3f8c07258..356119a7e5fe 100644
> > > > --- a/drivers/clk/clk.c
> > > > +++ b/drivers/clk/clk.c
> > > > [...]
> > > > @@ -1352,12 +1354,17 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
> > > >          * back to .disable
> > > >          */
> > > >         if (clk_core_is_enabled(core)) {
> > > > -               trace_clk_disable(core);
> > > > -               if (core->ops->disable_unused)
> > > > -                       core->ops->disable_unused(core->hw);
> > > > -               else if (core->ops->disable)
> > > > -                       core->ops->disable(core->hw);
> > > > -               trace_clk_disable_complete(core);
> > > > +               if (clk_unused_keep_on) {
> > > > +                       pr_warn("Keep unused clk \"%s\" on\n", core->name);
> > > > +                       clk_unused_keep_on -= 1;
> > > > +               } else {
> > > > +                       trace_clk_disable(core);
> > > 
> > > We have trace_clk_disable() here. Can you have this tracepoint print to
> > > the kernel log and watch over serial console? That would be faster than
> > > bisecting.
> > 
> > Well no, that doesn't work for all the problems where
> > clk_ignore_unused=7 could be useful. Consider that e.g. you know that
> > eth0 is broken, but with clk_ignore_unused is works. So one of the (say)
> > 25 nominally unused clks are required for eth0. But it's not possible to
> > test the network after each of the 25 clk_disable()s. Unless I'm missing
> > something and you can hook a userspace action on a trace line?!
> 
> In that case it sounds like you want to compile the kernel with the
> support for enabling clks from debugfs. Can you use that?

In some of the cases that might work. Unless for example the problem
makes the kernel fail to boot or the device is broken when the clk was
disabled and reenable doesn't help?!

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

* Re: [PATCH v4] clk: expand clk_ignore_unused mechanism to keep only a few clks on
  2023-03-30  6:06       ` Uwe Kleine-König
@ 2023-03-30 15:26         ` Sebastian Reichel
  2023-03-30 17:33           ` Uwe Kleine-König
  0 siblings, 1 reply; 8+ messages in thread
From: Sebastian Reichel @ 2023-03-30 15:26 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Stephen Boyd, linux-doc, Michael Turquette, Jonathan Corbet,
	linux-kernel, kernel, linux-clk

[-- Attachment #1: Type: text/plain, Size: 2662 bytes --]

Hi,

On Thu, Mar 30, 2023 at 08:06:01AM +0200, Uwe Kleine-König wrote:
> On Wed, Mar 29, 2023 at 02:27:08PM -0700, Stephen Boyd wrote:
> > Quoting Uwe Kleine-König (2023-03-29 13:46:32)
> > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > > > index c3c3f8c07258..356119a7e5fe 100644
> > > > > --- a/drivers/clk/clk.c
> > > > > +++ b/drivers/clk/clk.c
> > > > > [...]
> > > > > @@ -1352,12 +1354,17 @@ static void __init clk_disable_unused_subtree(struct clk_core *core)
> > > > >          * back to .disable
> > > > >          */
> > > > >         if (clk_core_is_enabled(core)) {
> > > > > -               trace_clk_disable(core);
> > > > > -               if (core->ops->disable_unused)
> > > > > -                       core->ops->disable_unused(core->hw);
> > > > > -               else if (core->ops->disable)
> > > > > -                       core->ops->disable(core->hw);
> > > > > -               trace_clk_disable_complete(core);
> > > > > +               if (clk_unused_keep_on) {
> > > > > +                       pr_warn("Keep unused clk \"%s\" on\n", core->name);
> > > > > +                       clk_unused_keep_on -= 1;
> > > > > +               } else {
> > > > > +                       trace_clk_disable(core);
> > > > 
> > > > We have trace_clk_disable() here. Can you have this tracepoint print to
> > > > the kernel log and watch over serial console? That would be faster than
> > > > bisecting.
> > > 
> > > Well no, that doesn't work for all the problems where
> > > clk_ignore_unused=7 could be useful. Consider that e.g. you know that
> > > eth0 is broken, but with clk_ignore_unused is works. So one of the (say)
> > > 25 nominally unused clks are required for eth0. But it's not possible to
> > > test the network after each of the 25 clk_disable()s. Unless I'm missing
> > > something and you can hook a userspace action on a trace line?!
> > 
> > In that case it sounds like you want to compile the kernel with the
> > support for enabling clks from debugfs. Can you use that?
> 
> In some of the cases that might work. Unless for example the problem
> makes the kernel fail to boot or the device is broken when the clk was
> disabled and reenable doesn't help?!

I recently debugged a similar issue like this:

1. build kernel with CLOCK_ALLOW_WRITE_DEBUGFS
2. boot with clk_ignore_unused, so clocks stay enabled
3. disable clocks via sysfs:
   echo 1 > /sys/kernel/debug/clk/${CLK}/clk_prepare_enable
   echo 0 > /sys/kernel/debug/clk/${CLK}/clk_prepare_enable
4. check if peripheral is still ok
5. repeat step 3 with the next 'interesting' clock

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

* Re: [PATCH v4] clk: expand clk_ignore_unused mechanism to keep only a few clks on
  2023-03-30 15:26         ` Sebastian Reichel
@ 2023-03-30 17:33           ` Uwe Kleine-König
  0 siblings, 0 replies; 8+ messages in thread
From: Uwe Kleine-König @ 2023-03-30 17:33 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Jonathan Corbet, Stephen Boyd, Michael Turquette, linux-doc,
	linux-kernel, kernel, linux-clk

[-- Attachment #1: Type: text/plain, Size: 1169 bytes --]

Hello Sebastian,

On Thu, Mar 30, 2023 at 05:26:17PM +0200, Sebastian Reichel wrote:
> > > In that case it sounds like you want to compile the kernel with the
> > > support for enabling clks from debugfs. Can you use that?
> > 
> > In some of the cases that might work. Unless for example the problem
> > makes the kernel fail to boot or the device is broken when the clk was
> > disabled and reenable doesn't help?!
> 
> I recently debugged a similar issue like this:
> 
> 1. build kernel with CLOCK_ALLOW_WRITE_DEBUGFS
> 2. boot with clk_ignore_unused, so clocks stay enabled
> 3. disable clocks via sysfs:
>    echo 1 > /sys/kernel/debug/clk/${CLK}/clk_prepare_enable
>    echo 0 > /sys/kernel/debug/clk/${CLK}/clk_prepare_enable
> 4. check if peripheral is still ok
> 5. repeat step 3 with the next 'interesting' clock

Ah, that makes sense. Thanks.

With that I cannot imagine a scenario that I can only debug with
clk_ignore_unused=n. So let's drop my patch.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

end of thread, other threads:[~2023-03-30 17:33 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-26 15:18 [PATCH v3] clk: expand clk_ignore_unused mechanism to keep only a few clks on Uwe Kleine-König
2023-03-13 18:27 ` Uwe Kleine-König
2023-03-29 19:49 ` Stephen Boyd
2023-03-29 20:46   ` [PATCH v4] " Uwe Kleine-König
2023-03-29 21:27     ` Stephen Boyd
2023-03-30  6:06       ` Uwe Kleine-König
2023-03-30 15:26         ` Sebastian Reichel
2023-03-30 17:33           ` Uwe Kleine-König

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).