diff mbox series

[1/4] clk: ti: Handle possible address in the node name

Message ID 20240213105730.5287-2-tony@atomide.com (mailing list archive)
State New, archived
Headers show
Series Use reg instead of ti,bit-shift for clksel | expand

Commit Message

Tony Lindgren Feb. 13, 2024, 10:56 a.m. UTC
In order to use #address-cells = <1> and start making use of the
standard reg property, let's prepare things to ignore the possible
address in the clock node name.

Unless the clock-output-names property is used, the legacy clocks still
fall back to matching the clock data based on the node name.

We use cleanup.h to simplify the return path for freeing tmp.

Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/clk/ti/clk.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Comments

Stephen Boyd Feb. 22, 2024, 5:11 a.m. UTC | #1
Quoting Tony Lindgren (2024-02-13 02:56:41)
> In order to use #address-cells = <1> and start making use of the
> standard reg property, let's prepare things to ignore the possible
> address in the clock node name.
> 
> Unless the clock-output-names property is used, the legacy clocks still
> fall back to matching the clock data based on the node name.
> 
> We use cleanup.h to simplify the return path for freeing tmp.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>
Geert Uytterhoeven Sept. 2, 2024, 2:03 p.m. UTC | #2
Hi Tony,

On Tue, Feb 13, 2024 at 11:59 AM Tony Lindgren <tony@atomide.com> wrote:
> In order to use #address-cells = <1> and start making use of the
> standard reg property, let's prepare things to ignore the possible
> address in the clock node name.
>
> Unless the clock-output-names property is used, the legacy clocks still
> fall back to matching the clock data based on the node name.
>
> We use cleanup.h to simplify the return path for freeing tmp.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Thanks for your patch, which is now commit 3516338543cafb65 ("clk: ti:
Handle possible address in the node name") in v6.9-rc1.
This causes an early boot crash on BeagleBone Black:

    ti_dt_clocks_register: failed to lookup clock node
clk-24mhz-clkctrl:0000:0, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
clk-24mhz-clkctrl:0000:0, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
l3-aon-clkctrl:0000:30, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
l3-aon-clkctrl:0000:19, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
l4-wkup-clkctrl:0008:18, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
l4ls-clkctrl:0074:18, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
l4ls-clkctrl:0078:18, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
l4ls-clkctrl:007c:18, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
l3-aon-clkctrl:0000:27, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
l3-aon-clkctrl:0000:22, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
l3-aon-clkctrl:0000:24, ret=-517
    ti_dt_clocks_register: failed to lookup clock node
l3-aon-clkctrl:0000:20, ret=-517
    8<--- cut here ---
    Unable to handle kernel paging request at virtual address fffffffe when read
    [fffffffe] *pgd=9fdf6841, *pte=00000000, *ppte=00000000
    Internal error: Oops: 27 [#1] SMP ARM
    CPU: 0 PID: 0 Comm: swapper/0 Not tainted
6.8.0-rc1-boneblack-00001-g3516338543ca #119
    Hardware name: Generic AM33XX (Flattened Device Tree)
    PC is at clk_set_parent+0x2c/0x6c
    LR is at __lock_acquire+0x3f8/0x29d4
    pc : [<c06ecd4c>]    lr : [<c01b2b14>]    psr: a0000093
    sp : c1001fb0  ip : 00000000  fp : 00000000
    r10: c0f5da88  r9 : 00000000  r8 : 00000078
    r7 : ffffffff  r6 : c11ba000  r5 : fffffffe  r4 : c20a0700
    r3 : 00000000  r2 : c100c580  r1 : 00000001  r0 : c209ac00
    Flags: NzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
    Control: 10c5387d  Table: 80004019  DAC: 00000051
    Register r0 information: slab kmalloc-192 start c209ac00 pointer
offset 0 size 192
    Register r1 information: non-paged memory
    Register r2 information: non-slab/vmalloc memory
    Register r3 information: NULL pointer
    Register r4 information: slab kmalloc-64 start c20a0700 pointer
offset 0 size 64
    Register r5 information: non-paged memory
    Register r6 information: non-slab/vmalloc memory
    Register r7 information: non-paged memory
    Register r8 information: non-paged memory
    Register r9 information: NULL pointer
    Register r10 information: non-slab/vmalloc memory
    Register r11 information: NULL pointer
    Register r12 information: NULL pointer
    Process swapper/0 (pid: 0, stack limit = 0x(ptrval))
    Stack: (0xc1001fb0 to 0xc1002000)
    1fa0:                                     c20a0700 c10093c0
c11ba000 c0f2ebdc
    1fc0: dfdffc40 c0f11380 dfdffc40 c0f01074 ffffffff ffffffff
00000000 c0f006f0
    1fe0: 00000000 c0f5da88 00000000 00000e05 00000000 00000000
00000000 00000000
     clk_set_parent from am33xx_dt_clk_init+0x84/0xa4
     am33xx_dt_clk_init from omap_init_time_of+0x8/0x10
     omap_init_time_of from start_kernel+0x430/0x638
     start_kernel from 0x0
    Code: e3530000 1a00000e e3550000 e5940000 (15955000)
    ---[ end trace 0000000000000000 ]---
    Kernel panic - not syncing: Attempted to kill the idle task!
    ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]---

Reverting the commit on top of a recent v6.11-rcX-based tree fixes
the issue.

BTW, bisecting this took a while as:
  1. The OMAP serial driver locks up when booted with "earlycon
     keep_bootcon",
  2. The TI SYSC sometimes crashes during early boot, too:

    Unhandled fault: external abort on non-linefetch (0x1008) at 0xe036d010
    [e036d010] *pgd=8276e811, *pte=47400653, *ppte=47400453
    Internal error: : 1008 [#1] SMP ARM
    Modules linked in:
    CPU: 0 PID: 33 Comm: kworker/u4:3 Not tainted
6.8.0-boneblack-05567-gaa7d6513d68b #78
    Hardware name: Generic AM33XX (Flattened Device Tree)
    Workqueue: events_unbound deferred_probe_work_func
    PC is at sysc_reset+0x118/0x210
    LR is at sysc_probe+0xe08/0x1440
    pc : [<c06d0ba8>]    lr : [<c06d1cd8>]    psr: 60000013

> --- a/drivers/clk/ti/clk.c
> +++ b/drivers/clk/ti/clk.c
> @@ -7,6 +7,7 @@
>   * Tero Kristo <t-kristo@ti.com>
>   */
>
> +#include <linux/cleanup.h>
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/clkdev.h>
> @@ -114,20 +115,26 @@ int ti_clk_setup_ll_ops(struct ti_clk_ll_ops *ops)
>
>  /*
>   * Eventually we could standardize to using '_' for clk-*.c files to follow the
> - * TRM naming and leave out the tmp name here.
> + * TRM naming.
>   */
>  static struct device_node *ti_find_clock_provider(struct device_node *from,
>                                                   const char *name)
>  {
> +       char *tmp __free(kfree) = NULL;
>         struct device_node *np;
>         bool found = false;
>         const char *n;
> -       char *tmp;
> +       char *p;
>
>         tmp = kstrdup_and_replace(name, '-', '_', GFP_KERNEL);
>         if (!tmp)
>                 return NULL;
>
> +       /* Ignore a possible address for the node name */
> +       p = strchr(tmp, '@');
> +       if (p)
> +               *p = '\0';
> +
>         /* Node named "clock" with "clock-output-names" */
>         for_each_of_allnodes_from(from, np) {
>                 if (of_property_read_string_index(np, "clock-output-names",
> @@ -140,7 +147,6 @@ static struct device_node *ti_find_clock_provider(struct device_node *from,
>                         break;
>                 }
>         }
> -       kfree(tmp);
>
>         if (found) {
>                 of_node_put(from);
> @@ -148,7 +154,7 @@ static struct device_node *ti_find_clock_provider(struct device_node *from,
>         }
>
>         /* Fall back to using old node name base provider name */
> -       return of_find_node_by_name(from, name);
> +       return of_find_node_by_name(from, tmp);
>  }
>
>  /**

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven Sept. 5, 2024, 9:54 a.m. UTC | #3
On Mon, Sep 2, 2024 at 4:03 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, Feb 13, 2024 at 11:59 AM Tony Lindgren <tony@atomide.com> wrote:
> > In order to use #address-cells = <1> and start making use of the
> > standard reg property, let's prepare things to ignore the possible
> > address in the clock node name.
> >
> > Unless the clock-output-names property is used, the legacy clocks still
> > fall back to matching the clock data based on the node name.
> >
> > We use cleanup.h to simplify the return path for freeing tmp.
> >
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
>
> Thanks for your patch, which is now commit 3516338543cafb65 ("clk: ti:
> Handle possible address in the node name") in v6.9-rc1.
> This causes an early boot crash on BeagleBone Black:
>
>     ti_dt_clocks_register: failed to lookup clock node
> clk-24mhz-clkctrl:0000:0, ret=-517

I found the culprit: after the move of .dts files to vendor
sub-directories, I had updated my boot script to:

    DTB=arch/arm/boot/dts/ti/am335x-boneblack.dtb
    if [ ! -e $DTB ]; then
        DTB=arch/arm/boot/dts/am335x-boneblack.dtb
    fi

I.e. I missed the "/omap" part, causing the install to fall back to
an old DTB file that no longer works with modern kernels.

Sorry for the noise.

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/drivers/clk/ti/clk.c b/drivers/clk/ti/clk.c
--- a/drivers/clk/ti/clk.c
+++ b/drivers/clk/ti/clk.c
@@ -7,6 +7,7 @@ 
  * Tero Kristo <t-kristo@ti.com>
  */
 
+#include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/clkdev.h>
@@ -114,20 +115,26 @@  int ti_clk_setup_ll_ops(struct ti_clk_ll_ops *ops)
 
 /*
  * Eventually we could standardize to using '_' for clk-*.c files to follow the
- * TRM naming and leave out the tmp name here.
+ * TRM naming.
  */
 static struct device_node *ti_find_clock_provider(struct device_node *from,
 						  const char *name)
 {
+	char *tmp __free(kfree) = NULL;
 	struct device_node *np;
 	bool found = false;
 	const char *n;
-	char *tmp;
+	char *p;
 
 	tmp = kstrdup_and_replace(name, '-', '_', GFP_KERNEL);
 	if (!tmp)
 		return NULL;
 
+	/* Ignore a possible address for the node name */
+	p = strchr(tmp, '@');
+	if (p)
+		*p = '\0';
+
 	/* Node named "clock" with "clock-output-names" */
 	for_each_of_allnodes_from(from, np) {
 		if (of_property_read_string_index(np, "clock-output-names",
@@ -140,7 +147,6 @@  static struct device_node *ti_find_clock_provider(struct device_node *from,
 			break;
 		}
 	}
-	kfree(tmp);
 
 	if (found) {
 		of_node_put(from);
@@ -148,7 +154,7 @@  static struct device_node *ti_find_clock_provider(struct device_node *from,
 	}
 
 	/* Fall back to using old node name base provider name */
-	return of_find_node_by_name(from, name);
+	return of_find_node_by_name(from, tmp);
 }
 
 /**